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

[Thierry Reding]

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 at 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.

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?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20180924/2743e67e/attachment.sig>