> > Subject: RE: [PATCH v2 30/32] drm/i915/cx0: Get encoder configuration > > for C10 and C20 PHY PLLs > > > > > Subject: [PATCH v2 30/32] drm/i915/cx0: Get encoder configuration > > > for > > > C10 and C20 PHY PLLs > > > > > > For DDI initialization get configuration for C10 and C20 chips. > > > > > > v2: Getting configuration either for a C10 or on the PTL port B > > > eDP on TypeC PHY case for a C20 PHY PLL. Hence refer to this > > > case as "non_tc_phy" instead of "c10phy". > > > > > > Signed-off-by: Imre Deak <[email protected]> > > > Signed-off-by: Mika Kahola <[email protected]> > > > --- > > > drivers/gpu/drm/i915/display/intel_ddi.c | 81 > > > ++++++++++++++++++++++-- > > > 1 file changed, 75 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > > index be25a6fdd491..689bd3224919 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > > @@ -4273,6 +4273,77 @@ static void mtl_ddi_get_config(struct > > > intel_encoder *encoder, > > > intel_ddi_get_config(encoder, crtc_state); } > > > > > > +static bool icl_ddi_tc_pll_is_tbt(const struct intel_dpll *pll) { > > > + return pll->info->id == DPLL_ID_ICL_TBTPLL; } > > > + > > > +static void mtl_ddi_cx0_get_config(struct intel_encoder *encoder, > > > + struct intel_crtc_state *crtc_state, > > > + enum icl_port_dpll_id port_dpll_id, > > > + enum intel_dpll_id pll_id) > > > +{ > > > + struct intel_display *display = to_intel_display(encoder); > > > + struct icl_port_dpll *port_dpll; > > > + struct intel_dpll *pll; > > > + bool pll_active; > > > + > > > + port_dpll = &crtc_state->icl_port_dplls[port_dpll_id]; > > > + pll = intel_get_dpll_by_id(display, pll_id); > > > + > > > + if (drm_WARN_ON(display->drm, !pll)) > > > + return; > > > + > > > + port_dpll->pll = pll; > > > + pll_active = intel_dpll_get_hw_state(display, pll, > > > &port_dpll->hw_state); > > > + drm_WARN_ON(display->drm, !pll_active); > > > + > > > + icl_set_active_port_dpll(crtc_state, port_dpll_id); > > > + > > > + if (icl_ddi_tc_pll_is_tbt(crtc_state->intel_dpll)) > > > + crtc_state->port_clock = > intel_mtl_tbt_calc_port_clock(encoder); > > > + else > > > + crtc_state->port_clock = intel_dpll_get_freq(display, > > > crtc_state- > > > >intel_dpll, > > > + &crtc_state- > > > >dpll_hw_state); > > > + > > > + intel_ddi_get_config(encoder, crtc_state); } > > > + > > > +/* > > > + * Get the configuration for either a port using a C10 PHY PLL, or > > > +in the case of > > > + * the PTL port B eDP on TypeC PHY case the configuration of a port > > > +using a C20 > > > + * PHY PLL. > > > + */ > > > +static void mtl_ddi_non_tc_phy_get_config(struct intel_encoder *encoder, > > > + struct intel_crtc_state > > > *crtc_state) { > > > + struct intel_display *display = to_intel_display(encoder); > > > + > > > + /* TODO: Remove when the PLL manager is in place. */ > > > > Is the comment needed anymore > > At this point of patch series, we don't have pll manager yet in place so we > can > keep this comment for a while. The last patch that enables pll manager and > framework will remove this comment. > > > > > > + mtl_ddi_get_config(encoder, crtc_state); > > > + return; > > > > Why the early return code after this point then serves no purpose. > > It serves a purpose that in this way the patch series is bisectable if we > need to > do that one day. This will be removed by that last patch of the series. > > > > > > + > > > + mtl_ddi_cx0_get_config(encoder, crtc_state, ICL_PORT_DPLL_DEFAULT, > > > + mtl_port_to_pll_id(display, encoder->port)); } > > > > Have the pll id in its own variable. > > I think this change would come down to code readability. In my taste the > function call to mtl_port_pll_id() is not too confusing and hence would be ok > to > use as is. > > > > > > + > > > +static void mtl_ddi_tc_phy_get_config(struct intel_encoder *encoder, > > > + struct intel_crtc_state *crtc_state) { > > > + struct intel_display *display = to_intel_display(encoder); > > > + > > > + /* TODO: Remove when the PLL manager is in place. */ > > > > No need for this comment > > This is removed by the last patch. > > > > > > + mtl_ddi_get_config(encoder, crtc_state); > > > + return; > > > > Same question why the early return ? > > This is again for bisectablity of the patch series. > > > > > > + > > > + if (intel_tc_port_in_tbt_alt_mode(enc_to_dig_port(encoder))) > > > + mtl_ddi_cx0_get_config(encoder, crtc_state, > > > ICL_PORT_DPLL_DEFAULT, > > > + DPLL_ID_ICL_TBTPLL); > > > + else > > > + mtl_ddi_cx0_get_config(encoder, crtc_state, > > > ICL_PORT_DPLL_MG_PHY, > > > + mtl_port_to_pll_id(display, encoder- > > > >port)); } > > > > You can have the pll id as its one variable In fact you can call > > mtl_ddi_cx0_get_config just once if you have both port and pll id > > variables assigned After checking if intel_tc_port_in_tbt_alt_mode > > This could be written that way we set these pll and port id's as variables > after > checking if intel_tc_port_in_tbt_alt_mode(). However, to me this change > wouldn't improve code readability but simply would be written differently. > > Thanks, > Mika >
Fair enough Reviewed-by: Suraj Kandpal <[email protected]> > > > > Regards, > > Suraj Kandpal > > > > > + > > > static void dg2_ddi_get_config(struct intel_encoder *encoder, > > > struct intel_crtc_state *crtc_state) { @@ - > > > 4310,11 +4381,6 @@ static void icl_ddi_combo_get_config(struct > > > intel_encoder *encoder, > > > intel_ddi_get_config(encoder, crtc_state); } > > > > > > -static bool icl_ddi_tc_pll_is_tbt(const struct intel_dpll *pll) -{ > > > - return pll->info->id == DPLL_ID_ICL_TBTPLL; > > > -} > > > - > > > static enum icl_port_dpll_id > > > icl_ddi_tc_port_pll_type(struct intel_encoder *encoder, > > > const struct intel_crtc_state *crtc_state) @@ -5260,7 > > > +5326,10 @@ void intel_ddi_init(struct intel_display *display, > > > encoder->enable_clock = intel_mtl_pll_enable_clock; > > > encoder->disable_clock = intel_mtl_pll_disable_clock; > > > encoder->port_pll_type = intel_mtl_port_pll_type; > > > - encoder->get_config = mtl_ddi_get_config; > > > + if (intel_encoder_is_tc(encoder)) > > > + encoder->get_config = mtl_ddi_tc_phy_get_config; > > > + else > > > + encoder->get_config = > mtl_ddi_non_tc_phy_get_config; > > > } else if (display->platform.dg2) { > > > encoder->enable_clock = intel_mpllb_enable; > > > encoder->disable_clock = intel_mpllb_disable; > > > -- > > > 2.34.1
