[v3,05/15] drm/i915: Defer default hardware context initialisation until first open

On 17/04/15 22:21, 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.
> 
> 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 | 25 +++++++++++++++++++++--
>  5 files changed, 63 insertions(+), 9 deletions(-)

Hi Alex,

the code is fine :) but I'd rather that this functionality went in ahead
of the GuC loader - it's actually a useful generic standalone that could
provide recovery from other early initialisation issues, even before we
consider GuC submission (there's a reason that EIO was already
considered nonfatal during startup).

The other reason I don't like this being added after the GuC loader is
because it changes the interface to intel_guc_load_ucode() which was
only just added in the patch before. If we swap the order then the
interface is defined the same way from day one, and no-one need consider
the half-baked state inbetween.

So I suggest this patch should be moved to the beginning of the series
(or even submitted independently), and include only the change to
i915_drv.h, the second block of changes to i915_gem.c, and all the
changes to i915_gem_context.c EXCEPT the three lines around the call to
intel_guc_load_ucode() [see attachment for clarification and a new
commit message].

Then the other changes -- which are all GuC-specific --  can be added
back in the *later* patch that adds the GuC loader.

> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 235fc08..d128ac4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1760,6 +1760,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 44154fe..b7bd288 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4888,8 +4888,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 */
> +

Don't include these five lines in this patch

>  	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 e4c57a3..3795df2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -447,23 +447,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);

Don't include these four either
> +
> +	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)

Then fold everything else into the patch(es) that create intel_guc.h and
intel_guc_loader.c ...

I think that will provide a simpler and more logical split between
generic and GuC-specific code :)

.Dave.

[v3,05/15] drm/i915: Defer default hardware context initialisation until first open

Commit Message

Patch