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.
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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
