Implement CRC and Add necessary infrastructure
Haneen Mohammed
reviewerv3 cover Implement CRC and Add necessary infrastructure
This patchset implement CRC debugfs API and add the necessary infrastructure required to enable to compute and add CRCs entries.
1. add functions to map/unmap buffers to kernel address space.
2. map/unmap buffers in the prepare/cleanup_fb hooks.
3. compute crc using crc32 on the visible portion of the buffer with appropriate synchronization methods.
Haneen Mohammed (5):
drm/vkms: Add functions to map/unmap GEM backing storage drm/vkms: map/unmap buffers in [prepare/cleanup]_fb hooks drm/vkms: Add atomic_helper_check_plane_state drm/vkms: subclass CRTC state
drm/vkms: Implement CRC debugfs API
drivers/gpu/drm/vkms/Makefile | 2 +- drivers/gpu/drm/vkms/vkms_crc.c | 66 ++++++++++++++++++ drivers/gpu/drm/vkms/vkms_crtc.c | 109 +++++++++++++++++++++++++++--- drivers/gpu/drm/vkms/vkms_drv.c | 1 + drivers/gpu/drm/vkms/vkms_drv.h | 41 +++++++++++ drivers/gpu/drm/vkms/vkms_gem.c | 80 +++++++++++++++++++++- drivers/gpu/drm/vkms/vkms_plane.c | 74 ++++++++++++++++++++ 7 files changed, 360 insertions(+), 13 deletions(-) create mode 100644 drivers/gpu/drm/vkms/vkms_crc.c
Signature · 6 lines
2.17.1 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Haneen Mohammed
reviewerPATCH v3 1/5 drm/vkms: Add functions to map/unmap GEM backing storage
This patch add the necessary functions to map/unmap GEM backing memory to the kernel's virtual address space.
---
Changes in v2:
- add vkms_gem_vunmap
- use vmap_count to guard against multiple prepare_fb calls on the same fb
Changes in v3:
- change vkms_gem_vmap to retrun int
- cleanup if vmap failed
- increment vmap_count after successful vmap
drivers/gpu/drm/vkms/vkms_drv.h | 9 ++++ drivers/gpu/drm/vkms/vkms_gem.c | 80 ++++++++++++++++++++++++++++++++- 2 files changed, 88 insertions(+), 1 deletion(-)
drivers/gpu/drm/vkms/vkms_drv.h +9 -0
@@ -39,6 +39,8 @@ struct vkms_gem_object {struct drm_gem_object gem;struct mutex pages_lock;/* Page lock used in page fault handler */ struct page **pages; +unsigned int vmap_count; +void *vaddr; };#define drm_crtc_to_vkms_output (target) \
@@ -47,6 +49,9 @@ struct vkms_gem_object {#define drm_device_to_vkms_device (target) \ container_of(target, struct vkms_device, drm) +#define drm_gem_to_vkms_gem (target)\ + container_of(target, struct vkms_gem_object, gem) +/* CRTC */ int vkms_crtc_init (struct drm_device *dev,struct drm_crtc *crtc,struct drm_plane *primary,struct drm_plane *cursor);
@@ -75,4 +80,8 @@ int vkms_dumb_map(struct drm_file *file, struct drm_device *dev,void vkms_gem_free_object (struct drm_gem_object *obj); +int vkms_gem_vmap (struct drm_gem_object *obj); + +void vkms_gem_vunmap (struct drm_gem_object *obj); +#endif /* _VKMS_DRV_H_ */
drivers/gpu/drm/vkms/vkms_gem.c +79 -1
89 lines not on this page · raw message
Signature · 6 lines
2.17.1 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 1/5] drm/vkms: Add functions to map/unmap GEM backing storage
Quoted 132 lines · Haneen Mohammed
This patch add the necessary functions to map/unmap GEM backing memory to the kernel's virtual address space.
Signed-off-by Haneen Mohammed <[email protected]>---
Changes in v2:
- addvkms_gem_vunmap
- usevmap_countto guard against multipleprepare_fbcalls on the same fb
Changes in v3:
- changevkms_gem_vmapto retrun int
- cleanup if vmap failed
- incrementvmap_countafter successful vmapdrivers/gpu/drm/vkms/vkms_drv.h | 9 ++++ drivers/gpu/drm/vkms/vkms_gem.c | 80 ++++++++++++++++++++++++++++++++- 2 files changed, 88 insertions(+), 1 deletion(-)drivers/gpu/drm/vkms/vkms_drv.h +9
-0@@ -39,6 +39,8 @@ struct vkms_gem_object {struct drm_gem_object gem;struct mutex pages_lock;/* Page lock used in page fault handler */ struct page **pages; +unsigned int vmap_count; +void *vaddr; };#define drm_crtc_to_vkms_output (target) \@@ -47,6 +49,9 @@ struct vkms_gem_object {#define drm_device_to_vkms_device (target) \ container_of(target, struct vkms_device, drm) +#define drm_gem_to_vkms_gem (target)\ + container_of(target, struct vkms_gem_object, gem) +/* CRTC */ int vkms_crtc_init (struct drm_device *dev,struct drm_crtc *crtc,struct drm_plane *primary,struct drm_plane *cursor);@@ -75,4 +80,8 @@ int vkms_dumb_map(struct drm_file *file, struct drm_device *dev,void vkms_gem_free_object (struct drm_gem_object *obj); +int vkms_gem_vmap (struct drm_gem_object *obj); + +void vkms_gem_vunmap (struct drm_gem_object *obj); +#endif /* _VKMS_DRV_H_ */ drivers/gpu/drm/vkms/vkms_gem.c +66
-1@@ -37,7 +37,9 @@ void vkms_gem_free_object(struct drm_gem_object *obj) struct vkms_gem_object *gem = container_of(obj, struct vkms_gem_object, gem); -kvfree (gem->pages); +WARN_ON (gem->pages); +WARN_ON (gem->vaddr); +mutex_destroy (&gem->pages_lock);drm_gem_object_release (obj);kfree (gem);@@ -177,3 +179,79 @@ int vkms_dumb_map(struct drm_file *file, struct drm_device *dev,return ret; } + +static struct page **_get_pages (struct vkms_gem_object *vkms_obj) +{ +struct drm_gem_object *gem_obj = &vkms_obj->gem; + +if (!vkms_obj->pages) { +struct page **pages =drm_gem_get_pages (gem_obj); + +if (IS_ERR (pages)) +return pages; + +if (cmpxchg (&vkms_obj->pages,NULL , pages)) +drm_gem_put_pages (gem_obj, pages, false, true); + } + +return vkms_obj->pages; +} + +void vkms_gem_vunmap (struct drm_gem_object *obj) +{ +struct vkms_gem_object *vkms_obj =drm_gem_to_vkms_gem (obj); + +mutex_lock (&vkms_obj->pages_lock); +if (vkms_obj->vmap_count <1 ) { +WARN_ON (vkms_obj->vaddr); +WARN_ON (vkms_obj->pages); +mutex_unlock (&vkms_obj->pages_lock); +return ; + } + + vkms_obj->vmap_count--; + +if (vkms_obj->vmap_count ==0 ) { +vunmap (vkms_obj->vaddr); + vkms_obj->vaddr =NULL ; +drm_gem_put_pages (obj, vkms_obj->pages, false, true); + vkms_obj->pages =NULL ; + } + +mutex_unlock (&vkms_obj->pages_lock); +} + +int vkms_gem_vmap (struct drm_gem_object *obj) +{ +struct vkms_gem_object *vkms_obj =drm_gem_to_vkms_gem (obj); +int ret =0 ; + +mutex_lock (&vkms_obj->pages_lock); + +if (!vkms_obj->vaddr) { +unsigned int n_pages = obj->size >>PAGE_SHIFT ; +struct page **pages =_get_pages (vkms_obj); + +if (IS_ERR (pages)) { + ret =PTR_ERR (pages); + goto fail; + } + + vkms_obj->vaddr =vmap (pages, n_pages,VM_MAP ,PAGE_KERNEL ); +if (!vkms_obj->vaddr) { + ret = -ENOMEM ; +drm_gem_put_pages (obj, vkms_obj->pages, false, true); + vkms_obj->pages =NULL ;
drivers/gpu/drm/vkms/vkms_gem.c int vkms_dumb_map(struct drm_file *file, struct drm_device *dev, · line 254
Quoted 10 lines
drivers/gpu/drm/vkms/vkms_gem.c +10
-0+ goto fail; + } + + vkms_obj->vmap_count++; + } + +mutex_unlock (&vkms_obj->pages_lock); +return 0 ; + +fail:
Since fail does the same as the success path, there are 2 ways of handling this that would be (IMO) better:
1- Rename fail to out and remove the redundant 2 lines above
2- Add a new label above fail called err_vmap and do the ret/put_pages/pages=NULL
cleanup there, dropping into the unlock and return after.
With either of these changes,
Sean
Quoted 6 lines
drivers/gpu/drm/vkms/vkms_gem.c +3
-0+mutex_unlock (&vkms_obj->pages_lock); +return ret; +}Signature · 1 line
2.17.1
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Haneen Mohammed
reviewerPATCH v3 2/5 drm/vkms: map/unmap buffers in [prepare/cleanup]_fb hooks
In reply to Haneen MohammedThis patch map/unmap GEM backing memory to kernel address space in prepare/cleanup_fb respectively and cache the virtual address for later use.
---
Changes in v2:
- use vkms_gem_vunmap
Changes in v3:
- return error number instead of vkms_obj->vaddr
drivers/gpu/drm/vkms/vkms_plane.c | 35 +++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)
drivers/gpu/drm/vkms/vkms_plane.c +35 -0
@@ -9,6 +9,7 @@#include "vkms_drv.h" #include <drm/drm_plane_helper.h> #include <drm/drm_atomic_helper.h> +#include <drm/drm_gem_framebuffer_helper.h> static const struct drm_plane_funcs vkms_plane_funcs = { .update_plane = drm_atomic_helper_update_plane,
@@ -24,8 +25,42 @@ static void vkms_primary_plane_update(struct drm_plane *plane, { } +static int vkms_prepare_fb (struct drm_plane *plane, +struct drm_plane_state *state) +{ +struct drm_gem_object *gem_obj; +struct vkms_gem_object *vkms_obj; +int ret; + +if (!state->fb) +return 0 ; + + gem_obj =drm_gem_fb_get_obj (state->fb,0 ); + vkms_obj =drm_gem_to_vkms_gem (gem_obj); + ret =vkms_gem_vmap (gem_obj); + +if (ret) +DRM_ERROR ("vmap failed\n" ); + +return drm_gem_fb_prepare_fb (plane, state); +} + +static void vkms_cleanup_fb (struct drm_plane *plane, +struct drm_plane_state *old_state) +{ +struct drm_gem_object *gem_obj; + +if (!old_state->fb) +return ; + + gem_obj =drm_gem_fb_get_obj (old_state->fb,0 ); +vkms_gem_vunmap (gem_obj); +} +static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = { .atomic_update = vkms_primary_plane_update, + .prepare_fb = vkms_prepare_fb, + .cleanup_fb = vkms_cleanup_fb, };struct drm_plane *vkms_plane_init (struct vkms_device *vkmsdev)
Signature · 6 lines
2.17.1 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 2/5] drm/vkms: map/unmap buffers in [prepare/cleanup]_fb hooks
Quoted 35 lines · Haneen Mohammed
This patch map/unmap GEM backing memory to kernel address space in prepare/cleanup_fb respectively and cache the virtual address for later use.
Signed-off-by Haneen Mohammed <[email protected]>---
Changes in v2:
- usevkms_gem_vunmapChanges in v3:
- return error number instead ofvkms_obj->vaddrdrivers/gpu/drm/vkms/vkms_plane.c | 35 +++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)drivers/gpu/drm/vkms/vkms_plane.c +5
-0@@ -9,6 +9,7 @@#include "vkms_drv.h" #include <drm/drm_plane_helper.h> #include <drm/drm_atomic_helper.h> +#include <drm/drm_gem_framebuffer_helper.h> static const struct drm_plane_funcs vkms_plane_funcs = { .update_plane = drm_atomic_helper_update_plane,@@ -24,8 +25,42 @@ static void vkms_primary_plane_update(struct drm_plane *plane, { } +static int vkms_prepare_fb (struct drm_plane *plane, +struct drm_plane_state *state) +{ +struct drm_gem_object *gem_obj;
drivers/gpu/drm/vkms/vkms_plane.c static void vkms_primary_plane_update(struct drm_plane *plane, · line 41
Quoted 10 lines
drivers/gpu/drm/vkms/vkms_plane.c +10
-0+struct vkms_gem_object *vkms_obj; +int ret; + +if (!state->fb) +return 0 ; + + gem_obj =drm_gem_fb_get_obj (state->fb,0 ); + vkms_obj =drm_gem_to_vkms_gem (gem_obj); + ret =vkms_gem_vmap (gem_obj); +
nit: extra line
drivers/gpu/drm/vkms/vkms_plane.c static void vkms_primary_plane_update(struct drm_plane *plane, · line 43
Quoted 2 lines
drivers/gpu/drm/vkms/vkms_plane.c +2
-0+if (ret) +DRM_ERROR ("vmap failed\n" );
Can we actually do anything useful if the vmap fails? Asked another way, what's the value in not returning the error here? (We should also print the value of ret).
Sean
Quoted 26 lines
drivers/gpu/drm/vkms/vkms_plane.c +18
-0+ +return drm_gem_fb_prepare_fb (plane, state); +} + +static void vkms_cleanup_fb (struct drm_plane *plane, +struct drm_plane_state *old_state) +{ +struct drm_gem_object *gem_obj; + +if (!old_state->fb) +return ; + + gem_obj =drm_gem_fb_get_obj (old_state->fb,0 ); +vkms_gem_vunmap (gem_obj); +} +static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = { .atomic_update = vkms_primary_plane_update, + .prepare_fb = vkms_prepare_fb, + .cleanup_fb = vkms_cleanup_fb, };struct drm_plane *vkms_plane_init (struct vkms_device *vkmsdev)Signature · 1 line
2.17.1
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Haneen Mohammed
reviewerPATCH v3 3/5 drm/vkms: Add atomic_helper_check_plane_state
In reply to Haneen MohammedCall atomic_helper_check_plane_state to clip plane coordinates.
---
Changes in v2:
- check for plane_state->visible since we can't handle a disabled primary plane yet.
drivers/gpu/drm/vkms/vkms_plane.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
drivers/gpu/drm/vkms/vkms_plane.c +29 -0
@@ -8,6 +8,7 @@#include "vkms_drv.h" #include <drm/drm_plane_helper.h> +#include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_gem_framebuffer_helper.h>
@@ -25,6 +26,33 @@ static void vkms_primary_plane_update(struct drm_plane *plane, { } +static int vkms_plane_atomic_check (struct drm_plane *plane, +struct drm_plane_state *state) +{ +struct drm_crtc_state *crtc_state; +int ret; + +if (!state->fb | !state->crtc) +return 0 ; + + crtc_state =drm_atomic_get_crtc_state (state->state, state->crtc); +if (IS_ERR (crtc_state)) +return PTR_ERR (crtc_state); + + ret =drm_atomic_helper_check_plane_state (state, crtc_state, +DRM_PLANE_HELPER_NO_SCALING , +DRM_PLANE_HELPER_NO_SCALING , + false, true); +if (ret !=0 ) +return ret; + +/* for now primary plane must be visible and full screen */ +if (!state->visible) +return -EINVAL ; + +return 0 ; +} +static int vkms_prepare_fb (struct drm_plane *plane,struct drm_plane_state *state) {
@@ -59,6 +87,7 @@ static void vkms_cleanup_fb(struct drm_plane *plane,static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = { .atomic_update = vkms_primary_plane_update, + .atomic_check = vkms_plane_atomic_check, .prepare_fb = vkms_prepare_fb, .cleanup_fb = vkms_cleanup_fb, };
Signature · 6 lines
2.17.1 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Haneen Mohammed
reviewerPATCH v3 4/5 drm/vkms: subclass CRTC state
In reply to Haneen MohammedSubclass CRTC state struct to enable storing driver's private state. This patch only adds the base drm_crtc_state struct and the atomic functions that handle it.
--- drivers/gpu/drm/vkms/vkms_crtc.c | 53 ++++++++++++++++++++++++++++++-- drivers/gpu/drm/vkms/vkms_drv.h | 11 +++++++ 2 files changed, 61 insertions(+), 3 deletions(-)
drivers/gpu/drm/vkms/vkms_crtc.c +50 -3
@@ -64,13 +64,60 @@ bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,return true; } +static void vkms_atomic_crtc_reset (struct drm_crtc *crtc) +{ +struct vkms_crtc_state *vkms_state =NULL ; + +if (crtc->state) { + vkms_state =to_vkms_crtc_state (crtc->state); +__drm_atomic_helper_crtc_destroy_state (crtc->state); +kfree (vkms_state); + crtc->state =NULL ; + } + + vkms_state =kzalloc (sizeof (*vkms_state),GFP_KERNEL ); +if (!vkms_state) +return ; + + crtc->state = &vkms_state->base; + crtc->state->crtc = crtc; +} + +static struct drm_crtc_state * +vkms_atomic_crtc_duplicate_state (struct drm_crtc *crtc) +{ +struct vkms_crtc_state *vkms_state; + +if (WARN_ON (!crtc->state)) +return NULL ; + + vkms_state =kzalloc (sizeof (*vkms_state),GFP_KERNEL ); +if (!vkms_state) +return NULL ; + +__drm_atomic_helper_crtc_duplicate_state (crtc, &vkms_state->base); + +return &vkms_state->base; +} + +static void vkms_atomic_crtc_destroy_state (struct drm_crtc *crtc, +struct drm_crtc_state *state) +{ +struct vkms_crtc_state *vkms_state; + + vkms_state =to_vkms_crtc_state (state); + +__drm_atomic_helper_crtc_destroy_state (state); +kfree (vkms_state); +} +static const struct drm_crtc_funcs vkms_crtc_funcs = { .set_config = drm_atomic_helper_set_config, .destroy = drm_crtc_cleanup, .page_flip = drm_atomic_helper_page_flip, - .reset = drm_atomic_helper_crtc_reset, - .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, - .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, + .reset = vkms_atomic_crtc_reset, + .atomic_duplicate_state = vkms_atomic_crtc_duplicate_state, + .atomic_destroy_state = vkms_atomic_crtc_destroy_state, .enable_vblank = vkms_enable_vblank, .disable_vblank = vkms_disable_vblank, };
drivers/gpu/drm/vkms/vkms_drv.h +11 -0
@@ -20,6 +20,14 @@ static const u32 vkms_formats[] = { DRM_FORMAT_XRGB8888, }; +/** +* vkms_crtc_state - Driver specific CRTC state +* @base: base CRTC state +*/ +struct vkms_crtc_state { +struct drm_crtc_state base; +}; +struct vkms_output {struct drm_crtc crtc;struct drm_encoder encoder;
@@ -52,6 +60,9 @@ struct vkms_gem_object {#define drm_gem_to_vkms_gem (target)\ container_of(target, struct vkms_gem_object, gem) +#define to_vkms_crtc_state (target)\ + container_of(target, struct vkms_crtc_state, base) +/* CRTC */ int vkms_crtc_init (struct drm_device *dev,struct drm_crtc *crtc,struct drm_plane *primary,struct drm_plane *cursor);
Signature · 6 lines
2.17.1 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Haneen Mohammed
reviewerPATCH v3 5/5 drm/vkms: Implement CRC debugfs API
In reply to Haneen MohammedImplement the set_crc_source() callback.
Compute CRC using crc32 on the visible part of the framebuffer. Use ordered workqueue to compute and add CRC at the end of a vblank.
Use appropriate synchronization methods since the CRC computation must be atomic wrt the generated vblank event for a given atomic update, by using spinlock across atomic_begin and atomic_flush to wrap the event handling code completely and match the flip event with the CRC.
---
Changes in v3:
- flush work item instead of workqueue
- include missing vkms_crc.c file
drivers/gpu/drm/vkms/Makefile | 2 +- drivers/gpu/drm/vkms/vkms_crc.c | 66 +++++++++++++++++++++++++++++++ drivers/gpu/drm/vkms/vkms_crtc.c | 58 ++++++++++++++++++++++----- drivers/gpu/drm/vkms/vkms_drv.c | 1 + drivers/gpu/drm/vkms/vkms_drv.h | 21 ++++++++++ drivers/gpu/drm/vkms/vkms_plane.c | 10 +++++ 6 files changed, 148 insertions(+), 10 deletions(-) create mode 100644 drivers/gpu/drm/vkms/vkms_crc.c
drivers/gpu/drm/vkms/Makefile +1 -1
@@ -1,3 +1,3 @@ -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o +vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o obj-$(CONFIG_DRM_VKMS) += vkms.o
drivers/gpu/drm/vkms/vkms_crc.c new file +66 -0
66 lines not on this page · raw message
drivers/gpu/drm/vkms/vkms_crtc.c +49 -9
122 lines not on this page · raw message
drivers/gpu/drm/vkms/vkms_drv.c +1 -0
@@ -46,6 +46,7 @@ static void vkms_release(struct drm_device *dev)platform_device_unregister (vkms->platform);drm_mode_config_cleanup (&vkms->drm);drm_dev_fini (&vkms->drm); +destroy_workqueue (vkms->output.crc_workq); }static struct drm_driver vkms_driver = {
drivers/gpu/drm/vkms/vkms_drv.h +21 -0
@@ -20,12 +20,23 @@ static const u32 vkms_formats[] = { DRM_FORMAT_XRGB8888, }; +struct vkms_crc_data { +struct drm_rect src; +struct drm_framebuffer fb; +}; +/** * vkms_crtc_state - Driver specific CRTC state * @base: base CRTC state +* @crc_work: work struct to compute and add CRC entries +* @data: data required for CRC computation +* @n_frame: frame number for computed CRC */ struct vkms_crtc_state {struct drm_crtc_state base; +struct work_struct crc_work; +struct vkms_crc_data data; +unsigned int n_frame; };struct vkms_output {
@@ -35,6 +46,11 @@ struct vkms_output {struct hrtimer vblank_hrtimer;ktime_t period_ns;struct drm_pending_vblank_event *event; +bool crc_enabled; +/* ordered wq for crc_work */ +struct workqueue_struct *crc_workq; +/* protects concurrent access to crc_data */ +spinlock_t lock; };struct vkms_device {
@@ -95,4 +111,9 @@ int vkms_gem_vmap(struct drm_gem_object *obj);void vkms_gem_vunmap (struct drm_gem_object *obj); +/* CRC Support */ +int vkms_set_crc_source (struct drm_crtc *crtc,const char *src_name, +size_t *values_cnt); +void vkms_crc_work_handle (struct work_struct *work); +#endif /* _VKMS_DRV_H_ */
drivers/gpu/drm/vkms/vkms_plane.c +10 -0
@@ -24,6 +24,16 @@ static const struct drm_plane_funcs vkms_plane_funcs = {static void vkms_primary_plane_update (struct drm_plane *plane,struct drm_plane_state *old_state) { +struct vkms_crtc_state *state; + +if (!plane->state->crtc || !plane->state->fb) +return ; + + state =to_vkms_crtc_state (plane->state->crtc->state); +memcpy (&state->data.src, &plane->state->src,sizeof (struct drm_rect )); +memcpy (&state->data.fb, plane->state->fb, +sizeof (struct drm_framebuffer )); +drm_framebuffer_get (&state->data.fb); } static int vkms_plane_atomic_check(struct drm_plane *plane,
Signature · 6 lines
2.17.1 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 5/5] drm/vkms: Implement CRC debugfs API
Quoted 49 lines · Haneen Mohammed
Implement the
set_crc_source()callback.
Compute CRC using crc32 on the visible part of the framebuffer. Use ordered workqueue to compute and add CRC at the end of a vblank.Use appropriate synchronization methods since the CRC computation must be atomic wrt the generated vblank event for a given atomic update, by using spinlock across
atomic_beginandatomic_flushto wrap the event handling code completely and match the flip event with the CRC.Signed-off-by Haneen Mohammed <[email protected]>---
Changes in v3:
- flush work item instead of workqueue
- include missingvkms_crc.cfiledrivers/gpu/drm/vkms/Makefile | 2 +- drivers/gpu/drm/vkms/vkms_crc.c | 66 +++++++++++++++++++++++++++++++ drivers/gpu/drm/vkms/vkms_crtc.c | 58 ++++++++++++++++++++++----- drivers/gpu/drm/vkms/vkms_drv.c | 1 + drivers/gpu/drm/vkms/vkms_drv.h | 21 ++++++++++ drivers/gpu/drm/vkms/vkms_plane.c | 10 +++++ 6 files changed, 148 insertions(+), 10 deletions(-) create mode 100644 drivers/gpu/drm/vkms/vkms_crc.cdrivers/gpu/drm/vkms/Makefile +1
-1@@ -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.odrivers/gpu/drm/vkms/vkms_crc.c new file +10
-0@@ -0,0 +1,66 @@ +// SPDX-License-Identifier: GPL-2.0 +#include "vkms_drv.h" +#include <linux/crc32.h> +#include <drm/drm_gem_framebuffer_helper.h> + +static uint32_t _vkms_get_crc (struct vkms_crc_data *crc_data) +{ +struct drm_framebuffer *fb = &crc_data->fb; +struct drm_gem_object *gem_obj =drm_gem_fb_get_obj (fb,0 ); +struct vkms_gem_object *vkms_obj =drm_gem_to_vkms_gem (gem_obj);
drivers/gpu/drm/vkms/vkms_crc.c line 20
Quoted 10 lines
drivers/gpu/drm/vkms/vkms_crc.c new file +10
-0+u32 crc =0 ; +int i =0 ; +unsigned int x = crc_data->src.x1 >>16 ; +unsigned int y = crc_data->src.y1 >>16 ; +unsigned int height =drm_rect_height (&crc_data->src) >>16 ; +unsigned int width =drm_rect_width (&crc_data->src) >>16 ; +unsigned int cpp = fb->format->cpp[0 ]; +unsigned int src_offset; +unsigned int size_byte = width * cpp; +void *vaddr = vkms_obj->vaddr;
You should hold the lock when copying the data from crc_data.
Quoted 23 lines
drivers/gpu/drm/vkms/vkms_crc.c new file +23
-0+ +if (WARN_ON (!vaddr)) +return crc; + +for (i = y; i < y + height; i++) { + src_offset = fb->offsets[0 ] + (i * fb->pitches[0 ]) + (x * cpp); + crc =crc32_le (crc, vaddr + src_offset, size_byte); + } + +return crc; +} + +void vkms_crc_work_handle (struct work_struct *work) +{ +struct vkms_crtc_state *crtc_state =container_of (work, + struct vkms_crtc_state, + crc_work); +struct drm_crtc *crtc = crtc_state->base.crtc; +struct vkms_output *out =drm_crtc_to_vkms_output (crtc); + +if (crtc_state && out->crc_enabled) { +u32 crc32 =_vkms_get_crc (&crtc_state->data); +unsigned int n_frame = crtc_state->n_frame;
drivers/gpu/drm/vkms/vkms_crc.c line 53
Quoted 10 lines
drivers/gpu/drm/vkms/vkms_crc.c new file +10
-0+ +drm_crtc_add_crc_entry (crtc, true, n_frame, &crc32); + } +} + +int vkms_set_crc_source (struct drm_crtc *crtc,const char *src_name, +size_t *values_cnt) +{ +struct vkms_output *out =drm_crtc_to_vkms_output (crtc); +
You should hold the lock when changing this value.
Quoted 65 lines
drivers/gpu/drm/vkms/vkms_crc.c new file +13
-0+if (!src_name) { + out->crc_enabled = false; + }else if (strcmp (src_name,"auto" ) ==0 ) { + out->crc_enabled = true; + }else { + out->crc_enabled = false; +return -EINVAL ; + } + + *values_cnt =1 ; + +return 0 ; +}drivers/gpu/drm/vkms/vkms_crtc.c +22
-5@@ -10,17 +10,32 @@#include <drm/drm_atomic_helper.h> #include <drm/drm_crtc_helper.h> -static enum hrtimer_restart vkms_vblank_simulate (struct hrtimer *timer) +static void _vblank_handle (struct vkms_output *output) { - struct vkms_output *output = container_of(timer, struct vkms_output, - vblank_hrtimer);struct drm_crtc *crtc = &output->crtc; -int ret_overrun; +struct vkms_crtc_state *state =to_vkms_crtc_state (crtc->state);bool ret; +spin_lock (&output->lock); ret =drm_crtc_handle_vblank (crtc);if (!ret) -DRM_ERROR ("vkms failure on handling vblank" ); +DRM_ERROR ("vkms failure on handling vblank\n" ); + +if (state && output->crc_enabled) { + state->n_frame =drm_crtc_accurate_vblank_count (crtc); +queue_work (output->crc_workq, &state->crc_work); + } + +spin_unlock (&output->lock); +} + +static enum hrtimer_restart vkms_vblank_simulate (struct hrtimer *timer) +{ + struct vkms_output *output = container_of(timer, struct vkms_output, + vblank_hrtimer); +int ret_overrun; + +_vblank_handle (output); ret_overrun =hrtimer_forward_now (&output->vblank_hrtimer, output->period_ns);@@ -97,6 +112,8 @@ vkms_atomic_crtc_duplicate_state(struct drm_crtc *crtc)__drm_atomic_helper_crtc_duplicate_state (crtc, &vkms_state->base); +INIT_WORK (&vkms_state->crc_work, vkms_crc_work_handle); +return &vkms_state->base; }
drivers/gpu/drm/vkms/vkms_crtc.c static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc, · line 131
Quoted 8 lines
drivers/gpu/drm/vkms/vkms_crtc.c +4
-1@@ -108,7 +125,13 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc, vkms_state =to_vkms_crtc_state (state);__drm_atomic_helper_crtc_destroy_state (state); -kfree (vkms_state); + +if (vkms_state) { +flush_work (&vkms_state->crc_work); +drm_framebuffer_put (&vkms_state->data.fb);
It'd be nice to leave a breadcrumb explaining where this reference is acquired.
ie: /* dropping the reference we acquired in vkms_primary_plane_update() */
Quoted 15 lines
drivers/gpu/drm/vkms/vkms_crtc.c +4
-0+memset (&vkms_state->data,0 ,sizeof (struct vkms_crc_data )); +kfree (vkms_state); + } }static const struct drm_crtc_funcs vkms_crtc_funcs = {@@ -120,6 +143,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = { .atomic_destroy_state = vkms_atomic_crtc_destroy_state, .enable_vblank = vkms_enable_vblank, .disable_vblank = vkms_disable_vblank, + .set_crc_source = vkms_set_crc_source, }; static void vkms_crtc_atomic_enable(struct drm_crtc *crtc,
drivers/gpu/drm/vkms/vkms_crtc.c static void vkms_crtc_atomic_disable(struct drm_crtc *crtc, · line 166
Quoted 9 lines
drivers/gpu/drm/vkms/vkms_crtc.c +6
-0@@ -134,26 +158,37 @@ static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,drm_crtc_vblank_off (crtc); } +static void vkms_crtc_atomic_begin (struct drm_crtc *crtc, +struct drm_crtc_state *old_crtc_state) +{ +struct vkms_output *vkms_output =drm_crtc_to_vkms_output (crtc); + +spin_lock_irq (&vkms_output->lock);
Can you please add a comment explaining that we're holding this across the commit to block the vblank timer, and why.
drivers/gpu/drm/vkms/vkms_crtc.c static void vkms_crtc_atomic_disable(struct drm_crtc *crtc, · line 175
Quoted 11 lines
drivers/gpu/drm/vkms/vkms_crtc.c +4
-2+} +static void vkms_crtc_atomic_flush (struct drm_crtc *crtc,struct drm_crtc_state *old_crtc_state) { -unsigned long flags; +struct vkms_output *vkms_output =drm_crtc_to_vkms_output (crtc);if (crtc->state->event) { -spin_lock_irqsave (&crtc->dev->event_lock, flags); +spin_lock (&crtc->dev->event_lock);
Either don't change this (since it's always safe to call _irqsave), or add a comment explaining why you don't need to.
Quoted 108 lines
drivers/gpu/drm/vkms/vkms_crtc.c +9
-1if (drm_crtc_vblank_get (crtc) !=0 )drm_crtc_send_vblank_event (crtc, crtc->state->event);else drm_crtc_arm_vblank_event (crtc, crtc->state->event); -spin_unlock_irqrestore (&crtc->dev->event_lock, flags); +spin_unlock (&crtc->dev->event_lock); crtc->state->event =NULL ; } + +spin_unlock_irq (&vkms_output->lock); }static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = { + .atomic_begin = vkms_crtc_atomic_begin, .atomic_flush = vkms_crtc_atomic_flush, .atomic_enable = vkms_crtc_atomic_enable, .atomic_disable = vkms_crtc_atomic_disable,@@ -162,6 +197,7 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {int vkms_crtc_init (struct drm_device *dev,struct drm_crtc *crtc,struct drm_plane *primary,struct drm_plane *cursor) { +struct vkms_output *vkms_out =drm_crtc_to_vkms_output (crtc);int ret; ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor,@@ -173,5 +209,9 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,drm_crtc_helper_add (crtc, &vkms_crtc_helper_funcs); +spin_lock_init (&vkms_out->lock); + + vkms_out->crc_workq =alloc_ordered_workqueue ("vkms_crc_workq" ,0 ); +return ret; }drivers/gpu/drm/vkms/vkms_drv.c +1
-0@@ -46,6 +46,7 @@ static void vkms_release(struct drm_device *dev)platform_device_unregister (vkms->platform);drm_mode_config_cleanup (&vkms->drm);drm_dev_fini (&vkms->drm); +destroy_workqueue (vkms->output.crc_workq); }static struct drm_driver vkms_driver = {drivers/gpu/drm/vkms/vkms_drv.h +21
-0@@ -20,12 +20,23 @@ static const u32 vkms_formats[] = { DRM_FORMAT_XRGB8888, }; +struct vkms_crc_data { +struct drm_rect src; +struct drm_framebuffer fb; +}; +/** * vkms_crtc_state - Driver specific CRTC state * @base: base CRTC state +* @crc_work: work struct to compute and add CRC entries +* @data: data required for CRC computation +* @n_frame: frame number for computed CRC */ struct vkms_crtc_state {struct drm_crtc_state base; +struct work_struct crc_work; +struct vkms_crc_data data; +unsigned int n_frame; };struct vkms_output {@@ -35,6 +46,11 @@ struct vkms_output {struct hrtimer vblank_hrtimer;ktime_t period_ns;struct drm_pending_vblank_event *event; +bool crc_enabled; +/* ordered wq for crc_work */ +struct workqueue_struct *crc_workq; +/* protects concurrent access to crc_data */ +spinlock_t lock; };struct vkms_device {@@ -95,4 +111,9 @@ int vkms_gem_vmap(struct drm_gem_object *obj);void vkms_gem_vunmap (struct drm_gem_object *obj); +/* CRC Support */ +int vkms_set_crc_source (struct drm_crtc *crtc,const char *src_name, +size_t *values_cnt); +void vkms_crc_work_handle (struct work_struct *work); +#endif /* _VKMS_DRV_H_ */ drivers/gpu/drm/vkms/vkms_plane.c +0
-0@@ -24,6 +24,16 @@ static const struct drm_plane_funcs vkms_plane_funcs = {static void vkms_primary_plane_update (struct drm_plane *plane,struct drm_plane_state *old_state) {
drivers/gpu/drm/vkms/vkms_plane.c static const struct drm_plane_funcs vkms_plane_funcs = { · line 36
Quoted 10 lines
drivers/gpu/drm/vkms/vkms_plane.c +10
-0+struct vkms_crtc_state *state; + +if (!plane->state->crtc || !plane->state->fb) +return ; + + state =to_vkms_crtc_state (plane->state->crtc->state); +memcpy (&state->data.src, &plane->state->src,sizeof (struct drm_rect )); +memcpy (&state->data.fb, plane->state->fb, +sizeof (struct drm_framebuffer )); +drm_framebuffer_get (&state->data.fb);
Similarly to above, a comment explaining this is released in state_destroy would be helpful.
Quoted 6 lines
drivers/gpu/drm/vkms/vkms_plane.c +0
-0} static int vkms_plane_atomic_check(struct drm_plane *plane,Signature · 1 line
2.17.1
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel