Messages in this thread | | | Subject | Re: [PATCH 01/12] dma-buf: add dynamic caching of sg_table | From | Christian König <> | Date | Wed, 22 May 2019 19:27:59 +0200 |
| |
Am 22.05.19 um 18:17 schrieb Sumit Semwal: > Hi Christian, > > On Sat, 27 Apr 2019 at 05:31, Liam Mark <lmark@codeaurora.org> wrote: >> On Tue, 16 Apr 2019, Christian König wrote: >> >>> To allow a smooth transition from pinning buffer objects to dynamic >>> invalidation we first start to cache the sg_table for an attachment >>> unless the driver explicitly says to not do so. >>> >>> --- >>> drivers/dma-buf/dma-buf.c | 24 ++++++++++++++++++++++++ >>> include/linux/dma-buf.h | 11 +++++++++++ >>> 2 files changed, 35 insertions(+) >>> >>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >>> index 7c858020d14b..65161a82d4d5 100644 >>> --- a/drivers/dma-buf/dma-buf.c >>> +++ b/drivers/dma-buf/dma-buf.c >>> @@ -573,6 +573,20 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, >>> list_add(&attach->node, &dmabuf->attachments); >>> >>> mutex_unlock(&dmabuf->lock); >>> + >>> + if (!dmabuf->ops->dynamic_sgt_mapping) { >>> + struct sg_table *sgt; >>> + >>> + sgt = dmabuf->ops->map_dma_buf(attach, DMA_BIDIRECTIONAL); >>> + if (!sgt) >>> + sgt = ERR_PTR(-ENOMEM); >>> + if (IS_ERR(sgt)) { >>> + dma_buf_detach(dmabuf, attach); >>> + return ERR_CAST(sgt); >>> + } >>> + attach->sgt = sgt; >>> + } >>> + >>> return attach; >>> >>> err_attach: >>> @@ -595,6 +609,10 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) >>> if (WARN_ON(!dmabuf || !attach)) >>> return; >>> >>> + if (attach->sgt) >>> + dmabuf->ops->unmap_dma_buf(attach, attach->sgt, >>> + DMA_BIDIRECTIONAL); >>> + >>> mutex_lock(&dmabuf->lock); >>> list_del(&attach->node); >>> if (dmabuf->ops->detach) >>> @@ -630,6 +648,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, >>> if (WARN_ON(!attach || !attach->dmabuf)) >>> return ERR_PTR(-EINVAL); >>> >>> + if (attach->sgt) >>> + return attach->sgt; >>> + >> I am concerned by this change to make caching the sg_table the default >> behavior as this will result in the exporter's map_dma_buf/unmap_dma_buf >> calls are no longer being called in >> dma_buf_map_attachment/dma_buf_unmap_attachment. > Probably this concern from Liam got lost between versions of your > patches; could we please request a reply to these points here?
Sorry I indeed never got this mail, but this is actually not an issue because Daniel had similar concerns and we didn't made this the default in the final version.
>> This seems concerning to me as it appears to ignore the cache maintenance >> aspect of the map_dma_buf/unmap_dma_buf calls. >> For example won't this potentially cause issues for clients of ION. >> >> If we had the following >> - #1 dma_buf_attach coherent_device >> - #2 dma_buf attach non_coherent_device >> - #3 dma_buf_map_attachment non_coherent_device >> - #4 non_coherent_device writes to buffer >> - #5 dma_buf_unmap_attachment non_coherent_device >> - #6 dma_buf_map_attachment coherent_device >> - #7 coherent_device reads buffer >> - #8 dma_buf_unmap_attachment coherent_device >> >> There wouldn't be any CMO at step #5 anymore (specifically no invalidate) >> so now at step #7 the coherent_device could read a stale cache line. >> >> Also, now by default dma_buf_unmap_attachment no longer removes the >> mappings from the iommu, so now by default dma_buf_unmap_attachment is not >> doing what I would expect and clients are losing the potential sandboxing >> benefits of removing the mappings. >> Shouldn't this caching behavior be something that clients opt into instead >> of being the default?
Well, it seems you are making incorrect assumptions about the cache maintenance of DMA-buf here.
At least for all DRM devices I'm aware of mapping/unmapping an attachment does *NOT* have any cache maintenance implications.
E.g. the use case you describe above would certainly fail with amdgpu, radeon, nouveau and i915 because mapping a DMA-buf doesn't stop the exporter from reading/writing to that buffer (just the opposite actually).
All of them assume perfectly coherent access to the underlying memory. As far as I know there is no documented cache maintenance requirements for DMA-buf.
The IOMMU concern on the other hand is certainly valid and I perfectly agree that keeping the mapping time as short as possible is desirable.
Regards, Christian.
>> Liam >> >> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, >> a Linux Foundation Collaborative Project >> > Best, > Sumit.
| |