Re: [PATCH v4 3/7] dma-buf: heaps: restricted_heap: Add private heap ops

On Fri, Jan 12, 2024 at 3:27 PM Jeffrey Kardatzke [off-list ref] wrote:

On Fri, Jan 12, 2024 at 2:52 PM John Stultz [off-list ref] wrote:

quoted

On Fri, Jan 12, 2024 at 1:21 AM Yong Wu [off-list ref] wrote:

quoted

diff --git a/drivers/dma-buf/heaps/restricted_heap.h b/drivers/dma-buf/heaps/restricted_heap.h
index 443028f6ba3b..ddeaf9805708 100644
--- a/drivers/dma-buf/heaps/restricted_heap.h
+++ b/drivers/dma-buf/heaps/restricted_heap.h

@@ -15,6 +15,18 @@ struct restricted_buffer {

 struct restricted_heap {
        const char              *name;
+
+       const struct restricted_heap_ops *ops;
+};
+
+struct restricted_heap_ops {
+       int     (*heap_init)(struct restricted_heap *heap);
+
+       int     (*memory_alloc)(struct restricted_heap *heap, struct restricted_buffer *buf);
+       void    (*memory_free)(struct restricted_heap *heap, struct restricted_buffer *buf);
+
+       int     (*memory_restrict)(struct restricted_heap *heap, struct restricted_buffer *buf);
+       void    (*memory_unrestrict)(struct restricted_heap *heap, struct restricted_buffer *buf);
 };

 int restricted_heap_add(struct restricted_heap *rstrd_heap);

So, I'm a little worried here, because you're basically turning the
restricted_heap dma-buf heap driver into a framework itself.
Where this patch is creating a subdriver framework.

Part of my hesitancy, is you're introducing this under the dma-buf
heaps. For things like CMA, that's more of a core subsystem that has
multiple users, and exporting cma buffers via dmabuf heaps is just an
additional interface.  What I like about that is the core kernel has
to define the semantics for the memory type and then the dmabuf heap
is just exporting that well understood type of buffer.

But with these restricted buffers, I'm not sure there's yet a well
understood set of semantics nor a central abstraction for that which
other drivers use directly.

I know part of this effort here is to start to centralize all these
vendor specific restricted buffer implementations, which I think is
great, but I just worry that in doing it in the dmabuf heap interface,
it is a bit too user-facing. The likelihood of encoding a particular
semantic to the singular "restricted_heap" name seems high.

In patch #5 it has the actual driver implementation for the MTK heap
that includes the heap name of "restricted_mtk_cm", so there shouldn't
be a heap named "restricted_heap" that's actually getting exposed. The

Ah, I appreciate that clarification! Indeed, you're right the name is
passed through. Apologies for missing that detail.

restricted_heap code is a framework, and not a driver itself.  Unless
I'm missing something in this patchset...but that's the way it's
*supposed* to be.

So I guess I'm not sure I understand the benefit of the extra
indirection. What then does the restricted_heap.c logic itself
provide?
The dmabuf heaps framework already provides a way to add heap implementations.

thanks
-john

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel