| Message ID | 20200121102858.3027175-1-thomas.preston@codethink.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [xf86-video-intel] sna: Use correct struct sna in sna_mode_wakeup | expand |
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
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;
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
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); }
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(-)