[Intel-gfx] [PATCH v2] drm/i915/display: Use unions per platform in intel_dpll_hw_state

[Intel-gfx] [PATCH v2] drm/i915/display: Use unions per platform in intel_dpll_hw_state

[José Roberto de Souza]

This will save us a few bytes in intel_dpll_hw_state struct now
and guarantee that it will not keep growing with each new platform.

v2:
- grouping skl and ICL+ combo (Imre)

Cc: Imre Deak <imre.deak@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c  |  71 +++++++-----
 .../drm/i915/display/intel_display_debugfs.c  |  63 +++++-----
 drivers/gpu/drm/i915/display/intel_dpll_mgr.h | 109 ++++++++++--------
 3 files changed, 136 insertions(+), 107 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 80b19c304c432..58a5bf0d30221 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -6303,38 +6303,45 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 	if (dev_priv->dpll.mgr) {
 		PIPE_CONF_CHECK_P(shared_dpll);
 
-		PIPE_CONF_CHECK_X(dpll_hw_state.dpll);
-		PIPE_CONF_CHECK_X(dpll_hw_state.dpll_md);
-		PIPE_CONF_CHECK_X(dpll_hw_state.fp0);
-		PIPE_CONF_CHECK_X(dpll_hw_state.fp1);
-		PIPE_CONF_CHECK_X(dpll_hw_state.wrpll);
-		PIPE_CONF_CHECK_X(dpll_hw_state.spll);
-		PIPE_CONF_CHECK_X(dpll_hw_state.ctrl1);
-		PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr1);
-		PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr2);
-		PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr0);
-		PIPE_CONF_CHECK_X(dpll_hw_state.div0);
-		PIPE_CONF_CHECK_X(dpll_hw_state.ebb0);
-		PIPE_CONF_CHECK_X(dpll_hw_state.ebb4);
-		PIPE_CONF_CHECK_X(dpll_hw_state.pll0);
-		PIPE_CONF_CHECK_X(dpll_hw_state.pll1);
-		PIPE_CONF_CHECK_X(dpll_hw_state.pll2);
-		PIPE_CONF_CHECK_X(dpll_hw_state.pll3);
-		PIPE_CONF_CHECK_X(dpll_hw_state.pll6);
-		PIPE_CONF_CHECK_X(dpll_hw_state.pll8);
-		PIPE_CONF_CHECK_X(dpll_hw_state.pll9);
-		PIPE_CONF_CHECK_X(dpll_hw_state.pll10);
-		PIPE_CONF_CHECK_X(dpll_hw_state.pcsdw12);
-		PIPE_CONF_CHECK_X(dpll_hw_state.mg_refclkin_ctl);
-		PIPE_CONF_CHECK_X(dpll_hw_state.mg_clktop2_coreclkctl1);
-		PIPE_CONF_CHECK_X(dpll_hw_state.mg_clktop2_hsclkctl);
-		PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_div0);
-		PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_div1);
-		PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_lf);
-		PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_frac_lock);
-		PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_ssc);
-		PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_bias);
-		PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_tdc_coldst_bias);
+		if (DISPLAY_VER(dev_priv) >= 11) {
+			PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr0);
+			PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr1);
+			PIPE_CONF_CHECK_X(dpll_hw_state.div0);
+			PIPE_CONF_CHECK_X(dpll_hw_state.mg_refclkin_ctl);
+			PIPE_CONF_CHECK_X(dpll_hw_state.mg_clktop2_coreclkctl1);
+			PIPE_CONF_CHECK_X(dpll_hw_state.mg_clktop2_hsclkctl);
+			PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_div0);
+			PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_div1);
+			PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_lf);
+			PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_frac_lock);
+			PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_ssc);
+			PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_bias);
+			PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_tdc_coldst_bias);
+		} else if (IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv)) {
+			PIPE_CONF_CHECK_X(dpll_hw_state.ebb0);
+			PIPE_CONF_CHECK_X(dpll_hw_state.ebb4);
+			PIPE_CONF_CHECK_X(dpll_hw_state.pll0);
+			PIPE_CONF_CHECK_X(dpll_hw_state.pll1);
+			PIPE_CONF_CHECK_X(dpll_hw_state.pll2);
+			PIPE_CONF_CHECK_X(dpll_hw_state.pll3);
+			PIPE_CONF_CHECK_X(dpll_hw_state.pll6);
+			PIPE_CONF_CHECK_X(dpll_hw_state.pll8);
+			PIPE_CONF_CHECK_X(dpll_hw_state.pll9);
+			PIPE_CONF_CHECK_X(dpll_hw_state.pll10);
+			PIPE_CONF_CHECK_X(dpll_hw_state.pcsdw12);
+		} else if (DISPLAY_VER(dev_priv) == 9) {
+			PIPE_CONF_CHECK_X(dpll_hw_state.ctrl1);
+			PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr1);
+			PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr2);
+		} else if (HAS_DDI(dev_priv)) {
+			PIPE_CONF_CHECK_X(dpll_hw_state.wrpll);
+			PIPE_CONF_CHECK_X(dpll_hw_state.spll);
+		} else if (HAS_PCH_IBX(dev_priv) || HAS_PCH_CPT(dev_priv)) {
+			PIPE_CONF_CHECK_X(dpll_hw_state.dpll);
+			PIPE_CONF_CHECK_X(dpll_hw_state.dpll_md);
+			PIPE_CONF_CHECK_X(dpll_hw_state.fp0);
+			PIPE_CONF_CHECK_X(dpll_hw_state.fp1);
+		}
 	}
 
 	PIPE_CONF_CHECK_X(dsi_pll.ctrl);
diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
index ffe6822d7414a..cd53f7da9b00d 100644
--- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
+++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
@@ -1007,35 +1007,40 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused)
 		seq_printf(m, " pipe_mask: 0x%x, active: 0x%x, on: %s\n",
 			   pll->state.pipe_mask, pll->active_mask, yesno(pll->on));
 		seq_printf(m, " tracked hardware state:\n");
-		seq_printf(m, " dpll:    0x%08x\n", pll->state.hw_state.dpll);
-		seq_printf(m, " dpll_md: 0x%08x\n",
-			   pll->state.hw_state.dpll_md);
-		seq_printf(m, " fp0:     0x%08x\n", pll->state.hw_state.fp0);
-		seq_printf(m, " fp1:     0x%08x\n", pll->state.hw_state.fp1);
-		seq_printf(m, " wrpll:   0x%08x\n", pll->state.hw_state.wrpll);
-		seq_printf(m, " cfgcr0:  0x%08x\n", pll->state.hw_state.cfgcr0);
-		seq_printf(m, " cfgcr1:  0x%08x\n", pll->state.hw_state.cfgcr1);
-		seq_printf(m, " div0:    0x%08x\n", pll->state.hw_state.div0);
-		seq_printf(m, " mg_refclkin_ctl:        0x%08x\n",
-			   pll->state.hw_state.mg_refclkin_ctl);
-		seq_printf(m, " mg_clktop2_coreclkctl1: 0x%08x\n",
-			   pll->state.hw_state.mg_clktop2_coreclkctl1);
-		seq_printf(m, " mg_clktop2_hsclkctl:    0x%08x\n",
-			   pll->state.hw_state.mg_clktop2_hsclkctl);
-		seq_printf(m, " mg_pll_div0:  0x%08x\n",
-			   pll->state.hw_state.mg_pll_div0);
-		seq_printf(m, " mg_pll_div1:  0x%08x\n",
-			   pll->state.hw_state.mg_pll_div1);
-		seq_printf(m, " mg_pll_lf:    0x%08x\n",
-			   pll->state.hw_state.mg_pll_lf);
-		seq_printf(m, " mg_pll_frac_lock: 0x%08x\n",
-			   pll->state.hw_state.mg_pll_frac_lock);
-		seq_printf(m, " mg_pll_ssc:   0x%08x\n",
-			   pll->state.hw_state.mg_pll_ssc);
-		seq_printf(m, " mg_pll_bias:  0x%08x\n",
-			   pll->state.hw_state.mg_pll_bias);
-		seq_printf(m, " mg_pll_tdc_coldst_bias: 0x%08x\n",
-			   pll->state.hw_state.mg_pll_tdc_coldst_bias);
+
+		if (DISPLAY_VER(dev_priv) >= 11) {
+			seq_printf(m, " cfgcr0:  0x%08x\n", pll->state.hw_state.cfgcr0);
+			seq_printf(m, " cfgcr1:  0x%08x\n", pll->state.hw_state.cfgcr1);
+			seq_printf(m, " div0:    0x%08x\n", pll->state.hw_state.div0);
+			seq_printf(m, " mg_refclkin_ctl:        0x%08x\n",
+				   pll->state.hw_state.mg_refclkin_ctl);
+			seq_printf(m, " mg_clktop2_coreclkctl1: 0x%08x\n",
+				   pll->state.hw_state.mg_clktop2_coreclkctl1);
+			seq_printf(m, " mg_clktop2_hsclkctl:    0x%08x\n",
+				   pll->state.hw_state.mg_clktop2_hsclkctl);
+			seq_printf(m, " mg_pll_div0:  0x%08x\n",
+				   pll->state.hw_state.mg_pll_div0);
+			seq_printf(m, " mg_pll_div1:  0x%08x\n",
+				   pll->state.hw_state.mg_pll_div1);
+			seq_printf(m, " mg_pll_lf:    0x%08x\n",
+				   pll->state.hw_state.mg_pll_lf);
+			seq_printf(m, " mg_pll_frac_lock: 0x%08x\n",
+				   pll->state.hw_state.mg_pll_frac_lock);
+			seq_printf(m, " mg_pll_ssc:   0x%08x\n",
+				   pll->state.hw_state.mg_pll_ssc);
+			seq_printf(m, " mg_pll_bias:  0x%08x\n",
+				   pll->state.hw_state.mg_pll_bias);
+			seq_printf(m, " mg_pll_tdc_coldst_bias: 0x%08x\n",
+				   pll->state.hw_state.mg_pll_tdc_coldst_bias);
+		} else if (HAS_DDI(dev_priv)) {
+			seq_printf(m, " wrpll:   0x%08x\n", pll->state.hw_state.wrpll);
+			seq_printf(m, " spll:   0x%08x\n", pll->state.hw_state.spll);
+		} else if (HAS_PCH_IBX(dev_priv) || HAS_PCH_CPT(dev_priv)) {
+			seq_printf(m, " dpll:    0x%08x\n", pll->state.hw_state.dpll);
+			seq_printf(m, " dpll_md: 0x%08x\n", pll->state.hw_state.dpll_md);
+			seq_printf(m, " fp0:     0x%08x\n", pll->state.hw_state.fp0);
+			seq_printf(m, " fp1:     0x%08x\n", pll->state.hw_state.fp1);
+		}
 	}
 	drm_modeset_unlock_all(dev);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
index ba2fdfce15792..0ac3cd44f5d49 100644
--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
@@ -184,52 +184,69 @@ enum icl_port_dpll_id {
 };
 
 struct intel_dpll_hw_state {
-	/* i9xx, pch plls */
-	u32 dpll;
-	u32 dpll_md;
-	u32 fp0;
-	u32 fp1;
-
-	/* hsw, bdw */
-	u32 wrpll;
-	u32 spll;
-
-	/* skl */
-	/*
-	 * DPLL_CTRL1 has 6 bits for each each this DPLL. We store those in
-	 * lower part of ctrl1 and they get shifted into position when writing
-	 * the register.  This allows us to easily compare the state to share
-	 * the DPLL.
-	 */
-	u32 ctrl1;
-	/* HDMI only, 0 when used for DP */
-	u32 cfgcr1, cfgcr2;
-
-	/* icl */
-	u32 cfgcr0;
-
-	/* tgl */
-	u32 div0;
-
-	/* bxt */
-	u32 ebb0, ebb4, pll0, pll1, pll2, pll3, pll6, pll8, pll9, pll10, pcsdw12;
-
-	/*
-	 * ICL uses the following, already defined:
-	 * u32 cfgcr0, cfgcr1;
-	 */
-	u32 mg_refclkin_ctl;
-	u32 mg_clktop2_coreclkctl1;
-	u32 mg_clktop2_hsclkctl;
-	u32 mg_pll_div0;
-	u32 mg_pll_div1;
-	u32 mg_pll_lf;
-	u32 mg_pll_frac_lock;
-	u32 mg_pll_ssc;
-	u32 mg_pll_bias;
-	u32 mg_pll_tdc_coldst_bias;
-	u32 mg_pll_bias_mask;
-	u32 mg_pll_tdc_coldst_bias_mask;
+	union {
+		/* icl+ TC */
+		struct {
+			u32 mg_refclkin_ctl;
+			u32 mg_clktop2_coreclkctl1;
+			u32 mg_clktop2_hsclkctl;
+			u32 mg_pll_div0;
+			u32 mg_pll_div1;
+			u32 mg_pll_lf;
+			u32 mg_pll_frac_lock;
+			u32 mg_pll_ssc;
+			u32 mg_pll_bias;
+			u32 mg_pll_tdc_coldst_bias;
+			u32 mg_pll_bias_mask;
+			u32 mg_pll_tdc_coldst_bias_mask;
+		};
+
+		/* bxt */
+		struct {
+			/* bxt */
+			u32 ebb0;
+			u32 ebb4;
+			u32 pll0;
+			u32 pll1;
+			u32 pll2;
+			u32 pll3;
+			u32 pll6;
+			u32 pll8;
+			u32 pll9;
+			u32 pll10;
+			u32 pcsdw12;
+		};
+
+		/* skl+ and icl+ combo */
+		struct {
+			/*
+			 * DPLL_CTRL1 has 6 bits for each this DPLL. We store those in
+			 * lower part of ctrl1 and they get shifted into position when writing
+			 * the register.  This allows us to easily compare the state to share
+			 * the DPLL.
+			 */
+			u32 ctrl1;
+			u32 cfgcr0;
+			u32 cfgcr1;
+			/* HDMI only, 0 when used for DP */
+			u32 cfgcr2;
+			u32 div0;
+		};
+
+		/* hsw, bdw */
+		struct {
+			u32 wrpll;
+			u32 spll;
+		};
+
+		/* i9xx, pch plls */
+		struct {
+			u32 dpll;
+			u32 dpll_md;
+			u32 fp0;
+			u32 fp1;
+		};
+	};
 };
 
 /**
-- 
2.35.1

Re: [Intel-gfx] [PATCH v2] drm/i915/display: Use unions per platform in intel_dpll_hw_state

[Ville Syrjälä]

On Wed, Feb 23, 2022 at 12:55:51PM -0800, José Roberto de Souza wrote:
<snip>
> +	union {
> +		/* icl+ TC */
> +		struct {
> +			u32 mg_refclkin_ctl;
> +			u32 mg_clktop2_coreclkctl1;
> +			u32 mg_clktop2_hsclkctl;
> +			u32 mg_pll_div0;
> +			u32 mg_pll_div1;
> +			u32 mg_pll_lf;
> +			u32 mg_pll_frac_lock;
> +			u32 mg_pll_ssc;
> +			u32 mg_pll_bias;
> +			u32 mg_pll_tdc_coldst_bias;
> +			u32 mg_pll_bias_mask;
> +			u32 mg_pll_tdc_coldst_bias_mask;
> +		};
> +
> +		/* bxt */
> +		struct {
> +			/* bxt */
> +			u32 ebb0;
> +			u32 ebb4;
> +			u32 pll0;
> +			u32 pll1;
> +			u32 pll2;
> +			u32 pll3;
> +			u32 pll6;
> +			u32 pll8;
> +			u32 pll9;
> +			u32 pll10;
> +			u32 pcsdw12;
> +		};

Wasn't there some funny compiler bug around anonymous unions?
git log --grep='anon.*union' seems to agree. Please double check
that stuff to make sure this is actually safe.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH v2] drm/i915/display: Use unions per platform in intel_dpll_hw_state

[Souza, Jose]

On Thu, 2022-02-24 at 12:20 +0200, Ville Syrjälä wrote:
> On Wed, Feb 23, 2022 at 12:55:51PM -0800, José Roberto de Souza wrote:
> <snip>
> > +	union {
> > +		/* icl+ TC */
> > +		struct {
> > +			u32 mg_refclkin_ctl;
> > +			u32 mg_clktop2_coreclkctl1;
> > +			u32 mg_clktop2_hsclkctl;
> > +			u32 mg_pll_div0;
> > +			u32 mg_pll_div1;
> > +			u32 mg_pll_lf;
> > +			u32 mg_pll_frac_lock;
> > +			u32 mg_pll_ssc;
> > +			u32 mg_pll_bias;
> > +			u32 mg_pll_tdc_coldst_bias;
> > +			u32 mg_pll_bias_mask;
> > +			u32 mg_pll_tdc_coldst_bias_mask;
> > +		};
> > +
> > +		/* bxt */
> > +		struct {
> > +			/* bxt */
> > +			u32 ebb0;
> > +			u32 ebb4;
> > +			u32 pll0;
> > +			u32 pll1;
> > +			u32 pll2;
> > +			u32 pll3;
> > +			u32 pll6;
> > +			u32 pll8;
> > +			u32 pll9;
> > +			u32 pll10;
> > +			u32 pcsdw12;
> > +		};
> 
> Wasn't there some funny compiler bug around anonymous unions?
> git log --grep='anon.*union' seems to agree. Please double check
> that stuff to make sure this is actually safe.

I don't see any patch referring to compiler issues with 'git log --grep='anon.*union', what I see is other subsystems making use of it too.
Can you share the commit hash that you are referring to?


> 

Re: [Intel-gfx] [PATCH v2] drm/i915/display: Use unions per platform in intel_dpll_hw_state

[Ville Syrjälä]

On Thu, Feb 24, 2022 at 01:17:35PM +0000, Souza, Jose wrote:
> On Thu, 2022-02-24 at 12:20 +0200, Ville Syrjälä wrote:
> > On Wed, Feb 23, 2022 at 12:55:51PM -0800, José Roberto de Souza wrote:
> > <snip>
> > > +	union {
> > > +		/* icl+ TC */
> > > +		struct {
> > > +			u32 mg_refclkin_ctl;
> > > +			u32 mg_clktop2_coreclkctl1;
> > > +			u32 mg_clktop2_hsclkctl;
> > > +			u32 mg_pll_div0;
> > > +			u32 mg_pll_div1;
> > > +			u32 mg_pll_lf;
> > > +			u32 mg_pll_frac_lock;
> > > +			u32 mg_pll_ssc;
> > > +			u32 mg_pll_bias;
> > > +			u32 mg_pll_tdc_coldst_bias;
> > > +			u32 mg_pll_bias_mask;
> > > +			u32 mg_pll_tdc_coldst_bias_mask;
> > > +		};
> > > +
> > > +		/* bxt */
> > > +		struct {
> > > +			/* bxt */
> > > +			u32 ebb0;
> > > +			u32 ebb4;
> > > +			u32 pll0;
> > > +			u32 pll1;
> > > +			u32 pll2;
> > > +			u32 pll3;
> > > +			u32 pll6;
> > > +			u32 pll8;
> > > +			u32 pll9;
> > > +			u32 pll10;
> > > +			u32 pcsdw12;
> > > +		};
> > 
> > Wasn't there some funny compiler bug around anonymous unions?
> > git log --grep='anon.*union' seems to agree. Please double check
> > that stuff to make sure this is actually safe.
> 
> I don't see any patch referring to compiler issues with 'git log --grep='anon.*union', what I see is other subsystems making use of it too.
> Can you share the commit hash that you are referring to?

$ git log --format=oneline --grep='anon.*union' -- drivers/gpu/drm/i915

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH v2] drm/i915/display: Use unions per platform in intel_dpll_hw_state

[Souza, Jose]

On Thu, 2022-02-24 at 15:25 +0200, Ville Syrjälä wrote:
> On Thu, Feb 24, 2022 at 01:17:35PM +0000, Souza, Jose wrote:
> > On Thu, 2022-02-24 at 12:20 +0200, Ville Syrjälä wrote:
> > > On Wed, Feb 23, 2022 at 12:55:51PM -0800, José Roberto de Souza wrote:
> > > <snip>
> > > > +	union {
> > > > +		/* icl+ TC */
> > > > +		struct {
> > > > +			u32 mg_refclkin_ctl;
> > > > +			u32 mg_clktop2_coreclkctl1;
> > > > +			u32 mg_clktop2_hsclkctl;
> > > > +			u32 mg_pll_div0;
> > > > +			u32 mg_pll_div1;
> > > > +			u32 mg_pll_lf;
> > > > +			u32 mg_pll_frac_lock;
> > > > +			u32 mg_pll_ssc;
> > > > +			u32 mg_pll_bias;
> > > > +			u32 mg_pll_tdc_coldst_bias;
> > > > +			u32 mg_pll_bias_mask;
> > > > +			u32 mg_pll_tdc_coldst_bias_mask;
> > > > +		};
> > > > +
> > > > +		/* bxt */
> > > > +		struct {
> > > > +			/* bxt */
> > > > +			u32 ebb0;
> > > > +			u32 ebb4;
> > > > +			u32 pll0;
> > > > +			u32 pll1;
> > > > +			u32 pll2;
> > > > +			u32 pll3;
> > > > +			u32 pll6;
> > > > +			u32 pll8;
> > > > +			u32 pll9;
> > > > +			u32 pll10;
> > > > +			u32 pcsdw12;
> > > > +		};
> > > 
> > > Wasn't there some funny compiler bug around anonymous unions?
> > > git log --grep='anon.*union' seems to agree. Please double check
> > > that stuff to make sure this is actually safe.
> > 
> > I don't see any patch referring to compiler issues with 'git log --grep='anon.*union', what I see is other subsystems making use of it too.
> > Can you share the commit hash that you are referring to?
> 
> $ git log --format=oneline --grep='anon.*union' -- drivers/gpu/drm/i915
> 

I see issues with initialization of anonymous union but we don't initialize intel_dpll_hw_state.
Also it was fixed on GCC 4.6 that is older than current GCC requirement to build kernel(GCC 5.1).

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=10676

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/display: Use unions per platform in intel_dpll_hw_state (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915/display: Use unions per platform in intel_dpll_hw_state (rev3)
URL   : https://patchwork.freedesktop.org/series/100577/
State : warning

== Summary ==

$ dim sparse --fast origin/drm-tip
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

Re: [Intel-gfx] [PATCH v2] drm/i915/display: Use unions per platform in intel_dpll_hw_state

[Ville Syrjälä]

On Thu, Feb 24, 2022 at 01:49:36PM +0000, Souza, Jose wrote:
> On Thu, 2022-02-24 at 15:25 +0200, Ville Syrjälä wrote:
> > On Thu, Feb 24, 2022 at 01:17:35PM +0000, Souza, Jose wrote:
> > > On Thu, 2022-02-24 at 12:20 +0200, Ville Syrjälä wrote:
> > > > On Wed, Feb 23, 2022 at 12:55:51PM -0800, José Roberto de Souza wrote:
> > > > <snip>
> > > > > +	union {
> > > > > +		/* icl+ TC */
> > > > > +		struct {
> > > > > +			u32 mg_refclkin_ctl;
> > > > > +			u32 mg_clktop2_coreclkctl1;
> > > > > +			u32 mg_clktop2_hsclkctl;
> > > > > +			u32 mg_pll_div0;
> > > > > +			u32 mg_pll_div1;
> > > > > +			u32 mg_pll_lf;
> > > > > +			u32 mg_pll_frac_lock;
> > > > > +			u32 mg_pll_ssc;
> > > > > +			u32 mg_pll_bias;
> > > > > +			u32 mg_pll_tdc_coldst_bias;
> > > > > +			u32 mg_pll_bias_mask;
> > > > > +			u32 mg_pll_tdc_coldst_bias_mask;
> > > > > +		};
> > > > > +
> > > > > +		/* bxt */
> > > > > +		struct {
> > > > > +			/* bxt */
> > > > > +			u32 ebb0;
> > > > > +			u32 ebb4;
> > > > > +			u32 pll0;
> > > > > +			u32 pll1;
> > > > > +			u32 pll2;
> > > > > +			u32 pll3;
> > > > > +			u32 pll6;
> > > > > +			u32 pll8;
> > > > > +			u32 pll9;
> > > > > +			u32 pll10;
> > > > > +			u32 pcsdw12;
> > > > > +		};
> > > > 
> > > > Wasn't there some funny compiler bug around anonymous unions?
> > > > git log --grep='anon.*union' seems to agree. Please double check
> > > > that stuff to make sure this is actually safe.
> > > 
> > > I don't see any patch referring to compiler issues with 'git log --grep='anon.*union', what I see is other subsystems making use of it too.
> > > Can you share the commit hash that you are referring to?
> > 
> > $ git log --format=oneline --grep='anon.*union' -- drivers/gpu/drm/i915
> > 
> 
> I see issues with initialization of anonymous union but we don't initialize intel_dpll_hw_state.
> Also it was fixed on GCC 4.6 that is older than current GCC requirement to build kernel(GCC 5.1).
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=10676

OK. However, after pondering this a bit I think naming things wpild
probably be better here. You have a bunch of if ladders now where each
branch only operates on one of the structs inside the union. IMO the
anonymity is making it rather hard to see if the code is even correct.

-- 
Ville Syrjälä
Intel

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/display: Use unions per platform in intel_dpll_hw_state (rev3)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 8878 bytes --]

== Series Details ==

Series: drm/i915/display: Use unions per platform in intel_dpll_hw_state (rev3)
URL   : https://patchwork.freedesktop.org/series/100577/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_11278 -> Patchwork_22392
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_22392 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_22392, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22392/index.html

Participating hosts (42 -> 41)
------------------------------

  Additional (1): fi-kbl-8809g 
  Missing    (2): fi-bdw-samus fi-pnv-d510 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_22392:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - fi-skl-6600u:       NOTRUN -> [FAIL][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22392/fi-skl-6600u/igt@i915_pm_rpm@basic-pci-d3-state.html

  
Known issues
------------

  Here are the changes found in Patchwork_22392 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s0@smem:
    - fi-kbl-8809g:       NOTRUN -> [DMESG-WARN][2] ([i915#4962]) +1 similar issue
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22392/fi-kbl-8809g/igt@gem_exec_suspend@basic-s0@smem.html

  * igt@gem_huc_copy@huc-copy:
    - fi-skl-6600u:       NOTRUN -> [SKIP][3] ([fdo#109271] / [i915#2190])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22392/fi-skl-6600u/igt@gem_huc_copy@huc-copy.html
    - fi-kbl-8809g:       NOTRUN -> [SKIP][4] ([fdo#109271] / [i915#2190])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22392/fi-kbl-8809g/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@random-engines:
    - fi-kbl-8809g:       NOTRUN -> [SKIP][5] ([fdo#109271] / [i915#4613]) +3 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22392/fi-kbl-8809g/igt@gem_lmem_swapping@random-engines.html

  * igt@gem_lmem_swapping@verify-random:
    - fi-skl-6600u:       NOTRUN -> [SKIP][6] ([fdo#109271] / [i915#4613]) +3 similar issues
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22392/fi-skl-6600u/igt@gem_lmem_swapping@verify-random.html

  * igt@i915_selftest@live:
    - fi-skl-6600u:       NOTRUN -> [FAIL][7] ([i915#4547])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22392/fi-skl-6600u/igt@i915_selftest@live.html

  * igt@kms_chamelium@hdmi-edid-read:
    - fi-kbl-8809g:       NOTRUN -> [SKIP][8] ([fdo#109271] / [fdo#111827]) +8 similar issues
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22392/fi-kbl-8809g/igt@kms_chamelium@hdmi-edid-read.html

  * igt@kms_chamelium@vga-edid-read:
    - fi-skl-6600u:       NOTRUN -> [SKIP][9] ([fdo#109271] / [fdo#111827]) +8 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22392/fi-skl-6600u/igt@kms_chamelium@vga-edid-read.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - fi-skl-6600u:       NOTRUN -> [SKIP][10] ([fdo#109271]) +3 similar issues
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22392/fi-skl-6600u/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d:
    - fi-skl-6600u:       NOTRUN -> [SKIP][11] ([fdo#109271] / [i915#533])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22392/fi-skl-6600u/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d.html
    - fi-kbl-8809g:       NOTRUN -> [SKIP][12] ([fdo#109271] / [i915#533])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22392/fi-kbl-8809g/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d.html

  * igt@kms_psr@cursor_plane_move:
    - fi-kbl-8809g:       NOTRUN -> [SKIP][13] ([fdo#109271]) +54 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22392/fi-kbl-8809g/igt@kms_psr@cursor_plane_move.html

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s3@smem:
    - {fi-rkl-11600}:     [INCOMPLETE][14] ([i915#5127]) -> [PASS][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11278/fi-rkl-11600/igt@gem_exec_suspend@basic-s3@smem.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22392/fi-rkl-11600/igt@gem_exec_suspend@basic-s3@smem.html

  * igt@gem_flink_basic@bad-flink:
    - fi-skl-6600u:       [FAIL][16] ([i915#4547]) -> [PASS][17]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11278/fi-skl-6600u/igt@gem_flink_basic@bad-flink.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22392/fi-skl-6600u/igt@gem_flink_basic@bad-flink.html

  * igt@i915_selftest@live@gem_contexts:
    - {fi-tgl-dsi}:       [INCOMPLETE][18] ([i915#402]) -> [PASS][19]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11278/fi-tgl-dsi/igt@i915_selftest@live@gem_contexts.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22392/fi-tgl-dsi/igt@i915_selftest@live@gem_contexts.html

  * igt@i915_selftest@live@hangcheck:
    - {fi-tgl-dsi}:       [INCOMPLETE][20] ([i915#750]) -> [PASS][21]
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11278/fi-tgl-dsi/igt@i915_selftest@live@hangcheck.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22392/fi-tgl-dsi/igt@i915_selftest@live@hangcheck.html

  
#### Warnings ####

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - fi-kbl-guc:         [SKIP][22] ([fdo#109271]) -> [FAIL][23] ([i915#3049])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11278/fi-kbl-guc/igt@i915_pm_rpm@basic-pci-d3-state.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22392/fi-kbl-guc/igt@i915_pm_rpm@basic-pci-d3-state.html

  * igt@runner@aborted:
    - fi-skl-6600u:       [FAIL][24] ([i915#4312]) -> [FAIL][25] ([i915#1436] / [i915#4312])
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11278/fi-skl-6600u/igt@runner@aborted.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22392/fi-skl-6600u/igt@runner@aborted.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1436]: https://gitlab.freedesktop.org/drm/intel/issues/1436
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#3012]: https://gitlab.freedesktop.org/drm/intel/issues/3012
  [i915#3049]: https://gitlab.freedesktop.org/drm/intel/issues/3049
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
  [i915#3576]: https://gitlab.freedesktop.org/drm/intel/issues/3576
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
  [i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4547]: https://gitlab.freedesktop.org/drm/intel/issues/4547
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4962]: https://gitlab.freedesktop.org/drm/intel/issues/4962
  [i915#5068]: https://gitlab.freedesktop.org/drm/intel/issues/5068
  [i915#5127]: https://gitlab.freedesktop.org/drm/intel/issues/5127
  [i915#533]: https://gitlab.freedesktop.org/drm/intel/issues/533
  [i915#750]: https://gitlab.freedesktop.org/drm/intel/issues/750


Build changes
-------------

  * Linux: CI_DRM_11278 -> Patchwork_22392

  CI-20190529: 20190529
  CI_DRM_11278: 3d3598ac909328595bdff47beadf7f53f7465416 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6356: b403d8e73c6888561eaec97835688313b0763ce9 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_22392: acf715c7ac49968184fd84228d985a65d2e13872 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

acf715c7ac49 drm/i915/display: Use unions per platform in intel_dpll_hw_state

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22392/index.html

[-- Attachment #2: Type: text/html, Size: 10146 bytes --]

Re: [Intel-gfx] [PATCH v2] drm/i915/display: Use unions per platform in intel_dpll_hw_state

[Souza, Jose]

On Thu, 2022-02-24 at 17:39 +0200, Ville Syrjälä wrote:
> On Thu, Feb 24, 2022 at 01:49:36PM +0000, Souza, Jose wrote:
> > On Thu, 2022-02-24 at 15:25 +0200, Ville Syrjälä wrote:
> > > On Thu, Feb 24, 2022 at 01:17:35PM +0000, Souza, Jose wrote:
> > > > On Thu, 2022-02-24 at 12:20 +0200, Ville Syrjälä wrote:
> > > > > On Wed, Feb 23, 2022 at 12:55:51PM -0800, José Roberto de Souza wrote:
> > > > > <snip>
> > > > > > +	union {
> > > > > > +		/* icl+ TC */
> > > > > > +		struct {
> > > > > > +			u32 mg_refclkin_ctl;
> > > > > > +			u32 mg_clktop2_coreclkctl1;
> > > > > > +			u32 mg_clktop2_hsclkctl;
> > > > > > +			u32 mg_pll_div0;
> > > > > > +			u32 mg_pll_div1;
> > > > > > +			u32 mg_pll_lf;
> > > > > > +			u32 mg_pll_frac_lock;
> > > > > > +			u32 mg_pll_ssc;
> > > > > > +			u32 mg_pll_bias;
> > > > > > +			u32 mg_pll_tdc_coldst_bias;
> > > > > > +			u32 mg_pll_bias_mask;
> > > > > > +			u32 mg_pll_tdc_coldst_bias_mask;
> > > > > > +		};
> > > > > > +
> > > > > > +		/* bxt */
> > > > > > +		struct {
> > > > > > +			/* bxt */
> > > > > > +			u32 ebb0;
> > > > > > +			u32 ebb4;
> > > > > > +			u32 pll0;
> > > > > > +			u32 pll1;
> > > > > > +			u32 pll2;
> > > > > > +			u32 pll3;
> > > > > > +			u32 pll6;
> > > > > > +			u32 pll8;
> > > > > > +			u32 pll9;
> > > > > > +			u32 pll10;
> > > > > > +			u32 pcsdw12;
> > > > > > +		};
> > > > > 
> > > > > Wasn't there some funny compiler bug around anonymous unions?
> > > > > git log --grep='anon.*union' seems to agree. Please double check
> > > > > that stuff to make sure this is actually safe.
> > > > 
> > > > I don't see any patch referring to compiler issues with 'git log --grep='anon.*union', what I see is other subsystems making use of it too.
> > > > Can you share the commit hash that you are referring to?
> > > 
> > > $ git log --format=oneline --grep='anon.*union' -- drivers/gpu/drm/i915
> > > 
> > 
> > I see issues with initialization of anonymous union but we don't initialize intel_dpll_hw_state.
> > Also it was fixed on GCC 4.6 that is older than current GCC requirement to build kernel(GCC 5.1).
> > 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=10676
> 
> OK. However, after pondering this a bit I think naming things wpild
> probably be better here. You have a bunch of if ladders now where each
> branch only operates on one of the structs inside the union. IMO the
> anonymity is making it rather hard to see if the code is even correct.

Just to mare sure we are in the same page, you want to have this?


struct icl_tc {
	u32 mg_refclkin_ctl;
	u32 mg_clktop2_coreclkctl1;
	u32 mg_clktop2_hsclkctl;
	u32 mg_pll_div0;
	u32 mg_pll_div1;
	u32 mg_pll_lf;
	u32 mg_pll_frac_lock;
	u32 mg_pll_ssc;
	u32 mg_pll_bias;
	u32 mg_pll_tdc_coldst_bias;
	u32 mg_pll_bias_mask;
	u32 mg_pll_tdc_coldst_bias_mask;
};

So we would need to access members with icl_tc.mg_refclkin_ctl?

I can do that but the diff will be huge.
Are you okay with that too Imre?


> 

Re: [Intel-gfx] [PATCH v2] drm/i915/display: Use unions per platform in intel_dpll_hw_state

[Ville Syrjälä]

On Thu, Feb 24, 2022 at 05:48:10PM +0000, Souza, Jose wrote:
> On Thu, 2022-02-24 at 17:39 +0200, Ville Syrjälä wrote:
> > On Thu, Feb 24, 2022 at 01:49:36PM +0000, Souza, Jose wrote:
> > > On Thu, 2022-02-24 at 15:25 +0200, Ville Syrjälä wrote:
> > > > On Thu, Feb 24, 2022 at 01:17:35PM +0000, Souza, Jose wrote:
> > > > > On Thu, 2022-02-24 at 12:20 +0200, Ville Syrjälä wrote:
> > > > > > On Wed, Feb 23, 2022 at 12:55:51PM -0800, José Roberto de Souza wrote:
> > > > > > <snip>
> > > > > > > +	union {
> > > > > > > +		/* icl+ TC */
> > > > > > > +		struct {
> > > > > > > +			u32 mg_refclkin_ctl;
> > > > > > > +			u32 mg_clktop2_coreclkctl1;
> > > > > > > +			u32 mg_clktop2_hsclkctl;
> > > > > > > +			u32 mg_pll_div0;
> > > > > > > +			u32 mg_pll_div1;
> > > > > > > +			u32 mg_pll_lf;
> > > > > > > +			u32 mg_pll_frac_lock;
> > > > > > > +			u32 mg_pll_ssc;
> > > > > > > +			u32 mg_pll_bias;
> > > > > > > +			u32 mg_pll_tdc_coldst_bias;
> > > > > > > +			u32 mg_pll_bias_mask;
> > > > > > > +			u32 mg_pll_tdc_coldst_bias_mask;
> > > > > > > +		};
> > > > > > > +
> > > > > > > +		/* bxt */
> > > > > > > +		struct {
> > > > > > > +			/* bxt */
> > > > > > > +			u32 ebb0;
> > > > > > > +			u32 ebb4;
> > > > > > > +			u32 pll0;
> > > > > > > +			u32 pll1;
> > > > > > > +			u32 pll2;
> > > > > > > +			u32 pll3;
> > > > > > > +			u32 pll6;
> > > > > > > +			u32 pll8;
> > > > > > > +			u32 pll9;
> > > > > > > +			u32 pll10;
> > > > > > > +			u32 pcsdw12;
> > > > > > > +		};
> > > > > > 
> > > > > > Wasn't there some funny compiler bug around anonymous unions?
> > > > > > git log --grep='anon.*union' seems to agree. Please double check
> > > > > > that stuff to make sure this is actually safe.
> > > > > 
> > > > > I don't see any patch referring to compiler issues with 'git log --grep='anon.*union', what I see is other subsystems making use of it too.
> > > > > Can you share the commit hash that you are referring to?
> > > > 
> > > > $ git log --format=oneline --grep='anon.*union' -- drivers/gpu/drm/i915
> > > > 
> > > 
> > > I see issues with initialization of anonymous union but we don't initialize intel_dpll_hw_state.
> > > Also it was fixed on GCC 4.6 that is older than current GCC requirement to build kernel(GCC 5.1).
> > > 
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=10676
> > 
> > OK. However, after pondering this a bit I think naming things wpild
> > probably be better here. You have a bunch of if ladders now where each
> > branch only operates on one of the structs inside the union. IMO the
> > anonymity is making it rather hard to see if the code is even correct.
> 
> Just to mare sure we are in the same page, you want to have this?
> 
> 
> struct icl_tc {
> 	u32 mg_refclkin_ctl;
> 	u32 mg_clktop2_coreclkctl1;
> 	u32 mg_clktop2_hsclkctl;
> 	u32 mg_pll_div0;
> 	u32 mg_pll_div1;
> 	u32 mg_pll_lf;
> 	u32 mg_pll_frac_lock;
> 	u32 mg_pll_ssc;
> 	u32 mg_pll_bias;
> 	u32 mg_pll_tdc_coldst_bias;
> 	u32 mg_pll_bias_mask;
> 	u32 mg_pll_tdc_coldst_bias_mask;
> };

struct {
	....
} whatever;

In this case the name that immediately came to mind was just "mg"
(+ then drop the extra mg_ namespace in the members).

We could name the types too I guess if we wanted to use those
somewhere. Eg. instead of passing in the whole union to some
function we could just pass in the specific substruct.

> 
> So we would need to access members with icl_tc.mg_refclkin_ctl?
> 
> I can do that but the diff will be huge.
> Are you okay with that too Imre?
> 
> 
> > 
> 

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH v2] drm/i915/display: Use unions per platform in intel_dpll_hw_state

[Imre Deak]

On Thu, Feb 24, 2022 at 07:48:10PM +0200, Souza, Jose wrote:
> On Thu, 2022-02-24 at 17:39 +0200, Ville Syrjälä wrote:
> > On Thu, Feb 24, 2022 at 01:49:36PM +0000, Souza, Jose wrote:
> > > On Thu, 2022-02-24 at 15:25 +0200, Ville Syrjälä wrote:
> > > > On Thu, Feb 24, 2022 at 01:17:35PM +0000, Souza, Jose wrote:
> > > > > On Thu, 2022-02-24 at 12:20 +0200, Ville Syrjälä wrote:
> > > > > > On Wed, Feb 23, 2022 at 12:55:51PM -0800, José Roberto de Souza wrote:
> > > > > > <snip>
> > > > > > > +	union {
> > > > > > > +		/* icl+ TC */
> > > > > > > +		struct {
> > > > > > > +			u32 mg_refclkin_ctl;
> > > > > > > +			u32 mg_clktop2_coreclkctl1;
> > > > > > > +			u32 mg_clktop2_hsclkctl;
> > > > > > > +			u32 mg_pll_div0;
> > > > > > > +			u32 mg_pll_div1;
> > > > > > > +			u32 mg_pll_lf;
> > > > > > > +			u32 mg_pll_frac_lock;
> > > > > > > +			u32 mg_pll_ssc;
> > > > > > > +			u32 mg_pll_bias;
> > > > > > > +			u32 mg_pll_tdc_coldst_bias;
> > > > > > > +			u32 mg_pll_bias_mask;
> > > > > > > +			u32 mg_pll_tdc_coldst_bias_mask;
> > > > > > > +		};
> > > > > > > +
> > > > > > > +		/* bxt */
> > > > > > > +		struct {
> > > > > > > +			/* bxt */
> > > > > > > +			u32 ebb0;
> > > > > > > +			u32 ebb4;
> > > > > > > +			u32 pll0;
> > > > > > > +			u32 pll1;
> > > > > > > +			u32 pll2;
> > > > > > > +			u32 pll3;
> > > > > > > +			u32 pll6;
> > > > > > > +			u32 pll8;
> > > > > > > +			u32 pll9;
> > > > > > > +			u32 pll10;
> > > > > > > +			u32 pcsdw12;
> > > > > > > +		};
> > > > > > 
> > > > > > Wasn't there some funny compiler bug around anonymous unions?
> > > > > > git log --grep='anon.*union' seems to agree. Please double check
> > > > > > that stuff to make sure this is actually safe.
> > > > > 
> > > > > I don't see any patch referring to compiler issues with 'git log --grep='anon.*union', what I see is other subsystems making use of it too.
> > > > > Can you share the commit hash that you are referring to?
> > > > 
> > > > $ git log --format=oneline --grep='anon.*union' -- drivers/gpu/drm/i915
> > > > 
> > > 
> > > I see issues with initialization of anonymous union but we don't initialize intel_dpll_hw_state.
> > > Also it was fixed on GCC 4.6 that is older than current GCC requirement to build kernel(GCC 5.1).
> > > 
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=10676
> > 
> > OK. However, after pondering this a bit I think naming things wpild
> > probably be better here. You have a bunch of if ladders now where each
> > branch only operates on one of the structs inside the union. IMO the
> > anonymity is making it rather hard to see if the code is even correct.
> 
> Just to mare sure we are in the same page, you want to have this?
> 
> 
> struct icl_tc {
> 	u32 mg_refclkin_ctl;
> 	u32 mg_clktop2_coreclkctl1;
> 	u32 mg_clktop2_hsclkctl;
> 	u32 mg_pll_div0;
> 	u32 mg_pll_div1;
> 	u32 mg_pll_lf;
> 	u32 mg_pll_frac_lock;
> 	u32 mg_pll_ssc;
> 	u32 mg_pll_bias;
> 	u32 mg_pll_tdc_coldst_bias;
> 	u32 mg_pll_bias_mask;
> 	u32 mg_pll_tdc_coldst_bias_mask;
> };
> 
> So we would need to access members with icl_tc.mg_refclkin_ctl?
> 
> I can do that but the diff will be huge.
> Are you okay with that too Imre?

Yes, makes sense to clarify the type of PLL params at each place they
are used.