[Intel-gfx] [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 at chris-wilson.co.uk> wrote:
> On Fri, 28 Sep 2012 13:05:51 +0300, Ville Syrjälä <ville.syrjala at 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 at 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