Re: [PATCH] drm/i915: hold forcewake around ring hw init

[Jani Nikula <jani.nikula@linux.intel.com>]

Hi Daniel, please find a couple of comments inline.

BR,
Jani.

On Mon, 04 Jun 2012, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 238a521..a0c76aa 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -233,6 +233,7 @@ static const struct intel_device_info intel_sandybridge_d_info = {
>  	.has_blt_ring = 1,
>  	.has_llc = 1,
>  	.has_pch_split = 1,
> +	.has_force_wake = 1
>  };

Don't know if you care, but sticking a comma at the would avoid touching
the line the next time you need to add a field. Ditto below.

>  
>  static const struct intel_device_info intel_sandybridge_m_info = {
> @@ -243,6 +244,7 @@ static const struct intel_device_info intel_sandybridge_m_info = {
>  	.has_blt_ring = 1,
>  	.has_llc = 1,
>  	.has_pch_split = 1,
> +	.has_force_wake = 1
>  };
>  
>  static const struct intel_device_info intel_ivybridge_d_info = {
> @@ -252,6 +254,7 @@ static const struct intel_device_info intel_ivybridge_d_info = {
>  	.has_blt_ring = 1,
>  	.has_llc = 1,
>  	.has_pch_split = 1,
> +	.has_force_wake = 1
>  };
>  
>  static const struct intel_device_info intel_ivybridge_m_info = {
> @@ -262,6 +265,7 @@ static const struct intel_device_info intel_ivybridge_m_info = {
>  	.has_blt_ring = 1,
>  	.has_llc = 1,
>  	.has_pch_split = 1,
> +	.has_force_wake = 1
>  };
>  
>  static const struct intel_device_info intel_valleyview_m_info = {
> @@ -289,6 +293,7 @@ static const struct intel_device_info intel_haswell_d_info = {
>  	.has_blt_ring = 1,
>  	.has_llc = 1,
>  	.has_pch_split = 1,
> +	.has_force_wake = 1
>  };
>  
>  static const struct intel_device_info intel_haswell_m_info = {
> @@ -298,6 +303,7 @@ static const struct intel_device_info intel_haswell_m_info = {
>  	.has_blt_ring = 1,
>  	.has_llc = 1,
>  	.has_pch_split = 1,
> +	.has_force_wake = 1
>  };
>  

[snip]

> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 89a5e7f..bdb37f2 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -266,10 +266,14 @@ u32 intel_ring_get_active_head(struct intel_ring_buffer *ring)
>  
>  static int init_ring_common(struct intel_ring_buffer *ring)
>  {
> -	drm_i915_private_t *dev_priv = ring->dev->dev_private;
> +	struct drm_device *dev = ring->dev;
> +	drm_i915_private_t *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_object *obj = ring->obj;
>  	u32 head;
>  
> +	if (HAS_FORCE_WAKE(dev))
> +		gen6_gt_force_wake_get(dev_priv);
> +
>  	/* Stop the ring if it's running. */
>  	I915_WRITE_CTL(ring, 0);
>  	I915_WRITE_HEAD(ring, 0);
> @@ -328,6 +332,9 @@ static int init_ring_common(struct intel_ring_buffer *ring)
>  		ring->space = ring_space(ring);
>  	}
>  

Above, outside of patch context, there's an error code path with return
-EIO, which I think would leak dev_priv->forcewake_count.

> +	if (HAS_FORCE_WAKE(dev))
> +		gen6_gt_force_wake_put(dev_priv);
> +
>  	return 0;
>  }