On 17/04/15 22:21, yu.dai@xxxxxxxxx wrote:
> From: Dave Gordon <david.s.gordon@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: Alex Dai <yu.dai@xxxxxxxxx>
> ---
> 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.
>From 160d40ab80d982aff479e3524a0442bb92f78b3d Mon Sep 17 00:00:00 2001
From: Dave Gordon <david.s.gordon@xxxxxxxxx>
Date: Fri, 17 Apr 2015 22:21:10 +0100
Subject: [PATCH 01/15] drm/i915: Defer default hardware context
initialisation until first open
To fully initialise the default contexts in i915_gem_context_enable(),
we have to execute batchbuffer commands on the GPU engines. But that
might not succeed during early initialisation, so we already treat a
return of EIO as nonfatal in gem_init_hw(), allowing the driver to load
despite not (yet) being able to submit batch commands.
This commit adds a flag to record whether the default contexts have
successfully been initialised; if they haven't, then we retry the
call to i915_gem_context_enable() when the device is first opened,
which should be late enough for all required resources to have become
available and any setup helper code to have run.
Issue: VIZ-4884
Signed-off-by: Dave Gordon <david.s.gordon@xxxxxxxxx>
Signed-off-by: Alex Dai <yu.dai@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/i915_gem.c | 4 +++-
drivers/gpu/drm/i915/i915_gem_context.c | 31 ++++++++++++++++++++++++++-----
3 files changed, 30 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 79da7f3..0e88ef2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1751,6 +1751,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 89d9ebe..1fef0bb 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4889,7 +4889,9 @@ i915_gem_init_hw(struct drm_device *dev)
}
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..92baba0 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -447,23 +447,44 @@ 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;
+
+ 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)
--
1.7.9.5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
