Re: [RFC/PATCH 2/2] drm: Use new DRM_BUS_FLAG_PIXDATA_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags

[Stefan Agner <stefan@agner.ch>]

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@ideasonboard.com>
>> >> ---
>> >>  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.
> 
> 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.
Especially since we also use driving view for
DISPLAY_FLAGS_PIXDATA_[POS|NEG]EDGE (see
include/video/display_timing.h).

Maybe we should just restate the fact that pixel clock is driving view
in kerneldoc of struct drm_bridge_timings...

--
Stefan

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel