From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomi Valkeinen Subject: Re: [PATCHv3 28/30] drm/omap: fix display SYNC/DE flags Date: Wed, 29 Mar 2017 13:09:28 +0300 Message-ID: <58b2f2c4-65a7-02fa-1dce-0db12c437214@ti.com> References: <1490706496-4959-1-git-send-email-tomi.valkeinen@ti.com> <1490706496-4959-29-git-send-email-tomi.valkeinen@ti.com> <31061557.pgB2iNIeAb@avalon> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0877872858==" Return-path: Received: from lelnx194.ext.ti.com (lelnx194.ext.ti.com [198.47.27.80]) by gabe.freedesktop.org (Postfix) with ESMTPS id 40D266E3DC for ; Wed, 29 Mar 2017 10:09:33 +0000 (UTC) In-Reply-To: <31061557.pgB2iNIeAb@avalon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Laurent Pinchart Cc: Jyri Sarha , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============0877872858== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="eNXborEbHgpEJHFf6RmiP3oguP6EKwON0" --eNXborEbHgpEJHFf6RmiP3oguP6EKwON0 Content-Type: multipart/mixed; boundary="bPCTFTlUHVGT3JMOWBbjHd4HaMbstsBJo"; protected-headers="v1" From: Tomi Valkeinen To: Laurent Pinchart Cc: dri-devel@lists.freedesktop.org, Jyri Sarha Message-ID: <58b2f2c4-65a7-02fa-1dce-0db12c437214@ti.com> Subject: Re: [PATCHv3 28/30] drm/omap: fix display SYNC/DE flags References: <1490706496-4959-1-git-send-email-tomi.valkeinen@ti.com> <1490706496-4959-29-git-send-email-tomi.valkeinen@ti.com> <31061557.pgB2iNIeAb@avalon> In-Reply-To: <31061557.pgB2iNIeAb@avalon> --bPCTFTlUHVGT3JMOWBbjHd4HaMbstsBJo Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 29/03/17 11:58, Laurent Pinchart wrote: > Hi Tomi, >=20 > Thank you for the patch. >=20 > On Tuesday 28 Mar 2017 16:08:14 Tomi Valkeinen wrote: >> At the moment VSYNC/HSYNC/DE high/low flags set by the panel/encoder >> drivers get lost when the videotimings are translated to DRM's >> videomode, as DRM's mode does not have corresponding flags. >> >> DRM has bus-flags for this purpose, and while it lacks a few flags at >> the moment, it should be used here. However, until we rewrite omapdrm'= s >> device model, using bus-flags is rather difficult. >> >> As a short term fix, this patch makes sure that every time the videomo= de >> is set in omap_crtc_mode_set_nofb(), the driver asks for the SYNC/DE >> flags from the panel/encoder drivers, and thus we get the correct flag= s >> into use. >> >> Signed-off-by: Tomi Valkeinen >> --- >> drivers/gpu/drm/omapdrm/omap_connector.c | 2 -- >> drivers/gpu/drm/omapdrm/omap_crtc.c | 28 +++++++++++++++++++++++= ++--- >> 2 files changed, 25 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c >> b/drivers/gpu/drm/omapdrm/omap_connector.c index 50d2b641c28b..d62c035= f050f >> 100644 >> --- a/drivers/gpu/drm/omapdrm/omap_connector.c >> +++ b/drivers/gpu/drm/omapdrm/omap_connector.c >> @@ -146,8 +146,6 @@ static int omap_connector_mode_valid(struct >> drm_connector *connector, int r, ret =3D MODE_BAD; >> >> drm_display_mode_to_videomode(mode, &vm); >> - vm.flags |=3D DISPLAY_FLAGS_DE_HIGH | DISPLAY_FLAGS_PIXDATA_POSEDGE = | >> - DISPLAY_FLAGS_SYNC_NEGEDGE; >=20 > The dssdrv->get_timings() call below will return a video mode with pola= rity=20 > flags set, which will then be compared to vm. As you don't set the vm f= lags=20 > anymore, won't the comparison fail ? Hmm, true. We never go to that code path, though, as we have check_timings in all the drivers, except panel-dsi-cs.c (but there's a patch adding it). And the panels don't check the flags, only things like resolution or porches. I think we can just require all drivers to have check_timings. It's hiding the problem a bit, but should work fine until this can be solved properly. Or actually, perhaps as a quick fix I should change the check to ignore the flags for now. >> mode->vrefresh =3D drm_mode_vrefresh(mode); >> >> /* >> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c >> b/drivers/gpu/drm/omapdrm/omap_crtc.c index 1db96b077ae8..606ef807741a= >> 100644 >> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c >> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c >> @@ -373,6 +373,11 @@ static void omap_crtc_mode_set_nofb(struct drm_cr= tc >> *crtc) { >> struct omap_crtc *omap_crtc =3D to_omap_crtc(crtc); >> struct drm_display_mode *mode =3D &crtc->state->adjusted_mode; >> + struct omap_drm_private *priv =3D crtc->dev->dev_private; >> + const u32 flags_mask =3D DISPLAY_FLAGS_DE_HIGH | DISPLAY_FLAGS_DE_LO= W | >> + DISPLAY_FLAGS_PIXDATA_POSEDGE | DISPLAY_FLAGS_PIXDATA_NEGEDGE=20 > | >> + DISPLAY_FLAGS_SYNC_POSEDGE | DISPLAY_FLAGS_SYNC_NEGEDGE; >> + int i; >=20 > i only takes positive values, you can make it an unsigned int. Yep. >> >> DBG("%s: set mode: %d:\"%s\" %d %d %d %d %d %d %d %d %d %d 0x%x 0x%x= ", >> omap_crtc->name, mode->base.id, mode->name, >> @@ -382,9 +387,26 @@ static void omap_crtc_mode_set_nofb(struct drm_cr= tc >> *crtc) mode->type, mode->flags); >> >> drm_display_mode_to_videomode(mode, &omap_crtc->vm); >> - omap_crtc->vm.flags |=3D DISPLAY_FLAGS_DE_HIGH | >> - DISPLAY_FLAGS_PIXDATA_POSEDGE | >> - DISPLAY_FLAGS_SYNC_NEGEDGE; >> + >=20 > Could you add a HACK/FIXME comment to explain how this should be fixed = > properly ? I want to make sure we won't forget to remove this hack. I'll add a comment. I don't know exactly how this should be fixed, but at least I can explain the hack there. Tomi --bPCTFTlUHVGT3JMOWBbjHd4HaMbstsBJo-- --eNXborEbHgpEJHFf6RmiP3oguP6EKwON0 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJY24fYAAoJEPo9qoy8lh71yNoQAID0N51klFoTI9eovAV/+cJH qZy10V+uFMwTCvwNwWvU6a/P3IS3g/Zkh0QzuYYlyBgJ+z3lFeO234vTNIqlrNfE d8On9FjXbrC6WJ5sSTUtdAM15E2yCNeqwzsZQ6fALqWcCF/rGsSwbOioYX8XE22I FVBBMqvL5oFSxJ0DfL5kyKYuNVuf5LrODt4agGAD1jQi0r4zCnho7bnlqYLn8RP/ 3CeB1j4gVVHlvu+A25fTF/UCNxGnN45vQ5kjOMslmc/EowNUPfVW3APWmpSIdtMl SvCldUsZVowVzpax6HnXspK/+UAfLRWDViRrJgDnVvMjwWr/kK4cSP0TS38GtWBk 2UTCYAwqy5abynjkb2wGk2BBaIF2SIN+t97MAsqdbhloFCKz/SpjBPiFxqKdPgPf 5eUq6TrLtl1uX01eADC3nV2xEyrB4zcTGtPHZh3c2I+sc9SXuQmri/jOyqpR8lA9 6xEbc04nlUBvYIIt7BfP2xorzMff8p19AhykA4h/D4dYWaR/wy5UrccR25uLc2bJ 1Vp/ksTyWrfFPWn0XY9GPzzTl17l80jffWlSh478V0Ys4X/AbY3vArMxmY3Rnd95 IB+XYGlnmU1Z2D8Fwe6t3+Ez31bNe1L9PMEUY+175L2fxFuuoRc+tbJgT5ucG1YX CvENb54WIACgsKoy6hR1 =vNkp -----END PGP SIGNATURE----- --eNXborEbHgpEJHFf6RmiP3oguP6EKwON0-- --===============0877872858== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0877872858==--