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. 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
