* [PATCH] drm/i915: Reject changes of fb base when we have a flip pending
@ 2014-03-04 13:15 Chris Wilson
2014-03-04 16:58 ` Jesse Barnes
0 siblings, 1 reply; 3+ messages in thread
From: Chris Wilson @ 2014-03-04 13:15 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/i915: Reject changes of fb base when we have a flip pending
2014-03-04 13:15 [PATCH] drm/i915: Reject changes of fb base when we have a flip pending Chris Wilson
@ 2014-03-04 16:58 ` Jesse Barnes
2014-03-04 17:09 ` Ville Syrjälä
0 siblings, 1 reply; 3+ messages in thread
From: Jesse Barnes @ 2014-03-04 16:58 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/i915: Reject changes of fb base when we have a flip pending
2014-03-04 16:58 ` Jesse Barnes
@ 2014-03-04 17:09 ` Ville Syrjälä
0 siblings, 0 replies; 3+ messages in thread
From: Ville Syrjälä @ 2014-03-04 17:09 UTC (permalink / raw)
To: Jesse Barnes; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-03-04 17:09 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-04 13:15 [PATCH] drm/i915: Reject changes of fb base when we have a flip pending Chris Wilson
2014-03-04 16:58 ` Jesse Barnes
2014-03-04 17:09 ` Ville Syrjälä
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox