| Message ID | 20250326070054.68355-1-bold.zone2373@fastmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/amd/display: replace use of msleep(<20) with usleep_range for better accuracy | expand |
On 3/26/25 01:00, James Flowers wrote: > msleep < 20ms will often sleep for ~20ms (according to Documentation/timers/timers-howto.rst). Can you elaborate and explain why this change is necessary? > > Signed-off-by: James Flowers <bold.zone2373@fastmail.com> > --- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > index 2cd35392e2da..2d225735602b 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > @@ -682,7 +682,7 @@ static bool execute_synaptics_rc_command(struct drm_dp_aux *aux, > if (rc_cmd == cmd) > // active is 0 > break; > - msleep(10); > + usleep_range(10000, 10200); > } > > // read rc result thanks, -- Shuah
On Fri, Mar 28, 2025, at 3:46 PM, Shuah Khan wrote: > On 3/26/25 01:00, James Flowers wrote: >> msleep < 20ms will often sleep for ~20ms (according to Documentation/timers/timers-howto.rst). > > Can you elaborate and explain why this change is necessary? scripts/checkpatch.pl highlighted it as a possible issue since msleep can cause the delay to be unexpectedly closer to 20ms. However, perhaps it would be better to submit a V2 with the change as usleep_range(10000, 20000) to match the actual delay range that msleep(10) has, which would help avoid a regression like this: https://gitlab.freedesktop.org/drm/amd/-/issues/3559#note_2532546. Is there any appetite for drm/amd to change instances of msleep(x) (x being < 20) to usleep_range(x*1000, 20000), so that the range of delay is clear to readers unfamiliar with the msleep quirk? Please let me know and I can submit a V2. There is at least one instance (fixed by commit 57995aa8ffb3e47a74763cf9106d34e9e8be9d8d) in drm/amdgpu where msleep(1) seemed to be causing an unexpectedly long delay, and so was replaced with usleep_range. An issue possibly related to that fix is https://gitlab.freedesktop.org/drm/amd/-/issues/462. Currently there are 37 msleep(1) calls that I can find in drm/amd. I'm just thinking that optimization opportunities like above could be found more easily if the delay range is made clear? Best regards, James Flowers
Am 26.03.25 um 08:00 schrieb James Flowers: > msleep < 20ms will often sleep for ~20ms (according to Documentation/timers/timers-howto.rst). Our display team has to decide but I don't think that this patch is justified. The time given to msleep is just the minimum time necessary for some HW action to take place. Waiting longer than that is usually not harmful except when you want to optimize total operation time. Regards, Christian. > > Signed-off-by: James Flowers <bold.zone2373@fastmail.com> > --- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > index 2cd35392e2da..2d225735602b 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > @@ -682,7 +682,7 @@ static bool execute_synaptics_rc_command(struct drm_dp_aux *aux, > if (rc_cmd == cmd) > // active is 0 > break; > - msleep(10); > + usleep_range(10000, 10200); > } > > // read rc result
On 2025-03-31 08:34, Christian König wrote: > Am 26.03.25 um 08:00 schrieb James Flowers: >> msleep < 20ms will often sleep for ~20ms (according to Documentation/timers/timers-howto.rst). > > Our display team has to decide but I don't think that this patch is justified. > > The time given to msleep is just the minimum time necessary for some HW action to take place. Waiting longer than that is usually not harmful except when you want to optimize total operation time. > Agreed. Little timing changes often have unintended effects. I have no desire to change working code unless it's required to fix a real-life issue. Harry > Regards, > Christian. > >> >> Signed-off-by: James Flowers <bold.zone2373@fastmail.com> >> --- >> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c >> index 2cd35392e2da..2d225735602b 100644 >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c >> @@ -682,7 +682,7 @@ static bool execute_synaptics_rc_command(struct drm_dp_aux *aux, >> if (rc_cmd == cmd) >> // active is 0 >> break; >> - msleep(10); >> + usleep_range(10000, 10200); >> } >> >> // read rc result >
On Mon, Mar 31, 2025, at 11:20 AM, Harry Wentland wrote: > Agreed. Little timing changes often have unintended effects. > I have no desire to change working code unless it's required > to fix a real-life issue. > > Harry Thanks for your explanation, and for taking the time to review. Best regards, James Flowers
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c index 2cd35392e2da..2d225735602b 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c @@ -682,7 +682,7 @@ static bool execute_synaptics_rc_command(struct drm_dp_aux *aux, if (rc_cmd == cmd) // active is 0 break; - msleep(10); + usleep_range(10000, 10200); } // read rc result
msleep < 20ms will often sleep for ~20ms (according to Documentation/timers/timers-howto.rst). Signed-off-by: James Flowers <bold.zone2373@fastmail.com> --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)