On 17/04/15 22:21, yu.dai@intel.com wrote: > From: Dave Gordon > > 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 > Signed-off-by: Alex Dai > --- > 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.