diff mbox series

drm/i915/icl: Fix read of memory frequency

Message ID 20211008205855.36778-1-jose.souza@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/icl: Fix read of memory frequency | expand

Commit Message

Souza, Jose Oct. 8, 2021, 8:58 p.m. UTC 
All display 9 and display 10 platforms has only 4 bits for the memory
frequency but display 11 platforms it changes to 8 bits.

Display 9 platforms has another register in bits 7:4 that prevents us
to have a single mask.
Also adding new mask with the current name in CRWebViewer, not
sure why current mask is named like this.

Fixes: f8112cb9574b ("drm/i915/gen11+: Only load DRAM information from pcode")
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h   | 1 +
 drivers/gpu/drm/i915/intel_dram.c | 7 +++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

Comments

Souza, Jose Oct. 8, 2021, 9:47 p.m. UTC | #1 
On Fri, 2021-10-08 at 13:58 -0700, José Roberto de Souza wrote:
> All display 9 and display 10 platforms has only 4 bits for the memory
> frequency but display 11 platforms it changes to 8 bits.
> 
> Display 9 platforms has another register in bits 7:4 that prevents us
> to have a single mask.
> Also adding new mask with the current name in CRWebViewer, not
> sure why current mask is named like this.
> 
> Fixes: f8112cb9574b ("drm/i915/gen11+: Only load DRAM information from pcode")

Ops hash should be: 5d0c938ec9cc
Will fix in the next version or when applying.

> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h   | 1 +
>  drivers/gpu/drm/i915/intel_dram.c | 7 +++++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index a897f4abea0c3..041f7dc9e0d94 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -11148,6 +11148,7 @@ enum skl_power_gate {
>  #define SKL_MEMORY_FREQ_MULTIPLIER_HZ		266666666
>  #define SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5E04)
>  #define  SKL_REQ_DATA_MASK			(0xF << 0)
> +#define  ICL_FREQ_MASK				(0xFF << 0)
>  #define  DG1_GEAR_TYPE				REG_BIT(16)
>  
>  #define SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5000)
> diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c
> index 30a0cab5eff46..558589b1202d6 100644
> --- a/drivers/gpu/drm/i915/intel_dram.c
> +++ b/drivers/gpu/drm/i915/intel_dram.c
> @@ -257,8 +257,11 @@ skl_get_dram_info(struct drm_i915_private *i915)
>  
>  	val = intel_uncore_read(&i915->uncore,
>  				SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU);
> -	mem_freq_khz = DIV_ROUND_UP((val & SKL_REQ_DATA_MASK) *
> -				    SKL_MEMORY_FREQ_MULTIPLIER_HZ, 1000);
> +	if (DISPLAY_VER(i915) == 11)
> +		val &= ICL_FREQ_MASK;
> +	else
> +		val &= SKL_REQ_DATA_MASK;
> +	mem_freq_khz = DIV_ROUND_UP(val * SKL_MEMORY_FREQ_MULTIPLIER_HZ, 1000);
>  
>  	if (dram_info->num_channels * mem_freq_khz == 0) {
>  		drm_info(&i915->drm,
Matt Roper Oct. 12, 2021, 9:20 p.m. UTC | #2 
On Fri, Oct 08, 2021 at 01:58:55PM -0700, José Roberto de Souza wrote:
> All display 9 and display 10 platforms has only 4 bits for the memory
> frequency but display 11 platforms it changes to 8 bits.
> 
> Display 9 platforms has another register in bits 7:4 that prevents us
> to have a single mask.
> Also adding new mask with the current name in CRWebViewer, not
> sure why current mask is named like this.
> 
> Fixes: f8112cb9574b ("drm/i915/gen11+: Only load DRAM information from pcode")
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h   | 1 +
>  drivers/gpu/drm/i915/intel_dram.c | 7 +++++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index a897f4abea0c3..041f7dc9e0d94 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -11148,6 +11148,7 @@ enum skl_power_gate {
>  #define SKL_MEMORY_FREQ_MULTIPLIER_HZ		266666666
>  #define SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5E04)
>  #define  SKL_REQ_DATA_MASK			(0xF << 0)
> +#define  ICL_FREQ_MASK				(0xFF << 0)

We might as well take this opportunity to switch over to REG_GENMASK
notation while we're here.

>  #define  DG1_GEAR_TYPE				REG_BIT(16)
>  
>  #define SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5000)
> diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c
> index 30a0cab5eff46..558589b1202d6 100644
> --- a/drivers/gpu/drm/i915/intel_dram.c
> +++ b/drivers/gpu/drm/i915/intel_dram.c
> @@ -257,8 +257,11 @@ skl_get_dram_info(struct drm_i915_private *i915)
>  
>  	val = intel_uncore_read(&i915->uncore,
>  				SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU);
> -	mem_freq_khz = DIV_ROUND_UP((val & SKL_REQ_DATA_MASK) *
> -				    SKL_MEMORY_FREQ_MULTIPLIER_HZ, 1000);
> +	if (DISPLAY_VER(i915) == 11)
> +		val &= ICL_FREQ_MASK;
> +	else
> +		val &= SKL_REQ_DATA_MASK;
> +	mem_freq_khz = DIV_ROUND_UP(val * SKL_MEMORY_FREQ_MULTIPLIER_HZ, 1000);

I'm not sure SKL_MEMORY_FREQ_MULTIPLIER_HZ is correct anymore either.
If I'm reading the register description correctly, it appears the value
is now given in units of 133.33 MHz instead of the old 266.66.


Matt

>  
>  	if (dram_info->num_channels * mem_freq_khz == 0) {
>  		drm_info(&i915->drm,
> -- 
> 2.33.0
>
Souza, Jose Oct. 12, 2021, 9:23 p.m. UTC | #3 
On Tue, 2021-10-12 at 14:20 -0700, Matt Roper wrote:
> On Fri, Oct 08, 2021 at 01:58:55PM -0700, José Roberto de Souza wrote:
> > All display 9 and display 10 platforms has only 4 bits for the memory
> > frequency but display 11 platforms it changes to 8 bits.
> > 
> > Display 9 platforms has another register in bits 7:4 that prevents us
> > to have a single mask.
> > Also adding new mask with the current name in CRWebViewer, not
> > sure why current mask is named like this.
> > 
> > Fixes: f8112cb9574b ("drm/i915/gen11+: Only load DRAM information from pcode")
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h   | 1 +
> >  drivers/gpu/drm/i915/intel_dram.c | 7 +++++--
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index a897f4abea0c3..041f7dc9e0d94 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -11148,6 +11148,7 @@ enum skl_power_gate {
> >  #define SKL_MEMORY_FREQ_MULTIPLIER_HZ		266666666
> >  #define SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5E04)
> >  #define  SKL_REQ_DATA_MASK			(0xF << 0)
> > +#define  ICL_FREQ_MASK				(0xFF << 0)
> 
> We might as well take this opportunity to switch over to REG_GENMASK
> notation while we're here.

Will do.

> 
> >  #define  DG1_GEAR_TYPE				REG_BIT(16)
> >  
> >  #define SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5000)
> > diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c
> > index 30a0cab5eff46..558589b1202d6 100644
> > --- a/drivers/gpu/drm/i915/intel_dram.c
> > +++ b/drivers/gpu/drm/i915/intel_dram.c
> > @@ -257,8 +257,11 @@ skl_get_dram_info(struct drm_i915_private *i915)
> >  
> >  	val = intel_uncore_read(&i915->uncore,
> >  				SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU);
> > -	mem_freq_khz = DIV_ROUND_UP((val & SKL_REQ_DATA_MASK) *
> > -				    SKL_MEMORY_FREQ_MULTIPLIER_HZ, 1000);
> > +	if (DISPLAY_VER(i915) == 11)
> > +		val &= ICL_FREQ_MASK;
> > +	else
> > +		val &= SKL_REQ_DATA_MASK;
> > +	mem_freq_khz = DIV_ROUND_UP(val * SKL_MEMORY_FREQ_MULTIPLIER_HZ, 1000);
> 
> I'm not sure SKL_MEMORY_FREQ_MULTIPLIER_HZ is correct anymore either.
> If I'm reading the register description correctly, it appears the value
> is now given in units of 133.33 MHz instead of the old 266.66.

Thought about that but as the calculated memory frequency here is not used for anything besides check if is not zero, I left as is.

> 
> 
> Matt
> 
> >  
> >  	if (dram_info->num_channels * mem_freq_khz == 0) {
> >  		drm_info(&i915->drm,
> > -- 
> > 2.33.0
> > 
>
Matt Roper Oct. 12, 2021, 9:40 p.m. UTC | #4 
On Tue, Oct 12, 2021 at 02:23:27PM -0700, Souza, Jose wrote:
> On Tue, 2021-10-12 at 14:20 -0700, Matt Roper wrote:
> > On Fri, Oct 08, 2021 at 01:58:55PM -0700, José Roberto de Souza wrote:
> > > All display 9 and display 10 platforms has only 4 bits for the memory
> > > frequency but display 11 platforms it changes to 8 bits.
> > > 
> > > Display 9 platforms has another register in bits 7:4 that prevents us
> > > to have a single mask.
> > > Also adding new mask with the current name in CRWebViewer, not
> > > sure why current mask is named like this.
> > > 
> > > Fixes: f8112cb9574b ("drm/i915/gen11+: Only load DRAM information from pcode")
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h   | 1 +
> > >  drivers/gpu/drm/i915/intel_dram.c | 7 +++++--
> > >  2 files changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index a897f4abea0c3..041f7dc9e0d94 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -11148,6 +11148,7 @@ enum skl_power_gate {
> > >  #define SKL_MEMORY_FREQ_MULTIPLIER_HZ		266666666
> > >  #define SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5E04)
> > >  #define  SKL_REQ_DATA_MASK			(0xF << 0)
> > > +#define  ICL_FREQ_MASK				(0xFF << 0)
> > 
> > We might as well take this opportunity to switch over to REG_GENMASK
> > notation while we're here.
> 
> Will do.
> 
> > 
> > >  #define  DG1_GEAR_TYPE				REG_BIT(16)
> > >  
> > >  #define SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5000)
> > > diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c
> > > index 30a0cab5eff46..558589b1202d6 100644
> > > --- a/drivers/gpu/drm/i915/intel_dram.c
> > > +++ b/drivers/gpu/drm/i915/intel_dram.c
> > > @@ -257,8 +257,11 @@ skl_get_dram_info(struct drm_i915_private *i915)
> > >  
> > >  	val = intel_uncore_read(&i915->uncore,
> > >  				SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU);
> > > -	mem_freq_khz = DIV_ROUND_UP((val & SKL_REQ_DATA_MASK) *
> > > -				    SKL_MEMORY_FREQ_MULTIPLIER_HZ, 1000);
> > > +	if (DISPLAY_VER(i915) == 11)
> > > +		val &= ICL_FREQ_MASK;
> > > +	else
> > > +		val &= SKL_REQ_DATA_MASK;
> > > +	mem_freq_khz = DIV_ROUND_UP(val * SKL_MEMORY_FREQ_MULTIPLIER_HZ, 1000);
> > 
> > I'm not sure SKL_MEMORY_FREQ_MULTIPLIER_HZ is correct anymore either.
> > If I'm reading the register description correctly, it appears the value
> > is now given in units of 133.33 MHz instead of the old 266.66.
> 
> Thought about that but as the calculated memory frequency here is not used for anything besides check if is not zero, I left as is.

Hmm, good point.  Although in that case do we really need to read this
register at all?  It seems like after

        commit f0b29707baa9e6f3d7b90090fcce62d2f1023fa1
        Author:     José Roberto de Souza <jose.souza@intel.com>
        AuthorDate: Thu Jan 28 08:43:10 2021 -0800
        Commit:     José Roberto de Souza <jose.souza@intel.com>
        CommitDate: Fri Jan 29 05:50:48 2021 -0800

            drm/i915: Nuke not needed members of dram_info

we're not calculating bandwidth_kbps, so checking if the register is
valid doesn't really gain us anything and could just be removed?  A
simple check for

        if (dram_info->num_channels == 0) {
                ...error...
        }

might be sufficient?


Matt

> 
> > 
> > 
> > Matt
> > 
> > >  
> > >  	if (dram_info->num_channels * mem_freq_khz == 0) {
> > >  		drm_info(&i915->drm,
> > > -- 
> > > 2.33.0
> > > 
> > 
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index a897f4abea0c3..041f7dc9e0d94 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -11148,6 +11148,7 @@  enum skl_power_gate {
 #define SKL_MEMORY_FREQ_MULTIPLIER_HZ		266666666
 #define SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5E04)
 #define  SKL_REQ_DATA_MASK			(0xF << 0)
+#define  ICL_FREQ_MASK				(0xFF << 0)
 #define  DG1_GEAR_TYPE				REG_BIT(16)
 
 #define SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5000)
diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c
index 30a0cab5eff46..558589b1202d6 100644
--- a/drivers/gpu/drm/i915/intel_dram.c
+++ b/drivers/gpu/drm/i915/intel_dram.c
@@ -257,8 +257,11 @@  skl_get_dram_info(struct drm_i915_private *i915)
 
 	val = intel_uncore_read(&i915->uncore,
 				SKL_MC_BIOS_DATA_0_0_0_MCHBAR_PCU);
-	mem_freq_khz = DIV_ROUND_UP((val & SKL_REQ_DATA_MASK) *
-				    SKL_MEMORY_FREQ_MULTIPLIER_HZ, 1000);
+	if (DISPLAY_VER(i915) == 11)
+		val &= ICL_FREQ_MASK;
+	else
+		val &= SKL_REQ_DATA_MASK;
+	mem_freq_khz = DIV_ROUND_UP(val * SKL_MEMORY_FREQ_MULTIPLIER_HZ, 1000);
 
 	if (dram_info->num_channels * mem_freq_khz == 0) {
 		drm_info(&i915->drm,