| Message ID | 20220223205551.813749-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915/display: Use unions per platform in intel_dpll_hw_state | expand |
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.
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? >
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
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
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.
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? >
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? > > > > >
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.
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; + }; + }; }; /**
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(-)