[PATCH V3 0/5] drm/vkms: Introduces writeback support

[PATCH V3 0/5] drm/vkms: Introduces writeback support

[Rodrigo Siqueira]

This is the V3 version of a series that introduces the writeback support
to vkms. As a result of the previous review, this patchset can be seen
in three parts: make vkms able to support multiple connector, pre-work
for vkms, and the vkms implementation. Follows the details:

* First part: The first patch of this series is a fix that enables vkms to
accept new connectors, such as writeback connector.

* Second part: The second part of this patchset starts on patch 02 and
finish on patch 04; basically it is a pre-work that aims to make vkms
composer operations a little bit more generic. These patches update the
CRC files and function to make it work as a composer; it also
centralizes the vkms framebuffer operations. Additionally, these changes
enable the composer to use the writeback framebuffer instead of creating
a copy.

* Third part: The final patch enables the support for writeback in vkms.

With this patchset, vkms can successfully pass all the kms_writeback
tests from IGT.

Note: Most of the changes in the V3 was suggested by Daniel Vetter as
can be seen at the link
https://patchwork.freedesktop.org/patch/311844/?series=61738&rev=2

Note: This patchset depends on Daniel's rework of CRC, see it at
https://patchwork.freedesktop.org/series/61737/

Rodrigo Siqueira (5):
  drm/vkms: Avoid assigning 0 for possible_crtc
  drm/vkms: Rename vkms_crc.c into vkms_composer.c
  drm/vkms: Decouple crc operations from composer
  drm/vkms: Compute CRC without change input data
  drm/vkms: Add support for writeback

 drivers/gpu/drm/vkms/Makefile                 |   9 +-
 .../drm/vkms/{vkms_crc.c => vkms_composer.c}  | 174 ++++++++++--------
 drivers/gpu/drm/vkms/vkms_crtc.c              |  30 +--
 drivers/gpu/drm/vkms/vkms_drv.c               |  10 +-
 drivers/gpu/drm/vkms/vkms_drv.h               |  40 ++--
 drivers/gpu/drm/vkms/vkms_output.c            |  16 +-
 drivers/gpu/drm/vkms/vkms_plane.c             |  40 ++--
 drivers/gpu/drm/vkms/vkms_writeback.c         | 141 ++++++++++++++
 8 files changed, 331 insertions(+), 129 deletions(-)
 rename drivers/gpu/drm/vkms/{vkms_crc.c => vkms_composer.c} (51%)
 create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c

-- 
2.21.0


-- 
Rodrigo Siqueira
https://siqueira.tech

[PATCH V3 1/5] drm/vkms: Avoid assigning 0 for possible_crtc

[Rodrigo Siqueira]

When vkms invoke drm_universal_plane_init(), it sets 0 for
possible_crtcs parameter which means that planes can't be attached to
any CRTC. It currently works due to some safeguard in the drm_crtc file;
however, it is possible to identify the problem by trying to append a
second connector. This patch fixes this issue by modifying
vkms_plane_init() to accept an index parameter which makes the code a
little bit more flexible and avoid set zero to possible_crtcs.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_drv.c    | 2 +-
 drivers/gpu/drm/vkms/vkms_drv.h    | 4 ++--
 drivers/gpu/drm/vkms/vkms_output.c | 6 +++---
 drivers/gpu/drm/vkms/vkms_plane.c  | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index cc53ef88a331..966b3d653189 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -127,7 +127,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
 	dev->mode_config.preferred_depth = 24;
 	dev->mode_config.helper_private = &vkms_mode_config_helpers;
 
-	return vkms_output_init(vkmsdev);
+	return vkms_output_init(vkmsdev, 0);
 }
 
 static int __init vkms_init(void)
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 12b4db7ac641..e2d1aa089dec 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -115,10 +115,10 @@ bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
 			       int *max_error, ktime_t *vblank_time,
 			       bool in_vblank_irq);
 
-int vkms_output_init(struct vkms_device *vkmsdev);
+int vkms_output_init(struct vkms_device *vkmsdev, int index);
 
 struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
-				  enum drm_plane_type type);
+				  enum drm_plane_type type, int index);
 
 /* Gem stuff */
 struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index 56fb5c2a2315..fb1941a6522c 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -35,7 +35,7 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
 	.get_modes    = vkms_conn_get_modes,
 };
 
-int vkms_output_init(struct vkms_device *vkmsdev)
+int vkms_output_init(struct vkms_device *vkmsdev, int index)
 {
 	struct vkms_output *output = &vkmsdev->output;
 	struct drm_device *dev = &vkmsdev->drm;
@@ -45,12 +45,12 @@ int vkms_output_init(struct vkms_device *vkmsdev)
 	struct drm_plane *primary, *cursor = NULL;
 	int ret;
 
-	primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY);
+	primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
 	if (IS_ERR(primary))
 		return PTR_ERR(primary);
 
 	if (enable_cursor) {
-		cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR);
+		cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index);
 		if (IS_ERR(cursor)) {
 			ret = PTR_ERR(cursor);
 			goto err_cursor;
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 0fceb6258422..18c630cfc485 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -176,7 +176,7 @@ static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
 };
 
 struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
-				  enum drm_plane_type type)
+				  enum drm_plane_type type, int index)
 {
 	struct drm_device *dev = &vkmsdev->drm;
 	const struct drm_plane_helper_funcs *funcs;
@@ -198,7 +198,7 @@ struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
 		funcs = &vkms_primary_helper_funcs;
 	}
 
-	ret = drm_universal_plane_init(dev, plane, 0,
+	ret = drm_universal_plane_init(dev, plane, 1 << index,
 				       &vkms_plane_funcs,
 				       formats, nformats,
 				       NULL, type, NULL);
-- 
2.21.0

[PATCH V3 2/5] drm/vkms: Rename vkms_crc.c into vkms_composer.c

[Rodrigo Siqueira]

As a preparation work for introducing writeback to vkms, this patch
renames the file vkms_crc.c into vkms_composer.c. Accordingly, it also
adjusts the functions and data structures to match the changes.

No functional change.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 drivers/gpu/drm/vkms/Makefile                 |  2 +-
 .../drm/vkms/{vkms_crc.c => vkms_composer.c}  | 98 ++++++++++---------
 drivers/gpu/drm/vkms/vkms_crtc.c              | 30 +++---
 drivers/gpu/drm/vkms/vkms_drv.c               |  4 +-
 drivers/gpu/drm/vkms/vkms_drv.h               | 28 +++---
 drivers/gpu/drm/vkms/vkms_plane.c             | 36 +++----
 6 files changed, 101 insertions(+), 97 deletions(-)
 rename drivers/gpu/drm/vkms/{vkms_crc.c => vkms_composer.c} (65%)

diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
index 89f09bec7b23..0b767d7efa24 100644
--- a/drivers/gpu/drm/vkms/Makefile
+++ b/drivers/gpu/drm/vkms/Makefile
@@ -1,4 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
-vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
+vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_composer.o
 
 obj-$(CONFIG_DRM_VKMS) += vkms.o
diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_composer.c
similarity index 65%
rename from drivers/gpu/drm/vkms/vkms_crc.c
rename to drivers/gpu/drm/vkms/vkms_composer.c
index 30b048b67a32..eb7ea8be1f98 100644
--- a/drivers/gpu/drm/vkms/vkms_crc.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -10,25 +10,25 @@
  * compute_crc - Compute CRC value on output frame
  *
  * @vaddr_out: address to final framebuffer
- * @crc_out: framebuffer's metadata
+ * @composer: framebuffer's metadata
  *
  * returns CRC value computed using crc32 on the visible portion of
  * the final framebuffer at vaddr_out
  */
-static uint32_t compute_crc(void *vaddr_out, struct vkms_crc_data *crc_out)
+static uint32_t compute_crc(void *vaddr_out, struct vkms_composer *composer)
 {
 	int i, j, src_offset;
-	int x_src = crc_out->src.x1 >> 16;
-	int y_src = crc_out->src.y1 >> 16;
-	int h_src = drm_rect_height(&crc_out->src) >> 16;
-	int w_src = drm_rect_width(&crc_out->src) >> 16;
+	int x_src = composer->src.x1 >> 16;
+	int y_src = composer->src.y1 >> 16;
+	int h_src = drm_rect_height(&composer->src) >> 16;
+	int w_src = drm_rect_width(&composer->src) >> 16;
 	u32 crc = 0;
 
 	for (i = y_src; i < y_src + h_src; ++i) {
 		for (j = x_src; j < x_src + w_src; ++j) {
-			src_offset = crc_out->offset
-				     + (i * crc_out->pitch)
-				     + (j * crc_out->cpp);
+			src_offset = composer->offset
+				     + (i * composer->pitch)
+				     + (j * composer->cpp);
 			/* XRGB format ignores Alpha channel */
 			memset(vaddr_out + src_offset + 24, 0,  8);
 			crc = crc32_le(crc, vaddr_out + src_offset,
@@ -43,8 +43,8 @@ static uint32_t compute_crc(void *vaddr_out, struct vkms_crc_data *crc_out)
  * blend - belnd value at vaddr_src with value at vaddr_dst
  * @vaddr_dst: destination address
  * @vaddr_src: source address
- * @crc_dst: destination framebuffer's metadata
- * @crc_src: source framebuffer's metadata
+ * @dest_composer: destination framebuffer's metadata
+ * @src_composer: source framebuffer's metadata
  *
  * Blend value at vaddr_src with value at vaddr_dst.
  * Currently, this function write value at vaddr_src on value
@@ -55,31 +55,31 @@ static uint32_t compute_crc(void *vaddr_out, struct vkms_crc_data *crc_out)
  *	 instead of overwriting it.
  */
 static void blend(void *vaddr_dst, void *vaddr_src,
-		  struct vkms_crc_data *crc_dst,
-		  struct vkms_crc_data *crc_src)
+		  struct vkms_composer *dest_composer,
+		  struct vkms_composer *src_composer)
 {
 	int i, j, j_dst, i_dst;
 	int offset_src, offset_dst;
 
-	int x_src = crc_src->src.x1 >> 16;
-	int y_src = crc_src->src.y1 >> 16;
+	int x_src = src_composer->src.x1 >> 16;
+	int y_src = src_composer->src.y1 >> 16;
 
-	int x_dst = crc_src->dst.x1;
-	int y_dst = crc_src->dst.y1;
-	int h_dst = drm_rect_height(&crc_src->dst);
-	int w_dst = drm_rect_width(&crc_src->dst);
+	int x_dst = src_composer->dst.x1;
+	int y_dst = src_composer->dst.y1;
+	int h_dst = drm_rect_height(&src_composer->dst);
+	int w_dst = drm_rect_width(&src_composer->dst);
 
 	int y_limit = y_src + h_dst;
 	int x_limit = x_src + w_dst;
 
 	for (i = y_src, i_dst = y_dst; i < y_limit; ++i) {
 		for (j = x_src, j_dst = x_dst; j < x_limit; ++j) {
-			offset_dst = crc_dst->offset
-				     + (i_dst * crc_dst->pitch)
-				     + (j_dst++ * crc_dst->cpp);
-			offset_src = crc_src->offset
-				     + (i * crc_src->pitch)
-				     + (j * crc_src->cpp);
+			offset_dst = dest_composer->offset
+				     + (i_dst * dest_composer->pitch)
+				     + (j_dst++ * dest_composer->cpp);
+			offset_src = src_composer->offset
+				     + (i * src_composer->pitch)
+				     + (j * src_composer->cpp);
 
 			memcpy(vaddr_dst + offset_dst,
 			       vaddr_src + offset_src, sizeof(u32));
@@ -88,25 +88,27 @@ static void blend(void *vaddr_dst, void *vaddr_src,
 	}
 }
 
-static void compose_cursor(struct vkms_crc_data *cursor_crc,
-			   struct vkms_crc_data *primary_crc, void *vaddr_out)
+static void compose_cursor(struct vkms_composer *cursor_composer,
+			   struct vkms_composer *primary_composer,
+			   void *vaddr_out)
 {
 	struct drm_gem_object *cursor_obj;
 	struct vkms_gem_object *cursor_vkms_obj;
 
-	cursor_obj = drm_gem_fb_get_obj(&cursor_crc->fb, 0);
+	cursor_obj = drm_gem_fb_get_obj(&cursor_composer->fb, 0);
 	cursor_vkms_obj = drm_gem_to_vkms_gem(cursor_obj);
 
 	if (WARN_ON(!cursor_vkms_obj->vaddr))
 		return;
 
-	blend(vaddr_out, cursor_vkms_obj->vaddr, primary_crc, cursor_crc);
+	blend(vaddr_out, cursor_vkms_obj->vaddr,
+	      primary_composer, cursor_composer);
 }
 
-static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
-			      struct vkms_crc_data *cursor_crc)
+static uint32_t _vkms_get_crc(struct vkms_composer *primary_composer,
+			      struct vkms_composer *cursor_composer)
 {
-	struct drm_framebuffer *fb = &primary_crc->fb;
+	struct drm_framebuffer *fb = &primary_composer->fb;
 	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
 	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(gem_obj);
 	void *vaddr_out = kzalloc(vkms_obj->gem.size, GFP_KERNEL);
@@ -124,10 +126,10 @@ static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
 
 	memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
 
-	if (cursor_crc)
-		compose_cursor(cursor_crc, primary_crc, vaddr_out);
+	if (cursor_composer)
+		compose_cursor(cursor_composer, primary_composer, vaddr_out);
 
-	crc = compute_crc(vaddr_out, primary_crc);
+	crc = compute_crc(vaddr_out, primary_composer);
 
 	kfree(vaddr_out);
 
@@ -135,35 +137,35 @@ static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
 }
 
 /**
- * vkms_crc_work_handle - ordered work_struct to compute CRC
+ * vkms_composer_worker - ordered work_struct to compute CRC
  *
  * @work: work_struct
  *
- * Work handler for computing CRCs. work_struct scheduled in
+ * Work handler for composing and computing CRCs. work_struct scheduled in
  * an ordered workqueue that's periodically scheduled to run by
  * _vblank_handle() and flushed at vkms_atomic_crtc_destroy_state().
  */
-void vkms_crc_work_handle(struct work_struct *work)
+void vkms_composer_worker(struct work_struct *work)
 {
 	struct vkms_crtc_state *crtc_state = container_of(work,
 						struct vkms_crtc_state,
-						crc_work);
+						composer_work);
 	struct drm_crtc *crtc = crtc_state->base.crtc;
 	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
-	struct vkms_crc_data *primary_crc = NULL;
-	struct vkms_crc_data *cursor_crc = NULL;
+	struct vkms_composer *primary_composer = NULL;
+	struct vkms_composer *cursor_composer = NULL;
 	u32 crc32 = 0;
 	u64 frame_start, frame_end;
 	bool crc_pending;
 
-	spin_lock_irq(&out->crc_lock);
+	spin_lock_irq(&out->composer_lock);
 	frame_start = crtc_state->frame_start;
 	frame_end = crtc_state->frame_end;
 	crc_pending = crtc_state->crc_pending;
 	crtc_state->frame_start = 0;
 	crtc_state->frame_end = 0;
 	crtc_state->crc_pending = false;
-	spin_unlock_irq(&out->crc_lock);
+	spin_unlock_irq(&out->composer_lock);
 
 	/*
 	 * We raced with the vblank hrtimer and previous work already computed
@@ -173,13 +175,13 @@ void vkms_crc_work_handle(struct work_struct *work)
 		return;
 
 	if (crtc_state->num_active_planes >= 1)
-		primary_crc = crtc_state->active_planes[0]->crc_data;
+		primary_composer = crtc_state->active_planes[0]->composer;
 
 	if (crtc_state->num_active_planes == 2)
-		cursor_crc = crtc_state->active_planes[1]->crc_data;
+		cursor_composer = crtc_state->active_planes[1]->composer;
 
-	if (primary_crc)
-		crc32 = _vkms_get_crc(primary_crc, cursor_crc);
+	if (primary_composer)
+		crc32 = _vkms_get_crc(primary_composer, cursor_composer);
 
 	/*
 	 * The worker can fall behind the vblank hrtimer, make sure we catch up.
@@ -237,7 +239,7 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name)
 	ret = vkms_crc_parse_source(src_name, &enabled);
 
 	spin_lock_irq(&out->lock);
-	out->crc_enabled = enabled;
+	out->composer_enabled = enabled;
 	spin_unlock_irq(&out->lock);
 
 	return ret;
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index a648892379c3..04c6f4250dea 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -24,14 +24,14 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
 	if (!ret)
 		DRM_ERROR("vkms failure on handling vblank");
 
-	state = output->crc_state;
-	if (state && output->crc_enabled) {
+	state = output->composer_state;
+	if (state && output->composer_enabled) {
 		u64 frame = drm_crtc_accurate_vblank_count(crtc);
 
-		/* update frame_start only if a queued vkms_crc_work_handle()
+		/* update frame_start only if a queued vkms_composer_worker()
 		 * has read the data
 		 */
-		spin_lock(&output->crc_lock);
+		spin_lock(&output->composer_lock);
 		if (!state->crc_pending)
 			state->frame_start = frame;
 		else
@@ -39,11 +39,11 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
 					 state->frame_start, frame);
 		state->frame_end = frame;
 		state->crc_pending = true;
-		spin_unlock(&output->crc_lock);
+		spin_unlock(&output->composer_lock);
 
-		ret = queue_work(output->crc_workq, &state->crc_work);
+		ret = queue_work(output->composer_workq, &state->composer_work);
 		if (!ret)
-			DRM_DEBUG_DRIVER("vkms_crc_work_handle already queued\n");
+			DRM_DEBUG_DRIVER("Composer worker already queued\n");
 	}
 
 	spin_unlock(&output->lock);
@@ -114,7 +114,7 @@ vkms_atomic_crtc_duplicate_state(struct drm_crtc *crtc)
 
 	__drm_atomic_helper_crtc_duplicate_state(crtc, &vkms_state->base);
 
-	INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
+	INIT_WORK(&vkms_state->composer_work, vkms_composer_worker);
 
 	return &vkms_state->base;
 }
@@ -126,7 +126,7 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
 
 	__drm_atomic_helper_crtc_destroy_state(state);
 
-	WARN_ON(work_pending(&vkms_state->crc_work));
+	WARN_ON(work_pending(&vkms_state->composer_work));
 	kfree(vkms_state->active_planes);
 	kfree(vkms_state);
 }
@@ -141,7 +141,7 @@ static void vkms_atomic_crtc_reset(struct drm_crtc *crtc)
 
 	__drm_atomic_helper_crtc_reset(crtc, &vkms_state->base);
 	if (vkms_state)
-		INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
+		INIT_WORK(&vkms_state->composer_work, vkms_composer_worker);
 }
 
 static const struct drm_crtc_funcs vkms_crtc_funcs = {
@@ -222,7 +222,7 @@ static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
 	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
 
 	/* This lock is held across the atomic commit to block vblank timer
-	 * from scheduling vkms_crc_work_handle until the crc_data is updated
+	 * from scheduling vkms_composer_worker until the composer is updated
 	 */
 	spin_lock_irq(&vkms_output->lock);
 }
@@ -245,7 +245,7 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
 		crtc->state->event = NULL;
 	}
 
-	vkms_output->crc_state = to_vkms_crtc_state(crtc->state);
+	vkms_output->composer_state = to_vkms_crtc_state(crtc->state);
 
 	spin_unlock_irq(&vkms_output->lock);
 }
@@ -274,10 +274,10 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 	drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
 
 	spin_lock_init(&vkms_out->lock);
-	spin_lock_init(&vkms_out->crc_lock);
+	spin_lock_init(&vkms_out->composer_lock);
 
-	vkms_out->crc_workq = alloc_ordered_workqueue("vkms_crc_workq", 0);
-	if (!vkms_out->crc_workq)
+	vkms_out->composer_workq = alloc_ordered_workqueue("vkms_composer", 0);
+	if (!vkms_out->composer_workq)
 		return -ENOMEM;
 
 	return ret;
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 966b3d653189..ac790b6527e4 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -56,7 +56,7 @@ static void vkms_release(struct drm_device *dev)
 	drm_atomic_helper_shutdown(&vkms->drm);
 	drm_mode_config_cleanup(&vkms->drm);
 	drm_dev_fini(&vkms->drm);
-	destroy_workqueue(vkms->output.crc_workq);
+	destroy_workqueue(vkms->output.composer_workq);
 }
 
 static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
@@ -82,7 +82,7 @@ static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
 		struct vkms_crtc_state *vkms_state =
 			to_vkms_crtc_state(old_crtc_state);
 
-		flush_work(&vkms_state->crc_work);
+		flush_work(&vkms_state->composer_work);
 	}
 
 	drm_atomic_helper_cleanup_planes(dev, old_state);
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index e2d1aa089dec..fc6cda164336 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -20,7 +20,7 @@
 
 extern bool enable_cursor;
 
-struct vkms_crc_data {
+struct vkms_composer {
 	struct drm_framebuffer fb;
 	struct drm_rect src, dst;
 	unsigned int offset;
@@ -31,29 +31,29 @@ struct vkms_crc_data {
 /**
  * vkms_plane_state - Driver specific plane state
  * @base: base plane state
- * @crc_data: data required for CRC computation
+ * @composer: data required for composing computation
  */
 struct vkms_plane_state {
 	struct drm_plane_state base;
-	struct vkms_crc_data *crc_data;
+	struct vkms_composer *composer;
 };
 
 /**
  * vkms_crtc_state - Driver specific CRTC state
  * @base: base CRTC state
- * @crc_work: work struct to compute and add CRC entries
+ * @composer_work: work struct to compose and add CRC entries
  * @n_frame_start: start frame number for computed CRC
  * @n_frame_end: end frame number for computed CRC
  */
 struct vkms_crtc_state {
 	struct drm_crtc_state base;
-	struct work_struct crc_work;
+	struct work_struct composer_work;
 
 	int num_active_planes;
 	/* stack of active planes for crc computation, should be in z order */
 	struct vkms_plane_state **active_planes;
 
-	/* below three are protected by vkms_output.crc_lock */
+	/* below three are protected by vkms_output.composer_lock */
 	bool crc_pending;
 	u64 frame_start;
 	u64 frame_end;
@@ -66,16 +66,16 @@ struct vkms_output {
 	struct hrtimer vblank_hrtimer;
 	ktime_t period_ns;
 	struct drm_pending_vblank_event *event;
-	/* ordered wq for crc_work */
-	struct workqueue_struct *crc_workq;
-	/* protects concurrent access to crc_data */
+	/* ordered wq for composer_work */
+	struct workqueue_struct *composer_workq;
+	/* protects concurrent access to composer */
 	spinlock_t lock;
 
 	/* protected by @lock */
-	bool crc_enabled;
-	struct vkms_crtc_state *crc_state;
+	bool composer_enabled;
+	struct vkms_crtc_state *composer_state;
 
-	spinlock_t crc_lock;
+	spinlock_t composer_lock;
 };
 
 struct vkms_device {
@@ -143,6 +143,8 @@ const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
 int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name);
 int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
 			   size_t *values_cnt);
-void vkms_crc_work_handle(struct work_struct *work);
+
+/* Composer Support */
+void vkms_composer_worker(struct work_struct *work);
 
 #endif /* _VKMS_DRV_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 18c630cfc485..8b60d3434d75 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -18,20 +18,20 @@ static struct drm_plane_state *
 vkms_plane_duplicate_state(struct drm_plane *plane)
 {
 	struct vkms_plane_state *vkms_state;
-	struct vkms_crc_data *crc_data;
+	struct vkms_composer *composer;
 
 	vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
 	if (!vkms_state)
 		return NULL;
 
-	crc_data = kzalloc(sizeof(*crc_data), GFP_KERNEL);
-	if (!crc_data) {
-		DRM_DEBUG_KMS("Couldn't allocate crc_data\n");
+	composer = kzalloc(sizeof(*composer), GFP_KERNEL);
+	if (!composer) {
+		DRM_DEBUG_KMS("Couldn't allocate composer\n");
 		kfree(vkms_state);
 		return NULL;
 	}
 
-	vkms_state->crc_data = crc_data;
+	vkms_state->composer = composer;
 
 	__drm_atomic_helper_plane_duplicate_state(plane,
 						  &vkms_state->base);
@@ -49,12 +49,12 @@ static void vkms_plane_destroy_state(struct drm_plane *plane,
 		/* dropping the reference we acquired in
 		 * vkms_primary_plane_update()
 		 */
-		if (drm_framebuffer_read_refcount(&vkms_state->crc_data->fb))
-			drm_framebuffer_put(&vkms_state->crc_data->fb);
+		if (drm_framebuffer_read_refcount(&vkms_state->composer->fb))
+			drm_framebuffer_put(&vkms_state->composer->fb);
 	}
 
-	kfree(vkms_state->crc_data);
-	vkms_state->crc_data = NULL;
+	kfree(vkms_state->composer);
+	vkms_state->composer = NULL;
 
 	__drm_atomic_helper_plane_destroy_state(old_state);
 	kfree(vkms_state);
@@ -91,21 +91,21 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
 {
 	struct vkms_plane_state *vkms_plane_state;
 	struct drm_framebuffer *fb = plane->state->fb;
-	struct vkms_crc_data *crc_data;
+	struct vkms_composer *composer;
 
 	if (!plane->state->crtc || !fb)
 		return;
 
 	vkms_plane_state = to_vkms_plane_state(plane->state);
 
-	crc_data = vkms_plane_state->crc_data;
-	memcpy(&crc_data->src, &plane->state->src, sizeof(struct drm_rect));
-	memcpy(&crc_data->dst, &plane->state->dst, sizeof(struct drm_rect));
-	memcpy(&crc_data->fb, fb, sizeof(struct drm_framebuffer));
-	drm_framebuffer_get(&crc_data->fb);
-	crc_data->offset = fb->offsets[0];
-	crc_data->pitch = fb->pitches[0];
-	crc_data->cpp = fb->format->cpp[0];
+	composer = vkms_plane_state->composer;
+	memcpy(&composer->src, &plane->state->src, sizeof(struct drm_rect));
+	memcpy(&composer->dst, &plane->state->dst, sizeof(struct drm_rect));
+	memcpy(&composer->fb, fb, sizeof(struct drm_framebuffer));
+	drm_framebuffer_get(&composer->fb);
+	composer->offset = fb->offsets[0];
+	composer->pitch = fb->pitches[0];
+	composer->cpp = fb->format->cpp[0];
 }
 
 static int vkms_plane_atomic_check(struct drm_plane *plane,
-- 
2.21.0

[PATCH V3 3/5] drm/vkms: Decouple crc operations from composer

[Rodrigo Siqueira]

In the vkms_composer.c, some of the functions related to CRC and compose
have interdependence between each other. This patch reworks some
functions inside vkms_composer to make crc and composer computation
decoupled.

This patch is preparation work for making vkms able to support new
features.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_composer.c | 49 ++++++++++++++++------------
 1 file changed, 29 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index eb7ea8be1f98..51a270514219 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -105,35 +105,31 @@ static void compose_cursor(struct vkms_composer *cursor_composer,
 	      primary_composer, cursor_composer);
 }
 
-static uint32_t _vkms_get_crc(struct vkms_composer *primary_composer,
-			      struct vkms_composer *cursor_composer)
+static int compose_planes(void **vaddr_out,
+			  struct vkms_composer *primary_composer,
+			  struct vkms_composer *cursor_composer)
 {
 	struct drm_framebuffer *fb = &primary_composer->fb;
 	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
 	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(gem_obj);
-	void *vaddr_out = kzalloc(vkms_obj->gem.size, GFP_KERNEL);
-	u32 crc = 0;
 
-	if (!vaddr_out) {
-		DRM_ERROR("Failed to allocate memory for output frame.");
-		return 0;
+	if (!*vaddr_out) {
+		*vaddr_out = kzalloc(vkms_obj->gem.size, GFP_KERNEL);
+		if (!*vaddr_out) {
+			DRM_ERROR("Cannot allocate memory for output frame.");
+			return -ENOMEM;
+		}
 	}
 
-	if (WARN_ON(!vkms_obj->vaddr)) {
-		kfree(vaddr_out);
-		return crc;
-	}
+	if (WARN_ON(!vkms_obj->vaddr))
+		return -EINVAL;
 
-	memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
+	memcpy(*vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
 
 	if (cursor_composer)
-		compose_cursor(cursor_composer, primary_composer, vaddr_out);
+		compose_cursor(cursor_composer, primary_composer, *vaddr_out);
 
-	crc = compute_crc(vaddr_out, primary_composer);
-
-	kfree(vaddr_out);
-
-	return crc;
+	return 0;
 }
 
 /**
@@ -154,9 +150,11 @@ void vkms_composer_worker(struct work_struct *work)
 	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
 	struct vkms_composer *primary_composer = NULL;
 	struct vkms_composer *cursor_composer = NULL;
+	void *vaddr_out = NULL;
 	u32 crc32 = 0;
 	u64 frame_start, frame_end;
 	bool crc_pending;
+	int ret;
 
 	spin_lock_irq(&out->composer_lock);
 	frame_start = crtc_state->frame_start;
@@ -180,14 +178,25 @@ void vkms_composer_worker(struct work_struct *work)
 	if (crtc_state->num_active_planes == 2)
 		cursor_composer = crtc_state->active_planes[1]->composer;
 
-	if (primary_composer)
-		crc32 = _vkms_get_crc(primary_composer, cursor_composer);
+	if (!primary_composer)
+		return;
+
+	ret = compose_planes(&vaddr_out, primary_composer, cursor_composer);
+	if (ret) {
+		if (ret == -EINVAL)
+			kfree(vaddr_out);
+		return;
+	}
+
+	crc32 = compute_crc(vaddr_out, primary_composer);
 
 	/*
 	 * The worker can fall behind the vblank hrtimer, make sure we catch up.
 	 */
 	while (frame_start <= frame_end)
 		drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
+
+	kfree(vaddr_out);
 }
 
 static const char * const pipe_crc_sources[] = {"auto"};
-- 
2.21.0

[PATCH V3 4/5] drm/vkms: Compute CRC without change input data

[Rodrigo Siqueira]

The compute_crc() function is responsible for calculating the
framebuffer CRC value; due to the XRGB format, this function has to
ignore the alpha channel during the CRC computation. Therefore,
compute_crc() set zero to the alpha channel directly in the input
framebuffer, which is not a problem since this function receives a copy
of the original buffer. However, if we want to use this function in a
context without a buffer copy, it will change the initial value. This
patch makes compute_crc() calculate the CRC value without modifying the
input framebuffer.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_composer.c | 31 +++++++++++++++++-----------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 51a270514219..8126aa0f968f 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -6,33 +6,40 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 
+static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
+				 const struct vkms_composer *composer)
+{
+	int src_offset = composer->offset + (y * composer->pitch)
+					  + (x * composer->cpp);
+
+	return *(u32 *)&buffer[src_offset];
+}
+
 /**
  * compute_crc - Compute CRC value on output frame
  *
- * @vaddr_out: address to final framebuffer
+ * @vaddr: address to final framebuffer
  * @composer: framebuffer's metadata
  *
  * returns CRC value computed using crc32 on the visible portion of
  * the final framebuffer at vaddr_out
  */
-static uint32_t compute_crc(void *vaddr_out, struct vkms_composer *composer)
+static uint32_t compute_crc(const u8 *vaddr,
+			    const struct vkms_composer *composer)
 {
-	int i, j, src_offset;
+	int x, y;
 	int x_src = composer->src.x1 >> 16;
 	int y_src = composer->src.y1 >> 16;
 	int h_src = drm_rect_height(&composer->src) >> 16;
 	int w_src = drm_rect_width(&composer->src) >> 16;
-	u32 crc = 0;
+	u32 crc = 0, pixel = 0;
 
-	for (i = y_src; i < y_src + h_src; ++i) {
-		for (j = x_src; j < x_src + w_src; ++j) {
-			src_offset = composer->offset
-				     + (i * composer->pitch)
-				     + (j * composer->cpp);
+	for (y = y_src; y < y_src + h_src; ++y) {
+		for (x = x_src; x < x_src + w_src; ++x) {
 			/* XRGB format ignores Alpha channel */
-			memset(vaddr_out + src_offset + 24, 0,  8);
-			crc = crc32_le(crc, vaddr_out + src_offset,
-				       sizeof(u32));
+			pixel = get_pixel_from_buffer(x, y, vaddr, composer);
+			bitmap_clear((void *)&pixel, 0, 8);
+			crc = crc32_le(crc, (void *)&pixel, sizeof(u32));
 		}
 	}
 
-- 
2.21.0

[PATCH V3 5/5] drm/vkms: Add support for writeback

[Rodrigo Siqueira]

This patch implements the necessary functions to add writeback support
for vkms. This feature is useful for testing compositors if you don't
have hardware with writeback support.

Change in V3 (Daniel):
- If writeback is enabled, compose everything into the writeback buffer
instead of CRC private buffer.
- Guarantees that the CRC will match exactly what we have in the
writeback buffer.

Change in V2:
- Rework signal completion (Brian)
- Integrates writeback with active_planes (Daniel)
- Compose cursor (Daniel)

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 drivers/gpu/drm/vkms/Makefile         |   9 +-
 drivers/gpu/drm/vkms/vkms_composer.c  |  16 ++-
 drivers/gpu/drm/vkms/vkms_drv.c       |   4 +
 drivers/gpu/drm/vkms/vkms_drv.h       |   8 ++
 drivers/gpu/drm/vkms/vkms_output.c    |  10 ++
 drivers/gpu/drm/vkms/vkms_writeback.c | 141 ++++++++++++++++++++++++++
 6 files changed, 185 insertions(+), 3 deletions(-)
 create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c

diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
index 0b767d7efa24..333d3cead0e3 100644
--- a/drivers/gpu/drm/vkms/Makefile
+++ b/drivers/gpu/drm/vkms/Makefile
@@ -1,4 +1,11 @@
 # SPDX-License-Identifier: GPL-2.0-only
-vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_composer.o
+vkms-y := \
+	vkms_drv.o \
+	vkms_plane.o \
+	vkms_output.o \
+	vkms_crtc.o \
+	vkms_gem.o \
+	vkms_composer.o \
+	vkms_writeback.o
 
 obj-$(CONFIG_DRM_VKMS) += vkms.o
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 8126aa0f968f..2317803e7320 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -157,16 +157,17 @@ void vkms_composer_worker(struct work_struct *work)
 	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
 	struct vkms_composer *primary_composer = NULL;
 	struct vkms_composer *cursor_composer = NULL;
+	bool crc_pending, wb_pending;
 	void *vaddr_out = NULL;
 	u32 crc32 = 0;
 	u64 frame_start, frame_end;
-	bool crc_pending;
 	int ret;
 
 	spin_lock_irq(&out->composer_lock);
 	frame_start = crtc_state->frame_start;
 	frame_end = crtc_state->frame_end;
 	crc_pending = crtc_state->crc_pending;
+	wb_pending = crtc_state->wb_pending;
 	crtc_state->frame_start = 0;
 	crtc_state->frame_end = 0;
 	crtc_state->crc_pending = false;
@@ -188,9 +189,12 @@ void vkms_composer_worker(struct work_struct *work)
 	if (!primary_composer)
 		return;
 
+	if (wb_pending)
+		vaddr_out = crtc_state->active_writeback;
+
 	ret = compose_planes(&vaddr_out, primary_composer, cursor_composer);
 	if (ret) {
-		if (ret == -EINVAL)
+		if (ret == -EINVAL && !wb_pending)
 			kfree(vaddr_out);
 		return;
 	}
@@ -203,6 +207,14 @@ void vkms_composer_worker(struct work_struct *work)
 	while (frame_start <= frame_end)
 		drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
 
+	if (wb_pending) {
+		drm_writeback_signal_completion(&out->wb_connector, 0);
+		spin_lock_irq(&out->composer_lock);
+		crtc_state->wb_pending = false;
+		spin_unlock_irq(&out->composer_lock);
+		return;
+	}
+
 	kfree(vaddr_out);
 }
 
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index ac790b6527e4..152d7de24a76 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -30,6 +30,10 @@ bool enable_cursor;
 module_param_named(enable_cursor, enable_cursor, bool, 0444);
 MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
 
+bool enable_writeback;
+module_param_named(enable_writeback, enable_writeback, bool, 0444);
+MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector");
+
 static const struct file_operations vkms_driver_fops = {
 	.owner		= THIS_MODULE,
 	.open		= drm_open,
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index fc6cda164336..9ff2cd4ebf81 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -7,6 +7,7 @@
 #include <drm/drm.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_encoder.h>
+#include <drm/drm_writeback.h>
 #include <linux/hrtimer.h>
 
 #define XRES_MIN    20
@@ -19,6 +20,7 @@
 #define YRES_MAX  8192
 
 extern bool enable_cursor;
+extern bool enable_writeback;
 
 struct vkms_composer {
 	struct drm_framebuffer fb;
@@ -52,9 +54,11 @@ struct vkms_crtc_state {
 	int num_active_planes;
 	/* stack of active planes for crc computation, should be in z order */
 	struct vkms_plane_state **active_planes;
+	void *active_writeback;
 
 	/* below three are protected by vkms_output.composer_lock */
 	bool crc_pending;
+	bool wb_pending;
 	u64 frame_start;
 	u64 frame_end;
 };
@@ -63,6 +67,7 @@ struct vkms_output {
 	struct drm_crtc crtc;
 	struct drm_encoder encoder;
 	struct drm_connector connector;
+	struct drm_writeback_connector wb_connector;
 	struct hrtimer vblank_hrtimer;
 	ktime_t period_ns;
 	struct drm_pending_vblank_event *event;
@@ -147,4 +152,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
 /* Composer Support */
 void vkms_composer_worker(struct work_struct *work);
 
+/* Writeback */
+int enable_writeback_connector(struct vkms_device *vkmsdev);
+
 #endif /* _VKMS_DRV_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index fb1941a6522c..aea1d4410864 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -84,6 +84,16 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
 		goto err_attach;
 	}
 
+	if (enable_writeback) {
+		ret = enable_writeback_connector(vkmsdev);
+		if (!ret) {
+			output->composer_enabled = true;
+			DRM_INFO("Writeback connector enabled");
+		} else {
+			DRM_ERROR("Failed to init writeback connector\n");
+		}
+	}
+
 	drm_mode_config_reset(dev);
 
 	return 0;
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
new file mode 100644
index 000000000000..34dad37a0236
--- /dev/null
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -0,0 +1,141 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include "vkms_drv.h"
+#include <drm/drm_writeback.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+
+static const u32 vkms_wb_formats[] = {
+	DRM_FORMAT_XRGB8888,
+};
+
+static const struct drm_connector_funcs vkms_wb_connector_funcs = {
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = drm_connector_cleanup,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
+					struct drm_crtc_state *crtc_state,
+					struct drm_connector_state *conn_state)
+{
+	struct drm_framebuffer *fb;
+	const struct drm_display_mode *mode = &crtc_state->mode;
+
+	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
+		return 0;
+
+	fb = conn_state->writeback_job->fb;
+	if (fb->width != mode->hdisplay || fb->height != mode->vdisplay) {
+		DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
+			      fb->width, fb->height);
+		return -EINVAL;
+	}
+
+	if (fb->format->format != DRM_FORMAT_XRGB8888) {
+		struct drm_format_name_buf format_name;
+
+		DRM_DEBUG_KMS("Invalid pixel format %s\n",
+			      drm_get_format_name(fb->format->format,
+						  &format_name));
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {
+	.atomic_check = vkms_wb_encoder_atomic_check,
+};
+
+static int vkms_wb_connector_get_modes(struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+
+	return drm_add_modes_noedid(connector, dev->mode_config.max_width,
+				    dev->mode_config.max_height);
+}
+
+static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
+			       struct drm_writeback_job *job)
+{
+	struct vkms_gem_object *vkms_obj;
+	struct drm_gem_object *gem_obj;
+	int ret;
+
+	if (!job->fb)
+		return 0;
+
+	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
+	ret = vkms_gem_vmap(gem_obj);
+	if (ret) {
+		DRM_ERROR("vmap failed: %d\n", ret);
+		return ret;
+	}
+
+	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
+	job->priv = vkms_obj->vaddr;
+
+	return 0;
+}
+
+static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
+				struct drm_writeback_job *job)
+{
+	struct drm_gem_object *gem_obj;
+
+	if (!job->fb)
+		return;
+
+	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
+	vkms_gem_vunmap(gem_obj);
+}
+
+static void vkms_wb_atomic_commit(struct drm_connector *conn,
+				  struct drm_connector_state *state)
+{
+	struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev);
+	struct vkms_output *output = &vkmsdev->output;
+	struct drm_writeback_connector *wb_conn = &output->wb_connector;
+	struct drm_connector_state *conn_state = wb_conn->base.state;
+	struct vkms_crtc_state *crtc_state = output->composer_state;
+
+	if (!conn_state)
+		return;
+
+	if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
+		DRM_DEBUG_DRIVER("Disable writeback\n");
+		return;
+	}
+
+	spin_lock_irq(&output->composer_lock);
+	crtc_state->active_writeback = conn_state->writeback_job->priv;
+	crtc_state->wb_pending = true;
+	spin_unlock_irq(&output->composer_lock);
+	drm_writeback_queue_job(wb_conn, state);
+}
+
+static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
+	.get_modes = vkms_wb_connector_get_modes,
+	.prepare_writeback_job = vkms_wb_prepare_job,
+	.cleanup_writeback_job = vkms_wb_cleanup_job,
+	.atomic_commit = vkms_wb_atomic_commit,
+};
+
+int enable_writeback_connector(struct vkms_device *vkmsdev)
+{
+	struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
+
+	vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
+	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
+
+	return drm_writeback_connector_init(&vkmsdev->drm, wb,
+					    &vkms_wb_connector_funcs,
+					    &vkms_wb_encoder_helper_funcs,
+					    vkms_wb_formats,
+					    ARRAY_SIZE(vkms_wb_formats));
+}
+
-- 
2.21.0

Re: [PATCH V3 4/5] drm/vkms: Compute CRC without change input data

[Vasilev, Oleg]

[-- Attachment #1: Type: text/plain, Size: 3346 bytes --]

On Tue, 2019-06-25 at 22:38 -0300, Rodrigo Siqueira wrote:
> The compute_crc() function is responsible for calculating the
> framebuffer CRC value; due to the XRGB format, this function has to
> ignore the alpha channel during the CRC computation. Therefore,
> compute_crc() set zero to the alpha channel directly in the input
> framebuffer, which is not a problem since this function receives a
> copy
> of the original buffer. However, if we want to use this function in a
> context without a buffer copy, it will change the initial value. This
> patch makes compute_crc() calculate the CRC value without modifying
> the
> input framebuffer.
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> ---
>  drivers/gpu/drm/vkms/vkms_composer.c | 31 +++++++++++++++++---------
> --
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
> b/drivers/gpu/drm/vkms/vkms_composer.c
> index 51a270514219..8126aa0f968f 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -6,33 +6,40 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
>  
> +static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
> +				 const struct vkms_composer *composer)
> +{
> +	int src_offset = composer->offset + (y * composer->pitch)
> +					  + (x * composer->cpp);
> +
> +	return *(u32 *)&buffer[src_offset];
> +}
> +
>  /**
>   * compute_crc - Compute CRC value on output frame
>   *
> - * @vaddr_out: address to final framebuffer
> + * @vaddr: address to final framebuffer
>   * @composer: framebuffer's metadata
>   *
>   * returns CRC value computed using crc32 on the visible portion of
>   * the final framebuffer at vaddr_out
>   */
> -static uint32_t compute_crc(void *vaddr_out, struct vkms_composer
> *composer)
> +static uint32_t compute_crc(const u8 *vaddr,
> +			    const struct vkms_composer *composer)
>  {
> -	int i, j, src_offset;
> +	int x, y;
>  	int x_src = composer->src.x1 >> 16;
>  	int y_src = composer->src.y1 >> 16;
>  	int h_src = drm_rect_height(&composer->src) >> 16;
>  	int w_src = drm_rect_width(&composer->src) >> 16;
> -	u32 crc = 0;
> +	u32 crc = 0, pixel = 0;
>  
> -	for (i = y_src; i < y_src + h_src; ++i) {
> -		for (j = x_src; j < x_src + w_src; ++j) {
> -			src_offset = composer->offset
> -				     + (i * composer->pitch)
> -				     + (j * composer->cpp);
> +	for (y = y_src; y < y_src + h_src; ++y) {
> +		for (x = x_src; x < x_src + w_src; ++x) {
>  			/* XRGB format ignores Alpha channel */
> -			memset(vaddr_out + src_offset + 24, 0,  8);
Hi Rodgrigo,

Do I understand correctly, that previous version with memset was
actually zeroing out the whole fb, except first 24 bytes? On the first
iteration bytes 24..32 would be zeroed, on the second 25..33, etc.

Should we add more CRC tests to IGT, so we can catch such mistakes? For
example, compute CRC, than augment random pixel and assert that CRC
changed.

Oleg
> -			crc = crc32_le(crc, vaddr_out + src_offset,
> -				       sizeof(u32));
> +			pixel = get_pixel_from_buffer(x, y, vaddr,
> composer);
> +			bitmap_clear((void *)&pixel, 0, 8);
> +			crc = crc32_le(crc, (void *)&pixel,
> sizeof(u32));
>  		}
>  	}
>  

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3261 bytes --]

Re: [PATCH V3 1/5] drm/vkms: Avoid assigning 0 for possible_crtc

[Daniel Vetter]

On Tue, Jun 25, 2019 at 10:36:18PM -0300, Rodrigo Siqueira wrote:
> When vkms invoke drm_universal_plane_init(), it sets 0 for
> possible_crtcs parameter which means that planes can't be attached to
> any CRTC. It currently works due to some safeguard in the drm_crtc file;
> however, it is possible to identify the problem by trying to append a
> second connector. This patch fixes this issue by modifying
> vkms_plane_init() to accept an index parameter which makes the code a
> little bit more flexible and avoid set zero to possible_crtcs.
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>

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

> ---
>  drivers/gpu/drm/vkms/vkms_drv.c    | 2 +-
>  drivers/gpu/drm/vkms/vkms_drv.h    | 4 ++--
>  drivers/gpu/drm/vkms/vkms_output.c | 6 +++---
>  drivers/gpu/drm/vkms/vkms_plane.c  | 4 ++--
>  4 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index cc53ef88a331..966b3d653189 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -127,7 +127,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
>  	dev->mode_config.preferred_depth = 24;
>  	dev->mode_config.helper_private = &vkms_mode_config_helpers;
>  
> -	return vkms_output_init(vkmsdev);
> +	return vkms_output_init(vkmsdev, 0);
>  }
>  
>  static int __init vkms_init(void)
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 12b4db7ac641..e2d1aa089dec 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -115,10 +115,10 @@ bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
>  			       int *max_error, ktime_t *vblank_time,
>  			       bool in_vblank_irq);
>  
> -int vkms_output_init(struct vkms_device *vkmsdev);
> +int vkms_output_init(struct vkms_device *vkmsdev, int index);
>  
>  struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> -				  enum drm_plane_type type);
> +				  enum drm_plane_type type, int index);
>  
>  /* Gem stuff */
>  struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index 56fb5c2a2315..fb1941a6522c 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -35,7 +35,7 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
>  	.get_modes    = vkms_conn_get_modes,
>  };
>  
> -int vkms_output_init(struct vkms_device *vkmsdev)
> +int vkms_output_init(struct vkms_device *vkmsdev, int index)
>  {
>  	struct vkms_output *output = &vkmsdev->output;
>  	struct drm_device *dev = &vkmsdev->drm;
> @@ -45,12 +45,12 @@ int vkms_output_init(struct vkms_device *vkmsdev)
>  	struct drm_plane *primary, *cursor = NULL;
>  	int ret;
>  
> -	primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY);
> +	primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
>  	if (IS_ERR(primary))
>  		return PTR_ERR(primary);
>  
>  	if (enable_cursor) {
> -		cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR);
> +		cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index);
>  		if (IS_ERR(cursor)) {
>  			ret = PTR_ERR(cursor);
>  			goto err_cursor;
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 0fceb6258422..18c630cfc485 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -176,7 +176,7 @@ static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
>  };
>  
>  struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> -				  enum drm_plane_type type)
> +				  enum drm_plane_type type, int index)
>  {
>  	struct drm_device *dev = &vkmsdev->drm;
>  	const struct drm_plane_helper_funcs *funcs;
> @@ -198,7 +198,7 @@ struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
>  		funcs = &vkms_primary_helper_funcs;
>  	}
>  
> -	ret = drm_universal_plane_init(dev, plane, 0,
> +	ret = drm_universal_plane_init(dev, plane, 1 << index,
>  				       &vkms_plane_funcs,
>  				       formats, nformats,
>  				       NULL, type, NULL);
> -- 
> 2.21.0

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

Re: [PATCH V3 2/5] drm/vkms: Rename vkms_crc.c into vkms_composer.c

[Daniel Vetter]

On Tue, Jun 25, 2019 at 10:37:05PM -0300, Rodrigo Siqueira wrote:
> As a preparation work for introducing writeback to vkms, this patch
> renames the file vkms_crc.c into vkms_composer.c. Accordingly, it also
> adjusts the functions and data structures to match the changes.
> 
> No functional change.
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Too lazy to check it still compiles :-)
-Daniel


> ---
>  drivers/gpu/drm/vkms/Makefile                 |  2 +-
>  .../drm/vkms/{vkms_crc.c => vkms_composer.c}  | 98 ++++++++++---------
>  drivers/gpu/drm/vkms/vkms_crtc.c              | 30 +++---
>  drivers/gpu/drm/vkms/vkms_drv.c               |  4 +-
>  drivers/gpu/drm/vkms/vkms_drv.h               | 28 +++---
>  drivers/gpu/drm/vkms/vkms_plane.c             | 36 +++----
>  6 files changed, 101 insertions(+), 97 deletions(-)
>  rename drivers/gpu/drm/vkms/{vkms_crc.c => vkms_composer.c} (65%)
> 
> diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> index 89f09bec7b23..0b767d7efa24 100644
> --- a/drivers/gpu/drm/vkms/Makefile
> +++ b/drivers/gpu/drm/vkms/Makefile
> @@ -1,4 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
> +vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_composer.o
>  
>  obj-$(CONFIG_DRM_VKMS) += vkms.o
> diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_composer.c
> similarity index 65%
> rename from drivers/gpu/drm/vkms/vkms_crc.c
> rename to drivers/gpu/drm/vkms/vkms_composer.c
> index 30b048b67a32..eb7ea8be1f98 100644
> --- a/drivers/gpu/drm/vkms/vkms_crc.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -10,25 +10,25 @@
>   * compute_crc - Compute CRC value on output frame
>   *
>   * @vaddr_out: address to final framebuffer
> - * @crc_out: framebuffer's metadata
> + * @composer: framebuffer's metadata
>   *
>   * returns CRC value computed using crc32 on the visible portion of
>   * the final framebuffer at vaddr_out
>   */
> -static uint32_t compute_crc(void *vaddr_out, struct vkms_crc_data *crc_out)
> +static uint32_t compute_crc(void *vaddr_out, struct vkms_composer *composer)
>  {
>  	int i, j, src_offset;
> -	int x_src = crc_out->src.x1 >> 16;
> -	int y_src = crc_out->src.y1 >> 16;
> -	int h_src = drm_rect_height(&crc_out->src) >> 16;
> -	int w_src = drm_rect_width(&crc_out->src) >> 16;
> +	int x_src = composer->src.x1 >> 16;
> +	int y_src = composer->src.y1 >> 16;
> +	int h_src = drm_rect_height(&composer->src) >> 16;
> +	int w_src = drm_rect_width(&composer->src) >> 16;
>  	u32 crc = 0;
>  
>  	for (i = y_src; i < y_src + h_src; ++i) {
>  		for (j = x_src; j < x_src + w_src; ++j) {
> -			src_offset = crc_out->offset
> -				     + (i * crc_out->pitch)
> -				     + (j * crc_out->cpp);
> +			src_offset = composer->offset
> +				     + (i * composer->pitch)
> +				     + (j * composer->cpp);
>  			/* XRGB format ignores Alpha channel */
>  			memset(vaddr_out + src_offset + 24, 0,  8);
>  			crc = crc32_le(crc, vaddr_out + src_offset,
> @@ -43,8 +43,8 @@ static uint32_t compute_crc(void *vaddr_out, struct vkms_crc_data *crc_out)
>   * blend - belnd value at vaddr_src with value at vaddr_dst
>   * @vaddr_dst: destination address
>   * @vaddr_src: source address
> - * @crc_dst: destination framebuffer's metadata
> - * @crc_src: source framebuffer's metadata
> + * @dest_composer: destination framebuffer's metadata
> + * @src_composer: source framebuffer's metadata
>   *
>   * Blend value at vaddr_src with value at vaddr_dst.
>   * Currently, this function write value at vaddr_src on value
> @@ -55,31 +55,31 @@ static uint32_t compute_crc(void *vaddr_out, struct vkms_crc_data *crc_out)
>   *	 instead of overwriting it.
>   */
>  static void blend(void *vaddr_dst, void *vaddr_src,
> -		  struct vkms_crc_data *crc_dst,
> -		  struct vkms_crc_data *crc_src)
> +		  struct vkms_composer *dest_composer,
> +		  struct vkms_composer *src_composer)
>  {
>  	int i, j, j_dst, i_dst;
>  	int offset_src, offset_dst;
>  
> -	int x_src = crc_src->src.x1 >> 16;
> -	int y_src = crc_src->src.y1 >> 16;
> +	int x_src = src_composer->src.x1 >> 16;
> +	int y_src = src_composer->src.y1 >> 16;
>  
> -	int x_dst = crc_src->dst.x1;
> -	int y_dst = crc_src->dst.y1;
> -	int h_dst = drm_rect_height(&crc_src->dst);
> -	int w_dst = drm_rect_width(&crc_src->dst);
> +	int x_dst = src_composer->dst.x1;
> +	int y_dst = src_composer->dst.y1;
> +	int h_dst = drm_rect_height(&src_composer->dst);
> +	int w_dst = drm_rect_width(&src_composer->dst);
>  
>  	int y_limit = y_src + h_dst;
>  	int x_limit = x_src + w_dst;
>  
>  	for (i = y_src, i_dst = y_dst; i < y_limit; ++i) {
>  		for (j = x_src, j_dst = x_dst; j < x_limit; ++j) {
> -			offset_dst = crc_dst->offset
> -				     + (i_dst * crc_dst->pitch)
> -				     + (j_dst++ * crc_dst->cpp);
> -			offset_src = crc_src->offset
> -				     + (i * crc_src->pitch)
> -				     + (j * crc_src->cpp);
> +			offset_dst = dest_composer->offset
> +				     + (i_dst * dest_composer->pitch)
> +				     + (j_dst++ * dest_composer->cpp);
> +			offset_src = src_composer->offset
> +				     + (i * src_composer->pitch)
> +				     + (j * src_composer->cpp);
>  
>  			memcpy(vaddr_dst + offset_dst,
>  			       vaddr_src + offset_src, sizeof(u32));
> @@ -88,25 +88,27 @@ static void blend(void *vaddr_dst, void *vaddr_src,
>  	}
>  }
>  
> -static void compose_cursor(struct vkms_crc_data *cursor_crc,
> -			   struct vkms_crc_data *primary_crc, void *vaddr_out)
> +static void compose_cursor(struct vkms_composer *cursor_composer,
> +			   struct vkms_composer *primary_composer,
> +			   void *vaddr_out)
>  {
>  	struct drm_gem_object *cursor_obj;
>  	struct vkms_gem_object *cursor_vkms_obj;
>  
> -	cursor_obj = drm_gem_fb_get_obj(&cursor_crc->fb, 0);
> +	cursor_obj = drm_gem_fb_get_obj(&cursor_composer->fb, 0);
>  	cursor_vkms_obj = drm_gem_to_vkms_gem(cursor_obj);
>  
>  	if (WARN_ON(!cursor_vkms_obj->vaddr))
>  		return;
>  
> -	blend(vaddr_out, cursor_vkms_obj->vaddr, primary_crc, cursor_crc);
> +	blend(vaddr_out, cursor_vkms_obj->vaddr,
> +	      primary_composer, cursor_composer);
>  }
>  
> -static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
> -			      struct vkms_crc_data *cursor_crc)
> +static uint32_t _vkms_get_crc(struct vkms_composer *primary_composer,
> +			      struct vkms_composer *cursor_composer)
>  {
> -	struct drm_framebuffer *fb = &primary_crc->fb;
> +	struct drm_framebuffer *fb = &primary_composer->fb;
>  	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
>  	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(gem_obj);
>  	void *vaddr_out = kzalloc(vkms_obj->gem.size, GFP_KERNEL);
> @@ -124,10 +126,10 @@ static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
>  
>  	memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
>  
> -	if (cursor_crc)
> -		compose_cursor(cursor_crc, primary_crc, vaddr_out);
> +	if (cursor_composer)
> +		compose_cursor(cursor_composer, primary_composer, vaddr_out);
>  
> -	crc = compute_crc(vaddr_out, primary_crc);
> +	crc = compute_crc(vaddr_out, primary_composer);
>  
>  	kfree(vaddr_out);
>  
> @@ -135,35 +137,35 @@ static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
>  }
>  
>  /**
> - * vkms_crc_work_handle - ordered work_struct to compute CRC
> + * vkms_composer_worker - ordered work_struct to compute CRC
>   *
>   * @work: work_struct
>   *
> - * Work handler for computing CRCs. work_struct scheduled in
> + * Work handler for composing and computing CRCs. work_struct scheduled in
>   * an ordered workqueue that's periodically scheduled to run by
>   * _vblank_handle() and flushed at vkms_atomic_crtc_destroy_state().
>   */
> -void vkms_crc_work_handle(struct work_struct *work)
> +void vkms_composer_worker(struct work_struct *work)
>  {
>  	struct vkms_crtc_state *crtc_state = container_of(work,
>  						struct vkms_crtc_state,
> -						crc_work);
> +						composer_work);
>  	struct drm_crtc *crtc = crtc_state->base.crtc;
>  	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
> -	struct vkms_crc_data *primary_crc = NULL;
> -	struct vkms_crc_data *cursor_crc = NULL;
> +	struct vkms_composer *primary_composer = NULL;
> +	struct vkms_composer *cursor_composer = NULL;
>  	u32 crc32 = 0;
>  	u64 frame_start, frame_end;
>  	bool crc_pending;
>  
> -	spin_lock_irq(&out->crc_lock);
> +	spin_lock_irq(&out->composer_lock);
>  	frame_start = crtc_state->frame_start;
>  	frame_end = crtc_state->frame_end;
>  	crc_pending = crtc_state->crc_pending;
>  	crtc_state->frame_start = 0;
>  	crtc_state->frame_end = 0;
>  	crtc_state->crc_pending = false;
> -	spin_unlock_irq(&out->crc_lock);
> +	spin_unlock_irq(&out->composer_lock);
>  
>  	/*
>  	 * We raced with the vblank hrtimer and previous work already computed
> @@ -173,13 +175,13 @@ void vkms_crc_work_handle(struct work_struct *work)
>  		return;
>  
>  	if (crtc_state->num_active_planes >= 1)
> -		primary_crc = crtc_state->active_planes[0]->crc_data;
> +		primary_composer = crtc_state->active_planes[0]->composer;
>  
>  	if (crtc_state->num_active_planes == 2)
> -		cursor_crc = crtc_state->active_planes[1]->crc_data;
> +		cursor_composer = crtc_state->active_planes[1]->composer;
>  
> -	if (primary_crc)
> -		crc32 = _vkms_get_crc(primary_crc, cursor_crc);
> +	if (primary_composer)
> +		crc32 = _vkms_get_crc(primary_composer, cursor_composer);
>  
>  	/*
>  	 * The worker can fall behind the vblank hrtimer, make sure we catch up.
> @@ -237,7 +239,7 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>  	ret = vkms_crc_parse_source(src_name, &enabled);
>  
>  	spin_lock_irq(&out->lock);
> -	out->crc_enabled = enabled;
> +	out->composer_enabled = enabled;
>  	spin_unlock_irq(&out->lock);
>  
>  	return ret;
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index a648892379c3..04c6f4250dea 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -24,14 +24,14 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
>  	if (!ret)
>  		DRM_ERROR("vkms failure on handling vblank");
>  
> -	state = output->crc_state;
> -	if (state && output->crc_enabled) {
> +	state = output->composer_state;
> +	if (state && output->composer_enabled) {
>  		u64 frame = drm_crtc_accurate_vblank_count(crtc);
>  
> -		/* update frame_start only if a queued vkms_crc_work_handle()
> +		/* update frame_start only if a queued vkms_composer_worker()
>  		 * has read the data
>  		 */
> -		spin_lock(&output->crc_lock);
> +		spin_lock(&output->composer_lock);
>  		if (!state->crc_pending)
>  			state->frame_start = frame;
>  		else
> @@ -39,11 +39,11 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
>  					 state->frame_start, frame);
>  		state->frame_end = frame;
>  		state->crc_pending = true;
> -		spin_unlock(&output->crc_lock);
> +		spin_unlock(&output->composer_lock);
>  
> -		ret = queue_work(output->crc_workq, &state->crc_work);
> +		ret = queue_work(output->composer_workq, &state->composer_work);
>  		if (!ret)
> -			DRM_DEBUG_DRIVER("vkms_crc_work_handle already queued\n");
> +			DRM_DEBUG_DRIVER("Composer worker already queued\n");
>  	}
>  
>  	spin_unlock(&output->lock);
> @@ -114,7 +114,7 @@ vkms_atomic_crtc_duplicate_state(struct drm_crtc *crtc)
>  
>  	__drm_atomic_helper_crtc_duplicate_state(crtc, &vkms_state->base);
>  
> -	INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
> +	INIT_WORK(&vkms_state->composer_work, vkms_composer_worker);
>  
>  	return &vkms_state->base;
>  }
> @@ -126,7 +126,7 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
>  
>  	__drm_atomic_helper_crtc_destroy_state(state);
>  
> -	WARN_ON(work_pending(&vkms_state->crc_work));
> +	WARN_ON(work_pending(&vkms_state->composer_work));
>  	kfree(vkms_state->active_planes);
>  	kfree(vkms_state);
>  }
> @@ -141,7 +141,7 @@ static void vkms_atomic_crtc_reset(struct drm_crtc *crtc)
>  
>  	__drm_atomic_helper_crtc_reset(crtc, &vkms_state->base);
>  	if (vkms_state)
> -		INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
> +		INIT_WORK(&vkms_state->composer_work, vkms_composer_worker);
>  }
>  
>  static const struct drm_crtc_funcs vkms_crtc_funcs = {
> @@ -222,7 +222,7 @@ static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
>  	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
>  
>  	/* This lock is held across the atomic commit to block vblank timer
> -	 * from scheduling vkms_crc_work_handle until the crc_data is updated
> +	 * from scheduling vkms_composer_worker until the composer is updated
>  	 */
>  	spin_lock_irq(&vkms_output->lock);
>  }
> @@ -245,7 +245,7 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
>  		crtc->state->event = NULL;
>  	}
>  
> -	vkms_output->crc_state = to_vkms_crtc_state(crtc->state);
> +	vkms_output->composer_state = to_vkms_crtc_state(crtc->state);
>  
>  	spin_unlock_irq(&vkms_output->lock);
>  }
> @@ -274,10 +274,10 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>  	drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
>  
>  	spin_lock_init(&vkms_out->lock);
> -	spin_lock_init(&vkms_out->crc_lock);
> +	spin_lock_init(&vkms_out->composer_lock);
>  
> -	vkms_out->crc_workq = alloc_ordered_workqueue("vkms_crc_workq", 0);
> -	if (!vkms_out->crc_workq)
> +	vkms_out->composer_workq = alloc_ordered_workqueue("vkms_composer", 0);
> +	if (!vkms_out->composer_workq)
>  		return -ENOMEM;
>  
>  	return ret;
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 966b3d653189..ac790b6527e4 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -56,7 +56,7 @@ static void vkms_release(struct drm_device *dev)
>  	drm_atomic_helper_shutdown(&vkms->drm);
>  	drm_mode_config_cleanup(&vkms->drm);
>  	drm_dev_fini(&vkms->drm);
> -	destroy_workqueue(vkms->output.crc_workq);
> +	destroy_workqueue(vkms->output.composer_workq);
>  }
>  
>  static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
> @@ -82,7 +82,7 @@ static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
>  		struct vkms_crtc_state *vkms_state =
>  			to_vkms_crtc_state(old_crtc_state);
>  
> -		flush_work(&vkms_state->crc_work);
> +		flush_work(&vkms_state->composer_work);
>  	}
>  
>  	drm_atomic_helper_cleanup_planes(dev, old_state);
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index e2d1aa089dec..fc6cda164336 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -20,7 +20,7 @@
>  
>  extern bool enable_cursor;
>  
> -struct vkms_crc_data {
> +struct vkms_composer {
>  	struct drm_framebuffer fb;
>  	struct drm_rect src, dst;
>  	unsigned int offset;
> @@ -31,29 +31,29 @@ struct vkms_crc_data {
>  /**
>   * vkms_plane_state - Driver specific plane state
>   * @base: base plane state
> - * @crc_data: data required for CRC computation
> + * @composer: data required for composing computation
>   */
>  struct vkms_plane_state {
>  	struct drm_plane_state base;
> -	struct vkms_crc_data *crc_data;
> +	struct vkms_composer *composer;
>  };
>  
>  /**
>   * vkms_crtc_state - Driver specific CRTC state
>   * @base: base CRTC state
> - * @crc_work: work struct to compute and add CRC entries
> + * @composer_work: work struct to compose and add CRC entries
>   * @n_frame_start: start frame number for computed CRC
>   * @n_frame_end: end frame number for computed CRC
>   */
>  struct vkms_crtc_state {
>  	struct drm_crtc_state base;
> -	struct work_struct crc_work;
> +	struct work_struct composer_work;
>  
>  	int num_active_planes;
>  	/* stack of active planes for crc computation, should be in z order */
>  	struct vkms_plane_state **active_planes;
>  
> -	/* below three are protected by vkms_output.crc_lock */
> +	/* below three are protected by vkms_output.composer_lock */
>  	bool crc_pending;
>  	u64 frame_start;
>  	u64 frame_end;
> @@ -66,16 +66,16 @@ struct vkms_output {
>  	struct hrtimer vblank_hrtimer;
>  	ktime_t period_ns;
>  	struct drm_pending_vblank_event *event;
> -	/* ordered wq for crc_work */
> -	struct workqueue_struct *crc_workq;
> -	/* protects concurrent access to crc_data */
> +	/* ordered wq for composer_work */
> +	struct workqueue_struct *composer_workq;
> +	/* protects concurrent access to composer */
>  	spinlock_t lock;
>  
>  	/* protected by @lock */
> -	bool crc_enabled;
> -	struct vkms_crtc_state *crc_state;
> +	bool composer_enabled;
> +	struct vkms_crtc_state *composer_state;
>  
> -	spinlock_t crc_lock;
> +	spinlock_t composer_lock;
>  };
>  
>  struct vkms_device {
> @@ -143,6 +143,8 @@ const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
>  int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name);
>  int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
>  			   size_t *values_cnt);
> -void vkms_crc_work_handle(struct work_struct *work);
> +
> +/* Composer Support */
> +void vkms_composer_worker(struct work_struct *work);
>  
>  #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 18c630cfc485..8b60d3434d75 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -18,20 +18,20 @@ static struct drm_plane_state *
>  vkms_plane_duplicate_state(struct drm_plane *plane)
>  {
>  	struct vkms_plane_state *vkms_state;
> -	struct vkms_crc_data *crc_data;
> +	struct vkms_composer *composer;
>  
>  	vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
>  	if (!vkms_state)
>  		return NULL;
>  
> -	crc_data = kzalloc(sizeof(*crc_data), GFP_KERNEL);
> -	if (!crc_data) {
> -		DRM_DEBUG_KMS("Couldn't allocate crc_data\n");
> +	composer = kzalloc(sizeof(*composer), GFP_KERNEL);
> +	if (!composer) {
> +		DRM_DEBUG_KMS("Couldn't allocate composer\n");
>  		kfree(vkms_state);
>  		return NULL;
>  	}
>  
> -	vkms_state->crc_data = crc_data;
> +	vkms_state->composer = composer;
>  
>  	__drm_atomic_helper_plane_duplicate_state(plane,
>  						  &vkms_state->base);
> @@ -49,12 +49,12 @@ static void vkms_plane_destroy_state(struct drm_plane *plane,
>  		/* dropping the reference we acquired in
>  		 * vkms_primary_plane_update()
>  		 */
> -		if (drm_framebuffer_read_refcount(&vkms_state->crc_data->fb))
> -			drm_framebuffer_put(&vkms_state->crc_data->fb);
> +		if (drm_framebuffer_read_refcount(&vkms_state->composer->fb))
> +			drm_framebuffer_put(&vkms_state->composer->fb);
>  	}
>  
> -	kfree(vkms_state->crc_data);
> -	vkms_state->crc_data = NULL;
> +	kfree(vkms_state->composer);
> +	vkms_state->composer = NULL;
>  
>  	__drm_atomic_helper_plane_destroy_state(old_state);
>  	kfree(vkms_state);
> @@ -91,21 +91,21 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
>  {
>  	struct vkms_plane_state *vkms_plane_state;
>  	struct drm_framebuffer *fb = plane->state->fb;
> -	struct vkms_crc_data *crc_data;
> +	struct vkms_composer *composer;
>  
>  	if (!plane->state->crtc || !fb)
>  		return;
>  
>  	vkms_plane_state = to_vkms_plane_state(plane->state);
>  
> -	crc_data = vkms_plane_state->crc_data;
> -	memcpy(&crc_data->src, &plane->state->src, sizeof(struct drm_rect));
> -	memcpy(&crc_data->dst, &plane->state->dst, sizeof(struct drm_rect));
> -	memcpy(&crc_data->fb, fb, sizeof(struct drm_framebuffer));
> -	drm_framebuffer_get(&crc_data->fb);
> -	crc_data->offset = fb->offsets[0];
> -	crc_data->pitch = fb->pitches[0];
> -	crc_data->cpp = fb->format->cpp[0];
> +	composer = vkms_plane_state->composer;
> +	memcpy(&composer->src, &plane->state->src, sizeof(struct drm_rect));
> +	memcpy(&composer->dst, &plane->state->dst, sizeof(struct drm_rect));
> +	memcpy(&composer->fb, fb, sizeof(struct drm_framebuffer));
> +	drm_framebuffer_get(&composer->fb);
> +	composer->offset = fb->offsets[0];
> +	composer->pitch = fb->pitches[0];
> +	composer->cpp = fb->format->cpp[0];
>  }
>  
>  static int vkms_plane_atomic_check(struct drm_plane *plane,
> -- 
> 2.21.0
> 

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

Re: [PATCH V3 3/5] drm/vkms: Decouple crc operations from composer

[Daniel Vetter]

On Tue, Jun 25, 2019 at 10:37:58PM -0300, Rodrigo Siqueira wrote:
> In the vkms_composer.c, some of the functions related to CRC and compose
> have interdependence between each other. This patch reworks some
> functions inside vkms_composer to make crc and composer computation
> decoupled.
> 
> This patch is preparation work for making vkms able to support new
> features.
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> ---
>  drivers/gpu/drm/vkms/vkms_composer.c | 49 ++++++++++++++++------------
>  1 file changed, 29 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index eb7ea8be1f98..51a270514219 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -105,35 +105,31 @@ static void compose_cursor(struct vkms_composer *cursor_composer,
>  	      primary_composer, cursor_composer);
>  }
>  
> -static uint32_t _vkms_get_crc(struct vkms_composer *primary_composer,
> -			      struct vkms_composer *cursor_composer)
> +static int compose_planes(void **vaddr_out,
> +			  struct vkms_composer *primary_composer,
> +			  struct vkms_composer *cursor_composer)
>  {
>  	struct drm_framebuffer *fb = &primary_composer->fb;
>  	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
>  	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> -	void *vaddr_out = kzalloc(vkms_obj->gem.size, GFP_KERNEL);
> -	u32 crc = 0;
>  
> -	if (!vaddr_out) {
> -		DRM_ERROR("Failed to allocate memory for output frame.");
> -		return 0;
> +	if (!*vaddr_out) {
> +		*vaddr_out = kzalloc(vkms_obj->gem.size, GFP_KERNEL);

Uh allocating memory here isn't great, since you effectily can't handle
the error at all. Also for big resolutions kzalloc will fall back to
kvmalloc I think, which is rather expensive to set up.

But I guess this is a preexisting issue, so welp.

What I would do is pull out the allocation at least, so that
compose_planes really only dos composing, can never fail because it
doesn't need to allocate memory.

> +		if (!*vaddr_out) {
> +			DRM_ERROR("Cannot allocate memory for output frame.");
> +			return -ENOMEM;
> +		}
>  	}
>  
> -	if (WARN_ON(!vkms_obj->vaddr)) {
> -		kfree(vaddr_out);
> -		return crc;
> -	}
> +	if (WARN_ON(!vkms_obj->vaddr))
> +		return -EINVAL;
>  
> -	memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
> +	memcpy(*vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
>  
>  	if (cursor_composer)
> -		compose_cursor(cursor_composer, primary_composer, vaddr_out);
> +		compose_cursor(cursor_composer, primary_composer, *vaddr_out);
>  
> -	crc = compute_crc(vaddr_out, primary_composer);
> -
> -	kfree(vaddr_out);
> -
> -	return crc;
> +	return 0;
>  }
>  
>  /**
> @@ -154,9 +150,11 @@ void vkms_composer_worker(struct work_struct *work)
>  	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
>  	struct vkms_composer *primary_composer = NULL;
>  	struct vkms_composer *cursor_composer = NULL;
> +	void *vaddr_out = NULL;
>  	u32 crc32 = 0;
>  	u64 frame_start, frame_end;
>  	bool crc_pending;
> +	int ret;
>  
>  	spin_lock_irq(&out->composer_lock);
>  	frame_start = crtc_state->frame_start;
> @@ -180,14 +178,25 @@ void vkms_composer_worker(struct work_struct *work)
>  	if (crtc_state->num_active_planes == 2)
>  		cursor_composer = crtc_state->active_planes[1]->composer;
>  
> -	if (primary_composer)
> -		crc32 = _vkms_get_crc(primary_composer, cursor_composer);
> +	if (!primary_composer)
> +		return;
> +
> +	ret = compose_planes(&vaddr_out, primary_composer, cursor_composer);
> +	if (ret) {
> +		if (ret == -EINVAL)
> +			kfree(vaddr_out);
> +		return;
> +	}
> +
> +	crc32 = compute_crc(vaddr_out, primary_composer);
>  
>  	/*
>  	 * The worker can fall behind the vblank hrtimer, make sure we catch up.
>  	 */
>  	while (frame_start <= frame_end)
>  		drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
> +
> +	kfree(vaddr_out);

Especially since you're freeing the memory _outside_ of compose_planes.

Aside: This all kinda doesn't go in the right direction for
high-performance composing, so I guess I need to get started with typing
up what that should look like.
-Daniel

>  }
>  
>  static const char * const pipe_crc_sources[] = {"auto"};
> -- 
> 2.21.0

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

Re: [PATCH V3 4/5] drm/vkms: Compute CRC without change input data

[Daniel Vetter]

On Tue, Jun 25, 2019 at 10:38:31PM -0300, Rodrigo Siqueira wrote:
> The compute_crc() function is responsible for calculating the
> framebuffer CRC value; due to the XRGB format, this function has to
> ignore the alpha channel during the CRC computation. Therefore,
> compute_crc() set zero to the alpha channel directly in the input
> framebuffer, which is not a problem since this function receives a copy
> of the original buffer. However, if we want to use this function in a
> context without a buffer copy, it will change the initial value. This
> patch makes compute_crc() calculate the CRC value without modifying the
> input framebuffer.

Uh why? For writeback we're writing the output too, so we can write
whatever we want to into the alpha channel. For writeback we should never
accept a pixel format where alpha actually matters, that doesn't make
sense. You can't see through a real screen either, they are all opaque :-)
-Daniel

> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> ---
>  drivers/gpu/drm/vkms/vkms_composer.c | 31 +++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 51a270514219..8126aa0f968f 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -6,33 +6,40 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
>  
> +static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
> +				 const struct vkms_composer *composer)
> +{
> +	int src_offset = composer->offset + (y * composer->pitch)
> +					  + (x * composer->cpp);
> +
> +	return *(u32 *)&buffer[src_offset];
> +}
> +
>  /**
>   * compute_crc - Compute CRC value on output frame
>   *
> - * @vaddr_out: address to final framebuffer
> + * @vaddr: address to final framebuffer
>   * @composer: framebuffer's metadata
>   *
>   * returns CRC value computed using crc32 on the visible portion of
>   * the final framebuffer at vaddr_out
>   */
> -static uint32_t compute_crc(void *vaddr_out, struct vkms_composer *composer)
> +static uint32_t compute_crc(const u8 *vaddr,
> +			    const struct vkms_composer *composer)
>  {
> -	int i, j, src_offset;
> +	int x, y;
>  	int x_src = composer->src.x1 >> 16;
>  	int y_src = composer->src.y1 >> 16;
>  	int h_src = drm_rect_height(&composer->src) >> 16;
>  	int w_src = drm_rect_width(&composer->src) >> 16;
> -	u32 crc = 0;
> +	u32 crc = 0, pixel = 0;
>  
> -	for (i = y_src; i < y_src + h_src; ++i) {
> -		for (j = x_src; j < x_src + w_src; ++j) {
> -			src_offset = composer->offset
> -				     + (i * composer->pitch)
> -				     + (j * composer->cpp);
> +	for (y = y_src; y < y_src + h_src; ++y) {
> +		for (x = x_src; x < x_src + w_src; ++x) {
>  			/* XRGB format ignores Alpha channel */
> -			memset(vaddr_out + src_offset + 24, 0,  8);
> -			crc = crc32_le(crc, vaddr_out + src_offset,
> -				       sizeof(u32));
> +			pixel = get_pixel_from_buffer(x, y, vaddr, composer);
> +			bitmap_clear((void *)&pixel, 0, 8);
> +			crc = crc32_le(crc, (void *)&pixel, sizeof(u32));
>  		}
>  	}
>  
> -- 
> 2.21.0

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

Re: [PATCH V3 3/5] drm/vkms: Decouple crc operations from composer

[Simon Ser]

On Thursday, July 11, 2019 11:19 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> Aside: This all kinda doesn't go in the right direction for
> high-performance composing, so I guess I need to get started with typing
> up what that should look like.

Some related logs from IRC:

2019-07-10 19:42:49     danvet  ickle, is there an idiot guide to reasonable fast blending/composing with the cpu?
2019-07-10 19:43:13     danvet  for vkms, so maitainability is high on the wishlist, but it needs to be somewhat fast to be able to keep up
2019-07-10 19:44:02     ickle   pixman for rectangles
2019-07-10 19:45:00     ickle   if you want reasonably fast, you want simd
2019-07-10 19:45:16     ickle   so better to use some prebaked library
2019-07-10 19:45:38     ickle   but within vkms?
2019-07-10 19:45:48     ickle   or just for igt/vkms?
2019-07-10 19:46:40     ickle   within vkms for writeback blending I guess
2019-07-10 19:51:38     ickle   http://paste.debian.net/1091097/
2019-07-10 19:53:08     ickle   http://paste.debian.net/1091098/
2019-07-10 19:54:36     emersion        danvet: what are your plans for the compositor refactoring?
2019-07-10 19:55:21     emersion        are we still really using macros instead of functions
2019-07-10 19:56:22     <--     nvishwa1 (~nvishwa1@192.55.54.44) has quit (Remote host closed the connection)
2019-07-10 19:56:44     emersion        is this coming from pixman, ickle?
2019-07-10 19:56:50     -->     karolherbst (~kherbst@2a02:8308:b0be:6900:d9e8:6dcd:2f6a:6cb5) has joined #intel-gfx
2019-07-10 19:57:23     ickle   yes, take it as a reference on how to do abgr32 premultiplied alpha blending
2019-07-10 19:57:33     <--     amathes (ajmathes@nat/intel/x-ywywuftprrqndbaj) has quit (Ping timeout: 245 seconds)
2019-07-10 19:57:48     emersion        yeah, that's a good idea
2019-07-10 19:57:49     ickle   or argb32 actually
2019-07-10 19:58:01     emersion        (asking for the source because license)
2019-07-10 19:58:04     ickle   MIT
2019-07-10 19:58:07     emersion        nice
2019-07-10 20:00:12     <--     sandeep (sandeep@nat/intel/x-buwbzkgopszvwbcr) has quit (Remote host closed the connection)
2019-07-10 20:01:34     danvet  ickle, yeah vkms in the kernel
2019-07-10 20:02:41     danvet  also might need more than 8 bits ...
2019-07-10 20:02:57     danvet  and kinda hoped I could just tell gcc to simdify it for me
2019-07-10 20:03:23     danvet  emersion, compositor refactoring
2019-07-10 20:03:46     danvet  ickle, higher level I figured something like a fetch fifo in a standard format
2019-07-10 20:04:02     danvet  with some drm_format->standard format conversion tools
2019-07-10 20:04:08     danvet  and then one blender
2019-07-10 20:04:39     danvet  and then either add that to the crc and toss it (again maybe scanline-by-scanline, or whatever fits reasonable into l1$ all together)
2019-07-10 20:04:44     danvet  or dump it into writeback
2019-07-10 20:08:25     danvet  https://gcc.gnu.org/onlinedocs/gcc/Vector-Extensions.html <- this stuff essentially, using generics
2019-07-10 20:08:38     danvet  *generic intrinsics
2019-07-10 20:08:49     danvet  or is that going to be real awful?
2019-07-10 20:09:35     ickle   shouldn't be required for _basic_ blending
2019-07-10 20:09:52     danvet  yeah I think all we want is premultiplied alpha
2019-07-10 20:09:53     ickle   if all you need is an over operator, then gcc should be pretty good
2019-07-10 20:09:57     <--     sdutt (sdutt@nat/intel/x-zhquyrigdztslyqh) has quit (Ping timeout: 268 seconds)
2019-07-10 20:09:59     danvet  maybe some yuv->rgb
2019-07-10 20:10:19     danvet  expanding from whatever silly format we have to the right vector
2019-07-10 20:10:33     emersion        what would be your universal format?
2019-07-10 20:10:46     danvet  a16r16b16g16
2019-07-10 20:10:50     danvet  except if gcc barfs on that
2019-07-10 20:11:02     danvet  or maybe go all in on 4x float :-)
2019-07-10 20:11:10     emersion        eh
2019-07-10 20:11:29     emersion        fp16 would work, but would also mean rounding errors, probably?
2019-07-10 20:11:35     danvet  uint16 is not going to be awesome for hdr, but good enough for everything else
2019-07-10 20:11:42     danvet  no cpu has fp16
2019-07-10 20:11:52     emersion        ah, fp32
2019-07-10 20:11:57     krh     wait for avx1024
2019-07-10 20:12:01     emersion        seems kind of overkill
2019-07-10 20:12:01     danvet  fg32 is probably fastest option we can get on common hw
2019-07-10 20:12:12     danvet  krh, very much aiming for good enough here
2019-07-10 20:12:26     ickle   one plan is to sneak ksim into the kernel as generic gpu-on-x86
2019-07-10 20:12:52     krh     can the kernel use avx2?
2019-07-10 20:13:05     danvet  well, all stuff I'd need to figure out
2019-07-10 20:13:07     danvet  I hope so
2019-07-10 20:13:12     ickle   it can
2019-07-10 20:13:19     ickle   easier than mmx
2019-07-10 20:13:49     emersion        when you say gcc barfs on a16r16b16g16: why?
2019-07-10 20:13:57     danvet  kernel_fpu_begin/end + telling gcc to optimize the crap out of the file with the blending functions
2019-07-10 20:14:11     emersion        uint64 too hard for gcc to optimize?
2019-07-10 20:14:14     danvet  emersion, I haven't checked, but if it generates silly code then might be better to go with fp32
2019-07-10 20:14:28     vsyrjala        the compiler always generates silly code
2019-07-10 20:14:30     danvet  ideally it should all boil down to sse/avx
2019-07-10 20:14:32     emersion        ahah
2019-07-10 20:14:51     danvet  and ideally all with generic intrinsics so the arm folks don't freak out
2019-07-10 20:15:07     -->     sandeep (~sandeep@134.134.139.76) has joined #intel-gfx
2019-07-10 20:15:23     emersion        +1 for generic intrinsics
2019-07-10 20:15:42     danvet  the conversion from uint to fp32 might be hilarious
2019-07-10 20:15:50     emersion        yeah, probably
2019-07-10 20:15:52     danvet  perhaps the one place where we want to use an sse or avx intrinsic
2019-07-10 20:16:19     danvet  especially if we convert to simd16 instead of something like 4x4
2019-07-10 20:16:22     emersion        we should probably do some little experiments before doing anything
2019-07-10 20:16:24     danvet  simd4x4
2019-07-10 20:16:31     danvet  yeah
2019-07-10 20:16:41     danvet  that's why I'm asking here, since I have roughly 0 clue about this
2019-07-10 20:17:48     danvet  I don't think we ever need a dot or anything like that, so plain simd is propably best
2019-07-10 20:18:07     danvet  except the input is usually 4x or 3x vectors
2019-07-10 20:18:07     emersion        a dot?
2019-07-10 20:18:12     danvet  dot product
2019-07-10 20:18:14     emersion        ah
2019-07-10 20:18:15     danvet  for vertex shaders
2019-07-10 20:18:20     emersion        yeah, probably not
2019-07-10 20:18:21     bwidawsk        danvet› btw, I think "generic intrinsic" is an ARM thing
2019-07-10 20:18:29     bwidawsk        I think everywhere else, they just say intrinsic
2019-07-10 20:18:41     bwidawsk        at least, I've never heard the generic prefix other than ARM compiler
2019-07-10 20:19:19     vsyrjala        just make the max supported resolution 8x8 or something and speed shouldn't be a huge issue
2019-07-10 20:19:30     emersion        i think he meant generic intrinsic vs. sse/avx/whatever
2019-07-10 20:19:49     bwidawsk        emersion› yes, I figured out what he meant, I just mentioned it because it was a source of confusion
2019-07-10 20:19:52     emersion        vsyrjala, helpful as always :)
2019-07-10 20:20:32     danvet  oh gcc has all the casting implementing in the intrinsics too
2019-07-10 20:20:50     danvet  bwidawsk, gcc manpage also calls them generic intrinsics
2019-07-10 20:21:25     bwidawsk        danvet› not my gcc manpage
2019-07-10 20:21:40     danvet  well "generic vector operations"
2019-07-10 20:21:46     danvet  ^^ that what you meant?
2019-07-10 20:22:03     danvet  vs "machine-specific vector intrinsics"
2019-07-10 20:22:38     bwidawsk        I thought the generic intrinsic term came from ARM's proprietary compiler
2019-07-10 20:22:40     bwidawsk        but I might be wrong

Re: [PATCH V3 4/5] drm/vkms: Compute CRC without change input data

[Simon Ser]

On Thursday, July 11, 2019 11:21 AM, Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Jun 25, 2019 at 10:38:31PM -0300, Rodrigo Siqueira wrote:
>
> > The compute_crc() function is responsible for calculating the
> > framebuffer CRC value; due to the XRGB format, this function has to
> > ignore the alpha channel during the CRC computation. Therefore,
> > compute_crc() set zero to the alpha channel directly in the input
> > framebuffer, which is not a problem since this function receives a copy
> > of the original buffer. However, if we want to use this function in a
> > context without a buffer copy, it will change the initial value. This
> > patch makes compute_crc() calculate the CRC value without modifying the
> > input framebuffer.
>
> Uh why? For writeback we're writing the output too, so we can write
> whatever we want to into the alpha channel. For writeback we should never
> accept a pixel format where alpha actually matters, that doesn't make
> sense. You can't see through a real screen either, they are all opaque :-)

I'm not sure about that. See e.g.
https://en.wikipedia.org/wiki/See-through_display

Many drivers already accept FBs with alpha channels for the primary
plane.
https://drmdb.emersion.fr/formats?plane=1

Just making sure we aren't painting ourselves into a corner. :P

Re: [PATCH V3 5/5] drm/vkms: Add support for writeback

[Daniel Vetter]

On Tue, Jun 25, 2019 at 10:39:03PM -0300, Rodrigo Siqueira wrote:
> This patch implements the necessary functions to add writeback support
> for vkms. This feature is useful for testing compositors if you don't
> have hardware with writeback support.
> 
> Change in V3 (Daniel):
> - If writeback is enabled, compose everything into the writeback buffer
> instead of CRC private buffer.
> - Guarantees that the CRC will match exactly what we have in the
> writeback buffer.
> 
> Change in V2:
> - Rework signal completion (Brian)
> - Integrates writeback with active_planes (Daniel)
> - Compose cursor (Daniel)
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> ---
>  drivers/gpu/drm/vkms/Makefile         |   9 +-
>  drivers/gpu/drm/vkms/vkms_composer.c  |  16 ++-
>  drivers/gpu/drm/vkms/vkms_drv.c       |   4 +
>  drivers/gpu/drm/vkms/vkms_drv.h       |   8 ++
>  drivers/gpu/drm/vkms/vkms_output.c    |  10 ++
>  drivers/gpu/drm/vkms/vkms_writeback.c | 141 ++++++++++++++++++++++++++
>  6 files changed, 185 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c
> 
> diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> index 0b767d7efa24..333d3cead0e3 100644
> --- a/drivers/gpu/drm/vkms/Makefile
> +++ b/drivers/gpu/drm/vkms/Makefile
> @@ -1,4 +1,11 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_composer.o
> +vkms-y := \
> +	vkms_drv.o \
> +	vkms_plane.o \
> +	vkms_output.o \
> +	vkms_crtc.o \
> +	vkms_gem.o \
> +	vkms_composer.o \
> +	vkms_writeback.o
>  
>  obj-$(CONFIG_DRM_VKMS) += vkms.o
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 8126aa0f968f..2317803e7320 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -157,16 +157,17 @@ void vkms_composer_worker(struct work_struct *work)
>  	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
>  	struct vkms_composer *primary_composer = NULL;
>  	struct vkms_composer *cursor_composer = NULL;
> +	bool crc_pending, wb_pending;

I'm not seeing how you schedule this work for writeback if there's no crc
enabled. Does that work?
>  	void *vaddr_out = NULL;
>  	u32 crc32 = 0;
>  	u64 frame_start, frame_end;
> -	bool crc_pending;
>  	int ret;
>  
>  	spin_lock_irq(&out->composer_lock);
>  	frame_start = crtc_state->frame_start;
>  	frame_end = crtc_state->frame_end;
>  	crc_pending = crtc_state->crc_pending;
> +	wb_pending = crtc_state->wb_pending;
>  	crtc_state->frame_start = 0;
>  	crtc_state->frame_end = 0;
>  	crtc_state->crc_pending = false;
> @@ -188,9 +189,12 @@ void vkms_composer_worker(struct work_struct *work)
>  	if (!primary_composer)
>  		return;
>  
> +	if (wb_pending)
> +		vaddr_out = crtc_state->active_writeback;
> +
>  	ret = compose_planes(&vaddr_out, primary_composer, cursor_composer);
>  	if (ret) {
> -		if (ret == -EINVAL)
> +		if (ret == -EINVAL && !wb_pending)
>  			kfree(vaddr_out);
>  		return;
>  	}
> @@ -203,6 +207,14 @@ void vkms_composer_worker(struct work_struct *work)
>  	while (frame_start <= frame_end)
>  		drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
>  
> +	if (wb_pending) {
> +		drm_writeback_signal_completion(&out->wb_connector, 0);
> +		spin_lock_irq(&out->composer_lock);
> +		crtc_state->wb_pending = false;
> +		spin_unlock_irq(&out->composer_lock);
> +		return;
> +	}
> +
>  	kfree(vaddr_out);
>  }
>  
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index ac790b6527e4..152d7de24a76 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -30,6 +30,10 @@ bool enable_cursor;
>  module_param_named(enable_cursor, enable_cursor, bool, 0444);
>  MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
>  
> +bool enable_writeback;
> +module_param_named(enable_writeback, enable_writeback, bool, 0444);
> +MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector");
> +
>  static const struct file_operations vkms_driver_fops = {
>  	.owner		= THIS_MODULE,
>  	.open		= drm_open,
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index fc6cda164336..9ff2cd4ebf81 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -7,6 +7,7 @@
>  #include <drm/drm.h>
>  #include <drm/drm_gem.h>
>  #include <drm/drm_encoder.h>
> +#include <drm/drm_writeback.h>
>  #include <linux/hrtimer.h>
>  
>  #define XRES_MIN    20
> @@ -19,6 +20,7 @@
>  #define YRES_MAX  8192
>  
>  extern bool enable_cursor;
> +extern bool enable_writeback;
>  
>  struct vkms_composer {
>  	struct drm_framebuffer fb;
> @@ -52,9 +54,11 @@ struct vkms_crtc_state {
>  	int num_active_planes;
>  	/* stack of active planes for crc computation, should be in z order */
>  	struct vkms_plane_state **active_planes;
> +	void *active_writeback;
>  
>  	/* below three are protected by vkms_output.composer_lock */
>  	bool crc_pending;
> +	bool wb_pending;
>  	u64 frame_start;
>  	u64 frame_end;
>  };
> @@ -63,6 +67,7 @@ struct vkms_output {
>  	struct drm_crtc crtc;
>  	struct drm_encoder encoder;
>  	struct drm_connector connector;
> +	struct drm_writeback_connector wb_connector;
>  	struct hrtimer vblank_hrtimer;
>  	ktime_t period_ns;
>  	struct drm_pending_vblank_event *event;
> @@ -147,4 +152,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
>  /* Composer Support */
>  void vkms_composer_worker(struct work_struct *work);
>  
> +/* Writeback */
> +int enable_writeback_connector(struct vkms_device *vkmsdev);
> +
>  #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index fb1941a6522c..aea1d4410864 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -84,6 +84,16 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
>  		goto err_attach;
>  	}
>  
> +	if (enable_writeback) {
> +		ret = enable_writeback_connector(vkmsdev);
> +		if (!ret) {
> +			output->composer_enabled = true;
> +			DRM_INFO("Writeback connector enabled");
> +		} else {
> +			DRM_ERROR("Failed to init writeback connector\n");
> +		}
> +	}
> +
>  	drm_mode_config_reset(dev);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> new file mode 100644
> index 000000000000..34dad37a0236
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -0,0 +1,141 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include "vkms_drv.h"
> +#include <drm/drm_writeback.h>
> +#include <drm/drm_probe_helper.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +
> +static const u32 vkms_wb_formats[] = {
> +	DRM_FORMAT_XRGB8888,
> +};
> +
> +static const struct drm_connector_funcs vkms_wb_connector_funcs = {
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.destroy = drm_connector_cleanup,
> +	.reset = drm_atomic_helper_connector_reset,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
> +					struct drm_crtc_state *crtc_state,
> +					struct drm_connector_state *conn_state)
> +{
> +	struct drm_framebuffer *fb;
> +	const struct drm_display_mode *mode = &crtc_state->mode;
> +
> +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> +		return 0;
> +
> +	fb = conn_state->writeback_job->fb;
> +	if (fb->width != mode->hdisplay || fb->height != mode->vdisplay) {
> +		DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
> +			      fb->width, fb->height);
> +		return -EINVAL;
> +	}
> +
> +	if (fb->format->format != DRM_FORMAT_XRGB8888) {
> +		struct drm_format_name_buf format_name;
> +
> +		DRM_DEBUG_KMS("Invalid pixel format %s\n",
> +			      drm_get_format_name(fb->format->format,
> +						  &format_name));
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {
> +	.atomic_check = vkms_wb_encoder_atomic_check,
> +};
> +
> +static int vkms_wb_connector_get_modes(struct drm_connector *connector)
> +{
> +	struct drm_device *dev = connector->dev;
> +
> +	return drm_add_modes_noedid(connector, dev->mode_config.max_width,
> +				    dev->mode_config.max_height);
> +}

Do we need a mode for writeb connector? Is that used by userspace to
figure out the max size for writeback?

> +
> +static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
> +			       struct drm_writeback_job *job)
> +{
> +	struct vkms_gem_object *vkms_obj;
> +	struct drm_gem_object *gem_obj;
> +	int ret;
> +
> +	if (!job->fb)
> +		return 0;

Can this happen?

> +
> +	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> +	ret = vkms_gem_vmap(gem_obj);
> +	if (ret) {
> +		DRM_ERROR("vmap failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> +	job->priv = vkms_obj->vaddr;
> +
> +	return 0;
> +}
> +
> +static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> +				struct drm_writeback_job *job)
> +{
> +	struct drm_gem_object *gem_obj;
> +
> +	if (!job->fb)
> +		return;

Same here?

> +
> +	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> +	vkms_gem_vunmap(gem_obj);
> +}
> +
> +static void vkms_wb_atomic_commit(struct drm_connector *conn,
> +				  struct drm_connector_state *state)
> +{
> +	struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev);
> +	struct vkms_output *output = &vkmsdev->output;
> +	struct drm_writeback_connector *wb_conn = &output->wb_connector;
> +	struct drm_connector_state *conn_state = wb_conn->base.state;
> +	struct vkms_crtc_state *crtc_state = output->composer_state;
> +
> +	if (!conn_state)
> +		return;
> +
> +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
> +		DRM_DEBUG_DRIVER("Disable writeback\n");
> +		return;
> +	}

All three checks above sound like helpers should take care of this?

> +
> +	spin_lock_irq(&output->composer_lock);
> +	crtc_state->active_writeback = conn_state->writeback_job->priv;
> +	crtc_state->wb_pending = true;
> +	spin_unlock_irq(&output->composer_lock);
> +	drm_writeback_queue_job(wb_conn, state);
> +}
> +
> +static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> +	.get_modes = vkms_wb_connector_get_modes,
> +	.prepare_writeback_job = vkms_wb_prepare_job,
> +	.cleanup_writeback_job = vkms_wb_cleanup_job,
> +	.atomic_commit = vkms_wb_atomic_commit,
> +};
> +
> +int enable_writeback_connector(struct vkms_device *vkmsdev)
> +{
> +	struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
> +
> +	vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
> +	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
> +
> +	return drm_writeback_connector_init(&vkmsdev->drm, wb,
> +					    &vkms_wb_connector_funcs,
> +					    &vkms_wb_encoder_helper_funcs,
> +					    vkms_wb_formats,
> +					    ARRAY_SIZE(vkms_wb_formats));
> +}

Overall looks like good approach.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH V3 4/5] drm/vkms: Compute CRC without change input data

[Daniel Vetter]

On Thu, Jul 11, 2019 at 10:28 AM Simon Ser <contact@emersion.fr> wrote:
>
> On Thursday, July 11, 2019 11:21 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> > On Tue, Jun 25, 2019 at 10:38:31PM -0300, Rodrigo Siqueira wrote:
> >
> > > The compute_crc() function is responsible for calculating the
> > > framebuffer CRC value; due to the XRGB format, this function has to
> > > ignore the alpha channel during the CRC computation. Therefore,
> > > compute_crc() set zero to the alpha channel directly in the input
> > > framebuffer, which is not a problem since this function receives a copy
> > > of the original buffer. However, if we want to use this function in a
> > > context without a buffer copy, it will change the initial value. This
> > > patch makes compute_crc() calculate the CRC value without modifying the
> > > input framebuffer.
> >
> > Uh why? For writeback we're writing the output too, so we can write
> > whatever we want to into the alpha channel. For writeback we should never
> > accept a pixel format where alpha actually matters, that doesn't make
> > sense. You can't see through a real screen either, they are all opaque :-)
>
> I'm not sure about that. See e.g.
> https://en.wikipedia.org/wiki/See-through_display

They have variable opaqueness, independent of the color value?

> Many drivers already accept FBs with alpha channels for the primary
> plane.
> https://drmdb.emersion.fr/formats?plane=1

If you have a background color (we assume it to be black) that makes
sense. Still doesn't mean we render transparent output, we don't.

> Just making sure we aren't painting ourselves into a corner. :P

You can add ARGB to your writeback format support list, there is no
corner here at all to get into (at least in the abstract sense).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH V3 4/5] drm/vkms: Compute CRC without change input data

[Rodrigo Siqueira]

[-- Attachment #1: Type: text/plain, Size: 3813 bytes --]

On 07/11, Daniel Vetter wrote:
> On Tue, Jun 25, 2019 at 10:38:31PM -0300, Rodrigo Siqueira wrote:
> > The compute_crc() function is responsible for calculating the
> > framebuffer CRC value; due to the XRGB format, this function has to
> > ignore the alpha channel during the CRC computation. Therefore,
> > compute_crc() set zero to the alpha channel directly in the input
> > framebuffer, which is not a problem since this function receives a copy
> > of the original buffer. However, if we want to use this function in a
> > context without a buffer copy, it will change the initial value. This
> > patch makes compute_crc() calculate the CRC value without modifying the
> > input framebuffer.
> 
> Uh why? For writeback we're writing the output too, so we can write
> whatever we want to into the alpha channel. For writeback we should never
> accept a pixel format where alpha actually matters, that doesn't make
> sense. You can't see through a real screen either, they are all opaque :-)
> -Daniel

Hmmm,

I see your point and I agree, but even though we can write whatever we
want in the output, don’t you think that is weird to change the
framebuffer value in the compute_crc() function?

Thanks
 
> > 
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > ---
> >  drivers/gpu/drm/vkms/vkms_composer.c | 31 +++++++++++++++++-----------
> >  1 file changed, 19 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> > index 51a270514219..8126aa0f968f 100644
> > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> > @@ -6,33 +6,40 @@
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_gem_framebuffer_helper.h>
> >  
> > +static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
> > +				 const struct vkms_composer *composer)
> > +{
> > +	int src_offset = composer->offset + (y * composer->pitch)
> > +					  + (x * composer->cpp);
> > +
> > +	return *(u32 *)&buffer[src_offset];
> > +}
> > +
> >  /**
> >   * compute_crc - Compute CRC value on output frame
> >   *
> > - * @vaddr_out: address to final framebuffer
> > + * @vaddr: address to final framebuffer
> >   * @composer: framebuffer's metadata
> >   *
> >   * returns CRC value computed using crc32 on the visible portion of
> >   * the final framebuffer at vaddr_out
> >   */
> > -static uint32_t compute_crc(void *vaddr_out, struct vkms_composer *composer)
> > +static uint32_t compute_crc(const u8 *vaddr,
> > +			    const struct vkms_composer *composer)
> >  {
> > -	int i, j, src_offset;
> > +	int x, y;
> >  	int x_src = composer->src.x1 >> 16;
> >  	int y_src = composer->src.y1 >> 16;
> >  	int h_src = drm_rect_height(&composer->src) >> 16;
> >  	int w_src = drm_rect_width(&composer->src) >> 16;
> > -	u32 crc = 0;
> > +	u32 crc = 0, pixel = 0;
> >  
> > -	for (i = y_src; i < y_src + h_src; ++i) {
> > -		for (j = x_src; j < x_src + w_src; ++j) {
> > -			src_offset = composer->offset
> > -				     + (i * composer->pitch)
> > -				     + (j * composer->cpp);
> > +	for (y = y_src; y < y_src + h_src; ++y) {
> > +		for (x = x_src; x < x_src + w_src; ++x) {
> >  			/* XRGB format ignores Alpha channel */
> > -			memset(vaddr_out + src_offset + 24, 0,  8);
> > -			crc = crc32_le(crc, vaddr_out + src_offset,
> > -				       sizeof(u32));
> > +			pixel = get_pixel_from_buffer(x, y, vaddr, composer);
> > +			bitmap_clear((void *)&pixel, 0, 8);
> > +			crc = crc32_le(crc, (void *)&pixel, sizeof(u32));
> >  		}
> >  	}
> >  
> > -- 
> > 2.21.0
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Rodrigo Siqueira
https://siqueira.tech

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V3 5/5] drm/vkms: Add support for writeback

[Rodrigo Siqueira]

[-- Attachment #1: Type: text/plain, Size: 13580 bytes --]

On 07/11, Daniel Vetter wrote:
> On Tue, Jun 25, 2019 at 10:39:03PM -0300, Rodrigo Siqueira wrote:
> > This patch implements the necessary functions to add writeback support
> > for vkms. This feature is useful for testing compositors if you don't
> > have hardware with writeback support.
> > 
> > Change in V3 (Daniel):
> > - If writeback is enabled, compose everything into the writeback buffer
> > instead of CRC private buffer.
> > - Guarantees that the CRC will match exactly what we have in the
> > writeback buffer.
> > 
> > Change in V2:
> > - Rework signal completion (Brian)
> > - Integrates writeback with active_planes (Daniel)
> > - Compose cursor (Daniel)
> > 
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > ---
> >  drivers/gpu/drm/vkms/Makefile         |   9 +-
> >  drivers/gpu/drm/vkms/vkms_composer.c  |  16 ++-
> >  drivers/gpu/drm/vkms/vkms_drv.c       |   4 +
> >  drivers/gpu/drm/vkms/vkms_drv.h       |   8 ++
> >  drivers/gpu/drm/vkms/vkms_output.c    |  10 ++
> >  drivers/gpu/drm/vkms/vkms_writeback.c | 141 ++++++++++++++++++++++++++
> >  6 files changed, 185 insertions(+), 3 deletions(-)
> >  create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c
> > 
> > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> > index 0b767d7efa24..333d3cead0e3 100644
> > --- a/drivers/gpu/drm/vkms/Makefile
> > +++ b/drivers/gpu/drm/vkms/Makefile
> > @@ -1,4 +1,11 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> > -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_composer.o
> > +vkms-y := \
> > +	vkms_drv.o \
> > +	vkms_plane.o \
> > +	vkms_output.o \
> > +	vkms_crtc.o \
> > +	vkms_gem.o \
> > +	vkms_composer.o \
> > +	vkms_writeback.o
> >  
> >  obj-$(CONFIG_DRM_VKMS) += vkms.o
> > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> > index 8126aa0f968f..2317803e7320 100644
> > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> > @@ -157,16 +157,17 @@ void vkms_composer_worker(struct work_struct *work)
> >  	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
> >  	struct vkms_composer *primary_composer = NULL;
> >  	struct vkms_composer *cursor_composer = NULL;
> > +	bool crc_pending, wb_pending;
> 
> I'm not seeing how you schedule this work for writeback if there's no crc
> enabled. Does that work?

If we enable writeback, we set true for the flag composer_enabled (see
vkms_output.c in this patch) which also make vkms_vblank_simulate able
to handle crc. Since writeback is oneshot, we only enable it on
vkms_wb_atomic_commit (crtc_state->wb_pending = true); in its turn
vkms_composer_worker() use writeback buffer instead of creating a new
one for computing crc. Finally, at the end of vkms_composer_worker() we
disable the writeback.

> >  	void *vaddr_out = NULL;
> >  	u32 crc32 = 0;
> >  	u64 frame_start, frame_end;
> > -	bool crc_pending;
> >  	int ret;
> >  
> >  	spin_lock_irq(&out->composer_lock);
> >  	frame_start = crtc_state->frame_start;
> >  	frame_end = crtc_state->frame_end;
> >  	crc_pending = crtc_state->crc_pending;
> > +	wb_pending = crtc_state->wb_pending;
> >  	crtc_state->frame_start = 0;
> >  	crtc_state->frame_end = 0;
> >  	crtc_state->crc_pending = false;
> > @@ -188,9 +189,12 @@ void vkms_composer_worker(struct work_struct *work)
> >  	if (!primary_composer)
> >  		return;
> >  
> > +	if (wb_pending)
> > +		vaddr_out = crtc_state->active_writeback;
> > +
> >  	ret = compose_planes(&vaddr_out, primary_composer, cursor_composer);
> >  	if (ret) {
> > -		if (ret == -EINVAL)
> > +		if (ret == -EINVAL && !wb_pending)
> >  			kfree(vaddr_out);
> >  		return;
> >  	}
> > @@ -203,6 +207,14 @@ void vkms_composer_worker(struct work_struct *work)
> >  	while (frame_start <= frame_end)
> >  		drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
> >  
> > +	if (wb_pending) {
> > +		drm_writeback_signal_completion(&out->wb_connector, 0);
> > +		spin_lock_irq(&out->composer_lock);
> > +		crtc_state->wb_pending = false;
> > +		spin_unlock_irq(&out->composer_lock);
> > +		return;
> > +	}
> > +
> >  	kfree(vaddr_out);
> >  }
> >  
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > index ac790b6527e4..152d7de24a76 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > @@ -30,6 +30,10 @@ bool enable_cursor;
> >  module_param_named(enable_cursor, enable_cursor, bool, 0444);
> >  MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
> >  
> > +bool enable_writeback;
> > +module_param_named(enable_writeback, enable_writeback, bool, 0444);
> > +MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector");
> > +
> >  static const struct file_operations vkms_driver_fops = {
> >  	.owner		= THIS_MODULE,
> >  	.open		= drm_open,
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > index fc6cda164336..9ff2cd4ebf81 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -7,6 +7,7 @@
> >  #include <drm/drm.h>
> >  #include <drm/drm_gem.h>
> >  #include <drm/drm_encoder.h>
> > +#include <drm/drm_writeback.h>
> >  #include <linux/hrtimer.h>
> >  
> >  #define XRES_MIN    20
> > @@ -19,6 +20,7 @@
> >  #define YRES_MAX  8192
> >  
> >  extern bool enable_cursor;
> > +extern bool enable_writeback;
> >  
> >  struct vkms_composer {
> >  	struct drm_framebuffer fb;
> > @@ -52,9 +54,11 @@ struct vkms_crtc_state {
> >  	int num_active_planes;
> >  	/* stack of active planes for crc computation, should be in z order */
> >  	struct vkms_plane_state **active_planes;
> > +	void *active_writeback;
> >  
> >  	/* below three are protected by vkms_output.composer_lock */
> >  	bool crc_pending;
> > +	bool wb_pending;
> >  	u64 frame_start;
> >  	u64 frame_end;
> >  };
> > @@ -63,6 +67,7 @@ struct vkms_output {
> >  	struct drm_crtc crtc;
> >  	struct drm_encoder encoder;
> >  	struct drm_connector connector;
> > +	struct drm_writeback_connector wb_connector;
> >  	struct hrtimer vblank_hrtimer;
> >  	ktime_t period_ns;
> >  	struct drm_pending_vblank_event *event;
> > @@ -147,4 +152,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> >  /* Composer Support */
> >  void vkms_composer_worker(struct work_struct *work);
> >  
> > +/* Writeback */
> > +int enable_writeback_connector(struct vkms_device *vkmsdev);
> > +
> >  #endif /* _VKMS_DRV_H_ */
> > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > index fb1941a6522c..aea1d4410864 100644
> > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > @@ -84,6 +84,16 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> >  		goto err_attach;
> >  	}
> >  
> > +	if (enable_writeback) {
> > +		ret = enable_writeback_connector(vkmsdev);
> > +		if (!ret) {
> > +			output->composer_enabled = true;
> > +			DRM_INFO("Writeback connector enabled");
> > +		} else {
> > +			DRM_ERROR("Failed to init writeback connector\n");
> > +		}
> > +	}
> > +
> >  	drm_mode_config_reset(dev);
> >  
> >  	return 0;
> > diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> > new file mode 100644
> > index 000000000000..34dad37a0236
> > --- /dev/null
> > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> > @@ -0,0 +1,141 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +#include "vkms_drv.h"
> > +#include <drm/drm_writeback.h>
> > +#include <drm/drm_probe_helper.h>
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_gem_framebuffer_helper.h>
> > +
> > +static const u32 vkms_wb_formats[] = {
> > +	DRM_FORMAT_XRGB8888,
> > +};
> > +
> > +static const struct drm_connector_funcs vkms_wb_connector_funcs = {
> > +	.fill_modes = drm_helper_probe_single_connector_modes,
> > +	.destroy = drm_connector_cleanup,
> > +	.reset = drm_atomic_helper_connector_reset,
> > +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > +};
> > +
> > +static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
> > +					struct drm_crtc_state *crtc_state,
> > +					struct drm_connector_state *conn_state)
> > +{
> > +	struct drm_framebuffer *fb;
> > +	const struct drm_display_mode *mode = &crtc_state->mode;
> > +
> > +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> > +		return 0;
> > +
> > +	fb = conn_state->writeback_job->fb;
> > +	if (fb->width != mode->hdisplay || fb->height != mode->vdisplay) {
> > +		DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
> > +			      fb->width, fb->height);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (fb->format->format != DRM_FORMAT_XRGB8888) {
> > +		struct drm_format_name_buf format_name;
> > +
> > +		DRM_DEBUG_KMS("Invalid pixel format %s\n",
> > +			      drm_get_format_name(fb->format->format,
> > +						  &format_name));
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {
> > +	.atomic_check = vkms_wb_encoder_atomic_check,
> > +};
> > +
> > +static int vkms_wb_connector_get_modes(struct drm_connector *connector)
> > +{
> > +	struct drm_device *dev = connector->dev;
> > +
> > +	return drm_add_modes_noedid(connector, dev->mode_config.max_width,
> > +				    dev->mode_config.max_height);
> > +}
> 
> Do we need a mode for writeb connector? Is that used by userspace to
> figure out the max size for writeback?

Yes, we need it. You're right about the userspace utilizing it to figure
out the max size writeback, kms_writeback strongly rely on this.

> > +
> > +static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
> > +			       struct drm_writeback_job *job)
> > +{
> > +	struct vkms_gem_object *vkms_obj;
> > +	struct drm_gem_object *gem_obj;
> > +	int ret;
> > +
> > +	if (!job->fb)
> > +		return 0;
> 
> Can this happen?

Yes, here is the path until this function is invoked:

  drm_atomic_helper_prepare_planes()

Inside the above function we have:

  for_each_new_connector_in_state(state, connector, new_conn_state, i)

which in turn invoke:

  drm_writeback_prepare_job(new_conn_state->writeback_job);

FWIU, new_conn_state->writeback_job could be NULL. Anyway, I also tested
it, and I notice this condition can be reached.
 
> > +
> > +	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > +	ret = vkms_gem_vmap(gem_obj);
> > +	if (ret) {
> > +		DRM_ERROR("vmap failed: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > +	job->priv = vkms_obj->vaddr;
> > +
> > +	return 0;
> > +}
> > +
> > +static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> > +				struct drm_writeback_job *job)
> > +{
> > +	struct drm_gem_object *gem_obj;
> > +
> > +	if (!job->fb)
> > +		return;
> 
> Same here?
> 
> > +
> > +	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > +	vkms_gem_vunmap(gem_obj);
> > +}
> > +
> > +static void vkms_wb_atomic_commit(struct drm_connector *conn,
> > +				  struct drm_connector_state *state)
> > +{
> > +	struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev);
> > +	struct vkms_output *output = &vkmsdev->output;
> > +	struct drm_writeback_connector *wb_conn = &output->wb_connector;
> > +	struct drm_connector_state *conn_state = wb_conn->base.state;
> > +	struct vkms_crtc_state *crtc_state = output->composer_state;
> > +
> > +	if (!conn_state)
> > +		return;
> > +
> > +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
> > +		DRM_DEBUG_DRIVER("Disable writeback\n");
> > +		return;
> > +	}
> 
> All three checks above sound like helpers should take care of this?

Maybe I missed something, but I did not find such helper. I also noticed
that vc4, rcar-du, and mali does the same check.
 
Thanks

> > +
> > +	spin_lock_irq(&output->composer_lock);
> > +	crtc_state->active_writeback = conn_state->writeback_job->priv;
> > +	crtc_state->wb_pending = true;
> > +	spin_unlock_irq(&output->composer_lock);
> > +	drm_writeback_queue_job(wb_conn, state);
> > +}
> > +
> > +static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> > +	.get_modes = vkms_wb_connector_get_modes,
> > +	.prepare_writeback_job = vkms_wb_prepare_job,
> > +	.cleanup_writeback_job = vkms_wb_cleanup_job,
> > +	.atomic_commit = vkms_wb_atomic_commit,
> > +};
> > +
> > +int enable_writeback_connector(struct vkms_device *vkmsdev)
> > +{
> > +	struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
> > +
> > +	vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
> > +	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
> > +
> > +	return drm_writeback_connector_init(&vkmsdev->drm, wb,
> > +					    &vkms_wb_connector_funcs,
> > +					    &vkms_wb_encoder_helper_funcs,
> > +					    vkms_wb_formats,
> > +					    ARRAY_SIZE(vkms_wb_formats));
> > +}
> 
> Overall looks like good approach.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Rodrigo Siqueira
https://siqueira.tech

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V3 4/5] drm/vkms: Compute CRC without change input data

[Daniel Vetter]

On Fri, Jul 12, 2019 at 12:14:49AM -0300, Rodrigo Siqueira wrote:
> On 07/11, Daniel Vetter wrote:
> > On Tue, Jun 25, 2019 at 10:38:31PM -0300, Rodrigo Siqueira wrote:
> > > The compute_crc() function is responsible for calculating the
> > > framebuffer CRC value; due to the XRGB format, this function has to
> > > ignore the alpha channel during the CRC computation. Therefore,
> > > compute_crc() set zero to the alpha channel directly in the input
> > > framebuffer, which is not a problem since this function receives a copy
> > > of the original buffer. However, if we want to use this function in a
> > > context without a buffer copy, it will change the initial value. This
> > > patch makes compute_crc() calculate the CRC value without modifying the
> > > input framebuffer.
> > 
> > Uh why? For writeback we're writing the output too, so we can write
> > whatever we want to into the alpha channel. For writeback we should never
> > accept a pixel format where alpha actually matters, that doesn't make
> > sense. You can't see through a real screen either, they are all opaque :-)
> > -Daniel
> 
> Hmmm,
> 
> I see your point and I agree, but even though we can write whatever we
> want in the output, don’t you think that is weird to change the
> framebuffer value in the compute_crc() function?

Not sure what you mean here ... ? From a quick look the memset only sets
our temporary buffer, so we're not changing the input framebuffer here.
And we have to somehow get rid of the X bits, since there's no alpha
value. For CRC computation, all we need is some value which is the same
for every frame (so that the CRC stays constant for the same visible
output). For writeback we could write whatever we want (which includes
whatever is there already). But there's no guarantee and definitely no
expectation that the X bits survive. Writing 0 is imo the most reasonable
thing to do. I'm not even sure whether modern gpus can still do channel
masking (i.e. only write out specific channels, instead of the entire
color). That was a "feature" of bitop blitters of the 80s/90s :-)
-Daniel

> 
> Thanks
>  
> > > 
> > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > ---
> > >  drivers/gpu/drm/vkms/vkms_composer.c | 31 +++++++++++++++++-----------
> > >  1 file changed, 19 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> > > index 51a270514219..8126aa0f968f 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> > > @@ -6,33 +6,40 @@
> > >  #include <drm/drm_atomic_helper.h>
> > >  #include <drm/drm_gem_framebuffer_helper.h>
> > >  
> > > +static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
> > > +				 const struct vkms_composer *composer)
> > > +{
> > > +	int src_offset = composer->offset + (y * composer->pitch)
> > > +					  + (x * composer->cpp);
> > > +
> > > +	return *(u32 *)&buffer[src_offset];
> > > +}
> > > +
> > >  /**
> > >   * compute_crc - Compute CRC value on output frame
> > >   *
> > > - * @vaddr_out: address to final framebuffer
> > > + * @vaddr: address to final framebuffer
> > >   * @composer: framebuffer's metadata
> > >   *
> > >   * returns CRC value computed using crc32 on the visible portion of
> > >   * the final framebuffer at vaddr_out
> > >   */
> > > -static uint32_t compute_crc(void *vaddr_out, struct vkms_composer *composer)
> > > +static uint32_t compute_crc(const u8 *vaddr,
> > > +			    const struct vkms_composer *composer)
> > >  {
> > > -	int i, j, src_offset;
> > > +	int x, y;
> > >  	int x_src = composer->src.x1 >> 16;
> > >  	int y_src = composer->src.y1 >> 16;
> > >  	int h_src = drm_rect_height(&composer->src) >> 16;
> > >  	int w_src = drm_rect_width(&composer->src) >> 16;
> > > -	u32 crc = 0;
> > > +	u32 crc = 0, pixel = 0;
> > >  
> > > -	for (i = y_src; i < y_src + h_src; ++i) {
> > > -		for (j = x_src; j < x_src + w_src; ++j) {
> > > -			src_offset = composer->offset
> > > -				     + (i * composer->pitch)
> > > -				     + (j * composer->cpp);
> > > +	for (y = y_src; y < y_src + h_src; ++y) {
> > > +		for (x = x_src; x < x_src + w_src; ++x) {
> > >  			/* XRGB format ignores Alpha channel */
> > > -			memset(vaddr_out + src_offset + 24, 0,  8);
> > > -			crc = crc32_le(crc, vaddr_out + src_offset,
> > > -				       sizeof(u32));
> > > +			pixel = get_pixel_from_buffer(x, y, vaddr, composer);
> > > +			bitmap_clear((void *)&pixel, 0, 8);
> > > +			crc = crc32_le(crc, (void *)&pixel, sizeof(u32));
> > >  		}
> > >  	}
> > >  
> > > -- 
> > > 2.21.0
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Rodrigo Siqueira
> https://siqueira.tech



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

Re: [PATCH V3 5/5] drm/vkms: Add support for writeback

[Daniel Vetter]

On Fri, Jul 12, 2019 at 12:37:22AM -0300, Rodrigo Siqueira wrote:
> On 07/11, Daniel Vetter wrote:
> > On Tue, Jun 25, 2019 at 10:39:03PM -0300, Rodrigo Siqueira wrote:
> > > This patch implements the necessary functions to add writeback support
> > > for vkms. This feature is useful for testing compositors if you don't
> > > have hardware with writeback support.
> > > 
> > > Change in V3 (Daniel):
> > > - If writeback is enabled, compose everything into the writeback buffer
> > > instead of CRC private buffer.
> > > - Guarantees that the CRC will match exactly what we have in the
> > > writeback buffer.
> > > 
> > > Change in V2:
> > > - Rework signal completion (Brian)
> > > - Integrates writeback with active_planes (Daniel)
> > > - Compose cursor (Daniel)
> > > 
> > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > ---
> > >  drivers/gpu/drm/vkms/Makefile         |   9 +-
> > >  drivers/gpu/drm/vkms/vkms_composer.c  |  16 ++-
> > >  drivers/gpu/drm/vkms/vkms_drv.c       |   4 +
> > >  drivers/gpu/drm/vkms/vkms_drv.h       |   8 ++
> > >  drivers/gpu/drm/vkms/vkms_output.c    |  10 ++
> > >  drivers/gpu/drm/vkms/vkms_writeback.c | 141 ++++++++++++++++++++++++++
> > >  6 files changed, 185 insertions(+), 3 deletions(-)
> > >  create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c
> > > 
> > > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> > > index 0b767d7efa24..333d3cead0e3 100644
> > > --- a/drivers/gpu/drm/vkms/Makefile
> > > +++ b/drivers/gpu/drm/vkms/Makefile
> > > @@ -1,4 +1,11 @@
> > >  # SPDX-License-Identifier: GPL-2.0-only
> > > -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_composer.o
> > > +vkms-y := \
> > > +	vkms_drv.o \
> > > +	vkms_plane.o \
> > > +	vkms_output.o \
> > > +	vkms_crtc.o \
> > > +	vkms_gem.o \
> > > +	vkms_composer.o \
> > > +	vkms_writeback.o
> > >  
> > >  obj-$(CONFIG_DRM_VKMS) += vkms.o
> > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> > > index 8126aa0f968f..2317803e7320 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> > > @@ -157,16 +157,17 @@ void vkms_composer_worker(struct work_struct *work)
> > >  	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
> > >  	struct vkms_composer *primary_composer = NULL;
> > >  	struct vkms_composer *cursor_composer = NULL;
> > > +	bool crc_pending, wb_pending;
> > 
> > I'm not seeing how you schedule this work for writeback if there's no crc
> > enabled. Does that work?
> 
> If we enable writeback, we set true for the flag composer_enabled (see
> vkms_output.c in this patch) which also make vkms_vblank_simulate able
> to handle crc. Since writeback is oneshot, we only enable it on
> vkms_wb_atomic_commit (crtc_state->wb_pending = true); in its turn
> vkms_composer_worker() use writeback buffer instead of creating a new
> one for computing crc. Finally, at the end of vkms_composer_worker() we
> disable the writeback.

Yeah I got all that. But who's doing the queue_work? That only happens
if crc_enabled is set, and I'm not seeing how that's changed. Might be in
one of the earlier patches ...
-Daniel

> 
> > >  	void *vaddr_out = NULL;
> > >  	u32 crc32 = 0;
> > >  	u64 frame_start, frame_end;
> > > -	bool crc_pending;
> > >  	int ret;
> > >  
> > >  	spin_lock_irq(&out->composer_lock);
> > >  	frame_start = crtc_state->frame_start;
> > >  	frame_end = crtc_state->frame_end;
> > >  	crc_pending = crtc_state->crc_pending;
> > > +	wb_pending = crtc_state->wb_pending;
> > >  	crtc_state->frame_start = 0;
> > >  	crtc_state->frame_end = 0;
> > >  	crtc_state->crc_pending = false;
> > > @@ -188,9 +189,12 @@ void vkms_composer_worker(struct work_struct *work)
> > >  	if (!primary_composer)
> > >  		return;
> > >  
> > > +	if (wb_pending)
> > > +		vaddr_out = crtc_state->active_writeback;
> > > +
> > >  	ret = compose_planes(&vaddr_out, primary_composer, cursor_composer);
> > >  	if (ret) {
> > > -		if (ret == -EINVAL)
> > > +		if (ret == -EINVAL && !wb_pending)
> > >  			kfree(vaddr_out);
> > >  		return;
> > >  	}
> > > @@ -203,6 +207,14 @@ void vkms_composer_worker(struct work_struct *work)
> > >  	while (frame_start <= frame_end)
> > >  		drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
> > >  
> > > +	if (wb_pending) {
> > > +		drm_writeback_signal_completion(&out->wb_connector, 0);
> > > +		spin_lock_irq(&out->composer_lock);
> > > +		crtc_state->wb_pending = false;
> > > +		spin_unlock_irq(&out->composer_lock);
> > > +		return;
> > > +	}
> > > +
> > >  	kfree(vaddr_out);
> > >  }
> > >  
> > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > index ac790b6527e4..152d7de24a76 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > @@ -30,6 +30,10 @@ bool enable_cursor;
> > >  module_param_named(enable_cursor, enable_cursor, bool, 0444);
> > >  MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
> > >  
> > > +bool enable_writeback;
> > > +module_param_named(enable_writeback, enable_writeback, bool, 0444);
> > > +MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector");
> > > +
> > >  static const struct file_operations vkms_driver_fops = {
> > >  	.owner		= THIS_MODULE,
> > >  	.open		= drm_open,
> > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > index fc6cda164336..9ff2cd4ebf81 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > @@ -7,6 +7,7 @@
> > >  #include <drm/drm.h>
> > >  #include <drm/drm_gem.h>
> > >  #include <drm/drm_encoder.h>
> > > +#include <drm/drm_writeback.h>
> > >  #include <linux/hrtimer.h>
> > >  
> > >  #define XRES_MIN    20
> > > @@ -19,6 +20,7 @@
> > >  #define YRES_MAX  8192
> > >  
> > >  extern bool enable_cursor;
> > > +extern bool enable_writeback;
> > >  
> > >  struct vkms_composer {
> > >  	struct drm_framebuffer fb;
> > > @@ -52,9 +54,11 @@ struct vkms_crtc_state {
> > >  	int num_active_planes;
> > >  	/* stack of active planes for crc computation, should be in z order */
> > >  	struct vkms_plane_state **active_planes;
> > > +	void *active_writeback;
> > >  
> > >  	/* below three are protected by vkms_output.composer_lock */
> > >  	bool crc_pending;
> > > +	bool wb_pending;
> > >  	u64 frame_start;
> > >  	u64 frame_end;
> > >  };
> > > @@ -63,6 +67,7 @@ struct vkms_output {
> > >  	struct drm_crtc crtc;
> > >  	struct drm_encoder encoder;
> > >  	struct drm_connector connector;
> > > +	struct drm_writeback_connector wb_connector;
> > >  	struct hrtimer vblank_hrtimer;
> > >  	ktime_t period_ns;
> > >  	struct drm_pending_vblank_event *event;
> > > @@ -147,4 +152,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> > >  /* Composer Support */
> > >  void vkms_composer_worker(struct work_struct *work);
> > >  
> > > +/* Writeback */
> > > +int enable_writeback_connector(struct vkms_device *vkmsdev);
> > > +
> > >  #endif /* _VKMS_DRV_H_ */
> > > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > > index fb1941a6522c..aea1d4410864 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > > @@ -84,6 +84,16 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> > >  		goto err_attach;
> > >  	}
> > >  
> > > +	if (enable_writeback) {
> > > +		ret = enable_writeback_connector(vkmsdev);
> > > +		if (!ret) {
> > > +			output->composer_enabled = true;
> > > +			DRM_INFO("Writeback connector enabled");
> > > +		} else {
> > > +			DRM_ERROR("Failed to init writeback connector\n");
> > > +		}
> > > +	}
> > > +
> > >  	drm_mode_config_reset(dev);
> > >  
> > >  	return 0;
> > > diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> > > new file mode 100644
> > > index 000000000000..34dad37a0236
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> > > @@ -0,0 +1,141 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +
> > > +#include "vkms_drv.h"
> > > +#include <drm/drm_writeback.h>
> > > +#include <drm/drm_probe_helper.h>
> > > +#include <drm/drm_atomic_helper.h>
> > > +#include <drm/drm_gem_framebuffer_helper.h>
> > > +
> > > +static const u32 vkms_wb_formats[] = {
> > > +	DRM_FORMAT_XRGB8888,
> > > +};
> > > +
> > > +static const struct drm_connector_funcs vkms_wb_connector_funcs = {
> > > +	.fill_modes = drm_helper_probe_single_connector_modes,
> > > +	.destroy = drm_connector_cleanup,
> > > +	.reset = drm_atomic_helper_connector_reset,
> > > +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > > +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > > +};
> > > +
> > > +static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
> > > +					struct drm_crtc_state *crtc_state,
> > > +					struct drm_connector_state *conn_state)
> > > +{
> > > +	struct drm_framebuffer *fb;
> > > +	const struct drm_display_mode *mode = &crtc_state->mode;
> > > +
> > > +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> > > +		return 0;
> > > +
> > > +	fb = conn_state->writeback_job->fb;
> > > +	if (fb->width != mode->hdisplay || fb->height != mode->vdisplay) {
> > > +		DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
> > > +			      fb->width, fb->height);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (fb->format->format != DRM_FORMAT_XRGB8888) {
> > > +		struct drm_format_name_buf format_name;
> > > +
> > > +		DRM_DEBUG_KMS("Invalid pixel format %s\n",
> > > +			      drm_get_format_name(fb->format->format,
> > > +						  &format_name));
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {
> > > +	.atomic_check = vkms_wb_encoder_atomic_check,
> > > +};
> > > +
> > > +static int vkms_wb_connector_get_modes(struct drm_connector *connector)
> > > +{
> > > +	struct drm_device *dev = connector->dev;
> > > +
> > > +	return drm_add_modes_noedid(connector, dev->mode_config.max_width,
> > > +				    dev->mode_config.max_height);
> > > +}
> > 
> > Do we need a mode for writeb connector? Is that used by userspace to
> > figure out the max size for writeback?
> 
> Yes, we need it. You're right about the userspace utilizing it to figure
> out the max size writeback, kms_writeback strongly rely on this.
> 
> > > +
> > > +static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
> > > +			       struct drm_writeback_job *job)
> > > +{
> > > +	struct vkms_gem_object *vkms_obj;
> > > +	struct drm_gem_object *gem_obj;
> > > +	int ret;
> > > +
> > > +	if (!job->fb)
> > > +		return 0;
> > 
> > Can this happen?
> 
> Yes, here is the path until this function is invoked:
> 
>   drm_atomic_helper_prepare_planes()
> 
> Inside the above function we have:
> 
>   for_each_new_connector_in_state(state, connector, new_conn_state, i)
> 
> which in turn invoke:
> 
>   drm_writeback_prepare_job(new_conn_state->writeback_job);
> 
> FWIU, new_conn_state->writeback_job could be NULL. Anyway, I also tested
> it, and I notice this condition can be reached.
>  
> > > +
> > > +	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > > +	ret = vkms_gem_vmap(gem_obj);
> > > +	if (ret) {
> > > +		DRM_ERROR("vmap failed: %d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > > +	job->priv = vkms_obj->vaddr;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> > > +				struct drm_writeback_job *job)
> > > +{
> > > +	struct drm_gem_object *gem_obj;
> > > +
> > > +	if (!job->fb)
> > > +		return;
> > 
> > Same here?
> > 
> > > +
> > > +	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> > > +	vkms_gem_vunmap(gem_obj);
> > > +}
> > > +
> > > +static void vkms_wb_atomic_commit(struct drm_connector *conn,
> > > +				  struct drm_connector_state *state)
> > > +{
> > > +	struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev);
> > > +	struct vkms_output *output = &vkmsdev->output;
> > > +	struct drm_writeback_connector *wb_conn = &output->wb_connector;
> > > +	struct drm_connector_state *conn_state = wb_conn->base.state;
> > > +	struct vkms_crtc_state *crtc_state = output->composer_state;
> > > +
> > > +	if (!conn_state)
> > > +		return;
> > > +
> > > +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
> > > +		DRM_DEBUG_DRIVER("Disable writeback\n");
> > > +		return;
> > > +	}
> > 
> > All three checks above sound like helpers should take care of this?
> 
> Maybe I missed something, but I did not find such helper. I also noticed
> that vc4, rcar-du, and mali does the same check.
>  
> Thanks
> 
> > > +
> > > +	spin_lock_irq(&output->composer_lock);
> > > +	crtc_state->active_writeback = conn_state->writeback_job->priv;
> > > +	crtc_state->wb_pending = true;
> > > +	spin_unlock_irq(&output->composer_lock);
> > > +	drm_writeback_queue_job(wb_conn, state);
> > > +}
> > > +
> > > +static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> > > +	.get_modes = vkms_wb_connector_get_modes,
> > > +	.prepare_writeback_job = vkms_wb_prepare_job,
> > > +	.cleanup_writeback_job = vkms_wb_cleanup_job,
> > > +	.atomic_commit = vkms_wb_atomic_commit,
> > > +};
> > > +
> > > +int enable_writeback_connector(struct vkms_device *vkmsdev)
> > > +{
> > > +	struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
> > > +
> > > +	vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
> > > +	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
> > > +
> > > +	return drm_writeback_connector_init(&vkmsdev->drm, wb,
> > > +					    &vkms_wb_connector_funcs,
> > > +					    &vkms_wb_encoder_helper_funcs,
> > > +					    vkms_wb_formats,
> > > +					    ARRAY_SIZE(vkms_wb_formats));
> > > +}
> > 
> > Overall looks like good approach.
> > -Daniel
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Rodrigo Siqueira
> https://siqueira.tech



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

Re: [PATCH V3 4/5] drm/vkms: Compute CRC without change input data

[Rodrigo Siqueira]

[-- Attachment #1: Type: text/plain, Size: 5547 bytes --]

On 07/16, Daniel Vetter wrote:
> On Fri, Jul 12, 2019 at 12:14:49AM -0300, Rodrigo Siqueira wrote:
> > On 07/11, Daniel Vetter wrote:
> > > On Tue, Jun 25, 2019 at 10:38:31PM -0300, Rodrigo Siqueira wrote:
> > > > The compute_crc() function is responsible for calculating the
> > > > framebuffer CRC value; due to the XRGB format, this function has to
> > > > ignore the alpha channel during the CRC computation. Therefore,
> > > > compute_crc() set zero to the alpha channel directly in the input
> > > > framebuffer, which is not a problem since this function receives a copy
> > > > of the original buffer. However, if we want to use this function in a
> > > > context without a buffer copy, it will change the initial value. This
> > > > patch makes compute_crc() calculate the CRC value without modifying the
> > > > input framebuffer.
> > > 
> > > Uh why? For writeback we're writing the output too, so we can write
> > > whatever we want to into the alpha channel. For writeback we should never
> > > accept a pixel format where alpha actually matters, that doesn't make
> > > sense. You can't see through a real screen either, they are all opaque :-)
> > > -Daniel
> > 
> > Hmmm,
> > 
> > I see your point and I agree, but even though we can write whatever we
> > want in the output, don’t you think that is weird to change the
> > framebuffer value in the compute_crc() function?
> 
> Not sure what you mean here ... ? From a quick look the memset only sets
> our temporary buffer, so we're not changing the input framebuffer here.
> And we have to somehow get rid of the X bits, since there's no alpha
> value. For CRC computation, all we need is some value which is the same
> for every frame (so that the CRC stays constant for the same visible
> output). For writeback we could write whatever we want (which includes
> whatever is there already). But there's no guarantee and definitely no
> expectation that the X bits survive. Writing 0 is imo the most reasonable
> thing to do. I'm not even sure whether modern gpus can still do channel
> masking (i.e. only write out specific channels, instead of the entire
> color). That was a "feature" of bitop blitters of the 80s/90s :-)
> -Daniel

Ahhh, now I can see that my mindset was in the 90s :)

Thanks
 
> > 
> > Thanks
> >  
> > > > 
> > > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > > ---
> > > >  drivers/gpu/drm/vkms/vkms_composer.c | 31 +++++++++++++++++-----------
> > > >  1 file changed, 19 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> > > > index 51a270514219..8126aa0f968f 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > > > +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> > > > @@ -6,33 +6,40 @@
> > > >  #include <drm/drm_atomic_helper.h>
> > > >  #include <drm/drm_gem_framebuffer_helper.h>
> > > >  
> > > > +static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
> > > > +				 const struct vkms_composer *composer)
> > > > +{
> > > > +	int src_offset = composer->offset + (y * composer->pitch)
> > > > +					  + (x * composer->cpp);
> > > > +
> > > > +	return *(u32 *)&buffer[src_offset];
> > > > +}
> > > > +
> > > >  /**
> > > >   * compute_crc - Compute CRC value on output frame
> > > >   *
> > > > - * @vaddr_out: address to final framebuffer
> > > > + * @vaddr: address to final framebuffer
> > > >   * @composer: framebuffer's metadata
> > > >   *
> > > >   * returns CRC value computed using crc32 on the visible portion of
> > > >   * the final framebuffer at vaddr_out
> > > >   */
> > > > -static uint32_t compute_crc(void *vaddr_out, struct vkms_composer *composer)
> > > > +static uint32_t compute_crc(const u8 *vaddr,
> > > > +			    const struct vkms_composer *composer)
> > > >  {
> > > > -	int i, j, src_offset;
> > > > +	int x, y;
> > > >  	int x_src = composer->src.x1 >> 16;
> > > >  	int y_src = composer->src.y1 >> 16;
> > > >  	int h_src = drm_rect_height(&composer->src) >> 16;
> > > >  	int w_src = drm_rect_width(&composer->src) >> 16;
> > > > -	u32 crc = 0;
> > > > +	u32 crc = 0, pixel = 0;
> > > >  
> > > > -	for (i = y_src; i < y_src + h_src; ++i) {
> > > > -		for (j = x_src; j < x_src + w_src; ++j) {
> > > > -			src_offset = composer->offset
> > > > -				     + (i * composer->pitch)
> > > > -				     + (j * composer->cpp);
> > > > +	for (y = y_src; y < y_src + h_src; ++y) {
> > > > +		for (x = x_src; x < x_src + w_src; ++x) {
> > > >  			/* XRGB format ignores Alpha channel */
> > > > -			memset(vaddr_out + src_offset + 24, 0,  8);
> > > > -			crc = crc32_le(crc, vaddr_out + src_offset,
> > > > -				       sizeof(u32));
> > > > +			pixel = get_pixel_from_buffer(x, y, vaddr, composer);
> > > > +			bitmap_clear((void *)&pixel, 0, 8);
> > > > +			crc = crc32_le(crc, (void *)&pixel, sizeof(u32));
> > > >  		}
> > > >  	}
> > > >  
> > > > -- 
> > > > 2.21.0
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > 
> > -- 
> > Rodrigo Siqueira
> > https://siqueira.tech
> 
> 
> 
> > _______________________________________________
> > 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

-- 
Rodrigo Siqueira
https://siqueira.tech

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]