[PATCH v2 0/2] drm/etnaviv: GC300 fixes

[PATCH v2 0/2] drm/etnaviv: GC300 fixes

[Doug Brown]

This series contains a few special cases for supporting the GC300
properly. These were found in the drivers in the vivante_kernel_drivers
repository. These changes were tested on a PXA168 with GC300 revision
0x2201 (date 0x20080814, time 0x12051100), which already has an existing
special case in the driver to modify the revision to 0x1051.

Changes since v1:
- Move power register address remapping to new gpu_read_power and
  gpu_write_power accessors instead of modifying gpu_read and gpu_write.

Doug Brown (2):
  drm/etnaviv: add missing quirks for GC300
  drm/etnaviv: fix power register offset on GC300

 drivers/gpu/drm/etnaviv/etnaviv_dump.c |  9 ++++++--
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c  | 31 ++++++++++++++++----------
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h  | 21 +++++++++++++++++
 3 files changed, 47 insertions(+), 14 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] drm/etnaviv: add missing quirks for GC300

[Doug Brown]

The GC300's features register doesn't specify that a 2D pipe is
available, and like the GC600, its idle register reports zero bits where
modules aren't present.

Signed-off-by: Doug Brown <doug@schmorgal.com>
Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com>
---
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 37018bc55810..f667e7906d1f 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -416,6 +416,12 @@ static void etnaviv_hw_identify(struct etnaviv_gpu *gpu)
 	if (gpu->identity.model == chipModel_GC700)
 		gpu->identity.features &= ~chipFeatures_FAST_CLEAR;
 
+	/* These models/revisions don't have the 2D pipe bit */
+	if ((gpu->identity.model == chipModel_GC500 &&
+	     gpu->identity.revision <= 2) ||
+	    gpu->identity.model == chipModel_GC300)
+		gpu->identity.features |= chipFeatures_PIPE_2D;
+
 	if ((gpu->identity.model == chipModel_GC500 &&
 	     gpu->identity.revision < 2) ||
 	    (gpu->identity.model == chipModel_GC300 &&
@@ -449,8 +455,9 @@ static void etnaviv_hw_identify(struct etnaviv_gpu *gpu)
 				gpu_read(gpu, VIVS_HI_CHIP_MINOR_FEATURE_5);
 	}
 
-	/* GC600 idle register reports zero bits where modules aren't present */
-	if (gpu->identity.model == chipModel_GC600)
+	/* GC600/300 idle register reports zero bits where modules aren't present */
+	if (gpu->identity.model == chipModel_GC600 ||
+	    gpu->identity.model == chipModel_GC300)
 		gpu->idle_mask = VIVS_HI_IDLE_STATE_TX |
 				 VIVS_HI_IDLE_STATE_RA |
 				 VIVS_HI_IDLE_STATE_SE |
-- 
2.25.1

[PATCH v2 2/2] drm/etnaviv: fix power register offset on GC300

[Doug Brown]

Older GC300 revisions have their power registers at an offset of 0x200
rather than 0x100. Add new gpu_read_power and gpu_write_power functions
to encapsulate accesses to the power addresses and fix the addresses.

Signed-off-by: Doug Brown <doug@schmorgal.com>
---
 drivers/gpu/drm/etnaviv/etnaviv_dump.c |  9 +++++++--
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c  | 20 ++++++++++----------
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h  | 21 +++++++++++++++++++++
 3 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_dump.c b/drivers/gpu/drm/etnaviv/etnaviv_dump.c
index f418e0b75772..df07a3c8f16a 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_dump.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_dump.c
@@ -83,10 +83,15 @@ static void etnaviv_core_dump_registers(struct core_dump_iterator *iter,
 {
 	struct etnaviv_dump_registers *reg = iter->data;
 	unsigned int i;
+	u32 addr;
 
 	for (i = 0; i < ARRAY_SIZE(etnaviv_dump_registers); i++, reg++) {
-		reg->reg = cpu_to_le32(etnaviv_dump_registers[i]);
-		reg->value = cpu_to_le32(gpu_read(gpu, etnaviv_dump_registers[i]));
+		addr = etnaviv_dump_registers[i];
+		if (addr >= VIVS_PM_POWER_CONTROLS &&
+		    addr <= VIVS_PM_PULSE_EATER)
+			addr = gpu_fix_power_address(gpu, addr);
+		reg->reg = cpu_to_le32(addr);
+		reg->value = cpu_to_le32(gpu_read(gpu, addr));
 	}
 
 	etnaviv_core_dump_header(iter, ETDUMP_BUF_REG, reg);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index f667e7906d1f..f6d08f556cf4 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -590,7 +590,7 @@ static void etnaviv_gpu_enable_mlcg(struct etnaviv_gpu *gpu)
 	u32 pmc, ppc;
 
 	/* enable clock gating */
-	ppc = gpu_read(gpu, VIVS_PM_POWER_CONTROLS);
+	ppc = gpu_read_power(gpu, VIVS_PM_POWER_CONTROLS);
 	ppc |= VIVS_PM_POWER_CONTROLS_ENABLE_MODULE_CLOCK_GATING;
 
 	/* Disable stall module clock gating for 4.3.0.1 and 4.3.0.2 revs */
@@ -598,9 +598,9 @@ static void etnaviv_gpu_enable_mlcg(struct etnaviv_gpu *gpu)
 	    gpu->identity.revision == 0x4302)
 		ppc |= VIVS_PM_POWER_CONTROLS_DISABLE_STALL_MODULE_CLOCK_GATING;
 
-	gpu_write(gpu, VIVS_PM_POWER_CONTROLS, ppc);
+	gpu_write_power(gpu, VIVS_PM_POWER_CONTROLS, ppc);
 
-	pmc = gpu_read(gpu, VIVS_PM_MODULE_CONTROLS);
+	pmc = gpu_read_power(gpu, VIVS_PM_MODULE_CONTROLS);
 
 	/* Disable PA clock gating for GC400+ without bugfix except for GC420 */
 	if (gpu->identity.model >= chipModel_GC400 &&
@@ -635,7 +635,7 @@ static void etnaviv_gpu_enable_mlcg(struct etnaviv_gpu *gpu)
 	pmc |= VIVS_PM_MODULE_CONTROLS_DISABLE_MODULE_CLOCK_GATING_RA_HZ;
 	pmc |= VIVS_PM_MODULE_CONTROLS_DISABLE_MODULE_CLOCK_GATING_RA_EZ;
 
-	gpu_write(gpu, VIVS_PM_MODULE_CONTROLS, pmc);
+	gpu_write_power(gpu, VIVS_PM_MODULE_CONTROLS, pmc);
 }
 
 void etnaviv_gpu_start_fe(struct etnaviv_gpu *gpu, u32 address, u16 prefetch)
@@ -695,11 +695,11 @@ static void etnaviv_gpu_setup_pulse_eater(struct etnaviv_gpu *gpu)
 	    (gpu->identity.features & chipFeatures_PIPE_3D))
 	{
 		/* Performance fix: disable internal DFS */
-		pulse_eater = gpu_read(gpu, VIVS_PM_PULSE_EATER);
+		pulse_eater = gpu_read_power(gpu, VIVS_PM_PULSE_EATER);
 		pulse_eater |= BIT(18);
 	}
 
-	gpu_write(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
+	gpu_write_power(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
 }
 
 static void etnaviv_gpu_hw_init(struct etnaviv_gpu *gpu)
@@ -1301,9 +1301,9 @@ static void sync_point_perfmon_sample_pre(struct etnaviv_gpu *gpu,
 	u32 val;
 
 	/* disable clock gating */
-	val = gpu_read(gpu, VIVS_PM_POWER_CONTROLS);
+	val = gpu_read_power(gpu, VIVS_PM_POWER_CONTROLS);
 	val &= ~VIVS_PM_POWER_CONTROLS_ENABLE_MODULE_CLOCK_GATING;
-	gpu_write(gpu, VIVS_PM_POWER_CONTROLS, val);
+	gpu_write_power(gpu, VIVS_PM_POWER_CONTROLS, val);
 
 	/* enable debug register */
 	val = gpu_read(gpu, VIVS_HI_CLOCK_CONTROL);
@@ -1334,9 +1334,9 @@ static void sync_point_perfmon_sample_post(struct etnaviv_gpu *gpu,
 	gpu_write(gpu, VIVS_HI_CLOCK_CONTROL, val);
 
 	/* enable clock gating */
-	val = gpu_read(gpu, VIVS_PM_POWER_CONTROLS);
+	val = gpu_read_power(gpu, VIVS_PM_POWER_CONTROLS);
 	val |= VIVS_PM_POWER_CONTROLS_ENABLE_MODULE_CLOCK_GATING;
-	gpu_write(gpu, VIVS_PM_POWER_CONTROLS, val);
+	gpu_write_power(gpu, VIVS_PM_POWER_CONTROLS, val);
 }
 
 
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
index 85eddd492774..d4d139e88942 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
@@ -10,6 +10,8 @@
 #include "etnaviv_gem.h"
 #include "etnaviv_mmu.h"
 #include "etnaviv_drv.h"
+#include "common.xml.h"
+#include "state_hi.xml.h"
 
 struct etnaviv_gem_submit;
 struct etnaviv_vram_mapping;
@@ -159,6 +161,25 @@ static inline u32 gpu_read(struct etnaviv_gpu *gpu, u32 reg)
 	return readl(gpu->mmio + reg);
 }
 
+static inline u32 gpu_fix_power_address(struct etnaviv_gpu *gpu, u32 reg)
+{
+	/* Power registers in GC300 < 2.0 are offset by 0x100 */
+	if (gpu->identity.model == chipModel_GC300 &&
+	    gpu->identity.revision < 0x2000)
+		reg += 0x100;
+	return reg;
+}
+
+static inline void gpu_write_power(struct etnaviv_gpu *gpu, u32 reg, u32 data)
+{
+	writel(data, gpu->mmio + gpu_fix_power_address(gpu, reg));
+}
+
+static inline u32 gpu_read_power(struct etnaviv_gpu *gpu, u32 reg)
+{
+	return readl(gpu->mmio + gpu_fix_power_address(gpu, reg));
+}
+
 int etnaviv_gpu_get_param(struct etnaviv_gpu *gpu, u32 param, u64 *value);
 
 int etnaviv_gpu_init(struct etnaviv_gpu *gpu);
-- 
2.25.1

Re: [PATCH v2 2/2] drm/etnaviv: fix power register offset on GC300

[Lucas Stach]

Am Dienstag, dem 06.09.2022 um 19:47 -0700 schrieb Doug Brown:
> Older GC300 revisions have their power registers at an offset of 0x200
> rather than 0x100. Add new gpu_read_power and gpu_write_power functions
> to encapsulate accesses to the power addresses and fix the addresses.
> 
> Signed-off-by: Doug Brown <doug@schmorgal.com>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_dump.c |  9 +++++++--
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.c  | 20 ++++++++++----------
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.h  | 21 +++++++++++++++++++++
>  3 files changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_dump.c b/drivers/gpu/drm/etnaviv/etnaviv_dump.c
> index f418e0b75772..df07a3c8f16a 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_dump.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_dump.c
> @@ -83,10 +83,15 @@ static void etnaviv_core_dump_registers(struct core_dump_iterator *iter,
>  {
>  	struct etnaviv_dump_registers *reg = iter->data;
>  	unsigned int i;
> +	u32 addr;
>  
>  	for (i = 0; i < ARRAY_SIZE(etnaviv_dump_registers); i++, reg++) {
> -		reg->reg = cpu_to_le32(etnaviv_dump_registers[i]);
> -		reg->value = cpu_to_le32(gpu_read(gpu, etnaviv_dump_registers[i]));
> +		addr = etnaviv_dump_registers[i];
> +		if (addr >= VIVS_PM_POWER_CONTROLS &&
> +		    addr <= VIVS_PM_PULSE_EATER)
> +			addr = gpu_fix_power_address(gpu, addr);
> +		reg->reg = cpu_to_le32(addr);

As the dumpdecoder tool would then need to reverse this address offset,
I think it would be preferable to keep writing the canonical (not fixed
up) register address into the dump. That way only the kernel needs to
know about this special offset on GC300.

> +		reg->value = cpu_to_le32(gpu_read(gpu, addr));
>  	}
>  
>  	etnaviv_core_dump_header(iter, ETDUMP_BUF_REG, reg);
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index f667e7906d1f..f6d08f556cf4 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -590,7 +590,7 @@ static void etnaviv_gpu_enable_mlcg(struct etnaviv_gpu *gpu)
>  	u32 pmc, ppc;
>  
>  	/* enable clock gating */
> -	ppc = gpu_read(gpu, VIVS_PM_POWER_CONTROLS);
> +	ppc = gpu_read_power(gpu, VIVS_PM_POWER_CONTROLS);
>  	ppc |= VIVS_PM_POWER_CONTROLS_ENABLE_MODULE_CLOCK_GATING;
>  
>  	/* Disable stall module clock gating for 4.3.0.1 and 4.3.0.2 revs */
> @@ -598,9 +598,9 @@ static void etnaviv_gpu_enable_mlcg(struct etnaviv_gpu *gpu)
>  	    gpu->identity.revision == 0x4302)
>  		ppc |= VIVS_PM_POWER_CONTROLS_DISABLE_STALL_MODULE_CLOCK_GATING;
>  
> -	gpu_write(gpu, VIVS_PM_POWER_CONTROLS, ppc);
> +	gpu_write_power(gpu, VIVS_PM_POWER_CONTROLS, ppc);
>  
> -	pmc = gpu_read(gpu, VIVS_PM_MODULE_CONTROLS);
> +	pmc = gpu_read_power(gpu, VIVS_PM_MODULE_CONTROLS);
>  
>  	/* Disable PA clock gating for GC400+ without bugfix except for GC420 */
>  	if (gpu->identity.model >= chipModel_GC400 &&
> @@ -635,7 +635,7 @@ static void etnaviv_gpu_enable_mlcg(struct etnaviv_gpu *gpu)
>  	pmc |= VIVS_PM_MODULE_CONTROLS_DISABLE_MODULE_CLOCK_GATING_RA_HZ;
>  	pmc |= VIVS_PM_MODULE_CONTROLS_DISABLE_MODULE_CLOCK_GATING_RA_EZ;
>  
> -	gpu_write(gpu, VIVS_PM_MODULE_CONTROLS, pmc);
> +	gpu_write_power(gpu, VIVS_PM_MODULE_CONTROLS, pmc);
>  }
>  
>  void etnaviv_gpu_start_fe(struct etnaviv_gpu *gpu, u32 address, u16 prefetch)
> @@ -695,11 +695,11 @@ static void etnaviv_gpu_setup_pulse_eater(struct etnaviv_gpu *gpu)
>  	    (gpu->identity.features & chipFeatures_PIPE_3D))
>  	{
>  		/* Performance fix: disable internal DFS */
> -		pulse_eater = gpu_read(gpu, VIVS_PM_PULSE_EATER);
> +		pulse_eater = gpu_read_power(gpu, VIVS_PM_PULSE_EATER);
>  		pulse_eater |= BIT(18);
>  	}
>  
> -	gpu_write(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
> +	gpu_write_power(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
>  }
>  
>  static void etnaviv_gpu_hw_init(struct etnaviv_gpu *gpu)
> @@ -1301,9 +1301,9 @@ static void sync_point_perfmon_sample_pre(struct etnaviv_gpu *gpu,
>  	u32 val;
>  
>  	/* disable clock gating */
> -	val = gpu_read(gpu, VIVS_PM_POWER_CONTROLS);
> +	val = gpu_read_power(gpu, VIVS_PM_POWER_CONTROLS);
>  	val &= ~VIVS_PM_POWER_CONTROLS_ENABLE_MODULE_CLOCK_GATING;
> -	gpu_write(gpu, VIVS_PM_POWER_CONTROLS, val);
> +	gpu_write_power(gpu, VIVS_PM_POWER_CONTROLS, val);
>  
>  	/* enable debug register */
>  	val = gpu_read(gpu, VIVS_HI_CLOCK_CONTROL);
> @@ -1334,9 +1334,9 @@ static void sync_point_perfmon_sample_post(struct etnaviv_gpu *gpu,
>  	gpu_write(gpu, VIVS_HI_CLOCK_CONTROL, val);
>  
>  	/* enable clock gating */
> -	val = gpu_read(gpu, VIVS_PM_POWER_CONTROLS);
> +	val = gpu_read_power(gpu, VIVS_PM_POWER_CONTROLS);
>  	val |= VIVS_PM_POWER_CONTROLS_ENABLE_MODULE_CLOCK_GATING;
> -	gpu_write(gpu, VIVS_PM_POWER_CONTROLS, val);
> +	gpu_write_power(gpu, VIVS_PM_POWER_CONTROLS, val);
>  }
>  
>  
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> index 85eddd492774..d4d139e88942 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> @@ -10,6 +10,8 @@
>  #include "etnaviv_gem.h"
>  #include "etnaviv_mmu.h"
>  #include "etnaviv_drv.h"
> +#include "common.xml.h"
> +#include "state_hi.xml.h"

I think you don't need the state_hi.xml.h include anymore, as you are
not checking for a register range in this version of the patch.

>  
>  struct etnaviv_gem_submit;
>  struct etnaviv_vram_mapping;
> @@ -159,6 +161,25 @@ static inline u32 gpu_read(struct etnaviv_gpu *gpu, u32 reg)
>  	return readl(gpu->mmio + reg);
>  }
>  
> +static inline u32 gpu_fix_power_address(struct etnaviv_gpu *gpu, u32 reg)
> +{
> +	/* Power registers in GC300 < 2.0 are offset by 0x100 */
> +	if (gpu->identity.model == chipModel_GC300 &&
> +	    gpu->identity.revision < 0x2000)
> +		reg += 0x100;

Add a newline here before the return, please.

Regards,
Lucas

> +	return reg;
> +}
> +
> +static inline void gpu_write_power(struct etnaviv_gpu *gpu, u32 reg, u32 data)
> +{
> +	writel(data, gpu->mmio + gpu_fix_power_address(gpu, reg));
> +}
> +
> +static inline u32 gpu_read_power(struct etnaviv_gpu *gpu, u32 reg)
> +{
> +	return readl(gpu->mmio + gpu_fix_power_address(gpu, reg));
> +}
> +
>  int etnaviv_gpu_get_param(struct etnaviv_gpu *gpu, u32 param, u64 *value);
>  
>  int etnaviv_gpu_init(struct etnaviv_gpu *gpu);

Re: [PATCH v2 2/2] drm/etnaviv: fix power register offset on GC300

[Doug Brown]

Hi Lucas,

On 9/9/2022 1:48 AM, Lucas Stach wrote:

>> @@ -83,10 +83,15 @@ static void etnaviv_core_dump_registers(struct core_dump_iterator *iter,
>>   {
>>   	struct etnaviv_dump_registers *reg = iter->data;
>>   	unsigned int i;
>> +	u32 addr;
>>   
>>   	for (i = 0; i < ARRAY_SIZE(etnaviv_dump_registers); i++, reg++) {
>> -		reg->reg = cpu_to_le32(etnaviv_dump_registers[i]);
>> -		reg->value = cpu_to_le32(gpu_read(gpu, etnaviv_dump_registers[i]));
>> +		addr = etnaviv_dump_registers[i];
>> +		if (addr >= VIVS_PM_POWER_CONTROLS &&
>> +		    addr <= VIVS_PM_PULSE_EATER)
>> +			addr = gpu_fix_power_address(gpu, addr);
>> +		reg->reg = cpu_to_le32(addr);
> 
> As the dumpdecoder tool would then need to reverse this address offset,
> I think it would be preferable to keep writing the canonical (not fixed
> up) register address into the dump. That way only the kernel needs to
> know about this special offset on GC300.

Ahh, I had no idea about how it worked on that side of things. Makes
complete sense. Thanks for reviewing! Will submit V3 with this and
everything else you mentioned fixed.

Doug