Re: [PATCH 2/4] drm/i915: Introduce intel_uncore_unclaimed_access

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org, miku@iki.fi
Subject: Re: [PATCH 2/4] drm/i915: Introduce intel_uncore_unclaimed_access
Date: Wed, 9 Dec 2015 15:15:34 +0000	[thread overview]
Message-ID: <20151209151534.GC29974@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <1449673301-6449-2-git-send-email-mika.kuoppala@intel.com>

On Wed, Dec 09, 2015 at 05:01:39PM +0200, Mika Kuoppala wrote:
> Currently interrupt code is the only place checking
> for the unclaimed register access prior to actual register
> macros using the same functionality. Rename the function
> and make it return bool so that the possible error message
> context is clear in the caller side. The motivation is to allow
> usage of unclaimed detection on arbitrary places.
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h     | 2 +-
>  drivers/gpu/drm/i915/i915_irq.c     | 3 ++-
>  drivers/gpu/drm/i915/intel_uncore.c | 5 ++---
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2e8c0cbc..30c1aeb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2725,7 +2725,7 @@ extern void intel_uncore_sanitize(struct drm_device *dev);
>  extern void intel_uncore_early_sanitize(struct drm_device *dev,
>  					bool restore_forcewake);
>  extern void intel_uncore_init(struct drm_device *dev);
> -extern void intel_uncore_check_errors(struct drm_device *dev);
> +extern bool intel_uncore_unclaimed_access(struct drm_device *dev);
>  extern void intel_uncore_fini(struct drm_device *dev);
>  extern void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore);
>  const char *intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index e88d692..1be3e11 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2167,7 +2167,8 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  
>  	/* We get interrupts on unclaimed registers, so check for this before we
>  	 * do any I915_{READ,WRITE}. */
> -	intel_uncore_check_errors(dev);
> +	if (intel_uncore_unclaimed_access(dev))
> +		DRM_ERROR("Unclaimed register before interrupt\n");

This reminds me that having this error message is spectacularly
unhelpful. Could I persuade you to adopt

http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=breadcrumbs&id=357b468e6555cd8a59858aaee1d408f846267896

it's been on the list a few times. With your new framework it may make
sense to move it about. For instance, doing the check every interrupt is
not great.

>  	/* disable master interrupt before clearing iir  */
>  	de_ier = I915_READ(DEIER);
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 9d672f7..96b04c7 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1602,8 +1602,7 @@ bool intel_has_gpu_reset(struct drm_device *dev)
>  	return intel_get_gpu_reset(dev) != NULL;
>  }
>  
> -void intel_uncore_check_errors(struct drm_device *dev)
> +bool intel_uncore_unclaimed_access(struct drm_device *dev)
>  {
> -	if (unclaimed_reg_access(to_i915(dev)))
> -		DRM_ERROR("Unclaimed register before interrupt\n");
> +	return unclaimed_reg_access(to_i915(dev));
>  }

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx