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

[Daniel Vetter <daniel@ffwll.ch>]

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:
> > > 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.

Oh right I've missed that there's you just moved the check around in your
patch. Still transitional helpers requires this, and I can't think of a
case where we don't need this really. So if you feel like a quick
follow-up patch that would be great.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel