[PATCH 00/14] plane update with damage

[PATCH 00/14] plane update with damage

[Deepak Rawat]

Hi,

Set of patches for a new plane property FB_DAMAGE_CLIPS to send damage during
plane update. I have incorporated review comments since last RFC. Also with
this patch series vmwgfx implementation of plane damage.

Based on Rob Clark earlier patch, implemented legacy dirtyfb with new damage
clips property.

I will be sending a separate patch series for unit test case for damage
interface using igt-gpu-tools. Also a separate patch series for a rudimentary
weston implementation, which for now only send damage for default scanout plane.

Testing: vmware xorg driver mainly use dirty_fb ioctl to update the screen so
it is a good overall test of damage interface. Weston implementation also works
as intended.

Let me know what you think. Thanks,
Deepak

Deepak Rawat (12):
  drm: add helper iterator functions for plane fb_damage_clips blob
  drm: clear plane damage during full modeset
  drm/vmwgfx: add a new interface for plane update on a display unit
  drm/vmwgfx: implement STDU plane update for surface backed fb
  drm/vmwgfx: implement STDU plane update for BO backed fb
  drm/vmwgfx: use the new interface for STDU plane update
  drm/vmwgfx: enable FB_DAMAGE_CLIPS property for STDU primary plane
  drm/vmwgfx: implement SOU plane update for surface backed fb
  drm/vmwgfx: implement SOU plane update for BO backed fb
  drm/vmwgfx: use the new interface for SOU plane update
  drm/vmwgfx: enable FB_DAMAGE_CLIPS property for SOU primary plane
  drm/vmwgfx: use atomic helper function for dirty fb IOCTL

Lukasz Spintzyk (1):
  drm: add new plane property FB_DAMAGE_CLIPS to send damage during
    plane update

Rob Clark (1):
  drm: add helper to implement legacy dirtyfb

 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  |   8 +
 drivers/gpu/drm/drm_damage_helper.c  | 301 +++++++++++++++++
 drivers/gpu/drm/drm_mode_config.c    |   6 +
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c  | 225 ++++++-------
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.h  | 171 ++++++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 361 ++++++++++++++++++--
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 483 +++++++++++++++++++++++++--
 include/drm/drm_damage_helper.h      |  97 ++++++
 include/drm/drm_mode_config.h        |  10 +
 include/drm/drm_plane.h              |   8 +
 include/uapi/drm/drm_mode.h          |  19 ++
 14 files changed, 1539 insertions(+), 174 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_damage_helper.c
 create mode 100644 include/drm/drm_damage_helper.h

-- 
2.17.1

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

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

[Deepak Rawat]

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

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

[PATCH 02/14] drm: add helper iterator functions for plane fb_damage_clips blob

[Deepak Rawat]

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;
+
+	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);
+
+	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);
+bool
+drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter,
+				   struct drm_rect *rect);
 
 #endif
-- 
2.17.1

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

[PATCH 03/14] drm: clear plane damage during full modeset

[Deepak Rawat]

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

[PATCH 04/14] drm: add helper to implement legacy dirtyfb

[Deepak Rawat]

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

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

[PATCH 05/14] drm/vmwgfx: add a new interface for plane update on a display unit

[Deepak Rawat]

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.
+ */
+int vmw_du_helper_plane_update(struct vmw_du_update_plane *update)
+{
+	struct drm_plane_state *state = update->plane->state;
+	struct drm_plane_state *old_state = update->old_state;
+	struct drm_atomic_helper_damage_iter iter;
+	struct drm_rect clip;
+	struct drm_rect bb;
+	uint32_t reserved_size = 0;
+	uint32_t submit_size = 0;
+	uint32_t curr_size = 0;
+	uint32_t num_hits = 0;
+	void *cmd_start;
+	char *cmd_next;
+	int ret;
+
+	/*
+	 * Iterate in advance to check if really need plane update and find the
+	 * number of clips that actually are in plane src for fifo allocation.
+	 * Iterator init returns negative error code if no need to do plane
+	 * update.
+	 */
+	ret = drm_atomic_helper_damage_iter_init(&iter, old_state, state);
+	if (ret)
+		return 0;
+
+	while (drm_atomic_helper_damage_iter_next(&iter, &clip))
+		num_hits++;
+
+	/*
+	 * If all clips are outside plane src no need to do plane update on
+	 * display unit
+	 */
+	if (num_hits == 0)
+		return 0;
+
+	ret = update->prepare(update);
+	if (ret) {
+		DRM_ERROR("Failed to validate FB for command submission\n");
+		return ret;
+	}
+
+	reserved_size = update->calc_fifo_size(update, num_hits);
+	cmd_start = vmw_fifo_reserve(update->dev_priv, reserved_size);
+	if (!cmd_start) {
+		DRM_ERROR("Failed to allocate FIFO space for plane update\n");
+		update->revert(update);
+		return -ENOMEM;
+	}
+	cmd_next = cmd_start;
+
+	if (update->post_prepare) {
+		curr_size = update->post_prepare(update, cmd_next);
+		cmd_next += curr_size;
+		submit_size += curr_size;
+	}
+
+	if (update->pre_clip) {
+		curr_size = update->pre_clip(update, cmd_next, num_hits);
+		cmd_next += curr_size;
+		submit_size += curr_size;
+	}
+
+	bb.x1 = INT_MAX;
+	bb.y1 = INT_MAX;
+	bb.x2 = INT_MIN;
+	bb.y2 = INT_MIN;
+
+	drm_atomic_helper_damage_iter_init(&iter, old_state, state);
+	while (drm_atomic_helper_damage_iter_next(&iter, &clip)) {
+		uint32_t fb_x = clip.x1;
+		uint32_t fb_y = clip.y1;
+
+		vmw_du_translate_to_crtc(state, &clip);
+		if (update->clip) {
+			curr_size = update->clip(update, cmd_next, &clip, fb_x,
+						 fb_y);
+			cmd_next += curr_size;
+			submit_size += curr_size;
+		}
+		bb.x1 = min_t(int, bb.x1, clip.x1);
+		bb.y1 = min_t(int, bb.y1, clip.y1);
+		bb.x2 = max_t(int, bb.x2, clip.x2);
+		bb.y2 = max_t(int, bb.y2, clip.y2);
+	}
+
+	curr_size = update->post_clip(update, cmd_next, &bb);
+	submit_size += curr_size;
+
+	if (reserved_size < submit_size)
+		submit_size = 0;
+
+	vmw_fifo_commit(update->dev_priv, submit_size);
+
+	update->finish(update);
+
+	return ret;
+}
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
index 31311298ec0b..6bda8d3fb52f 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
@@ -33,7 +33,132 @@
 #include <drm/drm_encoder.h>
 #include "vmwgfx_drv.h"
 
+/**
+ * struct vmw_du_update_plane - closure structure for vmw_du_helper_plane_update
+ *
+ * This structure loosely represent the set of operations needed to perform a
+ * plane update on a display unit. Implementer will define that functionality
+ * according to the function callbacks for this structure. In brief it involves
+ * surface/buffer object validation, populate FIFO commands and command
+ * submission to the device.
+ *
+ * @plane: plane which is being updated.
+ * @old_state: old state of plane.
+ * @dev_priv: device private.
+ * @du: display unit on which to update the plane.
+ * @vfb: framebuffer which is blitted to display unit.
+ * @out_fence: (optional) out fence for resource finish.
+ */
+struct vmw_du_update_plane {
+	/**
+	 * @prepare:
+	 *
+	 * This callback should be called to prepare the surface resource or the
+	 * buffer object to validate it before command submission.
+	 *
+	 * RETURNS:
+	 *
+	 * 0 on success or a negative error code on failure.
+	 */
+	int (*prepare)(struct vmw_du_update_plane *update);
+
+	/**
+	 * @calc_fifo_size:
+	 *
+	 * Determine fifo size for the commands needed for update. The number of
+	 * damage clips on display unit @num_hits will be passed to allocate
+	 * sufficient fifo space.
+	 *
+	 * RETURNS:
+	 *
+	 * Fifo size needed
+	 */
+	uint32_t (*calc_fifo_size)(struct vmw_du_update_plane *update,
+				   uint32_t num_hits);
 
+	/**
+	 * @post_prepare:
+	 *
+	 * Some surface resource or buffer object need some extra cmd submission
+	 * like update GB image for proxy surface and define a GMRFB for screen
+	 * object. That should should be done here as this callback will be
+	 * called after FIFO allocation with the address of command buufer.
+	 *
+	 * This callback is optional.
+	 *
+	 * RETURNS:
+	 *
+	 * Size of commands populated to command buffer.
+	 */
+	uint32_t (*post_prepare)(struct vmw_du_update_plane *update, void *cmd);
+
+	/**
+	 * @pre_clip:
+	 *
+	 * This is where pre clip related command should be populated like
+	 * surface copy/DMA, etc.
+	 *
+	 * This callback is optional.
+	 *
+	 * RETURNS:
+	 *
+	 * Size of commands populated to command buffer.
+	 */
+	uint32_t (*pre_clip)(struct vmw_du_update_plane *update, void *cmd,
+			     uint32_t num_hits);
+
+	/**
+	 * @clip:
+	 *
+	 * This is where to populate clips for surface copy/dma or blit commands
+	 * if needed. This will be called times have damage in display unit,
+	 * which is one if doing full update. @clip is the damage in destination
+	 * coordinates which is crtc/DU and @src_x, @src_y is damage clip src in
+	 * framebuffer coordinate.
+	 *
+	 * This callback is optional.
+	 *
+	 * RETURNS:
+	 *
+	 * Size of commands populated to command buffer.
+	 */
+	uint32_t (*clip)(struct vmw_du_update_plane *update, void *cmd,
+			 struct drm_rect *clip, uint32_t src_x, uint32_t src_y);
+
+	/**
+	 * @post_clip:
+	 *
+	 * This is where to populate display unit update commands or blit
+	 * commands.
+	 *
+	 * RETURNS:
+	 *
+	 * Size of commands populated to command buffer.
+	 */
+	uint32_t (*post_clip)(struct vmw_du_update_plane *update, void *cmd,
+				    struct drm_rect *bb);
+
+	/**
+	 * @finish:
+	 *
+	 * Finish surface resource or buffer object.
+	 */
+	void (*finish)(struct vmw_du_update_plane *update);
+
+	/**
+	 * @revert:
+	 *
+	 * Revert surface resource or buffer object in case of failure.
+	 */
+	void (*revert)(struct vmw_du_update_plane *update);
+
+	struct drm_plane *plane;
+	struct drm_plane_state *old_state;
+	struct vmw_private *dev_priv;
+	struct vmw_display_unit *du;
+	struct vmw_framebuffer *vfb;
+	struct vmw_fence_obj **out_fence;
+};
 
 /**
  * struct vmw_kms_dirty - closure structure for the vmw_kms_helper_dirty
@@ -470,4 +595,22 @@ int vmw_kms_stdu_dma(struct vmw_private *dev_priv,
 
 int vmw_kms_set_config(struct drm_mode_set *set,
 		       struct drm_modeset_acquire_ctx *ctx);
+
+int vmw_du_helper_plane_update(struct vmw_du_update_plane *update);
+
+/**
+ * vmw_du_translate_to_crtc - translate a rect from framebuffer to crtc
+ *
+ * @state: plane state.
+ * @r: rect to translate.
+ */
+static inline void vmw_du_translate_to_crtc(struct drm_plane_state *state,
+					    struct drm_rect *r)
+{
+	int translate_crtc_x = -((state->src_x >> 16) - state->crtc_x);
+	int translate_crtc_y = -((state->src_y >> 16) - state->crtc_y);
+
+	drm_rect_translate(r, translate_crtc_x, translate_crtc_y);
+}
+
 #endif
-- 
2.17.1

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

[PATCH 06/14] drm/vmwgfx: implement STDU plane update for surface backed fb

[Deepak Rawat]

Using the new interface implement STDU plane update for surface backed
fb.

Signed-off-by: Deepak Rawat <drawat@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.h  |  11 ++
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 209 ++++++++++++++++++++++++++-
 2 files changed, 219 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
index 6bda8d3fb52f..0ecc5231b617 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
@@ -160,6 +160,17 @@ struct vmw_du_update_plane {
 	struct vmw_fence_obj **out_fence;
 };
 
+/**
+ * struct vmw_du_update_plane_surface - closure structure for surface
+ *
+ * @base: base closure structure.
+ * @ctx: validation context.
+ */
+struct vmw_du_update_plane_surface {
+	struct vmw_du_update_plane base;
+	struct vmw_validation_ctx *ctx;
+};
+
 /**
  * struct vmw_kms_dirty - closure structure for the vmw_kms_helper_dirty
  * function.
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
index 93f6b96ca7bb..b5115b4a1787 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
@@ -30,7 +30,7 @@
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
-
+#include <drm/drm_damage_helper.h>
 
 #define vmw_crtc_to_stdu(x) \
 	container_of(x, struct vmw_screen_target_display_unit, base.crtc)
@@ -92,6 +92,10 @@ struct vmw_stdu_surface_copy {
 	SVGA3dCmdSurfaceCopy body;
 };
 
+struct vmw_stdu_update_gb_image {
+	SVGA3dCmdHeader header;
+	SVGA3dCmdUpdateGBImage body;
+};
 
 /**
  * struct vmw_screen_target_display_unit
@@ -1240,7 +1244,210 @@ vmw_stdu_primary_plane_prepare_fb(struct drm_plane *plane,
 	return ret;
 }
 
+static int vmw_stdu_surface_prepare(struct vmw_du_update_plane *update)
+{
+	struct vmw_du_update_plane_surface *srf_update;
+	struct vmw_framebuffer_surface *vfbs;
+
+	srf_update = container_of(update, typeof(*srf_update), base);
+	vfbs = container_of(update->vfb, typeof(*vfbs), base);
+
+	return vmw_kms_helper_resource_prepare(&vfbs->surface->res, true,
+					       srf_update->ctx);
+}
+
+static uint32_t
+vmw_stdu_surface_fifo_size_same_display(struct vmw_du_update_plane *update,
+					uint32_t num_hits)
+{
+	struct vmw_framebuffer_surface *vfbs;
+	uint32_t size = 0;
+
+	vfbs = container_of(update->vfb, typeof(*vfbs), base);
+
+	if (vfbs->is_bo_proxy)
+		size += sizeof(struct vmw_stdu_update_gb_image) * num_hits;
+
+	size += sizeof(struct vmw_stdu_update);
+
+	return size;
+}
+
+static uint32_t vmw_stdu_surface_fifo_size(struct vmw_du_update_plane *update,
+					   uint32_t num_hits)
+{
+	struct vmw_framebuffer_surface *vfbs;
+	uint32_t size = 0;
+
+	vfbs = container_of(update->vfb, typeof(*vfbs), base);
+
+	if (vfbs->is_bo_proxy)
+		size += sizeof(struct vmw_stdu_update_gb_image) * num_hits;
+
+	size += sizeof(struct vmw_stdu_surface_copy) + sizeof(SVGA3dCopyBox) *
+		num_hits + sizeof(struct vmw_stdu_update);
+
+	return size;
+}
+
+static uint32_t
+vmw_stdu_surface_update_proxy(struct vmw_du_update_plane *update, void *cmd)
+{
+	struct vmw_framebuffer_surface *vfbs;
+	struct drm_plane_state *state = update->plane->state;
+	struct drm_plane_state *old_state = update->old_state;
+	struct drm_atomic_helper_damage_iter iter;
+	struct vmw_stdu_update_gb_image *cmd_update = cmd;
+	struct drm_rect clip;
+	uint32_t copy_size = 0;
+
+	vfbs = container_of(update->vfb, typeof(*vfbs), base);
+
+	/*
+	 * proxy surface is special where a buffer object type fb is wrapped
+	 * in a surface and need an update gb image command to sync with device.
+	 */
+	drm_atomic_helper_damage_iter_init(&iter, old_state, state);
+	while (drm_atomic_helper_damage_iter_next(&iter, &clip)) {
+		SVGA3dBox *box = &cmd_update->body.box;
+
+		cmd_update->header.id = SVGA_3D_CMD_UPDATE_GB_IMAGE;
+		cmd_update->header.size = sizeof(cmd_update->body);
+		cmd_update->body.image.sid = vfbs->surface->res.id;
+		cmd_update->body.image.face = 0;
+		cmd_update->body.image.mipmap = 0;
+
+		box->x = clip.x1;
+		box->y = clip.y1;
+		box->z = 0;
+		box->w = drm_rect_width(&clip);
+		box->h = drm_rect_height(&clip);
+		box->d = 1;
+
+		copy_size += sizeof(*cmd_update);
+		cmd_update++;
+	}
+
+	return copy_size;
+}
+
+static uint32_t
+vmw_stdu_surface_populate_copy(struct vmw_du_update_plane  *update, void *cmd,
+			       uint32_t num_hits)
+{
+	struct vmw_screen_target_display_unit *stdu;
+	struct vmw_framebuffer_surface *vfbs;
+	struct vmw_stdu_surface_copy *cmd_copy = cmd;
+
+	stdu = container_of(update->du, typeof(*stdu), base);
+	vfbs = container_of(update->vfb, typeof(*vfbs), base);
 
+	cmd_copy->header.id = SVGA_3D_CMD_SURFACE_COPY;
+	cmd_copy->header.size = sizeof(cmd_copy->body) + sizeof(SVGA3dCopyBox) *
+		num_hits;
+	cmd_copy->body.src.sid = vfbs->surface->res.id;
+	cmd_copy->body.dest.sid = stdu->display_srf->res.id;
+
+	return sizeof(*cmd_copy);
+}
+
+static uint32_t
+vmw_stdu_surface_populate_clip(struct vmw_du_update_plane  *update, void *cmd,
+			       struct drm_rect *clip, uint32_t fb_x,
+			       uint32_t fb_y)
+{
+	struct SVGA3dCopyBox *box = cmd;
+
+	box->srcx = fb_x;
+	box->srcy = fb_y;
+	box->srcz = 0;
+	box->x = clip->x1;
+	box->y = clip->y1;
+	box->z = 0;
+	box->w = drm_rect_width(clip);
+	box->h = drm_rect_height(clip);
+	box->d = 1;
+
+	return sizeof(*box);
+}
+
+static uint32_t
+vmw_stud_surface_populate_update(struct vmw_du_update_plane  *update, void *cmd,
+				 struct drm_rect *bb)
+{
+	vmw_stdu_populate_update(cmd, update->du->unit, bb->x1, bb->x2, bb->y1,
+				 bb->y2);
+
+	return sizeof(struct vmw_stdu_update);
+}
+
+static void vmw_stdu_surface_finish(struct vmw_du_update_plane *update)
+{
+	struct vmw_du_update_plane_surface *srf_update =
+		container_of(update, typeof(*srf_update), base);
+
+	vmw_kms_helper_resource_finish(srf_update->ctx, update->out_fence);
+}
+
+/**
+ * vmw_stdu_plane_update_surface - update display unit for surface backed fb
+ * @dev_priv: device private
+ * @plane: plane state
+ * @old_state: old plane state
+ * @vfb: framebuffer which is blitted to display unit
+ * @out_fence: (optional) If non-NULL, will return a ref-counted pointer to a
+ * struct vmw_fence_obj. The returned fence pointer may be NULL in which case
+ * the device has already synchronized.
+ *
+ * RETURNS:
+ *
+ * 0 on success or a negative error code on failure.
+ */
+static int vmw_stdu_plane_update_surface(struct vmw_private *dev_priv,
+					 struct drm_plane *plane,
+					 struct drm_plane_state *old_state,
+					 struct vmw_framebuffer *vfb,
+					 struct vmw_fence_obj **out_fence)
+{
+	struct vmw_du_update_plane_surface srf_update;
+	struct vmw_screen_target_display_unit *stdu;
+	struct vmw_framebuffer_surface *vfbs;
+	struct vmw_validation_ctx ctx;
+
+	stdu = vmw_crtc_to_stdu(plane->state->crtc);
+	vfbs = container_of(vfb, typeof(*vfbs), base);
+
+	memset(&srf_update, 0, sizeof(struct vmw_du_update_plane_surface));
+	srf_update.base.plane = plane;
+	srf_update.base.old_state = old_state;
+	srf_update.base.dev_priv = dev_priv;
+	srf_update.base.du = vmw_crtc_to_du(plane->state->crtc);
+	srf_update.base.vfb = vfb;
+	srf_update.base.out_fence = out_fence;
+
+	srf_update.ctx = &ctx;
+
+	srf_update.base.prepare = vmw_stdu_surface_prepare;
+
+	if (vfbs->is_bo_proxy)
+		srf_update.base.post_prepare = vmw_stdu_surface_update_proxy;
+
+	if (vfbs->surface->res.id != stdu->display_srf->res.id) {
+		srf_update.base.calc_fifo_size = vmw_stdu_surface_fifo_size;
+		srf_update.base.pre_clip = vmw_stdu_surface_populate_copy;
+		srf_update.base.clip = vmw_stdu_surface_populate_clip;
+	} else {
+		srf_update.base.calc_fifo_size =
+			vmw_stdu_surface_fifo_size_same_display;
+	}
+
+	srf_update.base.post_clip = vmw_stud_surface_populate_update;
+	srf_update.base.finish = vmw_stdu_surface_finish;
+	/* Nothing special to do for revert */
+	srf_update.base.revert = vmw_stdu_surface_finish;
+
+	return vmw_du_helper_plane_update(&srf_update.base);
+}
 
 /**
  * vmw_stdu_primary_plane_atomic_update - formally switches STDU to new plane
-- 
2.17.1

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

[PATCH 07/14] drm/vmwgfx: implement STDU plane update for BO backed fb

[Deepak Rawat]

Using the new interface implement STDU plane update for BO backed fb.

Signed-off-by: Deepak Rawat <drawat@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.h  |  14 ++
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 235 +++++++++++++++++++++++++++
 2 files changed, 249 insertions(+)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
index 0ecc5231b617..1c3dd0e14414 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
@@ -171,6 +171,20 @@ struct vmw_du_update_plane_surface {
 	struct vmw_validation_ctx *ctx;
 };
 
+/**
+ * struct vmw_du_update_plane_buffer - closure structure for buffer object
+ *
+ * @base: base closure structure.
+ * @fb_left: x1 for fb damage bounding box.
+ * @fb_top: y1 for fb damage bounding box.
+ * @cpu_blit: true if need cpu blit.
+ */
+struct vmw_du_update_plane_buffer {
+	struct vmw_du_update_plane base;
+	int fb_left, fb_top;
+	bool cpu_blit;
+};
+
 /**
  * struct vmw_kms_dirty - closure structure for the vmw_kms_helper_dirty
  * function.
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
index b5115b4a1787..8c1ba874512a 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
@@ -1244,6 +1244,241 @@ vmw_stdu_primary_plane_prepare_fb(struct drm_plane *plane,
 	return ret;
 }
 
+static int vmw_stdu_bo_prepare(struct vmw_du_update_plane *update)
+{
+	struct vmw_du_update_plane_buffer *bo_update;
+	struct vmw_framebuffer_bo *vfbbo;
+
+	bo_update = container_of(update, typeof(*bo_update), base);
+	vfbbo = container_of(update->vfb, typeof(*vfbbo), base);
+
+	return vmw_kms_helper_buffer_prepare(update->dev_priv, vfbbo->buffer,
+					     false, false, bo_update->cpu_blit);
+}
+
+static uint32_t vmw_stdu_bo_fifo_size(struct vmw_du_update_plane *update,
+				      uint32_t num_hits)
+{
+	return sizeof(struct vmw_stdu_dma) + sizeof(SVGA3dCopyBox) * num_hits +
+		sizeof(SVGA3dCmdSurfaceDMASuffix) +
+		sizeof(struct vmw_stdu_update);
+}
+
+static uint32_t vmw_stdu_bo_fifo_size_cpu(struct vmw_du_update_plane *update,
+					  uint32_t num_hits)
+{
+	return sizeof(struct vmw_stdu_update_gb_image) +
+		sizeof(struct vmw_stdu_update);
+}
+
+static uint32_t vmw_stdu_bo_populate_dma(struct vmw_du_update_plane  *update,
+					 void *cmd, uint32_t num_hits)
+{
+	struct vmw_screen_target_display_unit *stdu;
+	struct vmw_framebuffer_bo *vfbbo;
+	struct vmw_stdu_dma *cmd_dma = cmd;
+
+	stdu = container_of(update->du, typeof(*stdu), base);
+	vfbbo = container_of(update->vfb, typeof(*vfbbo), base);
+
+	cmd_dma->header.id = SVGA_3D_CMD_SURFACE_DMA;
+	cmd_dma->header.size = sizeof(cmd_dma->body) +
+		sizeof(struct SVGA3dCopyBox) * num_hits +
+		sizeof(SVGA3dCmdSurfaceDMASuffix);
+	vmw_bo_get_guest_ptr(&vfbbo->buffer->base, &cmd_dma->body.guest.ptr);
+	cmd_dma->body.guest.pitch = update->vfb->base.pitches[0];
+	cmd_dma->body.host.sid = stdu->display_srf->res.id;
+	cmd_dma->body.host.face = 0;
+	cmd_dma->body.host.mipmap = 0;
+	cmd_dma->body.transfer = SVGA3D_WRITE_HOST_VRAM;
+
+	return sizeof(*cmd_dma);
+}
+
+static uint32_t vmw_stdu_bo_populate_clip(struct vmw_du_update_plane  *update,
+					  void *cmd, struct drm_rect *clip,
+					  uint32_t fb_x, uint32_t fb_y)
+{
+	struct SVGA3dCopyBox *box = cmd;
+
+	box->srcx = fb_x;
+	box->srcy = fb_y;
+	box->srcz = 0;
+	box->x = clip->x1;
+	box->y = clip->y1;
+	box->z = 0;
+	box->w = drm_rect_width(clip);
+	box->h = drm_rect_height(clip);
+	box->d = 1;
+
+	return sizeof(*box);
+}
+
+static uint32_t vmw_stud_bo_populate_update(struct vmw_du_update_plane  *update,
+					    void *cmd, struct drm_rect *bb)
+{
+	struct vmw_screen_target_display_unit *stdu;
+	struct vmw_framebuffer_bo *vfbbo;
+	SVGA3dCmdSurfaceDMASuffix *suffix = cmd;
+
+	stdu = container_of(update->du, typeof(*stdu), base);
+	vfbbo = container_of(update->vfb, typeof(*vfbbo), base);
+
+	suffix->suffixSize = sizeof(*suffix);
+	suffix->maximumOffset = vfbbo->buffer->base.num_pages * PAGE_SIZE;
+
+	vmw_stdu_populate_update(&suffix[1], stdu->base.unit, bb->x1, bb->x2,
+				 bb->y1, bb->y2);
+
+	return sizeof(*suffix) + sizeof(struct vmw_stdu_update);
+}
+
+static uint32_t vmw_stdu_bo_pre_clip_cpu(struct vmw_du_update_plane  *update,
+					 void *cmd, uint32_t num_hits)
+{
+	struct vmw_du_update_plane_buffer *bo_update =
+		container_of(update, typeof(*bo_update), base);
+
+	bo_update->fb_left = INT_MAX;
+	bo_update->fb_top = INT_MAX;
+
+	return 0;
+}
+
+static uint32_t vmw_stdu_bo_clip_cpu(struct vmw_du_update_plane  *update,
+				     void *cmd, struct drm_rect *clip,
+				     uint32_t fb_x, uint32_t fb_y)
+{
+	struct vmw_du_update_plane_buffer *bo_update =
+		container_of(update, typeof(*bo_update), base);
+
+	bo_update->fb_left = min_t(int, bo_update->fb_left, fb_x);
+	bo_update->fb_top = min_t(int, bo_update->fb_top, fb_y);
+
+	return 0;
+}
+
+static uint32_t
+vmw_stud_bo_populate_update_cpu(struct vmw_du_update_plane  *update, void *cmd,
+				struct drm_rect *bb)
+{
+	struct vmw_du_update_plane_buffer *bo_update;
+	struct vmw_screen_target_display_unit *stdu;
+	struct vmw_framebuffer_bo *vfbbo;
+	struct vmw_diff_cpy diff = VMW_CPU_BLIT_DIFF_INITIALIZER(0);
+	struct vmw_stdu_update_gb_image *cmd_img = cmd;
+	struct vmw_stdu_update *cmd_update;
+	struct ttm_buffer_object *src_bo, *dst_bo;
+	u32 src_offset, dst_offset;
+	s32 src_pitch, dst_pitch;
+	s32 width, height;
+
+	bo_update = container_of(update, typeof(*bo_update), base);
+	stdu = container_of(update->du, typeof(*stdu), base);
+	vfbbo = container_of(update->vfb, typeof(*vfbbo), base);
+
+	width = bb->x2 - bb->x1;
+	height = bb->y2 - bb->y1;
+
+	diff.cpp = stdu->cpp;
+
+	dst_bo = &stdu->display_srf->res.backup->base;
+	dst_pitch = stdu->display_srf->base_size.width * stdu->cpp;
+	dst_offset = bb->y1 * dst_pitch + bb->x1 * stdu->cpp;
+
+	src_bo = &vfbbo->buffer->base;
+	src_pitch = update->vfb->base.pitches[0];
+	src_offset = bo_update->fb_top * src_pitch + bo_update->fb_left *
+		stdu->cpp;
+
+	(void) vmw_bo_cpu_blit(dst_bo, dst_offset, dst_pitch, src_bo,
+			       src_offset, src_pitch, width * stdu->cpp, height,
+			       &diff);
+
+	if (drm_rect_visible(&diff.rect)) {
+		SVGA3dBox *box = &cmd_img->body.box;
+
+		cmd_img->header.id = SVGA_3D_CMD_UPDATE_GB_IMAGE;
+		cmd_img->header.size = sizeof(cmd_img->body);
+		cmd_img->body.image.sid = stdu->display_srf->res.id;
+		cmd_img->body.image.face = 0;
+		cmd_img->body.image.mipmap = 0;
+
+		box->x = diff.rect.x1;
+		box->y = diff.rect.y1;
+		box->z = 0;
+		box->w = drm_rect_width(&diff.rect);
+		box->h = drm_rect_height(&diff.rect);
+		box->d = 1;
+
+		cmd_update = (struct vmw_stdu_update *)&cmd_img[1];
+		vmw_stdu_populate_update(cmd_update, stdu->base.unit,
+					 diff.rect.x1, diff.rect.x2,
+					 diff.rect.y1, diff.rect.y2);
+
+		return sizeof(*cmd_img) + sizeof(*cmd_update);
+	}
+
+	return 0;
+}
+
+static void vmw_stdu_bo_finish(struct vmw_du_update_plane *update)
+{
+	struct vmw_framebuffer_bo *vfbbo = container_of(update->vfb,
+							typeof(*vfbbo), base);
+
+	vmw_kms_helper_buffer_finish(update->dev_priv, NULL, vfbbo->buffer,
+				     NULL, NULL);
+}
+
+/**
+ * vmw_stdu_plane_update_bo - update display unit for bo backed fb
+ * @dev_priv: device private
+ * @plane: plane state
+ * @old_state: old plane state
+ * @vfb: framebuffer which is blitted to display unit
+ *
+ * RETURNS:
+ *
+ * 0 on success or a negative error code on failure.
+ */
+static int vmw_stdu_plane_update_bo(struct vmw_private *dev_priv,
+				    struct drm_plane *plane,
+				    struct drm_plane_state *old_state,
+				    struct vmw_framebuffer *vfb)
+{
+	struct vmw_du_update_plane_buffer bo_update;
+
+	memset(&bo_update, 0, sizeof(struct vmw_du_update_plane_buffer));
+	bo_update.base.plane = plane;
+	bo_update.base.old_state = old_state;
+	bo_update.base.dev_priv = dev_priv;
+	bo_update.base.du = vmw_crtc_to_du(plane->state->crtc);
+	bo_update.base.vfb = vfb;
+
+	bo_update.cpu_blit = !(dev_priv->capabilities & SVGA_CAP_3D);
+
+	bo_update.base.prepare = vmw_stdu_bo_prepare;
+
+	if (bo_update.cpu_blit) {
+		bo_update.base.calc_fifo_size = vmw_stdu_bo_fifo_size_cpu;
+		bo_update.base.pre_clip = vmw_stdu_bo_pre_clip_cpu;
+		bo_update.base.clip = vmw_stdu_bo_clip_cpu;
+		bo_update.base.post_clip = vmw_stud_bo_populate_update_cpu;
+	} else {
+		bo_update.base.calc_fifo_size = vmw_stdu_bo_fifo_size;
+		bo_update.base.pre_clip = vmw_stdu_bo_populate_dma;
+		bo_update.base.clip = vmw_stdu_bo_populate_clip;
+		bo_update.base.post_clip = vmw_stud_bo_populate_update;
+	}
+
+	bo_update.base.finish = vmw_stdu_bo_finish;
+	/* Nothing special to do for revert */
+	bo_update.base.revert = vmw_stdu_bo_finish;
+
+	return vmw_du_helper_plane_update(&bo_update.base);
+}
+
 static int vmw_stdu_surface_prepare(struct vmw_du_update_plane *update)
 {
 	struct vmw_du_update_plane_surface *srf_update;
-- 
2.17.1

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

[PATCH 08/14] drm/vmwgfx: use the new interface for STDU plane update

[Deepak Rawat]

With new interface to do plane update on STDU available, use that
instead of old kms_dirty. Update the commet to sync with code.

Signed-off-by: Deepak Rawat <drawat@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 38 +++++++---------------------
 1 file changed, 9 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
index 8c1ba874512a..167190b75e2f 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
@@ -1706,14 +1706,10 @@ vmw_stdu_primary_plane_atomic_update(struct drm_plane *plane,
 	struct vmw_private *dev_priv;
 	int ret;
 
-	/*
-	 * We cannot really fail this function, so if we do, then output an
-	 * error and maintain consistent atomic state.
-	 */
+	/* If somehow gets a device error, maintain consistent atomic state */
 	if (crtc && plane->state->fb) {
 		struct vmw_framebuffer *vfb =
 			vmw_framebuffer_to_vfb(plane->state->fb);
-		struct drm_vmw_rect vclips;
 		stdu = vmw_crtc_to_stdu(crtc);
 		dev_priv = vmw_priv(crtc->dev);
 
@@ -1721,23 +1717,17 @@ vmw_stdu_primary_plane_atomic_update(struct drm_plane *plane,
 		stdu->content_fb_type = vps->content_fb_type;
 		stdu->cpp = vps->cpp;
 
-		vclips.x = crtc->x;
-		vclips.y = crtc->y;
-		vclips.w = crtc->mode.hdisplay;
-		vclips.h = crtc->mode.vdisplay;
-
 		ret = vmw_stdu_bind_st(dev_priv, stdu, &stdu->display_srf->res);
 		if (ret)
 			DRM_ERROR("Failed to bind surface to STDU.\n");
 
 		if (vfb->bo)
-			ret = vmw_kms_stdu_dma(dev_priv, NULL, vfb, NULL, NULL,
-					       &vclips, 1, 1, true, false,
-					       crtc);
+			ret = vmw_stdu_plane_update_bo(dev_priv, plane,
+						       old_state, vfb);
 		else
-			ret = vmw_kms_stdu_surface_dirty(dev_priv, vfb, NULL,
-							 &vclips, NULL, 0, 0,
-							 1, 1, NULL, crtc);
+			ret = vmw_stdu_plane_update_surface(dev_priv, plane,
+							    old_state, vfb,
+							    NULL);
 		if (ret)
 			DRM_ERROR("Failed to update STDU.\n");
 	} else {
@@ -1745,12 +1735,7 @@ vmw_stdu_primary_plane_atomic_update(struct drm_plane *plane,
 		stdu = vmw_crtc_to_stdu(crtc);
 		dev_priv = vmw_priv(crtc->dev);
 
-		/*
-		 * When disabling a plane, CRTC and FB should always be NULL
-		 * together, otherwise it's an error.
-		 * Here primary plane is being disable so blank the screen
-		 * target display unit, if not already done.
-		 */
+		/* Blank STDU when fb and crtc are NULL */
 		if (!stdu->defined)
 			return;
 
@@ -1765,20 +1750,15 @@ vmw_stdu_primary_plane_atomic_update(struct drm_plane *plane,
 		return;
 	}
 
+	/* In case of error vblank event is sent in vmw_du_crtc_atomic_flush */
 	event = crtc->state->event;
-	/*
-	 * In case of failure and other cases, vblank event will be sent in
-	 * vmw_du_crtc_atomic_flush.
-	 */
 	if (event && (ret == 0)) {
 		struct vmw_fence_obj *fence = NULL;
 		struct drm_file *file_priv = event->base.file_priv;
 
 		vmw_execbuf_fence_commands(NULL, dev_priv, &fence, NULL);
 
-		/*
-		 * If fence is NULL, then already sync.
-		 */
+		/* If fence is NULL, then already sync. */
 		if (fence) {
 			ret = vmw_event_fence_action_queue(
 				file_priv, fence, &event->base,
-- 
2.17.1

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

[PATCH 09/14] drm/vmwgfx: enable FB_DAMAGE_CLIPS property for STDU primary plane

[Deepak Rawat]

STDU primary plane now support damage clips, enable it for user-space.

Signed-off-by: Deepak Rawat <drawat@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
index 167190b75e2f..11a76aa96302 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
@@ -1883,6 +1883,7 @@ static int vmw_stdu_init(struct vmw_private *dev_priv, unsigned unit)
 	}
 
 	drm_plane_helper_add(primary, &vmw_stdu_primary_plane_helper_funcs);
+	drm_plane_enable_fb_damage_clips(primary);
 
 	/* Initialize cursor plane */
 	vmw_du_plane_reset(cursor);
-- 
2.17.1

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

[PATCH 10/14] drm/vmwgfx: implement SOU plane update for surface backed fb

[Deepak Rawat]

Using the new interface implement SOU plane update for surface backed
fb.

Signed-off-by: Deepak Rawat <drawat@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.h  |   3 +
 drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 187 +++++++++++++++++++++++++++
 2 files changed, 190 insertions(+)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
index 1c3dd0e14414..45b12448e138 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
@@ -165,10 +165,13 @@ struct vmw_du_update_plane {
  *
  * @base: base closure structure.
  * @ctx: validation context.
+ * @cmd_start: (optional) FIFO command start address (used by SOU only).
  */
 struct vmw_du_update_plane_surface {
 	struct vmw_du_update_plane base;
 	struct vmw_validation_ctx *ctx;
+	/* This member is to handle special case SOU surface update */
+	void *cmd_start;
 };
 
 /**
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
index ad0de7f0cd60..8427a51cde3f 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
@@ -499,6 +499,193 @@ vmw_sou_primary_plane_prepare_fb(struct drm_plane *plane,
 	return vmw_bo_pin_in_vram(dev_priv, vps->bo, true);
 }
 
+static int vmw_sou_surface_prepare(struct vmw_du_update_plane *update)
+{
+	struct vmw_du_update_plane_surface *srf_update;
+	struct vmw_framebuffer_surface *vfbs;
+
+	srf_update = container_of(update, typeof(*srf_update), base);
+	vfbs = container_of(update->vfb, typeof(*vfbs), base);
+
+	return vmw_kms_helper_resource_prepare(&vfbs->surface->res, true,
+					       srf_update->ctx);
+}
+
+static uint32_t vmw_sou_surface_fifo_size(struct vmw_du_update_plane *update,
+					  uint32_t num_hits)
+{
+	return sizeof(struct vmw_kms_sou_dirty_cmd) + sizeof(SVGASignedRect) *
+		num_hits;
+}
+
+static uint32_t vmw_sou_surface_post_prepare(struct vmw_du_update_plane *update,
+					     void *cmd)
+{
+	struct vmw_du_update_plane_surface *srf_update;
+
+	srf_update = container_of(update, typeof(*srf_update), base);
+
+	/*
+	 * SOU SVGA_3D_CMD_BLIT_SURFACE_TO_SCREEN is special in the sense that
+	 * it bounding box is filled before iterating over all the clips. So
+	 * store the FIFO start address and revisit to fill the details.
+	 */
+	srf_update->cmd_start = cmd;
+
+	return 0;
+}
+
+static uint32_t vmw_sou_surface_pre_clip(struct vmw_du_update_plane *update,
+					 void *cmd, uint32_t num_hits)
+{
+	struct vmw_kms_sou_dirty_cmd *blit = cmd;
+	struct vmw_framebuffer_surface *vfbs;
+
+	vfbs = container_of(update->vfb, typeof(*vfbs), base);
+
+	blit->header.id = SVGA_3D_CMD_BLIT_SURFACE_TO_SCREEN;
+	blit->header.size = sizeof(blit->body) + sizeof(SVGASignedRect) *
+		num_hits;
+
+	blit->body.srcImage.sid = vfbs->surface->res.id;
+	blit->body.destScreenId = update->du->unit;
+
+	/* Update the source and destination bounding box later in post_clip */
+	blit->body.srcRect.left = 0;
+	blit->body.srcRect.top = 0;
+	blit->body.srcRect.right = 0;
+	blit->body.srcRect.bottom = 0;
+
+	blit->body.destRect.left = 0;
+	blit->body.destRect.top = 0;
+	blit->body.destRect.right = 0;
+	blit->body.destRect.bottom = 0;
+
+	return sizeof(*blit);
+}
+
+static uint32_t vmw_sou_surface_clip_rect(struct vmw_du_update_plane *update,
+					  void *cmd, struct drm_rect *clip,
+					  uint32_t src_x, uint32_t src_y)
+{
+	SVGASignedRect *rect = cmd;
+
+	/*
+	 * rects are relative to dest bounding box rect on screen object, so
+	 * translate to it later in post_clip
+	 */
+	rect->left = clip->x1;
+	rect->top = clip->y1;
+	rect->right = clip->x2;
+	rect->bottom = clip->y2;
+
+	return sizeof(*rect);
+}
+
+static uint32_t vmw_sou_surface_post_clip(struct vmw_du_update_plane *update,
+					  void *cmd, struct drm_rect *bb)
+{
+	struct vmw_du_update_plane_surface *srf_update;
+	struct drm_plane_state *state = update->plane->state;
+	struct drm_rect src_bb;
+	struct vmw_kms_sou_dirty_cmd *blit;
+	SVGASignedRect *rect;
+	uint32_t num_hits;
+	int translate_src_x;
+	int translate_src_y;
+	int i;
+
+	srf_update = container_of(update, typeof(*srf_update), base);
+
+	blit = srf_update->cmd_start;
+	rect = (SVGASignedRect *)&blit[1];
+
+	num_hits = (blit->header.size - sizeof(blit->body))/
+		sizeof(SVGASignedRect);
+
+	src_bb = *bb;
+
+	/* To translate bb back to fb src coord */
+	translate_src_x = (state->src_x >> 16) - state->crtc_x;
+	translate_src_y = (state->src_y >> 16) - state->crtc_y;
+
+	drm_rect_translate(&src_bb, translate_src_x, translate_src_y);
+
+	blit->body.srcRect.left = src_bb.x1;
+	blit->body.srcRect.top = src_bb.y1;
+	blit->body.srcRect.right = src_bb.x2;
+	blit->body.srcRect.bottom = src_bb.y2;
+
+	blit->body.destRect.left = bb->x1;
+	blit->body.destRect.top = bb->y1;
+	blit->body.destRect.right = bb->x2;
+	blit->body.destRect.bottom = bb->y2;
+
+	/* rects are relative to dest bb rect */
+	for (i = 0; i < num_hits; i++) {
+		rect->left -= bb->x1;
+		rect->top -= bb->y1;
+		rect->right -= bb->x1;
+		rect->bottom -= bb->y1;
+		rect++;
+	}
+
+	return 0;
+}
+
+static void vmw_sou_surface_finish(struct vmw_du_update_plane *update)
+{
+	struct vmw_du_update_plane_surface *srf_update =
+		container_of(update, typeof(*srf_update), base);
+
+	vmw_kms_helper_resource_finish(srf_update->ctx, update->out_fence);
+}
+
+/**
+ * vmw_sou_plane_update_surface - update display unit for surface backed fb
+ * @dev_priv: device private
+ * @plane: plane state
+ * @old_state: old plane state
+ * @vfb: framebuffer which is blitted to display unit
+ * @out_fence: (optional) If non-NULL, will return a ref-counted pointer to a
+ * struct vmw_fence_obj. The returned fence pointer may be NULL in which case
+ * the device has already synchronized.
+ *
+ * RETURNS:
+ *
+ * 0 on success or a negative error code on failure.
+ */
+static int vmw_sou_plane_update_surface(struct vmw_private *dev_priv,
+					struct drm_plane *plane,
+					struct drm_plane_state *old_state,
+					struct vmw_framebuffer *vfb,
+					struct vmw_fence_obj **out_fence)
+{
+	struct vmw_du_update_plane_surface srf_update;
+	struct vmw_validation_ctx ctx;
+
+	memset(&srf_update, 0, sizeof(struct vmw_du_update_plane_surface));
+	srf_update.base.plane = plane;
+	srf_update.base.old_state = old_state;
+	srf_update.base.dev_priv = dev_priv;
+	srf_update.base.du = vmw_crtc_to_du(plane->state->crtc);
+	srf_update.base.vfb = vfb;
+	srf_update.base.out_fence = out_fence;
+
+	srf_update.ctx = &ctx;
+
+	srf_update.base.prepare = vmw_sou_surface_prepare;
+	srf_update.base.calc_fifo_size = vmw_sou_surface_fifo_size;
+	srf_update.base.post_prepare = vmw_sou_surface_post_prepare;
+	srf_update.base.pre_clip = vmw_sou_surface_pre_clip;
+	srf_update.base.clip = vmw_sou_surface_clip_rect;
+	srf_update.base.post_clip = vmw_sou_surface_post_clip;
+	srf_update.base.finish = vmw_sou_surface_finish;
+	/* Nothing special to do for revert */
+	srf_update.base.revert = vmw_sou_surface_finish;
+
+	return vmw_du_helper_plane_update(&srf_update.base);
+}
 
 static void
 vmw_sou_primary_plane_atomic_update(struct drm_plane *plane,
-- 
2.17.1

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

[PATCH 11/14] drm/vmwgfx: implement SOU plane update for BO backed fb

[Deepak Rawat]

Using the new interface implement SOU plane update for BO backed fb.

Signed-off-by: Deepak Rawat <drawat@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 134 +++++++++++++++++++++++++++
 1 file changed, 134 insertions(+)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
index 8427a51cde3f..a68a2c8dab47 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
@@ -76,6 +76,11 @@ struct vmw_kms_sou_dirty_cmd {
 	SVGA3dCmdBlitSurfaceToScreen body;
 };
 
+struct vmw_kms_sou_define_gmrfb {
+	uint32_t header;
+	SVGAFifoCmdDefineGMRFB body;
+};
+
 /**
  * Display unit using screen objects.
  */
@@ -499,6 +504,135 @@ vmw_sou_primary_plane_prepare_fb(struct drm_plane *plane,
 	return vmw_bo_pin_in_vram(dev_priv, vps->bo, true);
 }
 
+static int vmw_sou_bo_prepare(struct vmw_du_update_plane *update)
+{
+	struct vmw_du_update_plane_buffer *bo_update;
+	struct vmw_framebuffer_bo *vfbbo;
+
+	bo_update = container_of(update, typeof(*bo_update), base);
+	vfbbo = container_of(update->vfb, typeof(*vfbbo), base);
+
+	return vmw_kms_helper_buffer_prepare(update->dev_priv, vfbbo->buffer,
+					     true, false, bo_update->cpu_blit);
+}
+
+static uint32_t vmw_sou_bo_fifo_size(struct vmw_du_update_plane *update,
+				     uint32_t num_hits)
+{
+	return sizeof(struct vmw_kms_sou_define_gmrfb) +
+		sizeof(struct vmw_kms_sou_bo_blit) * num_hits;
+}
+
+static uint32_t vmw_sou_bo_define_gmrfb(struct vmw_du_update_plane *update,
+					void *cmd)
+{
+	struct vmw_framebuffer_bo *vfbbo =
+		container_of(update->vfb, typeof(*vfbbo), base);
+	struct vmw_kms_sou_define_gmrfb *gmr = cmd;
+	int depth = update->vfb->base.format->depth;
+
+	/* Emulate RGBA support, contrary to svga_reg.h this is not
+	 * supported by hosts. This is only a problem if we are reading
+	 * this value later and expecting what we uploaded back.
+	 */
+	if (depth == 32)
+		depth = 24;
+
+	gmr->header = SVGA_CMD_DEFINE_GMRFB;
+
+	gmr->body.format.bitsPerPixel = update->vfb->base.format->cpp[0] * 8;
+	gmr->body.format.colorDepth = depth;
+	gmr->body.format.reserved = 0;
+	gmr->body.bytesPerLine = update->vfb->base.pitches[0];
+	vmw_bo_get_guest_ptr(&vfbbo->buffer->base, &gmr->body.ptr);
+
+	return sizeof(*gmr);
+}
+
+static uint32_t vmw_sou_bo_populate_clip(struct vmw_du_update_plane  *update,
+					 void *cmd, struct drm_rect *clip,
+					 uint32_t fb_x, uint32_t fb_y)
+{
+	struct vmw_kms_sou_bo_blit *blit = cmd;
+
+	blit->header = SVGA_CMD_BLIT_GMRFB_TO_SCREEN;
+	blit->body.destScreenId = update->du->unit;
+	blit->body.srcOrigin.x = fb_x;
+	blit->body.srcOrigin.y = fb_y;
+	blit->body.destRect.left = clip->x1;
+	blit->body.destRect.top = clip->y1;
+	blit->body.destRect.right = clip->x2;
+	blit->body.destRect.bottom = clip->y2;
+
+	return sizeof(*blit);
+}
+
+static uint32_t vmw_stud_bo_post_clip(struct vmw_du_update_plane  *update,
+				      void *cmd, struct drm_rect *bb)
+{
+	return 0;
+}
+
+static void vmw_sou_bo_finish(struct vmw_du_update_plane *update)
+{
+	struct vmw_framebuffer_bo *vfbbo =
+		container_of(update->vfb, typeof(*vfbbo), base);
+
+	vmw_kms_helper_buffer_finish(update->dev_priv, NULL, vfbbo->buffer,
+				     update->out_fence, NULL);
+}
+
+static void vmw_sou_bo_revert(struct vmw_du_update_plane *update)
+{
+	struct vmw_framebuffer_bo *vfbbo =
+		container_of(update->vfb, typeof(*vfbbo), base);
+
+	vmw_kms_helper_buffer_revert(vfbbo->buffer);
+}
+
+/**
+ * vmw_sou_plane_update_bo - update display unit for bo backed fb
+ * @dev_priv: device private
+ * @plane: plane state
+ * @old_state: old plane state
+ * @vfb: framebuffer which is blitted to display unit
+ * @out_fence: (optional) If non-NULL, will return a ref-counted pointer to a
+ * struct vmw_fence_obj. The returned fence pointer may be NULL in which case
+ * the device has already synchronized.
+ *
+ * RETURNS:
+ *
+ * 0 on success or a negative error code on failure.
+ */
+static int vmw_sou_plane_update_bo(struct vmw_private *dev_priv,
+				   struct drm_plane *plane,
+				   struct drm_plane_state *old_state,
+				   struct vmw_framebuffer *vfb,
+				   struct vmw_fence_obj **out_fence)
+{
+	struct vmw_du_update_plane_buffer bo_update;
+
+	memset(&bo_update, 0, sizeof(struct vmw_du_update_plane_buffer));
+	bo_update.base.plane = plane;
+	bo_update.base.old_state = old_state;
+	bo_update.base.dev_priv = dev_priv;
+	bo_update.base.du = vmw_crtc_to_du(plane->state->crtc);
+	bo_update.base.vfb = vfb;
+	bo_update.base.out_fence = out_fence;
+
+	bo_update.cpu_blit = false;
+
+	bo_update.base.prepare = vmw_sou_bo_prepare;
+	bo_update.base.calc_fifo_size = vmw_sou_bo_fifo_size;
+	bo_update.base.post_prepare = vmw_sou_bo_define_gmrfb;
+	bo_update.base.clip = vmw_sou_bo_populate_clip;
+	bo_update.base.post_clip = vmw_stud_bo_post_clip;
+	bo_update.base.finish = vmw_sou_bo_finish;
+	bo_update.base.revert = vmw_sou_bo_revert;
+
+	return vmw_du_helper_plane_update(&bo_update.base);
+}
+
 static int vmw_sou_surface_prepare(struct vmw_du_update_plane *update)
 {
 	struct vmw_du_update_plane_surface *srf_update;
-- 
2.17.1

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

[PATCH 12/14] drm/vmwgfx: use the new interface for SOU plane update

[Deepak Rawat]

With new interface to do plane update on SOU available, use that instead
of old kms_dirty. Updated the comments to sync with code.

Signed-off-by: Deepak Rawat <drawat@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 38 +++++++---------------------
 1 file changed, 9 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
index a68a2c8dab47..290d550d1b77 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
@@ -830,52 +830,32 @@ vmw_sou_primary_plane_atomic_update(struct drm_plane *plane,
 	struct vmw_fence_obj *fence = NULL;
 	int ret;
 
+	 /* If somehow gets a device error maintain consistent atomic state */
 	if (crtc && plane->state->fb) {
 		struct vmw_private *dev_priv = vmw_priv(crtc->dev);
 		struct vmw_framebuffer *vfb =
 			vmw_framebuffer_to_vfb(plane->state->fb);
-		struct drm_vmw_rect vclips;
-
-		vclips.x = crtc->x;
-		vclips.y = crtc->y;
-		vclips.w = crtc->mode.hdisplay;
-		vclips.h = crtc->mode.vdisplay;
 
 		if (vfb->bo)
-			ret = vmw_kms_sou_do_bo_dirty(dev_priv, vfb, NULL,
-						      &vclips, 1, 1, true,
-						      &fence, crtc);
+			ret = vmw_sou_plane_update_bo(dev_priv, plane,
+						      old_state, vfb, &fence);
 		else
-			ret = vmw_kms_sou_do_surface_dirty(dev_priv, vfb, NULL,
-							   &vclips, NULL, 0, 0,
-							   1, 1, &fence, crtc);
-
-		/*
-		 * We cannot really fail this function, so if we do, then output
-		 * an error and maintain consistent atomic state.
-		 */
+			ret = vmw_sou_plane_update_surface(dev_priv, plane,
+							   old_state, vfb,
+							   &fence);
 		if (ret != 0)
 			DRM_ERROR("Failed to update screen.\n");
 	} else {
-		/*
-		 * When disabling a plane, CRTC and FB should always be NULL
-		 * together, otherwise it's an error.
-		 * Here primary plane is being disable so should really blank
-		 * the screen object display unit, if not already done.
-		 */
+		/* Do nothing when fb and crtc is NULL (blank crtc) */
 		return;
 	}
 
+	/* For error case vblank event is sent from vmw_du_crtc_atomic_flush */
 	event = crtc->state->event;
-	/*
-	 * In case of failure and other cases, vblank event will be sent in
-	 * vmw_du_crtc_atomic_flush.
-	 */
 	if (event && fence) {
 		struct drm_file *file_priv = event->base.file_priv;
 
-		ret = vmw_event_fence_action_queue(file_priv,
-						   fence,
+		ret = vmw_event_fence_action_queue(file_priv, fence,
 						   &event->base,
 						   &event->event.vbl.tv_sec,
 						   &event->event.vbl.tv_usec,
-- 
2.17.1

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

[PATCH 13/14] drm/vmwgfx: enable FB_DAMAGE_CLIPS property for SOU primary plane

[Deepak Rawat]

SOU primary plane now support damage clips, enable it for user-space.

Signed-off-by: Deepak Rawat <drawat@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
index 290d550d1b77..c4966652cdb7 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
@@ -29,6 +29,7 @@
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_damage_helper.h>
 
 
 #define vmw_crtc_to_sou(x) \
@@ -967,6 +968,7 @@ static int vmw_sou_init(struct vmw_private *dev_priv, unsigned unit)
 	}
 
 	drm_plane_helper_add(primary, &vmw_sou_primary_plane_helper_funcs);
+	drm_plane_enable_fb_damage_clips(primary);
 
 	/* Initialize cursor plane */
 	vmw_du_plane_reset(cursor);
-- 
2.17.1

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

[PATCH 14/14] drm/vmwgfx: use atomic helper function for dirty fb IOCTL

[Deepak Rawat]

With new atomic helper for dirty fb IOCTL is available which uses damage
interface, use that for dirty fb IOCTL.

Signed-off-by: Deepak Rawat <drawat@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 116 +---------------------------
 1 file changed, 2 insertions(+), 114 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index c1de8f609bf4..adf3227fb0dc 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -850,58 +850,6 @@ static void vmw_framebuffer_surface_destroy(struct drm_framebuffer *framebuffer)
 	kfree(vfbs);
 }
 
-static int vmw_framebuffer_surface_dirty(struct drm_framebuffer *framebuffer,
-				  struct drm_file *file_priv,
-				  unsigned flags, unsigned color,
-				  struct drm_clip_rect *clips,
-				  unsigned num_clips)
-{
-	struct vmw_private *dev_priv = vmw_priv(framebuffer->dev);
-	struct vmw_framebuffer_surface *vfbs =
-		vmw_framebuffer_to_vfbs(framebuffer);
-	struct drm_clip_rect norect;
-	int ret, inc = 1;
-
-	/* Legacy Display Unit does not support 3D */
-	if (dev_priv->active_display_unit == vmw_du_legacy)
-		return -EINVAL;
-
-	drm_modeset_lock_all(dev_priv->dev);
-
-	ret = ttm_read_lock(&dev_priv->reservation_sem, true);
-	if (unlikely(ret != 0)) {
-		drm_modeset_unlock_all(dev_priv->dev);
-		return ret;
-	}
-
-	if (!num_clips) {
-		num_clips = 1;
-		clips = &norect;
-		norect.x1 = norect.y1 = 0;
-		norect.x2 = framebuffer->width;
-		norect.y2 = framebuffer->height;
-	} else if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) {
-		num_clips /= 2;
-		inc = 2; /* skip source rects */
-	}
-
-	if (dev_priv->active_display_unit == vmw_du_screen_object)
-		ret = vmw_kms_sou_do_surface_dirty(dev_priv, &vfbs->base,
-						   clips, NULL, NULL, 0, 0,
-						   num_clips, inc, NULL, NULL);
-	else
-		ret = vmw_kms_stdu_surface_dirty(dev_priv, &vfbs->base,
-						 clips, NULL, NULL, 0, 0,
-						 num_clips, inc, NULL, NULL);
-
-	vmw_fifo_flush(dev_priv, false);
-	ttm_read_unlock(&dev_priv->reservation_sem);
-
-	drm_modeset_unlock_all(dev_priv->dev);
-
-	return 0;
-}
-
 /**
  * vmw_kms_readback - Perform a readback from the screen system to
  * a buffer-object backed framebuffer.
@@ -945,7 +893,7 @@ int vmw_kms_readback(struct vmw_private *dev_priv,
 
 static const struct drm_framebuffer_funcs vmw_framebuffer_surface_funcs = {
 	.destroy = vmw_framebuffer_surface_destroy,
-	.dirty = vmw_framebuffer_surface_dirty,
+	.dirty = drm_atomic_helper_dirtyfb,
 };
 
 static int vmw_kms_new_framebuffer_surface(struct vmw_private *dev_priv,
@@ -1056,69 +1004,9 @@ static void vmw_framebuffer_bo_destroy(struct drm_framebuffer *framebuffer)
 	kfree(vfbd);
 }
 
-static int vmw_framebuffer_bo_dirty(struct drm_framebuffer *framebuffer,
-				    struct drm_file *file_priv,
-				    unsigned int flags, unsigned int color,
-				    struct drm_clip_rect *clips,
-				    unsigned int num_clips)
-{
-	struct vmw_private *dev_priv = vmw_priv(framebuffer->dev);
-	struct vmw_framebuffer_bo *vfbd =
-		vmw_framebuffer_to_vfbd(framebuffer);
-	struct drm_clip_rect norect;
-	int ret, increment = 1;
-
-	drm_modeset_lock_all(dev_priv->dev);
-
-	ret = ttm_read_lock(&dev_priv->reservation_sem, true);
-	if (unlikely(ret != 0)) {
-		drm_modeset_unlock_all(dev_priv->dev);
-		return ret;
-	}
-
-	if (!num_clips) {
-		num_clips = 1;
-		clips = &norect;
-		norect.x1 = norect.y1 = 0;
-		norect.x2 = framebuffer->width;
-		norect.y2 = framebuffer->height;
-	} else if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) {
-		num_clips /= 2;
-		increment = 2;
-	}
-
-	switch (dev_priv->active_display_unit) {
-	case vmw_du_screen_target:
-		ret = vmw_kms_stdu_dma(dev_priv, NULL, &vfbd->base, NULL,
-				       clips, NULL, num_clips, increment,
-				       true, true, NULL);
-		break;
-	case vmw_du_screen_object:
-		ret = vmw_kms_sou_do_bo_dirty(dev_priv, &vfbd->base,
-					      clips, NULL, num_clips,
-					      increment, true, NULL, NULL);
-		break;
-	case vmw_du_legacy:
-		ret = vmw_kms_ldu_do_bo_dirty(dev_priv, &vfbd->base, 0, 0,
-					      clips, num_clips, increment);
-		break;
-	default:
-		ret = -EINVAL;
-		WARN_ONCE(true, "Dirty called with invalid display system.\n");
-		break;
-	}
-
-	vmw_fifo_flush(dev_priv, false);
-	ttm_read_unlock(&dev_priv->reservation_sem);
-
-	drm_modeset_unlock_all(dev_priv->dev);
-
-	return ret;
-}
-
 static const struct drm_framebuffer_funcs vmw_framebuffer_bo_funcs = {
 	.destroy = vmw_framebuffer_bo_destroy,
-	.dirty = vmw_framebuffer_bo_dirty,
+	.dirty = drm_atomic_helper_dirtyfb,
 };
 
 /**
-- 
2.17.1

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

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

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/14] drm: add helper iterator functions for plane fb_damage_clips blob

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 03/14] drm: clear plane damage during full modeset

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/14] drm: add helper to implement legacy dirtyfb

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

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

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

Re: [PATCH 03/14] drm: clear plane damage during full modeset

[Ville Syrjälä]

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

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

Re: [PATCH 03/14] drm: clear plane damage during full modeset

[Daniel Vetter]

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

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 03/14] drm: clear plane damage during full modeset

[Ville Syrjälä]

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

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

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

[Deepak Singh Rawat]

> >
> > +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&amp;data=02%7C01%7Cdrawat%40vmware.com%7C0f5c199805444f2
> 6d01508d613cc5abe%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C
> 636718165752004800&amp;sdata=zKJnKe90%2BMeBsSpb4t4HfkOlLYkgXkyxsc
> GwVOHCyro%3D&amp;reserved=0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH 02/14] drm: add helper iterator functions for plane fb_damage_clips blob

[Deepak Singh Rawat]

> >  #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 ?

> 
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH 03/14] drm: clear plane damage during full modeset

[Deepak Singh Rawat]

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

Yes I agree this is not the best place but it was the one with minimal code
change. IMO should have a separate function for clearing damage.

> >  	}
> >  }
> >
> > 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:

Agreed.

> 
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Deepak Singh Rawat]

> 
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 1966 bytes --]

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

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

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

[Ville Syrjälä]

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&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

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

Re: [PATCH 02/14] drm: add helper iterator functions for plane fb_damage_clips blob

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 05/14] drm/vmwgfx: add a new interface for plane update on a display unit

[Thomas Hellstrom]

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.

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

Re: [PATCH 08/14] drm/vmwgfx: use the new interface for STDU plane update

[Thomas Hellstrom]

On 09/06/2018 01:38 AM, Deepak Rawat wrote:
> With new interface to do plane update on STDU available, use that
> instead of old kms_dirty. Update the commet to sync with code.
s/commet/comment/
>
> Signed-off-by: Deepak Rawat <drawat@vmware.com>
> ---
>   drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 38 +++++++---------------------
>   1 file changed, 9 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> index 8c1ba874512a..167190b75e2f 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> @@ -1706,14 +1706,10 @@ vmw_stdu_primary_plane_atomic_update(struct drm_plane *plane,
>   	struct vmw_private *dev_priv;
>   	int ret;
>   
> -	/*
> -	 * We cannot really fail this function, so if we do, then output an
> -	 * error and maintain consistent atomic state.
> -	 */
> +	/* If somehow gets a device error, maintain consistent atomic state */
>   	if (crtc && plane->state->fb) {
>   		struct vmw_framebuffer *vfb =
>   			vmw_framebuffer_to_vfb(plane->state->fb);
> -		struct drm_vmw_rect vclips;
>   		stdu = vmw_crtc_to_stdu(crtc);
>   		dev_priv = vmw_priv(crtc->dev);
>   
> @@ -1721,23 +1717,17 @@ vmw_stdu_primary_plane_atomic_update(struct drm_plane *plane,
>   		stdu->content_fb_type = vps->content_fb_type;
>   		stdu->cpp = vps->cpp;
>   
> -		vclips.x = crtc->x;
> -		vclips.y = crtc->y;
> -		vclips.w = crtc->mode.hdisplay;
> -		vclips.h = crtc->mode.vdisplay;
> -
>   		ret = vmw_stdu_bind_st(dev_priv, stdu, &stdu->display_srf->res);
>   		if (ret)
>   			DRM_ERROR("Failed to bind surface to STDU.\n");
>   
>   		if (vfb->bo)
> -			ret = vmw_kms_stdu_dma(dev_priv, NULL, vfb, NULL, NULL,
> -					       &vclips, 1, 1, true, false,
> -					       crtc);
> +			ret = vmw_stdu_plane_update_bo(dev_priv, plane,
> +						       old_state, vfb);
>   		else
> -			ret = vmw_kms_stdu_surface_dirty(dev_priv, vfb, NULL,
> -							 &vclips, NULL, 0, 0,
> -							 1, 1, NULL, crtc);
> +			ret = vmw_stdu_plane_update_surface(dev_priv, plane,
> +							    old_state, vfb,
> +							    NULL);
>   		if (ret)
>   			DRM_ERROR("Failed to update STDU.\n");
>   	} else {
> @@ -1745,12 +1735,7 @@ vmw_stdu_primary_plane_atomic_update(struct drm_plane *plane,
>   		stdu = vmw_crtc_to_stdu(crtc);
>   		dev_priv = vmw_priv(crtc->dev);
>   
> -		/*
> -		 * When disabling a plane, CRTC and FB should always be NULL
> -		 * together, otherwise it's an error.
> -		 * Here primary plane is being disable so blank the screen
> -		 * target display unit, if not already done.
> -		 */
> +		/* Blank STDU when fb and crtc are NULL */
>   		if (!stdu->defined)
>   			return;
>   
> @@ -1765,20 +1750,15 @@ vmw_stdu_primary_plane_atomic_update(struct drm_plane *plane,
>   		return;
>   	}
>   
> +	/* In case of error vblank event is sent in vmw_du_crtc_atomic_flush */
>   	event = crtc->state->event;
> -	/*
> -	 * In case of failure and other cases, vblank event will be sent in
> -	 * vmw_du_crtc_atomic_flush.
> -	 */
>   	if (event && (ret == 0)) {
>   		struct vmw_fence_obj *fence = NULL;
>   		struct drm_file *file_priv = event->base.file_priv;
>   
>   		vmw_execbuf_fence_commands(NULL, dev_priv, &fence, NULL);
>   
> -		/*
> -		 * If fence is NULL, then already sync.
> -		 */
> +		/* If fence is NULL, then already sync. */

If comment changes are not directly related to the functional changes, 
please use a separate patch for them.

>   		if (fence) {
>   			ret = vmw_event_fence_action_queue(
>   				file_priv, fence, &event->base,

Thanks,
Thomas


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

Re: [PATCH 09/14] drm/vmwgfx: enable FB_DAMAGE_CLIPS property for STDU primary plane

[Thomas Hellstrom]

On 09/06/2018 01:38 AM, Deepak Rawat wrote:
> STDU primary plane now support damage clips, enable it for user-space.
>
> Signed-off-by: Deepak Rawat <drawat@vmware.com>
> ---
>   drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> index 167190b75e2f..11a76aa96302 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> @@ -1883,6 +1883,7 @@ static int vmw_stdu_init(struct vmw_private *dev_priv, unsigned unit)
>   	}
>   
>   	drm_plane_helper_add(primary, &vmw_stdu_primary_plane_helper_funcs);
> +	drm_plane_enable_fb_damage_clips(primary);

LGTM. We should, as a second pass, have a look at cursors as well, to 
avoid having to send the cursor image on each cursor position update..

>   
>   	/* Initialize cursor plane */
>   	vmw_du_plane_reset(cursor);

/Thomas


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

Re: [PATCH 05/14] drm/vmwgfx: add a new interface for plane update on a display unit

[Jani Nikula]

On Mon, 10 Sep 2018, Thomas Hellstrom <thellstrom@vmware.com> wrote:
> On 09/06/2018 01:38 AM, Deepak Rawat wrote:
>> +/**
>> + * 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

Up-to-date version at [1].

BR,
Jani.


[1] https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html


-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Deepak Singh Rawat]

> 
> 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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Deepak Singh Rawat]

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

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