Re: [PATCH 04/12] dma-buf: add optional invalidate_mappings callback v5

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Apr 17, 2019 at 04:01:16PM +0200, Daniel Vetter wrote:
> On Tue, Apr 16, 2019 at 08:38:33PM +0200, Christian König wrote:
> > Each importer can now provide an invalidate_mappings callback.
> > 
> > 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
> > 
> > Signed-off-by: Christian König <christian.koenig@amd.com>
> > ---
> >  drivers/dma-buf/dma-buf.c | 37 +++++++++++++++++++++++++++++++++++++
> >  include/linux/dma-buf.h   | 33 +++++++++++++++++++++++++++++++--
> >  2 files changed, 68 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> > index 83c92bfd964c..a3738fab3927 100644
> > --- a/drivers/dma-buf/dma-buf.c
> > +++ b/drivers/dma-buf/dma-buf.c
> > @@ -563,6 +563,8 @@ struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info
> >  
> >  	attach->dev = info->dev;
> >  	attach->dmabuf = dmabuf;
> > +	attach->importer_priv = info->importer_priv;
> > +	attach->invalidate = info->invalidate;
> >  
> >  	mutex_lock(&dmabuf->lock);
> >  
> > @@ -571,7 +573,9 @@ struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info
> >  		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);
> >  
> > @@ -615,7 +619,9 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
> >  					   DMA_BIDIRECTIONAL);
> >  
> >  	mutex_lock(&dmabuf->lock);
> > +	reservation_object_lock(dmabuf->resv, NULL);
> >  	list_del(&attach->node);
> > +	reservation_object_unlock(dmabuf->resv);
> >  	if (dmabuf->ops->detach)
> >  		dmabuf->ops->detach(dmabuf, attach);
> >  
> > @@ -653,7 +659,16 @@ dma_buf_map_attachment_locked(struct dma_buf_attachment *attach,
> >  	if (attach->sgt)
> >  		return attach->sgt;
> >  
> > +	/*
> > +	 * Mapping a DMA-buf can trigger its invalidation, prevent sending this
> > +	 * event to the caller by temporary removing this attachment from the
> > +	 * list.
> > +	 */
> > +	if (attach->invalidate)
> > +		list_del(&attach->node);
> >  	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
> > +	if (attach->invalidate)
> > +		list_add(&attach->node, &attach->dmabuf->attachments);
> >  	if (!sg_table)
> >  		sg_table = ERR_PTR(-ENOMEM);
> >  
> > @@ -751,6 +766,26 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
> >  }
> >  EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);
> >  
> > +/**
> > + * dma_buf_invalidate_mappings - invalidate all mappings of this dma_buf
> > + *
> > + * @dmabuf:	[in]	buffer which mappings should be invalidated
> > + *
> > + * Informs all attachmenst that they need to destroy and recreated all their
> > + * mappings.
> > + */
> > +void dma_buf_invalidate_mappings(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->invalidate)
> > +			attach->invalidate(attach);
> > +}
> > +EXPORT_SYMBOL_GPL(dma_buf_invalidate_mappings);
> > +
> >  /**
> >   * DOC: cpu access
> >   *
> > @@ -1163,10 +1198,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);
> >  
> >  		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 7e23758db3a4..ece4638359a8 100644
> > --- a/include/linux/dma-buf.h
> > +++ b/include/linux/dma-buf.h
> > @@ -324,6 +324,7 @@ struct dma_buf {
> >   * @dev: device attached to the buffer.
> >   * @node: list of dma_buf_attachment.
> >   * @priv: exporter specific attachment data.
> > + * @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
> > @@ -340,6 +341,29 @@ struct dma_buf_attachment {
> >  	struct list_head node;
> >  	void *priv;
> >  	struct sg_table *sgt;
> > +	void *importer_priv;
> > +
> > +	/**
> > +	 * @invalidate:
> > +	 *
> > +	 * Optional callback provided by the importer of the dma-buf.
> > +	 *
> > +	 * If provided the exporter can avoid pinning the backing store while
> > +	 * mappings exists.
> > +	 *
> > +	 * The function 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 invalidation.
> > +	 *
> > +	 * After the callback all existing mappings are still valid until all
> > +	 * fences in the dma_bufs reservation object are signaled, but should be
> > +	 * destroyed by the importer as soon as possible.
> > +	 *
> > +	 * New mappings can be created immediately, but can't be used before the
> > +	 * exclusive fence in the dma_bufs reservation object is signaled.
> > +	 */
> > +	void (*invalidate)(struct dma_buf_attachment *attach);
> 
> I would put the long kerneldoc into dma_buf_attach_info (as an inline
> comment, you can mix the styles). And reference it from here with
> something like
> 
> "Set from &dma_buf_attach_info.invalidate in dma_buf_attach(), see there
> for more information."
> 
> This here feels a bit too much hidden.

Question on semantics: Is invalidate allowed to add new fences? I think we
need that for pipelined buffer moves and stuff perhaps (or pipeline
pagetable invalidates or whatever you feel like pipelining). And it should
be possible (we already hold the reservation lock), and I think ttm copes
(but no idea really).

Either way, docs need to be clear on this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch