From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH v2 3/6] drm/plane: Add optional ->atomic_disable() callback Date: Tue, 25 Nov 2014 12:45:46 +0100 Message-ID: <20141125114544.GA29735@ulmo> References: <1416913789-23209-1-git-send-email-thierry.reding@gmail.com> <1416913789-23209-3-git-send-email-thierry.reding@gmail.com> <20141125112234.GX25711@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0891321290==" Return-path: Received: from mail-wi0-f180.google.com (mail-wi0-f180.google.com [209.85.212.180]) by gabe.freedesktop.org (Postfix) with ESMTP id A32906E3EC for ; Tue, 25 Nov 2014 03:45:48 -0800 (PST) Received: by mail-wi0-f180.google.com with SMTP id n3so1119891wiv.13 for ; Tue, 25 Nov 2014 03:45:47 -0800 (PST) In-Reply-To: <20141125112234.GX25711@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter Cc: Daniel Vetter , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============0891321290== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="n8g4imXOkfNTN/H1" Content-Disposition: inline --n8g4imXOkfNTN/H1 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Nov 25, 2014 at 12:22:34PM +0100, Daniel Vetter wrote: > On Tue, Nov 25, 2014 at 12:09:46PM +0100, Thierry Reding wrote: > > From: Thierry Reding > >=20 > > In order to prevent drivers from having to perform the same checks over > > and over again, add an optional ->atomic_disable callback which the core > > calls under the right circumstances. > >=20 > > v2: pass old state and detect edges to avoid calling ->atomic_disable on > > already disabled planes, remove redundant comment (Daniel Vetter) > >=20 > > Signed-off-by: Thierry Reding >=20 > Some minor bikesheds about consistency and clarity below. > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 15 +++++++++++++-- > > drivers/gpu/drm/drm_plane_helper.c | 10 +++++++++- > > include/drm/drm_crtc.h | 26 ++++++++++++++++++++++++++ > > include/drm/drm_plane_helper.h | 3 +++ > > 4 files changed, 51 insertions(+), 3 deletions(-) > >=20 > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_= atomic_helper.c > > index 7f020403ffe0..a1c7d1b73f86 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -1020,12 +1020,23 @@ void drm_atomic_helper_commit_planes(struct drm= _device *dev, > > continue; > > =20 > > funcs =3D plane->helper_private; > > - > > - if (!funcs || !funcs->atomic_update) > > + if (!funcs) > > continue; > > =20 > > old_plane_state =3D old_state->plane_states[i]; > > =20 > > + /* > > + * Special-case disabling the plane if drivers support it. > > + */ > > + if (drm_plane_disabled(plane, old_plane_state) && > > + funcs->atomic_disable) { > > + funcs->atomic_disable(plane, old_plane_state); > > + continue; > > + } > > + > > + if (!funcs->atomic_update) > > + continue; >=20 > This looks dangerous - we really should either have the atomic_disable or > at least atomic_update. And plane transitional helpers exactly require > that. Note that this isn't anything that this patch introduces. This function has been optional since the drm_atomic_helper_commit_planes() was first introduced. That said, I agree that ->atomic_update() should not be optional, but I'd propose making that change in a precursory patch. That is, remove the check for !funcs->atomic_update and let the kernel crash if the driver doesn't provide it (drm_plane_helper_commit() will already oops in that case anyway). > On the bikeshed front I also like the plane helper structure more > with the if () atomic_disalbel else atomic_update instead of the continue. The existing code was using this structure, but I think with the above change to make ->atomic_update() mandatory it would make more sense to turn this into a more idiomatic if/else. > > + > > funcs->atomic_update(plane, old_plane_state); > > } > > =20 > > diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_p= lane_helper.c > > index aa399db5d36d..8c81d3a9e625 100644 > > --- a/drivers/gpu/drm/drm_plane_helper.c > > +++ b/drivers/gpu/drm/drm_plane_helper.c > > @@ -443,7 +443,15 @@ int drm_plane_helper_commit(struct drm_plane *plan= e, > > crtc_funcs[i]->atomic_begin(crtc[i]); > > } > > =20 > > - plane_funcs->atomic_update(plane, plane_state); > > + /* > > + * Drivers may optionally implement the ->atomic_disable callback, so > > + * special-case that here. > > + */ > > + if (drm_plane_disabled(plane, plane_state) && > > + plane_funcs->atomic_disable) > > + plane_funcs->atomic_disable(plane, plane_state); > > + else > > + plane_funcs->atomic_update(plane, plane_state); > > =20 > > for (i =3D 0; i < 2; i++) { > > if (crtc_funcs[i] && crtc_funcs[i]->atomic_flush) > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > > index 6da67dfcb6fc..80d0f1c2b265 100644 > > --- a/include/drm/drm_crtc.h > > +++ b/include/drm/drm_crtc.h > > @@ -795,6 +795,32 @@ struct drm_plane { > > struct drm_plane_state *state; > > }; > > =20 > > +/* > > + * drm_plane_disabled - check whether a plane is being disabled > > + * @plane: plane object > > + * @old_state: previous atomic state > > + * > > + * Checks the atomic state of a plane to determine whether it's being = disabled > > + * or not. This also WARNs if it detects an invalid state (both CRTC a= nd FB > > + * need to either both be NULL or both be non-NULL). > > + * > > + * RETURNS: > > + * True if the plane is being disabled, false otherwise. > > + */ > > +static inline bool drm_plane_disabled(struct drm_plane *plane, > > + struct drm_plane_state *old_state) > > +{ > > + /* > > + * When disabling a plane, CRTC and FB should always be NULL together. > > + * Anything else should be considered a bug in the atomic core, so we > > + * gently warn about it. > > + */ > > + WARN_ON((plane->state->crtc =3D=3D NULL && plane->state->fb !=3D NULL= ) || > > + (plane->state->crtc !=3D NULL && plane->state->fb =3D=3D NULL)); > > + > > + return (!old_state || old_state->crtc) && !plane->state->crtc; >=20 > The !old_state check here confused me a bit, until I've realized that you > use this for transitional helpers too. What about adding >=20 > /* Cope with NULL old_state for transitional helpers. */ >=20 > right above? Sounds good. >=20 > > +} >=20 > Hm, given that this is used by helpers maybe place it into > drm_atomic_helper.h? It technically operates only on the drm_plane and drm_plane_state so could be useful outside of helpers, but I have no objections to moving it to the helpers. > I'm also not too happy about the name, disabled > doesn't clearly only mean the enable->disable transition. What about > drm_atomic_plane_disabling instead, to make it clear we only care about > the transition? After all your kerneldoc also uses continuous ("being > disabled"). Okay, I can't think of anything better, so drm_atomic_plane_disabling() it is. Thierry --n8g4imXOkfNTN/H1 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUdGvoAAoJEN0jrNd/PrOhPZoP/AwNicJEPDE/p8ymB/n/132s 6ayl1uhHgVUshA7L287+Rb295mupEdGgHaziwzH1v6Lp9N8zqPe2eahxmrcmQfmB 5oJA+Qs+RYazbh1ArP/lzYJrvjxYh2wNe5btrHQLTtqV3Qi5HIbWwZB53CVbiHgA NBfgK0yeoL9SQMKjeqB3GWZ+/i6o9Td8NwRuxx8dK2f2QuICzUskrNLa1cbswzeW bMUg8O2uH4KDLAE5kOT2hABndu3sksRYMALt7YLPXYzngxPJEe2c5M3ZLwz5TvbQ ZaUIwbfczgFtjToAVMlwjJ/+XRs5i3VtMMFMufkNC8kRHahwuJPBENIb5tIyC/Bi I+mHG0g5TTmztlAKHbJtx7T43NesaaG4SZar+xQ8Rq0vrl9k6HnC3iWRRhNmQC+g 5JzKp5lQId8oROStweGR6Pv9JymtHJNHrYNQUxEhxUzbdYMWMklNyHkdLKikZQLm q5Y9L9w43dfSF7wOTyWenCIW7cAa5lkHi701/w9y2Bd3trKDspyhymFroCptZemQ P9IKYUvY/4mP4d8ssHA5SUX/xghrJvSBL0e3KEYgcmpavebvSbsg4mMVRopCeovR 8pmNm4EclVqmLSH9cLL/FS21BCRFG0hC5omXgFsvaniHfwngUTNbxpc0yjJaZiut Da+Ka4HP6UXPpf/1osgG =hvol -----END PGP SIGNATURE----- --n8g4imXOkfNTN/H1-- --===============0891321290== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK --===============0891321290==--