[PATCH 0/2] Compute CRC with cursor plane support

[PATCH 0/2] Compute CRC with cursor plane support

[Haneen Mohammed]

This patchset add support for cursor plane and compute CRC for output
frame with cursor and primary planes.

This currently passes cursor-size-change, onscreen, offscreen, sliding,
random, dpms, and rapid-movement igt tests with 64x64 cursor.

Haneen Mohammed (2):
  drm/vkms: Add cursor plane support
  drm/vkms: Compute CRC with Cursor Plane

 drivers/gpu/drm/vkms/vkms_crc.c    | 149 ++++++++++++++++++++++++-----
 drivers/gpu/drm/vkms/vkms_drv.h    |  16 +++-
 drivers/gpu/drm/vkms/vkms_output.c |  16 +++-
 drivers/gpu/drm/vkms/vkms_plane.c  |  43 ++++++---
 4 files changed, 181 insertions(+), 43 deletions(-)

-- 
2.17.1

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

[PATCH 1/2] drm/vkms: Add cursor plane support

[Haneen Mohammed]

Add cursor plane support and update vkms_plane_atomic_check to enable
positioning cursor plane.

Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_drv.h    | 11 +++++++---
 drivers/gpu/drm/vkms/vkms_output.c | 16 ++++++++++++---
 drivers/gpu/drm/vkms/vkms_plane.c  | 33 +++++++++++++++++++++---------
 3 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index f156c930366a..36e524f791fe 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -7,8 +7,8 @@
 #include <drm/drm_encoder.h>
 #include <linux/hrtimer.h>
 
-#define XRES_MIN    32
-#define YRES_MIN    32
+#define XRES_MIN    20
+#define YRES_MIN    20
 
 #define XRES_DEF  1024
 #define YRES_DEF   768
@@ -20,6 +20,10 @@ static const u32 vkms_formats[] = {
 	DRM_FORMAT_XRGB8888,
 };
 
+static const u32 vkms_cursor_formats[] = {
+	DRM_FORMAT_ARGB8888,
+};
+
 struct vkms_crc_data {
 	struct drm_rect src;
 	struct drm_framebuffer fb;
@@ -100,7 +104,8 @@ bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
 
 int vkms_output_init(struct vkms_device *vkmsdev);
 
-struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev);
+struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
+				  enum drm_plane_type type);
 
 /* 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 901012cb1af1..7cf2867c93f8 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -49,14 +49,20 @@ int vkms_output_init(struct vkms_device *vkmsdev)
 	struct drm_connector *connector = &output->connector;
 	struct drm_encoder *encoder = &output->encoder;
 	struct drm_crtc *crtc = &output->crtc;
-	struct drm_plane *primary;
+	struct drm_plane *primary, *cursor;
 	int ret;
 
-	primary = vkms_plane_init(vkmsdev);
+	primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY);
 	if (IS_ERR(primary))
 		return PTR_ERR(primary);
 
-	ret = vkms_crtc_init(dev, crtc, primary, NULL);
+	cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR);
+	if (IS_ERR(cursor)) {
+		ret = PTR_ERR(cursor);
+		goto err_cursor;
+	}
+
+	ret = vkms_crtc_init(dev, crtc, primary, cursor);
 	if (ret)
 		goto err_crtc;
 
@@ -106,6 +112,10 @@ int vkms_output_init(struct vkms_device *vkmsdev)
 	drm_crtc_cleanup(crtc);
 
 err_crtc:
+	drm_plane_cleanup(cursor);
+
+err_cursor:
 	drm_plane_cleanup(primary);
+
 	return ret;
 }
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index c91661631c76..428247d403dc 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -81,8 +81,8 @@ static const struct drm_plane_funcs vkms_plane_funcs = {
 	.atomic_destroy_state	= vkms_plane_destroy_state,
 };
 
-static void vkms_primary_plane_update(struct drm_plane *plane,
-				      struct drm_plane_state *old_state)
+static void vkms_plane_atomic_update(struct drm_plane *plane,
+				     struct drm_plane_state *old_state)
 {
 	struct vkms_plane_state *vkms_plane_state;
 	struct vkms_crc_data *crc_data;
@@ -101,6 +101,7 @@ static int vkms_plane_atomic_check(struct drm_plane *plane,
 				   struct drm_plane_state *state)
 {
 	struct drm_crtc_state *crtc_state;
+	bool can_position = false;
 	int ret;
 
 	if (!state->fb | !state->crtc)
@@ -110,15 +111,18 @@ static int vkms_plane_atomic_check(struct drm_plane *plane,
 	if (IS_ERR(crtc_state))
 		return PTR_ERR(crtc_state);
 
+	if (plane->type == DRM_PLANE_TYPE_CURSOR)
+		can_position = true;
+
 	ret = drm_atomic_helper_check_plane_state(state, crtc_state,
 						  DRM_PLANE_HELPER_NO_SCALING,
 						  DRM_PLANE_HELPER_NO_SCALING,
-						  false, true);
+						  can_position, true);
 	if (ret != 0)
 		return ret;
 
 	/* for now primary plane must be visible and full screen */
-	if (!state->visible)
+	if (!state->visible && !can_position)
 		return -EINVAL;
 
 	return 0;
@@ -156,15 +160,17 @@ static void vkms_cleanup_fb(struct drm_plane *plane,
 }
 
 static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
-	.atomic_update		= vkms_primary_plane_update,
+	.atomic_update		= vkms_plane_atomic_update,
 	.atomic_check		= vkms_plane_atomic_check,
 	.prepare_fb		= vkms_prepare_fb,
 	.cleanup_fb		= vkms_cleanup_fb,
 };
 
-struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev)
+struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev,
+				  enum drm_plane_type type)
 {
 	struct drm_device *dev = &vkmsdev->drm;
+	const struct drm_plane_helper_funcs *funcs;
 	struct drm_plane *plane;
 	const u32 *formats;
 	int ret, nformats;
@@ -173,19 +179,26 @@ struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev)
 	if (!plane)
 		return ERR_PTR(-ENOMEM);
 
-	formats = vkms_formats;
-	nformats = ARRAY_SIZE(vkms_formats);
+	if (type == DRM_PLANE_TYPE_CURSOR) {
+		formats = vkms_cursor_formats;
+		nformats = ARRAY_SIZE(vkms_cursor_formats);
+		funcs = &vkms_primary_helper_funcs;
+	} else {
+		formats = vkms_formats;
+		nformats = ARRAY_SIZE(vkms_formats);
+		funcs = &vkms_primary_helper_funcs;
+	}
 
 	ret = drm_universal_plane_init(dev, plane, 0,
 				       &vkms_plane_funcs,
 				       formats, nformats,
-				       NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
+				       NULL, type, NULL);
 	if (ret) {
 		kfree(plane);
 		return ERR_PTR(ret);
 	}
 
-	drm_plane_helper_add(plane, &vkms_primary_helper_funcs);
+	drm_plane_helper_add(plane, funcs);
 
 	return plane;
 }
-- 
2.17.1

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

[PATCH 2/2] drm/vkms: Compute CRC with Cursor Plane

[Haneen Mohammed]

This patch compute CRC for output frame with cursor and primary plane.
Blend cursor with primary plane and compute CRC on the resulted frame.

Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_crc.c   | 149 +++++++++++++++++++++++++-----
 drivers/gpu/drm/vkms/vkms_drv.h   |   5 +-
 drivers/gpu/drm/vkms/vkms_plane.c |  10 +-
 3 files changed, 137 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
index 37d717f38e3c..4853a739ae5a 100644
--- a/drivers/gpu/drm/vkms/vkms_crc.c
+++ b/drivers/gpu/drm/vkms/vkms_crc.c
@@ -1,36 +1,137 @@
 // SPDX-License-Identifier: GPL-2.0
 #include "vkms_drv.h"
 #include <linux/crc32.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 
-static uint32_t _vkms_get_crc(struct vkms_crc_data *crc_data)
+/**
+ * compute_crc - Compute CRC value on output frame
+ *
+ * @vaddr_out: address to final framebuffer
+ * @crc_out: 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)
 {
-	struct drm_framebuffer *fb = &crc_data->fb;
+	int i, 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 size_byte = w_src * crc_out->cpp;
+	u32 crc = 0;
+
+	for (i = y_src; i < y_src + h_src; i++) {
+		src_offset = crc_out->offset
+			     + (i * crc_out->pitch)
+			     + (x_src * crc_out->cpp);
+		crc = crc32_le(crc, vaddr_out + src_offset, size_byte);
+	}
+
+	return crc;
+}
+
+/**
+ * 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
+ *
+ * Blend value at vaddr_src with value at vaddr_dst.
+ * Currently, this function write value at vaddr_src on value
+ * at vaddr_dst using buffer's metadata to locate the new values
+ * from vaddr_src and their distenation at vaddr_dst.
+ *
+ * Todo: Use the alpha value to blend vaddr_src with vaddr_dst
+ *	 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)
+{
+	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_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 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);
+
+			memcpy(vaddr_dst + offset_dst,
+			       vaddr_src + offset_src, sizeof(u32));
+		}
+		i_dst++;
+	}
+}
+
+static void compose_cursor(struct vkms_crc_data *cursor_crc,
+			   struct vkms_crc_data *primary_crc, 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_vkms_obj = drm_gem_to_vkms_gem(cursor_obj);
+
+	mutex_lock(&cursor_vkms_obj->pages_lock);
+	if (WARN_ON(!cursor_vkms_obj->vaddr))
+		goto out;
+
+	blend(vaddr_out, cursor_vkms_obj->vaddr, primary_crc, cursor_crc);
+
+out:
+	mutex_unlock(&cursor_vkms_obj->pages_lock);
+}
+
+static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
+			      struct vkms_crc_data *cursor_crc)
+{
+	struct drm_framebuffer *fb = &primary_crc->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;
-	int i = 0;
-	unsigned int x = crc_data->src.x1 >> 16;
-	unsigned int y = crc_data->src.y1 >> 16;
-	unsigned int height = drm_rect_height(&crc_data->src) >> 16;
-	unsigned int width = drm_rect_width(&crc_data->src) >> 16;
-	unsigned int cpp = fb->format->cpp[0];
-	unsigned int src_offset;
-	unsigned int size_byte = width * cpp;
-	void *vaddr;
 
-	mutex_lock(&vkms_obj->pages_lock);
-	vaddr = vkms_obj->vaddr;
-	if (WARN_ON(!vaddr))
-		goto out;
+	if (!vaddr_out) {
+		DRM_ERROR("Failed to allocate memory for output frame.");
+		return 0;
+	}
 
-	for (i = y; i < y + height; i++) {
-		src_offset = fb->offsets[0] + (i * fb->pitches[0]) + (x * cpp);
-		crc = crc32_le(crc, vaddr + src_offset, size_byte);
+	mutex_lock(&vkms_obj->pages_lock);
+	if (WARN_ON(!vkms_obj->vaddr)) {
+		mutex_lock(&vkms_obj->pages_lock);
+		return crc;
 	}
 
-out:
+	memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
 	mutex_unlock(&vkms_obj->pages_lock);
+
+	if (cursor_crc)
+		compose_cursor(cursor_crc, primary_crc, vaddr_out);
+
+	crc = compute_crc(vaddr_out, primary_crc);
+
+	kfree(vaddr_out);
+
 	return crc;
 }
 
@@ -44,8 +145,8 @@ void vkms_crc_work_handle(struct work_struct *work)
 	struct vkms_device *vdev = container_of(out, struct vkms_device,
 						output);
 	struct vkms_crc_data *primary_crc = NULL;
+	struct vkms_crc_data *cursor_crc = NULL;
 	struct drm_plane *plane;
-
 	u32 crc32 = 0;
 
 	drm_for_each_plane(plane, &vdev->drm) {
@@ -58,14 +159,14 @@ void vkms_crc_work_handle(struct work_struct *work)
 		if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
 			continue;
 
-		if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
+		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
 			primary_crc = crc_data;
-			break;
-		}
+		else
+			cursor_crc = crc_data;
 	}
 
 	if (primary_crc)
-		crc32 = _vkms_get_crc(primary_crc);
+		crc32 = _vkms_get_crc(primary_crc, cursor_crc);
 
 	drm_crtc_add_crc_entry(crtc, true, crtc_state->n_frame, &crc32);
 }
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 36e524f791fe..173875dc361e 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -25,8 +25,11 @@ static const u32 vkms_cursor_formats[] = {
 };
 
 struct vkms_crc_data {
-	struct drm_rect src;
 	struct drm_framebuffer fb;
+	struct drm_rect src, dst;
+	unsigned int offset;
+	unsigned int pitch;
+	unsigned int cpp;
 };
 
 /**
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 428247d403dc..7041007396ae 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -85,16 +85,22 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
 				     struct drm_plane_state *old_state)
 {
 	struct vkms_plane_state *vkms_plane_state;
+	struct drm_framebuffer *fb = plane->state->fb;
 	struct vkms_crc_data *crc_data;
 
-	if (!plane->state->crtc || !plane->state->fb)
+	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->fb, plane->state->fb, sizeof(struct drm_framebuffer));
+	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];
 }
 
 static int vkms_plane_atomic_check(struct drm_plane *plane,
-- 
2.17.1

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

Re: [PATCH 2/2] drm/vkms: Compute CRC with Cursor Plane

[Daniel Vetter]

On Mon, Aug 06, 2018 at 06:58:29AM +0300, Haneen Mohammed wrote:
> This patch compute CRC for output frame with cursor and primary plane.
> Blend cursor with primary plane and compute CRC on the resulted frame.
> 
> Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>

Nice!

I assume with this you're passing all the crc based cursor tests in igt?
If so, please mention this in the commit message, so that there's a record
of the testing done on this.

One fairly huge gap we iirc have in our cursor testing is that there's not
much (if any?) alpha blending coverage. We kinda need that to make sure
this all works correctly. The usual trick is to only check the extreme
alpha values, i.e. fully opaque and fully transparent, since intermediate
values are affected by hw-specific rounding modes.
> ---
>  drivers/gpu/drm/vkms/vkms_crc.c   | 149 +++++++++++++++++++++++++-----
>  drivers/gpu/drm/vkms/vkms_drv.h   |   5 +-
>  drivers/gpu/drm/vkms/vkms_plane.c |  10 +-
>  3 files changed, 137 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> index 37d717f38e3c..4853a739ae5a 100644
> --- a/drivers/gpu/drm/vkms/vkms_crc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> @@ -1,36 +1,137 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include "vkms_drv.h"
>  #include <linux/crc32.h>
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
>  
> -static uint32_t _vkms_get_crc(struct vkms_crc_data *crc_data)
> +/**
> + * compute_crc - Compute CRC value on output frame
> + *
> + * @vaddr_out: address to final framebuffer
> + * @crc_out: 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)
>  {
> -	struct drm_framebuffer *fb = &crc_data->fb;
> +	int i, 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 size_byte = w_src * crc_out->cpp;
> +	u32 crc = 0;
> +
> +	for (i = y_src; i < y_src + h_src; i++) {
> +		src_offset = crc_out->offset
> +			     + (i * crc_out->pitch)
> +			     + (x_src * crc_out->cpp);
> +		crc = crc32_le(crc, vaddr_out + src_offset, size_byte);
> +	}
> +
> +	return crc;
> +}
> +
> +/**
> + * 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
> + *
> + * Blend value at vaddr_src with value at vaddr_dst.
> + * Currently, this function write value at vaddr_src on value
> + * at vaddr_dst using buffer's metadata to locate the new values
> + * from vaddr_src and their distenation at vaddr_dst.
> + *
> + * Todo: Use the alpha value to blend vaddr_src with vaddr_dst
> + *	 instead of overwriting it.

Another todo: We must clear the alpha channel in the result after
blending. This also applies to the primary plane, where the XRGB for the
pixel format mandates that we entirely ignore the alpha channel.

This is also something we should probably have an igt testcase for.

Since there's a few open ends: How many weeks are left in your outreachy
internship? We need to make sure that at least all the issues are covered
in a vkms todo file. Would be great to add that in
Documentation/gpu/vkms.rst, like we have for other drivers.
-Daniel

> + */
> +static void blend(void *vaddr_dst, void *vaddr_src,
> +		  struct vkms_crc_data *crc_dst,
> +		  struct vkms_crc_data *crc_src)
> +{
> +	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_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 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);
> +
> +			memcpy(vaddr_dst + offset_dst,
> +			       vaddr_src + offset_src, sizeof(u32));
> +		}
> +		i_dst++;
> +	}
> +}
> +
> +static void compose_cursor(struct vkms_crc_data *cursor_crc,
> +			   struct vkms_crc_data *primary_crc, 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_vkms_obj = drm_gem_to_vkms_gem(cursor_obj);
> +
> +	mutex_lock(&cursor_vkms_obj->pages_lock);
> +	if (WARN_ON(!cursor_vkms_obj->vaddr))
> +		goto out;
> +
> +	blend(vaddr_out, cursor_vkms_obj->vaddr, primary_crc, cursor_crc);
> +
> +out:
> +	mutex_unlock(&cursor_vkms_obj->pages_lock);
> +}
> +
> +static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
> +			      struct vkms_crc_data *cursor_crc)
> +{
> +	struct drm_framebuffer *fb = &primary_crc->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;
> -	int i = 0;
> -	unsigned int x = crc_data->src.x1 >> 16;
> -	unsigned int y = crc_data->src.y1 >> 16;
> -	unsigned int height = drm_rect_height(&crc_data->src) >> 16;
> -	unsigned int width = drm_rect_width(&crc_data->src) >> 16;
> -	unsigned int cpp = fb->format->cpp[0];
> -	unsigned int src_offset;
> -	unsigned int size_byte = width * cpp;
> -	void *vaddr;
>  
> -	mutex_lock(&vkms_obj->pages_lock);
> -	vaddr = vkms_obj->vaddr;
> -	if (WARN_ON(!vaddr))
> -		goto out;
> +	if (!vaddr_out) {
> +		DRM_ERROR("Failed to allocate memory for output frame.");
> +		return 0;
> +	}
>  
> -	for (i = y; i < y + height; i++) {
> -		src_offset = fb->offsets[0] + (i * fb->pitches[0]) + (x * cpp);
> -		crc = crc32_le(crc, vaddr + src_offset, size_byte);
> +	mutex_lock(&vkms_obj->pages_lock);
> +	if (WARN_ON(!vkms_obj->vaddr)) {
> +		mutex_lock(&vkms_obj->pages_lock);
> +		return crc;
>  	}
>  
> -out:
> +	memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
>  	mutex_unlock(&vkms_obj->pages_lock);
> +
> +	if (cursor_crc)
> +		compose_cursor(cursor_crc, primary_crc, vaddr_out);
> +
> +	crc = compute_crc(vaddr_out, primary_crc);
> +
> +	kfree(vaddr_out);
> +
>  	return crc;
>  }
>  
> @@ -44,8 +145,8 @@ void vkms_crc_work_handle(struct work_struct *work)
>  	struct vkms_device *vdev = container_of(out, struct vkms_device,
>  						output);
>  	struct vkms_crc_data *primary_crc = NULL;
> +	struct vkms_crc_data *cursor_crc = NULL;
>  	struct drm_plane *plane;
> -
>  	u32 crc32 = 0;
>  
>  	drm_for_each_plane(plane, &vdev->drm) {
> @@ -58,14 +159,14 @@ void vkms_crc_work_handle(struct work_struct *work)
>  		if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
>  			continue;
>  
> -		if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> +		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
>  			primary_crc = crc_data;
> -			break;
> -		}
> +		else
> +			cursor_crc = crc_data;
>  	}
>  
>  	if (primary_crc)
> -		crc32 = _vkms_get_crc(primary_crc);
> +		crc32 = _vkms_get_crc(primary_crc, cursor_crc);
>  
>  	drm_crtc_add_crc_entry(crtc, true, crtc_state->n_frame, &crc32);
>  }
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 36e524f791fe..173875dc361e 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -25,8 +25,11 @@ static const u32 vkms_cursor_formats[] = {
>  };
>  
>  struct vkms_crc_data {
> -	struct drm_rect src;
>  	struct drm_framebuffer fb;
> +	struct drm_rect src, dst;
> +	unsigned int offset;
> +	unsigned int pitch;
> +	unsigned int cpp;
>  };
>  
>  /**
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 428247d403dc..7041007396ae 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -85,16 +85,22 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
>  				     struct drm_plane_state *old_state)
>  {
>  	struct vkms_plane_state *vkms_plane_state;
> +	struct drm_framebuffer *fb = plane->state->fb;
>  	struct vkms_crc_data *crc_data;
>  
> -	if (!plane->state->crtc || !plane->state->fb)
> +	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->fb, plane->state->fb, sizeof(struct drm_framebuffer));
> +	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];
>  }
>  
>  static int vkms_plane_atomic_check(struct drm_plane *plane,
> -- 
> 2.17.1
> 

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

Re: [PATCH 2/2] drm/vkms: Compute CRC with Cursor Plane

[Haneen Mohammed]

On Tue, Aug 07, 2018 at 06:33:36PM +0200, Daniel Vetter wrote:
> On Mon, Aug 06, 2018 at 06:58:29AM +0300, Haneen Mohammed wrote:
> > This patch compute CRC for output frame with cursor and primary plane.
> > Blend cursor with primary plane and compute CRC on the resulted frame.
> > 
> > Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
> 
> Nice!
> 
> I assume with this you're passing all the crc based cursor tests in igt?
> If so, please mention this in the commit message, so that there's a record
> of the testing done on this.
> 

Sure, I'll update the commit message.
Is there any other change I should add/fix to this patchset?

> One fairly huge gap we iirc have in our cursor testing is that there's not
> much (if any?) alpha blending coverage. We kinda need that to make sure
> this all works correctly. The usual trick is to only check the extreme
> alpha values, i.e. fully opaque and fully transparent, since intermediate
> values are affected by hw-specific rounding modes.
> > ---
> >  drivers/gpu/drm/vkms/vkms_crc.c   | 149 +++++++++++++++++++++++++-----
> >  drivers/gpu/drm/vkms/vkms_drv.h   |   5 +-
> >  drivers/gpu/drm/vkms/vkms_plane.c |  10 +-
> >  3 files changed, 137 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> > index 37d717f38e3c..4853a739ae5a 100644
> > --- a/drivers/gpu/drm/vkms/vkms_crc.c
> > +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> > @@ -1,36 +1,137 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  #include "vkms_drv.h"
> >  #include <linux/crc32.h>
> > +#include <drm/drm_atomic.h>
> > +#include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_gem_framebuffer_helper.h>
> >  
> > -static uint32_t _vkms_get_crc(struct vkms_crc_data *crc_data)
> > +/**
> > + * compute_crc - Compute CRC value on output frame
> > + *
> > + * @vaddr_out: address to final framebuffer
> > + * @crc_out: 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)
> >  {
> > -	struct drm_framebuffer *fb = &crc_data->fb;
> > +	int i, 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 size_byte = w_src * crc_out->cpp;
> > +	u32 crc = 0;
> > +
> > +	for (i = y_src; i < y_src + h_src; i++) {
> > +		src_offset = crc_out->offset
> > +			     + (i * crc_out->pitch)
> > +			     + (x_src * crc_out->cpp);
> > +		crc = crc32_le(crc, vaddr_out + src_offset, size_byte);
> > +	}
> > +
> > +	return crc;
> > +}
> > +
> > +/**
> > + * 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
> > + *
> > + * Blend value at vaddr_src with value at vaddr_dst.
> > + * Currently, this function write value at vaddr_src on value
> > + * at vaddr_dst using buffer's metadata to locate the new values
> > + * from vaddr_src and their distenation at vaddr_dst.
> > + *
> > + * Todo: Use the alpha value to blend vaddr_src with vaddr_dst
> > + *	 instead of overwriting it.
> 
> Another todo: We must clear the alpha channel in the result after
> blending. This also applies to the primary plane, where the XRGB for the
> pixel format mandates that we entirely ignore the alpha channel.
> 
> This is also something we should probably have an igt testcase for.
> 
> Since there's a few open ends: How many weeks are left in your outreachy
> internship? We need to make sure that at least all the issues are covered
> in a vkms todo file. Would be great to add that in
> Documentation/gpu/vkms.rst, like we have for other drivers.
> -Daniel
> 

I've one more week! I can use this week to prepare the todo file and
finalize this patch?

Thank you,
Haneen

> > + */
> > +static void blend(void *vaddr_dst, void *vaddr_src,
> > +		  struct vkms_crc_data *crc_dst,
> > +		  struct vkms_crc_data *crc_src)
> > +{
> > +	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_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 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);
> > +
> > +			memcpy(vaddr_dst + offset_dst,
> > +			       vaddr_src + offset_src, sizeof(u32));
> > +		}
> > +		i_dst++;
> > +	}
> > +}
> > +
> > +static void compose_cursor(struct vkms_crc_data *cursor_crc,
> > +			   struct vkms_crc_data *primary_crc, 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_vkms_obj = drm_gem_to_vkms_gem(cursor_obj);
> > +
> > +	mutex_lock(&cursor_vkms_obj->pages_lock);
> > +	if (WARN_ON(!cursor_vkms_obj->vaddr))
> > +		goto out;
> > +
> > +	blend(vaddr_out, cursor_vkms_obj->vaddr, primary_crc, cursor_crc);
> > +
> > +out:
> > +	mutex_unlock(&cursor_vkms_obj->pages_lock);
> > +}
> > +
> > +static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
> > +			      struct vkms_crc_data *cursor_crc)
> > +{
> > +	struct drm_framebuffer *fb = &primary_crc->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;
> > -	int i = 0;
> > -	unsigned int x = crc_data->src.x1 >> 16;
> > -	unsigned int y = crc_data->src.y1 >> 16;
> > -	unsigned int height = drm_rect_height(&crc_data->src) >> 16;
> > -	unsigned int width = drm_rect_width(&crc_data->src) >> 16;
> > -	unsigned int cpp = fb->format->cpp[0];
> > -	unsigned int src_offset;
> > -	unsigned int size_byte = width * cpp;
> > -	void *vaddr;
> >  
> > -	mutex_lock(&vkms_obj->pages_lock);
> > -	vaddr = vkms_obj->vaddr;
> > -	if (WARN_ON(!vaddr))
> > -		goto out;
> > +	if (!vaddr_out) {
> > +		DRM_ERROR("Failed to allocate memory for output frame.");
> > +		return 0;
> > +	}
> >  
> > -	for (i = y; i < y + height; i++) {
> > -		src_offset = fb->offsets[0] + (i * fb->pitches[0]) + (x * cpp);
> > -		crc = crc32_le(crc, vaddr + src_offset, size_byte);
> > +	mutex_lock(&vkms_obj->pages_lock);
> > +	if (WARN_ON(!vkms_obj->vaddr)) {
> > +		mutex_lock(&vkms_obj->pages_lock);
> > +		return crc;
> >  	}
> >  
> > -out:
> > +	memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
> >  	mutex_unlock(&vkms_obj->pages_lock);
> > +
> > +	if (cursor_crc)
> > +		compose_cursor(cursor_crc, primary_crc, vaddr_out);
> > +
> > +	crc = compute_crc(vaddr_out, primary_crc);
> > +
> > +	kfree(vaddr_out);
> > +
> >  	return crc;
> >  }
> >  
> > @@ -44,8 +145,8 @@ void vkms_crc_work_handle(struct work_struct *work)
> >  	struct vkms_device *vdev = container_of(out, struct vkms_device,
> >  						output);
> >  	struct vkms_crc_data *primary_crc = NULL;
> > +	struct vkms_crc_data *cursor_crc = NULL;
> >  	struct drm_plane *plane;
> > -
> >  	u32 crc32 = 0;
> >  
> >  	drm_for_each_plane(plane, &vdev->drm) {
> > @@ -58,14 +159,14 @@ void vkms_crc_work_handle(struct work_struct *work)
> >  		if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
> >  			continue;
> >  
> > -		if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> > +		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> >  			primary_crc = crc_data;
> > -			break;
> > -		}
> > +		else
> > +			cursor_crc = crc_data;
> >  	}
> >  
> >  	if (primary_crc)
> > -		crc32 = _vkms_get_crc(primary_crc);
> > +		crc32 = _vkms_get_crc(primary_crc, cursor_crc);
> >  
> >  	drm_crtc_add_crc_entry(crtc, true, crtc_state->n_frame, &crc32);
> >  }
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > index 36e524f791fe..173875dc361e 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -25,8 +25,11 @@ static const u32 vkms_cursor_formats[] = {
> >  };
> >  
> >  struct vkms_crc_data {
> > -	struct drm_rect src;
> >  	struct drm_framebuffer fb;
> > +	struct drm_rect src, dst;
> > +	unsigned int offset;
> > +	unsigned int pitch;
> > +	unsigned int cpp;
> >  };
> >  
> >  /**
> > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > index 428247d403dc..7041007396ae 100644
> > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > @@ -85,16 +85,22 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
> >  				     struct drm_plane_state *old_state)
> >  {
> >  	struct vkms_plane_state *vkms_plane_state;
> > +	struct drm_framebuffer *fb = plane->state->fb;
> >  	struct vkms_crc_data *crc_data;
> >  
> > -	if (!plane->state->crtc || !plane->state->fb)
> > +	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->fb, plane->state->fb, sizeof(struct drm_framebuffer));
> > +	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];
> >  }
> >  
> >  static int vkms_plane_atomic_check(struct drm_plane *plane,
> > -- 
> > 2.17.1
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/vkms: Compute CRC with Cursor Plane

[Daniel Vetter]

On Wed, Aug 08, 2018 at 06:53:17AM +0300, Haneen Mohammed wrote:
> On Tue, Aug 07, 2018 at 06:33:36PM +0200, Daniel Vetter wrote:
> > On Mon, Aug 06, 2018 at 06:58:29AM +0300, Haneen Mohammed wrote:
> > > This patch compute CRC for output frame with cursor and primary plane.
> > > Blend cursor with primary plane and compute CRC on the resulted frame.
> > > 
> > > Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
> > 
> > Nice!
> > 
> > I assume with this you're passing all the crc based cursor tests in igt?
> > If so, please mention this in the commit message, so that there's a record
> > of the testing done on this.
> > 
> 
> Sure, I'll update the commit message.
> Is there any other change I should add/fix to this patchset?
> 
> > One fairly huge gap we iirc have in our cursor testing is that there's not
> > much (if any?) alpha blending coverage. We kinda need that to make sure
> > this all works correctly. The usual trick is to only check the extreme
> > alpha values, i.e. fully opaque and fully transparent, since intermediate
> > values are affected by hw-specific rounding modes.
> > > ---
> > >  drivers/gpu/drm/vkms/vkms_crc.c   | 149 +++++++++++++++++++++++++-----
> > >  drivers/gpu/drm/vkms/vkms_drv.h   |   5 +-
> > >  drivers/gpu/drm/vkms/vkms_plane.c |  10 +-
> > >  3 files changed, 137 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> > > index 37d717f38e3c..4853a739ae5a 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_crc.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> > > @@ -1,36 +1,137 @@
> > >  // SPDX-License-Identifier: GPL-2.0
> > >  #include "vkms_drv.h"
> > >  #include <linux/crc32.h>
> > > +#include <drm/drm_atomic.h>
> > > +#include <drm/drm_atomic_helper.h>
> > >  #include <drm/drm_gem_framebuffer_helper.h>
> > >  
> > > -static uint32_t _vkms_get_crc(struct vkms_crc_data *crc_data)
> > > +/**
> > > + * compute_crc - Compute CRC value on output frame
> > > + *
> > > + * @vaddr_out: address to final framebuffer
> > > + * @crc_out: 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)
> > >  {
> > > -	struct drm_framebuffer *fb = &crc_data->fb;
> > > +	int i, 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 size_byte = w_src * crc_out->cpp;
> > > +	u32 crc = 0;
> > > +
> > > +	for (i = y_src; i < y_src + h_src; i++) {
> > > +		src_offset = crc_out->offset
> > > +			     + (i * crc_out->pitch)
> > > +			     + (x_src * crc_out->cpp);
> > > +		crc = crc32_le(crc, vaddr_out + src_offset, size_byte);
> > > +	}
> > > +
> > > +	return crc;
> > > +}
> > > +
> > > +/**
> > > + * 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
> > > + *
> > > + * Blend value at vaddr_src with value at vaddr_dst.
> > > + * Currently, this function write value at vaddr_src on value
> > > + * at vaddr_dst using buffer's metadata to locate the new values
> > > + * from vaddr_src and their distenation at vaddr_dst.
> > > + *
> > > + * Todo: Use the alpha value to blend vaddr_src with vaddr_dst
> > > + *	 instead of overwriting it.
> > 
> > Another todo: We must clear the alpha channel in the result after
> > blending. This also applies to the primary plane, where the XRGB for the
> > pixel format mandates that we entirely ignore the alpha channel.
> > 
> > This is also something we should probably have an igt testcase for.
> > 
> > Since there's a few open ends: How many weeks are left in your outreachy
> > internship? We need to make sure that at least all the issues are covered
> > in a vkms todo file. Would be great to add that in
> > Documentation/gpu/vkms.rst, like we have for other drivers.
> > -Daniel
> > 
> 
> I've one more week! I can use this week to prepare the todo file and
> finalize this patch?

Yeah sounds like the perfect wrap-up work. Since this wont be enough to
finish the cursor work it would be good to hide the cursor setup behind a
module option, perhaps "enable_cursor" or so. That way we wont have
not-totally-correct features enabled. And enabling/disabling cursor
support could be useful for testing.
-Daniel

> 
> Thank you,
> Haneen
> 
> > > + */
> > > +static void blend(void *vaddr_dst, void *vaddr_src,
> > > +		  struct vkms_crc_data *crc_dst,
> > > +		  struct vkms_crc_data *crc_src)
> > > +{
> > > +	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_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 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);
> > > +
> > > +			memcpy(vaddr_dst + offset_dst,
> > > +			       vaddr_src + offset_src, sizeof(u32));
> > > +		}
> > > +		i_dst++;
> > > +	}
> > > +}
> > > +
> > > +static void compose_cursor(struct vkms_crc_data *cursor_crc,
> > > +			   struct vkms_crc_data *primary_crc, 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_vkms_obj = drm_gem_to_vkms_gem(cursor_obj);
> > > +
> > > +	mutex_lock(&cursor_vkms_obj->pages_lock);
> > > +	if (WARN_ON(!cursor_vkms_obj->vaddr))
> > > +		goto out;
> > > +
> > > +	blend(vaddr_out, cursor_vkms_obj->vaddr, primary_crc, cursor_crc);
> > > +
> > > +out:
> > > +	mutex_unlock(&cursor_vkms_obj->pages_lock);
> > > +}
> > > +
> > > +static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
> > > +			      struct vkms_crc_data *cursor_crc)
> > > +{
> > > +	struct drm_framebuffer *fb = &primary_crc->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;
> > > -	int i = 0;
> > > -	unsigned int x = crc_data->src.x1 >> 16;
> > > -	unsigned int y = crc_data->src.y1 >> 16;
> > > -	unsigned int height = drm_rect_height(&crc_data->src) >> 16;
> > > -	unsigned int width = drm_rect_width(&crc_data->src) >> 16;
> > > -	unsigned int cpp = fb->format->cpp[0];
> > > -	unsigned int src_offset;
> > > -	unsigned int size_byte = width * cpp;
> > > -	void *vaddr;
> > >  
> > > -	mutex_lock(&vkms_obj->pages_lock);
> > > -	vaddr = vkms_obj->vaddr;
> > > -	if (WARN_ON(!vaddr))
> > > -		goto out;
> > > +	if (!vaddr_out) {
> > > +		DRM_ERROR("Failed to allocate memory for output frame.");
> > > +		return 0;
> > > +	}
> > >  
> > > -	for (i = y; i < y + height; i++) {
> > > -		src_offset = fb->offsets[0] + (i * fb->pitches[0]) + (x * cpp);
> > > -		crc = crc32_le(crc, vaddr + src_offset, size_byte);
> > > +	mutex_lock(&vkms_obj->pages_lock);
> > > +	if (WARN_ON(!vkms_obj->vaddr)) {
> > > +		mutex_lock(&vkms_obj->pages_lock);
> > > +		return crc;
> > >  	}
> > >  
> > > -out:
> > > +	memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
> > >  	mutex_unlock(&vkms_obj->pages_lock);
> > > +
> > > +	if (cursor_crc)
> > > +		compose_cursor(cursor_crc, primary_crc, vaddr_out);
> > > +
> > > +	crc = compute_crc(vaddr_out, primary_crc);
> > > +
> > > +	kfree(vaddr_out);
> > > +
> > >  	return crc;
> > >  }
> > >  
> > > @@ -44,8 +145,8 @@ void vkms_crc_work_handle(struct work_struct *work)
> > >  	struct vkms_device *vdev = container_of(out, struct vkms_device,
> > >  						output);
> > >  	struct vkms_crc_data *primary_crc = NULL;
> > > +	struct vkms_crc_data *cursor_crc = NULL;
> > >  	struct drm_plane *plane;
> > > -
> > >  	u32 crc32 = 0;
> > >  
> > >  	drm_for_each_plane(plane, &vdev->drm) {
> > > @@ -58,14 +159,14 @@ void vkms_crc_work_handle(struct work_struct *work)
> > >  		if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
> > >  			continue;
> > >  
> > > -		if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> > > +		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > >  			primary_crc = crc_data;
> > > -			break;
> > > -		}
> > > +		else
> > > +			cursor_crc = crc_data;
> > >  	}
> > >  
> > >  	if (primary_crc)
> > > -		crc32 = _vkms_get_crc(primary_crc);
> > > +		crc32 = _vkms_get_crc(primary_crc, cursor_crc);
> > >  
> > >  	drm_crtc_add_crc_entry(crtc, true, crtc_state->n_frame, &crc32);
> > >  }
> > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > index 36e524f791fe..173875dc361e 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > @@ -25,8 +25,11 @@ static const u32 vkms_cursor_formats[] = {
> > >  };
> > >  
> > >  struct vkms_crc_data {
> > > -	struct drm_rect src;
> > >  	struct drm_framebuffer fb;
> > > +	struct drm_rect src, dst;
> > > +	unsigned int offset;
> > > +	unsigned int pitch;
> > > +	unsigned int cpp;
> > >  };
> > >  
> > >  /**
> > > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > > index 428247d403dc..7041007396ae 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > > @@ -85,16 +85,22 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
> > >  				     struct drm_plane_state *old_state)
> > >  {
> > >  	struct vkms_plane_state *vkms_plane_state;
> > > +	struct drm_framebuffer *fb = plane->state->fb;
> > >  	struct vkms_crc_data *crc_data;
> > >  
> > > -	if (!plane->state->crtc || !plane->state->fb)
> > > +	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->fb, plane->state->fb, sizeof(struct drm_framebuffer));
> > > +	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];
> > >  }
> > >  
> > >  static int vkms_plane_atomic_check(struct drm_plane *plane,
> > > -- 
> > > 2.17.1
> > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

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

Re: [PATCH 2/2] drm/vkms: Compute CRC with Cursor Plane

[Haneen Mohammed]

On Wed, Aug 08, 2018 at 10:23:27AM +0200, Daniel Vetter wrote:
> On Wed, Aug 08, 2018 at 06:53:17AM +0300, Haneen Mohammed wrote:
> > On Tue, Aug 07, 2018 at 06:33:36PM +0200, Daniel Vetter wrote:
> > > On Mon, Aug 06, 2018 at 06:58:29AM +0300, Haneen Mohammed wrote:
> > > > This patch compute CRC for output frame with cursor and primary plane.
> > > > Blend cursor with primary plane and compute CRC on the resulted frame.
> > > > 
> > > > Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
> > > 
> > > Nice!
> > > 
> > > I assume with this you're passing all the crc based cursor tests in igt?
> > > If so, please mention this in the commit message, so that there's a record
> > > of the testing done on this.
> > > 
> > 
> > Sure, I'll update the commit message.
> > Is there any other change I should add/fix to this patchset?
> > 
> > > One fairly huge gap we iirc have in our cursor testing is that there's not
> > > much (if any?) alpha blending coverage. We kinda need that to make sure
> > > this all works correctly. The usual trick is to only check the extreme
> > > alpha values, i.e. fully opaque and fully transparent, since intermediate
> > > values are affected by hw-specific rounding modes.
> > > > ---
> > > >  drivers/gpu/drm/vkms/vkms_crc.c   | 149 +++++++++++++++++++++++++-----
> > > >  drivers/gpu/drm/vkms/vkms_drv.h   |   5 +-
> > > >  drivers/gpu/drm/vkms/vkms_plane.c |  10 +-
> > > >  3 files changed, 137 insertions(+), 27 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> > > > index 37d717f38e3c..4853a739ae5a 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_crc.c
> > > > +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> > > > @@ -1,36 +1,137 @@
> > > >  // SPDX-License-Identifier: GPL-2.0
> > > >  #include "vkms_drv.h"
> > > >  #include <linux/crc32.h>
> > > > +#include <drm/drm_atomic.h>
> > > > +#include <drm/drm_atomic_helper.h>
> > > >  #include <drm/drm_gem_framebuffer_helper.h>
> > > >  
> > > > -static uint32_t _vkms_get_crc(struct vkms_crc_data *crc_data)
> > > > +/**
> > > > + * compute_crc - Compute CRC value on output frame
> > > > + *
> > > > + * @vaddr_out: address to final framebuffer
> > > > + * @crc_out: 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)
> > > >  {
> > > > -	struct drm_framebuffer *fb = &crc_data->fb;
> > > > +	int i, 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 size_byte = w_src * crc_out->cpp;
> > > > +	u32 crc = 0;
> > > > +
> > > > +	for (i = y_src; i < y_src + h_src; i++) {
> > > > +		src_offset = crc_out->offset
> > > > +			     + (i * crc_out->pitch)
> > > > +			     + (x_src * crc_out->cpp);
> > > > +		crc = crc32_le(crc, vaddr_out + src_offset, size_byte);
> > > > +	}
> > > > +
> > > > +	return crc;
> > > > +}
> > > > +

Hey Daniel, 

When I change the above function "compute_crc" to compute the CRC for
each pixel so we can clear the Alpha value before computing the CRC as bellow,
the test "nonblocking-crc-pipe-A-frame-sequence" sometimes failes and
sometimes passes. Should I still continue with the change, or leave it
as it is?

----------------- vkms_crc.c -----------------
	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);
			memset(vaddr_out + src_offset + 24, 0,  8);
			crc = crc32_le(crc, vaddr_out + src_offset, sizeof(u32));
		}
	}
----------------- vkms_crc.c -----------------

Thank you,
Haneen

> > > > +/**
> > > > + * 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
> > > > + *
> > > > + * Blend value at vaddr_src with value at vaddr_dst.
> > > > + * Currently, this function write value at vaddr_src on value
> > > > + * at vaddr_dst using buffer's metadata to locate the new values
> > > > + * from vaddr_src and their distenation at vaddr_dst.
> > > > + *
> > > > + * Todo: Use the alpha value to blend vaddr_src with vaddr_dst
> > > > + *	 instead of overwriting it.
> > > 
> > > Another todo: We must clear the alpha channel in the result after
> > > blending. This also applies to the primary plane, where the XRGB for the
> > > pixel format mandates that we entirely ignore the alpha channel.
> > > 
> > > This is also something we should probably have an igt testcase for.
> > > 
> > > Since there's a few open ends: How many weeks are left in your outreachy
> > > internship? We need to make sure that at least all the issues are covered
> > > in a vkms todo file. Would be great to add that in
> > > Documentation/gpu/vkms.rst, like we have for other drivers.
> > > -Daniel
> > > 
> > 
> > I've one more week! I can use this week to prepare the todo file and
> > finalize this patch?
> 
> Yeah sounds like the perfect wrap-up work. Since this wont be enough to
> finish the cursor work it would be good to hide the cursor setup behind a
> module option, perhaps "enable_cursor" or so. That way we wont have
> not-totally-correct features enabled. And enabling/disabling cursor
> support could be useful for testing.
> -Daniel
> 
> > 
> > Thank you,
> > Haneen
> > 
> > > > + */
> > > > +static void blend(void *vaddr_dst, void *vaddr_src,
> > > > +		  struct vkms_crc_data *crc_dst,
> > > > +		  struct vkms_crc_data *crc_src)
> > > > +{
> > > > +	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_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 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);
> > > > +
> > > > +			memcpy(vaddr_dst + offset_dst,
> > > > +			       vaddr_src + offset_src, sizeof(u32));
> > > > +		}
> > > > +		i_dst++;
> > > > +	}
> > > > +}
> > > > +
> > > > +static void compose_cursor(struct vkms_crc_data *cursor_crc,
> > > > +			   struct vkms_crc_data *primary_crc, 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_vkms_obj = drm_gem_to_vkms_gem(cursor_obj);
> > > > +
> > > > +	mutex_lock(&cursor_vkms_obj->pages_lock);
> > > > +	if (WARN_ON(!cursor_vkms_obj->vaddr))
> > > > +		goto out;
> > > > +
> > > > +	blend(vaddr_out, cursor_vkms_obj->vaddr, primary_crc, cursor_crc);
> > > > +
> > > > +out:
> > > > +	mutex_unlock(&cursor_vkms_obj->pages_lock);
> > > > +}
> > > > +
> > > > +static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
> > > > +			      struct vkms_crc_data *cursor_crc)
> > > > +{
> > > > +	struct drm_framebuffer *fb = &primary_crc->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;
> > > > -	int i = 0;
> > > > -	unsigned int x = crc_data->src.x1 >> 16;
> > > > -	unsigned int y = crc_data->src.y1 >> 16;
> > > > -	unsigned int height = drm_rect_height(&crc_data->src) >> 16;
> > > > -	unsigned int width = drm_rect_width(&crc_data->src) >> 16;
> > > > -	unsigned int cpp = fb->format->cpp[0];
> > > > -	unsigned int src_offset;
> > > > -	unsigned int size_byte = width * cpp;
> > > > -	void *vaddr;
> > > >  
> > > > -	mutex_lock(&vkms_obj->pages_lock);
> > > > -	vaddr = vkms_obj->vaddr;
> > > > -	if (WARN_ON(!vaddr))
> > > > -		goto out;
> > > > +	if (!vaddr_out) {
> > > > +		DRM_ERROR("Failed to allocate memory for output frame.");
> > > > +		return 0;
> > > > +	}
> > > >  
> > > > -	for (i = y; i < y + height; i++) {
> > > > -		src_offset = fb->offsets[0] + (i * fb->pitches[0]) + (x * cpp);
> > > > -		crc = crc32_le(crc, vaddr + src_offset, size_byte);
> > > > +	mutex_lock(&vkms_obj->pages_lock);
> > > > +	if (WARN_ON(!vkms_obj->vaddr)) {
> > > > +		mutex_lock(&vkms_obj->pages_lock);
> > > > +		return crc;
> > > >  	}
> > > >  
> > > > -out:
> > > > +	memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
> > > >  	mutex_unlock(&vkms_obj->pages_lock);
> > > > +
> > > > +	if (cursor_crc)
> > > > +		compose_cursor(cursor_crc, primary_crc, vaddr_out);
> > > > +
> > > > +	crc = compute_crc(vaddr_out, primary_crc);
> > > > +
> > > > +	kfree(vaddr_out);
> > > > +
> > > >  	return crc;
> > > >  }
> > > >  
> > > > @@ -44,8 +145,8 @@ void vkms_crc_work_handle(struct work_struct *work)
> > > >  	struct vkms_device *vdev = container_of(out, struct vkms_device,
> > > >  						output);
> > > >  	struct vkms_crc_data *primary_crc = NULL;
> > > > +	struct vkms_crc_data *cursor_crc = NULL;
> > > >  	struct drm_plane *plane;
> > > > -
> > > >  	u32 crc32 = 0;
> > > >  
> > > >  	drm_for_each_plane(plane, &vdev->drm) {
> > > > @@ -58,14 +159,14 @@ void vkms_crc_work_handle(struct work_struct *work)
> > > >  		if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
> > > >  			continue;
> > > >  
> > > > -		if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> > > > +		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > > >  			primary_crc = crc_data;
> > > > -			break;
> > > > -		}
> > > > +		else
> > > > +			cursor_crc = crc_data;
> > > >  	}
> > > >  
> > > >  	if (primary_crc)
> > > > -		crc32 = _vkms_get_crc(primary_crc);
> > > > +		crc32 = _vkms_get_crc(primary_crc, cursor_crc);
> > > >  
> > > >  	drm_crtc_add_crc_entry(crtc, true, crtc_state->n_frame, &crc32);
> > > >  }
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > > index 36e524f791fe..173875dc361e 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > > @@ -25,8 +25,11 @@ static const u32 vkms_cursor_formats[] = {
> > > >  };
> > > >  
> > > >  struct vkms_crc_data {
> > > > -	struct drm_rect src;
> > > >  	struct drm_framebuffer fb;
> > > > +	struct drm_rect src, dst;
> > > > +	unsigned int offset;
> > > > +	unsigned int pitch;
> > > > +	unsigned int cpp;
> > > >  };
> > > >  
> > > >  /**
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > > > index 428247d403dc..7041007396ae 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > > > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > > > @@ -85,16 +85,22 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
> > > >  				     struct drm_plane_state *old_state)
> > > >  {
> > > >  	struct vkms_plane_state *vkms_plane_state;
> > > > +	struct drm_framebuffer *fb = plane->state->fb;
> > > >  	struct vkms_crc_data *crc_data;
> > > >  
> > > > -	if (!plane->state->crtc || !plane->state->fb)
> > > > +	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->fb, plane->state->fb, sizeof(struct drm_framebuffer));
> > > > +	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];
> > > >  }
> > > >  
> > > >  static int vkms_plane_atomic_check(struct drm_plane *plane,
> > > > -- 
> > > > 2.17.1
> > > > 
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/vkms: Compute CRC with Cursor Plane

[Daniel Vetter]

On Mon, Aug 13, 2018 at 11:04:11PM +0300, Haneen Mohammed wrote:
> On Wed, Aug 08, 2018 at 10:23:27AM +0200, Daniel Vetter wrote:
> > On Wed, Aug 08, 2018 at 06:53:17AM +0300, Haneen Mohammed wrote:
> > > On Tue, Aug 07, 2018 at 06:33:36PM +0200, Daniel Vetter wrote:
> > > > On Mon, Aug 06, 2018 at 06:58:29AM +0300, Haneen Mohammed wrote:
> > > > > This patch compute CRC for output frame with cursor and primary plane.
> > > > > Blend cursor with primary plane and compute CRC on the resulted frame.
> > > > > 
> > > > > Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
> > > > 
> > > > Nice!
> > > > 
> > > > I assume with this you're passing all the crc based cursor tests in igt?
> > > > If so, please mention this in the commit message, so that there's a record
> > > > of the testing done on this.
> > > > 
> > > 
> > > Sure, I'll update the commit message.
> > > Is there any other change I should add/fix to this patchset?
> > > 
> > > > One fairly huge gap we iirc have in our cursor testing is that there's not
> > > > much (if any?) alpha blending coverage. We kinda need that to make sure
> > > > this all works correctly. The usual trick is to only check the extreme
> > > > alpha values, i.e. fully opaque and fully transparent, since intermediate
> > > > values are affected by hw-specific rounding modes.
> > > > > ---
> > > > >  drivers/gpu/drm/vkms/vkms_crc.c   | 149 +++++++++++++++++++++++++-----
> > > > >  drivers/gpu/drm/vkms/vkms_drv.h   |   5 +-
> > > > >  drivers/gpu/drm/vkms/vkms_plane.c |  10 +-
> > > > >  3 files changed, 137 insertions(+), 27 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> > > > > index 37d717f38e3c..4853a739ae5a 100644
> > > > > --- a/drivers/gpu/drm/vkms/vkms_crc.c
> > > > > +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> > > > > @@ -1,36 +1,137 @@
> > > > >  // SPDX-License-Identifier: GPL-2.0
> > > > >  #include "vkms_drv.h"
> > > > >  #include <linux/crc32.h>
> > > > > +#include <drm/drm_atomic.h>
> > > > > +#include <drm/drm_atomic_helper.h>
> > > > >  #include <drm/drm_gem_framebuffer_helper.h>
> > > > >  
> > > > > -static uint32_t _vkms_get_crc(struct vkms_crc_data *crc_data)
> > > > > +/**
> > > > > + * compute_crc - Compute CRC value on output frame
> > > > > + *
> > > > > + * @vaddr_out: address to final framebuffer
> > > > > + * @crc_out: 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)
> > > > >  {
> > > > > -	struct drm_framebuffer *fb = &crc_data->fb;
> > > > > +	int i, 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 size_byte = w_src * crc_out->cpp;
> > > > > +	u32 crc = 0;
> > > > > +
> > > > > +	for (i = y_src; i < y_src + h_src; i++) {
> > > > > +		src_offset = crc_out->offset
> > > > > +			     + (i * crc_out->pitch)
> > > > > +			     + (x_src * crc_out->cpp);
> > > > > +		crc = crc32_le(crc, vaddr_out + src_offset, size_byte);
> > > > > +	}
> > > > > +
> > > > > +	return crc;
> > > > > +}
> > > > > +
> 
> Hey Daniel, 
> 
> When I change the above function "compute_crc" to compute the CRC for
> each pixel so we can clear the Alpha value before computing the CRC as bellow,
> the test "nonblocking-crc-pipe-A-frame-sequence" sometimes failes and
> sometimes passes. Should I still continue with the change, or leave it
> as it is?

Hm, how does it fail? Could be that the code becomes too slow, and then
the frame counters don't increment nicely anymore. Or there's a race
somewhere in the CRC code that gets exposed. But that's just me guessing.
-Daniel

> 
> ----------------- vkms_crc.c -----------------
> 	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);
> 			memset(vaddr_out + src_offset + 24, 0,  8);
> 			crc = crc32_le(crc, vaddr_out + src_offset, sizeof(u32));
> 		}
> 	}
> ----------------- vkms_crc.c -----------------
> 
> Thank you,
> Haneen
> 
> > > > > +/**
> > > > > + * 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
> > > > > + *
> > > > > + * Blend value at vaddr_src with value at vaddr_dst.
> > > > > + * Currently, this function write value at vaddr_src on value
> > > > > + * at vaddr_dst using buffer's metadata to locate the new values
> > > > > + * from vaddr_src and their distenation at vaddr_dst.
> > > > > + *
> > > > > + * Todo: Use the alpha value to blend vaddr_src with vaddr_dst
> > > > > + *	 instead of overwriting it.
> > > > 
> > > > Another todo: We must clear the alpha channel in the result after
> > > > blending. This also applies to the primary plane, where the XRGB for the
> > > > pixel format mandates that we entirely ignore the alpha channel.
> > > > 
> > > > This is also something we should probably have an igt testcase for.
> > > > 
> > > > Since there's a few open ends: How many weeks are left in your outreachy
> > > > internship? We need to make sure that at least all the issues are covered
> > > > in a vkms todo file. Would be great to add that in
> > > > Documentation/gpu/vkms.rst, like we have for other drivers.
> > > > -Daniel
> > > > 
> > > 
> > > I've one more week! I can use this week to prepare the todo file and
> > > finalize this patch?
> > 
> > Yeah sounds like the perfect wrap-up work. Since this wont be enough to
> > finish the cursor work it would be good to hide the cursor setup behind a
> > module option, perhaps "enable_cursor" or so. That way we wont have
> > not-totally-correct features enabled. And enabling/disabling cursor
> > support could be useful for testing.
> > -Daniel
> > 
> > > 
> > > Thank you,
> > > Haneen
> > > 
> > > > > + */
> > > > > +static void blend(void *vaddr_dst, void *vaddr_src,
> > > > > +		  struct vkms_crc_data *crc_dst,
> > > > > +		  struct vkms_crc_data *crc_src)
> > > > > +{
> > > > > +	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_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 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);
> > > > > +
> > > > > +			memcpy(vaddr_dst + offset_dst,
> > > > > +			       vaddr_src + offset_src, sizeof(u32));
> > > > > +		}
> > > > > +		i_dst++;
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +static void compose_cursor(struct vkms_crc_data *cursor_crc,
> > > > > +			   struct vkms_crc_data *primary_crc, 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_vkms_obj = drm_gem_to_vkms_gem(cursor_obj);
> > > > > +
> > > > > +	mutex_lock(&cursor_vkms_obj->pages_lock);
> > > > > +	if (WARN_ON(!cursor_vkms_obj->vaddr))
> > > > > +		goto out;
> > > > > +
> > > > > +	blend(vaddr_out, cursor_vkms_obj->vaddr, primary_crc, cursor_crc);
> > > > > +
> > > > > +out:
> > > > > +	mutex_unlock(&cursor_vkms_obj->pages_lock);
> > > > > +}
> > > > > +
> > > > > +static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
> > > > > +			      struct vkms_crc_data *cursor_crc)
> > > > > +{
> > > > > +	struct drm_framebuffer *fb = &primary_crc->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;
> > > > > -	int i = 0;
> > > > > -	unsigned int x = crc_data->src.x1 >> 16;
> > > > > -	unsigned int y = crc_data->src.y1 >> 16;
> > > > > -	unsigned int height = drm_rect_height(&crc_data->src) >> 16;
> > > > > -	unsigned int width = drm_rect_width(&crc_data->src) >> 16;
> > > > > -	unsigned int cpp = fb->format->cpp[0];
> > > > > -	unsigned int src_offset;
> > > > > -	unsigned int size_byte = width * cpp;
> > > > > -	void *vaddr;
> > > > >  
> > > > > -	mutex_lock(&vkms_obj->pages_lock);
> > > > > -	vaddr = vkms_obj->vaddr;
> > > > > -	if (WARN_ON(!vaddr))
> > > > > -		goto out;
> > > > > +	if (!vaddr_out) {
> > > > > +		DRM_ERROR("Failed to allocate memory for output frame.");
> > > > > +		return 0;
> > > > > +	}
> > > > >  
> > > > > -	for (i = y; i < y + height; i++) {
> > > > > -		src_offset = fb->offsets[0] + (i * fb->pitches[0]) + (x * cpp);
> > > > > -		crc = crc32_le(crc, vaddr + src_offset, size_byte);
> > > > > +	mutex_lock(&vkms_obj->pages_lock);
> > > > > +	if (WARN_ON(!vkms_obj->vaddr)) {
> > > > > +		mutex_lock(&vkms_obj->pages_lock);
> > > > > +		return crc;
> > > > >  	}
> > > > >  
> > > > > -out:
> > > > > +	memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
> > > > >  	mutex_unlock(&vkms_obj->pages_lock);
> > > > > +
> > > > > +	if (cursor_crc)
> > > > > +		compose_cursor(cursor_crc, primary_crc, vaddr_out);
> > > > > +
> > > > > +	crc = compute_crc(vaddr_out, primary_crc);
> > > > > +
> > > > > +	kfree(vaddr_out);
> > > > > +
> > > > >  	return crc;
> > > > >  }
> > > > >  
> > > > > @@ -44,8 +145,8 @@ void vkms_crc_work_handle(struct work_struct *work)
> > > > >  	struct vkms_device *vdev = container_of(out, struct vkms_device,
> > > > >  						output);
> > > > >  	struct vkms_crc_data *primary_crc = NULL;
> > > > > +	struct vkms_crc_data *cursor_crc = NULL;
> > > > >  	struct drm_plane *plane;
> > > > > -
> > > > >  	u32 crc32 = 0;
> > > > >  
> > > > >  	drm_for_each_plane(plane, &vdev->drm) {
> > > > > @@ -58,14 +159,14 @@ void vkms_crc_work_handle(struct work_struct *work)
> > > > >  		if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
> > > > >  			continue;
> > > > >  
> > > > > -		if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> > > > > +		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > > > >  			primary_crc = crc_data;
> > > > > -			break;
> > > > > -		}
> > > > > +		else
> > > > > +			cursor_crc = crc_data;
> > > > >  	}
> > > > >  
> > > > >  	if (primary_crc)
> > > > > -		crc32 = _vkms_get_crc(primary_crc);
> > > > > +		crc32 = _vkms_get_crc(primary_crc, cursor_crc);
> > > > >  
> > > > >  	drm_crtc_add_crc_entry(crtc, true, crtc_state->n_frame, &crc32);
> > > > >  }
> > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > > > index 36e524f791fe..173875dc361e 100644
> > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > > > @@ -25,8 +25,11 @@ static const u32 vkms_cursor_formats[] = {
> > > > >  };
> > > > >  
> > > > >  struct vkms_crc_data {
> > > > > -	struct drm_rect src;
> > > > >  	struct drm_framebuffer fb;
> > > > > +	struct drm_rect src, dst;
> > > > > +	unsigned int offset;
> > > > > +	unsigned int pitch;
> > > > > +	unsigned int cpp;
> > > > >  };
> > > > >  
> > > > >  /**
> > > > > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > > > > index 428247d403dc..7041007396ae 100644
> > > > > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > > > > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > > > > @@ -85,16 +85,22 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
> > > > >  				     struct drm_plane_state *old_state)
> > > > >  {
> > > > >  	struct vkms_plane_state *vkms_plane_state;
> > > > > +	struct drm_framebuffer *fb = plane->state->fb;
> > > > >  	struct vkms_crc_data *crc_data;
> > > > >  
> > > > > -	if (!plane->state->crtc || !plane->state->fb)
> > > > > +	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->fb, plane->state->fb, sizeof(struct drm_framebuffer));
> > > > > +	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];
> > > > >  }
> > > > >  
> > > > >  static int vkms_plane_atomic_check(struct drm_plane *plane,
> > > > > -- 
> > > > > 2.17.1
> > > > > 
> > > > 
> > > > -- 
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

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

Re: [PATCH 2/2] drm/vkms: Compute CRC with Cursor Plane

[Haneen Mohammed]

On Tue, Aug 14, 2018 at 10:21:29AM +0200, Daniel Vetter wrote:
> On Mon, Aug 13, 2018 at 11:04:11PM +0300, Haneen Mohammed wrote:
> > On Wed, Aug 08, 2018 at 10:23:27AM +0200, Daniel Vetter wrote:
> > > On Wed, Aug 08, 2018 at 06:53:17AM +0300, Haneen Mohammed wrote:
> > > > On Tue, Aug 07, 2018 at 06:33:36PM +0200, Daniel Vetter wrote:
> > > > > On Mon, Aug 06, 2018 at 06:58:29AM +0300, Haneen Mohammed wrote:
> > > > > > This patch compute CRC for output frame with cursor and primary plane.
> > > > > > Blend cursor with primary plane and compute CRC on the resulted frame.
> > > > > > 
> > > > > > Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
> > > > > 
> > > > > Nice!
> > > > > 
> > > > > I assume with this you're passing all the crc based cursor tests in igt?
> > > > > If so, please mention this in the commit message, so that there's a record
> > > > > of the testing done on this.
> > > > > 
> > > > 
> > > > Sure, I'll update the commit message.
> > > > Is there any other change I should add/fix to this patchset?
> > > > 
> > > > > One fairly huge gap we iirc have in our cursor testing is that there's not
> > > > > much (if any?) alpha blending coverage. We kinda need that to make sure
> > > > > this all works correctly. The usual trick is to only check the extreme
> > > > > alpha values, i.e. fully opaque and fully transparent, since intermediate
> > > > > values are affected by hw-specific rounding modes.
> > > > > > ---
> > > > > >  drivers/gpu/drm/vkms/vkms_crc.c   | 149 +++++++++++++++++++++++++-----
> > > > > >  drivers/gpu/drm/vkms/vkms_drv.h   |   5 +-
> > > > > >  drivers/gpu/drm/vkms/vkms_plane.c |  10 +-
> > > > > >  3 files changed, 137 insertions(+), 27 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> > > > > > index 37d717f38e3c..4853a739ae5a 100644
> > > > > > --- a/drivers/gpu/drm/vkms/vkms_crc.c
> > > > > > +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> > > > > > @@ -1,36 +1,137 @@
> > > > > >  // SPDX-License-Identifier: GPL-2.0
> > > > > >  #include "vkms_drv.h"
> > > > > >  #include <linux/crc32.h>
> > > > > > +#include <drm/drm_atomic.h>
> > > > > > +#include <drm/drm_atomic_helper.h>
> > > > > >  #include <drm/drm_gem_framebuffer_helper.h>
> > > > > >  
> > > > > > -static uint32_t _vkms_get_crc(struct vkms_crc_data *crc_data)
> > > > > > +/**
> > > > > > + * compute_crc - Compute CRC value on output frame
> > > > > > + *
> > > > > > + * @vaddr_out: address to final framebuffer
> > > > > > + * @crc_out: 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)
> > > > > >  {
> > > > > > -	struct drm_framebuffer *fb = &crc_data->fb;
> > > > > > +	int i, 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 size_byte = w_src * crc_out->cpp;
> > > > > > +	u32 crc = 0;
> > > > > > +
> > > > > > +	for (i = y_src; i < y_src + h_src; i++) {
> > > > > > +		src_offset = crc_out->offset
> > > > > > +			     + (i * crc_out->pitch)
> > > > > > +			     + (x_src * crc_out->cpp);
> > > > > > +		crc = crc32_le(crc, vaddr_out + src_offset, size_byte);
> > > > > > +	}
> > > > > > +
> > > > > > +	return crc;
> > > > > > +}
> > > > > > +
> > 
> > Hey Daniel, 
> > 
> > When I change the above function "compute_crc" to compute the CRC for
> > each pixel so we can clear the Alpha value before computing the CRC as bellow,
> > the test "nonblocking-crc-pipe-A-frame-sequence" sometimes failes and
> > sometimes passes. Should I still continue with the change, or leave it
> > as it is?
> 
> Hm, how does it fail? Could be that the code becomes too slow, and then
> the frame counters don't increment nicely anymore. Or there's a race
> somewhere in the CRC code that gets exposed. But that's just me guessing.
> -Daniel
> 

Oh sorry I should've elaborated more on how it fails. Yeah I think it
becomes too slow and that can cause mismatch to frame counters?

-----
(kms_pipe_crc_basic:1902) CRITICAL: Failed assertion: crcs[j].frame + 1 == crcs[j + 1].frame
(kms_pipe_crc_basic:1902) CRITICAL: Last errno: 9, Bad file descriptor
(kms_pipe_crc_basic:1902) CRITICAL: error: 1090928 != 1090929
-----

Maybe it's not that bad/slow because I've noticed that this happen
occasionally when I enable Ftrace.

> > 
> > ----------------- vkms_crc.c -----------------
> > 	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);
> > 			memset(vaddr_out + src_offset + 24, 0,  8);
> > 			crc = crc32_le(crc, vaddr_out + src_offset, sizeof(u32));
> > 		}
> > 	}
> > ----------------- vkms_crc.c -----------------
> > 
> > Thank you,
> > Haneen
> > 
> > > > > > +/**
> > > > > > + * 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
> > > > > > + *
> > > > > > + * Blend value at vaddr_src with value at vaddr_dst.
> > > > > > + * Currently, this function write value at vaddr_src on value
> > > > > > + * at vaddr_dst using buffer's metadata to locate the new values
> > > > > > + * from vaddr_src and their distenation at vaddr_dst.
> > > > > > + *
> > > > > > + * Todo: Use the alpha value to blend vaddr_src with vaddr_dst
> > > > > > + *	 instead of overwriting it.
> > > > > 
> > > > > Another todo: We must clear the alpha channel in the result after
> > > > > blending. This also applies to the primary plane, where the XRGB for the
> > > > > pixel format mandates that we entirely ignore the alpha channel.
> > > > > 
> > > > > This is also something we should probably have an igt testcase for.
> > > > > 
> > > > > Since there's a few open ends: How many weeks are left in your outreachy
> > > > > internship? We need to make sure that at least all the issues are covered
> > > > > in a vkms todo file. Would be great to add that in
> > > > > Documentation/gpu/vkms.rst, like we have for other drivers.
> > > > > -Daniel
> > > > > 
> > > > 
> > > > I've one more week! I can use this week to prepare the todo file and
> > > > finalize this patch?
> > > 
> > > Yeah sounds like the perfect wrap-up work. Since this wont be enough to
> > > finish the cursor work it would be good to hide the cursor setup behind a
> > > module option, perhaps "enable_cursor" or so. That way we wont have
> > > not-totally-correct features enabled. And enabling/disabling cursor
> > > support could be useful for testing.
> > > -Daniel
> > > 
> > > > 
> > > > Thank you,
> > > > Haneen
> > > > 
> > > > > > + */
> > > > > > +static void blend(void *vaddr_dst, void *vaddr_src,
> > > > > > +		  struct vkms_crc_data *crc_dst,
> > > > > > +		  struct vkms_crc_data *crc_src)
> > > > > > +{
> > > > > > +	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_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 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);
> > > > > > +
> > > > > > +			memcpy(vaddr_dst + offset_dst,
> > > > > > +			       vaddr_src + offset_src, sizeof(u32));
> > > > > > +		}
> > > > > > +		i_dst++;
> > > > > > +	}
> > > > > > +}
> > > > > > +
> > > > > > +static void compose_cursor(struct vkms_crc_data *cursor_crc,
> > > > > > +			   struct vkms_crc_data *primary_crc, 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_vkms_obj = drm_gem_to_vkms_gem(cursor_obj);
> > > > > > +
> > > > > > +	mutex_lock(&cursor_vkms_obj->pages_lock);
> > > > > > +	if (WARN_ON(!cursor_vkms_obj->vaddr))
> > > > > > +		goto out;
> > > > > > +
> > > > > > +	blend(vaddr_out, cursor_vkms_obj->vaddr, primary_crc, cursor_crc);
> > > > > > +
> > > > > > +out:
> > > > > > +	mutex_unlock(&cursor_vkms_obj->pages_lock);
> > > > > > +}
> > > > > > +
> > > > > > +static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
> > > > > > +			      struct vkms_crc_data *cursor_crc)
> > > > > > +{
> > > > > > +	struct drm_framebuffer *fb = &primary_crc->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;
> > > > > > -	int i = 0;
> > > > > > -	unsigned int x = crc_data->src.x1 >> 16;
> > > > > > -	unsigned int y = crc_data->src.y1 >> 16;
> > > > > > -	unsigned int height = drm_rect_height(&crc_data->src) >> 16;
> > > > > > -	unsigned int width = drm_rect_width(&crc_data->src) >> 16;
> > > > > > -	unsigned int cpp = fb->format->cpp[0];
> > > > > > -	unsigned int src_offset;
> > > > > > -	unsigned int size_byte = width * cpp;
> > > > > > -	void *vaddr;
> > > > > >  
> > > > > > -	mutex_lock(&vkms_obj->pages_lock);
> > > > > > -	vaddr = vkms_obj->vaddr;
> > > > > > -	if (WARN_ON(!vaddr))
> > > > > > -		goto out;
> > > > > > +	if (!vaddr_out) {
> > > > > > +		DRM_ERROR("Failed to allocate memory for output frame.");
> > > > > > +		return 0;
> > > > > > +	}
> > > > > >  
> > > > > > -	for (i = y; i < y + height; i++) {
> > > > > > -		src_offset = fb->offsets[0] + (i * fb->pitches[0]) + (x * cpp);
> > > > > > -		crc = crc32_le(crc, vaddr + src_offset, size_byte);
> > > > > > +	mutex_lock(&vkms_obj->pages_lock);
> > > > > > +	if (WARN_ON(!vkms_obj->vaddr)) {
> > > > > > +		mutex_lock(&vkms_obj->pages_lock);
> > > > > > +		return crc;
> > > > > >  	}
> > > > > >  
> > > > > > -out:
> > > > > > +	memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
> > > > > >  	mutex_unlock(&vkms_obj->pages_lock);
> > > > > > +
> > > > > > +	if (cursor_crc)
> > > > > > +		compose_cursor(cursor_crc, primary_crc, vaddr_out);
> > > > > > +
> > > > > > +	crc = compute_crc(vaddr_out, primary_crc);
> > > > > > +
> > > > > > +	kfree(vaddr_out);
> > > > > > +
> > > > > >  	return crc;
> > > > > >  }
> > > > > >  
> > > > > > @@ -44,8 +145,8 @@ void vkms_crc_work_handle(struct work_struct *work)
> > > > > >  	struct vkms_device *vdev = container_of(out, struct vkms_device,
> > > > > >  						output);
> > > > > >  	struct vkms_crc_data *primary_crc = NULL;
> > > > > > +	struct vkms_crc_data *cursor_crc = NULL;
> > > > > >  	struct drm_plane *plane;
> > > > > > -
> > > > > >  	u32 crc32 = 0;
> > > > > >  
> > > > > >  	drm_for_each_plane(plane, &vdev->drm) {
> > > > > > @@ -58,14 +159,14 @@ void vkms_crc_work_handle(struct work_struct *work)
> > > > > >  		if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
> > > > > >  			continue;
> > > > > >  
> > > > > > -		if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> > > > > > +		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > > > > >  			primary_crc = crc_data;
> > > > > > -			break;
> > > > > > -		}
> > > > > > +		else
> > > > > > +			cursor_crc = crc_data;
> > > > > >  	}
> > > > > >  
> > > > > >  	if (primary_crc)
> > > > > > -		crc32 = _vkms_get_crc(primary_crc);
> > > > > > +		crc32 = _vkms_get_crc(primary_crc, cursor_crc);
> > > > > >  
> > > > > >  	drm_crtc_add_crc_entry(crtc, true, crtc_state->n_frame, &crc32);
> > > > > >  }
> > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > > > > index 36e524f791fe..173875dc361e 100644
> > > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > > > > @@ -25,8 +25,11 @@ static const u32 vkms_cursor_formats[] = {
> > > > > >  };
> > > > > >  
> > > > > >  struct vkms_crc_data {
> > > > > > -	struct drm_rect src;
> > > > > >  	struct drm_framebuffer fb;
> > > > > > +	struct drm_rect src, dst;
> > > > > > +	unsigned int offset;
> > > > > > +	unsigned int pitch;
> > > > > > +	unsigned int cpp;
> > > > > >  };
> > > > > >  
> > > > > >  /**
> > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > > > > > index 428247d403dc..7041007396ae 100644
> > > > > > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > > > > > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > > > > > @@ -85,16 +85,22 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
> > > > > >  				     struct drm_plane_state *old_state)
> > > > > >  {
> > > > > >  	struct vkms_plane_state *vkms_plane_state;
> > > > > > +	struct drm_framebuffer *fb = plane->state->fb;
> > > > > >  	struct vkms_crc_data *crc_data;
> > > > > >  
> > > > > > -	if (!plane->state->crtc || !plane->state->fb)
> > > > > > +	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->fb, plane->state->fb, sizeof(struct drm_framebuffer));
> > > > > > +	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];
> > > > > >  }
> > > > > >  
> > > > > >  static int vkms_plane_atomic_check(struct drm_plane *plane,
> > > > > > -- 
> > > > > > 2.17.1
> > > > > > 
> > > > > 
> > > > > -- 
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/vkms: Compute CRC with Cursor Plane

[Haneen Mohammed]

On Tue, Aug 14, 2018 at 09:52:33PM +0200, Daniel Vetter wrote:
> On Tue, Aug 14, 2018 at 9:03 PM, Haneen Mohammed
> <hamohammed.sa@gmail.com> wrote:
> > On Tue, Aug 14, 2018 at 10:21:29AM +0200, Daniel Vetter wrote:
> >> On Mon, Aug 13, 2018 at 11:04:11PM +0300, Haneen Mohammed wrote:
> >> > On Wed, Aug 08, 2018 at 10:23:27AM +0200, Daniel Vetter wrote:
> >> > > On Wed, Aug 08, 2018 at 06:53:17AM +0300, Haneen Mohammed wrote:
> >> > > > On Tue, Aug 07, 2018 at 06:33:36PM +0200, Daniel Vetter wrote:
> >> > > > > On Mon, Aug 06, 2018 at 06:58:29AM +0300, Haneen Mohammed wrote:
> >> > > > > > This patch compute CRC for output frame with cursor and primary plane.
> >> > > > > > Blend cursor with primary plane and compute CRC on the resulted frame.
> >> > > > > >
> >> > > > > > Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
> >> > > > >
> >> > > > > Nice!
> >> > > > >
> >> > > > > I assume with this you're passing all the crc based cursor tests in igt?
> >> > > > > If so, please mention this in the commit message, so that there's a record
> >> > > > > of the testing done on this.
> >> > > > >
> >> > > >
> >> > > > Sure, I'll update the commit message.
> >> > > > Is there any other change I should add/fix to this patchset?
> >> > > >
> >> > > > > One fairly huge gap we iirc have in our cursor testing is that there's not
> >> > > > > much (if any?) alpha blending coverage. We kinda need that to make sure
> >> > > > > this all works correctly. The usual trick is to only check the extreme
> >> > > > > alpha values, i.e. fully opaque and fully transparent, since intermediate
> >> > > > > values are affected by hw-specific rounding modes.
> >> > > > > > ---
> >> > > > > >  drivers/gpu/drm/vkms/vkms_crc.c   | 149 +++++++++++++++++++++++++-----
> >> > > > > >  drivers/gpu/drm/vkms/vkms_drv.h   |   5 +-
> >> > > > > >  drivers/gpu/drm/vkms/vkms_plane.c |  10 +-
> >> > > > > >  3 files changed, 137 insertions(+), 27 deletions(-)
> >> > > > > >
> >> > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> >> > > > > > index 37d717f38e3c..4853a739ae5a 100644
> >> > > > > > --- a/drivers/gpu/drm/vkms/vkms_crc.c
> >> > > > > > +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> >> > > > > > @@ -1,36 +1,137 @@
> >> > > > > >  // SPDX-License-Identifier: GPL-2.0
> >> > > > > >  #include "vkms_drv.h"
> >> > > > > >  #include <linux/crc32.h>
> >> > > > > > +#include <drm/drm_atomic.h>
> >> > > > > > +#include <drm/drm_atomic_helper.h>
> >> > > > > >  #include <drm/drm_gem_framebuffer_helper.h>
> >> > > > > >
> >> > > > > > -static uint32_t _vkms_get_crc(struct vkms_crc_data *crc_data)
> >> > > > > > +/**
> >> > > > > > + * compute_crc - Compute CRC value on output frame
> >> > > > > > + *
> >> > > > > > + * @vaddr_out: address to final framebuffer
> >> > > > > > + * @crc_out: 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)
> >> > > > > >  {
> >> > > > > > -   struct drm_framebuffer *fb = &crc_data->fb;
> >> > > > > > +   int i, 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 size_byte = w_src * crc_out->cpp;
> >> > > > > > +   u32 crc = 0;
> >> > > > > > +
> >> > > > > > +   for (i = y_src; i < y_src + h_src; i++) {
> >> > > > > > +           src_offset = crc_out->offset
> >> > > > > > +                        + (i * crc_out->pitch)
> >> > > > > > +                        + (x_src * crc_out->cpp);
> >> > > > > > +           crc = crc32_le(crc, vaddr_out + src_offset, size_byte);
> >> > > > > > +   }
> >> > > > > > +
> >> > > > > > +   return crc;
> >> > > > > > +}
> >> > > > > > +
> >> >
> >> > Hey Daniel,
> >> >
> >> > When I change the above function "compute_crc" to compute the CRC for
> >> > each pixel so we can clear the Alpha value before computing the CRC as bellow,
> >> > the test "nonblocking-crc-pipe-A-frame-sequence" sometimes failes and
> >> > sometimes passes. Should I still continue with the change, or leave it
> >> > as it is?
> >>
> >> Hm, how does it fail? Could be that the code becomes too slow, and then
> >> the frame counters don't increment nicely anymore. Or there's a race
> >> somewhere in the CRC code that gets exposed. But that's just me guessing.
> >> -Daniel
> >>
> >
> > Oh sorry I should've elaborated more on how it fails. Yeah I think it
> > becomes too slow and that can cause mismatch to frame counters?
> >
> > -----
> > (kms_pipe_crc_basic:1902) CRITICAL: Failed assertion: crcs[j].frame + 1 == crcs[j + 1].frame
> > (kms_pipe_crc_basic:1902) CRITICAL: Last errno: 9, Bad file descriptor
> > (kms_pipe_crc_basic:1902) CRITICAL: error: 1090928 != 1090929
> > -----
> 
> Yeah misses the deadline it seems.
> 
> > Maybe it's not that bad/slow because I've noticed that this happen
> > occasionally when I enable Ftrace.
> 
> Yeah ftrace can have some decent overhead. And we're adding a pile of
> function calls, because crc32_le isn't inlined. Does it still happen
> without ftrace?
> 

No, and it only happen occasionally with ftrace.

> Optimizing this will be some real work - we'd need a local cache to
> compute an entire block of blended pixels into, then add an entire
> block. Block size needs to be large enough to be efficient, but not so
> big that we start trashing the cache or have to allocate memory (stack
> space in the kernel is very limited). Would be good if we can avoid
> all this work just now ...
> -Danie
> 
> >
> >> >
> >> > ----------------- vkms_crc.c -----------------
> >> >     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);
> >> >                     memset(vaddr_out + src_offset + 24, 0,  8);
> >> >                     crc = crc32_le(crc, vaddr_out + src_offset, sizeof(u32));
> >> >             }
> >> >     }
> >> > ----------------- vkms_crc.c -----------------
> >> >
> >> > Thank you,
> >> > Haneen
> >> >
> >> > > > > > +/**
> >> > > > > > + * 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
> >> > > > > > + *
> >> > > > > > + * Blend value at vaddr_src with value at vaddr_dst.
> >> > > > > > + * Currently, this function write value at vaddr_src on value
> >> > > > > > + * at vaddr_dst using buffer's metadata to locate the new values
> >> > > > > > + * from vaddr_src and their distenation at vaddr_dst.
> >> > > > > > + *
> >> > > > > > + * Todo: Use the alpha value to blend vaddr_src with vaddr_dst
> >> > > > > > + *  instead of overwriting it.
> >> > > > >
> >> > > > > Another todo: We must clear the alpha channel in the result after
> >> > > > > blending. This also applies to the primary plane, where the XRGB for the
> >> > > > > pixel format mandates that we entirely ignore the alpha channel.
> >> > > > >
> >> > > > > This is also something we should probably have an igt testcase for.
> >> > > > >
> >> > > > > Since there's a few open ends: How many weeks are left in your outreachy
> >> > > > > internship? We need to make sure that at least all the issues are covered
> >> > > > > in a vkms todo file. Would be great to add that in
> >> > > > > Documentation/gpu/vkms.rst, like we have for other drivers.
> >> > > > > -Daniel
> >> > > > >
> >> > > >
> >> > > > I've one more week! I can use this week to prepare the todo file and
> >> > > > finalize this patch?
> >> > >
> >> > > Yeah sounds like the perfect wrap-up work. Since this wont be enough to
> >> > > finish the cursor work it would be good to hide the cursor setup behind a
> >> > > module option, perhaps "enable_cursor" or so. That way we wont have
> >> > > not-totally-correct features enabled. And enabling/disabling cursor
> >> > > support could be useful for testing.
> >> > > -Daniel
> >> > >
> >> > > >
> >> > > > Thank you,
> >> > > > Haneen
> >> > > >
> >> > > > > > + */
> >> > > > > > +static void blend(void *vaddr_dst, void *vaddr_src,
> >> > > > > > +             struct vkms_crc_data *crc_dst,
> >> > > > > > +             struct vkms_crc_data *crc_src)
> >> > > > > > +{
> >> > > > > > +   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_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 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);
> >> > > > > > +
> >> > > > > > +                   memcpy(vaddr_dst + offset_dst,
> >> > > > > > +                          vaddr_src + offset_src, sizeof(u32));
> >> > > > > > +           }
> >> > > > > > +           i_dst++;
> >> > > > > > +   }
> >> > > > > > +}
> >> > > > > > +
> >> > > > > > +static void compose_cursor(struct vkms_crc_data *cursor_crc,
> >> > > > > > +                      struct vkms_crc_data *primary_crc, 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_vkms_obj = drm_gem_to_vkms_gem(cursor_obj);
> >> > > > > > +
> >> > > > > > +   mutex_lock(&cursor_vkms_obj->pages_lock);
> >> > > > > > +   if (WARN_ON(!cursor_vkms_obj->vaddr))
> >> > > > > > +           goto out;
> >> > > > > > +
> >> > > > > > +   blend(vaddr_out, cursor_vkms_obj->vaddr, primary_crc, cursor_crc);
> >> > > > > > +
> >> > > > > > +out:
> >> > > > > > +   mutex_unlock(&cursor_vkms_obj->pages_lock);
> >> > > > > > +}
> >> > > > > > +
> >> > > > > > +static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
> >> > > > > > +                         struct vkms_crc_data *cursor_crc)
> >> > > > > > +{
> >> > > > > > +   struct drm_framebuffer *fb = &primary_crc->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;
> >> > > > > > -   int i = 0;
> >> > > > > > -   unsigned int x = crc_data->src.x1 >> 16;
> >> > > > > > -   unsigned int y = crc_data->src.y1 >> 16;
> >> > > > > > -   unsigned int height = drm_rect_height(&crc_data->src) >> 16;
> >> > > > > > -   unsigned int width = drm_rect_width(&crc_data->src) >> 16;
> >> > > > > > -   unsigned int cpp = fb->format->cpp[0];
> >> > > > > > -   unsigned int src_offset;
> >> > > > > > -   unsigned int size_byte = width * cpp;
> >> > > > > > -   void *vaddr;
> >> > > > > >
> >> > > > > > -   mutex_lock(&vkms_obj->pages_lock);
> >> > > > > > -   vaddr = vkms_obj->vaddr;
> >> > > > > > -   if (WARN_ON(!vaddr))
> >> > > > > > -           goto out;
> >> > > > > > +   if (!vaddr_out) {
> >> > > > > > +           DRM_ERROR("Failed to allocate memory for output frame.");
> >> > > > > > +           return 0;
> >> > > > > > +   }
> >> > > > > >
> >> > > > > > -   for (i = y; i < y + height; i++) {
> >> > > > > > -           src_offset = fb->offsets[0] + (i * fb->pitches[0]) + (x * cpp);
> >> > > > > > -           crc = crc32_le(crc, vaddr + src_offset, size_byte);
> >> > > > > > +   mutex_lock(&vkms_obj->pages_lock);
> >> > > > > > +   if (WARN_ON(!vkms_obj->vaddr)) {
> >> > > > > > +           mutex_lock(&vkms_obj->pages_lock);
> >> > > > > > +           return crc;
> >> > > > > >     }
> >> > > > > >
> >> > > > > > -out:
> >> > > > > > +   memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
> >> > > > > >     mutex_unlock(&vkms_obj->pages_lock);
> >> > > > > > +
> >> > > > > > +   if (cursor_crc)
> >> > > > > > +           compose_cursor(cursor_crc, primary_crc, vaddr_out);
> >> > > > > > +
> >> > > > > > +   crc = compute_crc(vaddr_out, primary_crc);
> >> > > > > > +
> >> > > > > > +   kfree(vaddr_out);
> >> > > > > > +
> >> > > > > >     return crc;
> >> > > > > >  }
> >> > > > > >
> >> > > > > > @@ -44,8 +145,8 @@ void vkms_crc_work_handle(struct work_struct *work)
> >> > > > > >     struct vkms_device *vdev = container_of(out, struct vkms_device,
> >> > > > > >                                             output);
> >> > > > > >     struct vkms_crc_data *primary_crc = NULL;
> >> > > > > > +   struct vkms_crc_data *cursor_crc = NULL;
> >> > > > > >     struct drm_plane *plane;
> >> > > > > > -
> >> > > > > >     u32 crc32 = 0;
> >> > > > > >
> >> > > > > >     drm_for_each_plane(plane, &vdev->drm) {
> >> > > > > > @@ -58,14 +159,14 @@ void vkms_crc_work_handle(struct work_struct *work)
> >> > > > > >             if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
> >> > > > > >                     continue;
> >> > > > > >
> >> > > > > > -           if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> >> > > > > > +           if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> >> > > > > >                     primary_crc = crc_data;
> >> > > > > > -                   break;
> >> > > > > > -           }
> >> > > > > > +           else
> >> > > > > > +                   cursor_crc = crc_data;
> >> > > > > >     }
> >> > > > > >
> >> > > > > >     if (primary_crc)
> >> > > > > > -           crc32 = _vkms_get_crc(primary_crc);
> >> > > > > > +           crc32 = _vkms_get_crc(primary_crc, cursor_crc);
> >> > > > > >
> >> > > > > >     drm_crtc_add_crc_entry(crtc, true, crtc_state->n_frame, &crc32);
> >> > > > > >  }
> >> > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> >> > > > > > index 36e524f791fe..173875dc361e 100644
> >> > > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> >> > > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> >> > > > > > @@ -25,8 +25,11 @@ static const u32 vkms_cursor_formats[] = {
> >> > > > > >  };
> >> > > > > >
> >> > > > > >  struct vkms_crc_data {
> >> > > > > > -   struct drm_rect src;
> >> > > > > >     struct drm_framebuffer fb;
> >> > > > > > +   struct drm_rect src, dst;
> >> > > > > > +   unsigned int offset;
> >> > > > > > +   unsigned int pitch;
> >> > > > > > +   unsigned int cpp;
> >> > > > > >  };
> >> > > > > >
> >> > > > > >  /**
> >> > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> >> > > > > > index 428247d403dc..7041007396ae 100644
> >> > > > > > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> >> > > > > > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> >> > > > > > @@ -85,16 +85,22 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
> >> > > > > >                                  struct drm_plane_state *old_state)
> >> > > > > >  {
> >> > > > > >     struct vkms_plane_state *vkms_plane_state;
> >> > > > > > +   struct drm_framebuffer *fb = plane->state->fb;
> >> > > > > >     struct vkms_crc_data *crc_data;
> >> > > > > >
> >> > > > > > -   if (!plane->state->crtc || !plane->state->fb)
> >> > > > > > +   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->fb, plane->state->fb, sizeof(struct drm_framebuffer));
> >> > > > > > +   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];
> >> > > > > >  }
> >> > > > > >
> >> > > > > >  static int vkms_plane_atomic_check(struct drm_plane *plane,
> >> > > > > > --
> >> > > > > > 2.17.1
> >> > > > > >
> >> > > > >
> >> > > > > --
> >> > > > > Daniel Vetter
> >> > > > > Software Engineer, Intel Corporation
> >> > > > > http://blog.ffwll.ch
> >> > >
> >> > > --
> >> > > Daniel Vetter
> >> > > Software Engineer, Intel Corporation
> >> > > http://blog.ffwll.ch
> >>
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/vkms: Compute CRC with Cursor Plane

[Daniel Vetter]

On Tue, Aug 14, 2018 at 9:03 PM, Haneen Mohammed
<hamohammed.sa@gmail.com> wrote:
> On Tue, Aug 14, 2018 at 10:21:29AM +0200, Daniel Vetter wrote:
>> On Mon, Aug 13, 2018 at 11:04:11PM +0300, Haneen Mohammed wrote:
>> > On Wed, Aug 08, 2018 at 10:23:27AM +0200, Daniel Vetter wrote:
>> > > On Wed, Aug 08, 2018 at 06:53:17AM +0300, Haneen Mohammed wrote:
>> > > > On Tue, Aug 07, 2018 at 06:33:36PM +0200, Daniel Vetter wrote:
>> > > > > On Mon, Aug 06, 2018 at 06:58:29AM +0300, Haneen Mohammed wrote:
>> > > > > > This patch compute CRC for output frame with cursor and primary plane.
>> > > > > > Blend cursor with primary plane and compute CRC on the resulted frame.
>> > > > > >
>> > > > > > Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
>> > > > >
>> > > > > Nice!
>> > > > >
>> > > > > I assume with this you're passing all the crc based cursor tests in igt?
>> > > > > If so, please mention this in the commit message, so that there's a record
>> > > > > of the testing done on this.
>> > > > >
>> > > >
>> > > > Sure, I'll update the commit message.
>> > > > Is there any other change I should add/fix to this patchset?
>> > > >
>> > > > > One fairly huge gap we iirc have in our cursor testing is that there's not
>> > > > > much (if any?) alpha blending coverage. We kinda need that to make sure
>> > > > > this all works correctly. The usual trick is to only check the extreme
>> > > > > alpha values, i.e. fully opaque and fully transparent, since intermediate
>> > > > > values are affected by hw-specific rounding modes.
>> > > > > > ---
>> > > > > >  drivers/gpu/drm/vkms/vkms_crc.c   | 149 +++++++++++++++++++++++++-----
>> > > > > >  drivers/gpu/drm/vkms/vkms_drv.h   |   5 +-
>> > > > > >  drivers/gpu/drm/vkms/vkms_plane.c |  10 +-
>> > > > > >  3 files changed, 137 insertions(+), 27 deletions(-)
>> > > > > >
>> > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
>> > > > > > index 37d717f38e3c..4853a739ae5a 100644
>> > > > > > --- a/drivers/gpu/drm/vkms/vkms_crc.c
>> > > > > > +++ b/drivers/gpu/drm/vkms/vkms_crc.c
>> > > > > > @@ -1,36 +1,137 @@
>> > > > > >  // SPDX-License-Identifier: GPL-2.0
>> > > > > >  #include "vkms_drv.h"
>> > > > > >  #include <linux/crc32.h>
>> > > > > > +#include <drm/drm_atomic.h>
>> > > > > > +#include <drm/drm_atomic_helper.h>
>> > > > > >  #include <drm/drm_gem_framebuffer_helper.h>
>> > > > > >
>> > > > > > -static uint32_t _vkms_get_crc(struct vkms_crc_data *crc_data)
>> > > > > > +/**
>> > > > > > + * compute_crc - Compute CRC value on output frame
>> > > > > > + *
>> > > > > > + * @vaddr_out: address to final framebuffer
>> > > > > > + * @crc_out: 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)
>> > > > > >  {
>> > > > > > -   struct drm_framebuffer *fb = &crc_data->fb;
>> > > > > > +   int i, 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 size_byte = w_src * crc_out->cpp;
>> > > > > > +   u32 crc = 0;
>> > > > > > +
>> > > > > > +   for (i = y_src; i < y_src + h_src; i++) {
>> > > > > > +           src_offset = crc_out->offset
>> > > > > > +                        + (i * crc_out->pitch)
>> > > > > > +                        + (x_src * crc_out->cpp);
>> > > > > > +           crc = crc32_le(crc, vaddr_out + src_offset, size_byte);
>> > > > > > +   }
>> > > > > > +
>> > > > > > +   return crc;
>> > > > > > +}
>> > > > > > +
>> >
>> > Hey Daniel,
>> >
>> > When I change the above function "compute_crc" to compute the CRC for
>> > each pixel so we can clear the Alpha value before computing the CRC as bellow,
>> > the test "nonblocking-crc-pipe-A-frame-sequence" sometimes failes and
>> > sometimes passes. Should I still continue with the change, or leave it
>> > as it is?
>>
>> Hm, how does it fail? Could be that the code becomes too slow, and then
>> the frame counters don't increment nicely anymore. Or there's a race
>> somewhere in the CRC code that gets exposed. But that's just me guessing.
>> -Daniel
>>
>
> Oh sorry I should've elaborated more on how it fails. Yeah I think it
> becomes too slow and that can cause mismatch to frame counters?
>
> -----
> (kms_pipe_crc_basic:1902) CRITICAL: Failed assertion: crcs[j].frame + 1 == crcs[j + 1].frame
> (kms_pipe_crc_basic:1902) CRITICAL: Last errno: 9, Bad file descriptor
> (kms_pipe_crc_basic:1902) CRITICAL: error: 1090928 != 1090929
> -----

Yeah misses the deadline it seems.

> Maybe it's not that bad/slow because I've noticed that this happen
> occasionally when I enable Ftrace.

Yeah ftrace can have some decent overhead. And we're adding a pile of
function calls, because crc32_le isn't inlined. Does it still happen
without ftrace?

Optimizing this will be some real work - we'd need a local cache to
compute an entire block of blended pixels into, then add an entire
block. Block size needs to be large enough to be efficient, but not so
big that we start trashing the cache or have to allocate memory (stack
space in the kernel is very limited). Would be good if we can avoid
all this work just now ...
-Danie

>
>> >
>> > ----------------- vkms_crc.c -----------------
>> >     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);
>> >                     memset(vaddr_out + src_offset + 24, 0,  8);
>> >                     crc = crc32_le(crc, vaddr_out + src_offset, sizeof(u32));
>> >             }
>> >     }
>> > ----------------- vkms_crc.c -----------------
>> >
>> > Thank you,
>> > Haneen
>> >
>> > > > > > +/**
>> > > > > > + * 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
>> > > > > > + *
>> > > > > > + * Blend value at vaddr_src with value at vaddr_dst.
>> > > > > > + * Currently, this function write value at vaddr_src on value
>> > > > > > + * at vaddr_dst using buffer's metadata to locate the new values
>> > > > > > + * from vaddr_src and their distenation at vaddr_dst.
>> > > > > > + *
>> > > > > > + * Todo: Use the alpha value to blend vaddr_src with vaddr_dst
>> > > > > > + *  instead of overwriting it.
>> > > > >
>> > > > > Another todo: We must clear the alpha channel in the result after
>> > > > > blending. This also applies to the primary plane, where the XRGB for the
>> > > > > pixel format mandates that we entirely ignore the alpha channel.
>> > > > >
>> > > > > This is also something we should probably have an igt testcase for.
>> > > > >
>> > > > > Since there's a few open ends: How many weeks are left in your outreachy
>> > > > > internship? We need to make sure that at least all the issues are covered
>> > > > > in a vkms todo file. Would be great to add that in
>> > > > > Documentation/gpu/vkms.rst, like we have for other drivers.
>> > > > > -Daniel
>> > > > >
>> > > >
>> > > > I've one more week! I can use this week to prepare the todo file and
>> > > > finalize this patch?
>> > >
>> > > Yeah sounds like the perfect wrap-up work. Since this wont be enough to
>> > > finish the cursor work it would be good to hide the cursor setup behind a
>> > > module option, perhaps "enable_cursor" or so. That way we wont have
>> > > not-totally-correct features enabled. And enabling/disabling cursor
>> > > support could be useful for testing.
>> > > -Daniel
>> > >
>> > > >
>> > > > Thank you,
>> > > > Haneen
>> > > >
>> > > > > > + */
>> > > > > > +static void blend(void *vaddr_dst, void *vaddr_src,
>> > > > > > +             struct vkms_crc_data *crc_dst,
>> > > > > > +             struct vkms_crc_data *crc_src)
>> > > > > > +{
>> > > > > > +   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_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 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);
>> > > > > > +
>> > > > > > +                   memcpy(vaddr_dst + offset_dst,
>> > > > > > +                          vaddr_src + offset_src, sizeof(u32));
>> > > > > > +           }
>> > > > > > +           i_dst++;
>> > > > > > +   }
>> > > > > > +}
>> > > > > > +
>> > > > > > +static void compose_cursor(struct vkms_crc_data *cursor_crc,
>> > > > > > +                      struct vkms_crc_data *primary_crc, 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_vkms_obj = drm_gem_to_vkms_gem(cursor_obj);
>> > > > > > +
>> > > > > > +   mutex_lock(&cursor_vkms_obj->pages_lock);
>> > > > > > +   if (WARN_ON(!cursor_vkms_obj->vaddr))
>> > > > > > +           goto out;
>> > > > > > +
>> > > > > > +   blend(vaddr_out, cursor_vkms_obj->vaddr, primary_crc, cursor_crc);
>> > > > > > +
>> > > > > > +out:
>> > > > > > +   mutex_unlock(&cursor_vkms_obj->pages_lock);
>> > > > > > +}
>> > > > > > +
>> > > > > > +static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
>> > > > > > +                         struct vkms_crc_data *cursor_crc)
>> > > > > > +{
>> > > > > > +   struct drm_framebuffer *fb = &primary_crc->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;
>> > > > > > -   int i = 0;
>> > > > > > -   unsigned int x = crc_data->src.x1 >> 16;
>> > > > > > -   unsigned int y = crc_data->src.y1 >> 16;
>> > > > > > -   unsigned int height = drm_rect_height(&crc_data->src) >> 16;
>> > > > > > -   unsigned int width = drm_rect_width(&crc_data->src) >> 16;
>> > > > > > -   unsigned int cpp = fb->format->cpp[0];
>> > > > > > -   unsigned int src_offset;
>> > > > > > -   unsigned int size_byte = width * cpp;
>> > > > > > -   void *vaddr;
>> > > > > >
>> > > > > > -   mutex_lock(&vkms_obj->pages_lock);
>> > > > > > -   vaddr = vkms_obj->vaddr;
>> > > > > > -   if (WARN_ON(!vaddr))
>> > > > > > -           goto out;
>> > > > > > +   if (!vaddr_out) {
>> > > > > > +           DRM_ERROR("Failed to allocate memory for output frame.");
>> > > > > > +           return 0;
>> > > > > > +   }
>> > > > > >
>> > > > > > -   for (i = y; i < y + height; i++) {
>> > > > > > -           src_offset = fb->offsets[0] + (i * fb->pitches[0]) + (x * cpp);
>> > > > > > -           crc = crc32_le(crc, vaddr + src_offset, size_byte);
>> > > > > > +   mutex_lock(&vkms_obj->pages_lock);
>> > > > > > +   if (WARN_ON(!vkms_obj->vaddr)) {
>> > > > > > +           mutex_lock(&vkms_obj->pages_lock);
>> > > > > > +           return crc;
>> > > > > >     }
>> > > > > >
>> > > > > > -out:
>> > > > > > +   memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
>> > > > > >     mutex_unlock(&vkms_obj->pages_lock);
>> > > > > > +
>> > > > > > +   if (cursor_crc)
>> > > > > > +           compose_cursor(cursor_crc, primary_crc, vaddr_out);
>> > > > > > +
>> > > > > > +   crc = compute_crc(vaddr_out, primary_crc);
>> > > > > > +
>> > > > > > +   kfree(vaddr_out);
>> > > > > > +
>> > > > > >     return crc;
>> > > > > >  }
>> > > > > >
>> > > > > > @@ -44,8 +145,8 @@ void vkms_crc_work_handle(struct work_struct *work)
>> > > > > >     struct vkms_device *vdev = container_of(out, struct vkms_device,
>> > > > > >                                             output);
>> > > > > >     struct vkms_crc_data *primary_crc = NULL;
>> > > > > > +   struct vkms_crc_data *cursor_crc = NULL;
>> > > > > >     struct drm_plane *plane;
>> > > > > > -
>> > > > > >     u32 crc32 = 0;
>> > > > > >
>> > > > > >     drm_for_each_plane(plane, &vdev->drm) {
>> > > > > > @@ -58,14 +159,14 @@ void vkms_crc_work_handle(struct work_struct *work)
>> > > > > >             if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
>> > > > > >                     continue;
>> > > > > >
>> > > > > > -           if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
>> > > > > > +           if (plane->type == DRM_PLANE_TYPE_PRIMARY)
>> > > > > >                     primary_crc = crc_data;
>> > > > > > -                   break;
>> > > > > > -           }
>> > > > > > +           else
>> > > > > > +                   cursor_crc = crc_data;
>> > > > > >     }
>> > > > > >
>> > > > > >     if (primary_crc)
>> > > > > > -           crc32 = _vkms_get_crc(primary_crc);
>> > > > > > +           crc32 = _vkms_get_crc(primary_crc, cursor_crc);
>> > > > > >
>> > > > > >     drm_crtc_add_crc_entry(crtc, true, crtc_state->n_frame, &crc32);
>> > > > > >  }
>> > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
>> > > > > > index 36e524f791fe..173875dc361e 100644
>> > > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
>> > > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
>> > > > > > @@ -25,8 +25,11 @@ static const u32 vkms_cursor_formats[] = {
>> > > > > >  };
>> > > > > >
>> > > > > >  struct vkms_crc_data {
>> > > > > > -   struct drm_rect src;
>> > > > > >     struct drm_framebuffer fb;
>> > > > > > +   struct drm_rect src, dst;
>> > > > > > +   unsigned int offset;
>> > > > > > +   unsigned int pitch;
>> > > > > > +   unsigned int cpp;
>> > > > > >  };
>> > > > > >
>> > > > > >  /**
>> > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
>> > > > > > index 428247d403dc..7041007396ae 100644
>> > > > > > --- a/drivers/gpu/drm/vkms/vkms_plane.c
>> > > > > > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
>> > > > > > @@ -85,16 +85,22 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
>> > > > > >                                  struct drm_plane_state *old_state)
>> > > > > >  {
>> > > > > >     struct vkms_plane_state *vkms_plane_state;
>> > > > > > +   struct drm_framebuffer *fb = plane->state->fb;
>> > > > > >     struct vkms_crc_data *crc_data;
>> > > > > >
>> > > > > > -   if (!plane->state->crtc || !plane->state->fb)
>> > > > > > +   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->fb, plane->state->fb, sizeof(struct drm_framebuffer));
>> > > > > > +   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];
>> > > > > >  }
>> > > > > >
>> > > > > >  static int vkms_plane_atomic_check(struct drm_plane *plane,
>> > > > > > --
>> > > > > > 2.17.1
>> > > > > >
>> > > > >
>> > > > > --
>> > > > > Daniel Vetter
>> > > > > Software Engineer, Intel Corporation
>> > > > > http://blog.ffwll.ch
>> > >
>> > > --
>> > > Daniel Vetter
>> > > Software Engineer, Intel Corporation
>> > > http://blog.ffwll.ch
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/vkms: Compute CRC with Cursor Plane

[Haneen Mohammed]

On Tue, Aug 14, 2018 at 09:52:33PM +0200, Daniel Vetter wrote:
> On Tue, Aug 14, 2018 at 9:03 PM, Haneen Mohammed
> <hamohammed.sa@gmail.com> wrote:
> > On Tue, Aug 14, 2018 at 10:21:29AM +0200, Daniel Vetter wrote:
> >> On Mon, Aug 13, 2018 at 11:04:11PM +0300, Haneen Mohammed wrote:
> >> > On Wed, Aug 08, 2018 at 10:23:27AM +0200, Daniel Vetter wrote:
> >> > > On Wed, Aug 08, 2018 at 06:53:17AM +0300, Haneen Mohammed wrote:
> >> > > > On Tue, Aug 07, 2018 at 06:33:36PM +0200, Daniel Vetter wrote:
> >> > > > > On Mon, Aug 06, 2018 at 06:58:29AM +0300, Haneen Mohammed wrote:
> >> > > > > > This patch compute CRC for output frame with cursor and primary plane.
> >> > > > > > Blend cursor with primary plane and compute CRC on the resulted frame.
> >> > > > > >
> >> > > > > > Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
> >> > > > >
> >> > > > > Nice!
> >> > > > >
> >> > > > > I assume with this you're passing all the crc based cursor tests in igt?
> >> > > > > If so, please mention this in the commit message, so that there's a record
> >> > > > > of the testing done on this.
> >> > > > >
> >> > > >
> >> > > > Sure, I'll update the commit message.
> >> > > > Is there any other change I should add/fix to this patchset?
> >> > > >
> >> > > > > One fairly huge gap we iirc have in our cursor testing is that there's not
> >> > > > > much (if any?) alpha blending coverage. We kinda need that to make sure
> >> > > > > this all works correctly. The usual trick is to only check the extreme
> >> > > > > alpha values, i.e. fully opaque and fully transparent, since intermediate
> >> > > > > values are affected by hw-specific rounding modes.
> >> > > > > > ---
> >> > > > > >  drivers/gpu/drm/vkms/vkms_crc.c   | 149 +++++++++++++++++++++++++-----
> >> > > > > >  drivers/gpu/drm/vkms/vkms_drv.h   |   5 +-
> >> > > > > >  drivers/gpu/drm/vkms/vkms_plane.c |  10 +-
> >> > > > > >  3 files changed, 137 insertions(+), 27 deletions(-)
> >> > > > > >
> >> > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> >> > > > > > index 37d717f38e3c..4853a739ae5a 100644
> >> > > > > > --- a/drivers/gpu/drm/vkms/vkms_crc.c
> >> > > > > > +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> >> > > > > > @@ -1,36 +1,137 @@
> >> > > > > >  // SPDX-License-Identifier: GPL-2.0
> >> > > > > >  #include "vkms_drv.h"
> >> > > > > >  #include <linux/crc32.h>
> >> > > > > > +#include <drm/drm_atomic.h>
> >> > > > > > +#include <drm/drm_atomic_helper.h>
> >> > > > > >  #include <drm/drm_gem_framebuffer_helper.h>
> >> > > > > >
> >> > > > > > -static uint32_t _vkms_get_crc(struct vkms_crc_data *crc_data)
> >> > > > > > +/**
> >> > > > > > + * compute_crc - Compute CRC value on output frame
> >> > > > > > + *
> >> > > > > > + * @vaddr_out: address to final framebuffer
> >> > > > > > + * @crc_out: 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)
> >> > > > > >  {
> >> > > > > > -   struct drm_framebuffer *fb = &crc_data->fb;
> >> > > > > > +   int i, 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 size_byte = w_src * crc_out->cpp;
> >> > > > > > +   u32 crc = 0;
> >> > > > > > +
> >> > > > > > +   for (i = y_src; i < y_src + h_src; i++) {
> >> > > > > > +           src_offset = crc_out->offset
> >> > > > > > +                        + (i * crc_out->pitch)
> >> > > > > > +                        + (x_src * crc_out->cpp);
> >> > > > > > +           crc = crc32_le(crc, vaddr_out + src_offset, size_byte);
> >> > > > > > +   }
> >> > > > > > +
> >> > > > > > +   return crc;
> >> > > > > > +}
> >> > > > > > +
> >> >
> >> > Hey Daniel,
> >> >
> >> > When I change the above function "compute_crc" to compute the CRC for
> >> > each pixel so we can clear the Alpha value before computing the CRC as bellow,
> >> > the test "nonblocking-crc-pipe-A-frame-sequence" sometimes failes and
> >> > sometimes passes. Should I still continue with the change, or leave it
> >> > as it is?
> >>
> >> Hm, how does it fail? Could be that the code becomes too slow, and then
> >> the frame counters don't increment nicely anymore. Or there's a race
> >> somewhere in the CRC code that gets exposed. But that's just me guessing.
> >> -Daniel
> >>

Hi Daniel,

I figured out why nonblocking-crc-pipe-A-frame-sequence fails.
queue_work sometimes return false so vkms_crc_work_handle doesn't get scheduled
for a given frame number. I'm not sure though how this can be fixed.

- Haneen

> >
> > Oh sorry I should've elaborated more on how it fails. Yeah I think it
> > becomes too slow and that can cause mismatch to frame counters?
> >
> > -----
> > (kms_pipe_crc_basic:1902) CRITICAL: Failed assertion: crcs[j].frame + 1 == crcs[j + 1].frame
> > (kms_pipe_crc_basic:1902) CRITICAL: Last errno: 9, Bad file descriptor
> > (kms_pipe_crc_basic:1902) CRITICAL: error: 1090928 != 1090929
> > -----
> 
> Yeah misses the deadline it seems.
> 
> > Maybe it's not that bad/slow because I've noticed that this happen
> > occasionally when I enable Ftrace.
> 
> Yeah ftrace can have some decent overhead. And we're adding a pile of
> function calls, because crc32_le isn't inlined. Does it still happen
> without ftrace?
> 
> Optimizing this will be some real work - we'd need a local cache to
> compute an entire block of blended pixels into, then add an entire
> block. Block size needs to be large enough to be efficient, but not so
> big that we start trashing the cache or have to allocate memory (stack
> space in the kernel is very limited). Would be good if we can avoid
> all this work just now ...
> -Danie
> 
> >
> >> >
> >> > ----------------- vkms_crc.c -----------------
> >> >     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);
> >> >                     memset(vaddr_out + src_offset + 24, 0,  8);
> >> >                     crc = crc32_le(crc, vaddr_out + src_offset, sizeof(u32));
> >> >             }
> >> >     }
> >> > ----------------- vkms_crc.c -----------------
> >> >
> >> > Thank you,
> >> > Haneen
> >> >
> >> > > > > > +/**
> >> > > > > > + * 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
> >> > > > > > + *
> >> > > > > > + * Blend value at vaddr_src with value at vaddr_dst.
> >> > > > > > + * Currently, this function write value at vaddr_src on value
> >> > > > > > + * at vaddr_dst using buffer's metadata to locate the new values
> >> > > > > > + * from vaddr_src and their distenation at vaddr_dst.
> >> > > > > > + *
> >> > > > > > + * Todo: Use the alpha value to blend vaddr_src with vaddr_dst
> >> > > > > > + *  instead of overwriting it.
> >> > > > >
> >> > > > > Another todo: We must clear the alpha channel in the result after
> >> > > > > blending. This also applies to the primary plane, where the XRGB for the
> >> > > > > pixel format mandates that we entirely ignore the alpha channel.
> >> > > > >
> >> > > > > This is also something we should probably have an igt testcase for.
> >> > > > >
> >> > > > > Since there's a few open ends: How many weeks are left in your outreachy
> >> > > > > internship? We need to make sure that at least all the issues are covered
> >> > > > > in a vkms todo file. Would be great to add that in
> >> > > > > Documentation/gpu/vkms.rst, like we have for other drivers.
> >> > > > > -Daniel
> >> > > > >
> >> > > >
> >> > > > I've one more week! I can use this week to prepare the todo file and
> >> > > > finalize this patch?
> >> > >
> >> > > Yeah sounds like the perfect wrap-up work. Since this wont be enough to
> >> > > finish the cursor work it would be good to hide the cursor setup behind a
> >> > > module option, perhaps "enable_cursor" or so. That way we wont have
> >> > > not-totally-correct features enabled. And enabling/disabling cursor
> >> > > support could be useful for testing.
> >> > > -Daniel
> >> > >
> >> > > >
> >> > > > Thank you,
> >> > > > Haneen
> >> > > >
> >> > > > > > + */
> >> > > > > > +static void blend(void *vaddr_dst, void *vaddr_src,
> >> > > > > > +             struct vkms_crc_data *crc_dst,
> >> > > > > > +             struct vkms_crc_data *crc_src)
> >> > > > > > +{
> >> > > > > > +   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_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 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);
> >> > > > > > +
> >> > > > > > +                   memcpy(vaddr_dst + offset_dst,
> >> > > > > > +                          vaddr_src + offset_src, sizeof(u32));
> >> > > > > > +           }
> >> > > > > > +           i_dst++;
> >> > > > > > +   }
> >> > > > > > +}
> >> > > > > > +
> >> > > > > > +static void compose_cursor(struct vkms_crc_data *cursor_crc,
> >> > > > > > +                      struct vkms_crc_data *primary_crc, 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_vkms_obj = drm_gem_to_vkms_gem(cursor_obj);
> >> > > > > > +
> >> > > > > > +   mutex_lock(&cursor_vkms_obj->pages_lock);
> >> > > > > > +   if (WARN_ON(!cursor_vkms_obj->vaddr))
> >> > > > > > +           goto out;
> >> > > > > > +
> >> > > > > > +   blend(vaddr_out, cursor_vkms_obj->vaddr, primary_crc, cursor_crc);
> >> > > > > > +
> >> > > > > > +out:
> >> > > > > > +   mutex_unlock(&cursor_vkms_obj->pages_lock);
> >> > > > > > +}
> >> > > > > > +
> >> > > > > > +static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
> >> > > > > > +                         struct vkms_crc_data *cursor_crc)
> >> > > > > > +{
> >> > > > > > +   struct drm_framebuffer *fb = &primary_crc->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;
> >> > > > > > -   int i = 0;
> >> > > > > > -   unsigned int x = crc_data->src.x1 >> 16;
> >> > > > > > -   unsigned int y = crc_data->src.y1 >> 16;
> >> > > > > > -   unsigned int height = drm_rect_height(&crc_data->src) >> 16;
> >> > > > > > -   unsigned int width = drm_rect_width(&crc_data->src) >> 16;
> >> > > > > > -   unsigned int cpp = fb->format->cpp[0];
> >> > > > > > -   unsigned int src_offset;
> >> > > > > > -   unsigned int size_byte = width * cpp;
> >> > > > > > -   void *vaddr;
> >> > > > > >
> >> > > > > > -   mutex_lock(&vkms_obj->pages_lock);
> >> > > > > > -   vaddr = vkms_obj->vaddr;
> >> > > > > > -   if (WARN_ON(!vaddr))
> >> > > > > > -           goto out;
> >> > > > > > +   if (!vaddr_out) {
> >> > > > > > +           DRM_ERROR("Failed to allocate memory for output frame.");
> >> > > > > > +           return 0;
> >> > > > > > +   }
> >> > > > > >
> >> > > > > > -   for (i = y; i < y + height; i++) {
> >> > > > > > -           src_offset = fb->offsets[0] + (i * fb->pitches[0]) + (x * cpp);
> >> > > > > > -           crc = crc32_le(crc, vaddr + src_offset, size_byte);
> >> > > > > > +   mutex_lock(&vkms_obj->pages_lock);
> >> > > > > > +   if (WARN_ON(!vkms_obj->vaddr)) {
> >> > > > > > +           mutex_lock(&vkms_obj->pages_lock);
> >> > > > > > +           return crc;
> >> > > > > >     }
> >> > > > > >
> >> > > > > > -out:
> >> > > > > > +   memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
> >> > > > > >     mutex_unlock(&vkms_obj->pages_lock);
> >> > > > > > +
> >> > > > > > +   if (cursor_crc)
> >> > > > > > +           compose_cursor(cursor_crc, primary_crc, vaddr_out);
> >> > > > > > +
> >> > > > > > +   crc = compute_crc(vaddr_out, primary_crc);
> >> > > > > > +
> >> > > > > > +   kfree(vaddr_out);
> >> > > > > > +
> >> > > > > >     return crc;
> >> > > > > >  }
> >> > > > > >
> >> > > > > > @@ -44,8 +145,8 @@ void vkms_crc_work_handle(struct work_struct *work)
> >> > > > > >     struct vkms_device *vdev = container_of(out, struct vkms_device,
> >> > > > > >                                             output);
> >> > > > > >     struct vkms_crc_data *primary_crc = NULL;
> >> > > > > > +   struct vkms_crc_data *cursor_crc = NULL;
> >> > > > > >     struct drm_plane *plane;
> >> > > > > > -
> >> > > > > >     u32 crc32 = 0;
> >> > > > > >
> >> > > > > >     drm_for_each_plane(plane, &vdev->drm) {
> >> > > > > > @@ -58,14 +159,14 @@ void vkms_crc_work_handle(struct work_struct *work)
> >> > > > > >             if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
> >> > > > > >                     continue;
> >> > > > > >
> >> > > > > > -           if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> >> > > > > > +           if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> >> > > > > >                     primary_crc = crc_data;
> >> > > > > > -                   break;
> >> > > > > > -           }
> >> > > > > > +           else
> >> > > > > > +                   cursor_crc = crc_data;
> >> > > > > >     }
> >> > > > > >
> >> > > > > >     if (primary_crc)
> >> > > > > > -           crc32 = _vkms_get_crc(primary_crc);
> >> > > > > > +           crc32 = _vkms_get_crc(primary_crc, cursor_crc);
> >> > > > > >
> >> > > > > >     drm_crtc_add_crc_entry(crtc, true, crtc_state->n_frame, &crc32);
> >> > > > > >  }
> >> > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> >> > > > > > index 36e524f791fe..173875dc361e 100644
> >> > > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> >> > > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> >> > > > > > @@ -25,8 +25,11 @@ static const u32 vkms_cursor_formats[] = {
> >> > > > > >  };
> >> > > > > >
> >> > > > > >  struct vkms_crc_data {
> >> > > > > > -   struct drm_rect src;
> >> > > > > >     struct drm_framebuffer fb;
> >> > > > > > +   struct drm_rect src, dst;
> >> > > > > > +   unsigned int offset;
> >> > > > > > +   unsigned int pitch;
> >> > > > > > +   unsigned int cpp;
> >> > > > > >  };
> >> > > > > >
> >> > > > > >  /**
> >> > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> >> > > > > > index 428247d403dc..7041007396ae 100644
> >> > > > > > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> >> > > > > > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> >> > > > > > @@ -85,16 +85,22 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
> >> > > > > >                                  struct drm_plane_state *old_state)
> >> > > > > >  {
> >> > > > > >     struct vkms_plane_state *vkms_plane_state;
> >> > > > > > +   struct drm_framebuffer *fb = plane->state->fb;
> >> > > > > >     struct vkms_crc_data *crc_data;
> >> > > > > >
> >> > > > > > -   if (!plane->state->crtc || !plane->state->fb)
> >> > > > > > +   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->fb, plane->state->fb, sizeof(struct drm_framebuffer));
> >> > > > > > +   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];
> >> > > > > >  }
> >> > > > > >
> >> > > > > >  static int vkms_plane_atomic_check(struct drm_plane *plane,
> >> > > > > > --
> >> > > > > > 2.17.1
> >> > > > > >
> >> > > > >
> >> > > > > --
> >> > > > > Daniel Vetter
> >> > > > > Software Engineer, Intel Corporation
> >> > > > > http://blog.ffwll.ch
> >> > >
> >> > > --
> >> > > Daniel Vetter
> >> > > Software Engineer, Intel Corporation
> >> > > http://blog.ffwll.ch
> >>
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/vkms: Compute CRC with Cursor Plane

[Daniel Vetter]

On Thu, Aug 16, 2018 at 02:05:14AM +0300, Haneen Mohammed wrote:
> On Tue, Aug 14, 2018 at 09:52:33PM +0200, Daniel Vetter wrote:
> > On Tue, Aug 14, 2018 at 9:03 PM, Haneen Mohammed
> > <hamohammed.sa@gmail.com> wrote:
> > > On Tue, Aug 14, 2018 at 10:21:29AM +0200, Daniel Vetter wrote:
> > >> On Mon, Aug 13, 2018 at 11:04:11PM +0300, Haneen Mohammed wrote:
> > >> > On Wed, Aug 08, 2018 at 10:23:27AM +0200, Daniel Vetter wrote:
> > >> > > On Wed, Aug 08, 2018 at 06:53:17AM +0300, Haneen Mohammed wrote:
> > >> > > > On Tue, Aug 07, 2018 at 06:33:36PM +0200, Daniel Vetter wrote:
> > >> > > > > On Mon, Aug 06, 2018 at 06:58:29AM +0300, Haneen Mohammed wrote:
> > >> > > > > > This patch compute CRC for output frame with cursor and primary plane.
> > >> > > > > > Blend cursor with primary plane and compute CRC on the resulted frame.
> > >> > > > > >
> > >> > > > > > Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
> > >> > > > >
> > >> > > > > Nice!
> > >> > > > >
> > >> > > > > I assume with this you're passing all the crc based cursor tests in igt?
> > >> > > > > If so, please mention this in the commit message, so that there's a record
> > >> > > > > of the testing done on this.
> > >> > > > >
> > >> > > >
> > >> > > > Sure, I'll update the commit message.
> > >> > > > Is there any other change I should add/fix to this patchset?
> > >> > > >
> > >> > > > > One fairly huge gap we iirc have in our cursor testing is that there's not
> > >> > > > > much (if any?) alpha blending coverage. We kinda need that to make sure
> > >> > > > > this all works correctly. The usual trick is to only check the extreme
> > >> > > > > alpha values, i.e. fully opaque and fully transparent, since intermediate
> > >> > > > > values are affected by hw-specific rounding modes.
> > >> > > > > > ---
> > >> > > > > >  drivers/gpu/drm/vkms/vkms_crc.c   | 149 +++++++++++++++++++++++++-----
> > >> > > > > >  drivers/gpu/drm/vkms/vkms_drv.h   |   5 +-
> > >> > > > > >  drivers/gpu/drm/vkms/vkms_plane.c |  10 +-
> > >> > > > > >  3 files changed, 137 insertions(+), 27 deletions(-)
> > >> > > > > >
> > >> > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> > >> > > > > > index 37d717f38e3c..4853a739ae5a 100644
> > >> > > > > > --- a/drivers/gpu/drm/vkms/vkms_crc.c
> > >> > > > > > +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> > >> > > > > > @@ -1,36 +1,137 @@
> > >> > > > > >  // SPDX-License-Identifier: GPL-2.0
> > >> > > > > >  #include "vkms_drv.h"
> > >> > > > > >  #include <linux/crc32.h>
> > >> > > > > > +#include <drm/drm_atomic.h>
> > >> > > > > > +#include <drm/drm_atomic_helper.h>
> > >> > > > > >  #include <drm/drm_gem_framebuffer_helper.h>
> > >> > > > > >
> > >> > > > > > -static uint32_t _vkms_get_crc(struct vkms_crc_data *crc_data)
> > >> > > > > > +/**
> > >> > > > > > + * compute_crc - Compute CRC value on output frame
> > >> > > > > > + *
> > >> > > > > > + * @vaddr_out: address to final framebuffer
> > >> > > > > > + * @crc_out: 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)
> > >> > > > > >  {
> > >> > > > > > -   struct drm_framebuffer *fb = &crc_data->fb;
> > >> > > > > > +   int i, 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 size_byte = w_src * crc_out->cpp;
> > >> > > > > > +   u32 crc = 0;
> > >> > > > > > +
> > >> > > > > > +   for (i = y_src; i < y_src + h_src; i++) {
> > >> > > > > > +           src_offset = crc_out->offset
> > >> > > > > > +                        + (i * crc_out->pitch)
> > >> > > > > > +                        + (x_src * crc_out->cpp);
> > >> > > > > > +           crc = crc32_le(crc, vaddr_out + src_offset, size_byte);
> > >> > > > > > +   }
> > >> > > > > > +
> > >> > > > > > +   return crc;
> > >> > > > > > +}
> > >> > > > > > +
> > >> >
> > >> > Hey Daniel,
> > >> >
> > >> > When I change the above function "compute_crc" to compute the CRC for
> > >> > each pixel so we can clear the Alpha value before computing the CRC as bellow,
> > >> > the test "nonblocking-crc-pipe-A-frame-sequence" sometimes failes and
> > >> > sometimes passes. Should I still continue with the change, or leave it
> > >> > as it is?
> > >>
> > >> Hm, how does it fail? Could be that the code becomes too slow, and then
> > >> the frame counters don't increment nicely anymore. Or there's a race
> > >> somewhere in the CRC code that gets exposed. But that's just me guessing.
> > >> -Daniel
> > >>
> 
> Hi Daniel,
> 
> I figured out why nonblocking-crc-pipe-A-frame-sequence fails.
> queue_work sometimes return false so vkms_crc_work_handle doesn't get scheduled
> for a given frame number. I'm not sure though how this can be fixed.

Ah, I assumed that for each vblank we'd see a flip, and hence a new
crtc_state structure. But for the basic crc tests we just capture crcs for
the same frame all the time.

What we need is both a n_frame_start and n_frame_end instead just a single
n_frame. The vblank handler then needs to increment n_frame_end when it's
called (like it does now), and the vkms_crc_work_handle function needs to
have a loop to fill in crc for all frame numbers from n_frame_start to
n_frame_end (and then update n_frame_start ofc). A bit of trickery
required to get all the book-keeping in all cases correct.

The race condition around accessing crtc->state still exists, so that
needs to be fixed too.
-Daniel

> 
> - Haneen
> 
> > >
> > > Oh sorry I should've elaborated more on how it fails. Yeah I think it
> > > becomes too slow and that can cause mismatch to frame counters?
> > >
> > > -----
> > > (kms_pipe_crc_basic:1902) CRITICAL: Failed assertion: crcs[j].frame + 1 == crcs[j + 1].frame
> > > (kms_pipe_crc_basic:1902) CRITICAL: Last errno: 9, Bad file descriptor
> > > (kms_pipe_crc_basic:1902) CRITICAL: error: 1090928 != 1090929
> > > -----
> > 
> > Yeah misses the deadline it seems.
> > 
> > > Maybe it's not that bad/slow because I've noticed that this happen
> > > occasionally when I enable Ftrace.
> > 
> > Yeah ftrace can have some decent overhead. And we're adding a pile of
> > function calls, because crc32_le isn't inlined. Does it still happen
> > without ftrace?
> > 
> > Optimizing this will be some real work - we'd need a local cache to
> > compute an entire block of blended pixels into, then add an entire
> > block. Block size needs to be large enough to be efficient, but not so
> > big that we start trashing the cache or have to allocate memory (stack
> > space in the kernel is very limited). Would be good if we can avoid
> > all this work just now ...
> > -Danie
> > 
> > >
> > >> >
> > >> > ----------------- vkms_crc.c -----------------
> > >> >     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);
> > >> >                     memset(vaddr_out + src_offset + 24, 0,  8);
> > >> >                     crc = crc32_le(crc, vaddr_out + src_offset, sizeof(u32));
> > >> >             }
> > >> >     }
> > >> > ----------------- vkms_crc.c -----------------
> > >> >
> > >> > Thank you,
> > >> > Haneen
> > >> >
> > >> > > > > > +/**
> > >> > > > > > + * 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
> > >> > > > > > + *
> > >> > > > > > + * Blend value at vaddr_src with value at vaddr_dst.
> > >> > > > > > + * Currently, this function write value at vaddr_src on value
> > >> > > > > > + * at vaddr_dst using buffer's metadata to locate the new values
> > >> > > > > > + * from vaddr_src and their distenation at vaddr_dst.
> > >> > > > > > + *
> > >> > > > > > + * Todo: Use the alpha value to blend vaddr_src with vaddr_dst
> > >> > > > > > + *  instead of overwriting it.
> > >> > > > >
> > >> > > > > Another todo: We must clear the alpha channel in the result after
> > >> > > > > blending. This also applies to the primary plane, where the XRGB for the
> > >> > > > > pixel format mandates that we entirely ignore the alpha channel.
> > >> > > > >
> > >> > > > > This is also something we should probably have an igt testcase for.
> > >> > > > >
> > >> > > > > Since there's a few open ends: How many weeks are left in your outreachy
> > >> > > > > internship? We need to make sure that at least all the issues are covered
> > >> > > > > in a vkms todo file. Would be great to add that in
> > >> > > > > Documentation/gpu/vkms.rst, like we have for other drivers.
> > >> > > > > -Daniel
> > >> > > > >
> > >> > > >
> > >> > > > I've one more week! I can use this week to prepare the todo file and
> > >> > > > finalize this patch?
> > >> > >
> > >> > > Yeah sounds like the perfect wrap-up work. Since this wont be enough to
> > >> > > finish the cursor work it would be good to hide the cursor setup behind a
> > >> > > module option, perhaps "enable_cursor" or so. That way we wont have
> > >> > > not-totally-correct features enabled. And enabling/disabling cursor
> > >> > > support could be useful for testing.
> > >> > > -Daniel
> > >> > >
> > >> > > >
> > >> > > > Thank you,
> > >> > > > Haneen
> > >> > > >
> > >> > > > > > + */
> > >> > > > > > +static void blend(void *vaddr_dst, void *vaddr_src,
> > >> > > > > > +             struct vkms_crc_data *crc_dst,
> > >> > > > > > +             struct vkms_crc_data *crc_src)
> > >> > > > > > +{
> > >> > > > > > +   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_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 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);
> > >> > > > > > +
> > >> > > > > > +                   memcpy(vaddr_dst + offset_dst,
> > >> > > > > > +                          vaddr_src + offset_src, sizeof(u32));
> > >> > > > > > +           }
> > >> > > > > > +           i_dst++;
> > >> > > > > > +   }
> > >> > > > > > +}
> > >> > > > > > +
> > >> > > > > > +static void compose_cursor(struct vkms_crc_data *cursor_crc,
> > >> > > > > > +                      struct vkms_crc_data *primary_crc, 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_vkms_obj = drm_gem_to_vkms_gem(cursor_obj);
> > >> > > > > > +
> > >> > > > > > +   mutex_lock(&cursor_vkms_obj->pages_lock);
> > >> > > > > > +   if (WARN_ON(!cursor_vkms_obj->vaddr))
> > >> > > > > > +           goto out;
> > >> > > > > > +
> > >> > > > > > +   blend(vaddr_out, cursor_vkms_obj->vaddr, primary_crc, cursor_crc);
> > >> > > > > > +
> > >> > > > > > +out:
> > >> > > > > > +   mutex_unlock(&cursor_vkms_obj->pages_lock);
> > >> > > > > > +}
> > >> > > > > > +
> > >> > > > > > +static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
> > >> > > > > > +                         struct vkms_crc_data *cursor_crc)
> > >> > > > > > +{
> > >> > > > > > +   struct drm_framebuffer *fb = &primary_crc->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;
> > >> > > > > > -   int i = 0;
> > >> > > > > > -   unsigned int x = crc_data->src.x1 >> 16;
> > >> > > > > > -   unsigned int y = crc_data->src.y1 >> 16;
> > >> > > > > > -   unsigned int height = drm_rect_height(&crc_data->src) >> 16;
> > >> > > > > > -   unsigned int width = drm_rect_width(&crc_data->src) >> 16;
> > >> > > > > > -   unsigned int cpp = fb->format->cpp[0];
> > >> > > > > > -   unsigned int src_offset;
> > >> > > > > > -   unsigned int size_byte = width * cpp;
> > >> > > > > > -   void *vaddr;
> > >> > > > > >
> > >> > > > > > -   mutex_lock(&vkms_obj->pages_lock);
> > >> > > > > > -   vaddr = vkms_obj->vaddr;
> > >> > > > > > -   if (WARN_ON(!vaddr))
> > >> > > > > > -           goto out;
> > >> > > > > > +   if (!vaddr_out) {
> > >> > > > > > +           DRM_ERROR("Failed to allocate memory for output frame.");
> > >> > > > > > +           return 0;
> > >> > > > > > +   }
> > >> > > > > >
> > >> > > > > > -   for (i = y; i < y + height; i++) {
> > >> > > > > > -           src_offset = fb->offsets[0] + (i * fb->pitches[0]) + (x * cpp);
> > >> > > > > > -           crc = crc32_le(crc, vaddr + src_offset, size_byte);
> > >> > > > > > +   mutex_lock(&vkms_obj->pages_lock);
> > >> > > > > > +   if (WARN_ON(!vkms_obj->vaddr)) {
> > >> > > > > > +           mutex_lock(&vkms_obj->pages_lock);
> > >> > > > > > +           return crc;
> > >> > > > > >     }
> > >> > > > > >
> > >> > > > > > -out:
> > >> > > > > > +   memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size);
> > >> > > > > >     mutex_unlock(&vkms_obj->pages_lock);
> > >> > > > > > +
> > >> > > > > > +   if (cursor_crc)
> > >> > > > > > +           compose_cursor(cursor_crc, primary_crc, vaddr_out);
> > >> > > > > > +
> > >> > > > > > +   crc = compute_crc(vaddr_out, primary_crc);
> > >> > > > > > +
> > >> > > > > > +   kfree(vaddr_out);
> > >> > > > > > +
> > >> > > > > >     return crc;
> > >> > > > > >  }
> > >> > > > > >
> > >> > > > > > @@ -44,8 +145,8 @@ void vkms_crc_work_handle(struct work_struct *work)
> > >> > > > > >     struct vkms_device *vdev = container_of(out, struct vkms_device,
> > >> > > > > >                                             output);
> > >> > > > > >     struct vkms_crc_data *primary_crc = NULL;
> > >> > > > > > +   struct vkms_crc_data *cursor_crc = NULL;
> > >> > > > > >     struct drm_plane *plane;
> > >> > > > > > -
> > >> > > > > >     u32 crc32 = 0;
> > >> > > > > >
> > >> > > > > >     drm_for_each_plane(plane, &vdev->drm) {
> > >> > > > > > @@ -58,14 +159,14 @@ void vkms_crc_work_handle(struct work_struct *work)
> > >> > > > > >             if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
> > >> > > > > >                     continue;
> > >> > > > > >
> > >> > > > > > -           if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> > >> > > > > > +           if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > >> > > > > >                     primary_crc = crc_data;
> > >> > > > > > -                   break;
> > >> > > > > > -           }
> > >> > > > > > +           else
> > >> > > > > > +                   cursor_crc = crc_data;
> > >> > > > > >     }
> > >> > > > > >
> > >> > > > > >     if (primary_crc)
> > >> > > > > > -           crc32 = _vkms_get_crc(primary_crc);
> > >> > > > > > +           crc32 = _vkms_get_crc(primary_crc, cursor_crc);
> > >> > > > > >
> > >> > > > > >     drm_crtc_add_crc_entry(crtc, true, crtc_state->n_frame, &crc32);
> > >> > > > > >  }
> > >> > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > >> > > > > > index 36e524f791fe..173875dc361e 100644
> > >> > > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > >> > > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > >> > > > > > @@ -25,8 +25,11 @@ static const u32 vkms_cursor_formats[] = {
> > >> > > > > >  };
> > >> > > > > >
> > >> > > > > >  struct vkms_crc_data {
> > >> > > > > > -   struct drm_rect src;
> > >> > > > > >     struct drm_framebuffer fb;
> > >> > > > > > +   struct drm_rect src, dst;
> > >> > > > > > +   unsigned int offset;
> > >> > > > > > +   unsigned int pitch;
> > >> > > > > > +   unsigned int cpp;
> > >> > > > > >  };
> > >> > > > > >
> > >> > > > > >  /**
> > >> > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > >> > > > > > index 428247d403dc..7041007396ae 100644
> > >> > > > > > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > >> > > > > > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > >> > > > > > @@ -85,16 +85,22 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
> > >> > > > > >                                  struct drm_plane_state *old_state)
> > >> > > > > >  {
> > >> > > > > >     struct vkms_plane_state *vkms_plane_state;
> > >> > > > > > +   struct drm_framebuffer *fb = plane->state->fb;
> > >> > > > > >     struct vkms_crc_data *crc_data;
> > >> > > > > >
> > >> > > > > > -   if (!plane->state->crtc || !plane->state->fb)
> > >> > > > > > +   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->fb, plane->state->fb, sizeof(struct drm_framebuffer));
> > >> > > > > > +   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];
> > >> > > > > >  }
> > >> > > > > >
> > >> > > > > >  static int vkms_plane_atomic_check(struct drm_plane *plane,
> > >> > > > > > --
> > >> > > > > > 2.17.1
> > >> > > > > >
> > >> > > > >
> > >> > > > > --
> > >> > > > > Daniel Vetter
> > >> > > > > Software Engineer, Intel Corporation
> > >> > > > > http://blog.ffwll.ch
> > >> > >
> > >> > > --
> > >> > > Daniel Vetter
> > >> > > Software Engineer, Intel Corporation
> > >> > > http://blog.ffwll.ch
> > >>
> > >> --
> > >> Daniel Vetter
> > >> Software Engineer, Intel Corporation
> > >> http://blog.ffwll.ch
> > 
> > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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