Re: [PATCH 17/18] drm/i915: Ironlake rc6 can use context interfaces

[Daniel Vetter <daniel@ffwll.ch>]

On Sun, Mar 18, 2012 at 01:39:57PM -0700, Ben Widawsky wrote:
> Use the context interfaces to create the power context.  Assuming we
> have a default context, there should be no need to switch to
> the render context anymore as the default context should already serve
> this purpose.
> 
> As a double cautionary measure we check the CCID to make sure everything
> looks kosher, and don't enable RC6 if it doesn't.
> 
> There is an important difference in logic when switching to the context
> interface. The old code use MI_SUSPEND_FLUSH before MI_SET_CONTEXT which was an
> ILK specific workaround I remember seeing in old docs but can no longer find.
> That workaround is not implemented in the standard context code.
> 
> PS. I think there is a double mutex_unlock in the existing error path
> 
> CC: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

As I've said in the previous patch review, I don't think it makes much
sense to convert the powercontext to your context code. Afaics the word
context is the only common thing here.

The reaping of the add-hoc renderctx in here is rather nice though. But I
expected this to go hand-in-hand with enabling your context code for gen5
(so that this gets replaced with the default context).

I've been pretty confused when trying to figure out how your code decides
whether it should enable hw context, until I've noticed that you return -1
in get_context_size. Can we make this slightly more explicit, with e.g.
not even attempting to init hw contexts for gen < 5/6? HAS_HW_CONTEXT
macro that checks the gen would be nice ...
-Daniel
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |    8 +--
>  drivers/gpu/drm/i915/i915_drv.h      |    3 +-
>  drivers/gpu/drm/i915/i915_reg.h      |    1 +
>  drivers/gpu/drm/i915/intel_display.c |   89 +++-------------------------------
>  4 files changed, 10 insertions(+), 91 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index fdb7cce..6c98d18 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1372,13 +1372,7 @@ static int i915_context_status(struct seq_file *m, void *unused)
>  
>  	if (dev_priv->pwrctx) {
>  		seq_printf(m, "power context ");
> -		describe_obj(m, dev_priv->pwrctx);
> -		seq_printf(m, "\n");
> -	}
> -
> -	if (dev_priv->renderctx) {
> -		seq_printf(m, "render context ");
> -		describe_obj(m, dev_priv->renderctx);
> +		describe_obj(m, dev_priv->pwrctx->obj);
>  		seq_printf(m, "\n");
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 003b62e..1329b1f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -352,8 +352,7 @@ typedef struct drm_i915_private {
>  	drm_dma_handle_t *status_page_dmah;
>  	uint32_t counter;
>  	drm_local_map_t hws_map;
> -	struct drm_i915_gem_object *pwrctx;
> -	struct drm_i915_gem_object *renderctx;
> +	struct i915_hw_context *pwrctx;
>  
>  	struct resource mch_res;
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 6b6d685..4965638 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1360,6 +1360,7 @@
>   * Logical Context regs
>   */
>  #define CCID			0x2180
> +#define CCID_MASK		0xfffff000
>  #define   CCID_EN		(1<<0)
>  #define CXT_SIZE		0x21a0
>  #define GEN6_CXT_POWER_SIZE(cxt_reg)	((cxt_reg >> 24) & 0x3f)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index de1ba19..4ef968a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7948,42 +7948,6 @@ static const struct drm_mode_config_funcs intel_mode_funcs = {
>  	.output_poll_changed = intel_fb_output_poll_changed,
>  };
>  
> -static struct drm_i915_gem_object *
> -intel_alloc_context_page(struct drm_device *dev)
> -{
> -	struct drm_i915_gem_object *ctx;
> -	int ret;
> -
> -	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> -
> -	ctx = i915_gem_alloc_object(dev, 4096);
> -	if (!ctx) {
> -		DRM_DEBUG("failed to alloc power context, RC6 disabled\n");
> -		return NULL;
> -	}
> -
> -	ret = i915_gem_object_pin(ctx, 4096, true);
> -	if (ret) {
> -		DRM_ERROR("failed to pin power context: %d\n", ret);
> -		goto err_unref;
> -	}
> -
> -	ret = i915_gem_object_set_to_gtt_domain(ctx, 1);
> -	if (ret) {
> -		DRM_ERROR("failed to set-domain on power context: %d\n", ret);
> -		goto err_unpin;
> -	}
> -
> -	return ctx;
> -
> -err_unpin:
> -	i915_gem_object_unpin(ctx);
> -err_unref:
> -	drm_gem_object_unreference(&ctx->base);
> -	mutex_unlock(&dev->struct_mutex);
> -	return NULL;
> -}
> -
>  bool ironlake_set_drps(struct drm_device *dev, u8 val)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -8667,15 +8631,8 @@ static void ironlake_teardown_rc6(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	if (dev_priv->renderctx) {
> -		i915_gem_object_unpin(dev_priv->renderctx);
> -		drm_gem_object_unreference(&dev_priv->renderctx->base);
> -		dev_priv->renderctx = NULL;
> -	}
> -
>  	if (dev_priv->pwrctx) {
> -		i915_gem_object_unpin(dev_priv->pwrctx);
> -		drm_gem_object_unreference(&dev_priv->pwrctx->base);
> +		i915_context_destroy_anonymous(dev_priv->pwrctx);
>  		dev_priv->pwrctx = NULL;
>  	}
>  }
> @@ -8704,13 +8661,11 @@ static int ironlake_setup_rc6(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	if (dev_priv->renderctx == NULL)
> -		dev_priv->renderctx = intel_alloc_context_page(dev);
> -	if (!dev_priv->renderctx)
> -		return -ENOMEM;
> +	if (dev_priv->ring[RCS].default_context == NULL)
> +		return -EIO;
>  
>  	if (dev_priv->pwrctx == NULL)
> -		dev_priv->pwrctx = intel_alloc_context_page(dev);
> +		dev_priv->pwrctx = i915_context_alloc_anonymous(dev);
>  	if (!dev_priv->pwrctx) {
>  		ironlake_teardown_rc6(dev);
>  		return -ENOMEM;
> @@ -8737,43 +8692,13 @@ void ironlake_enable_rc6(struct drm_device *dev)
>  		return;
>  	}
>  
> -	/*
> -	 * GPU can automatically power down the render unit if given a page
> -	 * to save state.
> -	 */
> -	ret = BEGIN_LP_RING(6);
> -	if (ret) {
> -		ironlake_teardown_rc6(dev);
> -		mutex_unlock(&dev->struct_mutex);
> -		return;
> -	}
> -
> -	OUT_RING(MI_SUSPEND_FLUSH | MI_SUSPEND_FLUSH_EN);
> -	OUT_RING(MI_SET_CONTEXT);
> -	OUT_RING(dev_priv->renderctx->gtt_offset |
> -		 MI_MM_SPACE_GTT |
> -		 MI_SAVE_EXT_STATE_EN |
> -		 MI_RESTORE_EXT_STATE_EN |
> -		 MI_RESTORE_INHIBIT);
> -	OUT_RING(MI_SUSPEND_FLUSH);
> -	OUT_RING(MI_NOOP);
> -	OUT_RING(MI_FLUSH);
> -	ADVANCE_LP_RING();
> -
> -	/*
> -	 * Wait for the command parser to advance past MI_SET_CONTEXT. The HW
> -	 * does an implicit flush, combined with MI_FLUSH above, it should be
> -	 * safe to assume that renderctx is valid
> -	 */
> -	ret = intel_wait_ring_idle(LP_RING(dev_priv));
> -	if (ret) {
> -		DRM_ERROR("failed to enable ironlake power power savings\n");
> -		ironlake_teardown_rc6(dev);
> +	if ((I915_READ(CCID) & CCID_MASK) == 0) {
> +		DRM_ERROR("RC6 could not be enabled\n");
>  		mutex_unlock(&dev->struct_mutex);
>  		return;
>  	}
>  
> -	I915_WRITE(PWRCTXA, dev_priv->pwrctx->gtt_offset | PWRCTX_EN);
> +	I915_WRITE(PWRCTXA, dev_priv->pwrctx->obj->gtt_offset | PWRCTX_EN);
>  	I915_WRITE(RSTDBYCTL, I915_READ(RSTDBYCTL) & ~RCX_SW_EXIT);
>  	mutex_unlock(&dev->struct_mutex);
>  }
> -- 
> 1.7.9.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48