RE: [PATCH 05/11] drm/i915/dpll: Move away from using shared dpll

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 25 Feb 2025, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote:
>> -----Original Message-----
>> From: Kandpal, Suraj
>> Sent: Tuesday, February 25, 2025 2:25 PM
>> To: Jani Nikula <jani.nikula@linux.intel.com>; intel-xe@lists.freedesktop.org;
>> intel-gfx@lists.freedesktop.org
>> Cc: Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; Shankar, Uma
>> <uma.shankar@intel.com>; Kahola, Mika <mika.kahola@intel.com>
>> Subject: RE: [PATCH 05/11] drm/i915/dpll: Move away from using shared dpll
>> 
>> 
>> 
>> > -----Original Message-----
>> > From: Jani Nikula <jani.nikula@linux.intel.com>
>> > Sent: Tuesday, February 25, 2025 2:17 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>; Shankar, Uma
>> > <uma.shankar@intel.com>; Kahola, Mika <mika.kahola@intel.com>;
>> > Kandpal, Suraj <suraj.kandpal@intel.com>
>> > Subject: Re: [PATCH 05/11] drm/i915/dpll: Move away from using shared
>> > dpll
>> >
>> > On Tue, 25 Feb 2025, Suraj Kandpal <suraj.kandpal@intel.com> 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@intel.com>
>> >
>> > ...
>> >
>> > > 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.

BR,
Jani.

>
>> 
>> Regards,
>> Suraj Kandpal
>> 
>> > BR,
>> > Jani.
>> >
>> >
>> > --
>> > Jani Nikula, Intel

-- 
Jani Nikula, Intel