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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
