| Message ID | 1361535444-14625-1-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 22, 2013 at 02:17:24PM +0200, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Point crtc->fb the the new framebuffer only after we know that the flip > was succesfully queued. > > While at it, move the intel_fb and obj assignments a bit close to where > they're used. > > Cc: stable@vger.kernel.org > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Hmm, that exposes us to a FlipDone interrupt seeing the old crtc->fb. That looks safe enough, but can you see how ugly restoring the old_fb looks in comparison? -Chris
ville.syrjala@linux.intel.com writes: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Point crtc->fb the the new framebuffer only after we know that the flip > was succesfully queued. > > While at it, move the intel_fb and obj assignments a bit close to where > they're used. > > Cc: stable@vger.kernel.org > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > It looks like Mika hit this rather easily in his ARB_robustness work. > Waiting for him to confirm whether this really fixes the pin_count > underflow bug he's seeing. Tested-by: Mika Kuoppala <mika.kuoppala@intel.com> > drivers/gpu/drm/i915/intel_display.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 0ff10b3..e7684f1 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -7311,9 +7311,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > intel_crtc->unpin_work = work; > spin_unlock_irqrestore(&dev->event_lock, flags); > > - intel_fb = to_intel_framebuffer(fb); > - obj = intel_fb->obj; > - > if (atomic_read(&intel_crtc->unpin_work_count) >= 2) > flush_workqueue(dev_priv->wq); > > @@ -7321,12 +7318,13 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > if (ret) > goto cleanup; > > + intel_fb = to_intel_framebuffer(fb); > + obj = intel_fb->obj; > + > /* Reference the objects for the scheduled work. */ > drm_gem_object_reference(&work->old_fb_obj->base); > drm_gem_object_reference(&obj->base); > > - crtc->fb = fb; > - > work->pending_flip_obj = obj; > > work->enable_stall_check = true; > @@ -7338,6 +7336,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > if (ret) > goto cleanup_pending; > > + crtc->fb = fb; > + > intel_disable_fbc(dev); > intel_mark_fb_busy(obj); > mutex_unlock(&dev->struct_mutex); > -- > 1.7.12.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Feb 22, 2013 at 01:31:35PM +0000, Chris Wilson wrote: > On Fri, Feb 22, 2013 at 02:17:24PM +0200, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Point crtc->fb the the new framebuffer only after we know that the flip > > was succesfully queued. > > > > While at it, move the intel_fb and obj assignments a bit close to where > > they're used. > > > > Cc: stable@vger.kernel.org > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Hmm, that exposes us to a FlipDone interrupt seeing the old crtc->fb. > That looks safe enough, but can you see how ugly restoring the old_fb > looks in comparison? I don't think anyone should be poking at crtc->fb w/o holding the crtc mutex. Except that intel_update_fbc() actually does. That thing would appear to be just broken since it crawls around in the crtc state w/o proper protection. The fb could even disappear from under it. But if you prefer the set/restore approach I'll send out a version doing that.
On Fri, Feb 22, 2013 at 04:36:38PM +0200, Ville Syrjälä wrote: > On Fri, Feb 22, 2013 at 01:31:35PM +0000, Chris Wilson wrote: > > On Fri, Feb 22, 2013 at 02:17:24PM +0200, ville.syrjala@linux.intel.com wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Point crtc->fb the the new framebuffer only after we know that the flip > > > was succesfully queued. > > > > > > While at it, move the intel_fb and obj assignments a bit close to where > > > they're used. > > > > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Hmm, that exposes us to a FlipDone interrupt seeing the old crtc->fb. > > That looks safe enough, but can you see how ugly restoring the old_fb > > looks in comparison? > > I don't think anyone should be poking at crtc->fb w/o holding the crtc > mutex. Except that intel_update_fbc() actually does. That thing would > appear to be just broken since it crawls around in the crtc state w/o > proper protection. The fb could even disappear from under it. You'll find not a lot of argument from me here, just suggesting that we avoid the semantic change unless we are confident there are no surprises. > But if you prefer the set/restore approach I'll send out a version > doing that. I think I would prefer that approach for stable@ -Chris
On Fri, Feb 22, 2013 at 04:36:38PM +0200, Ville Syrjälä wrote: > I don't think anyone should be poking at crtc->fb w/o holding the crtc > mutex. Except that intel_update_fbc() actually does. That thing would > appear to be just broken since it crawls around in the crtc state w/o > proper protection. The fb could even disappear from under it. fbc has terminally broken locking, and that's been the case since forever afaict. Imo the right solution would be to keep around a bit of fbc state with it's own mutex, and update that (with proper locking) from the crtc enable/disable code at the right spots. But as long as fbc is disabled by default I don't care that much. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 0ff10b3..e7684f1 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7311,9 +7311,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, intel_crtc->unpin_work = work; spin_unlock_irqrestore(&dev->event_lock, flags); - intel_fb = to_intel_framebuffer(fb); - obj = intel_fb->obj; - if (atomic_read(&intel_crtc->unpin_work_count) >= 2) flush_workqueue(dev_priv->wq); @@ -7321,12 +7318,13 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, if (ret) goto cleanup; + intel_fb = to_intel_framebuffer(fb); + obj = intel_fb->obj; + /* Reference the objects for the scheduled work. */ drm_gem_object_reference(&work->old_fb_obj->base); drm_gem_object_reference(&obj->base); - crtc->fb = fb; - work->pending_flip_obj = obj; work->enable_stall_check = true; @@ -7338,6 +7336,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, if (ret) goto cleanup_pending; + crtc->fb = fb; + intel_disable_fbc(dev); intel_mark_fb_busy(obj); mutex_unlock(&dev->struct_mutex);