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