[RFC 3/3] drm/vkms: Implement CRC debugfs API
Daniel Vetter
daniel at ffwll.ch
Thu Jun 28 08:07:34 UTC 2018
On Thu, Jun 28, 2018 at 12:24:35AM +0300, Haneen Mohammed wrote:
> Implement the .set_crc_source() callback.
> Compute CRC using crc32 on the visible part of the framebuffer.
> Use work_struct to compute and add CRC at the end of a vblank.
>
> Signed-off-by: Haneen Mohammed <hamohammed.sa at gmail.com>
Ok locking review. I still don't have a clear idea how to best fix this,
but oh well.
> ---
> drivers/gpu/drm/vkms/vkms_crtc.c | 76 ++++++++++++++++++++++++++++++++
> drivers/gpu/drm/vkms/vkms_drv.h | 16 +++++++
> 2 files changed, 92 insertions(+)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 73aae129c37d..d78934b76e33 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -7,8 +7,78 @@
> */
>
> #include "vkms_drv.h"
> +#include <linux/crc32.h>
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_crtc_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +
> +uint32_t _vkms_get_crc(struct drm_framebuffer *fb)
> +{
> + struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
> + struct vkms_gem_object *vkms_obj = container_of(gem_obj,
> + struct vkms_gem_object,
> + gem);
> + u32 crc = 0;
> + int i = 0;
> + struct drm_plane_state *state = vkms_obj->plane_state;
vkms_obj->plane_state is the first locking issue: You store the pointer
without any locks or synchronization, which means the crc worker here
could access is while it's being changed, resulting in an inconsistent
crc. Note that the compiler/cpu is allowed to read a pointer multiple
times if it's not protected by locking/barriers, so all kinds of funny
stuff can happen here.
Second issue is that the memory you're pointing to isn't owned by the crc
subsystem, but by the atomic commit worker. That could release the memory
(and it might get reused for something else meanwhile) before the crc
worker here is done with it.
> + unsigned int x = state->src.x1 >> 16;
> + unsigned int y = state->src.y1 >> 16;
> + unsigned int height = drm_rect_height(&state->src) >> 16;
> + unsigned int width = drm_rect_width(&state->src) >> 16;
> + unsigned int cpp = fb->format->cpp[0];
> + unsigned int src_offset;
> + unsigned int size_byte = width * cpp;
> + void *vaddr = vkms_obj->vaddr;
> +
> + if (WARN_ON(!vaddr))
> + return crc;
> +
> + for (i = y; i < y + height; i++) {
> + src_offset = fb->offsets[0] + (i * fb->pitches[0]) + (x * cpp);
> + crc = crc32_le(crc, vaddr + src_offset, size_byte);
> + }
> +
> + return crc;
> +}
> +
> +void vkms_crc_work_handle(struct work_struct *work)
> +{
> + struct vkms_crtc_state *crtc_state = container_of(work,
> + struct vkms_crtc_state,
> + crc_workq);
> + struct drm_crtc *crtc = crtc_state->crtc;
> + struct drm_framebuffer *fb = (crtc->primary ? crtc->primary->fb : NULL);
The drm_plane->fb pointer has similar issues: No locking (the only thing
that's really allowed to change this is the atomic commit worker for
atomic drivers, nothing else), so same issues with inconsistent data and
using possibly freed memory.
On top Ville has a patch series to set drm_plane->fb == NULL for all
atomic drivers - it's really a leftover from the pre-atomic age and
doesn't fit for atomic.
On top of these issues for ->plane_state and ->fb we have a third issue:
The crc computation must be atomic wrt the generated vblank event for a
given atomic update. That means we must make sure that the crc we're
generating is exactly for the plane_state/fb that also issued the vblank
event. There's some atomic kms tests in igt that check this. More
concretely we need to make sure that:
- if the vblank code sees the new vblank event, then the crc _must_
compute the crc for the new configuration.
- if the vblank code does not yet see the new vblank event, then the crc
_must_ compute the crc for the old configuration.
One simple solution, but quite a bit of code to type would be.
1. add a spinlock to the vkms_crtc structure (probably need that first) to
protect all the crc/vblank event stuff. Needs to be a spinlock since we
must look at this from the hrtimer.
2. Create a new structure where you make a copy (not just pointers) of all
the data the hrtimer needs. So src offset, vblank event, all that stuff.
For the fb pointer you need to make sure it's properly reference counted
(drm_framebuffer_get/put).
3. For every atomic update, allocate a new such structure and store it in
the vkms_crtc structure (holding the spinlock while updating the pointer).
4. In the hrtimer grab that pointer and set it to NULL (again holding the
spinlock), and then explicitly send out the vblank event (using
drm_crtc_send_vblank_event). And then pass that structure with everything
the crc worker needs to the crc worker.
I think this should work, mostly.
-Daniel
> +
> + if (crtc_state->crc_enabled && fb) {
> + u32 crc32 = _vkms_get_crc(fb);
> + unsigned int n_frame = drm_crtc_accurate_vblank_count(crtc);
> +
> + drm_crtc_add_crc_entry(crtc, true, n_frame, &crc32);
> + }
> +}
> +
> +static int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
> + size_t *values_cnt)
> +{
> + struct drm_device *dev = crtc->dev;
> + struct vkms_device *vkms_dev = container_of(dev,
> + struct vkms_device,
> + drm);
> + struct vkms_crtc_state *crtc_state = &vkms_dev->output.crtc_state;
> +
> + if (!src_name) {
> + crtc_state->crc_enabled = false;
> + } else if (strcmp(src_name, "auto") == 0) {
> + crtc_state->crc_enabled = true;
> + } else {
> + crtc_state->crc_enabled = false;
> + return -EINVAL;
> + }
> +
> + *values_cnt = 1;
> +
> + return 0;
> +}
>
> static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> {
> @@ -23,6 +93,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> if (!ret)
> DRM_ERROR("vkms failure on handling vblank");
>
> + vkms_crc_work_handle(&output->crtc_state.crc_workq);
> +
> spin_lock_irqsave(&crtc->dev->event_lock, flags);
> if (output->event) {
> drm_crtc_send_vblank_event(crtc, output->event);
> @@ -46,6 +118,9 @@ static int vkms_enable_vblank(struct drm_crtc *crtc)
> out->period_ns = ktime_set(0, DEFAULT_VBLANK_NS);
> hrtimer_start(&out->vblank_hrtimer, out->period_ns, HRTIMER_MODE_ABS);
>
> + out->crtc_state.crtc = crtc;
> + INIT_WORK(&out->crtc_state.crc_workq, vkms_crc_work_handle);
> +
> return 0;
> }
>
> @@ -65,6 +140,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
> .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> .enable_vblank = vkms_enable_vblank,
> .disable_vblank = vkms_disable_vblank,
> + .set_crc_source = vkms_set_crc_source,
> };
>
> static int vkms_crtc_atomic_check(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 300e6a05d473..4a1458731dd7 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -22,6 +22,18 @@ static const u32 vkms_formats[] = {
> DRM_FORMAT_XRGB8888,
> };
>
> +/**
> + * struct vkms_crtc_state
> + * @crtc: backpointer to the CRTC
> + * @crc_workq: worker that captures CRCs for each frame
> + * @crc_enabled: flag to test if crc is enabled
> + */
> +struct vkms_crtc_state {
> + struct drm_crtc *crtc;
> + struct work_struct crc_workq;
> + bool crc_enabled;
> +};
> +
> struct vkms_output {
> struct drm_crtc crtc;
> struct drm_encoder encoder;
> @@ -29,6 +41,7 @@ struct vkms_output {
> struct hrtimer vblank_hrtimer;
> ktime_t period_ns;
> struct drm_pending_vblank_event *event;
> + struct vkms_crtc_state crtc_state;
> };
>
> struct vkms_device {
> @@ -55,6 +68,9 @@ int vkms_output_init(struct vkms_device *vkmsdev);
>
> struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev);
>
> +/* CRC Support */
> +void vkms_crc_work_handle(struct work_struct *work);
> +
> /* Gem stuff */
> struct drm_gem_object *vkms_gem_create(struct drm_device *dev,
> struct drm_file *file,
> --
> 2.17.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the dri-devel
mailing list