[Intel-gfx] [PATCH] drm/i915: Make mmio flip wait for seqno in the work function

[Chris Wilson]

On Wed, Nov 05, 2014 at 02:23:07PM +0200, Ander Conselvan de Oliveira wrote:
> On 11/05/2014 01:23 PM, Chris Wilson wrote:
> >On Wed, Nov 05, 2014 at 01:03:00PM +0200, Ander Conselvan de Oliveira wrote:
> >>This simplifies the code quite a bit compared to iterating over all
> >>rings during the ring interrupt.
> >>
> >>Also, it allows us to drop the mmio_flip spinlock, since the mmio_flip
> >>struct is only accessed in two places. The first is when the flip is
> >>queued and the other when the mmio writes are done. Since a flip cannot
> >>be queued while there is a pending flip, the two paths shouldn't ever
> >>run in parallel. We might need to revisit that if support for replacing
> >>flips is implemented though.
> >>
> >>Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira at intel.com>
> >>---
> >>
> >>I'm not sure if locking dev->struct_mutex in the work function might
> >>have any ill effects, so I'd appreciate if someone could enlighten me.
> >
> >Good news is you can wait on the seqno without holding any locks. You
> >need to use the lower level entry point __wait_seqno() though.
> 
> I considered the fact that __wait_seqno() is static as a warning
> that I shouldn't use it directly. Should I just change that so I can
> use it intel_display.c?
> 
> The other reason I avoided it is because I didn't really understand
> how the barrier for reading the reset_counter should work. Is it
> sufficient that I do
> 
>   atomic_read(&dev_priv->gpu_error.reset_counter))
> 
> before calling __wait_seqno() or is there something more to it?

We should already have a reset counter associated with the flip (at
least I do...) But yes, the least you need to do is pass down the
current atomic_read(&reset_counter).

That it is __wait_seqno() is indeed meant to make you think twice before
using it, but really the only problem with is the name and cumbersome
arguments. I have no qualms about renaming it as __i915_wait_seqno() and
using it here - it is what I settled on in the requests conversion.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre