[PATCH v1 1/2] drm/virtio: Don't attach GEM to a non-created context in gem_object_open()

[PATCH v1 1/2] drm/virtio: Don't attach GEM to a non-created context in gem_object_open()

[Dmitry Osipenko]

The vfpriv->ctx_id is always initialized to a non-zero value. Check whether
context was created before attaching GEM to this context ID. This left
unnoticed previously because host silently skips attachment if context
doesn't exist, still we shouldn't do that for consistency.

Fixes: 086b9f27f0ab ("drm/virtio: Don't create a context with default param if context_init is supported")
Cc: <stable@vger.kernel.org> # v6.14+
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/virtio/virtgpu_gem.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
index dde8fc1a3689..90c99d83c4cf 100644
--- a/drivers/gpu/drm/virtio/virtgpu_gem.c
+++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
@@ -115,13 +115,14 @@ int virtio_gpu_gem_object_open(struct drm_gem_object *obj,
 	if (!vgdev->has_context_init)
 		virtio_gpu_create_context(obj->dev, file);
 
-	objs = virtio_gpu_array_alloc(1);
-	if (!objs)
-		return -ENOMEM;
-	virtio_gpu_array_add_obj(objs, obj);
+	if (vfpriv->context_created) {
+		objs = virtio_gpu_array_alloc(1);
+		if (!objs)
+			return -ENOMEM;
+		virtio_gpu_array_add_obj(objs, obj);
 
-	if (vfpriv->ctx_id)
 		virtio_gpu_cmd_context_attach_resource(vgdev, vfpriv->ctx_id, objs);
+	}
 
 out_notify:
 	virtio_gpu_notify(vgdev);
-- 
2.49.0

[PATCH v1 2/2] drm/virtio: Fix missed dmabuf unpinning in error path of prepare_fb()

[Dmitry Osipenko]

Unpin imported dmabuf on fence allocation failure in prepare_fb().

Fixes: 4a696a2ee646 ("drm/virtio: Add prepare and cleanup routines for imported dmabuf obj")
Cc: <stable@vger.kernel.org> # v6.14+
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/virtio/virtgpu_plane.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
index a6f5a78f436a..dc1d91639931 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -342,6 +342,16 @@ static int virtio_gpu_prepare_imported_obj(struct drm_plane *plane,
 	return ret;
 }
 
+static void virtio_gpu_cleanup_imported_obj(struct drm_gem_object *obj)
+{
+	struct dma_buf_attachment *attach = obj->import_attach;
+	struct dma_resv *resv = attach->dmabuf->resv;
+
+	dma_resv_lock(resv, NULL);
+	dma_buf_unpin(attach);
+	dma_resv_unlock(resv);
+}
+
 static int virtio_gpu_plane_prepare_fb(struct drm_plane *plane,
 				       struct drm_plane_state *new_state)
 {
@@ -376,23 +386,16 @@ static int virtio_gpu_plane_prepare_fb(struct drm_plane *plane,
 		vgplane_st->fence = virtio_gpu_fence_alloc(vgdev,
 						     vgdev->fence_drv.context,
 						     0);
-		if (!vgplane_st->fence)
+		if (!vgplane_st->fence) {
+			if (obj->import_attach)
+				virtio_gpu_cleanup_imported_obj(obj);
 			return -ENOMEM;
+		}
 	}
 
 	return 0;
 }
 
-static void virtio_gpu_cleanup_imported_obj(struct drm_gem_object *obj)
-{
-	struct dma_buf_attachment *attach = obj->import_attach;
-	struct dma_resv *resv = attach->dmabuf->resv;
-
-	dma_resv_lock(resv, NULL);
-	dma_buf_unpin(attach);
-	dma_resv_unlock(resv);
-}
-
 static void virtio_gpu_plane_cleanup_fb(struct drm_plane *plane,
 					struct drm_plane_state *state)
 {
-- 
2.49.0

RE: [PATCH v1 2/2] drm/virtio: Fix missed dmabuf unpinning in error path of prepare_fb()

[Kasireddy, Vivek]

Hi Dmitry,

> Subject: [PATCH v1 2/2] drm/virtio: Fix missed dmabuf unpinning in error path
> of prepare_fb()
> 
> Unpin imported dmabuf on fence allocation failure in prepare_fb().
> 
> Fixes: 4a696a2ee646 ("drm/virtio: Add prepare and cleanup routines for
> imported dmabuf obj")
> Cc: <stable@vger.kernel.org> # v6.14+
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>  drivers/gpu/drm/virtio/virtgpu_plane.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c
> b/drivers/gpu/drm/virtio/virtgpu_plane.c
> index a6f5a78f436a..dc1d91639931 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
> @@ -342,6 +342,16 @@ static int virtio_gpu_prepare_imported_obj(struct
> drm_plane *plane,
>  	return ret;
>  }
> 
> +static void virtio_gpu_cleanup_imported_obj(struct drm_gem_object *obj)
> +{
> +	struct dma_buf_attachment *attach = obj->import_attach;
> +	struct dma_resv *resv = attach->dmabuf->resv;
> +
> +	dma_resv_lock(resv, NULL);
> +	dma_buf_unpin(attach);
> +	dma_resv_unlock(resv);
> +}
> +
>  static int virtio_gpu_plane_prepare_fb(struct drm_plane *plane,
>  				       struct drm_plane_state *new_state)
>  {
> @@ -376,23 +386,16 @@ static int virtio_gpu_plane_prepare_fb(struct
> drm_plane *plane,
>  		vgplane_st->fence = virtio_gpu_fence_alloc(vgdev,
>  						     vgdev->fence_drv.context,
>  						     0);
> -		if (!vgplane_st->fence)
> +		if (!vgplane_st->fence) {
> +			if (obj->import_attach)
> +				virtio_gpu_cleanup_imported_obj(obj);
I think checking for fence allocation failure before import would be much better.
In other words, cleaning up the fence in case of any import errors would be
much simpler IMO.

Regardless,
Acked-by: Vivek Kasireddy <vivek.kasireddy@intel.com> 

>  			return -ENOMEM;
> +		}
>  	}
> 
>  	return 0;
>  }
> 
> -static void virtio_gpu_cleanup_imported_obj(struct drm_gem_object *obj)
> -{
> -	struct dma_buf_attachment *attach = obj->import_attach;
> -	struct dma_resv *resv = attach->dmabuf->resv;
> -
> -	dma_resv_lock(resv, NULL);
> -	dma_buf_unpin(attach);
> -	dma_resv_unlock(resv);
> -}
> -
>  static void virtio_gpu_plane_cleanup_fb(struct drm_plane *plane,
>  					struct drm_plane_state *state)
>  {
> --
> 2.49.0
> 

Re: [PATCH v1 2/2] drm/virtio: Fix missed dmabuf unpinning in error path of prepare_fb()

[Dmitry Osipenko]

On 3/26/25 08:14, Kasireddy, Vivek wrote:
...
>>  static int virtio_gpu_plane_prepare_fb(struct drm_plane *plane,
>>  				       struct drm_plane_state *new_state)
>>  {
>> @@ -376,23 +386,16 @@ static int virtio_gpu_plane_prepare_fb(struct
>> drm_plane *plane,
>>  		vgplane_st->fence = virtio_gpu_fence_alloc(vgdev,
>>  						     vgdev->fence_drv.context,
>>  						     0);
>> -		if (!vgplane_st->fence)
>> +		if (!vgplane_st->fence) {
>> +			if (obj->import_attach)
>> +				virtio_gpu_cleanup_imported_obj(obj);
> I think checking for fence allocation failure before import would be much better.
> In other words, cleaning up the fence in case of any import errors would be
> much simpler IMO.
> 
> Regardless,
> Acked-by: Vivek Kasireddy <vivek.kasireddy@intel.com> 

Another question, why do we need this fencing for imported dmabuf?
Fencing isn't done host/guest blobs in this code, while dmabuf is
essentially a guest blob. Could you please clarify why this fence is
needed? Maybe we shouldn't allocate fence in the first place for the dmabuf.

-- 
Best regards,
Dmitry

RE: [PATCH v1 2/2] drm/virtio: Fix missed dmabuf unpinning in error path of prepare_fb()

[Kasireddy, Vivek]

Hi Dmitry,

> Subject: Re: [PATCH v1 2/2] drm/virtio: Fix missed dmabuf unpinning in error
> path of prepare_fb()
> 
> On 3/26/25 08:14, Kasireddy, Vivek wrote:
> ...
> >>  static int virtio_gpu_plane_prepare_fb(struct drm_plane *plane,
> >>  				       struct drm_plane_state *new_state)
> >>  {
> >> @@ -376,23 +386,16 @@ static int virtio_gpu_plane_prepare_fb(struct
> >> drm_plane *plane,
> >>  		vgplane_st->fence = virtio_gpu_fence_alloc(vgdev,
> >>  						     vgdev->fence_drv.context,
> >>  						     0);
> >> -		if (!vgplane_st->fence)
> >> +		if (!vgplane_st->fence) {
> >> +			if (obj->import_attach)
> >> +				virtio_gpu_cleanup_imported_obj(obj);
> > I think checking for fence allocation failure before import would be much
> better.
> > In other words, cleaning up the fence in case of any import errors would be
> > much simpler IMO.
> >
> > Regardless,
> > Acked-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> 
> Another question, why do we need this fencing for imported dmabuf?
> Fencing isn't done host/guest blobs in this code, while dmabuf is
> essentially a guest blob. Could you please clarify why this fence is
> needed? Maybe we shouldn't allocate fence in the first place for the dmabuf.
At-least for the non-virgl use-cases (where the rendering is done in the Guest such
as in passthrough), this Guest/Host synchronization fence serves two purposes:
- It prevents the Guest from reusing/destroying the submitted buffer (Guest compositor
  FB) until the Host is done using it. Otherwise, the Guest compositor might render
  into this buffer at the same time while the Host is consuming it, leading to issues
  such as tearing/flickering. This problem is more noticeable in cases where the
  Guest compositor has only one backbuffer such as Xorg + dirtfyFb.

- It also prevents the Guest compositor from rendering faster than the Host refresh
  rate. In other words, it just sets the upper bound in terms of the buffer submission
  rate as there is no point in going over the Host refresh rate, which would likely
  lead to wastage of GPU cycles and dropped frames.

Therefore, this fence is really needed for Guest blobs including imported dmabufs.

Thanks,
Vivek

> 
> --
> Best regards,
> Dmitry

Re: [PATCH v1 2/2] drm/virtio: Fix missed dmabuf unpinning in error path of prepare_fb()

[Dmitry Osipenko]

On 3/27/25 10:14, Kasireddy, Vivek wrote:
>> Another question, why do we need this fencing for imported dmabuf?
>> Fencing isn't done host/guest blobs in this code, while dmabuf is
>> essentially a guest blob. Could you please clarify why this fence is
>> needed? Maybe we shouldn't allocate fence in the first place for the dmabuf.
> At-least for the non-virgl use-cases (where the rendering is done in the Guest such
> as in passthrough), this Guest/Host synchronization fence serves two purposes:
> - It prevents the Guest from reusing/destroying the submitted buffer (Guest compositor
>   FB) until the Host is done using it. Otherwise, the Guest compositor might render
>   into this buffer at the same time while the Host is consuming it, leading to issues
>   such as tearing/flickering. This problem is more noticeable in cases where the
>   Guest compositor has only one backbuffer such as Xorg + dirtfyFb.
> 
> - It also prevents the Guest compositor from rendering faster than the Host refresh
>   rate. In other words, it just sets the upper bound in terms of the buffer submission
>   rate as there is no point in going over the Host refresh rate, which would likely
>   lead to wastage of GPU cycles and dropped frames.
> 
> Therefore, this fence is really needed for Guest blobs including imported dmabufs.

Thanks a lot

-- 
Best regards,
Dmitry