Re: [PATCH v3 2/2] misc: sram: Add DMA-BUF Heap exporting of SRAM areas

[Andrew Davis]

On 4/9/24 7:06 AM, Pascal FONTAIN wrote:

From: Andrew Davis 

This new export type exposes to userspace the SRAM area as a DMA-BUF
Heap,
this allows for allocations of DMA-BUFs that can be consumed by various
DMA-BUF supporting devices.

Signed-off-by: Andrew Davis 
Tested-by: Pascal Fontain 
---


The last time I posted this I got this comment[0], for which I
didn't really have a good response and so haven't reposted this
since then.

Bacically this driver is backed by something other than "normal"
memory. So we really need to be careful here, the page pointer is
really just a cookie to keep track for the real mappings. We might
be able to use something similar to dma_get_sgtable() to hide that,
but under the hood it would still a bit unsafe[1].

Andrew

[0] 
https://patchwork.kernel.org/project/linux-media/patch/20230713191316.116019-1-...@ti.com/#25475464
[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/dma/mapping.c#n388


  drivers/misc/Kconfig |   7 +
  drivers/misc/Makefile|   1 +
  drivers/misc/sram-dma-heap.c | 246 +++
  drivers/misc/sram.c  |   6 +
  drivers/misc/sram.h  |  16 +++
  5 files changed, 276 insertions(+)
  create mode 100644 drivers/misc/sram-dma-heap.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 9e4ad4d61f06..e6674e913168 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -448,6 +448,13 @@ config SRAM
  config SRAM_EXEC
bool
  
+config SRAM_DMA_HEAP

+   bool "Export on-chip SRAM pools using DMA-Heaps"
+   depends on DMABUF_HEAPS && SRAM
+   help
+ This driver allows the export of on-chip SRAM marked as both pool
+ and exportable to userspace using the DMA-Heaps interface.
+
  config DW_XDATA_PCIE
depends on PCI
tristate "Synopsys DesignWare xData PCIe driver"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index cdc6405584e3..63effdde5163 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_VMWARE_VMCI) += vmw_vmci/
  obj-$(CONFIG_LATTICE_ECP3_CONFIG) += lattice-ecp3-config.o
  obj-$(CONFIG_SRAM)+= sram.o
  obj-$(CONFIG_SRAM_EXEC)   += sram-exec.o
+obj-$(CONFIG_SRAM_DMA_HEAP)+= sram-dma-heap.o
  obj-$(CONFIG_GENWQE)  += genwqe/
  obj-$(CONFIG_ECHO)+= echo/
  obj-$(CONFIG_CXL_BASE)+= cxl/
diff --git a/drivers/misc/sram-dma-heap.c b/drivers/misc/sram-dma-heap.c
new file mode 100644
index ..e5a0bafe8cb9
--- /dev/null
+++ b/drivers/misc/sram-dma-heap.c
@@ -0,0 +1,246 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SRAM DMA-Heap userspace exporter
+ *
+ * Copyright (C) 2019-2022 Texas Instruments Incorporated - https://www.ti.com/
+ * Andrew Davis 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "sram.h"
+
+struct sram_dma_heap {
+   struct dma_heap *heap;
+   struct gen_pool *pool;
+};
+
+struct sram_dma_heap_buffer {
+   struct gen_pool *pool;
+   struct list_head attachments;
+   struct mutex attachments_lock;
+   unsigned long len;
+   void *vaddr;
+   phys_addr_t paddr;
+};
+
+struct dma_heap_attachment {
+   struct device *dev;
+   struct sg_table *table;
+   struct list_head list;
+};
+
+static int dma_heap_attach(struct dma_buf *dmabuf,
+  struct dma_buf_attachment *attachment)
+{
+   struct sram_dma_heap_buffer *buffer = dmabuf->priv;
+   struct dma_heap_attachment *a;
+   struct sg_table *table;
+
+   a = kzalloc(sizeof(*a), GFP_KERNEL);
+   if (!a)
+   return -ENOMEM;
+
+   table = kmalloc(sizeof(*table), GFP_KERNEL);
+   if (!table) {
+   kfree(a);
+   return -ENOMEM;
+   }
+   if (sg_alloc_table(table, 1, GFP_KERNEL)) {
+   kfree(table);
+   kfree(a);
+   return -ENOMEM;
+   }
+   sg_set_page(table->sgl, pfn_to_page(PFN_DOWN(buffer->paddr)), 
buffer->len, 0);
+
+   a->table = table;
+   a->dev = attachment->dev;
+   INIT_LIST_HEAD(>list);
+
+   attachment->priv = a;
+
+   mutex_lock(>attachments_lock);
+   list_add(>list, >attachments);
+   mutex_unlock(>attachments_lock);
+
+   return 0;
+}
+
+static void dma_heap_detatch(struct dma_buf *dmabuf,
+struct dma_buf_attachment *attachment)
+{
+   struct sram_dma_heap_buffer *buffer = dmabuf->priv;
+   struct dma_heap_attachment *a = attachment->priv;
+
+   mutex_lock(>attachments_lock);
+   list_del(>list);
+   mutex_unlock(>attachments_lock);
+
+   sg_free_table(a->table);
+   kfree(a->table);
+   kfree(a);
+}
+
+static struct sg_table *dma_heap_map_dma_buf(

Re: [PATCH v9 3/6] iio: core: Add new DMABUF interface infrastructure

[Andrew Davis]

On 3/10/24 7:48 AM, Paul Cercueil wrote:

Add the necessary infrastructure to the IIO core to support a new
optional DMABUF based interface.

With this new interface, DMABUF objects (externally created) can be
attached to a IIO buffer, and subsequently used for data transfer.

A userspace application can then use this interface to share DMABUF
objects between several interfaces, allowing it to transfer data in a
zero-copy fashion, for instance between IIO and the USB stack.

The userspace application can also memory-map the DMABUF objects, and
access the sample data directly. The advantage of doing this vs. the
read() interface is that it avoids an extra copy of the data between the
kernel and userspace. This is particularly userful for high-speed
devices which produce several megabytes or even gigabytes of data per
second.

As part of the interface, 3 new IOCTLs have been added:

IIO_BUFFER_DMABUF_ATTACH_IOCTL(int fd):
  Attach the DMABUF object identified by the given file descriptor to the
  buffer.

IIO_BUFFER_DMABUF_DETACH_IOCTL(int fd):
  Detach the DMABUF object identified by the given file descriptor from
  the buffer. Note that closing the IIO buffer's file descriptor will
  automatically detach all previously attached DMABUF objects.

IIO_BUFFER_DMABUF_ENQUEUE_IOCTL(struct iio_dmabuf *):
  Request a data transfer to/from the given DMABUF object. Its file
  descriptor, as well as the transfer size and flags are provided in the
  "iio_dmabuf" structure.

These three IOCTLs have to be performed on the IIO buffer's file
descriptor, obtained using the IIO_BUFFER_GET_FD_IOCTL() ioctl.

Signed-off-by: Paul Cercueil 
Signed-off-by: Nuno Sa 

---
v2: Only allow the new IOCTLs on the buffer FD created with
 IIO_BUFFER_GET_FD_IOCTL().

v3: - Get rid of the old IOCTLs. The IIO subsystem does not create or
 manage DMABUFs anymore, and only attaches/detaches externally
 created DMABUFs.
 - Add IIO_BUFFER_DMABUF_CYCLIC to the supported flags.

v5: - Use dev_err() instead of pr_err()
 - Inline to_iio_dma_fence()
 - Add comment to explain why we unref twice when detaching dmabuf
 - Remove TODO comment. It is actually safe to free the file's
   private data even when transfers are still pending because it
   won't be accessed.
 - Fix documentation of new fields in struct iio_buffer_access_funcs
 - iio_dma_resv_lock() does not need to be exported, make it static

v6: - Remove dead code in iio_dma_resv_lock()
 - Fix non-block actually blocking
 - Cache dma_buf_attachment instead of mapping/unmapping it for every
   transfer
 - Return -EINVAL instead of IIO_IOCTL_UNHANDLED for unknown ioctl
 - Make .block_enqueue() callback take a dma_fence pointer, which
   will be passed to iio_buffer_signal_dmabuf_done() instead of the
   dma_buf_attachment; and remove the backpointer from the priv
   structure to the dma_fence.
 - Use dma_fence_begin/end_signalling in the dma_fence critical
   sections
 - Unref dma_fence and dma_buf_attachment in worker, because they
   might try to lock the dma_resv, which would deadlock.
 - Add buffer ops to lock/unlock the queue. This is motivated by the
   fact that once the dma_fence has been installed, we cannot lock
   anything anymore - so the queue must be locked before the
   dma_fence is installed.
 - Use 'long retl' variable to handle the return value of
   dma_resv_wait_timeout()
 - Protect dmabufs list access with a mutex
 - Rework iio_buffer_find_attachment() to use the internal dmabufs
   list, instead of messing with dmabufs private data.
 - Add an atomically-increasing sequence number for fences

v8  - Fix swapped fence direction
 - Simplify fence wait mechanism
 - Remove "Buffer closed with active transfers" print, as it was dead
   code
 - Un-export iio_buffer_dmabuf_{get,put}. They are not used anywhere
   else so they can even be static.
 - Prevent attaching already-attached DMABUFs

v9: - Select DMA_SHARED_BUFFER in Kconfig
 - Remove useless forward declaration of 'iio_dma_fence'
 - Import DMA-BUF namespace
 - Add missing __user tag to iio_buffer_detach_dmabuf() argument
---
  drivers/iio/Kconfig   |   1 +
  drivers/iio/industrialio-buffer.c | 462 ++
  include/linux/iio/buffer_impl.h   |  30 ++
  include/uapi/linux/iio/buffer.h   |  22 ++
  4 files changed, 515 insertions(+)

diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index 9c351ffc7bed..661127aed2f9 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -14,6 +14,7 @@ if IIO
  
  config IIO_BUFFER

bool "Enable buffer support within IIO"
+   select DMA_SHARED_BUFFER
help
  Provide core support for various buffer based data
  acquisition methods.
diff --git a/drivers/iio/industrialio-buffer.c 
b/drivers/iio/industrialio-buffer.c
index b581a7e80566..a987654f82fc 

Re: [PATCH 2/3] udmabuf: Sync buffer mappings for attached devices

[Andrew Davis]

On 1/26/24 1:25 AM, Kasireddy, Vivek wrote:

Currently this driver creates a SGT table using the CPU as the
target device, then performs the dma_sync operations against
that SGT. This is backwards to how DMA-BUFs are supposed to behave.
This may have worked for the case where these buffers were given
only back to the same CPU that produced them as in the QEMU case.
And only then because the original author had the dma_sync
operations also backwards, syncing for the "device" on begin_cpu.
This was noticed and "fixed" in this patch[0].

That then meant we were sync'ing from the CPU to the CPU using
a pseudo-device "miscdevice". Which then caused another issue
due to the miscdevice not having a proper DMA mask (and why should
it, the CPU is not a DMA device). The fix for that was an even
more egregious hack[1] that declares the CPU is coherent with
itself and can access its own memory space..

Unwind all this and perform the correct action by doing the dma_sync
operations for each device currently attached to the backing buffer.

Makes sense.



[0] commit 1ffe09590121 ("udmabuf: fix dma-buf cpu access")
[1] commit 9e9fa6a9198b ("udmabuf: Set the DMA mask for the udmabuf
device (v2)")

Signed-off-by: Andrew Davis 
---
   drivers/dma-buf/udmabuf.c | 41 +++
   1 file changed, 16 insertions(+), 25 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 3a23f0a7d112a..ab6764322523c 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -26,8 +26,6 @@ MODULE_PARM_DESC(size_limit_mb, "Max size of a
dmabuf, in megabytes. Default is
   struct udmabuf {
pgoff_t pagecount;
struct page **pages;
-   struct sg_table *sg;
-   struct miscdevice *device;
struct list_head attachments;
struct mutex lock;
   };
@@ -169,12 +167,8 @@ static void unmap_udmabuf(struct
dma_buf_attachment *at,
   static void release_udmabuf(struct dma_buf *buf)
   {
struct udmabuf *ubuf = buf->priv;
-   struct device *dev = ubuf->device->this_device;
pgoff_t pg;

-   if (ubuf->sg)
-   put_sg_table(dev, ubuf->sg, DMA_BIDIRECTIONAL);

What happens if the last importer maps the dmabuf but erroneously
closes it immediately? Would unmap somehow get called in this case?



Good question, had to scan the framework code a bit here. I thought
closing a DMABUF handle would automatically unwind any current
attachments/mappings, but it seems nothing in the framework does that.

Looks like that is up to the importing drivers[0]:


Once a driver is done with a shared buffer it needs to call
dma_buf_detach() (after cleaning up any mappings) and then
release the reference acquired with dma_buf_get() by
calling dma_buf_put().


So closing a DMABUF after mapping without first unmapping it would
be a bug in the importer, it is not the exporters problem to check

It may be a bug in the importer but wouldn't the memory associated
with the sg table and attachment get leaked if unmap doesn't get called
in this scenario?



Yes the attachment data would be leaked if unattach was not called,
but that is true for all DMABUF exporters. The .release() callback
is meant to be the mirror of the export function and it only cleans
up that. Same for attach/unattach, map/unmap, etc.. If these calls
are not balanced then yes they can leak memory.

Since balance is guaranteed by the API, checking the balance should
be done at that level, not in each and every exporter. If your
comment is that we should add those checks into the DMABUF framework
layer then I would agree.

Andrew


Thanks,
Vivek


for (although some more warnings in the framework checking for that
might not be a bad idea..).

Andrew

[0] https://www.kernel.org/doc/html/v6.7/driver-api/dma-buf.html


Thanks,
Vivek


-
for (pg = 0; pg < ubuf->pagecount; pg++)
put_page(ubuf->pages[pg]);
kfree(ubuf->pages);
@@ -185,33 +179,31 @@ static int begin_cpu_udmabuf(struct dma_buf
*buf,
 enum dma_data_direction direction)
   {
struct udmabuf *ubuf = buf->priv;
-   struct device *dev = ubuf->device->this_device;
-   int ret = 0;
-
-   if (!ubuf->sg) {
-   ubuf->sg = get_sg_table(dev, buf, direction);
-   if (IS_ERR(ubuf->sg)) {
-   ret = PTR_ERR(ubuf->sg);
-   ubuf->sg = NULL;
-   }
-   } else {
-   dma_sync_sg_for_cpu(dev, ubuf->sg->sgl, ubuf->sg->nents,
-   direction);
-   }
+   struct udmabuf_attachment *a;

-   return ret;
+   mutex_lock(>lock);
+
+   list_for_each_entry(a, >attachments, list)
+   dma_sync_sgtable_for_cpu(a->dev, a->table, direction);
+
+   mutex_unlock(>lock);
+
+   return 0;
   }

Re: [Linaro-mm-sig] [PATCH 2/3] udmabuf: Sync buffer mappings for attached devices

[Andrew Davis]

On 1/25/24 2:30 PM, Daniel Vetter wrote:

On Tue, Jan 23, 2024 at 04:12:26PM -0600, Andrew Davis wrote:

Currently this driver creates a SGT table using the CPU as the
target device, then performs the dma_sync operations against
that SGT. This is backwards to how DMA-BUFs are supposed to behave.
This may have worked for the case where these buffers were given
only back to the same CPU that produced them as in the QEMU case.
And only then because the original author had the dma_sync
operations also backwards, syncing for the "device" on begin_cpu.
This was noticed and "fixed" in this patch[0].

That then meant we were sync'ing from the CPU to the CPU using
a pseudo-device "miscdevice". Which then caused another issue
due to the miscdevice not having a proper DMA mask (and why should
it, the CPU is not a DMA device). The fix for that was an even
more egregious hack[1] that declares the CPU is coherent with
itself and can access its own memory space..

Unwind all this and perform the correct action by doing the dma_sync
operations for each device currently attached to the backing buffer.

[0] commit 1ffe09590121 ("udmabuf: fix dma-buf cpu access")
[1] commit 9e9fa6a9198b ("udmabuf: Set the DMA mask for the udmabuf device 
(v2)")

Signed-off-by: Andrew Davis 


So yeah the above hacks are terrible, but I don't think this is better.
What you're doing now is that you're potentially doing the flushing
multiple times, so if you have a lot of importers with life mappings this
is a performance regression.


I'd take lower performing but correct than fast and broken. :)

Syncing for CPU/device is about making sure the CPU/device can see
the data produced by the other. Some devices might be dma-coherent
and syncing for them would be a NOP, but we cant know that here
in this driver. Let's say we have two attached devices, one that
is cache coherent and one that isn't. If we only sync for first
attached device then that is converted to a NOP and we never flush
like the second device needed.

Same is true for devices behind IOMMU or with an L3 cache when
syncing in the other direction for CPU. So we have to sync for all
attached devices to ensure we get even the lowest common denominator
device sync'd. It is up to the DMA-API layer to decide which syncs
need to actually do something. If all attached devices are coherent
then all syncs will be NOPs and we have no performance penalty.



It's probably time to bite the bullet and teach the dma-api about flushing
for multiple devices. Or some way we can figure out which is the one
device we need to pick which gives us the right amount of flushing.



Seems like a constraint solving micro-optimization. The DMA-API layer
would have to track which buffers have already been flushed from CPU
cache and also track that nothing has been written into those caches
since that point, only then could it skip the flush. But that is already
the point of the dirty bit in the caches themselves, cleaning already
clean cache lines is essentially free in hardware. And so is invalidating
lines, it is just flipping a bit.

Andrew


Cheers, Sima


---
  drivers/dma-buf/udmabuf.c | 41 +++
  1 file changed, 16 insertions(+), 25 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 3a23f0a7d112a..ab6764322523c 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -26,8 +26,6 @@ MODULE_PARM_DESC(size_limit_mb, "Max size of a dmabuf, in 
megabytes. Default is
  struct udmabuf {
pgoff_t pagecount;
struct page **pages;
-   struct sg_table *sg;
-   struct miscdevice *device;
struct list_head attachments;
struct mutex lock;
  };
@@ -169,12 +167,8 @@ static void unmap_udmabuf(struct dma_buf_attachment *at,
  static void release_udmabuf(struct dma_buf *buf)
  {
struct udmabuf *ubuf = buf->priv;
-   struct device *dev = ubuf->device->this_device;
pgoff_t pg;
  
-	if (ubuf->sg)

-   put_sg_table(dev, ubuf->sg, DMA_BIDIRECTIONAL);
-
for (pg = 0; pg < ubuf->pagecount; pg++)
put_page(ubuf->pages[pg]);
kfree(ubuf->pages);
@@ -185,33 +179,31 @@ static int begin_cpu_udmabuf(struct dma_buf *buf,
 enum dma_data_direction direction)
  {
struct udmabuf *ubuf = buf->priv;
-   struct device *dev = ubuf->device->this_device;
-   int ret = 0;
-
-   if (!ubuf->sg) {
-   ubuf->sg = get_sg_table(dev, buf, direction);
-   if (IS_ERR(ubuf->sg)) {
-   ret = PTR_ERR(ubuf->sg);
-   ubuf->sg = NULL;
-   }
-   } else {
-   dma_sync_sg_for_cpu(dev, ubuf->sg->sgl, ubuf->sg->nents,
-   direction);
-   }
+   struct u

Re: [PATCH 2/3] udmabuf: Sync buffer mappings for attached devices

[Andrew Davis]

On 1/24/24 5:05 PM, Kasireddy, Vivek wrote:

Hi Andrew,


Currently this driver creates a SGT table using the CPU as the
target device, then performs the dma_sync operations against
that SGT. This is backwards to how DMA-BUFs are supposed to behave.
This may have worked for the case where these buffers were given
only back to the same CPU that produced them as in the QEMU case.
And only then because the original author had the dma_sync
operations also backwards, syncing for the "device" on begin_cpu.
This was noticed and "fixed" in this patch[0].

That then meant we were sync'ing from the CPU to the CPU using
a pseudo-device "miscdevice". Which then caused another issue
due to the miscdevice not having a proper DMA mask (and why should
it, the CPU is not a DMA device). The fix for that was an even
more egregious hack[1] that declares the CPU is coherent with
itself and can access its own memory space..

Unwind all this and perform the correct action by doing the dma_sync
operations for each device currently attached to the backing buffer.

Makes sense.



[0] commit 1ffe09590121 ("udmabuf: fix dma-buf cpu access")
[1] commit 9e9fa6a9198b ("udmabuf: Set the DMA mask for the udmabuf
device (v2)")

Signed-off-by: Andrew Davis 
---
  drivers/dma-buf/udmabuf.c | 41 +++
  1 file changed, 16 insertions(+), 25 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 3a23f0a7d112a..ab6764322523c 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -26,8 +26,6 @@ MODULE_PARM_DESC(size_limit_mb, "Max size of a
dmabuf, in megabytes. Default is
  struct udmabuf {
pgoff_t pagecount;
struct page **pages;
-   struct sg_table *sg;
-   struct miscdevice *device;
struct list_head attachments;
struct mutex lock;
  };
@@ -169,12 +167,8 @@ static void unmap_udmabuf(struct
dma_buf_attachment *at,
  static void release_udmabuf(struct dma_buf *buf)
  {
struct udmabuf *ubuf = buf->priv;
-   struct device *dev = ubuf->device->this_device;
pgoff_t pg;

-   if (ubuf->sg)
-   put_sg_table(dev, ubuf->sg, DMA_BIDIRECTIONAL);

What happens if the last importer maps the dmabuf but erroneously
closes it immediately? Would unmap somehow get called in this case?



Good question, had to scan the framework code a bit here. I thought
closing a DMABUF handle would automatically unwind any current
attachments/mappings, but it seems nothing in the framework does that.

Looks like that is up to the importing drivers[0]:


Once a driver is done with a shared buffer it needs to call
dma_buf_detach() (after cleaning up any mappings) and then
release the reference acquired with dma_buf_get() by
calling dma_buf_put().


So closing a DMABUF after mapping without first unmapping it would
be a bug in the importer, it is not the exporters problem to check
for (although some more warnings in the framework checking for that
might not be a bad idea..).

Andrew

[0] https://www.kernel.org/doc/html/v6.7/driver-api/dma-buf.html


Thanks,
Vivek


-
for (pg = 0; pg < ubuf->pagecount; pg++)
put_page(ubuf->pages[pg]);
kfree(ubuf->pages);
@@ -185,33 +179,31 @@ static int begin_cpu_udmabuf(struct dma_buf
*buf,
 enum dma_data_direction direction)
  {
struct udmabuf *ubuf = buf->priv;
-   struct device *dev = ubuf->device->this_device;
-   int ret = 0;
-
-   if (!ubuf->sg) {
-   ubuf->sg = get_sg_table(dev, buf, direction);
-   if (IS_ERR(ubuf->sg)) {
-   ret = PTR_ERR(ubuf->sg);
-   ubuf->sg = NULL;
-   }
-   } else {
-   dma_sync_sg_for_cpu(dev, ubuf->sg->sgl, ubuf->sg->nents,
-   direction);
-   }
+   struct udmabuf_attachment *a;

-   return ret;
+   mutex_lock(>lock);
+
+   list_for_each_entry(a, >attachments, list)
+   dma_sync_sgtable_for_cpu(a->dev, a->table, direction);
+
+   mutex_unlock(>lock);
+
+   return 0;
  }

  static int end_cpu_udmabuf(struct dma_buf *buf,
   enum dma_data_direction direction)
  {
struct udmabuf *ubuf = buf->priv;
-   struct device *dev = ubuf->device->this_device;
+   struct udmabuf_attachment *a;

-   if (!ubuf->sg)
-   return -EINVAL;
+   mutex_lock(>lock);
+
+   list_for_each_entry(a, >attachments, list)
+   dma_sync_sgtable_for_device(a->dev, a->table, direction);
+
+   mutex_unlock(>lock);

-   dma_sync_sg_for_device(dev, ubuf->sg->sgl, ubuf->sg->nents,
direction);
return 0;
  }

@@ -307,7 +299,6 @@ static long udmabuf_create(struct miscdevice
*device,
exp_info

Re: [PATCH 1/3] udmabuf: Keep track current device mappings

[Andrew Davis]

On 1/24/24 4:36 PM, Kasireddy, Vivek wrote:

Hi Andrew,


When a device attaches to and maps our buffer we need to keep track
of this mapping/device. This is needed for synchronization with these
devices when beginning and ending CPU access for instance. Add a list
that tracks device mappings as part of {map,unmap}_udmabuf().

Signed-off-by: Andrew Davis 
---
  drivers/dma-buf/udmabuf.c | 43
+--
  1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index c406459996489..3a23f0a7d112a 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -28,6 +28,14 @@ struct udmabuf {
struct page **pages;
struct sg_table *sg;
struct miscdevice *device;
+   struct list_head attachments;
+   struct mutex lock;
+};
+
+struct udmabuf_attachment {
+   struct device *dev;
+   struct sg_table *table;
+   struct list_head list;
  };

  static vm_fault_t udmabuf_vm_fault(struct vm_fault *vmf)
@@ -120,14 +128,42 @@ static void put_sg_table(struct device *dev, struct
sg_table *sg,
  static struct sg_table *map_udmabuf(struct dma_buf_attachment *at,
enum dma_data_direction direction)
  {
-   return get_sg_table(at->dev, at->dmabuf, direction);
+   struct udmabuf *ubuf = at->dmabuf->priv;
+   struct udmabuf_attachment *a;
+
+   a = kzalloc(sizeof(*a), GFP_KERNEL);
+   if (!a)
+   return ERR_PTR(-ENOMEM);
+
+   a->table = get_sg_table(at->dev, at->dmabuf, direction);
+   if (IS_ERR(a->table)) {
+   kfree(a);
+   return a->table;

Isn't that a use-after-free bug?


Indeed it is, will fix.

Seems coccicheck also caught this but I missed it when
reviewing its output, my bad :(

Andrew


Rest of the patch lgtm.

Thanks,
Vivek


+   }
+
+   a->dev = at->dev;
+
+   mutex_lock(>lock);
+   list_add(>list, >attachments);
+   mutex_unlock(>lock);
+
+   return a->table;
  }

  static void unmap_udmabuf(struct dma_buf_attachment *at,
  struct sg_table *sg,
  enum dma_data_direction direction)
  {
-   return put_sg_table(at->dev, sg, direction);
+   struct udmabuf_attachment *a = at->priv;
+   struct udmabuf *ubuf = at->dmabuf->priv;
+
+   mutex_lock(>lock);
+   list_del(>list);
+   mutex_unlock(>lock);
+
+   put_sg_table(at->dev, sg, direction);
+
+   kfree(a);
  }

  static void release_udmabuf(struct dma_buf *buf)
@@ -263,6 +299,9 @@ static long udmabuf_create(struct miscdevice
*device,
memfd = NULL;
}

+   INIT_LIST_HEAD(>attachments);
+   mutex_init(>lock);
+
exp_info.ops  = _ops;
exp_info.size = ubuf->pagecount << PAGE_SHIFT;
exp_info.priv = ubuf;
--
2.39.2

Re: [Linaro-mm-sig] [PATCH v5 1/6] dma-buf: Add dma_buf_{begin,end}_access()

[Andrew Davis]

On 1/24/24 4:58 AM, Paul Cercueil wrote:

Hi Christian,

Le mardi 23 janvier 2024 à 14:28 +0100, Christian König a écrit :

  Am 23.01.24 um 14:02 schrieb Paul Cercueil:
  

[SNIP]
  
  
   
  
That an exporter has to call extra functions to access his

own
buffers
is a complete no-go for the design since this forces
exporters
into
doing extra steps for allowing importers to access their
data.
  
  
Then what about we add these dma_buf_{begin,end}_access(), with

only
implementations for "dumb" exporters e.g. udmabuf or the dmabuf
heaps?
And only importers (who cache the mapping and actually care
about
non-
coherency) would have to call these.
  
  
No, the problem is still that you would have to change all

importers
to
mandatory use dma_buf_begin/end.

But going a step back caching the mapping is irrelevant for
coherency.
Even if you don't cache the mapping you don't get coherency.
  
  
You actually do - at least with udmabuf, as in that case

dma_buf_map_attachment() / dma_buf_unmap_attachment() will handle
cache
coherency when the SGs are mapped/unmapped.
  
  
  Well I just double checked the source in 6.7.1 and I can't see

udmabuf doing anything for cache coherency in map/unmap.
  
  All it does is calling dma_map_sgtable() and dma_unmap_sgtable() to

create and destroy the SG table and those are not supposed to sync
anything to the CPU cache.
  
  In other words drivers usually use DMA_ATTR_SKIP_CPU_SYNC here, it's

just that this is missing from udmabuf.


Ok.
  
  
The problem was then that dma_buf_unmap_attachment cannot be called

before the dma_fence is signaled, and calling it after is already
too
late (because the fence would be signaled before the data is
sync'd).
  
  
  Well what sync are you talking about? CPU sync? In DMA-buf that is

handled differently.
  
  For importers it's mandatory that they can be coherent with the

exporter. That usually means they can snoop the CPU cache if the
exporter can snoop the CPU cache.


I seem to have such a system where one device can snoop the CPU cache
and the other cannot. Therefore if I want to support it properly, I do
need cache flush/sync. I don't actually try to access the data using
the CPU (and when I do, I call the sync start/end ioctls).



If you don't access the data using the CPU, then how did the data
end up in the CPU caches? If you have a device that can write-allocate
into your CPU cache, but some other device in the system cannot snoop
that data back out then that is just broken and those devices cannot
reasonably share buffers..

Now we do have systems where some hardware can snoop CPU(or L3) caches
and others cannot, but they will not *allocate* into those caches
(unless they also have the ability to sync them without CPU in the loop).

Your problem may be if you are still using udmabuf driver as your
DMA-BUF exporter, which as said before is broken (and I just sent some
patches with a few fixes just for you :)). For udmabuf, data starts
in the CPU domain (in caches) and is only ever synced for the CPU,
not for attached devices. So in this case the writing device might
update those cache lines but a non-snooping reader would never see
those updates.

I'm not saying there isn't a need for these new {begin,end}_access()
functions. I can think of a few interesting usecases, but as you
say below that would be good to work out in a different series.

Andrew




  For exporters you can implement the begin/end CPU access functions
which allows you to implement something even if your exporting device
can't snoop the CPU cache.


That only works if the importers call the begin_cpu_access() /
end_cpu_access(), which they don't.

  

Daniel / Sima suggested then that I cache the mapping and add new
functions to ensure cache coherency, which is what these patches
are
about.
  
  
  Yeah, I've now catched up on the latest mail. Sorry I haven't seen

that before.
  
  
  

  
  
In other words exporters are not require to call sync_to_cpu or

sync_to_device when you create a mapping.

What exactly is your use case here? And why does coherency
matters?
  
  
My use-case is, I create DMABUFs with udmabuf, that I attach to

USB/functionfs with the interface introduced by this patchset. I
attach
them to IIO with a similar interface (being upstreamed in
parallel),
and transfer data from USB to IIO and vice-versa in a zero-copy
fashion.

This works perfectly fine as long as the USB and IIO hardware are
coherent between themselves, which is the case on most of our
boards.
However I do have a board (with a Xilinx Ultrascale SoC) where it
is
not the case, and cache flushes/sync are needed. So I was trying to
rework these new interfaces to work on that system too.
  
  
  Yeah, that sounds strongly like one of the use cases we have

rejected so far.
  
  
  
  
If this really is a no-no, then I am fine with the assumption that

devices sharing a DMABUF must be coherent between themselves; but
that's something that should probably be enforced rather 

[PATCH 2/3] udmabuf: Sync buffer mappings for attached devices

[Andrew Davis]

Currently this driver creates a SGT table using the CPU as the
target device, then performs the dma_sync operations against
that SGT. This is backwards to how DMA-BUFs are supposed to behave.
This may have worked for the case where these buffers were given
only back to the same CPU that produced them as in the QEMU case.
And only then because the original author had the dma_sync
operations also backwards, syncing for the "device" on begin_cpu.
This was noticed and "fixed" in this patch[0].

That then meant we were sync'ing from the CPU to the CPU using
a pseudo-device "miscdevice". Which then caused another issue
due to the miscdevice not having a proper DMA mask (and why should
it, the CPU is not a DMA device). The fix for that was an even
more egregious hack[1] that declares the CPU is coherent with
itself and can access its own memory space..

Unwind all this and perform the correct action by doing the dma_sync
operations for each device currently attached to the backing buffer.

[0] commit 1ffe09590121 ("udmabuf: fix dma-buf cpu access")
[1] commit 9e9fa6a9198b ("udmabuf: Set the DMA mask for the udmabuf device 
(v2)")

Signed-off-by: Andrew Davis 
---
 drivers/dma-buf/udmabuf.c | 41 +++
 1 file changed, 16 insertions(+), 25 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 3a23f0a7d112a..ab6764322523c 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -26,8 +26,6 @@ MODULE_PARM_DESC(size_limit_mb, "Max size of a dmabuf, in 
megabytes. Default is
 struct udmabuf {
pgoff_t pagecount;
struct page **pages;
-   struct sg_table *sg;
-   struct miscdevice *device;
struct list_head attachments;
struct mutex lock;
 };
@@ -169,12 +167,8 @@ static void unmap_udmabuf(struct dma_buf_attachment *at,
 static void release_udmabuf(struct dma_buf *buf)
 {
struct udmabuf *ubuf = buf->priv;
-   struct device *dev = ubuf->device->this_device;
pgoff_t pg;
 
-   if (ubuf->sg)
-   put_sg_table(dev, ubuf->sg, DMA_BIDIRECTIONAL);
-
for (pg = 0; pg < ubuf->pagecount; pg++)
put_page(ubuf->pages[pg]);
kfree(ubuf->pages);
@@ -185,33 +179,31 @@ static int begin_cpu_udmabuf(struct dma_buf *buf,
 enum dma_data_direction direction)
 {
struct udmabuf *ubuf = buf->priv;
-   struct device *dev = ubuf->device->this_device;
-   int ret = 0;
-
-   if (!ubuf->sg) {
-   ubuf->sg = get_sg_table(dev, buf, direction);
-   if (IS_ERR(ubuf->sg)) {
-   ret = PTR_ERR(ubuf->sg);
-   ubuf->sg = NULL;
-   }
-   } else {
-   dma_sync_sg_for_cpu(dev, ubuf->sg->sgl, ubuf->sg->nents,
-   direction);
-   }
+   struct udmabuf_attachment *a;
 
-   return ret;
+   mutex_lock(>lock);
+
+   list_for_each_entry(a, >attachments, list)
+   dma_sync_sgtable_for_cpu(a->dev, a->table, direction);
+
+   mutex_unlock(>lock);
+
+   return 0;
 }
 
 static int end_cpu_udmabuf(struct dma_buf *buf,
   enum dma_data_direction direction)
 {
struct udmabuf *ubuf = buf->priv;
-   struct device *dev = ubuf->device->this_device;
+   struct udmabuf_attachment *a;
 
-   if (!ubuf->sg)
-   return -EINVAL;
+   mutex_lock(>lock);
+
+   list_for_each_entry(a, >attachments, list)
+   dma_sync_sgtable_for_device(a->dev, a->table, direction);
+
+   mutex_unlock(>lock);
 
-   dma_sync_sg_for_device(dev, ubuf->sg->sgl, ubuf->sg->nents, direction);
return 0;
 }
 
@@ -307,7 +299,6 @@ static long udmabuf_create(struct miscdevice *device,
exp_info.priv = ubuf;
exp_info.flags = O_RDWR;
 
-   ubuf->device = device;
buf = dma_buf_export(_info);
if (IS_ERR(buf)) {
ret = PTR_ERR(buf);
-- 
2.39.2

[PATCH 3/3] udmabuf: Use module_misc_device() to register this device

[Andrew Davis]

Now that we do not need to call dma_coerce_mask_and_coherent() on our
miscdevice device, use the module_misc_device() helper for registering
and module init/exit.

Signed-off-by: Andrew Davis 
---
 drivers/dma-buf/udmabuf.c | 30 +-
 1 file changed, 1 insertion(+), 29 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index ab6764322523c..3028ac3fd9f6a 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -392,34 +392,6 @@ static struct miscdevice udmabuf_misc = {
.name   = "udmabuf",
.fops   = _fops,
 };
-
-static int __init udmabuf_dev_init(void)
-{
-   int ret;
-
-   ret = misc_register(_misc);
-   if (ret < 0) {
-   pr_err("Could not initialize udmabuf device\n");
-   return ret;
-   }
-
-   ret = dma_coerce_mask_and_coherent(udmabuf_misc.this_device,
-  DMA_BIT_MASK(64));
-   if (ret < 0) {
-   pr_err("Could not setup DMA mask for udmabuf device\n");
-   misc_deregister(_misc);
-   return ret;
-   }
-
-   return 0;
-}
-
-static void __exit udmabuf_dev_exit(void)
-{
-   misc_deregister(_misc);
-}
-
-module_init(udmabuf_dev_init)
-module_exit(udmabuf_dev_exit)
+module_misc_device(udmabuf_misc);
 
 MODULE_AUTHOR("Gerd Hoffmann ");
-- 
2.39.2

[PATCH 1/3] udmabuf: Keep track current device mappings

[Andrew Davis]

When a device attaches to and maps our buffer we need to keep track
of this mapping/device. This is needed for synchronization with these
devices when beginning and ending CPU access for instance. Add a list
that tracks device mappings as part of {map,unmap}_udmabuf().

Signed-off-by: Andrew Davis 
---
 drivers/dma-buf/udmabuf.c | 43 +--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index c406459996489..3a23f0a7d112a 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -28,6 +28,14 @@ struct udmabuf {
struct page **pages;
struct sg_table *sg;
struct miscdevice *device;
+   struct list_head attachments;
+   struct mutex lock;
+};
+
+struct udmabuf_attachment {
+   struct device *dev;
+   struct sg_table *table;
+   struct list_head list;
 };
 
 static vm_fault_t udmabuf_vm_fault(struct vm_fault *vmf)
@@ -120,14 +128,42 @@ static void put_sg_table(struct device *dev, struct 
sg_table *sg,
 static struct sg_table *map_udmabuf(struct dma_buf_attachment *at,
enum dma_data_direction direction)
 {
-   return get_sg_table(at->dev, at->dmabuf, direction);
+   struct udmabuf *ubuf = at->dmabuf->priv;
+   struct udmabuf_attachment *a;
+
+   a = kzalloc(sizeof(*a), GFP_KERNEL);
+   if (!a)
+   return ERR_PTR(-ENOMEM);
+
+   a->table = get_sg_table(at->dev, at->dmabuf, direction);
+   if (IS_ERR(a->table)) {
+   kfree(a);
+   return a->table;
+   }
+
+   a->dev = at->dev;
+
+   mutex_lock(>lock);
+   list_add(>list, >attachments);
+   mutex_unlock(>lock);
+
+   return a->table;
 }
 
 static void unmap_udmabuf(struct dma_buf_attachment *at,
  struct sg_table *sg,
  enum dma_data_direction direction)
 {
-   return put_sg_table(at->dev, sg, direction);
+   struct udmabuf_attachment *a = at->priv;
+   struct udmabuf *ubuf = at->dmabuf->priv;
+
+   mutex_lock(>lock);
+   list_del(>list);
+   mutex_unlock(>lock);
+
+   put_sg_table(at->dev, sg, direction);
+
+   kfree(a);
 }
 
 static void release_udmabuf(struct dma_buf *buf)
@@ -263,6 +299,9 @@ static long udmabuf_create(struct miscdevice *device,
memfd = NULL;
}
 
+   INIT_LIST_HEAD(>attachments);
+   mutex_init(>lock);
+
exp_info.ops  = _ops;
exp_info.size = ubuf->pagecount << PAGE_SHIFT;
exp_info.priv = ubuf;
-- 
2.39.2

Re: [PATCH 08/11] ARM: dts: DRA7xx: Add device tree entry for SGX GPU

[Andrew Davis]

On 1/10/24 2:29 AM, Tony Lindgren wrote:

* Andrew Davis  [240109 17:20]:

--- a/arch/arm/boot/dts/ti/omap/dra7.dtsi
+++ b/arch/arm/boot/dts/ti/omap/dra7.dtsi
@@ -850,12 +850,19 @@ target-module@5600 {
;
ti,sysc-sidle = ,
,
-   ;
+   ,
+   ;


You probably checked this already.. But just in case, can you please
confirm this is intentional. The documentation lists the smart wakeup
capability bit as reserved for dra7, maybe the documentation is wrong.



It was an intentional change, although I'm not sure it is correct :)

This is how we had it in our "evil vendor tree" for years (back when it
was hwmod based), so when converting these nodes to use "ti,sysc" I noticed
this bit was set, but as you point out the documentation disagrees.

I'd rather go with what has worked before, but it doesn't seem to
break anything either way, so we could also break this change out into
its own patch if you would prefer.

Andrew


Regards,

Tony

Re: [PATCH v5 0/8] iio: new DMABUF based API, v5

[Andrew Davis]

On 1/11/24 3:20 AM, Paul Cercueil wrote:

Hi Andrew,

Le lundi 08 janvier 2024 à 15:12 -0600, Andrew Davis a écrit :

On 12/19/23 11:50 AM, Paul Cercueil wrote:

[V4 was: "iio: Add buffer write() support"][1]

Hi Jonathan,

This is a respin of the V3 of my patchset that introduced a new
interface based on DMABUF objects [2].

The V4 was a split of the patchset, to attempt to upstream buffer
write() support first. But since there is no current user upstream,
it
was not merged. This V5 is about doing the opposite, and contains
the
new DMABUF interface, without adding the buffer write() support. It
can
already be used with the upstream adi-axi-adc driver.

In user-space, Libiio uses it to transfer back and forth blocks of
samples between the hardware and the applications, without having
to
copy the data.

On a ZCU102 with a FMComms3 daughter board, running Libiio from the
pcercuei/dev-new-dmabuf-api branch [3], compiled with
WITH_LOCAL_DMABUF_API=OFF (so that it uses fileio):
    sudo utils/iio_rwdev -b 4096 -B cf-ad9361-lpc
    Throughput: 116 MiB/s

Same hardware, with the DMABUF API (WITH_LOCAL_DMABUF_API=ON):
    sudo utils/iio_rwdev -b 4096 -B cf-ad9361-lpc
    Throughput: 475 MiB/s

This benchmark only measures the speed at which the data can be
fetched
to iio_rwdev's internal buffers, and does not actually try to read
the
data (e.g. to pipe it to stdout). It shows that fetching the data
is
more than 4x faster using the new interface.

When actually reading the data, the performance difference isn't
that
impressive (maybe because in case of DMABUF the data is not in
cache):

WITH_LOCAL_DMABUF_API=OFF (so that it uses fileio):
    sudo utils/iio_rwdev -b 4096 cf-ad9361-lpc | dd of=/dev/zero
status=progress
    2446422528 bytes (2.4 GB, 2.3 GiB) copied, 22 s, 111 MB/s

WITH_LOCAL_DMABUF_API=ON:
    sudo utils/iio_rwdev -b 4096 cf-ad9361-lpc | dd of=/dev/zero
status=progress
    2334388736 bytes (2.3 GB, 2.2 GiB) copied, 21 s, 114 MB/s

One interesting thing to note is that fileio is (currently)
actually
faster than the DMABUF interface if you increase a lot the buffer
size.
My explanation is that the cache invalidation routine takes more
and
more time the bigger the DMABUF gets. This is because the DMABUF is
backed by small-size pages, so a (e.g.) 64 MiB DMABUF is backed by
up
to 16 thousands pages, that have to be invalidated one by one. This
can
be addressed by using huge pages, but the udmabuf driver does not
(yet)
support creating DMABUFs backed by huge pages.



Have you tried DMABUFs created using the DMABUF System heap exporter?
(drivers/dma-buf/heaps/system_heap.c) It should be able to handle
larger allocation better here, and if you don't have any active
mmaps or vmaps then it can skip CPU-side coherency maintenance
(useful for device to device transfers).


I didn't know about it!

But udmabuf also allows you to skip CPU-side coherency maintenance,
since DMABUFs have two ioctls to start/finish CPU access anyway.



The only way it lets you skip that is if your application just doesn't
call those begin/end ioctls, which is wrong. That may work on a system
where CPU caches can be snooped by all devices that could attach to
a buffer(x86), but that might not work on others(ARM). So calling
those begin/end ioctls is required[0]. If maintenance is not actually
needed then the kernel will turn those calls into NOPs for you, but only
the kernel can know when that is correct (based on the running system
and the devices attached to that buffer), not userspace.


Allocating DMABUFs out of user pages has a bunch of other issues you
might run into also. I'd argue udmabuf is now completely superseded
by DMABUF system heaps. Try it out :)


I'm curious, what other issues?



For starters the {begin,end}_cpu_access() callbacks don't actually
sync the pages for any of the devices attached to the DMABUF, it
only makes a fake mapping for the misc device(CPU) then syncs with
that. That probably works for the QEMU case it was designed for where
the device is always a VM instance running on the same CPU, but for
any real devices the sync never happens towards them.

I have some patches fixing the above I'll post this cycle, but it
wont help with folks doing reads/wrties on the original shmem/memfd
outside of the begin/end ioctls. So there is a fundamental issue
with the buffer's backing memory's ownership/lifecycle that makes
udmabuf broken by design.

The DMABUF System Heap owns the backing memory and manages that
memory's lifecycle as all correct DMABUF exporters must.


The good thing about udmabuf is that the memory is backed by pages, so
we can use MSG_ZEROCOPY on sockets to transfer the mmapped data over
the network (having a DMABUF interface to the network stack would be
better, but I'm not opening that can of worms).



Yes, having a DMABUF importer interface for the network stack would be
the best long-term solution here, and one will probably end up being
needed for zero-copy buffer passin

Re: [PATCH 01/11] dt-bindings: gpu: Rename img,powervr to img,powervr-rogue

[Andrew Davis]

On 1/9/24 1:17 PM, Krzysztof Kozlowski wrote:

On 09/01/2024 20:04, Andrew Davis wrote:

On 1/9/24 12:59 PM, Krzysztof Kozlowski wrote:

On 09/01/2024 18:19, Andrew Davis wrote:

This binding will be used for GPUs starting from Series6 (Rogue)
and later. A different binding document will describe Series5.
With that the name "img,powervr" is too generic, rename to
"img,powervr-rogue" to avoid confusion.

Suggested-by: Maxime Ripard 
Signed-off-by: Andrew Davis 
Reviewed-by: Javier Martinez Canillas 
Reviewed-by: Frank Binns 
---


Why do you send new version while we still talk about previous?

Please implement feedback from v1 (and this is v2, so next is v3) or
keep discussing.



I agreed with everything you said in the last round (RFC v2) and
made all requested changes. Did I miss something in this version?


The recommendation is that naming of the file matches generic compatible
and your file has only one generic compatible. Therefore I don't
understand why you claimed there are multiple compatibles.



I said "There are (or will be) multiple compatible strings", the rest
are on the way. So I didn't want to make this file less generic when
other bindings are almost ready.

Frank, can you help here, I'm assuming you have "img,img-bxs" and
"img,img-8xe" bindings staged for upstreaming somewhere; you'll be
putting those in this same file, right?

Thanks,
Andrew


Best regards,
Krzysztof

Re: [PATCH 01/11] dt-bindings: gpu: Rename img,powervr to img,powervr-rogue

[Andrew Davis]

On 1/9/24 12:59 PM, Krzysztof Kozlowski wrote:

On 09/01/2024 18:19, Andrew Davis wrote:

This binding will be used for GPUs starting from Series6 (Rogue)
and later. A different binding document will describe Series5.
With that the name "img,powervr" is too generic, rename to
"img,powervr-rogue" to avoid confusion.

Suggested-by: Maxime Ripard 
Signed-off-by: Andrew Davis 
Reviewed-by: Javier Martinez Canillas 
Reviewed-by: Frank Binns 
---


Why do you send new version while we still talk about previous?

Please implement feedback from v1 (and this is v2, so next is v3) or
keep discussing.



I agreed with everything you said in the last round (RFC v2) and
made all requested changes. Did I miss something in this version?

Thanks,
Andrew


Best regards,
Krzysztof

[PATCH 02/11] dt-bindings: gpu: Add PowerVR Series5 SGX GPUs

[Andrew Davis]

The Imagination PowerVR Series5 "SGX" GPU is part of several SoCs from
multiple vendors. Describe how the SGX GPU is integrated in these SoC,
including register space and interrupts. Clocks, reset, and power domain
information is SoC specific.

Signed-off-by: Andrew Davis 
Reviewed-by: Javier Martinez Canillas 
---
 .../bindings/gpu/img,powervr-sgx.yaml | 138 ++
 MAINTAINERS   |   1 +
 2 files changed, 139 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml

diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml 
b/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml
new file mode 100644
index 0..f5898b04381cb
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml
@@ -0,0 +1,138 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (c) 2023 Imagination Technologies Ltd.
+# Copyright (C) 2024 Texas Instruments Incorporated - https://www.ti.com/
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpu/img,powervr-sgx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Imagination Technologies PowerVR SGX GPUs
+
+maintainers:
+  - Frank Binns 
+
+properties:
+  compatible:
+oneOf:
+  - items:
+  - enum:
+  - ti,omap3430-gpu # Rev 121
+  - ti,omap3630-gpu # Rev 125
+  - const: img,powervr-sgx530
+  - items:
+  - enum:
+  - ingenic,jz4780-gpu # Rev 130
+  - ti,omap4430-gpu # Rev 120
+  - const: img,powervr-sgx540
+  - items:
+  - enum:
+  - allwinner,sun6i-a31-gpu # MP2 Rev 115
+  - ti,omap4470-gpu # MP1 Rev 112
+  - ti,omap5432-gpu # MP2 Rev 105
+  - ti,am5728-gpu # MP2 Rev 116
+  - ti,am6548-gpu # MP1 Rev 117
+  - const: img,powervr-sgx544
+
+  reg:
+maxItems: 1
+
+  interrupts:
+maxItems: 1
+
+  clocks:
+minItems: 1
+maxItems: 3
+
+  clock-names:
+minItems: 1
+items:
+  - const: core
+  - const: mem
+  - const: sys
+
+  power-domains:
+maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+allOf:
+  - if:
+  properties:
+compatible:
+  contains:
+const: ti,am6548-gpu
+then:
+  required:
+- power-domains
+else:
+  properties:
+power-domains: false
+  - if:
+  properties:
+compatible:
+  contains:
+enum:
+  - allwinner,sun6i-a31-gpu
+  - ingenic,jz4780-gpu
+then:
+  required:
+- clocks
+- clock-names
+else:
+  properties:
+clocks: false
+clock-names: false
+  - if:
+  properties:
+compatible:
+  contains:
+const: allwinner,sun6i-a31-gpu
+then:
+  properties:
+clocks:
+  minItems: 2
+  maxItems: 2
+clock-names:
+  minItems: 2
+  maxItems: 2
+  - if:
+  properties:
+compatible:
+  contains:
+const: ingenic,jz4780-gpu
+then:
+  properties:
+clocks:
+  maxItems: 1
+clock-names:
+  maxItems: 1
+
+additionalProperties: false
+
+examples:
+  - |
+#include 
+#include 
+#include 
+
+gpu@700 {
+compatible = "ti,am6548-gpu", "img,powervr-sgx544";
+reg = <0x700 0x1>;
+interrupts = ;
+power-domains = <_pds 65 TI_SCI_PD_EXCLUSIVE>;
+};
+
+  - |
+#include 
+#include 
+
+gpu: gpu@1c4 {
+compatible = "allwinner,sun6i-a31-gpu", "img,powervr-sgx544";
+reg = <0x01c4 0x1>;
+interrupts = ;
+clocks = < 1>, < 2>;
+clock-names = "core", "mem";
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index 2a4e8d2c69c40..b8b3aab5dd490 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10469,6 +10469,7 @@ M:  Matt Coster 
 S: Supported
 T: git git://anongit.freedesktop.org/drm/drm-misc
 F: Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
+F: Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml
 F: Documentation/gpu/imagination/
 F: drivers/gpu/drm/imagination/
 F: include/uapi/drm/pvr_drm.h
-- 
2.39.2

[PATCH 11/11] MIPS: DTS: jz4780: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base jz4780 dtsi file.

Signed-off-by: Andrew Davis 
Reviewed-by: Javier Martinez Canillas 
---
 arch/mips/boot/dts/ingenic/jz4780.dtsi | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi 
b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 18a85ce38..5ea6833f5e872 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -460,6 +460,17 @@ hdmi: hdmi@1018 {
status = "disabled";
};
 
+   gpu: gpu@1304 {
+   compatible = "ingenic,jz4780-gpu", "img,powervr-sgx540";
+   reg = <0x1304 0x4000>;
+
+   clocks = < JZ4780_CLK_GPU>;
+   clock-names = "core";
+
+   interrupt-parent = <>;
+   interrupts = <63>;
+   };
+
lcdc0: lcdc0@1305 {
compatible = "ingenic,jz4780-lcd";
reg = <0x1305 0x1800>;
-- 
2.39.2

[PATCH 09/11] arm64: dts: ti: k3-am654-main: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base AM654 dtsi file.

Signed-off-by: Andrew Davis 
Reviewed-by: Javier Martinez Canillas 
---
 arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi 
b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
index fcea544656360..64b52c8dafc6c 100644
--- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
@@ -1050,6 +1050,13 @@ dss_ports: ports {
};
};
 
+   gpu: gpu@700 {
+   compatible = "ti,am6548-gpu", "img,powervr-sgx544";
+   reg = <0x0 0x700 0x0 0x1>;
+   interrupts = ;
+   power-domains = <_pds 65 TI_SCI_PD_EXCLUSIVE>;
+   };
+
ehrpwm0: pwm@300 {
compatible = "ti,am654-ehrpwm", "ti,am3352-ehrpwm";
#pwm-cells = <3>;
-- 
2.39.2

[PATCH 00/11] Device tree support for Imagination Series5 GPU

[Andrew Davis]

Hello all,

I know this has been tried before[0], but given the recent upstreaming of
the Series6+ GPU bindings I figured it might be time to give the Series5
bindings another try.

While there is currently no mainline driver for these binding, there is an
open source out-of-tree kernel-side driver available[1]. Having a stable
and upstream binding for these devices allows us to describe this hardware
in device tree.

This is my vision for how these bindings should look, along with some
example uses in several SoC DT files. The compatible names have been
updated to match what was decided on for Series6+, but otherwise most
is the same as we have been using in our vendor tree for many years.

Thanks,
Andrew

Based on next-20240109.

[0]: https://lkml.org/lkml/2020/4/24/1222
[1]: https://github.com/openpvrsgx-devgroup

Changes for v1:
 - Added commit message to patch #1
 - Reworked Rogue binding title
 - Add TI copyright to new binding doc
 - Added default min/maxItems to clocks property
 - Moved "additionalProperties" to end
 - Flattened out allOf block logic
 - Added extra SGX binding example
 - Added Suggested/Reviewed tags

Changes for RFC v2:
 - Added patch to rename Rogue+ binding to img,powervr-rogue.yaml
 - Locked all property item counts
 - Removed nodename pattern check

Andrew Davis (11):
  dt-bindings: gpu: Rename img,powervr to img,powervr-rogue
  dt-bindings: gpu: Add PowerVR Series5 SGX GPUs
  ARM: dts: omap3: Add device tree entry for SGX GPU
  ARM: dts: omap4: Add device tree entry for SGX GPU
  ARM: dts: omap5: Add device tree entry for SGX GPU
  ARM: dts: AM33xx: Add device tree entry for SGX GPU
  ARM: dts: AM437x: Add device tree entry for SGX GPU
  ARM: dts: DRA7xx: Add device tree entry for SGX GPU
  arm64: dts: ti: k3-am654-main: Add device tree entry for SGX GPU
  ARM: dts: sun6i: Add device tree entry for SGX GPU
  MIPS: DTS: jz4780: Add device tree entry for SGX GPU

 ...mg,powervr.yaml => img,powervr-rogue.yaml} |   4 +-
 .../bindings/gpu/img,powervr-sgx.yaml | 138 ++
 MAINTAINERS   |   3 +-
 arch/arm/boot/dts/allwinner/sun6i-a31.dtsi|   9 ++
 arch/arm/boot/dts/ti/omap/am33xx.dtsi |   9 +-
 arch/arm/boot/dts/ti/omap/am3517.dtsi |  11 +-
 arch/arm/boot/dts/ti/omap/am4372.dtsi |   6 +
 arch/arm/boot/dts/ti/omap/dra7.dtsi   |   9 +-
 arch/arm/boot/dts/ti/omap/omap34xx.dtsi   |  11 +-
 arch/arm/boot/dts/ti/omap/omap36xx.dtsi   |   9 +-
 arch/arm/boot/dts/ti/omap/omap4.dtsi  |   9 +-
 arch/arm/boot/dts/ti/omap/omap5.dtsi  |   9 +-
 arch/arm64/boot/dts/ti/k3-am65-main.dtsi  |   7 +
 arch/mips/boot/dts/ingenic/jz4780.dtsi|  11 ++
 14 files changed, 215 insertions(+), 30 deletions(-)
 rename Documentation/devicetree/bindings/gpu/{img,powervr.yaml => 
img,powervr-rogue.yaml} (91%)
 create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml

-- 
2.39.2

[PATCH 01/11] dt-bindings: gpu: Rename img, powervr to img, powervr-rogue

[Andrew Davis]

This binding will be used for GPUs starting from Series6 (Rogue)
and later. A different binding document will describe Series5.
With that the name "img,powervr" is too generic, rename to
"img,powervr-rogue" to avoid confusion.

Suggested-by: Maxime Ripard 
Signed-off-by: Andrew Davis 
Reviewed-by: Javier Martinez Canillas 
Reviewed-by: Frank Binns 
---
 .../bindings/gpu/{img,powervr.yaml => img,powervr-rogue.yaml} | 4 ++--
 MAINTAINERS   | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
 rename Documentation/devicetree/bindings/gpu/{img,powervr.yaml => 
img,powervr-rogue.yaml} (91%)

diff --git a/Documentation/devicetree/bindings/gpu/img,powervr.yaml 
b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
similarity index 91%
rename from Documentation/devicetree/bindings/gpu/img,powervr.yaml
rename to Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
index a13298f1a1827..256e252f8087f 100644
--- a/Documentation/devicetree/bindings/gpu/img,powervr.yaml
+++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
@@ -2,10 +2,10 @@
 # Copyright (c) 2023 Imagination Technologies Ltd.
 %YAML 1.2
 ---
-$id: http://devicetree.org/schemas/gpu/img,powervr.yaml#
+$id: http://devicetree.org/schemas/gpu/img,powervr-rogue.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Imagination Technologies PowerVR and IMG GPU
+title: Imagination Technologies PowerVR and IMG Rogue GPUs
 
 maintainers:
   - Frank Binns 
diff --git a/MAINTAINERS b/MAINTAINERS
index bcacd665f2594..2a4e8d2c69c40 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10468,7 +10468,7 @@ M:  Donald Robson 
 M: Matt Coster 
 S: Supported
 T: git git://anongit.freedesktop.org/drm/drm-misc
-F: Documentation/devicetree/bindings/gpu/img,powervr.yaml
+F: Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
 F: Documentation/gpu/imagination/
 F: drivers/gpu/drm/imagination/
 F: include/uapi/drm/pvr_drm.h
-- 
2.39.2

[PATCH 05/11] ARM: dts: omap5: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base OMAP5 dtsi file.

Signed-off-by: Andrew Davis 
Reviewed-by: Javier Martinez Canillas 
---
 arch/arm/boot/dts/ti/omap/omap5.dtsi | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/ti/omap/omap5.dtsi 
b/arch/arm/boot/dts/ti/omap/omap5.dtsi
index bac6fa8387936..6a66214ad0e2f 100644
--- a/arch/arm/boot/dts/ti/omap/omap5.dtsi
+++ b/arch/arm/boot/dts/ti/omap/omap5.dtsi
@@ -453,10 +453,11 @@ target-module@5600 {
#size-cells = <1>;
ranges = <0 0x5600 0x200>;
 
-   /*
-* Closed source PowerVR driver, no child device
-* binding or driver in mainline
-*/
+   gpu@0 {
+   compatible = "ti,omap5432-gpu", 
"img,powervr-sgx544";
+   reg = <0x0 0x200>; /* 32MB */
+   interrupts = ;
+   };
};
 
target-module@5800 {
-- 
2.39.2

[PATCH 08/11] ARM: dts: DRA7xx: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base DRA7x dtsi file.

Signed-off-by: Andrew Davis 
Reviewed-by: Javier Martinez Canillas 
---
 arch/arm/boot/dts/ti/omap/dra7.dtsi | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/ti/omap/dra7.dtsi 
b/arch/arm/boot/dts/ti/omap/dra7.dtsi
index 6509c742fb58c..8527643cb69a8 100644
--- a/arch/arm/boot/dts/ti/omap/dra7.dtsi
+++ b/arch/arm/boot/dts/ti/omap/dra7.dtsi
@@ -850,12 +850,19 @@ target-module@5600 {
;
ti,sysc-sidle = ,
,
-   ;
+   ,
+   ;
clocks = <_clkctrl DRA7_GPU_CLKCTRL 0>;
clock-names = "fck";
#address-cells = <1>;
#size-cells = <1>;
ranges = <0 0x5600 0x200>;
+
+   gpu@0 {
+   compatible = "ti,am5728-gpu", 
"img,powervr-sgx544";
+   reg = <0x0 0x1>; /* 64kB */
+   interrupts = ;
+   };
};
 
crossbar_mpu: crossbar@4a002a48 {
-- 
2.39.2

[PATCH 04/11] ARM: dts: omap4: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base OMAP4 dtsi file.

Signed-off-by: Andrew Davis 
Reviewed-by: Javier Martinez Canillas 
---
 arch/arm/boot/dts/ti/omap/omap4.dtsi | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/ti/omap/omap4.dtsi 
b/arch/arm/boot/dts/ti/omap/omap4.dtsi
index 2bbff9032be3e..559b2bfe4ca7c 100644
--- a/arch/arm/boot/dts/ti/omap/omap4.dtsi
+++ b/arch/arm/boot/dts/ti/omap/omap4.dtsi
@@ -501,10 +501,11 @@ sgx_module: target-module@5600 {
#size-cells = <1>;
ranges = <0 0x5600 0x200>;
 
-   /*
-* Closed source PowerVR driver, no child device
-* binding or driver in mainline
-*/
+   gpu@0 {
+   compatible = "ti,omap4430-gpu", 
"img,powervr-sgx540";
+   reg = <0x0 0x200>; /* 32MB */
+   interrupts = ;
+   };
};
 
/*
-- 
2.39.2

[PATCH 10/11] ARM: dts: sun6i: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base sun6i-a31 dtsi file.

Signed-off-by: Andrew Davis 
Reviewed-by: Javier Martinez Canillas 
---
 arch/arm/boot/dts/allwinner/sun6i-a31.dtsi | 9 +
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/allwinner/sun6i-a31.dtsi 
b/arch/arm/boot/dts/allwinner/sun6i-a31.dtsi
index 5cce4918f84c9..e6998783b89aa 100644
--- a/arch/arm/boot/dts/allwinner/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/allwinner/sun6i-a31.dtsi
@@ -962,6 +962,15 @@ mdio: mdio {
};
};
 
+   gpu: gpu@1c4 {
+   compatible = "allwinner,sun6i-a31-gpu", 
"img,powervr-sgx544";
+   reg = <0x01c4 0x1>;
+   interrupts = ;
+   clocks = < CLK_GPU_CORE>, < CLK_GPU_MEMORY>;
+   clock-names = "core", "mem";
+   status = "disabled";
+   };
+
crypto: crypto-engine@1c15000 {
compatible = "allwinner,sun6i-a31-crypto",
 "allwinner,sun4i-a10-crypto";
-- 
2.39.2

[PATCH 03/11] ARM: dts: omap3: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entries to base OMAP3 dtsi files.

Signed-off-by: Andrew Davis 
Reviewed-by: Javier Martinez Canillas 
---
 arch/arm/boot/dts/ti/omap/am3517.dtsi   | 11 ++-
 arch/arm/boot/dts/ti/omap/omap34xx.dtsi | 11 ++-
 arch/arm/boot/dts/ti/omap/omap36xx.dtsi |  9 +
 3 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/arch/arm/boot/dts/ti/omap/am3517.dtsi 
b/arch/arm/boot/dts/ti/omap/am3517.dtsi
index 77e58e686fb17..19aad715dff70 100644
--- a/arch/arm/boot/dts/ti/omap/am3517.dtsi
+++ b/arch/arm/boot/dts/ti/omap/am3517.dtsi
@@ -162,12 +162,13 @@ sgx_module: target-module@5000 {
clock-names = "fck", "ick";
#address-cells = <1>;
#size-cells = <1>;
-   ranges = <0 0x5000 0x4000>;
+   ranges = <0 0x5000 0x1>;
 
-   /*
-* Closed source PowerVR driver, no child device
-* binding or driver in mainline
-*/
+   gpu@0 {
+   compatible = "ti,omap3430-gpu", 
"img,powervr-sgx530";
+   reg = <0x0 0x1>; /* 64kB */
+   interrupts = <21>;
+   };
};
};
 };
diff --git a/arch/arm/boot/dts/ti/omap/omap34xx.dtsi 
b/arch/arm/boot/dts/ti/omap/omap34xx.dtsi
index fc7233ac183a8..acdd0ee34421d 100644
--- a/arch/arm/boot/dts/ti/omap/omap34xx.dtsi
+++ b/arch/arm/boot/dts/ti/omap/omap34xx.dtsi
@@ -164,12 +164,13 @@ sgx_module: target-module@5000 {
clock-names = "fck", "ick";
#address-cells = <1>;
#size-cells = <1>;
-   ranges = <0 0x5000 0x4000>;
+   ranges = <0 0x5000 0x1>;
 
-   /*
-* Closed source PowerVR driver, no child device
-* binding or driver in mainline
-*/
+   gpu@0 {
+   compatible = "ti,omap3430-gpu", 
"img,powervr-sgx530";
+   reg = <0x0 0x1>; /* 64kB */
+   interrupts = <21>;
+   };
};
};
 
diff --git a/arch/arm/boot/dts/ti/omap/omap36xx.dtsi 
b/arch/arm/boot/dts/ti/omap/omap36xx.dtsi
index e6d8070c1bf88..c3d79ecd56e39 100644
--- a/arch/arm/boot/dts/ti/omap/omap36xx.dtsi
+++ b/arch/arm/boot/dts/ti/omap/omap36xx.dtsi
@@ -211,10 +211,11 @@ sgx_module: target-module@5000 {
#size-cells = <1>;
ranges = <0 0x5000 0x200>;
 
-   /*
-* Closed source PowerVR driver, no child device
-* binding or driver in mainline
-*/
+   gpu@0 {
+   compatible = "ti,omap3630-gpu", 
"img,powervr-sgx530";
+   reg = <0x0 0x200>; /* 32MB */
+   interrupts = <21>;
+   };
};
};
 
-- 
2.39.2

[PATCH 06/11] ARM: dts: AM33xx: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base AM33xx dtsi file.

Signed-off-by: Andrew Davis 
Reviewed-by: Javier Martinez Canillas 
---
 arch/arm/boot/dts/ti/omap/am33xx.dtsi | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/ti/omap/am33xx.dtsi 
b/arch/arm/boot/dts/ti/omap/am33xx.dtsi
index 5b9e01a8aa5d5..989d5a6edeed9 100644
--- a/arch/arm/boot/dts/ti/omap/am33xx.dtsi
+++ b/arch/arm/boot/dts/ti/omap/am33xx.dtsi
@@ -640,10 +640,11 @@ target-module@5600 {
#size-cells = <1>;
ranges = <0 0x5600 0x100>;
 
-   /*
-* Closed source PowerVR driver, no child device
-* binding or driver in mainline
-*/
+   gpu@0 {
+   compatible = "ti,omap3630-gpu", 
"img,powervr-sgx530";
+   reg = <0x0 0x1>; /* 64kB */
+   interrupts = <37>;
+   };
};
};
 };
-- 
2.39.2

[PATCH 07/11] ARM: dts: AM437x: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base AM437x dtsi file.

Signed-off-by: Andrew Davis 
Reviewed-by: Javier Martinez Canillas 
---
 arch/arm/boot/dts/ti/omap/am4372.dtsi | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/ti/omap/am4372.dtsi 
b/arch/arm/boot/dts/ti/omap/am4372.dtsi
index 9d2c064534f7d..5fd1b380ece62 100644
--- a/arch/arm/boot/dts/ti/omap/am4372.dtsi
+++ b/arch/arm/boot/dts/ti/omap/am4372.dtsi
@@ -719,6 +719,12 @@ target-module@5600 {
#address-cells = <1>;
#size-cells = <1>;
ranges = <0 0x5600 0x100>;
+
+   gpu@0 {
+   compatible = "ti,omap3630-gpu", 
"img,powervr-sgx530";
+   reg = <0x0 0x1>; /* 64kB */
+   interrupts = ;
+   };
};
};
 };
-- 
2.39.2

Re: [PATCH RFC v2 02/11] dt-bindings: gpu: Add PowerVR Series5 SGX GPUs

[Andrew Davis]

On 1/9/24 5:32 AM, Krzysztof Kozlowski wrote:

On 08/01/2024 19:32, Andrew Davis wrote:

The Imagination PowerVR Series5 "SGX" GPU is part of several SoCs from
multiple vendors. Describe how the SGX GPU is integrated in these SoC,
including register space and interrupts. Clocks, reset, and power domain
information is SoC specific.

Signed-off-by: Andrew Davis 
---
  .../bindings/gpu/img,powervr-sgx.yaml | 124 ++
  MAINTAINERS   |   1 +
  2 files changed, 125 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml

diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml 
b/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml
new file mode 100644
index 0..bb821e1184de9
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml
@@ -0,0 +1,124 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (c) 2023 Imagination Technologies Ltd.


Your email has @TI domain, are you sure you attribute your copyrights to
Imagination?



The file started as a copy/paste from a IMG copyrighted file, even
though it is now almost completely re-written I've left their (c)
for good measure. I'll add an additional TI (c).


...


+
+  reg:
+maxItems: 1
+
+  interrupts:
+maxItems: 1
+
+  clocks: true


Missing min/maxItems



These are set in the allOf/if/then blocks below, seems
if I don't set them to at least something here then I get
a warning:

   'clock-names', 'clocks' do not match any of the regexes: 'pinctrl-[0-9]+'

even if I define them in the allOf block below. I don't
know what the min/max should be until I check the compatible
in the allOf block.


+
+  clock-names:
+minItems: 1
+items:
+  - const: core
+  - const: mem
+  - const: sys
+
+  power-domains:
+maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+additionalProperties: false


This goes after allOf: block.



ACK


+
+allOf:
+  - if:
+  properties:
+compatible:
+  contains:
+const: ti,am6548-gpu
+then:
+  required:
+- power-domains
+else:
+  properties:
+power-domains: false
+  - if:
+  properties:
+compatible:
+  contains:
+enum:
+  - allwinner,sun6i-a31-gpu
+  - ingenic,jz4780-gpu
+then:
+  allOf:
+- if:


I don't understand why do you need to embed allOf inside another allOf.
The upper (outer) if:then: looks entirely useless.



It is so that both compatibles falls through to having
clock being required.

Logic in YAML always seems messy to me, here it is in pseudo C:

if (compatible == allwinner,sun6i-a31-gpu ||
compatible == ingenic,jz4780-gpu) {
if (compatible == allwinner,sun6i-a31-gpu)
clocks: ...
if (compatible == ingenic,jz4780-gpu)
clocks: ...
required:
- clocks
- clock-names
} else { /* disallow for all others */
properties:
clocks: false
clock-names: false
}

Now if I had an "else if" that didn't force the indention to keep
growing I would have used that. (does one exist?) I also cannot
simply add the clock properties only for the two compats need
them for the reasons above and so must add them unconditionally
before then explicitly disable them in a catch-all else path.

Andrew


+properties:
+  compatible:
+contains:
+  const: allwinner,sun6i-a31-gpu
+  then:
+properties:
+  clocks:
+minItems: 2
+maxItems: 2
+  clock-names:
+minItems: 2
+maxItems: 2



Best regards,
Krzysztof

Re: [PATCH RFC v2 01/11] dt-bindings: gpu: Rename img,powervr to img,powervr-rogue

[Andrew Davis]

On 1/9/24 5:28 AM, Krzysztof Kozlowski wrote:

On 08/01/2024 19:32, Andrew Davis wrote:

Signed-off-by: Andrew Davis 
---
  .../bindings/gpu/{img,powervr.yaml => img,powervr-rogue.yaml} | 4 ++--
  MAINTAINERS   | 2 +-
  2 files changed, 3 insertions(+), 3 deletions(-)


If you are renaming it, why not renaming to match compatible as we
usually expect?



There are (or will be) multiple compatible strings described in this
file, naming the file after just one would not fully convey the content
of the file. This generic style naming seems common already for bindings
with multiple compatibles.

Andrew


Best regards,
Krzysztof

Re: [PATCH v5 0/8] iio: new DMABUF based API, v5

[Andrew Davis]

On 12/19/23 11:50 AM, Paul Cercueil wrote:

[V4 was: "iio: Add buffer write() support"][1]

Hi Jonathan,

This is a respin of the V3 of my patchset that introduced a new
interface based on DMABUF objects [2].

The V4 was a split of the patchset, to attempt to upstream buffer
write() support first. But since there is no current user upstream, it
was not merged. This V5 is about doing the opposite, and contains the
new DMABUF interface, without adding the buffer write() support. It can
already be used with the upstream adi-axi-adc driver.

In user-space, Libiio uses it to transfer back and forth blocks of
samples between the hardware and the applications, without having to
copy the data.

On a ZCU102 with a FMComms3 daughter board, running Libiio from the
pcercuei/dev-new-dmabuf-api branch [3], compiled with
WITH_LOCAL_DMABUF_API=OFF (so that it uses fileio):
   sudo utils/iio_rwdev -b 4096 -B cf-ad9361-lpc
   Throughput: 116 MiB/s

Same hardware, with the DMABUF API (WITH_LOCAL_DMABUF_API=ON):
   sudo utils/iio_rwdev -b 4096 -B cf-ad9361-lpc
   Throughput: 475 MiB/s

This benchmark only measures the speed at which the data can be fetched
to iio_rwdev's internal buffers, and does not actually try to read the
data (e.g. to pipe it to stdout). It shows that fetching the data is
more than 4x faster using the new interface.

When actually reading the data, the performance difference isn't that
impressive (maybe because in case of DMABUF the data is not in cache):

WITH_LOCAL_DMABUF_API=OFF (so that it uses fileio):
   sudo utils/iio_rwdev -b 4096 cf-ad9361-lpc | dd of=/dev/zero status=progress
   2446422528 bytes (2.4 GB, 2.3 GiB) copied, 22 s, 111 MB/s

WITH_LOCAL_DMABUF_API=ON:
   sudo utils/iio_rwdev -b 4096 cf-ad9361-lpc | dd of=/dev/zero status=progress
   2334388736 bytes (2.3 GB, 2.2 GiB) copied, 21 s, 114 MB/s

One interesting thing to note is that fileio is (currently) actually
faster than the DMABUF interface if you increase a lot the buffer size.
My explanation is that the cache invalidation routine takes more and
more time the bigger the DMABUF gets. This is because the DMABUF is
backed by small-size pages, so a (e.g.) 64 MiB DMABUF is backed by up
to 16 thousands pages, that have to be invalidated one by one. This can
be addressed by using huge pages, but the udmabuf driver does not (yet)
support creating DMABUFs backed by huge pages.



Have you tried DMABUFs created using the DMABUF System heap exporter?
(drivers/dma-buf/heaps/system_heap.c) It should be able to handle
larger allocation better here, and if you don't have any active
mmaps or vmaps then it can skip CPU-side coherency maintenance
(useful for device to device transfers).

Allocating DMABUFs out of user pages has a bunch of other issues you
might run into also. I'd argue udmabuf is now completely superseded
by DMABUF system heaps. Try it out :)

Andrew


Anyway, the real benefits happen when the DMABUFs are either shared
between IIO devices, or between the IIO subsystem and another
filesystem. In that case, the DMABUFs are simply passed around drivers,
without the data being copied at any moment.

We use that feature to transfer samples from our transceivers to USB,
using a DMABUF interface to FunctionFS [4].

This drastically increases the throughput, to about 274 MiB/s over a
USB3 link, vs. 127 MiB/s using IIO's fileio interface + write() to the
FunctionFS endpoints, for a lower CPU usage (0.85 vs. 0.65 load avg.).

Based on linux-next/next-20231219.

Cheers,
-Paul

[1] https://lore.kernel.org/all/20230807112113.47157-1-p...@crapouillou.net/
[2] https://lore.kernel.org/all/20230403154800.215924-1-p...@crapouillou.net/
[3] https://github.com/analogdevicesinc/libiio/tree/pcercuei/dev-new-dmabuf-api
[4] https://lore.kernel.org/all/20230322092118.9213-1-p...@crapouillou.net/

---
Changelog:
- [3/8]: Replace V3's dmaengine_prep_slave_dma_array() with a new
   dmaengine_prep_slave_dma_vec(), which uses a new 'dma_vec' struct.
   Note that at some point we will need to support cyclic transfers
   using dmaengine_prep_slave_dma_vec(). Maybe with a new "flags"
   parameter to the function?

- [4/8]: Implement .device_prep_slave_dma_vec() instead of V3's
   .device_prep_slave_dma_array().

   @Vinod: this patch will cause a small conflict with my other
   patchset adding scatter-gather support to the axi-dmac driver.
   This patch adds a call to axi_dmac_alloc_desc(num_sgs), but the
   prototype of this function changed in my other patchset - it would
   have to be passed the "chan" variable. I don't know how you prefer it
   to be resolved. Worst case scenario (and if @Jonathan is okay with
   that) this one patch can be re-sent later, but it would make this
   patchset less "atomic".

- [5/8]:
   - Use dev_err() instead of pr_err()
   - Inline to_iio_dma_fence()
   - Add comment to explain why we unref twice when detaching dmabuf
   - Remove TODO comment. It is actually safe to free the file's
 private data even when transfers are 

[PATCH RFC v2 03/11] ARM: dts: omap3: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entries to base OMAP3 dtsi files.

Signed-off-by: Andrew Davis 
---
 arch/arm/boot/dts/ti/omap/am3517.dtsi   | 11 ++-
 arch/arm/boot/dts/ti/omap/omap34xx.dtsi | 11 ++-
 arch/arm/boot/dts/ti/omap/omap36xx.dtsi |  9 +
 3 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/arch/arm/boot/dts/ti/omap/am3517.dtsi 
b/arch/arm/boot/dts/ti/omap/am3517.dtsi
index 77e58e686fb17..19aad715dff70 100644
--- a/arch/arm/boot/dts/ti/omap/am3517.dtsi
+++ b/arch/arm/boot/dts/ti/omap/am3517.dtsi
@@ -162,12 +162,13 @@ sgx_module: target-module@5000 {
clock-names = "fck", "ick";
#address-cells = <1>;
#size-cells = <1>;
-   ranges = <0 0x5000 0x4000>;
+   ranges = <0 0x5000 0x1>;
 
-   /*
-* Closed source PowerVR driver, no child device
-* binding or driver in mainline
-*/
+   gpu@0 {
+   compatible = "ti,omap3430-gpu", 
"img,powervr-sgx530";
+   reg = <0x0 0x1>; /* 64kB */
+   interrupts = <21>;
+   };
};
};
 };
diff --git a/arch/arm/boot/dts/ti/omap/omap34xx.dtsi 
b/arch/arm/boot/dts/ti/omap/omap34xx.dtsi
index fc7233ac183a8..acdd0ee34421d 100644
--- a/arch/arm/boot/dts/ti/omap/omap34xx.dtsi
+++ b/arch/arm/boot/dts/ti/omap/omap34xx.dtsi
@@ -164,12 +164,13 @@ sgx_module: target-module@5000 {
clock-names = "fck", "ick";
#address-cells = <1>;
#size-cells = <1>;
-   ranges = <0 0x5000 0x4000>;
+   ranges = <0 0x5000 0x1>;
 
-   /*
-* Closed source PowerVR driver, no child device
-* binding or driver in mainline
-*/
+   gpu@0 {
+   compatible = "ti,omap3430-gpu", 
"img,powervr-sgx530";
+   reg = <0x0 0x1>; /* 64kB */
+   interrupts = <21>;
+   };
};
};
 
diff --git a/arch/arm/boot/dts/ti/omap/omap36xx.dtsi 
b/arch/arm/boot/dts/ti/omap/omap36xx.dtsi
index e6d8070c1bf88..c3d79ecd56e39 100644
--- a/arch/arm/boot/dts/ti/omap/omap36xx.dtsi
+++ b/arch/arm/boot/dts/ti/omap/omap36xx.dtsi
@@ -211,10 +211,11 @@ sgx_module: target-module@5000 {
#size-cells = <1>;
ranges = <0 0x5000 0x200>;
 
-   /*
-* Closed source PowerVR driver, no child device
-* binding or driver in mainline
-*/
+   gpu@0 {
+   compatible = "ti,omap3630-gpu", 
"img,powervr-sgx530";
+   reg = <0x0 0x200>; /* 32MB */
+   interrupts = <21>;
+   };
};
};
 
-- 
2.39.2

[PATCH RFC v2 00/11] Device tree support for Imagination Series5 GPU

[Andrew Davis]

Hello all,

I know this has been tried before[0], but given the recent upstreaming of
the Series6+ GPU bindings I figured it might be time to give the Series5
bindings another try.

While there is currently no mainline driver for these binding, there is an
open source out-of-tree kernel-side driver available[1]. Having a stable
and upstream binding for these devices allows us to describe this hardware
in device tree.

This is my vision for how these bindings should look, along with some
example uses in several SoC DT files. The compatible names have been
updated to match what was decided on for Series6+, but otherwise most
is the same as we have been using in our vendor tree for many years.

Thanks,
Andrew

Based on next-20240108.

[0]: https://lkml.org/lkml/2020/4/24/1222
[1]: https://github.com/openpvrsgx-devgroup

Changes for RFC v2:
 - Added patch to rename Rogue+ binding to img,powervr-rogue.yaml
 - Locked all property item counts
 - Removed nodename pattern check

Andrew Davis (11):
  dt-bindings: gpu: Rename img,powervr to img,powervr-rogue
  dt-bindings: gpu: Add PowerVR Series5 SGX GPUs
  ARM: dts: omap3: Add device tree entry for SGX GPU
  ARM: dts: omap4: Add device tree entry for SGX GPU
  ARM: dts: omap5: Add device tree entry for SGX GPU
  ARM: dts: AM33xx: Add device tree entry for SGX GPU
  ARM: dts: AM437x: Add device tree entry for SGX GPU
  ARM: dts: DRA7xx: Add device tree entry for SGX GPU
  arm64: dts: ti: k3-am654-main: Add device tree entry for SGX GPU
  ARM: dts: sun6i: Add device tree entry for SGX GPU
  MIPS: DTS: jz4780: Add device tree entry for SGX GPU

 ...mg,powervr.yaml => img,powervr-rogue.yaml} |   4 +-
 .../bindings/gpu/img,powervr-sgx.yaml | 124 ++
 MAINTAINERS   |   3 +-
 arch/arm/boot/dts/allwinner/sun6i-a31.dtsi|   9 ++
 arch/arm/boot/dts/ti/omap/am33xx.dtsi |   9 +-
 arch/arm/boot/dts/ti/omap/am3517.dtsi |  11 +-
 arch/arm/boot/dts/ti/omap/am4372.dtsi |   6 +
 arch/arm/boot/dts/ti/omap/dra7.dtsi   |   9 +-
 arch/arm/boot/dts/ti/omap/omap34xx.dtsi   |  11 +-
 arch/arm/boot/dts/ti/omap/omap36xx.dtsi   |   9 +-
 arch/arm/boot/dts/ti/omap/omap4.dtsi  |   9 +-
 arch/arm/boot/dts/ti/omap/omap5.dtsi  |   9 +-
 arch/arm64/boot/dts/ti/k3-am65-main.dtsi  |   7 +
 arch/mips/boot/dts/ingenic/jz4780.dtsi|  11 ++
 14 files changed, 201 insertions(+), 30 deletions(-)
 rename Documentation/devicetree/bindings/gpu/{img,powervr.yaml => 
img,powervr-rogue.yaml} (91%)
 create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml

-- 
2.39.2

[PATCH RFC v2 04/11] ARM: dts: omap4: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base OMAP4 dtsi file.

Signed-off-by: Andrew Davis 
---
 arch/arm/boot/dts/ti/omap/omap4.dtsi | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/ti/omap/omap4.dtsi 
b/arch/arm/boot/dts/ti/omap/omap4.dtsi
index 2bbff9032be3e..559b2bfe4ca7c 100644
--- a/arch/arm/boot/dts/ti/omap/omap4.dtsi
+++ b/arch/arm/boot/dts/ti/omap/omap4.dtsi
@@ -501,10 +501,11 @@ sgx_module: target-module@5600 {
#size-cells = <1>;
ranges = <0 0x5600 0x200>;
 
-   /*
-* Closed source PowerVR driver, no child device
-* binding or driver in mainline
-*/
+   gpu@0 {
+   compatible = "ti,omap4430-gpu", 
"img,powervr-sgx540";
+   reg = <0x0 0x200>; /* 32MB */
+   interrupts = ;
+   };
};
 
/*
-- 
2.39.2

[PATCH RFC v2 11/11] MIPS: DTS: jz4780: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base jz4780 dtsi file.

Signed-off-by: Andrew Davis 
---
 arch/mips/boot/dts/ingenic/jz4780.dtsi | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi 
b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 18a85ce38..5ea6833f5e872 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -460,6 +460,17 @@ hdmi: hdmi@1018 {
status = "disabled";
};
 
+   gpu: gpu@1304 {
+   compatible = "ingenic,jz4780-gpu", "img,powervr-sgx540";
+   reg = <0x1304 0x4000>;
+
+   clocks = < JZ4780_CLK_GPU>;
+   clock-names = "core";
+
+   interrupt-parent = <>;
+   interrupts = <63>;
+   };
+
lcdc0: lcdc0@1305 {
compatible = "ingenic,jz4780-lcd";
reg = <0x1305 0x1800>;
-- 
2.39.2

[PATCH RFC v2 08/11] ARM: dts: DRA7xx: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base DRA7x dtsi file.

Signed-off-by: Andrew Davis 
---
 arch/arm/boot/dts/ti/omap/dra7.dtsi | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/ti/omap/dra7.dtsi 
b/arch/arm/boot/dts/ti/omap/dra7.dtsi
index 6509c742fb58c..8527643cb69a8 100644
--- a/arch/arm/boot/dts/ti/omap/dra7.dtsi
+++ b/arch/arm/boot/dts/ti/omap/dra7.dtsi
@@ -850,12 +850,19 @@ target-module@5600 {
;
ti,sysc-sidle = ,
,
-   ;
+   ,
+   ;
clocks = <_clkctrl DRA7_GPU_CLKCTRL 0>;
clock-names = "fck";
#address-cells = <1>;
#size-cells = <1>;
ranges = <0 0x5600 0x200>;
+
+   gpu@0 {
+   compatible = "ti,am5728-gpu", 
"img,powervr-sgx544";
+   reg = <0x0 0x1>; /* 64kB */
+   interrupts = ;
+   };
};
 
crossbar_mpu: crossbar@4a002a48 {
-- 
2.39.2

[PATCH RFC v2 06/11] ARM: dts: AM33xx: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base AM33xx dtsi file.

Signed-off-by: Andrew Davis 
---
 arch/arm/boot/dts/ti/omap/am33xx.dtsi | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/ti/omap/am33xx.dtsi 
b/arch/arm/boot/dts/ti/omap/am33xx.dtsi
index 5b9e01a8aa5d5..989d5a6edeed9 100644
--- a/arch/arm/boot/dts/ti/omap/am33xx.dtsi
+++ b/arch/arm/boot/dts/ti/omap/am33xx.dtsi
@@ -640,10 +640,11 @@ target-module@5600 {
#size-cells = <1>;
ranges = <0 0x5600 0x100>;
 
-   /*
-* Closed source PowerVR driver, no child device
-* binding or driver in mainline
-*/
+   gpu@0 {
+   compatible = "ti,omap3630-gpu", 
"img,powervr-sgx530";
+   reg = <0x0 0x1>; /* 64kB */
+   interrupts = <37>;
+   };
};
};
 };
-- 
2.39.2

[PATCH RFC v2 09/11] arm64: dts: ti: k3-am654-main: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base AM654 dtsi file.

Signed-off-by: Andrew Davis 
---
 arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi 
b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
index fcea544656360..64b52c8dafc6c 100644
--- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
@@ -1050,6 +1050,13 @@ dss_ports: ports {
};
};
 
+   gpu: gpu@700 {
+   compatible = "ti,am6548-gpu", "img,powervr-sgx544";
+   reg = <0x0 0x700 0x0 0x1>;
+   interrupts = ;
+   power-domains = <_pds 65 TI_SCI_PD_EXCLUSIVE>;
+   };
+
ehrpwm0: pwm@300 {
compatible = "ti,am654-ehrpwm", "ti,am3352-ehrpwm";
#pwm-cells = <3>;
-- 
2.39.2

[PATCH RFC v2 10/11] ARM: dts: sun6i: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base sun6i-a31 dtsi file.

Signed-off-by: Andrew Davis 
---
 arch/arm/boot/dts/allwinner/sun6i-a31.dtsi | 9 +
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/allwinner/sun6i-a31.dtsi 
b/arch/arm/boot/dts/allwinner/sun6i-a31.dtsi
index 5cce4918f84c9..e6998783b89aa 100644
--- a/arch/arm/boot/dts/allwinner/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/allwinner/sun6i-a31.dtsi
@@ -962,6 +962,15 @@ mdio: mdio {
};
};
 
+   gpu: gpu@1c4 {
+   compatible = "allwinner,sun6i-a31-gpu", 
"img,powervr-sgx544";
+   reg = <0x01c4 0x1>;
+   interrupts = ;
+   clocks = < CLK_GPU_CORE>, < CLK_GPU_MEMORY>;
+   clock-names = "core", "mem";
+   status = "disabled";
+   };
+
crypto: crypto-engine@1c15000 {
compatible = "allwinner,sun6i-a31-crypto",
 "allwinner,sun4i-a10-crypto";
-- 
2.39.2

[PATCH RFC v2 01/11] dt-bindings: gpu: Rename img, powervr to img, powervr-rogue

[Andrew Davis]

Signed-off-by: Andrew Davis 
---
 .../bindings/gpu/{img,powervr.yaml => img,powervr-rogue.yaml} | 4 ++--
 MAINTAINERS   | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
 rename Documentation/devicetree/bindings/gpu/{img,powervr.yaml => 
img,powervr-rogue.yaml} (91%)

diff --git a/Documentation/devicetree/bindings/gpu/img,powervr.yaml 
b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
similarity index 91%
rename from Documentation/devicetree/bindings/gpu/img,powervr.yaml
rename to Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
index a13298f1a1827..03a8308b41ae7 100644
--- a/Documentation/devicetree/bindings/gpu/img,powervr.yaml
+++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
@@ -2,10 +2,10 @@
 # Copyright (c) 2023 Imagination Technologies Ltd.
 %YAML 1.2
 ---
-$id: http://devicetree.org/schemas/gpu/img,powervr.yaml#
+$id: http://devicetree.org/schemas/gpu/img,powervr-rogue.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Imagination Technologies PowerVR and IMG GPU
+title: Imagination Technologies PowerVR Rogue and IMG GPUs
 
 maintainers:
   - Frank Binns 
diff --git a/MAINTAINERS b/MAINTAINERS
index fa67e2624723f..5b205795da04e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10461,7 +10461,7 @@ M:  Donald Robson 
 M: Matt Coster 
 S: Supported
 T: git git://anongit.freedesktop.org/drm/drm-misc
-F: Documentation/devicetree/bindings/gpu/img,powervr.yaml
+F: Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
 F: Documentation/gpu/imagination/
 F: drivers/gpu/drm/imagination/
 F: include/uapi/drm/pvr_drm.h
-- 
2.39.2

[PATCH RFC v2 02/11] dt-bindings: gpu: Add PowerVR Series5 SGX GPUs

[Andrew Davis]

The Imagination PowerVR Series5 "SGX" GPU is part of several SoCs from
multiple vendors. Describe how the SGX GPU is integrated in these SoC,
including register space and interrupts. Clocks, reset, and power domain
information is SoC specific.

Signed-off-by: Andrew Davis 
---
 .../bindings/gpu/img,powervr-sgx.yaml | 124 ++
 MAINTAINERS   |   1 +
 2 files changed, 125 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml

diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml 
b/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml
new file mode 100644
index 0..bb821e1184de9
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml
@@ -0,0 +1,124 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (c) 2023 Imagination Technologies Ltd.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpu/img,powervr-sgx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Imagination Technologies PowerVR SGX GPUs
+
+maintainers:
+  - Frank Binns 
+
+properties:
+  compatible:
+oneOf:
+  - items:
+  - enum:
+  - ti,omap3430-gpu # Rev 121
+  - ti,omap3630-gpu # Rev 125
+  - const: img,powervr-sgx530
+  - items:
+  - enum:
+  - ingenic,jz4780-gpu # Rev 130
+  - ti,omap4430-gpu # Rev 120
+  - const: img,powervr-sgx540
+  - items:
+  - enum:
+  - allwinner,sun6i-a31-gpu # MP2 Rev 115
+  - ti,omap4470-gpu # MP1 Rev 112
+  - ti,omap5432-gpu # MP2 Rev 105
+  - ti,am5728-gpu # MP2 Rev 116
+  - ti,am6548-gpu # MP1 Rev 117
+  - const: img,powervr-sgx544
+
+  reg:
+maxItems: 1
+
+  interrupts:
+maxItems: 1
+
+  clocks: true
+
+  clock-names:
+minItems: 1
+items:
+  - const: core
+  - const: mem
+  - const: sys
+
+  power-domains:
+maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+additionalProperties: false
+
+allOf:
+  - if:
+  properties:
+compatible:
+  contains:
+const: ti,am6548-gpu
+then:
+  required:
+- power-domains
+else:
+  properties:
+power-domains: false
+  - if:
+  properties:
+compatible:
+  contains:
+enum:
+  - allwinner,sun6i-a31-gpu
+  - ingenic,jz4780-gpu
+then:
+  allOf:
+- if:
+properties:
+  compatible:
+contains:
+  const: allwinner,sun6i-a31-gpu
+  then:
+properties:
+  clocks:
+minItems: 2
+maxItems: 2
+  clock-names:
+minItems: 2
+maxItems: 2
+- if:
+properties:
+  compatible:
+contains:
+  const: ingenic,jz4780-gpu
+  then:
+properties:
+  clocks:
+maxItems: 1
+  clock-names:
+maxItems: 1
+  required:
+- clocks
+- clock-names
+else:
+  properties:
+clocks: false
+clock-names: false
+
+examples:
+  - |
+#include 
+#include 
+#include 
+
+gpu@700 {
+compatible = "ti,am6548-gpu", "img,powervr-sgx544";
+reg = <0x700 0x1>;
+interrupts = ;
+power-domains = <_pds 65 TI_SCI_PD_EXCLUSIVE>;
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index 5b205795da04e..00ba13e019fa6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10462,6 +10462,7 @@ M:  Matt Coster 
 S: Supported
 T: git git://anongit.freedesktop.org/drm/drm-misc
 F: Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
+F: Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml
 F: Documentation/gpu/imagination/
 F: drivers/gpu/drm/imagination/
 F: include/uapi/drm/pvr_drm.h
-- 
2.39.2

[PATCH RFC v2 05/11] ARM: dts: omap5: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base OMAP5 dtsi file.

Signed-off-by: Andrew Davis 
---
 arch/arm/boot/dts/ti/omap/omap5.dtsi | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/ti/omap/omap5.dtsi 
b/arch/arm/boot/dts/ti/omap/omap5.dtsi
index bac6fa8387936..6a66214ad0e2f 100644
--- a/arch/arm/boot/dts/ti/omap/omap5.dtsi
+++ b/arch/arm/boot/dts/ti/omap/omap5.dtsi
@@ -453,10 +453,11 @@ target-module@5600 {
#size-cells = <1>;
ranges = <0 0x5600 0x200>;
 
-   /*
-* Closed source PowerVR driver, no child device
-* binding or driver in mainline
-*/
+   gpu@0 {
+   compatible = "ti,omap5432-gpu", 
"img,powervr-sgx544";
+   reg = <0x0 0x200>; /* 32MB */
+   interrupts = ;
+   };
};
 
target-module@5800 {
-- 
2.39.2

[PATCH RFC v2 07/11] ARM: dts: AM437x: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base AM437x dtsi file.

Signed-off-by: Andrew Davis 
---
 arch/arm/boot/dts/ti/omap/am4372.dtsi | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/ti/omap/am4372.dtsi 
b/arch/arm/boot/dts/ti/omap/am4372.dtsi
index 9d2c064534f7d..5fd1b380ece62 100644
--- a/arch/arm/boot/dts/ti/omap/am4372.dtsi
+++ b/arch/arm/boot/dts/ti/omap/am4372.dtsi
@@ -719,6 +719,12 @@ target-module@5600 {
#address-cells = <1>;
#size-cells = <1>;
ranges = <0 0x5600 0x100>;
+
+   gpu@0 {
+   compatible = "ti,omap3630-gpu", 
"img,powervr-sgx530";
+   reg = <0x0 0x1>; /* 64kB */
+   interrupts = ;
+   };
};
};
 };
-- 
2.39.2

Re: [PATCH RFC 01/10] dt-bindings: gpu: Add PowerVR Series5 SGX GPUs

[Andrew Davis]

On 12/18/23 4:54 AM, H. Nikolaus Schaller wrote:




Am 18.12.2023 um 11:14 schrieb Maxime Ripard :

On Mon, Dec 18, 2023 at 10:28:09AM +0100, H. Nikolaus Schaller wrote:

Hi Maxime,


Am 15.12.2023 um 14:33 schrieb Maxime Ripard :



It's for a separate architecture, with a separate driver, maintained out
of tree by a separate community, with a separate set of requirements as
evidenced by the other thread. And that's all fine in itself, but
there's very little reason to put these two bindings in the same file.

We could also turn this around, why is it important that it's in the
same file?


Same vendor. And enough similarity in architectures, even a logical sequence
of development of versions (SGX = Version 5, Rogue = Version 6+) behind.
(SGX and Rogue seem to be just trade names for their architecture development).


Again, none of that matters for *where* the binding is stored.


So what then speaks against extending the existing bindings file as proposed
here?


I mean, apart from everything you quoted, then sure, nothing speaks
against it.


AFAIK bindings should describe hardware and not communities or drivers
or who is currently maintaining it. The latter can change, the first not.


Bindings are supposed to describe hardware indeed. Nothing was ever said
about where those bindings are supposed to be located.

There's hundreds of other YAML bindings describing devices of the same
vendors and different devices from the same generation.


Usually SoC seem to be split over multiple files by subsystem. Not by versions
or generations. If the subsystems are similar enough they share the same 
bindings
doc instead of having one for each generation duplicating a lot of code.

Here is a comparable example that combines multiple vendors and generations:

Documentation/devicetree/bindings/usb/generic-ehci.yaml


EHCI is a single interface for USB2.0 controllers. It's a standard API,
and is made of a single driver that requires minor modifications to deal
with multiple devices.

We're very far from the same situation here.


How far are we really? And, it is the purpose of the driver to handle different 
cases.

That there are currently two drivers is just a matter of history and not a 
necessity.




If anything it'll make it easier for you. I'm really not sure why it is
controversial and you're fighting this so hard.


Well, you made it controversial by proposing to split what IMHO belongs 
together.


No, reviews aren't controversial.
The controversy started when you chose
to oppose it while you could have just rolled with it.


Well, you asked

"I think it would be best to have a separate file for this, img,sgx.yaml
maybe?"

and

"Because it's more convenient?"

I understood that as an invitation for discussing the pros and cons and working 
out the
most convenient solution. And that involves playing the devil's advocate which 
of course
is controversial by principle.

Now, IMHO all the pros and cons are on the table and the question is who makes 
a decision
how to go.



As much as I would land on the side of same file for both, the answer to this 
question
is simple: the maintainer makes the decision :) So I'll respin with separate 
binding files.

The hidden unaddressed issue here is that by making these bindings separate it 
implies
they are not on equal footing (i.e. pre-series6 GPUs are not true "powervr" and 
so do not
belong in img,powervr.yaml). So if no one objects I'd also like to do the 
rename of that
file as suggested before and have:

img,powervr-sgx.yaml
img,powervr-rogue.yaml




I feel that the original patch is good enough for its purpose and follows
some design pattern that can be deduced from other binding docs.


[citation needed]


Joke: Documentation/devicetree/bindings/* - I am not aware of a formal analysis 
of course.

But see my example for ehci. It follows the pattern I mean. If clocks, regs, 
interrupts,
resets, and more properties are (almost) the same, then group them and just 
differentiate
by different compatible strings. If necessary use some - if: clauses.

It is the task of drivers to handle the details.

As my other (maybe more important) comment to this patch did indicate we IMHO 
can easily
live with something like

+  - items:
+  - enum:
+  - ti,am62-gpu # IMG AXE GPU model/revision is fully discoverable
+  - ti,omap3430-gpu # sgx530 Rev 121
+  - ti,omap3630-gpu # sgx530 Rev 125
+  - ingenic,jz4780-gpu # sgx540 Rev 130
+  - ti,omap4430-gpu # sgx540 Rev 120
+  - allwinner,sun6i-a31-gpu # sgx544 MP2 Rev 115
+  - ti,omap4470-gpu # sgx544 MP1 Rev 112
+  - ti,omap5432-gpu # sgx544 MP2 Rev 105
+  - ti,am5728-gpu # sgx544 MP2 Rev 116
+  - ti,am6548-gpu # sgx544 MP1 Rev 117



While we could live with this, the "compatible" groupings makes life just a bit
easier. This is true really for any DT compatible string and is not based on
any 

Re: [PATCH RFC 01/10] dt-bindings: gpu: Add PowerVR Series5 SGX GPUs

[Andrew Davis]

On 12/6/23 10:02 AM, Conor Dooley wrote:

On Tue, Dec 05, 2023 at 07:04:05PM +0100, H. Nikolaus Schaller wrote:

Am 05.12.2023 um 18:33 schrieb Andrew Davis :

On 12/5/23 2:17 AM, H. Nikolaus Schaller wrote:

+  - enum:
+  - ti,omap3430-gpu # Rev 121
+  - ti,omap3630-gpu # Rev 125

Is the "Rev 121" and "Rev 125" a property of the SoC integration 
(clock/reset/power
hookup etc.) or of the integrated SGX core?


The Rev is a property of the SGX core, not the SoC integration.


Then, it should belong there and not be a comment of the ti,omap*-gpu record.
In this way it does not seem to be a proper hardware description.

BTW: there are examples where the revision is part of the compatible string, 
even
if the (Linux) driver makes no use of it:

drivers/net/ethernet/xilinx/xilinx_emaclite.c


AFAICT these Xilinx devices that put the revisions in the compatible are
a different case - they're "soft" IP intended for use in the fabric of
an FPGA, and assigning a device specific compatible there does not make
sense.

In this case it appears that the revision is completely known once you
see "ti,omap3630-gpu", so encoding the extra "121" into the compatible
string is not required.




But it seems that
compatible string is being used to define both (as we see being debated in the 
other
thread on this series).


In my understanding the Revs are different variants of the SGX core (errata
fixes, instruction set, pipeline size etc.). And therefore the current driver 
code
has to be configured by some macros to handle such cases.
So the Rev should IMHO be part of the next line:

+  - const: img,powervr-sgx530

+  - enum:
+  - img,powervr-sgx530-121
+  - img,powervr-sgx530-125
We have a similar definition in the openpvrsgx code.
Example: compatible = "ti,omap3-sgx530-121", "img,sgx530-121", "img,sgx530";
(I don't mind about the powervr- prefix).
This would allow a generic and universal sgx driver (loaded through just 
matching
"img,sgx530") to handle the errata and revision specifics at runtime based on 
the
compatible entry ("img,sgx530-121") and know about SoC integration 
("ti,omap3-sgx530-121").


The "raw" sgx530 compatible does not seem helpful if the sgx530-121 or
sgx530-125 compatibles are also required to be present for the driver to
actually function. The revision specific compatibles I would not object
to, but everything in here has different revisions anyway - does the
same revision actually appear in multiple devices? If it doesn't then I
don't see any value in the suffixed compatibles either.



Everything here has different revisions because any device that uses
the same revision can also use the same base compatible string. For
instance AM335x SoCs use the SGX530 revision 125, same as OMAP3630,
so I simply reuse that compatible in their DT as you can see in
patch [5/10]. I didn't see the need for a new compatible string
for identical (i.e. "compatible") IP and integration.

The first device to use that IP/revision combo gets the named
compatible, all others re-use the same compatible where possible.

Andrew


And user-space can be made to load the right firmware variant based on 
"img,sgx530-121"
I don't know if there is some register which allows to discover the revision 
long
before the SGX subsystem is initialized and the firmware is up and running.
What I know is that it is possible to read out the revision after starting the 
firmware
but it may just echo the version number of the firmware binary provided from 
user-space.


We should be able to read out the revision (register EUR_CR_CORE_REVISION), the 
problem is
today the driver is built for a given revision at compile time.


Yes, that is something we had planned to get rid of for a long time by using 
different compatible
strings and some variant specific struct like many others drivers are doing it.
But it was a to big task so nobody did start with it.


That is a software issue,
not something that we need to encode in DT. While the core ID (SGX5xx) can be 
also detected
(EUR_CR_CORE_ID), the location of that register changes, and so it does need 
encoded in
DT compatible.


Ok, I didn't know about such registers as there is not much public information 
available.
Fair enough, there are some error reports about in different forums.

On the other hand we then must read out this register in more or less early 
initialization
stages. Even if we know this information to be static and it could be as simple 
as a list
of compatible strings in the driver.


The string "ti,omap3430-gpu" tells us the revision if we cannot detect it (as 
in the current
driver), and the SoC integration is generic anyway (just a reg and interrupt).


It of course tells, but may need a translation table that needs to be 
maintained in a
different format

Re: [PATCH RFC 01/10] dt-bindings: gpu: Add PowerVR Series5 SGX GPUs

[Andrew Davis]

On 12/5/23 2:17 AM, H. Nikolaus Schaller wrote:

Hi Andrew,


Am 04.12.2023 um 19:22 schrieb Andrew Davis :

The Imagination PowerVR Series5 "SGX" GPU is part of several SoCs from
multiple vendors.


Great and thanks for the new attempt to get at least the Device Tree side
upstream. Really appreciated!



Thanks for helping us maintain these GPUs with the OpenPVRSGX project :)


Describe how the SGX GPU is integrated in these SoC,
including register space and interrupts.



Clocks, reset, and power domain
information is SoC specific.


Indeed. This makes it understandable why you did not directly
take our scheme from the openpvrsgx project.



Signed-off-by: Andrew Davis 
---
.../devicetree/bindings/gpu/img,powervr.yaml  | 69 +--
1 file changed, 63 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpu/img,powervr.yaml 
b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
index a13298f1a1827..9f036891dad0b 100644
--- a/Documentation/devicetree/bindings/gpu/img,powervr.yaml
+++ b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
@@ -11,11 +11,33 @@ maintainers:
   - Frank Binns 

properties:
+  $nodename:
+pattern: '^gpu@[a-f0-9]+$'
+
   compatible:
-items:
-  - enum:
-  - ti,am62-gpu
-  - const: img,img-axe # IMG AXE GPU model/revision is fully discoverable
+oneOf:
+  - items:
+  - enum:
+  - ti,am62-gpu
+  - const: img,img-axe # IMG AXE GPU model/revision is fully 
discoverable
+  - items:
+  - enum:
+  - ti,omap3430-gpu # Rev 121
+  - ti,omap3630-gpu # Rev 125


Is the "Rev 121" and "Rev 125" a property of the SoC integration 
(clock/reset/power
hookup etc.) or of the integrated SGX core?



The Rev is a property of the SGX core, not the SoC integration. But it seems 
that
compatible string is being used to define both (as we see being debated in the 
other
thread on this series).


In my understanding the Revs are different variants of the SGX core (errata
fixes, instruction set, pipeline size etc.). And therefore the current driver 
code
has to be configured by some macros to handle such cases.

So the Rev should IMHO be part of the next line:


+  - const: img,powervr-sgx530


+  - enum:
+  - img,powervr-sgx530-121
+  - img,powervr-sgx530-125

We have a similar definition in the openpvrsgx code.
Example: compatible = "ti,omap3-sgx530-121", "img,sgx530-121", "img,sgx530";

(I don't mind about the powervr- prefix).

This would allow a generic and universal sgx driver (loaded through just 
matching
"img,sgx530") to handle the errata and revision specifics at runtime based on 
the
compatible entry ("img,sgx530-121") and know about SoC integration 
("ti,omap3-sgx530-121").

And user-space can be made to load the right firmware variant based on 
"img,sgx530-121"

I don't know if there is some register which allows to discover the revision 
long
before the SGX subsystem is initialized and the firmware is up and running.

What I know is that it is possible to read out the revision after starting the 
firmware
but it may just echo the version number of the firmware binary provided from 
user-space.



We should be able to read out the revision (register EUR_CR_CORE_REVISION), the 
problem is
today the driver is built for a given revision at compile time. That is a 
software issue,
not something that we need to encode in DT. While the core ID (SGX5xx) can be 
also detected
(EUR_CR_CORE_ID), the location of that register changes, and so it does need 
encoded in
DT compatible.

The string "ti,omap3430-gpu" tells us the revision if we cannot detect it (as 
in the current
driver), and the SoC integration is generic anyway (just a reg and interrupt).

Andrew


+  - items:
+  - enum:
+  - ingenic,jz4780-gpu # Rev 130
+  - ti,omap4430-gpu # Rev 120
+  - const: img,powervr-sgx540
+  - items:
+  - enum:
+  - allwinner,sun6i-a31-gpu # MP2 Rev 115
+  - ti,omap4470-gpu # MP1 Rev 112
+  - ti,omap5432-gpu # MP2 Rev 105
+  - ti,am5728-gpu # MP2 Rev 116
+  - ti,am6548-gpu # MP1 Rev 117
+  - const: img,powervr-sgx544

   reg:
 maxItems: 1
@@ -40,8 +62,6 @@ properties:
required:
   - compatible
   - reg
-  - clocks
-  - clock-names
   - interrupts

additionalProperties: false
@@ -56,6 +76,43 @@ allOf:
   properties:
 clocks:
   maxItems: 1
+  required:
+- clocks
+- clock-names
+  - if:
+  properties:
+compatible:
+  contains:
+const: ti,am654-sgx544
+then:
+  properties:
+power-domains:
+  minItems: 1
+  required:
+- power-domains
+  - if:
+  properties:
+compatible:
+  contains:
+  

[PATCH RFC 02/10] ARM: dts: omap3: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entries to base OMAP3 dtsi files.

Signed-off-by: Andrew Davis 
---
 arch/arm/boot/dts/ti/omap/am3517.dtsi   | 11 ++-
 arch/arm/boot/dts/ti/omap/omap34xx.dtsi | 11 ++-
 arch/arm/boot/dts/ti/omap/omap36xx.dtsi |  9 +
 3 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/arch/arm/boot/dts/ti/omap/am3517.dtsi 
b/arch/arm/boot/dts/ti/omap/am3517.dtsi
index 77e58e686fb17..19aad715dff70 100644
--- a/arch/arm/boot/dts/ti/omap/am3517.dtsi
+++ b/arch/arm/boot/dts/ti/omap/am3517.dtsi
@@ -162,12 +162,13 @@ sgx_module: target-module@5000 {
clock-names = "fck", "ick";
#address-cells = <1>;
#size-cells = <1>;
-   ranges = <0 0x5000 0x4000>;
+   ranges = <0 0x5000 0x1>;
 
-   /*
-* Closed source PowerVR driver, no child device
-* binding or driver in mainline
-*/
+   gpu@0 {
+   compatible = "ti,omap3430-gpu", 
"img,powervr-sgx530";
+   reg = <0x0 0x1>; /* 64kB */
+   interrupts = <21>;
+   };
};
};
 };
diff --git a/arch/arm/boot/dts/ti/omap/omap34xx.dtsi 
b/arch/arm/boot/dts/ti/omap/omap34xx.dtsi
index fc7233ac183a8..acdd0ee34421d 100644
--- a/arch/arm/boot/dts/ti/omap/omap34xx.dtsi
+++ b/arch/arm/boot/dts/ti/omap/omap34xx.dtsi
@@ -164,12 +164,13 @@ sgx_module: target-module@5000 {
clock-names = "fck", "ick";
#address-cells = <1>;
#size-cells = <1>;
-   ranges = <0 0x5000 0x4000>;
+   ranges = <0 0x5000 0x1>;
 
-   /*
-* Closed source PowerVR driver, no child device
-* binding or driver in mainline
-*/
+   gpu@0 {
+   compatible = "ti,omap3430-gpu", 
"img,powervr-sgx530";
+   reg = <0x0 0x1>; /* 64kB */
+   interrupts = <21>;
+   };
};
};
 
diff --git a/arch/arm/boot/dts/ti/omap/omap36xx.dtsi 
b/arch/arm/boot/dts/ti/omap/omap36xx.dtsi
index e6d8070c1bf88..c3d79ecd56e39 100644
--- a/arch/arm/boot/dts/ti/omap/omap36xx.dtsi
+++ b/arch/arm/boot/dts/ti/omap/omap36xx.dtsi
@@ -211,10 +211,11 @@ sgx_module: target-module@5000 {
#size-cells = <1>;
ranges = <0 0x5000 0x200>;
 
-   /*
-* Closed source PowerVR driver, no child device
-* binding or driver in mainline
-*/
+   gpu@0 {
+   compatible = "ti,omap3630-gpu", 
"img,powervr-sgx530";
+   reg = <0x0 0x200>; /* 32MB */
+   interrupts = <21>;
+   };
};
};
 
-- 
2.39.2

[PATCH RFC 09/10] ARM: dts: sun6i: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base sun6i-a31 dtsi file.

Signed-off-by: Andrew Davis 
---
 arch/arm/boot/dts/allwinner/sun6i-a31.dtsi | 9 +
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/allwinner/sun6i-a31.dtsi 
b/arch/arm/boot/dts/allwinner/sun6i-a31.dtsi
index 5cce4918f84c9..e6998783b89aa 100644
--- a/arch/arm/boot/dts/allwinner/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/allwinner/sun6i-a31.dtsi
@@ -962,6 +962,15 @@ mdio: mdio {
};
};
 
+   gpu: gpu@1c4 {
+   compatible = "allwinner,sun6i-a31-gpu", 
"img,powervr-sgx544";
+   reg = <0x01c4 0x1>;
+   interrupts = ;
+   clocks = < CLK_GPU_CORE>, < CLK_GPU_MEMORY>;
+   clock-names = "core", "mem";
+   status = "disabled";
+   };
+
crypto: crypto-engine@1c15000 {
compatible = "allwinner,sun6i-a31-crypto",
 "allwinner,sun4i-a10-crypto";
-- 
2.39.2

[PATCH RFC 03/10] ARM: dts: omap4: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base OMAP4 dtsi file.

Signed-off-by: Andrew Davis 
---
 arch/arm/boot/dts/ti/omap/omap4.dtsi | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/ti/omap/omap4.dtsi 
b/arch/arm/boot/dts/ti/omap/omap4.dtsi
index 2bbff9032be3e..559b2bfe4ca7c 100644
--- a/arch/arm/boot/dts/ti/omap/omap4.dtsi
+++ b/arch/arm/boot/dts/ti/omap/omap4.dtsi
@@ -501,10 +501,11 @@ sgx_module: target-module@5600 {
#size-cells = <1>;
ranges = <0 0x5600 0x200>;
 
-   /*
-* Closed source PowerVR driver, no child device
-* binding or driver in mainline
-*/
+   gpu@0 {
+   compatible = "ti,omap4430-gpu", 
"img,powervr-sgx540";
+   reg = <0x0 0x200>; /* 32MB */
+   interrupts = ;
+   };
};
 
/*
-- 
2.39.2

[PATCH RFC 10/10] MIPS: DTS: jz4780: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base jz4780 dtsi file.

Signed-off-by: Andrew Davis 
---
 arch/mips/boot/dts/ingenic/jz4780.dtsi | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi 
b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 18a85ce38..5ea6833f5e872 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -460,6 +460,17 @@ hdmi: hdmi@1018 {
status = "disabled";
};
 
+   gpu: gpu@1304 {
+   compatible = "ingenic,jz4780-gpu", "img,powervr-sgx540";
+   reg = <0x1304 0x4000>;
+
+   clocks = < JZ4780_CLK_GPU>;
+   clock-names = "core";
+
+   interrupt-parent = <>;
+   interrupts = <63>;
+   };
+
lcdc0: lcdc0@1305 {
compatible = "ingenic,jz4780-lcd";
reg = <0x1305 0x1800>;
-- 
2.39.2

[PATCH RFC 06/10] ARM: dts: AM437x: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base AM437x dtsi file.

Signed-off-by: Andrew Davis 
---
 arch/arm/boot/dts/ti/omap/am4372.dtsi | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/ti/omap/am4372.dtsi 
b/arch/arm/boot/dts/ti/omap/am4372.dtsi
index 9d2c064534f7d..5fd1b380ece62 100644
--- a/arch/arm/boot/dts/ti/omap/am4372.dtsi
+++ b/arch/arm/boot/dts/ti/omap/am4372.dtsi
@@ -719,6 +719,12 @@ target-module@5600 {
#address-cells = <1>;
#size-cells = <1>;
ranges = <0 0x5600 0x100>;
+
+   gpu@0 {
+   compatible = "ti,omap3630-gpu", 
"img,powervr-sgx530";
+   reg = <0x0 0x1>; /* 64kB */
+   interrupts = ;
+   };
};
};
 };
-- 
2.39.2

[PATCH RFC 04/10] ARM: dts: omap5: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base OMAP5 dtsi file.

Signed-off-by: Andrew Davis 
---
 arch/arm/boot/dts/ti/omap/omap5.dtsi | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/ti/omap/omap5.dtsi 
b/arch/arm/boot/dts/ti/omap/omap5.dtsi
index bac6fa8387936..6a66214ad0e2f 100644
--- a/arch/arm/boot/dts/ti/omap/omap5.dtsi
+++ b/arch/arm/boot/dts/ti/omap/omap5.dtsi
@@ -453,10 +453,11 @@ target-module@5600 {
#size-cells = <1>;
ranges = <0 0x5600 0x200>;
 
-   /*
-* Closed source PowerVR driver, no child device
-* binding or driver in mainline
-*/
+   gpu@0 {
+   compatible = "ti,omap5432-gpu", 
"img,powervr-sgx544";
+   reg = <0x0 0x200>; /* 32MB */
+   interrupts = ;
+   };
};
 
target-module@5800 {
-- 
2.39.2

[PATCH RFC 08/10] arm64: dts: ti: k3-am654-main: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base AM654 dtsi file.

Signed-off-by: Andrew Davis 
---
 arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi 
b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
index 29048d6577cf6..d3431aca41026 100644
--- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
@@ -1044,6 +1044,13 @@ dss_ports: ports {
};
};
 
+   gpu: gpu@700 {
+   compatible = "ti,am6548-gpu", "img,powervr-sgx544";
+   reg = <0x0 0x700 0x0 0x1>;
+   interrupts = ;
+   power-domains = <_pds 65 TI_SCI_PD_EXCLUSIVE>;
+   };
+
ehrpwm0: pwm@300 {
compatible = "ti,am654-ehrpwm", "ti,am3352-ehrpwm";
#pwm-cells = <3>;
-- 
2.39.2

[PATCH RFC 05/10] ARM: dts: AM33xx: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base AM33xx dtsi file.

Signed-off-by: Andrew Davis 
---
 arch/arm/boot/dts/ti/omap/am33xx.dtsi | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/ti/omap/am33xx.dtsi 
b/arch/arm/boot/dts/ti/omap/am33xx.dtsi
index 1a2cd5baf4021..1868aef16687f 100644
--- a/arch/arm/boot/dts/ti/omap/am33xx.dtsi
+++ b/arch/arm/boot/dts/ti/omap/am33xx.dtsi
@@ -639,10 +639,11 @@ target-module@5600 {
#size-cells = <1>;
ranges = <0 0x5600 0x100>;
 
-   /*
-* Closed source PowerVR driver, no child device
-* binding or driver in mainline
-*/
+   gpu@0 {
+   compatible = "ti,omap3630-gpu", 
"img,powervr-sgx530";
+   reg = <0x0 0x1>; /* 64kB */
+   interrupts = <37>;
+   };
};
};
 };
-- 
2.39.2

[PATCH RFC 01/10] dt-bindings: gpu: Add PowerVR Series5 SGX GPUs

[Andrew Davis]

The Imagination PowerVR Series5 "SGX" GPU is part of several SoCs from
multiple vendors. Describe how the SGX GPU is integrated in these SoC,
including register space and interrupts. Clocks, reset, and power domain
information is SoC specific.

Signed-off-by: Andrew Davis 
---
 .../devicetree/bindings/gpu/img,powervr.yaml  | 69 +--
 1 file changed, 63 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpu/img,powervr.yaml 
b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
index a13298f1a1827..9f036891dad0b 100644
--- a/Documentation/devicetree/bindings/gpu/img,powervr.yaml
+++ b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
@@ -11,11 +11,33 @@ maintainers:
   - Frank Binns 
 
 properties:
+  $nodename:
+pattern: '^gpu@[a-f0-9]+$'
+
   compatible:
-items:
-  - enum:
-  - ti,am62-gpu
-  - const: img,img-axe # IMG AXE GPU model/revision is fully discoverable
+oneOf:
+  - items:
+  - enum:
+  - ti,am62-gpu
+  - const: img,img-axe # IMG AXE GPU model/revision is fully 
discoverable
+  - items:
+  - enum:
+  - ti,omap3430-gpu # Rev 121
+  - ti,omap3630-gpu # Rev 125
+  - const: img,powervr-sgx530
+  - items:
+  - enum:
+  - ingenic,jz4780-gpu # Rev 130
+  - ti,omap4430-gpu # Rev 120
+  - const: img,powervr-sgx540
+  - items:
+  - enum:
+  - allwinner,sun6i-a31-gpu # MP2 Rev 115
+  - ti,omap4470-gpu # MP1 Rev 112
+  - ti,omap5432-gpu # MP2 Rev 105
+  - ti,am5728-gpu # MP2 Rev 116
+  - ti,am6548-gpu # MP1 Rev 117
+  - const: img,powervr-sgx544
 
   reg:
 maxItems: 1
@@ -40,8 +62,6 @@ properties:
 required:
   - compatible
   - reg
-  - clocks
-  - clock-names
   - interrupts
 
 additionalProperties: false
@@ -56,6 +76,43 @@ allOf:
   properties:
 clocks:
   maxItems: 1
+  required:
+- clocks
+- clock-names
+  - if:
+  properties:
+compatible:
+  contains:
+const: ti,am654-sgx544
+then:
+  properties:
+power-domains:
+  minItems: 1
+  required:
+- power-domains
+  - if:
+  properties:
+compatible:
+  contains:
+const: allwinner,sun6i-a31-gpu
+then:
+  properties:
+clocks:
+  minItems: 2
+clock-names:
+  minItems: 2
+  required:
+- clocks
+- clock-names
+  - if:
+  properties:
+compatible:
+  contains:
+const: ingenic,jz4780-gpu
+then:
+  required:
+- clocks
+- clock-names
 
 examples:
   - |
-- 
2.39.2

[PATCH RFC 07/10] ARM: dts: DRA7xx: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base DRA7x dtsi file.

Signed-off-by: Andrew Davis 
---
 arch/arm/boot/dts/ti/omap/dra7.dtsi | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/ti/omap/dra7.dtsi 
b/arch/arm/boot/dts/ti/omap/dra7.dtsi
index 6509c742fb58c..8527643cb69a8 100644
--- a/arch/arm/boot/dts/ti/omap/dra7.dtsi
+++ b/arch/arm/boot/dts/ti/omap/dra7.dtsi
@@ -850,12 +850,19 @@ target-module@5600 {
;
ti,sysc-sidle = ,
,
-   ;
+   ,
+   ;
clocks = <_clkctrl DRA7_GPU_CLKCTRL 0>;
clock-names = "fck";
#address-cells = <1>;
#size-cells = <1>;
ranges = <0 0x5600 0x200>;
+
+   gpu@0 {
+   compatible = "ti,am5728-gpu", 
"img,powervr-sgx544";
+   reg = <0x0 0x1>; /* 64kB */
+   interrupts = ;
+   };
};
 
crossbar_mpu: crossbar@4a002a48 {
-- 
2.39.2

[PATCH RFC 00/10] Device tree support for Imagination Series5 GPU

[Andrew Davis]

Hello all,

I know this has been tried before[0], but given the recent upstreaming of
the Series6+ GPU bindings I figured it might be time to give the Series5
bindings another try.

While there is currently no mainline driver for these binding, there is an
open source out-of-tree kernel-side driver available[1]. Having a stable
and upstream binding for these devices allows us to describe this hardware
in device tree.

This is my vision for how these bindings should look, along with some
example uses in several SoC DT files. The compatible names have been
updated to match what was decided on for Series6+, but otherwise most
is the same as we have been using in our vendor tree for many years.

Thanks,
Andrew

Based on next-20231204.

[0]: https://lkml.org/lkml/2020/4/24/1222
[1]: https://github.com/openpvrsgx-devgroup

Andrew Davis (10):
  dt-bindings: gpu: Add PowerVR Series5 SGX GPUs
  ARM: dts: omap3: Add device tree entry for SGX GPU
  ARM: dts: omap4: Add device tree entry for SGX GPU
  ARM: dts: omap5: Add device tree entry for SGX GPU
  ARM: dts: AM33xx: Add device tree entry for SGX GPU
  ARM: dts: AM437x: Add device tree entry for SGX GPU
  ARM: dts: DRA7xx: Add device tree entry for SGX GPU
  arm64: dts: ti: k3-am654-main: Add device tree entry for SGX GPU
  ARM: dts: sun6i: Add device tree entry for SGX GPU
  MIPS: DTS: jz4780: Add device tree entry for SGX GPU

 .../devicetree/bindings/gpu/img,powervr.yaml  | 69 +--
 arch/arm/boot/dts/allwinner/sun6i-a31.dtsi|  9 +++
 arch/arm/boot/dts/ti/omap/am33xx.dtsi |  9 +--
 arch/arm/boot/dts/ti/omap/am3517.dtsi | 11 +--
 arch/arm/boot/dts/ti/omap/am4372.dtsi |  6 ++
 arch/arm/boot/dts/ti/omap/dra7.dtsi   |  9 ++-
 arch/arm/boot/dts/ti/omap/omap34xx.dtsi   | 11 +--
 arch/arm/boot/dts/ti/omap/omap36xx.dtsi   |  9 +--
 arch/arm/boot/dts/ti/omap/omap4.dtsi  |  9 +--
 arch/arm/boot/dts/ti/omap/omap5.dtsi  |  9 +--
 arch/arm64/boot/dts/ti/k3-am65-main.dtsi  |  7 ++
 arch/mips/boot/dts/ingenic/jz4780.dtsi| 11 +++
 12 files changed, 136 insertions(+), 33 deletions(-)

-- 
2.39.2

[PATCH v3] drm: omapdrm: Improve check for contiguous buffers

[Andrew Davis]

While a scatter-gather table having only 1 entry does imply it is
contiguous, it is a logic error to assume the inverse. Tables can have
more than 1 entry and still be contiguous. Use a proper check here.

Signed-off-by: Andrew Davis 
---

Changes from v2:
 - Double check that these multi-segment SGTs are handled correctly elsewhere 
in the driver
 - Rebase on v6.7-rc1

Changes from v1:
 - Sent correct version of patch :)

 drivers/gpu/drm/omapdrm/omap_gem.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c 
b/drivers/gpu/drm/omapdrm/omap_gem.c
index c48fa531ca321..3421e8389222a 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -48,7 +48,7 @@ struct omap_gem_object {
 *   OMAP_BO_MEM_DMA_API flag set)
 *
 * - buffers imported from dmabuf (with the OMAP_BO_MEM_DMABUF flag set)
-*   if they are physically contiguous (when sgt->orig_nents == 1)
+*   if they are physically contiguous
 *
 * - buffers mapped through the TILER when pin_cnt is not zero, in which
 *   case the DMA address points to the TILER aperture
@@ -148,12 +148,18 @@ u64 omap_gem_mmap_offset(struct drm_gem_object *obj)
return drm_vma_node_offset_addr(>vma_node);
 }
 
+static bool omap_gem_sgt_is_contiguous(struct sg_table *sgt, size_t size)
+{
+   return !(drm_prime_get_contiguous_size(sgt) < size);
+}
+
 static bool omap_gem_is_contiguous(struct omap_gem_object *omap_obj)
 {
if (omap_obj->flags & OMAP_BO_MEM_DMA_API)
return true;
 
-   if ((omap_obj->flags & OMAP_BO_MEM_DMABUF) && omap_obj->sgt->nents == 1)
+   if ((omap_obj->flags & OMAP_BO_MEM_DMABUF) &&
+   omap_gem_sgt_is_contiguous(omap_obj->sgt, omap_obj->base.size))
return true;
 
return false;
@@ -1385,7 +1391,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct 
drm_device *dev, size_t size,
union omap_gem_size gsize;
 
/* Without a DMM only physically contiguous buffers can be supported. */
-   if (sgt->orig_nents != 1 && !priv->has_dmm)
+   if (!omap_gem_sgt_is_contiguous(sgt, size) && !priv->has_dmm)
return ERR_PTR(-EINVAL);
 
gsize.bytes = PAGE_ALIGN(size);
@@ -1399,7 +1405,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct 
drm_device *dev, size_t size,
 
omap_obj->sgt = sgt;
 
-   if (sgt->orig_nents == 1) {
+   if (omap_gem_sgt_is_contiguous(sgt, size)) {
omap_obj->dma_addr = sg_dma_address(sgt->sgl);
} else {
/* Create pages list from sgt */
-- 
2.39.2

Re: [PATCH v6 04/20] drm/imagination/uapi: Add PowerVR driver UAPI

[Andrew Davis]

On 9/22/23 12:57 PM, Adam Jackson wrote:


On Wed, Sep 6, 2023 at 5:57 AM Sarah Walker mailto:sarah.wal...@imgtec.com>> wrote:


+ *    :BYPASS_CACHE: There are very few situations where this flag is 
useful.


Could you also expand on what these few useful situations are?

Andrew


+ *       By default, the device flushes its memory caches after every job.


Presumably BYPASS_CACHE does something other than "after every job". Is that 
"never" or something else? Would be good if the comment was explicit.
- ajax

Re: [PATCH v3] misc: sram: Add DMA-BUF Heap exporting of SRAM areas

[Andrew Davis]

On 7/13/23 2:27 PM, Greg Kroah-Hartman wrote:

On Thu, Jul 13, 2023 at 02:13:16PM -0500, Andrew Davis wrote:

+int sram_add_dma_heap(struct sram_dev *sram,
+ struct sram_reserve *block,
+ phys_addr_t start,
+ struct sram_partition *part)
+{
+   struct sram_dma_heap *sram_dma_heap;
+   struct dma_heap_export_info exp_info;
+
+   dev_info(sram->dev, "Exporting SRAM Heap '%s'\n", block->label);


When drivers are working properly, they are quiet.



This should only be printed once in early boot when the memory is added,
I was wanting this to match the other memory exporters/output at the
beginning of boot logs.

But quiet is fine too, will change this to dev_dbg() for v4.

Thanks,
Andrew


thanks,

greg k-h

[PATCH v3] misc: sram: Add DMA-BUF Heap exporting of SRAM areas

[Andrew Davis]

This new export type exposes to userspace the SRAM area as a DMA-BUF Heap,
this allows for allocations of DMA-BUFs that can be consumed by various
DMA-BUF supporting devices.

Signed-off-by: Andrew Davis 
---

Changes from v2:
 - Make sram_dma_heap_allocate static (kernel test robot)
 - Rebase on v6.5-rc1

 drivers/misc/Kconfig |   7 +
 drivers/misc/Makefile|   1 +
 drivers/misc/sram-dma-heap.c | 245 +++
 drivers/misc/sram.c  |   6 +
 drivers/misc/sram.h  |  16 +++
 5 files changed, 275 insertions(+)
 create mode 100644 drivers/misc/sram-dma-heap.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 75e427f124b28..ee34dfb61605f 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -448,6 +448,13 @@ config SRAM
 config SRAM_EXEC
bool
 
+config SRAM_DMA_HEAP
+   bool "Export on-chip SRAM pools using DMA-Heaps"
+   depends on DMABUF_HEAPS && SRAM
+   help
+ This driver allows the export of on-chip SRAM marked as both pool
+ and exportable to userspace using the DMA-Heaps interface.
+
 config DW_XDATA_PCIE
depends on PCI
tristate "Synopsys DesignWare xData PCIe driver"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index f2a4d1ff65d46..5e7516bfaa8de 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_VMWARE_VMCI) += vmw_vmci/
 obj-$(CONFIG_LATTICE_ECP3_CONFIG)  += lattice-ecp3-config.o
 obj-$(CONFIG_SRAM) += sram.o
 obj-$(CONFIG_SRAM_EXEC)+= sram-exec.o
+obj-$(CONFIG_SRAM_DMA_HEAP)+= sram-dma-heap.o
 obj-$(CONFIG_GENWQE)   += genwqe/
 obj-$(CONFIG_ECHO) += echo/
 obj-$(CONFIG_CXL_BASE) += cxl/
diff --git a/drivers/misc/sram-dma-heap.c b/drivers/misc/sram-dma-heap.c
new file mode 100644
index 0..c054c04dff33e
--- /dev/null
+++ b/drivers/misc/sram-dma-heap.c
@@ -0,0 +1,245 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SRAM DMA-Heap userspace exporter
+ *
+ * Copyright (C) 2019-2022 Texas Instruments Incorporated - https://www.ti.com/
+ * Andrew Davis 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "sram.h"
+
+struct sram_dma_heap {
+   struct dma_heap *heap;
+   struct gen_pool *pool;
+};
+
+struct sram_dma_heap_buffer {
+   struct gen_pool *pool;
+   struct list_head attachments;
+   struct mutex attachments_lock;
+   unsigned long len;
+   void *vaddr;
+   phys_addr_t paddr;
+};
+
+struct dma_heap_attachment {
+   struct device *dev;
+   struct sg_table *table;
+   struct list_head list;
+};
+
+static int dma_heap_attach(struct dma_buf *dmabuf,
+  struct dma_buf_attachment *attachment)
+{
+   struct sram_dma_heap_buffer *buffer = dmabuf->priv;
+   struct dma_heap_attachment *a;
+   struct sg_table *table;
+
+   a = kzalloc(sizeof(*a), GFP_KERNEL);
+   if (!a)
+   return -ENOMEM;
+
+   table = kmalloc(sizeof(*table), GFP_KERNEL);
+   if (!table) {
+   kfree(a);
+   return -ENOMEM;
+   }
+   if (sg_alloc_table(table, 1, GFP_KERNEL)) {
+   kfree(table);
+   kfree(a);
+   return -ENOMEM;
+   }
+   sg_set_page(table->sgl, pfn_to_page(PFN_DOWN(buffer->paddr)), 
buffer->len, 0);
+
+   a->table = table;
+   a->dev = attachment->dev;
+   INIT_LIST_HEAD(>list);
+
+   attachment->priv = a;
+
+   mutex_lock(>attachments_lock);
+   list_add(>list, >attachments);
+   mutex_unlock(>attachments_lock);
+
+   return 0;
+}
+
+static void dma_heap_detatch(struct dma_buf *dmabuf,
+struct dma_buf_attachment *attachment)
+{
+   struct sram_dma_heap_buffer *buffer = dmabuf->priv;
+   struct dma_heap_attachment *a = attachment->priv;
+
+   mutex_lock(>attachments_lock);
+   list_del(>list);
+   mutex_unlock(>attachments_lock);
+
+   sg_free_table(a->table);
+   kfree(a->table);
+   kfree(a);
+}
+
+static struct sg_table *dma_heap_map_dma_buf(struct dma_buf_attachment 
*attachment,
+enum dma_data_direction direction)
+{
+   struct dma_heap_attachment *a = attachment->priv;
+   struct sg_table *table = a->table;
+
+   /*
+* As this heap is backed by uncached SRAM memory we do not need to
+* perform any sync operations on the buffer before allowing device
+* domain access. For this reason we use SKIP_CPU_SYNC and also do
+* not use or provide begin/end_cpu_access() dma-buf functions.
+*/
+   if (!dma_map_sg_attrs(attachment->dev, table->sgl, table->nents,
+ direction, DMA_ATTR_SKIP_CPU

Re: [PATCH v3 05/17] drm/imagination: Get GPU resources

[Andrew Davis]

On 6/13/23 9:47 AM, Sarah Walker wrote:

Acquire clock, regulator and register resources, and enable/map as
appropriate.

Signed-off-by: Sarah Walker 
---
  drivers/gpu/drm/imagination/Makefile |   1 +
  drivers/gpu/drm/imagination/pvr_device.c | 271 +++
  drivers/gpu/drm/imagination/pvr_device.h | 214 ++
  drivers/gpu/drm/imagination/pvr_drv.c|  11 +-
  4 files changed, 496 insertions(+), 1 deletion(-)
  create mode 100644 drivers/gpu/drm/imagination/pvr_device.c

diff --git a/drivers/gpu/drm/imagination/Makefile 
b/drivers/gpu/drm/imagination/Makefile
index 62ccf0ccbd51..186f920d615b 100644
--- a/drivers/gpu/drm/imagination/Makefile
+++ b/drivers/gpu/drm/imagination/Makefile
@@ -4,6 +4,7 @@
  subdir-ccflags-y := -I$(srctree)/$(src)
  
  powervr-y := \

+   pvr_device.o \
pvr_drv.o \
  
  obj-$(CONFIG_DRM_POWERVR) += powervr.o

diff --git a/drivers/gpu/drm/imagination/pvr_device.c 
b/drivers/gpu/drm/imagination/pvr_device.c
new file mode 100644
index ..790c36cebec1
--- /dev/null
+++ b/drivers/gpu/drm/imagination/pvr_device.c
@@ -0,0 +1,271 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/* Copyright (c) 2022 Imagination Technologies Ltd. */
+
+#include "pvr_device.h"
+
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/**
+ * pvr_device_reg_init() - Initialize kernel access to a PowerVR device's
+ * control registers.
+ * @pvr_dev: Target PowerVR device.
+ *
+ * Sets struct pvr_device->regs.
+ *
+ * This method of mapping the device control registers into memory ensures that
+ * they are unmapped when the driver is detached (i.e. no explicit cleanup is
+ * required).
+ *
+ * Return:
+ *  * 0 on success, or
+ *  * Any error returned by devm_platform_ioremap_resource().
+ */
+static int
+pvr_device_reg_init(struct pvr_device *pvr_dev)
+{
+   struct drm_device *drm_dev = from_pvr_device(pvr_dev);
+   struct platform_device *plat_dev = to_platform_device(drm_dev->dev);
+   struct resource *regs_resource;
+   void __iomem *regs;
+   int err;
+
+   pvr_dev->regs_resource = NULL;
+   pvr_dev->regs = NULL;
+
+   regs = devm_platform_get_and_ioremap_resource(plat_dev, 0, 
_resource);
+   if (IS_ERR(regs)) {
+   err = PTR_ERR(regs);
+   drm_err(drm_dev, "failed to ioremap gpu registers (err=%d)\n",
+   err);
+   return err;
+   }
+
+   pvr_dev->regs = regs;
+   pvr_dev->regs_resource = regs_resource;
+
+   return 0;
+}
+
+/**
+ * pvr_device_reg_fini() - Deinitialize kernel access to a PowerVR device's
+ * control registers.
+ * @pvr_dev: Target PowerVR device.
+ *
+ * This is essentially a no-op, since pvr_device_reg_init() already ensures 
that
+ * struct pvr_device->regs is unmapped when the device is detached. This
+ * function just sets struct pvr_device->regs to %NULL.
+ */
+static __always_inline void
+pvr_device_reg_fini(struct pvr_device *pvr_dev)
+{
+   pvr_dev->regs = NULL;


This function isn't needed, kinda defeats the purpose of using devm_*()
if you go an manually have no-op functions to unwind it..


+}
+
+/**
+ * pvr_device_clk_init() - Initialize clocks required by a PowerVR device
+ * @pvr_dev: Target PowerVR device.
+ *
+ * Sets struct pvr_device->core_clk, struct pvr_device->sys_clk and
+ * struct pvr_device->mem_clk.
+ *
+ * Three clocks are required by the PowerVR device: core, sys and mem. On
+ * return, this function guarantees that the clocks are in one of the following
+ * states:
+ *
+ *  * All successfully initialized,
+ *  * Core errored, sys and mem uninitialized,
+ *  * Core deinitialized, sys errored, mem uninitialized, or
+ *  * Core and sys deinitialized, mem errored.
+ *
+ * Return:
+ *  * 0 on success,
+ *  * Any error returned by devm_clk_get(), or
+ *  * Any error returned by clk_prepare_enable().
+ */
+static int pvr_device_clk_init(struct pvr_device *pvr_dev)
+{
+   struct drm_device *drm_dev = from_pvr_device(pvr_dev);
+   struct clk *core_clk;
+   struct clk *sys_clk;
+   struct clk *mem_clk;
+   int err;
+
+   pvr_dev->core_clk = NULL;
+   pvr_dev->sys_clk = NULL;
+   pvr_dev->mem_clk = NULL;


You could NULL these out on the error path, but is that even needed, looks
like if this functions fails we bail on the whole init where this is all
deallocated (plus NULL'd out again in that path).


+
+   core_clk = devm_clk_get(drm_dev->dev, "core");
+   if (IS_ERR(core_clk)) {
+   err = PTR_ERR(core_clk);
+   drm_err(drm_dev, "failed to get core clock (err=%d)\n", err);
+   goto err_out;
+   }
+
+   sys_clk = devm_clk_get(drm_dev->dev, "sys");
+   if (IS_ERR(sys_clk))
+   sys_clk = NULL;
+
+   mem_clk = devm_clk_get(drm_dev->dev, "mem");
+   if (IS_ERR(mem_clk))
+   mem_clk = NULL;
+
+   err = 

Re: [PATCH v3 02/17] dt-bindings: gpu: Add Imagination Technologies PowerVR GPU

[Andrew Davis]

On 6/13/23 9:47 AM, Sarah Walker wrote:

Add the device tree binding documentation for the Series AXE GPU used in
TI AM62 SoCs.

Signed-off-by: Sarah Walker 
---
  .../devicetree/bindings/gpu/img,powervr.yaml  | 71 +++
  MAINTAINERS   |  7 ++
  2 files changed, 78 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr.yaml

diff --git a/Documentation/devicetree/bindings/gpu/img,powervr.yaml 
b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
new file mode 100644
index ..652343876d1c
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
@@ -0,0 +1,71 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (c) 2022 Imagination Technologies Ltd.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpu/img,powervr.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Imagination Technologies PowerVR GPU
+
+maintainers:
+  - Sarah Walker 
+
+properties:
+  compatible:
+oneOf:


oneOf shouldn't be needed, you can just do the enum followed by const.


+  - items:
+  - enum:
+  - ti,am62-gpu
+  - const: img,powervr-seriesaxe
+
+  reg:
+maxItems: 1
+
+  clocks:
+minItems: 1
+maxItems: 3
+
+  clock-names:
+items:
+  - const: core
+  - const: mem
+  - const: sys
+minItems: 1
+
+  interrupts:
+items:
+  - description: GPU interrupt
+
+  interrupt-names:
+items:
+  - const: gpu
+
+  power-domains:
+maxItems: 1
+
+  power-supply: true


Why do you need power-supply?

Andrew


+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - interrupts
+  - interrupt-names
+
+additionalProperties: false
+
+examples:
+  - |
+#include 
+#include 
+
+gpu: gpu@fd0 {
+compatible = "ti,am62-gpu", "img,powervr-seriesaxe";
+reg = <0x0fd0 0x2>;
+power-domains = <_pds 187>;
+clocks = <_clks 187 0>;
+clock-names = "core";
+interrupts = ;
+interrupt-names = "gpu";
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index b344e1318ac3..a41517843a10 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10084,6 +10084,13 @@ IMGTEC IR DECODER DRIVER
  S:Orphan
  F:drivers/media/rc/img-ir/
  
+IMGTEC POWERVR DRM DRIVER

+M: Frank Binns 
+M: Sarah Walker 
+M: Donald Robson 
+S: Supported
+F: Documentation/devicetree/bindings/gpu/img,powervr.yaml
+
  IMON SOUNDGRAPH USB IR RECEIVER
  M:Sean Young 
  L:linux-me...@vger.kernel.org

Re: [PATCH] misc: sram: Add dma-heap-export reserved SRAM area type

[Andrew Davis]

On 4/14/23 12:44 AM, Christian Gmeiner wrote:

Hi Andrew

Am Di., 4. Apr. 2023 um 17:02 Uhr schrieb Christian Gmeiner
:



Hi Andrew




Okay, will split for v2.




Was there a follow-up v2 of this patchset? AFAICT this series did not
make it into the mainline kernel.
Do you have any plans to work on it? If not I would like to help out
as we have a use case where we want to
use a dma-buf sram exporter.




Sure, I've been keeping it alive in our evil vendor tree, but if
there is interest upstream now I'll post a v2 and CC you.


That would be great!



Did you find time to prepare a v2? If not, can you point me to the
evil vendor tree?




I did find some time and CC'd you on v2, the patch's subject was slightly
renamed, so maybe your emailer missed it?

https://patchwork.kernel.org/project/linux-media/patch/20230403192433.26648-1-...@ti.com/

Our evil vendor trees are here either way:

https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/

Andrew

Re: [PATCH] misc: sram: Add dma-heap-export reserved SRAM area type

[Andrew Davis]

On 4/1/23 3:35 AM, Christian Gmeiner wrote:

Hi Andrew




Okay, will split for v2.




Was there a follow-up v2 of this patchset? AFAICT this series did not
make it into the mainline kernel.
Do you have any plans to work on it? If not I would like to help out
as we have a use case where we want to
use a dma-buf sram exporter.




Sure, I've been keeping it alive in our evil vendor tree, but if
there is interest upstream now I'll post a v2 and CC you.

Thanks,
Andrew

[PATCH v2] misc: sram: Add DMA-BUF Heap exporting of SRAM areas

[Andrew Davis]

This new export type exposes to userspace the SRAM area as a DMA-BUF Heap,
this allows for allocations of DMA-BUFs that can be consumed by various
DMA-BUF supporting devices.

Signed-off-by: Andrew Davis 
---

Changes from v1:
 - Use existing DT flags, if both pool(device usable) and export(userspace
   usable) properties are in the SRAM node then export as a DMA-BUF Heap
 - Rebase on 6.3-rc5

 drivers/misc/Kconfig |   7 +
 drivers/misc/Makefile|   1 +
 drivers/misc/sram-dma-heap.c | 245 +++
 drivers/misc/sram.c  |   6 +
 drivers/misc/sram.h  |  16 +++
 5 files changed, 275 insertions(+)
 create mode 100644 drivers/misc/sram-dma-heap.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 433aa41977852..8b4c111a6493b 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -448,6 +448,13 @@ config SRAM
 config SRAM_EXEC
bool
 
+config SRAM_DMA_HEAP
+   bool "Export on-chip SRAM pools using DMA-Heaps"
+   depends on DMABUF_HEAPS && SRAM
+   help
+ This driver allows the export of on-chip SRAM marked as both pool
+ and exportable to userspace using the DMA-Heaps interface.
+
 config DW_XDATA_PCIE
depends on PCI
tristate "Synopsys DesignWare xData PCIe driver"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 56de43943cd51..bbdc64aa8af1a 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_VMWARE_VMCI) += vmw_vmci/
 obj-$(CONFIG_LATTICE_ECP3_CONFIG)  += lattice-ecp3-config.o
 obj-$(CONFIG_SRAM) += sram.o
 obj-$(CONFIG_SRAM_EXEC)+= sram-exec.o
+obj-$(CONFIG_SRAM_DMA_HEAP)+= sram-dma-heap.o
 obj-$(CONFIG_GENWQE)   += genwqe/
 obj-$(CONFIG_ECHO) += echo/
 obj-$(CONFIG_CXL_BASE) += cxl/
diff --git a/drivers/misc/sram-dma-heap.c b/drivers/misc/sram-dma-heap.c
new file mode 100644
index 0..c511f4ac1280e
--- /dev/null
+++ b/drivers/misc/sram-dma-heap.c
@@ -0,0 +1,245 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SRAM DMA-Heap userspace exporter
+ *
+ * Copyright (C) 2019-2022 Texas Instruments Incorporated - https://www.ti.com/
+ * Andrew Davis 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "sram.h"
+
+struct sram_dma_heap {
+   struct dma_heap *heap;
+   struct gen_pool *pool;
+};
+
+struct sram_dma_heap_buffer {
+   struct gen_pool *pool;
+   struct list_head attachments;
+   struct mutex attachments_lock;
+   unsigned long len;
+   void *vaddr;
+   phys_addr_t paddr;
+};
+
+struct dma_heap_attachment {
+   struct device *dev;
+   struct sg_table *table;
+   struct list_head list;
+};
+
+static int dma_heap_attach(struct dma_buf *dmabuf,
+  struct dma_buf_attachment *attachment)
+{
+   struct sram_dma_heap_buffer *buffer = dmabuf->priv;
+   struct dma_heap_attachment *a;
+   struct sg_table *table;
+
+   a = kzalloc(sizeof(*a), GFP_KERNEL);
+   if (!a)
+   return -ENOMEM;
+
+   table = kmalloc(sizeof(*table), GFP_KERNEL);
+   if (!table) {
+   kfree(a);
+   return -ENOMEM;
+   }
+   if (sg_alloc_table(table, 1, GFP_KERNEL)) {
+   kfree(table);
+   kfree(a);
+   return -ENOMEM;
+   }
+   sg_set_page(table->sgl, pfn_to_page(PFN_DOWN(buffer->paddr)), 
buffer->len, 0);
+
+   a->table = table;
+   a->dev = attachment->dev;
+   INIT_LIST_HEAD(>list);
+
+   attachment->priv = a;
+
+   mutex_lock(>attachments_lock);
+   list_add(>list, >attachments);
+   mutex_unlock(>attachments_lock);
+
+   return 0;
+}
+
+static void dma_heap_detatch(struct dma_buf *dmabuf,
+struct dma_buf_attachment *attachment)
+{
+   struct sram_dma_heap_buffer *buffer = dmabuf->priv;
+   struct dma_heap_attachment *a = attachment->priv;
+
+   mutex_lock(>attachments_lock);
+   list_del(>list);
+   mutex_unlock(>attachments_lock);
+
+   sg_free_table(a->table);
+   kfree(a->table);
+   kfree(a);
+}
+
+static struct sg_table *dma_heap_map_dma_buf(struct dma_buf_attachment 
*attachment,
+enum dma_data_direction direction)
+{
+   struct dma_heap_attachment *a = attachment->priv;
+   struct sg_table *table = a->table;
+
+   /*
+* As this heap is backed by uncached SRAM memory we do not need to
+* perform any sync operations on the buffer before allowing device
+* domain access. For this reason we use SKIP_CPU_SYNC and also do
+* not use or provide begin/end_cpu_access() dma-buf functions.
+*/
+   if (!dma_map_sg_attrs(at

Re: [PATCH v3] dma-buf: cma_heap: Check for device max segment size when attaching

[Andrew Davis]

On 3/6/23 8:48 PM, John Stultz wrote:

On Mon, Mar 6, 2023 at 8:52 AM Andrew Davis  wrote:


Although there is usually not such a limitation (and when there is it is
often only because the driver forgot to change the super small default),
it is still correct here to break scatterlist element into chunks of
dma_max_mapping_size().


Hey Andrew!
   Thanks for sending this out!

So *why* is it "correct here to break scatterlist element into chunks
of  dma_max_mapping_size()." ?



Good question, I'm not 100% sure on the background myself. It seems
since some devices have restrictions on how large a mapping they can
handle in a single run, we should not hand out single scatterlist
elements longer than that.

It is still a contiguous buffer, but some drivers forget to set their
mapping limits and also only check the number of elements == 1 to determine
if a sg is contiguous (which is not correct as there is no rule that
contiguous runs must be merged into a single scatterlist). For those
driver this would be an issue (I've only found one such case in-tree and
sent a fix, https://lore.kernel.org/lkml/20220825162609.14076-1-...@ti.com/)


This might cause some issues for users with misbehaving drivers. If
bisecting has landed you on this commit, make sure your drivers both set
dma_set_max_seg_size() and are checking for contiguousness correctly.


Why is this change worth the risk? (If this is really likely to break
folks, should we maybe provide warnings initially instead? Maybe
falling back to the old code if we can catch the failure?)

I don't really object to the change, just want to make sure the commit
message is more clear on why we should make this change, what the
benefit will be along with the potential downsides.



I'm not sure it is worth the risk today either, but figured this being a
young enough exporter it could be a good spot to start with for exposing
misbehaving drivers vs some legacy GPU driver exporter. Plus better to
make this change now rather than later in any case.

I don't have any strong reason for this yet though, so I'm find with
just considering this patch an RFC for now.

Thanks,
Andrew


thanks
-john

[PATCH v3] dma-buf: cma_heap: Check for device max segment size when attaching

[Andrew Davis]

Although there is usually not such a limitation (and when there is it is
often only because the driver forgot to change the super small default),
it is still correct here to break scatterlist element into chunks of
dma_max_mapping_size().

This might cause some issues for users with misbehaving drivers. If
bisecting has landed you on this commit, make sure your drivers both set
dma_set_max_seg_size() and are checking for contiguousness correctly.

Signed-off-by: Andrew Davis 
---

Changes from v2:
 - Rebase v6.3-rc1

Changes from v1:
 - Fixed mixed declarations and code warning

 drivers/dma-buf/heaps/cma_heap.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
index 1131fb943992..579261a46fa3 100644
--- a/drivers/dma-buf/heaps/cma_heap.c
+++ b/drivers/dma-buf/heaps/cma_heap.c
@@ -53,16 +53,18 @@ static int cma_heap_attach(struct dma_buf *dmabuf,
 {
struct cma_heap_buffer *buffer = dmabuf->priv;
struct dma_heap_attachment *a;
+   size_t max_segment;
int ret;
 
a = kzalloc(sizeof(*a), GFP_KERNEL);
if (!a)
return -ENOMEM;
 
-   ret = sg_alloc_table_from_pages(>table, buffer->pages,
-   buffer->pagecount, 0,
-   buffer->pagecount << PAGE_SHIFT,
-   GFP_KERNEL);
+   max_segment = dma_get_max_seg_size(attachment->dev);
+   ret = sg_alloc_table_from_pages_segment(>table, buffer->pages,
+   buffer->pagecount, 0,
+   buffer->pagecount << PAGE_SHIFT,
+   max_segment, GFP_KERNEL);
if (ret) {
kfree(a);
return ret;
-- 
2.39.2

Re: [PATCH v4 3/7] accel/ivpu: Add GEM buffer object management

[Andrew Davis]

On 1/9/23 5:47 AM, Jacek Lawrynowicz wrote:

Hi,

On 06.01.2023 14:29, Stanislaw Gruszka wrote:

Hi

On Thu, Jan 05, 2023 at 12:46:51PM -0600, Andrew Davis wrote:

On 12/8/22 5:07 AM, Jacek Lawrynowicz wrote:

Adds four types of GEM-based BOs for the VPU:
- shmem
- userptr


Do you have some specific need for userptr that would not
be covered by prime import + heaps? I'm just trying to get
a feel for the typical use-cases for these.


Honestly, I'm not sure. I think we have use-cases that justify
adding userptr, but have to check with our team members that
better understand the requirements.


It would be great if userptr could be replaced by dma-buf heaps.
I will add this to TODO and we will look into this after the driver is merged.



We should also be clear on the export capabilities up front
for these kinds of drivers. DRM allows re-exporting as DMA-BUF
no matter the allocation style/location which has caused issues.
Lets start accel framework with the rule that if you want a shareable
buffer you should allocate it from Heaps not the driver, then pass
it into the driver.

Merging as-is to save churn now would be fine, but it must be clear
that this is not a stable ABI just because it was allowed to be
merged. userptr/export will be removed later and should not be used
by the userspace driver.

Andrew


Regards,
Jacek

Re: [PATCH v4 3/7] accel/ivpu: Add GEM buffer object management

[Andrew Davis]

On 12/8/22 5:07 AM, Jacek Lawrynowicz wrote:

Adds four types of GEM-based BOs for the VPU:
   - shmem
   - userptr


Do you have some specific need for userptr that would not
be covered by prime import + heaps? I'm just trying to get
a feel for the typical use-cases for these.

Andrew


   - internal
   - prime

All types are implemented as struct ivpu_bo, based on
struct drm_gem_object. VPU address is allocated when buffer is created
except for imported prime buffers that allocate it in BO_INFO IOCTL due
to missing file_priv arg in gem_prime_import callback.
Internal buffers are pinned on creation, the rest of buffers types
can be pinned on demand (in SUBMIT IOCTL).
Buffer VPU address, allocated pages and mappings are relased when the
buffer is destroyed.
Eviction mechism is planned for future versions.

Add three new IOCTLs: BO_CREATE, BO_INFO, BO_USERPTR

Signed-off-by: Jacek Lawrynowicz 

Re: [PATCH] drm: tidss: Fix pixel format definition

[Andrew Davis]

On 12/1/22 6:18 PM, Randolph Sapp wrote:

There was a long-standing bug from a typo that created 2 ARGB1555 and
ABGR1555 pixel format entries. Weston 10 has a sanity check that alerted
me to this issue.

According to the Supported Pixel Data formats table we have the later
entries should have been for Alpha-X instead.



Fixes 32a1795f57eecc

Acked-by: Andrew Davis 


Signed-off-by: Randolph Sapp 
---
  drivers/gpu/drm/tidss/tidss_dispc.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
b/drivers/gpu/drm/tidss/tidss_dispc.c
index ad93acc9abd2..16301bdfead1 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -1858,8 +1858,8 @@ static const struct {
{ DRM_FORMAT_XBGR, 0x21, },
{ DRM_FORMAT_RGBX, 0x22, },
  
-	{ DRM_FORMAT_ARGB1555, 0x25, },

-   { DRM_FORMAT_ABGR1555, 0x26, },
+   { DRM_FORMAT_XRGB1555, 0x25, },
+   { DRM_FORMAT_XBGR1555, 0x26, },
  
  	{ DRM_FORMAT_XRGB, 0x27, },

{ DRM_FORMAT_XBGR, 0x28, },

Re: [PATCH] drm/tidss: Set max DMA segment size

[Andrew Davis]

On 8/22/22 7:16 PM, Andrew Davis wrote:

We have no segment size limitations. Set to unlimited.

Signed-off-by: Andrew Davis 
---


Ping, still valid.

Andrew


  drivers/gpu/drm/tidss/tidss_dispc.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
b/drivers/gpu/drm/tidss/tidss_dispc.c
index dd3c6a606ae2..624545e4636c 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -2685,6 +2685,7 @@ int dispc_init(struct tidss_device *tidss)
if (r)
dev_warn(dev, "cannot set DMA masks to 48-bit\n");
}
+   dma_set_max_seg_size(dev, UINT_MAX);
  
  	dispc = devm_kzalloc(dev, sizeof(*dispc), GFP_KERNEL);

if (!dispc)

Re: [PATCH v4] dma-buf: fix racing conflict of dma_heap_add()

[Andrew Davis]

On 11/4/22 11:05 AM, Dawei Li wrote:

Racing conflict could be:
task A task B
list_for_each_entry
strcmp(h->name))
list_for_each_entry
strcmp(h->name)
kzallockzalloc
.. .
device_create  device_create
list_add
list_add

The root cause is that task B has no idea about the fact someone
else(A) has inserted heap with same name when it calls list_add,
so a potential collision occurs.

Fixes: c02a81fba74f ("dma-buf: Add dma-buf heaps framework")
Signed-off-by: Dawei Li 


LGTM, Thanks,

Acked-by: Andrew Davis 


---
v1: 
https://lore.kernel.org/all/tycp286mb2323950197f60fc3473123b7ca...@tycp286mb2323.jpnp286.prod.outlook.com/
v1->v2: Narrow down locking scope, check the existence of heap before
insertion, as suggested by Andrew Davis.
v2->v3: Remove double checking.
v3->v4: Minor coding style and patch formatting adjustment.
---
  drivers/dma-buf/dma-heap.c | 28 +++-
  1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
index 8f5848aa144f..59d158873f4c 100644
--- a/drivers/dma-buf/dma-heap.c
+++ b/drivers/dma-buf/dma-heap.c
@@ -233,18 +233,6 @@ struct dma_heap *dma_heap_add(const struct 
dma_heap_export_info *exp_info)
return ERR_PTR(-EINVAL);
}
  
-	/* check the name is unique */

-   mutex_lock(_list_lock);
-   list_for_each_entry(h, _list, list) {
-   if (!strcmp(h->name, exp_info->name)) {
-   mutex_unlock(_list_lock);
-   pr_err("dma_heap: Already registered heap named %s\n",
-  exp_info->name);
-   return ERR_PTR(-EINVAL);
-   }
-   }
-   mutex_unlock(_list_lock);
-
heap = kzalloc(sizeof(*heap), GFP_KERNEL);
if (!heap)
return ERR_PTR(-ENOMEM);
@@ -283,13 +271,27 @@ struct dma_heap *dma_heap_add(const struct 
dma_heap_export_info *exp_info)
err_ret = ERR_CAST(dev_ret);
goto err2;
}
-   /* Add heap to the list */
+
mutex_lock(_list_lock);
+   /* check the name is unique */
+   list_for_each_entry(h, _list, list) {
+   if (!strcmp(h->name, exp_info->name)) {
+   mutex_unlock(_list_lock);
+   pr_err("dma_heap: Already registered heap named %s\n",
+  exp_info->name);
+   err_ret = ERR_PTR(-EINVAL);
+   goto err3;
+   }
+   }
+
+   /* Add heap to the list */
list_add(>list, _list);
mutex_unlock(_list_lock);
  
  	return heap;
  
+err3:

+   device_destroy(dma_heap_class, heap->heap_devt);
  err2:
cdev_del(>heap_cdev);
  err1:

Re: [PATCH v3] dma-buf: fix racing conflict of dma_heap_add()

[Andrew Davis]

On 11/2/22 10:58 AM, Dawei Li wrote:

Racing conflict could be:
task A task B
list_for_each_entry
strcmp(h->name))
list_for_each_entry
strcmp(h->name)
kzallockzalloc
.. .
device_create  device_create
list_add
list_add

The root cause is that task B has no idea about the fact someone
else(A) has inserted heap with same name when it calls list_add,
so a potential collision occurs.

v1: 
https://lore.kernel.org/all/tycp286mb2323950197f60fc3473123b7ca...@tycp286mb2323.jpnp286.prod.outlook.com/

v1->v2: Narrow down locking scope, check the existence of heap before
insertion, as suggested by Andrew Davis.

v2->v3: Remove double checking.


The above version info should be in a cover letter or below
the --- line so it doesn't end up in the commit message in tree.



Fixes: c02a81fba74f ("dma-buf: Add dma-buf heaps framework")

base-commit: 447fb14bf07905b880c9ed1ea92c53d6dd0649d7



Same as above, plus this is an odd base, maybe just use "v6.1-rc2".


Signed-off-by: Dawei Li 
---
  drivers/dma-buf/dma-heap.c | 29 +++--
  1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
index 8f5848aa144f..7a25e98259ea 100644
--- a/drivers/dma-buf/dma-heap.c
+++ b/drivers/dma-buf/dma-heap.c
@@ -233,18 +233,6 @@ struct dma_heap *dma_heap_add(const struct 
dma_heap_export_info *exp_info)
return ERR_PTR(-EINVAL);
}
  
-	/* check the name is unique */

-   mutex_lock(_list_lock);
-   list_for_each_entry(h, _list, list) {
-   if (!strcmp(h->name, exp_info->name)) {
-   mutex_unlock(_list_lock);
-   pr_err("dma_heap: Already registered heap named %s\n",
-  exp_info->name);
-   return ERR_PTR(-EINVAL);
-   }
-   }
-   mutex_unlock(_list_lock);
-
heap = kzalloc(sizeof(*heap), GFP_KERNEL);
if (!heap)
return ERR_PTR(-ENOMEM);
@@ -283,13 +271,26 @@ struct dma_heap *dma_heap_add(const struct 
dma_heap_export_info *exp_info)
err_ret = ERR_CAST(dev_ret);
goto err2;
}
-   /* Add heap to the list */
+
mutex_lock(_list_lock);
+   /* check the name is unique */
+   list_for_each_entry(h, _list, list) {
+   if (!strcmp(h->name, exp_info->name)) {
+   mutex_unlock(_list_lock);
+   pr_err("dma_heap: Already registered heap named %s\n",
+  exp_info->name);
+   err_ret = ERR_PTR(-EINVAL);
+   goto err3;
+   }
+   }
+
+   /* Add heap to the list */
list_add(>list, _list);
mutex_unlock(_list_lock);
  
  	return heap;

-


Would like to keep this new line after the return statement.

Andrew


+err3:
+   device_destroy(dma_heap_class, heap->heap_devt);
  err2:
cdev_del(>heap_cdev);
  err1:

Re: [PATCH] dma-buf: fix racing conflict of dma_heap_add()

[Andrew Davis]

On 10/30/22 6:37 AM, Dawei Li wrote:

Racing conflict could be:
task A task B
list_for_each_entry
strcmp(h->name))
list_for_each_entry
strcmp(h->name)
kzallockzalloc
.. .
device_create  device_create
list_add
list_add

The root cause is that task B has no idea about the fact someone
else(A) has inserted heap with same name when it calls list_add,
so a potential collision occurs.

Fixes: c02a81fba74f ("dma-buf: Add dma-buf heaps framework")

base-commit: 447fb14bf07905b880c9ed1ea92c53d6dd0649d7

Signed-off-by: Dawei Li 
---
  drivers/dma-buf/dma-heap.c | 7 ---
  1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
index 8f5848aa144f..ff44c2777b04 100644
--- a/drivers/dma-buf/dma-heap.c
+++ b/drivers/dma-buf/dma-heap.c
@@ -243,11 +243,12 @@ struct dma_heap *dma_heap_add(const struct 
dma_heap_export_info *exp_info)
return ERR_PTR(-EINVAL);
}
}
-   mutex_unlock(_list_lock);
  
  	heap = kzalloc(sizeof(*heap), GFP_KERNEL);

-   if (!heap)
+   if (!heap) {
+   mutex_unlock(_list_lock);
return ERR_PTR(-ENOMEM);
+   }
  
  	heap->name = exp_info->name;

heap->ops = exp_info->ops;
@@ -284,7 +285,6 @@ struct dma_heap *dma_heap_add(const struct 
dma_heap_export_info *exp_info)
goto err2;
}
/* Add heap to the list */
-   mutex_lock(_list_lock);



Good catch!

In general I'd like to hold locks for as short a time as possible and
only bracket the lock associated structure (heap_list). How about we
move the duplicate name check to down here so they are both inside
this one locked section here.

I know this will mean we take a longer unwind error path
if the names are duplicated, but that should be rare and
this will keep all heap_list accesses together.

Thanks,
Andrew



list_add(>list, _list);
mutex_unlock(_list_lock);
  
@@ -296,6 +296,7 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)

xa_erase(_heap_minors, minor);
  err0:
kfree(heap);
+   mutex_unlock(_list_lock);
return err_ret;
  }
  

[PATCH v3] drm: Move radeon and amdgpu Kconfig options into their directories

[Andrew Davis]

Most Kconfig options to enable a driver are in the Kconfig file
inside the relevant directory, move these two to the same.

Signed-off-by: Andrew Davis 
Reviewed-by: Christian König 
---

Changes from v2:
 - Rebased on latest

Changes from v1:
 - Fix whitespace issue pointed out by Randy

 drivers/gpu/drm/Kconfig| 57 --
 drivers/gpu/drm/amd/amdgpu/Kconfig | 29 +++
 drivers/gpu/drm/radeon/Kconfig | 30 
 3 files changed, 59 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index fa986075e8fb..9c2d9495cb3c 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -233,65 +233,8 @@ source "drivers/gpu/drm/i2c/Kconfig"
 
 source "drivers/gpu/drm/arm/Kconfig"
 
-config DRM_RADEON
-   tristate "ATI Radeon"
-   depends on DRM && PCI && MMU
-   depends on AGP || !AGP
-   select FW_LOADER
-   select DRM_DISPLAY_DP_HELPER
-   select DRM_DISPLAY_HELPER
-select DRM_KMS_HELPER
-select DRM_TTM
-   select DRM_TTM_HELPER
-   select SND_HDA_COMPONENT if SND_HDA_CORE
-   select POWER_SUPPLY
-   select HWMON
-   select BACKLIGHT_CLASS_DEVICE
-   select INTERVAL_TREE
-   # radeon depends on ACPI_VIDEO when ACPI is enabled, for select to work
-   # ACPI_VIDEO's dependencies must also be selected.
-   select INPUT if ACPI
-   select ACPI_VIDEO if ACPI
-   # On x86 ACPI_VIDEO also needs ACPI_WMI
-   select X86_PLATFORM_DEVICES if ACPI && X86
-   select ACPI_WMI if ACPI && X86
-   help
- Choose this option if you have an ATI Radeon graphics card.  There
- are both PCI and AGP versions.  You don't need to choose this to
- run the Radeon in plain VGA mode.
-
- If M is selected, the module will be called radeon.
-
 source "drivers/gpu/drm/radeon/Kconfig"
 
-config DRM_AMDGPU
-   tristate "AMD GPU"
-   depends on DRM && PCI && MMU
-   select FW_LOADER
-   select DRM_DISPLAY_DP_HELPER
-   select DRM_DISPLAY_HDMI_HELPER
-   select DRM_DISPLAY_HELPER
-   select DRM_KMS_HELPER
-   select DRM_SCHED
-   select DRM_TTM
-   select DRM_TTM_HELPER
-   select POWER_SUPPLY
-   select HWMON
-   select BACKLIGHT_CLASS_DEVICE
-   select INTERVAL_TREE
-   select DRM_BUDDY
-   # amdgpu depends on ACPI_VIDEO when ACPI is enabled, for select to work
-   # ACPI_VIDEO's dependencies must also be selected.
-   select INPUT if ACPI
-   select ACPI_VIDEO if ACPI
-   # On x86 ACPI_VIDEO also needs ACPI_WMI
-   select X86_PLATFORM_DEVICES if ACPI && X86
-   select ACPI_WMI if ACPI && X86
-   help
- Choose this option if you have a recent AMD Radeon graphics card.
-
- If M is selected, the module will be called amdgpu.
-
 source "drivers/gpu/drm/amd/amdgpu/Kconfig"
 
 source "drivers/gpu/drm/nouveau/Kconfig"
diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig 
b/drivers/gpu/drm/amd/amdgpu/Kconfig
index d55275de..5fcd510f1abb 100644
--- a/drivers/gpu/drm/amd/amdgpu/Kconfig
+++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
@@ -1,4 +1,33 @@
 # SPDX-License-Identifier: MIT
+
+config DRM_AMDGPU
+   tristate "AMD GPU"
+   depends on DRM && PCI && MMU
+   select FW_LOADER
+   select DRM_DISPLAY_DP_HELPER
+   select DRM_DISPLAY_HDMI_HELPER
+   select DRM_DISPLAY_HELPER
+   select DRM_KMS_HELPER
+   select DRM_SCHED
+   select DRM_TTM
+   select DRM_TTM_HELPER
+   select POWER_SUPPLY
+   select HWMON
+   select BACKLIGHT_CLASS_DEVICE
+   select INTERVAL_TREE
+   select DRM_BUDDY
+   # amdgpu depends on ACPI_VIDEO when ACPI is enabled, for select to work
+   # ACPI_VIDEO's dependencies must also be selected.
+   select INPUT if ACPI
+   select ACPI_VIDEO if ACPI
+   # On x86 ACPI_VIDEO also needs ACPI_WMI
+   select X86_PLATFORM_DEVICES if ACPI && X86
+   select ACPI_WMI if ACPI && X86
+   help
+ Choose this option if you have a recent AMD Radeon graphics card.
+
+ If M is selected, the module will be called amdgpu.
+
 config DRM_AMDGPU_SI
bool "Enable amdgpu support for SI parts"
depends on DRM_AMDGPU
diff --git a/drivers/gpu/drm/radeon/Kconfig b/drivers/gpu/drm/radeon/Kconfig
index 52819e7f1fca..2267c501f724 100644
--- a/drivers/gpu/drm/radeon/Kconfig
+++ b/drivers/gpu/drm/radeon/Kconfig
@@ -1,4 +1,34 @@
 # SPDX-License-Identifier: MIT
+
+config DRM_RADEON
+   tristate "ATI Radeon"
+   depends on DRM && PCI && MMU
+   depends on AGP || !AGP
+   select FW_LOADER
+   select DRM_DISPLAY_DP_HELPER
+   select DRM_DISPLAY_HELPER
+   select DRM_KMS_HELPER
+

[PATCH v2] drm: Move radeon and amdgpu Kconfig options into their directories

[Andrew Davis]

Most Kconfig options to enable a driver are in the Kconfig file
inside the relevant directory, move these two to the same.

Signed-off-by: Andrew Davis 
Reviewed-by: Christian König 
---

Changes from v1:
 - Fix whitespace issue pointed out by Randy

 drivers/gpu/drm/Kconfig| 42 --
 drivers/gpu/drm/amd/amdgpu/Kconfig | 22 
 drivers/gpu/drm/radeon/Kconfig | 22 
 3 files changed, 44 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 6c2256e8474be..24fa9ccd92a4e 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -234,50 +234,8 @@ source "drivers/gpu/drm/i2c/Kconfig"
 
 source "drivers/gpu/drm/arm/Kconfig"
 
-config DRM_RADEON
-   tristate "ATI Radeon"
-   depends on DRM && PCI && MMU
-   depends on AGP || !AGP
-   select FW_LOADER
-   select DRM_DISPLAY_DP_HELPER
-   select DRM_DISPLAY_HELPER
-select DRM_KMS_HELPER
-select DRM_TTM
-   select DRM_TTM_HELPER
-   select POWER_SUPPLY
-   select HWMON
-   select BACKLIGHT_CLASS_DEVICE
-   select INTERVAL_TREE
-   help
- Choose this option if you have an ATI Radeon graphics card.  There
- are both PCI and AGP versions.  You don't need to choose this to
- run the Radeon in plain VGA mode.
-
- If M is selected, the module will be called radeon.
-
 source "drivers/gpu/drm/radeon/Kconfig"
 
-config DRM_AMDGPU
-   tristate "AMD GPU"
-   depends on DRM && PCI && MMU
-   select FW_LOADER
-   select DRM_DISPLAY_DP_HELPER
-   select DRM_DISPLAY_HDMI_HELPER
-   select DRM_DISPLAY_HELPER
-   select DRM_KMS_HELPER
-   select DRM_SCHED
-   select DRM_TTM
-   select DRM_TTM_HELPER
-   select POWER_SUPPLY
-   select HWMON
-   select BACKLIGHT_CLASS_DEVICE
-   select INTERVAL_TREE
-   select DRM_BUDDY
-   help
- Choose this option if you have a recent AMD Radeon graphics card.
-
- If M is selected, the module will be called amdgpu.
-
 source "drivers/gpu/drm/amd/amdgpu/Kconfig"
 
 source "drivers/gpu/drm/nouveau/Kconfig"
diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig 
b/drivers/gpu/drm/amd/amdgpu/Kconfig
index d55275de8..36b1206124cff 100644
--- a/drivers/gpu/drm/amd/amdgpu/Kconfig
+++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
@@ -1,4 +1,26 @@
 # SPDX-License-Identifier: MIT
+
+config DRM_AMDGPU
+   tristate "AMD GPU"
+   depends on DRM && PCI && MMU
+   select FW_LOADER
+   select DRM_DISPLAY_DP_HELPER
+   select DRM_DISPLAY_HDMI_HELPER
+   select DRM_DISPLAY_HELPER
+   select DRM_KMS_HELPER
+   select DRM_SCHED
+   select DRM_TTM
+   select DRM_TTM_HELPER
+   select POWER_SUPPLY
+   select HWMON
+   select BACKLIGHT_CLASS_DEVICE
+   select INTERVAL_TREE
+   select DRM_BUDDY
+   help
+ Choose this option if you have a recent AMD Radeon graphics card.
+
+ If M is selected, the module will be called amdgpu.
+
 config DRM_AMDGPU_SI
bool "Enable amdgpu support for SI parts"
depends on DRM_AMDGPU
diff --git a/drivers/gpu/drm/radeon/Kconfig b/drivers/gpu/drm/radeon/Kconfig
index 52819e7f1fca1..5b0201cbf6b32 100644
--- a/drivers/gpu/drm/radeon/Kconfig
+++ b/drivers/gpu/drm/radeon/Kconfig
@@ -1,4 +1,26 @@
 # SPDX-License-Identifier: MIT
+
+config DRM_RADEON
+   tristate "ATI Radeon"
+   depends on DRM && PCI && MMU
+   depends on AGP || !AGP
+   select FW_LOADER
+   select DRM_DISPLAY_DP_HELPER
+   select DRM_DISPLAY_HELPER
+   select DRM_KMS_HELPER
+   select DRM_TTM
+   select DRM_TTM_HELPER
+   select POWER_SUPPLY
+   select HWMON
+   select BACKLIGHT_CLASS_DEVICE
+   select INTERVAL_TREE
+   help
+ Choose this option if you have an ATI Radeon graphics card.  There
+ are both PCI and AGP versions.  You don't need to choose this to
+ run the Radeon in plain VGA mode.
+
+ If M is selected, the module will be called radeon.
+
 config DRM_RADEON_USERPTR
bool "Always enable userptr support"
depends on DRM_RADEON
-- 
2.36.1

[PATCH v2] drm: omapdrm: Improve check for contiguous buffers

[Andrew Davis]

While a scatter-gather table having only 1 entry does imply it is
contiguous, it is a logic error to assume the inverse. Tables can have
more than 1 entry and still be contiguous. Use a proper check here.

Signed-off-by: Andrew Davis 
---

Changes from v1:
 - Sent correct version of patch :)

 drivers/gpu/drm/omapdrm/omap_gem.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c 
b/drivers/gpu/drm/omapdrm/omap_gem.c
index cf571796fd26e..c10f3d2dd61ce 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -48,7 +48,7 @@ struct omap_gem_object {
 *   OMAP_BO_MEM_DMA_API flag set)
 *
 * - buffers imported from dmabuf (with the OMAP_BO_MEM_DMABUF flag set)
-*   if they are physically contiguous (when sgt->orig_nents == 1)
+*   if they are physically contiguous
 *
 * - buffers mapped through the TILER when pin_cnt is not zero, in which
 *   case the DMA address points to the TILER aperture
@@ -148,12 +148,18 @@ u64 omap_gem_mmap_offset(struct drm_gem_object *obj)
return drm_vma_node_offset_addr(>vma_node);
 }
 
+static bool omap_gem_sgt_is_contiguous(struct sg_table *sgt, size_t size)
+{
+   return !(drm_prime_get_contiguous_size(sgt) < size);
+}
+
 static bool omap_gem_is_contiguous(struct omap_gem_object *omap_obj)
 {
if (omap_obj->flags & OMAP_BO_MEM_DMA_API)
return true;
 
-   if ((omap_obj->flags & OMAP_BO_MEM_DMABUF) && omap_obj->sgt->nents == 1)
+   if ((omap_obj->flags & OMAP_BO_MEM_DMABUF) &&
+   omap_gem_sgt_is_contiguous(omap_obj->sgt, omap_obj->base.size))
return true;
 
return false;
@@ -1398,7 +1404,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct 
drm_device *dev, size_t size,
union omap_gem_size gsize;
 
/* Without a DMM only physically contiguous buffers can be supported. */
-   if (sgt->orig_nents != 1 && !priv->has_dmm)
+   if (!omap_gem_sgt_is_contiguous(sgt, size) && !priv->has_dmm)
return ERR_PTR(-EINVAL);
 
gsize.bytes = PAGE_ALIGN(size);
@@ -1412,7 +1418,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct 
drm_device *dev, size_t size,
 
omap_obj->sgt = sgt;
 
-   if (sgt->orig_nents == 1) {
+   if (omap_gem_sgt_is_contiguous(sgt, size)) {
omap_obj->dma_addr = sg_dma_address(sgt->sgl);
} else {
/* Create pages list from sgt */
-- 
2.36.1

Re: [PATCH] drm: omapdrm: Improve check for contiguous buffers

[Andrew Davis]

On 8/22/22 7:03 PM, Andrew Davis wrote:

While a scatter-gather table having only 1 entry does imply it is
contiguous, it is a logic error to assume the inverse. Tables can have
more than 1 entry and still be contiguous. Use a proper check here.

Signed-off-by: Andrew Davis 
---



Looks like an old version of this patch got in my to-send queue,
please ignore this one, will send updated v2.

Thanks,
Andrew



  drivers/gpu/drm/omapdrm/omap_gem.c | 14 ++
  1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c 
b/drivers/gpu/drm/omapdrm/omap_gem.c
index cf571796fd26..52c00b795459 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -48,7 +48,7 @@ struct omap_gem_object {
 *   OMAP_BO_MEM_DMA_API flag set)
 *
 * - buffers imported from dmabuf (with the OMAP_BO_MEM_DMABUF flag set)
-*   if they are physically contiguous (when sgt->orig_nents == 1)
+*   if they are physically contiguous
 *
 * - buffers mapped through the TILER when pin_cnt is not zero, in which
 *   case the DMA address points to the TILER aperture
@@ -148,12 +148,18 @@ u64 omap_gem_mmap_offset(struct drm_gem_object *obj)
return drm_vma_node_offset_addr(>vma_node);
  }
  
+static bool omap_gem_sgt_is_contiguous(struct sg_table *sgt, size)

+{
+   return !(drm_prime_get_contiguous_size(sgt) < size);
+}
+
  static bool omap_gem_is_contiguous(struct omap_gem_object *omap_obj)
  {
if (omap_obj->flags & OMAP_BO_MEM_DMA_API)
return true;
  
-	if ((omap_obj->flags & OMAP_BO_MEM_DMABUF) && omap_obj->sgt->nents == 1)

+   if ((omap_obj->flags & OMAP_BO_MEM_DMABUF) &&
+   omap_gem_sgt_is_contiguous(omap_obj->sgt, omap_obj->base->size))
return true;
  
  	return false;

@@ -1398,7 +1404,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct 
drm_device *dev, size_t size,
union omap_gem_size gsize;
  
  	/* Without a DMM only physically contiguous buffers can be supported. */

-   if (sgt->orig_nents != 1 && !priv->has_dmm)
+   if (!omap_gem_sgt_is_contiguous(sgt, size) && !priv->has_dmm)
return ERR_PTR(-EINVAL);
  
  	gsize.bytes = PAGE_ALIGN(size);

@@ -1412,7 +1418,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct 
drm_device *dev, size_t size,
  
  	omap_obj->sgt = sgt;
  
-	if (sgt->orig_nents == 1) {

+   if (omap_gem_sgt_is_contiguous(sgt, size)) {
omap_obj->dma_addr = sg_dma_address(sgt->sgl);
} else {
/* Create pages list from sgt */

[PATCH v2] dma-buf: cma_heap: Check for device max segment size when attaching

[Andrew Davis]

Although there is usually not such a limitation (and when there is it is
often only because the driver forgot to change the super small default),
it is still correct here to break scatterlist element into chunks of
dma_max_mapping_size().

This might cause some issues for users with misbehaving drivers. If
bisecting has landed you on this commit, make sure your drivers both set
dma_set_max_seg_size() and are checking for contiguousness correctly.

Signed-off-by: Andrew Davis 
---

Changes from v1:
 - Fixed mixed declarations and code warning

 drivers/dma-buf/heaps/cma_heap.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
index 28fb04eccdd0..a927b248c045 100644
--- a/drivers/dma-buf/heaps/cma_heap.c
+++ b/drivers/dma-buf/heaps/cma_heap.c
@@ -52,16 +52,18 @@ static int cma_heap_attach(struct dma_buf *dmabuf,
 {
struct cma_heap_buffer *buffer = dmabuf->priv;
struct dma_heap_attachment *a;
+   size_t max_segment;
int ret;
 
a = kzalloc(sizeof(*a), GFP_KERNEL);
if (!a)
return -ENOMEM;
 
-   ret = sg_alloc_table_from_pages(>table, buffer->pages,
-   buffer->pagecount, 0,
-   buffer->pagecount << PAGE_SHIFT,
-   GFP_KERNEL);
+   max_segment = dma_get_max_seg_size(attachment->dev);
+   ret = sg_alloc_table_from_pages_segment(>table, buffer->pages,
+   buffer->pagecount, 0,
+   buffer->pagecount << PAGE_SHIFT,
+   max_segment, GFP_KERNEL);
if (ret) {
kfree(a);
return ret;
-- 
2.36.1

Re: [PATCH] drm: Move radeon and amdgpu Kconfig options into their directories

[Andrew Davis]

On 8/22/22 7:06 PM, Randy Dunlap wrote:

Hi--

On 8/22/22 17:01, Andrew Davis wrote:

Most Kconfig options to enable a driver are in the Kconfig file
inside the relevant directory, move these two to the same.

Signed-off-by: Andrew Davis 
---
  drivers/gpu/drm/Kconfig| 42 --
  drivers/gpu/drm/amd/amdgpu/Kconfig | 22 
  drivers/gpu/drm/radeon/Kconfig | 22 
  3 files changed, 44 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 6c2256e8474b..24fa9ccd92a4 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -234,50 +234,8 @@ source "drivers/gpu/drm/i2c/Kconfig"
  
  source "drivers/gpu/drm/arm/Kconfig"
  
-config DRM_RADEON

-   tristate "ATI Radeon"
-   depends on DRM && PCI && MMU
-   depends on AGP || !AGP
-   select FW_LOADER
-   select DRM_DISPLAY_DP_HELPER
-   select DRM_DISPLAY_HELPER
-select DRM_KMS_HELPER
-select DRM_TTM
-   select DRM_TTM_HELPER
-   select POWER_SUPPLY
-   select HWMON
-   select BACKLIGHT_CLASS_DEVICE
-   select INTERVAL_TREE
-   help
- Choose this option if you have an ATI Radeon graphics card.  There
- are both PCI and AGP versions.  You don't need to choose this to
- run the Radeon in plain VGA mode.
-
- If M is selected, the module will be called radeon.
-
  source "drivers/gpu/drm/radeon/Kconfig"
  
-config DRM_AMDGPU

-   tristate "AMD GPU"
-   depends on DRM && PCI && MMU
-   select FW_LOADER
-   select DRM_DISPLAY_DP_HELPER
-   select DRM_DISPLAY_HDMI_HELPER
-   select DRM_DISPLAY_HELPER
-   select DRM_KMS_HELPER
-   select DRM_SCHED
-   select DRM_TTM
-   select DRM_TTM_HELPER
-   select POWER_SUPPLY
-   select HWMON
-   select BACKLIGHT_CLASS_DEVICE
-   select INTERVAL_TREE
-   select DRM_BUDDY
-   help
- Choose this option if you have a recent AMD Radeon graphics card.
-
- If M is selected, the module will be called amdgpu.
-
  source "drivers/gpu/drm/amd/amdgpu/Kconfig"
  
  source "drivers/gpu/drm/nouveau/Kconfig"

diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig 
b/drivers/gpu/drm/amd/amdgpu/Kconfig
index d55275de..36b1206124cf 100644
--- a/drivers/gpu/drm/amd/amdgpu/Kconfig
+++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
@@ -1,4 +1,26 @@
  # SPDX-License-Identifier: MIT
+
+config DRM_AMDGPU
+   tristate "AMD GPU"
+   depends on DRM && PCI && MMU
+   select FW_LOADER
+   select DRM_DISPLAY_DP_HELPER
+   select DRM_DISPLAY_HDMI_HELPER
+   select DRM_DISPLAY_HELPER
+   select DRM_KMS_HELPER
+   select DRM_SCHED
+   select DRM_TTM
+   select DRM_TTM_HELPER
+   select POWER_SUPPLY
+   select HWMON
+   select BACKLIGHT_CLASS_DEVICE
+   select INTERVAL_TREE
+   select DRM_BUDDY
+   help
+ Choose this option if you have a recent AMD Radeon graphics card.
+
+ If M is selected, the module will be called amdgpu.
+
  config DRM_AMDGPU_SI
bool "Enable amdgpu support for SI parts"
depends on DRM_AMDGPU
diff --git a/drivers/gpu/drm/radeon/Kconfig b/drivers/gpu/drm/radeon/Kconfig
index 52819e7f1fca..3248d12c562d 100644
--- a/drivers/gpu/drm/radeon/Kconfig
+++ b/drivers/gpu/drm/radeon/Kconfig
@@ -1,4 +1,26 @@
  # SPDX-License-Identifier: MIT
+
+config DRM_RADEON
+   tristate "ATI Radeon"
+   depends on DRM && PCI && MMU
+   depends on AGP || !AGP
+   select FW_LOADER
+   select DRM_DISPLAY_DP_HELPER
+   select DRM_DISPLAY_HELPER
+select DRM_KMS_HELPER
+select DRM_TTM


Would you change those 2 lines above to use one tab + 2 spaces
for indentation, please?




Sure, I just copy/paste exactly as they are in the top level Kconfig,
white-space issues and all, to make it easy to see that nothing was changed.

I can fix the indent issue in a followup patch if that work better.
Whichever works better.

Andrew



+   select DRM_TTM_HELPER
+   select POWER_SUPPLY
+   select HWMON
+   select BACKLIGHT_CLASS_DEVICE
+   select INTERVAL_TREE
+   help
+ Choose this option if you have an ATI Radeon graphics card.  There
+ are both PCI and AGP versions.  You don't need to choose this to
+ run the Radeon in plain VGA mode.
+
+ If M is selected, the module will be called radeon.
+
  config DRM_RADEON_USERPTR
bool "Always enable userptr support"
depends on DRM_RADEON

[PATCH] drm/tidss: Set max DMA segment size

[Andrew Davis]

We have no segment size limitations. Set to unlimited.

Signed-off-by: Andrew Davis 
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
b/drivers/gpu/drm/tidss/tidss_dispc.c
index dd3c6a606ae2..624545e4636c 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -2685,6 +2685,7 @@ int dispc_init(struct tidss_device *tidss)
if (r)
dev_warn(dev, "cannot set DMA masks to 48-bit\n");
}
+   dma_set_max_seg_size(dev, UINT_MAX);
 
dispc = devm_kzalloc(dev, sizeof(*dispc), GFP_KERNEL);
if (!dispc)
-- 
2.36.1

[PATCH] drm: omapdrm: Improve check for contiguous buffers

[Andrew Davis]

While a scatter-gather table having only 1 entry does imply it is
contiguous, it is a logic error to assume the inverse. Tables can have
more than 1 entry and still be contiguous. Use a proper check here.

Signed-off-by: Andrew Davis 
---
 drivers/gpu/drm/omapdrm/omap_gem.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c 
b/drivers/gpu/drm/omapdrm/omap_gem.c
index cf571796fd26..52c00b795459 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -48,7 +48,7 @@ struct omap_gem_object {
 *   OMAP_BO_MEM_DMA_API flag set)
 *
 * - buffers imported from dmabuf (with the OMAP_BO_MEM_DMABUF flag set)
-*   if they are physically contiguous (when sgt->orig_nents == 1)
+*   if they are physically contiguous
 *
 * - buffers mapped through the TILER when pin_cnt is not zero, in which
 *   case the DMA address points to the TILER aperture
@@ -148,12 +148,18 @@ u64 omap_gem_mmap_offset(struct drm_gem_object *obj)
return drm_vma_node_offset_addr(>vma_node);
 }
 
+static bool omap_gem_sgt_is_contiguous(struct sg_table *sgt, size)
+{
+   return !(drm_prime_get_contiguous_size(sgt) < size);
+}
+
 static bool omap_gem_is_contiguous(struct omap_gem_object *omap_obj)
 {
if (omap_obj->flags & OMAP_BO_MEM_DMA_API)
return true;
 
-   if ((omap_obj->flags & OMAP_BO_MEM_DMABUF) && omap_obj->sgt->nents == 1)
+   if ((omap_obj->flags & OMAP_BO_MEM_DMABUF) &&
+   omap_gem_sgt_is_contiguous(omap_obj->sgt, omap_obj->base->size))
return true;
 
return false;
@@ -1398,7 +1404,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct 
drm_device *dev, size_t size,
union omap_gem_size gsize;
 
/* Without a DMM only physically contiguous buffers can be supported. */
-   if (sgt->orig_nents != 1 && !priv->has_dmm)
+   if (!omap_gem_sgt_is_contiguous(sgt, size) && !priv->has_dmm)
return ERR_PTR(-EINVAL);
 
gsize.bytes = PAGE_ALIGN(size);
@@ -1412,7 +1418,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct 
drm_device *dev, size_t size,
 
omap_obj->sgt = sgt;
 
-   if (sgt->orig_nents == 1) {
+   if (omap_gem_sgt_is_contiguous(sgt, size)) {
omap_obj->dma_addr = sg_dma_address(sgt->sgl);
} else {
/* Create pages list from sgt */
-- 
2.36.1

[PATCH] drm: Move radeon and amdgpu Kconfig options into their directories

[Andrew Davis]

Most Kconfig options to enable a driver are in the Kconfig file
inside the relevant directory, move these two to the same.

Signed-off-by: Andrew Davis 
---
 drivers/gpu/drm/Kconfig| 42 --
 drivers/gpu/drm/amd/amdgpu/Kconfig | 22 
 drivers/gpu/drm/radeon/Kconfig | 22 
 3 files changed, 44 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 6c2256e8474b..24fa9ccd92a4 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -234,50 +234,8 @@ source "drivers/gpu/drm/i2c/Kconfig"
 
 source "drivers/gpu/drm/arm/Kconfig"
 
-config DRM_RADEON
-   tristate "ATI Radeon"
-   depends on DRM && PCI && MMU
-   depends on AGP || !AGP
-   select FW_LOADER
-   select DRM_DISPLAY_DP_HELPER
-   select DRM_DISPLAY_HELPER
-select DRM_KMS_HELPER
-select DRM_TTM
-   select DRM_TTM_HELPER
-   select POWER_SUPPLY
-   select HWMON
-   select BACKLIGHT_CLASS_DEVICE
-   select INTERVAL_TREE
-   help
- Choose this option if you have an ATI Radeon graphics card.  There
- are both PCI and AGP versions.  You don't need to choose this to
- run the Radeon in plain VGA mode.
-
- If M is selected, the module will be called radeon.
-
 source "drivers/gpu/drm/radeon/Kconfig"
 
-config DRM_AMDGPU
-   tristate "AMD GPU"
-   depends on DRM && PCI && MMU
-   select FW_LOADER
-   select DRM_DISPLAY_DP_HELPER
-   select DRM_DISPLAY_HDMI_HELPER
-   select DRM_DISPLAY_HELPER
-   select DRM_KMS_HELPER
-   select DRM_SCHED
-   select DRM_TTM
-   select DRM_TTM_HELPER
-   select POWER_SUPPLY
-   select HWMON
-   select BACKLIGHT_CLASS_DEVICE
-   select INTERVAL_TREE
-   select DRM_BUDDY
-   help
- Choose this option if you have a recent AMD Radeon graphics card.
-
- If M is selected, the module will be called amdgpu.
-
 source "drivers/gpu/drm/amd/amdgpu/Kconfig"
 
 source "drivers/gpu/drm/nouveau/Kconfig"
diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig 
b/drivers/gpu/drm/amd/amdgpu/Kconfig
index d55275de..36b1206124cf 100644
--- a/drivers/gpu/drm/amd/amdgpu/Kconfig
+++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
@@ -1,4 +1,26 @@
 # SPDX-License-Identifier: MIT
+
+config DRM_AMDGPU
+   tristate "AMD GPU"
+   depends on DRM && PCI && MMU
+   select FW_LOADER
+   select DRM_DISPLAY_DP_HELPER
+   select DRM_DISPLAY_HDMI_HELPER
+   select DRM_DISPLAY_HELPER
+   select DRM_KMS_HELPER
+   select DRM_SCHED
+   select DRM_TTM
+   select DRM_TTM_HELPER
+   select POWER_SUPPLY
+   select HWMON
+   select BACKLIGHT_CLASS_DEVICE
+   select INTERVAL_TREE
+   select DRM_BUDDY
+   help
+ Choose this option if you have a recent AMD Radeon graphics card.
+
+ If M is selected, the module will be called amdgpu.
+
 config DRM_AMDGPU_SI
bool "Enable amdgpu support for SI parts"
depends on DRM_AMDGPU
diff --git a/drivers/gpu/drm/radeon/Kconfig b/drivers/gpu/drm/radeon/Kconfig
index 52819e7f1fca..3248d12c562d 100644
--- a/drivers/gpu/drm/radeon/Kconfig
+++ b/drivers/gpu/drm/radeon/Kconfig
@@ -1,4 +1,26 @@
 # SPDX-License-Identifier: MIT
+
+config DRM_RADEON
+   tristate "ATI Radeon"
+   depends on DRM && PCI && MMU
+   depends on AGP || !AGP
+   select FW_LOADER
+   select DRM_DISPLAY_DP_HELPER
+   select DRM_DISPLAY_HELPER
+select DRM_KMS_HELPER
+select DRM_TTM
+   select DRM_TTM_HELPER
+   select POWER_SUPPLY
+   select HWMON
+   select BACKLIGHT_CLASS_DEVICE
+   select INTERVAL_TREE
+   help
+ Choose this option if you have an ATI Radeon graphics card.  There
+ are both PCI and AGP versions.  You don't need to choose this to
+ run the Radeon in plain VGA mode.
+
+ If M is selected, the module will be called radeon.
+
 config DRM_RADEON_USERPTR
bool "Always enable userptr support"
depends on DRM_RADEON
-- 
2.36.1

[PATCH] dma-buf: cma_heap: Check for device max segment size when attaching

[Andrew Davis]

Although there is usually not such a limitation (and when there is it is
often only because the driver forgot to change the super small default),
it is still correct here to break scatterlist element into chunks of
dma_max_mapping_size().

This might cause some issues for users with misbehaving drivers. If
bisecting has landed you on this commit, make sure your drivers both set
dma_set_max_seg_size() and are checking for contiguousness correctly.

Signed-off-by: Andrew Davis 
---
 drivers/dma-buf/heaps/cma_heap.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
index 28fb04eccdd0..cacc84cb5ece 100644
--- a/drivers/dma-buf/heaps/cma_heap.c
+++ b/drivers/dma-buf/heaps/cma_heap.c
@@ -58,10 +58,11 @@ static int cma_heap_attach(struct dma_buf *dmabuf,
if (!a)
return -ENOMEM;
 
-   ret = sg_alloc_table_from_pages(>table, buffer->pages,
-   buffer->pagecount, 0,
-   buffer->pagecount << PAGE_SHIFT,
-   GFP_KERNEL);
+   size_t max_segment = dma_get_max_seg_size(attachment->dev);
+   ret = sg_alloc_table_from_pages_segment(>table, buffer->pages,
+   buffer->pagecount, 0,
+   buffer->pagecount << PAGE_SHIFT,
+   max_segment, GFP_KERNEL);
if (ret) {
kfree(a);
return ret;
-- 
2.36.1