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/ ??
> + * 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.
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.
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
