[Intel-gfx] [PATCH 08/34] drm/i915: Make all GPU resets atomic

[John Harrison]

On 1/21/2019 14:20, Chris Wilson wrote:
> In preparation for the next few commits, make resetting the GPU atomic.
> Currently, we have prepared gen6+ for atomic resetting of individual
> engines, but now there is a requirement to perform the whole device
> level reset (just the register poking) from inside an atomic context.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Reviewed-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_reset.c | 50 +++++++++++++++++--------------
>   1 file changed, 27 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c
> index 342d9ee42601..b9d0ea70361c 100644
> --- a/drivers/gpu/drm/i915/i915_reset.c
> +++ b/drivers/gpu/drm/i915/i915_reset.c
> @@ -144,14 +144,14 @@ static int i915_do_reset(struct drm_i915_private *i915,
>   
>   	/* Assert reset for at least 20 usec, and wait for acknowledgement. */
>   	pci_write_config_byte(pdev, I915_GDRST, GRDOM_RESET_ENABLE);
> -	usleep_range(50, 200);
> -	err = wait_for(i915_in_reset(pdev), 500);
> +	udelay(50);
> +	err = wait_for_atomic(i915_in_reset(pdev), 50);
Is it known to be safe to reduce all of these time out values? Where did 
the originally 500ms value come from? Is there any chance of getting 
sporadic failures because 50ms is borderline in the worst case scenario? 
It still sounds huge but an order of magnitude change in a timeout 
always seems worrying!

>   
>   	/* Clear the reset request. */
>   	pci_write_config_byte(pdev, I915_GDRST, 0);
> -	usleep_range(50, 200);
> +	udelay(50);
>   	if (!err)
> -		err = wait_for(!i915_in_reset(pdev), 500);
> +		err = wait_for_atomic(!i915_in_reset(pdev), 50);
>   
>   	return err;
>   }
> @@ -171,7 +171,7 @@ static int g33_do_reset(struct drm_i915_private *i915,
>   	struct pci_dev *pdev = i915->drm.pdev;
>   
>   	pci_write_config_byte(pdev, I915_GDRST, GRDOM_RESET_ENABLE);
> -	return wait_for(g4x_reset_complete(pdev), 500);
> +	return wait_for_atomic(g4x_reset_complete(pdev), 50);
>   }
>   
>   static int g4x_do_reset(struct drm_i915_private *dev_priv,
> @@ -182,13 +182,13 @@ static int g4x_do_reset(struct drm_i915_private *dev_priv,
>   	int ret;
>   
>   	/* WaVcpClkGateDisableForMediaReset:ctg,elk */
> -	I915_WRITE(VDECCLK_GATE_D,
> -		   I915_READ(VDECCLK_GATE_D) | VCP_UNIT_CLOCK_GATE_DISABLE);
> -	POSTING_READ(VDECCLK_GATE_D);
> +	I915_WRITE_FW(VDECCLK_GATE_D,
> +		      I915_READ(VDECCLK_GATE_D) | VCP_UNIT_CLOCK_GATE_DISABLE);
> +	POSTING_READ_FW(VDECCLK_GATE_D);
>   
>   	pci_write_config_byte(pdev, I915_GDRST,
>   			      GRDOM_MEDIA | GRDOM_RESET_ENABLE);
> -	ret =  wait_for(g4x_reset_complete(pdev), 500);
> +	ret =  wait_for_atomic(g4x_reset_complete(pdev), 50);
>   	if (ret) {
>   		DRM_DEBUG_DRIVER("Wait for media reset failed\n");
>   		goto out;
> @@ -196,7 +196,7 @@ static int g4x_do_reset(struct drm_i915_private *dev_priv,
>   
>   	pci_write_config_byte(pdev, I915_GDRST,
>   			      GRDOM_RENDER | GRDOM_RESET_ENABLE);
> -	ret =  wait_for(g4x_reset_complete(pdev), 500);
> +	ret =  wait_for_atomic(g4x_reset_complete(pdev), 50);
>   	if (ret) {
>   		DRM_DEBUG_DRIVER("Wait for render reset failed\n");
>   		goto out;
> @@ -205,9 +205,9 @@ static int g4x_do_reset(struct drm_i915_private *dev_priv,
>   out:
>   	pci_write_config_byte(pdev, I915_GDRST, 0);
>   
> -	I915_WRITE(VDECCLK_GATE_D,
> -		   I915_READ(VDECCLK_GATE_D) & ~VCP_UNIT_CLOCK_GATE_DISABLE);
> -	POSTING_READ(VDECCLK_GATE_D);
> +	I915_WRITE_FW(VDECCLK_GATE_D,
> +		      I915_READ(VDECCLK_GATE_D) & ~VCP_UNIT_CLOCK_GATE_DISABLE);
> +	POSTING_READ_FW(VDECCLK_GATE_D);
>   
>   	return ret;
>   }
> @@ -218,27 +218,29 @@ static int ironlake_do_reset(struct drm_i915_private *dev_priv,
>   {
>   	int ret;
>   
> -	I915_WRITE(ILK_GDSR, ILK_GRDOM_RENDER | ILK_GRDOM_RESET_ENABLE);
> -	ret = intel_wait_for_register(dev_priv,
> -				      ILK_GDSR, ILK_GRDOM_RESET_ENABLE, 0,
> -				      500);
> +	I915_WRITE_FW(ILK_GDSR, ILK_GRDOM_RENDER | ILK_GRDOM_RESET_ENABLE);
> +	ret = __intel_wait_for_register_fw(dev_priv, ILK_GDSR,
> +					   ILK_GRDOM_RESET_ENABLE, 0,
> +					   5000, 0,
> +					   NULL);
These two timeouts are now two orders of magnitude smaller? It was 500ms 
but is now 5000us (=5ms)?

John.


>   	if (ret) {
>   		DRM_DEBUG_DRIVER("Wait for render reset failed\n");
>   		goto out;
>   	}
>   
> -	I915_WRITE(ILK_GDSR, ILK_GRDOM_MEDIA | ILK_GRDOM_RESET_ENABLE);
> -	ret = intel_wait_for_register(dev_priv,
> -				      ILK_GDSR, ILK_GRDOM_RESET_ENABLE, 0,
> -				      500);
> +	I915_WRITE_FW(ILK_GDSR, ILK_GRDOM_MEDIA | ILK_GRDOM_RESET_ENABLE);
> +	ret = __intel_wait_for_register_fw(dev_priv, ILK_GDSR,
> +					   ILK_GRDOM_RESET_ENABLE, 0,
> +					   5000, 0,
> +					   NULL);
>   	if (ret) {
>   		DRM_DEBUG_DRIVER("Wait for media reset failed\n");
>   		goto out;
>   	}
>   
>   out:
> -	I915_WRITE(ILK_GDSR, 0);
> -	POSTING_READ(ILK_GDSR);
> +	I915_WRITE_FW(ILK_GDSR, 0);
> +	POSTING_READ_FW(ILK_GDSR);
>   	return ret;
>   }
>   
> @@ -572,7 +574,9 @@ int intel_gpu_reset(struct drm_i915_private *i915, unsigned int engine_mask)
>   		ret = -ENODEV;
>   		if (reset) {
>   			GEM_TRACE("engine_mask=%x\n", engine_mask);
> +			preempt_disable();
>   			ret = reset(i915, engine_mask, retry);
> +			preempt_enable();
>   		}
>   		if (ret != -ETIMEDOUT || engine_mask != ALL_ENGINES)
>   			break;