[PATCH] drm/i915: Busy-spin wait_for condition in atomic contexts

[PATCH] drm/i915: Busy-spin wait_for condition in atomic contexts

[Chris Wilson]

During modesetting, we need to wait for the hardware to report
readiness by polling the registers. Normally, we call msleep() between
reads, because some state changes may take a whole vblank or more
to complete. However during a panic, we are in an atomic context and
cannot sleep. Instead, busy spin polling the termination condition.

References: https://bugzilla.kernel.org/show_bug.cgi?id=31772
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_drv.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5daa991..ac70398 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -39,7 +39,7 @@
 			ret__ = -ETIMEDOUT;				\
 			break;						\
 		}							\
-		if (W && !in_dbg_master()) msleep(W);			\
+		if (W && !(in_dbg_master() || in_atomic())) msleep(W);	\
 	}								\
 	ret__;								\
 })
-- 
1.7.4.1

Re: [PATCH] drm/i915: Busy-spin wait_for condition in atomic contexts

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 499 bytes --]

On Thu, 24 Mar 2011 19:55:59 +0000, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> -		if (W && !in_dbg_master()) msleep(W);			\
> +		if (W && !(in_dbg_master() || in_atomic())) msleep(W);  \

If the MSLEEP macro were ever used, would it need the same fix?

wait_for_atomic is never used, so perhaps the _wait_for macro should
just be renamed wait_for and the W hard-coded to 1.

As a simple fix though,

Reviewed-by: Keith Packard <keithp@keithp.com>

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Busy-spin wait_for condition in atomic contexts

[Chris Wilson]

On Thu, 24 Mar 2011 17:38:32 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Thu, 24 Mar 2011 19:55:59 +0000, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > -		if (W && !in_dbg_master()) msleep(W);			\
> > +		if (W && !(in_dbg_master() || in_atomic())) msleep(W);  \
> 
> If the MSLEEP macro were ever used, would it need the same fix?

Not yet. There are places where we should be using it though I think, It
just requires inspecting each of the callpaths and seeing if we might be
called from an atomic modeswitch.
 
> wait_for_atomic is never used, so perhaps the _wait_for macro should
> just be renamed wait_for and the W hard-coded to 1.

A bit of over-engineering. I want to propose these as part of the core
kernel interface and so tried to cover all the bases. But I'm not sure if
they are actually clean enough to pass muster nor if anybody else is
interested in them.
 
> As a simple fix though,
> 
> Reviewed-by: Keith Packard <keithp@keithp.com>

Indeed, a fix should limit itself to the bug described. Every else is an
enhancement...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Busy-spin wait_for condition in atomic contexts

[Jesse Barnes]

On Thu, 24 Mar 2011 19:55:59 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> During modesetting, we need to wait for the hardware to report
> readiness by polling the registers. Normally, we call msleep() between
> reads, because some state changes may take a whole vblank or more
> to complete. However during a panic, we are in an atomic context and
> cannot sleep. Instead, busy spin polling the termination condition.
> 
> References: https://bugzilla.kernel.org/show_bug.cgi?id=31772
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_drv.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5daa991..ac70398 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -39,7 +39,7 @@
>  			ret__ = -ETIMEDOUT;				\
>  			break;						\
>  		}							\
> -		if (W && !in_dbg_master()) msleep(W);			\
> +		if (W && !(in_dbg_master() || in_atomic())) msleep(W);	\
>  	}								\
>  	ret__;								\
>  })

Yeah, this should be fine; though I think in_dbg_master always implies
in_atomic too, so you could just collapse the check.

Jason, can you confirm that?

-- 
Jesse Barnes, Intel Open Source Technology Center