[PATCH v2 0/3] Initial Panfrost driver

[PATCH v2 0/3] Initial Panfrost driver

[Rob Herring]

Here's v2 of the panfrost driver. Lots of improvements from the RFC
primarily with support for job hangs resetting the GPU and runtime-pm
thanks to Tomeu.

Several dependencies have been applied already, but the first 2 patches
are the remaining dependencies. We need to take the iommu change via
drm-misc or we need a stable branch.

A git branch is here[1].

Rob

[1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git panfrost-rebase-v2

Rob Herring (3):
  iommu: io-pgtable: Add ARM Mali midgard MMU page table format
  drm: Add a drm_gem_objects_lookup helper
  drm/panfrost: Add initial panfrost driver

 drivers/gpu/drm/Kconfig                      |   2 +
 drivers/gpu/drm/Makefile                     |   1 +
 drivers/gpu/drm/drm_gem.c                    |  46 +-
 drivers/gpu/drm/panfrost/Kconfig             |  14 +
 drivers/gpu/drm/panfrost/Makefile            |  12 +
 drivers/gpu/drm/panfrost/TODO                |  27 +
 drivers/gpu/drm/panfrost/panfrost_devfreq.c  | 191 +++++++
 drivers/gpu/drm/panfrost/panfrost_devfreq.h  |  14 +
 drivers/gpu/drm/panfrost/panfrost_device.c   | 227 ++++++++
 drivers/gpu/drm/panfrost/panfrost_device.h   | 118 ++++
 drivers/gpu/drm/panfrost/panfrost_drv.c      | 484 ++++++++++++++++
 drivers/gpu/drm/panfrost/panfrost_features.h | 309 +++++++++++
 drivers/gpu/drm/panfrost/panfrost_gem.c      |  92 +++
 drivers/gpu/drm/panfrost/panfrost_gem.h      |  29 +
 drivers/gpu/drm/panfrost/panfrost_gpu.c      | 374 +++++++++++++
 drivers/gpu/drm/panfrost/panfrost_gpu.h      |  19 +
 drivers/gpu/drm/panfrost/panfrost_issues.h   | 176 ++++++
 drivers/gpu/drm/panfrost/panfrost_job.c      | 556 +++++++++++++++++++
 drivers/gpu/drm/panfrost/panfrost_job.h      |  51 ++
 drivers/gpu/drm/panfrost/panfrost_mmu.c      | 366 ++++++++++++
 drivers/gpu/drm/panfrost/panfrost_mmu.h      |  17 +
 drivers/gpu/drm/panfrost/panfrost_regs.h     | 298 ++++++++++
 drivers/iommu/io-pgtable-arm.c               |  93 +++-
 drivers/iommu/io-pgtable.c                   |   1 +
 include/drm/drm_gem.h                        |   2 +
 include/linux/io-pgtable.h                   |   7 +
 include/uapi/drm/panfrost_drm.h              | 140 +++++
 27 files changed, 3633 insertions(+), 33 deletions(-)
 create mode 100644 drivers/gpu/drm/panfrost/Kconfig
 create mode 100644 drivers/gpu/drm/panfrost/Makefile
 create mode 100644 drivers/gpu/drm/panfrost/TODO
 create mode 100644 drivers/gpu/drm/panfrost/panfrost_devfreq.c
 create mode 100644 drivers/gpu/drm/panfrost/panfrost_devfreq.h
 create mode 100644 drivers/gpu/drm/panfrost/panfrost_device.c
 create mode 100644 drivers/gpu/drm/panfrost/panfrost_device.h
 create mode 100644 drivers/gpu/drm/panfrost/panfrost_drv.c
 create mode 100644 drivers/gpu/drm/panfrost/panfrost_features.h
 create mode 100644 drivers/gpu/drm/panfrost/panfrost_gem.c
 create mode 100644 drivers/gpu/drm/panfrost/panfrost_gem.h
 create mode 100644 drivers/gpu/drm/panfrost/panfrost_gpu.c
 create mode 100644 drivers/gpu/drm/panfrost/panfrost_gpu.h
 create mode 100644 drivers/gpu/drm/panfrost/panfrost_issues.h
 create mode 100644 drivers/gpu/drm/panfrost/panfrost_job.c
 create mode 100644 drivers/gpu/drm/panfrost/panfrost_job.h
 create mode 100644 drivers/gpu/drm/panfrost/panfrost_mmu.c
 create mode 100644 drivers/gpu/drm/panfrost/panfrost_mmu.h
 create mode 100644 drivers/gpu/drm/panfrost/panfrost_regs.h
 create mode 100644 include/uapi/drm/panfrost_drm.h

--
2.19.1

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

[PATCH v3 1/3] iommu: io-pgtable: Add ARM Mali midgard MMU page table format

[Rob Herring]

ARM Mali midgard GPU is similar to standard 64-bit stage 1 page tables, but
have a few differences. Add a new format type to represent the format. The
input address size is 48-bits and the output address size is 40-bits (and
possibly less?). Note that the later bifrost GPUs follow the standard
64-bit stage 1 format.

The differences in the format compared to 64-bit stage 1 format are:

The 3rd level page entry bits are 0x1 instead of 0x3 for page entries.

The access flags are not read-only and unprivileged, but read and write.
This is similar to stage 2 entries, but the memory attributes field matches
stage 1 being an index.

The nG bit is not set by the vendor driver. This one didn't seem to matter,
but we'll keep it aligned to the vendor driver.

Cc: Will Deacon <will.deacon@arm.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: iommu@lists.linux-foundation.org
Signed-off-by: Rob Herring <robh@kernel.org>
---
Please ack this as I need to apply it to the drm-misc tree. Or we need a
stable branch with this patch.

v3:
- Rework arm_lpae_prot_to_pte as written by Robin
- Fill in complete register values for TRANSTAB and MEMATTR registers


 drivers/iommu/io-pgtable-arm.c | 93 +++++++++++++++++++++++++---------
 drivers/iommu/io-pgtable.c     |  1 +
 include/linux/io-pgtable.h     |  7 +++
 3 files changed, 77 insertions(+), 24 deletions(-)

diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index d3700ec15cbd..98551d0cff59 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -172,6 +172,10 @@
 #define ARM_LPAE_MAIR_ATTR_IDX_CACHE	1
 #define ARM_LPAE_MAIR_ATTR_IDX_DEV	2

+#define ARM_MALI_LPAE_TTBR_ADRMODE_TABLE (3u << 0)
+#define ARM_MALI_LPAE_TTBR_READ_INNER	BIT(2)
+#define ARM_MALI_LPAE_TTBR_SHARE_OUTER	BIT(4)
+
 /* IOPTE accessors */
 #define iopte_deref(pte,d) __va(iopte_to_paddr(pte, d))

@@ -180,11 +184,6 @@

 #define iopte_prot(pte)	((pte) & ARM_LPAE_PTE_ATTR_MASK)

-#define iopte_leaf(pte,l)					\
-	(l == (ARM_LPAE_MAX_LEVELS - 1) ?			\
-		(iopte_type(pte,l) == ARM_LPAE_PTE_TYPE_PAGE) :	\
-		(iopte_type(pte,l) == ARM_LPAE_PTE_TYPE_BLOCK))
-
 struct arm_lpae_io_pgtable {
 	struct io_pgtable	iop;

@@ -198,6 +197,14 @@ struct arm_lpae_io_pgtable {

 typedef u64 arm_lpae_iopte;

+static inline bool iopte_leaf(arm_lpae_iopte pte, int l, enum io_pgtable_fmt fmt)
+{
+	if ((l == (ARM_LPAE_MAX_LEVELS - 1)) && (fmt != ARM_MALI_LPAE))
+		return iopte_type(pte,l) == ARM_LPAE_PTE_TYPE_PAGE;
+
+	return iopte_type(pte,l) == ARM_LPAE_PTE_TYPE_BLOCK;
+}
+
 static arm_lpae_iopte paddr_to_iopte(phys_addr_t paddr,
 				     struct arm_lpae_io_pgtable *data)
 {
@@ -303,12 +310,17 @@ static void __arm_lpae_init_pte(struct arm_lpae_io_pgtable *data,
 	if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_ARM_NS)
 		pte |= ARM_LPAE_PTE_NS;

-	if (lvl == ARM_LPAE_MAX_LEVELS - 1)
-		pte |= ARM_LPAE_PTE_TYPE_PAGE;
-	else
+	if (lvl == ARM_LPAE_MAX_LEVELS - 1) {
+		if (data->iop.fmt == ARM_MALI_LPAE)
+			pte |= ARM_LPAE_PTE_TYPE_BLOCK;
+		else
+			pte |= ARM_LPAE_PTE_TYPE_PAGE;
+	} else
 		pte |= ARM_LPAE_PTE_TYPE_BLOCK;

-	pte |= ARM_LPAE_PTE_AF | ARM_LPAE_PTE_SH_IS;
+	if (data->iop.fmt != ARM_MALI_LPAE)
+		pte |= ARM_LPAE_PTE_AF;
+	pte |= ARM_LPAE_PTE_SH_IS;
 	pte |= paddr_to_iopte(paddr, data);

 	__arm_lpae_set_pte(ptep, pte, &data->iop.cfg);
@@ -321,7 +333,7 @@ static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data,
 {
 	arm_lpae_iopte pte = *ptep;

-	if (iopte_leaf(pte, lvl)) {
+	if (iopte_leaf(pte, lvl, data->iop.fmt)) {
 		/* We require an unmap first */
 		WARN_ON(!selftest_running);
 		return -EEXIST;
@@ -409,7 +421,7 @@ static int __arm_lpae_map(struct arm_lpae_io_pgtable *data, unsigned long iova,
 		__arm_lpae_sync_pte(ptep, cfg);
 	}

-	if (pte && !iopte_leaf(pte, lvl)) {
+	if (pte && !iopte_leaf(pte, lvl, data->iop.fmt)) {
 		cptep = iopte_deref(pte, data);
 	} else if (pte) {
 		/* We require an unmap first */
@@ -429,31 +441,33 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
 	if (data->iop.fmt == ARM_64_LPAE_S1 ||
 	    data->iop.fmt == ARM_32_LPAE_S1) {
 		pte = ARM_LPAE_PTE_nG;
-
 		if (!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))
 			pte |= ARM_LPAE_PTE_AP_RDONLY;
-
 		if (!(prot & IOMMU_PRIV))
 			pte |= ARM_LPAE_PTE_AP_UNPRIV;
-
-		if (prot & IOMMU_MMIO)
-			pte |= (ARM_LPAE_MAIR_ATTR_IDX_DEV
-				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
-		else if (prot & IOMMU_CACHE)
-			pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE
-				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
 	} else {
 		pte = ARM_LPAE_PTE_HAP_FAULT;
 		if (prot & IOMMU_READ)
 			pte |= ARM_LPAE_PTE_HAP_READ;
 		if (prot & IOMMU_WRITE)
 			pte |= ARM_LPAE_PTE_HAP_WRITE;
+	}
+
+	if (data->iop.fmt == ARM_64_LPAE_S2 ||
+	    data->iop.fmt == ARM_32_LPAE_S2) {
 		if (prot & IOMMU_MMIO)
 			pte |= ARM_LPAE_PTE_MEMATTR_DEV;
 		else if (prot & IOMMU_CACHE)
 			pte |= ARM_LPAE_PTE_MEMATTR_OIWB;
 		else
 			pte |= ARM_LPAE_PTE_MEMATTR_NC;
+	} else {
+		if (prot & IOMMU_MMIO)
+			pte |= (ARM_LPAE_MAIR_ATTR_IDX_DEV
+				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
+		else if (prot & IOMMU_CACHE)
+			pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE
+				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
 	}

 	if (prot & IOMMU_NOEXEC)
@@ -511,7 +525,7 @@ static void __arm_lpae_free_pgtable(struct arm_lpae_io_pgtable *data, int lvl,
 	while (ptep != end) {
 		arm_lpae_iopte pte = *ptep++;

-		if (!pte || iopte_leaf(pte, lvl))
+		if (!pte || iopte_leaf(pte, lvl, data->iop.fmt))
 			continue;

 		__arm_lpae_free_pgtable(data, lvl + 1, iopte_deref(pte, data));
@@ -602,7 +616,7 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
 	if (size == ARM_LPAE_BLOCK_SIZE(lvl, data)) {
 		__arm_lpae_set_pte(ptep, 0, &iop->cfg);

-		if (!iopte_leaf(pte, lvl)) {
+		if (!iopte_leaf(pte, lvl, iop->fmt)) {
 			/* Also flush any partial walks */
 			io_pgtable_tlb_add_flush(iop, iova, size,
 						ARM_LPAE_GRANULE(data), false);
@@ -621,7 +635,7 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
 		}

 		return size;
-	} else if (iopte_leaf(pte, lvl)) {
+	} else if (iopte_leaf(pte, lvl, iop->fmt)) {
 		/*
 		 * Insert a table at the next level to map the old region,
 		 * minus the part we want to unmap
@@ -669,7 +683,7 @@ static phys_addr_t arm_lpae_iova_to_phys(struct io_pgtable_ops *ops,
 			return 0;

 		/* Leaf entry? */
-		if (iopte_leaf(pte,lvl))
+		if (iopte_leaf(pte, lvl, data->iop.fmt))
 			goto found_translation;

 		/* Take it to the next level */
@@ -995,6 +1009,32 @@ arm_32_lpae_alloc_pgtable_s2(struct io_pgtable_cfg *cfg, void *cookie)
 	return iop;
 }

+static struct io_pgtable *
+arm_mali_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
+{
+	struct io_pgtable *iop;
+
+	if (cfg->ias != 48 || cfg->oas > 40)
+		return NULL;
+
+	cfg->pgsize_bitmap &= (SZ_4K | SZ_2M | SZ_1G);
+	iop = arm_64_lpae_alloc_pgtable_s1(cfg, cookie);
+	if (iop) {
+		u64 mair, ttbr;
+
+		/* Copy values as union fields overlap */
+		mair = cfg->arm_lpae_s1_cfg.mair[0];
+		ttbr = cfg->arm_lpae_s1_cfg.ttbr[0];
+
+		cfg->arm_mali_lpae_cfg.memattr = mair;
+		cfg->arm_mali_lpae_cfg.transtab = ttbr |
+			ARM_MALI_LPAE_TTBR_READ_INNER |
+			ARM_MALI_LPAE_TTBR_ADRMODE_TABLE;
+	}
+
+	return iop;
+}
+
 struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns = {
 	.alloc	= arm_64_lpae_alloc_pgtable_s1,
 	.free	= arm_lpae_free_pgtable,
@@ -1015,6 +1055,11 @@ struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns = {
 	.free	= arm_lpae_free_pgtable,
 };

+struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns = {
+	.alloc	= arm_mali_lpae_alloc_pgtable,
+	.free	= arm_lpae_free_pgtable,
+};
+
 #ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE_SELFTEST

 static struct io_pgtable_cfg *cfg_cookie;
diff --git a/drivers/iommu/io-pgtable.c b/drivers/iommu/io-pgtable.c
index 93f2880be6c6..5227cfdbb65b 100644
--- a/drivers/iommu/io-pgtable.c
+++ b/drivers/iommu/io-pgtable.c
@@ -30,6 +30,7 @@ io_pgtable_init_table[IO_PGTABLE_NUM_FMTS] = {
 	[ARM_32_LPAE_S2] = &io_pgtable_arm_32_lpae_s2_init_fns,
 	[ARM_64_LPAE_S1] = &io_pgtable_arm_64_lpae_s1_init_fns,
 	[ARM_64_LPAE_S2] = &io_pgtable_arm_64_lpae_s2_init_fns,
+	[ARM_MALI_LPAE] = &io_pgtable_arm_mali_lpae_init_fns,
 #endif
 #ifdef CONFIG_IOMMU_IO_PGTABLE_ARMV7S
 	[ARM_V7S] = &io_pgtable_arm_v7s_init_fns,
diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
index 47d5ae559329..76969a564831 100644
--- a/include/linux/io-pgtable.h
+++ b/include/linux/io-pgtable.h
@@ -12,6 +12,7 @@ enum io_pgtable_fmt {
 	ARM_64_LPAE_S1,
 	ARM_64_LPAE_S2,
 	ARM_V7S,
+	ARM_MALI_LPAE,
 	IO_PGTABLE_NUM_FMTS,
 };

@@ -108,6 +109,11 @@ struct io_pgtable_cfg {
 			u32	nmrr;
 			u32	prrr;
 		} arm_v7s_cfg;
+
+		struct {
+			u64	transtab;
+			u64	memattr;
+		} arm_mali_lpae_cfg;
 	};
 };

@@ -209,5 +215,6 @@ extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns;
 extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns;
 extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns;
 extern struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns;
+extern struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns;

 #endif /* __IO_PGTABLE_H */
--
2.19.1

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

Re: [PATCH v3 1/3] iommu: io-pgtable: Add ARM Mali midgard MMU page table format

[Robin Murphy]

On 01/04/2019 08:47, Rob Herring wrote:
> ARM Mali midgard GPU is similar to standard 64-bit stage 1 page tables, but
> have a few differences. Add a new format type to represent the format. The
> input address size is 48-bits and the output address size is 40-bits (and
> possibly less?). Note that the later bifrost GPUs follow the standard
> 64-bit stage 1 format.
> 
> The differences in the format compared to 64-bit stage 1 format are:
> 
> The 3rd level page entry bits are 0x1 instead of 0x3 for page entries.
> 
> The access flags are not read-only and unprivileged, but read and write.
> This is similar to stage 2 entries, but the memory attributes field matches
> stage 1 being an index.
> 
> The nG bit is not set by the vendor driver. This one didn't seem to matter,
> but we'll keep it aligned to the vendor driver.
> 
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: iommu@lists.linux-foundation.org
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> Please ack this as I need to apply it to the drm-misc tree. Or we need a
> stable branch with this patch.

With the diff below squashed in to address my outstanding style nits,

Acked-by: Robin Murphy <robin.murphy@arm.com>

I don't foresee any conflicting io-pgtable changes to prevent this going 
via DRM, but I'll leave the final say up to Joerg.

Thanks,
Robin.

----->8-----
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 98551d0cff59..55ed039da166 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -197,12 +197,13 @@ struct arm_lpae_io_pgtable {

  typedef u64 arm_lpae_iopte;

-static inline bool iopte_leaf(arm_lpae_iopte pte, int l, enum 
io_pgtable_fmt fmt)
+static inline bool iopte_leaf(arm_lpae_iopte pte, int lvl,
+			      enum io_pgtable_fmt fmt)
  {
-	if ((l == (ARM_LPAE_MAX_LEVELS - 1)) && (fmt != ARM_MALI_LPAE))
-		return iopte_type(pte,l) == ARM_LPAE_PTE_TYPE_PAGE;
+	if (lvl == (ARM_LPAE_MAX_LEVELS - 1) && fmt != ARM_MALI_LPAE)
+		return iopte_type(pte, lvl) == ARM_LPAE_PTE_TYPE_PAGE;

-	return iopte_type(pte,l) == ARM_LPAE_PTE_TYPE_BLOCK;
+	return iopte_type(pte, lvl) == ARM_LPAE_PTE_TYPE_BLOCK;
  }

  static arm_lpae_iopte paddr_to_iopte(phys_addr_t paddr,
@@ -310,13 +311,10 @@ static void __arm_lpae_init_pte(struct 
arm_lpae_io_pgtable *data,
  	if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_ARM_NS)
  		pte |= ARM_LPAE_PTE_NS;

-	if (lvl == ARM_LPAE_MAX_LEVELS - 1) {
-		if (data->iop.fmt == ARM_MALI_LPAE)
-			pte |= ARM_LPAE_PTE_TYPE_BLOCK;
-		else
-			pte |= ARM_LPAE_PTE_TYPE_PAGE;
-	} else
+	if (data->iop.fmt != ARM_MALI_LPAE && lvl != ARM_LPAE_MAX_LEVELS - 1)
  		pte |= ARM_LPAE_PTE_TYPE_BLOCK;
+	else
+		pte |= ARM_LPAE_PTE_TYPE_PAGE;

  	if (data->iop.fmt != ARM_MALI_LPAE)
  		pte |= ARM_LPAE_PTE_AF;

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

Re: [PATCH v3 1/3] iommu: io-pgtable: Add ARM Mali midgard MMU page table format

[Robin Murphy]

On 01/04/2019 20:11, Robin Murphy wrote:
> On 01/04/2019 08:47, Rob Herring wrote:
>> ARM Mali midgard GPU is similar to standard 64-bit stage 1 page 
>> tables, but
>> have a few differences. Add a new format type to represent the format. 
>> The
>> input address size is 48-bits and the output address size is 40-bits (and
>> possibly less?). Note that the later bifrost GPUs follow the standard
>> 64-bit stage 1 format.
>>
>> The differences in the format compared to 64-bit stage 1 format are:
>>
>> The 3rd level page entry bits are 0x1 instead of 0x3 for page entries.
>>
>> The access flags are not read-only and unprivileged, but read and write.
>> This is similar to stage 2 entries, but the memory attributes field 
>> matches
>> stage 1 being an index.
>>
>> The nG bit is not set by the vendor driver. This one didn't seem to 
>> matter,
>> but we'll keep it aligned to the vendor driver.
>>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Robin Murphy <robin.murphy@arm.com>
>> Cc: Joerg Roedel <joro@8bytes.org>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: iommu@lists.linux-foundation.org
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> ---
>> Please ack this as I need to apply it to the drm-misc tree. Or we need a
>> stable branch with this patch.
> 
> With the diff below squashed in to address my outstanding style nits,
> 
> Acked-by: Robin Murphy <robin.murphy@arm.com>
> 
> I don't foresee any conflicting io-pgtable changes to prevent this going 
> via DRM, but I'll leave the final say up to Joerg.

Urgh, sorry, turns out I flipped one condition too many there. On 
reflection I may also forget my clever trick in future and inadvertently 
break it, so it probably warrants a comment. Please supersede my 
previous request with the (actually tested) diff below :)

Thanks,
Robin.

----->8-----
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 98551d0cff59..4f7be5a3e19b 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -197,12 +197,13 @@ struct arm_lpae_io_pgtable {

  typedef u64 arm_lpae_iopte;

-static inline bool iopte_leaf(arm_lpae_iopte pte, int l, enum 
io_pgtable_fmt fmt)
+static inline bool iopte_leaf(arm_lpae_iopte pte, int lvl,
+			      enum io_pgtable_fmt fmt)
  {
-	if ((l == (ARM_LPAE_MAX_LEVELS - 1)) && (fmt != ARM_MALI_LPAE))
-		return iopte_type(pte,l) == ARM_LPAE_PTE_TYPE_PAGE;
+	if (lvl == (ARM_LPAE_MAX_LEVELS - 1) && fmt != ARM_MALI_LPAE)
+		return iopte_type(pte, lvl) == ARM_LPAE_PTE_TYPE_PAGE;

-	return iopte_type(pte,l) == ARM_LPAE_PTE_TYPE_BLOCK;
+	return iopte_type(pte, lvl) == ARM_LPAE_PTE_TYPE_BLOCK;
  }

  static arm_lpae_iopte paddr_to_iopte(phys_addr_t paddr,
@@ -310,12 +311,9 @@ static void __arm_lpae_init_pte(struct 
arm_lpae_io_pgtable *data,
  	if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_ARM_NS)
  		pte |= ARM_LPAE_PTE_NS;

-	if (lvl == ARM_LPAE_MAX_LEVELS - 1) {
-		if (data->iop.fmt == ARM_MALI_LPAE)
-			pte |= ARM_LPAE_PTE_TYPE_BLOCK;
-		else
-			pte |= ARM_LPAE_PTE_TYPE_PAGE;
-	} else
+	if (data->iop.fmt != ARM_MALI_LPAE && lvl == ARM_LPAE_MAX_LEVELS - 1)
+		pte |= ARM_LPAE_PTE_TYPE_PAGE;
+	else
  		pte |= ARM_LPAE_PTE_TYPE_BLOCK;

  	if (data->iop.fmt != ARM_MALI_LPAE)
@@ -452,7 +450,10 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct 
arm_lpae_io_pgtable *data,
  		if (prot & IOMMU_WRITE)
  			pte |= ARM_LPAE_PTE_HAP_WRITE;
  	}
-
+	/*
+	 * Note that this logic is structured to accommodate Mali LPAE
+	 * having stage-1-like attributes but stage-2-like permissions.
+	 */
  	if (data->iop.fmt == ARM_64_LPAE_S2 ||
  	    data->iop.fmt == ARM_32_LPAE_S2) {
  		if (prot & IOMMU_MMIO)

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

Re: [PATCH v3 1/3] iommu: io-pgtable: Add ARM Mali midgard MMU page table format

[Joerg Roedel]

On Mon, Apr 01, 2019 at 08:11:00PM +0100, Robin Murphy wrote:
> With the diff below squashed in to address my outstanding style nits,
> 
> Acked-by: Robin Murphy <robin.murphy@arm.com>
> 
> I don't foresee any conflicting io-pgtable changes to prevent this going via
> DRM, but I'll leave the final say up to Joerg.

No objection from my side with merging this via DRM. With Robin's
concerns addressed:

	Acked-by: Joerg Roedel <jroedel@suse.de>

> 
> Thanks,
> Robin.
> 
> ----->8-----
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 98551d0cff59..55ed039da166 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -197,12 +197,13 @@ struct arm_lpae_io_pgtable {
> 
>  typedef u64 arm_lpae_iopte;
> 
> -static inline bool iopte_leaf(arm_lpae_iopte pte, int l, enum
> io_pgtable_fmt fmt)
> +static inline bool iopte_leaf(arm_lpae_iopte pte, int lvl,
> +			      enum io_pgtable_fmt fmt)
>  {
> -	if ((l == (ARM_LPAE_MAX_LEVELS - 1)) && (fmt != ARM_MALI_LPAE))
> -		return iopte_type(pte,l) == ARM_LPAE_PTE_TYPE_PAGE;
> +	if (lvl == (ARM_LPAE_MAX_LEVELS - 1) && fmt != ARM_MALI_LPAE)
> +		return iopte_type(pte, lvl) == ARM_LPAE_PTE_TYPE_PAGE;
> 
> -	return iopte_type(pte,l) == ARM_LPAE_PTE_TYPE_BLOCK;
> +	return iopte_type(pte, lvl) == ARM_LPAE_PTE_TYPE_BLOCK;
>  }
> 
>  static arm_lpae_iopte paddr_to_iopte(phys_addr_t paddr,
> @@ -310,13 +311,10 @@ static void __arm_lpae_init_pte(struct
> arm_lpae_io_pgtable *data,
>  	if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_ARM_NS)
>  		pte |= ARM_LPAE_PTE_NS;
> 
> -	if (lvl == ARM_LPAE_MAX_LEVELS - 1) {
> -		if (data->iop.fmt == ARM_MALI_LPAE)
> -			pte |= ARM_LPAE_PTE_TYPE_BLOCK;
> -		else
> -			pte |= ARM_LPAE_PTE_TYPE_PAGE;
> -	} else
> +	if (data->iop.fmt != ARM_MALI_LPAE && lvl != ARM_LPAE_MAX_LEVELS - 1)
>  		pte |= ARM_LPAE_PTE_TYPE_BLOCK;
> +	else
> +		pte |= ARM_LPAE_PTE_TYPE_PAGE;
> 
>  	if (data->iop.fmt != ARM_MALI_LPAE)
>  		pte |= ARM_LPAE_PTE_AF;

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

Re: [PATCH v3 1/3] iommu: io-pgtable: Add ARM Mali midgard MMU page table format

[Steven Price]

First let me say congratulations to everyone working on Panfrost - it's
an impressive achievement!

Full disclosure: I used to work on the Mali kbase driver. And have been
playing around with running the Mali user-space blob with the Panfrost
kernel driver.

On 01/04/2019 08:47, Rob Herring wrote:
> ARM Mali midgard GPU is similar to standard 64-bit stage 1 page tables, but
> have a few differences. Add a new format type to represent the format. The
> input address size is 48-bits and the output address size is 40-bits (and
> possibly less?). Note that the later bifrost GPUs follow the standard
> 64-bit stage 1 format.
> 
> The differences in the format compared to 64-bit stage 1 format are:
> 
> The 3rd level page entry bits are 0x1 instead of 0x3 for page entries.
> 
> The access flags are not read-only and unprivileged, but read and write.
> This is similar to stage 2 entries, but the memory attributes field matches
> stage 1 being an index.
> 
> The nG bit is not set by the vendor driver. This one didn't seem to matter,
> but we'll keep it aligned to the vendor driver.

The nG bit should be ignored by the hardware.

The MMU in Midgard/Bifrost has a quirk similar to
IO_PGTABLE_QUIRK_TLBI_ON_MAP - you must perform a cache flush for the
GPU to (reliably) pick up new page table mappings.

You may not have seen this because of the use of the JS_CONFIG_START_MMU
bit - this effectively performs a cache flush and TLB invalidate before
starting a job, however when using a GPU like T760 (e.g. on the Firefly
RK3288) this bit isn't being set. In my testing on the Firefly board I
saw GPU page faults because of this.

There's two options for fixing this - a patch like below adds the quirk
mode to the MMU. Or alternatively always set JS_CONFIG_START_MMU on
jobs. In my testing both options solve the page faults.

To be honest I don't know the reasoning behind kbase making the
JS_CONFIG_START_MMU bit conditional - I'm not aware of any reason why it
can't always be set. My guess is performance, but I haven't benchmarked
the difference between this and JS_CONFIG_START_MMU.

-----8<----------
From e3f75c7f04e43238dfc579029b8c11fb6b4a0c18 Mon Sep 17 00:00:00 2001
From: Steven Price <steven.price@arm.com>
Date: Thu, 4 Apr 2019 15:53:17 +0100
Subject: [PATCH] iommu: io-pgtable: IO_PGTABLE_QUIRK_TLBI_ON_MAP for LPAE

Midgard/Bifrost GPUs require a TLB invalidation when mapping pages,
implement IO_PGTABLE_QUIRK_TLBI_ON_MAP for LPAE iommu page table
formats and add the quirk bit to Panfrost.

Signed-off-by: Steven Price <steven.price@arm.com>
---
 drivers/gpu/drm/panfrost/panfrost_mmu.c |  1 +
 drivers/iommu/io-pgtable-arm.c          | 11 +++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c
b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index f3aad8591cf4..094312074d66 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -343,6 +343,7 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
 	mmu_write(pfdev, MMU_INT_MASK, ~0);

 	pfdev->mmu->pgtbl_cfg = (struct io_pgtable_cfg) {
+		.quirks		= IO_PGTABLE_QUIRK_TLBI_ON_MAP,
 		.pgsize_bitmap	= SZ_4K, // | SZ_2M | SZ_1G),
 		.ias		= 48,
 		.oas		= 40,	/* Should come from dma mask? */
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 84beea1f47a7..45fd7bbdf9aa 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -505,7 +505,13 @@ static int arm_lpae_map(struct io_pgtable_ops *ops,
unsigned long iova,
 	 * Synchronise all PTE updates for the new mapping before there's
 	 * a chance for anything to kick off a table walk for the new iova.
 	 */
-	wmb();
+	if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_TLBI_ON_MAP) {
+		io_pgtable_tlb_add_flush(&data->iop, iova, size,
+					 ARM_LPAE_BLOCK_SIZE(2, data), false);
+		io_pgtable_tlb_sync(&data->iop);
+	} else {
+		wmb();
+	}

 	return ret;
 }
@@ -800,7 +806,8 @@ arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg
*cfg, void *cookie)
 	struct arm_lpae_io_pgtable *data;

 	if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS | IO_PGTABLE_QUIRK_NO_DMA |
-			    IO_PGTABLE_QUIRK_NON_STRICT))
+			    IO_PGTABLE_QUIRK_NON_STRICT |
+			    IO_PGTABLE_QUIRK_TLBI_ON_MAP))
 		return NULL;

 	data = arm_lpae_alloc_pgtable(cfg);
-- 
2.20.1


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

Re: [PATCH v3 1/3] iommu: io-pgtable: Add ARM Mali midgard MMU page table format

[Robin Murphy]

Hi Steve,

On 05/04/2019 10:42, Steven Price wrote:
> First let me say congratulations to everyone working on Panfrost - it's
> an impressive achievement!
> 
> Full disclosure: I used to work on the Mali kbase driver. And have been
> playing around with running the Mali user-space blob with the Panfrost
> kernel driver.
> 
> On 01/04/2019 08:47, Rob Herring wrote:
>> ARM Mali midgard GPU is similar to standard 64-bit stage 1 page tables, but
>> have a few differences. Add a new format type to represent the format. The
>> input address size is 48-bits and the output address size is 40-bits (and
>> possibly less?). Note that the later bifrost GPUs follow the standard
>> 64-bit stage 1 format.
>>
>> The differences in the format compared to 64-bit stage 1 format are:
>>
>> The 3rd level page entry bits are 0x1 instead of 0x3 for page entries.
>>
>> The access flags are not read-only and unprivileged, but read and write.
>> This is similar to stage 2 entries, but the memory attributes field matches
>> stage 1 being an index.
>>
>> The nG bit is not set by the vendor driver. This one didn't seem to matter,
>> but we'll keep it aligned to the vendor driver.
> 
> The nG bit should be ignored by the hardware.
> 
> The MMU in Midgard/Bifrost has a quirk similar to
> IO_PGTABLE_QUIRK_TLBI_ON_MAP - you must perform a cache flush for the
> GPU to (reliably) pick up new page table mappings.
> 
> You may not have seen this because of the use of the JS_CONFIG_START_MMU
> bit - this effectively performs a cache flush and TLB invalidate before
> starting a job, however when using a GPU like T760 (e.g. on the Firefly
> RK3288) this bit isn't being set. In my testing on the Firefly board I
> saw GPU page faults because of this.
> 
> There's two options for fixing this - a patch like below adds the quirk
> mode to the MMU. Or alternatively always set JS_CONFIG_START_MMU on
> jobs. In my testing both options solve the page faults.
> 
> To be honest I don't know the reasoning behind kbase making the
> JS_CONFIG_START_MMU bit conditional - I'm not aware of any reason why it
> can't always be set. My guess is performance, but I haven't benchmarked
> the difference between this and JS_CONFIG_START_MMU.
> 
> -----8<----------
>  From e3f75c7f04e43238dfc579029b8c11fb6b4a0c18 Mon Sep 17 00:00:00 2001
> From: Steven Price <steven.price@arm.com>
> Date: Thu, 4 Apr 2019 15:53:17 +0100
> Subject: [PATCH] iommu: io-pgtable: IO_PGTABLE_QUIRK_TLBI_ON_MAP for LPAE
> 
> Midgard/Bifrost GPUs require a TLB invalidation when mapping pages,
> implement IO_PGTABLE_QUIRK_TLBI_ON_MAP for LPAE iommu page table
> formats and add the quirk bit to Panfrost.
> 
> Signed-off-by: Steven Price <steven.price@arm.com>
> ---
>   drivers/gpu/drm/panfrost/panfrost_mmu.c |  1 +
>   drivers/iommu/io-pgtable-arm.c          | 11 +++++++++--
>   2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index f3aad8591cf4..094312074d66 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -343,6 +343,7 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
>   	mmu_write(pfdev, MMU_INT_MASK, ~0);
> 
>   	pfdev->mmu->pgtbl_cfg = (struct io_pgtable_cfg) {
> +		.quirks		= IO_PGTABLE_QUIRK_TLBI_ON_MAP,
>   		.pgsize_bitmap	= SZ_4K, // | SZ_2M | SZ_1G),
>   		.ias		= 48,
>   		.oas		= 40,	/* Should come from dma mask? */
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 84beea1f47a7..45fd7bbdf9aa 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -505,7 +505,13 @@ static int arm_lpae_map(struct io_pgtable_ops *ops,
> unsigned long iova,
>   	 * Synchronise all PTE updates for the new mapping before there's
>   	 * a chance for anything to kick off a table walk for the new iova.
>   	 */
> -	wmb();
> +	if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_TLBI_ON_MAP) {
> +		io_pgtable_tlb_add_flush(&data->iop, iova, size,
> +					 ARM_LPAE_BLOCK_SIZE(2, data), false);

For correctness (in case this ever ends up used for something with 
VMSA-like invalidation behaviour), the granule would need to be "size" 
here, rather than effectively hard-coded.

However, since Mali's invalidations appear to operate on arbitrary 
ranges, it would probably be a lot more efficient for the driver to 
handle this case directly, by just issuing a single big invalidation 
after the for_each_sg() loop in panfrost_mmu_map().

Robin.

> +		io_pgtable_tlb_sync(&data->iop);
> +	} else {
> +		wmb();
> +	}
> 
>   	return ret;
>   }
> @@ -800,7 +806,8 @@ arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg
> *cfg, void *cookie)
>   	struct arm_lpae_io_pgtable *data;
> 
>   	if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS | IO_PGTABLE_QUIRK_NO_DMA |
> -			    IO_PGTABLE_QUIRK_NON_STRICT))
> +			    IO_PGTABLE_QUIRK_NON_STRICT |
> +			    IO_PGTABLE_QUIRK_TLBI_ON_MAP))
>   		return NULL;
> 
>   	data = arm_lpae_alloc_pgtable(cfg);
> 

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

Re: [PATCH v3 1/3] iommu: io-pgtable: Add ARM Mali midgard MMU page table format

[Steven Price]

On 05/04/2019 10:51, Robin Murphy wrote:
> Hi Steve,
> 
> On 05/04/2019 10:42, Steven Price wrote:
>> First let me say congratulations to everyone working on Panfrost - it's
>> an impressive achievement!
>>
>> Full disclosure: I used to work on the Mali kbase driver. And have been
>> playing around with running the Mali user-space blob with the Panfrost
>> kernel driver.
>>
>> On 01/04/2019 08:47, Rob Herring wrote:
>>> ARM Mali midgard GPU is similar to standard 64-bit stage 1 page
>>> tables, but
>>> have a few differences. Add a new format type to represent the
>>> format. The
>>> input address size is 48-bits and the output address size is 40-bits
>>> (and
>>> possibly less?). Note that the later bifrost GPUs follow the standard
>>> 64-bit stage 1 format.
>>>
>>> The differences in the format compared to 64-bit stage 1 format are:
>>>
>>> The 3rd level page entry bits are 0x1 instead of 0x3 for page entries.
>>>
>>> The access flags are not read-only and unprivileged, but read and write.
>>> This is similar to stage 2 entries, but the memory attributes field
>>> matches
>>> stage 1 being an index.
>>>
>>> The nG bit is not set by the vendor driver. This one didn't seem to
>>> matter,
>>> but we'll keep it aligned to the vendor driver.
>>
>> The nG bit should be ignored by the hardware.
>>
>> The MMU in Midgard/Bifrost has a quirk similar to
>> IO_PGTABLE_QUIRK_TLBI_ON_MAP - you must perform a cache flush for the
>> GPU to (reliably) pick up new page table mappings.
>>
>> You may not have seen this because of the use of the JS_CONFIG_START_MMU
>> bit - this effectively performs a cache flush and TLB invalidate before
>> starting a job, however when using a GPU like T760 (e.g. on the Firefly
>> RK3288) this bit isn't being set. In my testing on the Firefly board I
>> saw GPU page faults because of this.
>>
>> There's two options for fixing this - a patch like below adds the quirk
>> mode to the MMU. Or alternatively always set JS_CONFIG_START_MMU on
>> jobs. In my testing both options solve the page faults.
>>
>> To be honest I don't know the reasoning behind kbase making the
>> JS_CONFIG_START_MMU bit conditional - I'm not aware of any reason why it
>> can't always be set. My guess is performance, but I haven't benchmarked
>> the difference between this and JS_CONFIG_START_MMU.
>>
>> -----8<----------
>>  From e3f75c7f04e43238dfc579029b8c11fb6b4a0c18 Mon Sep 17 00:00:00 2001
>> From: Steven Price <steven.price@arm.com>
>> Date: Thu, 4 Apr 2019 15:53:17 +0100
>> Subject: [PATCH] iommu: io-pgtable: IO_PGTABLE_QUIRK_TLBI_ON_MAP for LPAE
>>
>> Midgard/Bifrost GPUs require a TLB invalidation when mapping pages,
>> implement IO_PGTABLE_QUIRK_TLBI_ON_MAP for LPAE iommu page table
>> formats and add the quirk bit to Panfrost.
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>>   drivers/gpu/drm/panfrost/panfrost_mmu.c |  1 +
>>   drivers/iommu/io-pgtable-arm.c          | 11 +++++++++--
>>   2 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> index f3aad8591cf4..094312074d66 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> @@ -343,6 +343,7 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
>>       mmu_write(pfdev, MMU_INT_MASK, ~0);
>>
>>       pfdev->mmu->pgtbl_cfg = (struct io_pgtable_cfg) {
>> +        .quirks        = IO_PGTABLE_QUIRK_TLBI_ON_MAP,
>>           .pgsize_bitmap    = SZ_4K, // | SZ_2M | SZ_1G),
>>           .ias        = 48,
>>           .oas        = 40,    /* Should come from dma mask? */
>> diff --git a/drivers/iommu/io-pgtable-arm.c
>> b/drivers/iommu/io-pgtable-arm.c
>> index 84beea1f47a7..45fd7bbdf9aa 100644
>> --- a/drivers/iommu/io-pgtable-arm.c
>> +++ b/drivers/iommu/io-pgtable-arm.c
>> @@ -505,7 +505,13 @@ static int arm_lpae_map(struct io_pgtable_ops *ops,
>> unsigned long iova,
>>        * Synchronise all PTE updates for the new mapping before there's
>>        * a chance for anything to kick off a table walk for the new iova.
>>        */
>> -    wmb();
>> +    if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_TLBI_ON_MAP) {
>> +        io_pgtable_tlb_add_flush(&data->iop, iova, size,
>> +                     ARM_LPAE_BLOCK_SIZE(2, data), false);
> 
> For correctness (in case this ever ends up used for something with
> VMSA-like invalidation behaviour), the granule would need to be "size"
> here, rather than effectively hard-coded.

Ah yes - I did rather just copy/paste this from io-pgtable-arm-v7s with
minor fix-ups.

> However, since Mali's invalidations appear to operate on arbitrary
> ranges, it would probably be a lot more efficient for the driver to
> handle this case directly, by just issuing a single big invalidation
> after the for_each_sg() loop in panfrost_mmu_map().

Yes - that would probably be a better option. Although I think
personally I'd lean towards just using JS_CONFIG_START_MMU for most
cases. The only thing that won't handle is modifying the MMU while the
job is running (e.g. faulting in pages). But that can be handled
internally in Panfrost by invalidating the exact region which is being
populated.

Steve

> Robin.
> 
>> +        io_pgtable_tlb_sync(&data->iop);
>> +    } else {
>> +        wmb();
>> +    }
>>
>>       return ret;
>>   }
>> @@ -800,7 +806,8 @@ arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg
>> *cfg, void *cookie)
>>       struct arm_lpae_io_pgtable *data;
>>
>>       if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS |
>> IO_PGTABLE_QUIRK_NO_DMA |
>> -                IO_PGTABLE_QUIRK_NON_STRICT))
>> +                IO_PGTABLE_QUIRK_NON_STRICT |
>> +                IO_PGTABLE_QUIRK_TLBI_ON_MAP))
>>           return NULL;
>>
>>       data = arm_lpae_alloc_pgtable(cfg);
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

Re: [PATCH v3 1/3] iommu: io-pgtable: Add ARM Mali midgard MMU page table format

[Steven Price]

On 05/04/2019 11:36, Steven Price wrote:
> On 05/04/2019 10:51, Robin Murphy wrote:
>> Hi Steve,
>>
>> On 05/04/2019 10:42, Steven Price wrote:
>>> First let me say congratulations to everyone working on Panfrost - it's
>>> an impressive achievement!
>>>
>>> Full disclosure: I used to work on the Mali kbase driver. And have been
>>> playing around with running the Mali user-space blob with the Panfrost
>>> kernel driver.
>>>
>>> On 01/04/2019 08:47, Rob Herring wrote:
>>>> ARM Mali midgard GPU is similar to standard 64-bit stage 1 page
>>>> tables, but
>>>> have a few differences. Add a new format type to represent the
>>>> format. The
>>>> input address size is 48-bits and the output address size is 40-bits
>>>> (and
>>>> possibly less?). Note that the later bifrost GPUs follow the standard
>>>> 64-bit stage 1 format.
>>>>
>>>> The differences in the format compared to 64-bit stage 1 format are:
>>>>
>>>> The 3rd level page entry bits are 0x1 instead of 0x3 for page entries.
>>>>
>>>> The access flags are not read-only and unprivileged, but read and write.
>>>> This is similar to stage 2 entries, but the memory attributes field
>>>> matches
>>>> stage 1 being an index.
>>>>
>>>> The nG bit is not set by the vendor driver. This one didn't seem to
>>>> matter,
>>>> but we'll keep it aligned to the vendor driver.
>>>
>>> The nG bit should be ignored by the hardware.
>>>
>>> The MMU in Midgard/Bifrost has a quirk similar to
>>> IO_PGTABLE_QUIRK_TLBI_ON_MAP - you must perform a cache flush for the
>>> GPU to (reliably) pick up new page table mappings.
>>>
>>> You may not have seen this because of the use of the JS_CONFIG_START_MMU
>>> bit - this effectively performs a cache flush and TLB invalidate before
>>> starting a job, however when using a GPU like T760 (e.g. on the Firefly
>>> RK3288) this bit isn't being set. In my testing on the Firefly board I
>>> saw GPU page faults because of this.
>>>
>>> There's two options for fixing this - a patch like below adds the quirk
>>> mode to the MMU. Or alternatively always set JS_CONFIG_START_MMU on
>>> jobs. In my testing both options solve the page faults.
>>>
>>> To be honest I don't know the reasoning behind kbase making the
>>> JS_CONFIG_START_MMU bit conditional - I'm not aware of any reason why it
>>> can't always be set. My guess is performance, but I haven't benchmarked
>>> the difference between this and JS_CONFIG_START_MMU.
>>>
>>> -----8<----------
>>>  From e3f75c7f04e43238dfc579029b8c11fb6b4a0c18 Mon Sep 17 00:00:00 2001
>>> From: Steven Price <steven.price@arm.com>
>>> Date: Thu, 4 Apr 2019 15:53:17 +0100
>>> Subject: [PATCH] iommu: io-pgtable: IO_PGTABLE_QUIRK_TLBI_ON_MAP for LPAE
>>>
>>> Midgard/Bifrost GPUs require a TLB invalidation when mapping pages,
>>> implement IO_PGTABLE_QUIRK_TLBI_ON_MAP for LPAE iommu page table
>>> formats and add the quirk bit to Panfrost.
>>>
>>> Signed-off-by: Steven Price <steven.price@arm.com>
>>> ---
>>>   drivers/gpu/drm/panfrost/panfrost_mmu.c |  1 +
>>>   drivers/iommu/io-pgtable-arm.c          | 11 +++++++++--
>>>   2 files changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>> b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>> index f3aad8591cf4..094312074d66 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>> @@ -343,6 +343,7 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
>>>       mmu_write(pfdev, MMU_INT_MASK, ~0);
>>>
>>>       pfdev->mmu->pgtbl_cfg = (struct io_pgtable_cfg) {
>>> +        .quirks        = IO_PGTABLE_QUIRK_TLBI_ON_MAP,
>>>           .pgsize_bitmap    = SZ_4K, // | SZ_2M | SZ_1G),
>>>           .ias        = 48,
>>>           .oas        = 40,    /* Should come from dma mask? */
>>> diff --git a/drivers/iommu/io-pgtable-arm.c
>>> b/drivers/iommu/io-pgtable-arm.c
>>> index 84beea1f47a7..45fd7bbdf9aa 100644
>>> --- a/drivers/iommu/io-pgtable-arm.c
>>> +++ b/drivers/iommu/io-pgtable-arm.c
>>> @@ -505,7 +505,13 @@ static int arm_lpae_map(struct io_pgtable_ops *ops,
>>> unsigned long iova,
>>>        * Synchronise all PTE updates for the new mapping before there's
>>>        * a chance for anything to kick off a table walk for the new iova.
>>>        */
>>> -    wmb();
>>> +    if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_TLBI_ON_MAP) {
>>> +        io_pgtable_tlb_add_flush(&data->iop, iova, size,
>>> +                     ARM_LPAE_BLOCK_SIZE(2, data), false);
>>
>> For correctness (in case this ever ends up used for something with
>> VMSA-like invalidation behaviour), the granule would need to be "size"
>> here, rather than effectively hard-coded.
> 
> Ah yes - I did rather just copy/paste this from io-pgtable-arm-v7s with
> minor fix-ups.
> 
>> However, since Mali's invalidations appear to operate on arbitrary
>> ranges, it would probably be a lot more efficient for the driver to
>> handle this case directly, by just issuing a single big invalidation
>> after the for_each_sg() loop in panfrost_mmu_map().
> 
> Yes - that would probably be a better option. Although I think
> personally I'd lean towards just using JS_CONFIG_START_MMU for most
> cases. The only thing that won't handle is modifying the MMU while the
> job is running (e.g. faulting in pages). But that can be handled
> internally in Panfrost by invalidating the exact region which is being
> populated.

I asked around. Apparently there are some interesting issues with
START_MMU on some hardware revisions. So best to follow mali_kbase here
and only use START_MMU on those hardware revisions that mali_kbase does
(what Panfrost is already doing). Which means we'll definitely need this
quirk in some form.

Steve

> 
> Steve
> 
>> Robin.
>>
>>> +        io_pgtable_tlb_sync(&data->iop);
>>> +    } else {
>>> +        wmb();
>>> +    }
>>>
>>>       return ret;
>>>   }
>>> @@ -800,7 +806,8 @@ arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg
>>> *cfg, void *cookie)
>>>       struct arm_lpae_io_pgtable *data;
>>>
>>>       if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS |
>>> IO_PGTABLE_QUIRK_NO_DMA |
>>> -                IO_PGTABLE_QUIRK_NON_STRICT))
>>> +                IO_PGTABLE_QUIRK_NON_STRICT |
>>> +                IO_PGTABLE_QUIRK_TLBI_ON_MAP))
>>>           return NULL;
>>>
>>>       data = arm_lpae_alloc_pgtable(cfg);
>>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 


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

[PATCH v2 2/3] drm: Add a drm_gem_objects_lookup helper

[Rob Herring]

Similar to the single handle drm_gem_object_lookup(),
drm_gem_objects_lookup() takes an array of handles and returns an array
of GEM objects.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: Sean Paul <sean@poorly.run>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/gpu/drm/drm_gem.c | 46 +++++++++++++++++++++++++++++++--------
 include/drm/drm_gem.h     |  2 ++
 2 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 388b3742e562..5c9bff45e5e1 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -663,6 +663,42 @@ void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages,
 }
 EXPORT_SYMBOL(drm_gem_put_pages);
 
+/**
+ * drm_gem_objects_lookup - look up GEM objects from an array of handles
+ * @filp: DRM file private date
+ * @handle: pointer to array of userspace handle
+ * @count: size of handle array
+ * @objs: pointer to array of drm_gem_object pointers
+ *
+ * Returns:
+ *
+ * @objs filled in with GEM object pointers. -ENOENT is returned on a lookup
+ * failure. 0 is returned on success.
+ */
+int drm_gem_objects_lookup(struct drm_file *filp, u32 *handle, int count,
+			   struct drm_gem_object **objs)
+{
+	int i, ret = 0;
+	struct drm_gem_object *obj;
+
+	spin_lock(&filp->table_lock);
+
+	for (i = 0; i < count; i++) {
+		/* Check if we currently have a reference on the object */
+		obj = idr_find(&filp->object_idr, handle[i]);
+		if (!obj) {
+			ret = -ENOENT;
+			break;
+		}
+		drm_gem_object_get(obj);
+		objs[i] = obj;
+	}
+	spin_unlock(&filp->table_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_gem_objects_lookup);
+
 /**
  * drm_gem_object_lookup - look up a GEM object from its handle
  * @filp: DRM file private date
@@ -678,15 +714,7 @@ drm_gem_object_lookup(struct drm_file *filp, u32 handle)
 {
 	struct drm_gem_object *obj;
 
-	spin_lock(&filp->table_lock);
-
-	/* Check if we currently have a reference on the object */
-	obj = idr_find(&filp->object_idr, handle);
-	if (obj)
-		drm_gem_object_get(obj);
-
-	spin_unlock(&filp->table_lock);
-
+	drm_gem_objects_lookup(filp, &handle, 1, &obj);
 	return obj;
 }
 EXPORT_SYMBOL(drm_gem_object_lookup);
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 2955aaab3dca..5404225e0194 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -381,6 +381,8 @@ struct page **drm_gem_get_pages(struct drm_gem_object *obj);
 void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages,
 		bool dirty, bool accessed);
 
+int drm_gem_objects_lookup(struct drm_file *filp, u32 *handle, int count,
+			   struct drm_gem_object **objs);
 struct drm_gem_object *drm_gem_object_lookup(struct drm_file *filp, u32 handle);
 long drm_gem_reservation_object_wait(struct drm_file *filep, u32 handle,
 				    bool wait_all, unsigned long timeout);
-- 
2.19.1


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

Re: [PATCH v2 2/3] drm: Add a drm_gem_objects_lookup helper

[Daniel Vetter]

On Mon, Apr 1, 2019 at 9:47 AM Rob Herring <robh@kernel.org> wrote:
>
> Similar to the single handle drm_gem_object_lookup(),
> drm_gem_objects_lookup() takes an array of handles and returns an array
> of GEM objects.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  drivers/gpu/drm/drm_gem.c | 46 +++++++++++++++++++++++++++++++--------
>  include/drm/drm_gem.h     |  2 ++
>  2 files changed, 39 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 388b3742e562..5c9bff45e5e1 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -663,6 +663,42 @@ void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages,
>  }
>  EXPORT_SYMBOL(drm_gem_put_pages);
>
> +/**
> + * drm_gem_objects_lookup - look up GEM objects from an array of handles
> + * @filp: DRM file private date
> + * @handle: pointer to array of userspace handle
> + * @count: size of handle array
> + * @objs: pointer to array of drm_gem_object pointers
> + *
> + * Returns:
> + *
> + * @objs filled in with GEM object pointers. -ENOENT is returned on a lookup
> + * failure. 0 is returned on success.

Bonus points for adding references between the array and normal lookup
functions to guide people around. Also a comment that the buffers need
to be released with drm_gem_object_put().

> + */
> +int drm_gem_objects_lookup(struct drm_file *filp, u32 *handle, int count,
> +                          struct drm_gem_object **objs)

With a pointer to a pointer I'd expect this function to do the
allocation, but it doesn't. Normal pointer is enough to pass an array.
Also maybe make the handle pointer
const, so it's clear that it's an input parameter.

With the bikesheds addressed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +{
> +       int i, ret = 0;
> +       struct drm_gem_object *obj;
> +
> +       spin_lock(&filp->table_lock);
> +
> +       for (i = 0; i < count; i++) {
> +               /* Check if we currently have a reference on the object */
> +               obj = idr_find(&filp->object_idr, handle[i]);
> +               if (!obj) {
> +                       ret = -ENOENT;
> +                       break;
> +               }
> +               drm_gem_object_get(obj);
> +               objs[i] = obj;
> +       }
> +       spin_unlock(&filp->table_lock);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL(drm_gem_objects_lookup);
> +
>  /**
>   * drm_gem_object_lookup - look up a GEM object from its handle
>   * @filp: DRM file private date
> @@ -678,15 +714,7 @@ drm_gem_object_lookup(struct drm_file *filp, u32 handle)
>  {
>         struct drm_gem_object *obj;
>
> -       spin_lock(&filp->table_lock);
> -
> -       /* Check if we currently have a reference on the object */
> -       obj = idr_find(&filp->object_idr, handle);
> -       if (obj)
> -               drm_gem_object_get(obj);
> -
> -       spin_unlock(&filp->table_lock);
> -
> +       drm_gem_objects_lookup(filp, &handle, 1, &obj);
>         return obj;
>  }
>  EXPORT_SYMBOL(drm_gem_object_lookup);
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index 2955aaab3dca..5404225e0194 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -381,6 +381,8 @@ struct page **drm_gem_get_pages(struct drm_gem_object *obj);
>  void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages,
>                 bool dirty, bool accessed);
>
> +int drm_gem_objects_lookup(struct drm_file *filp, u32 *handle, int count,
> +                          struct drm_gem_object **objs);
>  struct drm_gem_object *drm_gem_object_lookup(struct drm_file *filp, u32 handle);
>  long drm_gem_reservation_object_wait(struct drm_file *filep, u32 handle,
>                                     bool wait_all, unsigned long timeout);
> --
> 2.19.1
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

Re: [PATCH v2 2/3] drm: Add a drm_gem_objects_lookup helper

[Chris Wilson]

Quoting Daniel Vetter (2019-04-01 14:06:48)
> On Mon, Apr 1, 2019 at 9:47 AM Rob Herring <robh@kernel.org> wrote:
> > +{
> > +       int i, ret = 0;
> > +       struct drm_gem_object *obj;
> > +
> > +       spin_lock(&filp->table_lock);
> > +
> > +       for (i = 0; i < count; i++) {
> > +               /* Check if we currently have a reference on the object */
> > +               obj = idr_find(&filp->object_idr, handle[i]);
> > +               if (!obj) {
> > +                       ret = -ENOENT;

Unwind previous drm_gem_object_get(), the caller has no idea how many
were processed before the error.
-Chris

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

Re: [PATCH v2 2/3] drm: Add a drm_gem_objects_lookup helper

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 823 bytes --]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Daniel Vetter (2019-04-01 14:06:48)
>> On Mon, Apr 1, 2019 at 9:47 AM Rob Herring <robh@kernel.org> wrote:
>> > +{
>> > +       int i, ret = 0;
>> > +       struct drm_gem_object *obj;
>> > +
>> > +       spin_lock(&filp->table_lock);
>> > +
>> > +       for (i = 0; i < count; i++) {
>> > +               /* Check if we currently have a reference on the object */
>> > +               obj = idr_find(&filp->object_idr, handle[i]);
>> > +               if (!obj) {
>> > +                       ret = -ENOENT;
>
> Unwind previous drm_gem_object_get(), the caller has no idea how many
> were processed before the error.

I had the same thought, but the pattern we have is that you're loading
into a refcounted struct that will free the BOs when you're done,
anyway.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v2 2/3] drm: Add a drm_gem_objects_lookup helper

[Rob Herring]

On Mon, Apr 1, 2019 at 10:43 AM Eric Anholt <eric@anholt.net> wrote:
>
> Chris Wilson <chris@chris-wilson.co.uk> writes:
>
> > Quoting Daniel Vetter (2019-04-01 14:06:48)
> >> On Mon, Apr 1, 2019 at 9:47 AM Rob Herring <robh@kernel.org> wrote:
> >> > +{
> >> > +       int i, ret = 0;
> >> > +       struct drm_gem_object *obj;
> >> > +
> >> > +       spin_lock(&filp->table_lock);
> >> > +
> >> > +       for (i = 0; i < count; i++) {
> >> > +               /* Check if we currently have a reference on the object */
> >> > +               obj = idr_find(&filp->object_idr, handle[i]);
> >> > +               if (!obj) {
> >> > +                       ret = -ENOENT;
> >
> > Unwind previous drm_gem_object_get(), the caller has no idea how many
> > were processed before the error.
>
> I had the same thought, but the pattern we have is that you're loading
> into a refcounted struct that will free the BOs when you're done,
> anyway.

The real bug here is if allocation of the array fails. The BO array
may be NULL when the count is not. So this V3D cleanup hunk:

for (i = 0; i < exec->bo_count; i++)
  drm_gem_object_put_unlocked(&exec->bo[i]->base.base);
  kvfree(exec->bo);

Needs to be wrapped with 'if (exec->bo)'. We have a similar problem
with fence arrays too.

Thanks to Steven for noticing this copied code in panfrost.

Rob

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

Re: [PATCH v2 2/3] drm: Add a drm_gem_objects_lookup helper

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1483 bytes --]

Rob Herring <robh@kernel.org> writes:

> On Mon, Apr 1, 2019 at 10:43 AM Eric Anholt <eric@anholt.net> wrote:
>>
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>>
>> > Quoting Daniel Vetter (2019-04-01 14:06:48)
>> >> On Mon, Apr 1, 2019 at 9:47 AM Rob Herring <robh@kernel.org> wrote:
>> >> > +{
>> >> > +       int i, ret = 0;
>> >> > +       struct drm_gem_object *obj;
>> >> > +
>> >> > +       spin_lock(&filp->table_lock);
>> >> > +
>> >> > +       for (i = 0; i < count; i++) {
>> >> > +               /* Check if we currently have a reference on the object */
>> >> > +               obj = idr_find(&filp->object_idr, handle[i]);
>> >> > +               if (!obj) {
>> >> > +                       ret = -ENOENT;
>> >
>> > Unwind previous drm_gem_object_get(), the caller has no idea how many
>> > were processed before the error.
>>
>> I had the same thought, but the pattern we have is that you're loading
>> into a refcounted struct that will free the BOs when you're done,
>> anyway.
>
> The real bug here is if allocation of the array fails. The BO array
> may be NULL when the count is not. So this V3D cleanup hunk:
>
> for (i = 0; i < exec->bo_count; i++)
>   drm_gem_object_put_unlocked(&exec->bo[i]->base.base);
>   kvfree(exec->bo);
>
> Needs to be wrapped with 'if (exec->bo)'. We have a similar problem
> with fence arrays too.

Yeah, seems legit.  Not really going to write new patches when I've got
month-old critical patches I can't get acked, though. :/

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v2 2/3] drm: Add a drm_gem_objects_lookup helper

[Rob Herring]

On Mon, Apr 1, 2019 at 8:07 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Mon, Apr 1, 2019 at 9:47 AM Rob Herring <robh@kernel.org> wrote:
> >
> > Similar to the single handle drm_gem_object_lookup(),
> > drm_gem_objects_lookup() takes an array of handles and returns an array
> > of GEM objects.
> >
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > ---
> >  drivers/gpu/drm/drm_gem.c | 46 +++++++++++++++++++++++++++++++--------
> >  include/drm/drm_gem.h     |  2 ++
> >  2 files changed, 39 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 388b3742e562..5c9bff45e5e1 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -663,6 +663,42 @@ void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages,
> >  }
> >  EXPORT_SYMBOL(drm_gem_put_pages);
> >
> > +/**
> > + * drm_gem_objects_lookup - look up GEM objects from an array of handles
> > + * @filp: DRM file private date
> > + * @handle: pointer to array of userspace handle
> > + * @count: size of handle array
> > + * @objs: pointer to array of drm_gem_object pointers
> > + *
> > + * Returns:
> > + *
> > + * @objs filled in with GEM object pointers. -ENOENT is returned on a lookup
> > + * failure. 0 is returned on success.
>
> Bonus points for adding references between the array and normal lookup
> functions to guide people around. Also a comment that the buffers need
> to be released with drm_gem_object_put().
>
> > + */
> > +int drm_gem_objects_lookup(struct drm_file *filp, u32 *handle, int count,
> > +                          struct drm_gem_object **objs)
>
> With a pointer to a pointer I'd expect this function to do the
> allocation, but it doesn't. Normal pointer is enough to pass an array.
> Also maybe make the handle pointer
> const, so it's clear that it's an input parameter.

I thought about doing the allocation here, but then I couldn't
implement the single drm_gem_object_lookup() using this function.
Maybe I can refactor in 3 functions making this one a static function.

Rob

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

Re: [PATCH v2 2/3] drm: Add a drm_gem_objects_lookup helper

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 2508 bytes --]

Rob Herring <robh@kernel.org> writes:

> On Mon, Apr 1, 2019 at 8:07 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>> On Mon, Apr 1, 2019 at 9:47 AM Rob Herring <robh@kernel.org> wrote:
>> >
>> > Similar to the single handle drm_gem_object_lookup(),
>> > drm_gem_objects_lookup() takes an array of handles and returns an array
>> > of GEM objects.
>> >
>> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> > Cc: Maxime Ripard <maxime.ripard@bootlin.com>
>> > Cc: Sean Paul <sean@poorly.run>
>> > Cc: David Airlie <airlied@linux.ie>
>> > Cc: Daniel Vetter <daniel@ffwll.ch>
>> > Signed-off-by: Rob Herring <robh@kernel.org>
>> > ---
>> >  drivers/gpu/drm/drm_gem.c | 46 +++++++++++++++++++++++++++++++--------
>> >  include/drm/drm_gem.h     |  2 ++
>> >  2 files changed, 39 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>> > index 388b3742e562..5c9bff45e5e1 100644
>> > --- a/drivers/gpu/drm/drm_gem.c
>> > +++ b/drivers/gpu/drm/drm_gem.c
>> > @@ -663,6 +663,42 @@ void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages,
>> >  }
>> >  EXPORT_SYMBOL(drm_gem_put_pages);
>> >
>> > +/**
>> > + * drm_gem_objects_lookup - look up GEM objects from an array of handles
>> > + * @filp: DRM file private date
>> > + * @handle: pointer to array of userspace handle
>> > + * @count: size of handle array
>> > + * @objs: pointer to array of drm_gem_object pointers
>> > + *
>> > + * Returns:
>> > + *
>> > + * @objs filled in with GEM object pointers. -ENOENT is returned on a lookup
>> > + * failure. 0 is returned on success.
>>
>> Bonus points for adding references between the array and normal lookup
>> functions to guide people around. Also a comment that the buffers need
>> to be released with drm_gem_object_put().
>>
>> > + */
>> > +int drm_gem_objects_lookup(struct drm_file *filp, u32 *handle, int count,
>> > +                          struct drm_gem_object **objs)
>>
>> With a pointer to a pointer I'd expect this function to do the
>> allocation, but it doesn't. Normal pointer is enough to pass an array.
>> Also maybe make the handle pointer
>> const, so it's clear that it's an input parameter.
>
> I thought about doing the allocation here, but then I couldn't
> implement the single drm_gem_object_lookup() using this function.
> Maybe I can refactor in 3 functions making this one a static function.

FWIW, I was thinking for v3d that I'd probably add a helper that took in
the user's pointer to handles :)

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v2 0/3] Initial Panfrost driver

[Alyssa Rosenzweig]

Nice job!

Patches 1-2 are Acked-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Patch 3 is Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>

Excited to see this mainlined!

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

Re: [PATCH v2 3/3] drm/panfrost: Add initial panfrost driver

[Neil Armstrong]

On 01/04/2019 09:47, Rob Herring wrote:
> This adds the initial driver for panfrost which supports Arm Mali
> Midgard and Bifrost family of GPUs. Currently, only the T860 and
> T760 Midgard GPUs have been tested.
> 
> v2:
> - Add GPU reset on job hangs (Tomeu)
> - Add RuntimePM and devfreq support (Tomeu)
> - Fix T760 support (Tomeu)
> - Add a TODO file (Rob, Tomeu)
> - Support multiple in fences (Tomeu)
> - Drop support for shared fences (Tomeu)
> - Fill in MMU de-init (Rob)
> - Move register definitions back to single header (Rob)
> - Clean-up hardcoded job submit todos (Rob)
> - Implement feature setup based on features/issues (Rob)
> - Add remaining Midgard DT compatible strings (Rob)
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Eric Anholt <eric@anholt.net>
> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> Neil, I've kept your reset support separate for now. Let me know if you
> prefer me to squash it or keep it separate.

You can squash all my changes and add my sign-off.

Neil

> 
>  drivers/gpu/drm/Kconfig                      |   2 +
>  drivers/gpu/drm/Makefile                     |   1 +
>  drivers/gpu/drm/panfrost/Kconfig             |  14 +
>  drivers/gpu/drm/panfrost/Makefile            |  12 +
>  drivers/gpu/drm/panfrost/TODO                |  27 +
>  drivers/gpu/drm/panfrost/panfrost_devfreq.c  | 191 +++++++
>  drivers/gpu/drm/panfrost/panfrost_devfreq.h  |  14 +
>  drivers/gpu/drm/panfrost/panfrost_device.c   | 227 ++++++++
>  drivers/gpu/drm/panfrost/panfrost_device.h   | 118 ++++
>  drivers/gpu/drm/panfrost/panfrost_drv.c      | 484 ++++++++++++++++
>  drivers/gpu/drm/panfrost/panfrost_features.h | 309 +++++++++++
>  drivers/gpu/drm/panfrost/panfrost_gem.c      |  92 +++
>  drivers/gpu/drm/panfrost/panfrost_gem.h      |  29 +
>  drivers/gpu/drm/panfrost/panfrost_gpu.c      | 374 +++++++++++++
>  drivers/gpu/drm/panfrost/panfrost_gpu.h      |  19 +
>  drivers/gpu/drm/panfrost/panfrost_issues.h   | 176 ++++++
>  drivers/gpu/drm/panfrost/panfrost_job.c      | 556 +++++++++++++++++++
>  drivers/gpu/drm/panfrost/panfrost_job.h      |  51 ++
>  drivers/gpu/drm/panfrost/panfrost_mmu.c      | 366 ++++++++++++
>  drivers/gpu/drm/panfrost/panfrost_mmu.h      |  17 +
>  drivers/gpu/drm/panfrost/panfrost_regs.h     | 298 ++++++++++
>  include/uapi/drm/panfrost_drm.h              | 140 +++++
>  22 files changed, 3517 insertions(+)
>  create mode 100644 drivers/gpu/drm/panfrost/Kconfig
>  create mode 100644 drivers/gpu/drm/panfrost/Makefile
>  create mode 100644 drivers/gpu/drm/panfrost/TODO
>  create mode 100644 drivers/gpu/drm/panfrost/panfrost_devfreq.c
>  create mode 100644 drivers/gpu/drm/panfrost/panfrost_devfreq.h
>  create mode 100644 drivers/gpu/drm/panfrost/panfrost_device.c
>  create mode 100644 drivers/gpu/drm/panfrost/panfrost_device.h
>  create mode 100644 drivers/gpu/drm/panfrost/panfrost_drv.c
>  create mode 100644 drivers/gpu/drm/panfrost/panfrost_features.h
>  create mode 100644 drivers/gpu/drm/panfrost/panfrost_gem.c
>  create mode 100644 drivers/gpu/drm/panfrost/panfrost_gem.h
>  create mode 100644 drivers/gpu/drm/panfrost/panfrost_gpu.c
>  create mode 100644 drivers/gpu/drm/panfrost/panfrost_gpu.h
>  create mode 100644 drivers/gpu/drm/panfrost/panfrost_issues.h
>  create mode 100644 drivers/gpu/drm/panfrost/panfrost_job.c
>  create mode 100644 drivers/gpu/drm/panfrost/panfrost_job.h
>  create mode 100644 drivers/gpu/drm/panfrost/panfrost_mmu.c
>  create mode 100644 drivers/gpu/drm/panfrost/panfrost_mmu.h
>  create mode 100644 drivers/gpu/drm/panfrost/panfrost_regs.h
>  create mode 100644 include/uapi/drm/panfrost_drm.h
> 
[...]

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

Re: [PATCH v2 3/3] drm/panfrost: Add initial panfrost driver

[Robin Murphy]

On 01/04/2019 09:24, Neil Armstrong wrote:
> On 01/04/2019 09:47, Rob Herring wrote:
>> This adds the initial driver for panfrost which supports Arm Mali
>> Midgard and Bifrost family of GPUs. Currently, only the T860 and
>> T760 Midgard GPUs have been tested.
>>
>> v2:
>> - Add GPU reset on job hangs (Tomeu)
>> - Add RuntimePM and devfreq support (Tomeu)
>> - Fix T760 support (Tomeu)
>> - Add a TODO file (Rob, Tomeu)
>> - Support multiple in fences (Tomeu)
>> - Drop support for shared fences (Tomeu)
>> - Fill in MMU de-init (Rob)
>> - Move register definitions back to single header (Rob)
>> - Clean-up hardcoded job submit todos (Rob)
>> - Implement feature setup based on features/issues (Rob)
>> - Add remaining Midgard DT compatible strings (Rob)
>>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
>> Cc: Sean Paul <sean@poorly.run>
>> Cc: David Airlie <airlied@linux.ie>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io>
>> Cc: Lyude Paul <lyude@redhat.com>
>> Cc: Eric Anholt <eric@anholt.net>
>> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> ---
>> Neil, I've kept your reset support separate for now. Let me know if you
>> prefer me to squash it or keep it separate.
> 
> You can squash all my changes and add my sign-off.

FWIW, looking at Rob's branch there's a little bug in panfrost_reset_init():

+	if (IS_ERR(pfdev->rstc)) {
+		dev_err(pfdev->dev, "get reset failed %ld\n", PTR_ERR(pfdev->clock));

Looks like that "pfdev->clock" should be "pfdev->rstc".

Robin.

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

Re: [PATCH v2 3/3] drm/panfrost: Add initial panfrost driver

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 11330 bytes --]

Rob Herring <robh@kernel.org> writes:

> This adds the initial driver for panfrost which supports Arm Mali
> Midgard and Bifrost family of GPUs. Currently, only the T860 and
> T760 Midgard GPUs have been tested.
>
> v2:
> - Add GPU reset on job hangs (Tomeu)
> - Add RuntimePM and devfreq support (Tomeu)
> - Fix T760 support (Tomeu)
> - Add a TODO file (Rob, Tomeu)
> - Support multiple in fences (Tomeu)
> - Drop support for shared fences (Tomeu)
> - Fill in MMU de-init (Rob)
> - Move register definitions back to single header (Rob)
> - Clean-up hardcoded job submit todos (Rob)
> - Implement feature setup based on features/issues (Rob)
> - Add remaining Midgard DT compatible strings (Rob)
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Eric Anholt <eric@anholt.net>
> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> Neil, I've kept your reset support separate for now. Let me know if you
> prefer me to squash it or keep it separate.


> +static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
> +{
> +	struct panfrost_device *pfdev = job->pfdev;
> +	unsigned long flags;
> +	u32 cfg;
> +	u64 jc_head = job->jc;
> +	int ret;
> +
> +	panfrost_devfreq_update_utilization(pfdev, js, false);
> +
> +	ret = pm_runtime_get_sync(pfdev->dev);
> +	if (ret < 0)
> +		return;
> +
> +	if (WARN_ON(job_read(pfdev, JS_COMMAND_NEXT(js))))
> +		goto end;
> +
> +	spin_lock_irqsave(&pfdev->hwaccess_lock, flags);
> +
> +	job_write(pfdev, JS_HEAD_NEXT_LO(js), jc_head & 0xFFFFFFFF);
> +	job_write(pfdev, JS_HEAD_NEXT_HI(js), jc_head >> 32);
> +
> +	panfrost_job_write_affinity(pfdev, job->requirements, js);
> +
> +	/* start MMU, medium priority, cache clean/flush on end, clean/flush on
> +	 * start */
> +	// TODO: different address spaces
> +	cfg = JS_CONFIG_THREAD_PRI(8) |
> +		JS_CONFIG_START_FLUSH_CLEAN_INVALIDATE |
> +		JS_CONFIG_END_FLUSH_CLEAN_INVALIDATE;
> +
> +	if (panfrost_has_hw_feature(pfdev, HW_FEATURE_FLUSH_REDUCTION))
> +		cfg |= JS_CONFIG_ENABLE_FLUSH_REDUCTION;
> +
> +	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_10649))
> +		cfg |= JS_CONFIG_START_MMU;
> +
> +	job_write(pfdev, JS_CONFIG_NEXT(js), cfg);
> +
> +	if (panfrost_has_hw_feature(pfdev, HW_FEATURE_FLUSH_REDUCTION))
> +		job_write(pfdev, JS_FLUSH_ID_NEXT(js), job->flush_id);
> +
> +	/* GO ! */
> +	dev_dbg(pfdev->dev, "JS: Submitting atom %p to js[%d] with head=0x%llx",
> +				job, js, jc_head);
> +
> +	job_write(pfdev, JS_COMMAND_NEXT(js), JS_COMMAND_START);
> +
> +	spin_unlock_irqrestore(&pfdev->hwaccess_lock, flags);
> +
> +end:
> +	pm_runtime_mark_last_busy(pfdev->dev);
> +	pm_runtime_put_autosuspend(pfdev->dev);
> +}
> +
> +static void panfrost_acquire_object_fences(struct drm_gem_object **bos,
> +					   int bo_count,
> +					   struct dma_fence **implicit_fences)
> +{
> +	int i;
> +
> +	for (i = 0; i < bo_count; i++)
> +		implicit_fences[i] = reservation_object_get_excl_rcu(bos[i]->resv);
> +}

This is a little bit dodgy for implicit sync if the BOs are shared as
dma-bufs to some other device that distinguishes between shared vs excl
reservations.  (A distinction I think is not worth it, but it's the
interface we have).  If the other device has a read-only job, and you
submit a new job updating it, the other device may get the new contents
when it shouldn't.

However, this is a very minor bug and I don't think it should block
things.

> +int panfrost_job_push(struct panfrost_job *job)
> +{
> +	struct panfrost_device *pfdev = job->pfdev;
> +	int slot = panfrost_job_get_slot(job);
> +	struct drm_sched_entity *entity = &job->file_priv->sched_entity[slot];
> +	struct ww_acquire_ctx acquire_ctx;
> +	int ret = 0;
> +
> +	mutex_lock(&pfdev->sched_lock);
> +
> +	ret = drm_gem_lock_reservations(job->bos, job->bo_count,
> +					    &acquire_ctx);
> +	if (ret) {
> +		mutex_unlock(&pfdev->sched_lock);
> +		return ret;
> +	}
> +
> +	ret = drm_sched_job_init(&job->base, entity, NULL);
> +	if (ret) {
> +		mutex_unlock(&pfdev->sched_lock);
> +		goto unlock;
> +	}
> +
> +	job->render_done_fence = dma_fence_get(&job->base.s_fence->finished);
> +
> +	kref_get(&job->refcount); /* put by scheduler job completion */
> +
> +	drm_sched_entity_push_job(&job->base, entity);
> +
> +	mutex_unlock(&pfdev->sched_lock);
> +
> +	panfrost_acquire_object_fences(job->bos, job->bo_count,
> +				       job->implicit_fences);

I think your implicit fences need to be up above
drm_sched_entity_push_job(), since they might get referenced by the
scheduler any time after push_job.

> +	panfrost_attach_object_fences(job->bos, job->bo_count,
> +				      job->render_done_fence);
> +
> +unlock:
> +	drm_gem_unlock_reservations(job->bos, job->bo_count, &acquire_ctx);
> +
> +	return ret;
> +}

> +static void panfrost_job_timedout(struct drm_sched_job *sched_job)
> +{
> +	struct panfrost_job *job = to_panfrost_job(sched_job);
> +	struct panfrost_device *pfdev = job->pfdev;
> +	int js = panfrost_job_get_slot(job);
> +	int i;
> +
> +	/*
> +	 * If the GPU managed to complete this jobs fence, the timeout is
> +	 * spurious. Bail out.
> +	 */
> +	if (dma_fence_is_signaled(job->done_fence))
> +		return;
> +
> +	dev_err(pfdev->dev, "gpu sched timeout, js=%d, status=0x%x, head=0x%x, tail=0x%x, sched_job=%p",
> +		js,
> +		job_read(pfdev, JS_STATUS(js)),
> +		job_read(pfdev, JS_HEAD_LO(js)),
> +		job_read(pfdev, JS_TAIL_LO(js)),
> +		sched_job);
> +
> +	for (i = 0; i < NUM_JOB_SLOTS; i++)
> +		drm_sched_stop(&pfdev->js->queue[i].sched);
> +
> +	if(sched_job)
> +		drm_sched_increase_karma(sched_job);

whitespace

> +
> +	//panfrost_core_dump(pfdev);

Might want to go over the driver for // comments that should be removed?


> diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h
> new file mode 100644
> index 000000000000..508b9621d9db
> --- /dev/null
> +++ b/include/uapi/drm/panfrost_drm.h
> @@ -0,0 +1,140 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2014-2018 Broadcom
> + * Copyright © 2019 Collabora ltd.
> + */
> +#ifndef _PANFROST_DRM_H_
> +#define _PANFROST_DRM_H_
> +
> +#include "drm.h"
> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif
> +
> +#define DRM_PANFROST_SUBMIT			0x00
> +#define DRM_PANFROST_WAIT_BO			0x01
> +#define DRM_PANFROST_CREATE_BO			0x02
> +#define DRM_PANFROST_MMAP_BO			0x03
> +#define DRM_PANFROST_GET_PARAM			0x04
> +#define DRM_PANFROST_GET_BO_OFFSET		0x05
> +
> +#define DRM_IOCTL_PANFROST_SUBMIT		DRM_IOW(DRM_COMMAND_BASE + DRM_PANFROST_SUBMIT, struct drm_panfrost_submit)
> +#define DRM_IOCTL_PANFROST_WAIT_BO		DRM_IOW(DRM_COMMAND_BASE + DRM_PANFROST_WAIT_BO, struct drm_panfrost_wait_bo)
> +#define DRM_IOCTL_PANFROST_CREATE_BO		DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_CREATE_BO, struct drm_panfrost_create_bo)
> +#define DRM_IOCTL_PANFROST_MMAP_BO		DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_MMAP_BO, struct drm_panfrost_mmap_bo)
> +#define DRM_IOCTL_PANFROST_GET_PARAM		DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_GET_PARAM, struct drm_panfrost_get_param)
> +#define DRM_IOCTL_PANFROST_GET_BO_OFFSET	DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_GET_BO_OFFSET, struct drm_panfrost_get_bo_offset)
> +
> +#define PANFROST_JD_REQ_FS (1 << 0)
> +/**
> + * struct drm_panfrost_submit - ioctl argument for submitting commands to the 3D
> + * engine.
> + *
> + * This asks the kernel to have the GPU execute a render command list.
> + */
> +struct drm_panfrost_submit {
> +
> +	/** Address to GPU mapping of job descriptor */
> +	__u64 jc;
> +
> +	/** An optional array of sync objects to wait on before starting this job. */
> +	__u64 in_syncs;
> +
> +	/** Number of sync objects to wait on before starting this job. */
> +	__u32 in_sync_count;
> +
> +	/** An optional sync object to place the completion fence in. */
> +	__u32 out_sync;
> +
> +	/** Pointer to a u32 array of the BOs that are referenced by the job. */
> +	__u64 bo_handles;
> +
> +	/** Number of BO handles passed in (size is that times 4). */
> +	__u32 bo_handle_count;
> +
> +	/** A combination of PANFROST_JD_REQ_* */
> +	__u32 requirements;
> +};
> +
> +/**
> + * struct drm_panfrost_wait_bo - ioctl argument for waiting for
> + * completion of the last DRM_PANFROST_SUBMIT_CL on a BO.
> + *
> + * This is useful for cases where multiple processes might be
> + * rendering to a BO and you want to wait for all rendering to be
> + * completed.
> + */
> +struct drm_panfrost_wait_bo {
> +	__u32 handle;
> +	__u32 pad;
> +	__s64 timeout_ns;	/* absolute */
> +};
> +
> +/**
> + * struct drm_panfrost_create_bo - ioctl argument for creating Panfrost BOs.
> + *
> + * There are currently no values for the flags argument, but it may be
> + * used in a future extension.
> + */
> +struct drm_panfrost_create_bo {
> +	__u32 size;
> +	__u32 flags;
> +	/** Returned GEM handle for the BO. */
> +	__u32 handle;
> +	/**
> +	 * Returned offset for the BO in the GPU address space.  This offset
> +	 * is private to the DRM fd and is valid for the lifetime of the GEM
> +	 * handle.
> +	 *
> +	 * This offset value will always be nonzero, since various HW
> +	 * units treat 0 specially.
> +	 */
> +	__u64 offset;
> +};

I think you've got a u64 alignment issue here that needs explicit
padding so that you don't end up needing 32-vs-64 ioctl wrappers..

This and the implicit fence acquisition race are the two things I think
need fixing, then you can add:

Reviewed-by: Eric Anholt <eric@anholt.net>

The rest is just suggestions for later.

> +
> +/**
> + * struct drm_panfrost_mmap_bo - ioctl argument for mapping Panfrost BOs.
> + *
> + * This doesn't actually perform an mmap.  Instead, it returns the
> + * offset you need to use in an mmap on the DRM device node.  This
> + * means that tools like valgrind end up knowing about the mapped
> + * memory.
> + *
> + * There are currently no values for the flags argument, but it may be
> + * used in a future extension.
> + */
> +struct drm_panfrost_mmap_bo {
> +	/** Handle for the object being mapped. */
> +	__u32 handle;
> +	__u32 flags;
> +	/** offset into the drm node to use for subsequent mmap call. */
> +	__u64 offset;
> +};
> +
> +enum drm_panfrost_param {
> +	DRM_PANFROST_PARAM_GPU_ID,
> +};
> +
> +struct drm_panfrost_get_param {
> +	__u32 param;
> +	__u32 pad;
> +	__u64 value;
> +};
> +
> +/**
> + * Returns the offset for the BO in the GPU address space for this DRM fd.
> + * This is the same value returned by drm_panfrost_create_bo, if that was called
> + * from this DRM fd.
> + */
> +struct drm_panfrost_get_bo_offset {
> +	__u32 handle;
> +	__u32 pad;
> +	__u64 offset;
> +};
> +
> +#if defined(__cplusplus)
> +}
> +#endif
> +
> +#endif /* _PANFROST_DRM_H_ */
> --
> 2.19.1

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v2 3/3] drm/panfrost: Add initial panfrost driver

[Robin Murphy]

On 01/04/2019 08:47, Rob Herring wrote:
> This adds the initial driver for panfrost which supports Arm Mali
> Midgard and Bifrost family of GPUs. Currently, only the T860 and
> T760 Midgard GPUs have been tested.

FWIW, on an antique T624 (Juno) it seems to work no worse than the kbase 
driver plus panfrost-nondrm, which is to say it gets far enough to prove 
that the userspace definitely doesn't support T624 (kmscube manages to 
show a grey background, but the GPU is constantly falling over with page 
faults trying to dereference address 0 - for obvious reasons I'm not 
going to get any further involved in debugging that).

A couple of discoveries and general observations below.

> v2:
> - Add GPU reset on job hangs (Tomeu)
> - Add RuntimePM and devfreq support (Tomeu)
> - Fix T760 support (Tomeu)
> - Add a TODO file (Rob, Tomeu)
> - Support multiple in fences (Tomeu)
> - Drop support for shared fences (Tomeu)
> - Fill in MMU de-init (Rob)
> - Move register definitions back to single header (Rob)
> - Clean-up hardcoded job submit todos (Rob)
> - Implement feature setup based on features/issues (Rob)
> - Add remaining Midgard DT compatible strings (Rob)
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Eric Anholt <eric@anholt.net>
> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
[...]
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> new file mode 100644
> index 000000000000..227ba5202a6f
> --- /dev/null
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -0,0 +1,227 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright 2018 Marty E. Plummer <hanetzer@startmail.com> */
> +/* Copyright 2019 Linaro, Ltd, Rob Herring <robh@kernel.org> */
> +
> +#include <linux/clk.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include "panfrost_device.h"
> +#include "panfrost_devfreq.h"
> +#include "panfrost_features.h"
> +#include "panfrost_gpu.h"
> +#include "panfrost_job.h"
> +#include "panfrost_mmu.h"
> +
> +static int panfrost_clk_init(struct panfrost_device *pfdev)
> +{
> +	int err;
> +	unsigned long rate;
> +
> +	pfdev->clock = devm_clk_get(pfdev->dev, NULL);
> +	if (IS_ERR(pfdev->clock)) {

The DT binding says clocks are optional, but this doesn't treat them as 
such.

> +		dev_err(pfdev->dev, "get clock failed %ld\n", PTR_ERR(pfdev->clock));
> +		return PTR_ERR(pfdev->clock);
> +	}
> +
> +	rate = clk_get_rate(pfdev->clock);
> +	dev_info(pfdev->dev, "clock rate = %lu\n", rate);
> +
> +	err = clk_prepare_enable(pfdev->clock);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
[...]
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> new file mode 100644
> index 000000000000..57a99032bcc6
> --- /dev/null
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
[...]
> +static int panfrost_probe(struct platform_device *pdev)
> +{
> +	struct panfrost_device *pfdev;
> +	struct drm_device *ddev;
> +	int err;
> +
> +	pfdev = devm_kzalloc(&pdev->dev, sizeof(*pfdev), GFP_KERNEL);
> +	if (!pfdev)
> +		return -ENOMEM;
> +
> +	pfdev->pdev = pdev;
> +	pfdev->dev = &pdev->dev;
> +
> +	platform_set_drvdata(pdev, pfdev);
> +
> +	/* Allocate and initialze the DRM device. */
> +	ddev = drm_dev_alloc(&panfrost_drm_driver, &pdev->dev);
> +	if (IS_ERR(ddev))
> +		return PTR_ERR(ddev);
> +
> +	ddev->dev_private = pfdev;
> +	pfdev->ddev = ddev;
> +
> +	spin_lock_init(&pfdev->mm_lock);
> +
> +	/* 4G enough for now. can be 48-bit */
> +	drm_mm_init(&pfdev->mm, SZ_32M >> PAGE_SHIFT, SZ_4G);

You probably want a dma_set_mask_and_coherent() call for your 'real' 
output address size somewhere - the default 32-bit mask works out OK for 
RK3399, but on systems with RAM above 4GB io-pgtable will get very 
unhappy about DMA bounce-buffering.

> +
> +	pm_runtime_use_autosuspend(pfdev->dev);
> +	pm_runtime_set_autosuspend_delay(pfdev->dev, 50); /* ~3 frames */
> +	pm_runtime_enable(pfdev->dev);
> +
> +	err = panfrost_device_init(pfdev);
> +	if (err) {
> +		dev_err(&pdev->dev, "Fatal error during GPU init\n");
> +		goto err_out0;
> +	}
> +
> +	err = panfrost_devfreq_init(pfdev);
> +	if (err) {
> +		dev_err(&pdev->dev, "Fatal error during devfreq init\n");
> +		goto err_out1;
> +	}
> +
> +	/*
> +	 * Register the DRM device with the core and the connectors with
> +	 * sysfs
> +	 */
> +	err = drm_dev_register(ddev, 0);
> +	if (err < 0)
> +		goto err_out1;
> +
> +	return 0;
> +
> +err_out1:
> +	panfrost_device_fini(pfdev);
> +err_out0:
> +	drm_dev_put(ddev);

Reloading the module after a failed probe complains about an unbalanced 
pm_runtime_enable(), so I guess you need a disable somewhere around here.

> +	return err;
> +}
> +
> +static int panfrost_remove(struct platform_device *pdev)
> +{
> +	struct panfrost_device *pfdev = platform_get_drvdata(pdev);
> +	struct drm_device *ddev = pfdev->ddev;
> +
> +	drm_dev_unregister(ddev);
> +	pm_runtime_get_sync(pfdev->dev);
> +	pm_runtime_put_sync_autosuspend(pfdev->dev);
> +	pm_runtime_disable(pfdev->dev);
> +	panfrost_device_fini(pfdev);
> +	drm_dev_put(ddev);
> +	return 0;
> +}
> +
> +static const struct of_device_id dt_match[] = {
> +	{ .compatible = "arm,mali-t604" },
> +	{ .compatible = "arm,mali-t624" },
> +	{ .compatible = "arm,mali-t628" },
> +	{ .compatible = "arm,mali-t720" },
> +	{ .compatible = "arm,mali-t760" },
> +	{ .compatible = "arm,mali-t820" },
> +	{ .compatible = "arm,mali-t830" },
> +	{ .compatible = "arm,mali-t860" },
> +	{ .compatible = "arm,mali-t880" },

Any chance of resurrecting the generic "arm,mali-midgard" compatible? :P

> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, dt_match);
> +
> +static const struct dev_pm_ops panfrost_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> +	SET_RUNTIME_PM_OPS(panfrost_device_suspend, panfrost_device_resume, NULL)
> +};
> +
> +static struct platform_driver panfrost_driver = {
> +	.probe		= panfrost_probe,
> +	.remove		= panfrost_remove,
> +	.driver		= {
> +		.name	= "panfrost",
> +		.pm	= &panfrost_pm_ops,
> +		.of_match_table = dt_match,
> +	},
> +};
> +module_platform_driver(panfrost_driver);
> +
> +MODULE_AUTHOR("Panfrost Project Developers");
> +MODULE_DESCRIPTION("Panfrost DRM Driver");
> +MODULE_LICENSE("GPL v2");
[...]
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> new file mode 100644
> index 000000000000..867e2ba3a761
> --- /dev/null
> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
[...]
> +static void panfrost_gpu_init_quirks(struct panfrost_device *pfdev)
> +{
> +	u32 quirks = 0;
> +
> +	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8443) ||
> +	    panfrost_has_hw_issue(pfdev, HW_ISSUE_11035))
> +		quirks |= SC_LS_PAUSEBUFFER_DISABLE;
> +
> +	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_10327))
> +		quirks |= SC_SDC_DISABLE_OQ_DISCARD;
> +
> +	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_10797))
> +		quirks |= SC_ENABLE_TEXGRD_FLAGS;
> +
> +	if (!panfrost_has_hw_issue(pfdev, GPUCORE_1619)) {
> +		if (panfrost_model_cmp(pfdev, 0x750) < 0) /* T60x, T62x, T72x */
> +			quirks |= SC_LS_ATTR_CHECK_DISABLE;
> +		else if (panfrost_model_cmp(pfdev, 0x880) <= 0) /* T76x, T8xx */
> +			quirks |= SC_LS_ALLOW_ATTR_TYPES;
> +	}
> +
> +	if (panfrost_has_hw_feature(pfdev, HW_FEATURE_TLS_HASHING))
> +		quirks |= SC_TLS_HASH_ENABLE;
> +
> +	if (quirks)
> +		gpu_write(pfdev, GPU_SHADER_CONFIG, quirks);
> +
> +
> +	quirks = gpu_read(pfdev, GPU_TILER_CONFIG);
> +
> +	/* Set tiler clock gate override if required */
> +	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_T76X_3953))
> +		quirks |= TC_CLOCK_GATE_OVERRIDE;
> +
> +	gpu_write(pfdev, GPU_TILER_CONFIG, quirks);
> +
> +
> +	quirks = gpu_read(pfdev, GPU_L2_MMU_CONFIG);
> +
> +	/* Limit read & write ID width for AXI */
> +	if (panfrost_has_hw_feature(pfdev, HW_FEATURE_3BIT_EXT_RW_L2_MMU_CONFIG))
> +		quirks &= ~(L2_MMU_CONFIG_3BIT_LIMIT_EXTERNAL_READS |
> +			    L2_MMU_CONFIG_3BIT_LIMIT_EXTERNAL_WRITES);
> +	else
> +		quirks &= ~(L2_MMU_CONFIG_LIMIT_EXTERNAL_READS |
> +			    L2_MMU_CONFIG_LIMIT_EXTERNAL_WRITES);
> +
> +#if 0
> +	if (kbdev->system_coherency == COHERENCY_ACE) {
> +		/* Allow memory configuration disparity to be ignored, we
> +		 * optimize the use of shared memory and thus we expect
> +		 * some disparity in the memory configuration */
> +		quirks |= L2_MMU_CONFIG_ALLOW_SNOOP_DISPARITY;

Well that sounds terrifying; I rather wish my brain had preprocessed 
that #if already.

> +	}
> +#endif
> +	gpu_write(pfdev, GPU_L2_MMU_CONFIG, quirks);
> +
> +	quirks = 0;
> +	if ((panfrost_model_eq(pfdev, 0x860) || panfrost_model_eq(pfdev, 0x880)) &&
> +	    pfdev->features.revision >= 0x2000)
> +		quirks |= JM_MAX_JOB_THROTTLE_LIMIT << JM_JOB_THROTTLE_LIMIT_SHIFT;
> +	else if (panfrost_model_eq(pfdev, 0x6000) &&
> +		 pfdev->features.coherency_features == COHERENCY_ACE)
> +		quirks |= (COHERENCY_ACE_LITE | COHERENCY_ACE) <<
> +			   JM_FORCE_COHERENCY_FEATURES_SHIFT;

Experience says you can never really trust what ID registers claim about 
system integration stuff like coherency, because eventually someone will 
get a tieoff wrong and make it all fall apart. If even the vendor driver 
has a DT override for it you know you're on thin ice ;)

Ultimately, most of your I/O coherency behaviour will be governed by 
what the DMA API thinks (based on "dma-coherent"), so if you end up with 
mismatched expectations at the point coherency_features gets set up then 
you're liable to have a bad time. See the arm-smmu drivers for prior 
examples of handling the equivalent thing.

Robin.

> +
> +	if (quirks)
> +		gpu_write(pfdev, GPU_JM_CONFIG, quirks);
> +}

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

Re: [PATCH v2 3/3] drm/panfrost: Add initial panfrost driver

[Alyssa Rosenzweig]

> the userspace definitely doesn't support T624

This is true, yes. Shouldn't be too hard to backport; if there's still
interest in Midgard 1st/2nd gen, I suppose I can grab hardware and sort
it out...

> You probably want a dma_set_mask_and_coherent() call for your 'real' output
> address size somewhere - the default 32-bit mask works out OK for RK3399,
> but on systems with RAM above 4GB io-pgtable will get very unhappy about DMA
> bounce-buffering.

Out of curiosity, are there Mali systems with >4GB RAM? That sounds
awesome :)

> Any chance of resurrecting the generic "arm,mali-midgard" compatible? :P

...Would that require editing everybody's DT file?

-----

Thank you for the review!

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

Re: [PATCH v2 3/3] drm/panfrost: Add initial panfrost driver

[Robin Murphy]

On 02/04/2019 01:33, Alyssa Rosenzweig wrote:
>> the userspace definitely doesn't support T624
> 
> This is true, yes. Shouldn't be too hard to backport; if there's still
> interest in Midgard 1st/2nd gen, I suppose I can grab hardware and sort
> it out...

I'm quite likely the only person trying this on an Arm Juno board, and 
even then it's really only for giggles because I can :)  I guess there 
might be a fair few Odroid-XU3/XU4 (T628) users interested, though.

>> You probably want a dma_set_mask_and_coherent() call for your 'real' output
>> address size somewhere - the default 32-bit mask works out OK for RK3399,
>> but on systems with RAM above 4GB io-pgtable will get very unhappy about DMA
>> bounce-buffering.
> 
> Out of curiosity, are there Mali systems with >4GB RAM? That sounds
> awesome :)

Now that the "early-access Armv8 silicon" angle has well and truly 
expired, Juno is essentially a prototyping platform where the SoC just 
serves to (slowly) drive interesting things in FPGA cards, so although 
it may have 8GB of RAM, it's not all that exciting. There is one 
somewhat more realistic board I'm aware of, namely HiKey 970 with a G72 
and 6GB.

>> Any chance of resurrecting the generic "arm,mali-midgard" compatible? :P
> 
> ...Would that require editing everybody's DT file?

If they already have one of the strings from the current upstream 
binding, no - I only mean to suggest adding it as an additional 
last-level fallback. That would aid compatibility with downstream DTs, 
for example RK3288 which currently has zero overlap:

upstream: "rockchip,rk3288-mali", "arm,mali-t760";

downstream: "arm,malit764", "arm,malit76x", "arm,malit7xx", 
"arm,mali-midgard";

Similarly, it might be reasonable for panfrost_{gpu,mmu,job}_init() to 
retry platform_get_irq_byname() with uppercase interrupt names if the 
expected ones aren't found - obviously the upstream binding comes first 
and foremost, but I don't see any harm in quietly supporting bits of the 
downstream binding if it makes users' lives easier when switching 
between mainline and vendor kernels.

Cheers,
Robin.

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

Re: [PATCH v2 3/3] drm/panfrost: Add initial panfrost driver

[Rob Herring]

On Mon, Apr 1, 2019 at 2:12 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 01/04/2019 08:47, Rob Herring wrote:
> > This adds the initial driver for panfrost which supports Arm Mali
> > Midgard and Bifrost family of GPUs. Currently, only the T860 and
> > T760 Midgard GPUs have been tested.
>
> FWIW, on an antique T624 (Juno) it seems to work no worse than the kbase
> driver plus panfrost-nondrm, which is to say it gets far enough to prove
> that the userspace definitely doesn't support T624 (kmscube manages to
> show a grey background, but the GPU is constantly falling over with page
> faults trying to dereference address 0 - for obvious reasons I'm not
> going to get any further involved in debugging that).
>
> A couple of discoveries and general observations below.
>
> > v2:
> > - Add GPU reset on job hangs (Tomeu)
> > - Add RuntimePM and devfreq support (Tomeu)
> > - Fix T760 support (Tomeu)
> > - Add a TODO file (Rob, Tomeu)
> > - Support multiple in fences (Tomeu)
> > - Drop support for shared fences (Tomeu)
> > - Fill in MMU de-init (Rob)
> > - Move register definitions back to single header (Rob)
> > - Clean-up hardcoded job submit todos (Rob)
> > - Implement feature setup based on features/issues (Rob)
> > - Add remaining Midgard DT compatible strings (Rob)
> >
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: Eric Anholt <eric@anholt.net>
> > Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > ---
> [...]
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> > new file mode 100644
> > index 000000000000..227ba5202a6f
> > --- /dev/null
> > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> > @@ -0,0 +1,227 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright 2018 Marty E. Plummer <hanetzer@startmail.com> */
> > +/* Copyright 2019 Linaro, Ltd, Rob Herring <robh@kernel.org> */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#include "panfrost_device.h"
> > +#include "panfrost_devfreq.h"
> > +#include "panfrost_features.h"
> > +#include "panfrost_gpu.h"
> > +#include "panfrost_job.h"
> > +#include "panfrost_mmu.h"
> > +
> > +static int panfrost_clk_init(struct panfrost_device *pfdev)
> > +{
> > +     int err;
> > +     unsigned long rate;
> > +
> > +     pfdev->clock = devm_clk_get(pfdev->dev, NULL);
> > +     if (IS_ERR(pfdev->clock)) {
>
> The DT binding says clocks are optional, but this doesn't treat them as
> such.

Hum, I would think effectively clocks are always there and necessary
for thermal reasons.


> > +     spin_lock_init(&pfdev->mm_lock);
> > +
> > +     /* 4G enough for now. can be 48-bit */
> > +     drm_mm_init(&pfdev->mm, SZ_32M >> PAGE_SHIFT, SZ_4G);
>
> You probably want a dma_set_mask_and_coherent() call for your 'real'
> output address size somewhere - the default 32-bit mask works out OK for
> RK3399, but on systems with RAM above 4GB io-pgtable will get very
> unhappy about DMA bounce-buffering.

Yes, I have a todo for figuring out the # of physaddr bits in the mmu
setup (as this call is just relevant to the input address side).
Though maybe just calling dma_set_mask_and_coherent() is enough and I
don't need to know the exact number of output bits for the io-pgtable
setup?

> > +     return err;
> > +}
> > +
> > +static int panfrost_remove(struct platform_device *pdev)
> > +{
> > +     struct panfrost_device *pfdev = platform_get_drvdata(pdev);
> > +     struct drm_device *ddev = pfdev->ddev;
> > +
> > +     drm_dev_unregister(ddev);
> > +     pm_runtime_get_sync(pfdev->dev);
> > +     pm_runtime_put_sync_autosuspend(pfdev->dev);
> > +     pm_runtime_disable(pfdev->dev);
> > +     panfrost_device_fini(pfdev);
> > +     drm_dev_put(ddev);
> > +     return 0;
> > +}
> > +
> > +static const struct of_device_id dt_match[] = {
> > +     { .compatible = "arm,mali-t604" },
> > +     { .compatible = "arm,mali-t624" },
> > +     { .compatible = "arm,mali-t628" },
> > +     { .compatible = "arm,mali-t720" },
> > +     { .compatible = "arm,mali-t760" },
> > +     { .compatible = "arm,mali-t820" },
> > +     { .compatible = "arm,mali-t830" },
> > +     { .compatible = "arm,mali-t860" },
> > +     { .compatible = "arm,mali-t880" },
>
> Any chance of resurrecting the generic "arm,mali-midgard" compatible? :P

I wouldn't mind, but we still need all these and I don't think we'd be
adding more. For bifrost, we're adding 'arm,mali-bifrost' from the
start.

> > diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> > new file mode 100644
> > index 000000000000..867e2ba3a761
> > --- /dev/null
> > +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> [...]

> > +     /* Limit read & write ID width for AXI */
> > +     if (panfrost_has_hw_feature(pfdev, HW_FEATURE_3BIT_EXT_RW_L2_MMU_CONFIG))
> > +             quirks &= ~(L2_MMU_CONFIG_3BIT_LIMIT_EXTERNAL_READS |
> > +                         L2_MMU_CONFIG_3BIT_LIMIT_EXTERNAL_WRITES);
> > +     else
> > +             quirks &= ~(L2_MMU_CONFIG_LIMIT_EXTERNAL_READS |
> > +                         L2_MMU_CONFIG_LIMIT_EXTERNAL_WRITES);
> > +
> > +#if 0
> > +     if (kbdev->system_coherency == COHERENCY_ACE) {
> > +             /* Allow memory configuration disparity to be ignored, we
> > +              * optimize the use of shared memory and thus we expect
> > +              * some disparity in the memory configuration */
> > +             quirks |= L2_MMU_CONFIG_ALLOW_SNOOP_DISPARITY;
>
> Well that sounds terrifying; I rather wish my brain had preprocessed
> that #if already.

What can I say, copied from Arm's driver. I'm just going to drop for now.

>
> > +     }
> > +#endif
> > +     gpu_write(pfdev, GPU_L2_MMU_CONFIG, quirks);
> > +
> > +     quirks = 0;
> > +     if ((panfrost_model_eq(pfdev, 0x860) || panfrost_model_eq(pfdev, 0x880)) &&
> > +         pfdev->features.revision >= 0x2000)
> > +             quirks |= JM_MAX_JOB_THROTTLE_LIMIT << JM_JOB_THROTTLE_LIMIT_SHIFT;
> > +     else if (panfrost_model_eq(pfdev, 0x6000) &&
> > +              pfdev->features.coherency_features == COHERENCY_ACE)
> > +             quirks |= (COHERENCY_ACE_LITE | COHERENCY_ACE) <<
> > +                        JM_FORCE_COHERENCY_FEATURES_SHIFT;
>
> Experience says you can never really trust what ID registers claim about
> system integration stuff like coherency, because eventually someone will
> get a tieoff wrong and make it all fall apart. If even the vendor driver
> has a DT override for it you know you're on thin ice ;)

Unlike the vendor driver, we only have to care about cases seen upstream...

> Ultimately, most of your I/O coherency behaviour will be governed by
> what the DMA API thinks (based on "dma-coherent"), so if you end up with
> mismatched expectations at the point coherency_features gets set up then
> you're liable to have a bad time. See the arm-smmu drivers for prior
> examples of handling the equivalent thing.

None of this matters til bifrost and a platform implementing this, so
I'll worry about it then.

Thanks for the review.

Rob

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

Re: [PATCH v2 3/3] drm/panfrost: Add initial panfrost driver

[Robin Murphy]

On 03/04/2019 05:57, Rob Herring wrote:
[...]
>>> +static int panfrost_clk_init(struct panfrost_device *pfdev)
>>> +{
>>> +     int err;
>>> +     unsigned long rate;
>>> +
>>> +     pfdev->clock = devm_clk_get(pfdev->dev, NULL);
>>> +     if (IS_ERR(pfdev->clock)) {
>>
>> The DT binding says clocks are optional, but this doesn't treat them as
>> such.
> 
> Hum, I would think effectively clocks are always there and necessary
> for thermal reasons.

Should the binding be updated to move clocks from "optional" to 
"required" then? Juno does actually have a GPU clock for DVFS, but the 
clk-scmi driver didn't seem to want to play nicely with either 
mali_kbase or panfrost DRM, so I've just been leaving it out of my DT 
for now (and mali_kbase was perfectly happy without).

>>> +     spin_lock_init(&pfdev->mm_lock);
>>> +
>>> +     /* 4G enough for now. can be 48-bit */
>>> +     drm_mm_init(&pfdev->mm, SZ_32M >> PAGE_SHIFT, SZ_4G);
>>
>> You probably want a dma_set_mask_and_coherent() call for your 'real'
>> output address size somewhere - the default 32-bit mask works out OK for
>> RK3399, but on systems with RAM above 4GB io-pgtable will get very
>> unhappy about DMA bounce-buffering.
> 
> Yes, I have a todo for figuring out the # of physaddr bits in the mmu
> setup (as this call is just relevant to the input address side).
> Though maybe just calling dma_set_mask_and_coherent() is enough and I
> don't need to know the exact number of output bits for the io-pgtable
> setup?

True, io-pgtable itself only really depends on the input size, but in 
order for non-coherent pagtables to work correctly in general, the DMA 
mask does need to be set appropriately, at which point it may as well 
also be propagated into OAS for completeness (as we do in arm-smmu*).

FWIW I'm just gonna leave this quote here...

	gpu_props->mmu.va_bits = KBASE_UBFX32(raw->mmu_features, 0U, 8);
	gpu_props->mmu.pa_bits = KBASE_UBFX32(raw->mmu_features, 8U, 8);


Robin.

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

Re: [PATCH v2 3/3] drm/panfrost: Add initial panfrost driver

[Alyssa Rosenzweig]

> I'm also somewhat surprised that you don't need loads of other
> properties from the GPU - in particular knowing the number of shader
> cores is useful for allocating the right amount of memory for TLS (and
> can't be obtained purely from the GPU_ID).

Since I have no idea what TLS is (and in my notes, I've come across the
acronym once ever and have it as a "??"), I'm not sure how to respond to
that... We don't know how to allocate memory for the GPU-internal data
structures (the tiler heap, for instance, but also a few others I've
just named "misc_0" and "scratchpad" -- guessing one of those is for
"TLS"). With kbase, I took the worst-case strategy of allocating
gigantic chunks on startup with tiny commit counts and GROW_ON_GPF set.
With the new driver, well, our memory consumption is scary since
implementing GROW_ON_GPF in an upstream-friendly way is a bit more work
and isn't expected to hit the 5.2 window.

Given this is kernel facing, I'm hoping 're able to share some answers
here?

> I think this comment might have survived since the very earliest version
> of the Midgard driver! :)

^_^

> But I'm not sure anything will attempt to lock a region spanning two
> pages like that.

At least at the moment, I align everything kernel-facing to page
granularity in userspace, so it's not a cornercase I'm going to hit
anytime soon. Still probably better to have it technically correct.

> To be fair only for BASE_HW_ISSUE_6367/T60X - but yes it's not a
> pleasant workaround. There's no way on that hardware to reliably drain
> the write buffer other than waiting.

*wishing T60X disappeared intensifies* ;)

Granted there are enough other errata specific to it that aren't worked
around here that, well, it makes you wonder ;)

> Do we have a good way of user space determining which requirements are
> supported by the driver? At the moment it's just the one. kbase outgeew
> the original u16 and has an ugly "compat_core_req", so I suspect you're
> going to need to add several more along the way.

Oh, so that's why compat_/core_req is split off! I thought somebody just
thought it would be "fun" to break the UABI ;)

I've definitely issues using the wrong core_req field for the kbase I
had setup, that set me back a little bit on RK3399/T860 bringup *purses
lips*

To be fair, bunches of the kbase reqs are for soft jobs, which I don't
feel are a good fit for how the Panfrost kernel works. If we need to
implement functionality corresponding to softjobs, that would likely be
done with dedicated ioctl(s), instead of affecting the core_req field.

On that note

> You might also want to consider being able to submit more than one job
> chain at a time - but that could easily be implemented using a new ioctl
> in the future.

The issue with that at the bottom is having to introduce something akin
to kbase's annoyingly intra-job-chain dependency management (read: I
still don't understand how FBOs are supposed to work with kbase ;) ),
which AFAIK we push off to userspace right now via standard fencing. If
we want to submit batches at a time, we would potentially need to
express those somewhat complex dependency trees, which is a lot of work
for diminishing returns at this stage. Future ioctl indeed...

> There's no SUBMIT_CL in this posting? I think you just need s/_CL//.

+1

> You are probably going to want flags for at least:
> 
>  * No execute/No read/No write (good for security, especially with
> buffer sharing between processes)
> 
>  * Alignment (shader programs have quite strict alignment requirements,
> I believe kbase just ensures that the shader memory block doesn't cross
> a 16MB boundary which covers most cases).
> 
>  * Page fault behaviour (kbase has GROW_ON_GPF)
> 
>  * Coherency management

+1 for all of these. This is piped through in userspace (for kbase), but
the corresponding functionality isn't there yet in the Panfrost kernel.
You're right there should at least be a flags field for future use.

> One issue that I haven't got to the bottom of is that I can trigger a
> lockdep splat:

Oh, "fun"...

> This is with the below simple reproducer:

@Rob, ideas?

> Other than that in my testing (on a Firefly RK3288) I didn't experience
> any problems pushing jobs from the ARM userspace blob through it.

Nice!

Besides what was mentioned above, any other functionality you'll need
for that? (e.g. the infamous replay workaround...)

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

Re: [PATCH v2 3/3] drm/panfrost: Add initial panfrost driver

[Steven Price]

On 05/04/2019 17:16, Alyssa Rosenzweig wrote:
>> I'm also somewhat surprised that you don't need loads of other
>> properties from the GPU - in particular knowing the number of shader
>> cores is useful for allocating the right amount of memory for TLS (and
>> can't be obtained purely from the GPU_ID).
> 
> Since I have no idea what TLS is (and in my notes, I've come across the

Sorry - "Thread Local Storage" - e.g. registers spilled to memory from a
shader program.

> acronym once ever and have it as a "??"), I'm not sure how to respond to
> that... We don't know how to allocate memory for the GPU-internal data
> structures (the tiler heap, for instance, but also a few others I've
> just named "misc_0" and "scratchpad" -- guessing one of those is for
> "TLS"). With kbase, I took the worst-case strategy of allocating
> gigantic chunks on startup with tiny commit counts and GROW_ON_GPF set.
> With the new driver, well, our memory consumption is scary since
> implementing GROW_ON_GPF in an upstream-friendly way is a bit more work
> and isn't expected to hit the 5.2 window.

Yes GROW_ON_GPF is pretty much required for the tiler heap - it's not
(reasonably) possible to determine how big it should be. The Arm user
space driver does the same approach (tiny commit count, but allow it to
grow).

> 
> Given this is kernel facing, I'm hoping 're able to share some answers
> here?

At the moment I don't have any permission to share details which aren't
already public in the kbase driver. Hopefully that situation will
change. I'm also very much not an expert on anything but the kernel
driver (I tried to stay away from shader compilers and all that graphics
knowledge...). The details of the job descriptors is only really
publicly documented in terms of the "replay workaround" which is quite
limited.

> 
>> I think this comment might have survived since the very earliest version
>> of the Midgard driver! :)
> 
> ^_^
> 
>> But I'm not sure anything will attempt to lock a region spanning two
>> pages like that.
> 
> At least at the moment, I align everything kernel-facing to page
> granularity in userspace, so it's not a cornercase I'm going to hit
> anytime soon. Still probably better to have it technically correct.
> 
>> To be fair only for BASE_HW_ISSUE_6367/T60X - but yes it's not a
>> pleasant workaround. There's no way on that hardware to reliably drain
>> the write buffer other than waiting.
> 
> *wishing T60X disappeared intensifies* ;)

I think we all felt like that :) Still the Nexus 10 wasn't a bad tablet,
and the Chromebook was an exciting first!

> Granted there are enough other errata specific to it that aren't worked
> around here that, well, it makes you wonder ;)

A lot of the errata are things you only hit with soak testing. So to a
large extent you "get lucky".

>> Do we have a good way of user space determining which requirements are
>> supported by the driver? At the moment it's just the one. kbase outgeew
>> the original u16 and has an ugly "compat_core_req", so I suspect you're
>> going to need to add several more along the way.
> 
> Oh, so that's why compat_/core_req is split off! I thought somebody just
> thought it would be "fun" to break the UABI ;)

No that's a case of us actually not breaking the UABI for once :)

> I've definitely issues using the wrong core_req field for the kbase I
> had setup, that set me back a little bit on RK3399/T860 bringup *purses
> lips*
> 
> To be fair, bunches of the kbase reqs are for soft jobs, which I don't
> feel are a good fit for how the Panfrost kernel works. If we need to
> implement functionality corresponding to softjobs, that would likely be
> done with dedicated ioctl(s), instead of affecting the core_req field.
> 
> On that note
> 
>> You might also want to consider being able to submit more than one job
>> chain at a time - but that could easily be implemented using a new ioctl
>> in the future.
> 
> The issue with that at the bottom is having to introduce something akin
> to kbase's annoyingly intra-job-chain dependency management (read: I
> still don't understand how FBOs are supposed to work with kbase ;) ),
> which AFAIK we push off to userspace right now via standard fencing. If
> we want to submit batches at a time, we would potentially need to
> express those somewhat complex dependency trees, which is a lot of work
> for diminishing returns at this stage. Future ioctl indeed...

You should be able to express the dependencies using fences. At the time
kbase was started there was no fence mechanism in the kernel. We
invented horrible things like UMP[1] and KDS[2] for cross-driver sharing.

It all comes down to how small your job chains are - if you don't need
to squeeze too many through the hardware you should be fine. But there's
going to be some performance gain to be had implementing it.

[1] I forget what it actually stands for, but was an attempt to do
something like dma_buf

[2] "Kernel Dependency System" - a not-so-good version of dma_fence

>> There's no SUBMIT_CL in this posting? I think you just need s/_CL//.
> 
> +1
> 
>> You are probably going to want flags for at least:
>>
>>  * No execute/No read/No write (good for security, especially with
>> buffer sharing between processes)
>>
>>  * Alignment (shader programs have quite strict alignment requirements,
>> I believe kbase just ensures that the shader memory block doesn't cross
>> a 16MB boundary which covers most cases).
>>
>>  * Page fault behaviour (kbase has GROW_ON_GPF)
>>
>>  * Coherency management
> 
> +1 for all of these. This is piped through in userspace (for kbase), but
> the corresponding functionality isn't there yet in the Panfrost kernel.
> You're right there should at least be a flags field for future use.
> 
>> One issue that I haven't got to the bottom of is that I can trigger a
>> lockdep splat:
> 
> Oh, "fun"...
> 
>> This is with the below simple reproducer:
> 
> @Rob, ideas?
> 
>> Other than that in my testing (on a Firefly RK3288) I didn't experience
>> any problems pushing jobs from the ARM userspace blob through it.
> 
> Nice!
> 
> Besides what was mentioned above, any other functionality you'll need
> for that? (e.g. the infamous replay workaround...)

If you don't implement the replay workaround I'm very happy :)

The main missing part for the Arm user space is feature registers. That
and the lack of SAME_VA is horrible to emulate (keep allocating until it
happens to land in a free area of user space memory).

Arm user space also makes use of cached memory with explicit cache sync
operations. It of course works fine with uncached and ignoring the sync,
but again I'm not sure how much performance is being lost.

Steve

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

Re: [PATCH v2 3/3] drm/panfrost: Add initial panfrost driver

[Alyssa Rosenzweig]

> Sorry - "Thread Local Storage" - e.g. registers spilled to memory from a
> shader program.

Gotcha, thank you. Register spilling isn't implemented yet, so I haven't
run into this. (Partially because the blob's RA is very good so it's
somewhat nontrivial to get it to spill... not that I've tried, the real
reason is that the RA I have implemented right now works and I don't
want to mess with it ;P)

> At the moment I don't have any permission to share details which aren't
> already public in the kbase driver. Hopefully that situation will
> change. I'm also very much not an expert on anything but the kernel
> driver (I tried to stay away from shader compilers and all that graphics
> knowledge...). The details of the job descriptors is only really
> publicly documented in terms of the "replay workaround" which is quite
> limited.

Alright, no worries! We'll see where the tide turns, indeed :)

> I think we all felt like that :) Still the Nexus 10 wasn't a bad tablet,
> and the Chromebook was an exciting first!

*looks around to 2 Kevins and 2 Veyrons sprawled about* At first,
indeeed.... ;)

> You should be able to express the dependencies using fences. At the time
> kbase was started there was no fence mechanism in the kernel. We
> invented horrible things like UMP[1] and KDS[2] for cross-driver sharing.

Ah-ha, I see; I didn't know if there was an explicit reason kbase didn't
use fencing, but if it didn't exist, that's reason enough.

> It all comes down to how small your job chains are - if you don't need
> to squeeze too many through the hardware you should be fine. But there's
> going to be some performance gain to be had implementing it.

For sure.

> [1] I forget what it actually stands for, but was an attempt to do
> something like dma_buf

Unified Memory Provider, iirc.

> If you don't implement the replay workaround I'm very happy :)

Pff.

> The main missing part for the Arm user space is feature registers. That
> and the lack of SAME_VA is horrible to emulate (keep allocating until it
> happens to land in a free area of user space memory).

Alright, both of those will probably be needed for us sooner or later,
so no harm in implementing those. Thank you!

> Arm user space also makes use of cached memory with explicit cache sync
> operations. It of course works fine with uncached and ignoring the sync,
> but again I'm not sure how much performance is being lost.

I would be interested as well, since even when I used kbase for stuff, I set
everything uncached/unsynced to keep myself sane, but that could be a
very real performance issue on some workloads.

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

Re: [PATCH v2 3/3] drm/panfrost: Add initial panfrost driver

[Daniel Vetter]

On Fri, Apr 05, 2019 at 05:42:33PM +0100, Steven Price wrote:
> On 05/04/2019 17:16, Alyssa Rosenzweig wrote:
> > acronym once ever and have it as a "??"), I'm not sure how to respond to
> > that... We don't know how to allocate memory for the GPU-internal data
> > structures (the tiler heap, for instance, but also a few others I've
> > just named "misc_0" and "scratchpad" -- guessing one of those is for
> > "TLS"). With kbase, I took the worst-case strategy of allocating
> > gigantic chunks on startup with tiny commit counts and GROW_ON_GPF set.
> > With the new driver, well, our memory consumption is scary since
> > implementing GROW_ON_GPF in an upstream-friendly way is a bit more work
> > and isn't expected to hit the 5.2 window.
> 
> Yes GROW_ON_GPF is pretty much required for the tiler heap - it's not
> (reasonably) possible to determine how big it should be. The Arm user
> space driver does the same approach (tiny commit count, but allow it to
> grow).

Jumping in here with a drive through comment ...

Growing gem bo and dma-buf is going to be endless amounts of fun, since we
hard-coded that their size is invariant.

I think the only reasonable way to implement this is if you allocate a
really huge bo, map it, but only put the pages in on faulting. Or when
really evil userspace tries to export it. Actually changing the underlying
buffer size is not going to work I think.

Note: I didn't read kbase, so might be totally wrong in how GROW_ON_GPF
works.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

Re: [PATCH v2 3/3] drm/panfrost: Add initial panfrost driver

[Steven Price]

On 15/04/2019 10:18, Daniel Vetter wrote:
> On Fri, Apr 05, 2019 at 05:42:33PM +0100, Steven Price wrote:
>> On 05/04/2019 17:16, Alyssa Rosenzweig wrote:
>>> acronym once ever and have it as a "??"), I'm not sure how to respond to
>>> that... We don't know how to allocate memory for the GPU-internal data
>>> structures (the tiler heap, for instance, but also a few others I've
>>> just named "misc_0" and "scratchpad" -- guessing one of those is for
>>> "TLS"). With kbase, I took the worst-case strategy of allocating
>>> gigantic chunks on startup with tiny commit counts and GROW_ON_GPF set.
>>> With the new driver, well, our memory consumption is scary since
>>> implementing GROW_ON_GPF in an upstream-friendly way is a bit more work
>>> and isn't expected to hit the 5.2 window.
>>
>> Yes GROW_ON_GPF is pretty much required for the tiler heap - it's not
>> (reasonably) possible to determine how big it should be. The Arm user
>> space driver does the same approach (tiny commit count, but allow it to
>> grow).
> 
> Jumping in here with a drive through comment ...
> 
> Growing gem bo and dma-buf is going to be endless amounts of fun, since we
> hard-coded that their size is invariant.
> 
> I think the only reasonable way to implement this is if you allocate a
> really huge bo, map it, but only put the pages in on faulting. Or when
> really evil userspace tries to export it. Actually changing the underlying
> buffer size is not going to work I think.

Yes the idea is that you allocate a large amount of virtual address
space, but only put a few physical pages in. If the GPU needs more you
fault them in as necessary. The "buffer size" (i.e. virtual address
region) never changes size.

> Note: I didn't read kbase, so might be totally wrong in how GROW_ON_GPF
> works.

For kbase we simply don't support exporting this type of memory, and are
fairly restrictive about mapping it into user space (user space
shouldn't normally need to read it).

Since Panfrost is using GEM BO it will have to deal with malicious user
space. But it would be sufficient to simply fully back the region in
that case.

Recent version of kbase also support what is termed JIT (Just In Time
memory allocation). Basically this involves the kernel driver
allocating/freeing memory regions just before the job is loaded onto the
GPU. These regions might also be GROW_ON_GPF. The intention is that when
there isn't memory pressure these regions can be kept between jobs, but
under memory pressure they can be discarded and recreated if they turn
out to be needed again.

Given the differences between the Panfrost and the proprietary user
space I'm not sure exactly what form this will need to be for Panfrost,
but Vulkan makes memory management "more interesting"! Allocating
upfront for the worst case can become prohibitively expensive.

Steve

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

Re: [PATCH v2 3/3] drm/panfrost: Add initial panfrost driver

[Daniel Vetter]

On Mon, Apr 15, 2019 at 10:30:14AM +0100, Steven Price wrote:
> On 15/04/2019 10:18, Daniel Vetter wrote:
> > On Fri, Apr 05, 2019 at 05:42:33PM +0100, Steven Price wrote:
> >> On 05/04/2019 17:16, Alyssa Rosenzweig wrote:
> >>> acronym once ever and have it as a "??"), I'm not sure how to respond to
> >>> that... We don't know how to allocate memory for the GPU-internal data
> >>> structures (the tiler heap, for instance, but also a few others I've
> >>> just named "misc_0" and "scratchpad" -- guessing one of those is for
> >>> "TLS"). With kbase, I took the worst-case strategy of allocating
> >>> gigantic chunks on startup with tiny commit counts and GROW_ON_GPF set.
> >>> With the new driver, well, our memory consumption is scary since
> >>> implementing GROW_ON_GPF in an upstream-friendly way is a bit more work
> >>> and isn't expected to hit the 5.2 window.
> >>
> >> Yes GROW_ON_GPF is pretty much required for the tiler heap - it's not
> >> (reasonably) possible to determine how big it should be. The Arm user
> >> space driver does the same approach (tiny commit count, but allow it to
> >> grow).
> > 
> > Jumping in here with a drive through comment ...
> > 
> > Growing gem bo and dma-buf is going to be endless amounts of fun, since we
> > hard-coded that their size is invariant.
> > 
> > I think the only reasonable way to implement this is if you allocate a
> > really huge bo, map it, but only put the pages in on faulting. Or when
> > really evil userspace tries to export it. Actually changing the underlying
> > buffer size is not going to work I think.
> 
> Yes the idea is that you allocate a large amount of virtual address
> space, but only put a few physical pages in. If the GPU needs more you
> fault them in as necessary. The "buffer size" (i.e. virtual address
> region) never changes size.
> 
> > Note: I didn't read kbase, so might be totally wrong in how GROW_ON_GPF
> > works.
> 
> For kbase we simply don't support exporting this type of memory, and are
> fairly restrictive about mapping it into user space (user space
> shouldn't normally need to read it).

You can still disallow sharing with any other driver (in the dma-buf
attach callback), and then enforce whatever mapping restrictions you want
on the panfrost.ko ioctl interface. That should be roughly equivalent to
the restrictions kbase imposes.
> 
> Since Panfrost is using GEM BO it will have to deal with malicious user
> space. But it would be sufficient to simply fully back the region in
> that case.
> 
> Recent version of kbase also support what is termed JIT (Just In Time
> memory allocation). Basically this involves the kernel driver
> allocating/freeing memory regions just before the job is loaded onto the
> GPU. These regions might also be GROW_ON_GPF. The intention is that when
> there isn't memory pressure these regions can be kept between jobs, but
> under memory pressure they can be discarded and recreated if they turn
> out to be needed again.
> 
> Given the differences between the Panfrost and the proprietary user
> space I'm not sure exactly what form this will need to be for Panfrost,
> but Vulkan makes memory management "more interesting"! Allocating
> upfront for the worst case can become prohibitively expensive.

The usual way to do that is with a WONTNEED/WILLNEED madvise ioctl on the
gem bo. I guess that could also be set at create time to essentially only
require the bo to exist during an execbuf call. Should fit pretty well
into what other drivers are doing with gem shmem already I think.

ofc needs a shrinker to be able to reap these bo.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

Re: [PATCH v2 3/3] drm/panfrost: Add initial panfrost driver

[Rob Herring]

On Fri, Apr 5, 2019 at 7:30 AM Steven Price <steven.price@arm.com> wrote:
>
> On 01/04/2019 08:47, Rob Herring wrote:
> > This adds the initial driver for panfrost which supports Arm Mali
> > Midgard and Bifrost family of GPUs. Currently, only the T860 and
> > T760 Midgard GPUs have been tested.

[...]

> > +     case 0xD3: return "TRANSTAB_BUS_FAULT_LEVEL3";
> > +     case 0xD4: return "TRANSTAB_BUS_FAULT_LEVEL4";
> > +     case 0xD8: return "ACCESS_FLAG";
> > +     }
> > +
> > +     if (panfrost_has_hw_feature(pfdev, HW_FEATURE_AARCH64_MMU)) {
>
> There's not a great deal of point in this conditional - you won't get
> the below exception codes from hardware which doesn't support it AARCH64
> page tables.

I wasn't sure if "UNKNOWN" types could happen or not.

>
> > +             switch (exception_code) {
> > +             case 0xC9 ... 0xCF: return "PERMISSION_FAULT";
> > +             case 0xD9 ... 0xDF: return "ACCESS_FLAG";
> > +             case 0xE0 ... 0xE7: return "ADDRESS_SIZE_FAULT";
> > +             case 0xE8 ... 0xEF: return "MEMORY_ATTRIBUTES_FAULT";
> > +             }
> > +     }
> > +
> > +     return "UNKNOWN";
> > +}

> > +static inline int panfrost_model_cmp(struct panfrost_device *pfdev, s32 id)
> > +{
> > +     s32 match_id = pfdev->features.id;
> > +
> > +     if (match_id & 0xf000)
> > +             match_id &= 0xf00f;
>
> I'm puzzled by this, and really not sure if it's going to work out
> ignoring the ARCH_MINOR/ARCH_REV fields. But for now there's no real
> Bifrost support.

That seemed to be enough for kbase to select features/issues.

> > +     switch (param->param) {
> > +     case DRM_PANFROST_PARAM_GPU_ID:
> > +             param->value = pfdev->features.id;
>
> This is unfortunate naming - this parameter is *not* the GPU_ID. It is
> the top half of the GPU_ID which kbase calls the PRODUCT_ID.

I can rename it.

> I'm also somewhat surprised that you don't need loads of other
> properties from the GPU - in particular knowing the number of shader
> cores is useful for allocating the right amount of memory for TLS (and
> can't be obtained purely from the GPU_ID).

We'll add them as userspace needs them.

> > +static int
> > +panfrost_lookup_bos(struct drm_device *dev,
> > +               struct drm_file *file_priv,
> > +               struct drm_panfrost_submit *args,
> > +               struct panfrost_job *job)
> > +{
> > +     u32 *handles;
> > +     int ret = 0;
> > +
> > +     job->bo_count = args->bo_handle_count;
> > +
> > +     if (!job->bo_count)
> > +             return 0;
> > +
> > +     job->bos = kvmalloc_array(job->bo_count,
> > +                               sizeof(struct drm_gem_object *),
> > +                               GFP_KERNEL | __GFP_ZERO);
> > +     if (!job->bos)
>
> If we return here then job->bo_count has been set, but job->bos is
> invalid - this means that panfrost_job_cleanup() will then dereference
> NULL. Although maybe this is better fixed in panfrost_job_cleanup().

Good catch. The fence arrays have the same problem. As does V3D which we copied.

> > +             return -ENOMEM;
> > +
> > +     job->implicit_fences = kvmalloc_array(job->bo_count,
> > +                               sizeof(struct dma_fence *),
> > +                               GFP_KERNEL | __GFP_ZERO);
> > +     if (!job->implicit_fences)
> > +             return -ENOMEM;

> > +static irqreturn_t panfrost_gpu_irq_handler(int irq, void *data)
> > +{
> > +     struct panfrost_device *pfdev = data;
> > +     u32 state = gpu_read(pfdev, GPU_INT_STAT);
> > +     u32 fault_status = gpu_read(pfdev, GPU_FAULT_STATUS);
> > +     u64 address;
> > +     bool done = false;
> > +
> > +     if (!state)
> > +             return IRQ_NONE;
> > +
> > +     if (state & GPU_IRQ_MASK_ERROR) {
> > +             address = (u64) gpu_read(pfdev, GPU_FAULT_ADDRESS_HI) << 32;
> > +             address |= gpu_read(pfdev, GPU_FAULT_ADDRESS_LO);
> > +
> > +             dev_warn(pfdev->dev, "GPU Fault 0x%08x (%s) at 0x%016llx\n",
> > +                      fault_status & 0xFF, panfrost_exception_name(pfdev, fault_status),
> > +                      address);
> > +
> > +             if (state & GPU_IRQ_MULTIPLE_FAULT)
> > +                     dev_warn(pfdev->dev, "There were multiple GPU faults - some have not been reported\n");
> > +
> > +             gpu_write(pfdev, GPU_INT_MASK, 0);
>
> Do you intend to mask off future GPU interrupts?

Yes, maybe?

If we fault here, then we are going to reset the gpu on timeout which
then re-enables interrupts.


> > +static irqreturn_t panfrost_job_irq_handler(int irq, void *data)
> > +{
> > +     struct panfrost_device *pfdev = data;
> > +     u32 status = job_read(pfdev, JOB_INT_STAT);
> > +     int j;
> > +
> > +     dev_dbg(pfdev->dev, "jobslot irq status=%x\n", status);
> > +
> > +     if (!status)
> > +             return IRQ_NONE;
> > +
> > +     pm_runtime_mark_last_busy(pfdev->dev);
> > +
> > +     for (j = 0; status; j++) {
> > +             u32 mask = MK_JS_MASK(j);
> > +
> > +             if (!(status & mask))
> > +                     continue;
> > +
> > +             job_write(pfdev, JOB_INT_CLEAR, mask);
> > +
> > +             if (status & JOB_INT_MASK_ERR(j)) {
> > +                     job_write(pfdev, JS_COMMAND_NEXT(j), JS_COMMAND_NOP);
> > +                     job_write(pfdev, JS_COMMAND(j), JS_COMMAND_HARD_STOP_0);
>
> Hard-stopping an already completed job isn't likely to do very much :)
> Also you are using the "_0" version which is only valid when "job chain
> disambiguation" is present.
>
> I suspect in this case you should also be signalling the fence? At the
> moment you rely on the GPU timeout recovering from the fault.

I'll defer to Tomeu who wrote this (IIRC).


> > +static void lock_region(struct panfrost_device *pfdev, u32 as_nr,
> > +                     u64 iova, size_t size)
> > +{
> > +     u8 region_width;
> > +     u64 region = iova & PAGE_MASK;
> > +     /*
> > +      * fls returns (given the ASSERT above):
>
> But where's the assert? :p
>
> > +      * 1 .. 32
> > +      *
> > +      * 10 + fls(num_pages)
> > +      * results in the range (11 .. 42)
> > +      */
> > +
> > +     size = round_up(size, PAGE_SIZE);
>
> I'm not sure it matters, but this will break if called on a (small, i.e.
> less than a page) region spanning two pages. "region" will be rounded
> down to the page (iova & PAGE_MASK), but size will only be rounded up to
> the nearest page. This can miss the start of the second page.

This is probably redundant. Everything the driver does with memory is
in units of pages. Maybe imported buffers could be unaligned. Not sure
and we'd probably break in other places if that was the case.


> > +             /* terminal fault, print info about the fault */
> > +             dev_err(pfdev->dev,
> > +                     "Unhandled Page fault in AS%d at VA 0x%016llX\n"
> > +                     "Reason: %s\n"
> > +                     "raw fault status: 0x%X\n"
> > +                     "decoded fault status: %s\n"
> > +                     "exception type 0x%X: %s\n"
> > +                     "access type 0x%X: %s\n"
> > +                     "source id 0x%X\n",
> > +                     i, addr,
> > +                     "TODO",
> > +                     fault_status,
> > +                     (fault_status & (1 << 10) ? "DECODER FAULT" : "SLAVE FAULT"),
> > +                     exception_type, panfrost_exception_name(pfdev, exception_type),
> > +                     access_type, access_type_name(pfdev, fault_status),
> > +                     source_id);
> > +
> > +             mmu_write(pfdev, MMU_INT_CLEAR, mask);
>
> To fully handle the fault you will need to issue an MMU command (i.e.
> call mmu_hw_do_operation()) - obviously after doing something to fix the
> cause (e.g. mapping a page or switching the GPU to UNMAPPED mode to kill
> off the job). Otherwise you may see that the GPU hangs. Although in
> practice at this stage of the driver the generic timeout is probably the
> simplest way of handling an MMU fault.

Any fault currently is unexpected so all we really care about at this
point is getting a message.


> > +/**
> > + * struct drm_panfrost_create_bo - ioctl argument for creating Panfrost BOs.
> > + *
> > + * There are currently no values for the flags argument, but it may be
> > + * used in a future extension.
>
> You are probably going to want flags for at least:
>
>  * No execute/No read/No write (good for security, especially with
> buffer sharing between processes)
>
>  * Alignment (shader programs have quite strict alignment requirements,
> I believe kbase just ensures that the shader memory block doesn't cross
> a 16MB boundary which covers most cases).
>
>  * Page fault behaviour (kbase has GROW_ON_GPF)
>
>  * Coherency management

Yep, will add them as needed.

> One issue that I haven't got to the bottom of is that I can trigger a
> lockdep splat:
>
> -----8<------
> panfrost ffa30000.gpu: js fault, js=1, status=JOB_CONFIG_FAULT,
> head=0x0, tail=0x0
> root@debian:~/ddk_panfrost# panfrost ffa30000.gpu: gpu sched timeout,
> js=1, status=0x40, head=0x0, tail=0x0, sched_job=12a94ba6
>
> ======================================================
> WARNING: possible circular locking dependency detected
> 5.0.0+ #32 Not tainted
> ------------------------------------------------------
> kworker/1:0/608 is trying to acquire lock:
> 89b1e2d8 (&(&js->job_lock)->rlock){-.-.}, at:
> dma_fence_remove_callback+0x14/0x50
>
> but task is already holding lock:
> a887e4b2 (&(&sched->job_list_lock)->rlock){-.-.}, at:
> drm_sched_stop+0x24/0x10c
>
> which lock already depends on the new lock.
>
>
> the existing dependency chain (in reverse order) is:
>
> -> #1 (&(&sched->job_list_lock)->rlock){-.-.}:
>        drm_sched_process_job+0x60/0x208
>        dma_fence_signal+0x1dc/0x1fc
>        panfrost_job_irq_handler+0x160/0x194
>        __handle_irq_event_percpu+0x80/0x388
>        handle_irq_event_percpu+0x24/0x78
>        handle_irq_event+0x38/0x5c
>        handle_fasteoi_irq+0xb4/0x128
>        generic_handle_irq+0x18/0x28
>        __handle_domain_irq+0xa0/0xb4
>        gic_handle_irq+0x4c/0x78
>        __irq_svc+0x70/0x98
>        arch_cpu_idle+0x20/0x3c
>        arch_cpu_idle+0x20/0x3c
>        do_idle+0x11c/0x22c
>        cpu_startup_entry+0x18/0x20
>        start_kernel+0x398/0x420
>
> -> #0 (&(&js->job_lock)->rlock){-.-.}:
>        _raw_spin_lock_irqsave+0x50/0x64
>        dma_fence_remove_callback+0x14/0x50
>        drm_sched_stop+0x5c/0x10c
>        panfrost_job_timedout+0xd0/0x180
>        drm_sched_job_timedout+0x34/0x5c
>        process_one_work+0x2ac/0x6a4
>        worker_thread+0x28c/0x3fc
>        kthread+0x13c/0x158
>        ret_from_fork+0x14/0x20
>          (null)
>
> other info that might help us debug this:
>
>  Possible unsafe locking scenario:
>
>        CPU0                    CPU1
>        ----                    ----
>   lock(&(&sched->job_list_lock)->rlock);
>                                lock(&(&js->job_lock)->rlock);
>                                lock(&(&sched->job_list_lock)->rlock);
>   lock(&(&js->job_lock)->rlock);
>
>  *** DEADLOCK ***
>
> 3 locks held by kworker/1:0/608:
>  #0: 9b350627 ((wq_completion)"events"){+.+.}, at:
> process_one_work+0x1f8/0x6a4
>  #1: a802aa2d ((work_completion)(&(&sched->work_tdr)->work)){+.+.}, at:
> process_one_work+0x1f8/0x6a4
>  #2: a887e4b2 (&(&sched->job_list_lock)->rlock){-.-.}, at:
> drm_sched_stop+0x24/0x10c
>
> stack backtrace:
> CPU: 1 PID: 608 Comm: kworker/1:0 Not tainted 5.0.0+ #32
> Hardware name: Rockchip (Device Tree)
> Workqueue: events drm_sched_job_timedout
> [<c0111088>] (unwind_backtrace) from [<c010c9a8>] (show_stack+0x10/0x14)
> [<c010c9a8>] (show_stack) from [<c0773df4>] (dump_stack+0x9c/0xd4)
> [<c0773df4>] (dump_stack) from [<c016d034>]
> (print_circular_bug.constprop.15+0x1fc/0x2cc)
> [<c016d034>] (print_circular_bug.constprop.15) from [<c016f6c0>]
> (__lock_acquire+0xe5c/0x167c)
> [<c016f6c0>] (__lock_acquire) from [<c0170828>] (lock_acquire+0xc4/0x210)
> [<c0170828>] (lock_acquire) from [<c07920e0>]
> (_raw_spin_lock_irqsave+0x50/0x64)
> [<c07920e0>] (_raw_spin_lock_irqsave) from [<c0516784>]
> (dma_fence_remove_callback+0x14/0x50)
> [<c0516784>] (dma_fence_remove_callback) from [<c04def38>]
> (drm_sched_stop+0x5c/0x10c)
> [<c04def38>] (drm_sched_stop) from [<c04ec80c>]
> (panfrost_job_timedout+0xd0/0x180)
> [<c04ec80c>] (panfrost_job_timedout) from [<c04df340>]
> (drm_sched_job_timedout+0x34/0x5c)
> [<c04df340>] (drm_sched_job_timedout) from [<c013ec70>]
> (process_one_work+0x2ac/0x6a4)
> [<c013ec70>] (process_one_work) from [<c013fe48>]
> (worker_thread+0x28c/0x3fc)
> [<c013fe48>] (worker_thread) from [<c01452a0>] (kthread+0x13c/0x158)
> [<c01452a0>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20)
> Exception stack(0xeebd7fb0 to 0xeebd7ff8)
> 7fa0:                                     00000000 00000000 00000000
> 00000000
> 7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 00000000
> 7fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> ----8<----
>
> This is with the below simple reproducer:
>
> ----8<----
> #include <sys/ioctl.h>
> #include <fcntl.h>
> #include <stdio.h>
>
> #include <libdrm/drm.h>
> #include "panfrost_drm.h"
>
> int main(int argc, char **argv)
> {
>         int fd;
>
>         if (argc == 2)
>                 fd = open(argv[1], O_RDWR);
>         else
>                 fd = open("/dev/dri/renderD128", O_RDWR);
>         if (fd == -1) {
>                 perror("Failed to open");
>                 return 0;
>         }
>
>         struct drm_panfrost_submit submit = {
>                 .jc = 0,
>         };
>         return ioctl(fd, DRM_IOCTL_PANFROST_SUBMIT, &submit);
> }
> ----8<----
>
> Any ideas? I'm not an expert on DRM, so I got somewhat lost!

Tomeu?

Rob

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

Re: [PATCH v2 3/3] drm/panfrost: Add initial panfrost driver

[Tomeu Vizoso]

On Mon, 8 Apr 2019 at 23:04, Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Apr 5, 2019 at 7:30 AM Steven Price <steven.price@arm.com> wrote:
> >
> > On 01/04/2019 08:47, Rob Herring wrote:
> > > This adds the initial driver for panfrost which supports Arm Mali
> > > Midgard and Bifrost family of GPUs. Currently, only the T860 and
> > > T760 Midgard GPUs have been tested.
>
> [...]
> > > +
> > > +             if (status & JOB_INT_MASK_ERR(j)) {
> > > +                     job_write(pfdev, JS_COMMAND_NEXT(j), JS_COMMAND_NOP);
> > > +                     job_write(pfdev, JS_COMMAND(j), JS_COMMAND_HARD_STOP_0);
> >
> > Hard-stopping an already completed job isn't likely to do very much :)
> > Also you are using the "_0" version which is only valid when "job chain
> > disambiguation" is present.

Yeah, guess that can be removed.

> > I suspect in this case you should also be signalling the fence? At the
> > moment you rely on the GPU timeout recovering from the fault.
>
> I'll defer to Tomeu who wrote this (IIRC).

Yes, that would be an improvement.

> > One issue that I haven't got to the bottom of is that I can trigger a
> > lockdep splat:
> >
> > -----8<------
> > panfrost ffa30000.gpu: js fault, js=1, status=JOB_CONFIG_FAULT,
> > head=0x0, tail=0x0
> > root@debian:~/ddk_panfrost# panfrost ffa30000.gpu: gpu sched timeout,
> > js=1, status=0x40, head=0x0, tail=0x0, sched_job=12a94ba6
> >
> > ======================================================
> > WARNING: possible circular locking dependency detected
> > 5.0.0+ #32 Not tainted
> > ------------------------------------------------------
> > kworker/1:0/608 is trying to acquire lock:
> > 89b1e2d8 (&(&js->job_lock)->rlock){-.-.}, at:
> > dma_fence_remove_callback+0x14/0x50
> >
> > but task is already holding lock:
> > a887e4b2 (&(&sched->job_list_lock)->rlock){-.-.}, at:
> > drm_sched_stop+0x24/0x10c
> >
> > which lock already depends on the new lock.
> >
> >
> > the existing dependency chain (in reverse order) is:
> >
> > -> #1 (&(&sched->job_list_lock)->rlock){-.-.}:
> >        drm_sched_process_job+0x60/0x208
> >        dma_fence_signal+0x1dc/0x1fc
> >        panfrost_job_irq_handler+0x160/0x194
> >        __handle_irq_event_percpu+0x80/0x388
> >        handle_irq_event_percpu+0x24/0x78
> >        handle_irq_event+0x38/0x5c
> >        handle_fasteoi_irq+0xb4/0x128
> >        generic_handle_irq+0x18/0x28
> >        __handle_domain_irq+0xa0/0xb4
> >        gic_handle_irq+0x4c/0x78
> >        __irq_svc+0x70/0x98
> >        arch_cpu_idle+0x20/0x3c
> >        arch_cpu_idle+0x20/0x3c
> >        do_idle+0x11c/0x22c
> >        cpu_startup_entry+0x18/0x20
> >        start_kernel+0x398/0x420
> >
> > -> #0 (&(&js->job_lock)->rlock){-.-.}:
> >        _raw_spin_lock_irqsave+0x50/0x64
> >        dma_fence_remove_callback+0x14/0x50
> >        drm_sched_stop+0x5c/0x10c
> >        panfrost_job_timedout+0xd0/0x180
> >        drm_sched_job_timedout+0x34/0x5c
> >        process_one_work+0x2ac/0x6a4
> >        worker_thread+0x28c/0x3fc
> >        kthread+0x13c/0x158
> >        ret_from_fork+0x14/0x20
> >          (null)
> >
> > other info that might help us debug this:
> >
> >  Possible unsafe locking scenario:
> >
> >        CPU0                    CPU1
> >        ----                    ----
> >   lock(&(&sched->job_list_lock)->rlock);
> >                                lock(&(&js->job_lock)->rlock);
> >                                lock(&(&sched->job_list_lock)->rlock);
> >   lock(&(&js->job_lock)->rlock);
> >
> >  *** DEADLOCK ***
> >
> > 3 locks held by kworker/1:0/608:
> >  #0: 9b350627 ((wq_completion)"events"){+.+.}, at:
> > process_one_work+0x1f8/0x6a4
> >  #1: a802aa2d ((work_completion)(&(&sched->work_tdr)->work)){+.+.}, at:
> > process_one_work+0x1f8/0x6a4
> >  #2: a887e4b2 (&(&sched->job_list_lock)->rlock){-.-.}, at:
> > drm_sched_stop+0x24/0x10c
> >
> > stack backtrace:
> > CPU: 1 PID: 608 Comm: kworker/1:0 Not tainted 5.0.0+ #32
> > Hardware name: Rockchip (Device Tree)
> > Workqueue: events drm_sched_job_timedout
> > [<c0111088>] (unwind_backtrace) from [<c010c9a8>] (show_stack+0x10/0x14)
> > [<c010c9a8>] (show_stack) from [<c0773df4>] (dump_stack+0x9c/0xd4)
> > [<c0773df4>] (dump_stack) from [<c016d034>]
> > (print_circular_bug.constprop.15+0x1fc/0x2cc)
> > [<c016d034>] (print_circular_bug.constprop.15) from [<c016f6c0>]
> > (__lock_acquire+0xe5c/0x167c)
> > [<c016f6c0>] (__lock_acquire) from [<c0170828>] (lock_acquire+0xc4/0x210)
> > [<c0170828>] (lock_acquire) from [<c07920e0>]
> > (_raw_spin_lock_irqsave+0x50/0x64)
> > [<c07920e0>] (_raw_spin_lock_irqsave) from [<c0516784>]
> > (dma_fence_remove_callback+0x14/0x50)
> > [<c0516784>] (dma_fence_remove_callback) from [<c04def38>]
> > (drm_sched_stop+0x5c/0x10c)
> > [<c04def38>] (drm_sched_stop) from [<c04ec80c>]
> > (panfrost_job_timedout+0xd0/0x180)
> > [<c04ec80c>] (panfrost_job_timedout) from [<c04df340>]
> > (drm_sched_job_timedout+0x34/0x5c)
> > [<c04df340>] (drm_sched_job_timedout) from [<c013ec70>]
> > (process_one_work+0x2ac/0x6a4)
> > [<c013ec70>] (process_one_work) from [<c013fe48>]
> > (worker_thread+0x28c/0x3fc)
> > [<c013fe48>] (worker_thread) from [<c01452a0>] (kthread+0x13c/0x158)
> > [<c01452a0>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20)
> > Exception stack(0xeebd7fb0 to 0xeebd7ff8)
> > 7fa0:                                     00000000 00000000 00000000
> > 00000000
> > 7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> > 00000000
> > 7fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> > ----8<----
> >
> > This is with the below simple reproducer:
> >
> > ----8<----
> > #include <sys/ioctl.h>
> > #include <fcntl.h>
> > #include <stdio.h>
> >
> > #include <libdrm/drm.h>
> > #include "panfrost_drm.h"
> >
> > int main(int argc, char **argv)
> > {
> >         int fd;
> >
> >         if (argc == 2)
> >                 fd = open(argv[1], O_RDWR);
> >         else
> >                 fd = open("/dev/dri/renderD128", O_RDWR);
> >         if (fd == -1) {
> >                 perror("Failed to open");
> >                 return 0;
> >         }
> >
> >         struct drm_panfrost_submit submit = {
> >                 .jc = 0,
> >         };
> >         return ioctl(fd, DRM_IOCTL_PANFROST_SUBMIT, &submit);
> > }
> > ----8<----
> >
> > Any ideas? I'm not an expert on DRM, so I got somewhat lost!
>
> Tomeu?

Ran out of time today, but will be able to look at it tomorrow.

Thanks!

Tomeu

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

Re: [PATCH v2 3/3] drm/panfrost: Add initial panfrost driver

[Rob Herring]

On Tue, Apr 9, 2019 at 10:56 AM Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>
> On Mon, 8 Apr 2019 at 23:04, Rob Herring <robh@kernel.org> wrote:
> >
> > On Fri, Apr 5, 2019 at 7:30 AM Steven Price <steven.price@arm.com> wrote:
> > >
> > > On 01/04/2019 08:47, Rob Herring wrote:
> > > > This adds the initial driver for panfrost which supports Arm Mali
> > > > Midgard and Bifrost family of GPUs. Currently, only the T860 and
> > > > T760 Midgard GPUs have been tested.
> >
> > [...]
> > > > +
> > > > +             if (status & JOB_INT_MASK_ERR(j)) {
> > > > +                     job_write(pfdev, JS_COMMAND_NEXT(j), JS_COMMAND_NOP);
> > > > +                     job_write(pfdev, JS_COMMAND(j), JS_COMMAND_HARD_STOP_0);
> > >
> > > Hard-stopping an already completed job isn't likely to do very much :)
> > > Also you are using the "_0" version which is only valid when "job chain
> > > disambiguation" is present.
>
> Yeah, guess that can be removed.

Steven, TBC, are you suggesting removing both lines or leaving
JS_COMMAND_NOP? I don't think we can ever have a pending job at this
point as there's no queuing. So the NOP probably isn't needed, but
doesn't hurt to have it either.

> > > I suspect in this case you should also be signalling the fence? At the
> > > moment you rely on the GPU timeout recovering from the fault.
> >
> > I'll defer to Tomeu who wrote this (IIRC).
>
> Yes, that would be an improvement.

Actually, I think that would break recovery because the job timeout
will bail out if the done fence is signaled already. Perhaps we want
to timeout immediately if that is possible with the scheduler.

Rob

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

Re: [PATCH v2 3/3] drm/panfrost: Add initial panfrost driver

[Steven Price]

On 09/04/2019 17:15, Rob Herring wrote:
> On Tue, Apr 9, 2019 at 10:56 AM Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>>
>> On Mon, 8 Apr 2019 at 23:04, Rob Herring <robh@kernel.org> wrote:
>>>
>>> On Fri, Apr 5, 2019 at 7:30 AM Steven Price <steven.price@arm.com> wrote:
>>>>
>>>> On 01/04/2019 08:47, Rob Herring wrote:
>>>>> This adds the initial driver for panfrost which supports Arm Mali
>>>>> Midgard and Bifrost family of GPUs. Currently, only the T860 and
>>>>> T760 Midgard GPUs have been tested.
>>>
>>> [...]
>>>>> +
>>>>> +             if (status & JOB_INT_MASK_ERR(j)) {
>>>>> +                     job_write(pfdev, JS_COMMAND_NEXT(j), JS_COMMAND_NOP);
>>>>> +                     job_write(pfdev, JS_COMMAND(j), JS_COMMAND_HARD_STOP_0);
>>>>
>>>> Hard-stopping an already completed job isn't likely to do very much :)
>>>> Also you are using the "_0" version which is only valid when "job chain
>>>> disambiguation" is present.
>>
>> Yeah, guess that can be removed.
> 
> Steven, TBC, are you suggesting removing both lines or leaving
> JS_COMMAND_NOP? I don't think we can ever have a pending job at this
> point as there's no queuing. So the NOP probably isn't needed, but
> doesn't hurt to have it either.

Both lines are redundant and can be removed. But equally neither will
cause any problems.

Writing NOP into the next register is basically only needed if you know
there's a job there which you no longer want to execute.

kbase does this in certain situations. The main one is on a GPU without
command chain disambiguation if you want to kill a particular job
there's a potential race. For example:

 * Submit job A, followed by job B to slot 0. Job A is currently
executing, job B is waiting in the _NEXT registers.

 * Kernel decides it wants to kill job A (it's taking too long, or the
application has quit).

 * Simply executing a JS_COMMAND_HARD_STOP is racy. If job A finishes
just before doing the register write, then it's actually job B that gets
killed (and it's not always safe to just re-execute a killed job).

 * Instead write NOP to JS_COMMAND_NEXT, then check (again) whether the
job currently running is the one you want. When you then HARD_STOP you
either hit the correct job, or 'miss' and do nothing.

Job chain disambiguation solves this problem by allowing the kernel to
tag each job with a flag, the hard-stop can then be targetted at the job
with the correct flag. Writing NOP into JS_COMMAND_NEXT is also useful
if in the above situation you want to kill job B. In that case you can't
hard-stop it (it hasn't start), so you simply want to remove it from the
_NEXT registers.

>>>> I suspect in this case you should also be signalling the fence? At the
>>>> moment you rely on the GPU timeout recovering from the fault.
>>>
>>> I'll defer to Tomeu who wrote this (IIRC).
>>
>> Yes, that would be an improvement.
> 
> Actually, I think that would break recovery because the job timeout
> will bail out if the done fence is signaled already. Perhaps we want
> to timeout immediately if that is possible with the scheduler.

Ideally you would signal the fence with an error code (which is
presumably what recovery does). There's no actual need to trigger a
timeout. I'm not sure quite how the DRM infrastructure handles this though.

Steve

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

Re: [PATCH v2 3/3] drm/panfrost: Add initial panfrost driver

[Steven Price]

On 08/04/2019 22:04, Rob Herring wrote:
> On Fri, Apr 5, 2019 at 7:30 AM Steven Price <steven.price@arm.com> wrote:
>>
>> On 01/04/2019 08:47, Rob Herring wrote:
>>> This adds the initial driver for panfrost which supports Arm Mali
>>> Midgard and Bifrost family of GPUs. Currently, only the T860 and
>>> T760 Midgard GPUs have been tested.
> 
> [...]
> 
>>> +     case 0xD3: return "TRANSTAB_BUS_FAULT_LEVEL3";
>>> +     case 0xD4: return "TRANSTAB_BUS_FAULT_LEVEL4";
>>> +     case 0xD8: return "ACCESS_FLAG";
>>> +     }
>>> +
>>> +     if (panfrost_has_hw_feature(pfdev, HW_FEATURE_AARCH64_MMU)) {
>>
>> There's not a great deal of point in this conditional - you won't get
>> the below exception codes from hardware which doesn't support it AARCH64
>> page tables.
> 
> I wasn't sure if "UNKNOWN" types could happen or not.
> 
>>
>>> +             switch (exception_code) {
>>> +             case 0xC9 ... 0xCF: return "PERMISSION_FAULT";
>>> +             case 0xD9 ... 0xDF: return "ACCESS_FLAG";
>>> +             case 0xE0 ... 0xE7: return "ADDRESS_SIZE_FAULT";
>>> +             case 0xE8 ... 0xEF: return "MEMORY_ATTRIBUTES_FAULT";
>>> +             }
>>> +     }
>>> +
>>> +     return "UNKNOWN";
>>> +}
> 
>>> +static inline int panfrost_model_cmp(struct panfrost_device *pfdev, s32 id)
>>> +{
>>> +     s32 match_id = pfdev->features.id;
>>> +
>>> +     if (match_id & 0xf000)
>>> +             match_id &= 0xf00f;
>>
>> I'm puzzled by this, and really not sure if it's going to work out
>> ignoring the ARCH_MINOR/ARCH_REV fields. But for now there's no real
>> Bifrost support.
> 
> That seemed to be enough for kbase to select features/issues.

I can't deny that it seems to work for now, and indeed looking more
closely at kbase that does seem to be the effect of the current code.

>>> +     switch (param->param) {
>>> +     case DRM_PANFROST_PARAM_GPU_ID:
>>> +             param->value = pfdev->features.id;
>>
>> This is unfortunate naming - this parameter is *not* the GPU_ID. It is
>> the top half of the GPU_ID which kbase calls the PRODUCT_ID.
> 
> I can rename it.

It would help prevent confusion, thanks!

>> I'm also somewhat surprised that you don't need loads of other
>> properties from the GPU - in particular knowing the number of shader
>> cores is useful for allocating the right amount of memory for TLS (and
>> can't be obtained purely from the GPU_ID).
> 
> We'll add them as userspace needs them.

Fair enough. I'm not sure how much you want to provide "forward
compatibility" by providing them early - the basic definitions are
already in kbase. I found it a bit surprising that some feature
registers are printed on probe, but not available to be queried.

>>> +static int
>>> +panfrost_lookup_bos(struct drm_device *dev,
>>> +               struct drm_file *file_priv,
>>> +               struct drm_panfrost_submit *args,
>>> +               struct panfrost_job *job)
>>> +{
>>> +     u32 *handles;
>>> +     int ret = 0;
>>> +
>>> +     job->bo_count = args->bo_handle_count;
>>> +
>>> +     if (!job->bo_count)
>>> +             return 0;
>>> +
>>> +     job->bos = kvmalloc_array(job->bo_count,
>>> +                               sizeof(struct drm_gem_object *),
>>> +                               GFP_KERNEL | __GFP_ZERO);
>>> +     if (!job->bos)
>>
>> If we return here then job->bo_count has been set, but job->bos is
>> invalid - this means that panfrost_job_cleanup() will then dereference
>> NULL. Although maybe this is better fixed in panfrost_job_cleanup().
> 
> Good catch. The fence arrays have the same problem. As does V3D which we copied.
> 
>>> +             return -ENOMEM;
>>> +
>>> +     job->implicit_fences = kvmalloc_array(job->bo_count,
>>> +                               sizeof(struct dma_fence *),
>>> +                               GFP_KERNEL | __GFP_ZERO);
>>> +     if (!job->implicit_fences)
>>> +             return -ENOMEM;
> 
>>> +static irqreturn_t panfrost_gpu_irq_handler(int irq, void *data)
>>> +{
>>> +     struct panfrost_device *pfdev = data;
>>> +     u32 state = gpu_read(pfdev, GPU_INT_STAT);
>>> +     u32 fault_status = gpu_read(pfdev, GPU_FAULT_STATUS);
>>> +     u64 address;
>>> +     bool done = false;
>>> +
>>> +     if (!state)
>>> +             return IRQ_NONE;
>>> +
>>> +     if (state & GPU_IRQ_MASK_ERROR) {
>>> +             address = (u64) gpu_read(pfdev, GPU_FAULT_ADDRESS_HI) << 32;
>>> +             address |= gpu_read(pfdev, GPU_FAULT_ADDRESS_LO);
>>> +
>>> +             dev_warn(pfdev->dev, "GPU Fault 0x%08x (%s) at 0x%016llx\n",
>>> +                      fault_status & 0xFF, panfrost_exception_name(pfdev, fault_status),
>>> +                      address);
>>> +
>>> +             if (state & GPU_IRQ_MULTIPLE_FAULT)
>>> +                     dev_warn(pfdev->dev, "There were multiple GPU faults - some have not been reported\n");
>>> +
>>> +             gpu_write(pfdev, GPU_INT_MASK, 0);
>>
>> Do you intend to mask off future GPU interrupts?
> 
> Yes, maybe?
> 
> If we fault here, then we are going to reset the gpu on timeout which
> then re-enables interrupts.

Well fair enough. But there's no actual need to force a GPU reset.
Really there's nothing you can do other than report a GPU fault. kbase
simply reports it and otherwise ignores it (no GPU reset).

Also will you actually trigger the GPU timeout? This won't mask of the
JOB completion IRQ, so jobs can still complete.

When you integrate other GPU irq sources (counters/power management)
then you almost certainly don't want to mask those off just because of a
GPU fault.

>>> +static irqreturn_t panfrost_job_irq_handler(int irq, void *data)
>>> +{
>>> +     struct panfrost_device *pfdev = data;
>>> +     u32 status = job_read(pfdev, JOB_INT_STAT);
>>> +     int j;
>>> +
>>> +     dev_dbg(pfdev->dev, "jobslot irq status=%x\n", status);
>>> +
>>> +     if (!status)
>>> +             return IRQ_NONE;
>>> +
>>> +     pm_runtime_mark_last_busy(pfdev->dev);
>>> +
>>> +     for (j = 0; status; j++) {
>>> +             u32 mask = MK_JS_MASK(j);
>>> +
>>> +             if (!(status & mask))
>>> +                     continue;
>>> +
>>> +             job_write(pfdev, JOB_INT_CLEAR, mask);
>>> +
>>> +             if (status & JOB_INT_MASK_ERR(j)) {
>>> +                     job_write(pfdev, JS_COMMAND_NEXT(j), JS_COMMAND_NOP);
>>> +                     job_write(pfdev, JS_COMMAND(j), JS_COMMAND_HARD_STOP_0);
>>
>> Hard-stopping an already completed job isn't likely to do very much :)
>> Also you are using the "_0" version which is only valid when "job chain
>> disambiguation" is present.
>>
>> I suspect in this case you should also be signalling the fence? At the
>> moment you rely on the GPU timeout recovering from the fault.
> 
> I'll defer to Tomeu who wrote this (IIRC).
> 
> 
>>> +static void lock_region(struct panfrost_device *pfdev, u32 as_nr,
>>> +                     u64 iova, size_t size)
>>> +{
>>> +     u8 region_width;
>>> +     u64 region = iova & PAGE_MASK;
>>> +     /*
>>> +      * fls returns (given the ASSERT above):
>>
>> But where's the assert? :p
>>
>>> +      * 1 .. 32
>>> +      *
>>> +      * 10 + fls(num_pages)
>>> +      * results in the range (11 .. 42)
>>> +      */
>>> +
>>> +     size = round_up(size, PAGE_SIZE);
>>
>> I'm not sure it matters, but this will break if called on a (small, i.e.
>> less than a page) region spanning two pages. "region" will be rounded
>> down to the page (iova & PAGE_MASK), but size will only be rounded up to
>> the nearest page. This can miss the start of the second page.
> 
> This is probably redundant. Everything the driver does with memory is
> in units of pages. Maybe imported buffers could be unaligned. Not sure
> and we'd probably break in other places if that was the case.

Yes I don't think this case will occur at the moment. I just noticed it
because the interface was changed from kbase (kbase passes in a page
offset, this version uses a byte offset).

>>> +             /* terminal fault, print info about the fault */
>>> +             dev_err(pfdev->dev,
>>> +                     "Unhandled Page fault in AS%d at VA 0x%016llX\n"
>>> +                     "Reason: %s\n"
>>> +                     "raw fault status: 0x%X\n"
>>> +                     "decoded fault status: %s\n"
>>> +                     "exception type 0x%X: %s\n"
>>> +                     "access type 0x%X: %s\n"
>>> +                     "source id 0x%X\n",
>>> +                     i, addr,
>>> +                     "TODO",
>>> +                     fault_status,
>>> +                     (fault_status & (1 << 10) ? "DECODER FAULT" : "SLAVE FAULT"),
>>> +                     exception_type, panfrost_exception_name(pfdev, exception_type),
>>> +                     access_type, access_type_name(pfdev, fault_status),
>>> +                     source_id);
>>> +
>>> +             mmu_write(pfdev, MMU_INT_CLEAR, mask);
>>
>> To fully handle the fault you will need to issue an MMU command (i.e.
>> call mmu_hw_do_operation()) - obviously after doing something to fix the
>> cause (e.g. mapping a page or switching the GPU to UNMAPPED mode to kill
>> off the job). Otherwise you may see that the GPU hangs. Although in
>> practice at this stage of the driver the generic timeout is probably the
>> simplest way of handling an MMU fault.
> 
> Any fault currently is unexpected so all we really care about at this
> point is getting a message.

No problem. It will become relevant when you schedule work from multiple
applications at the same time.

[...]
>>
>> This is with the below simple reproducer:
>>
>> ----8<----
>> #include <sys/ioctl.h>
>> #include <fcntl.h>
>> #include <stdio.h>
>>
>> #include <libdrm/drm.h>
>> #include "panfrost_drm.h"
>>
>> int main(int argc, char **argv)
>> {
>>         int fd;
>>
>>         if (argc == 2)
>>                 fd = open(argv[1], O_RDWR);
>>         else
>>                 fd = open("/dev/dri/renderD128", O_RDWR);
>>         if (fd == -1) {
>>                 perror("Failed to open");
>>                 return 0;
>>         }
>>
>>         struct drm_panfrost_submit submit = {
>>                 .jc = 0,
>>         };
>>         return ioctl(fd, DRM_IOCTL_PANFROST_SUBMIT, &submit);
>> }
>> ----8<----
>>
>> Any ideas? I'm not an expert on DRM, so I got somewhat lost!

Interestingly this actually looks similar to this bug for amdgpu:

https://bugs.freedesktop.org/show_bug.cgi?id=109692

There's a patch on there changing the drm scheduler which might be
relevant. I haven't had chance to try it out.

Steve

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

Re: [PATCH v2 3/3] drm/panfrost: Add initial panfrost driver

[Tomeu Vizoso]

On Wed, 10 Apr 2019 at 12:20, Steven Price <steven.price@arm.com> wrote:
>
> On 08/04/2019 22:04, Rob Herring wrote:
> > On Fri, Apr 5, 2019 at 7:30 AM Steven Price <steven.price@arm.com> wrote:
> >>
> >> On 01/04/2019 08:47, Rob Herring wrote:
> >>> This adds the initial driver for panfrost which supports Arm Mali
> >>> Midgard and Bifrost family of GPUs. Currently, only the T860 and
> >>> T760 Midgard GPUs have been tested.
> >
> > [...]
> >
> >>> +     case 0xD3: return "TRANSTAB_BUS_FAULT_LEVEL3";
> >>> +     case 0xD4: return "TRANSTAB_BUS_FAULT_LEVEL4";
> >>> +     case 0xD8: return "ACCESS_FLAG";
> >>> +     }
> >>> +
> >>> +     if (panfrost_has_hw_feature(pfdev, HW_FEATURE_AARCH64_MMU)) {
> >>
> >> There's not a great deal of point in this conditional - you won't get
> >> the below exception codes from hardware which doesn't support it AARCH64
> >> page tables.
> >
> > I wasn't sure if "UNKNOWN" types could happen or not.
> >
> >>
> >>> +             switch (exception_code) {
> >>> +             case 0xC9 ... 0xCF: return "PERMISSION_FAULT";
> >>> +             case 0xD9 ... 0xDF: return "ACCESS_FLAG";
> >>> +             case 0xE0 ... 0xE7: return "ADDRESS_SIZE_FAULT";
> >>> +             case 0xE8 ... 0xEF: return "MEMORY_ATTRIBUTES_FAULT";
> >>> +             }
> >>> +     }
> >>> +
> >>> +     return "UNKNOWN";
> >>> +}
> >
> >>> +static inline int panfrost_model_cmp(struct panfrost_device *pfdev, s32 id)
> >>> +{
> >>> +     s32 match_id = pfdev->features.id;
> >>> +
> >>> +     if (match_id & 0xf000)
> >>> +             match_id &= 0xf00f;
> >>
> >> I'm puzzled by this, and really not sure if it's going to work out
> >> ignoring the ARCH_MINOR/ARCH_REV fields. But for now there's no real
> >> Bifrost support.
> >
> > That seemed to be enough for kbase to select features/issues.
>
> I can't deny that it seems to work for now, and indeed looking more
> closely at kbase that does seem to be the effect of the current code.
>
> >>> +     switch (param->param) {
> >>> +     case DRM_PANFROST_PARAM_GPU_ID:
> >>> +             param->value = pfdev->features.id;
> >>
> >> This is unfortunate naming - this parameter is *not* the GPU_ID. It is
> >> the top half of the GPU_ID which kbase calls the PRODUCT_ID.
> >
> > I can rename it.
>
> It would help prevent confusion, thanks!
>
> >> I'm also somewhat surprised that you don't need loads of other
> >> properties from the GPU - in particular knowing the number of shader
> >> cores is useful for allocating the right amount of memory for TLS (and
> >> can't be obtained purely from the GPU_ID).
> >
> > We'll add them as userspace needs them.
>
> Fair enough. I'm not sure how much you want to provide "forward
> compatibility" by providing them early - the basic definitions are
> already in kbase. I found it a bit surprising that some feature
> registers are printed on probe, but not available to be queried.
>
> >>> +static int
> >>> +panfrost_lookup_bos(struct drm_device *dev,
> >>> +               struct drm_file *file_priv,
> >>> +               struct drm_panfrost_submit *args,
> >>> +               struct panfrost_job *job)
> >>> +{
> >>> +     u32 *handles;
> >>> +     int ret = 0;
> >>> +
> >>> +     job->bo_count = args->bo_handle_count;
> >>> +
> >>> +     if (!job->bo_count)
> >>> +             return 0;
> >>> +
> >>> +     job->bos = kvmalloc_array(job->bo_count,
> >>> +                               sizeof(struct drm_gem_object *),
> >>> +                               GFP_KERNEL | __GFP_ZERO);
> >>> +     if (!job->bos)
> >>
> >> If we return here then job->bo_count has been set, but job->bos is
> >> invalid - this means that panfrost_job_cleanup() will then dereference
> >> NULL. Although maybe this is better fixed in panfrost_job_cleanup().
> >
> > Good catch. The fence arrays have the same problem. As does V3D which we copied.
> >
> >>> +             return -ENOMEM;
> >>> +
> >>> +     job->implicit_fences = kvmalloc_array(job->bo_count,
> >>> +                               sizeof(struct dma_fence *),
> >>> +                               GFP_KERNEL | __GFP_ZERO);
> >>> +     if (!job->implicit_fences)
> >>> +             return -ENOMEM;
> >
> >>> +static irqreturn_t panfrost_gpu_irq_handler(int irq, void *data)
> >>> +{
> >>> +     struct panfrost_device *pfdev = data;
> >>> +     u32 state = gpu_read(pfdev, GPU_INT_STAT);
> >>> +     u32 fault_status = gpu_read(pfdev, GPU_FAULT_STATUS);
> >>> +     u64 address;
> >>> +     bool done = false;
> >>> +
> >>> +     if (!state)
> >>> +             return IRQ_NONE;
> >>> +
> >>> +     if (state & GPU_IRQ_MASK_ERROR) {
> >>> +             address = (u64) gpu_read(pfdev, GPU_FAULT_ADDRESS_HI) << 32;
> >>> +             address |= gpu_read(pfdev, GPU_FAULT_ADDRESS_LO);
> >>> +
> >>> +             dev_warn(pfdev->dev, "GPU Fault 0x%08x (%s) at 0x%016llx\n",
> >>> +                      fault_status & 0xFF, panfrost_exception_name(pfdev, fault_status),
> >>> +                      address);
> >>> +
> >>> +             if (state & GPU_IRQ_MULTIPLE_FAULT)
> >>> +                     dev_warn(pfdev->dev, "There were multiple GPU faults - some have not been reported\n");
> >>> +
> >>> +             gpu_write(pfdev, GPU_INT_MASK, 0);
> >>
> >> Do you intend to mask off future GPU interrupts?
> >
> > Yes, maybe?
> >
> > If we fault here, then we are going to reset the gpu on timeout which
> > then re-enables interrupts.
>
> Well fair enough. But there's no actual need to force a GPU reset.
> Really there's nothing you can do other than report a GPU fault. kbase
> simply reports it and otherwise ignores it (no GPU reset).
>
> Also will you actually trigger the GPU timeout? This won't mask of the
> JOB completion IRQ, so jobs can still complete.
>
> When you integrate other GPU irq sources (counters/power management)
> then you almost certainly don't want to mask those off just because of a
> GPU fault.
>
> >>> +static irqreturn_t panfrost_job_irq_handler(int irq, void *data)
> >>> +{
> >>> +     struct panfrost_device *pfdev = data;
> >>> +     u32 status = job_read(pfdev, JOB_INT_STAT);
> >>> +     int j;
> >>> +
> >>> +     dev_dbg(pfdev->dev, "jobslot irq status=%x\n", status);
> >>> +
> >>> +     if (!status)
> >>> +             return IRQ_NONE;
> >>> +
> >>> +     pm_runtime_mark_last_busy(pfdev->dev);
> >>> +
> >>> +     for (j = 0; status; j++) {
> >>> +             u32 mask = MK_JS_MASK(j);
> >>> +
> >>> +             if (!(status & mask))
> >>> +                     continue;
> >>> +
> >>> +             job_write(pfdev, JOB_INT_CLEAR, mask);
> >>> +
> >>> +             if (status & JOB_INT_MASK_ERR(j)) {
> >>> +                     job_write(pfdev, JS_COMMAND_NEXT(j), JS_COMMAND_NOP);
> >>> +                     job_write(pfdev, JS_COMMAND(j), JS_COMMAND_HARD_STOP_0);
> >>
> >> Hard-stopping an already completed job isn't likely to do very much :)
> >> Also you are using the "_0" version which is only valid when "job chain
> >> disambiguation" is present.
> >>
> >> I suspect in this case you should also be signalling the fence? At the
> >> moment you rely on the GPU timeout recovering from the fault.
> >
> > I'll defer to Tomeu who wrote this (IIRC).
> >
> >
> >>> +static void lock_region(struct panfrost_device *pfdev, u32 as_nr,
> >>> +                     u64 iova, size_t size)
> >>> +{
> >>> +     u8 region_width;
> >>> +     u64 region = iova & PAGE_MASK;
> >>> +     /*
> >>> +      * fls returns (given the ASSERT above):
> >>
> >> But where's the assert? :p
> >>
> >>> +      * 1 .. 32
> >>> +      *
> >>> +      * 10 + fls(num_pages)
> >>> +      * results in the range (11 .. 42)
> >>> +      */
> >>> +
> >>> +     size = round_up(size, PAGE_SIZE);
> >>
> >> I'm not sure it matters, but this will break if called on a (small, i.e.
> >> less than a page) region spanning two pages. "region" will be rounded
> >> down to the page (iova & PAGE_MASK), but size will only be rounded up to
> >> the nearest page. This can miss the start of the second page.
> >
> > This is probably redundant. Everything the driver does with memory is
> > in units of pages. Maybe imported buffers could be unaligned. Not sure
> > and we'd probably break in other places if that was the case.
>
> Yes I don't think this case will occur at the moment. I just noticed it
> because the interface was changed from kbase (kbase passes in a page
> offset, this version uses a byte offset).
>
> >>> +             /* terminal fault, print info about the fault */
> >>> +             dev_err(pfdev->dev,
> >>> +                     "Unhandled Page fault in AS%d at VA 0x%016llX\n"
> >>> +                     "Reason: %s\n"
> >>> +                     "raw fault status: 0x%X\n"
> >>> +                     "decoded fault status: %s\n"
> >>> +                     "exception type 0x%X: %s\n"
> >>> +                     "access type 0x%X: %s\n"
> >>> +                     "source id 0x%X\n",
> >>> +                     i, addr,
> >>> +                     "TODO",
> >>> +                     fault_status,
> >>> +                     (fault_status & (1 << 10) ? "DECODER FAULT" : "SLAVE FAULT"),
> >>> +                     exception_type, panfrost_exception_name(pfdev, exception_type),
> >>> +                     access_type, access_type_name(pfdev, fault_status),
> >>> +                     source_id);
> >>> +
> >>> +             mmu_write(pfdev, MMU_INT_CLEAR, mask);
> >>
> >> To fully handle the fault you will need to issue an MMU command (i.e.
> >> call mmu_hw_do_operation()) - obviously after doing something to fix the
> >> cause (e.g. mapping a page or switching the GPU to UNMAPPED mode to kill
> >> off the job). Otherwise you may see that the GPU hangs. Although in
> >> practice at this stage of the driver the generic timeout is probably the
> >> simplest way of handling an MMU fault.
> >
> > Any fault currently is unexpected so all we really care about at this
> > point is getting a message.
>
> No problem. It will become relevant when you schedule work from multiple
> applications at the same time.
>
> [...]
> >>
> >> This is with the below simple reproducer:
> >>
> >> ----8<----
> >> #include <sys/ioctl.h>
> >> #include <fcntl.h>
> >> #include <stdio.h>
> >>
> >> #include <libdrm/drm.h>
> >> #include "panfrost_drm.h"
> >>
> >> int main(int argc, char **argv)
> >> {
> >>         int fd;
> >>
> >>         if (argc == 2)
> >>                 fd = open(argv[1], O_RDWR);
> >>         else
> >>                 fd = open("/dev/dri/renderD128", O_RDWR);
> >>         if (fd == -1) {
> >>                 perror("Failed to open");
> >>                 return 0;
> >>         }
> >>
> >>         struct drm_panfrost_submit submit = {
> >>                 .jc = 0,
> >>         };
> >>         return ioctl(fd, DRM_IOCTL_PANFROST_SUBMIT, &submit);
> >> }
> >> ----8<----
> >>
> >> Any ideas? I'm not an expert on DRM, so I got somewhat lost!
>
> Interestingly this actually looks similar to this bug for amdgpu:
>
> https://bugs.freedesktop.org/show_bug.cgi?id=109692
>
> There's a patch on there changing the drm scheduler which might be
> relevant. I haven't had chance to try it out.

Seems indeed to be the case, and I guess all drivers using gpu-sched
have the same problem.

There's ongoing discussions on the fix for it, so I will leave it be for now.

Thanks for the pointer!

Tomeu

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