[PATCH v2 0/5] drm/panfrost: Add extra GPU-usage flags

[PATCH v2 0/5] drm/panfrost: Add extra GPU-usage flags

[Boris Brezillon]

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-1-boris.brezillon@collabora.com/

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(-)

-- 
2.31.1


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

[PATCH v2 1/5] [RFC]iommu: Add a IOMMU_DEVONLY protection flag

[Boris Brezillon]

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;
-- 
2.31.1


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

Re: [PATCH v2 1/5] [RFC]iommu: Add a IOMMU_DEVONLY protection flag

[Alyssa Rosenzweig]

> 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

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

Re: [PATCH v2 1/5] [RFC]iommu: Add a IOMMU_DEVONLY protection flag

[Joerg Roedel]

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

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

Re: [PATCH v2 1/5] [RFC]iommu: Add a IOMMU_DEVONLY protection flag

[Boris Brezillon]

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

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

[PATCH v2 2/5] [RFC]iommu/io-pgtable-arm: Take the DEVONLY flag into account on ARM_MALI_LPAE

[Boris Brezillon]

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;
-- 
2.31.1


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

[PATCH v2 3/5] drm/panfrost: Add PANFROST_BO_NO{READ,WRITE} flags

[Boris Brezillon]

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.
-- 
2.31.1


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

Re: [PATCH v2 3/5] drm/panfrost: Add PANFROST_BO_NO{READ,WRITE} flags

[Boris Brezillon]

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.


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

[PATCH v2 4/5] drm/panfrost: Add a PANFROST_BO_GPUONLY flag

[Boris Brezillon]

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.
-- 
2.31.1


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

Re: [PATCH v2 4/5] drm/panfrost: Add a PANFROST_BO_GPUONLY flag

[Steven Price]

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.
> 


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

Re: [PATCH v2 4/5] drm/panfrost: Add a PANFROST_BO_GPUONLY flag

[Boris Brezillon]

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).

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

Re: [PATCH v2 4/5] drm/panfrost: Add a PANFROST_BO_GPUONLY flag

[Alyssa Rosenzweig]

> > 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.

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

[PATCH v2 5/5] drm/panfrost: Bump the driver version to 1.3

[Boris Brezillon]

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,
-- 
2.31.1


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