Add infrastructure for CRC support

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

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

---
 drivers/gpu/drm/vkms/vkms_crtc.c | 76 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_drv.h  | 16 +++++++
 2 files changed, 92 insertions(+)

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

index 73aae129c37d..d78934b76e33 100644
@@ -7,8 +7,78 @@
  */
 
 #include "vkms_drv.h"
+#include <linux/crc32.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>

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

+	unsigned int x = state->src.x1 >> 16;
+	unsigned int y = state->src.y1 >> 16;
+	unsigned int height = drm_rect_height(&state->src) >> 16;
+	unsigned int width = drm_rect_width(&state->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);
+	}
+

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

+
+	if (crtc_state->crc_enabled && fb) {
+		u32 crc32 = _vkms_get_crc(fb);
+		unsigned int n_frame = drm_crtc_accurate_vblank_count(crtc);
+
+		drm_crtc_add_crc_entry(crtc, true, n_frame, &crc32);
+	}
+}
+
+static int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
+			       size_t *values_cnt)
+{
+	struct drm_device *dev = crtc->dev;
+	struct vkms_device *vkms_dev = container_of(dev,
+						    struct vkms_device,
+						    drm);
+	struct vkms_crtc_state *crtc_state = &vkms_dev->output.crtc_state;
+
+	if (!src_name) {
+		crtc_state->crc_enabled = false;
+	} else if (strcmp(src_name, "auto") == 0) {
+		crtc_state->crc_enabled = true;
+	} else {
+		crtc_state->crc_enabled = false;
+		return -EINVAL;
+	}
+
+	*values_cnt = 1;
+
+	return 0;
+}
 
 static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
 {

@@ -23,6 +93,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
 	if (!ret)
 		DRM_ERROR("vkms failure on handling vblank");
 
+	vkms_crc_work_handle(&output->crtc_state.crc_workq);
+
 	spin_lock_irqsave(&crtc->dev->event_lock, flags);
 	if (output->event) {
 		drm_crtc_send_vblank_event(crtc, output->event);

@@ -46,6 +118,9 @@ static int vkms_enable_vblank(struct drm_crtc *crtc)
 	out->period_ns = ktime_set(0, DEFAULT_VBLANK_NS);
 	hrtimer_start(&out->vblank_hrtimer, out->period_ns, HRTIMER_MODE_ABS);
 
+	out->crtc_state.crtc = crtc;
+	INIT_WORK(&out->crtc_state.crc_workq, vkms_crc_work_handle);
+
 	return 0;
 }
 

@@ -65,6 +140,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
 	.atomic_destroy_state   = drm_atomic_helper_crtc_destroy_state,
 	.enable_vblank		= vkms_enable_vblank,
 	.disable_vblank		= vkms_disable_vblank,
+	.set_crc_source		= vkms_set_crc_source,
 };
 
 static int vkms_crtc_atomic_check(struct drm_crtc *crtc,

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

index 300e6a05d473..4a1458731dd7 100644
@@ -22,6 +22,18 @@ static const u32 vkms_formats[] = {
 	DRM_FORMAT_XRGB8888,
 };
 
+/**
+ * struct vkms_crtc_state
+ * @crtc: backpointer to the CRTC
+ * @crc_workq: worker that captures CRCs for each frame
+ * @crc_enabled: flag to test if crc is enabled
+ */
+struct vkms_crtc_state {
+	struct drm_crtc *crtc;
+	struct work_struct crc_workq;
+	bool crc_enabled;
+};
+
 struct vkms_output {
 	struct drm_crtc crtc;
 	struct drm_encoder encoder;

@@ -29,6 +41,7 @@ struct vkms_output {
 	struct hrtimer vblank_hrtimer;
 	ktime_t period_ns;
 	struct drm_pending_vblank_event *event;
+	struct vkms_crtc_state crtc_state;
 };
 
 struct vkms_device {

@@ -55,6 +68,9 @@ int vkms_output_init(struct vkms_device *vkmsdev);
 
 struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev);
 
+/* CRC Support */
+void vkms_crc_work_handle(struct work_struct *work);
+
 /* Gem stuff */
 struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
 				       struct drm_file *file,

Signature · 1 line

2.17.1

Quoted 5 lines · Haneen Mohammed

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

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

Ok locking review. I still don't have a clear idea how to best fix this, but oh well.

Quoted 17 lines

---
 drivers/gpu/drm/vkms/vkms_crtc.c | 76 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_drv.h  | 16 +++++++
 2 files changed, 92 insertions(+)

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

index 73aae129c37d..d78934b76e33 100644
@@ -7,8 +7,78 @@
  */
 
 #include "vkms_drv.h"
+#include <linux/crc32.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>

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

Quoted 10 lines

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

+
+uint32_t _vkms_get_crc(struct drm_framebuffer *fb)
+{
+	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
+	struct vkms_gem_object *vkms_obj = container_of(gem_obj,
+							struct vkms_gem_object,
+							gem);
+	u32 crc = 0;
+	int i = 0;
+	struct drm_plane_state *state = vkms_obj->plane_state;

vkms_obj->plane_state is the first locking issue: You store the pointer without any locks or synchronization, which means the crc worker here could access is while it's being changed, resulting in an inconsistent crc. Note that the compiler/cpu is allowed to read a pointer multiple times if it's not protected by locking/barriers, so all kinds of funny stuff can happen here.

Second issue is that the memory you're pointing to isn't owned by the crc subsystem, but by the atomic commit worker. That could release the memory (and it might get reused for something else meanwhile) before the crc worker here is done with it.

Quoted 17 lines

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

+	unsigned int x = state->src.x1 >> 16;
+	unsigned int y = state->src.y1 >> 16;
+	unsigned int height = drm_rect_height(&state->src) >> 16;
+	unsigned int width = drm_rect_width(&state->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);
+	}
+

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

Quoted 10 lines

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

+	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_workq);
+	struct drm_crtc *crtc = crtc_state->crtc;
+	struct drm_framebuffer *fb = (crtc->primary ? crtc->primary->fb : NULL);

The drm_plane->fb pointer has similar issues: No locking (the only thing that's really allowed to change this is the atomic commit worker for atomic drivers, nothing else), so same issues with inconsistent data and using possibly freed memory.

On top Ville has a patch series to set drm_plane->fb == NULL for all atomic drivers - it's really a leftover from the pre-atomic age and doesn't fit for atomic.

On top of these issues for ->plane_state and ->fb we have a third issue:
The crc computation must be atomic wrt the generated vblank event for a given atomic update. That means we must make sure that the crc we're generating is exactly for the plane_state/fb that also issued the vblank event. There's some atomic kms tests in igt that check this. More concretely we need to make sure that:
- if the vblank code sees the new vblank event, then the crc _must_ compute the crc for the new configuration.
- if the vblank code does not yet see the new vblank event, then the crc _must_ compute the crc for the old configuration.

One simple solution, but quite a bit of code to type would be.

1. add a spinlock to the vkms_crtc structure (probably need that first) to protect all the crc/vblank event stuff. Needs to be a spinlock since we must look at this from the hrtimer.

2. Create a new structure where you make a copy (not just pointers) of all the data the hrtimer needs. So src offset, vblank event, all that stuff. For the fb pointer you need to make sure it's properly reference counted (drm_framebuffer_get/put).

3. For every atomic update, allocate a new such structure and store it in the vkms_crtc structure (holding the spinlock while updating the pointer).

4. In the hrtimer grab that pointer and set it to NULL (again holding the spinlock), and then explicitly send out the vblank event (using drm_crtc_send_vblank_event). And then pass that structure with everything the crc worker needs to the crc worker.

I think this should work, mostly.

-Daniel

Quoted 105 lines

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

+
+	if (crtc_state->crc_enabled && fb) {
+		u32 crc32 = _vkms_get_crc(fb);
+		unsigned int n_frame = drm_crtc_accurate_vblank_count(crtc);
+
+		drm_crtc_add_crc_entry(crtc, true, n_frame, &crc32);
+	}
+}
+
+static int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
+			       size_t *values_cnt)
+{
+	struct drm_device *dev = crtc->dev;
+	struct vkms_device *vkms_dev = container_of(dev,
+						    struct vkms_device,
+						    drm);
+	struct vkms_crtc_state *crtc_state = &vkms_dev->output.crtc_state;
+
+	if (!src_name) {
+		crtc_state->crc_enabled = false;
+	} else if (strcmp(src_name, "auto") == 0) {
+		crtc_state->crc_enabled = true;
+	} else {
+		crtc_state->crc_enabled = false;
+		return -EINVAL;
+	}
+
+	*values_cnt = 1;
+
+	return 0;
+}
 
 static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
 {

@@ -23,6 +93,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
 	if (!ret)
 		DRM_ERROR("vkms failure on handling vblank");
 
+	vkms_crc_work_handle(&output->crtc_state.crc_workq);
+
 	spin_lock_irqsave(&crtc->dev->event_lock, flags);
 	if (output->event) {
 		drm_crtc_send_vblank_event(crtc, output->event);

@@ -46,6 +118,9 @@ static int vkms_enable_vblank(struct drm_crtc *crtc)
 	out->period_ns = ktime_set(0, DEFAULT_VBLANK_NS);
 	hrtimer_start(&out->vblank_hrtimer, out->period_ns, HRTIMER_MODE_ABS);
 
+	out->crtc_state.crtc = crtc;
+	INIT_WORK(&out->crtc_state.crc_workq, vkms_crc_work_handle);
+
 	return 0;
 }
 

@@ -65,6 +140,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
 	.atomic_destroy_state   = drm_atomic_helper_crtc_destroy_state,
 	.enable_vblank		= vkms_enable_vblank,
 	.disable_vblank		= vkms_disable_vblank,
+	.set_crc_source		= vkms_set_crc_source,
 };
 
 static int vkms_crtc_atomic_check(struct drm_crtc *crtc,

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

index 300e6a05d473..4a1458731dd7 100644
@@ -22,6 +22,18 @@ static const u32 vkms_formats[] = {
 	DRM_FORMAT_XRGB8888,
 };
 
+/**
+ * struct vkms_crtc_state
+ * @crtc: backpointer to the CRTC
+ * @crc_workq: worker that captures CRCs for each frame
+ * @crc_enabled: flag to test if crc is enabled
+ */
+struct vkms_crtc_state {
+	struct drm_crtc *crtc;
+	struct work_struct crc_workq;
+	bool crc_enabled;
+};
+
 struct vkms_output {
 	struct drm_crtc crtc;
 	struct drm_encoder encoder;

@@ -29,6 +41,7 @@ struct vkms_output {
 	struct hrtimer vblank_hrtimer;
 	ktime_t period_ns;
 	struct drm_pending_vblank_event *event;
+	struct vkms_crtc_state crtc_state;
 };
 
 struct vkms_device {

@@ -55,6 +68,9 @@ int vkms_output_init(struct vkms_device *vkmsdev);
 
 struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev);
 
+/* CRC Support */
+void vkms_crc_work_handle(struct work_struct *work);
+
 /* Gem stuff */
 struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
 				       struct drm_file *file,

Signature · 1 line

2.17.1

Signature · 3 lines

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

Quoted 5 lines · Haneen Mohammed

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

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

Ok locking review. I still don't have a clear idea how to best fix this, but oh well.

Quoted 17 lines

---
 drivers/gpu/drm/vkms/vkms_crtc.c | 76 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_drv.h  | 16 +++++++
 2 files changed, 92 insertions(+)

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

index 73aae129c37d..d78934b76e33 100644
@@ -7,8 +7,78 @@
  */
 
 #include "vkms_drv.h"
+#include <linux/crc32.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>

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

Quoted 10 lines

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

+
+uint32_t _vkms_get_crc(struct drm_framebuffer *fb)
+{
+     struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
+     struct vkms_gem_object *vkms_obj = container_of(gem_obj,
+                                                     struct vkms_gem_object,
+                                                     gem);
+     u32 crc = 0;
+     int i = 0;
+     struct drm_plane_state *state = vkms_obj->plane_state;

vkms_obj->plane_state is the first locking issue: You store the pointer without any locks or synchronization, which means the crc worker here could access is while it's being changed, resulting in an inconsistent crc. Note that the compiler/cpu is allowed to read a pointer multiple times if it's not protected by locking/barriers, so all kinds of funny stuff can happen here.

Second issue is that the memory you're pointing to isn't owned by the crc subsystem, but by the atomic commit worker. That could release the memory (and it might get reused for something else meanwhile) before the crc worker here is done with it.

Quoted 17 lines

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

+     unsigned int x = state->src.x1 >> 16;
+     unsigned int y = state->src.y1 >> 16;
+     unsigned int height = drm_rect_height(&state->src) >> 16;
+     unsigned int width = drm_rect_width(&state->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);
+     }
+

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

Quoted 10 lines

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

+     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_workq);
+     struct drm_crtc *crtc = crtc_state->crtc;
+     struct drm_framebuffer *fb = (crtc->primary ? crtc->primary->fb : NULL);

The drm_plane->fb pointer has similar issues: No locking (the only thing that's really allowed to change this is the atomic commit worker for atomic drivers, nothing else), so same issues with inconsistent data and using possibly freed memory.

On top Ville has a patch series to set drm_plane->fb == NULL for all atomic drivers - it's really a leftover from the pre-atomic age and doesn't fit for atomic.

On top of these issues for ->plane_state and ->fb we have a third issue:
The crc computation must be atomic wrt the generated vblank event for a given atomic update. That means we must make sure that the crc we're generating is exactly for the plane_state/fb that also issued the vblank event. There's some atomic kms tests in igt that check this. More concretely we need to make sure that:
- if the vblank code sees the new vblank event, then the crc _must_ compute the crc for the new configuration.
- if the vblank code does not yet see the new vblank event, then the crc _must_ compute the crc for the old configuration.

One simple solution, but quite a bit of code to type would be.

1. add a spinlock to the vkms_crtc structure (probably need that first) to protect all the crc/vblank event stuff. Needs to be a spinlock since we must look at this from the hrtimer.

2. Create a new structure where you make a copy (not just pointers) of all the data the hrtimer needs. So src offset, vblank event, all that stuff. For the fb pointer you need to make sure it's properly reference counted (drm_framebuffer_get/put).

3. For every atomic update, allocate a new such structure and store it in the vkms_crtc structure (holding the spinlock while updating the pointer).

4. In the hrtimer grab that pointer and set it to NULL (again holding the spinlock), and then explicitly send out the vblank event (using drm_crtc_send_vblank_event). And then pass that structure with everything the crc worker needs to the crc worker.

I think this should work, mostly.
-Daniel

Quoted 103 lines

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

+
+     if (crtc_state->crc_enabled && fb) {
+             u32 crc32 = _vkms_get_crc(fb);
+             unsigned int n_frame = drm_crtc_accurate_vblank_count(crtc);
+
+             drm_crtc_add_crc_entry(crtc, true, n_frame, &crc32);
+     }
+}
+
+static int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
+                            size_t *values_cnt)
+{
+     struct drm_device *dev = crtc->dev;
+     struct vkms_device *vkms_dev = container_of(dev,
+                                                 struct vkms_device,
+                                                 drm);
+     struct vkms_crtc_state *crtc_state = &vkms_dev->output.crtc_state;
+
+     if (!src_name) {
+             crtc_state->crc_enabled = false;
+     } else if (strcmp(src_name, "auto") == 0) {
+             crtc_state->crc_enabled = true;
+     } else {
+             crtc_state->crc_enabled = false;
+             return -EINVAL;
+     }
+
+     *values_cnt = 1;
+
+     return 0;
+}
 
 static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
 {

@@ -23,6 +93,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
      if (!ret)
              DRM_ERROR("vkms failure on handling vblank");
 
+     vkms_crc_work_handle(&output->crtc_state.crc_workq);
+
      spin_lock_irqsave(&crtc->dev->event_lock, flags);
      if (output->event) {
              drm_crtc_send_vblank_event(crtc, output->event);

@@ -46,6 +118,9 @@ static int vkms_enable_vblank(struct drm_crtc *crtc)
      out->period_ns = ktime_set(0, DEFAULT_VBLANK_NS);
      hrtimer_start(&out->vblank_hrtimer, out->period_ns, HRTIMER_MODE_ABS);
 
+     out->crtc_state.crtc = crtc;
+     INIT_WORK(&out->crtc_state.crc_workq, vkms_crc_work_handle);
+
      return 0;
 }
 

@@ -65,6 +140,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
      .atomic_destroy_state   = drm_atomic_helper_crtc_destroy_state,
      .enable_vblank          = vkms_enable_vblank,
      .disable_vblank         = vkms_disable_vblank,
+     .set_crc_source         = vkms_set_crc_source,
 };
 
 static int vkms_crtc_atomic_check(struct drm_crtc *crtc,

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

index 300e6a05d473..4a1458731dd7 100644
@@ -22,6 +22,18 @@ static const u32 vkms_formats[] = {
      DRM_FORMAT_XRGB8888,
 };
 
+/**
+ * struct vkms_crtc_state
+ * @crtc: backpointer to the CRTC
+ * @crc_workq: worker that captures CRCs for each frame
+ * @crc_enabled: flag to test if crc is enabled
+ */
+struct vkms_crtc_state {
+     struct drm_crtc *crtc;
+     struct work_struct crc_workq;
+     bool crc_enabled;
+};
+
 struct vkms_output {
      struct drm_crtc crtc;
      struct drm_encoder encoder;

@@ -29,6 +41,7 @@ struct vkms_output {
      struct hrtimer vblank_hrtimer;
      ktime_t period_ns;
      struct drm_pending_vblank_event *event;
+     struct vkms_crtc_state crtc_state;
 };
 
 struct vkms_device {

@@ -55,6 +68,9 @@ int vkms_output_init(struct vkms_device *vkmsdev);
 
 struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev);
 
+/* CRC Support */
+void vkms_crc_work_handle(struct work_struct *work);
+
 /* Gem stuff */
 struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
                                     struct drm_file *file,
--

Quoted 2 lines

2.17.1

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

Quoted 83 lines · Daniel Vetter

Quoted 5 lines · Haneen Mohammed

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

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

Ok locking review. I still don't have a clear idea how to best fix this, but oh well.

Quoted 17 lines

---
 drivers/gpu/drm/vkms/vkms_crtc.c | 76 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_drv.h  | 16 +++++++
 2 files changed, 92 insertions(+)

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

index 73aae129c37d..d78934b76e33 100644
@@ -7,8 +7,78 @@
  */
 
 #include "vkms_drv.h"
+#include <linux/crc32.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>

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

Quoted 10 lines

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

+
+uint32_t _vkms_get_crc(struct drm_framebuffer *fb)
+{
+     struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
+     struct vkms_gem_object *vkms_obj = container_of(gem_obj,
+                                                     struct vkms_gem_object,
+                                                     gem);
+     u32 crc = 0;
+     int i = 0;
+     struct drm_plane_state *state = vkms_obj->plane_state;

vkms_obj->plane_state is the first locking issue: You store the pointer without any locks or synchronization, which means the crc worker here could access is while it's being changed, resulting in an inconsistent crc. Note that the compiler/cpu is allowed to read a pointer multiple times if it's not protected by locking/barriers, so all kinds of funny stuff can happen here.

Second issue is that the memory you're pointing to isn't owned by the crc subsystem, but by the atomic commit worker. That could release the memory (and it might get reused for something else meanwhile) before the crc worker here is done with it.

Quoted 17 lines

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

+     unsigned int x = state->src.x1 >> 16;
+     unsigned int y = state->src.y1 >> 16;
+     unsigned int height = drm_rect_height(&state->src) >> 16;
+     unsigned int width = drm_rect_width(&state->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);
+     }
+

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

Quoted 10 lines

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

+     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_workq);
+     struct drm_crtc *crtc = crtc_state->crtc;
+     struct drm_framebuffer *fb = (crtc->primary ? crtc->primary->fb : NULL);

The drm_plane->fb pointer has similar issues: No locking (the only thing that's really allowed to change this is the atomic commit worker for atomic drivers, nothing else), so same issues with inconsistent data and using possibly freed memory.

Quoted 3 lines

On top Ville has a patch series to set drm_plane->fb == NULL for all atomic drivers - it's really a leftover from the pre-atomic age and doesn't fit for atomic.

Ville just said that he pushed all that. Are you sure that you're on latest drm-tip? On latest drm-tip primary->fb should be always NULL for an atomic driver like vkms ... If you're not on latest drm-tip probably good idea to rebase everything.
-Daniel

Quoted 130 lines

On top of these issues for ->plane_state and ->fb we have a third issue:
The crc computation must be atomic wrt the generated vblank event for a given atomic update. That means we must make sure that the crc we're generating is exactly for the plane_state/fb that also issued the vblank event. There's some atomic kms tests in igt that check this. More concretely we need to make sure that:
- if the vblank code sees the new vblank event, then the crc _must_ compute the crc for the new configuration.
- if the vblank code does not yet see the new vblank event, then the crc _must_ compute the crc for the old configuration.

One simple solution, but quite a bit of code to type would be.

1. add a spinlock to the vkms_crtc structure (probably need that first) to protect all the crc/vblank event stuff. Needs to be a spinlock since we must look at this from the hrtimer.

2. Create a new structure where you make a copy (not just pointers) of all the data the hrtimer needs. So src offset, vblank event, all that stuff. For the fb pointer you need to make sure it's properly reference counted (drm_framebuffer_get/put).

3. For every atomic update, allocate a new such structure and store it in the vkms_crtc structure (holding the spinlock while updating the pointer).

4. In the hrtimer grab that pointer and set it to NULL (again holding the spinlock), and then explicitly send out the vblank event (using drm_crtc_send_vblank_event). And then pass that structure with everything the crc worker needs to the crc worker.

I think this should work, mostly.
-Daniel

Quoted 96 lines

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

+
+     if (crtc_state->crc_enabled && fb) {
+             u32 crc32 = _vkms_get_crc(fb);
+             unsigned int n_frame = drm_crtc_accurate_vblank_count(crtc);
+
+             drm_crtc_add_crc_entry(crtc, true, n_frame, &crc32);
+     }
+}
+
+static int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
+                            size_t *values_cnt)
+{
+     struct drm_device *dev = crtc->dev;
+     struct vkms_device *vkms_dev = container_of(dev,
+                                                 struct vkms_device,
+                                                 drm);
+     struct vkms_crtc_state *crtc_state = &vkms_dev->output.crtc_state;
+
+     if (!src_name) {
+             crtc_state->crc_enabled = false;
+     } else if (strcmp(src_name, "auto") == 0) {
+             crtc_state->crc_enabled = true;
+     } else {
+             crtc_state->crc_enabled = false;
+             return -EINVAL;
+     }
+
+     *values_cnt = 1;
+
+     return 0;
+}
 
 static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
 {

@@ -23,6 +93,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
      if (!ret)
              DRM_ERROR("vkms failure on handling vblank");
 
+     vkms_crc_work_handle(&output->crtc_state.crc_workq);
+
      spin_lock_irqsave(&crtc->dev->event_lock, flags);
      if (output->event) {
              drm_crtc_send_vblank_event(crtc, output->event);

@@ -46,6 +118,9 @@ static int vkms_enable_vblank(struct drm_crtc *crtc)
      out->period_ns = ktime_set(0, DEFAULT_VBLANK_NS);
      hrtimer_start(&out->vblank_hrtimer, out->period_ns, HRTIMER_MODE_ABS);
 
+     out->crtc_state.crtc = crtc;
+     INIT_WORK(&out->crtc_state.crc_workq, vkms_crc_work_handle);
+
      return 0;
 }
 

@@ -65,6 +140,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
      .atomic_destroy_state   = drm_atomic_helper_crtc_destroy_state,
      .enable_vblank          = vkms_enable_vblank,
      .disable_vblank         = vkms_disable_vblank,
+     .set_crc_source         = vkms_set_crc_source,
 };
 
 static int vkms_crtc_atomic_check(struct drm_crtc *crtc,

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

index 300e6a05d473..4a1458731dd7 100644
@@ -22,6 +22,18 @@ static const u32 vkms_formats[] = {
      DRM_FORMAT_XRGB8888,
 };
 
+/**
+ * struct vkms_crtc_state
+ * @crtc: backpointer to the CRTC
+ * @crc_workq: worker that captures CRCs for each frame
+ * @crc_enabled: flag to test if crc is enabled
+ */
+struct vkms_crtc_state {
+     struct drm_crtc *crtc;
+     struct work_struct crc_workq;
+     bool crc_enabled;
+};
+
 struct vkms_output {
      struct drm_crtc crtc;
      struct drm_encoder encoder;

@@ -29,6 +41,7 @@ struct vkms_output {
      struct hrtimer vblank_hrtimer;
      ktime_t period_ns;
      struct drm_pending_vblank_event *event;
+     struct vkms_crtc_state crtc_state;
 };
 
 struct vkms_device {

@@ -55,6 +68,9 @@ int vkms_output_init(struct vkms_device *vkmsdev);
 
 struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev);
 

Quoted 14 lines

Quoted 9 lines

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

+/* CRC Support */
+void vkms_crc_work_handle(struct work_struct *work);
+
 /* Gem stuff */
 struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
                                     struct drm_file *file,
--

2.17.1

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

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch