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

[Ville Syrjälä]

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?  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?  > > 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 at 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(>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

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

[Ville Syrjälä]

On Fri, Sep 14, 2012 at 05:39:30PM +0200, Daniel Vetter wrote:
> On Fri, Sep 14, 2012 at 5:30 PM, Ville Syrj?l?
>  wrote:
> > 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.
> 
> intel_pipe_set_base is also called in the modeset case, i.e. when we
> could potentially change the height of the mode. And if we wait on a
> large enough scanline which doesn't exist in the new mode this would
> hang.

Yes, I know it's called in both cases. But my point is that there
doesn't seem to be any reason to call it in the pure set_base case.

> The other callsite of finish_fb is from intel_crtc_disable.

Yep. There it does make sense to me.

-- 
Ville Syrj?l?
Intel OTC

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

[Ville Syrjälä]

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?  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 at 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(>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.

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.


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.

-- 
Ville Syrj?l?
Intel OTC

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

[Daniel Vetter]

On Fri, Sep 14, 2012 at 5:30 PM, Ville Syrj?l?
 wrote:
> 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.

intel_pipe_set_base is also called in the modeset case, i.e. when we
could potentially change the height of the mode. And if we wait on a
large enough scanline which doesn't exist in the new mode this would
hang.

The other callsite of finish_fb is from intel_crtc_disable.

btw, some slight digging around with git blame gives you in both cases
the commits and comments that explain this all ;-)

Cheers, Daniel
-- 
Daniel Vetter
daniel.vetter at ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch

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

[Ville Syrjälä]

On Fri, Sep 14, 2012 at 02:57:26PM +0100, Chris Wilson wrote:
> On Wed, 12 Sep 2012 18:47:07 +0300, ville.syrjala at 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(>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?

I've not really dug into the GPU synchronization side and pin/fence stuff,
so it's no surprise my code is a bit fscked up in those areas.

-- 
Ville Syrj?l?
Intel OTC

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

[Chris Wilson]

On Fri, 14 Sep 2012 18:30:21 +0300, Ville Syrj??l??  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??  > 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 at 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(>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?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Fri, 14 Sep 2012 17:21:30 +0300, Ville Syrj??l??  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 at 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(>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.

The other aspect is to synchronize the modeset with any outstanding
pageflips.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Wed, 12 Sep 2012 18:47:07 +0300, ville.syrjala at 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(>struct_mutex);
> +
> + intel_finish_fb(intel_flip->old_bo);

So if I understand correctly, this code is called after the flip is
already complete?

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

> + intel_unpin_fb_obj(intel_flip->old_bo);
> +
> + mutex_unlock(>struct_mutex);
> + }
> +
> +}
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Wed, 12 Sep 2012 18:47:07 +0300, ville.syrj...@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?

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

 + intel_unpin_fb_obj(intel_flip-old_bo);
 +
 + mutex_unlock(dev-struct_mutex);
 + }
 +
 +}
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

On Fri, Sep 14, 2012 at 02:57:26PM +0100, Chris Wilson wrote:
 On Wed, 12 Sep 2012 18:47:07 +0300, ville.syrj...@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?

I've not really dug into the GPU synchronization side and pin/fence stuff,
so it's no surprise my code is a bit fscked up in those areas.

-- 
Ville Syrjälä
Intel OTC
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chris Wilson]

On Fri, 14 Sep 2012 17:21:30 +0300, Ville Syrjälä 
ville.syrj...@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.syrj...@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.

The other aspect is to synchronize the modeset with any outstanding
pageflips.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Fri, Sep 14, 2012 at 5:30 PM, Ville Syrjälä
ville.syrj...@linux.intel.com wrote:
 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.

intel_pipe_set_base is also called in the modeset case, i.e. when we
could potentially change the height of the mode. And if we wait on a
large enough scanline which doesn't exist in the new mode this would
hang.

The other callsite of finish_fb is from intel_crtc_disable.

btw, some slight digging around with git blame gives you in both cases
the commits and comments that explain this all ;-)

Cheers, Daniel
-- 
Daniel Vetter
daniel.vet...@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

On Fri, Sep 14, 2012 at 05:39:30PM +0200, Daniel Vetter wrote:
 On Fri, Sep 14, 2012 at 5:30 PM, Ville Syrjälä
 ville.syrj...@linux.intel.com wrote:
  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.
 
 intel_pipe_set_base is also called in the modeset case, i.e. when we
 could potentially change the height of the mode. And if we wait on a
 large enough scanline which doesn't exist in the new mode this would
 hang.

Yes, I know it's called in both cases. But my point is that there
doesn't seem to be any reason to call it in the pure set_base case.

 The other callsite of finish_fb is from intel_crtc_disable.

Yep. There it does make sense to me.

-- 
Ville Syrjälä
Intel OTC
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chris Wilson]

On Fri, 14 Sep 2012 18:30:21 +0300, Ville Syrjälä 
ville.syrj...@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.syrj...@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.syrj...@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?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

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.syrj...@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.syrj...@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.syrj...@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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

Use the drm_flip helper to implement atomic page flipping.

Work in progress. Ignore the huge mess in intel_sprite.c for now.
---
 drivers/gpu/drm/i915/i915_drv.h  |4 +
 drivers/gpu/drm/i915/i915_irq.c  |   10 +-
 drivers/gpu/drm/i915/intel_atomic.c  |  449 +-
 drivers/gpu/drm/i915/intel_display.c |  171 +
 drivers/gpu/drm/i915/intel_drv.h |   29 +++
 drivers/gpu/drm/i915/intel_sprite.c  |  386 ++---
 6 files changed, 841 insertions(+), 208 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 57e4894..e29994f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -41,6 +41,8 @@
 #include 
 #include 

+#include "drm_flip.h"
+
 /* General customization:
  */

@@ -845,6 +847,8 @@ typedef struct drm_i915_private {
struct work_struct parity_error_work;
bool hw_contexts_disabled;
uint32_t hw_context_size;
+
+   struct drm_flip_driver flip_driver;
 } drm_i915_private_t;

 /* Iterate over initialised rings */
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 23f2ea0..78ff3e0 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -37,6 +37,8 @@
 #include "i915_trace.h"
 #include "intel_drv.h"

+void intel_handle_vblank(struct drm_device *dev, int pipe);
+
 /* For display hotplug interrupt */
 static void
 ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
@@ -548,7 +550,7 @@ static irqreturn_t valleyview_irq_handler(DRM_IRQ_ARGS)

for_each_pipe(pipe) {
if (pipe_stats[pipe] & PIPE_VBLANK_INTERRUPT_STATUS)
-   drm_handle_vblank(dev, pipe);
+   intel_handle_vblank(dev, pipe);

if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
intel_prepare_page_flip(dev, pipe);
@@ -686,7 +688,7 @@ static irqreturn_t ivybridge_irq_handler(DRM_IRQ_ARGS)
intel_finish_page_flip_plane(dev, i);
}
if (de_iir & (DE_PIPEA_VBLANK_IVB << (5 * i)))
-   drm_handle_vblank(dev, i);
+   intel_handle_vblank(dev, i);
}

/* check event from PCH */
@@ -779,10 +781,10 @@ static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
}

if (de_iir & DE_PIPEA_VBLANK)
-   drm_handle_vblank(dev, 0);
+   intel_handle_vblank(dev, 0);

if (de_iir & DE_PIPEB_VBLANK)
-   drm_handle_vblank(dev, 1);
+   intel_handle_vblank(dev, 1);

/* check event from PCH */
if (de_iir & DE_PCH_EVENT) {
diff --git a/drivers/gpu/drm/i915/intel_atomic.c 
b/drivers/gpu/drm/i915/intel_atomic.c
index 0a96d15..6d8d7d3 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -3,6 +3,7 @@

 #include 
 #include 
+#include 

 #include "intel_drv.h"

@@ -1309,6 +1310,46 @@ static void update_props(const struct drm_device *dev,
}
 }

+static int atomic_pipe_commit(struct drm_device *dev,
+ struct intel_atomic_state *state,
+ int pipe);
+
+static int try_atomic_commit(struct drm_device *dev, struct intel_atomic_state 
*s)
+{
+   int ret;
+   int i;
+   u32 pipes = 0;
+
+   for (i = 0; i < dev->mode_config.num_crtc; i++) {
+   struct intel_crtc_state *st = >crtc[i];
+
+   if (st->mode_dirty)
+   return -EAGAIN;
+
+   if (st->fb_dirty)
+   pipes |= 1 << to_intel_crtc(st->crtc)->pipe;
+   }
+
+   for (i = 0; i < dev->mode_config.num_plane; i++) {
+   struct intel_plane_state *st = >plane[i];
+
+   if (st->dirty)
+   pipes |= 1 << to_intel_plane(st->plane)->pipe;
+   }
+
+   if (hweight32(pipes) != 1)
+   return -EAGAIN;
+
+   ret = atomic_pipe_commit(dev, s, ffs(pipes) - 1);
+   if (ret)
+   return ret;
+
+   /* don't restore the old state in end() */
+   s->dirty = false;
+
+   return 0;
+}
+
 static int intel_atomic_commit(struct drm_device *dev, void *state)
 {
struct intel_atomic_state *s = state;
@@ -1325,17 +1366,23 @@ static int intel_atomic_commit(struct drm_device *dev, 
void *state)
if (ret)
return ret;

-   ret = apply_config(dev, s);
+   /* try to apply asynchronously and atomically */
+   ret = try_atomic_commit(dev, s);
+
+   /* fall back to sync/non-atomic */
if (ret) {
-   unpin_fbs(dev, s);
-   unpin_cursors(dev, s);
-   s->restore_hw = true;
-   return ret;
-   }
+