Re: [PATCH 1/6] drm/bridge: use bus flags in bridge timings

[Laurent Pinchart]

Hi Stefan,

On Thursday, 6 September 2018 23:25:56 EEST Stefan Agner wrote:
> On 06.09.2018 04:07, Linus Walleij wrote:
> > On Wed, Sep 5, 2018 at 8:32 PM Stefan Agner <stefan@xxxxxxxx> wrote:
> >> On 05.09.2018 00:44, Laurent Pinchart wrote:
> >> 
> >> Good point! I actually really don't like that we use the same flags here
> >> but from a different perspective. Especially since the flags defines
> >> document things differently:
> >> 
> >> /* drive data on pos. edge */
> >> #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> >> /* drive data on neg. edge */
> >> #define DRM_BUS_FLAG_PIXDATA_NEGEDGE    (1<<3)
> > 
> > Maybe a stupid comment from my side, but can't we just change the
> > documentation to match the usecases?
> > 
> > /* Trigger pixel data latch on positive edge */
> > #define DRM_BUS_FLAG_PIXDATA_POSEDGE    (1<<2)
> > 
> >> Using the opposite perspective would also need translation in crtc
> >> drivers... So far no driver uses sampling_edge.
> >> 
> >> I would prefer if we always use the meaning as documented by the flags.
> >> 
> >> I guess we would need to convert DRM_BUS_FLAG_PIXDATA_POSEDGE ->
> >> DRM_BUS_FLAG_PIXDATA_NEGEDGE.
> >> 
> >> Linus Walleij, you added sampling edge, any thoughts?
> > 
> > I just thought it was generally useful to have triggering edge encoded
> > into the bridge as it makes it clear that this edge is something
> > that is a delayed version of the driving edge which is subject to
> > clock skew caused by the speed of electrons in silicon and
> > copper and slew rate caused by parasitic capacitance.
> 
> Ok, I read a bit up on the history of bridge timing, especially:
> https://www.spinics.net/lists/dri-devel/msg155618.html
> 
> IMHO, this got overengineered. For displays we do not need all that
> setup/sample delay timing information, and much longer cables are in
> use. So why is all that needed for bridges?
> 
> For Linus case, the THS8134(A/B) data sheet I found (revised March 2010)
> clearly states:
> Clock input. A rising edge on CLK latches RPr0-7, GY0-7, BPb0-7.
> 
> So we need to drive on negative edge, hence DRM_BUS_FLAG_PIXDATA_NEGEDGE
> should be used, which makes the pl111 driver setting TIM2_IPC:
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0121d/index.h
> tml
> 
> > DRM_BUS_FLAG_PIXDATA_POSEDGE is the right value for my use cases, but it
> > doesn't match how the ADV7123 operates. Using DRM_BUS_FLAG_PIXDATA_NEGEDGE
> > would match the hardware, but would break display for some modes,
> > depending on the display clock frequency as the internal 8.5ns output
> > delay applied to a falling clock edge would fall right into the 1.7ns
> > setup + hold time window of the ADV7123 around the rising edge. I can't
> > test this right now as I don't have local access to boards using the
> > ADV7123, but from a quick calculation that ignores the PCB transmission
> > delay modes with frequencies between 57MHz and 71MHz could break if the
> > data was output on the falling edge of the clock.
> 
> If clocks vs. data signal are really that much off on R-Car DU, then
> parallel displays must have the very same issue...
> 
> Are you sure that only the clock signal has an output delay? And that
> this output delay is a fixed value, clock independent?
> 
> Typically, delays apply to all signals equally, and often are clock
> frequency dependent...
> 
> Without really looking at the signals, I would not jump to conclusions
> here! I am pretty sure that driving on negative edge works just as well.

I've tested Linus' original patch and it broke display on R-Car, so, no, it 
doesn't work :-)

The R-Car display engine delays the clock internally (in some cases that delay 
is even configurable, and that's not uncommon in display controllers). We 
really need all this information, and I believe we need it for panels too, not 
just for bridges. The fact that we managed to get away without adding it to 
panels is likely due to the large number of panels out there, which makes it 
less likely that the same panel gets used by different systems in mainline 
with different clock delays. I expect that some panel drivers report the wrong 
clock edge to make things work on the board they were tested with, and I 
expect we'll eventually need to add the same information for panels too.

So please don't remove this useful API, otherwise you'll break my board, and I 
won't be happy.

-- 
Regards,

Laurent Pinchart