Hi Haneen,
On Thu, Aug 02, 2018 at 04:10:26AM +0300, Haneen Mohammed wrote:
> This patch implement the necessary functions to compute and add CRCs
> entries:
>
> - Implement the set_crc_source() callback.
> - Compute CRC using crc32 on the visible part of the framebuffer.
> - Use ordered workqueue per output to compute and add CRC at the end
> of a vblank.
> - Use appropriate synchronization methods since the CRC computation must
> be atomic wrt the generated vblank event for a given atomic update, by
> using spinlock across atomic_begin/atomic_flush to wrap the event
> handling code completely and match the flip event with the CRC.
>
> Since vkms_crc_work_handle() can sleep, spinlock can't be acquired
> while accessing vkms_output->primary_crc to compute CRC.
> To make sure the data is updated and released without conflict with
> the vkms_crc_work_handle(), the work_struct is flushed @crtc_destroy
> and the data is updated before scheduling the work handle again, as
> follow:
I'm not sure I get why this is a spinlock and not a mutex. With the
later you are able to sleep. Also, your spinlock held through the whole
atomic commit (from the begin to flush) which seems too much time for
busy waiting. Or am I missing something here?
Gustavo
>
> * CRC data update:
> 1- store vkms_crc_data {fb, src} per plane_state
> 2- @plane_duplicate_state -> allocate vkms_crc_data
> 3- during atomic commit (@atomic_update) ->
> a) copy {fb, src} to plane_state->crc_data
> b) get reference to fb,
> 3- @plane_destroy_state -> a) if (fb refcount) remove reference to fb
> b) deallocate crc_data
>
> * Atomic Commit:
> 1- vkms_plane_atomic_check
> 2- vkms_prepare_fb -> vmap vkms_gem_obj->vaddr
> 3- atomic_begin -> hold crc spinlock
> 4- atomic_plane_update -> a) update vkms_output->primary_crc
> b) get reference to fb
> 5- atomic_flush -> a) send vblank event while holding event_lock
> b) release crc spinlock
>
> * hrtimer regular callback:
> 1- hold crc spinlock
> 2- drm_crtc_handle_vblank()
> 3- queue vkms_crc_work_handle
> 4- release crc spinlock
>
> * cleanup:
> 1- @cleanup_fb ->vunmap vkms_gem_obj->vaddr
> 2- @crtc_destroy -> flush work struct
> 3- @plane_destroy -> a) if (fb refcount) remove reference to fb
> b) deallocate crc_data
>
> Signed-off-by: Haneen Mohammed <hamohammed.sa@xxxxxxxxx>
> ---
> Changes in v2:
> - remove vkms_crc_data for primary plane from vkms_output
> and access it from vkms_plane_state instead.
> - flush crc_workq in vkms_set_crc_source before changing out->enabled
> value.
>
> drivers/gpu/drm/vkms/Makefile | 2 +-
> drivers/gpu/drm/vkms/vkms_crc.c | 95 +++++++++++++++++++++++++++++++
> drivers/gpu/drm/vkms/vkms_crtc.c | 56 +++++++++++++++---
> drivers/gpu/drm/vkms/vkms_drv.c | 1 +
> drivers/gpu/drm/vkms/vkms_drv.h | 21 +++++++
> drivers/gpu/drm/vkms/vkms_plane.c | 30 ++++++++++
> 6 files changed, 196 insertions(+), 9 deletions(-)
> create mode 100644 drivers/gpu/drm/vkms/vkms_crc.c
>
> 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_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> new file mode 100644
> index 000000000000..9b909221943f
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "vkms_drv.h"
> +#include <linux/crc32.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +
> +static uint32_t _vkms_get_crc(struct vkms_crc_data *crc_data)
> +{
> + struct drm_framebuffer *fb = &crc_data->fb;
> + struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
> + struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(gem_obj);
> + 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;
> +
> + mutex_lock(&vkms_obj->pages_lock);
> + 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);
> + }
> + mutex_unlock(&vkms_obj->pages_lock);
> +
> + 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);
> + struct vkms_device *vdev = container_of(out, struct vkms_device,
> + output);
> + struct vkms_crc_data *primary_crc = NULL;
> + struct drm_plane *plane;
> +
> + u32 crc32 = 0;
> +
> + drm_for_each_plane(plane, &vdev->drm) {
> + struct vkms_plane_state *vplane_state;
> + struct vkms_crc_data *crc_data;
> +
> + vplane_state = to_vkms_plane_state(plane->state);
> + crc_data = vplane_state->crc_data;
> +
> + if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
> + continue;
> +
> + if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> + primary_crc = crc_data;
> + break;
> + }
> + }
> +
> + if (primary_crc)
> + crc32 = _vkms_get_crc(primary_crc);
> +
> + drm_crtc_add_crc_entry(crtc, true, crtc_state->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);
> + bool enabled = false;
> + unsigned long flags;
> + int ret = 0;
> +
> + if (src_name && strcmp(src_name, "auto") == 0)
> + enabled = true;
> + else if (src_name)
> + ret = -EINVAL;
> +
> + *values_cnt = 1;
> +
> + /* make sure nothing is scheduled on crtc workq */
> + flush_workqueue(out->crc_workq);
> +
> + spin_lock_irqsave(&out->lock, flags);
> + out->crc_enabled = enabled;
> + spin_unlock_irqrestore(&out->lock, flags);
> +
> + return ret;
> +}
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 26babb85ca77..a6c080d7f7ae 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -10,18 +10,33 @@
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_crtc_helper.h>
>
> -static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> +static void _vblank_handle(struct vkms_output *output)
> {
> - struct vkms_output *output = container_of(timer, struct vkms_output,
> - vblank_hrtimer);
> struct drm_crtc *crtc = &output->crtc;
> - int ret_overrun;
> + struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state);
> bool ret;
>
> + spin_lock(&output->lock);
> ret = drm_crtc_handle_vblank(crtc);
> if (!ret)
> DRM_ERROR("vkms failure on handling vblank");
>
> + if (state && output->crc_enabled) {
> + state->n_frame = drm_crtc_accurate_vblank_count(crtc);
> + queue_work(output->crc_workq, &state->crc_work);
> + }
> +
> + spin_unlock(&output->lock);
> +}
> +
> +static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> +{
> + struct vkms_output *output = container_of(timer, struct vkms_output,
> + vblank_hrtimer);
> + int ret_overrun;
> +
> + _vblank_handle(output);
> +
> ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
> output->period_ns);
>
> @@ -97,18 +112,22 @@ 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;
> }
>
> 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);
> + struct vkms_crtc_state *vkms_state = to_vkms_crtc_state(state);
>
> __drm_atomic_helper_crtc_destroy_state(state);
> - kfree(vkms_state);
> +
> + if (vkms_state) {
> + flush_work(&vkms_state->crc_work);
> + kfree(vkms_state);
> + }
> }
>
> static const struct drm_crtc_funcs vkms_crtc_funcs = {
> @@ -120,6 +139,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,9 +154,21 @@ 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);
> +
> + /* This lock is held across the atomic commit to block vblamk timer
> + * from scheduling vkms_crc_work_handle until the crc_data is updated
> + */
> + spin_lock_irq(&vkms_output->lock);
> +}
> +
> static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
> struct drm_crtc_state *old_crtc_state)
> {
> + struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
> unsigned long flags;
>
> if (crtc->state->event) {
> @@ -151,9 +183,12 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
>
> crtc->state->event = NULL;
> }
> +
> + spin_unlock_irq(&vkms_output->lock);
> }
>
> static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
> + .atomic_begin = vkms_crtc_atomic_begin,
> .atomic_flush = vkms_crtc_atomic_flush,
> .atomic_enable = vkms_crtc_atomic_enable,
> .atomic_disable = vkms_crtc_atomic_disable,
> @@ -162,6 +197,7 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
> int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> struct drm_plane *primary, struct drm_plane *cursor)
> {
> + struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
> int ret;
>
> ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor,
> @@ -173,5 +209,9 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>
> drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
>
> + spin_lock_init(&vkms_out->lock);
> +
> + vkms_out->crc_workq = alloc_ordered_workqueue("vkms_crc_workq", 0);
> +
> return ret;
> }
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index e340aa128b61..519a353cd909 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -79,6 +79,7 @@ static void vkms_release(struct drm_device *dev)
> drm_atomic_helper_shutdown(&vkms->drm);
> 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 a1b206643c10..de1ddf31341b 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -20,20 +20,31 @@ static const u32 vkms_formats[] = {
> DRM_FORMAT_XRGB8888,
> };
>
> +struct vkms_crc_data {
> + struct drm_rect src;
> + struct drm_framebuffer fb;
> +};
> +
> /**
> * vkms_plane_state - Driver specific plane state
> * @base: base plane state
> + * @crc_data: data required for CRC computation
> */
> struct vkms_plane_state {
> struct drm_plane_state base;
> + struct vkms_crc_data *crc_data;
> };
>
> /**
> * vkms_crtc_state - Driver specific CRTC state
> * @base: base CRTC state
> + * @crc_work: work struct to compute and add CRC entries
> + * @n_frame: frame number for computed CRC
> */
> struct vkms_crtc_state {
> struct drm_crtc_state base;
> + struct work_struct crc_work;
> + unsigned int n_frame;
> };
>
> struct vkms_output {
> @@ -43,6 +54,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 {
> @@ -106,4 +122,9 @@ int vkms_gem_vmap(struct drm_gem_object *obj);
>
> void vkms_gem_vunmap(struct drm_gem_object *obj);
>
> +/* CRC Support */
> +int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
> + size_t *values_cnt);
> +void vkms_crc_work_handle(struct work_struct *work);
> +
> #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 70fca22bc51b..c91661631c76 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -16,11 +16,18 @@ static struct drm_plane_state *
> vkms_plane_duplicate_state(struct drm_plane *plane)
> {
> struct vkms_plane_state *vkms_state;
> + struct vkms_crc_data *crc_data;
>
> vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
> if (!vkms_state)
> return NULL;
>
> + crc_data = kzalloc(sizeof(*crc_data), GFP_KERNEL);
> + if (WARN_ON(!crc_data))
> + DRM_INFO("Couldn't allocate crc_data");
> +
> + vkms_state->crc_data = crc_data;
> +
> __drm_atomic_helper_plane_duplicate_state(plane,
> &vkms_state->base);
>
> @@ -31,6 +38,18 @@ static void vkms_plane_destroy_state(struct drm_plane *plane,
> struct drm_plane_state *old_state)
> {
> struct vkms_plane_state *vkms_state = to_vkms_plane_state(old_state);
> + struct drm_crtc *crtc = vkms_state->base.crtc;
> +
> + if (crtc) {
> + /* dropping the reference we acquired in
> + * vkms_primary_plane_update()
> + */
> + if (drm_framebuffer_read_refcount(&vkms_state->crc_data->fb))
> + drm_framebuffer_put(&vkms_state->crc_data->fb);
> + }
> +
> + kfree(vkms_state->crc_data);
> + vkms_state->crc_data = NULL;
>
> __drm_atomic_helper_plane_destroy_state(old_state);
> kfree(vkms_state);
> @@ -65,6 +84,17 @@ 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_plane_state *vkms_plane_state;
> + struct vkms_crc_data *crc_data;
> +
> + if (!plane->state->crtc || !plane->state->fb)
> + return;
> +
> + vkms_plane_state = to_vkms_plane_state(plane->state);
> + crc_data = vkms_plane_state->crc_data;
> + memcpy(&crc_data->src, &plane->state->src, sizeof(struct drm_rect));
> + memcpy(&crc_data->fb, plane->state->fb, sizeof(struct drm_framebuffer));
> + drm_framebuffer_get(&crc_data->fb);
> }
>
> static int vkms_plane_atomic_check(struct drm_plane *plane,
> --
> 2.17.1
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
