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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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