[RFC 0/3] Add infrastructure for CRC support

[RFC 0/3] Add infrastructure for CRC support

[Haneen Mohammed]

This patchset add the necessary infrastructure required to enable to
compute and add CRCs entries.

1. add functions to map buffers to kernel address space.
2. map/unmap buffers in the prepare/cleanup_fb hooks.
3. compute crc using crc32 on the visible portion of the buffer.

Note:
This patchset was built on top of the following patch:
subject: "drm/vkms: Add vblank events simulated by hrtimers"
link: https://lists.freedesktop.org/archives/dri-devel/2018-June/181060.html

Haneen Mohammed (3):
  drm/vkms: Add functions to map GEM backing storage
  drm/vkms: map/unmap buffers in [prepare/cleanup]_fb hooks
  drm/vkms: Implement CRC debugfs API

 drivers/gpu/drm/vkms/vkms_crtc.c  | 76 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_drv.c   |  1 +
 drivers/gpu/drm/vkms/vkms_drv.h   | 22 +++++++++
 drivers/gpu/drm/vkms/vkms_gem.c   | 50 ++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_plane.c | 50 ++++++++++++++++++++
 5 files changed, 199 insertions(+)

-- 
2.17.1

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

[RFC 1/3] drm/vkms: Add functions to map GEM backing storage

[Haneen Mohammed]

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

Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_drv.c |  1 +
 drivers/gpu/drm/vkms/vkms_drv.h |  5 ++++
 drivers/gpu/drm/vkms/vkms_gem.c | 50 +++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index c56d66d9ec56..8ab53958e4b9 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -55,6 +55,7 @@ static struct drm_driver vkms_driver = {
 	.dumb_create		= vkms_dumb_create,
 	.dumb_map_offset	= vkms_dumb_map,
 	.gem_vm_ops		= &vkms_gem_vm_ops,
+	.gem_free_object_unlocked = vkms_gem_free_object,
 
 	.name			= DRIVER_NAME,
 	.desc			= DRIVER_DESC,
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index f115e7d1ae03..d6cb8824cee2 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -41,6 +41,7 @@ struct vkms_gem_object {
 	struct drm_gem_object gem;
 	struct mutex pages_lock; /* Page lock used in page fault handler */
 	struct page **pages;
+	void *vaddr;
 };
 
 #define drm_crtc_to_vkms_output(target) \
@@ -67,4 +68,8 @@ int vkms_dumb_create(struct drm_file *file, struct drm_device *dev,
 int vkms_dumb_map(struct drm_file *file, struct drm_device *dev,
 		  u32 handle, u64 *offset);
 
+void vkms_gem_free_object(struct drm_gem_object *obj);
+
+void *vkms_gem_vmap(struct drm_gem_object *obj);
+
 #endif /* _VKMS_DRV_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_gem.c b/drivers/gpu/drm/vkms/vkms_gem.c
index 9f820f56b9e0..249855dded63 100644
--- a/drivers/gpu/drm/vkms/vkms_gem.c
+++ b/drivers/gpu/drm/vkms/vkms_gem.c
@@ -166,3 +166,53 @@ int vkms_dumb_map(struct drm_file *file, struct drm_device *dev,
 
 	return ret;
 }
+
+void vkms_gem_free_object(struct drm_gem_object *obj)
+{
+	struct vkms_gem_object *vkms_obj = container_of(obj,
+							struct vkms_gem_object,
+							gem);
+	kvfree(vkms_obj->pages);
+	mutex_destroy(&vkms_obj->pages_lock);
+	drm_gem_object_release(obj);
+	kfree(vkms_obj);
+}
+
+struct page **get_pages(struct vkms_gem_object *vkms_obj)
+{
+	struct drm_gem_object *gem_obj = &vkms_obj->gem;
+	struct page **pages = vkms_obj->pages;
+
+	if (!pages) {
+		mutex_lock(&vkms_obj->pages_lock);
+		pages = drm_gem_get_pages(gem_obj);
+		if (IS_ERR(pages)) {
+			mutex_unlock(&vkms_obj->pages_lock);
+			return pages;
+		}
+
+		vkms_obj->pages = pages;
+		mutex_unlock(&vkms_obj->pages_lock);
+	}
+
+	return pages;
+}
+
+void *vkms_gem_vmap(struct drm_gem_object *obj)
+{
+	void *vaddr = NULL;
+	struct vkms_gem_object *vkms_obj = container_of(obj,
+							struct vkms_gem_object,
+							gem);
+	unsigned int n_pages = obj->size >> PAGE_SHIFT;
+	struct page **pages = get_pages(vkms_obj);
+
+	if (IS_ERR(pages)) {
+		DRM_INFO("pages allocation failed %ld\n", PTR_ERR(pages));
+		return vaddr;
+	}
+
+	vaddr = vmap(pages, n_pages, VM_MAP, PAGE_KERNEL);
+
+	return vaddr;
+}
-- 
2.17.1

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

[RFC 2/3] drm/vkms: map/unmap buffers in [prepare/cleanup]_fb hooks

[Haneen Mohammed]

This patch checks plane state and use prepare/cleanup_fb
to map/unmap GEM backing memory to kernel address space.

Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_drv.h   |  1 +
 drivers/gpu/drm/vkms/vkms_plane.c | 50 +++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index d6cb8824cee2..300e6a05d473 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -42,6 +42,7 @@ struct vkms_gem_object {
 	struct mutex pages_lock; /* Page lock used in page fault handler */
 	struct page **pages;
 	void *vaddr;
+	struct drm_plane_state *plane_state;
 };
 
 #define drm_crtc_to_vkms_output(target) \
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index f7f63143f6d0..c717f2a41c1d 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -9,6 +9,7 @@
 #include "vkms_drv.h"
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
 
 static const struct drm_plane_funcs vkms_plane_funcs = {
 	.update_plane		= drm_atomic_helper_update_plane,
@@ -22,6 +23,14 @@ static const struct drm_plane_funcs vkms_plane_funcs = {
 static int vkms_plane_atomic_check(struct drm_plane *plane,
 				   struct drm_plane_state *state)
 {
+	if (!state->crtc)
+		return 0;
+
+	drm_atomic_helper_check_plane_state(state, state->crtc->state,
+					    DRM_PLANE_HELPER_NO_SCALING,
+					    DRM_PLANE_HELPER_NO_SCALING,
+					    false, // false for primary planes
+					    true);
 	return 0;
 }
 
@@ -30,9 +39,50 @@ 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;
+	struct vkms_gem_object *vkms_obj;
+
+	if (!state->fb)
+		return 0;
+
+	gem_obj = drm_gem_fb_get_obj(state->fb, 0);
+	vkms_obj = container_of(gem_obj, struct vkms_gem_object, gem);
+	vkms_obj->plane_state = state;
+	vkms_obj->vaddr = vkms_gem_vmap(gem_obj);
+
+	if (!vkms_obj->vaddr)
+		DRM_INFO("vmap failed\n");
+
+	return drm_gem_fb_prepare_fb(plane, state);
+}
+
+static void vkms_cleanup_fb(struct drm_plane *plane,
+			    struct drm_plane_state *state)
+{
+	struct drm_gem_object *gem_obj;
+	struct vkms_gem_object *vkms_obj;
+
+	if (!state->fb)
+		return;
+
+	gem_obj = drm_gem_fb_get_obj(state->fb, 0);
+	vkms_obj = container_of(gem_obj, struct vkms_gem_object, gem);
+
+	if (vkms_obj && vkms_obj->pages) {
+		vunmap(vkms_obj->vaddr);
+		drm_gem_put_pages(gem_obj, vkms_obj->pages, false, true);
+		vkms_obj->pages = NULL;
+	}
+}
+
 static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
 	.atomic_check		= vkms_plane_atomic_check,
 	.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)
-- 
2.17.1

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

[RFC 3/3] drm/vkms: Implement CRC debugfs API

[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 <hamohammed.sa@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_crtc.c | 76 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_drv.h  | 16 +++++++
 2 files changed, 92 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 73aae129c37d..d78934b76e33 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -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>
+
+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;
+	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);
+	}
+
+	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);
+
+	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,
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 300e6a05d473..4a1458731dd7 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -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,
-- 
2.17.1

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

Re: [RFC 3/3] drm/vkms: Implement CRC debugfs API

[Daniel Vetter]

On Thu, Jun 28, 2018 at 12:24:35AM +0300, Haneen Mohammed wrote:
> 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 <hamohammed.sa@gmail.com>

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

> ---
>  drivers/gpu/drm/vkms/vkms_crtc.c | 76 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/vkms/vkms_drv.h  | 16 +++++++
>  2 files changed, 92 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 73aae129c37d..d78934b76e33 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -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>
> +
> +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.

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

> +
> +	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,
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 300e6a05d473..4a1458731dd7 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -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,
> -- 
> 2.17.1
> 

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

Re: [RFC 3/3] drm/vkms: Implement CRC debugfs API

[Daniel Vetter]

On Thu, Jun 28, 2018 at 10:07:34AM +0200, Daniel Vetter wrote:
> On Thu, Jun 28, 2018 at 12:24:35AM +0300, Haneen Mohammed wrote:
> > 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 <hamohammed.sa@gmail.com>
> 
> Ok locking review. I still don't have a clear idea how to best fix this,
> but oh well.
> 
> > ---
> >  drivers/gpu/drm/vkms/vkms_crtc.c | 76 ++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/vkms/vkms_drv.h  | 16 +++++++
> >  2 files changed, 92 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > index 73aae129c37d..d78934b76e33 100644
> > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > @@ -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>
> > +
> > +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.
> 
> > +	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);
> > +	}
> > +
> > +	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.

btw to make sure your code is correct please enable all the lock debugging
options in the kernel configuration. Especially CONFIG_PROVE_LOCKING.
-Daniel

> -Daniel
> 
> > +
> > +	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,
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > index 300e6a05d473..4a1458731dd7 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -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,
> > -- 
> > 2.17.1
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [RFC 3/3] drm/vkms: Implement CRC debugfs API

[Daniel Vetter]

On Thu, Jun 28, 2018 at 10:07 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Jun 28, 2018 at 12:24:35AM +0300, Haneen Mohammed wrote:
>> 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 <hamohammed.sa@gmail.com>
>
> Ok locking review. I still don't have a clear idea how to best fix this,
> but oh well.
>
>> ---
>>  drivers/gpu/drm/vkms/vkms_crtc.c | 76 ++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/vkms/vkms_drv.h  | 16 +++++++
>>  2 files changed, 92 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
>> index 73aae129c37d..d78934b76e33 100644
>> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
>> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
>> @@ -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>
>> +
>> +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.
>
>> +     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);
>> +     }
>> +
>> +     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.

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

>
> 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
>
>> +
>> +     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,
>> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
>> index 300e6a05d473..4a1458731dd7 100644
>> --- a/drivers/gpu/drm/vkms/vkms_drv.h
>> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
>> @@ -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,
>> --
>> 2.17.1
>>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



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

Re: [RFC 3/3] drm/vkms: Implement CRC debugfs API

[Haneen Mohammed]

On Thu, Jun 28, 2018 at 01:40:08PM +0200, Daniel Vetter wrote:
> On Thu, Jun 28, 2018 at 10:07 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, Jun 28, 2018 at 12:24:35AM +0300, Haneen Mohammed wrote:
> >> 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 <hamohammed.sa@gmail.com>
> >
> > Ok locking review. I still don't have a clear idea how to best fix this,
> > but oh well.
> >
> >> ---
> >>  drivers/gpu/drm/vkms/vkms_crtc.c | 76 ++++++++++++++++++++++++++++++++
> >>  drivers/gpu/drm/vkms/vkms_drv.h  | 16 +++++++
> >>  2 files changed, 92 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> >> index 73aae129c37d..d78934b76e33 100644
> >> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> >> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> >> @@ -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>
> >> +
> >> +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.
> >
> >> +     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);
> >> +     }
> >> +
> >> +     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.
> 
> 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
> 

Ok I will work on that, thank you so much! 

I did update my branch recently but then that resulted in networking
problems so I reverted the update. I will fix that issue first and then
work on the locking issues. 

Thank you so much again!
Haneen

> >
> > 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
> >
> >> +
> >> +     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,
> >> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> >> index 300e6a05d473..4a1458731dd7 100644
> >> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> >> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> >> @@ -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,
> >> --
> >> 2.17.1
> >>
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel