Re: [PATCH 1/4] Add bits for link_n_exended for DISPLAY >= 14

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 3/22/2025 12:01 AM, Ville Syrjälä wrote:
> On Fri, Mar 21, 2025 at 04:56:47PM +0530, Ankit Nautiyal wrote:
>> LINK_N register has bits 31:24 for extended link N value used for
>> HDMI2.1 and for an alternate mode of operation of DP TG DDA
>> (Bspec:50488).
>>
>> Add support for these extra bits.
>>
>> For displays with version 14 or higher, the `PIPE_LINK_N1_EXTENDED_MASK`
>> (bits 31:24) is used to handle the extended link N bits.
>> For older platforms, the `DATA_LINK_M_N_MASK` (bits 23:0) is used to
>> handle the standard link N bits. This distinction ensures clarity and
>> maintains the semantics for platforms that support the extended bits.
>> In subsequent changes the logic is updated to conditionally apply the
>> extended link N bits.
>>
>> v2: Drop extra link_n_ext member. (Jani)
>> v3: Avoid link_n_ext in set_m_n helper. (Jani)
>> v4: Rebase, and update commit message.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_display.c | 18 ++++++++++++++++--
>>   drivers/gpu/drm/i915/i915_reg.h              |  2 ++
>>   2 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index 3afb85fe8536..8fb0df388571 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -2583,14 +2583,22 @@ void intel_set_m_n(struct intel_display *display,
>>   		   i915_reg_t data_m_reg, i915_reg_t data_n_reg,
>>   		   i915_reg_t link_m_reg, i915_reg_t link_n_reg)
>>   {
>> +	u32 link_n = m_n->link_n;
>> +
>>   	intel_de_write(display, data_m_reg, TU_SIZE(m_n->tu) | m_n->data_m);
>>   	intel_de_write(display, data_n_reg, m_n->data_n);
>>   	intel_de_write(display, link_m_reg, m_n->link_m);
>> +
>> +	if (DISPLAY_VER(display) >= 14)
>> +		link_n &= ~PIPE_LINK_N1_EXTENDED_MASK;
>> +	else
>> +		link_n &= DATA_LINK_M_N_MASK;
> There should never be anything in those bits or we've screwed up
> somewhere.
>
> The actual w/a for the M/N > 10.0 looks like a pile of annoying
> hacks in the hardware. I wonder if we could just live without it
> to avoid complicating the code?

Hi Ville,

Apologies for replying late on this.

Yes for general case we should not need it, but for very high refresh modes.

For BMG since we can support higher resolution with ultrajoiner, this WA 
seems to be required for such cases to support till M/N ratio 15.

I am yet to come across such HW though.

>
> As for the 10.0 limit, I suspect there's nothing platform specific
> about it and it's always been there.

Yes that's right, but for newer platforms this seems to be getting changed.


> It's just not possible to hit
> it with 8b/10b encoding. The correct place to handle this would seem
> to be the link rate calculation, ie. just bump up the link rate until
> the limit is no longer an issue (or error out if we can't increase the
> link rate sufficiently).

Alright, will try this out.

Do you see a need to check the M/N ratio in modevalid to prune the modes 
for which, even with highest rate, the ratio is more than the limit?

Currently I have not added this change.


Regards,

Ankit

>
>> +
>>   	/*
>>   	 * On BDW+ writing LINK_N arms the double buffered update
>>   	 * of all the M/N registers, so it must be written last.
>>   	 */
>> -	intel_de_write(display, link_n_reg, m_n->link_n);
>> +	intel_de_write(display, link_n_reg, link_n);
>>   }
>>   
>>   bool intel_cpu_transcoder_has_m2_n2(struct intel_display *display,
>> @@ -3279,7 +3287,13 @@ void intel_get_m_n(struct intel_display *display,
>>   		   i915_reg_t link_m_reg, i915_reg_t link_n_reg)
>>   {
>>   	m_n->link_m = intel_de_read(display, link_m_reg) & DATA_LINK_M_N_MASK;
>> -	m_n->link_n = intel_de_read(display, link_n_reg) & DATA_LINK_M_N_MASK;
>> +	m_n->link_n = intel_de_read(display, link_n_reg);
>> +
>> +	if (DISPLAY_VER(display) >= 14)
>> +		m_n->link_n &= ~PIPE_LINK_N1_EXTENDED_MASK;
>> +	else
>> +		m_n->link_n &= DATA_LINK_M_N_MASK;
>> +
>>   	m_n->data_m = intel_de_read(display, data_m_reg) & DATA_LINK_M_N_MASK;
>>   	m_n->data_n = intel_de_read(display, data_n_reg) & DATA_LINK_M_N_MASK;
>>   	m_n->tu = REG_FIELD_GET(TU_SIZE_MASK, intel_de_read(display, data_m_reg)) + 1;
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index c5064eebe063..a2054aced4f8 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -1869,6 +1869,8 @@
>>   
>>   #define _PIPEA_LINK_N1		0x60044
>>   #define _PIPEB_LINK_N1		0x61044
>> +#define  PIPE_LINK_N1_EXTENDED_MASK	REG_GENMASK(31, 24)
>> +#define  PIPE_LINK_N1_EXTENDED(val)	REG_FIELD_PREP(PIPE_LINK_N1_EXTENDED_MASK, (val))
>>   #define PIPE_LINK_N1(dev_priv, tran) _MMIO_TRANS2(dev_priv, tran, _PIPEA_LINK_N1)
>>   
>>   #define _PIPEA_LINK_M2		0x60048
>> -- 
>> 2.45.2