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

[Sean Paul]

On Fri, Aug 03, 2018 at 03:10:57PM -0300, Gustavo Padovan wrote:
> On Fri, Aug 03, 2018 at 12:42:15PM -0400, Sean Paul wrote:
> > On Thu, Aug 02, 2018 at 08:09:51AM -0300, Gustavo Padovan wrote:
> > > 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?
> > 
> > I expressed some of the same concerns here:
> > 
> > https://lists.freedesktop.org/archives/dri-devel/2018-July/183584.html
> > 
> > The tl;dr is that the spinlock is required so we can acquire it in the vblank
> > hrtimer. It is held across begin/flush to prevent sending vblank eventss until
> > the crc has been fully calculated for the frame. More details in the thread,
> > ofc.
> 
> Sounds good to me!

Thanks! I've pushed the patches to drm-misc-next

Sean

> 
> Gustavo
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS