Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Lukasz Spintzyk <lukasz.spintzyk@displaylink.com>]

On 05/04/2018 01:49, Deepak Rawat wrote:
> From: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com>
>
> Optional plane property to mark damaged regions on the plane in
> framebuffer coordinates of the framebuffer attached to the plane.
>
> The layout of blob data is simply an array of drm_mode_rect with maximum
> array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
> coordinates, damage clips are not in 16.16 fixed point.
>
> Damage clips are a hint to kernel as which area of framebuffer has
> changed since last page-flip. This should be helpful for some drivers
> especially for virtual devices where each framebuffer change needs to
> be transmitted over network, usb, etc.
>
> Driver which are interested in enabling DAMAGE_CLIPS property for a
> plane should enable this property using drm_plane_enable_damage_clips.
>
> Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com>
> Signed-off-by: Deepak Rawat <drawat@vmware.com>
> ---
>   drivers/gpu/drm/drm_atomic.c        | 42 +++++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/drm_atomic_helper.c |  4 ++++
>   drivers/gpu/drm/drm_mode_config.c   |  5 +++++
>   drivers/gpu/drm/drm_plane.c         | 12 +++++++++++
>   include/drm/drm_mode_config.h       | 15 +++++++++++++
>   include/drm/drm_plane.h             | 16 ++++++++++++++
>   include/uapi/drm/drm_mode.h         | 15 +++++++++++++
>   7 files changed, 109 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7d25c42..9226d24 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p,
>   }
>   
>   /**
> + * drm_atomic_set_damage_for_plane - sets the damage clips property to plane
> + * @state: plane state
> + * @blob: damage clips in framebuffer coordinates
> + *
> + * Returns:
> + *
> + * Zero on success, error code on failure.
> + */
> +static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state,
> +					   struct drm_property_blob *blob)
> +{
> +	if (blob == state->damage_clips)
> +		return 0;
> +
> +	drm_property_blob_put(state->damage_clips);
> +	state->damage_clips = NULL;
> +
> +	if (blob) {
> +		uint32_t count = blob->length/sizeof(struct drm_rect);
> +
> +		if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
> +			return -EINVAL;
> +
> +		state->damage_clips = drm_property_blob_get(blob);
> +		state->num_clips = count;
> +	} else {
> +		state->damage_clips = NULL;
> +		state->num_clips = 0;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
>    * drm_atomic_get_plane_state - get plane state
>    * @state: global atomic state object
>    * @plane: plane to get state object for
> @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>   		state->color_encoding = val;
>   	} else if (property == plane->color_range_property) {
>   		state->color_range = val;
> +	} else if (property == config->prop_damage_clips) {
> +		struct drm_property_blob *blob =
> +			drm_property_lookup_blob(dev, val);
> +		int ret = drm_atomic_set_damage_for_plane(state, blob);
> +		drm_property_blob_put(blob);
> +		return ret;
>   	} else if (plane->funcs->atomic_set_property) {
>   		return plane->funcs->atomic_set_property(plane, state,
>   				property, val);
> @@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>   		*val = state->color_encoding;
>   	} else if (property == plane->color_range_property) {
>   		*val = state->color_range;
> +	} else if (property == config->prop_damage_clips) {
> +		*val = (state->damage_clips) ? state->damage_clips->base.id : 0;
>   	} else if (plane->funcs->atomic_get_property) {
>   		return plane->funcs->atomic_get_property(plane, state, property, val);
>   	} else {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index c356545..55b44e3 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3506,6 +3506,8 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>   
>   	state->fence = NULL;
>   	state->commit = NULL;
> +	state->damage_clips = NULL;
> +	state->num_clips = 0;
>   }
>   EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
>   
> @@ -3550,6 +3552,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>   
>   	if (state->commit)
>   		drm_crtc_commit_put(state->commit);
> +
> +	drm_property_blob_put(state->damage_clips);
>   }
>   EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>   
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index e5c6533..e93b127 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -293,6 +293,11 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>   		return -ENOMEM;
>   	dev->mode_config.prop_crtc_id = prop;
>   
> +	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, "DAMAGE_CLIPS", 0);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_damage_clips = prop;
> +
>   	prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
>   			"ACTIVE");
>   	if (!prop)
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 6d2a6e4..071221b 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1101,3 +1101,15 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>   
>   	return ret;
>   }
> +
> +/**
> + * drm_plane_enable_damage_clips - enable damage clips property
> + * @plane: plane on which this property to enable.
> + */
> +void drm_plane_enable_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_damage_clips, 0);
> +}
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 7569f22..d8767da 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -628,6 +628,21 @@ struct drm_mode_config {
>   	 */
>   	struct drm_property *prop_crtc_id;
>   	/**
> +	 * @prop_damage_clips: Optional plane property to mark damaged regions
> +	 * on the plane in framebuffer coordinates of the framebuffer attached
> +	 * to the plane.
> +	 *
> +	 * The layout of blob data is simply an array of drm_mode_rect with
> +	 * maximum array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike
> +	 * plane src coordinates, damage clips are not in 16.16 fixed point.
> +	 *
> +	 * Damage clips are a hint to kernel as which area of framebuffer has
> +	 * changed since last page-flip. This should be helpful
> +	 * for some drivers especially for virtual devices where each
> +	 * framebuffer change needs to be transmitted over network, usb, etc.
> +	 */
> +	struct drm_property *prop_damage_clips;
> +	/**
>   	 * @prop_active: Default atomic CRTC property to control the active
>   	 * state, which is the simplified implementation for DPMS in atomic
>   	 * drivers.
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index f7bf4a4..9f24548 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -146,6 +146,21 @@ struct drm_plane_state {
>   	 */
>   	struct drm_crtc_commit *commit;
>   
> +	/*
> +	 * @damage_clips
> +	 *
> +	 * blob property with damage as array of drm_rect in framebuffer
> +	 * coodinates.
> +	 */
> +	struct drm_property_blob *damage_clips;
> +
> +	/*
> +	 * @num_clips
> +	 *
> +	 * Number of drm_rect in @damage_clips.
> +	 */
> +	uint32_t num_clips;
> +
>   	struct drm_atomic_state *state;
>   };
>   
> @@ -611,6 +626,7 @@ int drm_plane_init(struct drm_device *dev,
>   		   const uint32_t *formats, unsigned int format_count,
>   		   bool is_primary);
>   void drm_plane_cleanup(struct drm_plane *plane);
> +void drm_plane_enable_damage_clips(struct drm_plane *plane);
>   
>   /**
>    * drm_plane_index - find the index of a registered plane
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 50bcf42..0ad0d5b 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -873,6 +873,21 @@ struct drm_mode_revoke_lease {
>   	__u32 lessee_id;
>   };
>   
> +/**
> + * struct drm_mode_rect - two dimensional rectangle drm_rect exported to
> + * user-space.
> + * @x1: horizontal starting coordinate (inclusive)
> + * @y1: vertical starting coordinate (inclusive)
> + * @x2: horizontal ending coordinate (exclusive)
> + * @y2: vertical ending coordinate (exclusive)
> + */
> +struct drm_mode_rect {
> +	__s32 x1;
> +	__s32 y1;
> +	__s32 x2;
> +	__s32 y2;
> +};
I wonder why we can't use move 'struct drm_rect'  definition from 
'include/drm/drm_rect.h'
and include 'uapi/drm/drm_mode.h' in private header 
'include/drm/drm_rect.h'.
Is there any general rule that disallows it?
> +
>   #if defined(__cplusplus)
>   }
>   #endif