Re: [Intel-gfx] [PATCH v3 3/8] drm/i915: add new helpers for accessing stepping info

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 26 Mar 2021, Jani Nikula <jani.nikula@intel.com> wrote:
> On Thu, 25 Mar 2021, "Souza, Jose" <jose.souza@intel.com> wrote:
>> On Mon, 2021-03-08 at 15:56 +0200, Jani Nikula wrote:
>>> Add new runtime info field for stepping. Add new helpers for accessing
>>> them. As we'll be switching platforms over to the new scheme
>>> incrementally, check for non-initialized steppings.
>>> 
>>> In case a platform does not have separate display and gt steppings, it's
>>> okay to use a common shorthand. However, in this case the display
>>> stepping must not be initialized, and gt stepping is the single point of
>>> truth.
>>> 
>>> v2: Rename stepping->step
>>> 
>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/i915_drv.h          | 24 +++++++++++++++---------
>>>  drivers/gpu/drm/i915/intel_device_info.h |  4 ++++
>>>  drivers/gpu/drm/i915/intel_step.h        | 14 ++++++++++++++
>>>  3 files changed, 33 insertions(+), 9 deletions(-)
>>> 
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 02170edd6628..a543b1ad9ba9 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -1274,6 +1274,21 @@ static inline struct drm_i915_private *pdev_to_i915(struct pci_dev *pdev)
>>>  #define IS_REVID(p, since, until) \
>>>  	(INTEL_REVID(p) >= (since) && INTEL_REVID(p) <= (until))
>>>  
>>> 
>>> +#define INTEL_DISPLAY_STEP(__i915) (RUNTIME_INFO(__i915)->step.disp_stepping)
>>> +#define INTEL_GT_STEP(__i915) (RUNTIME_INFO(__i915)->step.gt_stepping)
>>> +
>>> +#define IS_DISPLAY_STEP(__i915, since, until) \
>>> +	(drm_WARN_ON(&(__i915)->drm, INTEL_DISPLAY_STEP(__i915) == STEP_NONE), \
>>> +	 INTEL_DISPLAY_STEP(__i915) >= (since) && INTEL_DISPLAY_STEP(__i915) <= (until))
>>> +
>>> +#define IS_GT_STEP(__i915, since, until) \
>>> +	(drm_WARN_ON(&(__i915)->drm, INTEL_GT_STEP(__i915) == STEP_NONE), \
>>> +	 INTEL_GT_STEP(__i915) >= (since) && INTEL_GT_STEP(__i915) <= (until))
>>> +
>>> +#define IS_STEP(p, since, until) \
>>> +	(drm_WARN_ON(&(__i915)->drm, INTEL_DISPLAY_STEP(__i915) != STEP_NONE), \
>>
>> (drm_WARN_ON(&(__i915)->drm, INTEL_DISPLAY_STEP(__i915) == STEP_NONE), \
>>
>> But I don't think IS_STEP() is useful, better use IS_DISPLAY/GT_STEP even for platforms with the same display and GT version.
>
> The INTEL_DISPLAY_STEP(__i915) != STEP_NONE check is as I intended, not
> a mistake.
>
> The idea is that you'd only be able to use IS_STEP() on platforms where
> display step is not set, i.e. where the versions are the same for
> display and GT.
>
> I don't actually add users for this one, though, and we may indeed be
> better off just throwing it out and always using the specific GT/display
> macros.

Sent next version with IS_STEP() removed, and presumed the rb holds with
that.

Thanks,
Jani.

>
> BR,
> Jani.
>
>
>>
>> With the change above:
>>
>> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
>>
>>> +	 INTEL_GT_STEP(__i915, since, until))
>>> +
>>>  static __always_inline unsigned int
>>>  __platform_mask_index(const struct intel_runtime_info *info,
>>>  		      enum intel_platform p)
>>> @@ -1511,15 +1526,6 @@ enum {
>>>  #define IS_JSL_EHL_REVID(p, since, until) \
>>>  	(IS_JSL_EHL(p) && IS_REVID(p, since, until))
>>>  
>>> 
>>> -enum {
>>> -	STEP_A0,
>>> -	STEP_A2,
>>> -	STEP_B0,
>>> -	STEP_B1,
>>> -	STEP_C0,
>>> -	STEP_D0,
>>> -};
>>> -
>>>  static inline const struct i915_rev_steppings *
>>>  tgl_stepping_get(struct drm_i915_private *dev_priv)
>>>  {
>>> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
>>> index d44f64b57b7a..f84569e8e711 100644
>>> --- a/drivers/gpu/drm/i915/intel_device_info.h
>>> +++ b/drivers/gpu/drm/i915/intel_device_info.h
>>> @@ -27,6 +27,8 @@
>>>  
>>> 
>>>  #include <uapi/drm/i915_drm.h>
>>>  
>>> 
>>> +#include "intel_step.h"
>>> +
>>>  #include "display/intel_display.h"
>>>  
>>> 
>>>  #include "gt/intel_engine_types.h"
>>> @@ -225,6 +227,8 @@ struct intel_runtime_info {
>>>  	u8 num_scalers[I915_MAX_PIPES];
>>>  
>>> 
>>>  	u32 rawclk_freq;
>>> +
>>> +	struct i915_rev_steppings step;
>>>  };
>>>  
>>> 
>>>  struct intel_driver_caps {
>>> diff --git a/drivers/gpu/drm/i915/intel_step.h b/drivers/gpu/drm/i915/intel_step.h
>>> index af922ae3bb4e..8b3ef19d935b 100644
>>> --- a/drivers/gpu/drm/i915/intel_step.h
>>> +++ b/drivers/gpu/drm/i915/intel_step.h
>>> @@ -22,4 +22,18 @@ extern const struct i915_rev_steppings tgl_uy_revid_step_tbl[TGL_UY_REVID_STEP_T
>>>  extern const struct i915_rev_steppings tgl_revid_step_tbl[TGL_REVID_STEP_TBL_SIZE];
>>>  extern const struct i915_rev_steppings adls_revid_step_tbl[ADLS_REVID_STEP_TBL_SIZE];
>>>  
>>> 
>>> +/*
>>> + * 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,
>>> +};
>>> +
>>>  #endif /* __INTEL_STEP_H__ */
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx