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

From: Gustavo Padovan <gustavo@padovan.org>
To: Sean Paul <seanpaul@chromium.org>
Cc: rodrigosiqueiramelo@gmail.com,
	Haneen Mohammed <hamohammed.sa@gmail.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 2/2] drm/vkms: Implement CRC debugfs API
Date: Fri, 3 Aug 2018 15:10:57 -0300	[thread overview]
Message-ID: <20180803181057.GA17770@juma> (raw)
In-Reply-To: <20180803164215.GK20303@art_vandelay>

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!

Gustavo

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel