Skip to contentSkip to conversation

Implement CRC and Add necessary infrastructure

PATCH Superseded Haneen Mohammed 2 participants 9 messages Jul 19, 2018 – Jul 23, 2018

Subsystems (3)

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Haneen Mohammed

author reviewer
#0 lore raw

v3 cover Implement CRC and Add necessary infrastructure

This patchset implement CRC debugfs API and add the necessary infrastructure required to enable to compute and add CRCs entries.

1. add functions to map/unmap 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 with appropriate synchronization methods.

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

 drivers/gpu/drm/vkms/Makefile     |   2 +-
 drivers/gpu/drm/vkms/vkms_crc.c   |  66 ++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_crtc.c  | 109 +++++++++++++++++++++++++++---
 drivers/gpu/drm/vkms/vkms_drv.c   |   1 +
 drivers/gpu/drm/vkms/vkms_drv.h   |  41 +++++++++++
 drivers/gpu/drm/vkms/vkms_gem.c   |  80 +++++++++++++++++++++-
 drivers/gpu/drm/vkms/vkms_plane.c |  74 ++++++++++++++++++++
 7 files changed, 360 insertions(+), 13 deletions(-)
 create mode 100644 drivers/gpu/drm/vkms/vkms_crc.c
Signature · 6 lines
2.17.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author reviewer
#1 lore raw

PATCH v3 1/5 drm/vkms: Add functions to map/unmap GEM backing storage

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

---
Changes in v2:
- add vkms_gem_vunmap
- use vmap_count to guard against multiple prepare_fb calls on the same fb
Changes in v3:
- change vkms_gem_vmap to retrun int
- cleanup if vmap failed
- increment vmap_count after successful vmap

 drivers/gpu/drm/vkms/vkms_drv.h |  9 ++++
 drivers/gpu/drm/vkms/vkms_gem.c | 80 ++++++++++++++++++++++++++++++++-
 2 files changed, 88 insertions(+), 1 deletion(-)
drivers/gpu/drm/vkms/vkms_drv.h +9 -0
index 07be29f2dc44..47048f707566 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);
 
+int 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 +79 -1

89 lines not on this page · raw message

Signature · 6 lines
2.17.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Reviewed-by on v3 1/5
#2 lore raw

Re: [PATCH v3 1/5] drm/vkms: Add functions to map/unmap GEM backing storage

Quoted 132 lines · Haneen Mohammed

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

---
Changes in v2:
- add vkms_gem_vunmap
- use vmap_count to guard against multiple prepare_fb calls on the same fb
Changes in v3:
- change vkms_gem_vmap to retrun int
- cleanup if vmap failed
- increment vmap_count after successful vmap

 drivers/gpu/drm/vkms/vkms_drv.h |  9 ++++
 drivers/gpu/drm/vkms/vkms_gem.c | 80 ++++++++++++++++++++++++++++++++-
 2 files changed, 88 insertions(+), 1 deletion(-)
drivers/gpu/drm/vkms/vkms_drv.h +9 -0
index 07be29f2dc44..47048f707566 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);
 
+int 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 +66 -1
index c7e38368602b..2e55c906d9b2 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,79 @@ 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);
+	if (vkms_obj->vmap_count < 1) {
+		WARN_ON(vkms_obj->vaddr);
+		WARN_ON(vkms_obj->pages);
+		mutex_unlock(&vkms_obj->pages_lock);
+		return;
+	}
+
+	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);
+		vkms_obj->pages = NULL;
+	}
+
+	mutex_unlock(&vkms_obj->pages_lock);
+}
+
+int vkms_gem_vmap(struct drm_gem_object *obj)
+{
+	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(obj);
+	int ret = 0;
+
+	mutex_lock(&vkms_obj->pages_lock);
+
+	if (!vkms_obj->vaddr) {
+		unsigned int n_pages = obj->size >> PAGE_SHIFT;
+		struct page **pages = _get_pages(vkms_obj);
+
+		if (IS_ERR(pages)) {
+			ret = PTR_ERR(pages);
+			goto fail;
+		}
+
+		vkms_obj->vaddr = vmap(pages, n_pages, VM_MAP, PAGE_KERNEL);
+		if (!vkms_obj->vaddr) {
+			ret = -ENOMEM;
+			drm_gem_put_pages(obj, vkms_obj->pages, false, true);
+			vkms_obj->pages = NULL;

drivers/gpu/drm/vkms/vkms_gem.c int vkms_dumb_map(struct drm_file *file, struct drm_device *dev, · line 254

Quoted 10 lines
drivers/gpu/drm/vkms/vkms_gem.c +10 -0
+			goto fail;
+		}
+
+		vkms_obj->vmap_count++;
+	}
+
+	mutex_unlock(&vkms_obj->pages_lock);
+	return 0;
+
+fail:

Since fail does the same as the success path, there are 2 ways of handling this that would be (IMO) better:

1- Rename fail to out and remove the redundant 2 lines above
2- Add a new label above fail called err_vmap and do the ret/put_pages/pages=NULL
cleanup there, dropping into the unlock and return after.

With either of these changes,

Sean

Quoted 6 lines
drivers/gpu/drm/vkms/vkms_gem.c +3 -0
+	mutex_unlock(&vkms_obj->pages_lock);
+	return ret;
+}
Signature · 1 line
2.17.1
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author reviewer
#3 lore raw

PATCH v3 2/5 drm/vkms: map/unmap buffers in [prepare/cleanup]_fb hooks

In reply to Haneen Mohammed

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

---
Changes in v2:
- use vkms_gem_vunmap

Changes in v3:
- return error number instead of vkms_obj->vaddr

 drivers/gpu/drm/vkms/vkms_plane.c | 35 +++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)
drivers/gpu/drm/vkms/vkms_plane.c +35 -0
index 9f75b1e2c1c4..aa6cde34710c 100644
@@ -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,
@@ -24,8 +25,42 @@ 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;
+	int ret;
+
+	if (!state->fb)
+		return 0;
+
+	gem_obj = drm_gem_fb_get_obj(state->fb, 0);
+	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
+	ret = vkms_gem_vmap(gem_obj);
+
+	if (ret)
+		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 · 6 lines
2.17.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#4 lore raw

Re: [PATCH v3 2/5] drm/vkms: map/unmap buffers in [prepare/cleanup]_fb hooks

Quoted 35 lines · Haneen Mohammed

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

---
Changes in v2:
- use vkms_gem_vunmap

Changes in v3:
- return error number instead of vkms_obj->vaddr

 drivers/gpu/drm/vkms/vkms_plane.c | 35 +++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)
drivers/gpu/drm/vkms/vkms_plane.c +5 -0
index 9f75b1e2c1c4..aa6cde34710c 100644
@@ -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,
@@ -24,8 +25,42 @@ 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 static void vkms_primary_plane_update(struct drm_plane *plane, · line 41

Quoted 10 lines
drivers/gpu/drm/vkms/vkms_plane.c +10 -0
+	struct vkms_gem_object *vkms_obj;
+	int ret;
+
+	if (!state->fb)
+		return 0;
+
+	gem_obj = drm_gem_fb_get_obj(state->fb, 0);
+	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
+	ret = vkms_gem_vmap(gem_obj);
+

nit: extra line

drivers/gpu/drm/vkms/vkms_plane.c static void vkms_primary_plane_update(struct drm_plane *plane, · line 43

Quoted 2 lines
drivers/gpu/drm/vkms/vkms_plane.c +2 -0
+	if (ret)
+		DRM_ERROR("vmap failed\n");

Can we actually do anything useful if the vmap fails? Asked another way, what's the value in not returning the error here? (We should also print the value of ret).

Sean

Quoted 26 lines
drivers/gpu/drm/vkms/vkms_plane.c +18 -0
+
+	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
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author reviewer
#5 lore raw

PATCH v3 3/5 drm/vkms: Add atomic_helper_check_plane_state

In reply to Haneen Mohammed

Call atomic_helper_check_plane_state to clip plane coordinates.

---
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 aa6cde34710c..5b7c5d59f103 100644
@@ -8,6 +8,7 @@
 
 #include "vkms_drv.h"
 #include <drm/drm_plane_helper.h>
+#include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_gem_framebuffer_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)
 {
@@ -59,6 +87,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 · 6 lines
2.17.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author reviewer
#6 lore raw

PATCH v3 4/5 drm/vkms: subclass CRTC state

In reply to Haneen Mohammed

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.

---
 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 47048f707566..75e52d61e65d 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 · 6 lines
2.17.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author reviewer
#7 lore raw

PATCH v3 5/5 drm/vkms: Implement CRC debugfs API

In reply to 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.

Use appropriate synchronization methods since the CRC computation must be atomic wrt the generated vblank event for a given atomic update, by using spinlock across atomic_begin and atomic_flush to wrap the event handling code completely and match the flip event with the CRC.

---
Changes in v3:
- flush work item instead of workqueue
- include missing vkms_crc.c file

 drivers/gpu/drm/vkms/Makefile     |  2 +-
 drivers/gpu/drm/vkms/vkms_crc.c   | 66 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_crtc.c  | 58 ++++++++++++++++++++++-----
 drivers/gpu/drm/vkms/vkms_drv.c   |  1 +
 drivers/gpu/drm/vkms/vkms_drv.h   | 21 ++++++++++
 drivers/gpu/drm/vkms/vkms_plane.c | 10 +++++
 6 files changed, 148 insertions(+), 10 deletions(-)
 create mode 100644 drivers/gpu/drm/vkms/vkms_crc.c
drivers/gpu/drm/vkms/Makefile +1 -1
index 986297da51bf..37966914f70b 100644
@@ -1,3 +1,3 @@
-vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o
+vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
 
 obj-$(CONFIG_DRM_VKMS) += vkms.o
drivers/gpu/drm/vkms/vkms_crc.c new file +66 -0

66 lines not on this page · raw message

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

122 lines not on this page · raw message

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 +21 -0
index 75e52d61e65d..a80281a12010 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 {
@@ -35,6 +46,11 @@ struct vkms_output {
 	struct hrtimer vblank_hrtimer;
 	ktime_t period_ns;
 	struct drm_pending_vblank_event *event;
+	bool crc_enabled;
+	/* ordered wq for crc_work */
+	struct workqueue_struct *crc_workq;
+	/* protects concurrent access to crc_data */
+	spinlock_t lock;
 };
 
 struct vkms_device {
@@ -95,4 +111,9 @@ int vkms_gem_vmap(struct drm_gem_object *obj);
 
 void vkms_gem_vunmap(struct drm_gem_object *obj);
 
+/* CRC Support */
+int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
+			size_t *values_cnt);
+void vkms_crc_work_handle(struct work_struct *work);
+
 #endif /* _VKMS_DRV_H_ */
drivers/gpu/drm/vkms/vkms_plane.c +10 -0
index 5b7c5d59f103..b316b2af88ee 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 · 6 lines
2.17.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#8 lore raw

Re: [PATCH v3 5/5] drm/vkms: Implement CRC debugfs API

Quoted 49 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.

Use appropriate synchronization methods since the CRC computation must be atomic wrt the generated vblank event for a given atomic update, by using spinlock across atomic_begin and atomic_flush to wrap the event handling code completely and match the flip event with the CRC.

---
Changes in v3:
- flush work item instead of workqueue
- include missing vkms_crc.c file

 drivers/gpu/drm/vkms/Makefile     |  2 +-
 drivers/gpu/drm/vkms/vkms_crc.c   | 66 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_crtc.c  | 58 ++++++++++++++++++++++-----
 drivers/gpu/drm/vkms/vkms_drv.c   |  1 +
 drivers/gpu/drm/vkms/vkms_drv.h   | 21 ++++++++++
 drivers/gpu/drm/vkms/vkms_plane.c | 10 +++++
 6 files changed, 148 insertions(+), 10 deletions(-)
 create mode 100644 drivers/gpu/drm/vkms/vkms_crc.c
drivers/gpu/drm/vkms/Makefile +1 -1
index 986297da51bf..37966914f70b 100644
@@ -1,3 +1,3 @@
-vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o
+vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
 
 obj-$(CONFIG_DRM_VKMS) += vkms.o
drivers/gpu/drm/vkms/vkms_crc.c new file +10 -0
new file mode 100644
index 000000000000..9e8bb5c7fc80
@@ -0,0 +1,66 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "vkms_drv.h"
+#include <linux/crc32.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+
+static uint32_t _vkms_get_crc(struct vkms_crc_data *crc_data)
+{
+	struct drm_framebuffer *fb = &crc_data->fb;
+	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
+	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(gem_obj);

drivers/gpu/drm/vkms/vkms_crc.c line 20

Quoted 10 lines
drivers/gpu/drm/vkms/vkms_crc.c new file +10 -0
+	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;

You should hold the lock when copying the data from crc_data.

Quoted 23 lines
drivers/gpu/drm/vkms/vkms_crc.c new file +23 -0
+
+	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;

drivers/gpu/drm/vkms/vkms_crc.c line 53

Quoted 10 lines
drivers/gpu/drm/vkms/vkms_crc.c new file +10 -0
+
+		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);
+

You should hold the lock when changing this value.

Quoted 65 lines
drivers/gpu/drm/vkms/vkms_crc.c new file +13 -0
+	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;
+}
drivers/gpu/drm/vkms/vkms_crtc.c +22 -5
index 26babb85ca77..a7b39627b581 100644
@@ -10,17 +10,32 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 
-static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
+static void _vblank_handle(struct vkms_output *output)
 {
-	struct vkms_output *output = container_of(timer, struct vkms_output,
-						  vblank_hrtimer);
 	struct drm_crtc *crtc = &output->crtc;
-	int ret_overrun;
+	struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state);
 	bool ret;
 
+	spin_lock(&output->lock);
 	ret = drm_crtc_handle_vblank(crtc);
 	if (!ret)
-		DRM_ERROR("vkms failure on handling vblank");
+		DRM_ERROR("vkms failure on handling vblank\n");
+
+	if (state && output->crc_enabled) {
+		state->n_frame = drm_crtc_accurate_vblank_count(crtc);
+		queue_work(output->crc_workq, &state->crc_work);
+	}
+
+	spin_unlock(&output->lock);
+}
+
+static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
+{
+	struct vkms_output *output = container_of(timer, struct vkms_output,
+						  vblank_hrtimer);
+	int ret_overrun;
+
+	_vblank_handle(output);
 
 	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
 					  output->period_ns);
@@ -97,6 +112,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;
 }
 

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

Quoted 8 lines
drivers/gpu/drm/vkms/vkms_crtc.c +4 -1
@@ -108,7 +125,13 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
 	vkms_state = to_vkms_crtc_state(state);
 
 	__drm_atomic_helper_crtc_destroy_state(state);
-	kfree(vkms_state);
+
+	if (vkms_state) {
+		flush_work(&vkms_state->crc_work);
+		drm_framebuffer_put(&vkms_state->data.fb);

It'd be nice to leave a breadcrumb explaining where this reference is acquired.
ie: /* dropping the reference we acquired in vkms_primary_plane_update() */

Quoted 15 lines
drivers/gpu/drm/vkms/vkms_crtc.c +4 -0
+		memset(&vkms_state->data, 0, sizeof(struct vkms_crc_data));
+		kfree(vkms_state);
+	}
 }
 
 static const struct drm_crtc_funcs vkms_crtc_funcs = {
@@ -120,6 +143,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 static void vkms_crtc_atomic_disable(struct drm_crtc *crtc, · line 166

Quoted 9 lines
drivers/gpu/drm/vkms/vkms_crtc.c +6 -0
@@ -134,26 +158,37 @@ static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,
 	drm_crtc_vblank_off(crtc);
 }
 
+static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
+				   struct drm_crtc_state *old_crtc_state)
+{
+	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
+
+	spin_lock_irq(&vkms_output->lock);

Can you please add a comment explaining that we're holding this across the commit to block the vblank timer, and why.

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

Quoted 11 lines
drivers/gpu/drm/vkms/vkms_crtc.c +4 -2
+}
+
 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);
 
 	if (crtc->state->event) {
-		spin_lock_irqsave(&crtc->dev->event_lock, flags);
+		spin_lock(&crtc->dev->event_lock);

Either don't change this (since it's always safe to call _irqsave), or add a comment explaining why you don't need to.

Quoted 108 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 +197,7 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
 int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		   struct drm_plane *primary, struct drm_plane *cursor)
 {
+	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
 	int ret;
 
 	ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor,
@@ -173,5 +209,9 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 
 	drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
 
+	spin_lock_init(&vkms_out->lock);
+
+	vkms_out->crc_workq = alloc_ordered_workqueue("vkms_crc_workq", 0);
+
 	return ret;
 }
drivers/gpu/drm/vkms/vkms_drv.c +1 -0
index 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 +21 -0
index 75e52d61e65d..a80281a12010 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 {
@@ -35,6 +46,11 @@ struct vkms_output {
 	struct hrtimer vblank_hrtimer;
 	ktime_t period_ns;
 	struct drm_pending_vblank_event *event;
+	bool crc_enabled;
+	/* ordered wq for crc_work */
+	struct workqueue_struct *crc_workq;
+	/* protects concurrent access to crc_data */
+	spinlock_t lock;
 };
 
 struct vkms_device {
@@ -95,4 +111,9 @@ int vkms_gem_vmap(struct drm_gem_object *obj);
 
 void vkms_gem_vunmap(struct drm_gem_object *obj);
 
+/* CRC Support */
+int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
+			size_t *values_cnt);
+void vkms_crc_work_handle(struct work_struct *work);
+
 #endif /* _VKMS_DRV_H_ */
drivers/gpu/drm/vkms/vkms_plane.c +0 -0
index 5b7c5d59f103..b316b2af88ee 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)
 {

drivers/gpu/drm/vkms/vkms_plane.c static const struct drm_plane_funcs vkms_plane_funcs = { · line 36

Quoted 10 lines
drivers/gpu/drm/vkms/vkms_plane.c +10 -0
+	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);

Similarly to above, a comment explaining this is released in state_destroy would be helpful.

Quoted 6 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 · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel