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.
> > +
> > 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:
pgpteDYTQ9ptg.pgp
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
