Re: [Intel-gfx] [PATCH 01/12] drm/i915: Drop IPC from display 13 and newer

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

On Wed, 04 May 2022, José Roberto de Souza <jose.souza@intel.com> wrote:
> This feature is supported from display 9 to display 12 and was
> incorrectly being applied to DG2 and Alderlake-P.
>
> While at is also taking the oportunity to drop it from
> intel_device_info struct as a display check is more simple
> and less prone to be left enabled in future platforms.

Lacking a cover letter, I'll just reply here for the entire series.

We don't really have any rules for when to add a flag and when not. It's
basically been up to whoever has added each HAS_* macro to decide how to
implement it.

Indicators for when to add a flag would be:

- The are no clear cut boundaries for platform versions that have or
  don't have a feature.

- There may be a need to disable a feature for single platforms during
  development or enabling or debugging.

- It would be useful to have the flag show up in dmesg, debugfs or gpu
  error (see intel_device_info_print_static() calls).

- The platform comparison would be complicated.

Indicators for when to add a platform check:

- It's a clear cut platform version check, not a complex boolean
  condition.

- It's obvious for anyone debugging the platform whether the feature is
  there or not based on dmesg, without a dedicated logged flag.

- The feature only exists on legacy platforms and is not coming back,
  i.e. the platform check is pretty much fixed.

With that in mind, I think perhaps the following should remain a flag:

- has_dsb - expected to be adjusted for future platforms
- has_rc6p - complicated
- has_psr_hw_tracking - complicated

Another angle is, do we want to keep all the HAS_* macros in i915_drv.h?
For example, HAS_PSR_HW_TRACKING() could be a platform check localized
at the top of intel_psr.c. I think it's more acceptable to have
complicated platform checks if their use is not wide. This could promote
mode widespread use of HAS_* macros for things where we actually have
(possibly duplicated) inline platform checks, and it would be
self-documenting.

Finally, the main functional change in the series is dropping the
feature from the debug prints, and that's not mentioned anywhere.


BR,
Jani.


>
> BSpec: 50039
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h          | 3 ++-
>  drivers/gpu/drm/i915/i915_pci.c          | 3 ---
>  drivers/gpu/drm/i915/intel_device_info.h | 1 -
>  3 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2dddc27a1b0ed..695b35cd6b5e4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1344,7 +1344,8 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
>   */
>  #define NEEDS_COMPACT_PT(dev_priv) (INTEL_INFO(dev_priv)->needs_compact_pt)
>  
> -#define HAS_IPC(dev_priv)		 (INTEL_INFO(dev_priv)->display.has_ipc)
> +#define HAS_IPC(dev_priv)		 (DISPLAY_VER(dev_priv) >= 9 && \
> +					  DISPLAY_VER(dev_priv) <= 12)
>  
>  #define HAS_REGION(i915, i) (INTEL_INFO(i915)->memory_regions & (i))
>  #define HAS_LMEM(i915) HAS_REGION(i915, REGION_LMEM)
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index 498708b33924f..c4f9c805cffd1 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -646,7 +646,6 @@ static const struct intel_device_info chv_info = {
>  	.display.has_dmc = 1, \
>  	.has_gt_uc = 1, \
>  	.display.has_hdcp = 1, \
> -	.display.has_ipc = 1, \
>  	.display.has_psr = 1, \
>  	.display.has_psr_hw_tracking = 1, \
>  	.dbuf.size = 896 - 4, /* 4 blocks for bypass path allocation */ \
> @@ -712,7 +711,6 @@ static const struct intel_device_info skl_gt4_info = {
>  	.has_reset_engine = 1, \
>  	.has_snoop = true, \
>  	.has_coherent_ggtt = false, \
> -	.display.has_ipc = 1, \
>  	HSW_PIPE_OFFSETS, \
>  	IVB_CURSOR_OFFSETS, \
>  	IVB_COLORS, \
> @@ -955,7 +953,6 @@ static const struct intel_device_info adl_s_info = {
>  	.display.has_fpga_dbg = 1,						\
>  	.display.has_hdcp = 1,							\
>  	.display.has_hotplug = 1,						\
> -	.display.has_ipc = 1,							\
>  	.display.has_psr = 1,							\
>  	.display.ver = 13,							\
>  	.display.pipe_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C) | BIT(PIPE_D),	\
> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> index e7d2cf7d65c85..c9660b4282d9e 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -180,7 +180,6 @@ enum intel_ppgtt_type {
>  	func(has_hdcp); \
>  	func(has_hotplug); \
>  	func(has_hti); \
> -	func(has_ipc); \
>  	func(has_modular_fia); \
>  	func(has_overlay); \
>  	func(has_psr); \

-- 
Jani Nikula, Intel Open Source Graphics Center