[1/2] drm/i915: Do not return stale status / remove impossible WARN
diff mbox

Message ID 1455213790-4724-1-git-send-email-tvrtko.ursulin@linux.intel.com
State New
Headers show

Commit Message

Tvrtko Ursulin Feb. 11, 2016, 6:03 p.m. UTC
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Only caller to get_context_status ensures read pointer stays in
range so the WARN is impossible. Also, if the WARN would be
triggered by a hypothetical new caller stale status would be
returned to them.

Maybe it is better to wrap the pointer in the function itself
then to avoid both and even results in smaller code.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

Comments

Chris Wilson Feb. 11, 2016, 9 p.m. UTC | #1
On Thu, Feb 11, 2016 at 06:03:09PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Only caller to get_context_status ensures read pointer stays in
> range so the WARN is impossible. Also, if the WARN would be
> triggered by a hypothetical new caller stale status would be
> returned to them.
> 
> Maybe it is better to wrap the pointer in the function itself
> then to avoid both and even results in smaller code.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 89eb892df4ae..951f1e6af947 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -507,17 +507,16 @@ static bool execlists_check_remove_request(struct intel_engine_cs *ring,
>  	return false;
>  }
>  
> -static void get_context_status(struct intel_engine_cs *ring,
> -			       u8 read_pointer,
> -			       u32 *status, u32 *context_id)
> +static u32 get_context_status(struct intel_engine_cs *ring, u8 read_pointer,
> +			      u32 *context_id)
>  {
>  	struct drm_i915_private *dev_priv = ring->dev->dev_private;
>  
> -	if (WARN_ON(read_pointer >= GEN8_CSB_ENTRIES))
> -		return;
> +	read_pointer %= GEN8_CSB_ENTRIES;
>  
> -	*status = I915_READ(RING_CONTEXT_STATUS_BUF_LO(ring, read_pointer));
>  	*context_id = I915_READ(RING_CONTEXT_STATUS_BUF_HI(ring, read_pointer));

Micro-optimising hat says not to even do the uncached, spinlocked mmio
read when not required.
-Chris

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 89eb892df4ae..951f1e6af947 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -507,17 +507,16 @@  static bool execlists_check_remove_request(struct intel_engine_cs *ring,
 	return false;
 }
 
-static void get_context_status(struct intel_engine_cs *ring,
-			       u8 read_pointer,
-			       u32 *status, u32 *context_id)
+static u32 get_context_status(struct intel_engine_cs *ring, u8 read_pointer,
+			      u32 *context_id)
 {
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
 
-	if (WARN_ON(read_pointer >= GEN8_CSB_ENTRIES))
-		return;
+	read_pointer %= GEN8_CSB_ENTRIES;
 
-	*status = I915_READ(RING_CONTEXT_STATUS_BUF_LO(ring, read_pointer));
 	*context_id = I915_READ(RING_CONTEXT_STATUS_BUF_HI(ring, read_pointer));
+
+	return I915_READ(RING_CONTEXT_STATUS_BUF_LO(ring, read_pointer));
 }
 
 /**
@@ -547,9 +546,7 @@  void intel_lrc_irq_handler(struct intel_engine_cs *ring)
 	spin_lock(&ring->execlist_lock);
 
 	while (read_pointer < write_pointer) {
-
-		get_context_status(ring, ++read_pointer % GEN8_CSB_ENTRIES,
-				   &status, &status_id);
+		status = get_context_status(ring, ++read_pointer, &status_id);
 
 		if (status & GEN8_CTX_STATUS_IDLE_ACTIVE)
 			continue;