Re: [PATCH 6/6] drm/i915: Really wait for pending flips in intel_pipe_set_base()

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

On Wed, Feb 13, 2013 at 06:11:00PM +0100, Daniel Vetter wrote:
> On Wed, Feb 13, 2013 at 6:06 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> > index a2e04f7..7e047c1 100644
> >> > --- a/drivers/gpu/drm/i915/intel_display.c
> >> > +++ b/drivers/gpu/drm/i915/intel_display.c
> >> > @@ -2330,8 +2330,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
> >> >             return ret;
> >> >     }
> >> >
> >> > -   if (crtc->fb)
> >> > -           intel_finish_fb(crtc->fb);
> >> > +   intel_crtc_wait_for_pending_flips_locked(crtc);
> >>
> >> Ah, I see now why you grab dev->struct_mutex and need to kick waiters from
> >> the pending flip queue. I think grabbing the mutex twice isn't a major
> >> offense, since both the crtc disable code and set_base are slowpaths used
> >> rarely. So what about simply calling wait_for_pending_flips before
> >> grabbing the mutex in intel_pipe_set_base? We could then also inline
> >> finish_fb into it's only callsite ...
> >
> > I didn't want to slow down intel_pipe_set_base() too much. If we wait
> > for pending flips before pinning the new fb, we can never achieve any
> > parallelism there. But if no-one cares about that, we can reorder.
> 
> I'm confused here - where can we extract parallelism in set_base
> between waiting for pending flips and the pinning? And imo set_base
> isn't really critical: It's officially a synchronous thing (we have a
> vblank wait in there), and if we want to fix that imo the nuclear
> pageflip should be the answer.

The flip is making progress on the GPU side, and at the same time the
CPU side can make some progress with the pin operation. At least that
was my theory.

-- 
Ville Syrjälä
Intel OTC