Re: [PATCH v10 07/11] drm/etnaviv: Add support for the dma coherent device
From: Sui Jingfeng
Date: Fri Jun 23 2023 - 08:38:05 EST
Hi,
On 2023/6/23 19:52, Robin Murphy wrote:
On 2023-06-20 10:47, 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-coherent, but they are nearly outdated. So, those are trivial cases.
Nevertheless, kernel space still need to do the 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
platform,
When a buffer is accessed by both the GPU and the CPU, the driver should
prefer ETNA_BO_CACHED over ETNA_BO_WC.
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 compiled shader bin.
Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
Cc: Christian Gmeiner <christian.gmeiner@xxxxxxxxx>
Cc: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
Cc: Daniel Vetter <daniel@xxxxxxxx>
Signed-off-by: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
---
drivers/gpu/drm/etnaviv/etnaviv_drv.c | 35 +++++++++++++++++++++
drivers/gpu/drm/etnaviv/etnaviv_drv.h | 6 ++++
drivers/gpu/drm/etnaviv/etnaviv_gem.c | 22 ++++++++++---
drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 7 ++++-
drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 4 +++
include/uapi/drm/etnaviv_drm.h | 1 +
6 files changed, 70 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 0a365e96d371..d8e788aa16cb 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -5,7 +5,9 @@
#include <linux/component.h>
#include <linux/dma-mapping.h>
+#include <linux/dma-map-ops.h>
/*
* This header is for implementations of dma_map_ops and related code.
* It should not be included in drivers just using the DMA API.
*/
1)
The driver can not pass the compile, without include the
linux/dma-map-ops.h
You may be right, but Lucas suggest using dev_is_dma_coherent() function.
because the dev_is_dma_coherent() function is defined in dma-map-ops.h.
for our platform its:
```
static inline bool dev_is_dma_coherent(struct device *dev)
{
return true;
}
```
2. drm/exynos and drm/msm also include this handler
```
$ cd drivers/gpu/drm
$ find . -name "*.c" -type f | xargs grep "dma-map-ops.h"
./exynos/exynos_drm_dma.c:#include <linux/dma-map-ops.h>
./msm/msm_gem.c:#include <linux/dma-map-ops.h>
```
#include <linux/module.h>
+#include <linux/of_address.h>
#include <linux/of_platform.h>
#include <linux/uaccess.h>
@@ -24,6 +26,34 @@
#include "etnaviv_pci_drv.h"
#include "etnaviv_perfmon.h"
+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))
+ return core_node;
+ }
+
+ return NULL;
+}
+
+static bool etnaviv_is_dma_coherent(struct device *dev)
+{
+ struct device_node *np;
+ bool coherent;
+
+ np = etnaviv_of_first_available_node();
+ if (np) {
+ coherent = of_dma_is_coherent(np);
+ of_node_put(np);
+ } else {
+ coherent = dev_is_dma_coherent(dev);
+ }
Please use device_get_dma_attr() like other well-behaved drivers.
OK, I agree with you.
```
priv->dma_coherent = device_get_dma_attr(&pdev->dev) == DEV_DMA_COHERENT;
```
Lucas, Is above code Snippet looks fine to you?
+
+ return coherent;
+}
+
/*
* etnaviv private data construction and destructions:
*/
@@ -52,6 +82,11 @@ etnaviv_alloc_private(struct device *dev, struct
drm_device *drm)
return ERR_PTR(-ENOMEM);
}
+ priv->dma_coherent = etnaviv_is_dma_coherent(dev);
+
+ if (priv->dma_coherent)
+ drm_info(drm, "%s is dma coherent\n", dev_name(dev));
I'm pretty sure the end-user doesn't care.
OK, i will delete it at the next version.
+
return priv;
}
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
index 9cd72948cfad..644e5712c050 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
@@ -46,6 +46,12 @@ struct etnaviv_drm_private {
struct xarray active_contexts;
u32 next_context_id;
+ /*
+ * If true, the GPU is capable of snooping cpu cache. Here, it
+ * also means that cache coherency is enforced by the hardware.
+ */
+ bool dma_coherent;
+
/* list of GEM objects: */
struct mutex gem_lock;
struct list_head gem_list;
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index b5f73502e3dd..39bdc3774f2d 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -343,6 +343,7 @@ void *etnaviv_gem_vmap(struct drm_gem_object *obj)
static void *etnaviv_gem_vmap_impl(struct etnaviv_gem_object *obj)
{
struct page **pages;
+ pgprot_t prot;
lockdep_assert_held(&obj->lock);
@@ -350,8 +351,19 @@ static void *etnaviv_gem_vmap_impl(struct
etnaviv_gem_object *obj)
if (IS_ERR(pages))
return NULL;
- return vmap(pages, obj->base.size >> PAGE_SHIFT,
- VM_MAP, pgprot_writecombine(PAGE_KERNEL));
+ switch (obj->flags) {
+ case ETNA_BO_CACHED:
+ prot = PAGE_KERNEL;
+ break;
+ case ETNA_BO_UNCACHED:
+ prot = pgprot_noncached(PAGE_KERNEL);
+ break;
+ case ETNA_BO_WC:
+ default:
+ prot = pgprot_writecombine(PAGE_KERNEL);
+ }
+
+ return vmap(pages, obj->base.size >> PAGE_SHIFT, VM_MAP, prot);
}
static inline enum dma_data_direction etnaviv_op_to_dma_dir(u32 op)
@@ -369,6 +381,7 @@ int etnaviv_gem_cpu_prep(struct drm_gem_object
*obj, u32 op,
{
struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj);
struct drm_device *dev = obj->dev;
+ struct etnaviv_drm_private *priv = dev->dev_private;
bool write = !!(op & ETNA_PREP_WRITE);
int ret;
@@ -395,7 +408,7 @@ int etnaviv_gem_cpu_prep(struct drm_gem_object
*obj, u32 op,
return ret == 0 ? -ETIMEDOUT : ret;
}
- if (etnaviv_obj->flags & ETNA_BO_CACHED) {
+ if (!priv->dma_coherent && etnaviv_obj->flags & ETNA_BO_CACHED) {
TBH the existing condition already looks a bit sketchy - even if
userspace has a non-cacheable mapping, a DMA sync may need to do
things other than cache maintenance - but this change certainly isn't
making things any better. If you can demonstrate a *measurable* and
significant overhead from calling dma_sync_sgtable_*() on your
platform when the device is coherent and nothing is bounce-buffered,
then the first thing we can do is look at making that quicker (for
everyone). Otherwise, this seems like the famous bad kind of premature
optimisation.
OK, I agree with you, both you and Lucas told to remove this.
I will remove this at next version.
It doesn't hurt much to the performance. On our platform, the
performance is GPU bound.
Thanks,
Robin.
dma_sync_sgtable_for_cpu(dev->dev, etnaviv_obj->sgt,
etnaviv_op_to_dma_dir(op));
etnaviv_obj->last_cpu_prep_op = op;
@@ -408,8 +421,9 @@ int etnaviv_gem_cpu_fini(struct drm_gem_object *obj)
{
struct drm_device *dev = obj->dev;
struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj);
+ struct etnaviv_drm_private *priv = dev->dev_private;
- if (etnaviv_obj->flags & ETNA_BO_CACHED) {
+ if (!priv->dma_coherent && etnaviv_obj->flags & ETNA_BO_CACHED) {
/* fini without a prep is almost certainly a userspace
error */
WARN_ON(etnaviv_obj->last_cpu_prep_op == 0);
dma_sync_sgtable_for_device(dev->dev, etnaviv_obj->sgt,
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
index 3524b5811682..754126992264 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
@@ -112,11 +112,16 @@ static const struct etnaviv_gem_ops
etnaviv_gem_prime_ops = {
struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct
drm_device *dev,
struct dma_buf_attachment *attach, struct sg_table *sgt)
{
+ struct etnaviv_drm_private *priv = dev->dev_private;
struct etnaviv_gem_object *etnaviv_obj;
size_t size = PAGE_ALIGN(attach->dmabuf->size);
+ u32 cache_flags = ETNA_BO_WC;
int ret, npages;
- ret = etnaviv_gem_new_private(dev, size, ETNA_BO_WC,
+ if (priv->dma_coherent)
+ cache_flags = ETNA_BO_CACHED;
+
+ ret = etnaviv_gem_new_private(dev, size, cache_flags,
&etnaviv_gem_prime_ops, &etnaviv_obj);
if (ret < 0)
return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index d6a21e97feb1..d99ac675ce8b 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -164,6 +164,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;
diff --git a/include/uapi/drm/etnaviv_drm.h
b/include/uapi/drm/etnaviv_drm.h
index af024d90453d..76baf45d7158 100644
--- a/include/uapi/drm/etnaviv_drm.h
+++ b/include/uapi/drm/etnaviv_drm.h
@@ -77,6 +77,7 @@ struct drm_etnaviv_timespec {
#define ETNAVIV_PARAM_GPU_PRODUCT_ID 0x1c
#define ETNAVIV_PARAM_GPU_CUSTOMER_ID 0x1d
#define ETNAVIV_PARAM_GPU_ECO_ID 0x1e
+#define ETNAVIV_PARAM_GPU_COHERENT 0x1f
#define ETNA_MAX_PIPES 4
--
Jingfeng