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@xxxxxxxxx>
> 
> 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