| Message ID | 20250211104857.3501566-9-suraj.kandpal@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm_i915_private to intel_display cleanup | expand |
On Tue, 11 Feb 2025, Suraj Kandpal <suraj.kandpal@intel.com> wrote: > Use intel_display as an argument for intel_shared_dpll_init() and > replace drm_i915_private in function wherever possible. > While at it prefer using display->platform.xx over IS_PLATFORM. > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > --- > .../drm/i915/display/intel_display_driver.c | 2 +- > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 49 ++++++++++--------- > drivers/gpu/drm/i915/display/intel_dpll_mgr.h | 3 +- > 3 files changed, 27 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c > index 978f530c810e..852f1129a058 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_driver.c > +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c > @@ -448,7 +448,7 @@ int intel_display_driver_probe_nogem(struct intel_display *display) > } > > intel_plane_possible_crtcs_init(display); > - intel_shared_dpll_init(i915); > + intel_shared_dpll_init(display); > intel_fdi_pll_freq_update(i915); > > intel_update_czclk(i915); > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > index f94da1ffc8ce..26b6b9372fa3 100644 > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > @@ -2042,8 +2042,8 @@ static void bxt_ddi_pll_enable(struct intel_display *display, > { > const struct bxt_dpll_hw_state *hw_state = &dpll_hw_state->bxt; > enum port port = (enum port)pll->info->id; /* 1:1 port->PLL mapping */ > - enum dpio_phy phy; > - enum dpio_channel ch; > + enum dpio_phy phy = DPIO_PHY0; > + enum dpio_channel ch = DPIO_CH0; Unrelated change, please drop. With that fixed, Reviewed-by: Jani Nikula <jani.nikula@intel.com> > u32 temp; > > bxt_port_to_phy_channel(display, port, &phy, &ch); > @@ -4302,40 +4302,41 @@ static const struct intel_dpll_mgr adlp_pll_mgr = { > > /** > * intel_shared_dpll_init - Initialize shared DPLLs > - * @i915: i915 device > + * @display: intel_display device > * > - * Initialize shared DPLLs for @i915. > + * Initialize shared DPLLs for @display. > */ > -void intel_shared_dpll_init(struct drm_i915_private *i915) > +void intel_shared_dpll_init(struct intel_display *display) > { > + struct drm_i915_private *i915 = to_i915(display->drm); > const struct intel_dpll_mgr *dpll_mgr = NULL; > const struct dpll_info *dpll_info; > int i; > > - mutex_init(&i915->display.dpll.lock); > + mutex_init(&display->dpll.lock); > > - if (DISPLAY_VER(i915) >= 14 || IS_DG2(i915)) > + if (DISPLAY_VER(display) >= 14 || display->platform.dg2) > /* No shared DPLLs on DG2; port PLLs are part of the PHY */ > dpll_mgr = NULL; > - else if (IS_ALDERLAKE_P(i915)) > + else if (display->platform.alderlake_p) > dpll_mgr = &adlp_pll_mgr; > - else if (IS_ALDERLAKE_S(i915)) > + else if (display->platform.alderlake_s) > dpll_mgr = &adls_pll_mgr; > - else if (IS_DG1(i915)) > + else if (display->platform.dg1) > dpll_mgr = &dg1_pll_mgr; > - else if (IS_ROCKETLAKE(i915)) > + else if (display->platform.rocketlake) > dpll_mgr = &rkl_pll_mgr; > - else if (DISPLAY_VER(i915) >= 12) > + else if (DISPLAY_VER(display) >= 12) > dpll_mgr = &tgl_pll_mgr; > - else if (IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) > + else if (display->platform.jasperlake || display->platform.elkhartlake) > dpll_mgr = &ehl_pll_mgr; > - else if (DISPLAY_VER(i915) >= 11) > + else if (DISPLAY_VER(display) >= 11) > dpll_mgr = &icl_pll_mgr; > - else if (IS_GEMINILAKE(i915) || IS_BROXTON(i915)) > + else if (display->platform.geminilake || display->platform.broxton) > dpll_mgr = &bxt_pll_mgr; > - else if (DISPLAY_VER(i915) == 9) > + else if (DISPLAY_VER(display) == 9) > dpll_mgr = &skl_pll_mgr; > - else if (HAS_DDI(i915)) > + else if (HAS_DDI(display)) > dpll_mgr = &hsw_pll_mgr; > else if (HAS_PCH_IBX(i915) || HAS_PCH_CPT(i915)) > dpll_mgr = &pch_pll_mgr; > @@ -4346,20 +4347,20 @@ void intel_shared_dpll_init(struct drm_i915_private *i915) > dpll_info = dpll_mgr->dpll_info; > > for (i = 0; dpll_info[i].name; i++) { > - if (drm_WARN_ON(&i915->drm, > - i >= ARRAY_SIZE(i915->display.dpll.shared_dplls))) > + if (drm_WARN_ON(display->drm, > + i >= ARRAY_SIZE(display->dpll.shared_dplls))) > break; > > /* must fit into unsigned long bitmask on 32bit */ > - if (drm_WARN_ON(&i915->drm, dpll_info[i].id >= 32)) > + if (drm_WARN_ON(display->drm, dpll_info[i].id >= 32)) > break; > > - i915->display.dpll.shared_dplls[i].info = &dpll_info[i]; > - i915->display.dpll.shared_dplls[i].index = i; > + display->dpll.shared_dplls[i].info = &dpll_info[i]; > + display->dpll.shared_dplls[i].index = i; > } > > - i915->display.dpll.mgr = dpll_mgr; > - i915->display.dpll.num_shared_dpll = i; > + display->dpll.mgr = dpll_mgr; > + display->dpll.num_shared_dpll = i; > } > > /** > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > index 3d988f17f31d..caffb084830c 100644 > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > @@ -35,7 +35,6 @@ > ((__pll) = &(__display)->dpll.shared_dplls[(__i)]) ; (__i)++) > > enum tc_port; > -struct drm_i915_private; > struct drm_printer; > struct intel_atomic_state; > struct intel_crtc; > @@ -422,7 +421,7 @@ bool intel_dpll_get_hw_state(struct intel_display *display, > void intel_enable_shared_dpll(const struct intel_crtc_state *crtc_state); > void intel_disable_shared_dpll(const struct intel_crtc_state *crtc_state); > void intel_shared_dpll_swap_state(struct intel_atomic_state *state); > -void intel_shared_dpll_init(struct drm_i915_private *i915); > +void intel_shared_dpll_init(struct intel_display *display); > void intel_dpll_update_ref_clks(struct intel_display *display); > void intel_dpll_readout_hw_state(struct intel_display *display); > void intel_dpll_sanitize_state(struct intel_display *display);
> -----Original Message----- > From: Nikula, Jani <jani.nikula@intel.com> > Sent: Tuesday, February 11, 2025 6:45 PM > To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel- > xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org > Cc: Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; Kandpal, Suraj > <suraj.kandpal@intel.com> > Subject: Re: [PATCH 8/9] drm/i915/dpll: Accept intel_display as argument for > shared_dpll_init > > On Tue, 11 Feb 2025, Suraj Kandpal <suraj.kandpal@intel.com> wrote: > > Use intel_display as an argument for intel_shared_dpll_init() and > > replace drm_i915_private in function wherever possible. > > While at it prefer using display->platform.xx over IS_PLATFORM. > > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > --- > > .../drm/i915/display/intel_display_driver.c | 2 +- > > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 49 > > ++++++++++--------- drivers/gpu/drm/i915/display/intel_dpll_mgr.h | > > 3 +- > > 3 files changed, 27 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c > > b/drivers/gpu/drm/i915/display/intel_display_driver.c > > index 978f530c810e..852f1129a058 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_driver.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c > > @@ -448,7 +448,7 @@ int intel_display_driver_probe_nogem(struct > intel_display *display) > > } > > > > intel_plane_possible_crtcs_init(display); > > - intel_shared_dpll_init(i915); > > + intel_shared_dpll_init(display); > > intel_fdi_pll_freq_update(i915); > > > > intel_update_czclk(i915); > > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > > b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > > index f94da1ffc8ce..26b6b9372fa3 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > > @@ -2042,8 +2042,8 @@ static void bxt_ddi_pll_enable(struct > > intel_display *display, { > > const struct bxt_dpll_hw_state *hw_state = &dpll_hw_state->bxt; > > enum port port = (enum port)pll->info->id; /* 1:1 port->PLL mapping > */ > > - enum dpio_phy phy; > > - enum dpio_channel ch; > > + enum dpio_phy phy = DPIO_PHY0; > > + enum dpio_channel ch = DPIO_CH0; > > Unrelated change, please drop. > The problem is by dropping these changes I am not able to build the kernel and it throws the following warning because of which I had to add this drivers/gpu/drm/i915/display/intel_dpll_mgr.c: In function _bxt_ddi_pll_enable_: ./drivers/gpu/drm/xe/compat-i915-headers/../../i915/i915_reg_defs.h:240:56: error: _phy_ is used uninitialized [-Werror=uniniti alized] 240 | (BUILD_BUG_ON_ZERO(!__is_constexpr(__c_index)) + \ | ^ drivers/gpu/drm/i915/display/intel_dpll_mgr.c:2047:23: note: _phy_ was declared here 2047 | enum dpio_phy phy; | ^~~ In file included from ./drivers/gpu/drm/xe/compat-i915-headers/i915_reg_defs.h:6, from drivers/gpu/drm/i915/display/intel_display_reg_defs.h:9, from drivers/gpu/drm/i915/display/bxt_dpio_phy_regs.h:9, from drivers/gpu/drm/i915/display/intel_dpll_mgr.c:27: ./drivers/gpu/drm/xe/compat-i915-headers/../../i915/i915_reg_defs.h:213:58: error: _ch_ is used uninitialized [-Werror=uninitia lized] 213 | #define _PICK_EVEN(__index, __a, __b) ((__a) + (__index) * ((__b) - (__a))) | ^ drivers/gpu/drm/i915/display/intel_dpll_mgr.c:2048:27: note: _ch_ was declared here 2048 | enum dpio_channel ch; Regards, Suraj Kandpal > With that fixed, > > Reviewed-by: Jani Nikula <jani.nikula@intel.com> > > > > u32 temp; > > > > bxt_port_to_phy_channel(display, port, &phy, &ch); @@ -4302,40 > > +4302,41 @@ static const struct intel_dpll_mgr adlp_pll_mgr = { > > > > /** > > * intel_shared_dpll_init - Initialize shared DPLLs > > - * @i915: i915 device > > + * @display: intel_display device > > * > > - * Initialize shared DPLLs for @i915. > > + * Initialize shared DPLLs for @display. > > */ > > -void intel_shared_dpll_init(struct drm_i915_private *i915) > > +void intel_shared_dpll_init(struct intel_display *display) > > { > > + struct drm_i915_private *i915 = to_i915(display->drm); > > const struct intel_dpll_mgr *dpll_mgr = NULL; > > const struct dpll_info *dpll_info; > > int i; > > > > - mutex_init(&i915->display.dpll.lock); > > + mutex_init(&display->dpll.lock); > > > > - if (DISPLAY_VER(i915) >= 14 || IS_DG2(i915)) > > + if (DISPLAY_VER(display) >= 14 || display->platform.dg2) > > /* No shared DPLLs on DG2; port PLLs are part of the PHY */ > > dpll_mgr = NULL; > > - else if (IS_ALDERLAKE_P(i915)) > > + else if (display->platform.alderlake_p) > > dpll_mgr = &adlp_pll_mgr; > > - else if (IS_ALDERLAKE_S(i915)) > > + else if (display->platform.alderlake_s) > > dpll_mgr = &adls_pll_mgr; > > - else if (IS_DG1(i915)) > > + else if (display->platform.dg1) > > dpll_mgr = &dg1_pll_mgr; > > - else if (IS_ROCKETLAKE(i915)) > > + else if (display->platform.rocketlake) > > dpll_mgr = &rkl_pll_mgr; > > - else if (DISPLAY_VER(i915) >= 12) > > + else if (DISPLAY_VER(display) >= 12) > > dpll_mgr = &tgl_pll_mgr; > > - else if (IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) > > + else if (display->platform.jasperlake || > > +display->platform.elkhartlake) > > dpll_mgr = &ehl_pll_mgr; > > - else if (DISPLAY_VER(i915) >= 11) > > + else if (DISPLAY_VER(display) >= 11) > > dpll_mgr = &icl_pll_mgr; > > - else if (IS_GEMINILAKE(i915) || IS_BROXTON(i915)) > > + else if (display->platform.geminilake || display->platform.broxton) > > dpll_mgr = &bxt_pll_mgr; > > - else if (DISPLAY_VER(i915) == 9) > > + else if (DISPLAY_VER(display) == 9) > > dpll_mgr = &skl_pll_mgr; > > - else if (HAS_DDI(i915)) > > + else if (HAS_DDI(display)) > > dpll_mgr = &hsw_pll_mgr; > > else if (HAS_PCH_IBX(i915) || HAS_PCH_CPT(i915)) > > dpll_mgr = &pch_pll_mgr; > > @@ -4346,20 +4347,20 @@ void intel_shared_dpll_init(struct > drm_i915_private *i915) > > dpll_info = dpll_mgr->dpll_info; > > > > for (i = 0; dpll_info[i].name; i++) { > > - if (drm_WARN_ON(&i915->drm, > > - i >= ARRAY_SIZE(i915- > >display.dpll.shared_dplls))) > > + if (drm_WARN_ON(display->drm, > > + i >= ARRAY_SIZE(display->dpll.shared_dplls))) > > break; > > > > /* must fit into unsigned long bitmask on 32bit */ > > - if (drm_WARN_ON(&i915->drm, dpll_info[i].id >= 32)) > > + if (drm_WARN_ON(display->drm, dpll_info[i].id >= 32)) > > break; > > > > - i915->display.dpll.shared_dplls[i].info = &dpll_info[i]; > > - i915->display.dpll.shared_dplls[i].index = i; > > + display->dpll.shared_dplls[i].info = &dpll_info[i]; > > + display->dpll.shared_dplls[i].index = i; > > } > > > > - i915->display.dpll.mgr = dpll_mgr; > > - i915->display.dpll.num_shared_dpll = i; > > + display->dpll.mgr = dpll_mgr; > > + display->dpll.num_shared_dpll = i; > > } > > > > /** > > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > > b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > > index 3d988f17f31d..caffb084830c 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > > @@ -35,7 +35,6 @@ > > ((__pll) = &(__display)->dpll.shared_dplls[(__i)]) ; (__i)++) > > > > enum tc_port; > > -struct drm_i915_private; > > struct drm_printer; > > struct intel_atomic_state; > > struct intel_crtc; > > @@ -422,7 +421,7 @@ bool intel_dpll_get_hw_state(struct intel_display > > *display, void intel_enable_shared_dpll(const struct intel_crtc_state > > *crtc_state); void intel_disable_shared_dpll(const struct > > intel_crtc_state *crtc_state); void > > intel_shared_dpll_swap_state(struct intel_atomic_state *state); -void > > intel_shared_dpll_init(struct drm_i915_private *i915); > > +void intel_shared_dpll_init(struct intel_display *display); > > void intel_dpll_update_ref_clks(struct intel_display *display); void > > intel_dpll_readout_hw_state(struct intel_display *display); void > > intel_dpll_sanitize_state(struct intel_display *display); > > -- > Jani Nikula, Intel
On Tue, 11 Feb 2025, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote: >> -----Original Message----- >> From: Nikula, Jani <jani.nikula@intel.com> >> Sent: Tuesday, February 11, 2025 6:45 PM >> To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel- >> xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org >> Cc: Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; Kandpal, Suraj >> <suraj.kandpal@intel.com> >> Subject: Re: [PATCH 8/9] drm/i915/dpll: Accept intel_display as argument for >> shared_dpll_init >> >> On Tue, 11 Feb 2025, Suraj Kandpal <suraj.kandpal@intel.com> wrote: >> > Use intel_display as an argument for intel_shared_dpll_init() and >> > replace drm_i915_private in function wherever possible. >> > While at it prefer using display->platform.xx over IS_PLATFORM. >> > >> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> >> > --- >> > .../drm/i915/display/intel_display_driver.c | 2 +- >> > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 49 >> > ++++++++++--------- drivers/gpu/drm/i915/display/intel_dpll_mgr.h | >> > 3 +- >> > 3 files changed, 27 insertions(+), 27 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c >> > b/drivers/gpu/drm/i915/display/intel_display_driver.c >> > index 978f530c810e..852f1129a058 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_display_driver.c >> > +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c >> > @@ -448,7 +448,7 @@ int intel_display_driver_probe_nogem(struct >> intel_display *display) >> > } >> > >> > intel_plane_possible_crtcs_init(display); >> > - intel_shared_dpll_init(i915); >> > + intel_shared_dpll_init(display); >> > intel_fdi_pll_freq_update(i915); >> > >> > intel_update_czclk(i915); >> > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c >> > b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c >> > index f94da1ffc8ce..26b6b9372fa3 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c >> > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c >> > @@ -2042,8 +2042,8 @@ static void bxt_ddi_pll_enable(struct >> > intel_display *display, { >> > const struct bxt_dpll_hw_state *hw_state = &dpll_hw_state->bxt; >> > enum port port = (enum port)pll->info->id; /* 1:1 port->PLL mapping >> */ >> > - enum dpio_phy phy; >> > - enum dpio_channel ch; >> > + enum dpio_phy phy = DPIO_PHY0; >> > + enum dpio_channel ch = DPIO_CH0; >> >> Unrelated change, please drop. >> > > The problem is by dropping these changes I am not able to build the kernel and it throws the following warning because of which I had to add this > > > drivers/gpu/drm/i915/display/intel_dpll_mgr.c: In function _bxt_ddi_pll_enable_: > ./drivers/gpu/drm/xe/compat-i915-headers/../../i915/i915_reg_defs.h:240:56: error: _phy_ is used uninitialized [-Werror=uniniti > alized] > 240 | (BUILD_BUG_ON_ZERO(!__is_constexpr(__c_index)) + \ > | ^ > drivers/gpu/drm/i915/display/intel_dpll_mgr.c:2047:23: note: _phy_ was declared here > 2047 | enum dpio_phy phy; > | ^~~ > In file included from ./drivers/gpu/drm/xe/compat-i915-headers/i915_reg_defs.h:6, > from drivers/gpu/drm/i915/display/intel_display_reg_defs.h:9, > from drivers/gpu/drm/i915/display/bxt_dpio_phy_regs.h:9, > from drivers/gpu/drm/i915/display/intel_dpll_mgr.c:27: > ./drivers/gpu/drm/xe/compat-i915-headers/../../i915/i915_reg_defs.h:213:58: error: _ch_ is used uninitialized [-Werror=uninitia > lized] > 213 | #define _PICK_EVEN(__index, __a, __b) ((__a) + (__index) * ((__b) - (__a))) > | ^ > drivers/gpu/drm/i915/display/intel_dpll_mgr.c:2048:27: note: _ch_ was declared here > 2048 | enum dpio_channel ch; Did you think to look into why this happens? I encourage you to always do that instead of just silencing the warning. You'll learn about C and the compiler. It's quite interesting and subtle and deserves to be mentioned in the commit message. For i915.ko, bxt_port_to_phy_channel() is a regular function, and the compiler will likely assume it'll initialize the parameters. And it does. For xe.ko, bxt_port_to_phy_channel() is a static inline stub, and the compiler can be absolutely certain the parameters aren't initialized. So, why does this cause an error now? The above didn't change now! With IS_GEMINILAKE() || IS_BROXTON() the compiler can be sure it's false for xe.ko. The whole chain bxt_pll_mgr -> bxt_plls -> bxt_ddi_pll_funcs -> bxt_ddi_pll_enable can be optimized away. It's unreachable. Not so with display->platform.geminilake || display->platform.broxton. The compiler sees use of uninitialized variables. BR, Jani. > > Regards, > Suraj Kandpal > >> With that fixed, >> >> Reviewed-by: Jani Nikula <jani.nikula@intel.com> >> >> >> > u32 temp; >> > >> > bxt_port_to_phy_channel(display, port, &phy, &ch); @@ -4302,40 >> > +4302,41 @@ static const struct intel_dpll_mgr adlp_pll_mgr = { >> > >> > /** >> > * intel_shared_dpll_init - Initialize shared DPLLs >> > - * @i915: i915 device >> > + * @display: intel_display device >> > * >> > - * Initialize shared DPLLs for @i915. >> > + * Initialize shared DPLLs for @display. >> > */ >> > -void intel_shared_dpll_init(struct drm_i915_private *i915) >> > +void intel_shared_dpll_init(struct intel_display *display) >> > { >> > + struct drm_i915_private *i915 = to_i915(display->drm); >> > const struct intel_dpll_mgr *dpll_mgr = NULL; >> > const struct dpll_info *dpll_info; >> > int i; >> > >> > - mutex_init(&i915->display.dpll.lock); >> > + mutex_init(&display->dpll.lock); >> > >> > - if (DISPLAY_VER(i915) >= 14 || IS_DG2(i915)) >> > + if (DISPLAY_VER(display) >= 14 || display->platform.dg2) >> > /* No shared DPLLs on DG2; port PLLs are part of the PHY */ >> > dpll_mgr = NULL; >> > - else if (IS_ALDERLAKE_P(i915)) >> > + else if (display->platform.alderlake_p) >> > dpll_mgr = &adlp_pll_mgr; >> > - else if (IS_ALDERLAKE_S(i915)) >> > + else if (display->platform.alderlake_s) >> > dpll_mgr = &adls_pll_mgr; >> > - else if (IS_DG1(i915)) >> > + else if (display->platform.dg1) >> > dpll_mgr = &dg1_pll_mgr; >> > - else if (IS_ROCKETLAKE(i915)) >> > + else if (display->platform.rocketlake) >> > dpll_mgr = &rkl_pll_mgr; >> > - else if (DISPLAY_VER(i915) >= 12) >> > + else if (DISPLAY_VER(display) >= 12) >> > dpll_mgr = &tgl_pll_mgr; >> > - else if (IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) >> > + else if (display->platform.jasperlake || >> > +display->platform.elkhartlake) >> > dpll_mgr = &ehl_pll_mgr; >> > - else if (DISPLAY_VER(i915) >= 11) >> > + else if (DISPLAY_VER(display) >= 11) >> > dpll_mgr = &icl_pll_mgr; >> > - else if (IS_GEMINILAKE(i915) || IS_BROXTON(i915)) >> > + else if (display->platform.geminilake || display->platform.broxton) >> > dpll_mgr = &bxt_pll_mgr; >> > - else if (DISPLAY_VER(i915) == 9) >> > + else if (DISPLAY_VER(display) == 9) >> > dpll_mgr = &skl_pll_mgr; >> > - else if (HAS_DDI(i915)) >> > + else if (HAS_DDI(display)) >> > dpll_mgr = &hsw_pll_mgr; >> > else if (HAS_PCH_IBX(i915) || HAS_PCH_CPT(i915)) >> > dpll_mgr = &pch_pll_mgr; >> > @@ -4346,20 +4347,20 @@ void intel_shared_dpll_init(struct >> drm_i915_private *i915) >> > dpll_info = dpll_mgr->dpll_info; >> > >> > for (i = 0; dpll_info[i].name; i++) { >> > - if (drm_WARN_ON(&i915->drm, >> > - i >= ARRAY_SIZE(i915- >> >display.dpll.shared_dplls))) >> > + if (drm_WARN_ON(display->drm, >> > + i >= ARRAY_SIZE(display->dpll.shared_dplls))) >> > break; >> > >> > /* must fit into unsigned long bitmask on 32bit */ >> > - if (drm_WARN_ON(&i915->drm, dpll_info[i].id >= 32)) >> > + if (drm_WARN_ON(display->drm, dpll_info[i].id >= 32)) >> > break; >> > >> > - i915->display.dpll.shared_dplls[i].info = &dpll_info[i]; >> > - i915->display.dpll.shared_dplls[i].index = i; >> > + display->dpll.shared_dplls[i].info = &dpll_info[i]; >> > + display->dpll.shared_dplls[i].index = i; >> > } >> > >> > - i915->display.dpll.mgr = dpll_mgr; >> > - i915->display.dpll.num_shared_dpll = i; >> > + display->dpll.mgr = dpll_mgr; >> > + display->dpll.num_shared_dpll = i; >> > } >> > >> > /** >> > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h >> > b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h >> > index 3d988f17f31d..caffb084830c 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h >> > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h >> > @@ -35,7 +35,6 @@ >> > ((__pll) = &(__display)->dpll.shared_dplls[(__i)]) ; (__i)++) >> > >> > enum tc_port; >> > -struct drm_i915_private; >> > struct drm_printer; >> > struct intel_atomic_state; >> > struct intel_crtc; >> > @@ -422,7 +421,7 @@ bool intel_dpll_get_hw_state(struct intel_display >> > *display, void intel_enable_shared_dpll(const struct intel_crtc_state >> > *crtc_state); void intel_disable_shared_dpll(const struct >> > intel_crtc_state *crtc_state); void >> > intel_shared_dpll_swap_state(struct intel_atomic_state *state); -void >> > intel_shared_dpll_init(struct drm_i915_private *i915); >> > +void intel_shared_dpll_init(struct intel_display *display); >> > void intel_dpll_update_ref_clks(struct intel_display *display); void >> > intel_dpll_readout_hw_state(struct intel_display *display); void >> > intel_dpll_sanitize_state(struct intel_display *display); >> >> -- >> Jani Nikula, Intel
> -----Original Message----- > From: Nikula, Jani <jani.nikula@intel.com> > Sent: Tuesday, February 11, 2025 10:27 PM > To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel- > xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org > Cc: Nautiyal, Ankit K <ankit.k.nautiyal@intel.com> > Subject: RE: [PATCH 8/9] drm/i915/dpll: Accept intel_display as argument for > shared_dpll_init > > On Tue, 11 Feb 2025, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote: > >> -----Original Message----- > >> From: Nikula, Jani <jani.nikula@intel.com> > >> Sent: Tuesday, February 11, 2025 6:45 PM > >> To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel- > >> xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org > >> Cc: Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; Kandpal, Suraj > >> <suraj.kandpal@intel.com> > >> Subject: Re: [PATCH 8/9] drm/i915/dpll: Accept intel_display as > >> argument for shared_dpll_init > >> > >> On Tue, 11 Feb 2025, Suraj Kandpal <suraj.kandpal@intel.com> wrote: > >> > Use intel_display as an argument for intel_shared_dpll_init() and > >> > replace drm_i915_private in function wherever possible. > >> > While at it prefer using display->platform.xx over IS_PLATFORM. > >> > > >> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > >> > --- > >> > .../drm/i915/display/intel_display_driver.c | 2 +- > >> > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 49 > >> > ++++++++++--------- drivers/gpu/drm/i915/display/intel_dpll_mgr.h > >> > ++++++++++| > >> > 3 +- > >> > 3 files changed, 27 insertions(+), 27 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c > >> > b/drivers/gpu/drm/i915/display/intel_display_driver.c > >> > index 978f530c810e..852f1129a058 100644 > >> > --- a/drivers/gpu/drm/i915/display/intel_display_driver.c > >> > +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c > >> > @@ -448,7 +448,7 @@ int intel_display_driver_probe_nogem(struct > >> intel_display *display) > >> > } > >> > > >> > intel_plane_possible_crtcs_init(display); > >> > - intel_shared_dpll_init(i915); > >> > + intel_shared_dpll_init(display); > >> > intel_fdi_pll_freq_update(i915); > >> > > >> > intel_update_czclk(i915); > >> > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > >> > b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > >> > index f94da1ffc8ce..26b6b9372fa3 100644 > >> > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > >> > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > >> > @@ -2042,8 +2042,8 @@ static void bxt_ddi_pll_enable(struct > >> > intel_display *display, { > >> > const struct bxt_dpll_hw_state *hw_state = &dpll_hw_state->bxt; > >> > enum port port = (enum port)pll->info->id; /* 1:1 port->PLL > >> > mapping > >> */ > >> > - enum dpio_phy phy; > >> > - enum dpio_channel ch; > >> > + enum dpio_phy phy = DPIO_PHY0; > >> > + enum dpio_channel ch = DPIO_CH0; > >> > >> Unrelated change, please drop. > >> > > > > The problem is by dropping these changes I am not able to build the > > kernel and it throws the following warning because of which I had to > > add this > > > > > > drivers/gpu/drm/i915/display/intel_dpll_mgr.c: In function > _bxt_ddi_pll_enable_: > > ./drivers/gpu/drm/xe/compat-i915-headers/../../i915/i915_reg_defs.h:24 > > 0:56: error: _phy_ is used uninitialized [-Werror=uniniti alized] > > 240 | (BUILD_BUG_ON_ZERO(!__is_constexpr(__c_index)) + > \ > > | ^ > > drivers/gpu/drm/i915/display/intel_dpll_mgr.c:2047:23: note: _phy_ was > declared here > > 2047 | enum dpio_phy phy; > > | ^~~ > > In file included from ./drivers/gpu/drm/xe/compat-i915- > headers/i915_reg_defs.h:6, > > from drivers/gpu/drm/i915/display/intel_display_reg_defs.h:9, > > from drivers/gpu/drm/i915/display/bxt_dpio_phy_regs.h:9, > > from drivers/gpu/drm/i915/display/intel_dpll_mgr.c:27: > > ./drivers/gpu/drm/xe/compat-i915-headers/../../i915/i915_reg_defs.h:21 > > 3:58: error: _ch_ is used uninitialized [-Werror=uninitia lized] > > 213 | #define _PICK_EVEN(__index, __a, __b) ((__a) + (__index) * ((__b) - > (__a))) > > | ^ > > drivers/gpu/drm/i915/display/intel_dpll_mgr.c:2048:27: note: _ch_ was > declared here > > 2048 | enum dpio_channel ch; > > Did you think to look into why this happens? I encourage you to always do > that instead of just silencing the warning. You'll learn about C and the > compiler. > > It's quite interesting and subtle and deserves to be mentioned in the > commit message. > > For i915.ko, bxt_port_to_phy_channel() is a regular function, and the > compiler will likely assume it'll initialize the parameters. And it does. > > For xe.ko, bxt_port_to_phy_channel() is a static inline stub, and the > compiler can be absolutely certain the parameters aren't initialized. > > So, why does this cause an error now? The above didn't change now! > > With IS_GEMINILAKE() || IS_BROXTON() the compiler can be sure it's false > for xe.ko. The whole chain bxt_pll_mgr -> bxt_plls -> bxt_ddi_pll_funcs > -> bxt_ddi_pll_enable can be optimized away. It's unreachable. > > Not so with display->platform.geminilake || > display->platform.broxton. The compiler sees use of uninitialized > variables. Ahh ohkay, I understand it now thanks for explaining it. Will amend the commit message Will explore these things and add them to commit message in future too. Regards, Suraj Kandpal > > BR, > Jani. > > > > > > > Regards, > > Suraj Kandpal > > > >> With that fixed, > >> > >> Reviewed-by: Jani Nikula <jani.nikula@intel.com> > >> > >> > >> > u32 temp; > >> > > >> > bxt_port_to_phy_channel(display, port, &phy, &ch); @@ -4302,40 > >> > +4302,41 @@ static const struct intel_dpll_mgr adlp_pll_mgr = { > >> > > >> > /** > >> > * intel_shared_dpll_init - Initialize shared DPLLs > >> > - * @i915: i915 device > >> > + * @display: intel_display device > >> > * > >> > - * Initialize shared DPLLs for @i915. > >> > + * Initialize shared DPLLs for @display. > >> > */ > >> > -void intel_shared_dpll_init(struct drm_i915_private *i915) > >> > +void intel_shared_dpll_init(struct intel_display *display) > >> > { > >> > + struct drm_i915_private *i915 = to_i915(display->drm); > >> > const struct intel_dpll_mgr *dpll_mgr = NULL; > >> > const struct dpll_info *dpll_info; > >> > int i; > >> > > >> > - mutex_init(&i915->display.dpll.lock); > >> > + mutex_init(&display->dpll.lock); > >> > > >> > - if (DISPLAY_VER(i915) >= 14 || IS_DG2(i915)) > >> > + if (DISPLAY_VER(display) >= 14 || display->platform.dg2) > >> > /* No shared DPLLs on DG2; port PLLs are part of the PHY */ > >> > dpll_mgr = NULL; > >> > - else if (IS_ALDERLAKE_P(i915)) > >> > + else if (display->platform.alderlake_p) > >> > dpll_mgr = &adlp_pll_mgr; > >> > - else if (IS_ALDERLAKE_S(i915)) > >> > + else if (display->platform.alderlake_s) > >> > dpll_mgr = &adls_pll_mgr; > >> > - else if (IS_DG1(i915)) > >> > + else if (display->platform.dg1) > >> > dpll_mgr = &dg1_pll_mgr; > >> > - else if (IS_ROCKETLAKE(i915)) > >> > + else if (display->platform.rocketlake) > >> > dpll_mgr = &rkl_pll_mgr; > >> > - else if (DISPLAY_VER(i915) >= 12) > >> > + else if (DISPLAY_VER(display) >= 12) > >> > dpll_mgr = &tgl_pll_mgr; > >> > - else if (IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) > >> > + else if (display->platform.jasperlake || > >> > +display->platform.elkhartlake) > >> > dpll_mgr = &ehl_pll_mgr; > >> > - else if (DISPLAY_VER(i915) >= 11) > >> > + else if (DISPLAY_VER(display) >= 11) > >> > dpll_mgr = &icl_pll_mgr; > >> > - else if (IS_GEMINILAKE(i915) || IS_BROXTON(i915)) > >> > + else if (display->platform.geminilake || > >> > + display->platform.broxton) > >> > dpll_mgr = &bxt_pll_mgr; > >> > - else if (DISPLAY_VER(i915) == 9) > >> > + else if (DISPLAY_VER(display) == 9) > >> > dpll_mgr = &skl_pll_mgr; > >> > - else if (HAS_DDI(i915)) > >> > + else if (HAS_DDI(display)) > >> > dpll_mgr = &hsw_pll_mgr; > >> > else if (HAS_PCH_IBX(i915) || HAS_PCH_CPT(i915)) > >> > dpll_mgr = &pch_pll_mgr; @@ -4346,20 +4347,20 @@ void > >> > intel_shared_dpll_init(struct > >> drm_i915_private *i915) > >> > dpll_info = dpll_mgr->dpll_info; > >> > > >> > for (i = 0; dpll_info[i].name; i++) { > >> > - if (drm_WARN_ON(&i915->drm, > >> > - i >= ARRAY_SIZE(i915- > >> >display.dpll.shared_dplls))) > >> > + if (drm_WARN_ON(display->drm, > >> > + i >= > >> > + ARRAY_SIZE(display->dpll.shared_dplls))) > >> > break; > >> > > >> > /* must fit into unsigned long bitmask on 32bit */ > >> > - if (drm_WARN_ON(&i915->drm, dpll_info[i].id >= 32)) > >> > + if (drm_WARN_ON(display->drm, dpll_info[i].id >= 32)) > >> > break; > >> > > >> > - i915->display.dpll.shared_dplls[i].info = &dpll_info[i]; > >> > - i915->display.dpll.shared_dplls[i].index = i; > >> > + display->dpll.shared_dplls[i].info = &dpll_info[i]; > >> > + display->dpll.shared_dplls[i].index = i; > >> > } > >> > > >> > - i915->display.dpll.mgr = dpll_mgr; > >> > - i915->display.dpll.num_shared_dpll = i; > >> > + display->dpll.mgr = dpll_mgr; > >> > + display->dpll.num_shared_dpll = i; > >> > } > >> > > >> > /** > >> > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > >> > b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > >> > index 3d988f17f31d..caffb084830c 100644 > >> > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > >> > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > >> > @@ -35,7 +35,6 @@ > >> > ((__pll) = &(__display)->dpll.shared_dplls[(__i)]) > >> > ; (__i)++) > >> > > >> > enum tc_port; > >> > -struct drm_i915_private; > >> > struct drm_printer; > >> > struct intel_atomic_state; > >> > struct intel_crtc; > >> > @@ -422,7 +421,7 @@ bool intel_dpll_get_hw_state(struct > >> > intel_display *display, void intel_enable_shared_dpll(const struct > >> > intel_crtc_state *crtc_state); void > >> > intel_disable_shared_dpll(const struct intel_crtc_state > >> > *crtc_state); void intel_shared_dpll_swap_state(struct > >> > intel_atomic_state *state); -void intel_shared_dpll_init(struct > >> > drm_i915_private *i915); > >> > +void intel_shared_dpll_init(struct intel_display *display); > >> > void intel_dpll_update_ref_clks(struct intel_display *display); > >> > void intel_dpll_readout_hw_state(struct intel_display *display); > >> > void intel_dpll_sanitize_state(struct intel_display *display); > >> > >> -- > >> Jani Nikula, Intel > > -- > Jani Nikula, Intel
diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c index 978f530c810e..852f1129a058 100644 --- a/drivers/gpu/drm/i915/display/intel_display_driver.c +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c @@ -448,7 +448,7 @@ int intel_display_driver_probe_nogem(struct intel_display *display) } intel_plane_possible_crtcs_init(display); - intel_shared_dpll_init(i915); + intel_shared_dpll_init(display); intel_fdi_pll_freq_update(i915); intel_update_czclk(i915); diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c index f94da1ffc8ce..26b6b9372fa3 100644 --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c @@ -2042,8 +2042,8 @@ static void bxt_ddi_pll_enable(struct intel_display *display, { const struct bxt_dpll_hw_state *hw_state = &dpll_hw_state->bxt; enum port port = (enum port)pll->info->id; /* 1:1 port->PLL mapping */ - enum dpio_phy phy; - enum dpio_channel ch; + enum dpio_phy phy = DPIO_PHY0; + enum dpio_channel ch = DPIO_CH0; u32 temp; bxt_port_to_phy_channel(display, port, &phy, &ch); @@ -4302,40 +4302,41 @@ static const struct intel_dpll_mgr adlp_pll_mgr = { /** * intel_shared_dpll_init - Initialize shared DPLLs - * @i915: i915 device + * @display: intel_display device * - * Initialize shared DPLLs for @i915. + * Initialize shared DPLLs for @display. */ -void intel_shared_dpll_init(struct drm_i915_private *i915) +void intel_shared_dpll_init(struct intel_display *display) { + struct drm_i915_private *i915 = to_i915(display->drm); const struct intel_dpll_mgr *dpll_mgr = NULL; const struct dpll_info *dpll_info; int i; - mutex_init(&i915->display.dpll.lock); + mutex_init(&display->dpll.lock); - if (DISPLAY_VER(i915) >= 14 || IS_DG2(i915)) + if (DISPLAY_VER(display) >= 14 || display->platform.dg2) /* No shared DPLLs on DG2; port PLLs are part of the PHY */ dpll_mgr = NULL; - else if (IS_ALDERLAKE_P(i915)) + else if (display->platform.alderlake_p) dpll_mgr = &adlp_pll_mgr; - else if (IS_ALDERLAKE_S(i915)) + else if (display->platform.alderlake_s) dpll_mgr = &adls_pll_mgr; - else if (IS_DG1(i915)) + else if (display->platform.dg1) dpll_mgr = &dg1_pll_mgr; - else if (IS_ROCKETLAKE(i915)) + else if (display->platform.rocketlake) dpll_mgr = &rkl_pll_mgr; - else if (DISPLAY_VER(i915) >= 12) + else if (DISPLAY_VER(display) >= 12) dpll_mgr = &tgl_pll_mgr; - else if (IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) + else if (display->platform.jasperlake || display->platform.elkhartlake) dpll_mgr = &ehl_pll_mgr; - else if (DISPLAY_VER(i915) >= 11) + else if (DISPLAY_VER(display) >= 11) dpll_mgr = &icl_pll_mgr; - else if (IS_GEMINILAKE(i915) || IS_BROXTON(i915)) + else if (display->platform.geminilake || display->platform.broxton) dpll_mgr = &bxt_pll_mgr; - else if (DISPLAY_VER(i915) == 9) + else if (DISPLAY_VER(display) == 9) dpll_mgr = &skl_pll_mgr; - else if (HAS_DDI(i915)) + else if (HAS_DDI(display)) dpll_mgr = &hsw_pll_mgr; else if (HAS_PCH_IBX(i915) || HAS_PCH_CPT(i915)) dpll_mgr = &pch_pll_mgr; @@ -4346,20 +4347,20 @@ void intel_shared_dpll_init(struct drm_i915_private *i915) dpll_info = dpll_mgr->dpll_info; for (i = 0; dpll_info[i].name; i++) { - if (drm_WARN_ON(&i915->drm, - i >= ARRAY_SIZE(i915->display.dpll.shared_dplls))) + if (drm_WARN_ON(display->drm, + i >= ARRAY_SIZE(display->dpll.shared_dplls))) break; /* must fit into unsigned long bitmask on 32bit */ - if (drm_WARN_ON(&i915->drm, dpll_info[i].id >= 32)) + if (drm_WARN_ON(display->drm, dpll_info[i].id >= 32)) break; - i915->display.dpll.shared_dplls[i].info = &dpll_info[i]; - i915->display.dpll.shared_dplls[i].index = i; + display->dpll.shared_dplls[i].info = &dpll_info[i]; + display->dpll.shared_dplls[i].index = i; } - i915->display.dpll.mgr = dpll_mgr; - i915->display.dpll.num_shared_dpll = i; + display->dpll.mgr = dpll_mgr; + display->dpll.num_shared_dpll = i; } /** diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h index 3d988f17f31d..caffb084830c 100644 --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h @@ -35,7 +35,6 @@ ((__pll) = &(__display)->dpll.shared_dplls[(__i)]) ; (__i)++) enum tc_port; -struct drm_i915_private; struct drm_printer; struct intel_atomic_state; struct intel_crtc; @@ -422,7 +421,7 @@ bool intel_dpll_get_hw_state(struct intel_display *display, void intel_enable_shared_dpll(const struct intel_crtc_state *crtc_state); void intel_disable_shared_dpll(const struct intel_crtc_state *crtc_state); void intel_shared_dpll_swap_state(struct intel_atomic_state *state); -void intel_shared_dpll_init(struct drm_i915_private *i915); +void intel_shared_dpll_init(struct intel_display *display); void intel_dpll_update_ref_clks(struct intel_display *display); void intel_dpll_readout_hw_state(struct intel_display *display); void intel_dpll_sanitize_state(struct intel_display *display);
Use intel_display as an argument for intel_shared_dpll_init() and replace drm_i915_private in function wherever possible. While at it prefer using display->platform.xx over IS_PLATFORM. Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> --- .../drm/i915/display/intel_display_driver.c | 2 +- drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 49 ++++++++++--------- drivers/gpu/drm/i915/display/intel_dpll_mgr.h | 3 +- 3 files changed, 27 insertions(+), 27 deletions(-)