> -----Original Message-----
> From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Sent: Tuesday, February 25, 2025 9:00 PM
> To: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>; intel-xe@xxxxxxxxxxxxxxxxxxxxx;
> intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Syrjala, Ville <ville.syrjala@xxxxxxxxx>
> Cc: Nautiyal, Ankit K <ankit.k.nautiyal@xxxxxxxxx>; Shankar, Uma
> <uma.shankar@xxxxxxxxx>; Kahola, Mika <mika.kahola@xxxxxxxxx>
> Subject: RE: [PATCH 05/11] drm/i915/dpll: Move away from using shared dpll
>
> On Tue, 25 Feb 2025, "Kandpal, Suraj" <suraj.kandpal@xxxxxxxxx> wrote:
> >> -----Original Message-----
> >> From: Kandpal, Suraj
> >> Sent: Tuesday, February 25, 2025 2:25 PM
> >> To: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>;
> >> intel-xe@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> >> Cc: Nautiyal, Ankit K <ankit.k.nautiyal@xxxxxxxxx>; Shankar, Uma
> >> <uma.shankar@xxxxxxxxx>; Kahola, Mika <mika.kahola@xxxxxxxxx>
> >> Subject: RE: [PATCH 05/11] drm/i915/dpll: Move away from using shared
> >> dpll
> >>
> >>
> >>
> >> > -----Original Message-----
> >> > From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> >> > Sent: Tuesday, February 25, 2025 2:17 PM
> >> > To: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>;
> >> > intel-xe@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> >> > Cc: Nautiyal, Ankit K <ankit.k.nautiyal@xxxxxxxxx>; Shankar, Uma
> >> > <uma.shankar@xxxxxxxxx>; Kahola, Mika <mika.kahola@xxxxxxxxx>;
> >> > Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>
> >> > Subject: Re: [PATCH 05/11] drm/i915/dpll: Move away from using
> >> > shared dpll
> >> >
> >> > On Tue, 25 Feb 2025, Suraj Kandpal <suraj.kandpal@xxxxxxxxx> wrote:
> >> > > Rename functions to move away from using shared dpll in the dpll
> >> > > framework as much as possible since dpll may not always be shared.
> >> > >
> >> > > Signed-off-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
> >> >
> >> > ...
> >> >
> >> > > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> >> > > b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> >> > > index 6edd103eda55..ef66aca5da1d 100644
> >> > > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> >> > > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> >> > > @@ -387,24 +387,24 @@ struct intel_global_dpll { #define
> >> > > SKL_DPLL2
> >> > > 2 #define SKL_DPLL3 3
> >> > >
> >> > > -/* shared dpll functions */
> >> > > +/* global dpll functions */
> >> > > struct intel_global_dpll *
> >> > > -intel_get_shared_dpll_by_id(struct intel_display *display,
> >> > > +intel_get_global_dpll_by_id(struct intel_display *display,
> >> > > enum intel_dpll_id id);
> >> > > -void assert_shared_dpll(struct intel_display *display,
> >> > > +void assert_global_dpll(struct intel_display *display,
> >> > > struct intel_global_dpll *pll,
> >> > > bool state);
> >> > > -#define assert_shared_dpll_enabled(d, p) assert_shared_dpll(d,
> >> > > p,
> >> > > true) -#define assert_shared_dpll_disabled(d, p)
> >> > > assert_shared_dpll(d, p, false) -int
> >> > > intel_compute_shared_dplls(struct intel_atomic_state *state,
> >> > > +#define assert_global_dpll_enabled(d, p) assert_global_dpll(d,
> >> > > +p,
> >> > > +true) #define assert_global_dpll_disabled(d, p)
> >> > > +assert_global_dpll(d, p, false) int
> >> > > +intel_compute_global_dplls(struct intel_atomic_state *state,
> >> > > struct intel_crtc *crtc,
> >> > > struct intel_encoder *encoder); -int
> >> > > intel_reserve_shared_dplls(struct intel_atomic_state *state,
> >> > > +int intel_reserve_global_dplls(struct intel_atomic_state *state,
> >> > > struct intel_crtc *crtc,
> >> > > struct intel_encoder *encoder); -void
> >> > > intel_release_shared_dplls(struct intel_atomic_state *state,
> >> > > +void intel_release_global_dplls(struct intel_atomic_state
> >> > > +*state,
> >> > > struct intel_crtc *crtc);
> >> > > -void intel_unreference_shared_dpll_crtc(const struct intel_crtc
> >> > > *crtc,
> >> > > +void intel_unreference_global_dpll_crtc(const struct intel_crtc
> >> > > +*crtc,
> >> > > const struct intel_global_dpll *pll,
> >> > > struct intel_dpll_state
> >> > *shared_dpll_state); void
> >> > > icl_set_active_port_dpll(struct intel_crtc_state *crtc_state, @@
> >> > > -418,10 +418,10 @@ int intel_dpll_get_freq(struct intel_display
> >> > > *display, bool intel_dpll_get_hw_state(struct intel_display *display,
> >> > > struct intel_global_dpll *pll,
> >> > > struct intel_dpll_hw_state *dpll_hw_state); -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
> >> > > intel_display *display);
> >> > > +void intel_enable_global_dpll(const struct intel_crtc_state
> >> > > +*crtc_state); void intel_disable_global_dpll(const struct
> >> > > +intel_crtc_state *crtc_state); void intel_dpll_swap_state(struct
> >> > > +intel_atomic_state *state); void intel_global_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); @@
> >> > > -437,6
> >> > > +437,6 @@ bool intel_dpll_is_combophy(enum intel_dpll_id id);
> >> > >
> >> > > void intel_dpll_state_verify(struct intel_atomic_state *state,
> >> > > struct intel_crtc *crtc); -void
> >> > > intel_shared_dpll_verify_disabled(struct intel_atomic_state
> >> > > *state);
> >> > > +void intel_global_dpll_verify_disabled(struct intel_atomic_state
> >> > > +*state);
> >> > >
> >> > > #endif /* _INTEL_DPLL_MGR_H_ */
> >> >
> >> > If you're renaming almost everything anyway, I'd appreciate moving
> >> > towards naming functions according to the file name, i.e. functions
> >> > in intel_foo.[ch] would be named intel_foo_*().
> >> >
> >> > The dpll mgr is notoriously bad in this regard. I'm also open to
> >> > renaming the entire file, intel_dpll_mgr.[ch] isn't all that great.
> >> >
> >> > I'm not sure if the term "global" (instead of "shared") was very
> >> > well justified in patch 3. Maybe all of these should be thought out
> >> > together for the
> >> naming.
> >> >
> >>
> >> I agree with the renaming I would have very much have to keep the
> >> naming simple something like Intel_dpll_func but that exits !
> >> intel_dpll_mgr_funcs but intel_dpll_mgr already has some hooks defined
> inside It.
> >> I chose global since that way we will be able to represent both PLL
> >> using shared PHY and PLL with individual PHY.
> >> Also renaming intel_dpll_mgr.[ch] we have a intel_dpll.[ch] making it
> >> a problem What if we renamed the file to intel_global_dpll.[ch]
> >
> > Jani what do you think of this ?
>
> I think Ville probably has opinions on this. Cc'd.
Hi Ville,
Any thoughts ?
Regards,
Suraj Kandpal
>
> BR,
> Jani.
>
> >
> >>
> >> Regards,
> >> Suraj Kandpal
> >>
> >> > BR,
> >> > Jani.
> >> >
> >> >
> >> > --
> >> > Jani Nikula, Intel
>
> --
> Jani Nikula, Intel
