intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Don't clobber crtc->fb when queue_flip fails
@ 2013-02-22 12:17 ville.syrjala
  2013-02-22 13:31 ` [Intel-gfx] " Chris Wilson
  2013-02-22 13:46 ` Mika Kuoppala
  0 siblings, 2 replies; 6+ messages in thread
From: ville.syrjala @ 2013-02-22 12:17 UTC (permalink / raw)
  To: intel-gfx; +Cc: Ville Syrjälä, stable

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915: Don't clobber crtc->fb when queue_flip fails
  2013-02-22 12:17 [PATCH] drm/i915: Don't clobber crtc->fb when queue_flip fails ville.syrjala
@ 2013-02-22 13:31 ` Chris Wilson
  2013-02-22 14:36   ` Ville Syrjälä
  2013-02-22 13:46 ` Mika Kuoppala
  1 sibling, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2013-02-22 13:31 UTC (permalink / raw)
  To: ville.syrjala; +Cc: intel-gfx, stable

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915: Don't clobber crtc->fb when queue_flip fails
  2013-02-22 12:17 [PATCH] drm/i915: Don't clobber crtc->fb when queue_flip fails ville.syrjala
  2013-02-22 13:31 ` [Intel-gfx] " Chris Wilson
@ 2013-02-22 13:46 ` Mika Kuoppala
  1 sibling, 0 replies; 6+ messages in thread
From: Mika Kuoppala @ 2013-02-22 13:46 UTC (permalink / raw)
  To: ville.syrjala, intel-gfx; +Cc: stable

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915: Don't clobber crtc->fb when queue_flip fails
  2013-02-22 13:31 ` [Intel-gfx] " Chris Wilson
@ 2013-02-22 14:36   ` Ville Syrjälä
  2013-02-22 14:41     ` Chris Wilson
  2013-03-03 18:48     ` Daniel Vetter
  0 siblings, 2 replies; 6+ messages in thread
From: Ville Syrjälä @ 2013-02-22 14:36 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx, stable

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915: Don't clobber crtc->fb when queue_flip fails
  2013-02-22 14:36   ` Ville Syrjälä
@ 2013-02-22 14:41     ` Chris Wilson
  2013-03-03 18:48     ` Daniel Vetter
  1 sibling, 0 replies; 6+ messages in thread
From: Chris Wilson @ 2013-02-22 14:41 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx, stable

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915: Don't clobber crtc->fb when queue_flip fails
  2013-02-22 14:36   ` Ville Syrjälä
  2013-02-22 14:41     ` Chris Wilson
@ 2013-03-03 18:48     ` Daniel Vetter
  1 sibling, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2013-03-03 18:48 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: Chris Wilson, intel-gfx, stable

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2013-03-03 18:48 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-22 12:17 [PATCH] drm/i915: Don't clobber crtc->fb when queue_flip fails ville.syrjala
2013-02-22 13:31 ` [Intel-gfx] " Chris Wilson
2013-02-22 14:36   ` Ville Syrjälä
2013-02-22 14:41     ` Chris Wilson
2013-03-03 18:48     ` Daniel Vetter
2013-02-22 13:46 ` Mika Kuoppala

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).