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

2023-10-18 Thread Akihiko Odaki 

On 2023/10/19 8:25, Gurchetan Singh wrote:



On Tue, Oct 10, 2023 at 9:41 PM Huang Rui > 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 mailto:ray.hu...@amd.com>>
 > >> Reviewed-by: Akihiko Odaki mailto:akihiko.od...@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.hu...@amd.com/ 

 > >> v2:
https://lore.kernel.org/lkml/20231010032553.1138036-1-ray.hu...@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-gurchetansi...@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.


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.

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

2023-10-18 Thread Dmitry Osipenko 
On 10/19/23 02:25, Gurchetan Singh wrote:
> On Tue, Oct 10, 2023 at 9:41 PM Huang Rui  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 
> Reviewed-by: Akihiko Odaki 
> ---
>
> 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.hu...@amd.com/
> v2:
>> https://lore.kernel.org/lkml/20231010032553.1138036-1-ray.hu...@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-gurchetansi...@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

2023-10-18 Thread Gurchetan Singh 
On Tue, Oct 10, 2023 at 9:41 PM Huang Rui  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 
> > >> Reviewed-by: Akihiko Odaki 
> > >> ---
> > >>
> > >> 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.hu...@amd.com/
> > >> v2:
> https://lore.kernel.org/lkml/20231010032553.1138036-1-ray.hu...@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-gurchetansi...@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.


> >
>
> Gurchetan, may we have your insight?
>
> Thanks,
> Ray
>
> > --
> > Best regards,
> > Dmitry
> >
>

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

2023-10-10 Thread 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 
> >> Reviewed-by: Akihiko Odaki 
> >> ---
> >>
> >> 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.hu...@amd.com/
> >> v2: 
> >> https://lore.kernel.org/lkml/20231010032553.1138036-1-ray.hu...@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-gurchetansi...@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

2023-10-10 Thread 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 
>> Reviewed-by: Akihiko Odaki 
>> ---
>>
>> 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.hu...@amd.com/
>> v2: https://lore.kernel.org/lkml/20231010032553.1138036-1-ray.hu...@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-gurchetansi...@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

2023-10-10 Thread 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 
> Reviewed-by: Akihiko Odaki 
> ---
> 
> 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.hu...@amd.com/
> v2: https://lore.kernel.org/lkml/20231010032553.1138036-1-ray.hu...@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-gurchetansi...@chromium.org/

-- 
Best regards,
Dmitry

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

2023-10-10 Thread 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 
Reviewed-by: Akihiko Odaki 
---

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.hu...@amd.com/
v2: https://lore.kernel.org/lkml/20231010032553.1138036-1-ray.hu...@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