Re: [PATCH] [RFT] drm/i915: Ensure a context is loaded before RC6

[Ben Widawsky <ben@bwidawsk.net>]

On Thu, Jan 30, 2014 at 11:00:35AM -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 just bind objects where needed. Then
> enable RC6
> 2. Hold off enabling RC6 until userspace has had a chance to complete
> batches.
> 
> This patch is a bad hack. 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.

There is another flaw actually. If the objects for the indirect state
gets unbound before we enable RC6, or before we return from RC6.

I think for the case I want to test however, both of these are unlikely
- but it's another reason to not use a patch like this upstream.

> 
> Technically, this tactic is required for all platforms, though I am not
> certain we've seen real failures.
> 
> 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>
> ---
>  drivers/gpu/drm/i915/i915_gem.c      | 6 ++++++
>  drivers/gpu/drm/i915/intel_display.c | 4 ----
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 08331e1..83847fc 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2482,6 +2482,7 @@ void i915_gem_reset(struct drm_device *dev)
>  void
>  i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
>  {
> +	static bool rc6_enabled = false;
>  	uint32_t seqno;
>  
>  	if (list_empty(&ring->request_list))
> @@ -2505,6 +2506,11 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
>  		if (!i915_seqno_passed(seqno, obj->last_read_seqno))
>  			break;
>  
> +		if (unlikely(!rc6_enabled) && ring->id == RCS) {
> +			intel_enable_gt_powersave(ring->dev);
> +			rc6_enabled = true;
> +		}
> +
>  		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 4d4a0d9..990819a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10982,10 +10982,6 @@ void intel_modeset_init_hw(struct drm_device *dev)
>  	intel_init_clock_gating(dev);
>  
>  	intel_reset_dpio(dev);
> -
> -	mutex_lock(&dev->struct_mutex);
> -	intel_enable_gt_powersave(dev);
> -	mutex_unlock(&dev->struct_mutex);
>  }
>  
>  void intel_modeset_suspend_hw(struct drm_device *dev)
> -- 
> 1.8.5.3
> 

-- 
Ben Widawsky, Intel Open Source Technology Center