[xf86-video-intel] sna: Use correct struct sna in sna_mode_wakeup
diff mbox series

Message ID 20200121102858.3027175-1-thomas.preston@codethink.co.uk
State New
Headers show
Series
  • [xf86-video-intel] sna: Use correct struct sna in sna_mode_wakeup
Related show

Commit Message

Thomas Preston Jan. 21, 2020, 10:28 a.m. UTC
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@codethink.co.uk>
---
 src/sna/sna_display.c | 48 +++++++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 22 deletions(-)

Comments

Chris Wilson Jan. 21, 2020, 11:27 a.m. UTC | #1
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@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
Thomas Preston Jan. 21, 2020, 2:11 p.m. UTC | #2
On 21/01/2020 11:27, Chris Wilson wrote:
> 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@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.
> 

If it helps, the hanging display described in #184 only happens when
TearFree (shadow_enabled) is enabled for both displays. Maybe
sna_mode_wakeup is called on a :0.1 head event, but sna_dri2_vblank_handler
is never called because :0.0 flip_active >= 1, causing us to always defer
the event.

It's not entirely clear to me why this would stop :0.0 flip_active changing
though.

Anyway, paying more attention to which event's sna has
flip_active/shadow_enabled (as the original comments describe) makes the
problem go away.

We're actually debugging this to close-in on *another* ZaphodHead+TearFree
issue which appears on a 4.14 kernel (among other userland upgrades). At
some point :0.0 head gets stuck between two buffers (current + shadow) and
switches between the two causing a flicker or ghosting effect. It's
possibly got something to do with these patches:

	8bfac0f2 sna/dri2: Only force the TearFree/swcursor hack when using TearFree
	26f8ab54 sna: Restore local damage processing for TearFree/DRI2/swcursor early
	7cf67022 sna/dri2: Prevent the sw cursor from copyig to a buffer as we discard it

and it goes away when `-Dasync-swap=true` (APPLY_DAMAGE is 0) or we set:

	 sna->ignore_copy_area = false; //sna->flags & SNA_TEAR_FREE;
Thomas Preston Jan. 22, 2020, 1:10 p.m. UTC | #3
On 21/01/2020 14:11, Thomas Preston wrote:
> We're actually debugging this to close-in on *another* ZaphodHead+TearFree
> issue which appears on a 4.14 kernel (among other userland upgrades). At
> some point :0.0 head gets stuck between two buffers (current + shadow) and
> switches between the two causing a flicker or ghosting effect. It's
> possibly got something to do with these patches:
> 
> 	8bfac0f2 sna/dri2: Only force the TearFree/swcursor hack when using TearFree
> 	26f8ab54 sna: Restore local damage processing for TearFree/DRI2/swcursor early
> 	7cf67022 sna/dri2: Prevent the sw cursor from copyig to a buffer as we discard it
> 
> and it goes away when `-Dasync-swap=true` (APPLY_DAMAGE is 0) or we set:
> 
> 	 sna->ignore_copy_area = false; //sna->flags & SNA_TEAR_FREE;
> 

fyi the following patch fixes the flickering issue, so we just moved to
upstream + our struct sna fixup in sna_mode_wakeup

	95ea0564 sna: Rebalance prefer_blt to weight I915_TILING_Y higher

Patch
diff mbox series

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;
 	char buffer[1024];
 	int len, i;
 	int ret = 0;
@@ -9733,34 +9736,35 @@  again:
 	if (len < (int)sizeof(struct drm_event))
 		goto done;
 
-	/* Note that we cannot rely on the passed in struct sna matching
-	 * the struct sna used for the vblank event (in case it was submitted
-	 * by a different ZaphodHead). When processing the event, we must
-	 * ensure that we only use the pointer passed along with the event.
-	 */
-
 	DBG(("%s: len=%d\n", __FUNCTION__, len));
 
 	i = 0;
 	while (i < len) {
 		struct drm_event *e = (struct drm_event *)&buffer[i];
+
+		/* Note that we cannot rely on the passed in struct sna
+		 * matching the struct sna used for the vblank event (in case
+		 * it was submitted by a different ZaphodHead). When processing
+		 * the event, we must ensure that we only use the pointer
+		 * passed along with the event.
+		 */
+		struct drm_event_vblank *vbl = (struct drm_event_vblank *)e;
+		struct sna_crtc *crtc = (void *)(uintptr_t)vbl->user_data;
+		struct sna *sna_event = to_sna(crtc->base->scrn);
+
 		switch (e->type) {
 		case DRM_EVENT_VBLANK:
-			if (defer_vblanks)
+			if (sna_event->mode.flip_active && sna_event->mode.shadow_enabled)
 				defer_event(sna, e);
-			else if (((uintptr_t)((struct drm_event_vblank *)e)->user_data) & 2)
-				sna_present_vblank_handler((struct drm_event_vblank *)e);
+			else if (((uintptr_t)crtc) & 2)
+				sna_present_vblank_handler(vbl);
 			else
-				sna_dri2_vblank_handler((struct drm_event_vblank *)e);
+				sna_dri2_vblank_handler(vbl);
 			break;
 		case DRM_EVENT_FLIP_COMPLETE:
 			{
-				struct drm_event_vblank *vbl = (struct drm_event_vblank *)e;
-				struct sna_crtc *crtc = (void *)(uintptr_t)vbl->user_data;
 				uint64_t msc;
 
-				/* Beware Zaphod! */
-				sna = to_sna(crtc->base->scrn);
 
 				if (msc64(crtc, vbl->sequence, &msc)) {
 					DBG(("%s: recording last swap on pipe=%d, frame %d [%08llx], time %d.%06d\n",
@@ -9784,26 +9788,26 @@  again:
 					assert(crtc->bo->refcnt >= crtc->bo->active_scanout);
 
 					crtc->bo->active_scanout--;
-					kgem_bo_destroy(&sna->kgem, crtc->bo);
+					kgem_bo_destroy(&sna_event->kgem, crtc->bo);
 
 					if (crtc->shadow_bo) {
-						kgem_bo_destroy(&sna->kgem, crtc->shadow_bo);
+						kgem_bo_destroy(&sna_event->kgem, crtc->shadow_bo);
 						crtc->shadow_bo = NULL;
 					}
 
 					crtc->bo = crtc->flip_bo;
 					crtc->flip_bo = NULL;
 
-					assert_crtc_fb(sna, crtc);
+					assert_crtc_fb(sna_event, crtc);
 				} else {
 					crtc->flip_bo->active_scanout--;
-					kgem_bo_destroy(&sna->kgem, crtc->flip_bo);
+					kgem_bo_destroy(&sna_event->kgem, crtc->flip_bo);
 					crtc->flip_bo = NULL;
 				}
 
-				DBG(("%s: flip complete, pending? %d\n", __FUNCTION__, sna->mode.flip_active));
-				assert(sna->mode.flip_active);
-				if (--sna->mode.flip_active == 0) {
+				DBG(("%s: flip complete, pending? %d\n", __FUNCTION__, sna_event->mode.flip_active));
+				assert(sna_event->mode.flip_active);
+				if (--sna_event->mode.flip_active == 0) {
 					assert(crtc->flip_handler);
 					crtc->flip_handler(vbl, crtc->flip_data);
 				}