[PATCH v3] drm/virtio: add new virtio gpu capset definitions

[PATCH v3] drm/virtio: add new virtio gpu capset definitions

[Huang Rui]

These definitions are used fro qemu, and qemu imports this marco in the
headers to enable gfxstream, venus, cross domain, and drm (native
context) for virtio gpu. So it should add them even kernel doesn't use
this.

Signed-off-by: Huang Rui <ray.huang@amd.com>
Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---

Changes V1 -> V2:
- Add all capsets including gfxstream and venus in kernel header (Dmitry Osipenko)

Changes V2 -> V3:
- Add missed capsets including cross domain and drm (native context)
  (Dmitry Osipenko)

v1: https://lore.kernel.org/lkml/20230915105918.3763061-1-ray.huang@amd.com/
v2: https://lore.kernel.org/lkml/20231010032553.1138036-1-ray.huang@amd.com/

 include/uapi/linux/virtio_gpu.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/uapi/linux/virtio_gpu.h b/include/uapi/linux/virtio_gpu.h
index f556fde07b76..240911c8da31 100644
--- a/include/uapi/linux/virtio_gpu.h
+++ b/include/uapi/linux/virtio_gpu.h
@@ -309,6 +309,10 @@ struct virtio_gpu_cmd_submit {
 
 #define VIRTIO_GPU_CAPSET_VIRGL 1
 #define VIRTIO_GPU_CAPSET_VIRGL2 2
+#define VIRTIO_GPU_CAPSET_GFXSTREAM 3
+#define VIRTIO_GPU_CAPSET_VENUS 4
+#define VIRTIO_GPU_CAPSET_CROSS_DOMAIN 5
+#define VIRTIO_GPU_CAPSET_DRM 6
 
 /* VIRTIO_GPU_CMD_GET_CAPSET_INFO */
 struct virtio_gpu_get_capset_info {
-- 
2.25.1

Re: [PATCH v3] drm/virtio: add new virtio gpu capset definitions

[Dmitry Osipenko]

On 10/10/23 16:57, Huang Rui wrote:
> These definitions are used fro qemu, and qemu imports this marco in the
> headers to enable gfxstream, venus, cross domain, and drm (native
> context) for virtio gpu. So it should add them even kernel doesn't use
> this.
> 
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> 
> Changes V1 -> V2:
> - Add all capsets including gfxstream and venus in kernel header (Dmitry Osipenko)
> 
> Changes V2 -> V3:
> - Add missed capsets including cross domain and drm (native context)
>   (Dmitry Osipenko)
> 
> v1: https://lore.kernel.org/lkml/20230915105918.3763061-1-ray.huang@amd.com/
> v2: https://lore.kernel.org/lkml/20231010032553.1138036-1-ray.huang@amd.com/
> 
>  include/uapi/linux/virtio_gpu.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/uapi/linux/virtio_gpu.h b/include/uapi/linux/virtio_gpu.h
> index f556fde07b76..240911c8da31 100644
> --- a/include/uapi/linux/virtio_gpu.h
> +++ b/include/uapi/linux/virtio_gpu.h
> @@ -309,6 +309,10 @@ struct virtio_gpu_cmd_submit {
>  
>  #define VIRTIO_GPU_CAPSET_VIRGL 1
>  #define VIRTIO_GPU_CAPSET_VIRGL2 2
> +#define VIRTIO_GPU_CAPSET_GFXSTREAM 3

The GFXSTREAM capset isn't correct, it should be GFXSTREAM_VULKAN in
accordance to [1] and [2]. There are more capsets for GFXSTREAM.

[1]
https://github.com/google/crosvm/blob/main/rutabaga_gfx/src/rutabaga_utils.rs#L172

[2]
https://patchwork.kernel.org/project/qemu-devel/patch/20231006010835.444-7-gurchetansingh@chromium.org/

-- 
Best regards,
Dmitry

Re: [PATCH v3] drm/virtio: add new virtio gpu capset definitions

[Dmitry Osipenko]

On 10/10/23 18:40, Dmitry Osipenko wrote:
> On 10/10/23 16:57, Huang Rui wrote:
>> These definitions are used fro qemu, and qemu imports this marco in the
>> headers to enable gfxstream, venus, cross domain, and drm (native
>> context) for virtio gpu. So it should add them even kernel doesn't use
>> this.
>>
>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>
>> Changes V1 -> V2:
>> - Add all capsets including gfxstream and venus in kernel header (Dmitry Osipenko)
>>
>> Changes V2 -> V3:
>> - Add missed capsets including cross domain and drm (native context)
>>   (Dmitry Osipenko)
>>
>> v1: https://lore.kernel.org/lkml/20230915105918.3763061-1-ray.huang@amd.com/
>> v2: https://lore.kernel.org/lkml/20231010032553.1138036-1-ray.huang@amd.com/
>>
>>  include/uapi/linux/virtio_gpu.h | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/include/uapi/linux/virtio_gpu.h b/include/uapi/linux/virtio_gpu.h
>> index f556fde07b76..240911c8da31 100644
>> --- a/include/uapi/linux/virtio_gpu.h
>> +++ b/include/uapi/linux/virtio_gpu.h
>> @@ -309,6 +309,10 @@ struct virtio_gpu_cmd_submit {
>>  
>>  #define VIRTIO_GPU_CAPSET_VIRGL 1
>>  #define VIRTIO_GPU_CAPSET_VIRGL2 2
>> +#define VIRTIO_GPU_CAPSET_GFXSTREAM 3
> 
> The GFXSTREAM capset isn't correct, it should be GFXSTREAM_VULKAN in
> accordance to [1] and [2]. There are more capsets for GFXSTREAM.
> 
> [1]
> https://github.com/google/crosvm/blob/main/rutabaga_gfx/src/rutabaga_utils.rs#L172
> 
> [2]
> https://patchwork.kernel.org/project/qemu-devel/patch/20231006010835.444-7-gurchetansingh@chromium.org/

Though, maybe those are "rutabaga" capsets that not related to
virtio-gpu because crosvm has another defs for virtio-gpu capsets [3].
The DRM capset is oddly missing in [3] and code uses "rutabaga" capset
for DRM and virtio-gpu.

[3]
https://github.com/google/crosvm/blob/main/devices/src/virtio/gpu/protocol.rs#L416

Gurchetan, could you please clarify which capsets definitions are
related to virtio-gpu and gfxstream. The
GFXSTREAM_VULKAN/GLES/MAGMA/COMPOSER or just the single GFXSTREAM?

-- 
Best regards,
Dmitry

Re: [PATCH v3] drm/virtio: add new virtio gpu capset definitions

[Huang Rui]

On Tue, Oct 10, 2023 at 11:52:14PM +0800, Dmitry Osipenko wrote:
> On 10/10/23 18:40, Dmitry Osipenko wrote:
> > On 10/10/23 16:57, Huang Rui wrote:
> >> These definitions are used fro qemu, and qemu imports this marco in the
> >> headers to enable gfxstream, venus, cross domain, and drm (native
> >> context) for virtio gpu. So it should add them even kernel doesn't use
> >> this.
> >>
> >> Signed-off-by: Huang Rui <ray.huang@amd.com>
> >> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >> ---
> >>
> >> Changes V1 -> V2:
> >> - Add all capsets including gfxstream and venus in kernel header (Dmitry Osipenko)
> >>
> >> Changes V2 -> V3:
> >> - Add missed capsets including cross domain and drm (native context)
> >>   (Dmitry Osipenko)
> >>
> >> v1: https://lore.kernel.org/lkml/20230915105918.3763061-1-ray.huang@amd.com/
> >> v2: https://lore.kernel.org/lkml/20231010032553.1138036-1-ray.huang@amd.com/
> >>
> >>  include/uapi/linux/virtio_gpu.h | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/include/uapi/linux/virtio_gpu.h b/include/uapi/linux/virtio_gpu.h
> >> index f556fde07b76..240911c8da31 100644
> >> --- a/include/uapi/linux/virtio_gpu.h
> >> +++ b/include/uapi/linux/virtio_gpu.h
> >> @@ -309,6 +309,10 @@ struct virtio_gpu_cmd_submit {
> >>  
> >>  #define VIRTIO_GPU_CAPSET_VIRGL 1
> >>  #define VIRTIO_GPU_CAPSET_VIRGL2 2
> >> +#define VIRTIO_GPU_CAPSET_GFXSTREAM 3
> > 
> > The GFXSTREAM capset isn't correct, it should be GFXSTREAM_VULKAN in
> > accordance to [1] and [2]. There are more capsets for GFXSTREAM.
> > 
> > [1]
> > https://github.com/google/crosvm/blob/main/rutabaga_gfx/src/rutabaga_utils.rs#L172
> > 
> > [2]
> > https://patchwork.kernel.org/project/qemu-devel/patch/20231006010835.444-7-gurchetansingh@chromium.org/
> 
> Though, maybe those are "rutabaga" capsets that not related to
> virtio-gpu because crosvm has another defs for virtio-gpu capsets [3].
> The DRM capset is oddly missing in [3] and code uses "rutabaga" capset
> for DRM and virtio-gpu.
> 
> [3]
> https://github.com/google/crosvm/blob/main/devices/src/virtio/gpu/protocol.rs#L416

Yes, [3] is the file that I referred to add these capsets definitions. And
it's defined as gfxstream not gfxstream_vulkan.

> 
> Gurchetan, could you please clarify which capsets definitions are
> related to virtio-gpu and gfxstream. The
> GFXSTREAM_VULKAN/GLES/MAGMA/COMPOSER or just the single GFXSTREAM?
> 

Gurchetan, may we have your insight?

Thanks,
Ray

> -- 
> Best regards,
> Dmitry
> 

Re: [PATCH v3] drm/virtio: add new virtio gpu capset definitions

[Dmitry Osipenko]

On 10/19/23 02:25, Gurchetan Singh wrote:
> On Tue, Oct 10, 2023 at 9:41 PM Huang Rui <ray.huang@amd.com> wrote:
> 
>> On Tue, Oct 10, 2023 at 11:52:14PM +0800, Dmitry Osipenko wrote:
>>> On 10/10/23 18:40, Dmitry Osipenko wrote:
>>>> On 10/10/23 16:57, Huang Rui wrote:
>>>>> These definitions are used fro qemu, and qemu imports this marco in
>> the
>>>>> headers to enable gfxstream, venus, cross domain, and drm (native
>>>>> context) for virtio gpu. So it should add them even kernel doesn't use
>>>>> this.
>>>>>
>>>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>>>> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>>> ---
>>>>>
>>>>> Changes V1 -> V2:
>>>>> - Add all capsets including gfxstream and venus in kernel header
>> (Dmitry Osipenko)
>>>>>
>>>>> Changes V2 -> V3:
>>>>> - Add missed capsets including cross domain and drm (native context)
>>>>>   (Dmitry Osipenko)
>>>>>
>>>>> v1:
>> https://lore.kernel.org/lkml/20230915105918.3763061-1-ray.huang@amd.com/
>>>>> v2:
>> https://lore.kernel.org/lkml/20231010032553.1138036-1-ray.huang@amd.com/
>>>>>
>>>>>  include/uapi/linux/virtio_gpu.h | 4 ++++
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/include/uapi/linux/virtio_gpu.h
>> b/include/uapi/linux/virtio_gpu.h
>>>>> index f556fde07b76..240911c8da31 100644
>>>>> --- a/include/uapi/linux/virtio_gpu.h
>>>>> +++ b/include/uapi/linux/virtio_gpu.h
>>>>> @@ -309,6 +309,10 @@ struct virtio_gpu_cmd_submit {
>>>>>
>>>>>  #define VIRTIO_GPU_CAPSET_VIRGL 1
>>>>>  #define VIRTIO_GPU_CAPSET_VIRGL2 2
>>>>> +#define VIRTIO_GPU_CAPSET_GFXSTREAM 3
>>>>
>>>> The GFXSTREAM capset isn't correct, it should be GFXSTREAM_VULKAN in
>>>> accordance to [1] and [2]. There are more capsets for GFXSTREAM.
>>>>
>>>> [1]
>>>>
>> https://github.com/google/crosvm/blob/main/rutabaga_gfx/src/rutabaga_utils.rs#L172
>>>>
>>>> [2]
>>>>
>> https://patchwork.kernel.org/project/qemu-devel/patch/20231006010835.444-7-gurchetansingh@chromium.org/
>>>
>>> Though, maybe those are "rutabaga" capsets that not related to
>>> virtio-gpu because crosvm has another defs for virtio-gpu capsets [3].
>>> The DRM capset is oddly missing in [3] and code uses "rutabaga" capset
>>> for DRM and virtio-gpu.
>>>
>>> [3]
>>>
>> https://github.com/google/crosvm/blob/main/devices/src/virtio/gpu/protocol.rs#L416
>>
>> Yes, [3] is the file that I referred to add these capsets definitions. And
>> it's defined as gfxstream not gfxstream_vulkan.
>>
>>>
>>> Gurchetan, could you please clarify which capsets definitions are
>>> related to virtio-gpu and gfxstream. The
>>> GFXSTREAM_VULKAN/GLES/MAGMA/COMPOSER or just the single GFXSTREAM?
> 
> 
> It should be GFXSTREAM_VULKAN.  The rest are more experimental and easy to
> modify in terms of the enum value, should the need arise.
> 
> I imagine the virtio-spec update to reflect the GFXSTREAM to
> GFXSTREAM_VULKAN change will happen eventually.

Thanks for the clarification. Good point about the spec updating, we
should document DRM context too,

-- 
Best regards,
Dmitry

Re: [PATCH v3] drm/virtio: add new virtio gpu capset definitions

[Akihiko Odaki]

On 2023/10/19 8:25, Gurchetan Singh wrote:
> 
> 
> On Tue, Oct 10, 2023 at 9:41 PM Huang Rui <ray.huang@amd.com 
> <mailto:ray.huang@amd.com>> wrote:
> 
>     On Tue, Oct 10, 2023 at 11:52:14PM +0800, Dmitry Osipenko wrote:
>      > On 10/10/23 18:40, Dmitry Osipenko wrote:
>      > > On 10/10/23 16:57, Huang Rui wrote:
>      > >> These definitions are used fro qemu, and qemu imports this
>     marco in the
>      > >> headers to enable gfxstream, venus, cross domain, and drm (native
>      > >> context) for virtio gpu. So it should add them even kernel
>     doesn't use
>      > >> this.
>      > >>
>      > >> Signed-off-by: Huang Rui <ray.huang@amd.com
>     <mailto:ray.huang@amd.com>>
>      > >> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com
>     <mailto:akihiko.odaki@daynix.com>>
>      > >> ---
>      > >>
>      > >> Changes V1 -> V2:
>      > >> - Add all capsets including gfxstream and venus in kernel
>     header (Dmitry Osipenko)
>      > >>
>      > >> Changes V2 -> V3:
>      > >> - Add missed capsets including cross domain and drm (native
>     context)
>      > >>   (Dmitry Osipenko)
>      > >>
>      > >> v1:
>     https://lore.kernel.org/lkml/20230915105918.3763061-1-ray.huang@amd.com/ <https://lore.kernel.org/lkml/20230915105918.3763061-1-ray.huang@amd.com/>
>      > >> v2:
>     https://lore.kernel.org/lkml/20231010032553.1138036-1-ray.huang@amd.com/ <https://lore.kernel.org/lkml/20231010032553.1138036-1-ray.huang@amd.com/>
>      > >>
>      > >>  include/uapi/linux/virtio_gpu.h | 4 ++++
>      > >>  1 file changed, 4 insertions(+)
>      > >>
>      > >> diff --git a/include/uapi/linux/virtio_gpu.h
>     b/include/uapi/linux/virtio_gpu.h
>      > >> index f556fde07b76..240911c8da31 100644
>      > >> --- a/include/uapi/linux/virtio_gpu.h
>      > >> +++ b/include/uapi/linux/virtio_gpu.h
>      > >> @@ -309,6 +309,10 @@ struct virtio_gpu_cmd_submit {
>      > >>
>      > >>  #define VIRTIO_GPU_CAPSET_VIRGL 1
>      > >>  #define VIRTIO_GPU_CAPSET_VIRGL2 2
>      > >> +#define VIRTIO_GPU_CAPSET_GFXSTREAM 3
>      > >
>      > > The GFXSTREAM capset isn't correct, it should be
>     GFXSTREAM_VULKAN in
>      > > accordance to [1] and [2]. There are more capsets for GFXSTREAM.
>      > >
>      > > [1]
>      > >
>     https://github.com/google/crosvm/blob/main/rutabaga_gfx/src/rutabaga_utils.rs#L172 <https://github.com/google/crosvm/blob/main/rutabaga_gfx/src/rutabaga_utils.rs#L172>
>      > >
>      > > [2]
>      > >
>     https://patchwork.kernel.org/project/qemu-devel/patch/20231006010835.444-7-gurchetansingh@chromium.org/ <https://patchwork.kernel.org/project/qemu-devel/patch/20231006010835.444-7-gurchetansingh@chromium.org/>
>      >
>      > Though, maybe those are "rutabaga" capsets that not related to
>      > virtio-gpu because crosvm has another defs for virtio-gpu capsets
>     [3].
>      > The DRM capset is oddly missing in [3] and code uses "rutabaga"
>     capset
>      > for DRM and virtio-gpu.
>      >
>      > [3]
>      >
>     https://github.com/google/crosvm/blob/main/devices/src/virtio/gpu/protocol.rs#L416 <https://github.com/google/crosvm/blob/main/devices/src/virtio/gpu/protocol.rs#L416>
> 
>     Yes, [3] is the file that I referred to add these capsets
>     definitions. And
>     it's defined as gfxstream not gfxstream_vulkan.
> 
>      >
>      > Gurchetan, could you please clarify which capsets definitions are
>      > related to virtio-gpu and gfxstream. The
>      > GFXSTREAM_VULKAN/GLES/MAGMA/COMPOSER or just the single GFXSTREAM?
> 
> 
> It should be GFXSTREAM_VULKAN.  The rest are more experimental and easy 
> to modify in terms of the enum value, should the need arise.
> 
> I imagine the virtio-spec update to reflect the GFXSTREAM to 
> GFXSTREAM_VULKAN change will happen eventually.

I think this is a matter what the committee should determine, but in 
general I don't think it is OK to change the existing identifier.

I also think even experimental values should be added to virtio spec at 
an early stage unless such an "experiment" is done only on one laptop. 
We can obsolete a capset anytime so it's more important to avoid 
conflicts of capsets.