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@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
