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

[Nautiyal, Ankit K]

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