Re: [PATCH 01/14] drm: add new plane property FB_DAMAGE_CLIPS to send damage during plane update

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Sep 05, 2018 at 04:38:48PM -0700, Deepak Rawat wrote:
> From: Lukasz Spintzyk <lukasz.spintzyk@xxxxxxxxxxxxxxx>
> 
> FB_DAMAGE_CLIPS is an 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 "struct 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. As
> plane src in framebuffer cannot be negative so are damage clips. In
> damage clip, x1/y1 are inclusive and x2/y2 are exclusive.
> 
> This patch also exports the kernel internal drm_rect to userspace as
> drm_mode_rect. This is because "struct drm_clip_rect" is not sufficient
> to represent damage for current plane size.
> 
> Upper limit is set on the maximum number of damage clips to avoid
> overflow by user-space.
> 
> Driver which are interested in enabling FB_DAMAGE_CLIPS property for a
> plane should enable this property using drm_plane_enable_damage_clips.
> 
> Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@xxxxxxxxxxxxxxx>
> Signed-off-by: Deepak Rawat <drawat@xxxxxxxxxx>
> ---
>  Documentation/gpu/drm-kms.rst       |  9 +++
>  drivers/gpu/drm/Makefile            |  2 +-
>  drivers/gpu/drm/drm_atomic.c        | 13 ++++
>  drivers/gpu/drm/drm_atomic_helper.c |  4 ++
>  drivers/gpu/drm/drm_damage_helper.c | 92 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_mode_config.c   |  6 ++
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c |  1 +
>  include/drm/drm_damage_helper.h     | 63 ++++++++++++++++++++
>  include/drm/drm_mode_config.h       | 10 ++++
>  include/drm/drm_plane.h             |  8 +++
>  include/uapi/drm/drm_mode.h         | 19 ++++++
>  11 files changed, 226 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/drm_damage_helper.c
>  create mode 100644 include/drm/drm_damage_helper.h
> 
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 5dee6b8a4c12..78b66e628857 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -542,6 +542,15 @@ Plane Composition Properties
>  .. kernel-doc:: drivers/gpu/drm/drm_blend.c
>     :export:
>  
> +FB_DAMAGE_CLIPS
> +~~~~~~~~~~~~~~~
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_damage_helper.c
> +   :doc: overview
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_damage_helper.c
> +   :export:

You forgot to include your nice kerneldoc from the header. Please run

$ make htmldocs

and make sure the generated stuff looks all nice and is complete.

> +
>  Color Management Properties
>  ---------------------------
>  
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index a6771cef85e2..ca5be0decb3f 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -35,7 +35,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
>  		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
>  		drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
>  		drm_simple_kms_helper.o drm_modeset_helper.o \
> -		drm_scdc_helper.o drm_gem_framebuffer_helper.o
> +		drm_scdc_helper.o drm_gem_framebuffer_helper.o drm_damage_helper.o
>  
>  drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o
>  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 3eb061e11e2e..652e78ca1f81 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -857,6 +857,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>  {
>  	struct drm_device *dev = plane->dev;
>  	struct drm_mode_config *config = &dev->mode_config;
> +	bool replaced = false;
> +	int ret;
>  
>  	if (property == config->prop_fb_id) {
>  		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, NULL, val);
> @@ -908,6 +910,14 @@ 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_fb_damage_clips) {
> +		ret = drm_atomic_replace_property_blob_from_id(dev,
> +					&state->fb_damage_clips,
> +					val,
> +					-1,
> +					sizeof(struct drm_rect),
> +					&replaced);
> +		return ret;
>  	} else if (plane->funcs->atomic_set_property) {
>  		return plane->funcs->atomic_set_property(plane, state,
>  				property, val);
> @@ -976,6 +986,9 @@ 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_fb_damage_clips) {
> +		*val = (state->fb_damage_clips) ?
> +			state->fb_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 80be74df7ba6..be83e2763c18 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3576,6 +3576,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane *plane)
>  		/* Reset the alpha value to fully opaque if it matters */
>  		if (plane->alpha_property)
>  			plane->state->alpha = plane->alpha_property->values[1];
> +		plane->state->fb_damage_clips = NULL;

No need to set to 0, we require kzalloc.

>  	}
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
> @@ -3598,6 +3599,7 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>  
>  	state->fence = NULL;
>  	state->commit = NULL;
> +	state->fb_damage_clips = NULL;
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
>  
> @@ -3642,6 +3644,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->fb_damage_clips);
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>  
> diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
> new file mode 100644
> index 000000000000..3e7de5afe7f6
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_damage_helper.c
> @@ -0,0 +1,92 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/**************************************************************************
> + *
> + * Copyright (c) 2018 VMware, Inc., Palo Alto, CA., USA
> + * All Rights Reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction, including
> + * without limitation the rights to use, copy, modify, merge, publish,
> + * distribute, sub license, and/or sell copies of the Software, and to
> + * permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial portions
> + * of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
> + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
> + * USE OR OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors:
> + * Deepak Rawat <drawat@xxxxxxxxxx>
> + *
> + **************************************************************************/
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_damage_helper.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 (excluding the framebuffer area which is outside of
> + * plane src) that has changed since last plane update (also called page-flip).
> + * Only the area inside damage rectangles will be considered different 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 the damage clips property and in that case kernel 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 kernel can do
> + * full update or more area than specified in damage clips.
> + *
> + * FB_DAMAGE_CLIPS is a blob property with the layout of blob data is simply an
> + * array of drm_mode_rect(internally drm_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. Similar to plane src in framebuffer, damage
> + * clips cannot be negative. In damage clip, x1/y1 are inclusive and x2/y2 are
> + * exclusive.
> + *
> + * All the damage clips are in framebuffer coordinates and should be inside
> + * plane src and if any clip falls outside plane src it will be ignored and
> + * user-space won't be notified of the same. As mentioned above, sometimes
> + * kernel will do full plane update due to change in properties which can affect
> + * full plane e.g. color management properties. Also during full modeset damage
> + * is irrelevant so if provided by user-space it simply will be ignored.
> + * Whenever damage clips are ignored by kernel, user-space will not be informed.
> + * If a user-space provides damage clips which doesn't encompass the actual
> + * damage to framebuffer (since last plane update) will result in incorrect

s/will/can/ - it's an optimization

> + * rendering during plane update.
> + *
> + * While kernel does not error for overlapped damage clips, it is discouraged.

Some comments on the doc:

- You kinda explain it, but in convoluted way: I'd add a clear sentence
  that userspace _must_ always render the entire visible fb, since the
  driver is free to read more. Otherwise there can be corruptions.

- Why no input validation on the damage clips against the framebuffer
  size? Ime not validating just leads to funny driver bugs down the road,
  so what's the use-case you have in mind here?

- A short paragraph on how drivers are supposed to implement this would be
  good, with references to functions (will be automatically converted to
  hyperlinks)

> + */
> +
> +/**
> + * 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);
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 21e353bd3948..506f5a52733f 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -298,6 +298,12 @@ 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, "FB_DAMAGE_CLIPS",
> +				   0);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_fb_damage_clips = prop;
> +
>  	prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
>  			"ACTIVE");
>  	if (!prop)
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index 23beff5d8e3c..1edbae73d6d6 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -723,6 +723,7 @@ void vmw_du_plane_reset(struct drm_plane *plane)
>  	plane->state = &vps->base;
>  	plane->state->plane = plane;
>  	plane->state->rotation = DRM_MODE_ROTATE_0;
> +	plane->state->fb_damage_clips = NULL;
>  }
>  
>  
> diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h
> new file mode 100644
> index 000000000000..217694e9168c
> --- /dev/null
> +++ b/include/drm/drm_damage_helper.h
> @@ -0,0 +1,63 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> +/**************************************************************************
> + *
> + * Copyright (c) 2018 VMware, Inc., Palo Alto, CA., USA
> + * All Rights Reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction, including
> + * without limitation the rights to use, copy, modify, merge, publish,
> + * distribute, sub license, and/or sell copies of the Software, and to
> + * permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial portions
> + * of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
> + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
> + * USE OR OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors:
> + * Deepak Rawat <drawat@xxxxxxxxxx>
> + *
> + **************************************************************************/
> +
> +#ifndef DRM_DAMAGE_HELPER_H_
> +#define DRM_DAMAGE_HELPER_H_
> +
> +/**
> + * drm_plane_get_damage_clips_count - returns damage clips count
> + * @state: Plane state
> + *
> + * Returns: Number of clips in plane fb_damage_clips blob property.
> + */
> +static inline uint32_t
> +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_rect) : 0;
> +}
> +
> +/**
> + * drm_plane_get_damage_clips - returns damage clips
> + * @state: Plane state
> + *
> + * Returns: Clips in plane fb_damage_clips blob property.
> + */
> +static inline struct drm_rect *
> +drm_plane_get_damage_clips(const struct drm_plane_state *state)
> +{
> +	return (struct drm_rect *)((state && state->fb_damage_clips) ?
> +				   state->fb_damage_clips->data : NULL);
> +}
> +
> +void drm_plane_enable_fb_damage_clips(struct drm_plane *plane);
> +
> +#endif
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index a0b202e1d69a..8ccb5ddcd99d 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -627,6 +627,16 @@ struct drm_mode_config {
>  	 * &drm_crtc.
>  	 */
>  	struct drm_property *prop_crtc_id;
> +	/**
> +	 * @prop_fb_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.
> +	 */
> +	struct drm_property *prop_fb_damage_clips;
>  	/**
>  	 * @prop_active: Default atomic CRTC property to control the active
>  	 * state, which is the simplified implementation for DPMS in atomic
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 8a152dc16ea5..bb8b0934119c 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -182,6 +182,14 @@ struct drm_plane_state {
>  	 */
>  	struct drm_crtc_commit *commit;
>  
> +	/**
> +	 * @fb_damage_clips:
> +	 *
> +	 * Blob representing damage (area in plane framebuffer that changed
> +	 * since last page flip) as array of drm_rect in framebuffer coodinates.
> +	 */
> +	struct drm_property_blob *fb_damage_clips;
> +
>  	/** @state: backpointer to global drm_atomic_state */
>  	struct drm_atomic_state *state;
>  };
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 8d67243952f4..862ea0893e2e 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -887,6 +887,25 @@ struct drm_mode_revoke_lease {
>  	__u32 lessee_id;
>  };
>  
> +/**
> + * struct drm_mode_rect - two dimensional rectangle
> + * @x1: horizontal starting coordinate (inclusive)
> + * @y1: vertical starting coordinate (inclusive)
> + * @x2: horizontal ending coordinate (exclusive)
> + * @y2: vertical ending coordinate (exclusive)
> + *
> + * With drm subsystem using struct drm_rect to manage rectangular area this
> + * export it to user-space.
> + *
> + * Currently used by drm_mode_atomic blob property FB_DAMAGE_CLIPS.
> + */
> +struct drm_mode_rect {
> +	__s32 x1;
> +	__s32 y1;
> +	__s32 x2;
> +	__s32 y2;
> +};
> +
>  #if defined(__cplusplus)
>  }
>  #endif

Besides the small nits this looks good to me.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux