| Message ID | 20180905233901.2321-2-drawat@vmware.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | plane update with damage | expand |
On Wed, Sep 05, 2018 at 04:38:48PM -0700, Deepak Rawat wrote: > From: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> > > 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@displaylink.com> > Signed-off-by: Deepak Rawat <drawat@vmware.com> > --- > 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@vmware.com> > + * > + **************************************************************************/ > + > +#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@vmware.com> > + * > + **************************************************************************/ > + > +#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
On Wed, Sep 05, 2018 at 04:38:48PM -0700, Deepak Rawat wrote: > From: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> > > 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@displaylink.com> > Signed-off-by: Deepak Rawat <drawat@vmware.com> > --- > 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: > + > 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; > } > } > 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@vmware.com> > + * > + **************************************************************************/ > + > +#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 > + * rendering during plane update. > + * > + * While kernel does not error for overlapped damage clips, it is discouraged. > + */ > + > +/** > + * 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@vmware.com> > + * > + **************************************************************************/ > + > +#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) Wrong type, this needs to be the uapi drm_mode_rect, not the internal rect. The damge_iter needs to then convert this. Noticed while reviewing patch 4, which also uses the wrong type. -Daniel > +{ > + 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 > -- > 2.17.1 >
On Wed, Sep 05, 2018 at 04:38:48PM -0700, Deepak Rawat wrote: > From: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> > > 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@displaylink.com> > Signed-off-by: Deepak Rawat <drawat@vmware.com> > --- > 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: > + > 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; > } > } > 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@vmware.com> > + * > + **************************************************************************/ > + > +#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) Not sure why the plane src coordinates need to be mentioned here. The damage is just the part of the fb that has changed. Whether or not it extends past the src coordinates is totally irrelevant as far as I can see. > 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 > + * rendering during plane update. > + * > + * While kernel does not error for overlapped damage clips, it is discouraged. > + */ > + > +/** > + * 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@vmware.com> > + * > + **************************************************************************/ > + > +#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; Don't we have a better place for this? Next to some other props etc. in the plane state? What does "in framebuffer coordinates" mean? Integers, some fixed point representation? > + > /** @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 > -- > 2.17.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > +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. Thanks for the review Daniel, oops I added those docs later in header and forgot to update here. > > > + > > 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@vmware.com> > > + * > > + > ********************************************************** > ****************/ > > + > > +#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. Agreed , will make it more clear for V2 > > - 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? My motivation behind not informing user-space if damage is outside plane src, sent during modeset(where it should be provided) or damage is outside framebuffer coordinate (simply put damage clips are invalid) is that it's a hint. The contract between kernel and user-space is simple, if damage clips are valid, kernel might use them (but not always) otherwise they will be ignored. Damage can be ignored deep in the stack (like network), so all the input validation for nothing. Also, what all to consider in input validation ? * clips are outside plane src * damage clips sent during modeset should error ? * any other properties. This will lead to a complex input validation during modeset check like if some drivers are OK with damage while scaling whereas others cannot support, should this error? However I am alright doing input validation if this becomes clear what kind of validation to actually do. My understanding of drm core is limited. > > - A short paragraph on how drivers are supposed to implement this would be > good, with references to functions (will be automatically converted to > hyperlinks) Agreed > > > + */ > > + > > +/** > > + * 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@vmware.com> > > + * > > + > ********************************************************** > ****************/ > > + > > +#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 > https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ff > wll.ch&data=02%7C01%7Cdrawat%40vmware.com%7C0f5c199805444f2 > 6d01508d613cc5abe%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C > 636718165752004800&sdata=zKJnKe90%2BMeBsSpb4t4HfkOlLYkgXkyxsc > GwVOHCyro%3D&reserved=0
> > On Wed, Sep 05, 2018 at 04:38:48PM -0700, Deepak Rawat wrote: > > From: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> > > > > 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@displaylink.com> > > Signed-off-by: Deepak Rawat <drawat@vmware.com> > > --- > > 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: > > + > > 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; > > } > > } > > 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@vmware.com> > > + * > > + > ********************************************************** > ****************/ > > + > > +#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) > > Not sure why the plane src coordinates need to be mentioned here. The > damage is just the part of the fb that has changed. Whether or not it > extends past the src coordinates is totally irrelevant as far as I can > see. Thanks Ville for the review, Well this is plane damage property and only those clips which falls inside will be used for plane update, so outside plane src is not required. > > > 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 > > + * rendering during plane update. > > + * > > + * While kernel does not error for overlapped damage clips, it is > discouraged. > > + */ > > + > > +/** > > + * 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@vmware.com> > > + * > > + > ********************************************************** > ****************/ > > + > > +#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; > > Don't we have a better place for this? Next to some other props etc. in > the plane state? This is in plane state, sorry am I missing something ? > > What does "in framebuffer coordinates" mean? Integers, some fixed > point representation? I guess better wording would be "in framebuffer coordinates of the frambebuffer attached to plane". I would make it more clear for v2, > > > + > > /** @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 > > -- > > 2.17.1 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.fr > eedesktop.org%2Fmailman%2Flistinfo%2Fdri- > devel&data=02%7C01%7Cdrawat%40vmware.com%7C5c06b27d7534412 > 8cb0f08d613f0534a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C6 > 36718320248896001&sdata=2a2pS5CILMUyXDJgjIms59UUOso3LJRrpFW > Rypik40Y%3D&reserved=0 > > -- > Ville Syrjälä > Intel
On Thu, 6 Sep 2018 21:36:51 +0000 Deepak Singh Rawat <drawat@vmware.com> wrote: > > > > - 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? > > My motivation behind not informing user-space if damage is outside plane > src, sent during modeset(where it should be provided) or damage is outside > framebuffer coordinate (simply put damage clips are invalid) is that it's a > hint. The contract between kernel and user-space is simple, if damage > clips are valid, kernel might use them (but not always) otherwise they > will be ignored. Damage can be ignored deep in the stack (like network), > so all the input validation for nothing. > > Also, what all to consider in input validation ? > * clips are outside plane src > * damage clips sent during modeset should error ? > * any other properties. > > This will lead to a complex input validation during modeset check like > if some drivers are OK with damage while scaling whereas others > cannot support, should this error? > > However I am alright doing input validation if this becomes clear what > kind of validation to actually do. My understanding of drm core is limited. Hi, I think validating that the area of any clip rectangle does not extend beyond the framebuffer size would be good. A userspace violating that is arguably broken, so it would help catch userspace bugs. I also would not assume that damage is irrelevant on modeset. Userspace does not always know if an update will be a modeset or not: drivers vary there, and userspace that just wants the update through will allow modesets always while still providing damage. Userspace could also change video mode timings or pixel format while keeping the resolution the same, and in that case damage could be meaningful to a driver. Thanks, pq
On Thu, Sep 06, 2018 at 10:32:58PM +0000, Deepak Singh Rawat wrote: > > > > On Wed, Sep 05, 2018 at 04:38:48PM -0700, Deepak Rawat wrote: > > > From: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> > > > > > > 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@displaylink.com> > > > Signed-off-by: Deepak Rawat <drawat@vmware.com> > > > --- > > > 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: > > > + > > > 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; > > > } > > > } > > > 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@vmware.com> > > > + * > > > + > > ********************************************************** > > ****************/ > > > + > > > +#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) > > > > Not sure why the plane src coordinates need to be mentioned here. The > > damage is just the part of the fb that has changed. Whether or not it > > extends past the src coordinates is totally irrelevant as far as I can > > see. > > Thanks Ville for the review, > > Well this is plane damage property and only those clips which falls inside > will be used for plane update, so outside plane src is not required. Actually we've never specified that properly. My usual interpretation is that the plane is allowed to sample past the edge of the src rectangle (to get nicer looking edges). But not sure whether that would be too surprising to people though. So we might want follow the GL linear filter rules by default, and if someone wants better looking edges we could add a property that allows the filter to reach further past the edge. > > > > > > 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 > > > + * rendering during plane update. > > > + * > > > + * While kernel does not error for overlapped damage clips, it is > > discouraged. > > > + */ > > > + > > > +/** > > > + * 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@vmware.com> > > > + * > > > + > > ********************************************************** > > ****************/ > > > + > > > +#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; > > > > Don't we have a better place for this? Next to some other props etc. in > > the plane state? > > This is in plane state, sorry am I missing something ? Just the placement inside the plane state looks rather random. More logical to group it next to some other property stuff I think. > > > > > What does "in framebuffer coordinates" mean? Integers, some fixed > > point representation? > > I guess better wording would be "in framebuffer coordinates of the > frambebuffer attached to plane". I would make it more clear for v2, I'm basically asking whether one is supposed to do '.x1 = x' or '.x1 = x<<16' or something else, where x is an interger pixel coordinate. > > > > > > + > > > /** @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 > > > -- > > > 2.17.1 > > > > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > > > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.fr > > eedesktop.org%2Fmailman%2Flistinfo%2Fdri- > > devel&data=02%7C01%7Cdrawat%40vmware.com%7C5c06b27d7534412 > > 8cb0f08d613f0534a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C6 > > 36718320248896001&sdata=2a2pS5CILMUyXDJgjIms59UUOso3LJRrpFW > > Rypik40Y%3D&reserved=0 > > > > -- > > Ville Syrjälä > > Intel
> > On Thu, 6 Sep 2018 21:36:51 +0000 > Deepak Singh Rawat <drawat@vmware.com> wrote: > > > > > > > - 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? > > > > My motivation behind not informing user-space if damage is outside plane > > src, sent during modeset(where it should be provided) or damage is outside > > framebuffer coordinate (simply put damage clips are invalid) is that it's a > > hint. The contract between kernel and user-space is simple, if damage > > clips are valid, kernel might use them (but not always) otherwise they > > will be ignored. Damage can be ignored deep in the stack (like network), > > so all the input validation for nothing. > > > > Also, what all to consider in input validation ? > > * clips are outside plane src > > * damage clips sent during modeset should error ? > > * any other properties. > > > > This will lead to a complex input validation during modeset check like > > if some drivers are OK with damage while scaling whereas others > > cannot support, should this error? > > > > However I am alright doing input validation if this becomes clear what > > kind of validation to actually do. My understanding of drm core is limited. > > Hi, > > I think validating that the area of any clip rectangle does not extend > beyond the framebuffer size would be good. A userspace violating that > is arguably broken, so it would help catch userspace bugs. > > I also would not assume that damage is irrelevant on modeset. Userspace > does not always know if an update will be a modeset or not: drivers vary > there, and userspace that just wants the update through will allow > modesets always while still providing damage. Userspace could also > change video mode timings or pixel format while keeping the resolution > the same, and in that case damage could be meaningful to a driver. > Thanks Pekka, for explanation. I agree clips outside framebuffer should be errored and also it's not a lot for drm core. Also, will look into the cases where drm core should clear the damage clips, like resolution change, etc. May be the better approach would be drm core just provide the helper and driver takes care of driver specific scenarios.
> > > > +#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) > > > > > > Not sure why the plane src coordinates need to be mentioned here. The > > > damage is just the part of the fb that has changed. Whether or not it > > > extends past the src coordinates is totally irrelevant as far as I can > > > see. > > > > Thanks Ville for the review, > > > > Well this is plane damage property and only those clips which falls inside > > will be used for plane update, so outside plane src is not required. > > Actually we've never specified that properly. My usual interpretation is > that the plane is allowed to sample past the edge of the src rectangle > (to get nicer looking edges). But not sure whether that would be too > surprising to people though. So we might want follow the GL linear filter > rules by default, and if someone wants better looking edges we could > add a property that allows the filter to reach further past the edge. Ok, it makes sense not to specify plane src part, anyway with current implementation kernel do not error if damage clips are outside plane src. The helper iterator clip to plane_src but depending on the above use case can have other implementations as well. Thanks, Deepak
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: + 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; } } 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@vmware.com> + * + **************************************************************************/ + +#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 + * rendering during plane update. + * + * While kernel does not error for overlapped damage clips, it is discouraged. + */ + +/** + * 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@vmware.com> + * + **************************************************************************/ + +#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