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

["Souza, Jose" <jose.souza@intel.com>]

On Thu, 2022-05-05 at 13:45 +0300, Jani Nikula wrote:
> 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.

Sorry about that, was planning to write one but forgot.

> 
> 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.

Thought about this but for this cases we can edit the HAS_X() macros.

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

In my opinion developers can easily check that by reading the HAS_X() and comparing to the platform name and graphics IPs.
There is several other features that we don't have flags in device info already.

> 
> - 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

From what I see it is supported for all future display versions but if you are talking about current issues that we have it with, the flag has shown
that it is worst as some platforms was left with it enabled.

> - has_rc6p - complicated

Matt Ropper suggested to use IS_GRAPHICS_VER(i915, 6, 7) so it will become even less 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.

Good point, yes HAS_PSR_HW_TRACKING() is only used in intel_psr.c it could be moved to the top of it.

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

Like I said in the top, developers can read the macros and compare to IPs version like we do for other features.
For users this has no impact as it is not even printed.
But I will mention in the next version.

Thanks for the feedback

> 
> 
> 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); \
>