From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: Reject changes of fb base when we have a flip pending Date: Tue, 4 Mar 2014 19:09:44 +0200 Message-ID: <20140304170944.GK3852@intel.com> References: <1393938908-3586-1-git-send-email-chris@chris-wilson.co.uk> <20140304085831.6cd78551@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id F3754FB7DC for ; Tue, 4 Mar 2014 09:09:59 -0800 (PST) Content-Disposition: inline In-Reply-To: <20140304085831.6cd78551@jbarnes-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Jesse Barnes Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Tue, Mar 04, 2014 at 08:58:31AM -0800, Jesse Barnes wrote: > On Tue, 4 Mar 2014 13:15:08 +0000 > Chris Wilson 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=3D75502 > > Signed-off-by: Chris Wilson > > --- > > 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/i91= 5/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 =3D crtc->dev; > > + struct drm_i915_private *dev_priv =3D dev->dev_private; > > + struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > > + unsigned long flags; > > + bool pending; > > + > > + if (i915_reset_in_progress(&dev_priv->gpu_error) || > > + intel_crtc->reset_counter !=3D atomic_read(&dev_priv->gpu_error.r= eset_counter)) > > + return false; > > + > > + spin_lock_irqsave(&dev->event_lock, flags); > > + pending =3D to_intel_crtc(crtc)->unpin_work !=3D 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 =3D crtc->dev; > > - struct drm_i915_private *dev_priv =3D dev->dev_private; > > - struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > > - unsigned long flags; > > - bool pending; > > - > > - if (i915_reset_in_progress(&dev_priv->gpu_error) || > > - intel_crtc->reset_counter !=3D atomic_read(&dev_priv->gpu_error.r= eset_counter)) > > - return false; > > - > > - spin_lock_irqsave(&dev->event_lock, flags); > > - pending =3D to_intel_crtc(crtc)->unpin_work !=3D 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=E4l=E4 Intel OTC