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

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

Hi Gerd,

Some consultation would be appreciated on this thread to get this patch
out of limbo.  Is there a better solution that what I've proposed?
AFAICT, we don't have position fields to indicate the dmabuf plane is
relative to some scanout, so I think it represents the entire display.
Thanks,

Alex

On Tue, 5 Sep 2023 09:09:07 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> 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