Re: [RFC 5/7] drm/virtio: Ensure that bo's backing store is valid while updating plane

[Weifeng Liu <weifeng.liu.z@gmail.com>]

On Thu, 2024-03-28 at 01:32 -0700, Vivek Kasireddy wrote:
> To make sure that the imported bo's backing store is valid, we first
> pin the associated dmabuf, import the sgt if need be and then unpin
> it after the update is complete. Note that we pin/unpin the dmabuf
> even when the backing store is valid to ensure that it does not move
> when the host update (resource_flush) is in progress.
> 
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>  drivers/gpu/drm/virtio/virtgpu_plane.c | 56 +++++++++++++++++++++++++-
>  1 file changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
> index a72a2dbda031..3ccf88f9addc 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
> @@ -26,6 +26,7 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_damage_helper.h>
>  #include <drm/drm_fourcc.h>
> +#include <linux/virtio_dma_buf.h>
>  
>  #include "virtgpu_drv.h"
>  
> @@ -131,6 +132,45 @@ static void virtio_gpu_update_dumb_bo(struct virtio_gpu_device *vgdev,
>  					   objs, NULL);
>  }
>  
> +static bool virtio_gpu_update_dmabuf_bo(struct virtio_gpu_device *vgdev,
> +					struct drm_gem_object *obj)
> +{
> +	struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
> +	struct dma_buf_attachment *attach = obj->import_attach;
> +	struct dma_resv *resv = attach->dmabuf->resv;
> +	struct virtio_gpu_mem_entry *ents = NULL;
> +	unsigned int nents;
> +	int ret;
> +
> +	dma_resv_lock(resv, NULL);
> +
> +	ret = dma_buf_pin(attach);
> +	if (ret) {
> +		dma_resv_unlock(resv);
> +		return false;
> +	}
> +
> +	if (!bo->has_backing) {
> +		if (bo->sgt)
> +			dma_buf_unmap_attachment(attach,
> +						 bo->sgt,
> +						 DMA_BIDIRECTIONAL);
> +
> +		ret = virtgpu_dma_buf_import_sgt(&ents, &nents,
> +						 bo, attach);
> +		if (ret)
> +			goto err_import;
> +
> +		virtio_gpu_object_attach(vgdev, bo, ents, nents);
> +	}
> +	return true;
> +
> +err_import:
> +	dma_buf_unpin(attach);
> +	dma_resv_unlock(resv);
> +	return false;
> +}
> +
>  static void virtio_gpu_resource_flush(struct drm_plane *plane,
>  				      uint32_t x, uint32_t y,
>  				      uint32_t width, uint32_t height)
> @@ -174,7 +214,9 @@ static void virtio_gpu_primary_plane_update(struct drm_plane *plane,
>  	struct virtio_gpu_device *vgdev = dev->dev_private;
>  	struct virtio_gpu_output *output = NULL;
>  	struct virtio_gpu_object *bo;
> +	struct drm_gem_object *obj;
>  	struct drm_rect rect;
> +	bool updated = false;
>  
>  	if (plane->state->crtc)
>  		output = drm_crtc_to_virtio_gpu_output(plane->state->crtc);
> @@ -196,10 +238,17 @@ static void virtio_gpu_primary_plane_update(struct drm_plane *plane,
>  	if (!drm_atomic_helper_damage_merged(old_state, plane->state, &rect))
>  		return;
>  
> -	bo = gem_to_virtio_gpu_obj(plane->state->fb->obj[0]);
> +	obj = plane->state->fb->obj[0];
> +	bo = gem_to_virtio_gpu_obj(obj);
>  	if (bo->dumb)
>  		virtio_gpu_update_dumb_bo(vgdev, plane->state, &rect);
>  
> +	if (obj->import_attach) {
> +		updated = virtio_gpu_update_dmabuf_bo(vgdev, obj);
Hi Vivek,

It's possible that the objects imported from other devices are used in
other ways apart from being scanned out (e.g., they might act as
texture resources in 3D contexts in the virtio-GPU back-end).  Thus I
think we should find a better way of updating DMA-BUF objects, like
doing so in move_notify callback.

BTW, this patch set is very useful in implementing virtual display for
the case of SR-IOV, especially it supports sharing device local memory
between host and guest.  Thanks for your work and I am really hoping it
gets merged one day!

Best regards,
-Weifeng
> +		if (!updated)
> +			return;
> +	}
> +
>  	if (plane->state->fb != old_state->fb ||
>  	    plane->state->src_w != old_state->src_w ||
>  	    plane->state->src_h != old_state->src_h ||
> @@ -239,6 +288,11 @@ static void virtio_gpu_primary_plane_update(struct drm_plane *plane,
>  				  rect.y1,
>  				  rect.x2 - rect.x1,
>  				  rect.y2 - rect.y1);
> +
> +	if (obj->import_attach && updated) {
> +		dma_buf_unpin(obj->import_attach);
> +		dma_resv_unlock(obj->import_attach->dmabuf->resv);
> +	}
>  }
>  
>  static int virtio_gpu_plane_prepare_fb(struct drm_plane *plane,