Re: [PATCH v2 3/5] drm/i915: Make sprite updates atomic

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Jan 20, 2014 at 06:43:42PM +0100, Daniel Vetter wrote:
> On Mon, Jan 20, 2014 at 5:56 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >> > +{
> >> > +   struct drm_device *dev = crtc->dev;
> >> > +   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >> > +   const struct drm_display_mode *mode = &intel_crtc->config.adjusted_mode;
> >> > +   enum pipe pipe = intel_crtc->pipe;
> >> > +   /* FIXME needs to be calibrated sensibly */
> >> > +   unsigned int min = mode->crtc_vblank_start - usecs_to_scanlines(mode, 100);
> >> > +   unsigned int max = mode->crtc_vblank_start - 1;
> >> > +   long timeout = msecs_to_jiffies_timeout(1);
> >> > +   unsigned int scanline;
> >> > +
> >> > +   if (WARN_ON(drm_vblank_get(dev, pipe)))
> >> > +           return;
> >> > +
> >> > +   local_irq_disable();
> >> > +
> >> > +   intel_crtc->vbl_received = false;
> >
> > Now that you got me thinking about barriers again, I wonder if I should
> > add an explicit compiler barrier here. The intel_get_crtc_scanline() call
> > should act as a compiler barrier though, so it shouldn't be needed. So
> > maybe I should add a comment here too?
> 
> This piece of code here was the actual reason I've asked for barrier
> comments ;-) Ofc document the wake_up/wait_even barriers for the irq
> write -> read here is also good, but this write here is imo the
> crucial piece.
> 
> Also I think we should have a check here that the caller is holding
> the crtc lock, to make sure that only one thread is using this
> facility. Oh, and one more while I ponder this: We enable interrupt
> processing before crtcs are fully set up, so chasing the pipe->crtc
> mapping from the irq handling either needs to be done carefully (i.e.
> a small analysis of why we won't ever get an vblank interrupt before
> the crtc is set up) or needs to use something statically allocated in
> dev_priv.

We have the same problem already w/ underrun interrupts at least, no?

-- 
Ville Syrjälä
Intel OTC