| Message ID | 20210719213828.5738-1-anusha.srivatsa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [CI,1/2] drm/i915/step: Add macro magic for handling steps | expand |
On Mon, 19 Jul 2021, Anusha Srivatsa <anusha.srivatsa@intel.com> wrote: > With the addition of stepping info for > all platforms, lets use macros for handling them > and autogenerating code for all steps at a time. > > Suggested-by: Matt Roper <matthew.d.roper@intel.com> > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> > --- > drivers/gpu/drm/i915/intel_step.c | 14 ++++++++++++ > drivers/gpu/drm/i915/intel_step.h | 37 +++++++++++++++++++------------ > 2 files changed, 37 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_step.c b/drivers/gpu/drm/i915/intel_step.c > index 9fcf17708cc8..d150d138e889 100644 > --- a/drivers/gpu/drm/i915/intel_step.c > +++ b/drivers/gpu/drm/i915/intel_step.c > @@ -182,3 +182,17 @@ void intel_step_init(struct drm_i915_private *i915) > > RUNTIME_INFO(i915)->step = step; > } > + > +#define STEP_NAME_CASE(name) \ > + (case STEP_##name: \ > + return #name) > + > +const char *intel_step_name(enum intel_step step) > +{ > + switch (step) { > + STEP_NAME_LIST(STEP_NAME_CASE); > + > + default: > + return "**"; > + } > +} > diff --git a/drivers/gpu/drm/i915/intel_step.h b/drivers/gpu/drm/i915/intel_step.h > index 88a77159703e..f6641e2a3c77 100644 > --- a/drivers/gpu/drm/i915/intel_step.h > +++ b/drivers/gpu/drm/i915/intel_step.h > @@ -15,30 +15,39 @@ struct intel_step_info { > u8 display_step; > }; > > +#define STEP_ENUM_VAL(name) STEP_##name, > + > +#define STEP_NAME_LIST(func) \ > + func(A0) \ > + func(A1) \ > + func(A2) \ > + func(B0) \ > + func(B1) \ > + func(B2) \ > + func(C0) \ > + func(C1) \ > + func(D0) \ > + func(D1) \ > + func(E0) \ > + func(F0) \ > + func(G0) \ > + func(H0) \ > + func(I0) \ > + func(I1) \ > + func(J0) > + > /* > * Symbolic steppings that do not match the hardware. These are valid both as gt > * and display steppings as symbolic names. > */ > enum intel_step { > STEP_NONE = 0, > - STEP_A0, > - STEP_A2, > - STEP_B0, > - STEP_B1, > - STEP_C0, > - STEP_D0, > - STEP_D1, > - STEP_E0, > - STEP_F0, > - STEP_G0, > - STEP_H0, > - STEP_I0, > - STEP_I1, > - STEP_J0, > + STEP_NAME_LIST(STEP_ENUM_VAL) > STEP_FUTURE, > STEP_FOREVER, > }; It's been merged already, and so be it. I'll just note that the main reason I don't like adding this kind of macro abstractions is that it throws off source code tagging tools like ctags and gnu global. I can no longer navigate the definition and all the references of STEP_* using the tools, and it degenerates to git grep which is much worse as it doesn't know about C syntax or semantics. It's not always about how the code looks, it's also about how people can navigate the code base. BR, Jani. > > void intel_step_init(struct drm_i915_private *i915); > +const char *intel_step_name(enum intel_step step); > > #endif /* __INTEL_STEP_H__ */
> -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: Thursday, August 12, 2021 2:59 AM > To: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Roper, Matthew D <matthew.d.roper@intel.com>; De Marchi, Lucas > <lucas.demarchi@intel.com> > Subject: Re: [Intel-gfx] [CI 1/2] drm/i915/step: Add macro magic for handling > steps > > On Mon, 19 Jul 2021, Anusha Srivatsa <anusha.srivatsa@intel.com> wrote: > > With the addition of stepping info for all platforms, lets use macros > > for handling them and autogenerating code for all steps at a time. > > > > Suggested-by: Matt Roper <matthew.d.roper@intel.com> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > > Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> > > --- > > drivers/gpu/drm/i915/intel_step.c | 14 ++++++++++++ > > drivers/gpu/drm/i915/intel_step.h | 37 +++++++++++++++++++------------ > > 2 files changed, 37 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_step.c > > b/drivers/gpu/drm/i915/intel_step.c > > index 9fcf17708cc8..d150d138e889 100644 > > --- a/drivers/gpu/drm/i915/intel_step.c > > +++ b/drivers/gpu/drm/i915/intel_step.c > > @@ -182,3 +182,17 @@ void intel_step_init(struct drm_i915_private > > *i915) > > > > RUNTIME_INFO(i915)->step = step; > > } > > + > > +#define STEP_NAME_CASE(name) \ > > + (case STEP_##name: \ > > + return #name) > > + > > +const char *intel_step_name(enum intel_step step) { > > + switch (step) { > > + STEP_NAME_LIST(STEP_NAME_CASE); > > + > > + default: > > + return "**"; > > + } > > +} > > diff --git a/drivers/gpu/drm/i915/intel_step.h > > b/drivers/gpu/drm/i915/intel_step.h > > index 88a77159703e..f6641e2a3c77 100644 > > --- a/drivers/gpu/drm/i915/intel_step.h > > +++ b/drivers/gpu/drm/i915/intel_step.h > > @@ -15,30 +15,39 @@ struct intel_step_info { > > u8 display_step; > > }; > > > > +#define STEP_ENUM_VAL(name) STEP_##name, > > + > > +#define STEP_NAME_LIST(func) \ > > + func(A0) \ > > + func(A1) \ > > + func(A2) \ > > + func(B0) \ > > + func(B1) \ > > + func(B2) \ > > + func(C0) \ > > + func(C1) \ > > + func(D0) \ > > + func(D1) \ > > + func(E0) \ > > + func(F0) \ > > + func(G0) \ > > + func(H0) \ > > + func(I0) \ > > + func(I1) \ > > + func(J0) > > + > > /* > > * Symbolic steppings that do not match the hardware. These are valid > both as gt > > * and display steppings as symbolic names. > > */ > > enum intel_step { > > STEP_NONE = 0, > > - STEP_A0, > > - STEP_A2, > > - STEP_B0, > > - STEP_B1, > > - STEP_C0, > > - STEP_D0, > > - STEP_D1, > > - STEP_E0, > > - STEP_F0, > > - STEP_G0, > > - STEP_H0, > > - STEP_I0, > > - STEP_I1, > > - STEP_J0, > > + STEP_NAME_LIST(STEP_ENUM_VAL) > > STEP_FUTURE, > > STEP_FOREVER, > > }; > > It's been merged already, and so be it. > > I'll just note that the main reason I don't like adding this kind of macro > abstractions is that it throws off source code tagging tools like ctags and gnu > global. I can no longer navigate the definition and all the references of > STEP_* using the tools, and it degenerates to git grep which is much worse as > it doesn't know about C syntax or semantics. It's not always about how the > code looks, it's also about how people can navigate the code base. Had not considered that :-/ will keep this in mind from now on. Thanks for the feedback. Anusha > BR, > Jani. > > > > > > void intel_step_init(struct drm_i915_private *i915); > > +const char *intel_step_name(enum intel_step step); > > > > #endif /* __INTEL_STEP_H__ */ > > -- > Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/intel_step.c b/drivers/gpu/drm/i915/intel_step.c index 9fcf17708cc8..d150d138e889 100644 --- a/drivers/gpu/drm/i915/intel_step.c +++ b/drivers/gpu/drm/i915/intel_step.c @@ -182,3 +182,17 @@ void intel_step_init(struct drm_i915_private *i915) RUNTIME_INFO(i915)->step = step; } + +#define STEP_NAME_CASE(name) \ + (case STEP_##name: \ + return #name) + +const char *intel_step_name(enum intel_step step) +{ + switch (step) { + STEP_NAME_LIST(STEP_NAME_CASE); + + default: + return "**"; + } +} diff --git a/drivers/gpu/drm/i915/intel_step.h b/drivers/gpu/drm/i915/intel_step.h index 88a77159703e..f6641e2a3c77 100644 --- a/drivers/gpu/drm/i915/intel_step.h +++ b/drivers/gpu/drm/i915/intel_step.h @@ -15,30 +15,39 @@ struct intel_step_info { u8 display_step; }; +#define STEP_ENUM_VAL(name) STEP_##name, + +#define STEP_NAME_LIST(func) \ + func(A0) \ + func(A1) \ + func(A2) \ + func(B0) \ + func(B1) \ + func(B2) \ + func(C0) \ + func(C1) \ + func(D0) \ + func(D1) \ + func(E0) \ + func(F0) \ + func(G0) \ + func(H0) \ + func(I0) \ + func(I1) \ + func(J0) + /* * Symbolic steppings that do not match the hardware. These are valid both as gt * and display steppings as symbolic names. */ enum intel_step { STEP_NONE = 0, - STEP_A0, - STEP_A2, - STEP_B0, - STEP_B1, - STEP_C0, - STEP_D0, - STEP_D1, - STEP_E0, - STEP_F0, - STEP_G0, - STEP_H0, - STEP_I0, - STEP_I1, - STEP_J0, + STEP_NAME_LIST(STEP_ENUM_VAL) STEP_FUTURE, STEP_FOREVER, }; void intel_step_init(struct drm_i915_private *i915); +const char *intel_step_name(enum intel_step step); #endif /* __INTEL_STEP_H__ */