[Spice-devel,v2,10/12] spice/gl: create dummy primary surface (RfC)

Submitted by Gerd Hoffmann on Feb. 19, 2016, 9:14 a.m.

Details

Message ID 1455873289-349-11-git-send-email-kraxel@redhat.com
State New
Headers show
Series "spice: add opengl/virgl/dmabuf support" ( rev: 2 ) in Spice

Not browsing as part of any series.

Commit Message

Gerd Hoffmann Feb. 19, 2016, 9:14 a.m.
Current spice client expects we create a primary surface,
even if we do display updates using dma-bufs exclusively.

So just do that to get things going.

Not fully clear whenever that is intentional or a bug on
the spice side, so I keep this as separate patch for now.

I think this should either be squashed into the previous
patch, or dropped after fixing things on the spice side.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/spice-display.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Patch hide | download patch | download mbox

diff --git a/ui/spice-display.c b/ui/spice-display.c
index 15d7906..96beb02 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -856,13 +856,27 @@  static void spice_gl_switch(DisplayChangeListener *dcl,
 {
     SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
     EGLint stride, fourcc;
+    bool resize = true;
     int fd;
 
+    if (ssd->ds && new_surface &&
+        surface_width(ssd->ds)  == surface_width(new_surface)  &&
+        surface_height(ssd->ds) == surface_height(new_surface) &&
+        surface_format(ssd->ds) == surface_format(new_surface)) {
+        resize = false;
+    }
+
     if (ssd->ds) {
         surface_gl_destroy_texture(ssd->gls, ssd->ds);
+        if (resize) {
+            qemu_spice_destroy_host_primary(ssd);
+        }
     }
     ssd->ds = new_surface;
     if (ssd->ds) {
+        if (resize) {
+            qemu_spice_create_host_primary(ssd);
+        }
         surface_gl_create_texture(ssd->gls, ssd->ds);
         fd = egl_get_fd_for_texture(ssd->ds->texture,
                                     &stride, &fourcc);

Comments

On Fr, 2016-02-19 at 10:14 +0100, Gerd Hoffmann wrote:
> Current spice client expects we create a primary surface,
> even if we do display updates using dma-bufs exclusively.
> 
> So just do that to get things going.
> 
> Not fully clear whenever that is intentional or a bug on
> the spice side, so I keep this as separate patch for now.
> 
> I think this should either be squashed into the previous
> patch, or dropped after fixing things on the spice side.

Ping Marc?  Any comment on this?

thanks,
  Gerd
Hi

On Fri, Mar 18, 2016 at 2:17 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> Ping Marc?  Any comment on this?


Could you send a rebased series, for the patches that lead to the
issue? I assume "render DisplaySurface via opengl" patch?
On Fr, 2016-03-18 at 14:45 +0100, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Mar 18, 2016 at 2:17 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > Ping Marc?  Any comment on this?
> 
> 
> Could you send a rebased series, for the patches that lead to the
> issue?

Update pushed to https://www.kraxel.org/cgit/qemu/log/?h=work/virgl
with some cherry-picks to fix nettle build issue and to sidestep
conflict with in-flight pull request.

That work better for testing.  I'll resent the patch series once the
temporary issues with master are settled.  There hasn't changed much in
the code though, other than rebasing and minor tweaks to adapt to the
include changes.

>  I assume "render DisplaySurface via opengl" patch?

Yes, that one.

cheers,
  Gerd
Hi

On Mon, Mar 21, 2016 at 11:24 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On Fr, 2016-03-18 at 14:45 +0100, Marc-André Lureau wrote:
>> Hi
>>
>> On Fri, Mar 18, 2016 at 2:17 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> > Ping Marc?  Any comment on this?
>>
>>
>> Could you send a rebased series, for the patches that lead to the
>> issue?
>
> Update pushed to https://www.kraxel.org/cgit/qemu/log/?h=work/virgl
> with some cherry-picks to fix nettle build issue and to sidestep
> conflict with in-flight pull request.
>
> That work better for testing.  I'll resent the patch series once the
> temporary issues with master are settled.  There hasn't changed much in
> the code though, other than rebasing and minor tweaks to adapt to the
> include changes.
>
>>  I assume "render DisplaySurface via opengl" patch?
>
> Yes, that one.

I have done some more testing and sent a series for spice-gtk fixing
display with gl scanout-only case. And a minor patch to spice server
to solve a cursor initialization when there is no canvas. Your series
works ok with that, only when doing a reboot, virtio_gpu_virgl_reset()
calls spice_qxl_gl_scanout( fd = -1), and later spice_gl_refresh()
calls spice_qxl_gl_draw_async() and reaches the following error:

(process:21117): Spice-CRITICAL **:
red-qxl.c:900:spice_qxl_gl_draw_async: condition
`qxl_state->scanout.drm_dma_buf_fd != -1' failed
Hi,

Resuming to work on this after 2.6 freeze break ...

> I have done some more testing and sent a series for spice-gtk fixing
> display with gl scanout-only case. And a minor patch to spice server
> to solve a cursor initialization when there is no canvas. Your series
> works ok with that, only when doing a reboot, virtio_gpu_virgl_reset()
> calls spice_qxl_gl_scanout( fd = -1), and later spice_gl_refresh()
> calls spice_qxl_gl_draw_async() and reaches the following error:
> 
> (process:21117): Spice-CRITICAL **:
> red-qxl.c:900:spice_qxl_gl_draw_async: condition
> `qxl_state->scanout.drm_dma_buf_fd != -1' failed

Hmm, that is sort-of intentional.  It's valid for scanouts to not be
backed by a resource, and in that case qemu_spice_gl_scanout() calls
spice_qxl_gl_scanout() with fd=-1.

So, what do you think we should do here?  Fix spice to handle that case?
Should qemu do something else instead?  Such as not calling
spice_qxl_gl_scanout() to keep the previous dma-buf alive?

cheers,
  Gerd
Hi

On Mon, May 23, 2016 at 3:52 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
> Resuming to work on this after 2.6 freeze break ...
>
>> I have done some more testing and sent a series for spice-gtk fixing
>> display with gl scanout-only case. And a minor patch to spice server
>> to solve a cursor initialization when there is no canvas. Your series
>> works ok with that, only when doing a reboot, virtio_gpu_virgl_reset()
>> calls spice_qxl_gl_scanout( fd = -1), and later spice_gl_refresh()
>> calls spice_qxl_gl_draw_async() and reaches the following error:
>>
>> (process:21117): Spice-CRITICAL **:
>> red-qxl.c:900:spice_qxl_gl_draw_async: condition
>> `qxl_state->scanout.drm_dma_buf_fd != -1' failed
>
> Hmm, that is sort-of intentional.  It's valid for scanouts to not be
> backed by a resource, and in that case qemu_spice_gl_scanout() calls
> spice_qxl_gl_scanout() with fd=-1.

Ok, then I suppose the display should be marked as non-ready, and the
client should reflect that (blank display or "not ready" message)

>
> So, what do you think we should do here?  Fix spice to handle that case?
> Should qemu do something else instead?  Such as not calling
> spice_qxl_gl_scanout() to keep the previous dma-buf alive?

I can't really make sense of a call to spice_qxl_gl_draw_async() if
there is no scanout backing. So I can imagine the fix is on qemu side.

Do you have an up to date branch for testing?

thanks
On Mo, 2016-05-23 at 16:03 +0200, Marc-André Lureau wrote:
> Hi
> 
> On Mon, May 23, 2016 at 3:52 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >   Hi,
> >
> > Resuming to work on this after 2.6 freeze break ...
> >
> >> I have done some more testing and sent a series for spice-gtk fixing
> >> display with gl scanout-only case. And a minor patch to spice server
> >> to solve a cursor initialization when there is no canvas.

What is the upstream status of this?  Built a bunch of fresh packages
today, including new spice-server and spice-gtk from master branch.  And
a bunch spice-gtk dependencies, due to soname change.  Still not working
without the dummy primary surface.

> >> (process:21117): Spice-CRITICAL **:
> >> red-qxl.c:900:spice_qxl_gl_draw_async: condition
> >> `qxl_state->scanout.drm_dma_buf_fd != -1' failed

> I can't really make sense of a call to spice_qxl_gl_draw_async() if
> there is no scanout backing. So I can imagine the fix is on qemu side.

Indeed.  That one should be fixed now.

> Do you have an up to date branch for testing?

Pushed fresh branch to the usual work/virgl location.

cheers,
  Gerd