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

On Tue, May 31, 2016 at 8:18 AM, Gerd Hoffmann <kraxel at redhat.com>
wrote:
>> >
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().
>
> virtio-gpu is a bit simplified compared to real hardware, so there
isn't
> really separate plane/crtc state.
>
> Right now the virtual outputs are linked to drm_crtc.  To apply any
> changes I need to lookup the crtc to figure which virtual output should
> be updated.
>
> So, setting active_only should make sure I have a valid crtc pointer on
> plane updates, right?  It probably also skips the disable + enable crtc
> steps on commit?  What happens when outputs are disabled?
>
> Maybe it makes sense to link our virtual outputs to (primary) planes not
> crtcs?
Nah, I just misunderstood your patch. If it's all about finding the
corresponding crtc, then you're all good. I thought there was some
other reason (like the virtual hw getting upset about certain things).
I'll pick up your patch into my nonblocking atomic branch to make sure
virtio isn't accidentally broken. btw can you pls drop an ack or r-b
onto my virtio conversion? I already added your tested-by.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Hi,
> > Right now the virtual outputs are linked to drm_crtc.  To apply any
> > changes I need to lookup the crtc to figure which virtual output
should
> > be updated.
> > So, setting active_only should make sure I have a valid crtc pointer
on
> > plane updates, right?  It probably also skips the disable + enable
crtc
> > steps on commit?  What happens when outputs are disabled?
> Nah, I just misunderstood your patch. If it's all about finding the
> corresponding crtc, then you're all good.
Yes, it's all about finding the crtc.
> I thought there was some
> other reason (like the virtual hw getting upset about certain things).
virtio wouldn't be upset.

It's a pointless exercise though to first disable the output, just to
re-enable it the next moment with the new page-flipped framebuffer.  So
I guess I should look at the active_only thing nevertheless.
> btw can you pls drop an ack or r-b
> onto my virtio conversion? I already added your tested-by.
Grr, mail is not in my dri-devel folder.  Guess that is the <censored>
"avoid-duplicates" mailman option at work.

Feel free to just add the r-b too.  Or I'll send it for the next version
of the series.

cheers,
  Gerd

On Tue, May 31, 2016 at 9:34 AM, Gerd Hoffmann <kraxel at redhat.com>
wrote:
>   Hi,
>
>> > Right now the virtual outputs are linked to drm_crtc.  To apply
any
>> > changes I need to lookup the crtc to figure which virtual output
should
>> > be updated.
>
>> > So, setting active_only should make sure I have a valid crtc
pointer on
>> > plane updates, right?  It probably also skips the disable + enable
crtc
>> > steps on commit?  What happens when outputs are disabled?
>
>> Nah, I just misunderstood your patch. If it's all about finding the
>> corresponding crtc, then you're all good.
>
> Yes, it's all about finding the crtc.
>
>> I thought there was some
>> other reason (like the virtual hw getting upset about certain things).
>
> virtio wouldn't be upset.
>
> It's a pointless exercise though to first disable the output, just to
> re-enable it the next moment with the new page-flipped framebuffer.  So
> I guess I should look at the active_only thing nevertheless.
It's still possible that the plane can get disabled without it getting
enabled. Userspace is allowed to do this. But since this suprises a
lot of driver writers there's a special atomic_plane_disable hook you
can use for the disable path. Instead of hand-rolling that check in
your own code.
>> btw can you pls drop an ack or r-b
>> onto my virtio conversion? I already added your tested-by.
>
> Grr, mail is not in my dri-devel folder.  Guess that is the
<censored>
> "avoid-duplicates" mailman option at work.
>
> Feel free to just add the r-b too.  Or I'll send it for the next
version
> of the series.
Added your r-b locally so it won't get lost on the next round.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch