[PATCH] drm/i915: Use chipset-specific irq installers

[PATCH] drm/i915: Use chipset-specific irq installers

[Chris Wilson]

Konstantin Belousov pointed out that 4697995b98417 replaced the generic
i915_driver_irq_*install() functions with chipset specific routines
accessible only through driver->irq_*install(). So update the sanity
check in i915_request_wait() to match.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |   31 ++++++++++++++++++++-----------
 1 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 60268173..add0141 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2029,6 +2029,25 @@ i915_gem_retire_work_handler(struct work_struct *work)
 	mutex_unlock(&dev->struct_mutex);
 }
 
+static void assert_irq_enabled(struct intel_ring_buffer *ring)
+{
+	struct drm_device *dev = ring->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 ier;
+
+	if (HAS_PCH_SPLIT(dev))
+		ier = I915_READ(DEIER) | I915_READ(GTIER);
+	else
+		ier = I915_READ(IER);
+
+	if (!ier) {
+		DRM_ERROR("something (likely vbetool) disabled "
+			  "interrupts, re-enabling\n");
+		dev->driver->irq_preinstall(dev);
+		dev->driver->irq_postinstall(dev);
+	}
+}
+
 /**
  * Waits for a sequence number to be signaled, and cleans up the
  * request and object lists appropriately for that event.
@@ -2038,7 +2057,6 @@ i915_wait_request(struct intel_ring_buffer *ring,
 		  uint32_t seqno)
 {
 	drm_i915_private_t *dev_priv = ring->dev->dev_private;
-	u32 ier;
 	int ret = 0;
 
 	BUG_ON(seqno == 0);
@@ -2073,16 +2091,7 @@ i915_wait_request(struct intel_ring_buffer *ring,
 	}
 
 	if (!i915_seqno_passed(ring->get_seqno(ring), seqno)) {
-		if (HAS_PCH_SPLIT(ring->dev))
-			ier = I915_READ(DEIER) | I915_READ(GTIER);
-		else
-			ier = I915_READ(IER);
-		if (!ier) {
-			DRM_ERROR("something (likely vbetool) disabled "
-				  "interrupts, re-enabling\n");
-			i915_driver_irq_preinstall(ring->dev);
-			i915_driver_irq_postinstall(ring->dev);
-		}
+		assert_irq_enabled(ring);
 
 		trace_i915_gem_request_wait_begin(ring, seqno);
 
-- 
1.7.5.4

Re: [PATCH] drm/i915: Use chipset-specific irq installers

[Ben Widawsky]

On Tue, Jun 28, 2011 at 11:48:51AM +0100, Chris Wilson wrote:
> Konstantin Belousov pointed out that 4697995b98417 replaced the generic
> i915_driver_irq_*install() functions with chipset specific routines
> accessible only through driver->irq_*install(). So update the sanity
> check in i915_request_wait() to match.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |   31 ++++++++++++++++++++-----------
>  1 files changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 60268173..add0141 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2029,6 +2029,25 @@ i915_gem_retire_work_handler(struct work_struct *work)
>  	mutex_unlock(&dev->struct_mutex);
>  }
>  
> +static void assert_irq_enabled(struct intel_ring_buffer *ring)
> +{
> +	struct drm_device *dev = ring->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 ier;
> +
> +	if (HAS_PCH_SPLIT(dev))
> +		ier = I915_READ(DEIER) | I915_READ(GTIER);
> +	else
> +		ier = I915_READ(IER);
> +
> +	if (!ier) {
> +		DRM_ERROR("something (likely vbetool) disabled "
> +			  "interrupts, re-enabling\n");
> +		dev->driver->irq_preinstall(dev);
> +		dev->driver->irq_postinstall(dev);
> +	}
> +}
> +
>  /**
>   * Waits for a sequence number to be signaled, and cleans up the
>   * request and object lists appropriately for that event.
> @@ -2038,7 +2057,6 @@ i915_wait_request(struct intel_ring_buffer *ring,
>  		  uint32_t seqno)
>  {
>  	drm_i915_private_t *dev_priv = ring->dev->dev_private;
> -	u32 ier;
>  	int ret = 0;
>  
>  	BUG_ON(seqno == 0);
> @@ -2073,16 +2091,7 @@ i915_wait_request(struct intel_ring_buffer *ring,
>  	}
>  
>  	if (!i915_seqno_passed(ring->get_seqno(ring), seqno)) {
> -		if (HAS_PCH_SPLIT(ring->dev))
> -			ier = I915_READ(DEIER) | I915_READ(GTIER);
> -		else
> -			ier = I915_READ(IER);
> -		if (!ier) {
> -			DRM_ERROR("something (likely vbetool) disabled "
> -				  "interrupts, re-enabling\n");
> -			i915_driver_irq_preinstall(ring->dev);
> -			i915_driver_irq_postinstall(ring->dev);
> -		}
> +		assert_irq_enabled(ring);
>  
>  		trace_i915_gem_request_wait_begin(ring, seqno);
>  

I'm not totally thrilled with the name. It would make more sense to me
if you had something like:
if (!is_irq_enabled(ring)) {
	DRM_ERROR(...);
	dev->driver->irq_preinstall(ring->dev);
	dev->driver->irq_postinstall(ring->dev);
}

This allows allows for error checking at other points without changing
state.

I'm very nitpicky when it comes to the work assert :p

Ben

Re: [PATCH] drm/i915: Use chipset-specific irq installers

[Keith Packard]

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

On Tue, 28 Jun 2011 10:48:57 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:

> I'm very nitpicky when it comes to the work assert :p

I pushed a much simpler patch:

----------------------- drivers/gpu/drm/i915/i915_gem.c -----------------------
index c6389de..e81dbe5 100644
@@ -2080,8 +2080,8 @@ i915_wait_request(struct intel_ring_buffer *ring,
 		if (!ier) {
 			DRM_ERROR("something (likely vbetool) disabled "
 				  "interrupts, re-enabling\n");
-			i915_driver_irq_preinstall(ring->dev);
-			i915_driver_irq_postinstall(ring->dev);
+			ring->dev->driver->irq_preinstall(ring->dev);
+			ring->dev->driver->irq_postinstall(ring->dev);
 		}
 
 		trace_i915_gem_request_wait_begin(ring, seqno);

-- 
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: Use chipset-specific irq installers

[Chris Wilson]

On Tue, 28 Jun 2011 10:48:57 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> I'm not totally thrilled with the name. It would make more sense to me
> if you had something like:
> if (!is_irq_enabled(ring)) {
> 	DRM_ERROR(...);
> 	dev->driver->irq_preinstall(ring->dev);
> 	dev->driver->irq_postinstall(ring->dev);
> }
> 
> This allows allows for error checking at other points without changing
> state.
> 
> I'm very nitpicky when it comes to the work assert :p

*g* I was just following the idiom Jesse started with his
assert_plane_enabled() and friends. :)

You're right that expressed as a predicate function with a separate fixup
routine this would be more useful, but more so is idiomatic programming...

Feel free to supplement the code with more error checking! :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre