Message-ID: <20180405073525.GP3881@phenom.ffwll.local>
Date:   Thu, 5 Apr 2018 09:35:25 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Deepak Rawat <drawat@...are.com>
Cc:     dri-devel@...ts.freedesktop.org, thellstrom@...are.com,
        syeh@...are.com, linux-graphics-maintainer@...are.com,
        daniel@...ll.ch, ville.syrjala@...ux.intel.com,
        lukasz.spintzyk@...playlink.com, noralf@...nnes.org,
        robdclark@...il.com, gustavo@...ovan.org,
        maarten.lankhorst@...ux.intel.com, seanpaul@...omium.org,
        airlied@...ux.ie, linux-kernel@...r.kernel.org
Subject: Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:
> From: Lukasz Spintzyk <lukasz.spintzyk@...playlink.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@...playlink.com>
> Signed-off-by: Deepak Rawat <drawat@...are.com>

The property uapi section is missing, see:

https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-properties

Plane composition feels like the best place to put this. Please use that
section to tie all the various bits together, including the helpers you're
adding in the following patches for drivers to use.

Bunch of nitpicks below, but overall I'm agreeing now with just going with
fb coordinate damage rects.

Like you say, the thing needed here now is userspace + driver actually
implementing this. I'd also say the compat helper to map the legacy
fb->dirty to this new atomic way of doing things should be included here
(gives us a lot more testing for these new paths).

Icing on the cake would be an igt to make sure kernel rejects invalid clip
rects correctly.

> ---
>  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);

There's already a helper with size-checking built-in, see
drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips I'd
just provide a little inline helper that does the
blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast).

> +		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);

Bit a bikeshed, but since the coordinates are in fb pixels, not plane
pixels, I'd call this "FB_DAMAGE_CLIPS".

> +	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.

Why should we make this optional? Looks like just another thing drivers
might screw up, since we have multiple callbacks and things to set up for
proper dirty tracking.

One option I'm seeing is that if this is set, and it's an atomic driver,
then we just directly call into the default atomic fb->dirty
implementation. That way there's only 1 thing drivers need to do to set up
dirty rect tracking, and they'll get all of it.

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

I honestly have no idea where this limit is from. Worth keeping? I can
easily imagine that userspace could trip over this - it's fairly high, but
not unlimited.

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

I'd also clarify that userspace still must render the entire screen, i.e.
make it more clear that it's really just a hint and not mandatory to only
scan out the damaged parts.

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

&drm_rect gives you a nice hyperlink in the generated docs.

> +	 * 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;

Why signed? Negative damage rects on an fb don't make sense to me. Also,
please specify what this is exactly (to avoid confusion with the 16.16
fixed point src rects), and maybe mention in the commit message why we're
not using drm_clip_rect (going to proper uapi types and 32bit makes sense
to me).

Cheers, Daniel
> +};
> +
>  #if defined(__cplusplus)
>  }
>  #endif
> -- 
> 2.7.4
> 

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

Powered by blists - more mailing lists