Hello,
On Monday, 24 September 2018 15:34:54 EET Stefan Agner wrote:
> On 24.09.2018 14:13, Thierry Reding wrote:
> > On Mon, Sep 24, 2018 at 01:59:25PM +0200, Stefan Agner wrote:
> >> On 24.09.2018 13:48, Thierry Reding wrote:
> >>> On Sat, Sep 22, 2018 at 03:15:04PM +0300, Laurent Pinchart wrote:
> >>>> The DRM_BUS_FLAG_PIXDATA_(POS|NEG)EDGE flags are deprecated in favour
> >>>> of the new DRM_BUS_FLAG_PIXDATA_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags.
> >>>> Replace them through the code.
> >>>>
> >>>> Signed-off-by: Laurent Pinchart
> >>>> <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> >>>> ---
> >>>>
> >>>> drivers/gpu/drm/bridge/dumb-vga-dac.c | 6 +++---
> >>>> drivers/gpu/drm/drm_modes.c | 8 ++++----
> >>>> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 2 +-
> >>>> drivers/gpu/drm/imx/ipuv3-crtc.c | 2 +-
> >>>> drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 6 +++---
> >>>> drivers/gpu/drm/omapdrm/displays/encoder-tfp410.c | 2 +-
> >>>> .../drm/omapdrm/displays/panel-lgphilips-lb035q02.c | 2 +-
> >>>> .../gpu/drm/omapdrm/displays/panel-nec-nl8048hl11.c | 2 +-
> >>>> .../drm/omapdrm/displays/panel-sharp-ls037v7dw01.c | 2 +-
> >>>> .../gpu/drm/omapdrm/displays/panel-sony-acx565akm.c | 2 +-
> >>>> .../gpu/drm/omapdrm/displays/panel-tpo-td028ttec1.c | 2 +-
> >>>> .../gpu/drm/omapdrm/displays/panel-tpo-td043mtea1.c | 2 +-
> >>>> drivers/gpu/drm/omapdrm/dss/dsi.c | 2 +-
> >>>> drivers/gpu/drm/omapdrm/dss/sdi.c | 2 +-
> >>>> drivers/gpu/drm/omapdrm/omap_encoder.c | 4 ++--
> >>>> drivers/gpu/drm/panel/panel-arm-versatile.c | 4 ++--
> >>>> drivers/gpu/drm/panel/panel-ilitek-ili9322.c | 4 ++--
> >>>> drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 2 +-
> >>>> drivers/gpu/drm/panel/panel-simple.c | 20 ++++++------
> >>>> drivers/gpu/drm/pl111/pl111_display.c | 2 +-
> >>>> drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++--
> >>>> drivers/gpu/drm/tve200/tve200_display.c | 3 ++-
> >>>> include/drm/drm_bridge.h | 8 ++++----
> >>>> 23 files changed, 47 insertions(+), 46 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c
> >>>> b/drivers/gpu/drm/bridge/dumb-vga-dac.c index
> >>>> 9b706789a341..7dc14c22f7db 100644
> >>>> --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
> >>>> +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
> >>>> @@ -234,7 +234,7 @@ static int dumb_vga_remove(struct platform_device
> >>>> *pdev)
> >>>> */
> >>>> static const struct drm_bridge_timings default_dac_timings = {
> >>>> /* Timing specifications, datasheet page 7 */
> >>>> - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> >>>> + .sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
> >>>> .setup_time_ps = 500,
> >>>> .hold_time_ps = 1500,
> >>>> };
> >>>> @@ -245,7 +245,7 @@ static const struct drm_bridge_timings
> >>>> default_dac_timings = {
> >>>> */
> >>>> static const struct drm_bridge_timings ti_ths8134_dac_timings = {
> >>>> /* From timing diagram, datasheet page 9 */
> >>>> - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> >>>> + .sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
> >>>> /* From datasheet, page 12 */
> >>>> .setup_time_ps = 3000,
> >>>> /* I guess this means latched input */
> >>>> @@ -258,7 +258,7 @@ static const struct drm_bridge_timings
> >>>> ti_ths8134_dac_timings = {
> >>>> */
> >>>> static const struct drm_bridge_timings ti_ths8135_dac_timings = {
> >>>> /* From timing diagram, datasheet page 14 */
> >>>> - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> >>>> + .sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
> >>>> /* From datasheet, page 16 */
> >>>> .setup_time_ps = 2000,
> >>>> .hold_time_ps = 500,
> >>>
> >>> Now I'm confused. Aren't you effectively iverting the sampling edges
> >>> here?
> >>
> >> The meaning of the flag has been defined differently for the
> >> sampling_edge case, see current comment in include/drm/drm_bridge.h.
> >>
> >> This is correct. It essentially fixes the flags such that they can be
> >> interpreted by the driver with the usual assumption to drive on opposite
> >> edge. It is essentially the same as my patch did, but using the new
> >> flag:
> >> https://patchwork.kernel.org/patch/10589233/
> >>
> >> So far, no driver used sampling_edge, so we can change safely at this
> >> point.
> >
> > I was just wondering because the above clearly changes the value in
> > .sampling_edge, but if it isn't currently used it doesn't really matter.
Your concern was correct, the patch changes the value, and as Stefan explained
it is safe as no driver consumes these values. I will however update the
commit message to make this explicit.
> > One potential problem I see here is that the kerneldoc for sampling_edge
> > says that it should reuse the flags from the DRM connector. Now if the
> > DRM connector specifies these from a drive perspective and the bridge
> > interprets them from a sample perspective, this is obviously going to be
> > a problem. But then, the only places where these are used seems to be in
> > the VGA bridge driver, so I'm assuming that these are from a sampling
> > perspective and should be interpreted that way.
> >
> > So I think that's all that we need to define. If a driver specifies the
> > values in some structure, then they should be interpreted from the
> > driver's perspective. If a display driver uses the values provided by a
> > bridge driver it needs to interpret them from a sampling perspective as
> > well.
>
> Until struct drm_bridge_timings has been introduced, *everything* was
> driving perspective. Display timings
> (DISPLAY_FLAGS_PIXDATA_[POS|NEG]EDGE) and bus flags...
>
> struct drm_bridge_timings reuses the bus flags, and with that we have a
> somewhat conflicting definition:
>
> - At the #define site for the flags we document that they are drive
> perspective
> - sampling_edge reuses the same flag, but changes its meaning according
> to kerneldoc
>
> This is not ideal, and my patch as well as this patch series is an
> attempt to clear things up.
>
> > The issue I see with multiple definitions is that it doesn't actually
> > solve the problem. If a bridge driver specifies the flags from a
> > sampling perspective and the display driver interprets them as drive
> > flags, there will still be confusion, right?
>
> Having the drive/sample situation encoded in the flag avoids
> confusion...
>
> We assume that by default we drive/sample at the opposite edge, and that
> is how the flags are defined.
>
> What confusion do you still see?
>
>
> My attempt to clear things up was just using the definition we always
> had:
> https://lkml.org/lkml/2018/9/12/911
>
> I see Laurents point, but I am not sure if its worth the churn.
I'll handle the churn :-)
> Especially since we also use driving view for
> DISPLAY_FLAGS_PIXDATA_[POS|NEG]EDGE (see
> include/video/display_timing.h).
I will try to have a look at that after handling the DRM side.
> Maybe we should just restate the fact that pixel clock is driving view
> in kerneldoc of struct drm_bridge_timings...
I would prefer avoiding that. I think bridges should be as simple as possible
and just expose their own parameters, from their own point of view. The
complexity of deciding on which edge to drive should be on the source (display
controller) side, based on the sink's (bridge) intrinsic parameters (sampling
edge, setup/hold time), on the bus clock and on the source's intrinsic
parameters (internal signal delays). The computation should happen in a helper
function, and it should then be as simple as the following pseudo-code for
display controller drivers.
source_timings.pixdata_delay_ps = 8000;
driving_edge = drm_timings_driving_edge(bridge, mode, &source_timings);
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
