Hello,
This is a follow-up of [1], which was adding the read/write restrictions on GPU buffers. Robin and Steven suggested that I add a flag to restrict the shareability domain on GPU-private buffers, so here it is.
As you can see, the first patch is flagges RFC, since I'm not sure adding a new IOMMU_ flag is the right solution, but IOMMU_CACHE doesn't feel like a good fit either. Please let me know if you have better ideas.
Regards,
Boris
[1]https://patchwork.kernel.org/project/dri-devel/patch/20210930184723.1482426-...
Boris Brezillon (5): [RFC]iommu: Add a IOMMU_DEVONLY protection flag [RFC]iommu/io-pgtable-arm: Take the DEVONLY flag into account on ARM_MALI_LPAE drm/panfrost: Add PANFROST_BO_NO{READ,WRITE} flags drm/panfrost: Add a PANFROST_BO_GPUONLY flag drm/panfrost: Bump the driver version to 1.3
drivers/gpu/drm/panfrost/panfrost_drv.c | 15 +++++++++++++-- drivers/gpu/drm/panfrost/panfrost_gem.c | 3 +++ drivers/gpu/drm/panfrost/panfrost_gem.h | 3 +++ drivers/gpu/drm/panfrost/panfrost_mmu.c | 11 ++++++++++- drivers/iommu/io-pgtable-arm.c | 25 +++++++++++++++++-------- include/linux/iommu.h | 7 +++++++ include/uapi/drm/panfrost_drm.h | 3 +++ 7 files changed, 56 insertions(+), 11 deletions(-)
The IOMMU_DEVONLY flag allows the caller to flag a mappings backed by device-private buffers. That means other devices or CPUs are not expected to access the physical memory region pointed by the mapping, and the MMU driver can safely restrict the shareability domain to the device itself.
Will be used by the ARM MMU driver to flag Mali mappings accessed only by the GPU as Inner-shareable.
Signed-off-by: Boris Brezillon boris.brezillon@collabora.com --- include/linux/iommu.h | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h index d2f3435e7d17..db14781b522f 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -31,6 +31,13 @@ * if the IOMMU page table format is equivalent. */ #define IOMMU_PRIV (1 << 5) +/* + * Mapping is only accessed by the device behind the iommu. That means other + * devices or CPUs are not expected to access this physical memory region, + * and the MMU driver can safely restrict the shareability domain to the + * device itself. + */ +#define IOMMU_DEVONLY (1 << 6)
struct iommu_ops; struct iommu_group;
The IOMMU_DEVONLY flag allows the caller to flag a mappings backed by device-private buffers. That means other devices or CPUs are not expected to access the physical memory region pointed by the mapping, and the MMU driver can safely restrict the shareability domain to the device itself.
Will be used by the ARM MMU driver to flag Mali mappings accessed only by the GPU as Inner-shareable.
Signed-off-by: Boris Brezillon boris.brezillon@collabora.com
include/linux/iommu.h | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h index d2f3435e7d17..db14781b522f 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -31,6 +31,13 @@
- if the IOMMU page table format is equivalent.
*/ #define IOMMU_PRIV (1 << 5) +/*
- Mapping is only accessed by the device behind the iommu. That means other
- devices or CPUs are not expected to access this physical memory region,
- and the MMU driver can safely restrict the shareability domain to the
- device itself.
- */
+#define IOMMU_DEVONLY (1 << 6)
struct iommu_ops; struct iommu_group;
This seems totally reasonable to me, but it is well-known that I'm not on good terms with the iommu subsystem. Let's wait for Robin to NAK :-P
On Fri, Oct 01, 2021 at 04:34:23PM +0200, Boris Brezillon wrote:
+/*
- Mapping is only accessed by the device behind the iommu. That means other
- devices or CPUs are not expected to access this physical memory region,
- and the MMU driver can safely restrict the shareability domain to the
- device itself.
- */
+#define IOMMU_DEVONLY (1 << 6)
I am not entirely happy with the name, how about
IOMMU_DEV_PRIVATE?
PRIV would conflict with IOMMU_PRIV (which should probably also be IOMMU_PRIVILEGED, but thats another problem).
Regards,
Joerg
Hello Joerg,
On Mon, 18 Oct 2021 12:25:38 +0200 Joerg Roedel joro@8bytes.org wrote:
On Fri, Oct 01, 2021 at 04:34:23PM +0200, Boris Brezillon wrote:
+/*
- Mapping is only accessed by the device behind the iommu. That means other
- devices or CPUs are not expected to access this physical memory region,
- and the MMU driver can safely restrict the shareability domain to the
- device itself.
- */
+#define IOMMU_DEVONLY (1 << 6)
I am not entirely happy with the name, how about
IOMMU_DEV_PRIVATE?
Works for me.
PRIV would conflict with IOMMU_PRIV (which should probably also be IOMMU_PRIVILEGED, but thats another problem).
Yeah, IOMMU_PRIV is confusing. I thought I could use that flag before realizing PRIV was for privileged not private, but I'll leave that to someone else :-).
Regards,
Boris
Restrict the shareability domain when mapping buffers that are GPU-visible only.
Signed-off-by: Boris Brezillon boris.brezillon@collabora.com --- Flagged RFC because I'm not sure adding a new flag is the right way to convey the 'dev-private buffer' information. --- drivers/iommu/io-pgtable-arm.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c index dd9e47189d0d..6ac3defb9ae1 100644 --- a/drivers/iommu/io-pgtable-arm.c +++ b/drivers/iommu/io-pgtable-arm.c @@ -450,16 +450,25 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data, << ARM_LPAE_PTE_ATTRINDX_SHIFT); }
- /* - * Also Mali has its own notions of shareability wherein its Inner - * domain covers the cores within the GPU, and its Outer domain is - * "outside the GPU" (i.e. either the Inner or System domain in CPU - * terms, depending on coherency). - */ - if (prot & IOMMU_CACHE && data->iop.fmt != ARM_MALI_LPAE) + if (data->iop.fmt == ARM_MALI_LPAE) { + /* + * Mali has its own notions of shareability wherein its Inner + * domain covers the cores within the GPU, and its Outer domain + * is "outside the GPU" (i.e. either the Inner or System domain + * in CPU terms, depending on coherency). + * If the mapping is only device-visible, we can use the Inner + * domain, otherwise we need to stick to Outer domain + * shareability. + */ + if (prot & IOMMU_DEVONLY) + pte |= ARM_LPAE_PTE_SH_IS; + else + pte |= ARM_LPAE_PTE_SH_OS; + } else if (prot & IOMMU_CACHE) { pte |= ARM_LPAE_PTE_SH_IS; - else + } else { pte |= ARM_LPAE_PTE_SH_OS; + }
if (prot & IOMMU_NOEXEC) pte |= ARM_LPAE_PTE_XN;
So we can create GPU mappings without R/W permissions. Particularly useful to debug corruptions caused by out-of-bound writes.
Signed-off-by: Boris Brezillon boris.brezillon@collabora.com --- drivers/gpu/drm/panfrost/panfrost_drv.c | 11 ++++++++++- drivers/gpu/drm/panfrost/panfrost_gem.c | 2 ++ drivers/gpu/drm/panfrost/panfrost_gem.h | 2 ++ drivers/gpu/drm/panfrost/panfrost_mmu.c | 8 +++++++- include/uapi/drm/panfrost_drm.h | 2 ++ 5 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index 82ad9a67f251..b29ac942ae2d 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -75,6 +75,10 @@ static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct return 0; }
+#define PANFROST_BO_FLAGS \ + (PANFROST_BO_NOEXEC | PANFROST_BO_HEAP | \ + PANFROST_BO_NOREAD | PANFROST_BO_NOWRITE) + static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data, struct drm_file *file) { @@ -84,7 +88,7 @@ static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data, struct panfrost_gem_mapping *mapping;
if (!args->size || args->pad || - (args->flags & ~(PANFROST_BO_NOEXEC | PANFROST_BO_HEAP))) + (args->flags & ~PANFROST_BO_FLAGS)) return -EINVAL;
/* Heaps should never be executable */ @@ -92,6 +96,11 @@ static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data, !(args->flags & PANFROST_BO_NOEXEC)) return -EINVAL;
+ /* Executable implies readable */ + if ((args->flags & PANFROST_BO_NOREAD) && + !(args->flags & PANFROST_BO_NOEXEC)) + return -EINVAL; + bo = panfrost_gem_create_with_handle(file, dev, args->size, args->flags, &args->handle); if (IS_ERR(bo)) diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c index 23377481f4e3..d6c1bb1445f2 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem.c +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c @@ -251,6 +251,8 @@ panfrost_gem_create_with_handle(struct drm_file *file_priv,
bo = to_panfrost_bo(&shmem->base); bo->noexec = !!(flags & PANFROST_BO_NOEXEC); + bo->noread = !!(flags & PANFROST_BO_NOREAD); + bo->nowrite = !!(flags & PANFROST_BO_NOWRITE); bo->is_heap = !!(flags & PANFROST_BO_HEAP);
/* diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h index 8088d5fd8480..6246b5fef446 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem.h +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h @@ -37,6 +37,8 @@ struct panfrost_gem_object { atomic_t gpu_usecount;
bool noexec :1; + bool noread :1; + bool nowrite :1; bool is_heap :1; };
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c index f51d3f791a17..6a5c9d94d6f2 100644 --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c @@ -307,7 +307,7 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping) struct drm_gem_object *obj = &bo->base.base; struct panfrost_device *pfdev = to_panfrost_device(obj->dev); struct sg_table *sgt; - int prot = IOMMU_READ | IOMMU_WRITE; + int prot = 0;
if (WARN_ON(mapping->active)) return 0; @@ -315,6 +315,12 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping) if (bo->noexec) prot |= IOMMU_NOEXEC;
+ if (!bo->nowrite) + prot |= IOMMU_WRITE; + + if (!bo->noread) + prot |= IOMMU_READ; + sgt = drm_gem_shmem_get_pages_sgt(obj); if (WARN_ON(IS_ERR(sgt))) return PTR_ERR(sgt); diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h index 061e700dd06c..a2de81225125 100644 --- a/include/uapi/drm/panfrost_drm.h +++ b/include/uapi/drm/panfrost_drm.h @@ -86,6 +86,8 @@ struct drm_panfrost_wait_bo {
#define PANFROST_BO_NOEXEC 1 #define PANFROST_BO_HEAP 2 +#define PANFROST_BO_NOREAD 4 +#define PANFROST_BO_NOWRITE 8
/** * struct drm_panfrost_create_bo - ioctl argument for creating Panfrost BOs.
On Fri, 1 Oct 2021 16:34:25 +0200 Boris Brezillon boris.brezillon@collabora.com wrote:
So we can create GPU mappings without R/W permissions. Particularly useful to debug corruptions caused by out-of-bound writes.
Signed-off-by: Boris Brezillon boris.brezillon@collabora.com
Oops, forgot:
Reviewed-by: Alyssa Rosenzweig alyssa.rosenzweig@collabora.com Reviewed-by: Steven Price steven.price@arm.com
drivers/gpu/drm/panfrost/panfrost_drv.c | 11 ++++++++++- drivers/gpu/drm/panfrost/panfrost_gem.c | 2 ++ drivers/gpu/drm/panfrost/panfrost_gem.h | 2 ++ drivers/gpu/drm/panfrost/panfrost_mmu.c | 8 +++++++- include/uapi/drm/panfrost_drm.h | 2 ++ 5 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index 82ad9a67f251..b29ac942ae2d 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -75,6 +75,10 @@ static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct return 0; }
+#define PANFROST_BO_FLAGS \
- (PANFROST_BO_NOEXEC | PANFROST_BO_HEAP | \
PANFROST_BO_NOREAD | PANFROST_BO_NOWRITE)static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data, struct drm_file *file) { @@ -84,7 +88,7 @@ static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data, struct panfrost_gem_mapping *mapping;
if (!args->size || args->pad ||
(args->flags & ~(PANFROST_BO_NOEXEC | PANFROST_BO_HEAP)))
(args->flags & ~PANFROST_BO_FLAGS))return -EINVAL;
/* Heaps should never be executable */
@@ -92,6 +96,11 @@ static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data, !(args->flags & PANFROST_BO_NOEXEC)) return -EINVAL;
- /* Executable implies readable */
- if ((args->flags & PANFROST_BO_NOREAD) &&
!(args->flags & PANFROST_BO_NOEXEC))return -EINVAL;- bo = panfrost_gem_create_with_handle(file, dev, args->size, args->flags, &args->handle); if (IS_ERR(bo))
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c index 23377481f4e3..d6c1bb1445f2 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem.c +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c @@ -251,6 +251,8 @@ panfrost_gem_create_with_handle(struct drm_file *file_priv,
bo = to_panfrost_bo(&shmem->base); bo->noexec = !!(flags & PANFROST_BO_NOEXEC);
bo->noread = !!(flags & PANFROST_BO_NOREAD);
bo->nowrite = !!(flags & PANFROST_BO_NOWRITE); bo->is_heap = !!(flags & PANFROST_BO_HEAP);
/*
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h index 8088d5fd8480..6246b5fef446 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem.h +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h @@ -37,6 +37,8 @@ struct panfrost_gem_object { atomic_t gpu_usecount;
bool noexec :1;
- bool noread :1;
- bool nowrite :1; bool is_heap :1;
};
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c index f51d3f791a17..6a5c9d94d6f2 100644 --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c @@ -307,7 +307,7 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping) struct drm_gem_object *obj = &bo->base.base; struct panfrost_device *pfdev = to_panfrost_device(obj->dev); struct sg_table *sgt;
- int prot = IOMMU_READ | IOMMU_WRITE;
int prot = 0;
if (WARN_ON(mapping->active)) return 0;
@@ -315,6 +315,12 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping) if (bo->noexec) prot |= IOMMU_NOEXEC;
- if (!bo->nowrite)
prot |= IOMMU_WRITE;- if (!bo->noread)
prot |= IOMMU_READ;- sgt = drm_gem_shmem_get_pages_sgt(obj); if (WARN_ON(IS_ERR(sgt))) return PTR_ERR(sgt);
diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h index 061e700dd06c..a2de81225125 100644 --- a/include/uapi/drm/panfrost_drm.h +++ b/include/uapi/drm/panfrost_drm.h @@ -86,6 +86,8 @@ struct drm_panfrost_wait_bo {
#define PANFROST_BO_NOEXEC 1 #define PANFROST_BO_HEAP 2 +#define PANFROST_BO_NOREAD 4 +#define PANFROST_BO_NOWRITE 8
/**
- struct drm_panfrost_create_bo - ioctl argument for creating Panfrost BOs.
This lets the driver reduce shareability domain of the MMU mapping, which can in turn reduce access time and save power on cache-coherent systems.
Signed-off-by: Boris Brezillon boris.brezillon@collabora.com --- drivers/gpu/drm/panfrost/panfrost_drv.c | 3 ++- drivers/gpu/drm/panfrost/panfrost_gem.c | 1 + drivers/gpu/drm/panfrost/panfrost_gem.h | 1 + drivers/gpu/drm/panfrost/panfrost_mmu.c | 3 +++ include/uapi/drm/panfrost_drm.h | 1 + 5 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index b29ac942ae2d..b176921b9392 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -77,7 +77,8 @@ static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct
#define PANFROST_BO_FLAGS \ (PANFROST_BO_NOEXEC | PANFROST_BO_HEAP | \ - PANFROST_BO_NOREAD | PANFROST_BO_NOWRITE) + PANFROST_BO_NOREAD | PANFROST_BO_NOWRITE | \ + PANFROST_BO_GPUONLY)
static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data, struct drm_file *file) diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c index d6c1bb1445f2..4b1f85c0b98f 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem.c +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c @@ -254,6 +254,7 @@ panfrost_gem_create_with_handle(struct drm_file *file_priv, bo->noread = !!(flags & PANFROST_BO_NOREAD); bo->nowrite = !!(flags & PANFROST_BO_NOWRITE); bo->is_heap = !!(flags & PANFROST_BO_HEAP); + bo->gpuonly = !!(flags & PANFROST_BO_GPUONLY);
/* * Allocate an id of idr table where the obj is registered diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h index 6246b5fef446..e332d5a4c24f 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem.h +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h @@ -40,6 +40,7 @@ struct panfrost_gem_object { bool noread :1; bool nowrite :1; bool is_heap :1; + bool gpuonly :1; };
struct panfrost_gem_mapping { diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c index 6a5c9d94d6f2..89eee8e80aa5 100644 --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c @@ -321,6 +321,9 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping) if (!bo->noread) prot |= IOMMU_READ;
+ if (bo->gpuonly) + prot |= IOMMU_DEVONLY; + sgt = drm_gem_shmem_get_pages_sgt(obj); if (WARN_ON(IS_ERR(sgt))) return PTR_ERR(sgt); diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h index a2de81225125..538b58b2d095 100644 --- a/include/uapi/drm/panfrost_drm.h +++ b/include/uapi/drm/panfrost_drm.h @@ -88,6 +88,7 @@ struct drm_panfrost_wait_bo { #define PANFROST_BO_HEAP 2 #define PANFROST_BO_NOREAD 4 #define PANFROST_BO_NOWRITE 8 +#define PANFROST_BO_GPUONLY 0x10
/** * struct drm_panfrost_create_bo - ioctl argument for creating Panfrost BOs.
On 01/10/2021 15:34, Boris Brezillon wrote:
This lets the driver reduce shareability domain of the MMU mapping, which can in turn reduce access time and save power on cache-coherent systems.
Signed-off-by: Boris Brezillon boris.brezillon@collabora.com
This seems reasonable to me - it matches the kbase BASE_MEM_COHERENT_SYSTEM (only backwards obviously) and it worked reasonably well for the blob.
But I'm wondering if we need to do anything special to deal with the fact we will now have some non-coherent mappings on an otherwise coherent device.
There are certainly some oddities around how these buffers will be mapped into user space if requested, e.g. panfrost_gem_create_object() sets 'map_wc' based on pfdev->coherent which is arguably wrong for GPUONLY. So there are two things we could consider:
a) Actually prevent user space mapping GPUONLY flagged buffers. Which matches the intention of the name.
b) Attempt to provide user space with the tools to safely interact with the buffers (this is the kbase approach). This does have the benefit of allowing *mostly* GPU access. An example here is the tiler heap where the CPU could zero out as necessary but mostly the GPU has ownership and the CPU never reads the contents. GPUONLY/DEVONLY might not be the best name in that case.
Any thoughts?
Thanks,
Steve
drivers/gpu/drm/panfrost/panfrost_drv.c | 3 ++- drivers/gpu/drm/panfrost/panfrost_gem.c | 1 + drivers/gpu/drm/panfrost/panfrost_gem.h | 1 + drivers/gpu/drm/panfrost/panfrost_mmu.c | 3 +++ include/uapi/drm/panfrost_drm.h | 1 + 5 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index b29ac942ae2d..b176921b9392 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -77,7 +77,8 @@ static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct
#define PANFROST_BO_FLAGS \ (PANFROST_BO_NOEXEC | PANFROST_BO_HEAP | \
PANFROST_BO_NOREAD | PANFROST_BO_NOWRITE)
PANFROST_BO_NOREAD | PANFROST_BO_NOWRITE | \PANFROST_BO_GPUONLY)static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data, struct drm_file *file) diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c index d6c1bb1445f2..4b1f85c0b98f 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem.c +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c @@ -254,6 +254,7 @@ panfrost_gem_create_with_handle(struct drm_file *file_priv, bo->noread = !!(flags & PANFROST_BO_NOREAD); bo->nowrite = !!(flags & PANFROST_BO_NOWRITE); bo->is_heap = !!(flags & PANFROST_BO_HEAP);
bo->gpuonly = !!(flags & PANFROST_BO_GPUONLY);
/*
- Allocate an id of idr table where the obj is registered
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h index 6246b5fef446..e332d5a4c24f 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem.h +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h @@ -40,6 +40,7 @@ struct panfrost_gem_object { bool noread :1; bool nowrite :1; bool is_heap :1;
- bool gpuonly :1;
};
struct panfrost_gem_mapping { diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c index 6a5c9d94d6f2..89eee8e80aa5 100644 --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c @@ -321,6 +321,9 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping) if (!bo->noread) prot |= IOMMU_READ;
- if (bo->gpuonly)
prot |= IOMMU_DEVONLY;- sgt = drm_gem_shmem_get_pages_sgt(obj); if (WARN_ON(IS_ERR(sgt))) return PTR_ERR(sgt);
diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h index a2de81225125..538b58b2d095 100644 --- a/include/uapi/drm/panfrost_drm.h +++ b/include/uapi/drm/panfrost_drm.h @@ -88,6 +88,7 @@ struct drm_panfrost_wait_bo { #define PANFROST_BO_HEAP 2 #define PANFROST_BO_NOREAD 4 #define PANFROST_BO_NOWRITE 8 +#define PANFROST_BO_GPUONLY 0x10
/**
- struct drm_panfrost_create_bo - ioctl argument for creating Panfrost BOs.
On Fri, 1 Oct 2021 16:13:42 +0100 Steven Price steven.price@arm.com wrote:
On 01/10/2021 15:34, Boris Brezillon wrote:
This lets the driver reduce shareability domain of the MMU mapping, which can in turn reduce access time and save power on cache-coherent systems.
Signed-off-by: Boris Brezillon boris.brezillon@collabora.com
This seems reasonable to me - it matches the kbase BASE_MEM_COHERENT_SYSTEM (only backwards obviously) and it worked reasonably well for the blob.
But I'm wondering if we need to do anything special to deal with the fact we will now have some non-coherent mappings on an otherwise coherent device.
There are certainly some oddities around how these buffers will be mapped into user space if requested, e.g. panfrost_gem_create_object() sets 'map_wc' based on pfdev->coherent which is arguably wrong for GPUONLY. So there are two things we could consider:
a) Actually prevent user space mapping GPUONLY flagged buffers. Which matches the intention of the name.
I intended to do that, just forgot to add wrappers around drm_gem_shmem_{mmap,vmap}() to forbid CPU-mappings on gpuonly buffers.
b) Attempt to provide user space with the tools to safely interact with the buffers (this is the kbase approach). This does have the benefit of allowing *mostly* GPU access. An example here is the tiler heap where the CPU could zero out as necessary but mostly the GPU has ownership and the CPU never reads the contents. GPUONLY/DEVONLY might not be the best name in that case.
Uh, right, I forgot we had to zero the tiler heap on Midgard (most of the time done with a WRITE_VALUE job, but there's an exception on some old Midgard GPUs IIRC).
This seems reasonable to me - it matches the kbase BASE_MEM_COHERENT_SYSTEM (only backwards obviously) and it worked reasonably well for the blob.
Oh, is that what that was for? I remember seeing it set on Midgard for varyings. Good to go full circle now.
But I'm wondering if we need to do anything special to deal with the fact we will now have some non-coherent mappings on an otherwise coherent device.
There are certainly some oddities around how these buffers will be mapped into user space if requested, e.g. panfrost_gem_create_object() sets 'map_wc' based on pfdev->coherent which is arguably wrong for GPUONLY. So there are two things we could consider:
a) Actually prevent user space mapping GPUONLY flagged buffers. Which matches the intention of the name.
I intended to do that, just forgot to add wrappers around drm_gem_shmem_{mmap,vmap}() to forbid CPU-mappings on gpuonly buffers.
This feels like the cleaner solution to me.
b) Attempt to provide user space with the tools to safely interact with the buffers (this is the kbase approach). This does have the benefit of allowing *mostly* GPU access. An example here is the tiler heap where the CPU could zero out as necessary but mostly the GPU has ownership and the CPU never reads the contents. GPUONLY/DEVONLY might not be the best name in that case.
Uh, right, I forgot we had to zero the tiler heap on Midgard (most of the time done with a WRITE_VALUE job, but there's an exception on some old Midgard GPUs IIRC).
"Attempt" is the key word here :|
We indeed only touch the tiler heap from the CPU on v4, and life's too short to care about new optimizations for v4. Unless the patch is trivial, my vote is for a) preventing the mappings and only setting GPUONLY on the tiler_heap starting with v5.
Bump the driver version to 1.3 to account for the PANFROST_BO_NO{READ,WRITE,GPUONLY} flags addition.
Signed-off-by: Boris Brezillon boris.brezillon@collabora.com --- drivers/gpu/drm/panfrost/panfrost_drv.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index b176921b9392..5afcec5ab4db 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -530,6 +530,7 @@ DEFINE_DRM_GEM_FOPS(panfrost_drm_driver_fops); * - 1.0 - initial interface * - 1.1 - adds HEAP and NOEXEC flags for CREATE_BO * - 1.2 - adds AFBC_FEATURES query + * - 1.3 - adds PANFROST_BO_NO{READ,WRITE,GPUONLY} flags */ static const struct drm_driver panfrost_drm_driver = { .driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ, @@ -542,7 +543,7 @@ static const struct drm_driver panfrost_drm_driver = { .desc = "panfrost DRM", .date = "20180908", .major = 1, - .minor = 2, + .minor = 3,
.gem_create_object = panfrost_gem_create_object, .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
dri-devel@lists.freedesktop.org
-
Alyssa Rosenzweig -
Boris Brezillon -
Joerg Roedel -
Steven Price