[PATCH 02/12] dma-buf: add explicit buffer pinning v2
Christian König
ckoenig.leichtzumerken at gmail.com
Tue Apr 30 13:42:22 UTC 2019
Am 29.04.19 um 10:40 schrieb Daniel Vetter:
> On Fri, Apr 26, 2019 at 02:36:28PM +0200, Christian König wrote:
>> Add optional explicit pinning callbacks instead of implicitly assume the
>> exporter pins the buffer when a mapping is created.
>>
>> v2: move in patchset and pin the dma-buf in the old mapping code paths.
>>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
>> ---
>> drivers/dma-buf/dma-buf.c | 49 ++++++++++++++++++++++++++++++++++++++-
>> include/linux/dma-buf.h | 38 +++++++++++++++++++++++++-----
>> 2 files changed, 80 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>> index 50b4c6af04c7..0656dcf289be 100644
>> --- a/drivers/dma-buf/dma-buf.c
>> +++ b/drivers/dma-buf/dma-buf.c
>> @@ -529,6 +529,41 @@ void dma_buf_put(struct dma_buf *dmabuf)
>> }
>> EXPORT_SYMBOL_GPL(dma_buf_put);
>>
>> +/**
>> + * dma_buf_pin - Lock down the DMA-buf
>> + *
>> + * @dmabuf: [in] DMA-buf to lock down.
>> + *
>> + * Returns:
>> + * 0 on success, negative error code on failure.
>> + */
>> +int dma_buf_pin(struct dma_buf *dmabuf)
> I think this should be on the attachment, not on the buffer. Or is the
> idea that a pin is for the entire buffer, and all subsequent
> dma_buf_map_attachment must work for all attachments? I think this matters
> for sufficiently contrived p2p scenarios.
This is indeed for the DMA-buf, cause we are pinning the underlying
backing store and not just one attachment.
Key point is I want to call this in the exporter itself in the long run.
E.g. that the exporter stops working with its internal special handling
functions and uses this one instead.
> Either way, docs need to clarify this.
Going to add a bit more here.
>
>> +{
>> + int ret = 0;
>> +
>> + reservation_object_assert_held(dmabuf->resv);
>> +
>> + if (dmabuf->ops->pin)
>> + ret = dmabuf->ops->pin(dmabuf);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_pin);
>> +
>> +/**
>> + * dma_buf_unpin - Remove lock from DMA-buf
>> + *
>> + * @dmabuf: [in] DMA-buf to unlock.
>> + */
>> +void dma_buf_unpin(struct dma_buf *dmabuf)
>> +{
>> + reservation_object_assert_held(dmabuf->resv);
>> +
>> + if (dmabuf->ops->unpin)
>> + dmabuf->ops->unpin(dmabuf);
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_unpin);
>> +
>> /**
>> * dma_buf_attach - Add the device to dma_buf's attachments list; optionally,
>> * calls attach() of dma_buf_ops to allow device-specific attach functionality
>> @@ -548,7 +583,8 @@ EXPORT_SYMBOL_GPL(dma_buf_put);
>> * accessible to @dev, and cannot be moved to a more suitable place. This is
>> * indicated with the error code -EBUSY.
>> */
>> -struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info)
>> +struct dma_buf_attachment *
>> +dma_buf_attach(const struct dma_buf_attach_info *info)
>> {
>> struct dma_buf *dmabuf = info->dmabuf;
>> struct dma_buf_attachment *attach;
>> @@ -625,12 +661,19 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>> enum dma_data_direction direction)
>> {
>> struct sg_table *sg_table;
>> + int r;
>>
>> might_sleep();
>>
>> if (WARN_ON(!attach || !attach->dmabuf))
>> return ERR_PTR(-EINVAL);
>>
>> + reservation_object_lock(attach->dmabuf->resv, NULL);
>> + r = dma_buf_pin(attach->dmabuf);
>> + reservation_object_unlock(attach->dmabuf->resv);
> This adds an unconditional reservat lock to map/unmap, which is think
> pisses off drivers. This gets fixed later on with the caching, but means
> the series is broken here.
>
> Also, that super-fine grained split-up makes it harder for me to review
> the docs, since only until the very end are all the bits present for full
> dynamic dma-buf mappings.
>
> I think it'd be best to squash all the patches from pin up to the one that
> adds the invalidate callback into one patch. It's all changes to
> dma-buf.[hc] only anyway. If that is too big we can think about how to
> split it up again, but at least for me the current splitting doesn't make
> sense at all.
Fine with me, if you think that is easier to review. It just gave me a
big headache to add all that logic at the same time.
>
>> + if (r)
>> + return ERR_PTR(r);
>> +
>> sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
>> if (!sg_table)
>> sg_table = ERR_PTR(-ENOMEM);
> Leaks the pin if we fail here.
Going to fix that.
>
>> @@ -660,6 +703,10 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
>>
>> attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
>> direction);
>> +
>> + reservation_object_lock(attach->dmabuf->resv, NULL);
>> + dma_buf_unpin(attach->dmabuf);
>> + reservation_object_unlock(attach->dmabuf->resv);
>> }
>> EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);
>>
>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
>> index 2c312dfd31a1..0321939b1c3d 100644
>> --- a/include/linux/dma-buf.h
>> +++ b/include/linux/dma-buf.h
>> @@ -51,6 +51,34 @@ struct dma_buf_attachment;
>> * @vunmap: [optional] unmaps a vmap from the buffer
>> */
>> struct dma_buf_ops {
>> + /**
>> + * @pin:
>> + *
>> + * This is called by dma_buf_pin and lets the exporter know that the
>> + * DMA-buf can't be moved any more.
>> + *
>> + * This is called with the dmabuf->resv object locked.
>> + *
>> + * This callback is optional.
>> + *
>> + * Returns:
>> + *
>> + * 0 on success, negative error code on failure.
>> + */
>> + int (*pin)(struct dma_buf *);
>> +
>> + /**
>> + * @unpin:
>> + *
>> + * This is called by dma_buf_unpin and lets the exporter know that the
>> + * DMA-buf can be moved again.
>> + *
>> + * This is called with the dmabuf->resv object locked.
>> + *
>> + * This callback is optional.
> "This callback is optional, but must be provided if @pin is." Same for
> @pin I think, plus would be good to check in dma_buf_export that you have
> both or neither with
>
> WARN_ON(!!exp_info->ops->pin == !!exp_info->ops->unpin);
>
>> + */
>> + void (*unpin)(struct dma_buf *);
>> +
>> /**
>> * @attach:
>> *
>> @@ -95,9 +123,7 @@ struct dma_buf_ops {
>> *
>> * This is called by dma_buf_map_attachment() and is used to map a
>> * shared &dma_buf into device address space, and it is mandatory. It
>> - * can only be called if @attach has been called successfully. This
>> - * essentially pins the DMA buffer into place, and it cannot be moved
>> - * any more
>> + * can only be called if @attach has been called successfully.
> I think dropping this outright isn't correct, since for all current
> dma-buf exporters it's still what they should be doing. We just need to
> make this conditional on @pin and @unpin not being present:
>
> "If @pin and @unpin are not provided this essentially pins the DMA
> buffer into place, and it cannot be moved any more."
>
>> *
>> * This call may sleep, e.g. when the backing storage first needs to be
>> * allocated, or moved to a location suitable for all currently attached
>> @@ -135,9 +161,6 @@ struct dma_buf_ops {
>> *
>> * This is called by dma_buf_unmap_attachment() and should unmap and
>> * release the &sg_table allocated in @map_dma_buf, and it is mandatory.
> Same here, add a "If @pin and @unpin are not provided this should ..."
> qualifier instead of deleting.
>
> Cheers, Daniel
>
>
>> - * It should also unpin the backing storage if this is the last mapping
>> - * of the DMA buffer, it the exporter supports backing storage
>> - * migration.
>> */
>> void (*unmap_dma_buf)(struct dma_buf_attachment *,
>> struct sg_table *,
>> @@ -386,6 +409,9 @@ static inline void get_dma_buf(struct dma_buf *dmabuf)
>> get_file(dmabuf->file);
>> }
>>
>> +int dma_buf_pin(struct dma_buf *dmabuf);
>> +void dma_buf_unpin(struct dma_buf *dmabuf);
>> +
>> struct dma_buf_attachment *
>> dma_buf_attach(const struct dma_buf_attach_info *info);
>> void dma_buf_detach(struct dma_buf *dmabuf,
>> --
>> 2.17.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the dri-devel
mailing list