diff mbox series

[1/2] drm/i915/dmc: Use RUNTIME_INFO->step for DMC

Message ID 20210701193114.17531-2-anusha.srivatsa@intel.com (mailing list archive)
State New, archived
Headers show
Series Stepping/substepping reorg for DMC | expand

Commit Message

Srivatsa, Anusha July 1, 2021, 7:31 p.m. UTC
On the dmc side,we maintain a lookup table with different display
stepping-substepping combinations.

Instead of adding new table for every new platform, lets ues
the stepping info from RUNTIME_INFO(dev_priv)->step

v2: Add stepping table for older platforms in intel_step.c (Lucas)

Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dmc.c | 117 +++++++++++++++--------
 drivers/gpu/drm/i915/intel_step.c        |  41 ++++++++
 drivers/gpu/drm/i915/intel_step.h        |   7 ++
 3 files changed, 126 insertions(+), 39 deletions(-)

Comments

kernel test robot July 2, 2021, 8:56 a.m. UTC | #1
Hi Anusha,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-tip/drm-tip]
[also build test WARNING on next-20210701]
[cannot apply to drm-intel/for-linux-next v5.13]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Anusha-Srivatsa/Stepping-substepping-reorg-for-DMC/20210702-033236
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
config: x86_64-randconfig-a015-20210630 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 9eb613b2de3163686b1a4bd1160f15ac56a4b083)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/5d13218aefcba8e3bc4c4d4371988e08e0cf8bd3
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Anusha-Srivatsa/Stepping-substepping-reorg-for-DMC/20210702-033236
        git checkout 5d13218aefcba8e3bc4c4d4371988e08e0cf8bd3
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/i915/display/intel_dmc.c:250:35: warning: unused variable 'no_stepping_info' [-Wunused-const-variable]
   static const struct stepping_info no_stepping_info = { '*', '*' };
                                     ^
   1 warning generated.


vim +/no_stepping_info +250 drivers/gpu/drm/i915/display/intel_dmc.c

03256487fee340 drivers/gpu/drm/i915/display/intel_dmc.c Anusha Srivatsa 2021-05-26  249  
1bb4308e713042 drivers/gpu/drm/i915/intel_csr.c         Chris Wilson    2016-03-07 @250  static const struct stepping_info no_stepping_info = { '*', '*' };
5d13218aefcba8 drivers/gpu/drm/i915/display/intel_dmc.c Anusha Srivatsa 2021-07-01  251  struct stepping_info *display_step;
1bb4308e713042 drivers/gpu/drm/i915/intel_csr.c         Chris Wilson    2016-03-07  252  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Jani Nikula July 5, 2021, 10:01 a.m. UTC | #2
On Thu, 01 Jul 2021, Anusha Srivatsa <anusha.srivatsa@intel.com> wrote:
> On the dmc side,we maintain a lookup table with different display
> stepping-substepping combinations.
>
> Instead of adding new table for every new platform, lets ues
> the stepping info from RUNTIME_INFO(dev_priv)->step
>
> v2: Add stepping table for older platforms in intel_step.c (Lucas)
>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dmc.c | 117 +++++++++++++++--------

The changes here...

>  drivers/gpu/drm/i915/intel_step.c        |  41 ++++++++
>  drivers/gpu/drm/i915/intel_step.h        |   7 ++

...should be separate from, and depend on, the changes here. Please
don't try to do too much in one patch. Smaller is better.

>  3 files changed, 126 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
> index f8789d4543bf..df888a3d086e 100644
> --- a/drivers/gpu/drm/i915/display/intel_dmc.c
> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
> @@ -247,50 +247,89 @@ bool intel_dmc_has_payload(struct drm_i915_private *i915)
>  	return i915->dmc.dmc_info[DMC_FW_MAIN].payload;
>  }
>  
> -static const struct stepping_info skl_stepping_info[] = {
> -	{'A', '0'}, {'B', '0'}, {'C', '0'},
> -	{'D', '0'}, {'E', '0'}, {'F', '0'},
> -	{'G', '0'}, {'H', '0'}, {'I', '0'},
> -	{'J', '0'}, {'K', '0'}
> -};
> -
> -static const struct stepping_info bxt_stepping_info[] = {
> -	{'A', '0'}, {'A', '1'}, {'A', '2'},
> -	{'B', '0'}, {'B', '1'}, {'B', '2'}
> -};
> -
> -static const struct stepping_info icl_stepping_info[] = {
> -	{'A', '0'}, {'A', '1'}, {'A', '2'},
> -	{'B', '0'}, {'B', '2'},
> -	{'C', '0'}
> -};
> -
>  static const struct stepping_info no_stepping_info = { '*', '*' };
> +struct stepping_info *display_step;
>  
> -static const struct stepping_info *
> +static struct stepping_info *
>  intel_get_stepping_info(struct drm_i915_private *dev_priv)
>  {
> -	const struct stepping_info *si;
> -	unsigned int size;
> -
> -	if (IS_ICELAKE(dev_priv)) {
> -		size = ARRAY_SIZE(icl_stepping_info);
> -		si = icl_stepping_info;
> -	} else if (IS_SKYLAKE(dev_priv)) {
> -		size = ARRAY_SIZE(skl_stepping_info);
> -		si = skl_stepping_info;
> -	} else if (IS_BROXTON(dev_priv)) {
> -		size = ARRAY_SIZE(bxt_stepping_info);
> -		si = bxt_stepping_info;
> -	} else {
> -		size = 0;
> -		si = NULL;
> +	struct intel_step_info step = RUNTIME_INFO(dev_priv)->step;
> +
> +	switch (step.display_step) {
> +	case STEP_A0:
> +		display_step->stepping = 'A';
> +		display_step->substepping = '0';
> +		break;
> +	case STEP_A1:
> +		display_step->stepping = 'A';
> +		display_step->substepping = '1';
> +		break;
> +	case STEP_A2:
> +		display_step->stepping = 'A';
> +		display_step->substepping = '2';
> +		break;
> +	case STEP_B0:
> +		display_step->stepping = 'B';
> +		display_step->substepping = '0';
> +		break;
> +	case STEP_B1:
> +		display_step->stepping = 'B';
> +		display_step->substepping = '1';
> +		break;
> +	case STEP_B2:
> +		display_step->stepping = 'B';
> +		display_step->substepping = '2';
> +		break;
> +	case STEP_C0:
> +		display_step->stepping = 'C';
> +		display_step->substepping = '0';
> +		break;
> +	case STEP_C1:
> +		display_step->stepping = 'C';
> +		display_step->substepping = '1';
> +		break;
> +	case STEP_D0:
> +		display_step->stepping = 'D';
> +		display_step->substepping = '0';
> +		break;
> +	case STEP_D1:
> +		display_step->stepping = 'D';
> +		display_step->substepping = '1';
> +		break;
> +	case STEP_E0:
> +		display_step->stepping = 'E';
> +		display_step->substepping = '0';
> +		break;
> +	case STEP_F0:
> +		display_step->stepping = 'F';
> +		display_step->substepping = '0';
> +		break;
> +	case STEP_G0:
> +		display_step->stepping = 'G';
> +		display_step->substepping = '0';
> +		break;
> +	case STEP_H0:
> +		display_step->stepping = 'H';
> +		display_step->substepping = '0';
> +		break;
> +	case STEP_I0:
> +		display_step->stepping = 'I';
> +		display_step->substepping = '0';
> +		break;
> +	case STEP_J0:
> +		display_step->stepping = 'J';
> +		display_step->substepping = '0';
> +		break;
> +	case STEP_K0:
> +		display_step->stepping = 'K';
> +		display_step->substepping = '0';
> +		break;
> +	default:
> +		display_step->stepping = '*';
> +		display_step->substepping = '*';
> +		break;
>  	}
> -
> -	if (INTEL_REVID(dev_priv) < size)
> -		return si + INTEL_REVID(dev_priv);
> -
> -	return &no_stepping_info;
> +	return display_step;
>  }
>  
>  static void gen9_set_dc_state_debugmask(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/intel_step.c b/drivers/gpu/drm/i915/intel_step.c
> index ba9479a67521..c8542161c5d0 100644
> --- a/drivers/gpu/drm/i915/intel_step.c
> +++ b/drivers/gpu/drm/i915/intel_step.c
> @@ -15,6 +15,38 @@
>  
>  
>  /* FIXME: what about REVID_E0 */

So that comment was next to kbl_revids, and this makes it meaningless.

> +static const struct intel_step_info bxt_revid_step_tbl[] = {

Please just call them "bxt_revids" or "bxt_steps" or something simple. I
meant to clean up all the other names too, they're a bit excessive.

> +	[0] = { .gt_step = STEP_A0, .display_step = STEP_A0 },
> +	[1] = { .gt_step = STEP_A1, .display_step = STEP_A1 },
> +	[2] = { .gt_step = STEP_A2, .display_step = STEP_A2 },
> +	[6] = { .gt_step = STEP_B0, .display_step = STEP_B0 },
> +	[7] = { .gt_step = STEP_B1, .display_step = STEP_B1 },
> +	[8] = { .gt_step = STEP_B2, .display_step = STEP_B2 },
> +};

We might want to add something like this:

@@ -112,5 +112,8 @@ void intel_step_init(struct drm_i915_private *i915)
 	if (drm_WARN_ON(&i915->drm, step.gt_step == STEP_NONE))
 		return;
 
+	if (step.display_step == STEP_NONE)
+		step.display_step = step.gt_step;
+
 	RUNTIME_INFO(i915)->step = step;
 }

So that we could just initialize .gt_step when the .display_step matches
the .gt_step, and we could avoid the repetition. This wasn't useful for
the platforms currently added, because they actually have the
differences.

This can also be left for future cleanup.

> +
> +

Superfluous newline.

> +static const struct intel_step_info skl_revid_step_tbl[] = {
> +	[0] = { .gt_step = STEP_A0, .display_step = STEP_A0 },
> +	[1] = { .gt_step = STEP_B0, .display_step = STEP_B0 },
> +	[2] = { .gt_step = STEP_C0, .display_step = STEP_C0 },
> +	[3] = { .gt_step = STEP_D0, .display_step = STEP_D0 },
> +	[4] = { .gt_step = STEP_E0, .display_step = STEP_E0 },
> +	[5] = { .gt_step = STEP_F0, .display_step = STEP_F0 },
> +	[6] = { .gt_step = STEP_G0, .display_step = STEP_G0 },
> +	[7] = { .gt_step = STEP_H0, .display_step = STEP_H0 },
> +	[8] = { .gt_step = STEP_I0, .display_step = STEP_I0 },
> +	[9] = { .gt_step = STEP_J0, .display_step = STEP_J0 },

skl_stepping_info in intel_dmc.c has K0, this one doesn't.

> +};
> +
> +static const struct intel_step_info icl_revid_step_tbl[] = {
> +	[0] = { .gt_step = STEP_A0, .display_step = STEP_A0 },
> +	[3] = { .gt_step = STEP_B0, .display_step = STEP_B0 },
> +	[4] = { .gt_step = STEP_B2, .display_step = STEP_B2 },
> +	[5] = { .gt_step = STEP_C0, .display_step = STEP_C0 },
> +	[6] = { .gt_step = STEP_C1, .display_step = STEP_C1 },
> +	[7] = { .gt_step = STEP_D0, .display_step = STEP_D0 },

icl_stepping_info in intel_dmc.c has A1 and A2, this one doesn't. This
one has C1 and D0, the one in intel_dmc.c doesn't.

> +};
> +
>  static const struct intel_step_info kbl_revids[] = {
>  	[0] = { .gt_step = STEP_A0, .display_step = STEP_A0 },
>  	[1] = { .gt_step = STEP_B0, .display_step = STEP_B0 },
> @@ -76,6 +108,15 @@ void intel_step_init(struct drm_i915_private *i915)
>  	} else if (IS_KABYLAKE(i915)) {
>  		revids = kbl_revids;
>  		size = ARRAY_SIZE(kbl_revids);
> +	} else if (IS_ICELAKE(i915)) {
> +		revids = icl_revid_step_tbl;
> +		size = ARRAY_SIZE(icl_revid_step_tbl);
> +	} else if (IS_SKYLAKE(i915)) {
> +		revids = skl_revid_step_tbl;
> +		size = ARRAY_SIZE(skl_revid_step_tbl);
> +	} else if (IS_BROXTON(i915)) {
> +		revids = bxt_revid_step_tbl;
> +		size = ARRAY_SIZE(bxt_revid_step_tbl);

Doing this should be accompanied by:

* Removing *all* ICL_REVID_*, SKL_REVID_* and BXT_REVID_* macros from
  i915_drv.h.

* Converting IS_ICL_REVID(), IS_SKL_REVID(), and IS_BXT_REVID() to
  IS_{ICL,SKL,KBL}_{GT,DISPLAY}_STEP().

* And making their references use the generic STEP_* macros.

These changes should probably be done on a platform by platform basis,
just in case we hit any issues. This needs to be trivial to bisect and
backtrack as needed.

We currently have two schemes just for transition; we do not want to
have two schemes for a single platform effective at the same time.


BR,
Jani.

>  	}
>  
>  	/* Not using the stepping scheme for the platform yet. */
> diff --git a/drivers/gpu/drm/i915/intel_step.h b/drivers/gpu/drm/i915/intel_step.h
> index 958a8bb5d677..1bffa41f2660 100644
> --- a/drivers/gpu/drm/i915/intel_step.h
> +++ b/drivers/gpu/drm/i915/intel_step.h
> @@ -22,15 +22,22 @@ struct intel_step_info {
>  enum intel_step {
>  	STEP_NONE = 0,
>  	STEP_A0,
> +	STEP_A1,
>  	STEP_A2,
>  	STEP_B0,
>  	STEP_B1,
> +	STEP_B2,
>  	STEP_C0,
> +	STEP_C1,
>  	STEP_D0,
>  	STEP_D1,
>  	STEP_E0,
>  	STEP_F0,
>  	STEP_G0,
> +	STEP_H0,
> +	STEP_I0,
> +	STEP_J0,
> +	STEP_K0,
>  	STEP_FUTURE,
>  	STEP_FOREVER,
>  };
Srivatsa, Anusha July 6, 2021, 6:33 p.m. UTC | #3
> -----Original Message-----
> From: Jani Nikula <jani.nikula@linux.intel.com>
> Sent: Monday, July 5, 2021 3:01 AM
> To: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: De Marchi, Lucas <lucas.demarchi@intel.com>
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/dmc: Use RUNTIME_INFO-
> >step for DMC
> 
> On Thu, 01 Jul 2021, Anusha Srivatsa <anusha.srivatsa@intel.com> wrote:
> > On the dmc side,we maintain a lookup table with different display
> > stepping-substepping combinations.
> >
> > Instead of adding new table for every new platform, lets ues the
> > stepping info from RUNTIME_INFO(dev_priv)->step
> >
> > v2: Add stepping table for older platforms in intel_step.c (Lucas)
> >
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dmc.c | 117
> > +++++++++++++++--------
> 
> The changes here...
> 
> >  drivers/gpu/drm/i915/intel_step.c        |  41 ++++++++
> >  drivers/gpu/drm/i915/intel_step.h        |   7 ++
> 
> ...should be separate from, and depend on, the changes here. Please don't
> try to do too much in one patch. Smaller is better.
> 
> >  3 files changed, 126 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c
> > b/drivers/gpu/drm/i915/display/intel_dmc.c
> > index f8789d4543bf..df888a3d086e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dmc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
> > @@ -247,50 +247,89 @@ bool intel_dmc_has_payload(struct
> drm_i915_private *i915)
> >  	return i915->dmc.dmc_info[DMC_FW_MAIN].payload;
> >  }
> >
> > -static const struct stepping_info skl_stepping_info[] = {
> > -	{'A', '0'}, {'B', '0'}, {'C', '0'},
> > -	{'D', '0'}, {'E', '0'}, {'F', '0'},
> > -	{'G', '0'}, {'H', '0'}, {'I', '0'},
> > -	{'J', '0'}, {'K', '0'}
> > -};
> > -
> > -static const struct stepping_info bxt_stepping_info[] = {
> > -	{'A', '0'}, {'A', '1'}, {'A', '2'},
> > -	{'B', '0'}, {'B', '1'}, {'B', '2'}
> > -};
> > -
> > -static const struct stepping_info icl_stepping_info[] = {
> > -	{'A', '0'}, {'A', '1'}, {'A', '2'},
> > -	{'B', '0'}, {'B', '2'},
> > -	{'C', '0'}
> > -};
> > -
> >  static const struct stepping_info no_stepping_info = { '*', '*' };
> > +struct stepping_info *display_step;
> >
> > -static const struct stepping_info *
> > +static struct stepping_info *
> >  intel_get_stepping_info(struct drm_i915_private *dev_priv)  {
> > -	const struct stepping_info *si;
> > -	unsigned int size;
> > -
> > -	if (IS_ICELAKE(dev_priv)) {
> > -		size = ARRAY_SIZE(icl_stepping_info);
> > -		si = icl_stepping_info;
> > -	} else if (IS_SKYLAKE(dev_priv)) {
> > -		size = ARRAY_SIZE(skl_stepping_info);
> > -		si = skl_stepping_info;
> > -	} else if (IS_BROXTON(dev_priv)) {
> > -		size = ARRAY_SIZE(bxt_stepping_info);
> > -		si = bxt_stepping_info;
> > -	} else {
> > -		size = 0;
> > -		si = NULL;
> > +	struct intel_step_info step = RUNTIME_INFO(dev_priv)->step;
> > +
> > +	switch (step.display_step) {
> > +	case STEP_A0:
> > +		display_step->stepping = 'A';
> > +		display_step->substepping = '0';
> > +		break;
> > +	case STEP_A1:
> > +		display_step->stepping = 'A';
> > +		display_step->substepping = '1';
> > +		break;
> > +	case STEP_A2:
> > +		display_step->stepping = 'A';
> > +		display_step->substepping = '2';
> > +		break;
> > +	case STEP_B0:
> > +		display_step->stepping = 'B';
> > +		display_step->substepping = '0';
> > +		break;
> > +	case STEP_B1:
> > +		display_step->stepping = 'B';
> > +		display_step->substepping = '1';
> > +		break;
> > +	case STEP_B2:
> > +		display_step->stepping = 'B';
> > +		display_step->substepping = '2';
> > +		break;
> > +	case STEP_C0:
> > +		display_step->stepping = 'C';
> > +		display_step->substepping = '0';
> > +		break;
> > +	case STEP_C1:
> > +		display_step->stepping = 'C';
> > +		display_step->substepping = '1';
> > +		break;
> > +	case STEP_D0:
> > +		display_step->stepping = 'D';
> > +		display_step->substepping = '0';
> > +		break;
> > +	case STEP_D1:
> > +		display_step->stepping = 'D';
> > +		display_step->substepping = '1';
> > +		break;
> > +	case STEP_E0:
> > +		display_step->stepping = 'E';
> > +		display_step->substepping = '0';
> > +		break;
> > +	case STEP_F0:
> > +		display_step->stepping = 'F';
> > +		display_step->substepping = '0';
> > +		break;
> > +	case STEP_G0:
> > +		display_step->stepping = 'G';
> > +		display_step->substepping = '0';
> > +		break;
> > +	case STEP_H0:
> > +		display_step->stepping = 'H';
> > +		display_step->substepping = '0';
> > +		break;
> > +	case STEP_I0:
> > +		display_step->stepping = 'I';
> > +		display_step->substepping = '0';
> > +		break;
> > +	case STEP_J0:
> > +		display_step->stepping = 'J';
> > +		display_step->substepping = '0';
> > +		break;
> > +	case STEP_K0:
> > +		display_step->stepping = 'K';
> > +		display_step->substepping = '0';
> > +		break;
> > +	default:
> > +		display_step->stepping = '*';
> > +		display_step->substepping = '*';
> > +		break;
> >  	}
> > -
> > -	if (INTEL_REVID(dev_priv) < size)
> > -		return si + INTEL_REVID(dev_priv);
> > -
> > -	return &no_stepping_info;
> > +	return display_step;
> >  }
> >
> >  static void gen9_set_dc_state_debugmask(struct drm_i915_private
> > *dev_priv) diff --git a/drivers/gpu/drm/i915/intel_step.c
> > b/drivers/gpu/drm/i915/intel_step.c
> > index ba9479a67521..c8542161c5d0 100644
> > --- a/drivers/gpu/drm/i915/intel_step.c
> > +++ b/drivers/gpu/drm/i915/intel_step.c
> > @@ -15,6 +15,38 @@
> >
> >
> >  /* FIXME: what about REVID_E0 */
> 
> So that comment was next to kbl_revids, and this makes it meaningless.
> 
> > +static const struct intel_step_info bxt_revid_step_tbl[] = {
> 
> Please just call them "bxt_revids" or "bxt_steps" or something simple. I
> meant to clean up all the other names too, they're a bit excessive.
>
I ll send the patch to change the names.

> > +	[0] = { .gt_step = STEP_A0, .display_step = STEP_A0 },
> > +	[1] = { .gt_step = STEP_A1, .display_step = STEP_A1 },
> > +	[2] = { .gt_step = STEP_A2, .display_step = STEP_A2 },
> > +	[6] = { .gt_step = STEP_B0, .display_step = STEP_B0 },
> > +	[7] = { .gt_step = STEP_B1, .display_step = STEP_B1 },
> > +	[8] = { .gt_step = STEP_B2, .display_step = STEP_B2 }, };
> 
> We might want to add something like this:
> 
> @@ -112,5 +112,8 @@ void intel_step_init(struct drm_i915_private *i915)
>  	if (drm_WARN_ON(&i915->drm, step.gt_step == STEP_NONE))
>  		return;
> 
> +	if (step.display_step == STEP_NONE)
> +		step.display_step = step.gt_step;
> +
>  	RUNTIME_INFO(i915)->step = step;
>  }
> 
> So that we could just initialize .gt_step when the .display_step matches the
> .gt_step, and we could avoid the repetition. This wasn't useful for the
> platforms currently added, because they actually have the differences.
> 
> This can also be left for future cleanup.

Makes sense. I ll be changing this now. Thanks for feedback.

> > +
> > +
> 
> Superfluous newline.
> 
> > +static const struct intel_step_info skl_revid_step_tbl[] = {
> > +	[0] = { .gt_step = STEP_A0, .display_step = STEP_A0 },
> > +	[1] = { .gt_step = STEP_B0, .display_step = STEP_B0 },
> > +	[2] = { .gt_step = STEP_C0, .display_step = STEP_C0 },
> > +	[3] = { .gt_step = STEP_D0, .display_step = STEP_D0 },
> > +	[4] = { .gt_step = STEP_E0, .display_step = STEP_E0 },
> > +	[5] = { .gt_step = STEP_F0, .display_step = STEP_F0 },
> > +	[6] = { .gt_step = STEP_G0, .display_step = STEP_G0 },
> > +	[7] = { .gt_step = STEP_H0, .display_step = STEP_H0 },
> > +	[8] = { .gt_step = STEP_I0, .display_step = STEP_I0 },
> > +	[9] = { .gt_step = STEP_J0, .display_step = STEP_J0 },
> 
> skl_stepping_info in intel_dmc.c has K0, this one doesn't.
>
> > +};
> > +
> > +static const struct intel_step_info icl_revid_step_tbl[] = {
> > +	[0] = { .gt_step = STEP_A0, .display_step = STEP_A0 },
> > +	[3] = { .gt_step = STEP_B0, .display_step = STEP_B0 },
> > +	[4] = { .gt_step = STEP_B2, .display_step = STEP_B2 },
> > +	[5] = { .gt_step = STEP_C0, .display_step = STEP_C0 },
> > +	[6] = { .gt_step = STEP_C1, .display_step = STEP_C1 },
> > +	[7] = { .gt_step = STEP_D0, .display_step = STEP_D0 },
> 
> icl_stepping_info in intel_dmc.c has A1 and A2, this one doesn't. This one has
> C1 and D0, the one in intel_dmc.c doesn't.
> 
> > +};
> > +
> >  static const struct intel_step_info kbl_revids[] = {
> >  	[0] = { .gt_step = STEP_A0, .display_step = STEP_A0 },
> >  	[1] = { .gt_step = STEP_B0, .display_step = STEP_B0 }, @@ -76,6
> > +108,15 @@ void intel_step_init(struct drm_i915_private *i915)
> >  	} else if (IS_KABYLAKE(i915)) {
> >  		revids = kbl_revids;
> >  		size = ARRAY_SIZE(kbl_revids);
> > +	} else if (IS_ICELAKE(i915)) {
> > +		revids = icl_revid_step_tbl;
> > +		size = ARRAY_SIZE(icl_revid_step_tbl);
> > +	} else if (IS_SKYLAKE(i915)) {
> > +		revids = skl_revid_step_tbl;
> > +		size = ARRAY_SIZE(skl_revid_step_tbl);
> > +	} else if (IS_BROXTON(i915)) {
> > +		revids = bxt_revid_step_tbl;
> > +		size = ARRAY_SIZE(bxt_revid_step_tbl);
> 
> Doing this should be accompanied by:
> 
> * Removing *all* ICL_REVID_*, SKL_REVID_* and BXT_REVID_* macros from
>   i915_drv.h.
> 
> * Converting IS_ICL_REVID(), IS_SKL_REVID(), and IS_BXT_REVID() to
>   IS_{ICL,SKL,KBL}_{GT,DISPLAY}_STEP().
> 
> * And making their references use the generic STEP_* macros.
> 
> These changes should probably be done on a platform by platform basis, just
> in case we hit any issues. This needs to be trivial to bisect and backtrack as
> needed.
> 
> We currently have two schemes just for transition; we do not want to have
> two schemes for a single platform effective at the same time.
> 

Thanks for feedback. Taking care of these changes.

Anusha 
 
> BR,
> Jani.
> 
> >  	}
> >
> >  	/* Not using the stepping scheme for the platform yet. */ diff --git
> > a/drivers/gpu/drm/i915/intel_step.h
> > b/drivers/gpu/drm/i915/intel_step.h
> > index 958a8bb5d677..1bffa41f2660 100644
> > --- a/drivers/gpu/drm/i915/intel_step.h
> > +++ b/drivers/gpu/drm/i915/intel_step.h
> > @@ -22,15 +22,22 @@ struct intel_step_info {  enum intel_step {
> >  	STEP_NONE = 0,
> >  	STEP_A0,
> > +	STEP_A1,
> >  	STEP_A2,
> >  	STEP_B0,
> >  	STEP_B1,
> > +	STEP_B2,
> >  	STEP_C0,
> > +	STEP_C1,
> >  	STEP_D0,
> >  	STEP_D1,
> >  	STEP_E0,
> >  	STEP_F0,
> >  	STEP_G0,
> > +	STEP_H0,
> > +	STEP_I0,
> > +	STEP_J0,
> > +	STEP_K0,
> >  	STEP_FUTURE,
> >  	STEP_FOREVER,
> >  };
> 
> --
> Jani Nikula, Intel Open Source Graphics Center
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
index f8789d4543bf..df888a3d086e 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
@@ -247,50 +247,89 @@  bool intel_dmc_has_payload(struct drm_i915_private *i915)
 	return i915->dmc.dmc_info[DMC_FW_MAIN].payload;
 }
 
-static const struct stepping_info skl_stepping_info[] = {
-	{'A', '0'}, {'B', '0'}, {'C', '0'},
-	{'D', '0'}, {'E', '0'}, {'F', '0'},
-	{'G', '0'}, {'H', '0'}, {'I', '0'},
-	{'J', '0'}, {'K', '0'}
-};
-
-static const struct stepping_info bxt_stepping_info[] = {
-	{'A', '0'}, {'A', '1'}, {'A', '2'},
-	{'B', '0'}, {'B', '1'}, {'B', '2'}
-};
-
-static const struct stepping_info icl_stepping_info[] = {
-	{'A', '0'}, {'A', '1'}, {'A', '2'},
-	{'B', '0'}, {'B', '2'},
-	{'C', '0'}
-};
-
 static const struct stepping_info no_stepping_info = { '*', '*' };
+struct stepping_info *display_step;
 
-static const struct stepping_info *
+static struct stepping_info *
 intel_get_stepping_info(struct drm_i915_private *dev_priv)
 {
-	const struct stepping_info *si;
-	unsigned int size;
-
-	if (IS_ICELAKE(dev_priv)) {
-		size = ARRAY_SIZE(icl_stepping_info);
-		si = icl_stepping_info;
-	} else if (IS_SKYLAKE(dev_priv)) {
-		size = ARRAY_SIZE(skl_stepping_info);
-		si = skl_stepping_info;
-	} else if (IS_BROXTON(dev_priv)) {
-		size = ARRAY_SIZE(bxt_stepping_info);
-		si = bxt_stepping_info;
-	} else {
-		size = 0;
-		si = NULL;
+	struct intel_step_info step = RUNTIME_INFO(dev_priv)->step;
+
+	switch (step.display_step) {
+	case STEP_A0:
+		display_step->stepping = 'A';
+		display_step->substepping = '0';
+		break;
+	case STEP_A1:
+		display_step->stepping = 'A';
+		display_step->substepping = '1';
+		break;
+	case STEP_A2:
+		display_step->stepping = 'A';
+		display_step->substepping = '2';
+		break;
+	case STEP_B0:
+		display_step->stepping = 'B';
+		display_step->substepping = '0';
+		break;
+	case STEP_B1:
+		display_step->stepping = 'B';
+		display_step->substepping = '1';
+		break;
+	case STEP_B2:
+		display_step->stepping = 'B';
+		display_step->substepping = '2';
+		break;
+	case STEP_C0:
+		display_step->stepping = 'C';
+		display_step->substepping = '0';
+		break;
+	case STEP_C1:
+		display_step->stepping = 'C';
+		display_step->substepping = '1';
+		break;
+	case STEP_D0:
+		display_step->stepping = 'D';
+		display_step->substepping = '0';
+		break;
+	case STEP_D1:
+		display_step->stepping = 'D';
+		display_step->substepping = '1';
+		break;
+	case STEP_E0:
+		display_step->stepping = 'E';
+		display_step->substepping = '0';
+		break;
+	case STEP_F0:
+		display_step->stepping = 'F';
+		display_step->substepping = '0';
+		break;
+	case STEP_G0:
+		display_step->stepping = 'G';
+		display_step->substepping = '0';
+		break;
+	case STEP_H0:
+		display_step->stepping = 'H';
+		display_step->substepping = '0';
+		break;
+	case STEP_I0:
+		display_step->stepping = 'I';
+		display_step->substepping = '0';
+		break;
+	case STEP_J0:
+		display_step->stepping = 'J';
+		display_step->substepping = '0';
+		break;
+	case STEP_K0:
+		display_step->stepping = 'K';
+		display_step->substepping = '0';
+		break;
+	default:
+		display_step->stepping = '*';
+		display_step->substepping = '*';
+		break;
 	}
-
-	if (INTEL_REVID(dev_priv) < size)
-		return si + INTEL_REVID(dev_priv);
-
-	return &no_stepping_info;
+	return display_step;
 }
 
 static void gen9_set_dc_state_debugmask(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/intel_step.c b/drivers/gpu/drm/i915/intel_step.c
index ba9479a67521..c8542161c5d0 100644
--- a/drivers/gpu/drm/i915/intel_step.c
+++ b/drivers/gpu/drm/i915/intel_step.c
@@ -15,6 +15,38 @@ 
 
 
 /* FIXME: what about REVID_E0 */
+static const struct intel_step_info bxt_revid_step_tbl[] = {
+	[0] = { .gt_step = STEP_A0, .display_step = STEP_A0 },
+	[1] = { .gt_step = STEP_A1, .display_step = STEP_A1 },
+	[2] = { .gt_step = STEP_A2, .display_step = STEP_A2 },
+	[6] = { .gt_step = STEP_B0, .display_step = STEP_B0 },
+	[7] = { .gt_step = STEP_B1, .display_step = STEP_B1 },
+	[8] = { .gt_step = STEP_B2, .display_step = STEP_B2 },
+};
+
+
+static const struct intel_step_info skl_revid_step_tbl[] = {
+	[0] = { .gt_step = STEP_A0, .display_step = STEP_A0 },
+	[1] = { .gt_step = STEP_B0, .display_step = STEP_B0 },
+	[2] = { .gt_step = STEP_C0, .display_step = STEP_C0 },
+	[3] = { .gt_step = STEP_D0, .display_step = STEP_D0 },
+	[4] = { .gt_step = STEP_E0, .display_step = STEP_E0 },
+	[5] = { .gt_step = STEP_F0, .display_step = STEP_F0 },
+	[6] = { .gt_step = STEP_G0, .display_step = STEP_G0 },
+	[7] = { .gt_step = STEP_H0, .display_step = STEP_H0 },
+	[8] = { .gt_step = STEP_I0, .display_step = STEP_I0 },
+	[9] = { .gt_step = STEP_J0, .display_step = STEP_J0 },
+};
+
+static const struct intel_step_info icl_revid_step_tbl[] = {
+	[0] = { .gt_step = STEP_A0, .display_step = STEP_A0 },
+	[3] = { .gt_step = STEP_B0, .display_step = STEP_B0 },
+	[4] = { .gt_step = STEP_B2, .display_step = STEP_B2 },
+	[5] = { .gt_step = STEP_C0, .display_step = STEP_C0 },
+	[6] = { .gt_step = STEP_C1, .display_step = STEP_C1 },
+	[7] = { .gt_step = STEP_D0, .display_step = STEP_D0 },
+};
+
 static const struct intel_step_info kbl_revids[] = {
 	[0] = { .gt_step = STEP_A0, .display_step = STEP_A0 },
 	[1] = { .gt_step = STEP_B0, .display_step = STEP_B0 },
@@ -76,6 +108,15 @@  void intel_step_init(struct drm_i915_private *i915)
 	} else if (IS_KABYLAKE(i915)) {
 		revids = kbl_revids;
 		size = ARRAY_SIZE(kbl_revids);
+	} else if (IS_ICELAKE(i915)) {
+		revids = icl_revid_step_tbl;
+		size = ARRAY_SIZE(icl_revid_step_tbl);
+	} else if (IS_SKYLAKE(i915)) {
+		revids = skl_revid_step_tbl;
+		size = ARRAY_SIZE(skl_revid_step_tbl);
+	} else if (IS_BROXTON(i915)) {
+		revids = bxt_revid_step_tbl;
+		size = ARRAY_SIZE(bxt_revid_step_tbl);
 	}
 
 	/* Not using the stepping scheme for the platform yet. */
diff --git a/drivers/gpu/drm/i915/intel_step.h b/drivers/gpu/drm/i915/intel_step.h
index 958a8bb5d677..1bffa41f2660 100644
--- a/drivers/gpu/drm/i915/intel_step.h
+++ b/drivers/gpu/drm/i915/intel_step.h
@@ -22,15 +22,22 @@  struct intel_step_info {
 enum intel_step {
 	STEP_NONE = 0,
 	STEP_A0,
+	STEP_A1,
 	STEP_A2,
 	STEP_B0,
 	STEP_B1,
+	STEP_B2,
 	STEP_C0,
+	STEP_C1,
 	STEP_D0,
 	STEP_D1,
 	STEP_E0,
 	STEP_F0,
 	STEP_G0,
+	STEP_H0,
+	STEP_I0,
+	STEP_J0,
+	STEP_K0,
 	STEP_FUTURE,
 	STEP_FOREVER,
 };