[Intel-gfx] [PATCH xf86-video-intel] sna: Use correct struct sna in sna_mode_wakeup

[Chris Wilson]

Quoting Thomas Preston (2020-01-21 10:28:58)
> When deciding if we should defer_vblanks we should reference the event's
> struct sna, rather than the caller's struct sna. In order to do this, we
> must grab a new struct sna for each event in the buffer. Move this logic
> out of `case DRM_EVENT_FLIP_COMPLETE` and create a new variable
> sna_event, so that it is clear which struct sna we are referring to.
> Also add another ZaphodHead comment by the struct sna argument, in case
> someone misses the comment below.
> 
> Fixes issue #184 with ZaphodHead and TearFree, introduced in this commit:
> 
>         12db28ab sna: Reorder vblank/flip event handling to avoid TearFree recursion
> 
> Signed-off-by: Thomas Preston <thomas.preston at codethink.co.uk>
> ---
>  src/sna/sna_display.c | 48 +++++++++++++++++++++++--------------------
>  1 file changed, 26 insertions(+), 22 deletions(-)
> 
> diff --git a/src/sna/sna_display.c b/src/sna/sna_display.c
> index 874292bc..b40a6c4a 100644
> --- a/src/sna/sna_display.c
> +++ b/src/sna/sna_display.c
> @@ -9711,9 +9711,12 @@ fixup_flip:
>         RegionEmpty(region);
>  }
>  
> +/* In the case of ZaphodHead, there is only one event queue in the main
> + * struct sna. Only refer to this struct sna when dealing with the event queue.
> + * Otherwise, extract the struct sna from the event user_data.
> + */
>  int sna_mode_wakeup(struct sna *sna)
>  {
> -       bool defer_vblanks = sna->mode.flip_active && sna->mode.shadow_enabled;

My thinking was that I only cared about re-entrancy on the local sna for
processing this event queue. And there is no threading, so only one
sna is processed at a time... Hmm, I don't think we can in the situation
of being inside one shadow flip and care much about the other.

Nevertheless, you are confident enough in your test results. And there
should be nothing wrong with deferring the event to the head that is
expecting it. (If I were to tackle the problem again, I would split into
tasklets to avoid re-entrancy of the event handling and flip queue
entirely. Oh well, all I can advise is not to make the same mistakes I
did :)

Give me a moment to think about just how it ends up confused.
-Chris