Re: [PATCH RFC 3/3] drm/i915/dp: Configure PORT_AUX_CTL and then trigger the tx

["Murthy, Arun R" <arun.r.murthy@intel.com>]

On 09-03-2026 18:39, Jani Nikula wrote:
> On Mon, 09 Mar 2026, Arun R Murthy <arun.r.murthy@intel.com> wrote:
>> Use re_rmw and update the required bits for PORT_AUX_CTL and drop the
>> bit configurations that are not required(AUX Power Request setting of
>> bit 19). Also break writing to PORT_AUX_CTL into 2 steps with first step
>> for doing the configuration/settings and then second write to trigger
>> the AUX transaction.
> The primary question the commit message should answer is, "Why?"
>
> There's a whole lot of "What?" here, indeed too much since the patch is
> doing too many things in one go.
>
> The point of an RFC patch is to solicit feedback on the idea. But the
> idea remains vague here as there's no rationale why this is needed.
Before initiating the AUX transaction, power well is enabled by calling 
intel_display_power_get().

Here the AUX Power Request bit19 of PORT_AUX_CTL register is being set 
and with a timeout of 600us the AUX Power State is checked to see if the 
power is enabled.

Then as part of the AUX transaction, PORT_AUX_CTL is written using 
intel_reg_write. Since we are writing to the entire register the same 
bit19 AUX Power Request is again being set to ensure the bit is set. 
This setting of bit is un-necessary as its already being set in 
intel_display_power_get.
Hence in order to overcome this un-necessary setting of bit, 
read/mask/write is being used.

Thanks and Regards,
Arun R Murthy
-------------------

>
> BR,
> Jani.
>
>
>> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_display_types.h |  6 +-
>>   drivers/gpu/drm/i915/display/intel_dp_aux.c        | 83 ++++++++++++++--------
>>   drivers/gpu/drm/i915/display/intel_psr.c           | 29 +++++---
>>   3 files changed, 76 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>> index e189f8c39ccb440f99cd642de177b18f3b605753..341749452579acfc3e08715d2f0b211bf6489dd9 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -1882,10 +1882,10 @@ struct intel_dp {
>>   
>>   	u32 (*get_aux_clock_divider)(struct intel_dp *dp, int index);
>>   	/*
>> -	 * This function returns the value we have to program the AUX_CTL
>> -	 * register with to kick off an AUX transaction.
>> +	 * This function programs the configuration/settings for the AUX_CTL
>> +	 * register but dont kick off an AUX transaction.
>>   	 */
>> -	u32 (*get_aux_send_ctl)(struct intel_dp *dp, int send_bytes,
>> +	void (*get_aux_send_ctl)(struct intel_dp *dp, int send_bytes,
>>   				u32 aux_clock_divider);
>>   
>>   	i915_reg_t (*aux_ch_ctl_reg)(struct intel_dp *dp);
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c b/drivers/gpu/drm/i915/display/intel_dp_aux.c
>> index 0a9e2d6cdbc5d9e0d17b2db60a32cf20a3bad6b6..4fef378e0a8fbf79211fd98913e507e90b2b48ea 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
>> @@ -175,12 +175,13 @@ static int g4x_dp_aux_precharge_len(void)
>>   		precharge_min - preamble) / 2;
>>   }
>>   
>> -static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
>> -				int send_bytes,
>> -				u32 aux_clock_divider)
>> +static void g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
>> +				 int send_bytes,
>> +				 u32 aux_clock_divider)
>>   {
>>   	struct intel_display *display = to_intel_display(intel_dp);
>> -	u32 timeout;
>> +	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
>> +	u32 timeout, value;
>>   
>>   	/* Max timeout value on G4x-BDW: 1.6ms */
>>   	if (display->platform.broadwell)
>> @@ -188,8 +189,7 @@ static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
>>   	else
>>   		timeout = DP_AUX_CH_CTL_TIME_OUT_400us;
>>   
>> -	return DP_AUX_CH_CTL_SEND_BUSY |
>> -		DP_AUX_CH_CTL_DONE |
>> +	value = DP_AUX_CH_CTL_DONE |
>>   		DP_AUX_CH_CTL_INTERRUPT |
>>   		DP_AUX_CH_CTL_TIME_OUT_ERROR |
>>   		timeout |
>> @@ -197,23 +197,35 @@ static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
>>   		DP_AUX_CH_CTL_MESSAGE_SIZE(send_bytes) |
>>   		DP_AUX_CH_CTL_PRECHARGE_2US(g4x_dp_aux_precharge_len()) |
>>   		DP_AUX_CH_CTL_BIT_CLOCK_2X(aux_clock_divider);
>> +
>> +	intel_de_rmw(display, ch_ctl,
>> +		     (DP_AUX_CH_CTL_DONE |
>> +		      DP_AUX_CH_CTL_INTERRUPT |
>> +		      DP_AUX_CH_CTL_TIME_OUT_ERROR |
>> +		      DP_AUX_CH_CTL_TIME_OUT_MASK |
>> +		      DP_AUX_CH_CTL_RECEIVE_ERROR |
>> +		      DP_AUX_CH_CTL_MESSAGE_SIZE_MASK |
>> +		      DP_AUX_CH_CTL_PRECHARGE_2US_MASK |
>> +		      DP_AUX_CH_CTL_BIT_CLOCK_2X_MASK),
>> +		     value);
>> +	return;
>>   }
>>   
>> -static u32 skl_get_aux_send_ctl(struct intel_dp *intel_dp,
>> -				int send_bytes,
>> -				u32 unused)
>> +static void skl_get_aux_send_ctl(struct intel_dp *intel_dp,
>> +				 int send_bytes,
>> +				 u32 unused)
>>   {
>>   	struct intel_display *display = to_intel_display(intel_dp);
>>   	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> -	u32 ret;
>> +	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
>> +	u32 value;
>>   
>>   	/*
>>   	 * Max timeout values:
>>   	 * SKL-GLK: 1.6ms
>>   	 * ICL+: 4ms
>>   	 */
>> -	ret = DP_AUX_CH_CTL_SEND_BUSY |
>> -		DP_AUX_CH_CTL_DONE |
>> +	value = DP_AUX_CH_CTL_DONE |
>>   		DP_AUX_CH_CTL_INTERRUPT |
>>   		DP_AUX_CH_CTL_TIME_OUT_ERROR |
>>   		DP_AUX_CH_CTL_TIME_OUT_MAX |
>> @@ -222,17 +234,22 @@ static u32 skl_get_aux_send_ctl(struct intel_dp *intel_dp,
>>   		DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(intel_dp_aux_fw_sync_len(intel_dp)) |
>>   		DP_AUX_CH_CTL_SYNC_PULSE_SKL(intel_dp_aux_sync_len());
>>   
>> -	if (intel_tc_port_in_tbt_alt_mode(dig_port))
>> -		ret |= DP_AUX_CH_CTL_TBT_IO;
>> +	intel_de_rmw(display, ch_ctl,
>> +		     (DP_AUX_CH_CTL_DONE |
>> +		      DP_AUX_CH_CTL_INTERRUPT |
>> +		      DP_AUX_CH_CTL_TIME_OUT_ERROR |
>> +		      DP_AUX_CH_CTL_TIME_OUT_MASK |
>> +		      DP_AUX_CH_CTL_RECEIVE_ERROR |
>> +		      DP_AUX_CH_CTL_MESSAGE_SIZE_MASK |
>> +		      DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL_MASK |
>> +		      DP_AUX_CH_CTL_SYNC_PULSE_SKL_MASK),
>> +		     value);
>>   
>> -	/*
>> -	 * Power request bit is already set during aux power well enable.
>> -	 * Preserve the bit across aux transactions.
>> -	 */
>> -	if (DISPLAY_VER(display) >= 14)
>> -		ret |= XELPDP_DP_AUX_CH_CTL_POWER_REQUEST;
>> +	if (intel_tc_port_in_tbt_alt_mode(dig_port))
>> +		intel_de_rmw(display, ch_ctl, DP_AUX_CH_CTL_TBT_IO,
>> +			     DP_AUX_CH_CTL_TBT_IO);
>>   
>> -	return ret;
>> +	return;
>>   }
>>   
>>   static int
>> @@ -341,11 +358,12 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
>>   	}
>>   
>>   	while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {
>> -		u32 send_ctl = intel_dp->get_aux_send_ctl(intel_dp,
>> -							  send_bytes,
>> -							  aux_clock_divider);
>> +		intel_dp->get_aux_send_ctl(intel_dp, send_bytes,
>> +					   aux_clock_divider);
>>   
>> -		send_ctl |= aux_send_ctl_flags;
>> +		/* Update the flags */
>> +		intel_de_rmw(display, ch_ctl, DP_AUX_CH_CTL_AUX_AKSV_SELECT,
>> +			     aux_send_ctl_flags);
>>   
>>   		/* Must try at least 3 times according to DP spec */
>>   		for (try = 0; try < 5; try++) {
>> @@ -356,15 +374,20 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
>>   								 send_bytes - i));
>>   
>>   			/* Send the command and wait for it to complete */
>> -			intel_de_write(display, ch_ctl, send_ctl);
>> +			intel_de_rmw(display, ch_ctl,
>> +				     DP_AUX_CH_CTL_SEND_BUSY,
>> +				     DP_AUX_CH_CTL_SEND_BUSY);
>>   
>>   			status = intel_dp_aux_wait_done(intel_dp);
>>   
>>   			/* Clear done status and any errors */
>> -			intel_de_write(display, ch_ctl,
>> -				       status | DP_AUX_CH_CTL_DONE |
>> -				       DP_AUX_CH_CTL_TIME_OUT_ERROR |
>> -				       DP_AUX_CH_CTL_RECEIVE_ERROR);
>> +			intel_de_rmw(display, ch_ctl,
>> +				     (DP_AUX_CH_CTL_DONE |
>> +				      DP_AUX_CH_CTL_TIME_OUT_ERROR |
>> +				      DP_AUX_CH_CTL_RECEIVE_ERROR),
>> +				     (DP_AUX_CH_CTL_DONE |
>> +				      DP_AUX_CH_CTL_TIME_OUT_ERROR |
>> +				      DP_AUX_CH_CTL_RECEIVE_ERROR));
>>   
>>   			/*
>>   			 * DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>> index 9296ca3a4ff468a6e61babd81217e4ba19b15062..e06e04f20355d511e5c58fc28866aa763fd65a4b 100644
>> --- a/drivers/gpu/drm/i915/display/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>> @@ -722,7 +722,9 @@ static void hsw_psr_setup_aux(struct intel_dp *intel_dp)
>>   {
>>   	struct intel_display *display = to_intel_display(intel_dp);
>>   	enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
>> +	i915_reg_t ch_ctl = psr_aux_ctl_reg(display, cpu_transcoder);
>>   	u32 aux_clock_divider, aux_ctl;
>> +
>>   	/* write DP_SET_POWER=D0 */
>>   	static const u8 aux_msg[] = {
>>   		[0] = (DP_AUX_NATIVE_WRITE << 4) | ((DP_SET_POWER >> 16) & 0xf),
>> @@ -742,17 +744,26 @@ static void hsw_psr_setup_aux(struct intel_dp *intel_dp)
>>   	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
>>   
>>   	/* Start with bits set for DDI_AUX_CTL register */
>> -	aux_ctl = intel_dp->get_aux_send_ctl(intel_dp, sizeof(aux_msg),
>> -					     aux_clock_divider);
>> +	intel_dp->get_aux_send_ctl(intel_dp, sizeof(aux_msg),
>> +				   aux_clock_divider);
>>   
>>   	/* Select only valid bits for SRD_AUX_CTL */
>> -	aux_ctl &= EDP_PSR_AUX_CTL_TIME_OUT_MASK |
>> -		EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK |
>> -		EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK |
>> -		EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK;
>> -
>> -	intel_de_write(display, psr_aux_ctl_reg(display, cpu_transcoder),
>> -		       aux_ctl);
>> +	aux_ctl = EDP_PSR_AUX_CTL_TIME_OUT_MASK |
>> +		  EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK |
>> +		  EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK |
>> +		  EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK;
>> +
>> +	intel_de_rmw(display, ch_ctl,
>> +		     (EDP_PSR_AUX_CTL_TIME_OUT_MASK |
>> +		      EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK |
>> +		      EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK |
>> +		      EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK),
>> +		     aux_ctl);
>> +
>> +	/* Send the command or intitate the AUX transaction */
>> +	intel_de_rmw(display, ch_ctl,
>> +		     DP_AUX_CH_CTL_SEND_BUSY,
>> +		     DP_AUX_CH_CTL_SEND_BUSY);
>>   }
>>   
>>   static bool psr2_su_region_et_valid(struct intel_connector *connector, bool panel_replay)