Implement CRC and Add necessary infrastructure

This patch add the necessary functions to map/unmap GEM backing memory to the kernel's virtual address space.

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

---
Changes in v2:
- add vkms_gem_vunmap
- use vmap_count to guard against multiple prepare_fb calls on the same fb

 drivers/gpu/drm/vkms/vkms_drv.h |  9 ++++
 drivers/gpu/drm/vkms/vkms_gem.c | 73 ++++++++++++++++++++++++++++++++-
 2 files changed, 81 insertions(+), 1 deletion(-)

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

index 07be29f2dc44..855e1ea8bc35 100644
@@ -39,6 +39,8 @@ struct vkms_gem_object {
 	struct drm_gem_object gem;
 	struct mutex pages_lock; /* Page lock used in page fault handler */
 	struct page **pages;
+	unsigned int vmap_count;
+	void *vaddr;
 };
 
 #define drm_crtc_to_vkms_output(target) \

@@ -47,6 +49,9 @@ struct vkms_gem_object {
 #define drm_device_to_vkms_device(target) \
 	container_of(target, struct vkms_device, drm)
 
+#define drm_gem_to_vkms_gem(target)\
+	container_of(target, struct vkms_gem_object, gem)
+
 /* CRTC */
 int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		   struct drm_plane *primary, struct drm_plane *cursor);

@@ -75,4 +80,8 @@ int vkms_dumb_map(struct drm_file *file, struct drm_device *dev,
 
 void vkms_gem_free_object(struct drm_gem_object *obj);
 
+void *vkms_gem_vmap(struct drm_gem_object *obj);
+
+void vkms_gem_vunmap(struct drm_gem_object *obj);
+
 #endif /* _VKMS_DRV_H_ */

drivers/gpu/drm/vkms/vkms_gem.c +33 -1

index c7e38368602b..3f574e761b0f 100644
@@ -37,7 +37,9 @@ void vkms_gem_free_object(struct drm_gem_object *obj)
 	struct vkms_gem_object *gem = container_of(obj, struct vkms_gem_object,
 						   gem);
 
-	kvfree(gem->pages);
+	WARN_ON(gem->pages);
+	WARN_ON(gem->vaddr);
+
 	mutex_destroy(&gem->pages_lock);
 	drm_gem_object_release(obj);
 	kfree(gem);

@@ -177,3 +179,72 @@ int vkms_dumb_map(struct drm_file *file, struct drm_device *dev,
 
 	return ret;
 }
+
+static struct page **_get_pages(struct vkms_gem_object *vkms_obj)
+{
+	struct drm_gem_object *gem_obj = &vkms_obj->gem;
+
+	if (!vkms_obj->pages) {
+		struct page **pages = drm_gem_get_pages(gem_obj);
+
+		if (IS_ERR(pages))
+			return pages;
+
+		if (cmpxchg(&vkms_obj->pages, NULL, pages))
+			drm_gem_put_pages(gem_obj, pages, false, true);
+	}
+
+	return vkms_obj->pages;
+}
+
+void vkms_gem_vunmap(struct drm_gem_object *obj)
+{
+	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(obj);
+
+	mutex_lock(&vkms_obj->pages_lock);
+	WARN_ON(vkms_obj->vmap_count < 1);
+	vkms_obj->vmap_count--;
+
+	if (vkms_obj->vmap_count == 0) {
+		vunmap(vkms_obj->vaddr);
+		vkms_obj->vaddr = NULL;
+		drm_gem_put_pages(obj, vkms_obj->pages, false, true);

drivers/gpu/drm/vkms/vkms_gem.c +13 -0

+			ret = -ENOMEM;
+			goto fail;
+		}
+	}
+
+	mutex_unlock(&vkms_obj->pages_lock);
+	return vkms_obj->vaddr;
+
+fail:
+	vkms_obj->vmap_count--;
+	mutex_unlock(&vkms_obj->pages_lock);
+	return ERR_PTR(ret);
+}

Signature · 1 line

2.17.1

This patch map/unmap GEM backing memory to kernel address space in prepare/cleanup_fb respectively and cache the virtual address for later use.

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

---
Changes in v2:
- use vkms_gem_vunmap

 drivers/gpu/drm/vkms/vkms_plane.c | 36 ++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

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

index 9f75b1e2c1c4..877483984897 100644

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

 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_plane_helper.h>
 
 static const struct drm_plane_funcs vkms_plane_funcs = {
 	.update_plane		= drm_atomic_helper_update_plane,

@@ -24,8 +25,41 @@ static void vkms_primary_plane_update(struct drm_plane *plane,
 {
 }
 
+static int vkms_prepare_fb(struct drm_plane *plane,
+			   struct drm_plane_state *state)
+{
+	struct drm_gem_object *gem_obj;

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

+		DRM_ERROR("vmap failed\n");
+
+	return drm_gem_fb_prepare_fb(plane, state);
+}
+
+static void vkms_cleanup_fb(struct drm_plane *plane,
+			    struct drm_plane_state *old_state)
+{
+	struct drm_gem_object *gem_obj;
+
+	if (!old_state->fb)
+		return;
+
+	gem_obj = drm_gem_fb_get_obj(old_state->fb, 0);
+	vkms_gem_vunmap(gem_obj);
+}
+
 static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
 	.atomic_update		= vkms_primary_plane_update,
+	.prepare_fb		= vkms_prepare_fb,
+	.cleanup_fb		= vkms_cleanup_fb,
 };
 
 struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev)

Signature · 1 line

2.17.1

Call atomic_helper_check_plane_state to clip plane coordinates.

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

---
Changes in v2:
- check for plane_state->visible since we can't handle a disabled primary plane yet.

 drivers/gpu/drm/vkms/vkms_plane.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

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

index 877483984897..aaf7c35faed6 100644
@@ -7,6 +7,7 @@
  */
 
 #include "vkms_drv.h"
+#include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_plane_helper.h>

@@ -25,6 +26,33 @@ static void vkms_primary_plane_update(struct drm_plane *plane,
 {
 }
 
+static int vkms_plane_atomic_check(struct drm_plane *plane,
+				   struct drm_plane_state *state)
+{
+	struct drm_crtc_state *crtc_state;
+	int ret;
+
+	if (!state->fb | !state->crtc)
+		return 0;
+
+	crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc);
+	if (IS_ERR(crtc_state))
+		return PTR_ERR(crtc_state);
+
+	ret = drm_atomic_helper_check_plane_state(state, crtc_state,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  false, true);
+	if (ret != 0)
+		return ret;
+
+	/* for now primary plane must be visible and full screen */
+	if (!state->visible)
+		return -EINVAL;
+
+	return 0;
+}
+
 static int vkms_prepare_fb(struct drm_plane *plane,
 			   struct drm_plane_state *state)
 {

@@ -58,6 +86,7 @@ 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_check		= vkms_plane_atomic_check,
 	.prepare_fb		= vkms_prepare_fb,
 	.cleanup_fb		= vkms_cleanup_fb,
 };

Signature · 1 line

2.17.1

Subclass CRTC state struct to enable storing driver's private state. This patch only adds the base drm_crtc_state struct and the atomic functions that handle it.

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

Reviewed-by Daniel Vetter <[email protected]>

---
 drivers/gpu/drm/vkms/vkms_crtc.c | 53 ++++++++++++++++++++++++++++++--
 drivers/gpu/drm/vkms/vkms_drv.h  | 11 +++++++
 2 files changed, 61 insertions(+), 3 deletions(-)

drivers/gpu/drm/vkms/vkms_crtc.c +50 -3

index 875fca662ac0..26babb85ca77 100644
@@ -64,13 +64,60 @@ bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
 	return true;
 }
 
+static void vkms_atomic_crtc_reset(struct drm_crtc *crtc)
+{
+	struct vkms_crtc_state *vkms_state = NULL;
+
+	if (crtc->state) {
+		vkms_state = to_vkms_crtc_state(crtc->state);
+		__drm_atomic_helper_crtc_destroy_state(crtc->state);
+		kfree(vkms_state);
+		crtc->state = NULL;
+	}
+
+	vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
+	if (!vkms_state)
+		return;
+
+	crtc->state = &vkms_state->base;
+	crtc->state->crtc = crtc;
+}
+
+static struct drm_crtc_state *
+vkms_atomic_crtc_duplicate_state(struct drm_crtc *crtc)
+{
+	struct vkms_crtc_state *vkms_state;
+
+	if (WARN_ON(!crtc->state))
+		return NULL;
+
+	vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
+	if (!vkms_state)
+		return NULL;
+
+	__drm_atomic_helper_crtc_duplicate_state(crtc, &vkms_state->base);
+
+	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);
+
+	__drm_atomic_helper_crtc_destroy_state(state);
+	kfree(vkms_state);
+}
+
 static const struct drm_crtc_funcs vkms_crtc_funcs = {
 	.set_config             = drm_atomic_helper_set_config,
 	.destroy                = drm_crtc_cleanup,
 	.page_flip              = drm_atomic_helper_page_flip,
-	.reset                  = drm_atomic_helper_crtc_reset,
-	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
-	.atomic_destroy_state   = drm_atomic_helper_crtc_destroy_state,
+	.reset                  = vkms_atomic_crtc_reset,
+	.atomic_duplicate_state = vkms_atomic_crtc_duplicate_state,
+	.atomic_destroy_state   = vkms_atomic_crtc_destroy_state,
 	.enable_vblank		= vkms_enable_vblank,
 	.disable_vblank		= vkms_disable_vblank,
 };

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

index 855e1ea8bc35..bf811d0ec349 100644
@@ -20,6 +20,14 @@ static const u32 vkms_formats[] = {
 	DRM_FORMAT_XRGB8888,
 };
 
+/**
+ * vkms_crtc_state - Driver specific CRTC state
+ * @base: base CRTC state
+ */
+struct vkms_crtc_state {
+	struct drm_crtc_state base;
+};
+
 struct vkms_output {
 	struct drm_crtc crtc;
 	struct drm_encoder encoder;

@@ -52,6 +60,9 @@ struct vkms_gem_object {
 #define drm_gem_to_vkms_gem(target)\
 	container_of(target, struct vkms_gem_object, gem)
 
+#define to_vkms_crtc_state(target)\
+	container_of(target, struct vkms_crtc_state, base)
+
 /* CRTC */
 int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		   struct drm_plane *primary, struct drm_plane *cursor);

Signature · 1 line

2.17.1

Implement the set_crc_source() callback.
Compute CRC using crc32 on the visible part of the framebuffer. Use ordered workqueue 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,

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

---
Changes in v2:
- Include this patch in this patchset.

 drivers/gpu/drm/vkms/Makefile     |  2 +-
 drivers/gpu/drm/vkms/vkms_crtc.c  | 60 ++++++++++++++++++++++++++-----
 drivers/gpu/drm/vkms/vkms_drv.c   |  1 +
 drivers/gpu/drm/vkms/vkms_drv.h   | 21 +++++++++++
 drivers/gpu/drm/vkms/vkms_plane.c | 10 ++++++
 5 files changed, 85 insertions(+), 9 deletions(-)

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_crtc.c +1 -2

index 26babb85ca77..f3a674db33b8 100644
@@ -10,18 +10,36 @@
 #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,

drivers/gpu/drm/vkms/vkms_crtc.c +18 -0

+	if (state && 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,6 +115,8 @@ 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;
 }
 

@@ -104,11 +124,18 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
 					   struct drm_crtc_state *state)
 {

drivers/gpu/drm/vkms/vkms_crtc.c +5 -0

+		drm_framebuffer_put(&vkms_state->data.fb);
+		memset(&vkms_state->data, 0, sizeof(struct vkms_crc_data));
+		kfree(vkms_state);
+	}
 }
 
 static const struct drm_crtc_funcs vkms_crtc_funcs = {

@@ -120,6 +147,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,

drivers/gpu/drm/vkms/vkms_crtc.c +9 -1

 		if (drm_crtc_vblank_get(crtc) != 0)
 			drm_crtc_send_vblank_event(crtc, crtc->state->event);
 		else
 			drm_crtc_arm_vblank_event(crtc, crtc->state->event);
 
-		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
+		spin_unlock(&crtc->dev->event_lock);
 
 		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 +201,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 +213,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 37aa2ef33b21..5d78bd97e69c 100644
@@ -46,6 +46,7 @@ static void vkms_release(struct drm_device *dev)
 	platform_device_unregister(vkms->platform);
 	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 +11 -0

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

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

+	/* ordered wq for crc_work */
+	struct workqueue_struct *crc_workq;
+	/* protects concurrent access to crc_data */
+	spinlock_t lock;
 };
 
 struct vkms_device {

@@ -95,4 +111,9 @@ void *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 +10 -0

index aaf7c35faed6..1e0ee8ca8bd6 100644
@@ -24,6 +24,16 @@ 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_crtc_state *state;
+
+	if (!plane->state->crtc || !plane->state->fb)
+		return;
+
+	state = to_vkms_crtc_state(plane->state->crtc->state);
+	memcpy(&state->data.src, &plane->state->src, sizeof(struct drm_rect));
+	memcpy(&state->data.fb, plane->state->fb,
+	       sizeof(struct drm_framebuffer));
+	drm_framebuffer_get(&state->data.fb);
 }
 
 static int vkms_plane_atomic_check(struct drm_plane *plane,

Signature · 1 line

2.17.1

Quoted 4 lines · Haneen Mohammed

Implement the set_crc_source() callback.
Compute CRC using crc32 on the visible part of the framebuffer. Use ordered workqueue to compute and add CRC at the end of a vblank.

Quoted 4 lines

Use appropriate synchronization methods since the crc computation must be atomic wrt the generated vblank event for a given atomic update,

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

Hey Haneen,
Thanks for revising this patch set. Things are looking good across the series, just a few more comments :-)

Quoted 32 lines

---
Changes in v2:
- Include this patch in this patchset.

 drivers/gpu/drm/vkms/Makefile     |  2 +-
 drivers/gpu/drm/vkms/vkms_crtc.c  | 60 ++++++++++++++++++++++++++-----
 drivers/gpu/drm/vkms/vkms_drv.c   |  1 +
 drivers/gpu/drm/vkms/vkms_drv.h   | 21 +++++++++++
 drivers/gpu/drm/vkms/vkms_plane.c | 10 ++++++
 5 files changed, 85 insertions(+), 9 deletions(-)

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_crtc.c +1 -2

index 26babb85ca77..f3a674db33b8 100644
@@ -10,18 +10,36 @@
 #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,

drivers/gpu/drm/vkms/vkms_crtc.c line 22

Quoted 10 lines

drivers/gpu/drm/vkms/vkms_crtc.c +5 -2

-						  vblank_hrtimer);
 	struct drm_crtc *crtc = &output->crtc;
-	int ret_overrun;
+	struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state);
 	bool ret;
 
+	int crc_enabled = 0;
+
+	spin_lock(&output->lock);
+	crc_enabled = output->crc_enabled;

Aside from the implicit bool -> int cast, I don't think you need this local var, just use output->crc_enabled directly below.

drivers/gpu/drm/vkms/vkms_crtc.c line 25

Quoted 3 lines

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

 	ret = drm_crtc_handle_vblank(crtc);
 	if (!ret)
 		DRM_ERROR("vkms failure on handling vblank");

This would be more useful with the error code printed.

Quoted 37 lines

drivers/gpu/drm/vkms/vkms_crtc.c +19 -0

+	if (state && 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,6 +115,8 @@ 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;
 }
 

@@ -104,11 +124,18 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
 					   struct drm_crtc_state *state)
 {
 	struct vkms_crtc_state *vkms_state;
+	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
 
 	vkms_state = to_vkms_crtc_state(state);
 

drivers/gpu/drm/vkms/vkms_crtc.c static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc, · line 134

Quoted 5 lines

drivers/gpu/drm/vkms/vkms_crtc.c +3 -1

 	__drm_atomic_helper_crtc_destroy_state(state);
-	kfree(vkms_state);
+
+	if (vkms_state) {
+		flush_workqueue(vkms_out->crc_workq);

I'm a little worried about this bit. Since the workqueue is per-output, is it possible you'll be waiting for more frames to complete than you need to be?

Quoted 19 lines

drivers/gpu/drm/vkms/vkms_crtc.c +5 -0

+		drm_framebuffer_put(&vkms_state->data.fb);
+		memset(&vkms_state->data, 0, sizeof(struct vkms_crc_data));
+		kfree(vkms_state);
+	}
 }
 
 static const struct drm_crtc_funcs vkms_crtc_funcs = {

@@ -120,6 +147,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,26 +162,37 @@ static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,
 	drm_crtc_vblank_off(crtc);
 }
 

drivers/gpu/drm/vkms/vkms_crtc.c static void vkms_crtc_atomic_disable(struct drm_crtc *crtc, · line 170

Quoted 6 lines

drivers/gpu/drm/vkms/vkms_crtc.c +6 -0

+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);
+
+	spin_lock_irq(&vkms_output->lock);

Hmm, you can't lock across atomic_begin/flush. What is this protecting against?

Quoted 8 lines

drivers/gpu/drm/vkms/vkms_crtc.c +3 -1

+}
+
 static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
 				   struct drm_crtc_state *old_crtc_state)
 {
-	unsigned long flags;
+	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
 

drivers/gpu/drm/vkms/vkms_crtc.c static void vkms_crtc_atomic_disable(struct drm_crtc *crtc, · line 179

Quoted 3 lines

drivers/gpu/drm/vkms/vkms_crtc.c +1 -1

 	if (crtc->state->event) {
-		spin_lock_irqsave(&crtc->dev->event_lock, flags);
+		spin_lock(&crtc->dev->event_lock);

What's the rationale behind this change?

Quoted 77 lines

drivers/gpu/drm/vkms/vkms_crtc.c +9 -1

 		if (drm_crtc_vblank_get(crtc) != 0)
 			drm_crtc_send_vblank_event(crtc, crtc->state->event);
 		else
 			drm_crtc_arm_vblank_event(crtc, crtc->state->event);
 
-		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
+		spin_unlock(&crtc->dev->event_lock);
 
 		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 +201,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 +213,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 37aa2ef33b21..5d78bd97e69c 100644
@@ -46,6 +46,7 @@ static void vkms_release(struct drm_device *dev)
 	platform_device_unregister(vkms->platform);
 	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 +11 -0

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

drivers/gpu/drm/vkms/vkms_drv.h struct vkms_output { · line 49

Quoted 6 lines

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

 struct vkms_output {

@@ -35,6 +46,11 @@ struct vkms_output {
 	struct hrtimer vblank_hrtimer;
 	ktime_t period_ns;
 	struct drm_pending_vblank_event *event;
+	bool crc_enabled;

Where is this set to true?

Quoted 37 lines

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

+	/* ordered wq for crc_work */
+	struct workqueue_struct *crc_workq;
+	/* protects concurrent access to crc_data */
+	spinlock_t lock;
 };
 
 struct vkms_device {

@@ -95,4 +111,9 @@ void *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 +10 -0

index aaf7c35faed6..1e0ee8ca8bd6 100644
@@ -24,6 +24,16 @@ 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_crtc_state *state;
+
+	if (!plane->state->crtc || !plane->state->fb)
+		return;
+
+	state = to_vkms_crtc_state(plane->state->crtc->state);
+	memcpy(&state->data.src, &plane->state->src, sizeof(struct drm_rect));
+	memcpy(&state->data.fb, plane->state->fb,
+	       sizeof(struct drm_framebuffer));
+	drm_framebuffer_get(&state->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 5 lines · Sean Paul

Quoted 4 lines · Haneen Mohammed

Implement the set_crc_source() callback.
Compute CRC using crc32 on the visible part of the framebuffer. Use ordered workqueue to compute and add CRC at the end of a vblank.

Quoted 9 lines

Quoted 4 lines

Use appropriate synchronization methods since the crc computation must be atomic wrt the generated vblank event for a given atomic update,

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

Hey Haneen,
Thanks for revising this patch set. Things are looking good across the series, just a few more comments :-)

Thank you so much for the review!

Quoted 50 lines

Quoted 32 lines

---
Changes in v2:
- Include this patch in this patchset.

 drivers/gpu/drm/vkms/Makefile     |  2 +-
 drivers/gpu/drm/vkms/vkms_crtc.c  | 60 ++++++++++++++++++++++++++-----
 drivers/gpu/drm/vkms/vkms_drv.c   |  1 +
 drivers/gpu/drm/vkms/vkms_drv.h   | 21 +++++++++++
 drivers/gpu/drm/vkms/vkms_plane.c | 10 ++++++
 5 files changed, 85 insertions(+), 9 deletions(-)

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_crtc.c +1 -2

index 26babb85ca77..f3a674db33b8 100644
@@ -10,18 +10,36 @@
 #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,

drivers/gpu/drm/vkms/vkms_crtc.c line 22

Quoted 10 lines

drivers/gpu/drm/vkms/vkms_crtc.c +5 -2

-						  vblank_hrtimer);
 	struct drm_crtc *crtc = &output->crtc;
-	int ret_overrun;
+	struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state);
 	bool ret;
 
+	int crc_enabled = 0;
+
+	spin_lock(&output->lock);
+	crc_enabled = output->crc_enabled;

Aside from the implicit bool -> int cast, I don't think you need this local var, just use output->crc_enabled directly below.

Quoted 3 lines

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

 	ret = drm_crtc_handle_vblank(crtc);
 	if (!ret)
 		DRM_ERROR("vkms failure on handling vblank");

Quoted 2 lines

This would be more useful with the error code printed.

I think this only returns false on failure. Also I've noticed most of the usage of drm_crtc_handle_vblank don't check the return value, should I do the same as well and drop ret and error message?

Quoted 38 lines

Quoted 38 lines

drivers/gpu/drm/vkms/vkms_crtc.c +19 -0

+	if (state && 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,6 +115,8 @@ 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;
 }
 

@@ -104,11 +124,18 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
 					   struct drm_crtc_state *state)
 {
 	struct vkms_crtc_state *vkms_state;
+	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
 
 	vkms_state = to_vkms_crtc_state(state);
 

Quoted 9 lines

drivers/gpu/drm/vkms/vkms_crtc.c static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc, · line 134

Quoted 5 lines

drivers/gpu/drm/vkms/vkms_crtc.c +3 -1

 	__drm_atomic_helper_crtc_destroy_state(state);
-	kfree(vkms_state);
+
+	if (vkms_state) {
+		flush_workqueue(vkms_out->crc_workq);

I'm a little worried about this bit. Since the workqueue is per-output, is it possible you'll be waiting for more frames to complete than you need to be?

I see, maybe I should flush per work_struct instead?

Quoted 20 lines

Quoted 20 lines

drivers/gpu/drm/vkms/vkms_crtc.c +5 -0

+		drm_framebuffer_put(&vkms_state->data.fb);
+		memset(&vkms_state->data, 0, sizeof(struct vkms_crc_data));
+		kfree(vkms_state);
+	}
 }
 
 static const struct drm_crtc_funcs vkms_crtc_funcs = {

@@ -120,6 +147,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,26 +162,37 @@ static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,
 	drm_crtc_vblank_off(crtc);
 }
 

Quoted 9 lines

drivers/gpu/drm/vkms/vkms_crtc.c static void vkms_crtc_atomic_disable(struct drm_crtc *crtc, · line 170

Quoted 6 lines

drivers/gpu/drm/vkms/vkms_crtc.c +6 -0

+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);
+
+	spin_lock_irq(&vkms_output->lock);

Hmm, you can't lock across atomic_begin/flush. What is this protecting against?

I did this per Daniel recommendation:
1- spin_lock_irq -> vkms_crtc_atomic_begin
2- update vkms_crc_data and get ref to fb -> vkms_primary_plane_update 3- spin_unlock_irq after vblank event -> vkms_crtc_atomic_flush

Quoted 9 lines

Quoted 9 lines

drivers/gpu/drm/vkms/vkms_crtc.c +3 -1

+}
+
 static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
 				   struct drm_crtc_state *old_crtc_state)
 {
-	unsigned long flags;
+	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
 

Quoted 6 lines

drivers/gpu/drm/vkms/vkms_crtc.c static void vkms_crtc_atomic_disable(struct drm_crtc *crtc, · line 179

Quoted 3 lines

drivers/gpu/drm/vkms/vkms_crtc.c +1 -1

 	if (crtc->state->event) {
-		spin_lock_irqsave(&crtc->dev->event_lock, flags);
+		spin_lock(&crtc->dev->event_lock);

What's the rationale behind this change?

hm I am not sure if this is correct, but I assume because spin_lock_irq in atomic_begin would disable interrupts and since this is before we unlock vkms_output->lock with spin_unlock_irq so irqsave here is not necessary?

Quoted 77 lines

Quoted 77 lines

drivers/gpu/drm/vkms/vkms_crtc.c +9 -1

 		if (drm_crtc_vblank_get(crtc) != 0)
 			drm_crtc_send_vblank_event(crtc, crtc->state->event);
 		else
 			drm_crtc_arm_vblank_event(crtc, crtc->state->event);
 
-		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
+		spin_unlock(&crtc->dev->event_lock);
 
 		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 +201,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 +213,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 37aa2ef33b21..5d78bd97e69c 100644
@@ -46,6 +46,7 @@ static void vkms_release(struct drm_device *dev)
 	platform_device_unregister(vkms->platform);
 	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 +11 -0

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

Quoted 9 lines

drivers/gpu/drm/vkms/vkms_drv.h struct vkms_output { · line 49

Quoted 6 lines

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

 struct vkms_output {

@@ -35,6 +46,11 @@ struct vkms_output {
 	struct hrtimer vblank_hrtimer;
 	ktime_t period_ns;
 	struct drm_pending_vblank_event *event;
+	bool crc_enabled;

Where is this set to true?

Oh sorry I forgot to add a vkms_crc.c which sets crc_enabled in set_crc_source callback!

I'll work on fixing the issues you mentioned here and the other patches as well, thank you so much again!

Haneen

Quoted 44 lines

Quoted 41 lines

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

+	/* ordered wq for crc_work */
+	struct workqueue_struct *crc_workq;
+	/* protects concurrent access to crc_data */
+	spinlock_t lock;
 };
 
 struct vkms_device {

@@ -95,4 +111,9 @@ void *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 +10 -0

index aaf7c35faed6..1e0ee8ca8bd6 100644
@@ -24,6 +24,16 @@ 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_crtc_state *state;
+
+	if (!plane->state->crtc || !plane->state->fb)
+		return;
+
+	state = to_vkms_crtc_state(plane->state->crtc->state);
+	memcpy(&state->data.src, &plane->state->src, sizeof(struct drm_rect));
+	memcpy(&state->data.fb, plane->state->fb,
+	       sizeof(struct drm_framebuffer));
+	drm_framebuffer_get(&state->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

Quoted 68 lines · Haneen Mohammed

Quoted 5 lines · Sean Paul

Quoted 4 lines · Haneen Mohammed

Implement the set_crc_source() callback.
Compute CRC using crc32 on the visible part of the framebuffer. Use ordered workqueue to compute and add CRC at the end of a vblank.

Quoted 9 lines

Quoted 4 lines

Use appropriate synchronization methods since the crc computation must be atomic wrt the generated vblank event for a given atomic update,

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

Hey Haneen,
Thanks for revising this patch set. Things are looking good across the series, just a few more comments :-)

Thank you so much for the review!

Quoted 50 lines

Quoted 32 lines

---
Changes in v2:
- Include this patch in this patchset.

 drivers/gpu/drm/vkms/Makefile     |  2 +-
 drivers/gpu/drm/vkms/vkms_crtc.c  | 60 ++++++++++++++++++++++++++-----
 drivers/gpu/drm/vkms/vkms_drv.c   |  1 +
 drivers/gpu/drm/vkms/vkms_drv.h   | 21 +++++++++++
 drivers/gpu/drm/vkms/vkms_plane.c | 10 ++++++
 5 files changed, 85 insertions(+), 9 deletions(-)

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_crtc.c +1 -2

index 26babb85ca77..f3a674db33b8 100644
@@ -10,18 +10,36 @@
 #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,

drivers/gpu/drm/vkms/vkms_crtc.c line 22

Quoted 10 lines

drivers/gpu/drm/vkms/vkms_crtc.c +5 -2

-						  vblank_hrtimer);
 	struct drm_crtc *crtc = &output->crtc;
-	int ret_overrun;
+	struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state);
 	bool ret;
 
+	int crc_enabled = 0;
+
+	spin_lock(&output->lock);
+	crc_enabled = output->crc_enabled;

Aside from the implicit bool -> int cast, I don't think you need this local var, just use output->crc_enabled directly below.

Quoted 3 lines

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

 	ret = drm_crtc_handle_vblank(crtc);
 	if (!ret)
 		DRM_ERROR("vkms failure on handling vblank");

Quoted 6 lines

Quoted 2 lines

This would be more useful with the error code printed.

I think this only returns false on failure. Also I've noticed most of the usage of drm_crtc_handle_vblank don't check the return value, should I do the same as well and drop ret and error message?

Ahh, I didn't see that ret was bool. In that case, the error message is fine. It's always good practice to assume that if a function returns a status, it's worth checking.

Quoted 48 lines

Quoted 38 lines

Quoted 38 lines

drivers/gpu/drm/vkms/vkms_crtc.c +19 -0

+	if (state && 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,6 +115,8 @@ 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;
 }
 

@@ -104,11 +124,18 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
 					   struct drm_crtc_state *state)
 {
 	struct vkms_crtc_state *vkms_state;
+	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
 
 	vkms_state = to_vkms_crtc_state(state);
 

Quoted 9 lines

drivers/gpu/drm/vkms/vkms_crtc.c static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc, · line 134

Quoted 5 lines

drivers/gpu/drm/vkms/vkms_crtc.c +3 -1

 	__drm_atomic_helper_crtc_destroy_state(state);
-	kfree(vkms_state);
+
+	if (vkms_state) {
+		flush_workqueue(vkms_out->crc_workq);

I'm a little worried about this bit. Since the workqueue is per-output, is it possible you'll be waiting for more frames to complete than you need to be?

Quoted 1 line

I see, maybe I should flush per work_struct instead?

Yeah, that would make more sense IMO.

Quoted 30 lines

Quoted 20 lines

Quoted 20 lines

drivers/gpu/drm/vkms/vkms_crtc.c +5 -0

+		drm_framebuffer_put(&vkms_state->data.fb);
+		memset(&vkms_state->data, 0, sizeof(struct vkms_crc_data));
+		kfree(vkms_state);
+	}
 }
 
 static const struct drm_crtc_funcs vkms_crtc_funcs = {

@@ -120,6 +147,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,26 +162,37 @@ static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,
 	drm_crtc_vblank_off(crtc);
 }
 

Quoted 9 lines

drivers/gpu/drm/vkms/vkms_crtc.c static void vkms_crtc_atomic_disable(struct drm_crtc *crtc, · line 170

Quoted 6 lines

drivers/gpu/drm/vkms/vkms_crtc.c +6 -0

+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);
+
+	spin_lock_irq(&vkms_output->lock);

Hmm, you can't lock across atomic_begin/flush. What is this protecting against?

Quoted 4 lines

I did this per Daniel recommendation:
1- spin_lock_irq -> vkms_crtc_atomic_begin
2- update vkms_crc_data and get ref to fb -> vkms_primary_plane_update 3- spin_unlock_irq after vblank event -> vkms_crtc_atomic_flush

I think I'm interpreting his mail a little differently. The point of the spinlock is to protect the pointer to the crc_data in vkms_crtc (right now it's not a pointer, but it should be).

So I think you need to do the following:
- Change crc_data to a pointer
- In plane_update, allocate new memory to hold crc_data and, under the spinlock, update the crc_data to point to the new memory (add a WARN_ON if the pointer is NULL)
- In the hrtimer, while holding the spinlock, take the pointer from crc_data into a local variable and clear the crtc->crc_data pointer
- Pass the pointer (from the local variable) to the crc_worker

I don't think you need to hold the spinlock across the atomic hooks, just grab it in plane_update.

Sean

Quoted 16 lines

Quoted 9 lines

Quoted 9 lines

drivers/gpu/drm/vkms/vkms_crtc.c +3 -1

+}
+
 static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
 				   struct drm_crtc_state *old_crtc_state)
 {
-	unsigned long flags;
+	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
 

Quoted 6 lines

drivers/gpu/drm/vkms/vkms_crtc.c static void vkms_crtc_atomic_disable(struct drm_crtc *crtc, · line 179

Quoted 3 lines

drivers/gpu/drm/vkms/vkms_crtc.c +1 -1

 	if (crtc->state->event) {
-		spin_lock_irqsave(&crtc->dev->event_lock, flags);
+		spin_lock(&crtc->dev->event_lock);

What's the rationale behind this change?

Quoted 4 lines

hm I am not sure if this is correct, but I assume because spin_lock_irq in atomic_begin would disable interrupts and since this is before we unlock vkms_output->lock with spin_unlock_irq so irqsave here is not necessary?

Once you limit the scope of the spinlock above, you will want to revert this change.

Quoted 87 lines

Quoted 77 lines

Quoted 77 lines

drivers/gpu/drm/vkms/vkms_crtc.c +9 -1

 		if (drm_crtc_vblank_get(crtc) != 0)
 			drm_crtc_send_vblank_event(crtc, crtc->state->event);
 		else
 			drm_crtc_arm_vblank_event(crtc, crtc->state->event);
 
-		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
+		spin_unlock(&crtc->dev->event_lock);
 
 		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 +201,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 +213,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 37aa2ef33b21..5d78bd97e69c 100644
@@ -46,6 +46,7 @@ static void vkms_release(struct drm_device *dev)
 	platform_device_unregister(vkms->platform);
 	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 +11 -0

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

Quoted 9 lines

drivers/gpu/drm/vkms/vkms_drv.h struct vkms_output { · line 49

Quoted 6 lines

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

 struct vkms_output {

@@ -35,6 +46,11 @@ struct vkms_output {
 	struct hrtimer vblank_hrtimer;
 	ktime_t period_ns;
 	struct drm_pending_vblank_event *event;
+	bool crc_enabled;

Where is this set to true?

Quoted 5 lines

Oh sorry I forgot to add a vkms_crc.c which sets crc_enabled in set_crc_source callback!

I'll work on fixing the issues you mentioned here and the other patches as well, thank you so much again!

Thanks for working through this!

Sean

Quoted 46 lines

Haneen

Quoted 44 lines

Quoted 41 lines

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

+	/* ordered wq for crc_work */
+	struct workqueue_struct *crc_workq;
+	/* protects concurrent access to crc_data */
+	spinlock_t lock;
 };
 
 struct vkms_device {

@@ -95,4 +111,9 @@ void *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 +10 -0

index aaf7c35faed6..1e0ee8ca8bd6 100644
@@ -24,6 +24,16 @@ 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_crtc_state *state;
+
+	if (!plane->state->crtc || !plane->state->fb)
+		return;
+
+	state = to_vkms_crtc_state(plane->state->crtc->state);
+	memcpy(&state->data.src, &plane->state->src, sizeof(struct drm_rect));
+	memcpy(&state->data.fb, plane->state->fb,
+	       sizeof(struct drm_framebuffer));
+	drm_framebuffer_get(&state->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

Signature · 1 line

Sean Paul, Software Engineer, Google / Chromium OS

Quoted 171 lines · Sean Paul

Quoted 68 lines · Haneen Mohammed

Quoted 5 lines · Sean Paul

Quoted 4 lines · Haneen Mohammed

Implement the set_crc_source() callback.
Compute CRC using crc32 on the visible part of the framebuffer. Use ordered workqueue to compute and add CRC at the end of a vblank.

Quoted 9 lines

Quoted 4 lines

Use appropriate synchronization methods since the crc computation must be atomic wrt the generated vblank event for a given atomic update,

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

Hey Haneen,
Thanks for revising this patch set. Things are looking good across the series, just a few more comments :-)

Thank you so much for the review!

Quoted 50 lines

Quoted 32 lines

---
Changes in v2:
- Include this patch in this patchset.

 drivers/gpu/drm/vkms/Makefile     |  2 +-
 drivers/gpu/drm/vkms/vkms_crtc.c  | 60 ++++++++++++++++++++++++++-----
 drivers/gpu/drm/vkms/vkms_drv.c   |  1 +
 drivers/gpu/drm/vkms/vkms_drv.h   | 21 +++++++++++
 drivers/gpu/drm/vkms/vkms_plane.c | 10 ++++++
 5 files changed, 85 insertions(+), 9 deletions(-)

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_crtc.c +1 -2

index 26babb85ca77..f3a674db33b8 100644
@@ -10,18 +10,36 @@
 #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,

drivers/gpu/drm/vkms/vkms_crtc.c line 22

Quoted 10 lines

drivers/gpu/drm/vkms/vkms_crtc.c +5 -2

-						  vblank_hrtimer);
 	struct drm_crtc *crtc = &output->crtc;
-	int ret_overrun;
+	struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state);
 	bool ret;
 
+	int crc_enabled = 0;
+
+	spin_lock(&output->lock);
+	crc_enabled = output->crc_enabled;

Aside from the implicit bool -> int cast, I don't think you need this local var, just use output->crc_enabled directly below.

Quoted 3 lines

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

 	ret = drm_crtc_handle_vblank(crtc);
 	if (!ret)
 		DRM_ERROR("vkms failure on handling vblank");

Quoted 6 lines

Quoted 2 lines

This would be more useful with the error code printed.

I think this only returns false on failure. Also I've noticed most of the usage of drm_crtc_handle_vblank don't check the return value, should I do the same as well and drop ret and error message?

Ahh, I didn't see that ret was bool. In that case, the error message is fine. It's always good practice to assume that if a function returns a status, it's worth checking.

Quoted 48 lines

Quoted 38 lines

Quoted 38 lines

drivers/gpu/drm/vkms/vkms_crtc.c +19 -0

+	if (state && 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,6 +115,8 @@ 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;
 }
 

@@ -104,11 +124,18 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
 					   struct drm_crtc_state *state)
 {
 	struct vkms_crtc_state *vkms_state;
+	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
 
 	vkms_state = to_vkms_crtc_state(state);
 

Quoted 9 lines

drivers/gpu/drm/vkms/vkms_crtc.c static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc, · line 134

Quoted 5 lines

drivers/gpu/drm/vkms/vkms_crtc.c +3 -1

 	__drm_atomic_helper_crtc_destroy_state(state);
-	kfree(vkms_state);
+
+	if (vkms_state) {
+		flush_workqueue(vkms_out->crc_workq);

I'm a little worried about this bit. Since the workqueue is per-output, is it possible you'll be waiting for more frames to complete than you need to be?

Quoted 1 line

I see, maybe I should flush per work_struct instead?

Yeah, that would make more sense IMO.

Quoted 30 lines

Quoted 20 lines

Quoted 20 lines

drivers/gpu/drm/vkms/vkms_crtc.c +5 -0

+		drm_framebuffer_put(&vkms_state->data.fb);
+		memset(&vkms_state->data, 0, sizeof(struct vkms_crc_data));
+		kfree(vkms_state);
+	}
 }
 
 static const struct drm_crtc_funcs vkms_crtc_funcs = {

@@ -120,6 +147,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,26 +162,37 @@ static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,
 	drm_crtc_vblank_off(crtc);
 }
 

Quoted 9 lines

drivers/gpu/drm/vkms/vkms_crtc.c static void vkms_crtc_atomic_disable(struct drm_crtc *crtc, · line 170

Quoted 6 lines

drivers/gpu/drm/vkms/vkms_crtc.c +6 -0

+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);
+
+	spin_lock_irq(&vkms_output->lock);

Hmm, you can't lock across atomic_begin/flush. What is this protecting against?

Quoted 4 lines

I did this per Daniel recommendation:
1- spin_lock_irq -> vkms_crtc_atomic_begin
2- update vkms_crc_data and get ref to fb -> vkms_primary_plane_update 3- spin_unlock_irq after vblank event -> vkms_crtc_atomic_flush

I think I'm interpreting his mail a little differently. The point of the spinlock is to protect the pointer to the crc_data in vkms_crtc (right now it's not a pointer, but it should be).

Quoted 9 lines

So I think you need to do the following:
- Change crc_data to a pointer
- In plane_update, allocate new memory to hold crc_data and, under the spinlock, update the crc_data to point to the new memory (add a WARN_ON if the pointer is NULL)
- In the hrtimer, while holding the spinlock, take the pointer from crc_data into a local variable and clear the crtc->crc_data pointer
- Pass the pointer (from the local variable) to the crc_worker

Quoted 2 lines

I don't think you need to hold the spinlock across the atomic hooks, just grab it in plane_update.

So the more I think about this, I don't think it's quite right. Perhaps I'm missing an email from Daniel, or (more likely) misunderstanding what you're trying to do. However, the email I read suggested storing the crc_data to be consumed by the crc worker in vkms_crtc. However in this patch it's being stored (along with the work item) in crtc state. Further, the state cannot be destroyed without completing the crc_work, which kind of defeats the purpose of the workqueue in the first place.

Storing a copy of the crc_data in crtc allows you to free the crtc_state, so you can start working on the next frame while the crc worker is scheduled and picks up the crc_data from crtc. Storing the crc_data in crtc_state and waiting for the work to finish is easier since you don't need to worry about copying and locking (so much). However, it seems like we're doing both in this patch.

Again, I'm likely just not understanding what the goal of this is, so any clarification would be greatly appreciated :)

Sean

Quoted 122 lines

Sean

Quoted 16 lines

Quoted 9 lines

Quoted 9 lines

drivers/gpu/drm/vkms/vkms_crtc.c +3 -1

+}
+
 static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
 				   struct drm_crtc_state *old_crtc_state)
 {
-	unsigned long flags;
+	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
 

Quoted 6 lines

drivers/gpu/drm/vkms/vkms_crtc.c static void vkms_crtc_atomic_disable(struct drm_crtc *crtc, · line 179

Quoted 3 lines

drivers/gpu/drm/vkms/vkms_crtc.c +1 -1

 	if (crtc->state->event) {
-		spin_lock_irqsave(&crtc->dev->event_lock, flags);
+		spin_lock(&crtc->dev->event_lock);

What's the rationale behind this change?

Quoted 4 lines

hm I am not sure if this is correct, but I assume because spin_lock_irq in atomic_begin would disable interrupts and since this is before we unlock vkms_output->lock with spin_unlock_irq so irqsave here is not necessary?

Once you limit the scope of the spinlock above, you will want to revert this change.

Quoted 87 lines

Quoted 77 lines

Quoted 77 lines

drivers/gpu/drm/vkms/vkms_crtc.c +9 -1

 		if (drm_crtc_vblank_get(crtc) != 0)
 			drm_crtc_send_vblank_event(crtc, crtc->state->event);
 		else
 			drm_crtc_arm_vblank_event(crtc, crtc->state->event);
 
-		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
+		spin_unlock(&crtc->dev->event_lock);
 
 		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 +201,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 +213,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 37aa2ef33b21..5d78bd97e69c 100644
@@ -46,6 +46,7 @@ static void vkms_release(struct drm_device *dev)
 	platform_device_unregister(vkms->platform);
 	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 +11 -0

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

Quoted 9 lines

drivers/gpu/drm/vkms/vkms_drv.h struct vkms_output { · line 49

Quoted 6 lines

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

 struct vkms_output {

@@ -35,6 +46,11 @@ struct vkms_output {
 	struct hrtimer vblank_hrtimer;
 	ktime_t period_ns;
 	struct drm_pending_vblank_event *event;
+	bool crc_enabled;

Where is this set to true?

Quoted 5 lines

Oh sorry I forgot to add a vkms_crc.c which sets crc_enabled in set_crc_source callback!

I'll work on fixing the issues you mentioned here and the other patches as well, thank you so much again!

Thanks for working through this!

Quoted 49 lines

Sean

Quoted 46 lines

Haneen

Quoted 44 lines

Quoted 41 lines

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

+	/* ordered wq for crc_work */
+	struct workqueue_struct *crc_workq;
+	/* protects concurrent access to crc_data */
+	spinlock_t lock;
 };
 
 struct vkms_device {

@@ -95,4 +111,9 @@ void *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 +10 -0

index aaf7c35faed6..1e0ee8ca8bd6 100644
@@ -24,6 +24,16 @@ 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_crtc_state *state;
+
+	if (!plane->state->crtc || !plane->state->fb)
+		return;
+
+	state = to_vkms_crtc_state(plane->state->crtc->state);
+	memcpy(&state->data.src, &plane->state->src, sizeof(struct drm_rect));
+	memcpy(&state->data.fb, plane->state->fb,
+	       sizeof(struct drm_framebuffer));
+	drm_framebuffer_get(&state->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

Quoted 2 lines

Signature · 1 line

Sean Paul, Software Engineer, Google / Chromium OS

Signature · 1 line

Sean Paul, Software Engineer, Google / Chromium OS

First, just so you have the complete view, this is the missing part in this patch:
------ vkms_crc.c
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 = 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);
	}

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

	if (crtc_state && out->crc_enabled) {
		u32 crc32 = _vkms_get_crc(&crtc_state->data);
		unsigned int n_frame = crtc_state->n_frame;

		drm_crtc_add_crc_entry(crtc, true, 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);

	if (!src_name) {
		out->crc_enabled = false;
	} else if (strcmp(src_name, "auto") == 0) {
		out->crc_enabled = true;
	} else {
		out->crc_enabled = false;
		return -EINVAL;
	}

	*values_cnt = 1;

	return 0;
}

--- end vkms_crc.c

Quoted 19 lines

Quoted 19 lines

Quoted 10 lines

Quoted 9 lines

drivers/gpu/drm/vkms/vkms_crtc.c static void vkms_crtc_atomic_disable(struct drm_crtc *crtc, · line 170

Quoted 6 lines

drivers/gpu/drm/vkms/vkms_crtc.c +6 -0

+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);
+
+	spin_lock_irq(&vkms_output->lock);

Hmm, you can't lock across atomic_begin/flush. What is this protecting against?

Quoted 4 lines

I did this per Daniel recommendation:
1- spin_lock_irq -> vkms_crtc_atomic_begin
2- update vkms_crc_data and get ref to fb -> vkms_primary_plane_update 3- spin_unlock_irq after vblank event -> vkms_crtc_atomic_flush

I think I'm interpreting his mail a little differently. The point of the spinlock is to protect the pointer to the crc_data in vkms_crtc (right now it's not a pointer, but it should be).

Quoted 9 lines

Quoted 9 lines

So I think you need to do the following:
- Change crc_data to a pointer
- In plane_update, allocate new memory to hold crc_data and, under the spinlock, update the crc_data to point to the new memory (add a WARN_ON if the pointer is NULL)
- In the hrtimer, while holding the spinlock, take the pointer from crc_data into a local variable and clear the crtc->crc_data pointer
- Pass the pointer (from the local variable) to the crc_worker

The problem with this approach is managing the pointer allocation and deallocation. If I grab it in the hrtimer, then where/when should I free it? I can't after the crc worker is done with it because what I noticed is that the crc computation is expected to run multiple times after one plane_atomic_update.

Quoted 3 lines

Quoted 2 lines

I don't think you need to hold the spinlock across the atomic hooks, just grab it in plane_update.

I asked Daniel in irc about releasing the vkms_output->lock in the begginning of atomic_flush to avoid modifying the event_lock and he mentioned that "output->lock must wrap the event handling code completely since we need to make sure that the flip event
and the crc all match up".

So maybe I can grab the spinlock in plane_atomic_update and release it at the end of crtc_atomic_flush? Is plane_atomic_update always called before crtc_atomic_flush?

Otherwise if I grab the spinlock in atomic_begin I won't be able to use pointer for crc_data and allocate it in plane_atomic_update since I'll be holding a spinlock already.

I have based this patch on Daniel's second feedback to my email "[PATCH] drm/vkms: Update CRC support after lock review" which I didn't cc the dri mailing list in it, quoting Daniel:

"Overall flow:
1. in crtc_funcs->atomic_begin grab the spinlock, to make sure everything is visible completely or not at all to the vblank handler. Also update a pointer to the vkms_crtc_state in vkms_output->crc_data (or something like that.
2. copy the plane data (like you do here) in plane_funcs->atomic_commit.
3. drop the spinlock in crtc_funcs->atomic_flush after the vblank event is armed.

In the vblank hrtimer handler:
1. grab the spinlock
2. drm_crtc_handle_vblank() - we must do this while holding the spinlock, otherwise the event and crc might get out of sync. Also grab the current vblank number here and store it somewhere in vkms_crtc_state so that the worker can use the right one.
3. launch the worker. Note that the struct_work needs to be within the vkms_crtc_state structure, otherwise the crc computation worker doesn't know really for which data it should compute the crc - just having one copy in data[1] doesn't really solve this race.
4. drop spinlock

So one big problem we have here still is what happens if the crc worker falls behind with computing crcs. There's 2 cases, let's first look at how to fixe the crc worker falling behind atomic updates. The problem there is that the next atomic update might free the vkms_crtc_state structure before the worker is done with it. That's fairly easy to fix:
- add a flush_work call to your vkms_crtc_state_destroy function. This means you need to initialize the work already in vkms_crtc_state_duplicate (you'll create both of these when doing the vkms_crtc_state subclassing).
- 2nd we need to make sure crc workers are run in order easiest fix for that is to create your own private crc work queue (1 per crtc we have) with the WQ_ORDERED flag.

2nd issue if the vblanks get ahead of the crc worker. This happens when e.g. there's no atomic updates, but we still want to generate 1 vblank per frame. This is probably better done in a follow-up patch, but rough solution is:
- in the vblank hrtimer check whether the work is still scheduled and not yet complete (while holding the spinlock). in that case simply tell the worker to fill out the same crc for multiple vblanks, by using a vblank_seqno_start/end.
- if the worker is not busy, then we tell it to compute a new crc (in case something in the buffers has changed - in the future we might change this behaviour)."

Quoted 17 lines

So the more I think about this, I don't think it's quite right. Perhaps I'm missing an email from Daniel, or (more likely) misunderstanding what you're trying to do. However, the email I read suggested storing the crc_data to be consumed by the crc worker in vkms_crtc. However in this patch it's being stored (along with the work item) in crtc state. Further, the state cannot be destroyed without completing the crc_work, which kind of defeats the purpose of the workqueue in the first place.

Storing a copy of the crc_data in crtc allows you to free the crtc_state, so you can start working on the next frame while the crc worker is scheduled and picks up the crc_data from crtc. Storing the crc_data in crtc_state and waiting for the work to finish is easier since you don't need to worry about copying and locking (so much). However, it seems like we're doing both in this patch.

Again, I'm likely just not understanding what the goal of this is, so any clarification would be greatly appreciated :)

Quoted 8 lines

Sean

Quoted 6 lines

Sean

Quoted 3 lines

Quoted 3 lines

Review

Quoted 3 lines

(quoted diff) +2 -0

+}
+

Quoted 63 lines · Haneen Mohammed

First, just so you have the complete view, this is the missing part in this patch:
------ vkms_crc.c
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 = 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);
	}

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

	if (crtc_state && out->crc_enabled) {
		u32 crc32 = _vkms_get_crc(&crtc_state->data);
		unsigned int n_frame = crtc_state->n_frame;

		drm_crtc_add_crc_entry(crtc, true, 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);

	if (!src_name) {
		out->crc_enabled = false;
	} else if (strcmp(src_name, "auto") == 0) {
		out->crc_enabled = true;
	} else {
		out->crc_enabled = false;
		return -EINVAL;
	}

	*values_cnt = 1;

	return 0;
}

Quoted 1 line

--- end vkms_crc.c

/snip

Quoted 65 lines

Quoted 19 lines

Quoted 19 lines

Quoted 10 lines

Quoted 9 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+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);
+
+	spin_lock_irq(&vkms_output->lock);

Hmm, you can't lock across atomic_begin/flush. What is this protecting against?

Quoted 4 lines

I did this per Daniel recommendation:
1- spin_lock_irq -> vkms_crtc_atomic_begin
2- update vkms_crc_data and get ref to fb -> vkms_primary_plane_update 3- spin_unlock_irq after vblank event -> vkms_crtc_atomic_flush

I think I'm interpreting his mail a little differently. The point of the spinlock is to protect the pointer to the crc_data in vkms_crtc (right now it's not a pointer, but it should be).

Quoted 9 lines

Quoted 9 lines

So I think you need to do the following:
- Change crc_data to a pointer
- In plane_update, allocate new memory to hold crc_data and, under the spinlock, update the crc_data to point to the new memory (add a WARN_ON if the pointer is NULL)
- In the hrtimer, while holding the spinlock, take the pointer from crc_data into a local variable and clear the crtc->crc_data pointer
- Pass the pointer (from the local variable) to the crc_worker

The problem with this approach is managing the pointer allocation and deallocation. If I grab it in the hrtimer, then where/when should I free it? I can't after the crc worker is done with it because what I noticed is that the crc computation is expected to run multiple times after one plane_atomic_update.

Quoted 3 lines

Quoted 2 lines

I don't think you need to hold the spinlock across the atomic hooks, just grab it in plane_update.

I asked Daniel in irc about releasing the vkms_output->lock in the begginning of atomic_flush to avoid modifying the event_lock and he mentioned that "output->lock must wrap the event handling code completely since we need to make sure that the flip event
and the crc all match up".

So maybe I can grab the spinlock in plane_atomic_update and release it at the end of crtc_atomic_flush? Is plane_atomic_update always called before crtc_atomic_flush?

Otherwise if I grab the spinlock in atomic_begin I won't be able to use pointer for crc_data and allocate it in plane_atomic_update since I'll be holding a spinlock already.

I have based this patch on Daniel's second feedback to my email "[PATCH] drm/vkms: Update CRC support after lock review" which I didn't cc the dri mailing list in it, quoting Daniel:

"Overall flow:
1. in crtc_funcs->atomic_begin grab the spinlock, to make sure everything is visible completely or not at all to the vblank handler. Also update a pointer to the vkms_crtc_state in vkms_output->crc_data (or something like that.
2. copy the plane data (like you do here) in plane_funcs->atomic_commit.
3. drop the spinlock in crtc_funcs->atomic_flush after the vblank event is armed.

Quoted 11 lines

In the vblank hrtimer handler:
1. grab the spinlock
2. drm_crtc_handle_vblank() - we must do this while holding the spinlock, otherwise the event and crc might get out of sync. Also grab the current vblank number here and store it somewhere in vkms_crtc_state so that the worker can use the right one.
3. launch the worker. Note that the struct_work needs to be within the vkms_crtc_state structure, otherwise the crc computation worker doesn't know really for which data it should compute the crc - just having one copy in data[1] doesn't really solve this race.
4. drop spinlock

Ok, I think I understand what we're trying to do. It seems like the spinlock is simulating what we would do in hw by telling the display controller not to latch in new addresses until we're ready. By blocking the vblank handler (and, by extension, the event), we're delay sending an event out until the crc data is complete. In hw, this would be a dropped frame.

I'd probably prefer doing something more similar to hw. Something like simulating a latch to tell the vblank handler to skip sending the event until the next timer interrupt, instead of blocking it. I think that would avoid having to hold the spinlock across begin/<everything>/flush. However Daniel is smarter than I am, so I'll go with his suggestion since I'm sure there's a very good reason this is a very bad idea :-)

In that case, what you have makes sense to me, so if we could flush the work item instead of the whole queue, I think we're there.

Sean

Quoted 47 lines

So one big problem we have here still is what happens if the crc worker falls behind with computing crcs. There's 2 cases, let's first look at how to fixe the crc worker falling behind atomic updates. The problem there is that the next atomic update might free the vkms_crtc_state structure before the worker is done with it. That's fairly easy to fix:
- add a flush_work call to your vkms_crtc_state_destroy function. This means you need to initialize the work already in vkms_crtc_state_duplicate (you'll create both of these when doing the vkms_crtc_state subclassing).
- 2nd we need to make sure crc workers are run in order easiest fix for that is to create your own private crc work queue (1 per crtc we have) with the WQ_ORDERED flag.

2nd issue if the vblanks get ahead of the crc worker. This happens when e.g. there's no atomic updates, but we still want to generate 1 vblank per frame. This is probably better done in a follow-up patch, but rough solution is:
- in the vblank hrtimer check whether the work is still scheduled and not yet complete (while holding the spinlock). in that case simply tell the worker to fill out the same crc for multiple vblanks, by using a vblank_seqno_start/end.
- if the worker is not busy, then we tell it to compute a new crc (in case something in the buffers has changed - in the future we might change this behaviour)."

Quoted 20 lines

So the more I think about this, I don't think it's quite right. Perhaps I'm missing an email from Daniel, or (more likely) misunderstanding what you're trying to do. However, the email I read suggested storing the crc_data to be consumed by the crc worker in vkms_crtc. However in this patch it's being stored (along with the work item) in crtc state. Further, the state cannot be destroyed without completing the crc_work, which kind of defeats the purpose of the workqueue in the first place.

Storing a copy of the crc_data in crtc allows you to free the crtc_state, so you can start working on the next frame while the crc worker is scheduled and picks up the crc_data from crtc. Storing the crc_data in crtc_state and waiting for the work to finish is easier since you don't need to worry about copying and locking (so much). However, it seems like we're doing both in this patch.

Again, I'm likely just not understanding what the goal of this is, so any clarification would be greatly appreciated :)

Sean

Quoted 1 line

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Sean

Quoted 3 lines

Quoted 3 lines

Review

Quoted 3 lines

(quoted diff) +2 -0

+}
+

/snip

Signature · 1 line

Sean Paul, Software Engineer, Google / Chromium OS

Quoted 63 lines · Haneen Mohammed

First, just so you have the complete view, this is the missing part in this patch:
------ vkms_crc.c
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 = 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);
	}

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

	if (crtc_state && out->crc_enabled) {
		u32 crc32 = _vkms_get_crc(&crtc_state->data);
		unsigned int n_frame = crtc_state->n_frame;

		drm_crtc_add_crc_entry(crtc, true, 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);

	if (!src_name) {
		out->crc_enabled = false;
	} else if (strcmp(src_name, "auto") == 0) {
		out->crc_enabled = true;
	} else {
		out->crc_enabled = false;
		return -EINVAL;
	}

	*values_cnt = 1;

	return 0;
}

Quoted 1 line

--- end vkms_crc.c

/snip

Quoted 65 lines

Quoted 19 lines

Quoted 19 lines

Quoted 10 lines

Quoted 9 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+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);
+
+	spin_lock_irq(&vkms_output->lock);

Hmm, you can't lock across atomic_begin/flush. What is this protecting against?

Quoted 4 lines

I did this per Daniel recommendation:
1- spin_lock_irq -> vkms_crtc_atomic_begin
2- update vkms_crc_data and get ref to fb -> vkms_primary_plane_update 3- spin_unlock_irq after vblank event -> vkms_crtc_atomic_flush

I think I'm interpreting his mail a little differently. The point of the spinlock is to protect the pointer to the crc_data in vkms_crtc (right now it's not a pointer, but it should be).

Quoted 9 lines

Quoted 9 lines

So I think you need to do the following:
- Change crc_data to a pointer
- In plane_update, allocate new memory to hold crc_data and, under the spinlock, update the crc_data to point to the new memory (add a WARN_ON if the pointer is NULL)
- In the hrtimer, while holding the spinlock, take the pointer from crc_data into a local variable and clear the crtc->crc_data pointer
- Pass the pointer (from the local variable) to the crc_worker

The problem with this approach is managing the pointer allocation and deallocation. If I grab it in the hrtimer, then where/when should I free it? I can't after the crc worker is done with it because what I noticed is that the crc computation is expected to run multiple times after one plane_atomic_update.

Quoted 3 lines

Quoted 2 lines

I don't think you need to hold the spinlock across the atomic hooks, just grab it in plane_update.

I asked Daniel in irc about releasing the vkms_output->lock in the begginning of atomic_flush to avoid modifying the event_lock and he mentioned that "output->lock must wrap the event handling code completely since we need to make sure that the flip event
and the crc all match up".

So maybe I can grab the spinlock in plane_atomic_update and release it at the end of crtc_atomic_flush? Is plane_atomic_update always called before crtc_atomic_flush?

Otherwise if I grab the spinlock in atomic_begin I won't be able to use pointer for crc_data and allocate it in plane_atomic_update since I'll be holding a spinlock already.

I have based this patch on Daniel's second feedback to my email "[PATCH] drm/vkms: Update CRC support after lock review" which I didn't cc the dri mailing list in it, quoting Daniel:

"Overall flow:
1. in crtc_funcs->atomic_begin grab the spinlock, to make sure everything is visible completely or not at all to the vblank handler. Also update a pointer to the vkms_crtc_state in vkms_output->crc_data (or something like that.
2. copy the plane data (like you do here) in plane_funcs->atomic_commit.
3. drop the spinlock in crtc_funcs->atomic_flush after the vblank event is armed.

Quoted 11 lines

In the vblank hrtimer handler:
1. grab the spinlock
2. drm_crtc_handle_vblank() - we must do this while holding the spinlock, otherwise the event and crc might get out of sync. Also grab the current vblank number here and store it somewhere in vkms_crtc_state so that the worker can use the right one.
3. launch the worker. Note that the struct_work needs to be within the vkms_crtc_state structure, otherwise the crc computation worker doesn't know really for which data it should compute the crc - just having one copy in data[1] doesn't really solve this race.
4. drop spinlock

Ok, I think I understand what we're trying to do. It seems like the spinlock is simulating what we would do in hw by telling the display controller not to latch in new addresses until we're ready. By blocking the vblank handler (and, by extension, the event), we're delay sending an event out until the crc data is complete. In hw, this would be a dropped frame.

I'd probably prefer doing something more similar to hw. Something like simulating a latch to tell the vblank handler to skip sending the event until the next timer interrupt, instead of blocking it. I think that would avoid having to hold

the spinlock across begin/<everything>/flush. However Daniel is smarter than I am, so I'll go with his suggestion since I'm sure there's a very good reason this is a very bad idea :-)

In that case, what you have makes sense to me, so if we could flush the work item instead of the whole queue, I think we're there.

Sean

Quoted 47 lines

So one big problem we have here still is what happens if the crc worker falls behind with computing crcs. There's 2 cases, let's first look at how to fixe the crc worker falling behind atomic updates. The problem there is that the next atomic update might free the vkms_crtc_state structure before the worker is done with it. That's fairly easy to fix:
- add a flush_work call to your vkms_crtc_state_destroy function. This means you need to initialize the work already in vkms_crtc_state_duplicate (you'll create both of these when doing the vkms_crtc_state subclassing).
- 2nd we need to make sure crc workers are run in order easiest fix for that is to create your own private crc work queue (1 per crtc we have) with the WQ_ORDERED flag.

2nd issue if the vblanks get ahead of the crc worker. This happens when e.g. there's no atomic updates, but we still want to generate 1 vblank per frame. This is probably better done in a follow-up patch, but rough solution is:
- in the vblank hrtimer check whether the work is still scheduled and not yet complete (while holding the spinlock). in that case simply tell the worker to fill out the same crc for multiple vblanks, by using a vblank_seqno_start/end.
- if the worker is not busy, then we tell it to compute a new crc (in case something in the buffers has changed - in the future we might change this behaviour)."

Quoted 20 lines

So the more I think about this, I don't think it's quite right. Perhaps I'm missing an email from Daniel, or (more likely) misunderstanding what you're trying to do. However, the email I read suggested storing the crc_data to be consumed by the crc worker in vkms_crtc. However in this patch it's being stored (along with the work item) in crtc state. Further, the state cannot be destroyed without completing the crc_work, which kind of defeats the purpose of the workqueue in the first place.

Storing a copy of the crc_data in crtc allows you to free the crtc_state, so you can start working on the next frame while the crc worker is scheduled and picks up the crc_data from crtc. Storing the crc_data in crtc_state and waiting for the work to finish is easier since you don't need to worry about copying and locking (so much). However, it seems like we're doing both in this patch.

Again, I'm likely just not understanding what the goal of this is, so any clarification would be greatly appreciated :)

Sean

Quoted 1 line

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Sean

Quoted 3 lines

Quoted 3 lines

Review

Quoted 3 lines

(quoted diff) +2 -0

+}
+

/snip

Signature · 1 line

Sean Paul, Software Engineer, Google / Chromium OS