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