RE: [PATCH 2/2] Revert "drm/i915/dp: change aux_ctl reg read to polling read"

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 20 Nov 2025, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote:
>> -----Original Message-----
>> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville
>> Syrjala
>> Sent: Thursday, November 20, 2025 12:23 AM
>> To: intel-gfx@lists.freedesktop.org
>> Cc: intel-xe@lists.freedesktop.org
>> Subject: [PATCH 2/2] Revert "drm/i915/dp: change aux_ctl reg read to polling
>> read"
>> 
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> 
>> This reverts commit 5a9b0c7418448ed3766f61ba0a71d08f259c3181.
>> 
>> The switch from AUX interrupts to pollign was very hand-wavy.
>> Yes, there have been some situations in CI on a few platforms where the AUX
>> hardware seemingly forgets to signal the timeout, but those have been
>> happening after we switched to polling as well. So I don't think we have any
>> conclusive evidence that polling actually helps here.
>> 
>> Someone really should root cause the actual problem, and see if there is a
>> proper workaround we could implemnt (eg. disabling clock gating/etc.). In the
>> meantime just go back to using the interrupt for AUX completion.
>> 
>> If the hardware fails to signal the timeout we will just hit the
>> wait_event_timeout() software timeout instead. I suppose we could try to tune
>> the software timeout to more closely match the expected hardware timeout.
>> Might need to use
>> wait_event_hrtimeout() or something to avoid jiffies granularity issues...
>> 
>> The AUX polling is also a hinderance towards using poll_timeout_us() because
>> we have a very long timeout, but would need a fairly short polling interval to
>> keep AUX transfer reasonably fast. Someone would need to come up with good
>> numbers in a somewhat scientific way.
>> 
> Upon multiple rounds of validation based on the results polling had improvements when compared with the interrupt mechanism. We can optimize more by using poll_timeout, I am afraid that reverting back to interrupts may end up with more failures.

I'm not sure the issues were root caused properly.

BR,
Jani.


>
> Thanks and Regards,
> Arun R Murthy
> --------------------
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_dp_aux.c | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c
>> b/drivers/gpu/drm/i915/display/intel_dp_aux.c
>> index 809799f63e32..d1a93e4a59b5 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
>> @@ -6,6 +6,7 @@
>>  #include <drm/drm_print.h>
>> 
>>  #include "intel_de.h"
>> +#include "intel_display_jiffies.h"
>>  #include "intel_display_types.h"
>>  #include "intel_display_utils.h"
>>  #include "intel_dp.h"
>> @@ -60,16 +61,17 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp)
>>  	i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
>>  	const unsigned int timeout_ms = 10;
>>  	u32 status;
>> -	int ret;
>> +	bool done;
>> 
>> -	ret = intel_de_wait_ms(display, ch_ctl,
>> -			       DP_AUX_CH_CTL_SEND_BUSY, 0,
>> -			       timeout_ms, &status);
>> +#define C (((status = intel_de_read_notrace(display, ch_ctl)) &
>> DP_AUX_CH_CTL_SEND_BUSY) == 0)
>> +	done = wait_event_timeout(display->gmbus.wait_queue, C,
>> +				  msecs_to_jiffies_timeout(timeout_ms));
>> 
>> -	if (ret == -ETIMEDOUT)
>> +	if (!done)
>>  		drm_err(display->drm,
>>  			"%s: did not complete or timeout within %ums (status
>> 0x%08x)\n",
>>  			intel_dp->aux.name, timeout_ms, status);
>> +#undef C
>> 
>>  	return status;
>>  }
>> --
>> 2.49.1
>

-- 
Jani Nikula, Intel