[PATCH v4 2/5] drm/virtio: Add a helper to map and note the dma addrs and lengths

[Kasireddy, Vivek]

Hi Dmitry,

> Subject: Re: [PATCH v4 2/5] drm/virtio: Add a helper to map and note the
> dma addrs and lengths
> 
> > +int virtgpu_dma_buf_import_sgt(struct virtio_gpu_mem_entry **ents,
> > +			       unsigned int *nents,
> > +			       struct virtio_gpu_object *bo,
> > +			       struct dma_buf_attachment *attach)
> > +{
> > +	struct scatterlist *sl;
> > +	struct sg_table *sgt;
> > +	long i, ret;
> > +
> > +	dma_resv_assert_held(attach->dmabuf->resv);
> > +
> > +	ret = dma_resv_wait_timeout(attach->dmabuf->resv,
> > +				    DMA_RESV_USAGE_KERNEL,
> > +				    false, MAX_SCHEDULE_TIMEOUT);
> > +	if (ret <= 0)
> > +		return ret < 0 ? ret : -ETIMEDOUT;
> > +
> > +	sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
> > +	if (IS_ERR(sgt))
> > +		return PTR_ERR(sgt);
> > +
> > +	*ents = kvmalloc_array(sgt->nents,
> > +			       sizeof(struct virtio_gpu_mem_entry),
> > +			       GFP_KERNEL);
> > +	if (!(*ents)) {
> > +		dma_buf_unmap_attachment(attach, sgt,
> DMA_BIDIRECTIONAL);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	*nents = sgt->nents;
> > +	for_each_sgtable_dma_sg(sgt, sl, i) {
> > +		(*ents)[i].addr = cpu_to_le64(sg_dma_address(sl));
> > +		(*ents)[i].length = cpu_to_le32(sg_dma_len(sl));
> > +		(*ents)[i].padding = 0;
> > +	}
> > +
> > +	bo->sgt = sgt;
> > +	bo->detached = false;
> 
> This bo->detached=false was missing in v3 and it fixes the flickering
> problem, as expected. \o/
> 
> The DG2+virtio-gpu still don't work for me in guest using both i915 and
> Xe drivers like before:
> 
> Invalid read at addr 0x7200005014, size 1, region '(null)', reason: rejected
> Invalid write at addr 0x7200005014, size 1, region '(null)', reason:
> rejected
> Invalid write at addr 0x7200005000, size 4, region '(null)', reason:
> rejected
> Invalid read at addr 0x7200005004, size 4, region '(null)', reason: rejected
> Invalid write at addr 0x7200005000, size 4, region '(null)', reason:
> rejected
> Invalid read at addr 0x7200005004, size 4, region '(null)', reason: rejected
> [    0.876189] virtio_gpu virtio1: virtio: device uses modern interface
> but does not have VIRTIO_F_VERSION_1
> Invalid read at addr 0x7200005014, size 1, region '(null)', reason: rejected
> Invalid write at addr 0x7200005014, size 1, region '(null)', reason:
> rejected
> [    0.876597] virtio_gpu virtio1: probe with driver virtio_gpu failed
> with error -22
> 
> Wondering if it could be a problem with my guest kernel config. I
> attached my config to the email, please try to boot guest with my config
> if you'll have time.
Sure, let me try to test with your config. Could you also please share your
Qemu launch parameters?

> 
> I'm almost ready to apply the patches after a small code improvement.
> Let's move bo->detached=false into virtio_gpu_object_attach() and
> replace bo->detached with bo->attached. I can apply this change while
> merging patches, otherwise please make v5 if you agree with the suggestion:
I definitely agree with your suggestion; attached is much more intuitive than
detached. I'll include the following changes (along with your other suggestions) in v5.

Thanks,
Vivek

> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
> b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index ad6960aa464e..8328107edf78 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -93,7 +93,7 @@ struct virtio_gpu_object {
>  	uint32_t hw_res_handle;
>  	bool dumb;
>  	bool created;
> -	bool detached;
> +	bool attached;
>  	bool host3d_blob, guest_blob;
>  	uint32_t blob_mem, blob_flags;
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c
> b/drivers/gpu/drm/virtio/virtgpu_object.c
> index 2b19bb8c6ec3..5517cff8715c 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_object.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_object.c
> @@ -105,7 +105,7 @@ int virtio_gpu_detach_object_fenced(struct
> virtio_gpu_object *bo)
>  	struct virtio_gpu_device *vgdev = bo->base.base.dev->dev_private;
>  	struct virtio_gpu_fence *fence;
> 
> -	if (bo->detached)
> +	if (!bo->attached)
>  		return 0;
> 
>  	fence = virtio_gpu_fence_alloc(vgdev, vgdev->fence_drv.context, 0);
> @@ -118,8 +118,6 @@ int virtio_gpu_detach_object_fenced(struct
> virtio_gpu_object *bo)
>  	dma_fence_wait(&fence->f, false);
>  	dma_fence_put(&fence->f);
> 
> -	bo->detached = true;
> -
>  	return 0;
>  }
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c
> b/drivers/gpu/drm/virtio/virtgpu_prime.c
> index 5031b800c521..3c4ea592e23c 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_prime.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_prime.c
> @@ -181,7 +181,6 @@ int virtgpu_dma_buf_import_sgt(struct
> virtio_gpu_mem_entry **ents,
>  	}
> 
>  	bo->sgt = sgt;
> -	bo->detached = false;
>  	return 0;
>  }
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c
> b/drivers/gpu/drm/virtio/virtgpu_vq.c
> index b8b296b912a1..ad91624df42d 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
> @@ -1120,16 +1120,26 @@ void virtio_gpu_object_attach(struct
> virtio_gpu_device *vgdev,
>  			      struct virtio_gpu_mem_entry *ents,
>  			      unsigned int nents)
>  {
> +	if (obj->attached)
> +		return;
> +
>  	virtio_gpu_cmd_resource_attach_backing(vgdev, obj-
> >hw_res_handle,
>  					       ents, nents, NULL);
> +
> +	obj->attached = true;
>  }
> 
>  void virtio_gpu_object_detach(struct virtio_gpu_device *vgdev,
>  			      struct virtio_gpu_object *obj,
>  			      struct virtio_gpu_fence *fence)
>  {
> +	if (!obj->attached)
> +		return;
> +
>  	virtio_gpu_cmd_resource_detach_backing(vgdev, obj-
> >hw_res_handle,
>  					       fence);
> +
> +	obj->attached = false;
>  }
> 
>  void virtio_gpu_cursor_ping(struct virtio_gpu_device *vgdev,
> 
> 
> 
> --
> Best regards,
> Dmitry