diff mbox series

[2/2] drm/virtio: Include modifier as part of set_scanout_blob

Message ID 20210331030439.1564032-2-vivek.kasireddy@intel.com (mailing list archive)
State New, archived
Headers show
Series [1/2] drm/virtio: Create Dumb BOs as guest Blobs | expand

Commit Message

Kasireddy, Vivek March 31, 2021, 3:04 a.m. UTC 
With new use-cases coming up that include virtio-gpu:
https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/9592

the FB associated with a Guest blob may have a modifier. Therefore,
this modifier info needs to be included as part of set_scanout_blob.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 drivers/gpu/drm/virtio/virtgpu_display.c | 3 +++
 drivers/gpu/drm/virtio/virtgpu_vq.c      | 3 ++-
 include/uapi/linux/virtio_gpu.h          | 1 +
 3 files changed, 6 insertions(+), 1 deletion(-)

Comments

Gerd Hoffmann March 31, 2021, 7:59 a.m. UTC | #1 
Hi,

> -#define MAX_INLINE_CMD_SIZE   96
> +#define MAX_INLINE_CMD_SIZE   112

Separate patch please.

> --- a/include/uapi/linux/virtio_gpu.h
> +++ b/include/uapi/linux/virtio_gpu.h
> @@ -409,6 +409,7 @@ struct virtio_gpu_set_scanout_blob {
>  	__le32 width;
>  	__le32 height;
>  	__le32 format;
> +	__le64 modifier;
>  	__le32 padding;
>  	__le32 strides[4];
>  	__le32 offsets[4];

Nope.  This breaks the virtio protocol.

We'll need a virtio feature flag to negotiate modifier support between
guest and host.  When supported by both sides it can be used.  The new
field should be appended, not inserted in the middle.  Or we create a
new virtio_gpu_set_scanout_blob2 struct with new command for this.

Also: I guess the device should provide a list of supported modifiers,
maybe as capset?

Note: I think it is already possible to create resources with modifiers
when using virgl commands for that.  Allowing modifiers with virgl=off
too makes sense given your use case, but we should not use diverging
approaches for virgl=on vs. virgl=off.  Specifically I'm not sure
virtio_gpu_set_scanout_blob is the right place, I think with virgl=on
the modifier is linked to the resource not the scanout ...

Cc'ing Gurchetan Singh for comments.

take care,
  Gerd
Zhang, Tina April 2, 2021, 7:55 a.m. UTC | #2 
> -----Original Message-----
> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Gerd
> Hoffmann
> Sent: Wednesday, March 31, 2021 4:00 PM
> To: Kasireddy, Vivek <vivek.kasireddy@intel.com>
> Cc: dri-devel@lists.freedesktop.org; gurchetansingh@chromium.org
> Subject: Re: [PATCH 2/2] drm/virtio: Include modifier as part of
> set_scanout_blob
> 
>   Hi,
> 
> > -#define MAX_INLINE_CMD_SIZE   96
> > +#define MAX_INLINE_CMD_SIZE   112
> 
> Separate patch please.
> 
> > --- a/include/uapi/linux/virtio_gpu.h
> > +++ b/include/uapi/linux/virtio_gpu.h
> > @@ -409,6 +409,7 @@ struct virtio_gpu_set_scanout_blob {
> >  	__le32 width;
> >  	__le32 height;
> >  	__le32 format;
> > +	__le64 modifier;
> >  	__le32 padding;
> >  	__le32 strides[4];
> >  	__le32 offsets[4];
> 
> Nope.  This breaks the virtio protocol.
> 
> We'll need a virtio feature flag to negotiate modifier support between guest and
> host.  When supported by both sides it can be used.  The new field should be
> appended, not inserted in the middle.  Or we create a new
> virtio_gpu_set_scanout_blob2 struct with new command for this.
> 
> Also: I guess the device should provide a list of supported modifiers, maybe as
> capset?

Hi,

I agree that we need a way to get the supported modifiers' info to guest user space mesa, specifically to the iris driver working in kmsro mode.
So, from the guest mesa iris driver's point of view, the working flow may like this:
1) Get the modifier info from a display device through the kms_fd. In our case, the kms_fd comes from the virtio-gpu driver. So the info should come from virtio-gpu device.
2) When guest wants to allocate a scan-out buffer, the iris driver needs to check which format and modifier is suitable to be used.
3) Then, iris uses the kms_fd to allocate the scan-out buffer with the desired format.
     Maybe this time we'd better consider using VIRTGPU_RESOURCE_CREATE to allocate buffer instead of using DRM_IOCTL_MODE_CREATE_DUMB. It seems VIRTGPU_RESUORECE_CREATE can give more fb info.

BR,
Tina

> 
> Note: I think it is already possible to create resources with modifiers when using
> virgl commands for that.  Allowing modifiers with virgl=off too makes sense
> given your use case, but we should not use diverging approaches for virgl=on vs.
> virgl=off.  Specifically I'm not sure virtio_gpu_set_scanout_blob is the right place,
> I think with virgl=on the modifier is linked to the resource not the scanout ...
> 
> Cc'ing Gurchetan Singh for comments.
> 
> take care,
>   Gerd
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Gurchetan Singh April 2, 2021, 4:07 p.m. UTC | #3 
On Fri, Apr 2, 2021 at 12:56 AM Zhang, Tina <tina.zhang@intel.com> wrote:

>
>
> > -----Original Message-----
> > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
> Gerd
> > Hoffmann
> > Sent: Wednesday, March 31, 2021 4:00 PM
> > To: Kasireddy, Vivek <vivek.kasireddy@intel.com>
> > Cc: dri-devel@lists.freedesktop.org; gurchetansingh@chromium.org
> > Subject: Re: [PATCH 2/2] drm/virtio: Include modifier as part of
> > set_scanout_blob
> >
> >   Hi,
> >
> > > -#define MAX_INLINE_CMD_SIZE   96
> > > +#define MAX_INLINE_CMD_SIZE   112
> >
> > Separate patch please.
> >
> > > --- a/include/uapi/linux/virtio_gpu.h
> > > +++ b/include/uapi/linux/virtio_gpu.h
> > > @@ -409,6 +409,7 @@ struct virtio_gpu_set_scanout_blob {
> > >     __le32 width;
> > >     __le32 height;
> > >     __le32 format;
> > > +   __le64 modifier;
> > >     __le32 padding;
> > >     __le32 strides[4];
> > >     __le32 offsets[4];
> >
> > Nope.  This breaks the virtio protocol.
> >
> > We'll need a virtio feature flag to negotiate modifier support between
> guest and
> > host.  When supported by both sides it can be used.  The new field
> should be
> > appended, not inserted in the middle.  Or we create a new
> > virtio_gpu_set_scanout_blob2 struct with new command for this.
> >
> > Also: I guess the device should provide a list of supported modifiers,
> maybe as
> > capset?
>
> Hi,
>
> I agree that we need a way to get the supported modifiers' info to guest
> user space mesa, specifically to the iris driver working in kmsro mode.
> So, from the guest mesa iris driver's point of view, the working flow may
> like this:
> 1) Get the modifier info from a display device through the kms_fd. In our
> case, the kms_fd comes from the virtio-gpu driver. So the info should come
> from virtio-gpu device.
> 2) When guest wants to allocate a scan-out buffer, the iris driver needs
> to check which format and modifier is suitable to be used.
> 3) Then, iris uses the kms_fd to allocate the scan-out buffer with the
> desired format.
>      Maybe this time we'd better consider using VIRTGPU_RESOURCE_CREATE to
> allocate buffer instead of using DRM_IOCTL_MODE_CREATE_DUMB. It seems
> VIRTGPU_RESUORECE_CREATE can give more fb info.
>

Thank you Tina and Vivek for looking into this!  Added some commentary on
your Mesa side MR.


>
> BR,
> Tina
>
> >
> > Note: I think it is already possible to create resources with modifiers
> when using
> > virgl commands for that.  Allowing modifiers with virgl=off too makes
> sense
> > given your use case, but we should not use diverging approaches for
> virgl=on vs.
> > virgl=off.  Specifically I'm not sure virtio_gpu_set_scanout_blob is the
> right place,
> > I think with virgl=on the modifier is linked to the resource not the
> scanout ...
> >
> > Cc'ing Gurchetan Singh for comments.
> >
> > take care,
> >   Gerd
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c
index a6caebd4a0dd..e2e7e6c5cb91 100644
--- a/drivers/gpu/drm/virtio/virtgpu_display.c
+++ b/drivers/gpu/drm/virtio/virtgpu_display.c
@@ -344,6 +344,9 @@  int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
 	vgdev->ddev->mode_config.max_width = XRES_MAX;
 	vgdev->ddev->mode_config.max_height = YRES_MAX;
 
+	if (vgdev->has_resource_blob)
+		vgdev->ddev->mode_config.allow_fb_modifiers = true;
+
 	for (i = 0 ; i < vgdev->num_scanouts; ++i)
 		vgdev_output_init(vgdev, i);
 
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index cf84d382dd41..462f1beb9c11 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -34,7 +34,7 @@ 
 #include "virtgpu_drv.h"
 #include "virtgpu_trace.h"
 
-#define MAX_INLINE_CMD_SIZE   96
+#define MAX_INLINE_CMD_SIZE   112
 #define MAX_INLINE_RESP_SIZE  24
 #define VBUFFER_SIZE          (sizeof(struct virtio_gpu_vbuffer) \
 			       + MAX_INLINE_CMD_SIZE		 \
@@ -1294,6 +1294,7 @@  void virtio_gpu_cmd_set_scanout_blob(struct virtio_gpu_device *vgdev,
 	cmd_p->format = cpu_to_le32(format);
 	cmd_p->width  = cpu_to_le32(fb->width);
 	cmd_p->height = cpu_to_le32(fb->height);
+	cmd_p->modifier = cpu_to_le64(fb->modifier);
 
 	for (i = 0; i < 4; i++) {
 		cmd_p->strides[i] = cpu_to_le32(fb->pitches[i]);
diff --git a/include/uapi/linux/virtio_gpu.h b/include/uapi/linux/virtio_gpu.h
index 97523a95781d..c6424d769e62 100644
--- a/include/uapi/linux/virtio_gpu.h
+++ b/include/uapi/linux/virtio_gpu.h
@@ -409,6 +409,7 @@  struct virtio_gpu_set_scanout_blob {
 	__le32 width;
 	__le32 height;
 	__le32 format;
+	__le64 modifier;
 	__le32 padding;
 	__le32 strides[4];
 	__le32 offsets[4];