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

["Zhang, Tina" <tina.zhang@intel.com>]

> -----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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel