[v2,10/12] spice/gl: create dummy primary surface (RfC)
diff mbox

Message ID 1455873289-349-11-git-send-email-kraxel@redhat.com
State New
Headers show

Commit Message

Gerd Hoffmann Feb. 19, 2016, 9:14 a.m. UTC
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(+)

Comments

Gerd Hoffmann March 18, 2016, 1:17 p.m. UTC | #1
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
Marc-André Lureau March 18, 2016, 1:45 p.m. UTC | #2
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?
Gerd Hoffmann March 21, 2016, 10:24 a.m. UTC | #3
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
Marc-André Lureau March 21, 2016, 6:24 p.m. UTC | #4
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
Gerd Hoffmann May 23, 2016, 1:52 p.m. UTC | #5
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
Marc-André Lureau May 23, 2016, 2:03 p.m. UTC | #6
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
Gerd Hoffmann May 24, 2016, 1:39 p.m. UTC | #7
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

Patch
diff 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);