Re: [RFC 3/3] drm/vkms: Implement CRC debugfs API

[Haneen Mohammed <hamohammed.sa@gmail.com>]

On Thu, Jun 28, 2018 at 01:40:08PM +0200, Daniel Vetter wrote:
> On Thu, Jun 28, 2018 at 10:07 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > 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@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.
> 
> Ville just said that he pushed all that. Are you sure that you're on
> latest drm-tip? On latest drm-tip primary->fb should be always NULL
> for an atomic driver like vkms ... If you're not on latest drm-tip
> probably good idea to rebase everything.
> -Daniel
> 

Ok I will work on that, thank you so much! 

I did update my branch recently but then that resulted in networking
problems so I reverted the update. I will fix that issue first and then
work on the locking issues. 

Thank you so much again!
Haneen

> >
> > 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
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel