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

[Gurchetan Singh <gurchetansingh@chromium.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3050 bytes --]

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
>

[-- Attachment #1.2: Type: text/html, Size: 4419 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel