Re: [PATCH v4 05/12] drm/i915/display: Add crtc state for DIP transmission lines

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

On 9/8/2026 9:05 PM, Jani Nikula wrote:
> On Thu, 11 Jun 2026, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote:
>> The Adaptive Sync SDP is currently the only packet with a programmable
>> transmission line.
>>
>> Make a structure struct intel_dip for Data Island Packets. Add a member to
>> track Adaptive-Sync SDP transmission line. Include the new member in the
>> pipe configuration comparison.
>>
>> This will pave the way for supporting more packets' programmable
>> transmission lines, including the common base SDP transmission line
>> introduced with Xe3p_lpd.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_dip.h           | 8 ++++++++
>>   drivers/gpu/drm/i915/display/intel_display.c       | 2 ++
>>   drivers/gpu/drm/i915/display/intel_display_types.h | 3 +++
>>   3 files changed, 13 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dip.h b/drivers/gpu/drm/i915/display/intel_dip.h
>> index 25bae4a04d6b..37507ac3e645 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dip.h
>> +++ b/drivers/gpu/drm/i915/display/intel_dip.h
>> @@ -35,4 +35,12 @@ struct intel_crtc_state;
>>   u16 intel_dip_read_emp_as_sdp_tl(const struct intel_crtc_state *crtc_state);
>>   void intel_dip_write_emp_as_sdp_tl(const struct intel_crtc_state *crtc_state);
>>   
>> +struct intel_dip {
>> +	/*
>> +	 * DIP Transmission line, relative to the Vtotal.
>> +	 * The programmed transmit line is (Vtotal - value)
>> +	 */
>> +	u16 emp_as_sdp_tl;
>> +};
>> +
>>   #endif /* __INTEL_DIP_H__ */
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index bdf02b67c1d8..c84ee0e865ee 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -5507,6 +5507,8 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>>   
>>   	PIPE_CONF_CHECK_I(set_context_latency);
>>   
>> +	PIPE_CONF_CHECK_I(dip.emp_as_sdp_tl);
>> +
>>   #undef PIPE_CONF_CHECK_X
>>   #undef PIPE_CONF_CHECK_I
>>   #undef PIPE_CONF_CHECK_LLI
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>> index aa4772a1c208..6cc3a4c2f17c 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -46,6 +46,7 @@
>>   
>>   #include "i915_gtt_view_types.h"
>>   #include "intel_bios.h"
>> +#include "intel_dip.h"
> I understand the motivation, but I'd rather keep the struct here in
> intel_display_types.h than blow up the includes. There are already too
> many includes *and* this file is included just about everywhere.
>
> The point is, if you change anything unrelated to struct intel_dip in
> intel_dip.h, you'll end up rebuilding pretty much all the display code,
> and it'll slow everyone down.
>
> C really sucks at this. And embedding a lot of structs makes it worse.


Good point. I had not thought about this, though the pattern was always 
there, but it became clearer to me now.

I will move the structs to intel_display_types.h, and keep this in mind.

Thanks & Regards,
Ankit

>
>
> BR,
> Jani.
>
>
>
>>   #include "intel_display.h"
>>   #include "intel_display_conversion.h"
>>   #include "intel_display_limits.h"
>> @@ -1301,6 +1302,8 @@ struct intel_crtc_state {
>>   		struct drm_dp_as_sdp as_sdp;
>>   	} infoframes;
>>   
>> +	struct intel_dip dip;
>> +
>>   	u8 eld[MAX_ELD_BYTES];
>>   
>>   	/* HDMI scrambling status */