Re: [PATCH 2/5] drm/i915/snps_phy: Use HDMI PLL algorithm for DG2

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

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;
>>>   }

-- 
Jani Nikula, Intel