On Tue, Feb 18, 2020 at 2:20 PM Christian König ckoenig.leichtzumerken@gmail.com wrote:
Am 05.11.19 um 11:20 schrieb Daniel Vetter:
On Tue, Oct 29, 2019 at 11:40:45AM +0100, Christian König wrote: [SNIP]
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index d377b4ca66bf..ce293cee76ed 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -529,6 +529,10 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) exp_info->ops->dynamic_mapping)) return ERR_PTR(-EINVAL);
- if (WARN_ON(!exp_info->ops->dynamic_mapping &&
(exp_info->ops->pin || exp_info->ops->unpin)))return ERR_PTR(-EINVAL);Imo make this stronger, have a dynamic mapping iff there's both a pin and unpin function. Otherwise this doesn't make a lot of sense to me.
I want to avoid that for the initial implementation. So far dynamic only meant that we have the new locking semantics.
We could make that mandatory after this patch set when amdgpu is migrated and has implemented the necessary callbacks.
Ok if we go with CONFIG_EXPERIMENTAL_DYN_DMABUF or whatever it's going to be called I'm totally ok if we just note this somewhere as a FIXME (maybe just inline in a code comment next to the main #ifdef in dma-buf.h. Same for all your other comments below.
Cheers, Daniel
[SNIP] @@ -821,13 +877,23 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, return attach->sgt; }
- if (dma_buf_is_dynamic(attach->dmabuf))
- if (dma_buf_is_dynamic(attach->dmabuf)) { dma_resv_assert_held(attach->dmabuf->resv);
if (!attach->importer_ops->move_notify) {Imo just require ->move_notify for importers that give you an ops function. Doesn't really make sense to allow dynamic without support ->move_notify.
Same thing here. We could make that mandatory and clean it up after migrating amdgpu.
[SNIP] diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index af73f835c51c..7456bb937635 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -93,14 +93,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.I think we should add a warning here that pinning is only ok for limited use-cases (like scanout or similar), and not as part of general buffer management.
i915 uses temporary pins through it's execbuf management (and everywhere else), so we have a _lot_ of people in dri-devel with quite different ideas of what this might be for :-)
Yeah, that is also a good idea for us. Wrote a one liner, but you might want to double check the wording.
[SNIP] @@ -141,9 +167,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.This is still valid for non-dynamic exporters. Imo keep but clarify that.
OK, changed.
[SNIP] @@ -438,16 +491,19 @@ static inline bool dma_buf_is_dynamic(struct dma_buf *dmabuf) static inline bool dma_buf_attachment_is_dynamic(struct dma_buf_attachment *attach) {
- return attach->dynamic_mapping;
- return !!attach->importer_ops;
Hm why not do the same for exporters, and make them dynamic iff they have pin/unpin?
Same thing as before, to migrate amdgpu to the new interface first and then make it mandatory.
I think I will just write a cleanup patch into the series which comes after the amdgpu changes.
Thanks, Christian.