Re: [PATCH v2 3/6] drm/plane: Add optional ->atomic_disable() callback

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5992 bytes --]

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 <treding@nvidia.com>
> > 
> > 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.
> > 
> > v2: pass old state and detect edges to avoid calling ->atomic_disable on
> > already disabled planes, remove redundant comment (Daniel Vetter)
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> 
> 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(-)
> > 
> > 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;
> >  
> >  		funcs = plane->helper_private;
> > -
> > -		if (!funcs || !funcs->atomic_update)
> > +		if (!funcs)
> >  			continue;
> >  
> >  		old_plane_state = old_state->plane_states[i];
> >  
> > +		/*
> > +		 * 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;
> 
> 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);
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_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 *plane,
> >  			crtc_funcs[i]->atomic_begin(crtc[i]);
> >  	}
> >  
> > -	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);
> >  
> >  	for (i = 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;
> >  };
> >  
> > +/*
> > + * 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)
> > +{
> > +	/*
> > +	 * 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 == NULL && plane->state->fb != NULL) ||
> > +		(plane->state->crtc != NULL && plane->state->fb == NULL));
> > +
> > +	return (!old_state || old_state->crtc) && !plane->state->crtc;
> 
> The !old_state check here confused me a bit, until I've realized that you
> use this for transitional helpers too. What about adding
> 
> 	/* Cope with NULL old_state for transitional helpers. */
> 
> right above?

Sounds good.

> 
> > +}
> 
> 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

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel