[Intel-gfx] [PATCH 2/3] drm/plane: check that fb_damage is set up when used

[Daniel Vetter]

On Fri, Jul 23, 2021 at 10:34:56AM +0200, Daniel Vetter wrote:
> There's two stages of manual upload/invalidate displays:
> - just handling dirtyfb and uploading the entire fb all the time
> - looking at damage clips
> 
> In the latter case we support it through fbdev emulation (with
> fb_defio), atomic property, and with the dirtfy clip rects.
> 
> Make sure at least the atomic property is set up as the main official
> interface for this. Ideally we'd also check that
> drm_atomic_helper_dirtyfb() is used and that fbdev defio is set up,
> but that's quite a bit harder to do. Ideas very much welcome.
> 
> From a cursor audit drivers seem to be getting this right mostly, but
> better to make sure. At least no one is bypassing the accessor
> function.
> 
> v2:
> - use drm_warn_once with a meaningful warning string (José)
> - don't splat in the atomic check code for everyone (intel-gfx-ci)

v2 got rid of the false positive noise, going to push the series to
drm-misc-next.
-Daniel

> 
> Reviewed-by: José Roberto de Souza <jose.souza at intel.com> (v1)
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: Gwan-gyeong Mun <gwan-gyeong.mun at intel.com>
> Cc: José Roberto de Souza <jose.souza at intel.com>
> Cc: Hans de Goede <hdegoede at redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: Maxime Ripard <mripard at kernel.org>
> Cc: Thomas Zimmermann <tzimmermann at suse.de>
> Cc: David Airlie <airlied at linux.ie>
> Cc: Daniel Vetter <daniel at ffwll.ch>
> ---
>  drivers/gpu/drm/drm_atomic.c        |  2 +-
>  drivers/gpu/drm/drm_crtc_internal.h |  2 ++
>  drivers/gpu/drm/drm_plane.c         | 50 +++++++++++++++++++++++++++++
>  include/drm/drm_plane.h             | 36 +++------------------
>  4 files changed, 57 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index d820423fac32..c85dcfd69158 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -660,7 +660,7 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
>  		return -ENOSPC;
>  	}
>  
> -	clips = drm_plane_get_damage_clips(new_plane_state);
> +	clips = __drm_plane_get_damage_clips(new_plane_state);
>  	num_clips = drm_plane_get_damage_clips_count(new_plane_state);
>  
>  	/* Make sure damage clips are valid and inside the fb. */
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index 1ca51addb589..edb772947cb4 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -262,6 +262,8 @@ int drm_plane_register_all(struct drm_device *dev);
>  void drm_plane_unregister_all(struct drm_device *dev);
>  int drm_plane_check_pixel_format(struct drm_plane *plane,
>  				 u32 format, u64 modifier);
> +struct drm_mode_rect *
> +__drm_plane_get_damage_clips(const struct drm_plane_state *state);
>  
>  /* drm_bridge.c */
>  void drm_bridge_detach(struct drm_bridge *bridge);
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index b373958ecb30..f61315b61174 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1397,6 +1397,56 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	return ret;
>  }
>  
> +/**
> + * drm_plane_get_damage_clips_count - Returns damage clips count.
> + * @state: Plane state.
> + *
> + * Simple helper to get the number of &drm_mode_rect clips set by user-space
> + * during plane update.
> + *
> + * Return: Number of clips in plane fb_damage_clips blob property.
> + */
> +unsigned int
> +drm_plane_get_damage_clips_count(const struct drm_plane_state *state)
> +{
> +	return (state && state->fb_damage_clips) ?
> +		state->fb_damage_clips->length/sizeof(struct drm_mode_rect) : 0;
> +}
> +EXPORT_SYMBOL(drm_plane_get_damage_clips_count);
> +
> +struct drm_mode_rect *
> +__drm_plane_get_damage_clips(const struct drm_plane_state *state)
> +{
> +	return (struct drm_mode_rect *)((state && state->fb_damage_clips) ?
> +					state->fb_damage_clips->data : NULL);
> +}
> +
> +/**
> + * drm_plane_get_damage_clips - Returns damage clips.
> + * @state: Plane state.
> + *
> + * Note that this function returns uapi type &drm_mode_rect. Drivers might want
> + * to use the helper functions drm_atomic_helper_damage_iter_init() and
> + * drm_atomic_helper_damage_iter_next() or drm_atomic_helper_damage_merged() if
> + * the driver can only handle a single damage region at most.
> + *
> + * Return: Damage clips in plane fb_damage_clips blob property.
> + */
> +struct drm_mode_rect *
> +drm_plane_get_damage_clips(const struct drm_plane_state *state)
> +{
> +	struct drm_device *dev = state->plane->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	/* check that drm_plane_enable_fb_damage_clips() was called */
> +	if (!drm_mode_obj_find_prop_id(&state->plane->base,
> +				       config->prop_fb_damage_clips->base.id))
> +		drm_warn_once(dev, "drm_plane_enable_fb_damage_clips() not called\n");
> +
> +	return __drm_plane_get_damage_clips(state);
> +}
> +EXPORT_SYMBOL(drm_plane_get_damage_clips);
> +
>  struct drm_property *
>  drm_create_scaling_filter_prop(struct drm_device *dev,
>  			       unsigned int supported_filters)
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 7f7d5148310c..a2684aab8372 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -897,39 +897,11 @@ static inline struct drm_plane *drm_plane_find(struct drm_device *dev,
>  
>  bool drm_any_plane_has_format(struct drm_device *dev,
>  			      u32 format, u64 modifier);
> -/**
> - * drm_plane_get_damage_clips_count - Returns damage clips count.
> - * @state: Plane state.
> - *
> - * Simple helper to get the number of &drm_mode_rect clips set by user-space
> - * during plane update.
> - *
> - * Return: Number of clips in plane fb_damage_clips blob property.
> - */
> -static inline unsigned int
> -drm_plane_get_damage_clips_count(const struct drm_plane_state *state)
> -{
> -	return (state && state->fb_damage_clips) ?
> -		state->fb_damage_clips->length/sizeof(struct drm_mode_rect) : 0;
> -}
> +unsigned int
> +drm_plane_get_damage_clips_count(const struct drm_plane_state *state);
>  
> -/**
> - * drm_plane_get_damage_clips - Returns damage clips.
> - * @state: Plane state.
> - *
> - * Note that this function returns uapi type &drm_mode_rect. Drivers might want
> - * to use the helper functions drm_atomic_helper_damage_iter_init() and
> - * drm_atomic_helper_damage_iter_next() or drm_atomic_helper_damage_merged() if
> - * the driver can only handle a single damage region at most.
> - *
> - * Return: Damage clips in plane fb_damage_clips blob property.
> - */
> -static inline struct drm_mode_rect *
> -drm_plane_get_damage_clips(const struct drm_plane_state *state)
> -{
> -	return (struct drm_mode_rect *)((state && state->fb_damage_clips) ?
> -					state->fb_damage_clips->data : NULL);
> -}
> +struct drm_mode_rect *
> +drm_plane_get_damage_clips(const struct drm_plane_state *state);
>  
>  int drm_plane_create_scaling_filter_property(struct drm_plane *plane,
>  					     unsigned int supported_filters);
> -- 
> 2.32.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch