subject:"\[Intel\-gfx\] \[PATCH 1\/1\] drm\/i915\: MCHBAR memory info registers are moved since GEN 12."

Re: [Intel-gfx] [PATCH 1/1] drm/i915: MCHBAR memory info registers are moved since GEN 12.

2020-02-14 Thread Caz Yokoyama

On Thu, 2020-02-13 at 14:37 -0800, Matt Roper wrote:
> On Tue, Feb 11, 2020 at 10:11:42AM -0800, Caz Yokoyama wrote:
> > From: "Yokoyama, Caz" 
> > 
> > MAD_INTER_CHANNEL_0_0_0_MCHBAR:
> > code nameoffset   DRAM_DDR_TYPE
> > SKL  0x5000   1:0 DDR4/DDR3/LPDDR3
> > TGL  0x6048/0x6248/0xd800 2:0
> > DDR4/DDR3/LPDDR3/LPDDR4
> > ICL  0x5000/0x6048/0x48   2:0
> > DDR4/DDR3/LPDDR3/LPDDR4
> > EHL  0x5000/0x60482:0
> > DDR4/DDR3/LPDDR3/LPDDR4
> > CNL  0x5000   2:0
> > DDR4/DDR3/LPDDR3/LPDDR4
> > 
> > MAD_DIMM_CH0/1_0_0_0_MCHBAR:
> > code name  offset CH0/1
> > SKL0x500c/0x5010
> > TGL0x6054/0x6058
> > EHL0x500c/0x5010
> > ICL0x500c/0x5010
ICL0x500c/0x5010/0x6054/0x6058
as I editted in jira.

> > The bit definition of MAD_DIMM_CH0/1_0_0_0_MCHBAR is same between
> > CNL and TGL.
> 
> Have you actually sanity checked the register addresses here on real
> hardware?  I see the same offsets in the doc as what you've put here,
I expect CI does, i.e. CI builds the driver with my patch and run tests
for all platform. I only run the test on adls simics.
Are you asking me to run tests on several platform? I have i5-8600K as
a test machine.
 
> but since this is a different register database than we get most of
> our
> gfx-specific register details from, it would still be good to double
> check that you do indeed get sensible values back when reading from
> these addresses before we merge the patch.  Especially since the
> database indicates that some of these registers are present at
> multiple
> offsets.
So you are asking me to read value of the following registers on ICL
for example and find whether they have the same value, correct?

0x5000 and 0x6048
0x500c and 0x6054
0x5010 and 0x6058

> 
> > 
> > P_CR_MC_BIOS_REQ_0_0_0 is same on BXT and GLK in terms of its
> > address and
> > bit definition.
> > BXT_D_CR_DRP0_DUNIT accesses offset 0x1000, 0x1200, 0x1400, 0x1600.
> > Its register name is RAM_ACCESS_DATA1. There is no difference
> > between
> > BXT and GLK in terms of its address and bit definition.
> > 
> > Cc: Ville Syrjälä 
> > Cc: Matt Roper 
> > Signed-off-by: Yokoyama, Caz 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 15 ---
> >  drivers/gpu/drm/i915/i915_reg.h |  6 ++
> >  2 files changed, 18 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index 516536234e97..b9418583e503 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -806,12 +806,18 @@ skl_dram_get_channels_info(struct
> > drm_i915_private *dev_priv)
> > u32 val;
> > int ret;
> >  
> > -   val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
> > +   if (INTEL_GEN(dev_priv) >= 12)
> > +   val = I915_READ(TGL_MAD_DIMM_CH0_0_0_0_MCHBAR);
> > +   else
> > +   val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
> > ret = skl_dram_get_channel_info(dev_priv, &ch0, 0, val);
> > if (ret == 0)
> > dram_info->num_channels++;
> >  
> > -   val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
> > +   if (INTEL_GEN(dev_priv) >= 12)
> > +   val = I915_READ(TGL_MAD_DIMM_CH1_0_0_0_MCHBAR);
> > +   else
> > +   val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
> > ret = skl_dram_get_channel_info(dev_priv, &ch1, 1, val);
> > if (ret == 0)
> > dram_info->num_channels++;
> > @@ -852,7 +858,10 @@ skl_get_dram_type(struct drm_i915_private
> > *dev_priv)
> >  {
> > u32 val;
> >  
> > -   val = I915_READ(SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN);
> > +   if (INTEL_GEN(dev_priv) >= 12)
> > +   val = I915_READ(TGL_MAD_INTER_CHANNEL_0_0_0_MCHBAR);
> > +   else
> > +   val =
> > I915_READ(SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN);
> >  
> > switch (val & SKL_DRAM_DDR_TYPE_MASK) {
> > case SKL_DRAM_DDR_TYPE_DDR3:
> 
> I'm not sure if it might be cleaner to create regs structures for
> each
> platform to centralize the platform-selection logic and avoid all the
> if/else in the code.  E.g.,
> 
> struct i915_mchbar_regs {
> i915_reg_t mad_inter_channel;
> i915_reg_t mad_dimm_ch0;
> i915_reg_t mad_dimm_ch1;
> };
> 
> static const struct i915_mchbar_regs skl_mchbar_regs = {
> .mad_inter_channel =
> SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN,
> .mad_dimm_ch0 = SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN,
> .mad_dimm_ch1 = SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN,
> };
> 
> and then use the appropriate regs structure in each of these
> functions
> so that you can just do
> 
> val = I915_READ(regs->mad_inter_channel);
> 
> and not have to upda

Re: [Intel-gfx] [PATCH 1/1] drm/i915: MCHBAR memory info registers are moved since GEN 12.

2020-02-13 Thread Matt Roper

On Tue, Feb 11, 2020 at 10:11:42AM -0800, Caz Yokoyama wrote:
> From: "Yokoyama, Caz" 
> 
> MAD_INTER_CHANNEL_0_0_0_MCHBAR:
> code nameoffset   DRAM_DDR_TYPE
> SKL  0x5000   1:0 DDR4/DDR3/LPDDR3
> TGL  0x6048/0x6248/0xd800 2:0 DDR4/DDR3/LPDDR3/LPDDR4
> ICL  0x5000/0x6048/0x48   2:0 DDR4/DDR3/LPDDR3/LPDDR4
> EHL  0x5000/0x60482:0 DDR4/DDR3/LPDDR3/LPDDR4
> CNL  0x5000   2:0 DDR4/DDR3/LPDDR3/LPDDR4
> 
> MAD_DIMM_CH0/1_0_0_0_MCHBAR:
> code name  offset CH0/1
> SKL0x500c/0x5010
> TGL0x6054/0x6058
> EHL0x500c/0x5010
> ICL0x500c/0x5010
> The bit definition of MAD_DIMM_CH0/1_0_0_0_MCHBAR is same between
> CNL and TGL.

Have you actually sanity checked the register addresses here on real
hardware?  I see the same offsets in the doc as what you've put here,
but since this is a different register database than we get most of our
gfx-specific register details from, it would still be good to double
check that you do indeed get sensible values back when reading from
these addresses before we merge the patch.  Especially since the
database indicates that some of these registers are present at multiple
offsets.

> 
> P_CR_MC_BIOS_REQ_0_0_0 is same on BXT and GLK in terms of its address and
> bit definition.
> BXT_D_CR_DRP0_DUNIT accesses offset 0x1000, 0x1200, 0x1400, 0x1600.
> Its register name is RAM_ACCESS_DATA1. There is no difference between
> BXT and GLK in terms of its address and bit definition.
> 
> Cc: Ville Syrjälä 
> Cc: Matt Roper 
> Signed-off-by: Yokoyama, Caz 
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 15 ---
>  drivers/gpu/drm/i915/i915_reg.h |  6 ++
>  2 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 516536234e97..b9418583e503 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -806,12 +806,18 @@ skl_dram_get_channels_info(struct drm_i915_private 
> *dev_priv)
>   u32 val;
>   int ret;
>  
> - val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
> + if (INTEL_GEN(dev_priv) >= 12)
> + val = I915_READ(TGL_MAD_DIMM_CH0_0_0_0_MCHBAR);
> + else
> + val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
>   ret = skl_dram_get_channel_info(dev_priv, &ch0, 0, val);
>   if (ret == 0)
>   dram_info->num_channels++;
>  
> - val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
> + if (INTEL_GEN(dev_priv) >= 12)
> + val = I915_READ(TGL_MAD_DIMM_CH1_0_0_0_MCHBAR);
> + else
> + val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
>   ret = skl_dram_get_channel_info(dev_priv, &ch1, 1, val);
>   if (ret == 0)
>   dram_info->num_channels++;
> @@ -852,7 +858,10 @@ skl_get_dram_type(struct drm_i915_private *dev_priv)
>  {
>   u32 val;
>  
> - val = I915_READ(SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN);
> + if (INTEL_GEN(dev_priv) >= 12)
> + val = I915_READ(TGL_MAD_INTER_CHANNEL_0_0_0_MCHBAR);
> + else
> + val = I915_READ(SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN);
>  
>   switch (val & SKL_DRAM_DDR_TYPE_MASK) {
>   case SKL_DRAM_DDR_TYPE_DDR3:

I'm not sure if it might be cleaner to create regs structures for each
platform to centralize the platform-selection logic and avoid all the
if/else in the code.  E.g.,

struct i915_mchbar_regs {
i915_reg_t mad_inter_channel;
i915_reg_t mad_dimm_ch0;
i915_reg_t mad_dimm_ch1;
};

static const struct i915_mchbar_regs skl_mchbar_regs = {
.mad_inter_channel = SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN,
.mad_dimm_ch0 = SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN,
.mad_dimm_ch1 = SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN,
};

and then use the appropriate regs structure in each of these functions
so that you can just do

val = I915_READ(regs->mad_inter_channel);

and not have to update a bunch of different if-trees if more platforms
show up that move the registers to new offsets.

> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b09c1d6dc0aa..9f6ec44dad6e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -10508,6 +10508,9 @@ enum skl_power_gate {
>  #define  SKL_DRAM_DDR_TYPE_LPDDR3(2 << 0)
>  #define  SKL_DRAM_DDR_TYPE_LPDDR4(3 << 0)
>  
> +#define  TGL_MAD_INTER_CHANNEL_0_0_0_MCHBAR _MMIO(MCHBAR_MIRROR_BASE_SNB + 
> 0x6048)

I'd move this line directly under
SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN's definition to help make it
clear that the SKL_DRAM_DDR_TYPE_* bit definitions apply to this
register

[Intel-gfx] [PATCH 1/1] drm/i915: MCHBAR memory info registers are moved since GEN 12.

2020-02-11 Thread Caz Yokoyama

From: "Yokoyama, Caz" 

MAD_INTER_CHANNEL_0_0_0_MCHBAR:
code nameoffset   DRAM_DDR_TYPE
SKL  0x5000   1:0 DDR4/DDR3/LPDDR3
TGL  0x6048/0x6248/0xd800 2:0 DDR4/DDR3/LPDDR3/LPDDR4
ICL  0x5000/0x6048/0x48   2:0 DDR4/DDR3/LPDDR3/LPDDR4
EHL  0x5000/0x60482:0 DDR4/DDR3/LPDDR3/LPDDR4
CNL  0x5000   2:0 DDR4/DDR3/LPDDR3/LPDDR4

MAD_DIMM_CH0/1_0_0_0_MCHBAR:
code name  offset CH0/1
SKL0x500c/0x5010
TGL0x6054/0x6058
EHL0x500c/0x5010
ICL0x500c/0x5010
The bit definition of MAD_DIMM_CH0/1_0_0_0_MCHBAR is same between
CNL and TGL.

P_CR_MC_BIOS_REQ_0_0_0 is same on BXT and GLK in terms of its address and
bit definition.
BXT_D_CR_DRP0_DUNIT accesses offset 0x1000, 0x1200, 0x1400, 0x1600.
Its register name is RAM_ACCESS_DATA1. There is no difference between
BXT and GLK in terms of its address and bit definition.

Cc: Ville Syrjälä 
Cc: Matt Roper 
Signed-off-by: Yokoyama, Caz 
---
 drivers/gpu/drm/i915/i915_drv.c | 15 ---
 drivers/gpu/drm/i915/i915_reg.h |  6 ++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 516536234e97..b9418583e503 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -806,12 +806,18 @@ skl_dram_get_channels_info(struct drm_i915_private 
*dev_priv)
u32 val;
int ret;
 
-   val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
+   if (INTEL_GEN(dev_priv) >= 12)
+   val = I915_READ(TGL_MAD_DIMM_CH0_0_0_0_MCHBAR);
+   else
+   val = I915_READ(SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN);
ret = skl_dram_get_channel_info(dev_priv, &ch0, 0, val);
if (ret == 0)
dram_info->num_channels++;
 
-   val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
+   if (INTEL_GEN(dev_priv) >= 12)
+   val = I915_READ(TGL_MAD_DIMM_CH1_0_0_0_MCHBAR);
+   else
+   val = I915_READ(SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN);
ret = skl_dram_get_channel_info(dev_priv, &ch1, 1, val);
if (ret == 0)
dram_info->num_channels++;
@@ -852,7 +858,10 @@ skl_get_dram_type(struct drm_i915_private *dev_priv)
 {
u32 val;
 
-   val = I915_READ(SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN);
+   if (INTEL_GEN(dev_priv) >= 12)
+   val = I915_READ(TGL_MAD_INTER_CHANNEL_0_0_0_MCHBAR);
+   else
+   val = I915_READ(SKL_MAD_INTER_CHANNEL_0_0_0_MCHBAR_MCMAIN);
 
switch (val & SKL_DRAM_DDR_TYPE_MASK) {
case SKL_DRAM_DDR_TYPE_DDR3:
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b09c1d6dc0aa..9f6ec44dad6e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -10508,6 +10508,9 @@ enum skl_power_gate {
 #define  SKL_DRAM_DDR_TYPE_LPDDR3  (2 << 0)
 #define  SKL_DRAM_DDR_TYPE_LPDDR4  (3 << 0)
 
+#define  TGL_MAD_INTER_CHANNEL_0_0_0_MCHBAR _MMIO(MCHBAR_MIRROR_BASE_SNB + 
0x6048)
+#define  TGL_DRAM_DDR_TYPE_WIO2(4 << 0)
+
 #define SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN   _MMIO(MCHBAR_MIRROR_BASE_SNB + 
0x500C)
 #define SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN   _MMIO(MCHBAR_MIRROR_BASE_SNB + 
0x5010)
 #define  SKL_DRAM_S_SHIFT  16
@@ -10535,6 +10538,9 @@ enum skl_power_gate {
 #define  CNL_DRAM_RANK_3   (0x2 << 9)
 #define  CNL_DRAM_RANK_4   (0x3 << 9)
 
+#define TGL_MAD_DIMM_CH0_0_0_0_MCHBAR  _MMIO(MCHBAR_MIRROR_BASE_SNB + 
0x6054)
+#define TGL_MAD_DIMM_CH1_0_0_0_MCHBAR  _MMIO(MCHBAR_MIRROR_BASE_SNB + 
0x6058)
+
 /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this 
register,
  * since on HSW we can't write to it using I915_WRITE. */
 #define D_COMP_HSW _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
-- 
2.21.0.5.gaeb582a983

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/1] drm/i915: MCHBAR memory info registers are moved since GEN 12.

Re: [Intel-gfx] [PATCH 1/1] drm/i915: MCHBAR memory info registers are moved since GEN 12.

[Intel-gfx] [PATCH 1/1] drm/i915: MCHBAR memory info registers are moved since GEN 12.

3 matches

Site Navigation

Mail list logo

Footer information