| Message ID | 20181101083517.20193-5-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make GEN macros more similar | expand |
On 01/11/2018 08:35, Lucas De Marchi wrote: > This will allow us to make all gen comparisons simple gen comparisons > with the same kind of macro rather than resorting to a mix of IS_GEN(), > INTEL_GEN() >=, >, <=, <. > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 8d22353a3da6..eb636dfe5228 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2370,6 +2370,11 @@ intel_info(const struct drm_i915_private *dev_priv) > #define IS_GEN(dev_priv, ...) \ > CONCATENATE(_IS_GEN_ARG, COUNT_ARGS(__VA_ARGS__))((dev_priv), ##__VA_ARGS__) > > +#define IS_GEN_GT(dev_priv, g) (g > (dev_priv)->info.gen) Interesting :), I have at some point floated a similar patch and there was overwhelming dislike of the GT suffix, since we have GT2 GT3 etc.. as established acronyms for GPU configurations. Furthermore, I am pretty sure I've done it using the gen bitmask so hypothetical checks like IS_GEN_LT(5) || IS_GEN_RANGE(8, 9) could still be compile time optimized to a single conditional. That would also now become more difficult due removal of GEN_FOREVER. To summarise, I don't see a advantage of hiding this in macros if we are not getting the above benefit. Regards, Tvrtko > +#define IS_GEN_GE(dev_priv, g) (g >= (dev_priv)->info.gen) > +#define IS_GEN_LT(dev_priv, g) (g < (dev_priv)->info.gen) > +#define IS_GEN_LE(dev_priv, g) (g <= (dev_priv)->info.gen) > + > /* > * Return true if revision is in range [since,until] inclusive. > * >
On Thu, 01 Nov 2018, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > On 01/11/2018 08:35, Lucas De Marchi wrote: >> This will allow us to make all gen comparisons simple gen comparisons >> with the same kind of macro rather than resorting to a mix of IS_GEN(), >> INTEL_GEN() >=, >, <=, <. >> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 8d22353a3da6..eb636dfe5228 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -2370,6 +2370,11 @@ intel_info(const struct drm_i915_private *dev_priv) >> #define IS_GEN(dev_priv, ...) \ >> CONCATENATE(_IS_GEN_ARG, COUNT_ARGS(__VA_ARGS__))((dev_priv), ##__VA_ARGS__) >> >> +#define IS_GEN_GT(dev_priv, g) (g > (dev_priv)->info.gen) > > Interesting :), I have at some point floated a similar patch and there > was overwhelming dislike of the GT suffix, since we have GT2 GT3 etc.. > as established acronyms for GPU configurations. To be honest even *without* the GPU GT clash, I think GT/GE/LT/LE are a cognitive burden. Not a big one, but still less natural than >/>=/</<=. Feels like assembly. Not that I have better alternatives. IS_GEN_AFTER for GT, IS_GEN_OR_LATER for GE? Feels a bit contrived? > Furthermore, I am pretty sure I've done it using the gen bitmask so > hypothetical checks like IS_GEN_LT(5) || IS_GEN_RANGE(8, 9) could still > be compile time optimized to a single conditional. That would also now > become more difficult due removal of GEN_FOREVER. > > To summarise, I don't see a advantage of hiding this in macros if we are > not getting the above benefit. Agreed. I didn't realize the GEN_FOREVER removal was merged already, I wish you'd chimed in there. Of course, we can bring it back. BR, Jani. > > Regards, > > Tvrtko > >> +#define IS_GEN_GE(dev_priv, g) (g >= (dev_priv)->info.gen) >> +#define IS_GEN_LT(dev_priv, g) (g < (dev_priv)->info.gen) >> +#define IS_GEN_LE(dev_priv, g) (g <= (dev_priv)->info.gen) >> + >> /* >> * Return true if revision is in range [since,until] inclusive. >> * >>
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 8d22353a3da6..eb636dfe5228 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2370,6 +2370,11 @@ intel_info(const struct drm_i915_private *dev_priv) #define IS_GEN(dev_priv, ...) \ CONCATENATE(_IS_GEN_ARG, COUNT_ARGS(__VA_ARGS__))((dev_priv), ##__VA_ARGS__) +#define IS_GEN_GT(dev_priv, g) (g > (dev_priv)->info.gen) +#define IS_GEN_GE(dev_priv, g) (g >= (dev_priv)->info.gen) +#define IS_GEN_LT(dev_priv, g) (g < (dev_priv)->info.gen) +#define IS_GEN_LE(dev_priv, g) (g <= (dev_priv)->info.gen) + /* * Return true if revision is in range [since,until] inclusive. *
This will allow us to make all gen comparisons simple gen comparisons with the same kind of macro rather than resorting to a mix of IS_GEN(), INTEL_GEN() >=, >, <=, <. Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 5 +++++ 1 file changed, 5 insertions(+)