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

[Kasireddy, Vivek]

Hi Dmitry,

> Subject: Re: [PATCH v2 2/5] drm/virtio: Add a helper to map and note the
> dma addrs and lengths
> >>>>> +long 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);
> >>>>
> >>>> Why this wait is needed?
> >>> The intention was to wait for any pending operations on the backing
> object
> >>> to finish and let it become idle before mapping and accessing the
> >> underlying
> >>> memory. But I suspect this wait may not be necessary given that we
> would
> >>> have already called dma_buf_pin() at this point, which would have had
> the
> >>> desired effect?
> >>
> >> Looking at the dma_buf_map_attachment() code, I see that it does both
> of
> >> pinning and waiting for the fence by itself. Hence should be safe to
> >> drop both dma_buf_pin() and dma_resv_wait_timeout(), please test.
> > Sure, I'll retest again but it looks like dma_buf_map_attachment() pins and
> > or waits for fence only in specific situations. That is, it pins only if the
> exporter
> > is non-dynamic and if CONFIG_DMABUF_MOVE_NOTIFY is not enabled.
> And,
> > it waits for the fence only if the importer is non-dynamic. Since I have only
> tested
> > with a dynamic exporter (Xe driver), I did not encounter any issues but it
> makes
> > sense to augment the code to account for non-dynamic exporters as well.
> 
> If exporter or importer is not dynamic, then dma-buf core pins buffer at
> the buffer importing time, see dma_buf_attach(). Hence, I expect that
> everything should work fine.
Given that virtio-gpu would always be a dynamic importer (in other words,
dma_buf_attachment_is_dynamic() is true) as proposed in this series, the
question really is about whether the exporter is dynamic or not.
I tested with both Xe driver (dynamic exporter) and i915 (non-dynamic)
and noted the following:

For Xe (dma_buf_is_dynamic() is true):
- dma-buf core calls pin only if CONFIG_DMABUF_MOVE_NOTIFY is not
  enabled, and extracts the sgt as part of dma_buf_map_attachment()
- It does not wait for the fences as part of map

For i915 (dma_buf_is_dynamic() is false):
- dma-buf core does not call pin anywhere as it is a no-op for non-dynamic
  exporters, but maps/extracts the sgt as part of dma_buf_dynamic_attach()
- It does not wait for fences anywhere

And, the docs mention the following rules for dynamic importers:
"DYNAMIC IMPORTER RULES:

Dynamic importers, see dma_buf_attachment_is_dynamic(), have additional
constraints on how they set up fences:

Dynamic importers must obey the write fences and wait for them to signal
before allowing access to the buffer’s underlying storage through the device.

Dynamic importers should set fences for any access that they can’t disable
immediately from their dma_buf_attach_ops.move_notify callback."

So, I believe we need to call pin and or dma_resv_wait_timeout(resv,
DMA_RESV_USAGE_WRITE,....) at some point, as part of import, given
these rules.

> 
> >> BTW, is any DG2 GPU suitable for testing of this patchset? Will I be
> >> able to test it using a regular consumer A750 card?
> > Yes, you can test with any DG2 dGPU as long as you can passthrough it to
> the
> > Guest VM. And, if there is an iGPU available on the Host, you can use
> GTK/SDL UI
> > for local display or Spice UI for remote display if there is no iGPU on the
> Host.
> > This is exactly how I started testing this patch series but I am now
> predominantly
> > testing this series with SRIOV enabled iGPUs and dGPUs.
> 
> Was hoping to try out SR-IOV on A750 if it's even possible at all.
AFAIK, SRIOV is not supported on any versions of DG2 including A750.

Thanks,
Vivek

> But passthrough indeed is another an option, will try with passthrough, thanks.
> 
> --
> Best regards,
> Dmitry