Re: [Intel-gfx] [PATCHv6] drm/i915/dp: change aux_ctl reg read to polling read

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

On Mon, 19 Dec 2022, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote:
> Any comments?

From #intel-gfx:

<vsyrjala> bashing the hw for 500 usec seems a bit harsh

Which is true. The default for intel_de_wait_for_register() is 2
us. Should probably stick to that.

BR,
Jani.


>
> Thanks and Regards,
> Arun R Murthy
> --------------------
>
>> -----Original Message-----
>> From: Murthy, Arun R <arun.r.murthy@intel.com>
>> Sent: Thursday, December 15, 2022 4:44 PM
>> To: intel-gfx@lists.freedesktop.org; ville.syrjala@linux.intel.com; Nikula, Jani
>> <jani.nikula@intel.com>; Deak, Imre <imre.deak@intel.com>
>> Cc: Murthy, Arun R <arun.r.murthy@intel.com>
>> Subject: [PATCHv6] drm/i915/dp: change aux_ctl reg read to polling read
>>
>> The busy timeout logic checks for the AUX BUSY, then waits for the timeout
>> period and then after timeout reads the register for BUSY or Success.
>> Instead replace interrupt with polling so as to read the AUX CTL register often
>> before the timeout period. Looks like there might be some issue with
>> interrupt-on-read. Hence changing the logic to polling read.
>>
>> v2: replace interrupt with polling read
>> v3: use usleep_rang instead of msleep, updated commit msg
>> v4: use intel_wait_for_regiter internal function
>> v5: use __intel_de_wait_for_register with 500us slow and 10ms fast timeout
>> v6: check return value of __intel_de_wait_for_register
>>
>> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_dp_aux.c | 14 +++++---------
>>  1 file changed, 5 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c
>> b/drivers/gpu/drm/i915/display/intel_dp_aux.c
>> index 91c93c93e5fc..973dadecf712 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
>> @@ -41,20 +41,16 @@ 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;
>> -     bool done;
>> -
>> -#define C (((status = intel_de_read_notrace(i915, ch_ctl)) &
>> DP_AUX_CH_CTL_SEND_BUSY) == 0)
>> -     done = wait_event_timeout(i915->display.gmbus.wait_queue, C,
>> -                               msecs_to_jiffies_timeout(timeout_ms));
>> +     int ret;
>>
>> -     /* just trace the final value */
>> -     trace_i915_reg_rw(false, ch_ctl, status, sizeof(status), true);
>> +     ret = __intel_de_wait_for_register(i915, ch_ctl,
>> +                                        DP_AUX_CH_CTL_SEND_BUSY, 0,
>> +                                        500, timeout_ms, &status);
>>
>> -     if (!done)
>> +     if (ret == -ETIMEDOUT)
>>               drm_err(&i915->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.25.1
>

-- 
Jani Nikula, Intel Open Source Graphics Center