| Message ID | 20210630230624.25407-1-anusha.srivatsa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dmc: Use RUNTIME_INFO->stp for DMC | expand |
Typo: RUNTIME_INFO->stp On Wed, Jun 30, 2021 at 04:06:24PM -0700, Anusha Srivatsa 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 >Adding the helper intel_get_display_step(). > >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 | 49 ++++++++++++++++++++++-- > 1 file changed, 45 insertions(+), 4 deletions(-) > >diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c >index f8789d4543bf..c7ff7ff3f412 100644 >--- a/drivers/gpu/drm/i915/display/intel_dmc.c >+++ b/drivers/gpu/drm/i915/display/intel_dmc.c >@@ -266,14 +266,55 @@ static const struct stepping_info icl_stepping_info[] = { > }; > > static const struct stepping_info no_stepping_info = { '*', '*' }; >+struct stepping_info *display_step; >+ >+static struct stepping_info * >+intel_get_display_stepping(struct intel_step_info step) >+{ >+ >+ switch (step.display_step) { >+ case STEP_A0: >+ display_step->stepping = 'A'; >+ display_step->substepping = '0'; >+ 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_C0: >+ display_step->stepping = 'C'; >+ display_step->substepping = '0'; >+ break; >+ case STEP_D0: >+ display_step->stepping = 'D'; >+ display_step->substepping = '0'; >+ break; >+ default: >+ display_step->stepping = '*'; >+ display_step->substepping = '*'; >+ break; >+ } "crazy" idea that would avoid this type of conversion: changing the step enum to be: #define make_step(letter, num) (int)(((letter) << 8 ) | (num)) STEP_A0 = make_step('A', '0'), STEP_A1 = make_step('A', '1'), and adapt the rest of the code to play with u16 instead of u8, and handle the STEP_FUTURE/STEP_NONE/STEP_FOREVER. Maybe it is crazy, dunno. +Jani / +Jose. Thoughts? For this version the next comment is probably more important. >+ return display_step; >+} > > static const struct stepping_info * > intel_get_stepping_info(struct drm_i915_private *dev_priv) > { > const struct stepping_info *si; >+ struct intel_step_info step = RUNTIME_INFO(dev_priv)->step; > unsigned int size; > >- if (IS_ICELAKE(dev_priv)) { >+ if (DISPLAY_VER(dev_priv) >= 12) { >+ si = intel_get_display_stepping(step); >+ } else if (IS_ICELAKE(dev_priv)) { > size = ARRAY_SIZE(icl_stepping_info); > si = icl_stepping_info; can we move the other ones too? Just use display_step for all platforms. Notice that before the separation we will have display_step == graphics_step, so it should just work. Lucas De Marchi > } else if (IS_SKYLAKE(dev_priv)) { >@@ -287,10 +328,10 @@ intel_get_stepping_info(struct drm_i915_private *dev_priv) > si = NULL; > } > >- if (INTEL_REVID(dev_priv) < size) >- return si + INTEL_REVID(dev_priv); >+ if (DISPLAY_VER(dev_priv) < 12) >+ return INTEL_REVID(dev_priv) < size ? si + INTEL_REVID(dev_priv) : &no_stepping_info; > >- return &no_stepping_info; >+ return si; > } > > static void gen9_set_dc_state_debugmask(struct drm_i915_private *dev_priv) >-- >2.32.0 >
On Wed, 2021-06-30 at 17:01 -0700, Lucas De Marchi wrote: > Typo: RUNTIME_INFO->stp > > On Wed, Jun 30, 2021 at 04:06:24PM -0700, Anusha Srivatsa 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 > > Adding the helper intel_get_display_step(). > > > > 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 | 49 ++++++++++++++++++++++-- > > 1 file changed, 45 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c > > index f8789d4543bf..c7ff7ff3f412 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dmc.c > > +++ b/drivers/gpu/drm/i915/display/intel_dmc.c > > @@ -266,14 +266,55 @@ static const struct stepping_info icl_stepping_info[] = { > > }; > > > > static const struct stepping_info no_stepping_info = { '*', '*' }; > > +struct stepping_info *display_step; > > + > > +static struct stepping_info * > > +intel_get_display_stepping(struct intel_step_info step) > > +{ > > + > > + switch (step.display_step) { > > + case STEP_A0: > > + display_step->stepping = 'A'; > > + display_step->substepping = '0'; > > + 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_C0: > > + display_step->stepping = 'C'; > > + display_step->substepping = '0'; > > + break; > > + case STEP_D0: > > + display_step->stepping = 'D'; > > + display_step->substepping = '0'; > > + break; > > + default: > > + display_step->stepping = '*'; > > + display_step->substepping = '*'; > > + break; > > + } > > > "crazy" idea that would avoid this type of conversion: > changing the step enum to be: > > > #define make_step(letter, num) (int)(((letter) << 8 ) | (num)) > > STEP_A0 = make_step('A', '0'), > STEP_A1 = make_step('A', '1'), Looks a good idea to me, we could also keep it u8 using 4bits for each if there is memory concerns. > > and adapt the rest of the code to play with u16 instead of u8, and > handle the STEP_FUTURE/STEP_NONE/STEP_FOREVER. > Maybe it is crazy, dunno. > > +Jani / +Jose. Thoughts? > > > For this version the next comment is probably more important. > > > + return display_step; > > +} > > > > static const struct stepping_info * > > intel_get_stepping_info(struct drm_i915_private *dev_priv) > > { > > const struct stepping_info *si; > > + struct intel_step_info step = RUNTIME_INFO(dev_priv)->step; > > unsigned int size; > > > > - if (IS_ICELAKE(dev_priv)) { > > + if (DISPLAY_VER(dev_priv) >= 12) { > > + si = intel_get_display_stepping(step); > > + } else if (IS_ICELAKE(dev_priv)) { > > size = ARRAY_SIZE(icl_stepping_info); > > si = icl_stepping_info; > > can we move the other ones too? Just use display_step for all platforms. > Notice that before the separation we will have display_step == > graphics_step, so it should just work. > > > Lucas De Marchi > > > } else if (IS_SKYLAKE(dev_priv)) { > > @@ -287,10 +328,10 @@ intel_get_stepping_info(struct drm_i915_private *dev_priv) > > si = NULL; > > } > > > > - if (INTEL_REVID(dev_priv) < size) > > - return si + INTEL_REVID(dev_priv); > > + if (DISPLAY_VER(dev_priv) < 12) > > + return INTEL_REVID(dev_priv) < size ? si + INTEL_REVID(dev_priv) : &no_stepping_info; > > > > - return &no_stepping_info; > > + return si; > > } > > > > static void gen9_set_dc_state_debugmask(struct drm_i915_private *dev_priv) > > -- > > 2.32.0 > >
On Wed, Jun 30, 2021 at 05:15:00PM -0700, Jose Souza wrote: >On Wed, 2021-06-30 at 17:01 -0700, Lucas De Marchi wrote: >> Typo: RUNTIME_INFO->stp >> >> On Wed, Jun 30, 2021 at 04:06:24PM -0700, Anusha Srivatsa 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 >> > Adding the helper intel_get_display_step(). >> > >> > 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 | 49 ++++++++++++++++++++++-- >> > 1 file changed, 45 insertions(+), 4 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c >> > index f8789d4543bf..c7ff7ff3f412 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_dmc.c >> > +++ b/drivers/gpu/drm/i915/display/intel_dmc.c >> > @@ -266,14 +266,55 @@ static const struct stepping_info icl_stepping_info[] = { >> > }; >> > >> > static const struct stepping_info no_stepping_info = { '*', '*' }; >> > +struct stepping_info *display_step; >> > + >> > +static struct stepping_info * >> > +intel_get_display_stepping(struct intel_step_info step) >> > +{ >> > + >> > + switch (step.display_step) { >> > + case STEP_A0: >> > + display_step->stepping = 'A'; >> > + display_step->substepping = '0'; >> > + 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_C0: >> > + display_step->stepping = 'C'; >> > + display_step->substepping = '0'; >> > + break; >> > + case STEP_D0: >> > + display_step->stepping = 'D'; >> > + display_step->substepping = '0'; >> > + break; >> > + default: >> > + display_step->stepping = '*'; >> > + display_step->substepping = '*'; >> > + break; >> > + } >> >> >> "crazy" idea that would avoid this type of conversion: >> changing the step enum to be: >> >> >> #define make_step(letter, num) (int)(((letter) << 8 ) | (num)) >> >> STEP_A0 = make_step('A', '0'), >> STEP_A1 = make_step('A', '1'), > >Looks a good idea to me, we could also keep it u8 using 4bits for each if there is memory concerns. humn... indeed. Not much a concern actually, but not having to change additional code is already a good thing. I hope no stepping goes beyond Z9 :) Lucas De Marchi > >> >> and adapt the rest of the code to play with u16 instead of u8, and >> handle the STEP_FUTURE/STEP_NONE/STEP_FOREVER. >> Maybe it is crazy, dunno. >> >> +Jani / +Jose. Thoughts? >> >> >> For this version the next comment is probably more important. >> >> > + return display_step; >> > +} >> > >> > static const struct stepping_info * >> > intel_get_stepping_info(struct drm_i915_private *dev_priv) >> > { >> > const struct stepping_info *si; >> > + struct intel_step_info step = RUNTIME_INFO(dev_priv)->step; >> > unsigned int size; >> > >> > - if (IS_ICELAKE(dev_priv)) { >> > + if (DISPLAY_VER(dev_priv) >= 12) { >> > + si = intel_get_display_stepping(step); >> > + } else if (IS_ICELAKE(dev_priv)) { >> > size = ARRAY_SIZE(icl_stepping_info); >> > si = icl_stepping_info; >> >> can we move the other ones too? Just use display_step for all platforms. >> Notice that before the separation we will have display_step == >> graphics_step, so it should just work. >> >> >> Lucas De Marchi >> >> > } else if (IS_SKYLAKE(dev_priv)) { >> > @@ -287,10 +328,10 @@ intel_get_stepping_info(struct drm_i915_private *dev_priv) >> > si = NULL; >> > } >> > >> > - if (INTEL_REVID(dev_priv) < size) >> > - return si + INTEL_REVID(dev_priv); >> > + if (DISPLAY_VER(dev_priv) < 12) >> > + return INTEL_REVID(dev_priv) < size ? si + INTEL_REVID(dev_priv) : &no_stepping_info; >> > >> > - return &no_stepping_info; >> > + return si; >> > } >> > >> > static void gen9_set_dc_state_debugmask(struct drm_i915_private *dev_priv) >> > -- >> > 2.32.0 >> > >
On Wed, Jun 30, 2021 at 05:17:52PM -0700, Lucas De Marchi wrote: >On Wed, Jun 30, 2021 at 05:15:00PM -0700, Jose Souza wrote: >>On Wed, 2021-06-30 at 17:01 -0700, Lucas De Marchi wrote: >>>Typo: RUNTIME_INFO->stp >>> >>>On Wed, Jun 30, 2021 at 04:06:24PM -0700, Anusha Srivatsa 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 >>>> Adding the helper intel_get_display_step(). >>>> >>>> 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 | 49 ++++++++++++++++++++++-- >>>> 1 file changed, 45 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c >>>> index f8789d4543bf..c7ff7ff3f412 100644 >>>> --- a/drivers/gpu/drm/i915/display/intel_dmc.c >>>> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c >>>> @@ -266,14 +266,55 @@ static const struct stepping_info icl_stepping_info[] = { >>>> }; >>>> >>>> static const struct stepping_info no_stepping_info = { '*', '*' }; >>>> +struct stepping_info *display_step; >>>> + >>>> +static struct stepping_info * >>>> +intel_get_display_stepping(struct intel_step_info step) >>>> +{ >>>> + >>>> + switch (step.display_step) { >>>> + case STEP_A0: >>>> + display_step->stepping = 'A'; >>>> + display_step->substepping = '0'; >>>> + 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_C0: >>>> + display_step->stepping = 'C'; >>>> + display_step->substepping = '0'; >>>> + break; >>>> + case STEP_D0: >>>> + display_step->stepping = 'D'; >>>> + display_step->substepping = '0'; >>>> + break; >>>> + default: >>>> + display_step->stepping = '*'; >>>> + display_step->substepping = '*'; >>>> + break; >>>> + } >>> >>> >>>"crazy" idea that would avoid this type of conversion: >>>changing the step enum to be: >>> >>> >>>#define make_step(letter, num) (int)(((letter) << 8 ) | (num)) >>> >>>STEP_A0 = make_step('A', '0'), >>>STEP_A1 = make_step('A', '1'), >> >>Looks a good idea to me, we could also keep it u8 using 4bits for each if there is memory concerns. > >humn... indeed. Not much a concern actually, but not having to change >additional code is already a good thing. > > >I hope no stepping goes beyond Z9 :) I take that out. Even if we would do (number - 'A'), there is not enough room in 4bits to handle until Z. In fact we could go only until P8 - so if we are going that route we will need to have some extra build checks that we don't go beyond that. Lucas De Marchi
On Wed, 30 Jun 2021, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > Typo: RUNTIME_INFO->stp > > On Wed, Jun 30, 2021 at 04:06:24PM -0700, Anusha Srivatsa 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 >>Adding the helper intel_get_display_step(). >> >>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 | 49 ++++++++++++++++++++++-- >> 1 file changed, 45 insertions(+), 4 deletions(-) >> >>diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c >>index f8789d4543bf..c7ff7ff3f412 100644 >>--- a/drivers/gpu/drm/i915/display/intel_dmc.c >>+++ b/drivers/gpu/drm/i915/display/intel_dmc.c >>@@ -266,14 +266,55 @@ static const struct stepping_info icl_stepping_info[] = { >> }; >> >> static const struct stepping_info no_stepping_info = { '*', '*' }; >>+struct stepping_info *display_step; >>+ >>+static struct stepping_info * >>+intel_get_display_stepping(struct intel_step_info step) >>+{ >>+ >>+ switch (step.display_step) { >>+ case STEP_A0: >>+ display_step->stepping = 'A'; >>+ display_step->substepping = '0'; >>+ 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_C0: >>+ display_step->stepping = 'C'; >>+ display_step->substepping = '0'; >>+ break; >>+ case STEP_D0: >>+ display_step->stepping = 'D'; >>+ display_step->substepping = '0'; >>+ break; >>+ default: >>+ display_step->stepping = '*'; >>+ display_step->substepping = '*'; >>+ break; >>+ } > > > "crazy" idea that would avoid this type of conversion: > changing the step enum to be: > > > #define make_step(letter, num) (int)(((letter) << 8 ) | (num)) > > STEP_A0 = make_step('A', '0'), > STEP_A1 = make_step('A', '1'), > > and adapt the rest of the code to play with u16 instead of u8, and > handle the STEP_FUTURE/STEP_NONE/STEP_FOREVER. > Maybe it is crazy, dunno. > > +Jani / +Jose. Thoughts? Frankly, I think all of this should be incorporated to intel_step.[ch] instead of having a semi-overlapping handling here. Just look at the amount of duplication already. BR, Jani. > > > For this version the next comment is probably more important. > >>+ return display_step; >>+} >> >> static const struct stepping_info * >> intel_get_stepping_info(struct drm_i915_private *dev_priv) >> { >> const struct stepping_info *si; >>+ struct intel_step_info step = RUNTIME_INFO(dev_priv)->step; >> unsigned int size; >> >>- if (IS_ICELAKE(dev_priv)) { >>+ if (DISPLAY_VER(dev_priv) >= 12) { >>+ si = intel_get_display_stepping(step); >>+ } else if (IS_ICELAKE(dev_priv)) { >> size = ARRAY_SIZE(icl_stepping_info); >> si = icl_stepping_info; > > can we move the other ones too? Just use display_step for all platforms. > Notice that before the separation we will have display_step == > graphics_step, so it should just work. > > > Lucas De Marchi > >> } else if (IS_SKYLAKE(dev_priv)) { >>@@ -287,10 +328,10 @@ intel_get_stepping_info(struct drm_i915_private *dev_priv) >> si = NULL; >> } >> >>- if (INTEL_REVID(dev_priv) < size) >>- return si + INTEL_REVID(dev_priv); >>+ if (DISPLAY_VER(dev_priv) < 12) >>+ return INTEL_REVID(dev_priv) < size ? si + INTEL_REVID(dev_priv) : &no_stepping_info; >> >>- return &no_stepping_info; >>+ return si; >> } >> >> static void gen9_set_dc_state_debugmask(struct drm_i915_private *dev_priv) >>-- >>2.32.0 >>
On Fri, Jul 02, 2021 at 10:49:05AM +0300, Jani Nikula wrote: >On Wed, 30 Jun 2021, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >> Typo: RUNTIME_INFO->stp >> >> On Wed, Jun 30, 2021 at 04:06:24PM -0700, Anusha Srivatsa 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 >>>Adding the helper intel_get_display_step(). >>> >>>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 | 49 ++++++++++++++++++++++-- >>> 1 file changed, 45 insertions(+), 4 deletions(-) >>> >>>diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c >>>index f8789d4543bf..c7ff7ff3f412 100644 >>>--- a/drivers/gpu/drm/i915/display/intel_dmc.c >>>+++ b/drivers/gpu/drm/i915/display/intel_dmc.c >>>@@ -266,14 +266,55 @@ static const struct stepping_info icl_stepping_info[] = { >>> }; >>> >>> static const struct stepping_info no_stepping_info = { '*', '*' }; >>>+struct stepping_info *display_step; >>>+ >>>+static struct stepping_info * >>>+intel_get_display_stepping(struct intel_step_info step) >>>+{ >>>+ >>>+ switch (step.display_step) { >>>+ case STEP_A0: >>>+ display_step->stepping = 'A'; >>>+ display_step->substepping = '0'; >>>+ 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_C0: >>>+ display_step->stepping = 'C'; >>>+ display_step->substepping = '0'; >>>+ break; >>>+ case STEP_D0: >>>+ display_step->stepping = 'D'; >>>+ display_step->substepping = '0'; >>>+ break; >>>+ default: >>>+ display_step->stepping = '*'; >>>+ display_step->substepping = '*'; >>>+ break; >>>+ } >> >> >> "crazy" idea that would avoid this type of conversion: >> changing the step enum to be: >> >> >> #define make_step(letter, num) (int)(((letter) << 8 ) | (num)) >> >> STEP_A0 = make_step('A', '0'), >> STEP_A1 = make_step('A', '1'), >> >> and adapt the rest of the code to play with u16 instead of u8, and >> handle the STEP_FUTURE/STEP_NONE/STEP_FOREVER. >> Maybe it is crazy, dunno. >> >> +Jani / +Jose. Thoughts? > >Frankly, I think all of this should be incorporated to intel_step.[ch] >instead of having a semi-overlapping handling here. Just look at the >amount of duplication already. I guess you missed I gave the same suggestion below and that there is already a new version of this patch? Question here was about going a step further to avoid the conversion STEP_* to 2 chars. thanks Lucas De Marchi > > >BR, >Jani. > >> >> >> For this version the next comment is probably more important. >> >>>+ return display_step; >>>+} >>> >>> static const struct stepping_info * >>> intel_get_stepping_info(struct drm_i915_private *dev_priv) >>> { >>> const struct stepping_info *si; >>>+ struct intel_step_info step = RUNTIME_INFO(dev_priv)->step; >>> unsigned int size; >>> >>>- if (IS_ICELAKE(dev_priv)) { >>>+ if (DISPLAY_VER(dev_priv) >= 12) { >>>+ si = intel_get_display_stepping(step); >>>+ } else if (IS_ICELAKE(dev_priv)) { >>> size = ARRAY_SIZE(icl_stepping_info); >>> si = icl_stepping_info; >> >> can we move the other ones too? Just use display_step for all platforms. >> Notice that before the separation we will have display_step == >> graphics_step, so it should just work. >> >> >> Lucas De Marchi >> >>> } else if (IS_SKYLAKE(dev_priv)) { >>>@@ -287,10 +328,10 @@ intel_get_stepping_info(struct drm_i915_private *dev_priv) >>> si = NULL; >>> } >>> >>>- if (INTEL_REVID(dev_priv) < size) >>>- return si + INTEL_REVID(dev_priv); >>>+ if (DISPLAY_VER(dev_priv) < 12) >>>+ return INTEL_REVID(dev_priv) < size ? si + INTEL_REVID(dev_priv) : &no_stepping_info; >>> >>>- return &no_stepping_info; >>>+ return si; >>> } >>> >>> static void gen9_set_dc_state_debugmask(struct drm_i915_private *dev_priv) >>>-- >>>2.32.0 >>> > >-- >Jani Nikula, Intel Open Source Graphics Center
On Sat, 03 Jul 2021, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > On Fri, Jul 02, 2021 at 10:49:05AM +0300, Jani Nikula wrote: >>Frankly, I think all of this should be incorporated to intel_step.[ch] >>instead of having a semi-overlapping handling here. Just look at the >>amount of duplication already. > > I guess you missed I gave the same suggestion below and that there is > already a new version of this patch? Question here was about going a > step further to avoid the conversion STEP_* to 2 chars. Sorry for the noise. BR, Jani.
diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c index f8789d4543bf..c7ff7ff3f412 100644 --- a/drivers/gpu/drm/i915/display/intel_dmc.c +++ b/drivers/gpu/drm/i915/display/intel_dmc.c @@ -266,14 +266,55 @@ static const struct stepping_info icl_stepping_info[] = { }; static const struct stepping_info no_stepping_info = { '*', '*' }; +struct stepping_info *display_step; + +static struct stepping_info * +intel_get_display_stepping(struct intel_step_info step) +{ + + switch (step.display_step) { + case STEP_A0: + display_step->stepping = 'A'; + display_step->substepping = '0'; + 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_C0: + display_step->stepping = 'C'; + display_step->substepping = '0'; + break; + case STEP_D0: + display_step->stepping = 'D'; + display_step->substepping = '0'; + break; + default: + display_step->stepping = '*'; + display_step->substepping = '*'; + break; + } + return display_step; +} static const struct stepping_info * intel_get_stepping_info(struct drm_i915_private *dev_priv) { const struct stepping_info *si; + struct intel_step_info step = RUNTIME_INFO(dev_priv)->step; unsigned int size; - if (IS_ICELAKE(dev_priv)) { + if (DISPLAY_VER(dev_priv) >= 12) { + si = intel_get_display_stepping(step); + } else if (IS_ICELAKE(dev_priv)) { size = ARRAY_SIZE(icl_stepping_info); si = icl_stepping_info; } else if (IS_SKYLAKE(dev_priv)) { @@ -287,10 +328,10 @@ intel_get_stepping_info(struct drm_i915_private *dev_priv) si = NULL; } - if (INTEL_REVID(dev_priv) < size) - return si + INTEL_REVID(dev_priv); + if (DISPLAY_VER(dev_priv) < 12) + return INTEL_REVID(dev_priv) < size ? si + INTEL_REVID(dev_priv) : &no_stepping_info; - return &no_stepping_info; + return si; } static void gen9_set_dc_state_debugmask(struct drm_i915_private *dev_priv)
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 Adding the helper intel_get_display_step(). 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 | 49 ++++++++++++++++++++++-- 1 file changed, 45 insertions(+), 4 deletions(-)