[PATCH v2] drm/virtio: conditionally allocate virtio_gpu_fence

[PATCH v2] drm/virtio: conditionally allocate virtio_gpu_fence

[Gurchetan Singh]

We don't want to create a fence for every command submission.  It's
only necessary when userspace provides a waitable token for submission.
This could be:

1) bo_handles, to be used with VIRTGPU_WAIT
2) out_fence_fd, to be used with dma_fence apis
3) a ring_idx provided with VIRTGPU_CONTEXT_PARAM_POLL_RINGS_MASK
   + DRM event API
4) syncobjs in the future

The use case for just submitting a command to the host, and expected
no response.  For example, gfxstream has GFXSTREAM_CONTEXT_PING that
just wakes up the host side worker threads.  There's also
CROSS_DOMAIN_CMD_SEND which just sends data to the Wayland server.

This prevents the need to signal the automatically created
virtio_gpu_fence.

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
Reviewed-by: <dmitry.osipenko@collabora.com>
---
 v2: Fix indent (Dmitry)

 drivers/gpu/drm/virtio/virtgpu_submit.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_submit.c b/drivers/gpu/drm/virtio/virtgpu_submit.c
index cf3c04b16a7a..8c7e15c31164 100644
--- a/drivers/gpu/drm/virtio/virtgpu_submit.c
+++ b/drivers/gpu/drm/virtio/virtgpu_submit.c
@@ -168,9 +168,13 @@ static int virtio_gpu_init_submit(struct virtio_gpu_submit *submit,
 
 	memset(submit, 0, sizeof(*submit));
 
-	out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
-	if (!out_fence)
-		return -ENOMEM;
+	if ((exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_OUT) ||
+	    ((exbuf->flags & VIRTGPU_EXECBUF_RING_IDX) &&
+	    (vfpriv->ring_idx_mask & BIT_ULL(ring_idx))) ||
+	    exbuf->num_bo_handles)
+		out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
+	else
+		out_fence = NULL;
 
 	err = virtio_gpu_fence_event_create(dev, file, out_fence, ring_idx);
 	if (err) {
-- 
2.41.0.162.gfafddb0af9-goog

Re: [PATCH v2] drm/virtio: conditionally allocate virtio_gpu_fence

[Gurchetan Singh]

On Wed, Jun 28, 2023 at 8:58 AM Gurchetan Singh
<gurchetansingh@chromium.org> wrote:
>
> We don't want to create a fence for every command submission.  It's
> only necessary when userspace provides a waitable token for submission.
> This could be:
>
> 1) bo_handles, to be used with VIRTGPU_WAIT
> 2) out_fence_fd, to be used with dma_fence apis
> 3) a ring_idx provided with VIRTGPU_CONTEXT_PARAM_POLL_RINGS_MASK
>    + DRM event API
> 4) syncobjs in the future
>
> The use case for just submitting a command to the host, and expected
> no response.  For example, gfxstream has GFXSTREAM_CONTEXT_PING that
> just wakes up the host side worker threads.  There's also
> CROSS_DOMAIN_CMD_SEND which just sends data to the Wayland server.
>
> This prevents the need to signal the automatically created
> virtio_gpu_fence.
>
> Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
> Reviewed-by: <dmitry.osipenko@collabora.com>
> ---
>  v2: Fix indent (Dmitry)
>
>  drivers/gpu/drm/virtio/virtgpu_submit.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_submit.c b/drivers/gpu/drm/virtio/virtgpu_submit.c
> index cf3c04b16a7a..8c7e15c31164 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_submit.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_submit.c
> @@ -168,9 +168,13 @@ static int virtio_gpu_init_submit(struct virtio_gpu_submit *submit,
>
>         memset(submit, 0, sizeof(*submit));
>
> -       out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
> -       if (!out_fence)
> -               return -ENOMEM;
> +       if ((exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_OUT) ||
> +           ((exbuf->flags & VIRTGPU_EXECBUF_RING_IDX) &&
> +           (vfpriv->ring_idx_mask & BIT_ULL(ring_idx))) ||
> +           exbuf->num_bo_handles)
> +               out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
> +       else
> +               out_fence = NULL;
>
>         err = virtio_gpu_fence_event_create(dev, file, out_fence, ring_idx);
>         if (err) {
> --

Ping for additional reviews or merge.

> 2.41.0.162.gfafddb0af9-goog
>

Re: [PATCH v2] drm/virtio: conditionally allocate virtio_gpu_fence

[Dmitry Osipenko]

On 7/5/23 18:54, Gurchetan Singh wrote:
> On Wed, Jun 28, 2023 at 8:58 AM Gurchetan Singh
> <gurchetansingh@chromium.org> wrote:
>>
>> We don't want to create a fence for every command submission.  It's
>> only necessary when userspace provides a waitable token for submission.
>> This could be:
>>
>> 1) bo_handles, to be used with VIRTGPU_WAIT
>> 2) out_fence_fd, to be used with dma_fence apis
>> 3) a ring_idx provided with VIRTGPU_CONTEXT_PARAM_POLL_RINGS_MASK
>>    + DRM event API
>> 4) syncobjs in the future
>>
>> The use case for just submitting a command to the host, and expected
>> no response.  For example, gfxstream has GFXSTREAM_CONTEXT_PING that
>> just wakes up the host side worker threads.  There's also
>> CROSS_DOMAIN_CMD_SEND which just sends data to the Wayland server.
>>
>> This prevents the need to signal the automatically created
>> virtio_gpu_fence.
>>
>> Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
>> Reviewed-by: <dmitry.osipenko@collabora.com>
>> ---
>>  v2: Fix indent (Dmitry)
>>
>>  drivers/gpu/drm/virtio/virtgpu_submit.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_submit.c b/drivers/gpu/drm/virtio/virtgpu_submit.c
>> index cf3c04b16a7a..8c7e15c31164 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_submit.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_submit.c
>> @@ -168,9 +168,13 @@ static int virtio_gpu_init_submit(struct virtio_gpu_submit *submit,
>>
>>         memset(submit, 0, sizeof(*submit));
>>
>> -       out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
>> -       if (!out_fence)
>> -               return -ENOMEM;
>> +       if ((exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_OUT) ||
>> +           ((exbuf->flags & VIRTGPU_EXECBUF_RING_IDX) &&
>> +           (vfpriv->ring_idx_mask & BIT_ULL(ring_idx))) ||
>> +           exbuf->num_bo_handles)
>> +               out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
>> +       else
>> +               out_fence = NULL;
>>
>>         err = virtio_gpu_fence_event_create(dev, file, out_fence, ring_idx);
>>         if (err) {
>> --
> 
> Ping for additional reviews or merge.

I tested this patch with virgl,venus and nctx. No problems spotted.
Going to apply it tomorrow if there won't be additional comments from
anyone.

Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

-- 
Best regards,
Dmitry

Re: [PATCH v2] drm/virtio: conditionally allocate virtio_gpu_fence

[Dmitry Osipenko]

On 6/28/23 18:58, Gurchetan Singh wrote:
> @@ -168,9 +168,13 @@ static int virtio_gpu_init_submit(struct virtio_gpu_submit *submit,
>  
>  	memset(submit, 0, sizeof(*submit));
>  
> -	out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
> -	if (!out_fence)
> -		return -ENOMEM;
> +	if ((exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_OUT) ||
> +	    ((exbuf->flags & VIRTGPU_EXECBUF_RING_IDX) &&
> +	    (vfpriv->ring_idx_mask & BIT_ULL(ring_idx))) ||

Looks like there is a problem here. The virtio_gpu_fence_event_create()
doesn't check whether VIRTGPU_EXECBUF_RING_IDX flag is set, so it's
possible to trigger NULL-deref in that function if userspace will set
ring_idx_mask=1. Perhaps virtio_gpu_fence_event_create() need to be
changed to check the flag presence.

-- 
Best regards,
Dmitry

Re: [PATCH v2] drm/virtio: conditionally allocate virtio_gpu_fence

[Dmitry Osipenko]

On 7/7/23 05:49, Dmitry Osipenko wrote:
> On 6/28/23 18:58, Gurchetan Singh wrote:
>> @@ -168,9 +168,13 @@ static int virtio_gpu_init_submit(struct virtio_gpu_submit *submit,
>>  
>>  	memset(submit, 0, sizeof(*submit));
>>  
>> -	out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
>> -	if (!out_fence)
>> -		return -ENOMEM;
>> +	if ((exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_OUT) ||
>> +	    ((exbuf->flags & VIRTGPU_EXECBUF_RING_IDX) &&
>> +	    (vfpriv->ring_idx_mask & BIT_ULL(ring_idx))) ||
> 
> Looks like there is a problem here. The virtio_gpu_fence_event_create()
> doesn't check whether VIRTGPU_EXECBUF_RING_IDX flag is set, so it's
> possible to trigger NULL-deref in that function if userspace will set
> ring_idx_mask=1. Perhaps virtio_gpu_fence_event_create() need to be
> changed to check the flag presence.

Or check whether fence is NULL

-- 
Best regards,
Dmitry

Re: [PATCH v2] drm/virtio: conditionally allocate virtio_gpu_fence

[Dmitry Osipenko]

On 7/7/23 05:53, Dmitry Osipenko wrote:
> On 7/7/23 05:49, Dmitry Osipenko wrote:
>> On 6/28/23 18:58, Gurchetan Singh wrote:
>>> @@ -168,9 +168,13 @@ static int virtio_gpu_init_submit(struct virtio_gpu_submit *submit,
>>>  
>>>  	memset(submit, 0, sizeof(*submit));
>>>  
>>> -	out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
>>> -	if (!out_fence)
>>> -		return -ENOMEM;
>>> +	if ((exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_OUT) ||
>>> +	    ((exbuf->flags & VIRTGPU_EXECBUF_RING_IDX) &&
>>> +	    (vfpriv->ring_idx_mask & BIT_ULL(ring_idx))) ||
>>
>> Looks like there is a problem here. The virtio_gpu_fence_event_create()
>> doesn't check whether VIRTGPU_EXECBUF_RING_IDX flag is set, so it's
>> possible to trigger NULL-deref in that function if userspace will set
>> ring_idx_mask=1. Perhaps virtio_gpu_fence_event_create() need to be
>> changed to check the flag presence.
> 
> Or check whether fence is NULL

Actually, maybe this code shouldn't check VIRTGPU_EXECBUF_RING_IDX flag
at all. This flag tells which ring to use fo submission, but not which
ring to poll. Please check and correct it in v3.

-- 
Best regards,
Dmitry

[PATCH v3] drm/virtio: conditionally allocate virtio_gpu_fence

[Gurchetan Singh]

We don't want to create a fence for every command submission.  It's
only necessary when userspace provides a waitable token for submission.
This could be:

1) bo_handles, to be used with VIRTGPU_WAIT
2) out_fence_fd, to be used with dma_fence apis
3) a ring_idx provided with VIRTGPU_CONTEXT_PARAM_POLL_RINGS_MASK
   + DRM event API
4) syncobjs in the future

The use case for just submitting a command to the host, and expecting
no response.  For example, gfxstream has GFXSTREAM_CONTEXT_PING that
just wakes up the host side worker threads.  There's also
CROSS_DOMAIN_CMD_SEND which just sends data to the Wayland server.

This prevents the need to signal the automatically created
virtio_gpu_fence.

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
---
 v2: Fix indent (Dmitry)
 v3: Refactor drm fence event checks to avoid possible NULL deref (Dmitry)

 drivers/gpu/drm/virtio/virtgpu_submit.c | 28 +++++++++++++------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_submit.c b/drivers/gpu/drm/virtio/virtgpu_submit.c
index cf3c04b16a7a..004364cf86d7 100644
--- a/drivers/gpu/drm/virtio/virtgpu_submit.c
+++ b/drivers/gpu/drm/virtio/virtgpu_submit.c
@@ -64,13 +64,9 @@ static int virtio_gpu_fence_event_create(struct drm_device *dev,
 					 struct virtio_gpu_fence *fence,
 					 u32 ring_idx)
 {
-	struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
 	struct virtio_gpu_fence_event *e = NULL;
 	int ret;
 
-	if (!(vfpriv->ring_idx_mask & BIT_ULL(ring_idx)))
-		return 0;
-
 	e = kzalloc(sizeof(*e), GFP_KERNEL);
 	if (!e)
 		return -ENOMEM;
@@ -161,21 +157,27 @@ static int virtio_gpu_init_submit(struct virtio_gpu_submit *submit,
 				  struct drm_file *file,
 				  u64 fence_ctx, u32 ring_idx)
 {
+	int err;
+	struct virtio_gpu_fence *out_fence;
 	struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
 	struct virtio_gpu_device *vgdev = dev->dev_private;
-	struct virtio_gpu_fence *out_fence;
-	int err;
+	bool drm_fence_event = (exbuf->flags & VIRTGPU_EXECBUF_RING_IDX) &&
+			       (vfpriv->ring_idx_mask & BIT_ULL(ring_idx));
 
 	memset(submit, 0, sizeof(*submit));
 
-	out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
-	if (!out_fence)
-		return -ENOMEM;
+	if ((exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_OUT) || drm_fence_event ||
+	     exbuf->num_bo_handles)
+		out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
+	else
+		out_fence = NULL;
 
-	err = virtio_gpu_fence_event_create(dev, file, out_fence, ring_idx);
-	if (err) {
-		dma_fence_put(&out_fence->f);
-		return err;
+	if (drm_fence_event) {
+		err = virtio_gpu_fence_event_create(dev, file, out_fence, ring_idx);
+		if (err) {
+			dma_fence_put(&out_fence->f);
+			return err;
+		}
 	}
 
 	submit->out_fence = out_fence;
-- 
2.41.0.255.g8b1d071c50-goog

Re: [PATCH v3] drm/virtio: conditionally allocate virtio_gpu_fence

[Dmitry Osipenko]

On 7/7/23 18:43, Gurchetan Singh wrote:
> @@ -161,21 +157,27 @@ static int virtio_gpu_init_submit(struct virtio_gpu_submit *submit,
>  				  struct drm_file *file,
>  				  u64 fence_ctx, u32 ring_idx)
>  {
> +	int err;
> +	struct virtio_gpu_fence *out_fence;
>  	struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
>  	struct virtio_gpu_device *vgdev = dev->dev_private;
> -	struct virtio_gpu_fence *out_fence;
> -	int err;
> +	bool drm_fence_event = (exbuf->flags & VIRTGPU_EXECBUF_RING_IDX) &&
> +			       (vfpriv->ring_idx_mask & BIT_ULL(ring_idx));

Previously, when VIRTGPU_EXECBUF_RING_IDX flag wasn't specified, the
fence event was created for a default ring_idx=0. Now you changed this
behaviour and event will never be created without
VIRTGPU_EXECBUF_RING_IDX flag being set.

Could you please point me at the corresponding userspace code that polls
DRM FD fence event?

It's unclear whether there is a possible userspace regression here or
not. If there is no regression, then in general such behavioural changes
should be done in a separate commit having detailed description
explaining why behaviour changes.

-- 
Best regards,
Dmitry

Re: [PATCH v3] drm/virtio: conditionally allocate virtio_gpu_fence

[Dmitry Osipenko]

On 7/7/23 20:04, Dmitry Osipenko wrote:
> On 7/7/23 18:43, Gurchetan Singh wrote:
>> @@ -161,21 +157,27 @@ static int virtio_gpu_init_submit(struct virtio_gpu_submit *submit,
>>  				  struct drm_file *file,
>>  				  u64 fence_ctx, u32 ring_idx)
>>  {
>> +	int err;
>> +	struct virtio_gpu_fence *out_fence;
>>  	struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
>>  	struct virtio_gpu_device *vgdev = dev->dev_private;
>> -	struct virtio_gpu_fence *out_fence;
>> -	int err;
>> +	bool drm_fence_event = (exbuf->flags & VIRTGPU_EXECBUF_RING_IDX) &&
>> +			       (vfpriv->ring_idx_mask & BIT_ULL(ring_idx));
> 
> Previously, when VIRTGPU_EXECBUF_RING_IDX flag wasn't specified, the
> fence event was created for a default ring_idx=0. Now you changed this
> behaviour and event will never be created without
> VIRTGPU_EXECBUF_RING_IDX flag being set.
> 
> Could you please point me at the corresponding userspace code that polls
> DRM FD fence event?
> 
> It's unclear whether there is a possible userspace regression here or
> not. If there is no regression, then in general such behavioural changes
> should be done in a separate commit having detailed description
> explaining why behaviour changes.

I see that venus does the polling and ring_idx_mask is a
context-specific param, hence it's irrelevant to a generic ctx 0. Still
it's now necessary to specify the EXECBUF_RING_IDX flag even if ctx has
one ring, which is UAPI change.

-- 
Best regards,
Dmitry

Re: [PATCH v3] drm/virtio: conditionally allocate virtio_gpu_fence

[Gurchetan Singh]

On Fri, Jul 7, 2023 at 10:35 AM Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> On 7/7/23 20:04, Dmitry Osipenko wrote:
> > On 7/7/23 18:43, Gurchetan Singh wrote:
> >> @@ -161,21 +157,27 @@ static int virtio_gpu_init_submit(struct virtio_gpu_submit *submit,
> >>                                struct drm_file *file,
> >>                                u64 fence_ctx, u32 ring_idx)
> >>  {
> >> +    int err;
> >> +    struct virtio_gpu_fence *out_fence;
> >>      struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
> >>      struct virtio_gpu_device *vgdev = dev->dev_private;
> >> -    struct virtio_gpu_fence *out_fence;
> >> -    int err;
> >> +    bool drm_fence_event = (exbuf->flags & VIRTGPU_EXECBUF_RING_IDX) &&
> >> +                           (vfpriv->ring_idx_mask & BIT_ULL(ring_idx));
> >
> > Previously, when VIRTGPU_EXECBUF_RING_IDX flag wasn't specified, the
> > fence event was created for a default ring_idx=0. Now you changed this
> > behaviour and event will never be created without
> > VIRTGPU_EXECBUF_RING_IDX flag being set.

ring_idx = 0 is fine, but without VIRTGPU_EXECBUF_RING_IDX that means
the global timeline.

It's an additional check for where the userspace specifies they want
to use per-context fencing and polling, but actually uses the global
timeline.  Userspace never does this since it wouldn't work, so it's a
bit of a pathological edge case check than any UAPI change.

> >
> > Could you please point me at the corresponding userspace code that polls
> > DRM FD fence event?

https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/vm_tools/sommelier/virtualization/virtgpu_channel.cc#216

Used with the following flow:

https://crosvm.dev/book/devices/wayland.html

If you wish to test, please do apply this change:

https://chromium-review.googlesource.com/c/chromiumos/platform2/+/4605854

> >
> > It's unclear whether there is a possible userspace regression here or
> > not. If there is no regression, then in general such behavioural changes
> > should be done in a separate commit having detailed description
> > explaining why behaviour changes.

Sommelier isn't formally packaged yet in the Linux distro style and it
always specifies RING_IDX when polling, so no regressions here.  Maybe
a separate commit is overkill (since the 2nd commit would delete the
newly added checks), what about just more detail in the commit
message?

>
> I see that venus does the polling and ring_idx_mask is a
> context-specific param, hence it's irrelevant to a generic ctx 0. Still
> it's now necessary to specify the EXECBUF_RING_IDX flag even if ctx has
> one ring, which is UAPI change.

It doesn't seem like venus enables POLL_RINGS_MASK to poll since that
param is zero?

https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/virtio/vulkan/vn_renderer_virtgpu.c#L617



>
> --
> Best regards,
> Dmitry
>

Re: [PATCH v3] drm/virtio: conditionally allocate virtio_gpu_fence

[Dmitry Osipenko]

On 7/7/23 20:59, Gurchetan Singh wrote:
///
>>> Previously, when VIRTGPU_EXECBUF_RING_IDX flag wasn't specified, the
>>> fence event was created for a default ring_idx=0. Now you changed this
>>> behaviour and event will never be created without
>>> VIRTGPU_EXECBUF_RING_IDX flag being set.
> 
> ring_idx = 0 is fine, but without VIRTGPU_EXECBUF_RING_IDX that means
> the global timeline.
> 
> It's an additional check for where the userspace specifies they want
> to use per-context fencing and polling, but actually uses the global
> timeline.  Userspace never does this since it wouldn't work, so it's a
> bit of a pathological edge case check than any UAPI change.
> 
>>>
>>> Could you please point me at the corresponding userspace code that polls
>>> DRM FD fence event?
> 
> https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/vm_tools/sommelier/virtualization/virtgpu_channel.cc#216
> 
> Used with the following flow:
> 
> https://crosvm.dev/book/devices/wayland.html
> 
> If you wish to test, please do apply this change:
> 
> https://chromium-review.googlesource.com/c/chromiumos/platform2/+/4605854

Thanks for the links! I tested v2 with sommelier, though wasn't aware
about CL4605854 and alternatives to sommelier

>>> It's unclear whether there is a possible userspace regression here or
>>> not. If there is no regression, then in general such behavioural changes
>>> should be done in a separate commit having detailed description
>>> explaining why behaviour changes.
> 
> Sommelier isn't formally packaged yet in the Linux distro style and it
> always specifies RING_IDX when polling, so no regressions here.  Maybe
> a separate commit is overkill (since the 2nd commit would delete the
> newly added checks), what about just more detail in the commit
> message?

More detail will be fine

>> I see that venus does the polling and ring_idx_mask is a
>> context-specific param, hence it's irrelevant to a generic ctx 0. Still
>> it's now necessary to specify the EXECBUF_RING_IDX flag even if ctx has
>> one ring, which is UAPI change.
> 
> It doesn't seem like venus enables POLL_RINGS_MASK to poll since that
> param is zero?
> 
> https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/virtio/vulkan/vn_renderer_virtgpu.c#L617

Indeed, good catch

-- 
Best regards,
Dmitry

[PATCH v4] drm/virtio: conditionally allocate virtio_gpu_fence

[Gurchetan Singh]

We don't want to create a fence for every command submission.  It's
only necessary when userspace provides a waitable token for submission.
This could be:

1) bo_handles, to be used with VIRTGPU_WAIT
2) out_fence_fd, to be used with dma_fence apis
3) a ring_idx provided with VIRTGPU_CONTEXT_PARAM_POLL_RINGS_MASK
   + DRM event API
4) syncobjs in the future

The use case for just submitting a command to the host, and expecting
no response.  For example, gfxstream has GFXSTREAM_CONTEXT_PING that
just wakes up the host side worker threads.  There's also
CROSS_DOMAIN_CMD_SEND which just sends data to the Wayland server.

This prevents the need to signal the automatically created
virtio_gpu_fence.

In addition, VIRTGPU_EXECBUF_RING_IDX is checked when creating a
DRM event object.  VIRTGPU_CONTEXT_PARAM_POLL_RINGS_MASK is
already defined in terms of per-context rings.  It was theoretically
possible to create a DRM event on the global timeline (ring_idx == 0),
if the context enabled DRM event polling.  However, that wouldn't
work and userspace (Sommelier).  Explicitly disallow it for
clarity.

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
---
 v2: Fix indent (Dmitry)
 v3: Refactor drm fence event checks to avoid possible NULL deref (Dmitry)
 v4: More detailed commit message about addition drm fence event checks (Dmitry)

 drivers/gpu/drm/virtio/virtgpu_submit.c | 28 +++++++++++++------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_submit.c b/drivers/gpu/drm/virtio/virtgpu_submit.c
index cf3c04b16a7a..004364cf86d7 100644
--- a/drivers/gpu/drm/virtio/virtgpu_submit.c
+++ b/drivers/gpu/drm/virtio/virtgpu_submit.c
@@ -64,13 +64,9 @@ static int virtio_gpu_fence_event_create(struct drm_device *dev,
 					 struct virtio_gpu_fence *fence,
 					 u32 ring_idx)
 {
-	struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
 	struct virtio_gpu_fence_event *e = NULL;
 	int ret;
 
-	if (!(vfpriv->ring_idx_mask & BIT_ULL(ring_idx)))
-		return 0;
-
 	e = kzalloc(sizeof(*e), GFP_KERNEL);
 	if (!e)
 		return -ENOMEM;
@@ -161,21 +157,27 @@ static int virtio_gpu_init_submit(struct virtio_gpu_submit *submit,
 				  struct drm_file *file,
 				  u64 fence_ctx, u32 ring_idx)
 {
+	int err;
+	struct virtio_gpu_fence *out_fence;
 	struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
 	struct virtio_gpu_device *vgdev = dev->dev_private;
-	struct virtio_gpu_fence *out_fence;
-	int err;
+	bool drm_fence_event = (exbuf->flags & VIRTGPU_EXECBUF_RING_IDX) &&
+			       (vfpriv->ring_idx_mask & BIT_ULL(ring_idx));
 
 	memset(submit, 0, sizeof(*submit));
 
-	out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
-	if (!out_fence)
-		return -ENOMEM;
+	if ((exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_OUT) || drm_fence_event ||
+	     exbuf->num_bo_handles)
+		out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
+	else
+		out_fence = NULL;
 
-	err = virtio_gpu_fence_event_create(dev, file, out_fence, ring_idx);
-	if (err) {
-		dma_fence_put(&out_fence->f);
-		return err;
+	if (drm_fence_event) {
+		err = virtio_gpu_fence_event_create(dev, file, out_fence, ring_idx);
+		if (err) {
+			dma_fence_put(&out_fence->f);
+			return err;
+		}
 	}
 
 	submit->out_fence = out_fence;
-- 
2.41.0.255.g8b1d071c50-goog

Re: [PATCH v4] drm/virtio: conditionally allocate virtio_gpu_fence

[Dmitry Osipenko]

On 7/8/23 00:31, Gurchetan Singh wrote:
> We don't want to create a fence for every command submission.  It's
> only necessary when userspace provides a waitable token for submission.
> This could be:
> 
> 1) bo_handles, to be used with VIRTGPU_WAIT
> 2) out_fence_fd, to be used with dma_fence apis
> 3) a ring_idx provided with VIRTGPU_CONTEXT_PARAM_POLL_RINGS_MASK
>    + DRM event API
> 4) syncobjs in the future
> 
> The use case for just submitting a command to the host, and expecting
> no response.  For example, gfxstream has GFXSTREAM_CONTEXT_PING that
> just wakes up the host side worker threads.  There's also
> CROSS_DOMAIN_CMD_SEND which just sends data to the Wayland server.
> 
> This prevents the need to signal the automatically created
> virtio_gpu_fence.
> 
> In addition, VIRTGPU_EXECBUF_RING_IDX is checked when creating a
> DRM event object.  VIRTGPU_CONTEXT_PARAM_POLL_RINGS_MASK is
> already defined in terms of per-context rings.  It was theoretically
> possible to create a DRM event on the global timeline (ring_idx == 0),
> if the context enabled DRM event polling.  However, that wouldn't
> work and userspace (Sommelier).  Explicitly disallow it for
> clarity.
> 
> Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
> ---
>  v2: Fix indent (Dmitry)
>  v3: Refactor drm fence event checks to avoid possible NULL deref (Dmitry)
>  v4: More detailed commit message about addition drm fence event checks (Dmitry)
> 
>  drivers/gpu/drm/virtio/virtgpu_submit.c | 28 +++++++++++++------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_submit.c b/drivers/gpu/drm/virtio/virtgpu_submit.c
> index cf3c04b16a7a..004364cf86d7 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_submit.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_submit.c
> @@ -64,13 +64,9 @@ static int virtio_gpu_fence_event_create(struct drm_device *dev,
>  					 struct virtio_gpu_fence *fence,
>  					 u32 ring_idx)
>  {
> -	struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
>  	struct virtio_gpu_fence_event *e = NULL;
>  	int ret;
>  
> -	if (!(vfpriv->ring_idx_mask & BIT_ULL(ring_idx)))
> -		return 0;
> -
>  	e = kzalloc(sizeof(*e), GFP_KERNEL);
>  	if (!e)
>  		return -ENOMEM;
> @@ -161,21 +157,27 @@ static int virtio_gpu_init_submit(struct virtio_gpu_submit *submit,
>  				  struct drm_file *file,
>  				  u64 fence_ctx, u32 ring_idx)
>  {
> +	int err;
> +	struct virtio_gpu_fence *out_fence;
>  	struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
>  	struct virtio_gpu_device *vgdev = dev->dev_private;
> -	struct virtio_gpu_fence *out_fence;
> -	int err;
> +	bool drm_fence_event = (exbuf->flags & VIRTGPU_EXECBUF_RING_IDX) &&
> +			       (vfpriv->ring_idx_mask & BIT_ULL(ring_idx));

The common coding style for variables definition in kernel is a "reverse
xmas tree". It makes code easier to read.

*********
******
***

I'll change the style while applying to:

	if ((exbuf->flags & VIRTGPU_EXECBUF_RING_IDX) &&
	    (vfpriv->ring_idx_mask & BIT_ULL(ring_idx)))
		drm_fence_event = true;
	else
		drm_fence_event = false;

>  	memset(submit, 0, sizeof(*submit));
>  
> -	out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
> -	if (!out_fence)
> -		return -ENOMEM;
> +	if ((exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_OUT) || drm_fence_event ||
> +	     exbuf->num_bo_handles)
> +		out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
> +	else
> +		out_fence = NULL;
>  
> -	err = virtio_gpu_fence_event_create(dev, file, out_fence, ring_idx);
> -	if (err) {
> -		dma_fence_put(&out_fence->f);
> -		return err;
> +	if (drm_fence_event) {
> +		err = virtio_gpu_fence_event_create(dev, file, out_fence, ring_idx);
> +		if (err) {
> +			dma_fence_put(&out_fence->f);
> +			return err;
> +		}
>  	}
>  
>  	submit->out_fence = out_fence;

Another small note for the future is that you should always start a new
email thread for every new version of the patch, i.e. don't reply with
new version to the old thread. This is not a problem here since it's
just a single patch, nevertheless please take it into account later on.
It eases patch tracking for reviewers.

I tested v4, including the applied CL4605854 to Sommilier. Everything
works well as before. Thank you for addressing all the issues.

Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

-- 
Best regards,
Dmitry

Re: [PATCH v4] drm/virtio: conditionally allocate virtio_gpu_fence

[Dmitry Osipenko]

08.07.2023 00:31, Gurchetan Singh пишет:
> We don't want to create a fence for every command submission.  It's
> only necessary when userspace provides a waitable token for submission.
> This could be:
> 
> 1) bo_handles, to be used with VIRTGPU_WAIT
> 2) out_fence_fd, to be used with dma_fence apis
> 3) a ring_idx provided with VIRTGPU_CONTEXT_PARAM_POLL_RINGS_MASK
>    + DRM event API
> 4) syncobjs in the future
> 
> The use case for just submitting a command to the host, and expecting
> no response.  For example, gfxstream has GFXSTREAM_CONTEXT_PING that
> just wakes up the host side worker threads.  There's also
> CROSS_DOMAIN_CMD_SEND which just sends data to the Wayland server.
> 
> This prevents the need to signal the automatically created
> virtio_gpu_fence.
> 
> In addition, VIRTGPU_EXECBUF_RING_IDX is checked when creating a
> DRM event object.  VIRTGPU_CONTEXT_PARAM_POLL_RINGS_MASK is
> already defined in terms of per-context rings.  It was theoretically
> possible to create a DRM event on the global timeline (ring_idx == 0),
> if the context enabled DRM event polling.  However, that wouldn't
> work and userspace (Sommelier).  Explicitly disallow it for
> clarity.
> 
> Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
> ---
>  v2: Fix indent (Dmitry)
>  v3: Refactor drm fence event checks to avoid possible NULL deref (Dmitry)
>  v4: More detailed commit message about addition drm fence event checks (Dmitry)
> 
>  drivers/gpu/drm/virtio/virtgpu_submit.c | 28 +++++++++++++------------
>  1 file changed, 15 insertions(+), 13 deletions(-)

Applied to misc-next

-- 
Best regards,
Dmitry