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

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

On Thu, Feb 13, 2014 at 04:01:52PM +0000, Chris Wilson wrote:
> On Thu, Feb 13, 2014 at 05:42:52PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Add a mechanism by which we can evade the leading edge of vblank. This
> > guarantees that no two sprite register writes will straddle on either
> > side of the vblank start, and that means all the writes will be latched
> > together in one atomic operation.
> > 
> > We do the vblank evade by checking the scanline counter, and if it's too
> > close to the start of vblank (too close has been hardcoded to 100usec
> > for now), we will wait for the vblank start to pass. In order to
> > eliminate random delayes from the rest of the system, we operate with
> > interrupts disabled, except when waiting for the vblank obviously.
> > 
> > Note that we now go digging through pipe_to_crtc_mapping[] in the
> > vblank interrupt handler, which is a bit dangerous since we set up
> > interrupts before the crtcs. However in this case since it's the vblank
> > interrupt, we don't actually unmask it until some piece of code
> > requests it.
> > 
> > v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
> >     Hook up the vblank irq stuff on BDW as well
> > v3: Pass intel_crtc instead of drm_crtc (Daniel)
> >     Warn if crtc.mutex isn't locked (Daniel)
> >     Add an explicit compiler barrier and document the barriers (Daniel)
> >     Note the irq vs. modeset setup madness in the commit message (Daniel)
> > v4: Use prepare_to_wait() & co. directly and eliminate vbl_received
> 
> intel_pipe_update_start / intel_pipe_update_end are unbalanced (end()
> does too much in the cases where start() failed.)

Ah the drm_vblank_get() fail. Should never happen. But I guess I could
make intel_pipe_update_start() return something to tell the caller
whether it needs to call intel_pipe_update_end().

> 
> intel_pipe_update_start should check for min <= 0 (i.e.
> usecs_to_scanline() returns a value greater than vblank_start).

Mode w/ vertical active portion of < 100 usec. Sounds rather unlikely,
but I suppose I could add a check for that too while I'm at it.

> 
> intel_pipe_update_end() could do a sanity check that it is in the same
> frame as start() and so give us warning when the code is broken.

I think I had something like that a long time, but it vanished along the
way. I'll add it. Getting a clear indication that things went south is
better than just some silent glitch on the screen. On gen2 we don't have
a frame counter but since it always returns 0, I don't even need to
add a special case. We just can't detect the failure on gen2.

> 
> Looks like drm_handle_vblank() could be moved to
> intel_pipe_handle_vblank() for a small refactoring win.

Hmm, yeah. I just need to make intel_pipe_handle_vblank() propagate
the return value from drm_handle_vblank(). Seems cleaner, consider
it done.

-- 
Ville Syrjälä
Intel OTC