[PATCH] drm/i915: Don't clobber crtc->fb when queue_flip fails

[PATCH] drm/i915: Don't clobber crtc->fb when queue_flip fails

[ville.syrjala]

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.

 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

Re: [Intel-gfx] [PATCH] drm/i915: Don't clobber crtc->fb when queue_flip fails

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH] drm/i915: Don't clobber crtc->fb when queue_flip fails

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH] drm/i915: Don't clobber crtc->fb when queue_flip fails

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH] drm/i915: Don't clobber crtc->fb when queue_flip fails

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH] drm/i915: Don't clobber crtc->fb when queue_flip fails

[Mika Kuoppala]

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