> -----Original Message-----
> From: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>
> Sent: Tuesday, 11 March 2025 10.16
> To: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: intel-xe@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Nautiyal, Ankit
> K <ankit.k.nautiyal@xxxxxxxxx>; Shankar, Uma <uma.shankar@xxxxxxxxx>;
> Kahola, Mika <mika.kahola@xxxxxxxxx>
> Subject: RE: [PATCH 07/11] drm/i915/dpll: Change argument for enable hook in
> intel_global_dpll_funcs
>
>
>
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Sent: Friday, March 7, 2025 7:36 PM
> > To: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>
> > Cc: intel-xe@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx;
> > Nautiyal, Ankit K <ankit.k.nautiyal@xxxxxxxxx>; Shankar, Uma
> > <uma.shankar@xxxxxxxxx>; Kahola, Mika <mika.kahola@xxxxxxxxx>
> > Subject: Re: [PATCH 07/11] drm/i915/dpll: Change argument for enable
> > hook in intel_global_dpll_funcs
> >
> > On Tue, Feb 25, 2025 at 01:39:23PM +0530, Suraj Kandpal wrote:
> > > Change the arguments for enable hook in intel_global_dpll_funcs to
> > > only accept crtc_state. This is because we really don't need those
> > > extra arguments everything can be derived from crtc_state and we
> > > need intel_encoder for PLL enablement when DISPLAY_VER() >= 14.
> > >
> > > Signed-off-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 100
> > > ++++++++++--------
> > > 1 file changed, 54 insertions(+), 46 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > > b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > > index bd623fdddfdc..c39f7d73a89f 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > > @@ -66,9 +66,8 @@ struct intel_global_dpll_funcs {
> > > * Hook for enabling the pll, called from intel_enable_global_dpll() if
> > > * the pll is not already enabled.
> > > */
> > > - void (*enable)(struct intel_display *display,
> > > - struct intel_global_dpll *pll,
> > > - const struct intel_dpll_hw_state *dpll_hw_state);
> > > + void (*enable)(const struct intel_crtc_state *state,
> >
> > This feels like the wrong direction. We want the plls to be independent of crtcs.
> > The crtc_state should only be necessary to compute a plls's state.
>
> The problem is how much cx0 really depends on the parameters present in
> crtc_state which are not a part of pll_state and wouldn't make sense to track
> them in pll state since they are not just pll specific.
> Mika What do you think ? I feel this will be needed if you need to plug in the
> newer individual PHY PLL hooks In the current dpll framework.
It seems that we use a lot intel_crtc_has_type() function for checking different types of connectors. We could pass the required parameters separately and extract those parameters from crtc_state. In case of crtc_state changes or is updated we should remember to update these parameters as well. In that sense it feels a bit cumbersome solution. I do agree that as such a large structure intel_crtc_state is not the ideal solution either. I wonder what could be the middleground solution?
>
> Regards,
> Suraj Kandpal
>
> >
> > --
> > Ville Syrjälä
> > Intel
