Re: [PATCH 06/18] drm/i915: Defer default hardware context initialisation until first open

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Mar 26, 2015 at 12:41:13PM -0700, yu.dai@intel.com wrote:
> From: Dave Gordon <david.s.gordon@intel.com>
> 
> In order to fully initialise the default contexts, we have to execute
> batchbuffer commands on the GPU engines. But we can't do that until any
> required firmware has been loaded, which may not be possible during
> driver load, because the filesystem(s) containing the firmware may not
> be mounted until later.
> 
> Therefore, we now allow the first call to the firmware-loading code to
> return -EAGAIN to indicate that it's not yet ready, and that it should
> be retried when the device is first opened from user code, by which
> time we expect that all required filesystems will have been mounted.
> The late-retry code will then re-attempt to load the firmware if the
> early attempt failed.

We've tried a similar approach a while back and it doesn't work well in
conjunction with rps - the hw tends to fall over if the context state
isn't properly initialized when going into rc6.

Why exactly can't we load that firmware right at boot-up, or at least
stall correctly until it's there?
-Daniel

> 
> Issue: VIZ-4884
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  1 +
>  drivers/gpu/drm/i915/i915_gem.c         |  9 ++++++++-
>  drivers/gpu/drm/i915/i915_gem_context.c | 35 ++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_guc.h        |  2 +-
>  drivers/gpu/drm/i915/intel_guc_loader.c | 22 ++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_uc_loader.c  | 12 +++++++++--
>  6 files changed, 69 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1a60f86..45bdf30 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1755,6 +1755,7 @@ struct drm_i915_private {
>  	/* hda/i915 audio component */
>  	bool audio_component_registered;
>  
> +	bool contexts_ready;
>  	uint32_t hw_context_size;
>  	struct list_head context_list;
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 38e49d9..0037ccf 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4828,8 +4828,15 @@ i915_gem_init_hw(struct drm_device *dev)
>  		i915_gem_cleanup_ringbuffer(dev);
>  	}
>  
> +	/* We can't enable contexts until all firmware is loaded */
> +	ret = intel_guc_load_ucode(dev, false);
> +	if (ret == -EAGAIN)
> +		return 0;		/* too early */
> +
>  	ret = i915_gem_context_enable(dev_priv);
> -	if (ret && ret != -EIO) {
> +	if (ret == 0) {
> +		dev_priv->contexts_ready = true;
> +	} else if (ret && ret != -EIO) {
>  		DRM_ERROR("Context enable failed %d\n", ret);
>  		i915_gem_cleanup_ringbuffer(dev);
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index f3e84c4..2353d8f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -445,23 +445,48 @@ static int context_idr_cleanup(int id, void *p, void *data)
>  	return 0;
>  }
>  
> +/* Complete any late initialisation here */
> +static int i915_gem_context_first_open(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret;
> +
> +	/* We can't enable contexts until all firmware is loaded */
> +	ret = intel_guc_load_ucode(dev, true);
> +	WARN_ON(ret == -EAGAIN);
> +
> +	ret = i915_gem_context_enable(dev_priv);
> +	if (ret == 0)
> +		dev_priv->contexts_ready = true;
> +	return ret;
> +}
> +
>  int i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
>  {
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_file_private *file_priv = file->driver_priv;
>  	struct intel_context *ctx;
> +	int ret = 0;
>  
>  	idr_init(&file_priv->context_idr);
>  
>  	mutex_lock(&dev->struct_mutex);
> -	ctx = i915_gem_create_context(dev, file_priv);
> +
> +	if (!dev_priv->contexts_ready)
> +		ret = i915_gem_context_first_open(dev);
> +
> +	if (ret == 0) {
> +		ctx = i915_gem_create_context(dev, file_priv);
> +		if (IS_ERR(ctx))
> +			ret = PTR_ERR(ctx);
> +	}
> +
>  	mutex_unlock(&dev->struct_mutex);
>  
> -	if (IS_ERR(ctx)) {
> +	if (ret)
>  		idr_destroy(&file_priv->context_idr);
> -		return PTR_ERR(ctx);
> -	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index a06a7b3..d8262cf 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -89,7 +89,7 @@ struct intel_guc {
>  				 GUC_ENABLE_READ_CACHE_FOR_WOPCM_DATA)
>  
>  /* intel_guc_loader.c */
> -extern int intel_guc_load_ucode(struct drm_device *dev);
> +extern int intel_guc_load_ucode(struct drm_device *dev, bool wait);
>  extern void intel_guc_ucode_fini(struct drm_device *dev);
>  extern void intel_guc_ucode_init(struct drm_device *dev);
>  
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index b9923b7..315e5d9 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -217,25 +217,41 @@ out:
>  }
>  
>  /*
> - * Called from gem_init_hw() during driver loading and also after a GPU reset.
>   * Check that the firmware fetching process has succeeded, and if so transfer
>   * the loaded image to the hardware.
> + *
> + * However, there are a few checks to do first. The very first call should have
> + * (wait == FALSE), but the fetch_state will still be PENDING as the firmware may
> + * not be available that early. Therefore, on this first call, we just return.
> + *
> + * The second call should come from the first open of the device (wait == TRUE).
> + * This is a good time to load the firmware into the device, as by this point it
> + * must be available.
> + *
> + * Any subsequent calls are expected to have wait == FALSE, and indicate that the
> + * hardware has been reset and so the firmware should be reloaded.
>   */
> -int intel_guc_load_ucode(struct drm_device *dev)
> +int intel_guc_load_ucode(struct drm_device *dev, bool wait)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
>  	int err;
>  
> +	DRM_DEBUG_DRIVER("GuC: wait %d, fetch status %d, load status %d\n",
> +		wait, guc_fw->uc_fw_fetch_status, guc_fw->uc_fw_load_status);
> +
> +	if (guc_fw->uc_fw_fetch_status == INTEL_UC_FIRMWARE_PENDING && !wait)
> +		return -EAGAIN;
> +
>  	guc_fw->uc_fw_load_status = INTEL_UC_FIRMWARE_NONE;
>  	if (guc_fw->uc_fw_fetch_status == INTEL_UC_FIRMWARE_NONE)
>  		return 0;
>  
> -	guc_fw->uc_fw_load_status = INTEL_UC_FIRMWARE_PENDING;
>  	err = intel_uc_fw_check(dev, guc_fw);
>  	if (err)
>  		goto fail;
>  
> +	guc_fw->uc_fw_load_status = INTEL_UC_FIRMWARE_PENDING;
>  	err = guc_load_ucode(dev);
>  	if (err)
>  		goto fail;
> diff --git a/drivers/gpu/drm/i915/intel_uc_loader.c b/drivers/gpu/drm/i915/intel_uc_loader.c
> index 986dcf3..bc4b1e5 100644
> --- a/drivers/gpu/drm/i915/intel_uc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_uc_loader.c
> @@ -49,8 +49,16 @@ static void uc_fw_finish(struct drm_device *dev, struct intel_uc_fw *uc_fw)
>  		uc_fw->uc_name, uc_fw->uc_fw_fetch_status, uc_fw->uc_fw_blob);
>  
>  	fw = uc_fw->uc_fw_blob;
> -	if (!fw)
> -		goto fail;
> +	if (!fw) {
> +		/* no firmware found; try again in case FS was not mounted */
> +		DRM_DEBUG_DRIVER("retry fetching %s fw from <%s>\n",
> +			uc_fw->uc_name, uc_fw->uc_fw_path);
> +		if (request_firmware(&fw, uc_fw->uc_fw_path, &dev->pdev->dev))
> +			goto fail;
> +		DRM_DEBUG_DRIVER("fetch %s fw from <%s> succeeded, fw %p\n",
> +			uc_fw->uc_name, uc_fw->uc_fw_path, fw);
> +		uc_fw->uc_fw_blob = fw;
> +	}
>  
>  	if (uc_fw->uc_fw_check && !uc_fw->uc_fw_check(uc_fw))
>  		goto fail;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx