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
