On Thu, 1 Jun 2017 03:01:28 +0000
"Chen, Xiaoguang" <[email protected]> wrote:
> Hi Kirti,
>
> >-----Original Message-----
> >From: Kirti Wankhede [mailto:[email protected]]
> >Sent: Thursday, June 01, 2017 1:23 AM
> >To: Chen, Xiaoguang <[email protected]>; Gerd Hoffmann
> ><[email protected]>; [email protected]; [email protected];
> >[email protected]; [email protected];
> >[email protected]; Lv, Zhiyuan <[email protected]>; intel-gvt-
> >[email protected]; Wang, Zhi A <[email protected]>; Tian, Kevin
> ><[email protected]>
> >Subject: Re: [PATCH v6 4/6] vfio: Define vfio based vgpu's dma-buf operations
> >
> >
> >
> >On 5/31/2017 11:48 AM, Chen, Xiaoguang wrote:
> >> Hi,
> >>
> >>> -----Original Message-----
> >>> From: Gerd Hoffmann [mailto:[email protected]]
> >>> Sent: Monday, May 29, 2017 3:20 PM
> >>> To: Chen, Xiaoguang <[email protected]>;
> >>> [email protected]; [email protected]; intel-
> >>> [email protected]; [email protected];
> >>> [email protected]; Lv, Zhiyuan <[email protected]>;
> >>> intel-gvt- [email protected]; Wang, Zhi A
> >>> <[email protected]>; Tian, Kevin <[email protected]>
> >>> Subject: Re: [PATCH v6 4/6] vfio: Define vfio based vgpu's dma-buf
> >>> operations
> >>>
> >>>> +struct vfio_vgpu_dmabuf_info {
> >>>> + __u32 argsz;
> >>>> + __u32 flags;
> >>>> + struct vfio_vgpu_plane_info plane_info;
> >>>> + __s32 fd;
> >>>> + __u32 pad;
> >>>> +};
> >>>
> >>> Hmm, now you have argsz and flags twice in vfio_vgpu_dmabuf_info ...
> >>>
> >>> I think we should have something like this:
> >>>
> >>> struct vfio_vgpu_plane_info {
> >>> __u64 start;
> >>> __u64 drm_format_mod;
> >>> __u32 drm_format;
> >>> __u32 width;
> >>> __u32 height;
> >>> __u32 stride;
> >>> __u32 size;
> >>> __u32 x_pos;
> >>> __u32 y_pos;
> >>> __u32 padding;
> >>> };
> >>>
> >>> struct vfio_vgpu_query_plane {
> >>> __u32 argsz;
> >>> __u32 flags;
> >>> struct vfio_vgpu_plane_info plane_info;
> >>> __u32 plane_id;
> >>> __u32 padding;
> >>> };
> >>>
> >>> struct vfio_vgpu_create_dmabuf {
> >>> __u32 argsz;
> >>> __u32 flags;
> >>> struct vfio_vgpu_plane_info plane_info;
> >>> __u32 plane_id;
> >>> __s32 fd;
> >>> };
> >> Good suggestion will apply in the next version.
> >> Thanks for review :)
> >>
> >
> >Can you define what are the expected values of 'flags' would be?
> Flags is not used in this case. It is defined to follow the rules of vfio ioctls.
An important note about flags, the vendor driver must validate it. If
they don't and the user passes an arbitrary value there, then we have a
backwards compatibility issue with ever attempting to use the flags
field. The user passing in a flag unknown to the vendor driver should
return an -EINVAL response. In this case, we haven't defined any
flags, so the vendor driver needs to force the user to pass zero.
Thanks,
Alex
On 6/1/2017 10:08 PM, Alex Williamson wrote:
> On Thu, 1 Jun 2017 03:01:28 +0000
> "Chen, Xiaoguang" <[email protected]> wrote:
>
>> Hi Kirti,
>>
>>> -----Original Message-----
>>> From: Kirti Wankhede [mailto:[email protected]]
>>> Sent: Thursday, June 01, 2017 1:23 AM
>>> To: Chen, Xiaoguang <[email protected]>; Gerd Hoffmann
>>> <[email protected]>; [email protected]; [email protected];
>>> [email protected]; [email protected];
>>> [email protected]; Lv, Zhiyuan <[email protected]>; intel-gvt-
>>> [email protected]; Wang, Zhi A <[email protected]>; Tian, Kevin
>>> <[email protected]>
>>> Subject: Re: [PATCH v6 4/6] vfio: Define vfio based vgpu's dma-buf operations
>>>
>>>
>>>
>>> On 5/31/2017 11:48 AM, Chen, Xiaoguang wrote:
>>>> Hi,
>>>>
>>>>> -----Original Message-----
>>>>> From: Gerd Hoffmann [mailto:[email protected]]
>>>>> Sent: Monday, May 29, 2017 3:20 PM
>>>>> To: Chen, Xiaoguang <[email protected]>;
>>>>> [email protected]; [email protected]; intel-
>>>>> [email protected]; [email protected];
>>>>> [email protected]; Lv, Zhiyuan <[email protected]>;
>>>>> intel-gvt- [email protected]; Wang, Zhi A
>>>>> <[email protected]>; Tian, Kevin <[email protected]>
>>>>> Subject: Re: [PATCH v6 4/6] vfio: Define vfio based vgpu's dma-buf
>>>>> operations
>>>>>
>>>>>> +struct vfio_vgpu_dmabuf_info {
>>>>>> + __u32 argsz;
>>>>>> + __u32 flags;
>>>>>> + struct vfio_vgpu_plane_info plane_info;
>>>>>> + __s32 fd;
>>>>>> + __u32 pad;
>>>>>> +};
>>>>>
>>>>> Hmm, now you have argsz and flags twice in vfio_vgpu_dmabuf_info ...
>>>>>
>>>>> I think we should have something like this:
>>>>>
>>>>> struct vfio_vgpu_plane_info {
>>>>> __u64 start;
>>>>> __u64 drm_format_mod;
>>>>> __u32 drm_format;
>>>>> __u32 width;
>>>>> __u32 height;
>>>>> __u32 stride;
>>>>> __u32 size;
>>>>> __u32 x_pos;
>>>>> __u32 y_pos;
>>>>> __u32 padding;
>>>>> };
>>>>>
>>>>> struct vfio_vgpu_query_plane {
>>>>> __u32 argsz;
>>>>> __u32 flags;
>>>>> struct vfio_vgpu_plane_info plane_info;
>>>>> __u32 plane_id;
>>>>> __u32 padding;
>>>>> };
>>>>>
>>>>> struct vfio_vgpu_create_dmabuf {
>>>>> __u32 argsz;
>>>>> __u32 flags;
>>>>> struct vfio_vgpu_plane_info plane_info;
>>>>> __u32 plane_id;
>>>>> __s32 fd;
>>>>> };
>>>> Good suggestion will apply in the next version.
>>>> Thanks for review :)
>>>>
>>>
>>> Can you define what are the expected values of 'flags' would be?
>> Flags is not used in this case. It is defined to follow the rules of vfio ioctls.
>
> An important note about flags, the vendor driver must validate it. If
> they don't and the user passes an arbitrary value there, then we have a
> backwards compatibility issue with ever attempting to use the flags
> field. The user passing in a flag unknown to the vendor driver should
> return an -EINVAL response. In this case, we haven't defined any
> flags, so the vendor driver needs to force the user to pass zero.
There are two ways QEMU can get surface for console:
1. adding a region using region capability
2. dmabuf
In both the above case surface parameters need to be queried from vendor
driver are same. The structure would be :
struct vfio_vgpu_surface_info {
__u64 start;
__u32 width;
__u32 height;
__u32 stride;
__u32 size;
__u32 x_pos;
__u32 y_pos;
__u32 padding;
/* Only used when VFIO_VGPU_SURFACE_DMABUF_* flags set */
__u64 drm_format_mod;
__u32 drm_format;
};
We can use one ioctl to query surface information from vendor driver,
structure would look like:
struct vfio_vgpu_get_surface_info{
__u32 argsz;
__u32 flags;
#define VFIO_VGPU_SURFACE_DMABUF_CREATE (1 << 0) /* Create dmabuf */
#define VFIO_VGPU_SURFACE_DMABUF_QUERY (1 << 1) /* Query surface info
for dmabuf */
#define VFIO_VGPU_SURFACE_REGION_QUERY (1 << 2) /* Query surface info
for REGION type */
struct vfio_vgpu_surface_info surface;
__u32 plane_id;
__s32 fd;
};
#define VFIO_DEVICE_SURFACE_INFO _IO(VFIO_TYPE, VFIO_BASE + 15)
Vendor driver should return -EINVAL, if that type of query is not
supported.
I would like to design this interface to support both type, region cap
and dmabuf.
Thanks,
Kirti
> struct vfio_vgpu_surface_info {
> __u64 start;
> __u32 width;
> __u32 height;
> __u32 stride;
> __u32 size;
> __u32 x_pos;
> __u32 y_pos;
> __u32 padding;
> /* Only used when VFIO_VGPU_SURFACE_DMABUF_* flags set */
> __u64 drm_format_mod;
> __u32 drm_format;
Why for dmabufs only? Shouldn't the region specify the format too?
Even in case you are using a fixed one (say DRM_FORMAT_XRGB8888) you
can explicitly say so in drm_format (and set drm_format_mod to zero).
cheers,
Gerd
On 6/2/2017 2:08 PM, Gerd Hoffmann wrote:
>
>> struct vfio_vgpu_surface_info {
>> __u64 start;
>> __u32 width;
>> __u32 height;
>> __u32 stride;
>> __u32 size;
>> __u32 x_pos;
>> __u32 y_pos;
>> __u32 padding;
>> /* Only used when VFIO_VGPU_SURFACE_DMABUF_* flags set */
>> __u64 drm_format_mod;
>> __u32 drm_format;
>
> Why for dmabufs only? Shouldn't the region specify the format too?
> Even in case you are using a fixed one (say DRM_FORMAT_XRGB8888) you
> can explicitly say so in drm_format (and set drm_format_mod to zero).
>
Definitions for PIXMAN formats and DRM formats are different. I think
we need a flag to specify the format of surface that vendor driver is
going to provide, PIXMAN or DRM.
If surface is provided through region in PIXMAN format, existing
functions in QEMU can be used to get format from bpp value,
qemu_default_pixman_format(). Similarly, display surface can be updated
by QEMU using qemu_create_displaysurface_from() from mmaped region.
Thanks,
Kirti
> cheers,
> Gerd
>
On Mon, 2017-06-05 at 13:56 +0530, Kirti Wankhede wrote:
>
> On 6/2/2017 2:08 PM, Gerd Hoffmann wrote:
> >
> > > struct vfio_vgpu_surface_info {
> > > __u64 start;
> > > __u32 width;
> > > __u32 height;
> > > __u32 stride;
> > > __u32 size;
> > > __u32 x_pos;
> > > __u32 y_pos;
> > > __u32 padding;
> > > /* Only used when VFIO_VGPU_SURFACE_DMABUF_* flags set */
> > > __u64 drm_format_mod;
> > > __u32 drm_format;
> >
> > Why for dmabufs only? Shouldn't the region specify the format
> > too?
> > Even in case you are using a fixed one (say
> > DRM_FORMAT_XRGB8888) you
> > can explicitly say so in drm_format (and set drm_format_mod to
> > zero).
> >
>
> Definitions for PIXMAN formats and DRM formats are different. I think
> we need a flag to specify the format of surface that vendor driver is
> going to provide, PIXMAN or DRM.
No need to put that into the ioctl interface. First, the kernel should
not worry about what userspace uses to process the data. Second (most)
drm formats can trivially be mapped into pixman formats.
For example: PIXMAN_x8r8g8b8 (little endian) == DRM_FORMAT_XRGB8888
> If surface is provided through region in PIXMAN format, existing
> functions in QEMU can be used to get format from bpp value,
> qemu_default_pixman_format(). Similarly, display surface can be
> updated
> by QEMU using qemu_create_displaysurface_from() from mmaped region.
A thin wrapper which accepts a struct vfio_vgpu_surface_info and
translates that into a qemu_create_displaysurface_from() call should be
pretty small.
cheers,
Gerd