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

[Chris Wilson]

On Fri, 2 Nov 2012 16:02:39 +0200, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> On Fri, Nov 02, 2012 at 01:26:56PM +0000, Chris Wilson wrote:
> > On Thu,  1 Nov 2012 20:06:00 +0200, ville.syrjala at linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala at 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 at 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.

Oh well, I thought we could arrange the code such that we only had a
single place were we needed to wait. The simplicity of that was
appealing. In light of that, your approach looks reasonable.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre