[Intel-gfx] [PATCH 1/6] dma-buf: add dynamic DMA-buf handling v12

Christian König ckoenig.leichtzumerken at gmail.com
Tue Jun 25 14:45:42 UTC 2019


Am 25.06.19 um 16:35 schrieb Daniel Vetter:
> Hi Christian,
>
> On Tue, Jun 25, 2019 at 02:46:49PM +0200, Christian König wrote:
>> On the exporter side we add optional explicit pinning callbacks. If those
>> callbacks are implemented the framework no longer caches sg tables and the
>> map/unmap callbacks are always called with the lock of the reservation object
>> held.
>>
>> On the importer side we add an optional invalidate callback. This callback is
>> used by the exporter to inform the importers that their mappings should be
>> destroyed as soon as possible.
>>
>> This allows the exporter to provide the mappings without the need to pin
>> the backing store.
>>
>> v2: don't try to invalidate mappings when the callback is NULL,
>>      lock the reservation obj while using the attachments,
>>      add helper to set the callback
>> v3: move flag for invalidation support into the DMA-buf,
>>      use new attach_info structure to set the callback
>> v4: use importer_priv field instead of mangling exporter priv.
>> v5: drop invalidation_supported flag
>> v6: squash together with pin/unpin changes
>> v7: pin/unpin takes an attachment now
>> v8: nuke dma_buf_attachment_(map|unmap)_locked,
>>      everything is now handled backward compatible
>> v9: always cache when export/importer don't agree on dynamic handling
>> v10: minimal style cleanup
>> v11: drop automatically re-entry avoidance
>> v12: rename callback to move_notify
>>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
>> ---
>>   drivers/dma-buf/dma-buf.c | 179 ++++++++++++++++++++++++++++++++++++--
>>   include/linux/dma-buf.h   | 108 +++++++++++++++++++++--
>>   2 files changed, 273 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>> index 6c15deb5d4ad..216f76109f3f 100644
>> --- a/drivers/dma-buf/dma-buf.c
>> +++ b/drivers/dma-buf/dma-buf.c
>> @@ -531,6 +531,9 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info)
>>   		return ERR_PTR(-EINVAL);
>>   	}
>>   
>> +	if (WARN_ON(exp_info->ops->cache_sgt_mapping && exp_info->ops->pin))
>> +		return ERR_PTR(-EINVAL);
>> +
>>   	if (!try_module_get(exp_info->owner))
>>   		return ERR_PTR(-ENOENT);
>>   
>> @@ -651,10 +654,12 @@ void dma_buf_put(struct dma_buf *dmabuf)
>>   EXPORT_SYMBOL_GPL(dma_buf_put);
>>   
>>   /**
>> - * dma_buf_attach - Add the device to dma_buf's attachments list; optionally,
>> + * dma_buf_dynamic_attach - Add the device to dma_buf's attachments list; optionally,
>>    * calls attach() of dma_buf_ops to allow device-specific attach functionality
>> - * @dmabuf:	[in]	buffer to attach device to.
>> - * @dev:	[in]	device to be attached.
>> + * @dmabuf:		[in]	buffer to attach device to.
>> + * @dev:		[in]	device to be attached.
>> + * @importer_ops	[in]	importer operations for the attachment
>> + * @importer_priv	[in]	importer private pointer for the attachment
>>    *
>>    * Returns struct dma_buf_attachment pointer for this attachment. Attachments
>>    * must be cleaned up by calling dma_buf_detach().
>> @@ -668,8 +673,10 @@ 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(struct dma_buf *dmabuf,
>> -					  struct device *dev)
>> +struct dma_buf_attachment *
>> +dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev,
>> +		       const struct dma_buf_attach_ops *importer_ops,
>> +		       void *importer_priv)
>>   {
>>   	struct dma_buf_attachment *attach;
>>   	int ret;
>> @@ -683,6 +690,8 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
>>   
>>   	attach->dev = dev;
>>   	attach->dmabuf = dmabuf;
>> +	attach->importer_ops = importer_ops;
>> +	attach->importer_priv = importer_priv;
>>   
>>   	mutex_lock(&dmabuf->lock);
>>   
>> @@ -691,16 +700,72 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
>>   		if (ret)
>>   			goto err_attach;
>>   	}
>> +	reservation_object_lock(dmabuf->resv, NULL);
>>   	list_add(&attach->node, &dmabuf->attachments);
>> +	reservation_object_unlock(dmabuf->resv);
>>   
>>   	mutex_unlock(&dmabuf->lock);
>>   
>> +	/* When either the importer or the exporter can't handle dynamic
>> +	 * mappings we cache the mapping here to avoid issues with the
>> +	 * reservation object lock.
>> +	 */
>> +	if (dma_buf_attachment_is_dynamic(attach) !=
>> +	    dma_buf_is_dynamic(dmabuf)) {
>> +		struct sg_table *sgt;
>> +
>> +		if (dma_buf_is_dynamic(attach->dmabuf)) {
>> +			reservation_object_lock(attach->dmabuf->resv, NULL);
>> +			ret = dma_buf_pin(attach);
>> +			if (ret)
>> +				goto err_unlock;
>> +		}
>> +
>> +		sgt = dmabuf->ops->map_dma_buf(attach, DMA_BIDIRECTIONAL);
>> +		if (!sgt)
>> +			sgt = ERR_PTR(-ENOMEM);
>> +		if (IS_ERR(sgt)) {
>> +			ret = PTR_ERR(sgt);
>> +			goto err_unpin;
>> +		}
>> +		if (dma_buf_is_dynamic(attach->dmabuf))
>> +			reservation_object_unlock(attach->dmabuf->resv);
>> +		attach->sgt = sgt;
>> +		attach->dir = DMA_BIDIRECTIONAL;
>> +	}
>> +
>>   	return attach;
>>   
>>   err_attach:
>>   	kfree(attach);
>>   	mutex_unlock(&dmabuf->lock);
>>   	return ERR_PTR(ret);
>> +
>> +err_unpin:
>> +	if (dma_buf_is_dynamic(attach->dmabuf))
>> +		dma_buf_unpin(attach);
>> +
>> +err_unlock:
>> +	if (dma_buf_is_dynamic(attach->dmabuf))
>> +		reservation_object_unlock(attach->dmabuf->resv);
>> +
>> +	dma_buf_detach(dmabuf, attach);
>> +	return ERR_PTR(ret);
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_dynamic_attach);
>> +
>> +/**
>> + * dma_buf_attach - Wrapper for dma_buf_dynamic_attach
>> + * @dmabuf:	[in]	buffer to attach device to.
>> + * @dev:	[in]	device to be attached.
>> + *
>> + * Wrapper to call dma_buf_dynamic_attach() for drivers which still use a static
>> + * mapping.
>> + */
>> +struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
>> +					  struct device *dev)
>> +{
>> +	return dma_buf_dynamic_attach(dmabuf, dev, NULL, NULL);
>>   }
>>   EXPORT_SYMBOL_GPL(dma_buf_attach);
>>   
>> @@ -717,11 +782,22 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
>>   	if (WARN_ON(!dmabuf || !attach))
>>   		return;
>>   
>> -	if (attach->sgt)
>> +	if (attach->sgt) {
>> +		if (dma_buf_is_dynamic(attach->dmabuf))
>> +			reservation_object_lock(attach->dmabuf->resv, NULL);
>> +
>>   		dmabuf->ops->unmap_dma_buf(attach, attach->sgt, attach->dir);
>>   
>> +		if (dma_buf_is_dynamic(attach->dmabuf)) {
>> +			dma_buf_unpin(attach);
>> +			reservation_object_unlock(attach->dmabuf->resv);
>> +		}
>> +	}
>> +
>>   	mutex_lock(&dmabuf->lock);
> Time to ditch dmabuf->lock in favour of the reservation obj? We have a
> fallback resv_obj in struct dma_buf already, so this is never null, and I
> think would clean up the code a bit.

Yeah, thought about that as well. But then decided against it for now.

Key point is that exporters currently doesn't care about dmabuf->lock, 
but they do care about the reservation lock.

So we will probably have a bunch of cases where we have to fix up 
exporters because they will try to grab the reservation lock as well.

On the other hand we maybe not need a lock at all here if we just can 
live with multiple attach/detach callbacks running in parallel.

>
>
>> +	reservation_object_lock(dmabuf->resv, NULL);
>>   	list_del(&attach->node);
>> +	reservation_object_unlock(dmabuf->resv);
>>   	if (dmabuf->ops->detach)
>>   		dmabuf->ops->detach(dmabuf, attach);
>>   
>> @@ -730,6 +806,44 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
>>   }
>>   EXPORT_SYMBOL_GPL(dma_buf_detach);
>>   
>> +/**
>> + * dma_buf_pin - Lock down the DMA-buf
>> + *
>> + * @attach:	[in]	attachment which should be pinned
>> + *
>> + * Returns:
>> + * 0 on success, negative error code on failure.
>> + */
>> +int dma_buf_pin(struct dma_buf_attachment *attach)
>> +{
>> +	struct dma_buf *dmabuf = attach->dmabuf;
>> +	int ret = 0;
>> +
>> +	reservation_object_assert_held(dmabuf->resv);
>> +
>> +	if (dmabuf->ops->pin)
>> +		ret = dmabuf->ops->pin(attach);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_pin);
>> +
>> +/**
>> + * dma_buf_unpin - Remove lock from DMA-buf
>> + *
>> + * @attach:	[in]	attachment which should be unpinned
>> + */
>> +void dma_buf_unpin(struct dma_buf_attachment *attach)
>> +{
>> +	struct dma_buf *dmabuf = attach->dmabuf;
>> +
>> +	reservation_object_assert_held(dmabuf->resv);
>> +
>> +	if (dmabuf->ops->unpin)
>> +		dmabuf->ops->unpin(attach);
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_unpin);
>> +
>>   /**
>>    * dma_buf_map_attachment - Returns the scatterlist table of the attachment;
>>    * mapped into _device_ address space. Is a wrapper for map_dma_buf() of the
>> @@ -749,6 +863,7 @@ 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();
>>   
>> @@ -767,10 +882,29 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>>   		return attach->sgt;
>>   	}
>>   
>> +	if (dma_buf_attachment_is_dynamic(attach)) {
>> +		reservation_object_assert_held(attach->dmabuf->resv);
>> +
>> +	} else if (dma_buf_is_dynamic(attach->dmabuf)) {
>> +		reservation_object_lock(attach->dmabuf->resv, NULL);
>> +		r = dma_buf_pin(attach);
>> +		if (r) {
>> +			reservation_object_unlock(attach->dmabuf->resv);
>> +			return ERR_PTR(r);
>> +		}
>> +	}
> With this design (because we can't cache at attach time unconditionally)
> we inflict the reservation lock on all importers. So needs:
>
> 	} else {
> 		/* neither importer nor exporter are dynamice */
> 		might_lock(dmabuf->resv->ww_mutex);
> 	}
>
> Same for dma_buf_unmap_attachment.

Good idea.

>
> I also expect that this will blow up in intel-gfx-ci :-/ I did look at
> previous intel-gfx-ci runs of your series, and the last one that did pass
> was the one that still had the caching in _attach():

Well as far as I can see the current one passes as well. Actually just 
recently found a typo because of this.

>
> Geez I was sooooooooo locking forward to slapping an r-b on this and
> volunteering myself to polish the kerneldoc (which isn't great, but also
> pre-existing condition plus on my todo list anyway) :-(

Going to add the might_lock and send again, let's see what intel-gfx-ci 
has say to this.

Christian.

>
>> +
>>   	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
>>   	if (!sg_table)
>>   		sg_table = ERR_PTR(-ENOMEM);
>>   
>> +	if (!dma_buf_attachment_is_dynamic(attach) &&
>> +	    dma_buf_is_dynamic(attach->dmabuf)) {
>> +		if (IS_ERR(sg_table))
>> +			dma_buf_unpin(attach);
>> +		reservation_object_unlock(attach->dmabuf->resv);
>> +	}
>> +
>>   	if (!IS_ERR(sg_table) && attach->dmabuf->ops->cache_sgt_mapping) {
>>   		attach->sgt = sg_table;
>>   		attach->dir = direction;
>> @@ -802,10 +936,41 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
>>   	if (attach->sgt == sg_table)
>>   		return;
>>   
>> +	if (dma_buf_attachment_is_dynamic(attach))
>> +		reservation_object_assert_held(attach->dmabuf->resv);
>> +	else if (dma_buf_is_dynamic(attach->dmabuf))
>> +		reservation_object_lock(attach->dmabuf->resv, NULL);
>> +
>>   	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction);
>> +
>> +	if (dma_buf_is_dynamic(attach->dmabuf) &&
>> +	    !dma_buf_attachment_is_dynamic(attach)) {
>> +		dma_buf_unpin(attach);
>> +		reservation_object_unlock(attach->dmabuf->resv);
>> +	}
>>   }
>>   EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);
>>   
>> +/**
>> + * dma_buf_move_notify - notify attachments that DMA-buf is moving
>> + *
>> + * @dmabuf:	[in]	buffer which is moving
>> + *
>> + * Informs all attachmenst that they need to destroy and recreated all their
>> + * mappings.
>> + */
>> +void dma_buf_move_notify(struct dma_buf *dmabuf)
>> +{
>> +	struct dma_buf_attachment *attach;
>> +
>> +	reservation_object_assert_held(dmabuf->resv);
>> +
>> +	list_for_each_entry(attach, &dmabuf->attachments, node)
>> +		if (attach->importer_ops && attach->importer_ops->move_notify)
>> +			attach->importer_ops->move_notify(attach);
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_move_notify);
>> +
>>   /**
>>    * DOC: cpu access
>>    *
>> @@ -1225,10 +1390,12 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused)
>>   		seq_puts(s, "\tAttached Devices:\n");
>>   		attach_count = 0;
>>   
>> +		reservation_object_lock(buf_obj->resv, NULL);
>>   		list_for_each_entry(attach_obj, &buf_obj->attachments, node) {
>>   			seq_printf(s, "\t%s\n", dev_name(attach_obj->dev));
>>   			attach_count++;
>>   		}
>> +		reservation_object_unlock(buf_obj->resv);
> Yeah definitely time to retire dmabuf->lock I think.
>
> Cheers, Daniel
>
>>   
>>   		seq_printf(s, "Total %d devices attached\n\n",
>>   				attach_count);
>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
>> index 01ad5b942a6f..ccad2fc1f640 100644
>> --- a/include/linux/dma-buf.h
>> +++ b/include/linux/dma-buf.h
>> @@ -92,14 +92,40 @@ struct dma_buf_ops {
>>   	 */
>>   	void (*detach)(struct dma_buf *, struct dma_buf_attachment *);
>>   
>> +	/**
>> +	 * @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_attachment *attach);
>> +
>> +	/**
>> +	 * @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.
>> +	 */
>> +	void (*unpin)(struct dma_buf_attachment *attach);
>> +
>>   	/**
>>   	 * @map_dma_buf:
>>   	 *
>>   	 * 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.
>>   	 *
>>   	 * 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
>> @@ -120,6 +146,9 @@ struct dma_buf_ops {
>>   	 * any other kind of sharing that the exporter might wish to make
>>   	 * available to buffer-users.
>>   	 *
>> +	 * This is always called with the dmabuf->resv object locked when
>> +	 * the pin/unpin callbacks are implemented.
>> +	 *
>>   	 * Returns:
>>   	 *
>>   	 * A &sg_table scatter list of or the backing storage of the DMA buffer,
>> @@ -137,9 +166,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.
>> -	 * 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 *,
>> @@ -330,6 +356,34 @@ struct dma_buf {
>>   	} cb_excl, cb_shared;
>>   };
>>   
>> +/**
>> + * struct dma_buf_attach_ops - importer operations for an attachment
>> + * @move_notify: [optional] notification that the DMA-buf is moving
>> + *
>> + * Attachment operations implemented by the importer.
>> + */
>> +struct dma_buf_attach_ops {
>> +	/**
>> +	 * @move_notify
>> +	 *
>> +	 * If this callback is provided the framework can avoid pinning the
>> +	 * backing store while mappings exists.
>> +	 *
>> +	 * This callback is called with the lock of the reservation object
>> +	 * associated with the dma_buf held and the mapping function must be
>> +	 * called with this lock held as well. This makes sure that no mapping
>> +	 * is created concurrently with an ongoing move operation.
>> +	 *
>> +	 * Mappings stay valid and are not directly affected by this callback.
>> +	 * But the DMA-buf can now be in a different physical location, so all
>> +	 * mappings should be destroyed and re-created as soon as possible.
>> +	 *
>> +	 * New mappings can be created after this callback returns, and will
>> +	 * point to the new location of the DMA-buf.
>> +	 */
>> +	void (*move_notify)(struct dma_buf_attachment *attach);
>> +};
>> +
>>   /**
>>    * struct dma_buf_attachment - holds device-buffer attachment data
>>    * @dmabuf: buffer for this attachment.
>> @@ -338,6 +392,8 @@ struct dma_buf {
>>    * @sgt: cached mapping.
>>    * @dir: direction of cached mapping.
>>    * @priv: exporter specific attachment data.
>> + * @importer_ops: importer operations for this attachment.
>> + * @importer_priv: importer specific attachment data.
>>    *
>>    * This structure holds the attachment information between the dma_buf buffer
>>    * and its user device(s). The list contains one attachment struct per device
>> @@ -355,6 +411,9 @@ struct dma_buf_attachment {
>>   	struct sg_table *sgt;
>>   	enum dma_data_direction dir;
>>   	void *priv;
>> +
>> +	const struct dma_buf_attach_ops *importer_ops;
>> +	void *importer_priv;
>>   };
>>   
>>   /**
>> @@ -405,10 +464,42 @@ static inline void get_dma_buf(struct dma_buf *dmabuf)
>>   	get_file(dmabuf->file);
>>   }
>>   
>> +/**
>> + * dma_buf_is_dynamic - check if a DMA-buf uses dynamic mappings.
>> + * @dmabuf: the DMA-buf to check
>> + *
>> + * Returns true if a DMA-buf exporter wants to create dynamic sg table mappings
>> + * for each attachment. False if only a single static sg table should be used.
>> + */
>> +static inline bool dma_buf_is_dynamic(struct dma_buf *dmabuf)
>> +{
>> +	return !!dmabuf->ops->pin;
>> +}
>> +
>> +/**
>> + * dma_buf_attachment_is_dynamic - check if a DMA-buf attachment uses dynamic
>> + * mappinsg
>> + * @attach: the DMA-buf attachment to check
>> + *
>> + * Returns true if a DMA-buf importer wants to use dynamic sg table mappings and
>> + * calls the map/unmap functions with the reservation object locked.
>> + */
>> +static inline bool
>> +dma_buf_attachment_is_dynamic(struct dma_buf_attachment *attach)
>> +{
>> +	return attach->importer_ops && attach->importer_ops->move_notify;
>> +}
>> +
>>   struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
>> -							struct device *dev);
>> +					  struct device *dev);
>> +struct dma_buf_attachment *
>> +dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev,
>> +		       const struct dma_buf_attach_ops *importer_ops,
>> +		       void *importer_priv);
>>   void dma_buf_detach(struct dma_buf *dmabuf,
>> -				struct dma_buf_attachment *dmabuf_attach);
>> +		    struct dma_buf_attachment *attach);
>> +int dma_buf_pin(struct dma_buf_attachment *attach);
>> +void dma_buf_unpin(struct dma_buf_attachment *attach);
>>   
>>   struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info);
>>   
>> @@ -420,6 +511,7 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,
>>   					enum dma_data_direction);
>>   void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *,
>>   				enum dma_data_direction);
>> +void dma_buf_move_notify(struct dma_buf *dma_buf);
>>   int dma_buf_begin_cpu_access(struct dma_buf *dma_buf,
>>   			     enum dma_data_direction dir);
>>   int dma_buf_end_cpu_access(struct dma_buf *dma_buf,
>> -- 
>> 2.17.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



More information about the amd-gfx mailing list