Re: [RFC][PATCH 4/4] drm: i915: Atomic pageflip WIP

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

On Fri, Sep 14, 2012 at 04:56:00PM +0100, Chris Wilson wrote:
> On Fri, 14 Sep 2012 18:30:21 +0300, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Sep 14, 2012 at 03:27:05PM +0100, Chris Wilson wrote:
> > > On Fri, 14 Sep 2012 17:21:30 +0300, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > > On Fri, Sep 14, 2012 at 02:57:26PM +0100, Chris Wilson wrote:
> > > > > On Wed, 12 Sep 2012 18:47:07 +0300, ville.syrjala@linux.intel.com wrote:
> > > > > > +static void intel_flip_finish(struct drm_flip *flip)
> > > > > > +{
> > > > > > +	struct intel_flip *intel_flip =
> > > > > > +		container_of(flip, struct intel_flip, base);
> > > > > > +	struct drm_device *dev = intel_flip->crtc->dev;
> > > > > > +
> > > > > > +	if (intel_flip->old_bo) {
> > > > > > +		mutex_lock(&dev->struct_mutex);
> > > > > > +
> > > > > > +		intel_finish_fb(intel_flip->old_bo);
> > > > > 
> > > > > So if I understand correctly, this code is called after the flip is
> > > > > already complete?
> > > > 
> > > > Yes.
> > > > 
> > > > > The intel_finish_fb() exists to flush pending batches and flips on the
> > > > > current fb, prior to changing the scanout registers. (There is a
> > > > > hardware dependency such that the GPU may be executing a command that
> > > > > required the current modesetting.) In the case of flip completion, all
> > > > > of those dependencies have already been retired and so the finish should
> > > > > be a no-op. And so it should no be required, nor the changes to
> > > > > intel_finish_fb (which should have included a change in the name to
> > > > > indicate that is now taking the fb_obj).
> > > > 
> > > > Actually I'm not quite sure where this intel_finish_fb() call originated.
> > > > Based on the name it didn't make sense to me, but I left it there for
> > > > now. Hmm. OK it came from one patch from Imre while I was on vacation.
> > > > I suppose he got it from intel_pipe_set_base() which does call
> > > > intel_finish_fb() on the old fb. Why does it do that?
> > > 
> > > It all boils down to the modeset being asynchronous to the GPU
> > > processing the command stream. So we may be currently processing a batch
> > > that is waiting on the pipe to go past a particular scanline, and if the
> > > modesetting were to disable that pipe, or to change its size, then we
> > > risk the WAIT_FOR_EVENT never completing - leading to hangcheck
> > > detecting the frozen display and an angry user.
> > 
> > intel_pipe_set_base() won't disable the pipe or change the size,
> > it'll just flip the primary plane. So that doesn't quite explain
> > why the call is there, as opposed to being called just from the
> > full modeset path.
> 
> Hmm, at the time it was a convenient point. Now, it is clearly called too
> late in the modeset sequence. Daniel, fix please. :)
> 
> > Also wouldn't any batch buffer with WAIT_FOR_EVENT be in risk of
> > stalling, not just ones related to the old fb?
> > 
> > > The other aspect is to synchronize the modeset with any outstanding
> > > pageflips.
> > 
> > Right, that does make sense. But doing it from a function called
> > intel_finish_fb() is a bit confusing, as the condition really
> > shouldn't depend on any specific fb object. But I suppose this is
> > just a result of the "only one outstanding flip" policy.
> 
> Again, a nice convenient point, calling it an intel_crtc_wait_*() would
> probably help (after fixing the ordering).
> 
> > BTW regarding this WAIT_FOR_EVENT thing. I got the impression that
> > the scanline window wait doesn't work on recent hardware generations
> > any more. Is that true? I was thinking that perhaps I could use it
> > along with the load register command to perform the flips through
> > the command queue.
> 
> That impression is pretty accurate. There is a suggestion that some form
> of scanline wait was restored for IVB, but driving it seems pretty hit
> and miss. Atomic flipping should all be possible with MI_DISPLAY_FLIP,
> so presumably you are mostly thinking about atomic modeset? Is the
> presumption that it will be an infrequent request and so better to keep
> as simple as possible?

MI_DISPLAY_FLIP is _very_ limited. You can't really do anything
interesting with. Sure it's enough to flip the buffers of some game
or whatever, but it's practically useless for hardware composition
type stuff.

-- 
Ville Syrjälä
Intel OTC