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

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

I was a little unsure of the "correct" way to do the fixup to the power
register offsets. I implemented the change in gpu_write and gpu_read,
but if someone has a better idea I would be happy to do it differently.

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

 drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 11 +++++++++--
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 16 ++++++++++++++--
 2 files changed, 23 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH 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>
---
 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

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

[Christian Gmeiner]

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


-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info/privacypolicy

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

Signed-off-by: Doug Brown <doug@schmorgal.com>
---
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
index 85eddd492774..b375612df862 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;
@@ -149,14 +151,24 @@ struct etnaviv_gpu {
 	unsigned long base_rate_shader;
 };
 
+static inline u32 gpu_fix_reg_address(struct etnaviv_gpu *gpu, u32 reg)
+{
+	/* Power registers in GC300 < 2.0 are offset by 0x100 */
+	if (reg >= VIVS_PM_POWER_CONTROLS && reg <= VIVS_PM_PULSE_EATER &&
+	    gpu->identity.model == chipModel_GC300 &&
+	    gpu->identity.revision < 0x2000)
+		reg += 0x100;
+	return reg;
+}
+
 static inline void gpu_write(struct etnaviv_gpu *gpu, u32 reg, u32 data)
 {
-	writel(data, gpu->mmio + reg);
+	writel(data, gpu->mmio + gpu_fix_reg_address(gpu, reg));
 }
 
 static inline u32 gpu_read(struct etnaviv_gpu *gpu, u32 reg)
 {
-	return readl(gpu->mmio + reg);
+	return readl(gpu->mmio + gpu_fix_reg_address(gpu, reg));
 }
 
 int etnaviv_gpu_get_param(struct etnaviv_gpu *gpu, u32 param, u64 *value);
-- 
2.25.1

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

[Christian Gmeiner]

Hi

>
> Older GC300 revisions have their power registers at an offset of 0x200
> rather than 0x100.
>
> Signed-off-by: Doug Brown <doug@schmorgal.com>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> index 85eddd492774..b375612df862 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;
> @@ -149,14 +151,24 @@ struct etnaviv_gpu {
>         unsigned long base_rate_shader;
>  };
>
> +static inline u32 gpu_fix_reg_address(struct etnaviv_gpu *gpu, u32 reg)
> +{
> +       /* Power registers in GC300 < 2.0 are offset by 0x100 */
> +       if (reg >= VIVS_PM_POWER_CONTROLS && reg <= VIVS_PM_PULSE_EATER &&
> +           gpu->identity.model == chipModel_GC300 &&
> +           gpu->identity.revision < 0x2000)
> +               reg += 0x100;
> +       return reg;
> +}
> +
>  static inline void gpu_write(struct etnaviv_gpu *gpu, u32 reg, u32 data)
>  {
> -       writel(data, gpu->mmio + reg);
> +       writel(data, gpu->mmio + gpu_fix_reg_address(gpu, reg));
>  }
>
>  static inline u32 gpu_read(struct etnaviv_gpu *gpu, u32 reg)
>  {
> -       return readl(gpu->mmio + reg);
> +       return readl(gpu->mmio + gpu_fix_reg_address(gpu, reg));
>  }
>

I had a quick look at what vivantes kernel driver did. It uses a per
gpu instance variable powerBaseAddress
that gets set accordingly. I am not sure if I really like the
gpu_fix_reg_address(..) idea, as it gets called on every
register read and write. For me I see two other possible solutions:

1) Add two seperate helpers ala gpu_read_power() and gpu_write_power()
where we do the if beast.
2) Add a power register offset variable to etnaviv_gpu and explicitly
use it on for reads and writes - like the Vivante driver does.

But that's just my personal opinion. Can't wait to hear what Lucas thinks.

-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info/privacypolicy

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

[Doug Brown]

Hi Christian,

On 9/3/2022 4:49 AM, Christian Gmeiner wrote:

> I had a quick look at what vivantes kernel driver did. It uses a per
> gpu instance variable powerBaseAddress
> that gets set accordingly. I am not sure if I really like the
> gpu_fix_reg_address(..) idea, as it gets called on every
> register read and write. For me I see two other possible solutions:
> 
> 1) Add two seperate helpers ala gpu_read_power() and gpu_write_power()
> where we do the if beast.
> 2) Add a power register offset variable to etnaviv_gpu and explicitly
> use it on for reads and writes - like the Vivante driver does.
> 
> But that's just my personal opinion. Can't wait to hear what Lucas thinks.
> 

Thanks for reviewing so fast! I honestly agree. It felt kind of dirty
modifying gpu_write and gpu_read. The reason I went for it is I was
thinking that in most cases the compiler is going to optimize the ugly
"if" out.

The two solutions listed above both sound good. They would need a
special case in etnaviv_core_dump_registers, but that's probably much
less nasty than modifying gpu_read and gpu_write. Any preferences from
everyone on which of the other two options I should do? The
gpu_read_power and gpu_write_power approach sounds pretty clean to me.

Also, dropping David Airlie from the chain -- his email address bounced
on my original patch messages.

Doug

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

[Lucas Stach]

Hi Doug,

Am Montag, dem 05.09.2022 um 14:05 -0700 schrieb Doug Brown:
> Hi Christian,
> 
> On 9/3/2022 4:49 AM, Christian Gmeiner wrote:
> 
> > I had a quick look at what vivantes kernel driver did. It uses a per
> > gpu instance variable powerBaseAddress
> > that gets set accordingly. I am not sure if I really like the
> > gpu_fix_reg_address(..) idea, as it gets called on every
> > register read and write. For me I see two other possible solutions:
> > 
> > 1) Add two seperate helpers ala gpu_read_power() and gpu_write_power()
> > where we do the if beast.
> > 2) Add a power register offset variable to etnaviv_gpu and explicitly
> > use it on for reads and writes - like the Vivante driver does.
> > 
> > But that's just my personal opinion. Can't wait to hear what Lucas thinks.
> > 
> 
> Thanks for reviewing so fast! I honestly agree. It felt kind of dirty
> modifying gpu_write and gpu_read. The reason I went for it is I was
> thinking that in most cases the compiler is going to optimize the ugly
> "if" out.
> 
> The two solutions listed above both sound good. They would need a
> special case in etnaviv_core_dump_registers, but that's probably much
> less nasty than modifying gpu_read and gpu_write. Any preferences from
> everyone on which of the other two options I should do? The
> gpu_read_power and gpu_write_power approach sounds pretty clean to me.

Yes, please add a specialized wrapper for the power register accesses.

Regards,
Lucas