[PATCH for-8.1] vfio/display: Fix missing update to set backing fields
Hi, I can confirm that the patch indeed fixes the issue. Kind regards, Edmund Raile Tested-by: Edmund Raile
Re: [PATCH for-8.1] vfio/display: Fix missing update to set backing fields
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 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.au...@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
Re: [PATCH for-8.1] vfio/display: Fix missing update to set backing fields
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 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
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 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
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 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
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
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 wrote:
On Mon, 4 Sep 2023 21:00:53 +0400
Marc-André Lureau wrote:
Hi
On Mon, Sep 4, 2023 at 6:11 PM Alex Williamson
wrote:
On Mon, 4 Sep 2023 15:06:21 +0400
Marc-André Lureau wrote:
Hi
On Thu, Aug 17, 2023 at 8:29 PM Kim, Dongwon 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
---
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
Re: [PATCH for-8.1] vfio/display: Fix missing update to set backing fields
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 wrote:
> On Mon, 4 Sep 2023 21:00:53 +0400
> Marc-André Lureau wrote:
>
> > Hi
> >
> > On Mon, Sep 4, 2023 at 6:11 PM Alex Williamson
> > wrote:
> > >
> > > On Mon, 4 Sep 2023 15:06:21 +0400
> > > Marc-André Lureau wrote:
> > >
> > > > Hi
> > > >
> > > > On Thu, Aug 17, 2023 at 8:29 PM Kim, Dongwon
> > > > 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
> > > > > >> ---
> > > > > >>
> > > > > >> 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
Re: [PATCH for-8.1] vfio/display: Fix missing update to set backing fields
On Mon, 4 Sep 2023 21:00:53 +0400
Marc-André Lureau wrote:
> Hi
>
> On Mon, Sep 4, 2023 at 6:11 PM Alex Williamson
> wrote:
> >
> > On Mon, 4 Sep 2023 15:06:21 +0400
> > Marc-André Lureau wrote:
> >
> > > Hi
> > >
> > > On Thu, Aug 17, 2023 at 8:29 PM Kim, Dongwon
> > > 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
> > > > >> ---
> > > > >>
> > > > >> 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 */
> >
Re: [PATCH for-8.1] vfio/display: Fix missing update to set backing fields
Hi
On Mon, Sep 4, 2023 at 6:11 PM Alex Williamson
wrote:
>
> On Mon, 4 Sep 2023 15:06:21 +0400
> Marc-André Lureau wrote:
>
> > Hi
> >
> > On Thu, Aug 17, 2023 at 8:29 PM Kim, Dongwon 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
> > > >> ---
> > > >>
> > > >> 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é
Re: [PATCH for-8.1] vfio/display: Fix missing update to set backing fields
On Mon, 4 Sep 2023 15:06:21 +0400
Marc-André Lureau wrote:
> Hi
>
> On Thu, Aug 17, 2023 at 8:29 PM Kim, Dongwon 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
> > >> ---
> > >>
> > >> 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
Hi
On Thu, Aug 17, 2023 at 8:29 PM Kim, Dongwon 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
> >> ---
> >>
> >> 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
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
---
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
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
---
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;
[PATCH for-8.1] vfio/display: Fix missing update to set backing fields
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
---
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
