The wait_for macro's for Broadcom VC4 and V3D drivers used msleep which is inappropriate due to its inaccuracy at low values (minimum wait time is about 30ms on the Raspberry Pi).
This patch replaces the macro with the one from the Intel i915 version which uses usleep_range to provide more accurate waits.
Signed-off-by: James Hughes james.hughes@raspberrypi.com --- drivers/gpu/drm/v3d/v3d_drv.h | 41 ++++++++++++++++++++++----------- drivers/gpu/drm/vc4/vc4_drv.h | 43 +++++++++++++++++++++-------------- 2 files changed, 54 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h index 7b4f3b5a086e..069fefe16d28 100644 --- a/drivers/gpu/drm/v3d/v3d_drv.h +++ b/drivers/gpu/drm/v3d/v3d_drv.h @@ -267,27 +267,42 @@ struct v3d_csd_job { };
/** - * _wait_for - magic (register) wait macro + * __wait_for - magic wait macro * - * Does the right thing for modeset paths when run under kdgb or similar atomic - * contexts. Note that it's important that we check the condition again after - * having timed out, since the timeout could be due to preemption or similar and - * we've never had a chance to check the condition before the timeout. + * Macro to help avoid open coding check/wait/timeout patterns. Note that it's + * important that we check the condition again after having timed out, since the + * timeout could be due to preemption or similar and we've never had a chance to + * check the condition before the timeout. */ -#define wait_for(COND, MS) ({ \ - unsigned long timeout__ = jiffies + msecs_to_jiffies(MS) + 1; \ - int ret__ = 0; \ - while (!(COND)) { \ - if (time_after(jiffies, timeout__)) { \ - if (!(COND)) \ - ret__ = -ETIMEDOUT; \ +#define __wait_for(OP, COND, US, Wmin, Wmax) ({ \ + const ktime_t end__ = ktime_add_ns(ktime_get_raw(), 1000ll * (US)); \ + long wait__ = (Wmin); /* recommended min for usleep is 10 us */ \ + int ret__; \ + might_sleep(); \ + for (;;) { \ + const bool expired__ = ktime_after(ktime_get_raw(), end__); \ + OP; \ + /* Guarantee COND check prior to timeout */ \ + barrier(); \ + if (COND) { \ + ret__ = 0; \ break; \ } \ - msleep(1); \ + if (expired__) { \ + ret__ = -ETIMEDOUT; \ + break; \ + } \ + usleep_range(wait__, wait__ * 2); \ + if (wait__ < (Wmax)) \ + wait__ <<= 1; \ } \ ret__; \ })
+#define _wait_for(COND, US, Wmin, Wmax) __wait_for(, (COND), (US), (Wmin), \ + (Wmax)) +#define wait_for(COND, MS) _wait_for((COND), (MS) * 1000, 10, 1000) + static inline unsigned long nsecs_to_jiffies_timeout(const u64 n) { /* nsecs_to_jiffies64() does not guard against overflow */ diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index 7956f6ed9b6a..c59be5e1d52b 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -629,32 +629,41 @@ struct vc4_validated_shader_info { };
/** - * _wait_for - magic (register) wait macro + * __wait_for - magic wait macro * - * Does the right thing for modeset paths when run under kdgb or similar atomic - * contexts. Note that it's important that we check the condition again after - * having timed out, since the timeout could be due to preemption or similar and - * we've never had a chance to check the condition before the timeout. + * Macro to help avoid open coding check/wait/timeout patterns. Note that it's + * important that we check the condition again after having timed out, since the + * timeout could be due to preemption or similar and we've never had a chance to + * check the condition before the timeout. */ -#define _wait_for(COND, MS, W) ({ \ - unsigned long timeout__ = jiffies + msecs_to_jiffies(MS) + 1; \ - int ret__ = 0; \ - while (!(COND)) { \ - if (time_after(jiffies, timeout__)) { \ - if (!(COND)) \ - ret__ = -ETIMEDOUT; \ +#define __wait_for(OP, COND, US, Wmin, Wmax) ({ \ + const ktime_t end__ = ktime_add_ns(ktime_get_raw(), 1000ll * (US)); \ + long wait__ = (Wmin); /* recommended min for usleep is 10 us */ \ + int ret__; \ + might_sleep(); \ + for (;;) { \ + const bool expired__ = ktime_after(ktime_get_raw(), end__); \ + OP; \ + /* Guarantee COND check prior to timeout */ \ + barrier(); \ + if (COND) { \ + ret__ = 0; \ break; \ } \ - if (W && drm_can_sleep()) { \ - msleep(W); \ - } else { \ - cpu_relax(); \ + if (expired__) { \ + ret__ = -ETIMEDOUT; \ + break; \ } \ + usleep_range(wait__, wait__ * 2); \ + if (wait__ < (Wmax)) \ + wait__ <<= 1; \ } \ ret__; \ })
-#define wait_for(COND, MS) _wait_for(COND, MS, 1) +#define _wait_for(COND, US, Wmin, Wmax) __wait_for(, (COND), (US), (Wmin), \ + (Wmax)) +#define wait_for(COND, MS) _wait_for((COND), (MS) * 1000, 10, 1000)
/* vc4_bo.c */ struct drm_gem_object *vc4_create_object(struct drm_device *dev, size_t size);
On Mon, Feb 17, 2020 at 7:41 AM James Hughes james.hughes@raspberrypi.com wrote:
The wait_for macro's for Broadcom VC4 and V3D drivers used msleep which is inappropriate due to its inaccuracy at low values (minimum wait time is about 30ms on the Raspberry Pi).
This patch replaces the macro with the one from the Intel i915 version which uses usleep_range to provide more accurate waits.
Signed-off-by: James Hughes james.hughes@raspberrypi.com
To apply this, we're going to want to split the patch up between v3d (with a fixes tag to the introduction of the driver, since it's a pretty critical fix) and vc4 (where it's used in KMS, and we're pretty sure we want it but changing timing like this in KMS paths is risky so we don't want to backport to stable). And adjust the commit messages to have consistent prefixes to the rest of the commits to those drivers.
I've been fighting with the drm maintainer tools today to try to apply the patch, with no luck. I'll keep trying, and if I succeed, I'll push it.
On Wed, 19 Feb 2020 at 22:51, Eric Anholt eric@anholt.net wrote:
On Mon, Feb 17, 2020 at 7:41 AM James Hughes james.hughes@raspberrypi.com wrote:
The wait_for macro's for Broadcom VC4 and V3D drivers used msleep which is inappropriate due to its inaccuracy at low values (minimum wait time is about 30ms on the Raspberry Pi).
This patch replaces the macro with the one from the Intel i915 version which uses usleep_range to provide more accurate waits.
Signed-off-by: James Hughes james.hughes@raspberrypi.com
To apply this, we're going to want to split the patch up between v3d (with a fixes tag to the introduction of the driver, since it's a pretty critical fix) and vc4 (where it's used in KMS, and we're pretty sure we want it but changing timing like this in KMS paths is risky so we don't want to backport to stable). And adjust the commit messages to have consistent prefixes to the rest of the commits to those drivers.
I've been fighting with the drm maintainer tools today to try to apply the patch, with no luck. I'll keep trying, and if I succeed, I'll push it.
Hi Eric,
unclear whether you want me to do the split or whether you are going to (your last paragraph). Also I'm a bit unclear on the exact requirements for the prefixes etc.
James
On Thu, Feb 20, 2020 at 1:44 AM James Hughes james.hughes@raspberrypi.com wrote:
On Wed, 19 Feb 2020 at 22:51, Eric Anholt eric@anholt.net wrote:
On Mon, Feb 17, 2020 at 7:41 AM James Hughes james.hughes@raspberrypi.com wrote:
The wait_for macro's for Broadcom VC4 and V3D drivers used msleep which is inappropriate due to its inaccuracy at low values (minimum wait time is about 30ms on the Raspberry Pi).
This patch replaces the macro with the one from the Intel i915 version which uses usleep_range to provide more accurate waits.
Signed-off-by: James Hughes james.hughes@raspberrypi.com
To apply this, we're going to want to split the patch up between v3d (with a fixes tag to the introduction of the driver, since it's a pretty critical fix) and vc4 (where it's used in KMS, and we're pretty sure we want it but changing timing like this in KMS paths is risky so we don't want to backport to stable). And adjust the commit messages to have consistent prefixes to the rest of the commits to those drivers.
I've been fighting with the drm maintainer tools today to try to apply the patch, with no luck. I'll keep trying, and if I succeed, I'll push it.
Hi Eric,
unclear whether you want me to do the split or whether you are going to (your last paragraph). Also I'm a bit unclear on the exact requirements for the prefixes etc.
I debugged what was going on with my maintainer tools and got the patches applied:
https://cgit.freedesktop.org/drm/drm-misc/commit/?id=9daee6141cc9c75b09659b0... https://cgit.freedesktop.org/drm/drm-misc/commit/?id=7f2a09ecf2e8d86e22598df...
Apologies for the wait! I've fixed some mail filters I think so I should notice pings like this in the future. But also I hope Maxime can feel enabled to merge patches to vc4/v3d in the future -- I certainly don't want to be the limiting factor here, and it's under drm-misc so any drm-misc maintainer can apply stuff.
dri-devel@lists.freedesktop.org