Re: [PATCH for-8.1] vfio/display: Fix missing update to set backing fields

[Alex Williamson]

On Mon, 4 Sep 2023 21:00:53 +0400
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> Hi
> 
> On Mon, Sep 4, 2023 at 6:11 PM Alex Williamson
> <alex.williamson@redhat.com> wrote:
> >
> > On Mon, 4 Sep 2023 15:06:21 +0400
> > Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> >  
> > > Hi
> > >
> > > On Thu, Aug 17, 2023 at 8:29 PM Kim, Dongwon <dongwon.kim@intel.com> 
> > > wrote:  
> > > >
> > > > Ok, this regression happened not just because of renaming. Originally
> > > > width and height were representing the size of whole surface that guest
> > > > shares while scanout width and height are for the each scanout. We
> > > > realized backing_width/height are more commonly used to specify the size
> > > > of the whole guest surface so put them in the place of width/height then
> > > > replaced scanout_width/height as well with normal width/height.
> > > >
> > > > On 8/16/2023 3:31 PM, Philippe Mathieu-Daudé wrote:  
> > > > > On 16/8/23 23:55, Alex Williamson wrote:  
> > > > >> The below referenced commit renames scanout_width/height to
> > > > >> backing_width/height, but also promotes these fields in various 
> > > > >> portions
> > > > >> of the egl interface.  Meanwhile vfio dmabuf support has never used 
> > > > >> the
> > > > >> previous scanout fields and is therefore missed in the update. This
> > > > >> results in a black screen when transitioning from ramfb to dmabuf
> > > > >> display
> > > > >> when using Intel vGPU with these features.  
> > > > >
> > > > > Referenced commit isn't trivial. Maybe because it is too late here.
> > > > > I'd have tried to split it. Anyhow, too late (again).
> > > > >
> > > > > Is vhost-user-gpu also affected? (see VHOST_USER_GPU_DMABUF_SCANOUT
> > > > > in vhost_user_gpu_handle_display()).  
> > > >
> > > > Yeah, backing_width/height should be programmed with plane.width/height
> > > > as well in vhost_user_gpu_handle_display().
> > > >
> > > > Link: 
> > > > https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg02726.html  
> > > > >> Fixes: 9ac06df8b684 ("virtio-gpu-udmabuf: correct naming of
> > > > >> QemuDmaBuf size properties")
> > > > >> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > > >> ---
> > > > >>
> > > > >> This fixes a regression in dmabuf/EGL support for Intel GVT-g and
> > > > >> potentially the mbochs mdev driver as well.  Once validated by those
> > > > >> that understand dmabuf/EGL integration, I'd welcome QEMU maintainers 
> > > > >> to
> > > > >> take this directly for v8.1 or queue it as soon as possible for 
> > > > >> v8.1.1.
> > > > >>
> > > > >>   hw/vfio/display.c | 2 ++
> > > > >>   1 file changed, 2 insertions(+)
> > > > >>
> > > > >> 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;  
> > > >
> > > > One thing to note here is the normal width and height in the QemuDmaBuf
> > > > are of a scanout, which could be just a partial area of the guest plane
> > > > here. So we should not use those as normal width and height of the
> > > > QemuDmaBuf unless it is guaranteed the given guest surface (plane in
> > > > this case) is always of single display's.
> > > >
> > > > https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg04737.html
> > > >  
> > > > >> +    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 agree with what Kim said. Alex, are you sending a new patch?  
> >
> > What would be different?
> >
> > struct vfio_device_gfx_plane_info {
> >         __u32 argsz;
> >         __u32 flags;
> > #define VFIO_GFX_PLANE_TYPE_PROBE (1 << 0)
> > #define VFIO_GFX_PLANE_TYPE_DMABUF (1 << 1)
> > #define VFIO_GFX_PLANE_TYPE_REGION (1 << 2)
> >         /* in */
> >         __u32 drm_plane_type;   /* type of plane: DRM_PLANE_TYPE_* */
> >         /* out */
> >         __u32 drm_format;       /* drm format of plane */
> >         __u64 drm_format_mod;   /* tiled mode */
> >         __u32 width;    /* width of plane */
> >         __u32 height;   /* height of plane */
> >         __u32 stride;   /* stride of plane */
> >         __u32 size;     /* size of plane in bytes, align on page*/
> >         __u32 x_pos;    /* horizontal position of cursor plane */
> >         __u32 y_pos;    /* vertical position of cursor plane*/
> >         __u32 x_hot;    /* horizontal position of cursor hotspot */
> >         __u32 y_hot;    /* vertical position of cursor hotspot */
> >         union {
> >                 __u32 region_index;     /* region index */
> >                 __u32 dmabuf_id;        /* dma-buf id */
> >         };
> > };
> >  
> 
> Perhaps VFIO is missing extra infos to set the actual x/y/w/h
> region(s) of the visible monitor(s). This could be an extra message. I
> am not familiar with the kernel/driver side for this, perhaps it is
> always guaranteed to be the whole plane (+0+0+w*h). In which case, we
> simply to set the QemuDmabuf fields accordingly.

Isn't that what the proposed patch does?  Gerd is likely going to need
to chime in for any sort of authoritative answer.  Gerd?  Thanks,

Alex