> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Sent: Friday, 7 March 2025 15.07
> To: Kahola, Mika <mika.kahola@xxxxxxxxx>
> Cc: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>; Jani Nikula
> <jani.nikula@xxxxxxxxxxxxxxx>; intel-xe@xxxxxxxxxxxxxxxxxxxxx; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx; Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Nautiyal, Ankit
> K <ankit.k.nautiyal@xxxxxxxxx>; Shankar, Uma <uma.shankar@xxxxxxxxx>
> Subject: Re: [PATCH 05/11] drm/i915/dpll: Move away from using shared dpll
>
> On Fri, Mar 07, 2025 at 12:02:09PM +0000, Kahola, Mika wrote:
> > > -----Original Message-----
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Sent: Monday, 3 March 2025 14.52
> > > To: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>
> > > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>;
> > > intel-xe@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx;
> > > Syrjala, Ville <ville.syrjala@xxxxxxxxx>; 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 Fri, Feb 28, 2025 at 03:31:39PM +0000, Kandpal, Suraj wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > > Sent: Friday, February 28, 2025 7:57 PM
> > > > > To: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>
> > > > > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>;
> > > > > intel-xe@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx;
> > > > > Syrjala, Ville <ville.syrjala@xxxxxxxxx>; 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 Thu, Feb 27, 2025 at 10:18:31AM +0000, Kandpal, Suraj wrote:
> > > > > >
> > > > > >
> > > > > > > -----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 ?
> > > > >
> > > > > IMO it should just be intel_dpll_*. We want all PLLs to provide
> > > > > the same uniform interface for enable/disble/readout/state_dump/etc.
> > > > > Whether the PLL is shared/global or not isn't interesting
> > > > > outside the actual modeset sequence and PLL selection logic.
> > > >
> > > > But that still leaves us with the question what would be the most
> > > > appropriate way to do away with the
> > > > Intel_shared_dpll_* naming what does it become if not
> > > > intel_global_dpll_* (since intel_dpll wouldn't be a
> > > > Straightforward answer to
> > > this) intel_dpll_global ?
> > >
> > > What do you mean intel_dpll_* isn't a straightforward answer?
> > > It is the right answer.
> >
> > About this naming convention, we have defined (intel_display_core.h) structure
> for intel_dpll which contains structures for intel_shared_dpll and intel_dpll_mgr.
> Wouldn't renaming intel_shared_dpll to intel_dpll cause a conflict with already
> existing intel_dpll structure? Or should we keep the intel_shared_dpll structure
> intact and rename intel_shared_dpll_* simply intel_dpll_*?
>
> I think just rename the current intel_dpll to something more appropriate.
>
> Or just forget about naming for now and move ahead with the actual work. We've
> been doing fine with the _shared_dpll name up to now, so clearly it's not *that*
> important what it's called.
Ok. Let's put this naming stuff on hold for a while. I will move on to cx0 pll's as part of pll framework.
Thanks!
Mika
>
> --
> Ville Syrjälä
> Intel
