[PATCH] drm/i915: Flush the pending flips on the CRTC before modification

[PATCH] drm/i915: Flush the pending flips on the CRTC before modification

[Chris Wilson]

This was meant to be the purpose of the
intel_crtc_wait_for_pending_flips() function which is called whilst
preparing the CRTC for a modeset or before disabling. However, as Ville
Syrjala pointed out, we set the pending flip notification on the old
framebuffer that is no longer attached to the CRTC by the time we come
to flush the pending operations. Instead, we can simply wait on the
pending unpin work to be finished on this CRTC, knowning that the
hardware has therefore finished modifying the registers, before proceeding
with our direct access.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |   24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a262326..39df185 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2896,15 +2896,36 @@ 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;
+	unsigned long flags;
+	bool pending;
+
+	if (atomic_read(&dev_priv->mm.wedged))
+		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 void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	flush_work_sync(&to_intel_crtc(crtc)->vblank_work.work);
 
 	if (crtc->fb == NULL)
 		return;
 
+	wait_event(dev_priv->pending_flip_queue,
+		   !intel_crtc_has_pending_flip(crtc));
+
 	mutex_lock(&dev->struct_mutex);
 	intel_finish_fb(crtc->fb);
 	mutex_unlock(&dev->struct_mutex);
@@ -6388,9 +6409,8 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
 
 	atomic_clear_mask(1 << intel_crtc->plane,
 			  &obj->pending_flip.counter);
-	if (atomic_read(&obj->pending_flip) == 0)
-		wake_up(&dev_priv->pending_flip_queue);
 
+	wake_up(&dev_priv->pending_flip_queue);
 	queue_work(dev_priv->wq, &work->work);
 
 	trace_i915_flip_complete(intel_crtc->plane, work->pending_flip_obj);
-- 
1.7.10.4

Re: [PATCH] drm/i915: Flush the pending flips on the CRTC before modification

[Daniel Vetter]

On Thu, Sep 27, 2012 at 09:25:58PM +0100, Chris Wilson wrote:
> This was meant to be the purpose of the
> intel_crtc_wait_for_pending_flips() function which is called whilst
> preparing the CRTC for a modeset or before disabling. However, as Ville
> Syrjala pointed out, we set the pending flip notification on the old
> framebuffer that is no longer attached to the CRTC by the time we come
> to flush the pending operations. Instead, we can simply wait on the
> pending unpin work to be finished on this CRTC, knowning that the
> hardware has therefore finished modifying the registers, before proceeding
> with our direct access.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Can I haz testcase plz?

Shouldn't be too hard to race a few pageflips with dpms off and crtc
disabling, maybe we need to add some busy load onto the gpu first to delay
things for long enough.

Thanks, Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c |   24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a262326..39df185 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2896,15 +2896,36 @@ 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;
> +	unsigned long flags;
> +	bool pending;
> +
> +	if (atomic_read(&dev_priv->mm.wedged))
> +		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 void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	flush_work_sync(&to_intel_crtc(crtc)->vblank_work.work);
>  
>  	if (crtc->fb == NULL)
>  		return;
>  
> +	wait_event(dev_priv->pending_flip_queue,
> +		   !intel_crtc_has_pending_flip(crtc));
> +
>  	mutex_lock(&dev->struct_mutex);
>  	intel_finish_fb(crtc->fb);
>  	mutex_unlock(&dev->struct_mutex);
> @@ -6388,9 +6409,8 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
>  
>  	atomic_clear_mask(1 << intel_crtc->plane,
>  			  &obj->pending_flip.counter);
> -	if (atomic_read(&obj->pending_flip) == 0)
> -		wake_up(&dev_priv->pending_flip_queue);
>  
> +	wake_up(&dev_priv->pending_flip_queue);
>  	queue_work(dev_priv->wq, &work->work);
>  
>  	trace_i915_flip_complete(intel_crtc->plane, work->pending_flip_obj);
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Flush the pending flips on the CRTC before modification

[Chris Wilson]

On Fri, 28 Sep 2012 08:37:20 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Sep 27, 2012 at 09:25:58PM +0100, Chris Wilson wrote:
> > This was meant to be the purpose of the
> > intel_crtc_wait_for_pending_flips() function which is called whilst
> > preparing the CRTC for a modeset or before disabling. However, as Ville
> > Syrjala pointed out, we set the pending flip notification on the old
> > framebuffer that is no longer attached to the CRTC by the time we come
> > to flush the pending operations. Instead, we can simply wait on the
> > pending unpin work to be finished on this CRTC, knowning that the
> > hardware has therefore finished modifying the registers, before proceeding
> > with our direct access.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Can I haz testcase plz?

Compiz? :-p
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Flush the pending flips on the CRTC before modification

[Chris Wilson]

On Fri, 28 Sep 2012 08:37:20 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Sep 27, 2012 at 09:25:58PM +0100, Chris Wilson wrote:
> > This was meant to be the purpose of the
> > intel_crtc_wait_for_pending_flips() function which is called whilst
> > preparing the CRTC for a modeset or before disabling. However, as Ville
> > Syrjala pointed out, we set the pending flip notification on the old
> > framebuffer that is no longer attached to the CRTC by the time we come
> > to flush the pending operations. Instead, we can simply wait on the
> > pending unpin work to be finished on this CRTC, knowning that the
> > hardware has therefore finished modifying the registers, before proceeding
> > with our direct access.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Can I haz testcase plz?

Fixes i-g-t/flip_test
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Flush the pending flips on the CRTC before modification

[Daniel Vetter]

On Fri, Sep 28, 2012 at 01:04:03PM +0100, Chris Wilson wrote:
> On Fri, 28 Sep 2012 08:37:20 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, Sep 27, 2012 at 09:25:58PM +0100, Chris Wilson wrote:
> > > This was meant to be the purpose of the
> > > intel_crtc_wait_for_pending_flips() function which is called whilst
> > > preparing the CRTC for a modeset or before disabling. However, as Ville
> > > Syrjala pointed out, we set the pending flip notification on the old
> > > framebuffer that is no longer attached to the CRTC by the time we come
> > > to flush the pending operations. Instead, we can simply wait on the
> > > pending unpin work to be finished on this CRTC, knowning that the
> > > hardware has therefore finished modifying the registers, before proceeding
> > > with our direct access.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > Can I haz testcase plz?
> 
> Fixes i-g-t/flip_test

Picked up for -fixes (with the i-g-t note and cc: stable added), thanks
for the patch. I've tried to make the test work on pch platforms, too. But
those seem to simply complete the flip immediately when the pipe is off.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Flush the pending flips on the CRTC before modification

[Ville Syrjälä]

On Thu, Sep 27, 2012 at 09:25:58PM +0100, Chris Wilson wrote:
> This was meant to be the purpose of the
> intel_crtc_wait_for_pending_flips() function which is called whilst
> preparing the CRTC for a modeset or before disabling. However, as Ville
> Syrjala pointed out, we set the pending flip notification on the old
> framebuffer that is no longer attached to the CRTC by the time we come
> to flush the pending operations. Instead, we can simply wait on the
> pending unpin work to be finished on this CRTC, knowning that the
> hardware has therefore finished modifying the registers, before proceeding
> with our direct access.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a262326..39df185 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2896,15 +2896,36 @@ 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;
> +	unsigned long flags;
> +	bool pending;
> +
> +	if (atomic_read(&dev_priv->mm.wedged))
> +		return false;
> +
> +	spin_lock_irqsave(&dev->event_lock, flags);
> +	pending = to_intel_crtc(crtc)->unpin_work != NULL;
> +	spin_unlock_irqrestore(&dev->event_lock, flags);

The locking looks pointless here.

> +
> +	return pending;
> +}
> +
>  static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	flush_work_sync(&to_intel_crtc(crtc)->vblank_work.work);
>  
>  	if (crtc->fb == NULL)
>  		return;
>  
> +	wait_event(dev_priv->pending_flip_queue,
> +		   !intel_crtc_has_pending_flip(crtc));
> +
>  	mutex_lock(&dev->struct_mutex);
>  	intel_finish_fb(crtc->fb);
>  	mutex_unlock(&dev->struct_mutex);
> @@ -6388,9 +6409,8 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
>  
>  	atomic_clear_mask(1 << intel_crtc->plane,
>  			  &obj->pending_flip.counter);
> -	if (atomic_read(&obj->pending_flip) == 0)
> -		wake_up(&dev_priv->pending_flip_queue);
>  
> +	wake_up(&dev_priv->pending_flip_queue);
>  	queue_work(dev_priv->wq, &work->work);
>  
>  	trace_i915_flip_complete(intel_crtc->plane, work->pending_flip_obj);

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: Flush the pending flips on the CRTC before modification

[Chris Wilson]

[-- Attachment #1: Type: text/plain, Size: 2092 bytes --]

On Fri, 28 Sep 2012 13:05:51 +0300, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Sep 27, 2012 at 09:25:58PM +0100, Chris Wilson wrote:
> > This was meant to be the purpose of the
> > intel_crtc_wait_for_pending_flips() function which is called whilst
> > preparing the CRTC for a modeset or before disabling. However, as Ville
> > Syrjala pointed out, we set the pending flip notification on the old
> > framebuffer that is no longer attached to the CRTC by the time we come
> > to flush the pending operations. Instead, we can simply wait on the
> > pending unpin work to be finished on this CRTC, knowning that the
> > hardware has therefore finished modifying the registers, before proceeding
> > with our direct access.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |   24 ++++++++++++++++++++++--
> >  1 file changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index a262326..39df185 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2896,15 +2896,36 @@ 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;
> > +	unsigned long flags;
> > +	bool pending;
> > +
> > +	if (atomic_read(&dev_priv->mm.wedged))
> > +		return false;
> > +
> > +	spin_lock_irqsave(&dev->event_lock, flags);
> > +	pending = to_intel_crtc(crtc)->unpin_work != NULL;
> > +	spin_unlock_irqrestore(&dev->event_lock, flags);
> 
> The locking looks pointless here.

It does rather. Being pedagogical we should probably leave a mb of some
sort in there...

  pending = to_intel_crtc(crtc)->unpin_work != NULL;
  smp_rmb();

with the existing spin_lock providing the necessary barriers before the
wake_up();
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Flush the pending flips on the CRTC before modification

[Daniel Vetter]

On Fri, Sep 28, 2012 at 12:22 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, 28 Sep 2012 13:05:51 +0300, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> On Thu, Sep 27, 2012 at 09:25:58PM +0100, Chris Wilson wrote:
>> > This was meant to be the purpose of the
>> > intel_crtc_wait_for_pending_flips() function which is called whilst
>> > preparing the CRTC for a modeset or before disabling. However, as Ville
>> > Syrjala pointed out, we set the pending flip notification on the old
>> > framebuffer that is no longer attached to the CRTC by the time we come
>> > to flush the pending operations. Instead, we can simply wait on the
>> > pending unpin work to be finished on this CRTC, knowning that the
>> > hardware has therefore finished modifying the registers, before proceeding
>> > with our direct access.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c |   24 ++++++++++++++++++++++--
>> >  1 file changed, 22 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index a262326..39df185 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -2896,15 +2896,36 @@ 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;
>> > +   unsigned long flags;
>> > +   bool pending;
>> > +
>> > +   if (atomic_read(&dev_priv->mm.wedged))
>> > +           return false;
>> > +
>> > +   spin_lock_irqsave(&dev->event_lock, flags);
>> > +   pending = to_intel_crtc(crtc)->unpin_work != NULL;
>> > +   spin_unlock_irqrestore(&dev->event_lock, flags);
>>
>> The locking looks pointless here.
>
> It does rather. Being pedagogical we should probably leave a mb of some
> sort in there...
>
>   pending = to_intel_crtc(crtc)->unpin_work != NULL;
>   smp_rmb();
>
> with the existing spin_lock providing the necessary barriers before the
> wake_up();

tbh I don't mind superflous spinlocks laying around ... this is
modeset code, performance at the instruction-counting level totally
does not matter. And just using spinlocks avoids the need to review
barriers ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Flush the pending flips on the CRTC before modification

[Ville Syrjälä]

On Fri, Sep 28, 2012 at 11:22:45AM +0100, Chris Wilson wrote:
> On Fri, 28 Sep 2012 13:05:51 +0300, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Thu, Sep 27, 2012 at 09:25:58PM +0100, Chris Wilson wrote:
> > > This was meant to be the purpose of the
> > > intel_crtc_wait_for_pending_flips() function which is called whilst
> > > preparing the CRTC for a modeset or before disabling. However, as Ville
> > > Syrjala pointed out, we set the pending flip notification on the old
> > > framebuffer that is no longer attached to the CRTC by the time we come
> > > to flush the pending operations. Instead, we can simply wait on the
> > > pending unpin work to be finished on this CRTC, knowning that the
> > > hardware has therefore finished modifying the registers, before proceeding
> > > with our direct access.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c |   24 ++++++++++++++++++++++--
> > >  1 file changed, 22 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index a262326..39df185 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -2896,15 +2896,36 @@ 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;
> > > +	unsigned long flags;
> > > +	bool pending;
> > > +
> > > +	if (atomic_read(&dev_priv->mm.wedged))
> > > +		return false;
> > > +
> > > +	spin_lock_irqsave(&dev->event_lock, flags);
> > > +	pending = to_intel_crtc(crtc)->unpin_work != NULL;
> > > +	spin_unlock_irqrestore(&dev->event_lock, flags);
> > 
> > The locking looks pointless here.
> 
> It does rather. Being pedagogical we should probably leave a mb of some
> sort in there...
> 
>   pending = to_intel_crtc(crtc)->unpin_work != NULL;
>   smp_rmb();
> 
> with the existing spin_lock providing the necessary barriers before the
> wake_up();

IIRC wake_up()/wait_event() already have the necessary barriers. And
based on a quick glance Documentation/memory-barriers.txt seems to
agree with me.

-- 
Ville Syrjälä
Intel OTC