[PATCH 1/3] drm/plane: remove drm_helper_get_plane_damage_clips

[PATCH 1/3] drm/plane: remove drm_helper_get_plane_damage_clips

[Daniel Vetter]

It's not used. Drivers should instead use the helpers anyway.

Currently both vbox and i915 hand-roll this and it's not the greatest.
vbox looks buggy, and i915 does a bit much that helpers would take
care of I think.

Also improve the kerneldocs while we're at it.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
Cc: José Roberto de Souza <jose.souza@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
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_damage_helper.c |  2 +-
 include/drm/drm_damage_helper.h     | 17 -----------------
 include/drm/drm_plane.h             | 10 +++++++---
 include/drm/drm_rect.h              |  3 +++
 4 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
index 3a4126dc2520..eb69b7123af5 100644
--- a/drivers/gpu/drm/drm_damage_helper.c
+++ b/drivers/gpu/drm/drm_damage_helper.c
@@ -282,7 +282,7 @@ drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
 	if (!state || !state->crtc || !state->fb || !state->visible)
 		return;
 
-	iter->clips = drm_helper_get_plane_damage_clips(state);
+	iter->clips = (struct drm_rect *)drm_plane_get_damage_clips(state);
 	iter->num_clips = drm_plane_get_damage_clips_count(state);
 
 	/* Round down for x1/y1 and round up for x2/y2 to catch all pixels */
diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h
index 40c34a5bf149..1ae8bce6a5ce 100644
--- a/include/drm/drm_damage_helper.h
+++ b/include/drm/drm_damage_helper.h
@@ -82,21 +82,4 @@ bool drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state,
 				     struct drm_plane_state *state,
 				     struct drm_rect *rect);
 
-/**
- * drm_helper_get_plane_damage_clips - Returns damage clips in &drm_rect.
- * @state: Plane state.
- *
- * Returns plane damage rectangles in internal &drm_rect. Currently &drm_rect
- * can be obtained by simply typecasting &drm_mode_rect. This is because both
- * are signed 32 and during drm_atomic_check_only() it is verified that damage
- * clips are inside fb.
- *
- * Return: Clips in plane fb_damage_clips blob property.
- */
-static inline struct drm_rect *
-drm_helper_get_plane_damage_clips(const struct drm_plane_state *state)
-{
-	return (struct drm_rect *)drm_plane_get_damage_clips(state);
-}
-
 #endif
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 1294610e84f4..7f7d5148310c 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -186,6 +186,9 @@ struct drm_plane_state {
 	 * since last plane update) as an array of &drm_mode_rect in framebuffer
 	 * coodinates of the attached framebuffer. Note that unlike plane src,
 	 * damage clips are not in 16.16 fixed point.
+	 *
+	 * See drm_plane_get_damage_clips() and
+	 * drm_plane_get_damage_clips_count() for accessing these.
 	 */
 	struct drm_property_blob *fb_damage_clips;
 
@@ -914,9 +917,10 @@ 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
- * instead be interested in internal &drm_rect which can be obtained by calling
- * drm_helper_get_plane_damage_clips().
+ * 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.
  */
diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
index 39f2deee709c..6f6e19bd4dac 100644
--- a/include/drm/drm_rect.h
+++ b/include/drm/drm_rect.h
@@ -39,6 +39,9 @@
  * @x2: horizontal ending coordinate (exclusive)
  * @y1: vertical starting coordinate (inclusive)
  * @y2: vertical ending coordinate (exclusive)
+ *
+ * Note that this must match the layout of struct drm_mode_rect or the damage
+ * helpers like drm_atomic_helper_damage_iter_init() break.
  */
 struct drm_rect {
 	int x1, y1, x2, y2;
-- 
2.32.0

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

[Daniel Vetter]

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.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
Cc: José Roberto de Souza <jose.souza@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
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_plane.c | 42 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_plane.h     | 36 ++++---------------------------
 2 files changed, 46 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index b373958ecb30..40f099c67a8d 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1397,6 +1397,48 @@ 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);
+
+/**
+ * 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_mode_config *config = &state->plane->dev->mode_config;
+
+	/* check that drm_plane_enable_fb_damage_clips() was called */
+	WARN_ON_ONCE(!drm_mode_obj_find_prop_id(&state->plane->base,
+						config->prop_fb_damage_clips->base.id));
+
+	return (struct drm_mode_rect *)((state && state->fb_damage_clips) ?
+					state->fb_damage_clips->data : NULL);
+}
+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

[PATCH 3/3] drm/plane: Move drm_plane_enable_fb_damage_clips into core

[Daniel Vetter]

We're trying to have a fairly strict split between core functionality
that defines the uapi, including the docs, and the helper functions to
implement it.

Move drm_plane_enable_fb_damage_clips and associated kerneldoc into
drm_plane from drm_damage_helper.c to fix this.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
Cc: José Roberto de Souza <jose.souza@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
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>
---
 Documentation/gpu/drm-kms.rst       |  4 +--
 drivers/gpu/drm/drm_damage_helper.c | 54 -----------------------------
 drivers/gpu/drm/drm_plane.c         | 54 +++++++++++++++++++++++++++++
 include/drm/drm_damage_helper.h     |  1 -
 include/drm/drm_plane.h             |  3 +-
 5 files changed, 58 insertions(+), 58 deletions(-)

diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
index 87e5023e3f55..7399f497e7e2 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -508,8 +508,8 @@ Plane Composition Properties
 Damage Tracking Properties
 --------------------------
 
-.. kernel-doc:: drivers/gpu/drm/drm_damage_helper.c
-   :doc: overview
+.. kernel-doc:: drivers/gpu/drm/drm_plane.c
+   :doc: damage tracking
 
 Color Management Properties
 ---------------------------
diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
index eb69b7123af5..245959dad7bb 100644
--- a/drivers/gpu/drm/drm_damage_helper.c
+++ b/drivers/gpu/drm/drm_damage_helper.c
@@ -34,44 +34,6 @@
 #include <drm/drm_damage_helper.h>
 #include <drm/drm_device.h>
 
-/**
- * DOC: overview
- *
- * FB_DAMAGE_CLIPS is an optional plane property which provides a means to
- * specify a list of damage rectangles on a plane in framebuffer coordinates of
- * the framebuffer attached to the plane. In current context damage is the area
- * of plane framebuffer that has changed since last plane update (also called
- * page-flip), irrespective of whether currently attached framebuffer is same as
- * framebuffer attached during last plane update or not.
- *
- * FB_DAMAGE_CLIPS is a hint to kernel which could be helpful for some drivers
- * to optimize internally especially for virtual devices where each framebuffer
- * change needs to be transmitted over network, usb, etc.
- *
- * Since FB_DAMAGE_CLIPS is a hint so it is an optional property. User-space can
- * ignore damage clips property and in that case driver will do a full plane
- * update. In case damage clips are provided then it is guaranteed that the area
- * inside damage clips will be updated to plane. For efficiency driver can do
- * full update or can update more than specified in damage clips. Since driver
- * is free to read more, user-space must always render the entire visible
- * framebuffer. Otherwise there can be corruptions. Also, if a user-space
- * provides damage clips which doesn't encompass the actual damage to
- * framebuffer (since last plane update) can result in incorrect rendering.
- *
- * FB_DAMAGE_CLIPS is a blob property with the layout of blob data is simply an
- * array of &drm_mode_rect. Unlike plane &drm_plane_state.src coordinates,
- * damage clips are not in 16.16 fixed point. Similar to plane src in
- * framebuffer, damage clips cannot be negative. In damage clip, x1/y1 are
- * inclusive and x2/y2 are exclusive. While kernel does not error for overlapped
- * damage clips, it is strongly discouraged.
- *
- * Drivers that are interested in damage interface for plane should enable
- * FB_DAMAGE_CLIPS property by calling drm_plane_enable_fb_damage_clips().
- * Drivers implementing damage can use drm_atomic_helper_damage_iter_init() and
- * drm_atomic_helper_damage_iter_next() helper iterator function to get damage
- * rectangles clipped to &drm_plane_state.src.
- */
-
 static void convert_clip_rect_to_rect(const struct drm_clip_rect *src,
 				      struct drm_mode_rect *dest,
 				      uint32_t num_clips, uint32_t src_inc)
@@ -87,22 +49,6 @@ static void convert_clip_rect_to_rect(const struct drm_clip_rect *src,
 	}
 }
 
-/**
- * drm_plane_enable_fb_damage_clips - Enables plane fb damage clips property.
- * @plane: Plane on which to enable damage clips property.
- *
- * This function lets driver to enable the damage clips property on a plane.
- */
-void drm_plane_enable_fb_damage_clips(struct drm_plane *plane)
-{
-	struct drm_device *dev = plane->dev;
-	struct drm_mode_config *config = &dev->mode_config;
-
-	drm_object_attach_property(&plane->base, config->prop_fb_damage_clips,
-				   0);
-}
-EXPORT_SYMBOL(drm_plane_enable_fb_damage_clips);
-
 /**
  * drm_atomic_helper_check_plane_damage - Verify plane damage on atomic_check.
  * @state: The driver state object.
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 40f099c67a8d..b68d06f536fa 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1397,6 +1397,60 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	return ret;
 }
 
+/**
+ * DOC: damage tracking
+ *
+ * FB_DAMAGE_CLIPS is an optional plane property which provides a means to
+ * specify a list of damage rectangles on a plane in framebuffer coordinates of
+ * the framebuffer attached to the plane. In current context damage is the area
+ * of plane framebuffer that has changed since last plane update (also called
+ * page-flip), irrespective of whether currently attached framebuffer is same as
+ * framebuffer attached during last plane update or not.
+ *
+ * FB_DAMAGE_CLIPS is a hint to kernel which could be helpful for some drivers
+ * to optimize internally especially for virtual devices where each framebuffer
+ * change needs to be transmitted over network, usb, etc.
+ *
+ * Since FB_DAMAGE_CLIPS is a hint so it is an optional property. User-space can
+ * ignore damage clips property and in that case driver will do a full plane
+ * update. In case damage clips are provided then it is guaranteed that the area
+ * inside damage clips will be updated to plane. For efficiency driver can do
+ * full update or can update more than specified in damage clips. Since driver
+ * is free to read more, user-space must always render the entire visible
+ * framebuffer. Otherwise there can be corruptions. Also, if a user-space
+ * provides damage clips which doesn't encompass the actual damage to
+ * framebuffer (since last plane update) can result in incorrect rendering.
+ *
+ * FB_DAMAGE_CLIPS is a blob property with the layout of blob data is simply an
+ * array of &drm_mode_rect. Unlike plane &drm_plane_state.src coordinates,
+ * damage clips are not in 16.16 fixed point. Similar to plane src in
+ * framebuffer, damage clips cannot be negative. In damage clip, x1/y1 are
+ * inclusive and x2/y2 are exclusive. While kernel does not error for overlapped
+ * damage clips, it is strongly discouraged.
+ *
+ * Drivers that are interested in damage interface for plane should enable
+ * FB_DAMAGE_CLIPS property by calling drm_plane_enable_fb_damage_clips().
+ * Drivers implementing damage can use drm_atomic_helper_damage_iter_init() and
+ * drm_atomic_helper_damage_iter_next() helper iterator function to get damage
+ * rectangles clipped to &drm_plane_state.src.
+ */
+
+/**
+ * drm_plane_enable_fb_damage_clips - Enables plane fb damage clips property.
+ * @plane: Plane on which to enable damage clips property.
+ *
+ * This function lets driver to enable the damage clips property on a plane.
+ */
+void drm_plane_enable_fb_damage_clips(struct drm_plane *plane)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+
+	drm_object_attach_property(&plane->base, config->prop_fb_damage_clips,
+				   0);
+}
+EXPORT_SYMBOL(drm_plane_enable_fb_damage_clips);
+
 /**
  * drm_plane_get_damage_clips_count - Returns damage clips count.
  * @state: Plane state.
diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h
index 1ae8bce6a5ce..effda42cce31 100644
--- a/include/drm/drm_damage_helper.h
+++ b/include/drm/drm_damage_helper.h
@@ -64,7 +64,6 @@ struct drm_atomic_helper_damage_iter {
 	bool full_update;
 };
 
-void drm_plane_enable_fb_damage_clips(struct drm_plane *plane);
 void drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state,
 					  struct drm_plane_state *plane_state);
 int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index a2684aab8372..fed97e35626f 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -897,9 +897,10 @@ 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);
+
+void drm_plane_enable_fb_damage_clips(struct drm_plane *plane);
 unsigned int
 drm_plane_get_damage_clips_count(const struct drm_plane_state *state);
-
 struct drm_mode_rect *
 drm_plane_get_damage_clips(const struct drm_plane_state *state);
 
-- 
2.32.0

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

[Souza, Jose]

On Wed, 2021-07-21 at 15:30 +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.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> 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_plane.c | 42 +++++++++++++++++++++++++++++++++++++
>  include/drm/drm_plane.h     | 36 ++++---------------------------
>  2 files changed, 46 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index b373958ecb30..40f099c67a8d 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1397,6 +1397,48 @@ 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);
> +
> +/**
> + * 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_mode_config *config = &state->plane->dev->mode_config;
> +
> +	/* check that drm_plane_enable_fb_damage_clips() was called */
> +	WARN_ON_ONCE(!drm_mode_obj_find_prop_id(&state->plane->base,
> +						config->prop_fb_damage_clips->base.id));

Changing to drm_warn_once()

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> +
> +	return (struct drm_mode_rect *)((state && state->fb_damage_clips) ?
> +					state->fb_damage_clips->data : NULL);
> +}
> +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);

Re: [PATCH 3/3] drm/plane: Move drm_plane_enable_fb_damage_clips into core

[Souza, Jose]

On Wed, 2021-07-21 at 15:30 +0200, Daniel Vetter wrote:
> We're trying to have a fairly strict split between core functionality
> that defines the uapi, including the docs, and the helper functions to
> implement it.
> 
> Move drm_plane_enable_fb_damage_clips and associated kerneldoc into
> drm_plane from drm_damage_helper.c to fix this.

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> 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>
> ---
>  Documentation/gpu/drm-kms.rst       |  4 +--
>  drivers/gpu/drm/drm_damage_helper.c | 54 -----------------------------
>  drivers/gpu/drm/drm_plane.c         | 54 +++++++++++++++++++++++++++++
>  include/drm/drm_damage_helper.h     |  1 -
>  include/drm/drm_plane.h             |  3 +-
>  5 files changed, 58 insertions(+), 58 deletions(-)
> 
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 87e5023e3f55..7399f497e7e2 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -508,8 +508,8 @@ Plane Composition Properties
>  Damage Tracking Properties
>  --------------------------
>  
> -.. kernel-doc:: drivers/gpu/drm/drm_damage_helper.c
> -   :doc: overview
> +.. kernel-doc:: drivers/gpu/drm/drm_plane.c
> +   :doc: damage tracking
>  
>  Color Management Properties
>  ---------------------------
> diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
> index eb69b7123af5..245959dad7bb 100644
> --- a/drivers/gpu/drm/drm_damage_helper.c
> +++ b/drivers/gpu/drm/drm_damage_helper.c
> @@ -34,44 +34,6 @@
>  #include <drm/drm_damage_helper.h>
>  #include <drm/drm_device.h>
>  
> -/**
> - * DOC: overview
> - *
> - * FB_DAMAGE_CLIPS is an optional plane property which provides a means to
> - * specify a list of damage rectangles on a plane in framebuffer coordinates of
> - * the framebuffer attached to the plane. In current context damage is the area
> - * of plane framebuffer that has changed since last plane update (also called
> - * page-flip), irrespective of whether currently attached framebuffer is same as
> - * framebuffer attached during last plane update or not.
> - *
> - * FB_DAMAGE_CLIPS is a hint to kernel which could be helpful for some drivers
> - * to optimize internally especially for virtual devices where each framebuffer
> - * change needs to be transmitted over network, usb, etc.
> - *
> - * Since FB_DAMAGE_CLIPS is a hint so it is an optional property. User-space can
> - * ignore damage clips property and in that case driver will do a full plane
> - * update. In case damage clips are provided then it is guaranteed that the area
> - * inside damage clips will be updated to plane. For efficiency driver can do
> - * full update or can update more than specified in damage clips. Since driver
> - * is free to read more, user-space must always render the entire visible
> - * framebuffer. Otherwise there can be corruptions. Also, if a user-space
> - * provides damage clips which doesn't encompass the actual damage to
> - * framebuffer (since last plane update) can result in incorrect rendering.
> - *
> - * FB_DAMAGE_CLIPS is a blob property with the layout of blob data is simply an
> - * array of &drm_mode_rect. Unlike plane &drm_plane_state.src coordinates,
> - * damage clips are not in 16.16 fixed point. Similar to plane src in
> - * framebuffer, damage clips cannot be negative. In damage clip, x1/y1 are
> - * inclusive and x2/y2 are exclusive. While kernel does not error for overlapped
> - * damage clips, it is strongly discouraged.
> - *
> - * Drivers that are interested in damage interface for plane should enable
> - * FB_DAMAGE_CLIPS property by calling drm_plane_enable_fb_damage_clips().
> - * Drivers implementing damage can use drm_atomic_helper_damage_iter_init() and
> - * drm_atomic_helper_damage_iter_next() helper iterator function to get damage
> - * rectangles clipped to &drm_plane_state.src.
> - */
> -
>  static void convert_clip_rect_to_rect(const struct drm_clip_rect *src,
>  				      struct drm_mode_rect *dest,
>  				      uint32_t num_clips, uint32_t src_inc)
> @@ -87,22 +49,6 @@ static void convert_clip_rect_to_rect(const struct drm_clip_rect *src,
>  	}
>  }
>  
> -/**
> - * drm_plane_enable_fb_damage_clips - Enables plane fb damage clips property.
> - * @plane: Plane on which to enable damage clips property.
> - *
> - * This function lets driver to enable the damage clips property on a plane.
> - */
> -void drm_plane_enable_fb_damage_clips(struct drm_plane *plane)
> -{
> -	struct drm_device *dev = plane->dev;
> -	struct drm_mode_config *config = &dev->mode_config;
> -
> -	drm_object_attach_property(&plane->base, config->prop_fb_damage_clips,
> -				   0);
> -}
> -EXPORT_SYMBOL(drm_plane_enable_fb_damage_clips);
> -
>  /**
>   * drm_atomic_helper_check_plane_damage - Verify plane damage on atomic_check.
>   * @state: The driver state object.
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 40f099c67a8d..b68d06f536fa 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1397,6 +1397,60 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	return ret;
>  }
>  
> +/**
> + * DOC: damage tracking
> + *
> + * FB_DAMAGE_CLIPS is an optional plane property which provides a means to
> + * specify a list of damage rectangles on a plane in framebuffer coordinates of
> + * the framebuffer attached to the plane. In current context damage is the area
> + * of plane framebuffer that has changed since last plane update (also called
> + * page-flip), irrespective of whether currently attached framebuffer is same as
> + * framebuffer attached during last plane update or not.
> + *
> + * FB_DAMAGE_CLIPS is a hint to kernel which could be helpful for some drivers
> + * to optimize internally especially for virtual devices where each framebuffer
> + * change needs to be transmitted over network, usb, etc.
> + *
> + * Since FB_DAMAGE_CLIPS is a hint so it is an optional property. User-space can
> + * ignore damage clips property and in that case driver will do a full plane
> + * update. In case damage clips are provided then it is guaranteed that the area
> + * inside damage clips will be updated to plane. For efficiency driver can do
> + * full update or can update more than specified in damage clips. Since driver
> + * is free to read more, user-space must always render the entire visible
> + * framebuffer. Otherwise there can be corruptions. Also, if a user-space
> + * provides damage clips which doesn't encompass the actual damage to
> + * framebuffer (since last plane update) can result in incorrect rendering.
> + *
> + * FB_DAMAGE_CLIPS is a blob property with the layout of blob data is simply an
> + * array of &drm_mode_rect. Unlike plane &drm_plane_state.src coordinates,
> + * damage clips are not in 16.16 fixed point. Similar to plane src in
> + * framebuffer, damage clips cannot be negative. In damage clip, x1/y1 are
> + * inclusive and x2/y2 are exclusive. While kernel does not error for overlapped
> + * damage clips, it is strongly discouraged.
> + *
> + * Drivers that are interested in damage interface for plane should enable
> + * FB_DAMAGE_CLIPS property by calling drm_plane_enable_fb_damage_clips().
> + * Drivers implementing damage can use drm_atomic_helper_damage_iter_init() and
> + * drm_atomic_helper_damage_iter_next() helper iterator function to get damage
> + * rectangles clipped to &drm_plane_state.src.
> + */
> +
> +/**
> + * drm_plane_enable_fb_damage_clips - Enables plane fb damage clips property.
> + * @plane: Plane on which to enable damage clips property.
> + *
> + * This function lets driver to enable the damage clips property on a plane.
> + */
> +void drm_plane_enable_fb_damage_clips(struct drm_plane *plane)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	drm_object_attach_property(&plane->base, config->prop_fb_damage_clips,
> +				   0);
> +}
> +EXPORT_SYMBOL(drm_plane_enable_fb_damage_clips);
> +
>  /**
>   * drm_plane_get_damage_clips_count - Returns damage clips count.
>   * @state: Plane state.
> diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h
> index 1ae8bce6a5ce..effda42cce31 100644
> --- a/include/drm/drm_damage_helper.h
> +++ b/include/drm/drm_damage_helper.h
> @@ -64,7 +64,6 @@ struct drm_atomic_helper_damage_iter {
>  	bool full_update;
>  };
>  
> -void drm_plane_enable_fb_damage_clips(struct drm_plane *plane);
>  void drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state,
>  					  struct drm_plane_state *plane_state);
>  int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index a2684aab8372..fed97e35626f 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -897,9 +897,10 @@ 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);
> +
> +void drm_plane_enable_fb_damage_clips(struct drm_plane *plane);
>  unsigned int
>  drm_plane_get_damage_clips_count(const struct drm_plane_state *state);
> -
>  struct drm_mode_rect *
>  drm_plane_get_damage_clips(const struct drm_plane_state *state);
>  

Re: [PATCH 1/3] drm/plane: remove drm_helper_get_plane_damage_clips

[Souza, Jose]

On Wed, 2021-07-21 at 15:30 +0200, Daniel Vetter wrote:
> It's not used. Drivers should instead use the helpers anyway.
> 
> Currently both vbox and i915 hand-roll this and it's not the greatest.
> vbox looks buggy, and i915 does a bit much that helpers would take
> care of I think.
> 
> Also improve the kerneldocs while we're at it.

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> 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_damage_helper.c |  2 +-
>  include/drm/drm_damage_helper.h     | 17 -----------------
>  include/drm/drm_plane.h             | 10 +++++++---
>  include/drm/drm_rect.h              |  3 +++
>  4 files changed, 11 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
> index 3a4126dc2520..eb69b7123af5 100644
> --- a/drivers/gpu/drm/drm_damage_helper.c
> +++ b/drivers/gpu/drm/drm_damage_helper.c
> @@ -282,7 +282,7 @@ drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
>  	if (!state || !state->crtc || !state->fb || !state->visible)
>  		return;
>  
> -	iter->clips = drm_helper_get_plane_damage_clips(state);
> +	iter->clips = (struct drm_rect *)drm_plane_get_damage_clips(state);
>  	iter->num_clips = drm_plane_get_damage_clips_count(state);
>  
>  	/* Round down for x1/y1 and round up for x2/y2 to catch all pixels */
> diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h
> index 40c34a5bf149..1ae8bce6a5ce 100644
> --- a/include/drm/drm_damage_helper.h
> +++ b/include/drm/drm_damage_helper.h
> @@ -82,21 +82,4 @@ bool drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state,
>  				     struct drm_plane_state *state,
>  				     struct drm_rect *rect);
>  
> -/**
> - * drm_helper_get_plane_damage_clips - Returns damage clips in &drm_rect.
> - * @state: Plane state.
> - *
> - * Returns plane damage rectangles in internal &drm_rect. Currently &drm_rect
> - * can be obtained by simply typecasting &drm_mode_rect. This is because both
> - * are signed 32 and during drm_atomic_check_only() it is verified that damage
> - * clips are inside fb.
> - *
> - * Return: Clips in plane fb_damage_clips blob property.
> - */
> -static inline struct drm_rect *
> -drm_helper_get_plane_damage_clips(const struct drm_plane_state *state)
> -{
> -	return (struct drm_rect *)drm_plane_get_damage_clips(state);
> -}
> -
>  #endif
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 1294610e84f4..7f7d5148310c 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -186,6 +186,9 @@ struct drm_plane_state {
>  	 * since last plane update) as an array of &drm_mode_rect in framebuffer
>  	 * coodinates of the attached framebuffer. Note that unlike plane src,
>  	 * damage clips are not in 16.16 fixed point.
> +	 *
> +	 * See drm_plane_get_damage_clips() and
> +	 * drm_plane_get_damage_clips_count() for accessing these.
>  	 */
>  	struct drm_property_blob *fb_damage_clips;
>  
> @@ -914,9 +917,10 @@ 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
> - * instead be interested in internal &drm_rect which can be obtained by calling
> - * drm_helper_get_plane_damage_clips().
> + * 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.
>   */
> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
> index 39f2deee709c..6f6e19bd4dac 100644
> --- a/include/drm/drm_rect.h
> +++ b/include/drm/drm_rect.h
> @@ -39,6 +39,9 @@
>   * @x2: horizontal ending coordinate (exclusive)
>   * @y1: vertical starting coordinate (inclusive)
>   * @y2: vertical ending coordinate (exclusive)
> + *
> + * Note that this must match the layout of struct drm_mode_rect or the damage
> + * helpers like drm_atomic_helper_damage_iter_init() break.
>   */
>  struct drm_rect {
>  	int x1, y1, x2, y2;

Re: [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@intel.com> (v1)
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> 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.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

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

[Daniel Vetter]

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)

Reviewed-by: José Roberto de Souza <jose.souza@intel.com> (v1)
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
Cc: José Roberto de Souza <jose.souza@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
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.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