From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [RFC/PATCH 2/2] drm: Use new DRM_BUS_FLAG_PIXDATA_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags Date: Mon, 24 Sep 2018 14:13:36 +0200 Message-ID: <20180924121336.GA23547@ulmo> References: <20180922121504.31220-1-laurent.pinchart+renesas@ideasonboard.com> <20180922121504.31220-3-laurent.pinchart+renesas@ideasonboard.com> <20180924114828.GU21032@ulmo> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0231222208==" Return-path: Received: from mail-wm1-x344.google.com (mail-wm1-x344.google.com [IPv6:2a00:1450:4864:20::344]) by gabe.freedesktop.org (Postfix) with ESMTPS id 06E066E250 for ; Mon, 24 Sep 2018 12:13:40 +0000 (UTC) Received: by mail-wm1-x344.google.com with SMTP id y13-v6so5028798wmi.1 for ; Mon, 24 Sep 2018 05:13:39 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Stefan Agner Cc: Laurent Pinchart , Maxime Ripard , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============0231222208== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="huq684BweRXVnRxX" Content-Disposition: inline --huq684BweRXVnRxX Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. Repla= ce > >> them through the code. > >> > >> Signed-off-by: Laurent Pinchart > >> --- > >> 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/b= ridge/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 =3D { > >> /* Timing specifications, datasheet page 7 */ > >> - .sampling_edge =3D DRM_BUS_FLAG_PIXDATA_POSEDGE, > >> + .sampling_edge =3D DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, > >> .setup_time_ps =3D 500, > >> .hold_time_ps =3D 1500, > >> }; > >> @@ -245,7 +245,7 @@ static const struct drm_bridge_timings default_dac= _timings =3D { > >> */ > >> static const struct drm_bridge_timings ti_ths8134_dac_timings =3D { > >> /* From timing diagram, datasheet page 9 */ > >> - .sampling_edge =3D DRM_BUS_FLAG_PIXDATA_POSEDGE, > >> + .sampling_edge =3D DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, > >> /* From datasheet, page 12 */ > >> .setup_time_ps =3D 3000, > >> /* I guess this means latched input */ > >> @@ -258,7 +258,7 @@ static const struct drm_bridge_timings ti_ths8134_= dac_timings =3D { > >> */ > >> static const struct drm_bridge_timings ti_ths8135_dac_timings =3D { > >> /* From timing diagram, datasheet page 14 */ > >> - .sampling_edge =3D DRM_BUS_FLAG_PIXDATA_POSEDGE, > >> + .sampling_edge =3D DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, > >> /* From datasheet, page 16 */ > >> .setup_time_ps =3D 2000, > >> .hold_time_ps =3D 500, > >=20 > > Now I'm confused. Aren't you effectively iverting the sampling edges > > here? >=20 > The meaning of the flag has been defined differently for the > sampling_edge case, see current comment in include/drm/drm_bridge.h. >=20 > 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/ >=20 > 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 =2Esampling_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 --huq684BweRXVnRxX Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAluo1O0ACgkQ3SOs138+ s6FwFA/6AgfeeFFl6V0leGoV/fPOoZC2875N9ORc3iip8QrA9dizboDdnB6pBEjC gGZb60dTq8Aah7fjBYxpvUhrIZssCUJdJvRS+yT5l3aJIfR9oMJ1txbW+AmRPYVE tLkirNTouhzs9ois6NpJWdYIWcOnyH3MQGw0+ZIo6JSb6KryeRg4wPTr3Wvmim3j 9Z7bZLjjqjRux9gogy+JVe/kD6iTC/wHNuEZk7U0XTHFHJsLJULeOxZFtEy0QciG QhesVDq6kPIrMAs6ih+vy36LSnndUk+B+O5vSFg6/jduj2cI4Hwx1nHmV8lG0eNP q5yVL9Y/DIFpRQpHo4SG9s6m0BPbvEKL3s6HTAv44pAitmm+X93sxvf/b8jnclPz 70dOyuoPcGbLlwwuv+g6frCU05tb8QlB4qCDJcWXP0Q23p0zpoCtanioYYTSTkxV Q4M0AdduBkjPKPMBT1qa0uhbsIBPCLxpSL9Alw44j2bpZPDStw/W/770I96eu4jr FZSLE0FknCXl9JF0sU48ODpkRqlv393GFk10W03p1MxzIhRH+RQ23yBCMogXZln7 sB+JDPGjyTb5DP8YZPZzu5gNE1/5I9KcagtZqWijW9TtT9/o4/PWL5piEwCbJ9Wm bUYYjmPb1XzXNAOwqm63neRMev60JJfnSsnjAfTy99MCpbxG9Ag= =GObi -----END PGP SIGNATURE----- --huq684BweRXVnRxX-- --===============0231222208== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0231222208==--