Re: [PATCH 02/12] dma-buf: add explicit buffer pinning v2

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

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@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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel