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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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