[Intel-gfx] [PATCH] drm/i915: Modify reset func to handle per engine resets

[Chris Wilson]

On Wed, Mar 16, 2016 at 01:52:04PM +0200, Mika Kuoppala wrote:
> In full gpu reset we prime all engines and reset domains corresponding to
> each engine. Per engine reset is just a special case of this process
> wherein only a single engine is reset. This change is aimed to modify
> relevant functions to achieve this. There are some other steps we carry out
> in case of engine reset which are addressed in later patches.
> 
> Reset func now accepts a mask of all engines that need to be reset. Where
> per engine resets are supported, error handler populates the mask
> accordingly otherwise all engines are specified.
> 
> v2: ALL_ENGINES mask fixup, better for_each_ring_masked (Chris)
> 
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index b854af2c4141..68d766af2ed0 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5018,7 +5018,7 @@ i915_gem_cleanup_ringbuffer(struct drm_device *dev)
>               * expects the system to be in execlists mode on startup,
>               * so we need to reset the GPU back to legacy mode.
>               */
> -            intel_gpu_reset(dev);
> +	    intel_gpu_reset(dev, ALL_ENGINES);

Hmm, whitespace cleanup required around here?

> +/**
> + * gen6_reset_engines - reset individual engines
> + * @dev: DRM device
> + * @engine_mask: mask of intel_ring_flag() engines or ALL_ENGINES for full reset
> + *
> + * This function will reset the individual engines that are set in engine_mask.
> + * If you provide ALL_ENGINES as mask, full global domain reset will be issued.
> + *
> + * Note: It is responsibility of the caller to handle the difference between
> + * asking full domain reset versus reset for all available individual engines.
> + *
> + * Returns 0 on success, nonzero on error.
> + */
> +static int gen6_reset_engines(struct drm_device *dev, unsigned engine_mask)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_engine_cs *engine;
> +	const u32 hw_engine_mask[I915_NUM_RINGS] = {
> +		[RCS] = GEN6_GRDOM_RENDER,
> +		[BCS] = GEN6_GRDOM_BLT,
> +		[VCS] = GEN6_GRDOM_MEDIA,
> +		[VCS2] = GEN8_GRDOM_MEDIA2,
> +		[VECS] = GEN6_GRDOM_VECS,
> +	};
> +	int ret;
> +	u32 hw_mask;

u32 hw_mask;
int ret;

looks neater;

>  
> -	/* Spin waiting for the device to ack the reset request */
> -	ret = wait_for((__raw_i915_read32(dev_priv, GEN6_GDRST) & GEN6_GRDOM_FULL) == 0, 500);
> +	if (engine_mask == ALL_ENGINES) {
> +		hw_mask = GEN6_GRDOM_FULL;
> +	} else {
> +		hw_mask = 0;
> +
and this whitespace doesn't help since this block is entirely about the
computation of hw_mask

That's the sum total of my criticism.
Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre