On Thu, 27 Jun 2024, "Nautiyal, Ankit K" <ankit.k.nautiyal@xxxxxxxxx> wrote:
> On 6/26/2024 3:37 PM, Jani Nikula wrote:
>> On Wed, 26 Jun 2024, Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx> wrote:
>>> Try SNPS_PHY HDMI tables computed using the algorithm, before using
>>> consolidated tables.
>>>
>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
>>> ---
>>> 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;
>>> }
--
Jani Nikula, Intel
