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.

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&amp;data=02%7C01%7Cdrawat%40vmware.com%7C5c06b27d7534412 8cb0f08d613f0534a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C6 36718320248896001&amp;sdata=2a2pS5CILMUyXDJgjIms59UUOso3LJRrpFW Rypik40Y%3D&amp;reserved=0

-- Ville Syrjälä Intel

On Wed, Sep 05, 2018 at 04:38:49PM -0700, Deepak Rawat wrote:

With fb_damage_clips blob property in drm_plane_state, this patch adds helper iterator to traverse the damage clips that lie inside plane src. Iterator will return full plane src as damage in case need full plane update or damage is not specified.

Signed-off-by: Deepak Rawat drawat@vmware.com

drivers/gpu/drm/drm_damage_helper.c | 93 +++++++++++++++++++++++++++++ include/drm/drm_damage_helper.h | 20 +++++++ 2 files changed, 113 insertions(+)

diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c index 3e7de5afe7f6..7d70f6001889 100644 --- a/drivers/gpu/drm/drm_damage_helper.c +++ b/drivers/gpu/drm/drm_damage_helper.c @@ -30,6 +30,7 @@ **************************************************************************/

#include <drm/drmP.h> +#include <drm/drm_atomic.h> #include <drm/drm_damage_helper.h>

/** @@ -75,6 +76,11 @@

• While kernel does not error for overlapped damage clips, it is discouraged.

*/

+static int convert_fixed_to_32(int fixed) +{

• return ((fixed >> 15) & 1) + (fixed >> 16);

+}

/**

• drm_plane_enable_fb_damage_clips - enables plane fb damage clips property
• @plane: plane on which to enable damage clips property

@@ -90,3 +96,90 @@ void drm_plane_enable_fb_damage_clips(struct drm_plane *plane) 0); } EXPORT_SYMBOL(drm_plane_enable_fb_damage_clips);

+/**

• • drm_atomic_helper_damage_iter_init - initialize the damage iterator
• • @iter: The iterator to initialize.
• • @old_state: old plane state for validation.
• • @new_state: plane state from which to iterate the damage clips.
• • Initialize an iterator that clip framebuffer damage in plane fb_damage_clips
• • blob to plane src clip. The iterator returns full plane src in case needing
• • full update e.g. during full modeset.
• • With this helper iterator, drivers which enabled fb_damage_clips property can
• • iterate over the damage clips that falls inside plane src during plane
• • update.
• • Returns: 0 on success and negative error code on error. If an error code is
• • returned then it means the plane state shouldn't update with attached fb.
• */

+int +drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,

• 		   const struct drm_plane_state *old_state,
  

• 		   const struct drm_plane_state *new_state)
  

+{

• if (!new_state || !new_state->crtc || !new_state->fb)

• return -EINVAL;
  

• if (!new_state->visible)

• return -EINVAL;
  

Can't we handle this by iterating 0 damage rects instead? Would make the code a bit cleaner I think.

• memset(iter, 0, sizeof(*iter));
• iter->clips = drm_plane_get_damage_clips(new_state);
• iter->num_clips = drm_plane_get_damage_clips_count(new_state);
• if (!iter->clips)

• iter->full_update = true;
  

• if (!drm_rect_equals(&new_state->src, &old_state->src))

• iter->full_update = true;
  

• iter->plane_src.x1 = convert_fixed_to_32(new_state->src.x1);
• iter->plane_src.y1 = convert_fixed_to_32(new_state->src.y1);
• iter->plane_src.x2 = convert_fixed_to_32(new_state->src.x2);
• iter->plane_src.y2 = convert_fixed_to_32(new_state->src.y2);

I think you want to clip with the clipped rectangles here, not with the ones userspace provides.

Also I think you're rounding is wrong here - I think you need to round down for x/y1, and round up for x/y2 to make sure you catch all the pixels?

Unit tests for this, in the form of a drm selftest (like we have for drm_mm.c already, but for kms helpers) would be perfect. Much easier to review a testcase than do bitmath in my head :-)

• if (iter->full_update) {

• iter->clips = 0;
  

• iter->curr_clip = 0;
  

• iter->num_clips = 0;
  

• }
• return 0;

+} +EXPORT_SYMBOL(drm_atomic_helper_damage_iter_init);

+/**

• • drm_atomic_helper_damage_iter_next - advance the damage iterator
• • @iter: The iterator to advance.
• • @rect: Return a rectangle in fb coordinate clipped to plane src.
• • Returns: true if the output is valid, false if we've reached the end of the
• • rectangle list.
• */

+bool +drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter,

• 		   struct drm_rect *rect)
  

+{

• bool ret = false;
• if (iter->full_update) {

• *rect = iter->plane_src;
  

• iter->full_update = false;
  

• return true;
  

• }
• while (iter->curr_clip < iter->num_clips) {

• *rect = iter->clips[iter->curr_clip];
  

• iter->curr_clip++;
  

• if (drm_rect_intersect(rect, &iter->plane_src)) {
  

• 	ret = true;
  

• 	break;
  

• }
  

• }
• return ret;

+} +EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next); diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h index 217694e9168c..f1282b459a4f 100644 --- a/include/drm/drm_damage_helper.h +++ b/include/drm/drm_damage_helper.h @@ -32,6 +32,19 @@ #ifndef DRM_DAMAGE_HELPER_H_ #define DRM_DAMAGE_HELPER_H_

+/**

• • struct drm_atomic_helper_damage_iter - damage clip iterator
• • This iterator tracks state needed to walk the list of damage clips.
• */

+struct drm_atomic_helper_damage_iter {

• const struct drm_rect *clips;
• struct drm_rect plane_src;
• uint32_t num_clips;
• uint32_t curr_clip;
• bool full_update;

+};

/**

• drm_plane_get_damage_clips_count - returns damage clips count
• @state: Plane state

@@ -59,5 +72,12 @@ drm_plane_get_damage_clips(const struct drm_plane_state *state) }

void drm_plane_enable_fb_damage_clips(struct drm_plane *plane); +int +drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,

• 		   const struct drm_plane_state *old_state,
  

• 		   const struct drm_plane_state *new_state);
  

I think a for_each_damage macro would be sweet on top of these here. But easy to add later on. -Daniel

+bool +drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter,

• 		   struct drm_rect *rect);
  
  

#endif

2.17.1

On Thu, Sep 06, 2018 at 09:44:25PM +0000, Deepak Singh Rawat wrote:

...

...

#include <drm/drm_damage_helper.h>

/** @@ -75,6 +76,11 @@

• While kernel does not error for overlapped damage clips, it is

discouraged.

...

*/

+static int convert_fixed_to_32(int fixed) +{

• return ((fixed >> 15) & 1) + (fixed >> 16);

+}

/**

• drm_plane_enable_fb_damage_clips - enables plane fb damage clips

property

...

• @plane: plane on which to enable damage clips property

@@ -90,3 +96,90 @@ void drm_plane_enable_fb_damage_clips(struct

drm_plane *plane)

...

		   0);

} EXPORT_SYMBOL(drm_plane_enable_fb_damage_clips);

+/**

• • drm_atomic_helper_damage_iter_init - initialize the damage iterator
• • @iter: The iterator to initialize.
• • @old_state: old plane state for validation.
• • @new_state: plane state from which to iterate the damage clips.
• • Initialize an iterator that clip framebuffer damage in plane

fb_damage_clips

...

• • blob to plane src clip. The iterator returns full plane src in case needing
• • full update e.g. during full modeset.
• • With this helper iterator, drivers which enabled fb_damage_clips

property can

...

• • iterate over the damage clips that falls inside plane src during plane
• • update.
• • Returns: 0 on success and negative error code on error. If an error code

is

...

• • returned then it means the plane state shouldn't update with attached

fb.

...

• */

+int +drm_atomic_helper_damage_iter_init(struct

drm_atomic_helper_damage_iter *iter,

...

• 		   const struct drm_plane_state *old_state,
  

• 		   const struct drm_plane_state *new_state)
  

+{

• if (!new_state || !new_state->crtc || !new_state->fb)

• return -EINVAL;
  

• if (!new_state->visible)

• return -EINVAL;
  

Can't we handle this by iterating 0 damage rects instead? Would make the code a bit cleaner I think.

Agreed.

...

...

• memset(iter, 0, sizeof(*iter));
• iter->clips = drm_plane_get_damage_clips(new_state);
• iter->num_clips = drm_plane_get_damage_clips_count(new_state);
• if (!iter->clips)

• iter->full_update = true;
  

• if (!drm_rect_equals(&new_state->src, &old_state->src))

• iter->full_update = true;
  

• iter->plane_src.x1 = convert_fixed_to_32(new_state->src.x1);
• iter->plane_src.y1 = convert_fixed_to_32(new_state->src.y1);
• iter->plane_src.x2 = convert_fixed_to_32(new_state->src.x2);
• iter->plane_src.y2 = convert_fixed_to_32(new_state->src.y2);

I think you want to clip with the clipped rectangles here, not with the ones userspace provides.

new_state->src.x1 is the clipped one, clipped in the function drm_atomic_helper_check_plane_state. Or am I missing something ?

You're right, I misremembered. This looks correct. Would be good to explain this in the kerneldoc, and that you must call drm_atomic_helper_check_plane_state() first before calling this.

Cheers, Daniel

...

Also I think you're rounding is wrong here - I think you need to round down for x/y1, and round up for x/y2 to make sure you catch all the pixels?

Unit tests for this, in the form of a drm selftest (like we have for drm_mm.c already, but for kms helpers) would be perfect. Much easier to review a testcase than do bitmath in my head :-)

Sure I will work on a unit test case for this scenario and work on round up of plane_src.

...

...

• if (iter->full_update) {

• iter->clips = 0;
  

• iter->curr_clip = 0;
  

• iter->num_clips = 0;
  

• }
• return 0;

+} +EXPORT_SYMBOL(drm_atomic_helper_damage_iter_init);

+/**

• • drm_atomic_helper_damage_iter_next - advance the damage iterator
• • @iter: The iterator to advance.
• • @rect: Return a rectangle in fb coordinate clipped to plane src.
• • Returns: true if the output is valid, false if we've reached the end of

the

...

• • rectangle list.
• */

+bool +drm_atomic_helper_damage_iter_next(struct

drm_atomic_helper_damage_iter *iter,

...

• 		   struct drm_rect *rect)
  

+{

• bool ret = false;
• if (iter->full_update) {

• *rect = iter->plane_src;
  

• iter->full_update = false;
  

• return true;
  

• }
• while (iter->curr_clip < iter->num_clips) {

• *rect = iter->clips[iter->curr_clip];
  

• iter->curr_clip++;
  

• if (drm_rect_intersect(rect, &iter->plane_src)) {
  

• 	ret = true;
  

• 	break;
  

• }
  

• }
• return ret;

+} +EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next); diff --git a/include/drm/drm_damage_helper.h

b/include/drm/drm_damage_helper.h

...

index 217694e9168c..f1282b459a4f 100644 --- a/include/drm/drm_damage_helper.h +++ b/include/drm/drm_damage_helper.h @@ -32,6 +32,19 @@ #ifndef DRM_DAMAGE_HELPER_H_ #define DRM_DAMAGE_HELPER_H_

+/**

• • struct drm_atomic_helper_damage_iter - damage clip iterator
• • This iterator tracks state needed to walk the list of damage clips.
• */

+struct drm_atomic_helper_damage_iter {

• const struct drm_rect *clips;
• struct drm_rect plane_src;
• uint32_t num_clips;
• uint32_t curr_clip;
• bool full_update;

+};

/**

• drm_plane_get_damage_clips_count - returns damage clips count
• @state: Plane state

@@ -59,5 +72,12 @@ drm_plane_get_damage_clips(const struct

drm_plane_state *state)

...

}

void drm_plane_enable_fb_damage_clips(struct drm_plane *plane); +int +drm_atomic_helper_damage_iter_init(struct

drm_atomic_helper_damage_iter *iter,

...

• 		   const struct drm_plane_state *old_state,
  

• 		   const struct drm_plane_state *new_state);
  

I think a for_each_damage macro would be sweet on top of these here. But easy to add later on. -Daniel

...

+bool +drm_atomic_helper_damage_iter_next(struct

drm_atomic_helper_damage_iter *iter,

...

• 		   struct drm_rect *rect);
  
  

#endif

2.17.1

-- Daniel Vetter Software Engineer, Intel Corporation https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ff wll.ch&amp;data=02%7C01%7Cdrawat%40vmware.com%7C117739774a7341f d8c0208d613cd83d5%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C 636718170756943488&amp;sdata=tNK2zTqJrq8U5U4W96wp49FZ8LAB9fl2WV UixbOSWsU%3D&amp;reserved=0

On Wed, Sep 05, 2018 at 04:38:50PM -0700, Deepak Rawat wrote:

Plane damage is irrelevant when full modeset happens so clear the damage blob property(If set by user-space). With damage blob cleared damage helper iterator will return full plane src as damage clip.

Signed-off-by: Deepak Rawat drawat@vmware.com

drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ include/drm/drm_damage_helper.h | 10 ++++++++++ 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index be83e2763c18..e06d2d5d582f 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -31,6 +31,7 @@ #include <drm/drm_crtc_helper.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_writeback.h> +#include <drm/drm_damage_helper.h> #include <linux/dma-fence.h>

#include "drm_crtc_helper_internal.h" @@ -88,6 +89,9 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state, return;

crtc_state->planes_changed = true;

• if (drm_atomic_crtc_needs_modeset(crtc_state))
  

• 	drm_plane_clear_damage(plane_state);
  

I'm not 100% sure this is the best place to put this. I'm also wondering whether we should clear damage when moving planes between crtc on top of this. But I guess vmwgfx doesn't allow that, we can figure this out when the first driver with moveable planes adds damage support.

} }

diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h index f1282b459a4f..1f988f7fdd72 100644 --- a/include/drm/drm_damage_helper.h +++ b/include/drm/drm_damage_helper.h @@ -71,6 +71,16 @@ drm_plane_get_damage_clips(const struct drm_plane_state *state) state->fb_damage_clips->data : NULL); }

+/**

• • drm_plane_clear_damage - clears damage blob in a plane state
• • @state: Plane state

A bit more kerneldoc would be good. Maybe explain how that impacts the damage iterator - you get full damage after calling this, which is a bit confusing for a function called clear_damage. So definitely worth explaining.

With the kerneldoc beefed up:

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

• */

+static inline void drm_plane_clear_damage(struct drm_plane_state *state) +{

• drm_property_blob_put(state->fb_damage_clips);
• state->fb_damage_clips = NULL;

+}

void drm_plane_enable_fb_damage_clips(struct drm_plane *plane); int drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, -- 2.17.1

On Wed, Sep 05, 2018 at 04:38:50PM -0700, Deepak Rawat wrote:

Plane damage is irrelevant when full modeset happens so clear the damage blob property(If set by user-space). With damage blob cleared damage helper iterator will return full plane src as damage clip.

Signed-off-by: Deepak Rawat drawat@vmware.com

drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ include/drm/drm_damage_helper.h | 10 ++++++++++ 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index be83e2763c18..e06d2d5d582f 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -31,6 +31,7 @@ #include <drm/drm_crtc_helper.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_writeback.h> +#include <drm/drm_damage_helper.h> #include <linux/dma-fence.h>

#include "drm_crtc_helper_internal.h" @@ -88,6 +89,9 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state, return;

crtc_state->planes_changed = true;

• if (drm_atomic_crtc_needs_modeset(crtc_state))
  

• 	drm_plane_clear_damage(plane_state);
  

So if we sometimes magically clear it, where do we reinitialize it with the user provided damage for the next update?

} }

diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h index f1282b459a4f..1f988f7fdd72 100644 --- a/include/drm/drm_damage_helper.h +++ b/include/drm/drm_damage_helper.h @@ -71,6 +71,16 @@ drm_plane_get_damage_clips(const struct drm_plane_state *state) state->fb_damage_clips->data : NULL); }

+/**

• • drm_plane_clear_damage - clears damage blob in a plane state
• • @state: Plane state
• */

+static inline void drm_plane_clear_damage(struct drm_plane_state *state) +{

• drm_property_blob_put(state->fb_damage_clips);
• state->fb_damage_clips = NULL;

Ah. So you're trying to clear out the user provided damage here. That doesn't really seem like sane sematics to me. Either the user provided damage should stick always instead of just sometimes, or it should always be cleared once the update is done (ie. could clear in duplicate_state() in that case).

+}

void drm_plane_enable_fb_damage_clips(struct drm_plane *plane); int drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, -- 2.17.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Thu, Sep 06, 2018 at 04:12:39PM +0200, Daniel Vetter wrote:

On Thu, Sep 06, 2018 at 03:02:32PM +0300, Ville Syrjälä wrote:

...

On Wed, Sep 05, 2018 at 04:38:50PM -0700, Deepak Rawat wrote:

...

Plane damage is irrelevant when full modeset happens so clear the damage blob property(If set by user-space). With damage blob cleared damage helper iterator will return full plane src as damage clip.

Signed-off-by: Deepak Rawat drawat@vmware.com

drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ include/drm/drm_damage_helper.h | 10 ++++++++++ 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index be83e2763c18..e06d2d5d582f 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -31,6 +31,7 @@ #include <drm/drm_crtc_helper.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_writeback.h> +#include <drm/drm_damage_helper.h> #include <linux/dma-fence.h>

#include "drm_crtc_helper_internal.h" @@ -88,6 +89,9 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state, return;

crtc_state->planes_changed = true;

• if (drm_atomic_crtc_needs_modeset(crtc_state))
  

• 	drm_plane_clear_damage(plane_state);
  

So if we sometimes magically clear it, where do we reinitialize it with the user provided damage for the next update?

...

} }

diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h index f1282b459a4f..1f988f7fdd72 100644 --- a/include/drm/drm_damage_helper.h +++ b/include/drm/drm_damage_helper.h @@ -71,6 +71,16 @@ drm_plane_get_damage_clips(const struct drm_plane_state *state) state->fb_damage_clips->data : NULL); }

+/**

• • drm_plane_clear_damage - clears damage blob in a plane state
• • @state: Plane state
• */

+static inline void drm_plane_clear_damage(struct drm_plane_state *state) +{

• drm_property_blob_put(state->fb_damage_clips);
• state->fb_damage_clips = NULL;

Ah. So you're trying to clear out the user provided damage here. That doesn't really seem like sane sematics to me. Either the user provided damage should stick always instead of just sometimes, or it should always be cleared once the update is done (ie. could clear in duplicate_state() in that case).

Read patch 2 more careful, we always reset damage when duplicating state. So it's one-shot (like fences).

Ah, thanks for pointing it out. Was actually in patch 1 :)

But you might also want to ignore damage (and do a full upload) in other cases, like a modeset, or when you move your plane between crtc. This function here is for that.

And yes this should be explained in the kerneldoc. And _clear_damage is not a great name for what it's meant to do, that's just what it does. I think in the past I've suggested _full_damage() or something like that. -Daniel

...

...

+}

void drm_plane_enable_fb_damage_clips(struct drm_plane *plane); int drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, -- 2.17.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- Ville Syrjälä Intel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

On Wed, Sep 05, 2018 at 04:38:51PM -0700, Deepak Rawat wrote:

From: Rob Clark robdclark@gmail.com

Add an atomic helper to implement dirtyfb support. This is needed to support DSI command-mode panels with x11 userspace (ie. when we can't rely on pageflips to trigger a flush to the panel).

v2: Modified the helper to use plane fb_damage_clips property and removed plane_state::dirty flag.

Signed-off-by: Rob Clark robdclark@gmail.com Signed-off-by: Deepak Rawat drawat@vmware.com

---

drivers/gpu/drm/drm_damage_helper.c | 116 ++++++++++++++++++++++++++++ include/drm/drm_damage_helper.h | 4 + 2 files changed, 120 insertions(+)

diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c index 7d70f6001889..b016ec375295 100644 --- a/drivers/gpu/drm/drm_damage_helper.c +++ b/drivers/gpu/drm/drm_damage_helper.c @@ -26,6 +26,7 @@

• Authors:
• Deepak Rawat drawat@vmware.com

• • Rob Clark robdclark@gmail.com
  **************************************************************************/

@@ -81,6 +82,21 @@ static int convert_fixed_to_32(int fixed) return ((fixed >> 15) & 1) + (fixed >> 16); }

+static void convert_clip_rect_to_rect(const struct drm_clip_rect *src,

• 		      struct drm_rect *dest,
  

• 		      uint32_t num_clips)
  

I think we need to use drm_mode_rect here, because it's essentially the uapi one.

+{

• while (num_clips > 0) {

• dest->x1 = src->x1;
  

• dest->y1 = src->y1;
  

• dest->x2 = src->x2;
  

• dest->y2 = src->y2;
  

• src++;
  

• dest++;
  

• num_clips--;
  

• }

+}

/**

• drm_plane_enable_fb_damage_clips - enables plane fb damage clips property
• @plane: plane on which to enable damage clips property

@@ -183,3 +199,103 @@ drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter, return ret; } EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);

+/**

• • drm_atomic_helper_dirtyfb - helper for dirtyfb
• • @fb: DRM framebuffer
• • @file_priv: drm file for the ioctl call
• • @flags: dirty fb annotate flags
• • @color: color for annotate fill
• • @clips: dirty region
• • @num_clips: count of clip in clips
• • A helper to implement drm_framebuffer_funcs::dirty. This helper will do a
• • full update in case of annotate flags.
• • Returns: Zero on success, negative errno on failure.
• */

+int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,

• 	      struct drm_file *file_priv, unsigned flags,
  

• 	      unsigned color, struct drm_clip_rect *clips,
  

• 	      unsigned num_clips)
  

+{

• struct drm_modeset_acquire_ctx ctx;
• struct drm_property_blob *damage = NULL;
• struct drm_atomic_state *state;
• struct drm_plane *plane;
• struct drm_rect *rects = NULL;
• int ret = 0;
• /*

• * When called from ioctl, we are interruptable, but not when called
  

• * internally (ie. defio worker)
  

• */
  

• drm_modeset_acquire_init(&ctx,

• file_priv ? DRM_MODESET_ACQUIRE_INTERRUPTIBLE : 0);
  

• state = drm_atomic_state_alloc(fb->dev);
• if (!state) {

• ret = -ENOMEM;
  

• goto out;
  

• }
• state->acquire_ctx = &ctx;
• WARN_ONCE(flags & DRM_MODE_FB_DIRTY_FLAGS,

•   "Full update for dirtyfb with DRM_MODE_FB_DIRTY_FLAGS\n");
  

User-triggerable backtrace isn't good. I think you just need to handle the FILL/COPY meaning.

For COPY we need to ignore all the odd rects (iirc those are the sources). FILL we can just handle. See e.g. what tinydrm_merge_clips does.

Otherwise I think this looks good. -Daniel

• if (!(flags & DRM_MODE_FB_DIRTY_FLAGS)) {

• rects = kcalloc(num_clips, sizeof(*rects), GFP_KERNEL);
  

• if (!rects) {
  

• 	ret = -ENOMEM;
  

• 	goto out;
  

• }
  

• convert_clip_rect_to_rect(clips, rects, num_clips);
  

• damage = drm_property_create_blob(fb->dev,
  

• 				  num_clips * sizeof(*rects),
  

• 				  rects);
  

• if (IS_ERR(damage)) {
  

• 	ret = PTR_ERR(damage);
  

• 	damage = NULL;
  

• 	goto out;
  

• }
  

• }

+retry:

• drm_for_each_plane(plane, fb->dev) {

• struct drm_plane_state *plane_state;
  

• if (plane->state->fb != fb)
  

• 	continue;
  

• plane_state = drm_atomic_get_plane_state(state, plane);
  

• if (IS_ERR(plane_state)) {
  

• 	ret = PTR_ERR(plane_state);
  

• 	goto out;
  

• }
  

• drm_property_replace_blob(&plane_state->fb_damage_clips,
  

• 			  damage);
  

• }
• ret = drm_atomic_commit(state);

+out:

• if (ret == -EDEADLK) {

• drm_atomic_state_clear(state);
  

• ret = drm_modeset_backoff(&ctx);
  

• if (!ret)
  

• 	goto retry;
  

• }
• drm_property_blob_put(damage);
• kfree(rects);
• drm_atomic_state_put(state);
• drm_modeset_drop_locks(&ctx);
• drm_modeset_acquire_fini(&ctx);
• return ret;

+} +EXPORT_SYMBOL(drm_atomic_helper_dirtyfb); diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h index 1f988f7fdd72..9d5c9444761b 100644 --- a/include/drm/drm_damage_helper.h +++ b/include/drm/drm_damage_helper.h @@ -89,5 +89,9 @@ drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, bool drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter, struct drm_rect *rect); +int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,

• 	      struct drm_file *file_priv, unsigned flags,
  

• 	      unsigned color, struct drm_clip_rect *clips,
  

• 	      unsigned num_clips);
  
  

#endif

2.17.1

On 09/06/2018 01:38 AM, Deepak Rawat wrote:

Add a new struct vmw_du_update_plane similar to vmw_kms_dirty which represent the flow of operations needed to update a display unit from surface or bo(blit a new framebuffer).

Signed-off-by: Deepak Rawat drawat@vmware.com

drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 108 +++++++++++++++++++++ drivers/gpu/drm/vmwgfx/vmwgfx_kms.h | 143 ++++++++++++++++++++++++++++ 2 files changed, 251 insertions(+)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 1edbae73d6d6..c1de8f609bf4 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -30,6 +30,7 @@ #include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_rect.h> +#include <drm/drm_damage_helper.h>

/* Might need a hrtimer here? */ #define VMWGFX_PRESENT_RATE ((HZ / 60 > 0) ? HZ / 60 : 1) @@ -3077,3 +3078,110 @@ void vmw_kms_lost_device(struct drm_device *dev) { drm_atomic_helper_shutdown(dev); }

+/**

• • vmw_du_helper_plane_update - helper to do plane update on display unit
• • @update: The closure structure.
• • RETURNS:
• • 0 on success or a negative error code on failure.
• */

We should try to comply better with the kerneldoc description https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt

Otherwise This patch LGTM.