[Intel-gfx] [PATCH CI 5/7] drm/i915: Drop has_ddi from device info

Souza, Jose jose.souza at intel.com
Mon May 9 14:52:25 UTC 2022 

On Mon, 2022-05-09 at 15:27 +0100, Tvrtko Ursulin wrote:
> On 09/05/2022 15:01, Souza, Jose wrote:
> > On Mon, 2022-05-09 at 14:32 +0100, Tvrtko Ursulin wrote:
> > > On 05/05/2022 20:35, José Roberto de Souza wrote:
> > > > No need to have this parameter in intel_device_info struct
> > > > as all platforms with display version 9 or newer, haswell or broadwell
> > > > supports it.
> > > > 
> > > > As a side effect of the of removal this flag, it will not be printed
> > > > in dmesg during driver load anymore and developers will have to rely
> > > > on to check the macro and compare with platform being used and IP
> > > > versions of it.
> > > > 
> > > > Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > > > ---
> > > >    drivers/gpu/drm/i915/i915_drv.h          | 4 +++-
> > > >    drivers/gpu/drm/i915/i915_pci.c          | 3 ---
> > > >    drivers/gpu/drm/i915/intel_device_info.h | 1 -
> > > >    3 files changed, 3 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 5538564bc1d25..600d8cee272da 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -1298,7 +1298,9 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
> > > >    #define HAS_DP20(dev_priv)	(IS_DG2(dev_priv))
> > > >    
> > > >    #define HAS_CDCLK_CRAWL(dev_priv)	 (INTEL_INFO(dev_priv)->display.has_cdclk_crawl)
> > > > -#define HAS_DDI(dev_priv)		 (INTEL_INFO(dev_priv)->display.has_ddi)
> > > > +#define HAS_DDI(dev_priv)		 (DISPLAY_VER(dev_priv) >= 9 || \
> > > > +					  IS_BROADWELL(dev_priv) || \
> > > > +					  IS_HASWELL(dev_priv))
> > > 
> > > This one is a bit borderline, not sure it passes Jani's criteria of
> > > simplicity, which I thought was a good one. And from the OCD angle it
> > > kind of sucks to expand the conditionals to all call sites (when it's
> > > even called from i915_irq.c, justifiably or not I don't know).
> > 
> > This might increase code size but I don't believe it will case any performance impact even for interruption handling.
> 
> Probably won't, but its IMO ugly and at some point a death of thousand 
> cuts come to play ie. maybe you can't measure an effect of a single 
> change, but over time pointless wastage of cycles accumulates. Not 
> saying that I looked whether it applies to this concrete example, just a 
> general principle - if the condition is not straightforward I would 
> recommend looking at the number and context of callers.
> 
> > > What is the high level motivation for this work?
> > 
> > Add new platforms definitions are becoming huge burden, there is too many features to check if a new platform supports each one of it, what is leading
> > to platform definition errors.
> 
> How does this change help with that? That work is always required, no? 
> With flags it is at least mostly centralized in one file and with this 
> series some parts become spread around so you have to not even know what 
> feature supports what, but also where in code to look for places which 
> need to be adjusted. (Example engine reset and further issues when/if 
> other macros start getting out i915_drv.h.)

There is already several features that don't have a device info flag.
What helps is this case is define the HAS_XXX() using IP version.

> 
> > Also usually when a feature is dropped a HSD will be filed, so the person taking care of that can just adjust the macro upper platform or IP bound and
> > disable it for good.
> 
> Or can equally adjust the has flags assignments at a single file.

What happens it that a feature is disabled for a single platform and on the next one the information that from IP X and newer this feature is gone is
lost in the source code.

> 
> To be clear I don't have a strong preference either way (in principle) 
> at the moment, but think more consensus and discussion is needed here 
> before changing it all.

Okay

> 
> Regards,
> 
> Tvrtko
> 
> > > Also, why is this in drm-intel-gt-next? :)
> > 
> > To reduce conflicts, moving just one of this patches around already causes conflicts.
> > 
> > > 
> > > Regards,
> > > 
> > > Tvrtko
> > > 
> > > 
> > > >    #define HAS_FPGA_DBG_UNCLAIMED(dev_priv) (INTEL_INFO(dev_priv)->display.has_fpga_dbg)
> > > >    #define HAS_PSR(dev_priv)		 (INTEL_INFO(dev_priv)->display.has_psr)
> > > >    #define HAS_PSR_HW_TRACKING(dev_priv) \
> > > > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> > > > index 2dc0284629d30..a0693d9ff9cee 100644
> > > > --- a/drivers/gpu/drm/i915/i915_pci.c
> > > > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > > > @@ -535,7 +535,6 @@ static const struct intel_device_info vlv_info = {
> > > >    	.platform_engine_mask = BIT(RCS0) | BIT(VCS0) | BIT(BCS0) | BIT(VECS0), \
> > > >    	.display.cpu_transcoder_mask = BIT(TRANSCODER_A) | BIT(TRANSCODER_B) | \
> > > >    		BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP), \
> > > > -	.display.has_ddi = 1, \
> > > >    	.display.has_fpga_dbg = 1, \
> > > >    	.display.has_dp_mst = 1, \
> > > >    	.has_rc6p = 0 /* RC6p removed-by HSW */, \
> > > > @@ -683,7 +682,6 @@ static const struct intel_device_info skl_gt4_info = {
> > > >    		BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP) | \
> > > >    		BIT(TRANSCODER_DSI_A) | BIT(TRANSCODER_DSI_C), \
> > > >    	.has_64bit_reloc = 1, \
> > > > -	.display.has_ddi = 1, \
> > > >    	.display.has_fpga_dbg = 1, \
> > > >    	.display.fbc_mask = BIT(INTEL_FBC_A), \
> > > >    	.display.has_hdcp = 1, \
> > > > @@ -932,7 +930,6 @@ static const struct intel_device_info adl_s_info = {
> > > >    	.dbuf.size = 4096,							\
> > > >    	.dbuf.slice_mask = BIT(DBUF_S1) | BIT(DBUF_S2) | BIT(DBUF_S3) |		\
> > > >    		BIT(DBUF_S4),							\
> > > > -	.display.has_ddi = 1,							\
> > > >    	.display.has_dmc = 1,							\
> > > >    	.display.has_dp_mst = 1,						\
> > > >    	.display.has_dsb = 1,							\
> > > > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> > > > index bef65e3f02c55..bc71ce48763ad 100644
> > > > --- a/drivers/gpu/drm/i915/intel_device_info.h
> > > > +++ b/drivers/gpu/drm/i915/intel_device_info.h
> > > > @@ -167,7 +167,6 @@ enum intel_ppgtt_type {
> > > >    	func(cursor_needs_physical); \
> > > >    	func(has_cdclk_crawl); \
> > > >    	func(has_dmc); \
> > > > -	func(has_ddi); \
> > > >    	func(has_dp_mst); \
> > > >    	func(has_dsb); \
> > > >    	func(has_dsc); \
> >

More information about the Intel-gfx mailing list