Re: [PATCH 11/11] [v4] drm/i915/bdw: Ensure a context is loaded before RC6

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Feb 19, 2014 at 10:27:20PM -0800, Ben Widawsky wrote:
> RC6 works a lot like HW contexts in that when the GPU enters RC6 it
> saves away the state to a context, and loads it upon wake.
> 
> It's to be somewhat expected that BIOS will not set up valid GPU state.
> As a result, if loading bad state can cause the GPU to get angry, it
> would make sense then that we need to load state first. There are two
> ways in which we can do this:
> 
> 1. Create 3d state in the driver, load it up, then enable RC6.
> 1b. Reuse a known good state, [and if needed,] just bind objects where
> needed. Then enable RC6.
> 2. Hold off enabling RC6 until userspace has had a chance to complete
> batches.
> 
> There has been discussions in the past with #1 as it has been
> recommended for fixes elsewhere. I'm not opposed to it, I'd just like to
> do the easy thing now to enable the platform.
> 
> This patch is a hack that implement option #2. It will defer enabling
> rc6 until the first batch from userspace has been retired. It suffers
> two flaws. The first is, if the driver is loaded, but a batch is not
> submitted/completed, we'll never enter rc6. The other is, it expects
> userspace to submit a batch with 3d state first. Both of these things
> are not actual flaws for most users because most users will boot to a
> graphical composited desktop. Both mesa, and X will always emit the
> necessary 3d state.
> 
> Once a context is loaded and we enable rc6, the default context should
> inherit the proper state because we always inhibit the restore for the
> default context. This assumes certain things about the workaround/issue
> itself to which I am not privy (primarily that the indirect state
> objects don't actually need to exist).
> 
> With that, there are currently 4 options for BDW:
> 1. Don't use RC6.
> 2. Use RC6 and expect a hang on the first batch submitted for every
> context.
> 3. Use RC6 and use this patch.
> 4. Wait for another workaround implementation.
> 
> NOTE: This patch could be used against other platforms as well.
> 
> v2: Re-add accidentally dropped hunk (Ben)
> 
> v3: Now more compilable (Ben)
> 
> v4: Use the existing enable flag for rc6. This will also make the
> suspend/resume case work properly, which is broken in v3.
> Disable rc6 on reset, and defer re-enabling until the first batch.
> 
> The fact that RC6 residency continues to increment, and that this patch
> prevents a hang on BDW silicon has been:
> Tested-by: Kenneth Graunke <kenneth@whitecape.org> (v1)
> 
> Cc: David E. Box <david.e.box@intel.com>
> Cc: Kristen Carlson Accardi <kristen@linux.intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> squash! drm/i915/bdw: Ensure a context is loaded before RC6
> ---
>  drivers/gpu/drm/i915/i915_drv.c      |  4 +++-
>  drivers/gpu/drm/i915/i915_gem.c      | 10 ++++++++++
>  drivers/gpu/drm/i915/intel_display.c |  5 +++++
>  3 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 2d05d7c..7fdfc0e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -679,6 +679,8 @@ int i915_reset(struct drm_device *dev)
>  	mutex_lock(&dev->struct_mutex);
>  
>  	i915_gem_reset(dev);
> +	if (IS_BROADWELL(dev))
> +		intel_disable_gt_powersave(dev);
>  
>  	simulated = dev_priv->gpu_error.stop_rings != 0;
>  
> @@ -733,7 +735,7 @@ int i915_reset(struct drm_device *dev)
>  		 * reset and the re-install of drm irq. Skip for ironlake per
>  		 * previous concerns that it doesn't respond well to some forms
>  		 * of re-init after reset. */
> -		if (INTEL_INFO(dev)->gen > 5) {
> +		if (INTEL_INFO(dev)->gen > 5 && !IS_BROADWELL(dev)) {
>  			mutex_lock(&dev->struct_mutex);
>  			intel_enable_gt_powersave(dev);
>  			mutex_unlock(&dev->struct_mutex);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 3618bb0..25a97a6 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2420,6 +2420,7 @@ void i915_gem_reset(struct drm_device *dev)
>  void
>  i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
>  {
> +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
>  	uint32_t seqno;
>  
>  	if (list_empty(&ring->request_list))
> @@ -2443,6 +2444,15 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
>  		if (!i915_seqno_passed(seqno, obj->last_read_seqno))
>  			break;
>  
> +		/* Wa: can't find the w/a name.
> +		 * This doesn't actually implement the w/a, but it a workaround
> +		 * for the workaround. It defers using rc6 until we know valid
> +		 * state exists.
> +		 */
> +		if (IS_BROADWELL(ring->dev) && intel_enable_rc6(ring->dev) &&
> +		    !dev_priv->rps.enabled && ring->id == RCS)
> +			intel_enable_gt_powersave(ring->dev);

Nope, we won't rely on userspace submitting a command to enable a power
saving feature. There's a good chance that userspace won't submitting
anything, but still wake up from suspend, e.g. for background mail
checking. Especially on Android where opportunistic is used to
aggressively conserve battery power.

If just a MI_SET_CONTEXT isn't good enough and we need to save a valid 3d
context then we need to (shock, horror) emit the relevant minimal set of
3d state commands in the kernel before enabling rc6.

Aside: Series looks good otherwise, so I'll happily merge once it's
reviewed. No opinion on the RP* bikeshed (besides that your new names for
dev_priv->rps.foo are definitely clearer) as long as the connection
between the sane names and the Bspec names is documented somewhere.
-Daniel

> +
>  		i915_gem_object_move_to_inactive(obj);
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f19e6ea..72c8e1d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10983,6 +10983,11 @@ void intel_modeset_init_hw(struct drm_device *dev)
>  
>  	intel_reset_dpio(dev);
>  
> +	if (IS_BROADWELL(dev)) {
> +		DRM_DEBUG_DRIVER("Deferring RC6 enabling until first batch is complete\n");
> +		return;
> +	}
> +
>  	mutex_lock(&dev->struct_mutex);
>  	intel_enable_gt_powersave(dev);
>  	mutex_unlock(&dev->struct_mutex);
> -- 
> 1.9.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch