On Tue, Jun 25, 2019 at 4:45 PM Christian König <ckoenig.leichtzumerken@xxxxxxxxx> wrote: > > 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@xxxxxxx> > >> --- > >> 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. Well looking through the code I thought that all the places you grab dmabuf->resv we also grab dmabuf->lock, so seemed fully redundant. But I didn't check completely. > > > > > > >> + 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. Hm indeed the amd/i915 prime tests are still in BAT and that passed. I'm surprised. Maybe Chris Wilson moved along quite a lot with our locking rework and i915 isn't the bad one out anymore. > > 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. Yeah that should at least increase the chances we're seeing something. Ofc intel-gfx-ci doesn't test everything :-) -Daniel > > 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@xxxxxxxxxxxxxxxxxxxxx > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel