Re: [PATCH 2/5] drm/i915/dp: Add fields to store CMN_SDP_TL register state in crtc_state

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

On 3/6/2026 5:25 PM, Ville Syrjälä wrote:
> On Thu, Mar 05, 2026 at 09:31:15AM +0530, Ankit Nautiyal wrote:
>> Xe3p_lpd introduces new register bits to program a common SDP
>> Transmission Line, which the hardware uses to position various
>> SDPs. It also adds a separate control register to stagger the different
>> SDPs (VSC EXT, PPS, GMP).
>>
>> Add fields in struct intel_crtc_state to store the state of these new
>> registers. Add register readback and pipe config comparison for the new
>> fields.
>>
>> Also add a display version check (HAS_CMN_SDP_TL) to gate access to the
>> new registers.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_ddi.c      |  1 +
>>   drivers/gpu/drm/i915/display/intel_display.c  |  6 ++++++
>>   .../drm/i915/display/intel_display_device.h   |  1 +
>>   .../drm/i915/display/intel_display_types.h    |  9 ++++++++
>>   drivers/gpu/drm/i915/display/intel_dp.c       | 21 +++++++++++++++++++
>>   drivers/gpu/drm/i915/display/intel_dp.h       |  2 ++
>>   6 files changed, 40 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
>> index 94ae583e907f..bdbd89600bee 100644
>> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
>> @@ -4217,6 +4217,7 @@ static void intel_ddi_get_config(struct intel_encoder *encoder,
>>   	intel_read_dp_sdp(encoder, pipe_config, HDMI_PACKET_TYPE_GAMUT_METADATA);
>>   	intel_read_dp_sdp(encoder, pipe_config, DP_SDP_VSC);
>>   	intel_read_dp_sdp(encoder, pipe_config, DP_SDP_ADAPTIVE_SYNC);
>> +	intel_dp_cmn_sdp_transmission_line_get_config(pipe_config);
>>   
>>   	intel_audio_codec_get_config(encoder, pipe_config);
>>   }
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index 27354585ba92..76eea9d23766 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -5461,6 +5461,12 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>>   	}
>>   	PIPE_CONF_CHECK_DP_VSC_SDP(vsc);
>>   
>> +	PIPE_CONF_CHECK_BOOL(cmn_sdp_tl.enable);
>> +	PIPE_CONF_CHECK_I(cmn_sdp_tl.transmission_line);
>> +	PIPE_CONF_CHECK_I(cmn_sdp_tl.vsc_ext_stagger);
>> +	PIPE_CONF_CHECK_I(cmn_sdp_tl.pps_stagger);
>> +	PIPE_CONF_CHECK_I(cmn_sdp_tl.gmp_stagger);
> This will make things fail every time until you add the actual
> code to program these. Ie. you are intentionally introducing
> broken bisection steps here.

Hmm I was wondering that since at this point in the commit we have not 
written these fields, the readout will also be 0 and will always match.

However I now realize that GOP might have programmed these and perhaps 
we will immediately start seeing mismatch.

In general when we add a new thing, that we want to track in crtc state, 
what should be the best ordering of the patches, does the below makes sense?

  - Introduce the new registers

  - Add the new members to crtc_state. In the same patch add functions 
to read from HW and fill the new member, and also add function to write 
into the HW from the crtc_state. (Since at this point of time, nobody is 
setting them, so there is effectively no writing yet).

  - Write functions that configure the new member of crtc_state in the 
(should be part of atomic check phase). Also add PIPE CONF check in same 
patch.


>
>> +
>>   	PIPE_CONF_CHECK_X(sync_mode_slaves_mask);
>>   	PIPE_CONF_CHECK_I(master_transcoder);
>>   	PIPE_CONF_CHECK_X(joiner_pipes);
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
>> index e84c190dcc4f..43e259761048 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
>> @@ -154,6 +154,7 @@ struct intel_display_platforms {
>>   #define HAS_CASF(__display)		(DISPLAY_VER(__display) >= 20)
>>   #define HAS_CDCLK_CRAWL(__display)	(DISPLAY_INFO(__display)->has_cdclk_crawl)
>>   #define HAS_CDCLK_SQUASH(__display)	(DISPLAY_INFO(__display)->has_cdclk_squash)
>> +#define HAS_CMN_SDP_TL(__display)	(DISPLAY_VER(__display) >= 35)
>>   #define HAS_CMRR(__display)		(DISPLAY_VER(__display) >= 20)
>>   #define HAS_CMTG(__display)		(!(__display)->platform.dg2 && DISPLAY_VER(__display) >= 13)
>>   #define HAS_CUR_FBC(__display)		(!HAS_GMCH(__display) && IS_DISPLAY_VER(__display, 7, 13))
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>> index 8a2b37c7bccf..474d6e2ae34b 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -1298,6 +1298,15 @@ struct intel_crtc_state {
>>   		struct drm_dp_as_sdp as_sdp;
>>   	} infoframes;
>>   
>> +	struct {
>> +		/* Common SDP Transmission line */
>> +		bool enable;
>> +		int transmission_line;
>> +		int vsc_ext_stagger;
>> +		int pps_stagger;
>> +		int gmp_stagger;
>> +	} cmn_sdp_tl;
>> +
>>   	u8 eld[MAX_ELD_BYTES];
>>   
>>   	/* HDMI scrambling status */
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index 025e906b63a9..86390553800d 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -7287,3 +7287,24 @@ bool intel_dp_joiner_candidate_valid(struct intel_connector *connector,
>>   
>>   	return true;
>>   }
>> +
>> +void intel_dp_cmn_sdp_transmission_line_get_config(struct intel_crtc_state *crtc_state)
>> +{
>> +	struct intel_display *display = to_intel_display(crtc_state);
>> +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>> +	u32 val;
>> +
>> +	if (!HAS_CMN_SDP_TL(display))
>> +		return;
>> +
>> +	val = intel_de_read(display, CMN_SDP_TL(display, cpu_transcoder));
>> +
>> +	crtc_state->cmn_sdp_tl.enable = val & TRANSMISSION_LINE_ENABLE;
> Tracking the enable bit seems fairly pointless. If it's not set we could
> just skip the readout. I don't think we should ever want TL==0?


Makes sense, we can drop enable and just track the base transmission line.


Regards,

Ankit

>
>> +	crtc_state->cmn_sdp_tl.transmission_line = REG_FIELD_GET(BASE_TRANSMISSION_LINE_MASK, val);
>> +
>> +	val = intel_de_read(display, CMN_SDP_TL_STGR_CTL(display, cpu_transcoder));
>> +
>> +	crtc_state->cmn_sdp_tl.vsc_ext_stagger = REG_FIELD_GET(VSC_EXT_STAGGER_MASK, val);
>> +	crtc_state->cmn_sdp_tl.pps_stagger = REG_FIELD_GET(PPS_STAGGER_MASK, val);
>> +	crtc_state->cmn_sdp_tl.gmp_stagger = REG_FIELD_GET(GMP_STAGGER_MASK, val);
>> +}
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
>> index b0bbd5981f57..24df234a43d3 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.h
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
>> @@ -237,4 +237,6 @@ bool intel_dp_joiner_candidate_valid(struct intel_connector *connector,
>>   	for ((__num_joined_pipes) = 1; (__num_joined_pipes) <= (I915_MAX_PIPES); (__num_joined_pipes)++) \
>>   		for_each_if(intel_dp_joiner_candidate_valid(__connector, (__mode)->hdisplay, __num_joined_pipes))
>>   
>> +void intel_dp_cmn_sdp_transmission_line_get_config(struct intel_crtc_state *crtc_state);
>> +
>>   #endif /* __INTEL_DP_H__ */
>> -- 
>> 2.45.2