[PATCH] drm/i915: Reject changes of fb base when we have a flip pending

[PATCH] drm/i915: Reject changes of fb base when we have a flip pending

[Chris Wilson]

This should be impossible due to the wait for outstanding flips that the
caller is meant to perform prior to updating the scanout base. Paranoia
tells me to check anyway.

References: https://bugs.freedesktop.org/show_bug.cgi?id=75502
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 43 ++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 25c486d5fb6a..6dc93bd6594f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2349,6 +2349,25 @@ intel_finish_fb(struct drm_framebuffer *old_fb)
 	return ret;
 }
 
+static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	unsigned long flags;
+	bool pending;
+
+	if (i915_reset_in_progress(&dev_priv->gpu_error) ||
+	    intel_crtc->reset_counter != atomic_read(&dev_priv->gpu_error.reset_counter))
+		return false;
+
+	spin_lock_irqsave(&dev->event_lock, flags);
+	pending = to_intel_crtc(crtc)->unpin_work != NULL;
+	spin_unlock_irqrestore(&dev->event_lock, flags);
+
+	return pending;
+}
+
 static int
 intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 		    struct drm_framebuffer *fb)
@@ -2359,6 +2378,11 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 	struct drm_framebuffer *old_fb;
 	int ret;
 
+	if (intel_crtc_has_pending_flip(crtc)) {
+		DRM_ERROR("pipe is still busy with an old pageflip\n");
+		return -EBUSY;
+	}
+
 	/* no fb bound */
 	if (!fb) {
 		DRM_ERROR("No FB bound\n");
@@ -2984,25 +3008,6 @@ static void ironlake_fdi_disable(struct drm_crtc *crtc)
 	udelay(100);
 }
 
-static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	unsigned long flags;
-	bool pending;
-
-	if (i915_reset_in_progress(&dev_priv->gpu_error) ||
-	    intel_crtc->reset_counter != atomic_read(&dev_priv->gpu_error.reset_counter))
-		return false;
-
-	spin_lock_irqsave(&dev->event_lock, flags);
-	pending = to_intel_crtc(crtc)->unpin_work != NULL;
-	spin_unlock_irqrestore(&dev->event_lock, flags);
-
-	return pending;
-}
-
 bool intel_has_pending_fb_unpin(struct drm_device *dev)
 {
 	struct intel_crtc *crtc;
-- 
1.9.0

Re: [PATCH] drm/i915: Reject changes of fb base when we have a flip pending

[Jesse Barnes]

On Tue,  4 Mar 2014 13:15:08 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> This should be impossible due to the wait for outstanding flips that the
> caller is meant to perform prior to updating the scanout base. Paranoia
> tells me to check anyway.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=75502
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 43 ++++++++++++++++++++----------------
>  1 file changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 25c486d5fb6a..6dc93bd6594f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2349,6 +2349,25 @@ intel_finish_fb(struct drm_framebuffer *old_fb)
>  	return ret;
>  }
>  
> +static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	unsigned long flags;
> +	bool pending;
> +
> +	if (i915_reset_in_progress(&dev_priv->gpu_error) ||
> +	    intel_crtc->reset_counter != atomic_read(&dev_priv->gpu_error.reset_counter))
> +		return false;
> +
> +	spin_lock_irqsave(&dev->event_lock, flags);
> +	pending = to_intel_crtc(crtc)->unpin_work != NULL;
> +	spin_unlock_irqrestore(&dev->event_lock, flags);
> +
> +	return pending;
> +}
> +
>  static int
>  intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>  		    struct drm_framebuffer *fb)
> @@ -2359,6 +2378,11 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>  	struct drm_framebuffer *old_fb;
>  	int ret;
>  
> +	if (intel_crtc_has_pending_flip(crtc)) {
> +		DRM_ERROR("pipe is still busy with an old pageflip\n");
> +		return -EBUSY;
> +	}
> +
>  	/* no fb bound */
>  	if (!fb) {
>  		DRM_ERROR("No FB bound\n");
> @@ -2984,25 +3008,6 @@ static void ironlake_fdi_disable(struct drm_crtc *crtc)
>  	udelay(100);
>  }
>  
> -static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	unsigned long flags;
> -	bool pending;
> -
> -	if (i915_reset_in_progress(&dev_priv->gpu_error) ||
> -	    intel_crtc->reset_counter != atomic_read(&dev_priv->gpu_error.reset_counter))
> -		return false;
> -
> -	spin_lock_irqsave(&dev->event_lock, flags);
> -	pending = to_intel_crtc(crtc)->unpin_work != NULL;
> -	spin_unlock_irqrestore(&dev->event_lock, flags);
> -
> -	return pending;
> -}
> -
>  bool intel_has_pending_fb_unpin(struct drm_device *dev)
>  {
>  	struct intel_crtc *crtc;

Looks fine, my only comment is do we want this to be a DRM_ERROR?  It
would be easy for userspace to trigger this by queueing a flip on a
busy ring, then doing a mode set that ends up doing just a pipe base
update, right?

Otherwise,
Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Reject changes of fb base when we have a flip pending

[Ville Syrjälä]

On Tue, Mar 04, 2014 at 08:58:31AM -0800, Jesse Barnes wrote:
> On Tue,  4 Mar 2014 13:15:08 +0000
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > This should be impossible due to the wait for outstanding flips that the
> > caller is meant to perform prior to updating the scanout base. Paranoia
> > tells me to check anyway.
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=75502
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 43 ++++++++++++++++++++----------------
> >  1 file changed, 24 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 25c486d5fb6a..6dc93bd6594f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2349,6 +2349,25 @@ intel_finish_fb(struct drm_framebuffer *old_fb)
> >  	return ret;
> >  }
> >  
> > +static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
> > +{
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	unsigned long flags;
> > +	bool pending;
> > +
> > +	if (i915_reset_in_progress(&dev_priv->gpu_error) ||
> > +	    intel_crtc->reset_counter != atomic_read(&dev_priv->gpu_error.reset_counter))
> > +		return false;
> > +
> > +	spin_lock_irqsave(&dev->event_lock, flags);
> > +	pending = to_intel_crtc(crtc)->unpin_work != NULL;
> > +	spin_unlock_irqrestore(&dev->event_lock, flags);
> > +
> > +	return pending;
> > +}
> > +
> >  static int
> >  intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
> >  		    struct drm_framebuffer *fb)
> > @@ -2359,6 +2378,11 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
> >  	struct drm_framebuffer *old_fb;
> >  	int ret;
> >  
> > +	if (intel_crtc_has_pending_flip(crtc)) {
> > +		DRM_ERROR("pipe is still busy with an old pageflip\n");
> > +		return -EBUSY;
> > +	}
> > +
> >  	/* no fb bound */
> >  	if (!fb) {
> >  		DRM_ERROR("No FB bound\n");
> > @@ -2984,25 +3008,6 @@ static void ironlake_fdi_disable(struct drm_crtc *crtc)
> >  	udelay(100);
> >  }
> >  
> > -static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
> > -{
> > -	struct drm_device *dev = crtc->dev;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > -	unsigned long flags;
> > -	bool pending;
> > -
> > -	if (i915_reset_in_progress(&dev_priv->gpu_error) ||
> > -	    intel_crtc->reset_counter != atomic_read(&dev_priv->gpu_error.reset_counter))
> > -		return false;
> > -
> > -	spin_lock_irqsave(&dev->event_lock, flags);
> > -	pending = to_intel_crtc(crtc)->unpin_work != NULL;
> > -	spin_unlock_irqrestore(&dev->event_lock, flags);
> > -
> > -	return pending;
> > -}
> > -
> >  bool intel_has_pending_fb_unpin(struct drm_device *dev)
> >  {
> >  	struct intel_crtc *crtc;
> 
> Looks fine, my only comment is do we want this to be a DRM_ERROR?  It
> would be easy for userspace to trigger this by queueing a flip on a
> busy ring, then doing a mode set that ends up doing just a pipe base
> update, right?

It should never get this far with a pending flip. If it does, then there
must be a bug somewhere.

-- 
Ville Syrjälä
Intel OTC