drm/vkms: Add CRC support

This patch implement the necessary functions to compute and add CRCs entries:

- Implement the set_crc_source() callback.
- Compute CRC using crc32 on the visible part of the framebuffer.
- Use ordered workqueue per output to compute and add CRC at the end of a vblank.
- Use appropriate synchronization methods since the CRC computation must be atomic wrt the generated vblank event for a given atomic update, by using spinlock across atomic_begin/atomic_flush to wrap the event handling code completely and match the flip event with the CRC.

Since vkms_crc_work_handle() can sleep, spinlock can't be acquired while accessing vkms_output->primary_crc to compute CRC. To make sure the data is updated and released without conflict with the vkms_crc_work_handle(), the work_struct is flushed @crtc_destroy and the data is updated before scheduling the work handle again, as follow:

* CRC data update:
1- store vkms_crc_data {fb, src} per plane_state 2- @plane_duplicate_state -> allocate vkms_crc_data 3- during atomic commit (@atomic_update) ->

	a) copy {fb, src} to plane_state->crc_data
	b) get reference to fb,

3- @plane_destroy_state -> a) if (fb refcount) remove reference to fb
b) deallocate crc_data

* Atomic Commit:
1- vkms_plane_atomic_check
2- vkms_prepare_fb -> vmap vkms_gem_obj->vaddr
3- atomic_begin -> hold crc spinlock
4- atomic_plane_update -> a) update vkms_output->primary_crc
b) get reference to fb
5- atomic_flush -> a) send vblank event while holding event_lock
b) release crc spinlock

* hrtimer regular callback:
1- hold crc spinlock
2- drm_crtc_handle_vblank()
3- queue vkms_crc_work_handle
4- release crc spinlock

* cleanup:
1- @cleanup_fb ->vunmap vkms_gem_obj->vaddr
2- @crtc_destroy -> flush work struct
3- @plane_destroy -> a) if (fb refcount) remove reference to fb
b) deallocate crc_data

Signed-off-by Haneen Mohammed <[email protected]>

---
Changes in v2:
- remove vkms_crc_data for primary plane from vkms_output and access it from vkms_plane_state instead.
- flush crc_workq in vkms_set_crc_source before changing out->enabled value.

 drivers/gpu/drm/vkms/Makefile     |  2 +-
 drivers/gpu/drm/vkms/vkms_crc.c   | 95 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_crtc.c  | 56 +++++++++++++++---
 drivers/gpu/drm/vkms/vkms_drv.c   |  1 +
 drivers/gpu/drm/vkms/vkms_drv.h   | 21 +++++++
 drivers/gpu/drm/vkms/vkms_plane.c | 30 ++++++++++
 6 files changed, 196 insertions(+), 9 deletions(-)
 create mode 100644 drivers/gpu/drm/vkms/vkms_crc.c

drivers/gpu/drm/vkms/Makefile +1 -1

index 986297da51bf..37966914f70b 100644
@@ -1,3 +1,3 @@
-vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o
+vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
 
 obj-$(CONFIG_DRM_VKMS) += vkms.o

drivers/gpu/drm/vkms/vkms_crc.c new file +95 -0

new file mode 100644
index 000000000000..9b909221943f
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "vkms_drv.h"
+#include <linux/crc32.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+
+static uint32_t _vkms_get_crc(struct vkms_crc_data *crc_data)
+{
+	struct drm_framebuffer *fb = &crc_data->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);
+	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))
+		return crc;
+
+	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_unlock(&vkms_obj->pages_lock);
+
+	return crc;
+}
+
+void vkms_crc_work_handle(struct work_struct *work)
+{
+	struct vkms_crtc_state *crtc_state = container_of(work,
+						struct vkms_crtc_state,
+						crc_work);
+	struct drm_crtc *crtc = crtc_state->base.crtc;
+	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
+	struct vkms_device *vdev = container_of(out, struct vkms_device,
+						output);
+	struct vkms_crc_data *primary_crc = NULL;
+	struct drm_plane *plane;
+
+	u32 crc32 = 0;
+
+	drm_for_each_plane(plane, &vdev->drm) {
+		struct vkms_plane_state *vplane_state;
+		struct vkms_crc_data *crc_data;
+
+		vplane_state = to_vkms_plane_state(plane->state);
+		crc_data = vplane_state->crc_data;
+
+		if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
+			continue;
+
+		if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
+			primary_crc = crc_data;
+			break;
+		}
+	}
+
+	if (primary_crc)
+		crc32 = _vkms_get_crc(primary_crc);
+
+	drm_crtc_add_crc_entry(crtc, true, crtc_state->n_frame, &crc32);
+}
+
+int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
+			size_t *values_cnt)
+{
+	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
+	bool enabled = false;
+	unsigned long flags;
+	int ret = 0;
+
+	if (src_name && strcmp(src_name, "auto") == 0)
+		enabled = true;
+	else if (src_name)
+		ret = -EINVAL;
+
+	*values_cnt = 1;
+
+	/* make sure nothing is scheduled on crtc workq */
+	flush_workqueue(out->crc_workq);
+
+	spin_lock_irqsave(&out->lock, flags);
+	out->crc_enabled = enabled;
+	spin_unlock_irqrestore(&out->lock, flags);
+
+	return ret;
+}

drivers/gpu/drm/vkms/vkms_crtc.c +48 -8

index 26babb85ca77..a6c080d7f7ae 100644
@@ -10,18 +10,33 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 
-static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
+static void _vblank_handle(struct vkms_output *output)
 {
-	struct vkms_output *output = container_of(timer, struct vkms_output,
-						  vblank_hrtimer);
 	struct drm_crtc *crtc = &output->crtc;
-	int ret_overrun;
+	struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state);
 	bool ret;
 
+	spin_lock(&output->lock);
 	ret = drm_crtc_handle_vblank(crtc);
 	if (!ret)
 		DRM_ERROR("vkms failure on handling vblank");
 
+	if (state && output->crc_enabled) {
+		state->n_frame = drm_crtc_accurate_vblank_count(crtc);
+		queue_work(output->crc_workq, &state->crc_work);
+	}
+
+	spin_unlock(&output->lock);
+}
+
+static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
+{
+	struct vkms_output *output = container_of(timer, struct vkms_output,
+						  vblank_hrtimer);
+	int ret_overrun;
+
+	_vblank_handle(output);
+
 	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
 					  output->period_ns);
 

@@ -97,18 +112,22 @@ vkms_atomic_crtc_duplicate_state(struct drm_crtc *crtc)
 
 	__drm_atomic_helper_crtc_duplicate_state(crtc, &vkms_state->base);
 
+	INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
+
 	return &vkms_state->base;
 }
 
 static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
 					   struct drm_crtc_state *state)
 {
-	struct vkms_crtc_state *vkms_state;
-
-	vkms_state = to_vkms_crtc_state(state);
+	struct vkms_crtc_state *vkms_state = to_vkms_crtc_state(state);
 
 	__drm_atomic_helper_crtc_destroy_state(state);
-	kfree(vkms_state);
+
+	if (vkms_state) {
+		flush_work(&vkms_state->crc_work);
+		kfree(vkms_state);
+	}
 }
 
 static const struct drm_crtc_funcs vkms_crtc_funcs = {

@@ -120,6 +139,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
 	.atomic_destroy_state   = vkms_atomic_crtc_destroy_state,
 	.enable_vblank		= vkms_enable_vblank,
 	.disable_vblank		= vkms_disable_vblank,
+	.set_crc_source		= vkms_set_crc_source,
 };
 
 static void vkms_crtc_atomic_enable(struct drm_crtc *crtc,

@@ -134,9 +154,21 @@ static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,
 	drm_crtc_vblank_off(crtc);
 }
 
+static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
+				   struct drm_crtc_state *old_crtc_state)
+{
+	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
+
+	/* This lock is held across the atomic commit to block vblamk timer
+	 * from scheduling vkms_crc_work_handle until the crc_data is updated
+	 */
+	spin_lock_irq(&vkms_output->lock);
+}
+
 static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
 				   struct drm_crtc_state *old_crtc_state)
 {
+	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
 	unsigned long flags;
 
 	if (crtc->state->event) {

@@ -151,9 +183,12 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
 
 		crtc->state->event = NULL;
 	}
+
+	spin_unlock_irq(&vkms_output->lock);
 }
 
 static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
+	.atomic_begin	= vkms_crtc_atomic_begin,
 	.atomic_flush	= vkms_crtc_atomic_flush,
 	.atomic_enable	= vkms_crtc_atomic_enable,
 	.atomic_disable	= vkms_crtc_atomic_disable,

@@ -162,6 +197,7 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
 int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		   struct drm_plane *primary, struct drm_plane *cursor)
 {
+	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
 	int ret;
 
 	ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor,

@@ -173,5 +209,9 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 
 	drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
 
+	spin_lock_init(&vkms_out->lock);
+
+	vkms_out->crc_workq = alloc_ordered_workqueue("vkms_crc_workq", 0);
+
 	return ret;
 }

drivers/gpu/drm/vkms/vkms_drv.c +1 -0

index e340aa128b61..519a353cd909 100644
@@ -79,6 +79,7 @@ static void vkms_release(struct drm_device *dev)
 	drm_atomic_helper_shutdown(&vkms->drm);
 	drm_mode_config_cleanup(&vkms->drm);
 	drm_dev_fini(&vkms->drm);
+	destroy_workqueue(vkms->output.crc_workq);
 }
 
 static struct drm_driver vkms_driver = {

drivers/gpu/drm/vkms/vkms_drv.h +21 -0

index a1b206643c10..de1ddf31341b 100644
@@ -20,20 +20,31 @@ static const u32 vkms_formats[] = {
 	DRM_FORMAT_XRGB8888,
 };
 
+struct vkms_crc_data {
+	struct drm_rect src;
+	struct drm_framebuffer fb;
+};
+
 /**
  * vkms_plane_state - Driver specific plane state
  * @base: base plane state
+ * @crc_data: data required for CRC computation
  */
 struct vkms_plane_state {
 	struct drm_plane_state base;
+	struct vkms_crc_data *crc_data;
 };
 
 /**
  * vkms_crtc_state - Driver specific CRTC state
  * @base: base CRTC state
+ * @crc_work: work struct to compute and add CRC entries
+ * @n_frame: frame number for computed CRC
  */
 struct vkms_crtc_state {
 	struct drm_crtc_state base;
+	struct work_struct crc_work;
+	unsigned int n_frame;
 };
 
 struct vkms_output {

@@ -43,6 +54,11 @@ struct vkms_output {
 	struct hrtimer vblank_hrtimer;
 	ktime_t period_ns;
 	struct drm_pending_vblank_event *event;
+	bool crc_enabled;
+	/* ordered wq for crc_work */
+	struct workqueue_struct *crc_workq;
+	/* protects concurrent access to crc_data */
+	spinlock_t lock;
 };
 
 struct vkms_device {

@@ -106,4 +122,9 @@ int vkms_gem_vmap(struct drm_gem_object *obj);
 
 void vkms_gem_vunmap(struct drm_gem_object *obj);
 
+/* CRC Support */
+int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
+			size_t *values_cnt);
+void vkms_crc_work_handle(struct work_struct *work);
+
 #endif /* _VKMS_DRV_H_ */

drivers/gpu/drm/vkms/vkms_plane.c +30 -0

index 70fca22bc51b..c91661631c76 100644
@@ -16,11 +16,18 @@ static struct drm_plane_state *
 vkms_plane_duplicate_state(struct drm_plane *plane)
 {
 	struct vkms_plane_state *vkms_state;
+	struct vkms_crc_data *crc_data;
 
 	vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
 	if (!vkms_state)
 		return NULL;
 
+	crc_data = kzalloc(sizeof(*crc_data), GFP_KERNEL);
+	if (WARN_ON(!crc_data))
+		DRM_INFO("Couldn't allocate crc_data");
+
+	vkms_state->crc_data = crc_data;
+
 	__drm_atomic_helper_plane_duplicate_state(plane,
 						  &vkms_state->base);
 

@@ -31,6 +38,18 @@ static void vkms_plane_destroy_state(struct drm_plane *plane,
 				     struct drm_plane_state *old_state)
 {
 	struct vkms_plane_state *vkms_state = to_vkms_plane_state(old_state);
+	struct drm_crtc *crtc = vkms_state->base.crtc;
+
+	if (crtc) {
+		/* dropping the reference we acquired in
+		 * vkms_primary_plane_update()
+		 */
+		if (drm_framebuffer_read_refcount(&vkms_state->crc_data->fb))
+			drm_framebuffer_put(&vkms_state->crc_data->fb);
+	}
+
+	kfree(vkms_state->crc_data);
+	vkms_state->crc_data = NULL;
 
 	__drm_atomic_helper_plane_destroy_state(old_state);
 	kfree(vkms_state);

@@ -65,6 +84,17 @@ static const struct drm_plane_funcs vkms_plane_funcs = {
 static void vkms_primary_plane_update(struct drm_plane *plane,
 				      struct drm_plane_state *old_state)
 {
+	struct vkms_plane_state *vkms_plane_state;
+	struct vkms_crc_data *crc_data;
+
+	if (!plane->state->crtc || !plane->state->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));
+	drm_framebuffer_get(&crc_data->fb);
 }
 
 static int vkms_plane_atomic_check(struct drm_plane *plane,

Signature · 1 line

2.17.1

Hi Haneen,

Quoted 12 lines · Haneen Mohammed

This patch implement the necessary functions to compute and add CRCs entries:

- Implement the set_crc_source() callback.
- Compute CRC using crc32 on the visible part of the framebuffer.
- Use ordered workqueue per output to compute and add CRC at the end of a vblank.
- Use appropriate synchronization methods since the CRC computation must be atomic wrt the generated vblank event for a given atomic update, by using spinlock across atomic_begin/atomic_flush to wrap the event handling code completely and match the flip event with the CRC.

Quoted 6 lines

Since vkms_crc_work_handle() can sleep, spinlock can't be acquired while accessing vkms_output->primary_crc to compute CRC. To make sure the data is updated and released without conflict with the vkms_crc_work_handle(), the work_struct is flushed @crtc_destroy and the data is updated before scheduling the work handle again, as follow:

I'm not sure I get why this is a spinlock and not a mutex. With the later you are able to sleep. Also, your spinlock held through the whole atomic commit (from the begin to flush) which seems too much time for busy waiting. Or am I missing something here?

Gustavo

Quoted 421 lines

* CRC data update:
1- store vkms_crc_data {fb, src} per plane_state 2- @plane_duplicate_state -> allocate vkms_crc_data 3- during atomic commit (@atomic_update) ->

	a) copy {fb, src} to plane_state->crc_data
	b) get reference to fb,

3- @plane_destroy_state -> a) if (fb refcount) remove reference to fb
b) deallocate crc_data

* Atomic Commit:
1- vkms_plane_atomic_check
2- vkms_prepare_fb -> vmap vkms_gem_obj->vaddr
3- atomic_begin -> hold crc spinlock
4- atomic_plane_update -> a) update vkms_output->primary_crc
b) get reference to fb
5- atomic_flush -> a) send vblank event while holding event_lock
b) release crc spinlock

* hrtimer regular callback:
1- hold crc spinlock
2- drm_crtc_handle_vblank()
3- queue vkms_crc_work_handle
4- release crc spinlock

* cleanup:
1- @cleanup_fb ->vunmap vkms_gem_obj->vaddr
2- @crtc_destroy -> flush work struct
3- @plane_destroy -> a) if (fb refcount) remove reference to fb
b) deallocate crc_data

Signed-off-by Haneen Mohammed <[email protected]>

---
Changes in v2:
- remove vkms_crc_data for primary plane from vkms_output and access it from vkms_plane_state instead.
- flush crc_workq in vkms_set_crc_source before changing out->enabled value.

 drivers/gpu/drm/vkms/Makefile     |  2 +-
 drivers/gpu/drm/vkms/vkms_crc.c   | 95 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_crtc.c  | 56 +++++++++++++++---
 drivers/gpu/drm/vkms/vkms_drv.c   |  1 +
 drivers/gpu/drm/vkms/vkms_drv.h   | 21 +++++++
 drivers/gpu/drm/vkms/vkms_plane.c | 30 ++++++++++
 6 files changed, 196 insertions(+), 9 deletions(-)
 create mode 100644 drivers/gpu/drm/vkms/vkms_crc.c

drivers/gpu/drm/vkms/Makefile +1 -1

index 986297da51bf..37966914f70b 100644
@@ -1,3 +1,3 @@
-vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o
+vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
 
 obj-$(CONFIG_DRM_VKMS) += vkms.o

drivers/gpu/drm/vkms/vkms_crc.c new file +95 -0

new file mode 100644
index 000000000000..9b909221943f
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "vkms_drv.h"
+#include <linux/crc32.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+
+static uint32_t _vkms_get_crc(struct vkms_crc_data *crc_data)
+{
+	struct drm_framebuffer *fb = &crc_data->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);
+	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))
+		return crc;
+
+	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_unlock(&vkms_obj->pages_lock);
+
+	return crc;
+}
+
+void vkms_crc_work_handle(struct work_struct *work)
+{
+	struct vkms_crtc_state *crtc_state = container_of(work,
+						struct vkms_crtc_state,
+						crc_work);
+	struct drm_crtc *crtc = crtc_state->base.crtc;
+	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
+	struct vkms_device *vdev = container_of(out, struct vkms_device,
+						output);
+	struct vkms_crc_data *primary_crc = NULL;
+	struct drm_plane *plane;
+
+	u32 crc32 = 0;
+
+	drm_for_each_plane(plane, &vdev->drm) {
+		struct vkms_plane_state *vplane_state;
+		struct vkms_crc_data *crc_data;
+
+		vplane_state = to_vkms_plane_state(plane->state);
+		crc_data = vplane_state->crc_data;
+
+		if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
+			continue;
+
+		if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
+			primary_crc = crc_data;
+			break;
+		}
+	}
+
+	if (primary_crc)
+		crc32 = _vkms_get_crc(primary_crc);
+
+	drm_crtc_add_crc_entry(crtc, true, crtc_state->n_frame, &crc32);
+}
+
+int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
+			size_t *values_cnt)
+{
+	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
+	bool enabled = false;
+	unsigned long flags;
+	int ret = 0;
+
+	if (src_name && strcmp(src_name, "auto") == 0)
+		enabled = true;
+	else if (src_name)
+		ret = -EINVAL;
+
+	*values_cnt = 1;
+
+	/* make sure nothing is scheduled on crtc workq */
+	flush_workqueue(out->crc_workq);
+
+	spin_lock_irqsave(&out->lock, flags);
+	out->crc_enabled = enabled;
+	spin_unlock_irqrestore(&out->lock, flags);
+
+	return ret;
+}

drivers/gpu/drm/vkms/vkms_crtc.c +48 -8

index 26babb85ca77..a6c080d7f7ae 100644
@@ -10,18 +10,33 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 
-static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
+static void _vblank_handle(struct vkms_output *output)
 {
-	struct vkms_output *output = container_of(timer, struct vkms_output,
-						  vblank_hrtimer);
 	struct drm_crtc *crtc = &output->crtc;
-	int ret_overrun;
+	struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state);
 	bool ret;
 
+	spin_lock(&output->lock);
 	ret = drm_crtc_handle_vblank(crtc);
 	if (!ret)
 		DRM_ERROR("vkms failure on handling vblank");
 
+	if (state && output->crc_enabled) {
+		state->n_frame = drm_crtc_accurate_vblank_count(crtc);
+		queue_work(output->crc_workq, &state->crc_work);
+	}
+
+	spin_unlock(&output->lock);
+}
+
+static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
+{
+	struct vkms_output *output = container_of(timer, struct vkms_output,
+						  vblank_hrtimer);
+	int ret_overrun;
+
+	_vblank_handle(output);
+
 	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
 					  output->period_ns);
 

@@ -97,18 +112,22 @@ vkms_atomic_crtc_duplicate_state(struct drm_crtc *crtc)
 
 	__drm_atomic_helper_crtc_duplicate_state(crtc, &vkms_state->base);
 
+	INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
+
 	return &vkms_state->base;
 }
 
 static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
 					   struct drm_crtc_state *state)
 {
-	struct vkms_crtc_state *vkms_state;
-
-	vkms_state = to_vkms_crtc_state(state);
+	struct vkms_crtc_state *vkms_state = to_vkms_crtc_state(state);
 
 	__drm_atomic_helper_crtc_destroy_state(state);
-	kfree(vkms_state);
+
+	if (vkms_state) {
+		flush_work(&vkms_state->crc_work);
+		kfree(vkms_state);
+	}
 }
 
 static const struct drm_crtc_funcs vkms_crtc_funcs = {

@@ -120,6 +139,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
 	.atomic_destroy_state   = vkms_atomic_crtc_destroy_state,
 	.enable_vblank		= vkms_enable_vblank,
 	.disable_vblank		= vkms_disable_vblank,
+	.set_crc_source		= vkms_set_crc_source,
 };
 
 static void vkms_crtc_atomic_enable(struct drm_crtc *crtc,

@@ -134,9 +154,21 @@ static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,
 	drm_crtc_vblank_off(crtc);
 }
 
+static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
+				   struct drm_crtc_state *old_crtc_state)
+{
+	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
+
+	/* This lock is held across the atomic commit to block vblamk timer
+	 * from scheduling vkms_crc_work_handle until the crc_data is updated
+	 */
+	spin_lock_irq(&vkms_output->lock);
+}
+
 static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
 				   struct drm_crtc_state *old_crtc_state)
 {
+	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
 	unsigned long flags;
 
 	if (crtc->state->event) {

@@ -151,9 +183,12 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
 
 		crtc->state->event = NULL;
 	}
+
+	spin_unlock_irq(&vkms_output->lock);
 }
 
 static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
+	.atomic_begin	= vkms_crtc_atomic_begin,
 	.atomic_flush	= vkms_crtc_atomic_flush,
 	.atomic_enable	= vkms_crtc_atomic_enable,
 	.atomic_disable	= vkms_crtc_atomic_disable,

@@ -162,6 +197,7 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
 int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		   struct drm_plane *primary, struct drm_plane *cursor)
 {
+	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
 	int ret;
 
 	ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor,

@@ -173,5 +209,9 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 
 	drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
 
+	spin_lock_init(&vkms_out->lock);
+
+	vkms_out->crc_workq = alloc_ordered_workqueue("vkms_crc_workq", 0);
+
 	return ret;
 }

drivers/gpu/drm/vkms/vkms_drv.c +1 -0

index e340aa128b61..519a353cd909 100644
@@ -79,6 +79,7 @@ static void vkms_release(struct drm_device *dev)
 	drm_atomic_helper_shutdown(&vkms->drm);
 	drm_mode_config_cleanup(&vkms->drm);
 	drm_dev_fini(&vkms->drm);
+	destroy_workqueue(vkms->output.crc_workq);
 }
 
 static struct drm_driver vkms_driver = {

drivers/gpu/drm/vkms/vkms_drv.h +21 -0

index a1b206643c10..de1ddf31341b 100644
@@ -20,20 +20,31 @@ static const u32 vkms_formats[] = {
 	DRM_FORMAT_XRGB8888,
 };
 
+struct vkms_crc_data {
+	struct drm_rect src;
+	struct drm_framebuffer fb;
+};
+
 /**
  * vkms_plane_state - Driver specific plane state
  * @base: base plane state
+ * @crc_data: data required for CRC computation
  */
 struct vkms_plane_state {
 	struct drm_plane_state base;
+	struct vkms_crc_data *crc_data;
 };
 
 /**
  * vkms_crtc_state - Driver specific CRTC state
  * @base: base CRTC state
+ * @crc_work: work struct to compute and add CRC entries
+ * @n_frame: frame number for computed CRC
  */
 struct vkms_crtc_state {
 	struct drm_crtc_state base;
+	struct work_struct crc_work;
+	unsigned int n_frame;
 };
 
 struct vkms_output {

@@ -43,6 +54,11 @@ struct vkms_output {
 	struct hrtimer vblank_hrtimer;
 	ktime_t period_ns;
 	struct drm_pending_vblank_event *event;
+	bool crc_enabled;
+	/* ordered wq for crc_work */
+	struct workqueue_struct *crc_workq;
+	/* protects concurrent access to crc_data */
+	spinlock_t lock;
 };
 
 struct vkms_device {

@@ -106,4 +122,9 @@ int vkms_gem_vmap(struct drm_gem_object *obj);
 
 void vkms_gem_vunmap(struct drm_gem_object *obj);
 
+/* CRC Support */
+int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
+			size_t *values_cnt);
+void vkms_crc_work_handle(struct work_struct *work);
+
 #endif /* _VKMS_DRV_H_ */

drivers/gpu/drm/vkms/vkms_plane.c +30 -0

index 70fca22bc51b..c91661631c76 100644
@@ -16,11 +16,18 @@ static struct drm_plane_state *
 vkms_plane_duplicate_state(struct drm_plane *plane)
 {
 	struct vkms_plane_state *vkms_state;
+	struct vkms_crc_data *crc_data;
 
 	vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
 	if (!vkms_state)
 		return NULL;
 
+	crc_data = kzalloc(sizeof(*crc_data), GFP_KERNEL);
+	if (WARN_ON(!crc_data))
+		DRM_INFO("Couldn't allocate crc_data");
+
+	vkms_state->crc_data = crc_data;
+
 	__drm_atomic_helper_plane_duplicate_state(plane,
 						  &vkms_state->base);
 

@@ -31,6 +38,18 @@ static void vkms_plane_destroy_state(struct drm_plane *plane,
 				     struct drm_plane_state *old_state)
 {
 	struct vkms_plane_state *vkms_state = to_vkms_plane_state(old_state);
+	struct drm_crtc *crtc = vkms_state->base.crtc;
+
+	if (crtc) {
+		/* dropping the reference we acquired in
+		 * vkms_primary_plane_update()
+		 */
+		if (drm_framebuffer_read_refcount(&vkms_state->crc_data->fb))
+			drm_framebuffer_put(&vkms_state->crc_data->fb);
+	}
+
+	kfree(vkms_state->crc_data);
+	vkms_state->crc_data = NULL;
 
 	__drm_atomic_helper_plane_destroy_state(old_state);
 	kfree(vkms_state);

@@ -65,6 +84,17 @@ static const struct drm_plane_funcs vkms_plane_funcs = {
 static void vkms_primary_plane_update(struct drm_plane *plane,
 				      struct drm_plane_state *old_state)
 {
+	struct vkms_plane_state *vkms_plane_state;
+	struct vkms_crc_data *crc_data;
+
+	if (!plane->state->crtc || !plane->state->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));
+	drm_framebuffer_get(&crc_data->fb);
 }
 
 static int vkms_plane_atomic_check(struct drm_plane *plane,

Signature · 1 line

2.17.1

This patch implement the necessary functions to compute and add CRCs entries:

- Implement the set_crc_source() callback.
- Compute CRC using crc32 on the visible part of the framebuffer.
- Use ordered workqueue per output to compute and add CRC at the end of a vblank.
- Use appropriate synchronization methods since the CRC computation must be atomic wrt the generated vblank event for a given atomic update, by using spinlock across atomic_begin/atomic_flush to wrap the event handling code completely and match the flip event with the CRC.

Since vkms_crc_work_handle() can sleep, spinlock can't be acquired while accessing vkms_output->primary_crc to compute CRC. To make sure the data is updated and released without conflict with the vkms_crc_work_handle(), the work_struct is flushed @crtc_destroy and the data is updated before scheduling the work handle again, as follow:

* CRC data update:
1- store vkms_crc_data {fb, src} per plane_state 2- @plane_duplicate_state -> allocate vkms_crc_data 3- during atomic commit (@atomic_update) ->

	a) copy {fb, src} to plane_state->crc_data
	b) get reference to fb,

3- @plane_destroy_state -> a) if (fb refcount) remove reference to fb
b) deallocate crc_data

* Atomic Commit:
1- vkms_plane_atomic_check
2- vkms_prepare_fb -> vmap vkms_gem_obj->vaddr
3- atomic_begin -> hold crc spinlock
4- atomic_plane_update -> a) update vkms_output->primary_crc
b) get reference to fb
5- atomic_flush -> a) send vblank event while holding event_lock
b) release crc spinlock

* hrtimer regular callback:
1- hold crc spinlock
2- drm_crtc_handle_vblank()
3- queue vkms_crc_work_handle
4- release crc spinlock

* cleanup:
1- @cleanup_fb ->vunmap vkms_gem_obj->vaddr
2- @crtc_destroy -> flush work struct
3- @plane_destroy -> a) if (fb refcount) remove reference to fb
b) deallocate crc_data

Signed-off-by Haneen Mohammed <[email protected]>

---
Changes in v2:
- remove vkms_crc_data for primary plane from vkms_output and access it from vkms_plane_state instead.
- flush crc_workq in vkms_set_crc_source before changing out->enabled value.

 drivers/gpu/drm/vkms/Makefile     |  2 +-
 drivers/gpu/drm/vkms/vkms_crc.c   | 95 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_crtc.c  | 56 +++++++++++++++---
 drivers/gpu/drm/vkms/vkms_drv.c   |  1 +
 drivers/gpu/drm/vkms/vkms_drv.h   | 21 +++++++
 drivers/gpu/drm/vkms/vkms_plane.c | 30 ++++++++++
 6 files changed, 196 insertions(+), 9 deletions(-)
 create mode 100644 drivers/gpu/drm/vkms/vkms_crc.c

drivers/gpu/drm/vkms/Makefile +1 -1

index 986297da51bf..37966914f70b 100644
@@ -1,3 +1,3 @@
-vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o
+vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
 
 obj-$(CONFIG_DRM_VKMS) += vkms.o

drivers/gpu/drm/vkms/vkms_crc.c new file +95 -0

new file mode 100644
index 000000000000..9b909221943f
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "vkms_drv.h"
+#include <linux/crc32.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+
+static uint32_t _vkms_get_crc(struct vkms_crc_data *crc_data)
+{
+	struct drm_framebuffer *fb = &crc_data->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);
+	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))
+		return crc;
+
+	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_unlock(&vkms_obj->pages_lock);
+
+	return crc;
+}
+
+void vkms_crc_work_handle(struct work_struct *work)
+{
+	struct vkms_crtc_state *crtc_state = container_of(work,
+						struct vkms_crtc_state,
+						crc_work);
+	struct drm_crtc *crtc = crtc_state->base.crtc;
+	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
+	struct vkms_device *vdev = container_of(out, struct vkms_device,
+						output);
+	struct vkms_crc_data *primary_crc = NULL;
+	struct drm_plane *plane;
+
+	u32 crc32 = 0;
+
+	drm_for_each_plane(plane, &vdev->drm) {
+		struct vkms_plane_state *vplane_state;
+		struct vkms_crc_data *crc_data;
+
+		vplane_state = to_vkms_plane_state(plane->state);
+		crc_data = vplane_state->crc_data;
+
+		if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
+			continue;
+
+		if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
+			primary_crc = crc_data;
+			break;
+		}
+	}
+
+	if (primary_crc)
+		crc32 = _vkms_get_crc(primary_crc);
+
+	drm_crtc_add_crc_entry(crtc, true, crtc_state->n_frame, &crc32);
+}
+
+int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
+			size_t *values_cnt)
+{
+	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
+	bool enabled = false;
+	unsigned long flags;
+	int ret = 0;
+
+	if (src_name && strcmp(src_name, "auto") == 0)
+		enabled = true;
+	else if (src_name)
+		ret = -EINVAL;
+
+	*values_cnt = 1;
+
+	/* make sure nothing is scheduled on crtc workq */
+	flush_workqueue(out->crc_workq);
+
+	spin_lock_irqsave(&out->lock, flags);
+	out->crc_enabled = enabled;
+	spin_unlock_irqrestore(&out->lock, flags);
+
+	return ret;
+}

drivers/gpu/drm/vkms/vkms_crtc.c +48 -8

index 26babb85ca77..a6c080d7f7ae 100644
@@ -10,18 +10,33 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 
-static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
+static void _vblank_handle(struct vkms_output *output)
 {
-	struct vkms_output *output = container_of(timer, struct vkms_output,
-						  vblank_hrtimer);
 	struct drm_crtc *crtc = &output->crtc;
-	int ret_overrun;
+	struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state);
 	bool ret;
 
+	spin_lock(&output->lock);
 	ret = drm_crtc_handle_vblank(crtc);
 	if (!ret)
 		DRM_ERROR("vkms failure on handling vblank");
 
+	if (state && output->crc_enabled) {
+		state->n_frame = drm_crtc_accurate_vblank_count(crtc);
+		queue_work(output->crc_workq, &state->crc_work);
+	}
+
+	spin_unlock(&output->lock);
+}
+
+static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
+{
+	struct vkms_output *output = container_of(timer, struct vkms_output,
+						  vblank_hrtimer);
+	int ret_overrun;
+
+	_vblank_handle(output);
+
 	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
 					  output->period_ns);
 

@@ -97,18 +112,22 @@ vkms_atomic_crtc_duplicate_state(struct drm_crtc *crtc)
 
 	__drm_atomic_helper_crtc_duplicate_state(crtc, &vkms_state->base);
 
+	INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
+
 	return &vkms_state->base;
 }
 
 static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
 					   struct drm_crtc_state *state)
 {
-	struct vkms_crtc_state *vkms_state;
-
-	vkms_state = to_vkms_crtc_state(state);
+	struct vkms_crtc_state *vkms_state = to_vkms_crtc_state(state);
 
 	__drm_atomic_helper_crtc_destroy_state(state);
-	kfree(vkms_state);
+
+	if (vkms_state) {
+		flush_work(&vkms_state->crc_work);
+		kfree(vkms_state);
+	}
 }
 
 static const struct drm_crtc_funcs vkms_crtc_funcs = {

@@ -120,6 +139,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
 	.atomic_destroy_state   = vkms_atomic_crtc_destroy_state,
 	.enable_vblank		= vkms_enable_vblank,
 	.disable_vblank		= vkms_disable_vblank,
+	.set_crc_source		= vkms_set_crc_source,
 };
 
 static void vkms_crtc_atomic_enable(struct drm_crtc *crtc,

@@ -134,9 +154,21 @@ static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,
 	drm_crtc_vblank_off(crtc);
 }
 
+static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
+				   struct drm_crtc_state *old_crtc_state)
+{
+	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
+
+	/* This lock is held across the atomic commit to block vblamk timer
+	 * from scheduling vkms_crc_work_handle until the crc_data is updated
+	 */
+	spin_lock_irq(&vkms_output->lock);
+}
+
 static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
 				   struct drm_crtc_state *old_crtc_state)
 {
+	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
 	unsigned long flags;
 
 	if (crtc->state->event) {

@@ -151,9 +183,12 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
 
 		crtc->state->event = NULL;
 	}
+
+	spin_unlock_irq(&vkms_output->lock);
 }
 
 static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
+	.atomic_begin	= vkms_crtc_atomic_begin,
 	.atomic_flush	= vkms_crtc_atomic_flush,
 	.atomic_enable	= vkms_crtc_atomic_enable,
 	.atomic_disable	= vkms_crtc_atomic_disable,

@@ -162,6 +197,7 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
 int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		   struct drm_plane *primary, struct drm_plane *cursor)
 {
+	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
 	int ret;
 
 	ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor,

@@ -173,5 +209,9 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 
 	drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
 
+	spin_lock_init(&vkms_out->lock);
+
+	vkms_out->crc_workq = alloc_ordered_workqueue("vkms_crc_workq", 0);
+
 	return ret;
 }

drivers/gpu/drm/vkms/vkms_drv.c +1 -0

index e340aa128b61..519a353cd909 100644
@@ -79,6 +79,7 @@ static void vkms_release(struct drm_device *dev)
 	drm_atomic_helper_shutdown(&vkms->drm);
 	drm_mode_config_cleanup(&vkms->drm);
 	drm_dev_fini(&vkms->drm);
+	destroy_workqueue(vkms->output.crc_workq);
 }
 
 static struct drm_driver vkms_driver = {

drivers/gpu/drm/vkms/vkms_drv.h +21 -0

index a1b206643c10..de1ddf31341b 100644
@@ -20,20 +20,31 @@ static const u32 vkms_formats[] = {
 	DRM_FORMAT_XRGB8888,
 };
 
+struct vkms_crc_data {
+	struct drm_rect src;
+	struct drm_framebuffer fb;
+};
+
 /**
  * vkms_plane_state - Driver specific plane state
  * @base: base plane state
+ * @crc_data: data required for CRC computation
  */
 struct vkms_plane_state {
 	struct drm_plane_state base;
+	struct vkms_crc_data *crc_data;
 };
 
 /**
  * vkms_crtc_state - Driver specific CRTC state
  * @base: base CRTC state
+ * @crc_work: work struct to compute and add CRC entries
+ * @n_frame: frame number for computed CRC
  */
 struct vkms_crtc_state {
 	struct drm_crtc_state base;
+	struct work_struct crc_work;
+	unsigned int n_frame;
 };
 
 struct vkms_output {

@@ -43,6 +54,11 @@ struct vkms_output {
 	struct hrtimer vblank_hrtimer;
 	ktime_t period_ns;
 	struct drm_pending_vblank_event *event;
+	bool crc_enabled;
+	/* ordered wq for crc_work */
+	struct workqueue_struct *crc_workq;
+	/* protects concurrent access to crc_data */
+	spinlock_t lock;
 };
 
 struct vkms_device {

@@ -106,4 +122,9 @@ int vkms_gem_vmap(struct drm_gem_object *obj);
 
 void vkms_gem_vunmap(struct drm_gem_object *obj);
 
+/* CRC Support */
+int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
+			size_t *values_cnt);
+void vkms_crc_work_handle(struct work_struct *work);
+
 #endif /* _VKMS_DRV_H_ */

drivers/gpu/drm/vkms/vkms_plane.c +30 -0

index 70fca22bc51b..c91661631c76 100644
@@ -16,11 +16,18 @@ static struct drm_plane_state *
 vkms_plane_duplicate_state(struct drm_plane *plane)
 {
 	struct vkms_plane_state *vkms_state;
+	struct vkms_crc_data *crc_data;
 
 	vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
 	if (!vkms_state)
 		return NULL;
 
+	crc_data = kzalloc(sizeof(*crc_data), GFP_KERNEL);
+	if (WARN_ON(!crc_data))
+		DRM_INFO("Couldn't allocate crc_data");
+
+	vkms_state->crc_data = crc_data;
+
 	__drm_atomic_helper_plane_duplicate_state(plane,
 						  &vkms_state->base);
 

@@ -31,6 +38,18 @@ static void vkms_plane_destroy_state(struct drm_plane *plane,
 				     struct drm_plane_state *old_state)
 {
 	struct vkms_plane_state *vkms_state = to_vkms_plane_state(old_state);
+	struct drm_crtc *crtc = vkms_state->base.crtc;
+
+	if (crtc) {
+		/* dropping the reference we acquired in
+		 * vkms_primary_plane_update()
+		 */
+		if (drm_framebuffer_read_refcount(&vkms_state->crc_data->fb))
+			drm_framebuffer_put(&vkms_state->crc_data->fb);
+	}
+
+	kfree(vkms_state->crc_data);
+	vkms_state->crc_data = NULL;
 
 	__drm_atomic_helper_plane_destroy_state(old_state);
 	kfree(vkms_state);

@@ -65,6 +84,17 @@ static const struct drm_plane_funcs vkms_plane_funcs = {
 static void vkms_primary_plane_update(struct drm_plane *plane,
 				      struct drm_plane_state *old_state)
 {
+	struct vkms_plane_state *vkms_plane_state;
+	struct vkms_crc_data *crc_data;
+
+	if (!plane->state->crtc || !plane->state->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));
+	drm_framebuffer_get(&crc_data->fb);
 }
 
 static int vkms_plane_atomic_check(struct drm_plane *plane,

Signature · 1 line

2.17.1

Quoted 22 lines · Gustavo Padovan

Hi Haneen,

Quoted 12 lines · Haneen Mohammed

This patch implement the necessary functions to compute and add CRCs entries:

- Implement the set_crc_source() callback.
- Compute CRC using crc32 on the visible part of the framebuffer.
- Use ordered workqueue per output to compute and add CRC at the end of a vblank.
- Use appropriate synchronization methods since the CRC computation must be atomic wrt the generated vblank event for a given atomic update, by using spinlock across atomic_begin/atomic_flush to wrap the event handling code completely and match the flip event with the CRC.

Quoted 6 lines

Since vkms_crc_work_handle() can sleep, spinlock can't be acquired while accessing vkms_output->primary_crc to compute CRC. To make sure the data is updated and released without conflict with the vkms_crc_work_handle(), the work_struct is flushed @crtc_destroy and the data is updated before scheduling the work handle again, as follow:

Quoted 4 lines

I'm not sure I get why this is a spinlock and not a mutex. With the later you are able to sleep. Also, your spinlock held through the whole atomic commit (from the begin to flush) which seems too much time for busy waiting. Or am I missing something here?

I expressed some of the same concerns here:

https://lists.freedesktop.org/archives/dri-devel/2018-July/183584.html

The tl;dr is that the spinlock is required so we can acquire it in the vblank hrtimer. It is held across begin/flush to prevent sending vblank eventss until the crc has been fully calculated for the frame. More details in the thread, ofc.

Quoted 30 lines · Sean Paul

Quoted 22 lines · Gustavo Padovan

Hi Haneen,

Quoted 12 lines · Haneen Mohammed

This patch implement the necessary functions to compute and add CRCs entries:

- Implement the set_crc_source() callback.
- Compute CRC using crc32 on the visible part of the framebuffer.
- Use ordered workqueue per output to compute and add CRC at the end of a vblank.
- Use appropriate synchronization methods since the CRC computation must be atomic wrt the generated vblank event for a given atomic update, by using spinlock across atomic_begin/atomic_flush to wrap the event handling code completely and match the flip event with the CRC.

Quoted 6 lines

Since vkms_crc_work_handle() can sleep, spinlock can't be acquired while accessing vkms_output->primary_crc to compute CRC. To make sure the data is updated and released without conflict with the vkms_crc_work_handle(), the work_struct is flushed @crtc_destroy and the data is updated before scheduling the work handle again, as follow:

Quoted 4 lines

I'm not sure I get why this is a spinlock and not a mutex. With the later you are able to sleep. Also, your spinlock held through the whole atomic commit (from the begin to flush) which seems too much time for busy waiting. Or am I missing something here?

I expressed some of the same concerns here:

Quoted 6 lines

https://lists.freedesktop.org/archives/dri-devel/2018-July/183584.html

The tl;dr is that the spinlock is required so we can acquire it in the vblank hrtimer. It is held across begin/flush to prevent sending vblank eventss until the crc has been fully calculated for the frame. More details in the thread, ofc.

Sounds good to me!

Gustavo

Quoted 43 lines · Haneen Mohammed

This patch implement the necessary functions to compute and add CRCs entries:

- Implement the set_crc_source() callback.
- Compute CRC using crc32 on the visible part of the framebuffer.
- Use ordered workqueue per output to compute and add CRC at the end of a vblank.
- Use appropriate synchronization methods since the CRC computation must be atomic wrt the generated vblank event for a given atomic update, by using spinlock across atomic_begin/atomic_flush to wrap the event handling code completely and match the flip event with the CRC.

Since vkms_crc_work_handle() can sleep, spinlock can't be acquired while accessing vkms_output->primary_crc to compute CRC. To make sure the data is updated and released without conflict with the vkms_crc_work_handle(), the work_struct is flushed @crtc_destroy and the data is updated before scheduling the work handle again, as follow:

* CRC data update:
1- store vkms_crc_data {fb, src} per plane_state 2- @plane_duplicate_state -> allocate vkms_crc_data 3- during atomic commit (@atomic_update) ->

	a) copy {fb, src} to plane_state->crc_data
	b) get reference to fb,

3- @plane_destroy_state -> a) if (fb refcount) remove reference to fb
b) deallocate crc_data

* Atomic Commit:
1- vkms_plane_atomic_check
2- vkms_prepare_fb -> vmap vkms_gem_obj->vaddr
3- atomic_begin -> hold crc spinlock
4- atomic_plane_update -> a) update vkms_output->primary_crc
b) get reference to fb
5- atomic_flush -> a) send vblank event while holding event_lock
b) release crc spinlock

* hrtimer regular callback:
1- hold crc spinlock
2- drm_crtc_handle_vblank()
3- queue vkms_crc_work_handle
4- release crc spinlock

Quoted 7 lines

* cleanup:
1- @cleanup_fb ->vunmap vkms_gem_obj->vaddr
2- @crtc_destroy -> flush work struct
3- @plane_destroy -> a) if (fb refcount) remove reference to fb
b) deallocate crc_data

Signed-off-by Haneen Mohammed <[email protected]>

Ok, everything looks good to me (aside from the "vblamk" typo, which we can fixup on apply).

I'll wait for Gustavo to respond to the other thread and then we'll go from there.

Reviewed-by Sean Paul <[email protected]>

Thanks!

Sean

Quoted 385 lines

---
Changes in v2:
- remove vkms_crc_data for primary plane from vkms_output and access it from vkms_plane_state instead.
- flush crc_workq in vkms_set_crc_source before changing out->enabled value.

 drivers/gpu/drm/vkms/Makefile     |  2 +-
 drivers/gpu/drm/vkms/vkms_crc.c   | 95 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_crtc.c  | 56 +++++++++++++++---
 drivers/gpu/drm/vkms/vkms_drv.c   |  1 +
 drivers/gpu/drm/vkms/vkms_drv.h   | 21 +++++++
 drivers/gpu/drm/vkms/vkms_plane.c | 30 ++++++++++
 6 files changed, 196 insertions(+), 9 deletions(-)
 create mode 100644 drivers/gpu/drm/vkms/vkms_crc.c

drivers/gpu/drm/vkms/Makefile +1 -1

index 986297da51bf..37966914f70b 100644
@@ -1,3 +1,3 @@
-vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o
+vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
 
 obj-$(CONFIG_DRM_VKMS) += vkms.o

drivers/gpu/drm/vkms/vkms_crc.c new file +95 -0

new file mode 100644
index 000000000000..9b909221943f
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "vkms_drv.h"
+#include <linux/crc32.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+
+static uint32_t _vkms_get_crc(struct vkms_crc_data *crc_data)
+{
+	struct drm_framebuffer *fb = &crc_data->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);
+	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))
+		return crc;
+
+	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_unlock(&vkms_obj->pages_lock);
+
+	return crc;
+}
+
+void vkms_crc_work_handle(struct work_struct *work)
+{
+	struct vkms_crtc_state *crtc_state = container_of(work,
+						struct vkms_crtc_state,
+						crc_work);
+	struct drm_crtc *crtc = crtc_state->base.crtc;
+	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
+	struct vkms_device *vdev = container_of(out, struct vkms_device,
+						output);
+	struct vkms_crc_data *primary_crc = NULL;
+	struct drm_plane *plane;
+
+	u32 crc32 = 0;
+
+	drm_for_each_plane(plane, &vdev->drm) {
+		struct vkms_plane_state *vplane_state;
+		struct vkms_crc_data *crc_data;
+
+		vplane_state = to_vkms_plane_state(plane->state);
+		crc_data = vplane_state->crc_data;
+
+		if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
+			continue;
+
+		if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
+			primary_crc = crc_data;
+			break;
+		}
+	}
+
+	if (primary_crc)
+		crc32 = _vkms_get_crc(primary_crc);
+
+	drm_crtc_add_crc_entry(crtc, true, crtc_state->n_frame, &crc32);
+}
+
+int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
+			size_t *values_cnt)
+{
+	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
+	bool enabled = false;
+	unsigned long flags;
+	int ret = 0;
+
+	if (src_name && strcmp(src_name, "auto") == 0)
+		enabled = true;
+	else if (src_name)
+		ret = -EINVAL;
+
+	*values_cnt = 1;
+
+	/* make sure nothing is scheduled on crtc workq */
+	flush_workqueue(out->crc_workq);
+
+	spin_lock_irqsave(&out->lock, flags);
+	out->crc_enabled = enabled;
+	spin_unlock_irqrestore(&out->lock, flags);
+
+	return ret;
+}

drivers/gpu/drm/vkms/vkms_crtc.c +48 -8

index 26babb85ca77..a6c080d7f7ae 100644
@@ -10,18 +10,33 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 
-static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
+static void _vblank_handle(struct vkms_output *output)
 {
-	struct vkms_output *output = container_of(timer, struct vkms_output,
-						  vblank_hrtimer);
 	struct drm_crtc *crtc = &output->crtc;
-	int ret_overrun;
+	struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state);
 	bool ret;
 
+	spin_lock(&output->lock);
 	ret = drm_crtc_handle_vblank(crtc);
 	if (!ret)
 		DRM_ERROR("vkms failure on handling vblank");
 
+	if (state && output->crc_enabled) {
+		state->n_frame = drm_crtc_accurate_vblank_count(crtc);
+		queue_work(output->crc_workq, &state->crc_work);
+	}
+
+	spin_unlock(&output->lock);
+}
+
+static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
+{
+	struct vkms_output *output = container_of(timer, struct vkms_output,
+						  vblank_hrtimer);
+	int ret_overrun;
+
+	_vblank_handle(output);
+
 	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
 					  output->period_ns);
 

@@ -97,18 +112,22 @@ vkms_atomic_crtc_duplicate_state(struct drm_crtc *crtc)
 
 	__drm_atomic_helper_crtc_duplicate_state(crtc, &vkms_state->base);
 
+	INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
+
 	return &vkms_state->base;
 }
 
 static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
 					   struct drm_crtc_state *state)
 {
-	struct vkms_crtc_state *vkms_state;
-
-	vkms_state = to_vkms_crtc_state(state);
+	struct vkms_crtc_state *vkms_state = to_vkms_crtc_state(state);
 
 	__drm_atomic_helper_crtc_destroy_state(state);
-	kfree(vkms_state);
+
+	if (vkms_state) {
+		flush_work(&vkms_state->crc_work);
+		kfree(vkms_state);
+	}
 }
 
 static const struct drm_crtc_funcs vkms_crtc_funcs = {

@@ -120,6 +139,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
 	.atomic_destroy_state   = vkms_atomic_crtc_destroy_state,
 	.enable_vblank		= vkms_enable_vblank,
 	.disable_vblank		= vkms_disable_vblank,
+	.set_crc_source		= vkms_set_crc_source,
 };
 
 static void vkms_crtc_atomic_enable(struct drm_crtc *crtc,

@@ -134,9 +154,21 @@ static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,
 	drm_crtc_vblank_off(crtc);
 }
 
+static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
+				   struct drm_crtc_state *old_crtc_state)
+{
+	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
+
+	/* This lock is held across the atomic commit to block vblamk timer
+	 * from scheduling vkms_crc_work_handle until the crc_data is updated
+	 */
+	spin_lock_irq(&vkms_output->lock);
+}
+
 static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
 				   struct drm_crtc_state *old_crtc_state)
 {
+	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
 	unsigned long flags;
 
 	if (crtc->state->event) {

@@ -151,9 +183,12 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
 
 		crtc->state->event = NULL;
 	}
+
+	spin_unlock_irq(&vkms_output->lock);
 }
 
 static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
+	.atomic_begin	= vkms_crtc_atomic_begin,
 	.atomic_flush	= vkms_crtc_atomic_flush,
 	.atomic_enable	= vkms_crtc_atomic_enable,
 	.atomic_disable	= vkms_crtc_atomic_disable,

@@ -162,6 +197,7 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
 int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		   struct drm_plane *primary, struct drm_plane *cursor)
 {
+	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
 	int ret;
 
 	ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor,

@@ -173,5 +209,9 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 
 	drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
 
+	spin_lock_init(&vkms_out->lock);
+
+	vkms_out->crc_workq = alloc_ordered_workqueue("vkms_crc_workq", 0);
+
 	return ret;
 }

drivers/gpu/drm/vkms/vkms_drv.c +1 -0

index e340aa128b61..519a353cd909 100644
@@ -79,6 +79,7 @@ static void vkms_release(struct drm_device *dev)
 	drm_atomic_helper_shutdown(&vkms->drm);
 	drm_mode_config_cleanup(&vkms->drm);
 	drm_dev_fini(&vkms->drm);
+	destroy_workqueue(vkms->output.crc_workq);
 }
 
 static struct drm_driver vkms_driver = {

drivers/gpu/drm/vkms/vkms_drv.h +21 -0

index a1b206643c10..de1ddf31341b 100644
@@ -20,20 +20,31 @@ static const u32 vkms_formats[] = {
 	DRM_FORMAT_XRGB8888,
 };
 
+struct vkms_crc_data {
+	struct drm_rect src;
+	struct drm_framebuffer fb;
+};
+
 /**
  * vkms_plane_state - Driver specific plane state
  * @base: base plane state
+ * @crc_data: data required for CRC computation
  */
 struct vkms_plane_state {
 	struct drm_plane_state base;
+	struct vkms_crc_data *crc_data;
 };
 
 /**
  * vkms_crtc_state - Driver specific CRTC state
  * @base: base CRTC state
+ * @crc_work: work struct to compute and add CRC entries
+ * @n_frame: frame number for computed CRC
  */
 struct vkms_crtc_state {
 	struct drm_crtc_state base;
+	struct work_struct crc_work;
+	unsigned int n_frame;
 };
 
 struct vkms_output {

@@ -43,6 +54,11 @@ struct vkms_output {
 	struct hrtimer vblank_hrtimer;
 	ktime_t period_ns;
 	struct drm_pending_vblank_event *event;
+	bool crc_enabled;
+	/* ordered wq for crc_work */
+	struct workqueue_struct *crc_workq;
+	/* protects concurrent access to crc_data */
+	spinlock_t lock;
 };
 
 struct vkms_device {

@@ -106,4 +122,9 @@ int vkms_gem_vmap(struct drm_gem_object *obj);
 
 void vkms_gem_vunmap(struct drm_gem_object *obj);
 
+/* CRC Support */
+int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
+			size_t *values_cnt);
+void vkms_crc_work_handle(struct work_struct *work);
+
 #endif /* _VKMS_DRV_H_ */

drivers/gpu/drm/vkms/vkms_plane.c +30 -0

index 70fca22bc51b..c91661631c76 100644
@@ -16,11 +16,18 @@ static struct drm_plane_state *
 vkms_plane_duplicate_state(struct drm_plane *plane)
 {
 	struct vkms_plane_state *vkms_state;
+	struct vkms_crc_data *crc_data;
 
 	vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
 	if (!vkms_state)
 		return NULL;
 
+	crc_data = kzalloc(sizeof(*crc_data), GFP_KERNEL);
+	if (WARN_ON(!crc_data))
+		DRM_INFO("Couldn't allocate crc_data");
+
+	vkms_state->crc_data = crc_data;
+
 	__drm_atomic_helper_plane_duplicate_state(plane,
 						  &vkms_state->base);
 

@@ -31,6 +38,18 @@ static void vkms_plane_destroy_state(struct drm_plane *plane,
 				     struct drm_plane_state *old_state)
 {
 	struct vkms_plane_state *vkms_state = to_vkms_plane_state(old_state);
+	struct drm_crtc *crtc = vkms_state->base.crtc;
+
+	if (crtc) {
+		/* dropping the reference we acquired in
+		 * vkms_primary_plane_update()
+		 */
+		if (drm_framebuffer_read_refcount(&vkms_state->crc_data->fb))
+			drm_framebuffer_put(&vkms_state->crc_data->fb);
+	}
+
+	kfree(vkms_state->crc_data);
+	vkms_state->crc_data = NULL;
 
 	__drm_atomic_helper_plane_destroy_state(old_state);
 	kfree(vkms_state);

@@ -65,6 +84,17 @@ static const struct drm_plane_funcs vkms_plane_funcs = {
 static void vkms_primary_plane_update(struct drm_plane *plane,
 				      struct drm_plane_state *old_state)
 {
+	struct vkms_plane_state *vkms_plane_state;
+	struct vkms_crc_data *crc_data;
+
+	if (!plane->state->crtc || !plane->state->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));
+	drm_framebuffer_get(&crc_data->fb);
 }
 

Quoted 4 lines

drivers/gpu/drm/vkms/vkms_plane.c +0 -0

 static int vkms_plane_atomic_check(struct drm_plane *plane,

Signature · 1 line

2.17.1

Signature · 1 line

Sean Paul, Software Engineer, Google / Chromium OS

Quoted 60 lines · Sean Paul

Quoted 43 lines · Haneen Mohammed

This patch implement the necessary functions to compute and add CRCs entries:

- Implement the set_crc_source() callback.
- Compute CRC using crc32 on the visible part of the framebuffer.
- Use ordered workqueue per output to compute and add CRC at the end of a vblank.
- Use appropriate synchronization methods since the CRC computation must be atomic wrt the generated vblank event for a given atomic update, by using spinlock across atomic_begin/atomic_flush to wrap the event handling code completely and match the flip event with the CRC.

Since vkms_crc_work_handle() can sleep, spinlock can't be acquired while accessing vkms_output->primary_crc to compute CRC. To make sure the data is updated and released without conflict with the vkms_crc_work_handle(), the work_struct is flushed @crtc_destroy and the data is updated before scheduling the work handle again, as follow:

* CRC data update:
1- store vkms_crc_data {fb, src} per plane_state 2- @plane_duplicate_state -> allocate vkms_crc_data 3- during atomic commit (@atomic_update) ->

	a) copy {fb, src} to plane_state->crc_data
	b) get reference to fb,

3- @plane_destroy_state -> a) if (fb refcount) remove reference to fb
b) deallocate crc_data

* Atomic Commit:
1- vkms_plane_atomic_check
2- vkms_prepare_fb -> vmap vkms_gem_obj->vaddr
3- atomic_begin -> hold crc spinlock
4- atomic_plane_update -> a) update vkms_output->primary_crc
b) get reference to fb
5- atomic_flush -> a) send vblank event while holding event_lock
b) release crc spinlock

* hrtimer regular callback:
1- hold crc spinlock
2- drm_crtc_handle_vblank()
3- queue vkms_crc_work_handle
4- release crc spinlock

Quoted 7 lines

* cleanup:
1- @cleanup_fb ->vunmap vkms_gem_obj->vaddr
2- @crtc_destroy -> flush work struct
3- @plane_destroy -> a) if (fb refcount) remove reference to fb
b) deallocate crc_data

Signed-off-by Haneen Mohammed <[email protected]>

Ok, everything looks good to me (aside from the "vblamk" typo, which we can fixup on apply).

I'll wait for Gustavo to respond to the other thread and then we'll go from there.

Reviewed-by Sean Paul <[email protected]>

Quoted 4 lines

Thanks!

Sean

Thank you! I'm sorry I've failed to notice that I didnt release pages_lock when vkms_obj->vaddr is NULL in vkms_get_crc(). I'll send a patch to fix that immediately.

Quoted 393 lines

Quoted 390 lines

---
Changes in v2:
- remove vkms_crc_data for primary plane from vkms_output and access it from vkms_plane_state instead.
- flush crc_workq in vkms_set_crc_source before changing out->enabled value.

 drivers/gpu/drm/vkms/Makefile     |  2 +-
 drivers/gpu/drm/vkms/vkms_crc.c   | 95 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_crtc.c  | 56 +++++++++++++++---
 drivers/gpu/drm/vkms/vkms_drv.c   |  1 +
 drivers/gpu/drm/vkms/vkms_drv.h   | 21 +++++++
 drivers/gpu/drm/vkms/vkms_plane.c | 30 ++++++++++
 6 files changed, 196 insertions(+), 9 deletions(-)
 create mode 100644 drivers/gpu/drm/vkms/vkms_crc.c

drivers/gpu/drm/vkms/Makefile +1 -1

index 986297da51bf..37966914f70b 100644
@@ -1,3 +1,3 @@
-vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o
+vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
 
 obj-$(CONFIG_DRM_VKMS) += vkms.o

drivers/gpu/drm/vkms/vkms_crc.c new file +95 -0

new file mode 100644
index 000000000000..9b909221943f
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "vkms_drv.h"
+#include <linux/crc32.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+
+static uint32_t _vkms_get_crc(struct vkms_crc_data *crc_data)
+{
+	struct drm_framebuffer *fb = &crc_data->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);
+	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))
+		return crc;
+
+	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_unlock(&vkms_obj->pages_lock);
+
+	return crc;
+}
+
+void vkms_crc_work_handle(struct work_struct *work)
+{
+	struct vkms_crtc_state *crtc_state = container_of(work,
+						struct vkms_crtc_state,
+						crc_work);
+	struct drm_crtc *crtc = crtc_state->base.crtc;
+	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
+	struct vkms_device *vdev = container_of(out, struct vkms_device,
+						output);
+	struct vkms_crc_data *primary_crc = NULL;
+	struct drm_plane *plane;
+
+	u32 crc32 = 0;
+
+	drm_for_each_plane(plane, &vdev->drm) {
+		struct vkms_plane_state *vplane_state;
+		struct vkms_crc_data *crc_data;
+
+		vplane_state = to_vkms_plane_state(plane->state);
+		crc_data = vplane_state->crc_data;
+
+		if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
+			continue;
+
+		if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
+			primary_crc = crc_data;
+			break;
+		}
+	}
+
+	if (primary_crc)
+		crc32 = _vkms_get_crc(primary_crc);
+
+	drm_crtc_add_crc_entry(crtc, true, crtc_state->n_frame, &crc32);
+}
+
+int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
+			size_t *values_cnt)
+{
+	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
+	bool enabled = false;
+	unsigned long flags;
+	int ret = 0;
+
+	if (src_name && strcmp(src_name, "auto") == 0)
+		enabled = true;
+	else if (src_name)
+		ret = -EINVAL;
+
+	*values_cnt = 1;
+
+	/* make sure nothing is scheduled on crtc workq */
+	flush_workqueue(out->crc_workq);
+
+	spin_lock_irqsave(&out->lock, flags);
+	out->crc_enabled = enabled;
+	spin_unlock_irqrestore(&out->lock, flags);
+
+	return ret;
+}

drivers/gpu/drm/vkms/vkms_crtc.c +48 -8

index 26babb85ca77..a6c080d7f7ae 100644
@@ -10,18 +10,33 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 
-static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
+static void _vblank_handle(struct vkms_output *output)
 {
-	struct vkms_output *output = container_of(timer, struct vkms_output,
-						  vblank_hrtimer);
 	struct drm_crtc *crtc = &output->crtc;
-	int ret_overrun;
+	struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state);
 	bool ret;
 
+	spin_lock(&output->lock);
 	ret = drm_crtc_handle_vblank(crtc);
 	if (!ret)
 		DRM_ERROR("vkms failure on handling vblank");
 
+	if (state && output->crc_enabled) {
+		state->n_frame = drm_crtc_accurate_vblank_count(crtc);
+		queue_work(output->crc_workq, &state->crc_work);
+	}
+
+	spin_unlock(&output->lock);
+}
+
+static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
+{
+	struct vkms_output *output = container_of(timer, struct vkms_output,
+						  vblank_hrtimer);
+	int ret_overrun;
+
+	_vblank_handle(output);
+
 	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
 					  output->period_ns);
 

@@ -97,18 +112,22 @@ vkms_atomic_crtc_duplicate_state(struct drm_crtc *crtc)
 
 	__drm_atomic_helper_crtc_duplicate_state(crtc, &vkms_state->base);
 
+	INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
+
 	return &vkms_state->base;
 }
 
 static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
 					   struct drm_crtc_state *state)
 {
-	struct vkms_crtc_state *vkms_state;
-
-	vkms_state = to_vkms_crtc_state(state);
+	struct vkms_crtc_state *vkms_state = to_vkms_crtc_state(state);
 
 	__drm_atomic_helper_crtc_destroy_state(state);
-	kfree(vkms_state);
+
+	if (vkms_state) {
+		flush_work(&vkms_state->crc_work);
+		kfree(vkms_state);
+	}
 }
 
 static const struct drm_crtc_funcs vkms_crtc_funcs = {

@@ -120,6 +139,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
 	.atomic_destroy_state   = vkms_atomic_crtc_destroy_state,
 	.enable_vblank		= vkms_enable_vblank,
 	.disable_vblank		= vkms_disable_vblank,
+	.set_crc_source		= vkms_set_crc_source,
 };
 
 static void vkms_crtc_atomic_enable(struct drm_crtc *crtc,

@@ -134,9 +154,21 @@ static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,
 	drm_crtc_vblank_off(crtc);
 }
 
+static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
+				   struct drm_crtc_state *old_crtc_state)
+{
+	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
+
+	/* This lock is held across the atomic commit to block vblamk timer
+	 * from scheduling vkms_crc_work_handle until the crc_data is updated
+	 */
+	spin_lock_irq(&vkms_output->lock);
+}
+
 static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
 				   struct drm_crtc_state *old_crtc_state)
 {
+	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
 	unsigned long flags;
 
 	if (crtc->state->event) {

@@ -151,9 +183,12 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
 
 		crtc->state->event = NULL;
 	}
+
+	spin_unlock_irq(&vkms_output->lock);
 }
 
 static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
+	.atomic_begin	= vkms_crtc_atomic_begin,
 	.atomic_flush	= vkms_crtc_atomic_flush,
 	.atomic_enable	= vkms_crtc_atomic_enable,
 	.atomic_disable	= vkms_crtc_atomic_disable,

@@ -162,6 +197,7 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
 int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		   struct drm_plane *primary, struct drm_plane *cursor)
 {
+	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
 	int ret;
 
 	ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor,

@@ -173,5 +209,9 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 
 	drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
 
+	spin_lock_init(&vkms_out->lock);
+
+	vkms_out->crc_workq = alloc_ordered_workqueue("vkms_crc_workq", 0);
+
 	return ret;
 }

drivers/gpu/drm/vkms/vkms_drv.c +1 -0

index e340aa128b61..519a353cd909 100644
@@ -79,6 +79,7 @@ static void vkms_release(struct drm_device *dev)
 	drm_atomic_helper_shutdown(&vkms->drm);
 	drm_mode_config_cleanup(&vkms->drm);
 	drm_dev_fini(&vkms->drm);
+	destroy_workqueue(vkms->output.crc_workq);
 }
 
 static struct drm_driver vkms_driver = {

drivers/gpu/drm/vkms/vkms_drv.h +21 -0

index a1b206643c10..de1ddf31341b 100644
@@ -20,20 +20,31 @@ static const u32 vkms_formats[] = {
 	DRM_FORMAT_XRGB8888,
 };
 
+struct vkms_crc_data {
+	struct drm_rect src;
+	struct drm_framebuffer fb;
+};
+
 /**
  * vkms_plane_state - Driver specific plane state
  * @base: base plane state
+ * @crc_data: data required for CRC computation
  */
 struct vkms_plane_state {
 	struct drm_plane_state base;
+	struct vkms_crc_data *crc_data;
 };
 
 /**
  * vkms_crtc_state - Driver specific CRTC state
  * @base: base CRTC state
+ * @crc_work: work struct to compute and add CRC entries
+ * @n_frame: frame number for computed CRC
  */
 struct vkms_crtc_state {
 	struct drm_crtc_state base;
+	struct work_struct crc_work;
+	unsigned int n_frame;
 };
 
 struct vkms_output {

@@ -43,6 +54,11 @@ struct vkms_output {
 	struct hrtimer vblank_hrtimer;
 	ktime_t period_ns;
 	struct drm_pending_vblank_event *event;
+	bool crc_enabled;
+	/* ordered wq for crc_work */
+	struct workqueue_struct *crc_workq;
+	/* protects concurrent access to crc_data */
+	spinlock_t lock;
 };
 
 struct vkms_device {

@@ -106,4 +122,9 @@ int vkms_gem_vmap(struct drm_gem_object *obj);
 
 void vkms_gem_vunmap(struct drm_gem_object *obj);
 
+/* CRC Support */
+int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
+			size_t *values_cnt);
+void vkms_crc_work_handle(struct work_struct *work);
+
 #endif /* _VKMS_DRV_H_ */

drivers/gpu/drm/vkms/vkms_plane.c +30 -0

index 70fca22bc51b..c91661631c76 100644
@@ -16,11 +16,18 @@ static struct drm_plane_state *
 vkms_plane_duplicate_state(struct drm_plane *plane)
 {
 	struct vkms_plane_state *vkms_state;
+	struct vkms_crc_data *crc_data;
 
 	vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
 	if (!vkms_state)
 		return NULL;
 
+	crc_data = kzalloc(sizeof(*crc_data), GFP_KERNEL);
+	if (WARN_ON(!crc_data))
+		DRM_INFO("Couldn't allocate crc_data");
+
+	vkms_state->crc_data = crc_data;
+
 	__drm_atomic_helper_plane_duplicate_state(plane,
 						  &vkms_state->base);
 

@@ -31,6 +38,18 @@ static void vkms_plane_destroy_state(struct drm_plane *plane,
 				     struct drm_plane_state *old_state)
 {
 	struct vkms_plane_state *vkms_state = to_vkms_plane_state(old_state);
+	struct drm_crtc *crtc = vkms_state->base.crtc;
+
+	if (crtc) {
+		/* dropping the reference we acquired in
+		 * vkms_primary_plane_update()
+		 */
+		if (drm_framebuffer_read_refcount(&vkms_state->crc_data->fb))
+			drm_framebuffer_put(&vkms_state->crc_data->fb);
+	}
+
+	kfree(vkms_state->crc_data);
+	vkms_state->crc_data = NULL;
 
 	__drm_atomic_helper_plane_destroy_state(old_state);
 	kfree(vkms_state);

@@ -65,6 +84,17 @@ static const struct drm_plane_funcs vkms_plane_funcs = {
 static void vkms_primary_plane_update(struct drm_plane *plane,
 				      struct drm_plane_state *old_state)
 {
+	struct vkms_plane_state *vkms_plane_state;
+	struct vkms_crc_data *crc_data;
+
+	if (!plane->state->crtc || !plane->state->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));
+	drm_framebuffer_get(&crc_data->fb);
 }
 
 static int vkms_plane_atomic_check(struct drm_plane *plane,

Signature · 1 line

2.17.1

Signature · 1 line

Sean Paul, Software Engineer, Google / Chromium OS