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

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

[Alex Williamson]

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.

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;
+    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;
-- 
2.40.1

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

[Philippe Mathieu-Daudé]

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()).

> 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;
> +    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;

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

[Kim, Dongwon]

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;
>

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

[Marc-André Lureau]

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?
thanks

-- 
Marc-André Lureau

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

[Alex Williamson]

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 */
        };
};

Thanks,
Alex

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

[Marc-André Lureau]

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.

-- 
Marc-André Lureau

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

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

[Alex Williamson]

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

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

[Cédric Le Goater]

On 9/13/23 21:18, Alex Williamson wrote:
> 
> 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?

This does fix a regression reproducible on systems with an Intel Gen 8,
my T480 laptop for instance.

Tested-by: Cédric Le Goater <clg@redhat.com>

Also, queuing it in vfio-next.

Thanks,

C.


> 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
> 
> 

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

[Michael Tokarev]

22.09.2023 12:38, Cédric Le Goater wrote:
> On 9/13/23 21:18, Alex Williamson wrote:
>>
>> 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?
> 
> This does fix a regression reproducible on systems with an Intel Gen 8,
> my T480 laptop for instance.
> 
> Tested-by: Cédric Le Goater <clg@redhat.com>
> 
> Also, queuing it in vfio-next.
>

Is it not https://gitlab.com/qemu-project/qemu/-/issues/1891 ?

/mjt

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

[Cédric Le Goater]

On 9/22/23 11:49, Michael Tokarev wrote:
> 22.09.2023 12:38, Cédric Le Goater wrote:
>> On 9/13/23 21:18, Alex Williamson wrote:
>>>
>>> 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?
>>
>> This does fix a regression reproducible on systems with an Intel Gen 8,
>> my T480 laptop for instance.
>>
>> Tested-by: Cédric Le Goater <clg@redhat.com>
>>
>> Also, queuing it in vfio-next.
>>
> 
> Is it not https://gitlab.com/qemu-project/qemu/-/issues/1891 ?

It is.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1891

Thanks,

C.

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

[Michael Tokarev]

22.09.2023 12:38, Cédric Le Goater:
> On 9/13/23 21:18, Alex Williamson wrote:
>>
>> 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?
> 
> This does fix a regression reproducible on systems with an Intel Gen 8,
> my T480 laptop for instance.
> 
> Tested-by: Cédric Le Goater <clg@redhat.com>
> 
> Also, queuing it in vfio-next.

Cédric, can we get this in time for 8.1.2 please
(which I'm planning for Oct-14 for now)?  Looks like
it hit quite some people already.

I dunno what are your plans for vfio-next, maybe this
one (which missed 8.1.0 already) can be pushed as a
bugfix?

Thank you!

/mjt

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

[Cédric Le Goater]

On 10/3/23 19:03, Michael Tokarev wrote:
> 22.09.2023 12:38, Cédric Le Goater:
>> On 9/13/23 21:18, Alex Williamson wrote:
>>>
>>> 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?
>>
>> This does fix a regression reproducible on systems with an Intel Gen 8,
>> my T480 laptop for instance.
>>
>> Tested-by: Cédric Le Goater <clg@redhat.com>
>>
>> Also, queuing it in vfio-next.
> 
> Cédric, can we get this in time for 8.1.2 please
> (which I'm planning for Oct-14 for now)?  Looks like
> it hit quite some people already.

yes. I would like to include this series [1] from Eric in my next PR.
It doesn't need much, an Ack from the s390 team mostly, the rest is
aesthetic. So, I hope this week, else I will send a smaller PR.

Thanks,

C.

[1] https://lore.kernel.org/qemu-devel/20231003101530.288864-1-eric.auger@redhat.com/


> 
> I dunno what are your plans for vfio-next, maybe this
> one (which missed 8.1.0 already) can be pushed as a
> bugfix?
> 
> Thank you!
> 
> /mjt
> 

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

[Edmund Raile]

Hi,
I can confirm that the patch indeed fixes the issue.
Kind regards,
Edmund Raile

Tested-by: Edmund Raile <edmund.raile@proton.me>