diff mbox series

[PATCHv6] drm/i915/dp: change aux_ctl reg read to polling read

Message ID 20221215111338.1080698-1-arun.r.murthy@intel.com (mailing list archive)
State New, archived
Headers show
Series [PATCHv6] drm/i915/dp: change aux_ctl reg read to polling read | expand

Commit Message

Arun R Murthy Dec. 15, 2022, 11:13 a.m. UTC
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(-)

Comments

Arun R Murthy Dec. 19, 2022, 10:17 a.m. UTC | #1
Any comments?

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 Dec. 20, 2022, 4:02 p.m. UTC | #2
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
>
Arun R Murthy Dec. 20, 2022, 4:54 p.m. UTC | #3
> -----Original Message-----
> From: Nikula, Jani <jani.nikula@intel.com>
> Sent: Tuesday, December 20, 2022 9:33 PM
> To: Murthy, Arun R <arun.r.murthy@intel.com>; intel-
> gfx@lists.freedesktop.org; ville.syrjala@linux.intel.com; Deak, Imre
> <imre.deak@intel.com>
> Subject: RE: [PATCHv6] drm/i915/dp: change aux_ctl reg read to polling read
> 
> 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.

Recommendation as per windows code base is 50us interval for polling the register, so will change it to 50us.

Thanks and Regards,
Arun R Murthy
--------------------
> 
> 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
Jani Nikula Dec. 20, 2022, 8:58 p.m. UTC | #4
On Tue, 20 Dec 2022, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote:
>> -----Original Message-----
>> From: Nikula, Jani <jani.nikula@intel.com>
>> Sent: Tuesday, December 20, 2022 9:33 PM
>> To: Murthy, Arun R <arun.r.murthy@intel.com>; intel-
>> gfx@lists.freedesktop.org; ville.syrjala@linux.intel.com; Deak, Imre
>> <imre.deak@intel.com>
>> Subject: RE: [PATCHv6] drm/i915/dp: change aux_ctl reg read to polling read
>>
>> 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.
>
> Recommendation as per windows code base is 50us interval for polling
> the register, so will change it to 50us.

The parameter is *not* the interval. It's the timeout for fast wait in a
loop with just cpu_relax() in between. So please keep it 2 us.

If the condition doesn't happen in the fast timeout, it'll fall back to
the slow wait (in this case 10 ms timeout), which starts off with 10 us
interval, doubling on every poll until it's above 1000 us.


BR,
Jani.


>
> Thanks and Regards,
> Arun R Murthy
> --------------------
>>
>> 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
diff mbox series

Patch

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;
 }