Re: [Intel-gfx] [PATCH 07/15] drm/atomic-helper: make drm_gem_plane_helper_prepare_fb the default

[Sam Ravnborg <sam@ravnborg.org>]

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@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx