Linux Virtualization - May 2016 - [PATCH v3 7/7] [wip] virtio-gpu: add page flip support

On Fri, May 27, 2016 at 09:50:27AM +0200, Daniel Vetter
wrote:
> On Fri, May 27, 2016 at 09:46:03AM +0200, Gerd Hoffmann wrote:
> > On Mi, 2016-05-25 at 18:37 +0200, Daniel Vetter wrote:
> > > On Fri, Oct 2, 2015 at 1:58 PM, Gerd Hoffmann <kraxel at
redhat.com> wrote:
> > > > Signed-off-by: Gerd Hoffmann <kraxel at redhat.com>
> > > 
> > > So I entirely missed this, but this isn't really how to
implement
> > > page_flip for an atomic driver. Working on some stuff and will
hack up
> > > a likely totally broken patch, but should be enough as guideline.
> > > -Daniel
> > 
> > Hmm, no patch in my inbox yet.  Care to send it over or give some
hints
> > what is wrong with this?
> 
> You should use drm_atomic_helper_page_flip as .page_flip hook instead of
> rolling your own, when you have a brand-new atomic driver. If that helper
> doesn't work that means you have a bug in your driver somewhere.
> 
> And indeed virtio just plugs in drm_atomic_helper_commit, which thus far
> doesnt' do nonblocking commits, which means the page flip stuff
won't
> work. I'm working on generic nonblocking atomic to fix this up.
Currently
> still wip and buggy though, hence no patches.
> 
> But I'll take you up on the implied offer to help out and test ;-)
Ok, patches are now on the list and also in my fdo repo at:

https://cgit.freedesktop.org/~danvet/drm/log/

git://people.freedesktop.org/~danvet/drm stuff

Would be really awesome if you could test this on virtio. Note that the
new nonblocking helpers require that your atomic backend gets the drm
event handling right. So if there's a bug in that logic then you'll see
lots of dmesg noise about waits timing out (after 10s of waiting). From a
quick inspection it should work though.

Upshot is that once you get normal modeset to work with those new helpers,
then nonblocking should Just Work, and there's no need anymore for
additional testing. Or at least shouldn't.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Hi,
> > But I'll take you up on the implied offer to help out and test ;-)
> 
> git://people.freedesktop.org/~danvet/drm stuff
Tried that branch.
> Would be really awesome if you could test this on virtio. Note that the
> new nonblocking helpers require that your atomic backend gets the drm
> event handling right. So if there's a bug in that logic then you'll
see
> lots of dmesg noise about waits timing out (after 10s of waiting). From a
> quick inspection it should work though.
No timeouts.  Yay!

But it seems crtcs can be (temporarely) disabled now, so we might have
to pick up the crtc from old_state in virtio_gpu_plane_atomic_update to
figure which virtual output needs to be turned off.  Ran into this last
week already.  Happened with multihead setups only, but the same patch
fixes this one too ;)

https://lists.freedesktop.org/archives/dri-devel/2016-May/108772.html

cheers,
  Gerd

On Mon, May 30, 2016 at 02:06:50PM +0200, Gerd Hoffmann
wrote:
>   Hi,
> 
> > > But I'll take you up on the implied offer to help out and
test ;-)
> > 
> > git://people.freedesktop.org/~danvet/drm stuff
> 
> Tried that branch.
> 
> > Would be really awesome if you could test this on virtio. Note that
the
> > new nonblocking helpers require that your atomic backend gets the drm
> > event handling right. So if there's a bug in that logic then
you'll see
> > lots of dmesg noise about waits timing out (after 10s of waiting).
From a
> > quick inspection it should work though.
> 
> No timeouts.  Yay!
> 
> But it seems crtcs can be (temporarely) disabled now, so we might have
> to pick up the crtc from old_state in virtio_gpu_plane_atomic_update to
> figure which virtual output needs to be turned off.  Ran into this last
> week already.  Happened with multihead setups only, but the same patch
> fixes this one too ;)
> 
> https://lists.freedesktop.org/archives/dri-devel/2016-May/108772.html
Hm, smells more like virtio isn't too happy with the default ordering of
the commit operation. The default is:

- Disable any crtc/encoders that need to be disabled/change.
- Bash new plane setup into hw.
- Enable all crtcs/encoders that need to be enabled/have changed.

There's two problems:
- some hw gets real grumpy if you bash in plane state without the crtc
  state yet matching.
- if you do runtime pm nothing is enabled and the writes get lost at best,
  or hang your interconnect at worst.

That's why you can overwrite atomic_commit_tail, and use something more
sensible. See for example what it looks like for rockchip. I have a gut
feeling that should also take care of your troubles. Using the old crtc is
definitely not what you want.

Another option is that virtio isn't happy about bashing in plane state for
disabled crtc. Again helpers have you covered, look at the active_only
parameter for drm_atomic_helper_commit_planes().

Aside, if you wonder why these defaults: They match what the crtc helpers
are doing, but they are a bit surprising indeed.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch