[PATCH 0/9] drm/panfrost: Initial Valhall support

[PATCH 0/9] drm/panfrost: Initial Valhall support

[alyssa.rosenzweig]

From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

This patch series adds preliminary support for Mali "Valhall" GPUs into
the Panfrost kernel driver. The series has been tested on the Mali-G57
on a MediaTek MT8192 system. However, that system requires additional
MediaTek-specific patches [1] as well as core mainlining for MediaTek.
I'll post the MT8192-specific Panfrost patches soon; they depend on this
core series.

On the userspace side, pre-CSF Valhall (what is supported here) uses an
identical UABI as Bifrost. Mesa support for Valhall is being worked on
in parallel [2]. I'm hoping basic support for Valhall will be available
in Mesa 22.1.

[1] https://gitlab.freedesktop.org/panfrost/linux/-/tree/mt8192-branch
[2] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/14795

Alyssa Rosenzweig (9):
  dt-bindings: Add arm,mali-valhall compatible
  drm/panfrost: Handle HW_ISSUE_TTRX_2968_TTRX_3162
  drm/panfrost: Constify argument to has_hw_issue
  drm/panfrost: Handle HW_ISSUE_TTRX_3076
  drm/panfrost: Add HW_ISSUE_TTRX_3485 quirk
  drm/panfrost: Add "clean only safe" feature bit
  drm/panfrost: Don't set L2_MMU_CONFIG quirks
  drm/panfrost: Add Mali-G57 "Natt" support
  drm/panfrost: Handle arm,mali-valhall compatible

 .../bindings/gpu/arm,mali-bifrost.yaml          |  1 +
 drivers/gpu/drm/panfrost/panfrost_device.c      |  9 +++++++--
 drivers/gpu/drm/panfrost/panfrost_drv.c         |  1 +
 drivers/gpu/drm/panfrost/panfrost_features.h    | 13 +++++++++++++
 drivers/gpu/drm/panfrost/panfrost_gpu.c         | 17 +++++------------
 drivers/gpu/drm/panfrost/panfrost_issues.h      | 17 ++++++++++++++++-
 drivers/gpu/drm/panfrost/panfrost_regs.h        |  1 +
 7 files changed, 44 insertions(+), 15 deletions(-)

-- 
2.34.1

[PATCH 1/9] dt-bindings: Add arm,mali-valhall compatible

[alyssa.rosenzweig]

From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

From the kernel's perspective, pre-CSF Valhall is more or less
compatible with Bifrost, although they differ to userspace. Add a
compatible for Valhall to the existing Bifrost bindings documentation.

Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
Cc: devicetree@vger.kernel.org
---
 Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
index 63a08f3f321d..48aeabd2ed68 100644
--- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
+++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
@@ -23,6 +23,7 @@ properties:
           - rockchip,px30-mali
           - rockchip,rk3568-mali
       - const: arm,mali-bifrost # Mali Bifrost GPU model/revision is fully discoverable
+      - const: arm,mali-valhall # Mali Valhall GPU model/revision is fully discoverable
 
   reg:
     maxItems: 1
-- 
2.34.1

[PATCH 1/9] dt-bindings: Add arm,mali-valhall compatible

[alyssa.rosenzweig]

From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

From the kernel's perspective, pre-CSF Valhall is more or less
compatible with Bifrost, although they differ to userspace. Add a
compatible for Valhall to the existing Bifrost bindings documentation.

Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
Cc: devicetree@vger.kernel.org
---
 Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
index 63a08f3f321d..48aeabd2ed68 100644
--- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
+++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
@@ -23,6 +23,7 @@ properties:
           - rockchip,px30-mali
           - rockchip,rk3568-mali
       - const: arm,mali-bifrost # Mali Bifrost GPU model/revision is fully discoverable
+      - const: arm,mali-valhall # Mali Valhall GPU model/revision is fully discoverable
 
   reg:
     maxItems: 1
-- 
2.34.1

[PATCH 2/9] drm/panfrost: Handle HW_ISSUE_TTRX_2968_TTRX_3162

[alyssa.rosenzweig]

From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

Add handling for the HW_ISSUE_TTRX_2968_TTRX_3162 quirk. Logic ported
from kbase. kbase lists this workaround as used on Mali-G57.

Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_gpu.c    | 3 +++
 drivers/gpu/drm/panfrost/panfrost_issues.h | 3 +++
 drivers/gpu/drm/panfrost/panfrost_regs.h   | 1 +
 3 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
index 50c8922694d7..1c1e2017aa80 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
@@ -108,6 +108,9 @@ static void panfrost_gpu_init_quirks(struct panfrost_device *pfdev)
 			quirks |= SC_LS_ALLOW_ATTR_TYPES;
 	}
 
+	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_TTRX_2968_TTRX_3162))
+		quirks |= SC_VAR_ALGORITHM;
+
 	if (panfrost_has_hw_feature(pfdev, HW_FEATURE_TLS_HASHING))
 		quirks |= SC_TLS_HASH_ENABLE;
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_issues.h b/drivers/gpu/drm/panfrost/panfrost_issues.h
index 8e59d765bf19..3af7d723377e 100644
--- a/drivers/gpu/drm/panfrost/panfrost_issues.h
+++ b/drivers/gpu/drm/panfrost/panfrost_issues.h
@@ -125,6 +125,9 @@ enum panfrost_hw_issue {
 	 * kernel must fiddle with L2 caches to prevent data leakage */
 	HW_ISSUE_TGOX_R1_1234,
 
+	/* Must set SC_VAR_ALGORITHM */
+	HW_ISSUE_TTRX_2968_TTRX_3162,
+
 	HW_ISSUE_END
 };
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_regs.h b/drivers/gpu/drm/panfrost/panfrost_regs.h
index 16e776cc82ea..fa1e1af56e17 100644
--- a/drivers/gpu/drm/panfrost/panfrost_regs.h
+++ b/drivers/gpu/drm/panfrost/panfrost_regs.h
@@ -195,6 +195,7 @@
 #define SC_TLS_HASH_ENABLE		BIT(17)
 #define SC_LS_ATTR_CHECK_DISABLE	BIT(18)
 #define SC_ENABLE_TEXGRD_FLAGS		BIT(25)
+#define SC_VAR_ALGORITHM		BIT(29)
 /* End SHADER_CONFIG register */
 
 /* TILER_CONFIG register */
-- 
2.34.1

[PATCH 3/9] drm/panfrost: Constify argument to has_hw_issue

[alyssa.rosenzweig]

From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

Logically, this function is free of side effects, so any pointers it
takes should be const. Needed to avoid a warning in the next patch.

Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_issues.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_issues.h b/drivers/gpu/drm/panfrost/panfrost_issues.h
index 3af7d723377e..a66692663833 100644
--- a/drivers/gpu/drm/panfrost/panfrost_issues.h
+++ b/drivers/gpu/drm/panfrost/panfrost_issues.h
@@ -251,7 +251,7 @@ enum panfrost_hw_issue {
 
 #define hw_issues_g76 0
 
-static inline bool panfrost_has_hw_issue(struct panfrost_device *pfdev,
+static inline bool panfrost_has_hw_issue(const struct panfrost_device *pfdev,
 					 enum panfrost_hw_issue issue)
 {
 	return test_bit(issue, pfdev->features.hw_issues);
-- 
2.34.1

[PATCH 4/9] drm/panfrost: Handle HW_ISSUE_TTRX_3076

[alyssa.rosenzweig]

From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

Some Valhall GPUs require resets when encountering bus faults due to
occlusion query writes. Add the issue bit for this and handle it.

Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.c | 9 +++++++--
 drivers/gpu/drm/panfrost/panfrost_issues.h | 4 ++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index 7f51a4682ccb..ee612303f076 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -11,6 +11,7 @@
 #include "panfrost_device.h"
 #include "panfrost_devfreq.h"
 #include "panfrost_features.h"
+#include "panfrost_issues.h"
 #include "panfrost_gpu.h"
 #include "panfrost_job.h"
 #include "panfrost_mmu.h"
@@ -380,9 +381,13 @@ const char *panfrost_exception_name(u32 exception_code)
 bool panfrost_exception_needs_reset(const struct panfrost_device *pfdev,
 				    u32 exception_code)
 {
-	/* Right now, none of the GPU we support need a reset, but this
-	 * might change.
+	/* If an occlusion query write causes a bus fault on affected GPUs,
+	 * future fragment jobs may hang. Reset to workaround.
 	 */
+	if (exception_code == DRM_PANFROST_EXCEPTION_JOB_BUS_FAULT)
+		return panfrost_has_hw_issue(pfdev, HW_ISSUE_TTRX_3076);
+
+	/* No other GPUs we support need a reset */
 	return false;
 }
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_issues.h b/drivers/gpu/drm/panfrost/panfrost_issues.h
index a66692663833..058f6a4c8435 100644
--- a/drivers/gpu/drm/panfrost/panfrost_issues.h
+++ b/drivers/gpu/drm/panfrost/panfrost_issues.h
@@ -128,6 +128,10 @@ enum panfrost_hw_issue {
 	/* Must set SC_VAR_ALGORITHM */
 	HW_ISSUE_TTRX_2968_TTRX_3162,
 
+	/* Bus fault from occlusion query write may cause future fragment jobs
+	 * to hang */
+	HW_ISSUE_TTRX_3076,
+
 	HW_ISSUE_END
 };
 
-- 
2.34.1

[PATCH 5/9] drm/panfrost: Add HW_ISSUE_TTRX_3485 quirk

[alyssa.rosenzweig]

From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

TTRX_3485 requires the infamous "dummy job" workaround. I have this
workaround implemented in a local branch, but I have not yet hit a case
that requires it so I cannot test whether the implementation is correct.
In the mean time, add the quirk bit so we can document which platforms
may need it in the future.

Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_issues.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_issues.h b/drivers/gpu/drm/panfrost/panfrost_issues.h
index 058f6a4c8435..b8865fc9efce 100644
--- a/drivers/gpu/drm/panfrost/panfrost_issues.h
+++ b/drivers/gpu/drm/panfrost/panfrost_issues.h
@@ -132,6 +132,9 @@ enum panfrost_hw_issue {
 	 * to hang */
 	HW_ISSUE_TTRX_3076,
 
+	/* Must issue a dummy job before starting real work to prevent hangs */
+	HW_ISSUE_TTRX_3485,
+
 	HW_ISSUE_END
 };
 
-- 
2.34.1

[PATCH 6/9] drm/panfrost: Add "clean only safe" feature bit

[alyssa.rosenzweig]

From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

Add the HW_FEATURE_CLEAN_ONLY_SAFE bit based on kbase. When I actually
tried to port the logic from kbase, trivial jobs raised Data Invalid
Faults, so this may depend on other coherency details. It's still useful
to have the bit to record the feature bit when adding new models.

Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_features.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_features.h b/drivers/gpu/drm/panfrost/panfrost_features.h
index 36fadcf9634e..1a8bdebc86a3 100644
--- a/drivers/gpu/drm/panfrost/panfrost_features.h
+++ b/drivers/gpu/drm/panfrost/panfrost_features.h
@@ -21,6 +21,7 @@ enum panfrost_hw_feature {
 	HW_FEATURE_TLS_HASHING,
 	HW_FEATURE_THREAD_GROUP_SPLIT,
 	HW_FEATURE_IDVS_GROUP_SIZE,
+	HW_FEATURE_CLEAN_ONLY_SAFE,
 	HW_FEATURE_3BIT_EXT_RW_L2_MMU_CONFIG,
 };
 
-- 
2.34.1

[PATCH 7/9] drm/panfrost: Don't set L2_MMU_CONFIG quirks

[alyssa.rosenzweig]

From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

L2_MMU_CONFIG is an implementation-defined register. Different Mali GPUs
define slightly different MAX_READS and MAX_WRITES fields, which
throttle outstanding reads and writes when set to non-zero values. When
left as zero, reads and writes are not throttled.

Both kbase and panfrost always zero these registers. Per discussion with
Steven Price, there are two reasons these quirks may be used:

1. Simulating slower memory subsystems. This use case is only of
   interest to system-on-chip designers; it is not relevant to mainline.

2. Working around broken memory subsystems. Hopefully we never see this
   case in mainline. If we do, we'll need to set this register based on
   an SoC-compatible, rather than generally matching on the GPU model.

To the best of our knowledge, these fields are zero at reset, so the
write is not necessary. Let's remove the write to aid porting to new
Mali GPUs, which have different layouts for the L2_MMU_CONFIG register.

Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
Suggested-by: Steven Price <steven.price@arm.com>
---
 drivers/gpu/drm/panfrost/panfrost_gpu.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
index 1c1e2017aa80..73e5774f01c1 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
@@ -127,18 +127,6 @@ static void panfrost_gpu_init_quirks(struct panfrost_device *pfdev)
 	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);
-
-	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)
-- 
2.34.1

[PATCH 8/9] drm/panfrost: Add Mali-G57 "Natt" support

[alyssa.rosenzweig]

From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

Add the features, issues, and GPU ID for Mali-G57, a first-generation
Valhall GPU. Other first- and second-generation Valhall GPUs should be
similar.

Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_features.h | 12 ++++++++++++
 drivers/gpu/drm/panfrost/panfrost_gpu.c      |  2 ++
 drivers/gpu/drm/panfrost/panfrost_issues.h   |  5 +++++
 3 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_features.h b/drivers/gpu/drm/panfrost/panfrost_features.h
index 1a8bdebc86a3..7ed0cd3ea2d4 100644
--- a/drivers/gpu/drm/panfrost/panfrost_features.h
+++ b/drivers/gpu/drm/panfrost/panfrost_features.h
@@ -106,6 +106,18 @@ enum panfrost_hw_feature {
 	BIT_ULL(HW_FEATURE_TLS_HASHING) | \
 	BIT_ULL(HW_FEATURE_3BIT_EXT_RW_L2_MMU_CONFIG))
 
+#define hw_features_g57 (\
+	BIT_ULL(HW_FEATURE_JOBCHAIN_DISAMBIGUATION) | \
+	BIT_ULL(HW_FEATURE_PWRON_DURING_PWROFF_TRANS) | \
+	BIT_ULL(HW_FEATURE_XAFFINITY) | \
+	BIT_ULL(HW_FEATURE_FLUSH_REDUCTION) | \
+	BIT_ULL(HW_FEATURE_PROTECTED_MODE) | \
+	BIT_ULL(HW_FEATURE_PROTECTED_DEBUG_MODE) | \
+	BIT_ULL(HW_FEATURE_COHERENCY_REG) | \
+	BIT_ULL(HW_FEATURE_AARCH64_MMU) | \
+	BIT_ULL(HW_FEATURE_IDVS_GROUP_SIZE) | \
+	BIT_ULL(HW_FEATURE_CLEAN_ONLY_SAFE))
+
 static inline bool panfrost_has_hw_feature(struct panfrost_device *pfdev,
 					   enum panfrost_hw_feature feat)
 {
diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
index 73e5774f01c1..08d657527099 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
@@ -201,6 +201,8 @@ static const struct panfrost_model gpu_models[] = {
 	GPU_MODEL(g52, 0x7002),
 	GPU_MODEL(g31, 0x7003,
 		GPU_REV(g31, 1, 0)),
+
+	GPU_MODEL(g57, 0x9001),
 };
 
 static void panfrost_gpu_init_features(struct panfrost_device *pfdev)
diff --git a/drivers/gpu/drm/panfrost/panfrost_issues.h b/drivers/gpu/drm/panfrost/panfrost_issues.h
index b8865fc9efce..1a0dc7f7f857 100644
--- a/drivers/gpu/drm/panfrost/panfrost_issues.h
+++ b/drivers/gpu/drm/panfrost/panfrost_issues.h
@@ -258,6 +258,11 @@ enum panfrost_hw_issue {
 
 #define hw_issues_g76 0
 
+#define hw_issues_g57 (\
+	BIT_ULL(HW_ISSUE_TTRX_2968_TTRX_3162) | \
+	BIT_ULL(HW_ISSUE_TTRX_3076) | \
+	BIT_ULL(HW_ISSUE_TTRX_3485))
+
 static inline bool panfrost_has_hw_issue(const struct panfrost_device *pfdev,
 					 enum panfrost_hw_issue issue)
 {
-- 
2.34.1

[PATCH 9/9] drm/panfrost: Handle arm,mali-valhall compatible

[alyssa.rosenzweig]

From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

The most important Valhall-specific quirks have been handled, so add the
Valhall compatible and probe.

Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 96bb5a465627..12977454af75 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -663,6 +663,7 @@ static const struct of_device_id dt_match[] = {
 	{ .compatible = "arm,mali-t860", .data = &default_data, },
 	{ .compatible = "arm,mali-t880", .data = &default_data, },
 	{ .compatible = "arm,mali-bifrost", .data = &default_data, },
+	{ .compatible = "arm,mali-valhall", .data = &default_data, },
 	{ .compatible = "mediatek,mt8183-mali", .data = &mediatek_mt8183_data },
 	{}
 };
-- 
2.34.1

Re: [PATCH 1/9] dt-bindings: Add arm,mali-valhall compatible

[Steven Price]

On 11/02/2022 20:27, alyssa.rosenzweig@collabora.com wrote:
> From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> 
> From the kernel's perspective, pre-CSF Valhall is more or less
> compatible with Bifrost, although they differ to userspace. Add a
> compatible for Valhall to the existing Bifrost bindings documentation.
> 
> Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Cc: devicetree@vger.kernel.org
> ---
>  Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> index 63a08f3f321d..48aeabd2ed68 100644
> --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> @@ -23,6 +23,7 @@ properties:
>            - rockchip,px30-mali
>            - rockchip,rk3568-mali
>        - const: arm,mali-bifrost # Mali Bifrost GPU model/revision is fully discoverable
> +      - const: arm,mali-valhall # Mali Valhall GPU model/revision is fully discoverable

It might be worth spelling out here that this is *pre-CSF* Valhall. I'm
pretty sure we're going to need different bindings for CSF GPUs.

Steve

>  
>    reg:
>      maxItems: 1

Re: [PATCH 1/9] dt-bindings: Add arm,mali-valhall compatible

[Steven Price]

On 11/02/2022 20:27, alyssa.rosenzweig@collabora.com wrote:
> From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> 
> From the kernel's perspective, pre-CSF Valhall is more or less
> compatible with Bifrost, although they differ to userspace. Add a
> compatible for Valhall to the existing Bifrost bindings documentation.
> 
> Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Cc: devicetree@vger.kernel.org
> ---
>  Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> index 63a08f3f321d..48aeabd2ed68 100644
> --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> @@ -23,6 +23,7 @@ properties:
>            - rockchip,px30-mali
>            - rockchip,rk3568-mali
>        - const: arm,mali-bifrost # Mali Bifrost GPU model/revision is fully discoverable
> +      - const: arm,mali-valhall # Mali Valhall GPU model/revision is fully discoverable

It might be worth spelling out here that this is *pre-CSF* Valhall. I'm
pretty sure we're going to need different bindings for CSF GPUs.

Steve

>  
>    reg:
>      maxItems: 1

Re: [PATCH 2/9] drm/panfrost: Handle HW_ISSUE_TTRX_2968_TTRX_3162

[Steven Price]

On 11/02/2022 20:27, alyssa.rosenzweig@collabora.com wrote:
> From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> 
> Add handling for the HW_ISSUE_TTRX_2968_TTRX_3162 quirk. Logic ported
> from kbase. kbase lists this workaround as used on Mali-G57.
> 
> Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/panfrost/panfrost_gpu.c    | 3 +++
>  drivers/gpu/drm/panfrost/panfrost_issues.h | 3 +++
>  drivers/gpu/drm/panfrost/panfrost_regs.h   | 1 +
>  3 files changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> index 50c8922694d7..1c1e2017aa80 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> @@ -108,6 +108,9 @@ static void panfrost_gpu_init_quirks(struct panfrost_device *pfdev)
>  			quirks |= SC_LS_ALLOW_ATTR_TYPES;
>  	}
>  
> +	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_TTRX_2968_TTRX_3162))
> +		quirks |= SC_VAR_ALGORITHM;
> +
>  	if (panfrost_has_hw_feature(pfdev, HW_FEATURE_TLS_HASHING))
>  		quirks |= SC_TLS_HASH_ENABLE;
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_issues.h b/drivers/gpu/drm/panfrost/panfrost_issues.h
> index 8e59d765bf19..3af7d723377e 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_issues.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_issues.h
> @@ -125,6 +125,9 @@ enum panfrost_hw_issue {
>  	 * kernel must fiddle with L2 caches to prevent data leakage */
>  	HW_ISSUE_TGOX_R1_1234,
>  
> +	/* Must set SC_VAR_ALGORITHM */
> +	HW_ISSUE_TTRX_2968_TTRX_3162,
> +
>  	HW_ISSUE_END
>  };
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_regs.h b/drivers/gpu/drm/panfrost/panfrost_regs.h
> index 16e776cc82ea..fa1e1af56e17 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_regs.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_regs.h
> @@ -195,6 +195,7 @@
>  #define SC_TLS_HASH_ENABLE		BIT(17)
>  #define SC_LS_ATTR_CHECK_DISABLE	BIT(18)
>  #define SC_ENABLE_TEXGRD_FLAGS		BIT(25)
> +#define SC_VAR_ALGORITHM		BIT(29)
>  /* End SHADER_CONFIG register */
>  
>  /* TILER_CONFIG register */

Re: [PATCH 3/9] drm/panfrost: Constify argument to has_hw_issue

[Steven Price]

On 11/02/2022 20:27, alyssa.rosenzweig@collabora.com wrote:
> From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> 
> Logically, this function is free of side effects, so any pointers it
> takes should be const. Needed to avoid a warning in the next patch.
> 
> Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/panfrost/panfrost_issues.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_issues.h b/drivers/gpu/drm/panfrost/panfrost_issues.h
> index 3af7d723377e..a66692663833 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_issues.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_issues.h
> @@ -251,7 +251,7 @@ enum panfrost_hw_issue {
>  
>  #define hw_issues_g76 0
>  
> -static inline bool panfrost_has_hw_issue(struct panfrost_device *pfdev,
> +static inline bool panfrost_has_hw_issue(const struct panfrost_device *pfdev,
>  					 enum panfrost_hw_issue issue)
>  {
>  	return test_bit(issue, pfdev->features.hw_issues);

Re: [PATCH 4/9] drm/panfrost: Handle HW_ISSUE_TTRX_3076

[Steven Price]

On 11/02/2022 20:27, alyssa.rosenzweig@collabora.com wrote:
> From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> 
> Some Valhall GPUs require resets when encountering bus faults due to
> occlusion query writes. Add the issue bit for this and handle it.
> 
> Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

Reviewed-by: Steven Price <steven.price@arm.com>
(although one nit below)

Just in case any one is wondering - these bus faults occur when
switching the GPU's MMU to unmapped - it's not a normal "bus fault" from
the external bus. This is triggered by an attempt to read unmapped
memory which is completed by the driver by switching the entire MMU to
unmapped.

> ---
>  drivers/gpu/drm/panfrost/panfrost_device.c | 9 +++++++--
>  drivers/gpu/drm/panfrost/panfrost_issues.h | 4 ++++
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 7f51a4682ccb..ee612303f076 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -11,6 +11,7 @@
>  #include "panfrost_device.h"
>  #include "panfrost_devfreq.h"
>  #include "panfrost_features.h"
> +#include "panfrost_issues.h"
>  #include "panfrost_gpu.h"
>  #include "panfrost_job.h"
>  #include "panfrost_mmu.h"
> @@ -380,9 +381,13 @@ const char *panfrost_exception_name(u32 exception_code)
>  bool panfrost_exception_needs_reset(const struct panfrost_device *pfdev,
>  				    u32 exception_code)
>  {
> -	/* Right now, none of the GPU we support need a reset, but this
> -	 * might change.
> +	/* If an occlusion query write causes a bus fault on affected GPUs,
> +	 * future fragment jobs may hang. Reset to workaround.
>  	 */
> +	if (exception_code == DRM_PANFROST_EXCEPTION_JOB_BUS_FAULT)
> +		return panfrost_has_hw_issue(pfdev, HW_ISSUE_TTRX_3076);
> +
> +	/* No other GPUs we support need a reset */
>  	return false;
>  }
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_issues.h b/drivers/gpu/drm/panfrost/panfrost_issues.h
> index a66692663833..058f6a4c8435 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_issues.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_issues.h
> @@ -128,6 +128,10 @@ enum panfrost_hw_issue {
>  	/* Must set SC_VAR_ALGORITHM */
>  	HW_ISSUE_TTRX_2968_TTRX_3162,
>  
> +	/* Bus fault from occlusion query write may cause future fragment jobs
> +	 * to hang */

NIT: Kernel comment style has the "/*" and "*/" on lines by themselves
for multi-line comments. checkpatch will complain!

> +	HW_ISSUE_TTRX_3076,
> +
>  	HW_ISSUE_END
>  };
>  

Re: [PATCH 5/9] drm/panfrost: Add HW_ISSUE_TTRX_3485 quirk

[Steven Price]

On 11/02/2022 20:27, alyssa.rosenzweig@collabora.com wrote:
> From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> 
> TTRX_3485 requires the infamous "dummy job" workaround. I have this
> workaround implemented in a local branch, but I have not yet hit a case
> that requires it so I cannot test whether the implementation is correct.
> In the mean time, add the quirk bit so we can document which platforms
> may need it in the future.

This one is hideous ;) Although to me this isn't the 'infamous' one as
it's not the earliest example of a dummy job.

However... I believe as Panfrost currently stands this is probably not
very possible to hit. It requires a job to be stopped (soft or hard) at
a critical point during submission - which at the moment Panfrost
basically never does (the exception is if you close the fd immediately
while a job is in progress). And of course the timing has to be 'just
right' to hit the bug.

That said I think we should probably add pre-emption support sometime at
which point this could become an issue.

> Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/panfrost/panfrost_issues.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_issues.h b/drivers/gpu/drm/panfrost/panfrost_issues.h
> index 058f6a4c8435..b8865fc9efce 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_issues.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_issues.h
> @@ -132,6 +132,9 @@ enum panfrost_hw_issue {
>  	 * to hang */
>  	HW_ISSUE_TTRX_3076,
>  
> +	/* Must issue a dummy job before starting real work to prevent hangs */
> +	HW_ISSUE_TTRX_3485,
> +
>  	HW_ISSUE_END
>  };
>  

Re: [PATCH 6/9] drm/panfrost: Add "clean only safe" feature bit

[Steven Price]

On 11/02/2022 20:27, alyssa.rosenzweig@collabora.com wrote:
> From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> 
> Add the HW_FEATURE_CLEAN_ONLY_SAFE bit based on kbase. When I actually
> tried to port the logic from kbase, trivial jobs raised Data Invalid
> Faults, so this may depend on other coherency details. It's still useful
> to have the bit to record the feature bit when adding new models.
> 
> Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

Reviewed-by: Steven Price <steven.price@arm.com>

Sadly I don't have the hardware to try this out on, but it should be a
simple case of the below (untested):

----8<----
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 908d79520853..602e51c4966e 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -212,9 +212,13 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
         * start */
        cfg |= JS_CONFIG_THREAD_PRI(8) |
                JS_CONFIG_START_FLUSH_CLEAN_INVALIDATE |
-               JS_CONFIG_END_FLUSH_CLEAN_INVALIDATE |
                panfrost_get_job_chain_flag(job);
 
+       if (panfrost_has_hw_feature(pfdev, HW_FEATURE_CLEAN_ONLY_SAFE))
+               cfg |= JS_CONFIG_END_FLUSH_CLEAN;
+       else
+               cfg |= JS_CONFIG_END_FLUSH_CLEAN_INVALIDATE;
+
        if (panfrost_has_hw_feature(pfdev, HW_FEATURE_FLUSH_REDUCTION))
                cfg |= JS_CONFIG_ENABLE_FLUSH_REDUCTION;
 
----8<----

Steve

> ---
>  drivers/gpu/drm/panfrost/panfrost_features.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_features.h b/drivers/gpu/drm/panfrost/panfrost_features.h
> index 36fadcf9634e..1a8bdebc86a3 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_features.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_features.h
> @@ -21,6 +21,7 @@ enum panfrost_hw_feature {
>  	HW_FEATURE_TLS_HASHING,
>  	HW_FEATURE_THREAD_GROUP_SPLIT,
>  	HW_FEATURE_IDVS_GROUP_SIZE,
> +	HW_FEATURE_CLEAN_ONLY_SAFE,
>  	HW_FEATURE_3BIT_EXT_RW_L2_MMU_CONFIG,
>  };
>  

Re: [PATCH 7/9] drm/panfrost: Don't set L2_MMU_CONFIG quirks

[Steven Price]

On 11/02/2022 20:27, alyssa.rosenzweig@collabora.com wrote:
> From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> 
> L2_MMU_CONFIG is an implementation-defined register. Different Mali GPUs
> define slightly different MAX_READS and MAX_WRITES fields, which
> throttle outstanding reads and writes when set to non-zero values. When
> left as zero, reads and writes are not throttled.
> 
> Both kbase and panfrost always zero these registers. Per discussion with
> Steven Price, there are two reasons these quirks may be used:
> 
> 1. Simulating slower memory subsystems. This use case is only of
>    interest to system-on-chip designers; it is not relevant to mainline.
> 
> 2. Working around broken memory subsystems. Hopefully we never see this
>    case in mainline. If we do, we'll need to set this register based on
>    an SoC-compatible, rather than generally matching on the GPU model.
> 
> To the best of our knowledge, these fields are zero at reset, so the
> write is not necessary. Let's remove the write to aid porting to new
> Mali GPUs, which have different layouts for the L2_MMU_CONFIG register.
> 
> Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Suggested-by: Steven Price <steven.price@arm.com>

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/panfrost/panfrost_gpu.c | 12 ------------
>  1 file changed, 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> index 1c1e2017aa80..73e5774f01c1 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> @@ -127,18 +127,6 @@ static void panfrost_gpu_init_quirks(struct panfrost_device *pfdev)
>  	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);
> -
> -	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)

Re: [PATCH 8/9] drm/panfrost: Add Mali-G57 "Natt" support

[Steven Price]

On 11/02/2022 20:27, alyssa.rosenzweig@collabora.com wrote:
> From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> 
> Add the features, issues, and GPU ID for Mali-G57, a first-generation
> Valhall GPU. Other first- and second-generation Valhall GPUs should be
> similar.
> 
> Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_features.h | 12 ++++++++++++
>  drivers/gpu/drm/panfrost/panfrost_gpu.c      |  2 ++
>  drivers/gpu/drm/panfrost/panfrost_issues.h   |  5 +++++
>  3 files changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_features.h b/drivers/gpu/drm/panfrost/panfrost_features.h
> index 1a8bdebc86a3..7ed0cd3ea2d4 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_features.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_features.h
> @@ -106,6 +106,18 @@ enum panfrost_hw_feature {
>  	BIT_ULL(HW_FEATURE_TLS_HASHING) | \
>  	BIT_ULL(HW_FEATURE_3BIT_EXT_RW_L2_MMU_CONFIG))
>  
> +#define hw_features_g57 (\
> +	BIT_ULL(HW_FEATURE_JOBCHAIN_DISAMBIGUATION) | \
> +	BIT_ULL(HW_FEATURE_PWRON_DURING_PWROFF_TRANS) | \
> +	BIT_ULL(HW_FEATURE_XAFFINITY) | \
> +	BIT_ULL(HW_FEATURE_FLUSH_REDUCTION) | \
> +	BIT_ULL(HW_FEATURE_PROTECTED_MODE) | \
> +	BIT_ULL(HW_FEATURE_PROTECTED_DEBUG_MODE) | \
> +	BIT_ULL(HW_FEATURE_COHERENCY_REG) | \
> +	BIT_ULL(HW_FEATURE_AARCH64_MMU) | \
> +	BIT_ULL(HW_FEATURE_IDVS_GROUP_SIZE) | \
> +	BIT_ULL(HW_FEATURE_CLEAN_ONLY_SAFE))
> +
>  static inline bool panfrost_has_hw_feature(struct panfrost_device *pfdev,
>  					   enum panfrost_hw_feature feat)
>  {
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> index 73e5774f01c1..08d657527099 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> @@ -201,6 +201,8 @@ static const struct panfrost_model gpu_models[] = {
>  	GPU_MODEL(g52, 0x7002),
>  	GPU_MODEL(g31, 0x7003,
>  		GPU_REV(g31, 1, 0)),
> +
> +	GPU_MODEL(g57, 0x9001),
>  };
>  
>  static void panfrost_gpu_init_features(struct panfrost_device *pfdev)
> diff --git a/drivers/gpu/drm/panfrost/panfrost_issues.h b/drivers/gpu/drm/panfrost/panfrost_issues.h
> index b8865fc9efce..1a0dc7f7f857 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_issues.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_issues.h
> @@ -258,6 +258,11 @@ enum panfrost_hw_issue {
>  
>  #define hw_issues_g76 0
>  
> +#define hw_issues_g57 (\
> +	BIT_ULL(HW_ISSUE_TTRX_2968_TTRX_3162) | \
> +	BIT_ULL(HW_ISSUE_TTRX_3076) | \
> +	BIT_ULL(HW_ISSUE_TTRX_3485))

Do you know whether you have an r0p0 or an r0p1 Natt? Only the r0p0 has
the 3485 issue, and we might be lucky and it's the r0p1 that's "in the
wild".

It would be good to annotate these lists with the hardware revisions
when there is a difference.

Steve

> +
>  static inline bool panfrost_has_hw_issue(const struct panfrost_device *pfdev,
>  					 enum panfrost_hw_issue issue)
>  {

Re: [PATCH 9/9] drm/panfrost: Handle arm,mali-valhall compatible

[Steven Price]

On 11/02/2022 20:27, alyssa.rosenzweig@collabora.com wrote:
> From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> 
> The most important Valhall-specific quirks have been handled, so add the
> Valhall compatible and probe.
> 
> Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 96bb5a465627..12977454af75 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -663,6 +663,7 @@ static const struct of_device_id dt_match[] = {
>  	{ .compatible = "arm,mali-t860", .data = &default_data, },
>  	{ .compatible = "arm,mali-t880", .data = &default_data, },
>  	{ .compatible = "arm,mali-bifrost", .data = &default_data, },
> +	{ .compatible = "arm,mali-valhall", .data = &default_data, },
>  	{ .compatible = "mediatek,mt8183-mali", .data = &mediatek_mt8183_data },
>  	{}
>  };

Re: [PATCH 1/9] dt-bindings: Add arm,mali-valhall compatible

[Alyssa Rosenzweig]

> > diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> > index 63a08f3f321d..48aeabd2ed68 100644
> > --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> > +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> > @@ -23,6 +23,7 @@ properties:
> >            - rockchip,px30-mali
> >            - rockchip,rk3568-mali
> >        - const: arm,mali-bifrost # Mali Bifrost GPU model/revision is fully discoverable
> > +      - const: arm,mali-valhall # Mali Valhall GPU model/revision is fully discoverable
> 
> It might be worth spelling out here that this is *pre-CSF* Valhall. I'm
> pretty sure we're going to need different bindings for CSF GPUs.

Yes, agreed, will make a note for v2.

Re: [PATCH 1/9] dt-bindings: Add arm,mali-valhall compatible

[Alyssa Rosenzweig]

> > diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> > index 63a08f3f321d..48aeabd2ed68 100644
> > --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> > +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> > @@ -23,6 +23,7 @@ properties:
> >            - rockchip,px30-mali
> >            - rockchip,rk3568-mali
> >        - const: arm,mali-bifrost # Mali Bifrost GPU model/revision is fully discoverable
> > +      - const: arm,mali-valhall # Mali Valhall GPU model/revision is fully discoverable
> 
> It might be worth spelling out here that this is *pre-CSF* Valhall. I'm
> pretty sure we're going to need different bindings for CSF GPUs.

Yes, agreed, will make a note for v2.

Re: [PATCH 6/9] drm/panfrost: Add "clean only safe" feature bit

[Alyssa Rosenzweig]

> > Add the HW_FEATURE_CLEAN_ONLY_SAFE bit based on kbase. When I actually
> > tried to port the logic from kbase, trivial jobs raised Data Invalid
> > Faults, so this may depend on other coherency details. It's still useful
> > to have the bit to record the feature bit when adding new models.
> > 
> > Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> 
> Reviewed-by: Steven Price <steven.price@arm.com>
> 
> Sadly I don't have the hardware to try this out on, but it should be a
> simple case of the below (untested):
> 
> ----8<----
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 908d79520853..602e51c4966e 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -212,9 +212,13 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
>          * start */
>         cfg |= JS_CONFIG_THREAD_PRI(8) |
>                 JS_CONFIG_START_FLUSH_CLEAN_INVALIDATE |
> -               JS_CONFIG_END_FLUSH_CLEAN_INVALIDATE |
>                 panfrost_get_job_chain_flag(job);
>  
> +       if (panfrost_has_hw_feature(pfdev, HW_FEATURE_CLEAN_ONLY_SAFE))
> +               cfg |= JS_CONFIG_END_FLUSH_CLEAN;
> +       else
> +               cfg |= JS_CONFIG_END_FLUSH_CLEAN_INVALIDATE;
> +
>         if (panfrost_has_hw_feature(pfdev, HW_FEATURE_FLUSH_REDUCTION))
>                 cfg |= JS_CONFIG_ENABLE_FLUSH_REDUCTION;

Yes, this is the patch I typed out... causes DATA_INVALID_FAULTs for me
with Mesa. Which makes me wonder if userspace needs to respect some
extra rules for this to be safe.

Re: [PATCH 8/9] drm/panfrost: Add Mali-G57 "Natt" support

[Alyssa Rosenzweig]

> > index b8865fc9efce..1a0dc7f7f857 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_issues.h
> > +++ b/drivers/gpu/drm/panfrost/panfrost_issues.h
> > @@ -258,6 +258,11 @@ enum panfrost_hw_issue {
> >  
> >  #define hw_issues_g76 0
> >  
> > +#define hw_issues_g57 (\
> > +	BIT_ULL(HW_ISSUE_TTRX_2968_TTRX_3162) | \
> > +	BIT_ULL(HW_ISSUE_TTRX_3076) | \
> > +	BIT_ULL(HW_ISSUE_TTRX_3485))
> 
> Do you know whether you have an r0p0 or an r0p1 Natt? Only the r0p0 has
> the 3485 issue, and we might be lucky and it's the r0p1 that's "in the
> wild".

Sadly, I believe I have an r0p0. I don't know if future spins of the
same SoC would be bumped up, but I'm skeptical.

> It would be good to annotate these lists with the hardware revisions
> when there is a difference.

Sure.

Re: [PATCH 4/9] drm/panfrost: Handle HW_ISSUE_TTRX_3076

[Alyssa Rosenzweig]

On Mon, Feb 14, 2022 at 04:23:18PM +0000, Steven Price wrote:
> On 11/02/2022 20:27, alyssa.rosenzweig@collabora.com wrote:
> > From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> > 
> > Some Valhall GPUs require resets when encountering bus faults due to
> > occlusion query writes. Add the issue bit for this and handle it.
> > 
> > Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> 
> Reviewed-by: Steven Price <steven.price@arm.com>
> (although one nit below)
> 
> Just in case any one is wondering - these bus faults occur when
> switching the GPU's MMU to unmapped - it's not a normal "bus fault" from
> the external bus. This is triggered by an attempt to read unmapped
> memory which is completed by the driver by switching the entire MMU to
> unmapped.

Ouch, that's subtle.

> > diff --git a/drivers/gpu/drm/panfrost/panfrost_issues.h b/drivers/gpu/drm/panfrost/panfrost_issues.h
> > index a66692663833..058f6a4c8435 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_issues.h
> > +++ b/drivers/gpu/drm/panfrost/panfrost_issues.h
> > @@ -128,6 +128,10 @@ enum panfrost_hw_issue {
> >  	/* Must set SC_VAR_ALGORITHM */
> >  	HW_ISSUE_TTRX_2968_TTRX_3162,
> >  
> > +	/* Bus fault from occlusion query write may cause future fragment jobs
> > +	 * to hang */
> 
> NIT: Kernel comment style has the "/*" and "*/" on lines by themselves
> for multi-line comments. checkpatch will complain!

Yes, I am aware (and checkpatch did complain). The existing multi-line
comments in that file do not have the extra lines. Consistency within
the file seemed like the lesser evil. If you think it's better to
appease checkpatch, I can reformat for v2.

(I can also throw in a patch fixing the rest of that file's multiline
comments but that seems a bit extra.)

Re: [PATCH 5/9] drm/panfrost: Add HW_ISSUE_TTRX_3485 quirk

[Alyssa Rosenzweig]

> > TTRX_3485 requires the infamous "dummy job" workaround. I have this
> > workaround implemented in a local branch, but I have not yet hit a case
> > that requires it so I cannot test whether the implementation is correct.
> > In the mean time, add the quirk bit so we can document which platforms
> > may need it in the future.
> 
> This one is hideous ;) Although to me this isn't the 'infamous' one as
> it's not the earliest example of a dummy job.

Terrifying. I guess we narrowly avoided the 'replay' workaround which
was far worse than this one...

> However... I believe as Panfrost currently stands this is probably not
> very possible to hit. It requires a job to be stopped (soft or hard) at
> a critical point during submission - which at the moment Panfrost
> basically never does (the exception is if you close the fd immediately
> while a job is in progress). And of course the timing has to be 'just
> right' to hit the bug.

OK, that's good to know. Still "should" be fixed but that definitely
lowers the priority of it. Frankly the multithreading bugs we have on
the CPU side would hang the machine sooner...

Re: [PATCH 1/9] dt-bindings: Add arm,mali-valhall compatible

[Daniel Stone]

On Mon, 14 Feb 2022 at 16:23, Steven Price <steven.price@arm.com> wrote:
> On 11/02/2022 20:27, alyssa.rosenzweig@collabora.com wrote:
> > From the kernel's perspective, pre-CSF Valhall is more or less
> > compatible with Bifrost, although they differ to userspace. Add a
> > compatible for Valhall to the existing Bifrost bindings documentation.
> >
> > diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> > index 63a08f3f321d..48aeabd2ed68 100644
> > --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> > +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> > @@ -23,6 +23,7 @@ properties:
> >            - rockchip,px30-mali
> >            - rockchip,rk3568-mali
> >        - const: arm,mali-bifrost # Mali Bifrost GPU model/revision is fully discoverable
> > +      - const: arm,mali-valhall # Mali Valhall GPU model/revision is fully discoverable
>
> It might be worth spelling out here that this is *pre-CSF* Valhall. I'm
> pretty sure we're going to need different bindings for CSF GPUs.

Good point - maybe either make it arm,mali-valhall-jm then?

Cheers,
Daniel

Re: [PATCH 1/9] dt-bindings: Add arm,mali-valhall compatible

[Daniel Stone]

On Mon, 14 Feb 2022 at 16:23, Steven Price <steven.price@arm.com> wrote:
> On 11/02/2022 20:27, alyssa.rosenzweig@collabora.com wrote:
> > From the kernel's perspective, pre-CSF Valhall is more or less
> > compatible with Bifrost, although they differ to userspace. Add a
> > compatible for Valhall to the existing Bifrost bindings documentation.
> >
> > diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> > index 63a08f3f321d..48aeabd2ed68 100644
> > --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> > +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> > @@ -23,6 +23,7 @@ properties:
> >            - rockchip,px30-mali
> >            - rockchip,rk3568-mali
> >        - const: arm,mali-bifrost # Mali Bifrost GPU model/revision is fully discoverable
> > +      - const: arm,mali-valhall # Mali Valhall GPU model/revision is fully discoverable
>
> It might be worth spelling out here that this is *pre-CSF* Valhall. I'm
> pretty sure we're going to need different bindings for CSF GPUs.

Good point - maybe either make it arm,mali-valhall-jm then?

Cheers,
Daniel

Re: [PATCH 1/9] dt-bindings: Add arm,mali-valhall compatible

[Robin Murphy]

On 2022-02-11 20:27, alyssa.rosenzweig@collabora.com wrote:
> From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> 
>  From the kernel's perspective, pre-CSF Valhall is more or less
> compatible with Bifrost, although they differ to userspace. Add a
> compatible for Valhall to the existing Bifrost bindings documentation.
> 
> Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Cc: devicetree@vger.kernel.org
> ---
>   Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> index 63a08f3f321d..48aeabd2ed68 100644
> --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> @@ -23,6 +23,7 @@ properties:
>             - rockchip,px30-mali
>             - rockchip,rk3568-mali
>         - const: arm,mali-bifrost # Mali Bifrost GPU model/revision is fully discoverable
> +      - const: arm,mali-valhall # Mali Valhall GPU model/revision is fully discoverable

This requires all existing Bifrost users to add the Valhall compatible 
as well - I don't think that's what you want. From what we tossed about 
on IRC the other week, I'd imagined something more in the shape of:

   compatible:
     oneOf:
       - items:
           - enum:
               - vendor,soc-mali
               - ...
           - const: arm,mali-bifrost
       - items:
           - enum:
               - vendor,soc-mali
               - ...
           - const: arm,mali-valhall
           - const: arm,mali-bifrost #or not, depending on 
forward-compatibility preferences

Cheers,
Robin.

Re: [PATCH 1/9] dt-bindings: Add arm,mali-valhall compatible

[Robin Murphy]

On 2022-02-11 20:27, alyssa.rosenzweig@collabora.com wrote:
> From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> 
>  From the kernel's perspective, pre-CSF Valhall is more or less
> compatible with Bifrost, although they differ to userspace. Add a
> compatible for Valhall to the existing Bifrost bindings documentation.
> 
> Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Cc: devicetree@vger.kernel.org
> ---
>   Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> index 63a08f3f321d..48aeabd2ed68 100644
> --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> @@ -23,6 +23,7 @@ properties:
>             - rockchip,px30-mali
>             - rockchip,rk3568-mali
>         - const: arm,mali-bifrost # Mali Bifrost GPU model/revision is fully discoverable
> +      - const: arm,mali-valhall # Mali Valhall GPU model/revision is fully discoverable

This requires all existing Bifrost users to add the Valhall compatible 
as well - I don't think that's what you want. From what we tossed about 
on IRC the other week, I'd imagined something more in the shape of:

   compatible:
     oneOf:
       - items:
           - enum:
               - vendor,soc-mali
               - ...
           - const: arm,mali-bifrost
       - items:
           - enum:
               - vendor,soc-mali
               - ...
           - const: arm,mali-valhall
           - const: arm,mali-bifrost #or not, depending on 
forward-compatibility preferences

Cheers,
Robin.

Re: [PATCH 1/9] dt-bindings: Add arm,mali-valhall compatible

[Rob Herring]

On Fri, 11 Feb 2022 15:27:20 -0500, alyssa.rosenzweig@collabora.com wrote:
> From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> 
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.example.dt.yaml: gpu@ffe40000: compatible: ['amlogic,meson-g12a-mali', 'arm,mali-bifrost'] is too short
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1591823

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 1/9] dt-bindings: Add arm,mali-valhall compatible

[Rob Herring]

On Fri, 11 Feb 2022 15:27:20 -0500, alyssa.rosenzweig@collabora.com wrote:
> From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> 
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.example.dt.yaml: gpu@ffe40000: compatible: ['amlogic,meson-g12a-mali', 'arm,mali-bifrost'] is too short
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1591823

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 4/9] drm/panfrost: Handle HW_ISSUE_TTRX_3076

[Steven Price]

On 14/02/2022 17:06, Alyssa Rosenzweig wrote:
> On Mon, Feb 14, 2022 at 04:23:18PM +0000, Steven Price wrote:
>> On 11/02/2022 20:27, alyssa.rosenzweig@collabora.com wrote:
>>> From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
>>>
>>> Some Valhall GPUs require resets when encountering bus faults due to
>>> occlusion query writes. Add the issue bit for this and handle it.
>>>
>>> Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
>>
>> Reviewed-by: Steven Price <steven.price@arm.com>
>> (although one nit below)
>>
>> Just in case any one is wondering - these bus faults occur when
>> switching the GPU's MMU to unmapped - it's not a normal "bus fault" from
>> the external bus. This is triggered by an attempt to read unmapped
>> memory which is completed by the driver by switching the entire MMU to
>> unmapped.
> 
> Ouch, that's subtle.
> 
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_issues.h b/drivers/gpu/drm/panfrost/panfrost_issues.h
>>> index a66692663833..058f6a4c8435 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_issues.h
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_issues.h
>>> @@ -128,6 +128,10 @@ enum panfrost_hw_issue {
>>>  	/* Must set SC_VAR_ALGORITHM */
>>>  	HW_ISSUE_TTRX_2968_TTRX_3162,
>>>  
>>> +	/* Bus fault from occlusion query write may cause future fragment jobs
>>> +	 * to hang */
>>
>> NIT: Kernel comment style has the "/*" and "*/" on lines by themselves
>> for multi-line comments. checkpatch will complain!
> 
> Yes, I am aware (and checkpatch did complain). The existing multi-line
> comments in that file do not have the extra lines. Consistency within
> the file seemed like the lesser evil. If you think it's better to
> appease checkpatch, I can reformat for v2.
> 
> (I can also throw in a patch fixing the rest of that file's multiline
> comments but that seems a bit extra.)

Nah! I've never been very keen on that style rule myself, but I usually
try to keep checkpatch happy when working on the kernel. Lets just
follow the rest of the file for now.

Thanks,

Steve

Re: [PATCH 6/9] drm/panfrost: Add "clean only safe" feature bit

[Steven Price]

On 14/02/2022 17:01, Alyssa Rosenzweig wrote:
>>> Add the HW_FEATURE_CLEAN_ONLY_SAFE bit based on kbase. When I actually
>>> tried to port the logic from kbase, trivial jobs raised Data Invalid
>>> Faults, so this may depend on other coherency details. It's still useful
>>> to have the bit to record the feature bit when adding new models.
>>>
>>> Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
>>
>> Reviewed-by: Steven Price <steven.price@arm.com>
>>
>> Sadly I don't have the hardware to try this out on, but it should be a
>> simple case of the below (untested):
>>
>> ----8<----
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
>> index 908d79520853..602e51c4966e 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
>> @@ -212,9 +212,13 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
>>          * start */
>>         cfg |= JS_CONFIG_THREAD_PRI(8) |
>>                 JS_CONFIG_START_FLUSH_CLEAN_INVALIDATE |
>> -               JS_CONFIG_END_FLUSH_CLEAN_INVALIDATE |
>>                 panfrost_get_job_chain_flag(job);
>>  
>> +       if (panfrost_has_hw_feature(pfdev, HW_FEATURE_CLEAN_ONLY_SAFE))
>> +               cfg |= JS_CONFIG_END_FLUSH_CLEAN;
>> +       else
>> +               cfg |= JS_CONFIG_END_FLUSH_CLEAN_INVALIDATE;
>> +
>>         if (panfrost_has_hw_feature(pfdev, HW_FEATURE_FLUSH_REDUCTION))
>>                 cfg |= JS_CONFIG_ENABLE_FLUSH_REDUCTION;
> 
> Yes, this is the patch I typed out... causes DATA_INVALID_FAULTs for me
> with Mesa. Which makes me wonder if userspace needs to respect some
> extra rules for this to be safe.

Odd - the invalidate at the end of the job shouldn't be needed to read
the job descriptors from userspace only the one at the beginning.

However I'm wondering if there's something fishy happening with the
flush reduction. That allows skipping the cache maintenance at the
beginning of a job if there has already been one for other reasons. But
I can't immediately see any difference in the way kbase handles this.

Steve