| Message ID | 20170719125502.25696-8-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Daniel Vetter (2017-07-19 13:55:00) > A bit an oversight - the current code did nothing, since only > legacy flips used the unpin_work_count and assorted logic. > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> There's a fence deadlock testcase for this, kms_flip/fence? -Chris
On Wed, Jul 19, 2017 at 02:06:43PM +0100, Chris Wilson wrote: > Quoting Daniel Vetter (2017-07-19 13:55:00) > > A bit an oversight - the current code did nothing, since only > > legacy flips used the unpin_work_count and assorted logic. > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > There's a fence deadlock testcase for this, kms_flip/fence? Crunching through them, but since I tend to test my stuff all mixed into one pile I've hit a bug in a different patch series of mine. Given that we've run without this for a while, not sure it's that critical really. -Daniel
Quoting Daniel Vetter (2017-07-19 14:15:44) > On Wed, Jul 19, 2017 at 02:06:43PM +0100, Chris Wilson wrote: > > Quoting Daniel Vetter (2017-07-19 13:55:00) > > > A bit an oversight - the current code did nothing, since only > > > legacy flips used the unpin_work_count and assorted logic. > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > There's a fence deadlock testcase for this, kms_flip/fence? > > Crunching through them, but since I tend to test my stuff all mixed into > one pile I've hit a bug in a different patch series of mine. Given that > we've run without this for a while, not sure it's that critical really. It's only going to affect gen2 (realistically using 14 fences in a gen3 batch is unlikely) if we can have 3 fb in the pipeline as userspace assumes 2 are reserved for the flip. Or at least if we may race while fb holds 3 fences due to the wq. EDEADLK is not something I've seen outside of buggy code, but it is certainly possible and this patch should fix it. -Chris
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 02620f31374b..343883214113 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4140,21 +4140,22 @@ static void ironlake_fdi_disable(struct drm_crtc *crtc) bool intel_has_pending_fb_unpin(struct drm_i915_private *dev_priv) { - struct intel_crtc *crtc; - - /* Note that we don't need to be called with mode_config.lock here - * as our list of CRTC objects is static for the lifetime of the - * device and so cannot disappear as we iterate. Similarly, we can - * happily treat the predicates as racy, atomic checks as userspace - * cannot claim and pin a new fb without at least acquring the - * struct_mutex and so serialising with us. - */ - for_each_intel_crtc(&dev_priv->drm, crtc) { - if (atomic_read(&crtc->unpin_work_count) == 0) + struct drm_crtc *crtc; + bool cleanup_done; + + drm_for_each_crtc(crtc, &dev_priv->drm) { + struct drm_crtc_commit *commit; + spin_lock(&crtc->commit_lock); + commit = list_first_entry_or_null(&crtc->commit_list, + struct drm_crtc_commit, commit_entry); + cleanup_done = commit ? + try_wait_for_completion(&commit->cleanup_done) : true; + spin_unlock(&crtc->commit_lock); + + if (cleanup_done) continue; - if (crtc->flip_work) - intel_wait_for_vblank(dev_priv, crtc->pipe); + drm_crtc_wait_one_vblank(crtc); return true; }
A bit an oversight - the current code did nothing, since only legacy flips used the unpin_work_count and assorted logic. Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-)