Re: [8.1 regression] Re: [PULL 05/19] virtio-gpu-udmabuf: correct naming of QemuDmaBuf size properties

[Alex Williamson <alex.williamson@redhat.com>]

On Mon, 21 Aug 2023 14:20:38 +0400
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:

> Hi Alex
> 
> On Thu, Aug 17, 2023 at 1:25 AM Alex Williamson
> <alex.williamson@redhat.com> wrote:
> >
> > On Wed, 16 Aug 2023 15:08:10 -0600
> > Alex Williamson <alex.williamson@redhat.com> wrote:  
> > > > diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
> > > > index 8f9fbf583e..3d19dbe382 100644
> > > > --- a/ui/egl-helpers.c
> > > > +++ b/ui/egl-helpers.c
> > > > @@ -314,9 +314,9 @@ void egl_dmabuf_import_texture(QemuDmaBuf *dmabuf)
> > > >      }
> > > >
> > > >      attrs[i++] = EGL_WIDTH;
> > > > -    attrs[i++] = dmabuf->width;
> > > > +    attrs[i++] = dmabuf->backing_width;
> > > >      attrs[i++] = EGL_HEIGHT;
> > > > -    attrs[i++] = dmabuf->height;
> > > > +    attrs[i++] = dmabuf->backing_height;
> > > >      attrs[i++] = EGL_LINUX_DRM_FOURCC_EXT;
> > > >      attrs[i++] = dmabuf->fourcc;
> > > >
> > > > diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
> > > > index 42db1bb6cf..eee821d73a 100644
> > > > --- a/ui/gtk-egl.c
> > > > +++ b/ui/gtk-egl.c
> > > > @@ -262,9 +262,10 @@ void gd_egl_scanout_dmabuf(DisplayChangeListener *dcl,
> > > >      }
> > > >
> > > >      gd_egl_scanout_texture(dcl, dmabuf->texture,
> > > > -                           dmabuf->y0_top, dmabuf->width, dmabuf->height,
> > > > -                           dmabuf->x, dmabuf->y, dmabuf->scanout_width,
> > > > -                           dmabuf->scanout_height, NULL);
> > > > +                           dmabuf->y0_top,
> > > > +                           dmabuf->backing_width, dmabuf->backing_height,
> > > > +                           dmabuf->x, dmabuf->y, dmabuf->width,
> > > > +                           dmabuf->height, NULL);
> > > >
> > > >      if (dmabuf->allow_fences) {
> > > >          vc->gfx.guest_fb.dmabuf = dmabuf;
> > > > @@ -284,7 +285,8 @@ void gd_egl_cursor_dmabuf(DisplayChangeListener *dcl,
> > > >          if (!dmabuf->texture) {
> > > >              return;
> > > >          }
> > > > -        egl_fb_setup_for_tex(&vc->gfx.cursor_fb, dmabuf->width, dmabuf->height,
> > > > +        egl_fb_setup_for_tex(&vc->gfx.cursor_fb,
> > > > +                             dmabuf->backing_width, dmabuf->backing_height,
> > > >                               dmabuf->texture, false);
> > > >      } else {
> > > >          egl_fb_destroy(&vc->gfx.cursor_fb);
> > > > diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c
> > > > index a9a7fdf50c..4513d3d059 100644
> > > > --- a/ui/gtk-gl-area.c
> > > > +++ b/ui/gtk-gl-area.c
> > > > @@ -301,9 +301,10 @@ void gd_gl_area_scanout_dmabuf(DisplayChangeListener *dcl,
> > > >      }
> > > >
> > > >      gd_gl_area_scanout_texture(dcl, dmabuf->texture,
> > > > -                               dmabuf->y0_top, dmabuf->width, dmabuf->height,
> > > > -                               dmabuf->x, dmabuf->y, dmabuf->scanout_width,
> > > > -                               dmabuf->scanout_height, NULL);
> > > > +                               dmabuf->y0_top,
> > > > +                               dmabuf->backing_width, dmabuf->backing_height,
> > > > +                               dmabuf->x, dmabuf->y, dmabuf->width,
> > > > +                               dmabuf->height, NULL);
> > > >
> > > >      if (dmabuf->allow_fences) {
> > > >          vc->gfx.guest_fb.dmabuf = dmabuf;  
> > >  
> >
> > I suspect the issues is in these last few chunks where width and height
> > are replaced with backing_width and backing height, but
> > hw/vfio/display.c never sets backing_*.  It appears that the following
> > resolves the issue:
> >
> > diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> > index bec864f482f4..837d9e6a309e 100644
> > --- a/hw/vfio/display.c
> > +++ b/hw/vfio/display.c
> > @@ -243,6 +243,8 @@ static VFIODMABuf *vfio_display_get_dmabuf(VFIOPCIDevice *vdev,
> >      dmabuf->dmabuf_id  = plane.dmabuf_id;
> >      dmabuf->buf.width  = plane.width;
> >      dmabuf->buf.height = plane.height;
> > +    dmabuf->buf.backing_width = plane.width;
> > +    dmabuf->buf.backing_height = plane.height;
> >      dmabuf->buf.stride = plane.stride;
> >      dmabuf->buf.fourcc = plane.drm_format;
> >      dmabuf->buf.modifier = plane.drm_format_mod;
> >
> > I'll post that formally, but I really have no idea how dmabuf display
> > works, so confirmation would be appreciated.  Thanks,  
> 
> Looks correct to me. I wish Kim would chime in.
> 
> I am not familiar with vfio/display. Looking at the kernel side, it
> seems it doesn't have a concept for scanout geometry that is different
> from the backing dmabuf/texture dimension.
> 
> Should we make this a blocker for release? Are you sending the patch?

I did send a patch, Kim commented there:

https://lore.kernel.org/all/20230816215550.1723696-1-alex.williamson@redhat.com/

Follow-up suggest vhost-user-gpu is also affected.  Empirically the
patch I sent works, I think it's correct, but Gerd is probably most
qualified to respond to the comments.  I don't know how a "scanout"
relates to what we provide in vfio_device_gfx_plane_info.  Thanks,

Alex