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: Fri, 16 Jan 2015 15:35:10 +0100 Message-ID: <20150116143508.GA7775@ulmo.nvidia.com> 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> <20141125114544.GA29735@ulmo> <20141125115702.GB29735@ulmo> <20141125122634.GB25711@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1833021229==" Return-path: Received: from mail-pa0-f43.google.com (mail-pa0-f43.google.com [209.85.220.43]) by gabe.freedesktop.org (Postfix) with ESMTP id 302446E2B6 for ; Fri, 16 Jan 2015 06:35:15 -0800 (PST) Received: by mail-pa0-f43.google.com with SMTP id kx10so24646072pab.2 for ; Fri, 16 Jan 2015 06:35:15 -0800 (PST) In-Reply-To: <20141125122634.GB25711@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 --===============1833021229== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="d6Gm4EdcadzBjdND" Content-Disposition: inline --d6Gm4EdcadzBjdND Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Nov 25, 2014 at 01:26:34PM +0100, Daniel Vetter wrote: > On Tue, Nov 25, 2014 at 12:57:04PM +0100, Thierry Reding wrote: > > On Tue, Nov 25, 2014 at 12:45:46PM +0100, Thierry Reding wrote: > > > 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: > > [...] > > > > > +/* > > > > > + * 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 and 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) > > > > > +{ > > [...] > > > > > + return (!old_state || old_state->crtc) && !plane->state->crtc; > > > >=20 > > > > The !old_state check here confused me a bit, until I've realized th= at you > > > > use this for transitional helpers too. What about adding > > > >=20 > > > > /* Cope with NULL old_state for transitional helpers. */ > > > >=20 > > > > right above? > > >=20 > > > Sounds good. > >=20 > > When I now thought about the reason again it took me a while to > > reconstruct, so I figured I'd be extra verbose and used this: > >=20 > > /* > > * When using the transitional helpers, old_state may be NULL. If so, > > * we know nothing about the current state and have to assume that it > > * might be enabled. > > */ > >=20 > > Does that sound accurate and sufficient to you? >=20 > Hm, thinking about this some more this will result in a slight difference > in behaviour, at least when drivers just use the helper ->reset functions > but don't disable everything: > - With transitional helpers we assume we know nothing and call > ->atomic_disable. > - With atomic old_state->crtc =3D=3D NULL in the same situation right aft= er > boot-up, but we asssume the plane is really off and _dont_ call > ->atomic_disable. >=20 > Should we instead check for (old_state && old_state->crtc) and state that > drivers need to make sure they don't have stuff hanging around? I don't think we can check for old_state because otherwise this will always return false, whereas we really want it to force-disable planes that could be on (lacking any more accurate information). For transitional helpers anyway. For the atomic helpers, old_state will never be NULL, but I'd assume that the driver would reconstruct the current state in ->reset(). > Or maybe just a note that there's a difference in behaviour here? One other option would be to split this up into two functions: - drm_plane_helper_disabling() which is called from drm_plane_helper_commit() and checks for the validity of old_state - drm_atomic_plane_disabling() which is called from drm_atomic_helper_commit_planes() and doesn't have to check for the validity of old_state because it's always valid. On second thought, that doesn't really help because the very first call would still not know whether old_state->crtc is NULL because it's just the default or because the plane is really disabled. So I think the only sane solution to this would be to either have the drivers reconstruct the correct value for state->crtc in ->reset() or make sure to comply with the semantics that planes are initially considered to be disabled. Maybe complementing the above comment like this would help: /* * When using the transitional helpers, old_state may be NULL. If so, * we know nothing about the current state and have to assume that it * might be enabled. This usually happens only on the very first call * of the drm_plane_helper_commit() helper. * * When using the atomic helpers, old_state won't be NULL. Therefore * this check assumes that either the driver will have reconstructed * the current state in ->reset() or that the driver will have taken * measures to disable all planes. */ Thierry --d6Gm4EdcadzBjdND Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUuSGcAAoJEN0jrNd/PrOh2GoQAMHYDy8tTkYVrqVTcYtejt3s 6KCQ3Nu7LbgaWIbdgDRe3XRAKyFeXUBgZeTuBl6tuyRMzbJ2v+jeDmfBd5UhBJRV K9HKSGKooI7a0OId43aYsoT6qtlvNKXAIhlZAl4bOfyMgTSxAZDFHdh0JRDi91TW ClM8qaHutp5qkgNpLWYTIzUdlvW9hL7EczZ/JzBJ5rkuISY4Y/uMb43KDkb0E8Mq uaLnlwwUjr6gSA6/XAr1o1U4tMoTacQ9LNOynGoVtCIURCvbkUJ48nRKFvt74fAL RDOeueJKPN7OCGyeGi8E6tYbHkHAJ7KGgFTI0cckht40YG35mCR0t7I1KuzMtGgd fFuxVuAFE3tMeualG/sBVArl4nirl33bMra9PsKJwSNMC4N7yu4C1+haEtr7QFU/ JMtl/mEmbDL8l02ZGZjzZ9uHN7ebnIrcC6WjIigtBJh36QwDOVQZe1NQc6e7A/YM NSxD4Ef6yIA+85d1upd7IE/3/Lf3lLKUKULPWZH7ATmPTGjGJud8JdqmoONKKqbb RjQ1dRq9cKBe1qOdsP+GXnLOltXb0qsiFiKjdH0+Pk5jnMBUWB5vs34MPF0q4HSm Ta3HDKfNL1qGJhL80oUQopSk5og0UMrrs4oK+XavUw4VFezFwqqt+3RRCMZVzv8G exNgSWe9JE7MS8iv6qJo =LNBe -----END PGP SIGNATURE----- --d6Gm4EdcadzBjdND-- --===============1833021229== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK --===============1833021229==--