[PATCH v2 0/5] Implement CRC and Add necessary infrastructure

[PATCH v2 0/5] Implement CRC and Add necessary infrastructure

[Haneen Mohammed]

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_crtc.c  | 111 +++++++++++++++++++++++++++---
 drivers/gpu/drm/vkms/vkms_drv.c   |   1 +
 drivers/gpu/drm/vkms/vkms_drv.h   |  41 +++++++++++
 drivers/gpu/drm/vkms/vkms_gem.c   |  73 +++++++++++++++++++-
 drivers/gpu/drm/vkms/vkms_plane.c |  75 +++++++++++++++++++-
 6 files changed, 290 insertions(+), 13 deletions(-)

-- 
2.17.1

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

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

[Haneen Mohammed]

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

Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
---
Changes in v2:
- add vkms_gem_vunmap
- use vmap_count to guard against multiple prepare_fb calls on the same
  fb

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

diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 07be29f2dc44..855e1ea8bc35 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -39,6 +39,8 @@ struct vkms_gem_object {
 	struct drm_gem_object gem;
 	struct mutex pages_lock; /* Page lock used in page fault handler */
 	struct page **pages;
+	unsigned int vmap_count;
+	void *vaddr;
 };
 
 #define drm_crtc_to_vkms_output(target) \
@@ -47,6 +49,9 @@ struct vkms_gem_object {
 #define drm_device_to_vkms_device(target) \
 	container_of(target, struct vkms_device, drm)
 
+#define drm_gem_to_vkms_gem(target)\
+	container_of(target, struct vkms_gem_object, gem)
+
 /* CRTC */
 int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		   struct drm_plane *primary, struct drm_plane *cursor);
@@ -75,4 +80,8 @@ int vkms_dumb_map(struct drm_file *file, struct drm_device *dev,
 
 void vkms_gem_free_object(struct drm_gem_object *obj);
 
+void *vkms_gem_vmap(struct drm_gem_object *obj);
+
+void vkms_gem_vunmap(struct drm_gem_object *obj);
+
 #endif /* _VKMS_DRV_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_gem.c b/drivers/gpu/drm/vkms/vkms_gem.c
index c7e38368602b..3f574e761b0f 100644
--- a/drivers/gpu/drm/vkms/vkms_gem.c
+++ b/drivers/gpu/drm/vkms/vkms_gem.c
@@ -37,7 +37,9 @@ void vkms_gem_free_object(struct drm_gem_object *obj)
 	struct vkms_gem_object *gem = container_of(obj, struct vkms_gem_object,
 						   gem);
 
-	kvfree(gem->pages);
+	WARN_ON(gem->pages);
+	WARN_ON(gem->vaddr);
+
 	mutex_destroy(&gem->pages_lock);
 	drm_gem_object_release(obj);
 	kfree(gem);
@@ -177,3 +179,72 @@ int vkms_dumb_map(struct drm_file *file, struct drm_device *dev,
 
 	return ret;
 }
+
+static struct page **_get_pages(struct vkms_gem_object *vkms_obj)
+{
+	struct drm_gem_object *gem_obj = &vkms_obj->gem;
+
+	if (!vkms_obj->pages) {
+		struct page **pages = drm_gem_get_pages(gem_obj);
+
+		if (IS_ERR(pages))
+			return pages;
+
+		if (cmpxchg(&vkms_obj->pages, NULL, pages))
+			drm_gem_put_pages(gem_obj, pages, false, true);
+	}
+
+	return vkms_obj->pages;
+}
+
+void vkms_gem_vunmap(struct drm_gem_object *obj)
+{
+	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(obj);
+
+	mutex_lock(&vkms_obj->pages_lock);
+	WARN_ON(vkms_obj->vmap_count < 1);
+	vkms_obj->vmap_count--;
+
+	if (vkms_obj->vmap_count == 0) {
+		vunmap(vkms_obj->vaddr);
+		vkms_obj->vaddr = NULL;
+		drm_gem_put_pages(obj, vkms_obj->pages, false, true);
+		vkms_obj->pages = NULL;
+	}
+
+	mutex_unlock(&vkms_obj->pages_lock);
+}
+
+void *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);
+	vkms_obj->vmap_count++;
+
+	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;
+			goto fail;
+		}
+	}
+
+	mutex_unlock(&vkms_obj->pages_lock);
+	return vkms_obj->vaddr;
+
+fail:
+	vkms_obj->vmap_count--;
+	mutex_unlock(&vkms_obj->pages_lock);
+	return ERR_PTR(ret);
+}
-- 
2.17.1

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

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

[Sean Paul]

On Sat, Jul 14, 2018 at 03:18:55PM +0300, Haneen Mohammed wrote:
> This patch add the necessary functions to map/unmap GEM
> backing memory to the kernel's virtual address space.
> 
> Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
> ---
> Changes in v2:
> - add vkms_gem_vunmap
> - use vmap_count to guard against multiple prepare_fb calls on the same
>   fb
> 
>  drivers/gpu/drm/vkms/vkms_drv.h |  9 ++++
>  drivers/gpu/drm/vkms/vkms_gem.c | 73 ++++++++++++++++++++++++++++++++-
>  2 files changed, 81 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 07be29f2dc44..855e1ea8bc35 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -39,6 +39,8 @@ struct vkms_gem_object {
>  	struct drm_gem_object gem;
>  	struct mutex pages_lock; /* Page lock used in page fault handler */
>  	struct page **pages;
> +	unsigned int vmap_count;
> +	void *vaddr;
>  };
>  
>  #define drm_crtc_to_vkms_output(target) \
> @@ -47,6 +49,9 @@ struct vkms_gem_object {
>  #define drm_device_to_vkms_device(target) \
>  	container_of(target, struct vkms_device, drm)
>  
> +#define drm_gem_to_vkms_gem(target)\
> +	container_of(target, struct vkms_gem_object, gem)
> +
>  /* CRTC */
>  int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>  		   struct drm_plane *primary, struct drm_plane *cursor);
> @@ -75,4 +80,8 @@ int vkms_dumb_map(struct drm_file *file, struct drm_device *dev,
>  
>  void vkms_gem_free_object(struct drm_gem_object *obj);
>  
> +void *vkms_gem_vmap(struct drm_gem_object *obj);
> +
> +void vkms_gem_vunmap(struct drm_gem_object *obj);
> +
>  #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_gem.c b/drivers/gpu/drm/vkms/vkms_gem.c
> index c7e38368602b..3f574e761b0f 100644
> --- a/drivers/gpu/drm/vkms/vkms_gem.c
> +++ b/drivers/gpu/drm/vkms/vkms_gem.c
> @@ -37,7 +37,9 @@ void vkms_gem_free_object(struct drm_gem_object *obj)
>  	struct vkms_gem_object *gem = container_of(obj, struct vkms_gem_object,
>  						   gem);
>  
> -	kvfree(gem->pages);
> +	WARN_ON(gem->pages);
> +	WARN_ON(gem->vaddr);
> +
>  	mutex_destroy(&gem->pages_lock);
>  	drm_gem_object_release(obj);
>  	kfree(gem);
> @@ -177,3 +179,72 @@ int vkms_dumb_map(struct drm_file *file, struct drm_device *dev,
>  
>  	return ret;
>  }
> +
> +static struct page **_get_pages(struct vkms_gem_object *vkms_obj)
> +{
> +	struct drm_gem_object *gem_obj = &vkms_obj->gem;
> +
> +	if (!vkms_obj->pages) {
> +		struct page **pages = drm_gem_get_pages(gem_obj);
> +
> +		if (IS_ERR(pages))
> +			return pages;
> +
> +		if (cmpxchg(&vkms_obj->pages, NULL, pages))
> +			drm_gem_put_pages(gem_obj, pages, false, true);
> +	}
> +
> +	return vkms_obj->pages;
> +}
> +
> +void vkms_gem_vunmap(struct drm_gem_object *obj)
> +{
> +	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(obj);
> +
> +	mutex_lock(&vkms_obj->pages_lock);
> +	WARN_ON(vkms_obj->vmap_count < 1);
> +	vkms_obj->vmap_count--;
> +
> +	if (vkms_obj->vmap_count == 0) {
> +		vunmap(vkms_obj->vaddr);
> +		vkms_obj->vaddr = NULL;
> +		drm_gem_put_pages(obj, vkms_obj->pages, false, true);
> +		vkms_obj->pages = NULL;
> +	}
> +
> +	mutex_unlock(&vkms_obj->pages_lock);
> +}
> +
> +void *vkms_gem_vmap(struct drm_gem_object *obj)
> +{
> +	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(obj);
> +

nit: extra line

> +	int ret = 0;
> +
> +	mutex_lock(&vkms_obj->pages_lock);
> +	vkms_obj->vmap_count++;

If you increment this below the conditional block, you don't need to decrement
it in fail.

> +
> +	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) {

Do you need to cleanup from _get_pages() here?

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[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.

Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
---
Changes in v2:
- use vkms_gem_vunmap

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

diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 9f75b1e2c1c4..877483984897 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -7,8 +7,9 @@
  */
 
 #include "vkms_drv.h"
-#include <drm/drm_plane_helper.h>
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_plane_helper.h>
 
 static const struct drm_plane_funcs vkms_plane_funcs = {
 	.update_plane		= drm_atomic_helper_update_plane,
@@ -24,8 +25,41 @@ static void vkms_primary_plane_update(struct drm_plane *plane,
 {
 }
 
+static int vkms_prepare_fb(struct drm_plane *plane,
+			   struct drm_plane_state *state)
+{
+	struct drm_gem_object *gem_obj;
+	struct vkms_gem_object *vkms_obj;
+
+	if (!state->fb)
+		return 0;
+
+	gem_obj = drm_gem_fb_get_obj(state->fb, 0);
+	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
+	vkms_obj->vaddr = vkms_gem_vmap(gem_obj);
+
+	if (!vkms_obj->vaddr)
+		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)
-- 
2.17.1

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

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

[Sean Paul]

On Sat, Jul 14, 2018 at 03:20:21PM +0300, Haneen Mohammed wrote:
> This patch map/unmap GEM backing memory to kernel address space
> in prepare/cleanup_fb respectively and cache the virtual address
> for later use.
> 
> Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
> ---
> Changes in v2:
> - use vkms_gem_vunmap
> 
>  drivers/gpu/drm/vkms/vkms_plane.c | 36 ++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 9f75b1e2c1c4..877483984897 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -7,8 +7,9 @@
>   */
>  
>  #include "vkms_drv.h"
> -#include <drm/drm_plane_helper.h>

nit: This seems unrelated to the patch

>  #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_plane_helper.h>
>  
>  static const struct drm_plane_funcs vkms_plane_funcs = {
>  	.update_plane		= drm_atomic_helper_update_plane,
> @@ -24,8 +25,41 @@ static void vkms_primary_plane_update(struct drm_plane *plane,
>  {
>  }
>  
> +static int vkms_prepare_fb(struct drm_plane *plane,
> +			   struct drm_plane_state *state)
> +{
> +	struct drm_gem_object *gem_obj;
> +	struct vkms_gem_object *vkms_obj;
> +
> +	if (!state->fb)
> +		return 0;
> +
> +	gem_obj = drm_gem_fb_get_obj(state->fb, 0);
> +	vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> +	vkms_obj->vaddr = vkms_gem_vmap(gem_obj);
> +
> +	if (!vkms_obj->vaddr)

- vkms_gem_vmap() returns ERR_PTR(), not NULL. So IS_ERR_OR_NULL would be
  more appropriate
- You shouldn't be assigning the return of vkms_gem_vmap() to vkms_obj->vaddr
  since vkms_gem_vmap() already does this internally (with proper locks and
  checks)

So you should probably change vkms_gem_vmap() to return int and accept a
vkms_obj upon which it'll populate vaddr for you.

Sean

> +		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)
> -- 
> 2.17.1
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/5] drm/vkms: Add atomic_helper_check_plane_state

[Haneen Mohammed]

Call atomic_helper_check_plane_state to clip plane coordinates.

Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
---
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(+)

diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 877483984897..aaf7c35faed6 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -7,6 +7,7 @@
  */
 
 #include "vkms_drv.h"
+#include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_plane_helper.h>
@@ -25,6 +26,33 @@ static void vkms_primary_plane_update(struct drm_plane *plane,
 {
 }
 
+static int vkms_plane_atomic_check(struct drm_plane *plane,
+				   struct drm_plane_state *state)
+{
+	struct drm_crtc_state *crtc_state;
+	int ret;
+
+	if (!state->fb | !state->crtc)
+		return 0;
+
+	crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc);
+	if (IS_ERR(crtc_state))
+		return PTR_ERR(crtc_state);
+
+	ret = drm_atomic_helper_check_plane_state(state, crtc_state,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  false, true);
+	if (ret != 0)
+		return ret;
+
+	/* for now primary plane must be visible and full screen */
+	if (!state->visible)
+		return -EINVAL;
+
+	return 0;
+}
+
 static int vkms_prepare_fb(struct drm_plane *plane,
 			   struct drm_plane_state *state)
 {
@@ -58,6 +86,7 @@ static void vkms_cleanup_fb(struct drm_plane *plane,
 
 static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
 	.atomic_update		= vkms_primary_plane_update,
+	.atomic_check		= vkms_plane_atomic_check,
 	.prepare_fb		= vkms_prepare_fb,
 	.cleanup_fb		= vkms_cleanup_fb,
 };
-- 
2.17.1

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

Re: [PATCH v2 3/5] drm/vkms: Add atomic_helper_check_plane_state

[Sean Paul]

On Sat, Jul 14, 2018 at 03:21:13PM +0300, Haneen Mohammed wrote:
> Call atomic_helper_check_plane_state to clip plane coordinates.
> 
> Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
> 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(+)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 877483984897..aaf7c35faed6 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include "vkms_drv.h"
> +#include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_plane_helper.h>
> @@ -25,6 +26,33 @@ static void vkms_primary_plane_update(struct drm_plane *plane,
>  {
>  }
>  
> +static int vkms_plane_atomic_check(struct drm_plane *plane,
> +				   struct drm_plane_state *state)
> +{
> +	struct drm_crtc_state *crtc_state;
> +	int ret;
> +
> +	if (!state->fb | !state->crtc)
> +		return 0;
> +
> +	crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc);
> +	if (IS_ERR(crtc_state))
> +		return PTR_ERR(crtc_state);
> +
> +	ret = drm_atomic_helper_check_plane_state(state, crtc_state,
> +						  DRM_PLANE_HELPER_NO_SCALING,
> +						  DRM_PLANE_HELPER_NO_SCALING,
> +						  false, true);
> +	if (ret != 0)
> +		return ret;
> +
> +	/* for now primary plane must be visible and full screen */
> +	if (!state->visible)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static int vkms_prepare_fb(struct drm_plane *plane,
>  			   struct drm_plane_state *state)
>  {
> @@ -58,6 +86,7 @@ static void vkms_cleanup_fb(struct drm_plane *plane,
>  
>  static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
>  	.atomic_update		= vkms_primary_plane_update,
> +	.atomic_check		= vkms_plane_atomic_check,
>  	.prepare_fb		= vkms_prepare_fb,
>  	.cleanup_fb		= vkms_cleanup_fb,
>  };
> -- 
> 2.17.1
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 4/5] drm/vkms: subclass CRTC state

[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.

Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/vkms/vkms_crtc.c | 53 ++++++++++++++++++++++++++++++--
 drivers/gpu/drm/vkms/vkms_drv.h  | 11 +++++++
 2 files changed, 61 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 875fca662ac0..26babb85ca77 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -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,
 };
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 855e1ea8bc35..bf811d0ec349 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -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);
-- 
2.17.1

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

Re: [PATCH v2 4/5] drm/vkms: subclass CRTC state

[Sean Paul]

On Sat, Jul 14, 2018 at 03:22:36PM +0300, Haneen Mohammed wrote:
> Subclass CRTC state struct to enable storing driver's private
> state. This patch only adds the base drm_crtc_state struct and
> the atomic functions that handle it.
> 
> Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
>  drivers/gpu/drm/vkms/vkms_crtc.c | 53 ++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/vkms/vkms_drv.h  | 11 +++++++
>  2 files changed, 61 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 875fca662ac0..26babb85ca77 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -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,
>  };
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 855e1ea8bc35..bf811d0ec349 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -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);
> -- 
> 2.17.1
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 5/5] 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 ordered workqueue to compute and add CRC at the end of a vblank.

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

Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
---
Changes in v2:
- Include this patch in this patchset.

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

diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
index 986297da51bf..37966914f70b 100644
--- a/drivers/gpu/drm/vkms/Makefile
+++ b/drivers/gpu/drm/vkms/Makefile
@@ -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
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 26babb85ca77..f3a674db33b8 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -10,18 +10,36 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 
-static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
+static void _vblank_handle(struct vkms_output *output)
 {
-	struct vkms_output *output = container_of(timer, struct vkms_output,
-						  vblank_hrtimer);
 	struct drm_crtc *crtc = &output->crtc;
-	int ret_overrun;
+	struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state);
 	bool ret;
 
+	int crc_enabled = 0;
+
+	spin_lock(&output->lock);
+	crc_enabled = output->crc_enabled;
 	ret = drm_crtc_handle_vblank(crtc);
 	if (!ret)
 		DRM_ERROR("vkms failure on handling vblank");
 
+	if (state && crc_enabled) {
+		state->n_frame = drm_crtc_accurate_vblank_count(crtc);
+		queue_work(output->crc_workq, &state->crc_work);
+	}
+
+	spin_unlock(&output->lock);
+}
+
+static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
+{
+	struct vkms_output *output = container_of(timer, struct vkms_output,
+						  vblank_hrtimer);
+	int ret_overrun;
+
+	_vblank_handle(output);
+
 	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
 					  output->period_ns);
 
@@ -97,6 +115,8 @@ vkms_atomic_crtc_duplicate_state(struct drm_crtc *crtc)
 
 	__drm_atomic_helper_crtc_duplicate_state(crtc, &vkms_state->base);
 
+	INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
+
 	return &vkms_state->base;
 }
 
@@ -104,11 +124,18 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
 					   struct drm_crtc_state *state)
 {
 	struct vkms_crtc_state *vkms_state;
+	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
 
 	vkms_state = to_vkms_crtc_state(state);
 
 	__drm_atomic_helper_crtc_destroy_state(state);
-	kfree(vkms_state);
+
+	if (vkms_state) {
+		flush_workqueue(vkms_out->crc_workq);
+		drm_framebuffer_put(&vkms_state->data.fb);
+		memset(&vkms_state->data, 0, sizeof(struct vkms_crc_data));
+		kfree(vkms_state);
+	}
 }
 
 static const struct drm_crtc_funcs vkms_crtc_funcs = {
@@ -120,6 +147,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
 	.atomic_destroy_state   = vkms_atomic_crtc_destroy_state,
 	.enable_vblank		= vkms_enable_vblank,
 	.disable_vblank		= vkms_disable_vblank,
+	.set_crc_source		= vkms_set_crc_source,
 };
 
 static void vkms_crtc_atomic_enable(struct drm_crtc *crtc,
@@ -134,26 +162,37 @@ static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,
 	drm_crtc_vblank_off(crtc);
 }
 
+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);
+}
+
 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);
 
 		if (drm_crtc_vblank_get(crtc) != 0)
 			drm_crtc_send_vblank_event(crtc, crtc->state->event);
 		else
 			drm_crtc_arm_vblank_event(crtc, crtc->state->event);
 
-		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
+		spin_unlock(&crtc->dev->event_lock);
 
 		crtc->state->event = NULL;
 	}
+
+	spin_unlock_irq(&vkms_output->lock);
 }
 
 static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
+	.atomic_begin	= vkms_crtc_atomic_begin,
 	.atomic_flush	= vkms_crtc_atomic_flush,
 	.atomic_enable	= vkms_crtc_atomic_enable,
 	.atomic_disable	= vkms_crtc_atomic_disable,
@@ -162,6 +201,7 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
 int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		   struct drm_plane *primary, struct drm_plane *cursor)
 {
+	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
 	int ret;
 
 	ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor,
@@ -173,5 +213,9 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 
 	drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
 
+	spin_lock_init(&vkms_out->lock);
+
+	vkms_out->crc_workq = alloc_ordered_workqueue("vkms_crc_workq", 0);
+
 	return ret;
 }
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 37aa2ef33b21..5d78bd97e69c 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -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 = {
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index bf811d0ec349..95985649768c 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -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 @@ void *vkms_gem_vmap(struct drm_gem_object *obj);
 
 void vkms_gem_vunmap(struct drm_gem_object *obj);
 
+/* CRC Support */
+int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
+			size_t *values_cnt);
+void vkms_crc_work_handle(struct work_struct *work);
+
 #endif /* _VKMS_DRV_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index aaf7c35faed6..1e0ee8ca8bd6 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -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,
-- 
2.17.1

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

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

[Sean Paul]

On Sat, Jul 14, 2018 at 03:23:32PM +0300, Haneen Mohammed wrote:
> Implement the set_crc_source() callback.
> Compute CRC using crc32 on the visible part of the framebuffer.
> Use ordered workqueue to compute and add CRC at the end of a vblank.
> 
> Use appropriate synchronization methods since the crc computation must
> be atomic wrt the generated vblank event for a given atomic update,
> 
> Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>

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

> ---
> Changes in v2:
> - Include this patch in this patchset.
> 
>  drivers/gpu/drm/vkms/Makefile     |  2 +-
>  drivers/gpu/drm/vkms/vkms_crtc.c  | 60 ++++++++++++++++++++++++++-----
>  drivers/gpu/drm/vkms/vkms_drv.c   |  1 +
>  drivers/gpu/drm/vkms/vkms_drv.h   | 21 +++++++++++
>  drivers/gpu/drm/vkms/vkms_plane.c | 10 ++++++
>  5 files changed, 85 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> index 986297da51bf..37966914f70b 100644
> --- a/drivers/gpu/drm/vkms/Makefile
> +++ b/drivers/gpu/drm/vkms/Makefile
> @@ -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
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 26babb85ca77..f3a674db33b8 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -10,18 +10,36 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc_helper.h>
>  
> -static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> +static void _vblank_handle(struct vkms_output *output)
>  {
> -	struct vkms_output *output = container_of(timer, struct vkms_output,
> -						  vblank_hrtimer);
>  	struct drm_crtc *crtc = &output->crtc;
> -	int ret_overrun;
> +	struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state);
>  	bool ret;
>  
> +	int crc_enabled = 0;
> +
> +	spin_lock(&output->lock);
> +	crc_enabled = output->crc_enabled;

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

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

This would be more useful with the error code printed.

>  
> +	if (state && crc_enabled) {
> +		state->n_frame = drm_crtc_accurate_vblank_count(crtc);
> +		queue_work(output->crc_workq, &state->crc_work);
> +	}
> +
> +	spin_unlock(&output->lock);
> +}
> +
> +static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> +{
> +	struct vkms_output *output = container_of(timer, struct vkms_output,
> +						  vblank_hrtimer);
> +	int ret_overrun;
> +
> +	_vblank_handle(output);
> +
>  	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
>  					  output->period_ns);
>  
> @@ -97,6 +115,8 @@ vkms_atomic_crtc_duplicate_state(struct drm_crtc *crtc)
>  
>  	__drm_atomic_helper_crtc_duplicate_state(crtc, &vkms_state->base);
>  
> +	INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
> +
>  	return &vkms_state->base;
>  }
>  
> @@ -104,11 +124,18 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
>  					   struct drm_crtc_state *state)
>  {
>  	struct vkms_crtc_state *vkms_state;
> +	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
>  
>  	vkms_state = to_vkms_crtc_state(state);
>  
>  	__drm_atomic_helper_crtc_destroy_state(state);
> -	kfree(vkms_state);
> +
> +	if (vkms_state) {
> +		flush_workqueue(vkms_out->crc_workq);

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

> +		drm_framebuffer_put(&vkms_state->data.fb);
> +		memset(&vkms_state->data, 0, sizeof(struct vkms_crc_data));
> +		kfree(vkms_state);
> +	}
>  }
>  
>  static const struct drm_crtc_funcs vkms_crtc_funcs = {
> @@ -120,6 +147,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
>  	.atomic_destroy_state   = vkms_atomic_crtc_destroy_state,
>  	.enable_vblank		= vkms_enable_vblank,
>  	.disable_vblank		= vkms_disable_vblank,
> +	.set_crc_source		= vkms_set_crc_source,
>  };
>  
>  static void vkms_crtc_atomic_enable(struct drm_crtc *crtc,
> @@ -134,26 +162,37 @@ static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,
>  	drm_crtc_vblank_off(crtc);
>  }
>  
> +static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
> +				   struct drm_crtc_state *old_crtc_state)
> +{
> +	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
> +
> +	spin_lock_irq(&vkms_output->lock);

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

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

What's the rationale behind this change?

>  
>  		if (drm_crtc_vblank_get(crtc) != 0)
>  			drm_crtc_send_vblank_event(crtc, crtc->state->event);
>  		else
>  			drm_crtc_arm_vblank_event(crtc, crtc->state->event);
>  
> -		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> +		spin_unlock(&crtc->dev->event_lock);
>  
>  		crtc->state->event = NULL;
>  	}
> +
> +	spin_unlock_irq(&vkms_output->lock);
>  }
>  
>  static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
> +	.atomic_begin	= vkms_crtc_atomic_begin,
>  	.atomic_flush	= vkms_crtc_atomic_flush,
>  	.atomic_enable	= vkms_crtc_atomic_enable,
>  	.atomic_disable	= vkms_crtc_atomic_disable,
> @@ -162,6 +201,7 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
>  int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>  		   struct drm_plane *primary, struct drm_plane *cursor)
>  {
> +	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
>  	int ret;
>  
>  	ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor,
> @@ -173,5 +213,9 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>  
>  	drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
>  
> +	spin_lock_init(&vkms_out->lock);
> +
> +	vkms_out->crc_workq = alloc_ordered_workqueue("vkms_crc_workq", 0);
> +
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 37aa2ef33b21..5d78bd97e69c 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -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 = {
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index bf811d0ec349..95985649768c 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -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;

Where is this set to true?

> +	/* ordered wq for crc_work */
> +	struct workqueue_struct *crc_workq;
> +	/* protects concurrent access to crc_data */
> +	spinlock_t lock;
>  };
>  
>  struct vkms_device {
> @@ -95,4 +111,9 @@ void *vkms_gem_vmap(struct drm_gem_object *obj);
>  
>  void vkms_gem_vunmap(struct drm_gem_object *obj);
>  
> +/* CRC Support */
> +int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
> +			size_t *values_cnt);
> +void vkms_crc_work_handle(struct work_struct *work);
> +
>  #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index aaf7c35faed6..1e0ee8ca8bd6 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -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,
> -- 
> 2.17.1
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Haneen Mohammed]

On Mon, Jul 16, 2018 at 02:22:56PM -0400, Sean Paul wrote:
> On Sat, Jul 14, 2018 at 03:23:32PM +0300, Haneen Mohammed wrote:
> > Implement the set_crc_source() callback.
> > Compute CRC using crc32 on the visible part of the framebuffer.
> > Use ordered workqueue to compute and add CRC at the end of a vblank.
> > 
> > Use appropriate synchronization methods since the crc computation must
> > be atomic wrt the generated vblank event for a given atomic update,
> > 
> > Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
> 
> Hey Haneen,
> Thanks for revising this patch set. Things are looking good across the series,
> just a few more comments :-)
> 

Thank you so much for the review! 

> > ---
> > Changes in v2:
> > - Include this patch in this patchset.
> > 
> >  drivers/gpu/drm/vkms/Makefile     |  2 +-
> >  drivers/gpu/drm/vkms/vkms_crtc.c  | 60 ++++++++++++++++++++++++++-----
> >  drivers/gpu/drm/vkms/vkms_drv.c   |  1 +
> >  drivers/gpu/drm/vkms/vkms_drv.h   | 21 +++++++++++
> >  drivers/gpu/drm/vkms/vkms_plane.c | 10 ++++++
> >  5 files changed, 85 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> > index 986297da51bf..37966914f70b 100644
> > --- a/drivers/gpu/drm/vkms/Makefile
> > +++ b/drivers/gpu/drm/vkms/Makefile
> > @@ -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
> > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > index 26babb85ca77..f3a674db33b8 100644
> > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > @@ -10,18 +10,36 @@
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_crtc_helper.h>
> >  
> > -static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> > +static void _vblank_handle(struct vkms_output *output)
> >  {
> > -	struct vkms_output *output = container_of(timer, struct vkms_output,
> > -						  vblank_hrtimer);
> >  	struct drm_crtc *crtc = &output->crtc;
> > -	int ret_overrun;
> > +	struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state);
> >  	bool ret;
> >  
> > +	int crc_enabled = 0;
> > +
> > +	spin_lock(&output->lock);
> > +	crc_enabled = output->crc_enabled;
> 
> Aside from the implicit bool -> int cast, I don't think you need this local var,
> just use output->crc_enabled directly below.
> 
> >  	ret = drm_crtc_handle_vblank(crtc);
> >  	if (!ret)
> >  		DRM_ERROR("vkms failure on handling vblank");
> 
> This would be more useful with the error code printed.
> 

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

> >  
> > +	if (state && crc_enabled) {
> > +		state->n_frame = drm_crtc_accurate_vblank_count(crtc);
> > +		queue_work(output->crc_workq, &state->crc_work);
> > +	}
> > +
> > +	spin_unlock(&output->lock);
> > +}
> > +
> > +static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> > +{
> > +	struct vkms_output *output = container_of(timer, struct vkms_output,
> > +						  vblank_hrtimer);
> > +	int ret_overrun;
> > +
> > +	_vblank_handle(output);
> > +
> >  	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
> >  					  output->period_ns);
> >  
> > @@ -97,6 +115,8 @@ vkms_atomic_crtc_duplicate_state(struct drm_crtc *crtc)
> >  
> >  	__drm_atomic_helper_crtc_duplicate_state(crtc, &vkms_state->base);
> >  
> > +	INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
> > +
> >  	return &vkms_state->base;
> >  }
> >  
> > @@ -104,11 +124,18 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
> >  					   struct drm_crtc_state *state)
> >  {
> >  	struct vkms_crtc_state *vkms_state;
> > +	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
> >  
> >  	vkms_state = to_vkms_crtc_state(state);
> >  
> >  	__drm_atomic_helper_crtc_destroy_state(state);
> > -	kfree(vkms_state);
> > +
> > +	if (vkms_state) {
> > +		flush_workqueue(vkms_out->crc_workq);
> 
> I'm a little worried about this bit. Since the workqueue is per-output, is it
> possible you'll be waiting for more frames to complete than you need to be?
> 

I see, maybe I should flush per work_struct instead?

> > +		drm_framebuffer_put(&vkms_state->data.fb);
> > +		memset(&vkms_state->data, 0, sizeof(struct vkms_crc_data));
> > +		kfree(vkms_state);
> > +	}
> >  }
> >  
> >  static const struct drm_crtc_funcs vkms_crtc_funcs = {
> > @@ -120,6 +147,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
> >  	.atomic_destroy_state   = vkms_atomic_crtc_destroy_state,
> >  	.enable_vblank		= vkms_enable_vblank,
> >  	.disable_vblank		= vkms_disable_vblank,
> > +	.set_crc_source		= vkms_set_crc_source,
> >  };
> >  
> >  static void vkms_crtc_atomic_enable(struct drm_crtc *crtc,
> > @@ -134,26 +162,37 @@ static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,
> >  	drm_crtc_vblank_off(crtc);
> >  }
> >  
> > +static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
> > +				   struct drm_crtc_state *old_crtc_state)
> > +{
> > +	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
> > +
> > +	spin_lock_irq(&vkms_output->lock);
> 
> Hmm, you can't lock across atomic_begin/flush. What is this protecting against?
> 

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

> > +}
> > +
> >  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);
> 
> What's the rationale behind this change?
> 

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

> >  
> >  		if (drm_crtc_vblank_get(crtc) != 0)
> >  			drm_crtc_send_vblank_event(crtc, crtc->state->event);
> >  		else
> >  			drm_crtc_arm_vblank_event(crtc, crtc->state->event);
> >  
> > -		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> > +		spin_unlock(&crtc->dev->event_lock);
> >  
> >  		crtc->state->event = NULL;
> >  	}
> > +
> > +	spin_unlock_irq(&vkms_output->lock);
> >  }
> >  
> >  static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
> > +	.atomic_begin	= vkms_crtc_atomic_begin,
> >  	.atomic_flush	= vkms_crtc_atomic_flush,
> >  	.atomic_enable	= vkms_crtc_atomic_enable,
> >  	.atomic_disable	= vkms_crtc_atomic_disable,
> > @@ -162,6 +201,7 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
> >  int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> >  		   struct drm_plane *primary, struct drm_plane *cursor)
> >  {
> > +	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
> >  	int ret;
> >  
> >  	ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor,
> > @@ -173,5 +213,9 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> >  
> >  	drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
> >  
> > +	spin_lock_init(&vkms_out->lock);
> > +
> > +	vkms_out->crc_workq = alloc_ordered_workqueue("vkms_crc_workq", 0);
> > +
> >  	return ret;
> >  }
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > index 37aa2ef33b21..5d78bd97e69c 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > @@ -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 = {
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > index bf811d0ec349..95985649768c 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -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;
> 
> Where is this set to true?
> 

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

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

> > +	/* ordered wq for crc_work */
> > +	struct workqueue_struct *crc_workq;
> > +	/* protects concurrent access to crc_data */
> > +	spinlock_t lock;
> >  };
> >  
> >  struct vkms_device {
> > @@ -95,4 +111,9 @@ void *vkms_gem_vmap(struct drm_gem_object *obj);
> >  
> >  void vkms_gem_vunmap(struct drm_gem_object *obj);
> >  
> > +/* CRC Support */
> > +int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
> > +			size_t *values_cnt);
> > +void vkms_crc_work_handle(struct work_struct *work);
> > +
> >  #endif /* _VKMS_DRV_H_ */
> > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > index aaf7c35faed6..1e0ee8ca8bd6 100644
> > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > @@ -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,
> > -- 
> > 2.17.1
> > 
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sean Paul]

On Tue, Jul 17, 2018 at 02:52:20AM +0300, Haneen Mohammed wrote:
> On Mon, Jul 16, 2018 at 02:22:56PM -0400, Sean Paul wrote:
> > On Sat, Jul 14, 2018 at 03:23:32PM +0300, Haneen Mohammed wrote:
> > > Implement the set_crc_source() callback.
> > > Compute CRC using crc32 on the visible part of the framebuffer.
> > > Use ordered workqueue to compute and add CRC at the end of a vblank.
> > > 
> > > Use appropriate synchronization methods since the crc computation must
> > > be atomic wrt the generated vblank event for a given atomic update,
> > > 
> > > Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
> > 
> > Hey Haneen,
> > Thanks for revising this patch set. Things are looking good across the series,
> > just a few more comments :-)
> > 
> 
> Thank you so much for the review! 
> 
> > > ---
> > > Changes in v2:
> > > - Include this patch in this patchset.
> > > 
> > >  drivers/gpu/drm/vkms/Makefile     |  2 +-
> > >  drivers/gpu/drm/vkms/vkms_crtc.c  | 60 ++++++++++++++++++++++++++-----
> > >  drivers/gpu/drm/vkms/vkms_drv.c   |  1 +
> > >  drivers/gpu/drm/vkms/vkms_drv.h   | 21 +++++++++++
> > >  drivers/gpu/drm/vkms/vkms_plane.c | 10 ++++++
> > >  5 files changed, 85 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> > > index 986297da51bf..37966914f70b 100644
> > > --- a/drivers/gpu/drm/vkms/Makefile
> > > +++ b/drivers/gpu/drm/vkms/Makefile
> > > @@ -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
> > > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > index 26babb85ca77..f3a674db33b8 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > @@ -10,18 +10,36 @@
> > >  #include <drm/drm_atomic_helper.h>
> > >  #include <drm/drm_crtc_helper.h>
> > >  
> > > -static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> > > +static void _vblank_handle(struct vkms_output *output)
> > >  {
> > > -	struct vkms_output *output = container_of(timer, struct vkms_output,
> > > -						  vblank_hrtimer);
> > >  	struct drm_crtc *crtc = &output->crtc;
> > > -	int ret_overrun;
> > > +	struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state);
> > >  	bool ret;
> > >  
> > > +	int crc_enabled = 0;
> > > +
> > > +	spin_lock(&output->lock);
> > > +	crc_enabled = output->crc_enabled;
> > 
> > Aside from the implicit bool -> int cast, I don't think you need this local var,
> > just use output->crc_enabled directly below.
> > 
> > >  	ret = drm_crtc_handle_vblank(crtc);
> > >  	if (!ret)
> > >  		DRM_ERROR("vkms failure on handling vblank");
> > 
> > This would be more useful with the error code printed.
> > 
> 
> I think this only returns false on failure. Also I've noticed most of the usage of
> drm_crtc_handle_vblank don't check the return value, should I do the
> same as well and drop ret and error message?

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

> 
> > >  
> > > +	if (state && crc_enabled) {
> > > +		state->n_frame = drm_crtc_accurate_vblank_count(crtc);
> > > +		queue_work(output->crc_workq, &state->crc_work);
> > > +	}
> > > +
> > > +	spin_unlock(&output->lock);
> > > +}
> > > +
> > > +static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> > > +{
> > > +	struct vkms_output *output = container_of(timer, struct vkms_output,
> > > +						  vblank_hrtimer);
> > > +	int ret_overrun;
> > > +
> > > +	_vblank_handle(output);
> > > +
> > >  	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
> > >  					  output->period_ns);
> > >  
> > > @@ -97,6 +115,8 @@ vkms_atomic_crtc_duplicate_state(struct drm_crtc *crtc)
> > >  
> > >  	__drm_atomic_helper_crtc_duplicate_state(crtc, &vkms_state->base);
> > >  
> > > +	INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
> > > +
> > >  	return &vkms_state->base;
> > >  }
> > >  
> > > @@ -104,11 +124,18 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
> > >  					   struct drm_crtc_state *state)
> > >  {
> > >  	struct vkms_crtc_state *vkms_state;
> > > +	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
> > >  
> > >  	vkms_state = to_vkms_crtc_state(state);
> > >  
> > >  	__drm_atomic_helper_crtc_destroy_state(state);
> > > -	kfree(vkms_state);
> > > +
> > > +	if (vkms_state) {
> > > +		flush_workqueue(vkms_out->crc_workq);
> > 
> > I'm a little worried about this bit. Since the workqueue is per-output, is it
> > possible you'll be waiting for more frames to complete than you need to be?
> > 
> 
> I see, maybe I should flush per work_struct instead?

Yeah, that would make more sense IMO.

> 
> > > +		drm_framebuffer_put(&vkms_state->data.fb);
> > > +		memset(&vkms_state->data, 0, sizeof(struct vkms_crc_data));
> > > +		kfree(vkms_state);
> > > +	}
> > >  }
> > >  
> > >  static const struct drm_crtc_funcs vkms_crtc_funcs = {
> > > @@ -120,6 +147,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
> > >  	.atomic_destroy_state   = vkms_atomic_crtc_destroy_state,
> > >  	.enable_vblank		= vkms_enable_vblank,
> > >  	.disable_vblank		= vkms_disable_vblank,
> > > +	.set_crc_source		= vkms_set_crc_source,
> > >  };
> > >  
> > >  static void vkms_crtc_atomic_enable(struct drm_crtc *crtc,
> > > @@ -134,26 +162,37 @@ static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,
> > >  	drm_crtc_vblank_off(crtc);
> > >  }
> > >  
> > > +static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
> > > +				   struct drm_crtc_state *old_crtc_state)
> > > +{
> > > +	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
> > > +
> > > +	spin_lock_irq(&vkms_output->lock);
> > 
> > Hmm, you can't lock across atomic_begin/flush. What is this protecting against?
> > 
> 
> I did this per Daniel recommendation:
> 1- spin_lock_irq -> vkms_crtc_atomic_begin
> 2- update vkms_crc_data and get ref to fb -> vkms_primary_plane_update
> 3- spin_unlock_irq after vblank event -> vkms_crtc_atomic_flush

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

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

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

Sean

> 
> > > +}
> > > +
> > >  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);
> > 
> > What's the rationale behind this change?
> > 
> 
> hm I am not sure if this is correct, but I assume because spin_lock_irq in atomic_begin
> would disable interrupts and since this is before we unlock vkms_output->lock with 
> spin_unlock_irq so irqsave here is not necessary?
> 

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

> > >  
> > >  		if (drm_crtc_vblank_get(crtc) != 0)
> > >  			drm_crtc_send_vblank_event(crtc, crtc->state->event);
> > >  		else
> > >  			drm_crtc_arm_vblank_event(crtc, crtc->state->event);
> > >  
> > > -		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> > > +		spin_unlock(&crtc->dev->event_lock);
> > >  
> > >  		crtc->state->event = NULL;
> > >  	}
> > > +
> > > +	spin_unlock_irq(&vkms_output->lock);
> > >  }
> > >  
> > >  static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
> > > +	.atomic_begin	= vkms_crtc_atomic_begin,
> > >  	.atomic_flush	= vkms_crtc_atomic_flush,
> > >  	.atomic_enable	= vkms_crtc_atomic_enable,
> > >  	.atomic_disable	= vkms_crtc_atomic_disable,
> > > @@ -162,6 +201,7 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
> > >  int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> > >  		   struct drm_plane *primary, struct drm_plane *cursor)
> > >  {
> > > +	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
> > >  	int ret;
> > >  
> > >  	ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor,
> > > @@ -173,5 +213,9 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> > >  
> > >  	drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
> > >  
> > > +	spin_lock_init(&vkms_out->lock);
> > > +
> > > +	vkms_out->crc_workq = alloc_ordered_workqueue("vkms_crc_workq", 0);
> > > +
> > >  	return ret;
> > >  }
> > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > index 37aa2ef33b21..5d78bd97e69c 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > @@ -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 = {
> > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > index bf811d0ec349..95985649768c 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > @@ -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;
> > 
> > Where is this set to true?
> > 
> 
> Oh sorry I forgot to add a vkms_crc.c which sets crc_enabled in
> set_crc_source callback! 
> 
> I'll work on fixing the issues you mentioned here and the other patches
> as well, thank you so much again!

Thanks for working through this!

Sean

> Haneen
> 
> > > +	/* ordered wq for crc_work */
> > > +	struct workqueue_struct *crc_workq;
> > > +	/* protects concurrent access to crc_data */
> > > +	spinlock_t lock;
> > >  };
> > >  
> > >  struct vkms_device {
> > > @@ -95,4 +111,9 @@ void *vkms_gem_vmap(struct drm_gem_object *obj);
> > >  
> > >  void vkms_gem_vunmap(struct drm_gem_object *obj);
> > >  
> > > +/* CRC Support */
> > > +int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
> > > +			size_t *values_cnt);
> > > +void vkms_crc_work_handle(struct work_struct *work);
> > > +
> > >  #endif /* _VKMS_DRV_H_ */
> > > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > > index aaf7c35faed6..1e0ee8ca8bd6 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > > @@ -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,
> > > -- 
> > > 2.17.1
> > > 
> > 
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sean Paul]

On Tue, Jul 17, 2018 at 04:43:16PM -0400, Sean Paul wrote:
> On Tue, Jul 17, 2018 at 02:52:20AM +0300, Haneen Mohammed wrote:
> > On Mon, Jul 16, 2018 at 02:22:56PM -0400, Sean Paul wrote:
> > > On Sat, Jul 14, 2018 at 03:23:32PM +0300, Haneen Mohammed wrote:
> > > > Implement the set_crc_source() callback.
> > > > Compute CRC using crc32 on the visible part of the framebuffer.
> > > > Use ordered workqueue to compute and add CRC at the end of a vblank.
> > > > 
> > > > Use appropriate synchronization methods since the crc computation must
> > > > be atomic wrt the generated vblank event for a given atomic update,
> > > > 
> > > > Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
> > > 
> > > Hey Haneen,
> > > Thanks for revising this patch set. Things are looking good across the series,
> > > just a few more comments :-)
> > > 
> > 
> > Thank you so much for the review! 
> > 
> > > > ---
> > > > Changes in v2:
> > > > - Include this patch in this patchset.
> > > > 
> > > >  drivers/gpu/drm/vkms/Makefile     |  2 +-
> > > >  drivers/gpu/drm/vkms/vkms_crtc.c  | 60 ++++++++++++++++++++++++++-----
> > > >  drivers/gpu/drm/vkms/vkms_drv.c   |  1 +
> > > >  drivers/gpu/drm/vkms/vkms_drv.h   | 21 +++++++++++
> > > >  drivers/gpu/drm/vkms/vkms_plane.c | 10 ++++++
> > > >  5 files changed, 85 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> > > > index 986297da51bf..37966914f70b 100644
> > > > --- a/drivers/gpu/drm/vkms/Makefile
> > > > +++ b/drivers/gpu/drm/vkms/Makefile
> > > > @@ -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
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > > index 26babb85ca77..f3a674db33b8 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > > > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > > @@ -10,18 +10,36 @@
> > > >  #include <drm/drm_atomic_helper.h>
> > > >  #include <drm/drm_crtc_helper.h>
> > > >  
> > > > -static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> > > > +static void _vblank_handle(struct vkms_output *output)
> > > >  {
> > > > -	struct vkms_output *output = container_of(timer, struct vkms_output,
> > > > -						  vblank_hrtimer);
> > > >  	struct drm_crtc *crtc = &output->crtc;
> > > > -	int ret_overrun;
> > > > +	struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state);
> > > >  	bool ret;
> > > >  
> > > > +	int crc_enabled = 0;
> > > > +
> > > > +	spin_lock(&output->lock);
> > > > +	crc_enabled = output->crc_enabled;
> > > 
> > > Aside from the implicit bool -> int cast, I don't think you need this local var,
> > > just use output->crc_enabled directly below.
> > > 
> > > >  	ret = drm_crtc_handle_vblank(crtc);
> > > >  	if (!ret)
> > > >  		DRM_ERROR("vkms failure on handling vblank");
> > > 
> > > This would be more useful with the error code printed.
> > > 
> > 
> > I think this only returns false on failure. Also I've noticed most of the usage of
> > drm_crtc_handle_vblank don't check the return value, should I do the
> > same as well and drop ret and error message?
> 
> Ahh, I didn't see that ret was bool. In that case, the error message is fine.
> It's always good practice to assume that if a function returns a status, it's
> worth checking.
> 
> > 
> > > >  
> > > > +	if (state && crc_enabled) {
> > > > +		state->n_frame = drm_crtc_accurate_vblank_count(crtc);
> > > > +		queue_work(output->crc_workq, &state->crc_work);
> > > > +	}
> > > > +
> > > > +	spin_unlock(&output->lock);
> > > > +}
> > > > +
> > > > +static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> > > > +{
> > > > +	struct vkms_output *output = container_of(timer, struct vkms_output,
> > > > +						  vblank_hrtimer);
> > > > +	int ret_overrun;
> > > > +
> > > > +	_vblank_handle(output);
> > > > +
> > > >  	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
> > > >  					  output->period_ns);
> > > >  
> > > > @@ -97,6 +115,8 @@ vkms_atomic_crtc_duplicate_state(struct drm_crtc *crtc)
> > > >  
> > > >  	__drm_atomic_helper_crtc_duplicate_state(crtc, &vkms_state->base);
> > > >  
> > > > +	INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
> > > > +
> > > >  	return &vkms_state->base;
> > > >  }
> > > >  
> > > > @@ -104,11 +124,18 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
> > > >  					   struct drm_crtc_state *state)
> > > >  {
> > > >  	struct vkms_crtc_state *vkms_state;
> > > > +	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
> > > >  
> > > >  	vkms_state = to_vkms_crtc_state(state);
> > > >  
> > > >  	__drm_atomic_helper_crtc_destroy_state(state);
> > > > -	kfree(vkms_state);
> > > > +
> > > > +	if (vkms_state) {
> > > > +		flush_workqueue(vkms_out->crc_workq);
> > > 
> > > I'm a little worried about this bit. Since the workqueue is per-output, is it
> > > possible you'll be waiting for more frames to complete than you need to be?
> > > 
> > 
> > I see, maybe I should flush per work_struct instead?
> 
> Yeah, that would make more sense IMO.
> 
> > 
> > > > +		drm_framebuffer_put(&vkms_state->data.fb);
> > > > +		memset(&vkms_state->data, 0, sizeof(struct vkms_crc_data));
> > > > +		kfree(vkms_state);
> > > > +	}
> > > >  }
> > > >  
> > > >  static const struct drm_crtc_funcs vkms_crtc_funcs = {
> > > > @@ -120,6 +147,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
> > > >  	.atomic_destroy_state   = vkms_atomic_crtc_destroy_state,
> > > >  	.enable_vblank		= vkms_enable_vblank,
> > > >  	.disable_vblank		= vkms_disable_vblank,
> > > > +	.set_crc_source		= vkms_set_crc_source,
> > > >  };
> > > >  
> > > >  static void vkms_crtc_atomic_enable(struct drm_crtc *crtc,
> > > > @@ -134,26 +162,37 @@ static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,
> > > >  	drm_crtc_vblank_off(crtc);
> > > >  }
> > > >  
> > > > +static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
> > > > +				   struct drm_crtc_state *old_crtc_state)
> > > > +{
> > > > +	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
> > > > +
> > > > +	spin_lock_irq(&vkms_output->lock);
> > > 
> > > Hmm, you can't lock across atomic_begin/flush. What is this protecting against?
> > > 
> > 
> > I did this per Daniel recommendation:
> > 1- spin_lock_irq -> vkms_crtc_atomic_begin
> > 2- update vkms_crc_data and get ref to fb -> vkms_primary_plane_update
> > 3- spin_unlock_irq after vblank event -> vkms_crtc_atomic_flush
> 
> I think I'm interpreting his mail a little differently. The point of the
> spinlock is to protect the pointer to the crc_data in vkms_crtc (right now it's
> not a pointer, but it should be).
> 
> So I think you need to do the following:
> - Change crc_data to a pointer
> - In plane_update, allocate new memory to hold crc_data and, under the spinlock,
>   update the crc_data to point to the new memory (add a WARN_ON if the pointer
>   is NULL)
> - In the hrtimer, while holding the spinlock, take the pointer from crc_data
>   into a local variable and clear the crtc->crc_data pointer
> - Pass the pointer (from the local variable) to the crc_worker
> 
> I don't think you need to hold the spinlock across the atomic hooks, just grab
> it in plane_update.

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

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

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

Sean

> 
> Sean
> 
> > 
> > > > +}
> > > > +
> > > >  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);
> > > 
> > > What's the rationale behind this change?
> > > 
> > 
> > hm I am not sure if this is correct, but I assume because spin_lock_irq in atomic_begin
> > would disable interrupts and since this is before we unlock vkms_output->lock with 
> > spin_unlock_irq so irqsave here is not necessary?
> > 
> 
> Once you limit the scope of the spinlock above, you will want to revert this
> change.
> 
> > > >  
> > > >  		if (drm_crtc_vblank_get(crtc) != 0)
> > > >  			drm_crtc_send_vblank_event(crtc, crtc->state->event);
> > > >  		else
> > > >  			drm_crtc_arm_vblank_event(crtc, crtc->state->event);
> > > >  
> > > > -		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> > > > +		spin_unlock(&crtc->dev->event_lock);
> > > >  
> > > >  		crtc->state->event = NULL;
> > > >  	}
> > > > +
> > > > +	spin_unlock_irq(&vkms_output->lock);
> > > >  }
> > > >  
> > > >  static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
> > > > +	.atomic_begin	= vkms_crtc_atomic_begin,
> > > >  	.atomic_flush	= vkms_crtc_atomic_flush,
> > > >  	.atomic_enable	= vkms_crtc_atomic_enable,
> > > >  	.atomic_disable	= vkms_crtc_atomic_disable,
> > > > @@ -162,6 +201,7 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
> > > >  int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> > > >  		   struct drm_plane *primary, struct drm_plane *cursor)
> > > >  {
> > > > +	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
> > > >  	int ret;
> > > >  
> > > >  	ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor,
> > > > @@ -173,5 +213,9 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> > > >  
> > > >  	drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
> > > >  
> > > > +	spin_lock_init(&vkms_out->lock);
> > > > +
> > > > +	vkms_out->crc_workq = alloc_ordered_workqueue("vkms_crc_workq", 0);
> > > > +
> > > >  	return ret;
> > > >  }
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > index 37aa2ef33b21..5d78bd97e69c 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > @@ -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 = {
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > > index bf811d0ec349..95985649768c 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > > @@ -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;
> > > 
> > > Where is this set to true?
> > > 
> > 
> > Oh sorry I forgot to add a vkms_crc.c which sets crc_enabled in
> > set_crc_source callback! 
> > 
> > I'll work on fixing the issues you mentioned here and the other patches
> > as well, thank you so much again!
> 
> Thanks for working through this!
> 
> Sean
> 
> > Haneen
> > 
> > > > +	/* ordered wq for crc_work */
> > > > +	struct workqueue_struct *crc_workq;
> > > > +	/* protects concurrent access to crc_data */
> > > > +	spinlock_t lock;
> > > >  };
> > > >  
> > > >  struct vkms_device {
> > > > @@ -95,4 +111,9 @@ void *vkms_gem_vmap(struct drm_gem_object *obj);
> > > >  
> > > >  void vkms_gem_vunmap(struct drm_gem_object *obj);
> > > >  
> > > > +/* CRC Support */
> > > > +int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
> > > > +			size_t *values_cnt);
> > > > +void vkms_crc_work_handle(struct work_struct *work);
> > > > +
> > > >  #endif /* _VKMS_DRV_H_ */
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > > > index aaf7c35faed6..1e0ee8ca8bd6 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > > > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > > > @@ -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,
> > > > -- 
> > > > 2.17.1
> > > > 
> > > 
> > > -- 
> > > Sean Paul, Software Engineer, Google / Chromium OS
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Haneen Mohammed]

First, just so you have the complete view, this is the missing part in this patch:
------ vkms_crc.c
static uint32_t _vkms_get_crc(struct vkms_crc_data *crc_data)
{
	struct drm_framebuffer *fb = &crc_data->fb;
	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(gem_obj);
	u32 crc = 0;
	int i = 0;
	unsigned int x = crc_data->src.x1 >> 16;
	unsigned int y = crc_data->src.y1 >> 16;
	unsigned int height = drm_rect_height(&crc_data->src) >> 16;
	unsigned int width = drm_rect_width(&crc_data->src) >> 16;
	unsigned int cpp = fb->format->cpp[0];
	unsigned int src_offset;
	unsigned int size_byte = width * cpp;
	void *vaddr = vkms_obj->vaddr;

	if (WARN_ON(!vaddr))
		return crc;

	for (i = y; i < y + height; i++) {
		src_offset = fb->offsets[0] + (i * fb->pitches[0]) + (x * cpp);
		crc = crc32_le(crc, vaddr + src_offset, size_byte);
	}

	return crc;
}

void vkms_crc_work_handle(struct work_struct *work)
{
	struct vkms_crtc_state *crtc_state = container_of(work,
						struct vkms_crtc_state,
						crc_work);
	struct drm_crtc *crtc = crtc_state->base.crtc;
	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);

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

		drm_crtc_add_crc_entry(crtc, true, n_frame, &crc32);
	}
}

int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
			size_t *values_cnt)
{
	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);

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

	*values_cnt = 1;

	return 0;
}
--- end vkms_crc.c

On Tue, Jul 17, 2018 at 05:05:03PM -0400, Sean Paul wrote:
> On Tue, Jul 17, 2018 at 04:43:16PM -0400, Sean Paul wrote:
> > On Tue, Jul 17, 2018 at 02:52:20AM +0300, Haneen Mohammed wrote:
> > > On Mon, Jul 16, 2018 at 02:22:56PM -0400, Sean Paul wrote:
> > > > On Sat, Jul 14, 2018 at 03:23:32PM +0300, Haneen Mohammed wrote:
> > > > > Implement the set_crc_source() callback.
> > > > > Compute CRC using crc32 on the visible part of the framebuffer.
> > > > > Use ordered workqueue to compute and add CRC at the end of a vblank.
> > > > > 
> > > > > Use appropriate synchronization methods since the crc computation must
> > > > > be atomic wrt the generated vblank event for a given atomic update,
> > > > > 
> > > > > Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
> > > > 
> > > > Hey Haneen,
> > > > Thanks for revising this patch set. Things are looking good across the series,
> > > > just a few more comments :-)
> > > > 
> > > 
> > > Thank you so much for the review! 
> > > 
> > > > > ---
> > > > > Changes in v2:
> > > > > - Include this patch in this patchset.
> > > > > 
> > > > >  drivers/gpu/drm/vkms/Makefile     |  2 +-
> > > > >  drivers/gpu/drm/vkms/vkms_crtc.c  | 60 ++++++++++++++++++++++++++-----
> > > > >  drivers/gpu/drm/vkms/vkms_drv.c   |  1 +
> > > > >  drivers/gpu/drm/vkms/vkms_drv.h   | 21 +++++++++++
> > > > >  drivers/gpu/drm/vkms/vkms_plane.c | 10 ++++++
> > > > >  5 files changed, 85 insertions(+), 9 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> > > > > index 986297da51bf..37966914f70b 100644
> > > > > --- a/drivers/gpu/drm/vkms/Makefile
> > > > > +++ b/drivers/gpu/drm/vkms/Makefile
> > > > > @@ -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
> > > > > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > > > index 26babb85ca77..f3a674db33b8 100644
> > > > > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > > > > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > > > @@ -10,18 +10,36 @@
> > > > >  #include <drm/drm_atomic_helper.h>
> > > > >  #include <drm/drm_crtc_helper.h>
> > > > >  
> > > > > -static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> > > > > +static void _vblank_handle(struct vkms_output *output)
> > > > >  {
> > > > > -	struct vkms_output *output = container_of(timer, struct vkms_output,
> > > > > -						  vblank_hrtimer);
> > > > >  	struct drm_crtc *crtc = &output->crtc;
> > > > > -	int ret_overrun;
> > > > > +	struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state);
> > > > >  	bool ret;
> > > > >  
> > > > > +	int crc_enabled = 0;
> > > > > +
> > > > > +	spin_lock(&output->lock);
> > > > > +	crc_enabled = output->crc_enabled;
> > > > 
> > > > Aside from the implicit bool -> int cast, I don't think you need this local var,
> > > > just use output->crc_enabled directly below.
> > > > 
> > > > >  	ret = drm_crtc_handle_vblank(crtc);
> > > > >  	if (!ret)
> > > > >  		DRM_ERROR("vkms failure on handling vblank");
> > > > 
> > > > This would be more useful with the error code printed.
> > > > 
> > > 
> > > I think this only returns false on failure. Also I've noticed most of the usage of
> > > drm_crtc_handle_vblank don't check the return value, should I do the
> > > same as well and drop ret and error message?
> > 
> > Ahh, I didn't see that ret was bool. In that case, the error message is fine.
> > It's always good practice to assume that if a function returns a status, it's
> > worth checking.
> > 
> > > 
> > > > >  
> > > > > +	if (state && crc_enabled) {
> > > > > +		state->n_frame = drm_crtc_accurate_vblank_count(crtc);
> > > > > +		queue_work(output->crc_workq, &state->crc_work);
> > > > > +	}
> > > > > +
> > > > > +	spin_unlock(&output->lock);
> > > > > +}
> > > > > +
> > > > > +static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> > > > > +{
> > > > > +	struct vkms_output *output = container_of(timer, struct vkms_output,
> > > > > +						  vblank_hrtimer);
> > > > > +	int ret_overrun;
> > > > > +
> > > > > +	_vblank_handle(output);
> > > > > +
> > > > >  	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
> > > > >  					  output->period_ns);
> > > > >  
> > > > > @@ -97,6 +115,8 @@ vkms_atomic_crtc_duplicate_state(struct drm_crtc *crtc)
> > > > >  
> > > > >  	__drm_atomic_helper_crtc_duplicate_state(crtc, &vkms_state->base);
> > > > >  
> > > > > +	INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
> > > > > +
> > > > >  	return &vkms_state->base;
> > > > >  }
> > > > >  
> > > > > @@ -104,11 +124,18 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
> > > > >  					   struct drm_crtc_state *state)
> > > > >  {
> > > > >  	struct vkms_crtc_state *vkms_state;
> > > > > +	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
> > > > >  
> > > > >  	vkms_state = to_vkms_crtc_state(state);
> > > > >  
> > > > >  	__drm_atomic_helper_crtc_destroy_state(state);
> > > > > -	kfree(vkms_state);
> > > > > +
> > > > > +	if (vkms_state) {
> > > > > +		flush_workqueue(vkms_out->crc_workq);
> > > > 
> > > > I'm a little worried about this bit. Since the workqueue is per-output, is it
> > > > possible you'll be waiting for more frames to complete than you need to be?
> > > > 
> > > 
> > > I see, maybe I should flush per work_struct instead?
> > 
> > Yeah, that would make more sense IMO.
> > 
> > > 
> > > > > +		drm_framebuffer_put(&vkms_state->data.fb);
> > > > > +		memset(&vkms_state->data, 0, sizeof(struct vkms_crc_data));
> > > > > +		kfree(vkms_state);
> > > > > +	}
> > > > >  }
> > > > >  
> > > > >  static const struct drm_crtc_funcs vkms_crtc_funcs = {
> > > > > @@ -120,6 +147,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
> > > > >  	.atomic_destroy_state   = vkms_atomic_crtc_destroy_state,
> > > > >  	.enable_vblank		= vkms_enable_vblank,
> > > > >  	.disable_vblank		= vkms_disable_vblank,
> > > > > +	.set_crc_source		= vkms_set_crc_source,
> > > > >  };
> > > > >  
> > > > >  static void vkms_crtc_atomic_enable(struct drm_crtc *crtc,
> > > > > @@ -134,26 +162,37 @@ static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,
> > > > >  	drm_crtc_vblank_off(crtc);
> > > > >  }
> > > > >  
> > > > > +static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
> > > > > +				   struct drm_crtc_state *old_crtc_state)
> > > > > +{
> > > > > +	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
> > > > > +
> > > > > +	spin_lock_irq(&vkms_output->lock);
> > > > 
> > > > Hmm, you can't lock across atomic_begin/flush. What is this protecting against?
> > > > 
> > > 
> > > I did this per Daniel recommendation:
> > > 1- spin_lock_irq -> vkms_crtc_atomic_begin
> > > 2- update vkms_crc_data and get ref to fb -> vkms_primary_plane_update
> > > 3- spin_unlock_irq after vblank event -> vkms_crtc_atomic_flush
> > 
> > I think I'm interpreting his mail a little differently. The point of the
> > spinlock is to protect the pointer to the crc_data in vkms_crtc (right now it's
> > not a pointer, but it should be).
> > 
> > So I think you need to do the following:
> > - Change crc_data to a pointer
> > - In plane_update, allocate new memory to hold crc_data and, under the spinlock,
> >   update the crc_data to point to the new memory (add a WARN_ON if the pointer
> >   is NULL)
> > - In the hrtimer, while holding the spinlock, take the pointer from crc_data
> >   into a local variable and clear the crtc->crc_data pointer
> > - Pass the pointer (from the local variable) to the crc_worker
> > 

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

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

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

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

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

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

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

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

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

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

> So the more I think about this, I don't think it's quite right. Perhaps I'm
> missing an email from Daniel, or (more likely) misunderstanding what you're
> trying to do. However, the email I read suggested storing the crc_data
> to be consumed by the crc worker in vkms_crtc. However in this patch it's being
> stored (along with the work item) in crtc state. Further, the state cannot be
> destroyed without completing the crc_work, which kind of defeats the purpose of
> the workqueue in the first place.
> 
> Storing a copy of the crc_data in crtc allows you to free the crtc_state, so you
> can start working on the next frame while the crc worker is scheduled and picks
> up the crc_data from crtc. Storing the crc_data in crtc_state and waiting for
> the work to finish is easier since you don't need to worry about copying and
> locking (so much). However, it seems like we're doing both in this patch.
> 
> Again, I'm likely just not understanding what the goal of this is, so any
> clarification would be greatly appreciated :)
> 
> Sean
> 
> > 
> > Sean
> > 
> > > 
> > > > > +}
> > > > > +
> > > > >  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);
> > > > 
> > > > What's the rationale behind this change?
> > > > 
> > > 
> > > hm I am not sure if this is correct, but I assume because spin_lock_irq in atomic_begin
> > > would disable interrupts and since this is before we unlock vkms_output->lock with 
> > > spin_unlock_irq so irqsave here is not necessary?
> > > 
> > 
> > Once you limit the scope of the spinlock above, you will want to revert this
> > change.
> > 
> > > > >  
> > > > >  		if (drm_crtc_vblank_get(crtc) != 0)
> > > > >  			drm_crtc_send_vblank_event(crtc, crtc->state->event);
> > > > >  		else
> > > > >  			drm_crtc_arm_vblank_event(crtc, crtc->state->event);
> > > > >  
> > > > > -		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> > > > > +		spin_unlock(&crtc->dev->event_lock);
> > > > >  
> > > > >  		crtc->state->event = NULL;
> > > > >  	}
> > > > > +
> > > > > +	spin_unlock_irq(&vkms_output->lock);
> > > > >  }
> > > > >  
> > > > >  static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
> > > > > +	.atomic_begin	= vkms_crtc_atomic_begin,
> > > > >  	.atomic_flush	= vkms_crtc_atomic_flush,
> > > > >  	.atomic_enable	= vkms_crtc_atomic_enable,
> > > > >  	.atomic_disable	= vkms_crtc_atomic_disable,
> > > > > @@ -162,6 +201,7 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
> > > > >  int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> > > > >  		   struct drm_plane *primary, struct drm_plane *cursor)
> > > > >  {
> > > > > +	struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
> > > > >  	int ret;
> > > > >  
> > > > >  	ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor,
> > > > > @@ -173,5 +213,9 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> > > > >  
> > > > >  	drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
> > > > >  
> > > > > +	spin_lock_init(&vkms_out->lock);
> > > > > +
> > > > > +	vkms_out->crc_workq = alloc_ordered_workqueue("vkms_crc_workq", 0);
> > > > > +
> > > > >  	return ret;
> > > > >  }
> > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > index 37aa2ef33b21..5d78bd97e69c 100644
> > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > @@ -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 = {
> > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > > > index bf811d0ec349..95985649768c 100644
> > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > > > @@ -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;
> > > > 
> > > > Where is this set to true?
> > > > 
> > > 
> > > Oh sorry I forgot to add a vkms_crc.c which sets crc_enabled in
> > > set_crc_source callback! 
> > > 
> > > I'll work on fixing the issues you mentioned here and the other patches
> > > as well, thank you so much again!
> > 
> > Thanks for working through this!
> > 

Thank you for your feedback!

- Haneen

> > Sean
> > 
> > > Haneen
> > > 
> > > > > +	/* ordered wq for crc_work */
> > > > > +	struct workqueue_struct *crc_workq;
> > > > > +	/* protects concurrent access to crc_data */
> > > > > +	spinlock_t lock;
> > > > >  };
> > > > >  
> > > > >  struct vkms_device {
> > > > > @@ -95,4 +111,9 @@ void *vkms_gem_vmap(struct drm_gem_object *obj);
> > > > >  
> > > > >  void vkms_gem_vunmap(struct drm_gem_object *obj);
> > > > >  
> > > > > +/* CRC Support */
> > > > > +int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
> > > > > +			size_t *values_cnt);
> > > > > +void vkms_crc_work_handle(struct work_struct *work);
> > > > > +
> > > > >  #endif /* _VKMS_DRV_H_ */
> > > > > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > > > > index aaf7c35faed6..1e0ee8ca8bd6 100644
> > > > > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > > > > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > > > > @@ -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,
> > > > > -- 
> > > > > 2.17.1
> > > > > 
> > > > 
> > > > -- 
> > > > Sean Paul, Software Engineer, Google / Chromium OS
> > 
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sean Paul]

On Wed, Jul 18, 2018 at 07:24:34PM +0300, Haneen Mohammed wrote:
> First, just so you have the complete view, this is the missing part in this patch:
> ------ vkms_crc.c
> static uint32_t _vkms_get_crc(struct vkms_crc_data *crc_data)
> {
> 	struct drm_framebuffer *fb = &crc_data->fb;
> 	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
> 	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> 	u32 crc = 0;
> 	int i = 0;
> 	unsigned int x = crc_data->src.x1 >> 16;
> 	unsigned int y = crc_data->src.y1 >> 16;
> 	unsigned int height = drm_rect_height(&crc_data->src) >> 16;
> 	unsigned int width = drm_rect_width(&crc_data->src) >> 16;
> 	unsigned int cpp = fb->format->cpp[0];
> 	unsigned int src_offset;
> 	unsigned int size_byte = width * cpp;
> 	void *vaddr = vkms_obj->vaddr;
> 
> 	if (WARN_ON(!vaddr))
> 		return crc;
> 
> 	for (i = y; i < y + height; i++) {
> 		src_offset = fb->offsets[0] + (i * fb->pitches[0]) + (x * cpp);
> 		crc = crc32_le(crc, vaddr + src_offset, size_byte);
> 	}
> 
> 	return crc;
> }
> 
> void vkms_crc_work_handle(struct work_struct *work)
> {
> 	struct vkms_crtc_state *crtc_state = container_of(work,
> 						struct vkms_crtc_state,
> 						crc_work);
> 	struct drm_crtc *crtc = crtc_state->base.crtc;
> 	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
> 
> 	if (crtc_state && out->crc_enabled) {
> 		u32 crc32 = _vkms_get_crc(&crtc_state->data);
> 		unsigned int n_frame = crtc_state->n_frame;
> 
> 		drm_crtc_add_crc_entry(crtc, true, n_frame, &crc32);
> 	}
> }
> 
> int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
> 			size_t *values_cnt)
> {
> 	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
> 
> 	if (!src_name) {
> 		out->crc_enabled = false;
> 	} else if (strcmp(src_name, "auto") == 0) {
> 		out->crc_enabled = true;
> 	} else {
> 		out->crc_enabled = false;
> 		return -EINVAL;
> 	}
> 
> 	*values_cnt = 1;
> 
> 	return 0;
> }
> --- end vkms_crc.c

/snip

> > > > > > +static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
> > > > > > +				   struct drm_crtc_state *old_crtc_state)
> > > > > > +{
> > > > > > +	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
> > > > > > +
> > > > > > +	spin_lock_irq(&vkms_output->lock);
> > > > > 
> > > > > Hmm, you can't lock across atomic_begin/flush. What is this protecting against?
> > > > > 
> > > > 
> > > > I did this per Daniel recommendation:
> > > > 1- spin_lock_irq -> vkms_crtc_atomic_begin
> > > > 2- update vkms_crc_data and get ref to fb -> vkms_primary_plane_update
> > > > 3- spin_unlock_irq after vblank event -> vkms_crtc_atomic_flush
> > > 
> > > I think I'm interpreting his mail a little differently. The point of the
> > > spinlock is to protect the pointer to the crc_data in vkms_crtc (right now it's
> > > not a pointer, but it should be).
> > > 
> > > So I think you need to do the following:
> > > - Change crc_data to a pointer
> > > - In plane_update, allocate new memory to hold crc_data and, under the spinlock,
> > >   update the crc_data to point to the new memory (add a WARN_ON if the pointer
> > >   is NULL)
> > > - In the hrtimer, while holding the spinlock, take the pointer from crc_data
> > >   into a local variable and clear the crtc->crc_data pointer
> > > - Pass the pointer (from the local variable) to the crc_worker
> > > 
> 
> The problem with this approach is managing the pointer allocation and
> deallocation. If I grab it in the hrtimer, then where/when should I free it?
> I can't after the crc worker is done with it because what I noticed is
> that the crc computation is expected to run multiple times after one
> plane_atomic_update.
> 
> > > I don't think you need to hold the spinlock across the atomic hooks, just grab
> > > it in plane_update.
> > 
> 
> I asked Daniel in irc about releasing the vkms_output->lock in the begginning of atomic_flush
> to avoid modifying the event_lock and he mentioned that "output->lock must wrap the event
> handling code completely since we need to make sure that the flip event
> and the crc all match up".
> 
> So maybe I can grab the spinlock in plane_atomic_update and release it at
> the end of crtc_atomic_flush? Is plane_atomic_update always called
> before crtc_atomic_flush?
> 
> Otherwise if I grab the spinlock in atomic_begin I won't be able to
> use pointer for crc_data and allocate it in plane_atomic_update since
> I'll be holding a spinlock already. 
> 
> I have based this patch on Daniel's second feedback to my email
> "[PATCH] drm/vkms: Update CRC support after lock review" which I didn't
> cc the dri mailing list in it, quoting Daniel:
> 
> "Overall flow:
> 1. in crtc_funcs->atomic_begin grab the spinlock, to make sure
> everything is visible completely or not at all to the vblank handler.
> Also update a pointer to the vkms_crtc_state in vkms_output->crc_data
> (or something like that.
> 2. copy the plane data (like you do here) in plane_funcs->atomic_commit.
> 3. drop the spinlock in crtc_funcs->atomic_flush after the vblank
> event is armed.
> 
> In the vblank hrtimer handler:
> 1. grab the spinlock
> 2. drm_crtc_handle_vblank() - we must do this while holding the
> spinlock, otherwise the event and crc might get out of sync. Also grab
> the current vblank number here and store it somewhere in
> vkms_crtc_state so that the worker can use the right one.
> 3. launch the worker. Note that the struct_work needs to be within the
> vkms_crtc_state structure, otherwise the crc computation worker
> doesn't know really for which data it should compute the crc - just
> having one copy in data[1] doesn't really solve this race.
> 4. drop spinlock

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

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

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

Sean


> 
> So one big problem we have here still is what happens if the crc
> worker falls behind with computing crcs. There's 2 cases, let's first
> look at how to fixe the crc worker falling behind atomic updates. The
> problem there is that the next atomic update might free the
> vkms_crtc_state structure before the worker is done with it. That's
> fairly easy to fix:
> - add a flush_work call to your vkms_crtc_state_destroy function. This
> means you need to initialize the work already in
> vkms_crtc_state_duplicate (you'll create both of these when doing the
> vkms_crtc_state subclassing).
> - 2nd we need to make sure crc workers are run in order easiest fix
> for that is to create your own private crc work queue (1 per crtc we
> have) with the WQ_ORDERED flag.
> 
> 2nd issue if the vblanks get ahead of the crc worker. This happens
> when e.g. there's no atomic updates, but  we still want to generate 1
> vblank per frame. This is probably better done in a follow-up patch,
> but rough solution is:
> - in the vblank hrtimer check whether the work is still scheduled and
> not yet complete (while holding the spinlock). in that case simply
> tell the worker to fill out the same crc for multiple vblanks, by
> using a vblank_seqno_start/end.
> - if the worker is not busy, then we tell it to compute a new crc (in
> case something in the buffers has changed - in the future we might
> change this behaviour)."
> 
> > So the more I think about this, I don't think it's quite right. Perhaps I'm
> > missing an email from Daniel, or (more likely) misunderstanding what you're
> > trying to do. However, the email I read suggested storing the crc_data
> > to be consumed by the crc worker in vkms_crtc. However in this patch it's being
> > stored (along with the work item) in crtc state. Further, the state cannot be
> > destroyed without completing the crc_work, which kind of defeats the purpose of
> > the workqueue in the first place.
> > 
> > Storing a copy of the crc_data in crtc allows you to free the crtc_state, so you
> > can start working on the next frame while the crc worker is scheduled and picks
> > up the crc_data from crtc. Storing the crc_data in crtc_state and waiting for
> > the work to finish is easier since you don't need to worry about copying and
> > locking (so much). However, it seems like we're doing both in this patch.
> > 
> > Again, I'm likely just not understanding what the goal of this is, so any
> > clarification would be greatly appreciated :)
> > 
> > Sean
> > 
> > > 
> > > Sean
> > > 
> > > > 
> > > > > > +}
> > > > > > +

/snip

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Haneen Mohammed]

On Wed, Jul 18, 2018 at 02:44:18PM -0400, Sean Paul wrote:
> On Wed, Jul 18, 2018 at 07:24:34PM +0300, Haneen Mohammed wrote:
> > First, just so you have the complete view, this is the missing part in this patch:
> > ------ vkms_crc.c
> > static uint32_t _vkms_get_crc(struct vkms_crc_data *crc_data)
> > {
> > 	struct drm_framebuffer *fb = &crc_data->fb;
> > 	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
> > 	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > 	u32 crc = 0;
> > 	int i = 0;
> > 	unsigned int x = crc_data->src.x1 >> 16;
> > 	unsigned int y = crc_data->src.y1 >> 16;
> > 	unsigned int height = drm_rect_height(&crc_data->src) >> 16;
> > 	unsigned int width = drm_rect_width(&crc_data->src) >> 16;
> > 	unsigned int cpp = fb->format->cpp[0];
> > 	unsigned int src_offset;
> > 	unsigned int size_byte = width * cpp;
> > 	void *vaddr = vkms_obj->vaddr;
> > 
> > 	if (WARN_ON(!vaddr))
> > 		return crc;
> > 
> > 	for (i = y; i < y + height; i++) {
> > 		src_offset = fb->offsets[0] + (i * fb->pitches[0]) + (x * cpp);
> > 		crc = crc32_le(crc, vaddr + src_offset, size_byte);
> > 	}
> > 
> > 	return crc;
> > }
> > 
> > void vkms_crc_work_handle(struct work_struct *work)
> > {
> > 	struct vkms_crtc_state *crtc_state = container_of(work,
> > 						struct vkms_crtc_state,
> > 						crc_work);
> > 	struct drm_crtc *crtc = crtc_state->base.crtc;
> > 	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
> > 
> > 	if (crtc_state && out->crc_enabled) {
> > 		u32 crc32 = _vkms_get_crc(&crtc_state->data);
> > 		unsigned int n_frame = crtc_state->n_frame;
> > 
> > 		drm_crtc_add_crc_entry(crtc, true, n_frame, &crc32);
> > 	}
> > }
> > 
> > int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
> > 			size_t *values_cnt)
> > {
> > 	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
> > 
> > 	if (!src_name) {
> > 		out->crc_enabled = false;
> > 	} else if (strcmp(src_name, "auto") == 0) {
> > 		out->crc_enabled = true;
> > 	} else {
> > 		out->crc_enabled = false;
> > 		return -EINVAL;
> > 	}
> > 
> > 	*values_cnt = 1;
> > 
> > 	return 0;
> > }
> > --- end vkms_crc.c
> 
> /snip
> 
> > > > > > > +static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
> > > > > > > +				   struct drm_crtc_state *old_crtc_state)
> > > > > > > +{
> > > > > > > +	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
> > > > > > > +
> > > > > > > +	spin_lock_irq(&vkms_output->lock);
> > > > > > 
> > > > > > Hmm, you can't lock across atomic_begin/flush. What is this protecting against?
> > > > > > 
> > > > > 
> > > > > I did this per Daniel recommendation:
> > > > > 1- spin_lock_irq -> vkms_crtc_atomic_begin
> > > > > 2- update vkms_crc_data and get ref to fb -> vkms_primary_plane_update
> > > > > 3- spin_unlock_irq after vblank event -> vkms_crtc_atomic_flush
> > > > 
> > > > I think I'm interpreting his mail a little differently. The point of the
> > > > spinlock is to protect the pointer to the crc_data in vkms_crtc (right now it's
> > > > not a pointer, but it should be).
> > > > 
> > > > So I think you need to do the following:
> > > > - Change crc_data to a pointer
> > > > - In plane_update, allocate new memory to hold crc_data and, under the spinlock,
> > > >   update the crc_data to point to the new memory (add a WARN_ON if the pointer
> > > >   is NULL)
> > > > - In the hrtimer, while holding the spinlock, take the pointer from crc_data
> > > >   into a local variable and clear the crtc->crc_data pointer
> > > > - Pass the pointer (from the local variable) to the crc_worker
> > > > 
> > 
> > The problem with this approach is managing the pointer allocation and
> > deallocation. If I grab it in the hrtimer, then where/when should I free it?
> > I can't after the crc worker is done with it because what I noticed is
> > that the crc computation is expected to run multiple times after one
> > plane_atomic_update.
> > 
> > > > I don't think you need to hold the spinlock across the atomic hooks, just grab
> > > > it in plane_update.
> > > 
> > 
> > I asked Daniel in irc about releasing the vkms_output->lock in the begginning of atomic_flush
> > to avoid modifying the event_lock and he mentioned that "output->lock must wrap the event
> > handling code completely since we need to make sure that the flip event
> > and the crc all match up".
> > 
> > So maybe I can grab the spinlock in plane_atomic_update and release it at
> > the end of crtc_atomic_flush? Is plane_atomic_update always called
> > before crtc_atomic_flush?
> > 
> > Otherwise if I grab the spinlock in atomic_begin I won't be able to
> > use pointer for crc_data and allocate it in plane_atomic_update since
> > I'll be holding a spinlock already. 
> > 
> > I have based this patch on Daniel's second feedback to my email
> > "[PATCH] drm/vkms: Update CRC support after lock review" which I didn't
> > cc the dri mailing list in it, quoting Daniel:
> > 
> > "Overall flow:
> > 1. in crtc_funcs->atomic_begin grab the spinlock, to make sure
> > everything is visible completely or not at all to the vblank handler.
> > Also update a pointer to the vkms_crtc_state in vkms_output->crc_data
> > (or something like that.
> > 2. copy the plane data (like you do here) in plane_funcs->atomic_commit.
> > 3. drop the spinlock in crtc_funcs->atomic_flush after the vblank
> > event is armed.
> > 
> > In the vblank hrtimer handler:
> > 1. grab the spinlock
> > 2. drm_crtc_handle_vblank() - we must do this while holding the
> > spinlock, otherwise the event and crc might get out of sync. Also grab
> > the current vblank number here and store it somewhere in
> > vkms_crtc_state so that the worker can use the right one.
> > 3. launch the worker. Note that the struct_work needs to be within the
> > vkms_crtc_state structure, otherwise the crc computation worker
> > doesn't know really for which data it should compute the crc - just
> > having one copy in data[1] doesn't really solve this race.
> > 4. drop spinlock
> 
> Ok, I think I understand what we're trying to do. It seems like the spinlock is
> simulating what we would do in hw by telling the display controller not to latch
> in new addresses until we're ready. By blocking the vblank handler (and, by
> extension, the event), we're delay sending an event out until the crc data is
> complete. In hw, this would be a dropped frame.
> 
> I'd probably prefer doing something more similar to hw. Something like simulating
> a latch to tell the vblank handler to skip sending the event until the next
> timer interrupt, instead of blocking it. I think that would avoid having to hold
> the spinlock across begin/<everything>/flush. However Daniel is smarter than I am,
> so I'll go with his suggestion since I'm sure there's a very good reason this is a
> very bad idea :-)
> 
> In that case, what you have makes sense to me, so if we could flush the work
> item instead of the whole queue, I think we're there.
> 

Alright, will do that. Thank you so much!

- Haneen
> Sean
> 
> 
> > 
> > So one big problem we have here still is what happens if the crc
> > worker falls behind with computing crcs. There's 2 cases, let's first
> > look at how to fixe the crc worker falling behind atomic updates. The
> > problem there is that the next atomic update might free the
> > vkms_crtc_state structure before the worker is done with it. That's
> > fairly easy to fix:
> > - add a flush_work call to your vkms_crtc_state_destroy function. This
> > means you need to initialize the work already in
> > vkms_crtc_state_duplicate (you'll create both of these when doing the
> > vkms_crtc_state subclassing).
> > - 2nd we need to make sure crc workers are run in order easiest fix
> > for that is to create your own private crc work queue (1 per crtc we
> > have) with the WQ_ORDERED flag.
> > 
> > 2nd issue if the vblanks get ahead of the crc worker. This happens
> > when e.g. there's no atomic updates, but  we still want to generate 1
> > vblank per frame. This is probably better done in a follow-up patch,
> > but rough solution is:
> > - in the vblank hrtimer check whether the work is still scheduled and
> > not yet complete (while holding the spinlock). in that case simply
> > tell the worker to fill out the same crc for multiple vblanks, by
> > using a vblank_seqno_start/end.
> > - if the worker is not busy, then we tell it to compute a new crc (in
> > case something in the buffers has changed - in the future we might
> > change this behaviour)."
> > 
> > > So the more I think about this, I don't think it's quite right. Perhaps I'm
> > > missing an email from Daniel, or (more likely) misunderstanding what you're
> > > trying to do. However, the email I read suggested storing the crc_data
> > > to be consumed by the crc worker in vkms_crtc. However in this patch it's being
> > > stored (along with the work item) in crtc state. Further, the state cannot be
> > > destroyed without completing the crc_work, which kind of defeats the purpose of
> > > the workqueue in the first place.
> > > 
> > > Storing a copy of the crc_data in crtc allows you to free the crtc_state, so you
> > > can start working on the next frame while the crc worker is scheduled and picks
> > > up the crc_data from crtc. Storing the crc_data in crtc_state and waiting for
> > > the work to finish is easier since you don't need to worry about copying and
> > > locking (so much). However, it seems like we're doing both in this patch.
> > > 
> > > Again, I'm likely just not understanding what the goal of this is, so any
> > > clarification would be greatly appreciated :)
> > > 
> > > Sean
> > > 
> > > > 
> > > > Sean
> > > > 
> > > > > 
> > > > > > > +}
> > > > > > > +
> 
> /snip
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Haneen Mohammed]

On Wed, Jul 18, 2018 at 02:44:18PM -0400, Sean Paul wrote:
> On Wed, Jul 18, 2018 at 07:24:34PM +0300, Haneen Mohammed wrote:
> > First, just so you have the complete view, this is the missing part in this patch:
> > ------ vkms_crc.c
> > static uint32_t _vkms_get_crc(struct vkms_crc_data *crc_data)
> > {
> > 	struct drm_framebuffer *fb = &crc_data->fb;
> > 	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
> > 	struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> > 	u32 crc = 0;
> > 	int i = 0;
> > 	unsigned int x = crc_data->src.x1 >> 16;
> > 	unsigned int y = crc_data->src.y1 >> 16;
> > 	unsigned int height = drm_rect_height(&crc_data->src) >> 16;
> > 	unsigned int width = drm_rect_width(&crc_data->src) >> 16;
> > 	unsigned int cpp = fb->format->cpp[0];
> > 	unsigned int src_offset;
> > 	unsigned int size_byte = width * cpp;
> > 	void *vaddr = vkms_obj->vaddr;
> > 
> > 	if (WARN_ON(!vaddr))
> > 		return crc;
> > 
> > 	for (i = y; i < y + height; i++) {
> > 		src_offset = fb->offsets[0] + (i * fb->pitches[0]) + (x * cpp);
> > 		crc = crc32_le(crc, vaddr + src_offset, size_byte);
> > 	}
> > 
> > 	return crc;
> > }
> > 
> > void vkms_crc_work_handle(struct work_struct *work)
> > {
> > 	struct vkms_crtc_state *crtc_state = container_of(work,
> > 						struct vkms_crtc_state,
> > 						crc_work);
> > 	struct drm_crtc *crtc = crtc_state->base.crtc;
> > 	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
> > 
> > 	if (crtc_state && out->crc_enabled) {
> > 		u32 crc32 = _vkms_get_crc(&crtc_state->data);
> > 		unsigned int n_frame = crtc_state->n_frame;
> > 
> > 		drm_crtc_add_crc_entry(crtc, true, n_frame, &crc32);
> > 	}
> > }
> > 
> > int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
> > 			size_t *values_cnt)
> > {
> > 	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
> > 
> > 	if (!src_name) {
> > 		out->crc_enabled = false;
> > 	} else if (strcmp(src_name, "auto") == 0) {
> > 		out->crc_enabled = true;
> > 	} else {
> > 		out->crc_enabled = false;
> > 		return -EINVAL;
> > 	}
> > 
> > 	*values_cnt = 1;
> > 
> > 	return 0;
> > }
> > --- end vkms_crc.c
> 
> /snip
> 
> > > > > > > +static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
> > > > > > > +				   struct drm_crtc_state *old_crtc_state)
> > > > > > > +{
> > > > > > > +	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
> > > > > > > +
> > > > > > > +	spin_lock_irq(&vkms_output->lock);
> > > > > > 
> > > > > > Hmm, you can't lock across atomic_begin/flush. What is this protecting against?
> > > > > > 
> > > > > 
> > > > > I did this per Daniel recommendation:
> > > > > 1- spin_lock_irq -> vkms_crtc_atomic_begin
> > > > > 2- update vkms_crc_data and get ref to fb -> vkms_primary_plane_update
> > > > > 3- spin_unlock_irq after vblank event -> vkms_crtc_atomic_flush
> > > > 
> > > > I think I'm interpreting his mail a little differently. The point of the
> > > > spinlock is to protect the pointer to the crc_data in vkms_crtc (right now it's
> > > > not a pointer, but it should be).
> > > > 
> > > > So I think you need to do the following:
> > > > - Change crc_data to a pointer
> > > > - In plane_update, allocate new memory to hold crc_data and, under the spinlock,
> > > >   update the crc_data to point to the new memory (add a WARN_ON if the pointer
> > > >   is NULL)
> > > > - In the hrtimer, while holding the spinlock, take the pointer from crc_data
> > > >   into a local variable and clear the crtc->crc_data pointer
> > > > - Pass the pointer (from the local variable) to the crc_worker
> > > > 
> > 
> > The problem with this approach is managing the pointer allocation and
> > deallocation. If I grab it in the hrtimer, then where/when should I free it?
> > I can't after the crc worker is done with it because what I noticed is
> > that the crc computation is expected to run multiple times after one
> > plane_atomic_update.
> > 
> > > > I don't think you need to hold the spinlock across the atomic hooks, just grab
> > > > it in plane_update.
> > > 
> > 
> > I asked Daniel in irc about releasing the vkms_output->lock in the begginning of atomic_flush
> > to avoid modifying the event_lock and he mentioned that "output->lock must wrap the event
> > handling code completely since we need to make sure that the flip event
> > and the crc all match up".
> > 
> > So maybe I can grab the spinlock in plane_atomic_update and release it at
> > the end of crtc_atomic_flush? Is plane_atomic_update always called
> > before crtc_atomic_flush?
> > 
> > Otherwise if I grab the spinlock in atomic_begin I won't be able to
> > use pointer for crc_data and allocate it in plane_atomic_update since
> > I'll be holding a spinlock already. 
> > 
> > I have based this patch on Daniel's second feedback to my email
> > "[PATCH] drm/vkms: Update CRC support after lock review" which I didn't
> > cc the dri mailing list in it, quoting Daniel:
> > 
> > "Overall flow:
> > 1. in crtc_funcs->atomic_begin grab the spinlock, to make sure
> > everything is visible completely or not at all to the vblank handler.
> > Also update a pointer to the vkms_crtc_state in vkms_output->crc_data
> > (or something like that.
> > 2. copy the plane data (like you do here) in plane_funcs->atomic_commit.
> > 3. drop the spinlock in crtc_funcs->atomic_flush after the vblank
> > event is armed.
> > 
> > In the vblank hrtimer handler:
> > 1. grab the spinlock
> > 2. drm_crtc_handle_vblank() - we must do this while holding the
> > spinlock, otherwise the event and crc might get out of sync. Also grab
> > the current vblank number here and store it somewhere in
> > vkms_crtc_state so that the worker can use the right one.
> > 3. launch the worker. Note that the struct_work needs to be within the
> > vkms_crtc_state structure, otherwise the crc computation worker
> > doesn't know really for which data it should compute the crc - just
> > having one copy in data[1] doesn't really solve this race.
> > 4. drop spinlock
> 
> Ok, I think I understand what we're trying to do. It seems like the spinlock is
> simulating what we would do in hw by telling the display controller not to latch
> in new addresses until we're ready. By blocking the vblank handler (and, by
> extension, the event), we're delay sending an event out until the crc data is
> complete. In hw, this would be a dropped frame.
> 

Hi Sean, now that I've moved the crc_data to plane_state, I think I can
move the spinlock to plane_update.

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

I didn't completely understand this method, but the best I can think of
is that if I want to try this approach, then I should move the spinlock to
plane_update, which mean it can be acquired again by the vblank_handle()
before we send the vblank event in crtc_atomic_flush(), and thus we need
to delay the call to vblank_handle() until we make sure the event was
sent from atomic_flush?


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

> In that case, what you have makes sense to me, so if we could flush the work
> item instead of the whole queue, I think we're there.
> 
> Sean
> 
> 
> > 
> > So one big problem we have here still is what happens if the crc
> > worker falls behind with computing crcs. There's 2 cases, let's first
> > look at how to fixe the crc worker falling behind atomic updates. The
> > problem there is that the next atomic update might free the
> > vkms_crtc_state structure before the worker is done with it. That's
> > fairly easy to fix:
> > - add a flush_work call to your vkms_crtc_state_destroy function. This
> > means you need to initialize the work already in
> > vkms_crtc_state_duplicate (you'll create both of these when doing the
> > vkms_crtc_state subclassing).
> > - 2nd we need to make sure crc workers are run in order easiest fix
> > for that is to create your own private crc work queue (1 per crtc we
> > have) with the WQ_ORDERED flag.
> > 
> > 2nd issue if the vblanks get ahead of the crc worker. This happens
> > when e.g. there's no atomic updates, but  we still want to generate 1
> > vblank per frame. This is probably better done in a follow-up patch,
> > but rough solution is:
> > - in the vblank hrtimer check whether the work is still scheduled and
> > not yet complete (while holding the spinlock). in that case simply
> > tell the worker to fill out the same crc for multiple vblanks, by
> > using a vblank_seqno_start/end.
> > - if the worker is not busy, then we tell it to compute a new crc (in
> > case something in the buffers has changed - in the future we might
> > change this behaviour)."
> > 
> > > So the more I think about this, I don't think it's quite right. Perhaps I'm
> > > missing an email from Daniel, or (more likely) misunderstanding what you're
> > > trying to do. However, the email I read suggested storing the crc_data
> > > to be consumed by the crc worker in vkms_crtc. However in this patch it's being
> > > stored (along with the work item) in crtc state. Further, the state cannot be
> > > destroyed without completing the crc_work, which kind of defeats the purpose of
> > > the workqueue in the first place.
> > > 
> > > Storing a copy of the crc_data in crtc allows you to free the crtc_state, so you
> > > can start working on the next frame while the crc worker is scheduled and picks
> > > up the crc_data from crtc. Storing the crc_data in crtc_state and waiting for
> > > the work to finish is easier since you don't need to worry about copying and
> > > locking (so much). However, it seems like we're doing both in this patch.
> > > 
> > > Again, I'm likely just not understanding what the goal of this is, so any
> > > clarification would be greatly appreciated :)
> > > 
> > > Sean
> > > 
> > > > 
> > > > Sean
> > > > 
> > > > > 
> > > > > > > +}
> > > > > > > +
> 
> /snip
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel