Re: [PATCH v7 7/7] drm/etnaviv: add support for the dma coherent device
From: Sui Jingfeng
Date: Tue Jun 06 2023 - 14:44:22 EST
Hi, I love your reviews
On 2023/6/7 00:56, Bjorn Helgaas wrote:
On Sat, Jun 03, 2023 at 06:59:43PM +0800, Sui Jingfeng wrote:
From: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
Loongson CPUs maintain cache coherency by hardware, which means that the
data in the CPU cache is identical to the data in main system memory. As
for the peripheral device, most of Loongson chips chose to define the
peripherals as DMA coherent by default, device drivers do not need to
maintain the coherency between a processor and an I/O device manually.
There are exceptions, for LS2K1000 SoC, part of peripheral device can be
configured as dma non-coherent. But there is no released version of such
firmware exist in the market. Peripherals of older ls2k1000 is also DMA
non-conherent, but they are nearly outdated. So, those are trivial cases.
s/dma/DMA/
s/non-conherent/non-coherent/
s/ls2k1000/LS2K1000/
So sharpen eyes, as before. :-)
I guess when you say these are "trivial cases," you mean you don't
care about supporting those devices?
Not exactly, Its just that its kernel side thing.
the kernel side should tell us whether a peripheral device is dma
coherent or not.
I do try to support the LSDC/GC1000 as DMA non-coherent peripheral
device in the past,
It's no fun, Only helps to study knowledge, experiment, testing and
comparison(with the dma coherent case).
It requires me compile the PMON firmware on myself. And flash it to the
ROM the board.
change firmware is complex, there a lot of address windows and cross
bar(control
a access either go the cached path or go the non cached path) setting which
only firmware engineer know about.
If there a user want ask me to do it, I will try to test this driver on
such old chip again.
Now, I believe that the support is *already* there.
As etnaviv already works on DMA non-coherent platform originally.
The DC in Loongson Mips/LoongArch SoC can scanout from cached system RAM.
low-end application relay on CPU software rendering only.
There no need to do something like flush cache(write the data in the cache
back to system ram), invalid the cache. This is pretty convenient.
The old Ingenic SoC(jz4770 for example, mips32) also has vivante gpu
integrated.
but they are dma non-coherent, see more addition material at [1].
Therefore, still need to consider the support from cross(various)
devices perspective.
[1] https://lkml.org/lkml/2021/5/15/177
Nevertheless, kernel space still need to do probe work, because vivante GPU
IP has been integrated into various platform. Hence, this patch add runtime
detection code to probe if a specific gpu is DMA coherent, If the answer is
yes, we are going to utilize such features. On Loongson platfform, When a
buffer is accesed by both the GPU and the CPU, The driver should prefer
ETNA_BO_CACHED over ETNA_BO_WC.
s/gpu/GPU/
s/platfform/platform/
s/accesed/accessed/
OK, will be fixed at next version,
I don't find this on myself. :-(
I guess the only way to discover this coherency attribute is via the
DT "vivante,gc" property? Seems a little weird but I'm really not a
DT person.
I'm not sure it is *only*, but it is very convenient to achieve such a
thing with DT.
Just need to put the "dma-coherent;" or "dma-noncoherent" inside the
"vivante,gc" node.
see of_dma_is_coherent() function.
DT is flexible. But this is just used to describe the hardware, it don't
not changing the hardware.
Put any property only has a influence to the software or driver side.
The hardware still as it is.
Changing hardware to a different working mode probably still need the
firmware(uefi, uboot, pmon etc) to do it
This patch also add a new parameter: etnaviv_param_gpu_coherent, which
allow userspace to know if such a feature is available. Because
write-combined BO is still preferred in some case, especially where don't
need CPU read, for example, uploading shader bin.
...
+static struct device_node *etnaviv_of_first_available_node(void)
+{
+ struct device_node *core_node;
+
+ for_each_compatible_node(core_node, NULL, "vivante,gc") {
+ if (!of_device_is_available(core_node))
+ continue;
+
+ return core_node;
+ }
+
+ return NULL;
Seems like this would be simpler as:
for_each_compatible_node(core_node, NULL, "vivante,gc") {
if (of_device_is_available(core_node))
return core_node;
}
return NULL;
Indeed, I don't realize this when I create this patch.
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -8,6 +8,7 @@
#include <linux/delay.h>
#include <linux/dma-fence.h>
#include <linux/dma-mapping.h>
+#include <linux/dma-map-ops.h>
It looks like this #include might not be needed?
No,
the dev_is_dma_coherent() function is declared and defined in dma-map-ops.h.
if remove it, gcc will complain:
drivers/gpu/drm/etnaviv/etnaviv_drv.c: In function
‘etnaviv_is_dma_coherent’:
drivers/gpu/drm/etnaviv/etnaviv_drv.c:56:14: error: implicit declaration
of function ‘dev_is_dma_coherent’; did you mean
‘etnaviv_is_dma_coherent’? [-Werror=implicit-function-declaration]
56 | coherent = dev_is_dma_coherent(dev);
| ^~~~~~~~~~~~~~~~~~~
You're only adding a
new reference to priv->dma_coherent, which looks like it was added to
etnaviv_drv.h.
#include <linux/module.h>
#include <linux/of_device.h>
#include <linux/platform_device.h>
@@ -164,6 +165,10 @@ int etnaviv_gpu_get_param(struct etnaviv_gpu *gpu, u32 param, u64 *value)
*value = gpu->identity.eco_id;
break;
+ case ETNAVIV_PARAM_GPU_COHERENT:
+ *value = priv->dma_coherent;
+ break;
+
default:
DBG("%s: invalid param: %u", dev_name(gpu->dev), param);
return -EINVAL;
@@ -1861,7 +1866,7 @@ static int etnaviv_gpu_register_irq(struct etnaviv_gpu *gpu, int irq)
gpu->irq = irq;
- dev_info(dev, "IRQ handler registered, irq = %d\n", irq);
+ dev_info(dev, "irq(%d) handler registered\n", irq);
Looks possibly unnecessary, or at least unrelated to this patch.
Indeed, catched by you again.
return 0;
}
--
Jingfeng