| Message ID | 1348777558-17881-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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);
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
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.
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
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
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);
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(-)