| Message ID | 20240626050056.3996349-3-ankit.k.nautiyal@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add HDMI PLL Algorithm for SNPS/C10PHY | expand |
On Wed, 26 Jun 2024, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote: > Try SNPS_PHY HDMI tables computed using the algorithm, before using > consolidated tables. > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > --- > drivers/gpu/drm/i915/display/intel_snps_phy.c | 20 ++++++++----------- > 1 file changed, 8 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_snps_phy.c b/drivers/gpu/drm/i915/display/intel_snps_phy.c > index e6df1f92def5..10fe28af0d11 100644 > --- a/drivers/gpu/drm/i915/display/intel_snps_phy.c > +++ b/drivers/gpu/drm/i915/display/intel_snps_phy.c > @@ -12,6 +12,7 @@ > #include "intel_display_types.h" > #include "intel_snps_phy.h" > #include "intel_snps_phy_regs.h" > +#include "intel_pll_algorithm.h" Keep includes sorted. > > /** > * DOC: Synopsis PHY support > @@ -1787,22 +1788,14 @@ intel_mpllb_tables_get(struct intel_crtc_state *crtc_state, > int intel_mpllb_calc_state(struct intel_crtc_state *crtc_state, > struct intel_encoder *encoder) > { > - struct drm_i915_private *i915 = to_i915(encoder->base.dev); > const struct intel_mpllb_state * const *tables; > int i; > > if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) { > - if (intel_snps_phy_check_hdmi_link_rate(crtc_state->port_clock) > - != MODE_OK) { > - /* > - * FIXME: Can only support fixed HDMI frequencies > - * until we have a proper algorithm under a valid > - * license. > - */ > - drm_dbg_kms(&i915->drm, "Can't support HDMI link rate %d\n", > - crtc_state->port_clock); > - return -EINVAL; > - } > + /* try computed SNPS_PHY HDMI tables before using consolidated tables */ Computed tables vs. consolidated tables? Huh? Anyway, I think we have two choices here: - Always use computed values. - Prefer fixed tables, fall back to computed values. But we definitely should not try to compute first and fall back to fixed tables. > + if (intel_snps_phy_compute_hdmi_tmds_pll(crtc_state->port_clock, > + &crtc_state->dpll_hw_state.mpllb) == 0) > + return 0; > } > > tables = intel_mpllb_tables_get(crtc_state, encoder); > @@ -1991,6 +1984,9 @@ int intel_snps_phy_check_hdmi_link_rate(int clock) > return MODE_OK; > } > > + if (clock >= 25175 && clock <= 594000) > + return MODE_OK; > + How's this related to the patch at hand? > return MODE_CLOCK_RANGE; > }
On 6/26/2024 3:37 PM, Jani Nikula wrote: > On Wed, 26 Jun 2024, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote: >> Try SNPS_PHY HDMI tables computed using the algorithm, before using >> consolidated tables. >> >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_snps_phy.c | 20 ++++++++----------- >> 1 file changed, 8 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_snps_phy.c b/drivers/gpu/drm/i915/display/intel_snps_phy.c >> index e6df1f92def5..10fe28af0d11 100644 >> --- a/drivers/gpu/drm/i915/display/intel_snps_phy.c >> +++ b/drivers/gpu/drm/i915/display/intel_snps_phy.c >> @@ -12,6 +12,7 @@ >> #include "intel_display_types.h" >> #include "intel_snps_phy.h" >> #include "intel_snps_phy_regs.h" >> +#include "intel_pll_algorithm.h" > Keep includes sorted. Noted. Thanks for pointing this out. > >> >> /** >> * DOC: Synopsis PHY support >> @@ -1787,22 +1788,14 @@ intel_mpllb_tables_get(struct intel_crtc_state *crtc_state, >> int intel_mpllb_calc_state(struct intel_crtc_state *crtc_state, >> struct intel_encoder *encoder) >> { >> - struct drm_i915_private *i915 = to_i915(encoder->base.dev); >> const struct intel_mpllb_state * const *tables; >> int i; >> >> if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) { >> - if (intel_snps_phy_check_hdmi_link_rate(crtc_state->port_clock) >> - != MODE_OK) { >> - /* >> - * FIXME: Can only support fixed HDMI frequencies >> - * until we have a proper algorithm under a valid >> - * license. >> - */ >> - drm_dbg_kms(&i915->drm, "Can't support HDMI link rate %d\n", >> - crtc_state->port_clock); >> - return -EINVAL; >> - } >> + /* try computed SNPS_PHY HDMI tables before using consolidated tables */ > Computed tables vs. consolidated tables? Huh? > > Anyway, I think we have two choices here: > > - Always use computed values. > > - Prefer fixed tables, fall back to computed values. > > But we definitely should not try to compute first and fall back to fixed > tables. Hmm I was not sure if we need the fixed tables after this and whether we should remove them altogether. But it makes more sense to use prefer the fixed tables and fall back to computed values. I'll make the changes in the next version. > >> + if (intel_snps_phy_compute_hdmi_tmds_pll(crtc_state->port_clock, >> + &crtc_state->dpll_hw_state.mpllb) == 0) >> + return 0; >> } >> >> tables = intel_mpllb_tables_get(crtc_state, encoder); >> @@ -1991,6 +1984,9 @@ int intel_snps_phy_check_hdmi_link_rate(int clock) >> return MODE_OK; >> } >> >> + if (clock >= 25175 && clock <= 594000) >> + return MODE_OK; >> + > How's this related to the patch at hand? Currently we prune the modes if the clock does not match that given in the table. Now that we support all clocks between 25175 and 594000 we need this, but perhaps will add as a separate patch. Perhaps I can remove this function all together and put the condition in hdmi_port_clock valid, in separate patch. Regards, Ankit > >> return MODE_CLOCK_RANGE; >> }
On Thu, 27 Jun 2024, "Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com> wrote: > On 6/26/2024 3:37 PM, Jani Nikula wrote: >> On Wed, 26 Jun 2024, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote: >>> Try SNPS_PHY HDMI tables computed using the algorithm, before using >>> consolidated tables. >>> >>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >>> --- >>> drivers/gpu/drm/i915/display/intel_snps_phy.c | 20 ++++++++----------- >>> 1 file changed, 8 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/display/intel_snps_phy.c b/drivers/gpu/drm/i915/display/intel_snps_phy.c >>> index e6df1f92def5..10fe28af0d11 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_snps_phy.c >>> +++ b/drivers/gpu/drm/i915/display/intel_snps_phy.c >>> @@ -12,6 +12,7 @@ >>> #include "intel_display_types.h" >>> #include "intel_snps_phy.h" >>> #include "intel_snps_phy_regs.h" >>> +#include "intel_pll_algorithm.h" >> Keep includes sorted. > Noted. Thanks for pointing this out. >> >>> >>> /** >>> * DOC: Synopsis PHY support >>> @@ -1787,22 +1788,14 @@ intel_mpllb_tables_get(struct intel_crtc_state *crtc_state, >>> int intel_mpllb_calc_state(struct intel_crtc_state *crtc_state, >>> struct intel_encoder *encoder) >>> { >>> - struct drm_i915_private *i915 = to_i915(encoder->base.dev); >>> const struct intel_mpllb_state * const *tables; >>> int i; >>> >>> if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) { >>> - if (intel_snps_phy_check_hdmi_link_rate(crtc_state->port_clock) >>> - != MODE_OK) { >>> - /* >>> - * FIXME: Can only support fixed HDMI frequencies >>> - * until we have a proper algorithm under a valid >>> - * license. >>> - */ >>> - drm_dbg_kms(&i915->drm, "Can't support HDMI link rate %d\n", >>> - crtc_state->port_clock); >>> - return -EINVAL; >>> - } >>> + /* try computed SNPS_PHY HDMI tables before using consolidated tables */ >> Computed tables vs. consolidated tables? Huh? >> >> Anyway, I think we have two choices here: >> >> - Always use computed values. >> >> - Prefer fixed tables, fall back to computed values. >> >> But we definitely should not try to compute first and fall back to fixed >> tables. > > Hmm I was not sure if we need the fixed tables after this and whether we > should remove them altogether. > > But it makes more sense to use prefer the fixed tables and fall back to > computed values. > > I'll make the changes in the next version. > > >> >>> + if (intel_snps_phy_compute_hdmi_tmds_pll(crtc_state->port_clock, >>> + &crtc_state->dpll_hw_state.mpllb) == 0) >>> + return 0; >>> } >>> >>> tables = intel_mpllb_tables_get(crtc_state, encoder); >>> @@ -1991,6 +1984,9 @@ int intel_snps_phy_check_hdmi_link_rate(int clock) >>> return MODE_OK; >>> } >>> >>> + if (clock >= 25175 && clock <= 594000) >>> + return MODE_OK; >>> + >> How's this related to the patch at hand? > > Currently we prune the modes if the clock does not match that given in > the table. > > Now that we support all clocks between 25175 and 594000 we need this, > but perhaps will add as a separate patch. > > Perhaps I can remove this function all together and put the condition in > hdmi_port_clock valid, in separate patch. But we already have intel_hdmi_source_max_tmds_clock(), which also takes into account platform specifics. For example the fact that 594000 is not the max on all platforms. BR, Jani. > > Regards, > > Ankit > > >> >>> return MODE_CLOCK_RANGE; >>> }
On 6/28/2024 12:00 AM, Jani Nikula wrote: > On Thu, 27 Jun 2024, "Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com> wrote: >> On 6/26/2024 3:37 PM, Jani Nikula wrote: >>> On Wed, 26 Jun 2024, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote: >>>> Try SNPS_PHY HDMI tables computed using the algorithm, before using >>>> consolidated tables. >>>> >>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/display/intel_snps_phy.c | 20 ++++++++----------- >>>> 1 file changed, 8 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_snps_phy.c b/drivers/gpu/drm/i915/display/intel_snps_phy.c >>>> index e6df1f92def5..10fe28af0d11 100644 >>>> --- a/drivers/gpu/drm/i915/display/intel_snps_phy.c >>>> +++ b/drivers/gpu/drm/i915/display/intel_snps_phy.c >>>> @@ -12,6 +12,7 @@ >>>> #include "intel_display_types.h" >>>> #include "intel_snps_phy.h" >>>> #include "intel_snps_phy_regs.h" >>>> +#include "intel_pll_algorithm.h" >>> Keep includes sorted. >> Noted. Thanks for pointing this out. >>>> >>>> /** >>>> * DOC: Synopsis PHY support >>>> @@ -1787,22 +1788,14 @@ intel_mpllb_tables_get(struct intel_crtc_state *crtc_state, >>>> int intel_mpllb_calc_state(struct intel_crtc_state *crtc_state, >>>> struct intel_encoder *encoder) >>>> { >>>> - struct drm_i915_private *i915 = to_i915(encoder->base.dev); >>>> const struct intel_mpllb_state * const *tables; >>>> int i; >>>> >>>> if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) { >>>> - if (intel_snps_phy_check_hdmi_link_rate(crtc_state->port_clock) >>>> - != MODE_OK) { >>>> - /* >>>> - * FIXME: Can only support fixed HDMI frequencies >>>> - * until we have a proper algorithm under a valid >>>> - * license. >>>> - */ >>>> - drm_dbg_kms(&i915->drm, "Can't support HDMI link rate %d\n", >>>> - crtc_state->port_clock); >>>> - return -EINVAL; >>>> - } >>>> + /* try computed SNPS_PHY HDMI tables before using consolidated tables */ >>> Computed tables vs. consolidated tables? Huh? >>> >>> Anyway, I think we have two choices here: >>> >>> - Always use computed values. >>> >>> - Prefer fixed tables, fall back to computed values. >>> >>> But we definitely should not try to compute first and fall back to fixed >>> tables. >> Hmm I was not sure if we need the fixed tables after this and whether we >> should remove them altogether. >> >> But it makes more sense to use prefer the fixed tables and fall back to >> computed values. >> >> I'll make the changes in the next version. >> >> >>>> + if (intel_snps_phy_compute_hdmi_tmds_pll(crtc_state->port_clock, >>>> + &crtc_state->dpll_hw_state.mpllb) == 0) >>>> + return 0; >>>> } >>>> >>>> tables = intel_mpllb_tables_get(crtc_state, encoder); >>>> @@ -1991,6 +1984,9 @@ int intel_snps_phy_check_hdmi_link_rate(int clock) >>>> return MODE_OK; >>>> } >>>> >>>> + if (clock >= 25175 && clock <= 594000) >>>> + return MODE_OK; >>>> + >>> How's this related to the patch at hand? >> Currently we prune the modes if the clock does not match that given in >> the table. >> >> Now that we support all clocks between 25175 and 594000 we need this, >> but perhaps will add as a separate patch. >> >> Perhaps I can remove this function all together and put the condition in >> hdmi_port_clock valid, in separate patch. > But we already have intel_hdmi_source_max_tmds_clock(), which also takes > into account platform specifics. For example the fact that 594000 is not > the max on all platforms. You are right,as per Bspec platform overview and port clock programming pages maximum is indeed 600Mhz. I am wondering, we need to fix existing intel_c20_phy_check_hdmi_link_rate too, as per Bspec:74165. I think we can remove the intel_cx0_phy_check_hdmi_link_rate and intel_snps_phy_check_hdmi_link_rate checks from hdmi_port_clock_valid. Thanks & Regards, Ankit > > BR, > Jani. > > > > > >> Regards, >> >> Ankit >> >> >>>> return MODE_CLOCK_RANGE; >>>> }
diff --git a/drivers/gpu/drm/i915/display/intel_snps_phy.c b/drivers/gpu/drm/i915/display/intel_snps_phy.c index e6df1f92def5..10fe28af0d11 100644 --- a/drivers/gpu/drm/i915/display/intel_snps_phy.c +++ b/drivers/gpu/drm/i915/display/intel_snps_phy.c @@ -12,6 +12,7 @@ #include "intel_display_types.h" #include "intel_snps_phy.h" #include "intel_snps_phy_regs.h" +#include "intel_pll_algorithm.h" /** * DOC: Synopsis PHY support @@ -1787,22 +1788,14 @@ intel_mpllb_tables_get(struct intel_crtc_state *crtc_state, int intel_mpllb_calc_state(struct intel_crtc_state *crtc_state, struct intel_encoder *encoder) { - struct drm_i915_private *i915 = to_i915(encoder->base.dev); const struct intel_mpllb_state * const *tables; int i; if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) { - if (intel_snps_phy_check_hdmi_link_rate(crtc_state->port_clock) - != MODE_OK) { - /* - * FIXME: Can only support fixed HDMI frequencies - * until we have a proper algorithm under a valid - * license. - */ - drm_dbg_kms(&i915->drm, "Can't support HDMI link rate %d\n", - crtc_state->port_clock); - return -EINVAL; - } + /* try computed SNPS_PHY HDMI tables before using consolidated tables */ + if (intel_snps_phy_compute_hdmi_tmds_pll(crtc_state->port_clock, + &crtc_state->dpll_hw_state.mpllb) == 0) + return 0; } tables = intel_mpllb_tables_get(crtc_state, encoder); @@ -1991,6 +1984,9 @@ int intel_snps_phy_check_hdmi_link_rate(int clock) return MODE_OK; } + if (clock >= 25175 && clock <= 594000) + return MODE_OK; + return MODE_CLOCK_RANGE; }
Try SNPS_PHY HDMI tables computed using the algorithm, before using consolidated tables. Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> --- drivers/gpu/drm/i915/display/intel_snps_phy.c | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-)