On Tue, Jun 22, 2021 at 9:10 PM Sam Ravnborg <sam@xxxxxxxxxxxx> wrote:
>
> Hi Daniel,
>
> On Tue, Jun 22, 2021 at 06:55:03PM +0200, Daniel Vetter wrote:
> > There's a bunch of atomic drivers who don't do this quite correctly,
> > luckily most of them aren't in wide use or people would have noticed
> > the tearing.
> >
> > By making this the default we avoid the constant audit pain and can
> > additionally remove a ton of lines from vfuncs for a bit more clarity
> > in smaller drivers.
> >
> > While at it complain if there's a cleanup_fb hook but no prepare_fb
> > hook, because that makes no sense. I haven't found any driver which
> > violates this, but better safe than sorry.
> >
> > Subsequent patches will reap the benefits.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> > Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > Cc: David Airlie <airlied@xxxxxxxx>
> > Cc: Daniel Vetter <daniel@xxxxxxxx>
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c | 10 ++++++++++
> > drivers/gpu/drm/drm_gem_atomic_helper.c | 3 +++
> > include/drm/drm_modeset_helper_vtables.h | 7 +++++--
> > 3 files changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 531f2374b072..9f6c5f21c4d6 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -35,6 +35,7 @@
> > #include <drm/drm_damage_helper.h>
> > #include <drm/drm_device.h>
> > #include <drm/drm_drv.h>
> > +#include <drm/drm_gem_atomic_helper.h>
> > #include <drm/drm_plane_helper.h>
> > #include <drm/drm_print.h>
> > #include <drm/drm_self_refresh_helper.h>
> > @@ -2408,6 +2409,15 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
> > ret = funcs->prepare_fb(plane, new_plane_state);
> > if (ret)
> > goto fail;
> > + } else {
> > + WARN_ON_ONCE(funcs->cleanup_fb);
> > +
> > + if (!drm_core_check_feature(dev, DRIVER_GEM))
> > + continue;
> > +
> > + ret = drm_gem_plane_helper_prepare_fb(plane, new_plane_state);
> > + if (ret)
> > + goto fail;
> > }
> > }
> >
> > diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c
> > index a27135084ae5..bc9396f2a0ed 100644
> > --- a/drivers/gpu/drm/drm_gem_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
> > @@ -135,6 +135,9 @@
> > * GEM based framebuffer drivers which have their buffers always pinned in
> > * memory.
> > *
> > + * This function is the default implementation for GEM drivers of
> > + * &drm_plane_helper_funcs.prepare_fb if no callback is provided.
> > + *
> > * See drm_atomic_set_fence_for_plane() for a discussion of implicit and
> > * explicit fencing in atomic modeset updates.
> > */
> > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > index f3a4b47b3986..4e727261dca5 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -1178,8 +1178,11 @@ struct drm_plane_helper_funcs {
> > * equivalent functionality should be implemented through private
> > * members in the plane structure.
> > *
> > - * Drivers which always have their buffers pinned should use
> > - * drm_gem_plane_helper_prepare_fb() for this hook.
> > + * For GEM drivers who neither have a @prepare_fb not @cleanup_fb hook
> s/not/nor/ ??
Yup.
> > + * set drm_gem_plane_helper_prepare_fb() is called automatically to
> ^add comma?
> > + * implement this.
>
>
> Leave cleanup_fb out of the description to make it more readable.
With the not->nor typo fixed, why does this make it more readable?
Afaiui neither ... nor ... is fairly standard English, and I really
want to make this the default only if you specify absolutely no plane
fb handling of your own.
> In the description of cleanup_fb you can document that it is wrong to
> have it without a matcching prepare_fb if you feel for it.
So the reason I didn't document things that way is that imo the
"cleanup_fb but not prepare_fb" case is just nonsense. But I also
didn't want to accidentally paper over bugs where people set only
cleanup_fb and forget to hook up the other one, hence the warning. But
if you think we should explain that in docs, I guess I can shuffle it
around. Just feel like specifying everything in the comments doesn't
help the readability of the docs.
-Daniel
>
> Sam
>
>
> * Other drivers which need additional plane processing
> > + * can call drm_gem_plane_helper_prepare_fb() from their @prepare_fb
> > + * hook.
> > *
> > * The helpers will call @cleanup_fb with matching arguments for every
> > * successful call to this hook.
> > --
> > 2.32.0.rc2
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
