Re: [PATCH 1/3] drm/i915: Wait for pending flips in intel_pipe_set_base()

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

On Fri, Nov 02, 2012 at 01:26:56PM +0000, Chris Wilson wrote:
> On Thu,  1 Nov 2012 20:06:00 +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > intel_pipe_set_base() never actually waited for any pending page flips
> > on the CRTC. It looks like it tried to, by calling intel_finish_fb() on
> > the current front buffer. But the pending flips were actually tracked
> > in the BO of the previous front buffer, so the call to intel_finish_fb()
> > never did anything useful.
> > 
> > Now even the pending_flip counter is gone, so we should just
> > use intel_crtc_wait_for_pending_flips(), but since we're already holding
> > struct_mutex when we would call that function, we need another version
> > of it, that itself doesn't lock struct_mutex.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Your earlier point was that intel_finish_fb() is being called in the wrong
> place, if you fix that first you should not need the major surgery.

I don't think it's the wrong place as such. We do need it for the
panning case. The only issue with the current place is that we end up
calling it twice in the full modeset path; once in crtc_disable(),
and then later in intel_pipe_set_base().

I could move the call up from intel_pipe_set_base() to intel_crtc_set_config()
so that it only gets called for panning. This would also solve the
locking issue, but it doesn't seem as efficient as the current
sequence, because we'd end up pinning the new buffer after waiting
for page flips. With the current sequence the flip can complete in
parallel while we're doing the pin operation.

Another alternative would be to leave the call where it is in
intel_pipe_set_base(), but simply drop and reacquire struct_mutex
around the call. That would avoid the need for the _locked()
variant. That would still leave us with the double call for
full modeset, but I'm not sure that part is worth fixing. If we really
want to fix it, then we could add a paramter to intel_pipe_set_base(),
or maybe look at some other bit of state to detect the full modeset
case, and skip the call.

-- 
Ville Syrjälä
Intel OTC