From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomi Valkeinen Subject: Re: [PATCH v3 11/20] drm: omapdrm: Check DSS manager state in the enable/disable helpers Date: Tue, 20 Sep 2016 16:57:59 +0300 Message-ID: <8f075fac-af07-cec2-2371-24e0fcdc8ba5@ti.com> References: <1474288063-5315-1-git-send-email-laurent.pinchart@ideasonboard.com> <1474288063-5315-12-git-send-email-laurent.pinchart@ideasonboard.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1527862119==" Return-path: Received: from comal.ext.ti.com (comal.ext.ti.com [198.47.26.152]) by gabe.freedesktop.org (Postfix) with ESMTPS id 45FD86E168 for ; Tue, 20 Sep 2016 13:58:05 +0000 (UTC) In-Reply-To: <1474288063-5315-12-git-send-email-laurent.pinchart@ideasonboard.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Laurent Pinchart , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============1527862119== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="lBFqgwR1klduD4n6H6mPL7UCxU0DqfD8e" --lBFqgwR1klduD4n6H6mPL7UCxU0DqfD8e Content-Type: multipart/mixed; boundary="KVesFrHuL9VMKnwTBDb28FjuXqcgaAOpm"; protected-headers="v1" From: Tomi Valkeinen To: Laurent Pinchart , dri-devel@lists.freedesktop.org Message-ID: <8f075fac-af07-cec2-2371-24e0fcdc8ba5@ti.com> Subject: Re: [PATCH v3 11/20] drm: omapdrm: Check DSS manager state in the enable/disable helpers References: <1474288063-5315-1-git-send-email-laurent.pinchart@ideasonboard.com> <1474288063-5315-12-git-send-email-laurent.pinchart@ideasonboard.com> In-Reply-To: <1474288063-5315-12-git-send-email-laurent.pinchart@ideasonboard.com> --KVesFrHuL9VMKnwTBDb28FjuXqcgaAOpm Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 19/09/16 15:27, Laurent Pinchart wrote: > The omapdrm DSS manager enable/disable operations check the DSS manager= > state to avoid double enabling/disabling. Move that code to the DSS > manager to decrease the dependency of the DRM layer to the DSS layer. >=20 > Signed-off-by: Laurent Pinchart > --- > drivers/gpu/drm/omapdrm/dss/dispc.c | 1 - > drivers/gpu/drm/omapdrm/dss/output.c | 6 ++++++ > drivers/gpu/drm/omapdrm/omap_crtc.c | 3 --- > 3 files changed, 6 insertions(+), 4 deletions(-) >=20 > diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omap= drm/dss/dispc.c > index 535240fba671..ab150bf21dd8 100644 > --- a/drivers/gpu/drm/omapdrm/dss/dispc.c > +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c > @@ -2911,7 +2911,6 @@ bool dispc_mgr_is_enabled(enum omap_channel chann= el) > { > return !!mgr_fld_read(channel, DISPC_MGR_FLD_ENABLE); > } > -EXPORT_SYMBOL(dispc_mgr_is_enabled); > =20 > void dispc_wb_enable(bool enable) > { > diff --git a/drivers/gpu/drm/omapdrm/dss/output.c b/drivers/gpu/drm/oma= pdrm/dss/output.c > index 24f859488201..f0be621895fa 100644 > --- a/drivers/gpu/drm/omapdrm/dss/output.c > +++ b/drivers/gpu/drm/omapdrm/dss/output.c > @@ -217,12 +217,18 @@ EXPORT_SYMBOL(dss_mgr_set_lcd_config); > =20 > int dss_mgr_enable(enum omap_channel channel) > { > + if (dispc_mgr_is_enabled(channel)) > + return 0; > + > return dss_mgr_ops->enable(channel); > } > EXPORT_SYMBOL(dss_mgr_enable); > =20 > void dss_mgr_disable(enum omap_channel channel) > { > + if (!dispc_mgr_is_enabled(channel)) > + return; > + > dss_mgr_ops->disable(channel); > } > EXPORT_SYMBOL(dss_mgr_disable); > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omap= drm/omap_crtc.c > index 4b7e16786e1e..a0c26592fc69 100644 > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c > @@ -141,9 +141,6 @@ static void omap_crtc_set_enabled(struct drm_crtc *= crtc, bool enable) > return; > } > =20 > - if (dispc_mgr_is_enabled(channel) =3D=3D enable) > - return; > - > if (omap_crtc->channel =3D=3D OMAP_DSS_CHANNEL_DIGIT) { > /* > * Digit output produces some sync lost interrupts during the >=20 With this change omap_crtc_set_enabled() will do extra work if the output is already enabled/disabled, and, if I'm not mistaken, will do omap_irq_wait() there which might lead to issues. If you remove the check, then I think the driver should make sure that omap_crtc_set_enabled() is not called if the output is already enabled/disabled. Maybe that can be done in omap_crtc_dss_enable/disable, using the new enabled flag. Tomi --KVesFrHuL9VMKnwTBDb28FjuXqcgaAOpm-- --lBFqgwR1klduD4n6H6mPL7UCxU0DqfD8e 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 iQIcBAEBCAAGBQJX4UBnAAoJEPo9qoy8lh71rHkP/jj70bFsYKl025P3C9iNBIu/ UfOJQZxuexyVidTFlE+rvYKzquV4mzehjqjS4LI9uNt+rpCqE3Res/QiyK6P2L4V aqx+3+rsHGRCwquxJfcfvIVS0BA2hiHxyTeUQ451dJy2RsxNWnK2Ungf6QU6Q9H/ bAZLu7dTcXTrcKAEBesX79NsNTNoonCZG/YQVvR9kBAqUgs42XCfzKlY1VDlFJIi rlx3YDufXtQrRNspiv0DjGM0jRZvwVgUuNDuDdwj/ELFTucclPvPdsTg/95IqsCg ZTJKpwwcaCiSwgOiB0aBDzEkt2tmFC9pxkqEDbepg6wii0ZOHRu9V24pP7IN9jNv 9bZPX6p5gTDg4KxqIIlCuLwjgqp1HjFnDmd9Wn9yAz5ehy/4lyuu/pP83/66P/Ag /VaU//L971jFTw7kCD7dueQxG4na/Aw/NQf63sJXcavBC0Gs03K+Ma/DDOrelXzk FTxakehtOamnxRdEuE+70Yh91LjFMM19QMH0NLjZyUh8obX8gLhBTK1CtVqdao2s XrXWCDv2uB77+7KfLUP+gxL8bnnvK2imBw0iD4QsfH1g9S6ChVmKjyUCYXwbewqo qifJUGdE8XwdbK0CqSYe2Bnah9Cc/LiaG9NJC0q/cgUvxB5BiIcjZTYDX2k6Jpb7 6SXG4+4eagRDoeGqZnF1 =XNcj -----END PGP SIGNATURE----- --lBFqgwR1klduD4n6H6mPL7UCxU0DqfD8e-- --===============1527862119== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============1527862119==--