[PATCH 0/5] GuC Scrub vol. 1

[PATCH 0/5] GuC Scrub vol. 1

[Arkadiusz Hiler]

General GuC cleanup simplifying logic, and moving chooks around either for
simplification or cleaner HuC accommodation.

A lot of logic were extracted from intel_guc_load() to other functions - it not
only did handle the actual loading but had WA implementations and submission
enabling code baked it.

This is first the part of effort to clean it up.

Arkadiusz Hiler (5):
  drm/i915/guc: Rename _setup() to _load()
  drm/i915/guc: Introduce intel_uc_init()
  drm/i915/guc: Simplify intel_guc_load()
  drm/i915/guc: Extract param logic form guc_init
  drm/i915/guc: Simplify guc_fw_path

 drivers/gpu/drm/i915/i915_drv.c         |   2 +-
 drivers/gpu/drm/i915/i915_gem.c         |   2 +-
 drivers/gpu/drm/i915/intel_guc_loader.c | 169 +++++---------------------------
 drivers/gpu/drm/i915/intel_uc.c         | 109 ++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uc.h         |  11 ++-
 5 files changed, 148 insertions(+), 145 deletions(-)

-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/5] drm/i915/guc: Rename _setup() to _load()

[Arkadiusz Hiler]

GuC historically has two "startup" functions called _init() and _setup()

Then HuC came with it's _init() and _load().

To make naming more consistent this commit renames intel_guc_setup() to
intel_guc_load() as it it seams more fitting (it's in intel_guc_loader.c
after all).

Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Jeff McGee <jeff.mcgee@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c         |  2 +-
 drivers/gpu/drm/i915/intel_guc_loader.c | 12 ++++++------
 drivers/gpu/drm/i915/intel_uc.h         |  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f86a71d9..6af4e85 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4412,7 +4412,7 @@ i915_gem_init_hw(struct drm_i915_private *dev_priv)
 	intel_mocs_init_l3cc_table(dev_priv);
 
 	/* We can't enable contexts until all firmware is loaded */
-	ret = intel_guc_setup(dev_priv);
+	ret = intel_guc_load(dev_priv);
 	if (ret)
 		goto out;
 
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 21db697..f8b28b1 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -436,19 +436,19 @@ static int guc_hw_reset(struct drm_i915_private *dev_priv)
 }
 
 /**
- * intel_guc_setup() - finish preparing the GuC for activity
+ * intel_guc_load() - finish preparing the GuC for activity
  * @dev_priv:	i915 device private
  *
- * Called from gem_init_hw() during driver loading and also after a GPU reset.
+ * Called during driver loading and also after a GPU reset.
  *
  * The main action required here it to load the GuC uCode into the device.
  * The firmware image should have already been fetched into memory by the
- * earlier call to intel_guc_init(), so here we need only check that worked,
- * and then transfer the image to the h/w.
+ * earlier call to intel_guc_init(), so here we need only check that
+ * worked, and then transfer the image to the h/w.
  *
  * Return:	non-zero code on error
  */
-int intel_guc_setup(struct drm_i915_private *dev_priv)
+int intel_guc_load(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	const char *fw_path = guc_fw->guc_fw_path;
@@ -717,7 +717,7 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
  * Called early during driver load, but after GEM is initialised.
  *
  * The firmware will be transferred to the GuC's memory later,
- * when intel_guc_setup() is called.
+ * when intel_guc_load() is called.
  */
 void intel_guc_init(struct drm_i915_private *dev_priv)
 {
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 11f5608..7222e6c 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -179,7 +179,7 @@ int intel_guc_log_control(struct intel_guc *guc, u32 control_val);
 
 /* intel_guc_loader.c */
 extern void intel_guc_init(struct drm_i915_private *dev_priv);
-extern int intel_guc_setup(struct drm_i915_private *dev_priv);
+extern int intel_guc_load(struct drm_i915_private *dev_priv);
 extern void intel_guc_fini(struct drm_i915_private *dev_priv);
 extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status);
 extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/guc: Rename _setup() to _load()

[Chris Wilson]

On Thu, Dec 15, 2016 at 04:47:04PM +0100, Arkadiusz Hiler wrote:
> GuC historically has two "startup" functions called _init() and _setup()
> 
> Then HuC came with it's _init() and _load().
> 
> To make naming more consistent this commit renames intel_guc_setup() to
> intel_guc_load() as it it seams more fitting (it's in intel_guc_loader.c
> after all).

Or init_hw as that is the initialisation phase it is called from. 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/guc: Rename _setup() to _load()

[Arkadiusz Hiler]

On Thu, Dec 15, 2016 at 03:57:01PM +0000, Chris Wilson wrote:
> On Thu, Dec 15, 2016 at 04:47:04PM +0100, Arkadiusz Hiler wrote:
> > GuC historically has two "startup" functions called _init() and _setup()
> > 
> > Then HuC came with it's _init() and _load().
> > 
> > To make naming more consistent this commit renames intel_guc_setup() to
> > intel_guc_load() as it it seams more fitting (it's in intel_guc_loader.c
> > after all).
> 
> Or init_hw as that is the initialisation phase it is called from. 
> -Chris

Since it's intel_guc_loader.c I somehow prefer _load() here.

But intel_uc_load() which, is introduced with the series and call
intel_guc_load() can be renamed to intel_uc_init_hw()

What do you think?


-- 
Cheers,
Arek
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/guc: Rename _setup() to _load()

[Chris Wilson]

On Fri, Dec 16, 2016 at 12:47:26PM +0100, Arkadiusz Hiler wrote:
> On Thu, Dec 15, 2016 at 03:57:01PM +0000, Chris Wilson wrote:
> > On Thu, Dec 15, 2016 at 04:47:04PM +0100, Arkadiusz Hiler wrote:
> > > GuC historically has two "startup" functions called _init() and _setup()
> > > 
> > > Then HuC came with it's _init() and _load().
> > > 
> > > To make naming more consistent this commit renames intel_guc_setup() to
> > > intel_guc_load() as it it seams more fitting (it's in intel_guc_loader.c
> > > after all).
> > 
> > Or init_hw as that is the initialisation phase it is called from. 
> > -Chris
> 
> Since it's intel_guc_loader.c I somehow prefer _load() here.
> 
> But intel_uc_load() which, is introduced with the series and call
> intel_guc_load() can be renamed to intel_uc_init_hw()
> 
> What do you think?

We want to push the "phase verb" as far as it makes sense, especially
along the chain i.e. driver -> subsystem -> subsubsystem -> ...

Once we are in the handler, it should use the right functions named
appropriate. I still think carrying the phase verb as far as possible is
important (more important say for init_early) as that carries the
information about the rest of the driver state and the limitations we
must keep in mind. Good taste should prevail ofc; the actual work must
be done by sensibly named functions.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/guc: Rename _setup() to _load()

[Michal Wajdeczko]

On Thu, Dec 15, 2016 at 04:47:04PM +0100, Arkadiusz Hiler wrote:
> GuC historically has two "startup" functions called _init() and _setup()
> 
> Then HuC came with it's _init() and _load().
> 
> To make naming more consistent this commit renames intel_guc_setup() to
> intel_guc_load() as it it seams more fitting (it's in intel_guc_loader.c
> after all).
> 
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Jeff McGee <jeff.mcgee@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c         |  2 +-
>  drivers/gpu/drm/i915/intel_guc_loader.c | 12 ++++++------
>  drivers/gpu/drm/i915/intel_uc.h         |  2 +-
>  3 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index f86a71d9..6af4e85 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4412,7 +4412,7 @@ i915_gem_init_hw(struct drm_i915_private *dev_priv)
>  	intel_mocs_init_l3cc_table(dev_priv);
>  
>  	/* We can't enable contexts until all firmware is loaded */
> -	ret = intel_guc_setup(dev_priv);
> +	ret = intel_guc_load(dev_priv);
>  	if (ret)
>  		goto out;
>  
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 21db697..f8b28b1 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -436,19 +436,19 @@ static int guc_hw_reset(struct drm_i915_private *dev_priv)
>  }
>  
>  /**
> - * intel_guc_setup() - finish preparing the GuC for activity
> + * intel_guc_load() - finish preparing the GuC for activity
>   * @dev_priv:	i915 device private
>   *
> - * Called from gem_init_hw() during driver loading and also after a GPU reset.
> + * Called during driver loading and also after a GPU reset.
>   *
>   * The main action required here it to load the GuC uCode into the device.
>   * The firmware image should have already been fetched into memory by the
> - * earlier call to intel_guc_init(), so here we need only check that worked,
> - * and then transfer the image to the h/w.
> + * earlier call to intel_guc_init(), so here we need only check that
> + * worked, and then transfer the image to the h/w.
>   *
>   * Return:	non-zero code on error
>   */
> -int intel_guc_setup(struct drm_i915_private *dev_priv)
> +int intel_guc_load(struct drm_i915_private *dev_priv)

Can we use this refactor effort to fix also inconsistency in params
passed to functions that start with "intel_guc_" prefix ?
Some require dev_priv, while other expect pointer to intel_guc struct.
My preferrence would be latter syntax with *guc.

Note that if required we can easily get dev_priv using guc_to_i915().

Other option, use "i915_guc" prefix and then pass dev_priv to distinguish
between these two forms.

Michal

>  {
>  	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>  	const char *fw_path = guc_fw->guc_fw_path;
> @@ -717,7 +717,7 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
>   * Called early during driver load, but after GEM is initialised.
>   *
>   * The firmware will be transferred to the GuC's memory later,
> - * when intel_guc_setup() is called.
> + * when intel_guc_load() is called.
>   */
>  void intel_guc_init(struct drm_i915_private *dev_priv)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 11f5608..7222e6c 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -179,7 +179,7 @@ int intel_guc_log_control(struct intel_guc *guc, u32 control_val);
>  
>  /* intel_guc_loader.c */
>  extern void intel_guc_init(struct drm_i915_private *dev_priv);
> -extern int intel_guc_setup(struct drm_i915_private *dev_priv);
> +extern int intel_guc_load(struct drm_i915_private *dev_priv);
>  extern void intel_guc_fini(struct drm_i915_private *dev_priv);
>  extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status);
>  extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
> -- 
> 2.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/guc: Rename _setup() to _load()

[Arkadiusz Hiler]

On Thu, Dec 15, 2016 at 05:22:53PM +0100, Michal Wajdeczko wrote:
> On Thu, Dec 15, 2016 at 04:47:04PM +0100, Arkadiusz Hiler wrote:
> > GuC historically has two "startup" functions called _init() and _setup()
> > 
> > Then HuC came with it's _init() and _load().
> > 
> > To make naming more consistent this commit renames intel_guc_setup() to
> > intel_guc_load() as it it seams more fitting (it's in intel_guc_loader.c
> > after all).
> > 
> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Cc: Jeff McGee <jeff.mcgee@intel.com>
> > Cc: Michal Winiarski <michal.winiarski@intel.com>
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c         |  2 +-
> >  drivers/gpu/drm/i915/intel_guc_loader.c | 12 ++++++------
> >  drivers/gpu/drm/i915/intel_uc.h         |  2 +-
> >  3 files changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index f86a71d9..6af4e85 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4412,7 +4412,7 @@ i915_gem_init_hw(struct drm_i915_private *dev_priv)
> >  	intel_mocs_init_l3cc_table(dev_priv);
> >  
> >  	/* We can't enable contexts until all firmware is loaded */
> > -	ret = intel_guc_setup(dev_priv);
> > +	ret = intel_guc_load(dev_priv);
> >  	if (ret)
> >  		goto out;
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> > index 21db697..f8b28b1 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> > @@ -436,19 +436,19 @@ static int guc_hw_reset(struct drm_i915_private *dev_priv)
> >  }
> >  
> >  /**
> > - * intel_guc_setup() - finish preparing the GuC for activity
> > + * intel_guc_load() - finish preparing the GuC for activity
> >   * @dev_priv:	i915 device private
> >   *
> > - * Called from gem_init_hw() during driver loading and also after a GPU reset.
> > + * Called during driver loading and also after a GPU reset.
> >   *
> >   * The main action required here it to load the GuC uCode into the device.
> >   * The firmware image should have already been fetched into memory by the
> > - * earlier call to intel_guc_init(), so here we need only check that worked,
> > - * and then transfer the image to the h/w.
> > + * earlier call to intel_guc_init(), so here we need only check that
> > + * worked, and then transfer the image to the h/w.
> >   *
> >   * Return:	non-zero code on error
> >   */
> > -int intel_guc_setup(struct drm_i915_private *dev_priv)
> > +int intel_guc_load(struct drm_i915_private *dev_priv)
> 
> Can we use this refactor effort to fix also inconsistency in params
> passed to functions that start with "intel_guc_" prefix ?
> Some require dev_priv, while other expect pointer to intel_guc struct.
> My preferrence would be latter syntax with *guc.
> 
> Note that if required we can easily get dev_priv using guc_to_i915().
> 
> Other option, use "i915_guc" prefix and then pass dev_priv to distinguish
> between these two forms.
> 

Good idea. I'll include it in vol. 2.

> Michal
> 
> >  {
> >  	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> >  	const char *fw_path = guc_fw->guc_fw_path;
> > @@ -717,7 +717,7 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> >   * Called early during driver load, but after GEM is initialised.
> >   *
> >   * The firmware will be transferred to the GuC's memory later,
> > - * when intel_guc_setup() is called.
> > + * when intel_guc_load() is called.
> >   */
> >  void intel_guc_init(struct drm_i915_private *dev_priv)
> >  {
> > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> > index 11f5608..7222e6c 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.h
> > +++ b/drivers/gpu/drm/i915/intel_uc.h
> > @@ -179,7 +179,7 @@ int intel_guc_log_control(struct intel_guc *guc, u32 control_val);
> >  
> >  /* intel_guc_loader.c */
> >  extern void intel_guc_init(struct drm_i915_private *dev_priv);
> > -extern int intel_guc_setup(struct drm_i915_private *dev_priv);
> > +extern int intel_guc_load(struct drm_i915_private *dev_priv);
> >  extern void intel_guc_fini(struct drm_i915_private *dev_priv);
> >  extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status);
> >  extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
> > -- 
> > 2.9.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Cheers,
Arek
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/5] drm/i915/guc: Introduce intel_uc_init()

[Arkadiusz Hiler]

We will be able to bulk call all firmware _init() function from single
point and offset some general logic there.

Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Jeff McGee <jeff.mcgee@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 2 +-
 drivers/gpu/drm/i915/intel_uc.c | 5 +++++
 drivers/gpu/drm/i915/intel_uc.h | 1 +
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6428588..55a5546c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -600,7 +600,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (ret)
 		goto cleanup_irq;
 
-	intel_guc_init(dev_priv);
+	intel_uc_init(dev_priv);
 
 	ret = i915_gem_init(dev_priv);
 	if (ret)
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 8ae6795..8eec035 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -30,6 +30,11 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
 	mutex_init(&dev_priv->guc.send_mutex);
 }
 
+void intel_uc_init(struct drm_i915_private *dev_priv)
+{
+	intel_guc_init(dev_priv);
+}
+
 /*
  * Read GuC command/status register (SOFT_SCRATCH_0)
  * Return true if it contains a response rather than a command
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 7222e6c..ec1a5b2 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -170,6 +170,7 @@ struct intel_guc {
 
 /* intel_uc.c */
 void intel_uc_init_early(struct drm_i915_private *dev_priv);
+void intel_uc_init(struct drm_i915_private *dev_priv);
 bool intel_guc_recv(struct drm_i915_private *dev_priv, u32 *status);
 int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
 int intel_guc_sample_forcewake(struct intel_guc *guc);
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915/guc: Introduce intel_uc_init()

[Srivatsa, Anusha]

>-----Original Message-----
>From: Hiler, Arkadiusz
>Sent: Thursday, December 15, 2016 7:47 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; Mcgee, Jeff
><jeff.mcgee@intel.com>; Winiarski, Michal <michal.winiarski@intel.com>
>Subject: [PATCH 2/5] drm/i915/guc: Introduce intel_uc_init()
>
>We will be able to bulk call all firmware _init() function from single point and
>offset some general logic there.

Following this logic shouldn't we call intel_huc_init() as well?
Anusha

>Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>Cc: Jeff McGee <jeff.mcgee@intel.com>
>Cc: Michal Winiarski <michal.winiarski@intel.com>
>Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
>---
> drivers/gpu/drm/i915/i915_drv.c | 2 +-
> drivers/gpu/drm/i915/intel_uc.c | 5 +++++  drivers/gpu/drm/i915/intel_uc.h | 1 +
> 3 files changed, 7 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>index 6428588..55a5546c 100644
>--- a/drivers/gpu/drm/i915/i915_drv.c
>+++ b/drivers/gpu/drm/i915/i915_drv.c
>@@ -600,7 +600,7 @@ static int i915_load_modeset_init(struct drm_device
>*dev)
> 	if (ret)
> 		goto cleanup_irq;
>
>-	intel_guc_init(dev_priv);
>+	intel_uc_init(dev_priv);
>
> 	ret = i915_gem_init(dev_priv);
> 	if (ret)
>diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
>index 8ae6795..8eec035 100644
>--- a/drivers/gpu/drm/i915/intel_uc.c
>+++ b/drivers/gpu/drm/i915/intel_uc.c
>@@ -30,6 +30,11 @@ void intel_uc_init_early(struct drm_i915_private
>*dev_priv)
> 	mutex_init(&dev_priv->guc.send_mutex);
> }
>
>+void intel_uc_init(struct drm_i915_private *dev_priv) {
>+	intel_guc_init(dev_priv);
>+}
>+
> /*
>  * Read GuC command/status register (SOFT_SCRATCH_0)
>  * Return true if it contains a response rather than a command diff --git
>a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h index
>7222e6c..ec1a5b2 100644
>--- a/drivers/gpu/drm/i915/intel_uc.h
>+++ b/drivers/gpu/drm/i915/intel_uc.h
>@@ -170,6 +170,7 @@ struct intel_guc {
>
> /* intel_uc.c */
> void intel_uc_init_early(struct drm_i915_private *dev_priv);
>+void intel_uc_init(struct drm_i915_private *dev_priv);
> bool intel_guc_recv(struct drm_i915_private *dev_priv, u32 *status);  int
>intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);  int
>intel_guc_sample_forcewake(struct intel_guc *guc);
>--
>2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915/guc: Introduce intel_uc_init()

[Arkadiusz Hiler]

On Tue, Dec 20, 2016 at 11:00:09PM +0100, Srivatsa, Anusha wrote:
> 
> 
> >-----Original Message-----
> >From: Hiler, Arkadiusz
> >Sent: Thursday, December 15, 2016 7:47 AM
> >To: intel-gfx@lists.freedesktop.org
> >Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; Mcgee, Jeff
> ><jeff.mcgee@intel.com>; Winiarski, Michal <michal.winiarski@intel.com>
> >Subject: [PATCH 2/5] drm/i915/guc: Introduce intel_uc_init()
> >
> >We will be able to bulk call all firmware _init() function from single point and
> >offset some general logic there.
> 
> Following this logic shouldn't we call intel_huc_init() as well?

We will, once your patches will made into upstream. There is no sense
calling nonexisting functions yet.

HuC is partically the reason for introducing common init.

> >Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> >Cc: Jeff McGee <jeff.mcgee@intel.com>
> >Cc: Michal Winiarski <michal.winiarski@intel.com>
> >Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> >---
> > drivers/gpu/drm/i915/i915_drv.c | 2 +-
> > drivers/gpu/drm/i915/intel_uc.c | 5 +++++  drivers/gpu/drm/i915/intel_uc.h | 1 +
> > 3 files changed, 7 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >index 6428588..55a5546c 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.c
> >+++ b/drivers/gpu/drm/i915/i915_drv.c
> >@@ -600,7 +600,7 @@ static int i915_load_modeset_init(struct drm_device
> >*dev)
> > 	if (ret)
> > 		goto cleanup_irq;
> >
> >-	intel_guc_init(dev_priv);
> >+	intel_uc_init(dev_priv);
> >
> > 	ret = i915_gem_init(dev_priv);
> > 	if (ret)
> >diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> >index 8ae6795..8eec035 100644
> >--- a/drivers/gpu/drm/i915/intel_uc.c
> >+++ b/drivers/gpu/drm/i915/intel_uc.c
> >@@ -30,6 +30,11 @@ void intel_uc_init_early(struct drm_i915_private
> >*dev_priv)
> > 	mutex_init(&dev_priv->guc.send_mutex);
> > }
> >
> >+void intel_uc_init(struct drm_i915_private *dev_priv) {
> >+	intel_guc_init(dev_priv);
> >+}
> >+
> > /*
> >  * Read GuC command/status register (SOFT_SCRATCH_0)
> >  * Return true if it contains a response rather than a command diff --git
> >a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h index
> >7222e6c..ec1a5b2 100644
> >--- a/drivers/gpu/drm/i915/intel_uc.h
> >+++ b/drivers/gpu/drm/i915/intel_uc.h
> >@@ -170,6 +170,7 @@ struct intel_guc {
> >
> > /* intel_uc.c */
> > void intel_uc_init_early(struct drm_i915_private *dev_priv);
> >+void intel_uc_init(struct drm_i915_private *dev_priv);
> > bool intel_guc_recv(struct drm_i915_private *dev_priv, u32 *status);  int
> >intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);  int
> >intel_guc_sample_forcewake(struct intel_guc *guc);
> >--
> >2.9.3
> 

-- 
Cheers,
Arek
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/5] drm/i915/guc: Simplify intel_guc_load()

[Arkadiusz Hiler]

Current version of intel_guc_load() does a lot:
 - cares about submission
 - loads huc
 - implement WA

This change offloads some of the logic to intel_uc_load(), which now
cares about the above.

Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Jeff McGee <jeff.mcgee@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c         |   2 +-
 drivers/gpu/drm/i915/intel_guc_loader.c | 126 +++++---------------------------
 drivers/gpu/drm/i915/intel_uc.c         |  83 +++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uc.h         |   8 ++
 4 files changed, 110 insertions(+), 109 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6af4e85..76b52c6 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4412,7 +4412,7 @@ i915_gem_init_hw(struct drm_i915_private *dev_priv)
 	intel_mocs_init_l3cc_table(dev_priv);
 
 	/* We can't enable contexts until all firmware is loaded */
-	ret = intel_guc_load(dev_priv);
+	ret = intel_uc_load(dev_priv);
 	if (ret)
 		goto out;
 
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index f8b28b1..b76b556 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -97,7 +97,7 @@ const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status)
 	}
 };
 
-static void guc_interrupts_release(struct drm_i915_private *dev_priv)
+void guc_interrupts_release(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
@@ -115,7 +115,7 @@ static void guc_interrupts_release(struct drm_i915_private *dev_priv)
 	I915_WRITE(GUC_WD_VECS_IER, 0);
 }
 
-static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
+void guc_interrupts_capture(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
@@ -334,7 +334,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
 	return ret;
 }
 
-static u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
+u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
 {
 	u32 wopcm_size = GUC_WOPCM_TOP;
 
@@ -417,7 +417,7 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-static int guc_hw_reset(struct drm_i915_private *dev_priv)
+int guc_hw_reset(struct drm_i915_private *dev_priv)
 {
 	int ret;
 	u32 guc_status;
@@ -452,75 +452,37 @@ int intel_guc_load(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	const char *fw_path = guc_fw->guc_fw_path;
-	int retries, ret, err;
+	int ret;
 
 	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
 		fw_path,
 		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
 		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
 
-	/* Loading forbidden, or no firmware to load? */
-	if (!i915.enable_guc_loading) {
-		err = 0;
-		goto fail;
-	} else if (fw_path == NULL) {
+	if (fw_path == NULL) {
 		/* Device is known to have no uCode (e.g. no GuC) */
-		err = -ENXIO;
-		goto fail;
+		return -ENXIO;
 	} else if (*fw_path == '\0') {
 		/* Device has a GuC but we don't know what f/w to load? */
 		WARN(1, "No GuC firmware known for this platform!\n");
-		err = -ENODEV;
-		goto fail;
+		return -ENODEV;
 	}
 
 	/* Fetch failed, or already fetched but failed to load? */
 	if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) {
-		err = -EIO;
-		goto fail;
+		return -EIO;
 	} else if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) {
-		err = -ENOEXEC;
-		goto fail;
+		return -ENOEXEC;
 	}
 
-	guc_interrupts_release(dev_priv);
-	gen9_reset_guc_interrupts(dev_priv);
-
 	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
 
-	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
-		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
-		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
-
-	err = i915_guc_submission_init(dev_priv);
-	if (err)
-		goto fail;
-
-	/*
-	 * WaEnableuKernelHeaderValidFix:skl,bxt
-	 * For BXT, this is only upto B0 but below WA is required for later
-	 * steppings also so this is extended as well.
-	 */
 	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
-	for (retries = 3; ; ) {
-		/*
-		 * Always reset the GuC just before (re)loading, so
-		 * that the state and timing are fairly predictable
-		 */
-		err = guc_hw_reset(dev_priv);
-		if (err)
-			goto fail;
+	/* we may want to retry guc ucode transfer */
+	ret = guc_ucode_xfer(dev_priv);
 
-		err = guc_ucode_xfer(dev_priv);
-		if (!err)
-			break;
-
-		if (--retries == 0)
-			goto fail;
-
-		DRM_INFO("GuC fw load failed: %d; will reset and "
-			 "retry %d more time(s)\n", err, retries);
-	}
+	if (ret)
+		return -EAGAIN;
 
 	guc_fw->guc_fw_load_status = GUC_FIRMWARE_SUCCESS;
 
@@ -528,63 +490,7 @@ int intel_guc_load(struct drm_i915_private *dev_priv)
 		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
 		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
 
-	if (i915.enable_guc_submission) {
-		if (i915.guc_log_level >= 0)
-			gen9_enable_guc_interrupts(dev_priv);
-
-		err = i915_guc_submission_enable(dev_priv);
-		if (err)
-			goto fail;
-		guc_interrupts_capture(dev_priv);
-	}
-
 	return 0;
-
-fail:
-	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
-		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;
-
-	guc_interrupts_release(dev_priv);
-	i915_guc_submission_disable(dev_priv);
-	i915_guc_submission_fini(dev_priv);
-
-	/*
-	 * We've failed to load the firmware :(
-	 *
-	 * Decide whether to disable GuC submission and fall back to
-	 * execlist mode, and whether to hide the error by returning
-	 * zero or to return -EIO, which the caller will treat as a
-	 * nonfatal error (i.e. it doesn't prevent driver load, but
-	 * marks the GPU as wedged until reset).
-	 */
-	if (i915.enable_guc_loading > 1) {
-		ret = -EIO;
-	} else if (i915.enable_guc_submission > 1) {
-		ret = -EIO;
-	} else {
-		ret = 0;
-	}
-
-	if (err == 0 && !HAS_GUC_UCODE(dev_priv))
-		;	/* Don't mention the GuC! */
-	else if (err == 0)
-		DRM_INFO("GuC firmware load skipped\n");
-	else if (ret != -EIO)
-		DRM_NOTE("GuC firmware load failed: %d\n", err);
-	else
-		DRM_WARN("GuC firmware load failed: %d\n", err);
-
-	if (i915.enable_guc_submission) {
-		if (fw_path == NULL)
-			DRM_INFO("GuC submission without firmware not supported\n");
-		if (ret == 0)
-			DRM_NOTE("Falling back from GuC submission to execlist mode\n");
-		else
-			DRM_ERROR("GuC init failed: %d\n", ret);
-	}
-	i915.enable_guc_submission = 0;
-
-	return ret;
 }
 
 static void guc_fw_fetch(struct drm_i915_private *dev_priv,
@@ -757,6 +663,10 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
 	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
 	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
 
+	/* can't enable guc submission without guc */
+	if (!i915.enable_guc_loading)
+		i915.enable_guc_submission = 0;
+
 	/* Early (and silent) return if GuC loading is disabled */
 	if (!i915.enable_guc_loading)
 		return;
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 8eec035..4e184edb 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -35,6 +35,89 @@ void intel_uc_init(struct drm_i915_private *dev_priv)
 	intel_guc_init(dev_priv);
 }
 
+int intel_uc_load(struct drm_i915_private *dev_priv)
+{
+	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
+	int ret, retries;
+
+	/* guc not enabled, nothing to do */
+	if (!i915.enable_guc_loading)
+		return 0;
+
+	guc_interrupts_release(dev_priv);
+	gen9_reset_guc_interrupts(dev_priv);
+
+	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
+
+	if (i915.enable_guc_submission) {
+		ret = i915_guc_submission_init(dev_priv);
+		if (ret)
+			goto fail;
+	}
+
+	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
+	retries = GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS;
+	while (retries--) {
+		/*
+		 * Always reset the GuC just before (re)loading, so
+		 * that the state and timing are fairly predictable
+		 */
+		ret = guc_hw_reset(dev_priv);
+		if (ret)
+			goto fail;
+
+		ret = intel_guc_load(dev_priv);
+		if (ret == 0 || ret != -EAGAIN)
+			break;
+
+		DRM_INFO("GuC fw load failed: %d; will reset and "
+			 "retry %d more time(s)\n", ret, retries);
+	}
+
+	/* did we succeded or run out of retries? */
+	if (ret)
+		goto fail;
+
+	if (i915.enable_guc_submission) {
+		if (i915.guc_log_level >= 0)
+			gen9_enable_guc_interrupts(dev_priv);
+
+		ret = i915_guc_submission_enable(dev_priv);
+		if (ret)
+			goto fail;
+		guc_interrupts_capture(dev_priv);
+	}
+
+	return 0;
+
+fail:
+	/*
+	 * We've failed to load the firmware :(
+	 *
+	 * Decide whether to disable GuC submission and fall back to
+	 * execlist mode, and whether to hide the error by returning
+	 * zero or to return -EIO, which the caller will treat as a
+	 * nonfatal error (i.e. it doesn't prevent driver load, but
+	 * marks the GPU as wedged until reset).
+	 */
+	if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1)
+		ret = -EIO;
+	else
+		ret = 0;
+
+	if (i915.enable_guc_submission) {
+		i915.enable_guc_submission = 0;
+		DRM_INFO("GuC submission without firmware not supported\n");
+		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
+	}
+
+	guc_interrupts_release(dev_priv);
+	i915_guc_submission_disable(dev_priv);
+	i915_guc_submission_fini(dev_priv);
+
+	return ret;
+}
+
 /*
  * Read GuC command/status register (SOFT_SCRATCH_0)
  * Return true if it contains a response rather than a command
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index ec1a5b2..89c7e1f 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -28,6 +28,9 @@
 #include "i915_guc_reg.h"
 #include "intel_ringbuffer.h"
 
+/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
+#define GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS 3
+
 struct drm_i915_gem_request;
 
 /*
@@ -171,6 +174,7 @@ struct intel_guc {
 /* intel_uc.c */
 void intel_uc_init_early(struct drm_i915_private *dev_priv);
 void intel_uc_init(struct drm_i915_private *dev_priv);
+int intel_uc_load(struct drm_i915_private *dev_priv);
 bool intel_guc_recv(struct drm_i915_private *dev_priv, u32 *status);
 int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
 int intel_guc_sample_forcewake(struct intel_guc *guc);
@@ -185,6 +189,10 @@ extern void intel_guc_fini(struct drm_i915_private *dev_priv);
 extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status);
 extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
 extern int intel_guc_resume(struct drm_i915_private *dev_priv);
+void guc_interrupts_release(struct drm_i915_private *dev_priv);
+void guc_interrupts_capture(struct drm_i915_private *dev_priv);
+int guc_hw_reset(struct drm_i915_private *dev_priv);
+u32 guc_wopcm_size(struct drm_i915_private *dev_priv);
 
 /* i915_guc_submission.c */
 int i915_guc_submission_init(struct drm_i915_private *dev_priv);
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915/guc: Simplify intel_guc_load()

[Michal Wajdeczko]

On Thu, Dec 15, 2016 at 04:47:06PM +0100, Arkadiusz Hiler wrote:
> Current version of intel_guc_load() does a lot:
>  - cares about submission
>  - loads huc
>  - implement WA
> 
> This change offloads some of the logic to intel_uc_load(), which now
> cares about the above.
> 
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Jeff McGee <jeff.mcgee@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c         |   2 +-
>  drivers/gpu/drm/i915/intel_guc_loader.c | 126 +++++---------------------------
>  drivers/gpu/drm/i915/intel_uc.c         |  83 +++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_uc.h         |   8 ++
>  4 files changed, 110 insertions(+), 109 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6af4e85..76b52c6 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4412,7 +4412,7 @@ i915_gem_init_hw(struct drm_i915_private *dev_priv)
>  	intel_mocs_init_l3cc_table(dev_priv);
>  
>  	/* We can't enable contexts until all firmware is loaded */
> -	ret = intel_guc_load(dev_priv);
> +	ret = intel_uc_load(dev_priv);
>  	if (ret)
>  		goto out;
>  
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index f8b28b1..b76b556 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -97,7 +97,7 @@ const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status)
>  	}
>  };
>  
> -static void guc_interrupts_release(struct drm_i915_private *dev_priv)
> +void guc_interrupts_release(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_engine_cs *engine;
>  	enum intel_engine_id id;
> @@ -115,7 +115,7 @@ static void guc_interrupts_release(struct drm_i915_private *dev_priv)
>  	I915_WRITE(GUC_WD_VECS_IER, 0);
>  }
>  
> -static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
> +void guc_interrupts_capture(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_engine_cs *engine;
>  	enum intel_engine_id id;
> @@ -334,7 +334,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
>  	return ret;
>  }
>  
> -static u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
> +u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
>  {
>  	u32 wopcm_size = GUC_WOPCM_TOP;
>  
> @@ -417,7 +417,7 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
>  	return ret;
>  }
>  
> -static int guc_hw_reset(struct drm_i915_private *dev_priv)
> +int guc_hw_reset(struct drm_i915_private *dev_priv)
>  {
>  	int ret;
>  	u32 guc_status;
> @@ -452,75 +452,37 @@ int intel_guc_load(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>  	const char *fw_path = guc_fw->guc_fw_path;
> -	int retries, ret, err;
> +	int ret;
>  
>  	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
>  		fw_path,
>  		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
>  		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
>  
> -	/* Loading forbidden, or no firmware to load? */
> -	if (!i915.enable_guc_loading) {
> -		err = 0;
> -		goto fail;
> -	} else if (fw_path == NULL) {
> +	if (fw_path == NULL) {
>  		/* Device is known to have no uCode (e.g. no GuC) */
> -		err = -ENXIO;
> -		goto fail;
> +		return -ENXIO;
>  	} else if (*fw_path == '\0') {
>  		/* Device has a GuC but we don't know what f/w to load? */
>  		WARN(1, "No GuC firmware known for this platform!\n");
> -		err = -ENODEV;
> -		goto fail;
> +		return -ENODEV;
>  	}
>  
>  	/* Fetch failed, or already fetched but failed to load? */
>  	if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) {
> -		err = -EIO;
> -		goto fail;
> +		return -EIO;
>  	} else if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) {
> -		err = -ENOEXEC;
> -		goto fail;
> +		return -ENOEXEC;
>  	}
>  
> -	guc_interrupts_release(dev_priv);
> -	gen9_reset_guc_interrupts(dev_priv);
> -
>  	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
>  
> -	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> -
> -	err = i915_guc_submission_init(dev_priv);
> -	if (err)
> -		goto fail;
> -
> -	/*
> -	 * WaEnableuKernelHeaderValidFix:skl,bxt
> -	 * For BXT, this is only upto B0 but below WA is required for later
> -	 * steppings also so this is extended as well.
> -	 */
>  	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> -	for (retries = 3; ; ) {
> -		/*
> -		 * Always reset the GuC just before (re)loading, so
> -		 * that the state and timing are fairly predictable
> -		 */
> -		err = guc_hw_reset(dev_priv);
> -		if (err)
> -			goto fail;
> +	/* we may want to retry guc ucode transfer */
> +	ret = guc_ucode_xfer(dev_priv);
>  
> -		err = guc_ucode_xfer(dev_priv);
> -		if (!err)
> -			break;
> -
> -		if (--retries == 0)
> -			goto fail;
> -
> -		DRM_INFO("GuC fw load failed: %d; will reset and "
> -			 "retry %d more time(s)\n", err, retries);
> -	}
> +	if (ret)
> +		return -EAGAIN;
>  
>  	guc_fw->guc_fw_load_status = GUC_FIRMWARE_SUCCESS;
>  
> @@ -528,63 +490,7 @@ int intel_guc_load(struct drm_i915_private *dev_priv)
>  		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
>  		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
>  
> -	if (i915.enable_guc_submission) {
> -		if (i915.guc_log_level >= 0)
> -			gen9_enable_guc_interrupts(dev_priv);
> -
> -		err = i915_guc_submission_enable(dev_priv);
> -		if (err)
> -			goto fail;
> -		guc_interrupts_capture(dev_priv);
> -	}
> -
>  	return 0;
> -
> -fail:
> -	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
> -		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;
> -
> -	guc_interrupts_release(dev_priv);
> -	i915_guc_submission_disable(dev_priv);
> -	i915_guc_submission_fini(dev_priv);
> -
> -	/*
> -	 * We've failed to load the firmware :(
> -	 *
> -	 * Decide whether to disable GuC submission and fall back to
> -	 * execlist mode, and whether to hide the error by returning
> -	 * zero or to return -EIO, which the caller will treat as a
> -	 * nonfatal error (i.e. it doesn't prevent driver load, but
> -	 * marks the GPU as wedged until reset).
> -	 */
> -	if (i915.enable_guc_loading > 1) {
> -		ret = -EIO;
> -	} else if (i915.enable_guc_submission > 1) {
> -		ret = -EIO;
> -	} else {
> -		ret = 0;
> -	}
> -
> -	if (err == 0 && !HAS_GUC_UCODE(dev_priv))
> -		;	/* Don't mention the GuC! */
> -	else if (err == 0)
> -		DRM_INFO("GuC firmware load skipped\n");
> -	else if (ret != -EIO)
> -		DRM_NOTE("GuC firmware load failed: %d\n", err);
> -	else
> -		DRM_WARN("GuC firmware load failed: %d\n", err);
> -
> -	if (i915.enable_guc_submission) {
> -		if (fw_path == NULL)
> -			DRM_INFO("GuC submission without firmware not supported\n");
> -		if (ret == 0)
> -			DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> -		else
> -			DRM_ERROR("GuC init failed: %d\n", ret);
> -	}
> -	i915.enable_guc_submission = 0;
> -
> -	return ret;
>  }
>  
>  static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> @@ -757,6 +663,10 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
>  	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
>  	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
>  
> +	/* can't enable guc submission without guc */
> +	if (!i915.enable_guc_loading)
> +		i915.enable_guc_submission = 0;
> +
>  	/* Early (and silent) return if GuC loading is disabled */
>  	if (!i915.enable_guc_loading)
>  		return;
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 8eec035..4e184edb 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -35,6 +35,89 @@ void intel_uc_init(struct drm_i915_private *dev_priv)
>  	intel_guc_init(dev_priv);
>  }
>  
> +int intel_uc_load(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> +	int ret, retries;
> +
> +	/* guc not enabled, nothing to do */
> +	if (!i915.enable_guc_loading)
> +		return 0;
> +
> +	guc_interrupts_release(dev_priv);
> +	gen9_reset_guc_interrupts(dev_priv);
> +
> +	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
> +
> +	if (i915.enable_guc_submission) {
> +		ret = i915_guc_submission_init(dev_priv);
> +		if (ret)
> +			goto fail;
> +	}
> +
> +	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> +	retries = GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS;
> +	while (retries--) {
> +		/*
> +		 * Always reset the GuC just before (re)loading, so
> +		 * that the state and timing are fairly predictable
> +		 */
> +		ret = guc_hw_reset(dev_priv);
> +		if (ret)
> +			goto fail;
> +
> +		ret = intel_guc_load(dev_priv);
> +		if (ret == 0 || ret != -EAGAIN)
> +			break;
> +
> +		DRM_INFO("GuC fw load failed: %d; will reset and "
> +			 "retry %d more time(s)\n", ret, retries);
> +	}
> +
> +	/* did we succeded or run out of retries? */
> +	if (ret)
> +		goto fail;
> +
> +	if (i915.enable_guc_submission) {
> +		if (i915.guc_log_level >= 0)
> +			gen9_enable_guc_interrupts(dev_priv);
> +
> +		ret = i915_guc_submission_enable(dev_priv);

Hmm, enabling guc submission in this function is still something more
than one can expect from function name suggesting pure "load"...
Do you plan to offload this function in the future ?

Michal 

> +		if (ret)
> +			goto fail;
> +		guc_interrupts_capture(dev_priv);
> +	}
> +
> +	return 0;
> +
> +fail:
> +	/*
> +	 * We've failed to load the firmware :(
> +	 *
> +	 * Decide whether to disable GuC submission and fall back to
> +	 * execlist mode, and whether to hide the error by returning
> +	 * zero or to return -EIO, which the caller will treat as a
> +	 * nonfatal error (i.e. it doesn't prevent driver load, but
> +	 * marks the GPU as wedged until reset).
> +	 */
> +	if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1)
> +		ret = -EIO;
> +	else
> +		ret = 0;
> +
> +	if (i915.enable_guc_submission) {
> +		i915.enable_guc_submission = 0;
> +		DRM_INFO("GuC submission without firmware not supported\n");
> +		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> +	}
> +
> +	guc_interrupts_release(dev_priv);
> +	i915_guc_submission_disable(dev_priv);
> +	i915_guc_submission_fini(dev_priv);
> +
> +	return ret;
> +}
> +
>  /*
>   * Read GuC command/status register (SOFT_SCRATCH_0)
>   * Return true if it contains a response rather than a command
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index ec1a5b2..89c7e1f 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -28,6 +28,9 @@
>  #include "i915_guc_reg.h"
>  #include "intel_ringbuffer.h"
>  
> +/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> +#define GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS 3
> +
>  struct drm_i915_gem_request;
>  
>  /*
> @@ -171,6 +174,7 @@ struct intel_guc {
>  /* intel_uc.c */
>  void intel_uc_init_early(struct drm_i915_private *dev_priv);
>  void intel_uc_init(struct drm_i915_private *dev_priv);
> +int intel_uc_load(struct drm_i915_private *dev_priv);
>  bool intel_guc_recv(struct drm_i915_private *dev_priv, u32 *status);
>  int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
>  int intel_guc_sample_forcewake(struct intel_guc *guc);
> @@ -185,6 +189,10 @@ extern void intel_guc_fini(struct drm_i915_private *dev_priv);
>  extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status);
>  extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
>  extern int intel_guc_resume(struct drm_i915_private *dev_priv);
> +void guc_interrupts_release(struct drm_i915_private *dev_priv);
> +void guc_interrupts_capture(struct drm_i915_private *dev_priv);
> +int guc_hw_reset(struct drm_i915_private *dev_priv);
> +u32 guc_wopcm_size(struct drm_i915_private *dev_priv);
>  
>  /* i915_guc_submission.c */
>  int i915_guc_submission_init(struct drm_i915_private *dev_priv);
> -- 
> 2.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915/guc: Simplify intel_guc_load()

[Arkadiusz Hiler]

On Thu, Dec 15, 2016 at 05:38:16PM +0100, Michal Wajdeczko wrote:
> On Thu, Dec 15, 2016 at 04:47:06PM +0100, Arkadiusz Hiler wrote:
> > Current version of intel_guc_load() does a lot:
> >  - cares about submission
> >  - loads huc
> >  - implement WA
> > 
> > This change offloads some of the logic to intel_uc_load(), which now
> > cares about the above.
> > 
> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Cc: Jeff McGee <jeff.mcgee@intel.com>
> > Cc: Michal Winiarski <michal.winiarski@intel.com>
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c         |   2 +-
> >  drivers/gpu/drm/i915/intel_guc_loader.c | 126 +++++---------------------------
> >  drivers/gpu/drm/i915/intel_uc.c         |  83 +++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_uc.h         |   8 ++
> >  4 files changed, 110 insertions(+), 109 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 6af4e85..76b52c6 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4412,7 +4412,7 @@ i915_gem_init_hw(struct drm_i915_private *dev_priv)
> >  	intel_mocs_init_l3cc_table(dev_priv);
> >  
> >  	/* We can't enable contexts until all firmware is loaded */
> > -	ret = intel_guc_load(dev_priv);
> > +	ret = intel_uc_load(dev_priv);
> >  	if (ret)
> >  		goto out;
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> > index f8b28b1..b76b556 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> > @@ -97,7 +97,7 @@ const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status)
> >  	}
> >  };
> >  
> > -static void guc_interrupts_release(struct drm_i915_private *dev_priv)
> > +void guc_interrupts_release(struct drm_i915_private *dev_priv)
> >  {
> >  	struct intel_engine_cs *engine;
> >  	enum intel_engine_id id;
> > @@ -115,7 +115,7 @@ static void guc_interrupts_release(struct drm_i915_private *dev_priv)
> >  	I915_WRITE(GUC_WD_VECS_IER, 0);
> >  }
> >  
> > -static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
> > +void guc_interrupts_capture(struct drm_i915_private *dev_priv)
> >  {
> >  	struct intel_engine_cs *engine;
> >  	enum intel_engine_id id;
> > @@ -334,7 +334,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
> >  	return ret;
> >  }
> >  
> > -static u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
> > +u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
> >  {
> >  	u32 wopcm_size = GUC_WOPCM_TOP;
> >  
> > @@ -417,7 +417,7 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
> >  	return ret;
> >  }
> >  
> > -static int guc_hw_reset(struct drm_i915_private *dev_priv)
> > +int guc_hw_reset(struct drm_i915_private *dev_priv)
> >  {
> >  	int ret;
> >  	u32 guc_status;
> > @@ -452,75 +452,37 @@ int intel_guc_load(struct drm_i915_private *dev_priv)
> >  {
> >  	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> >  	const char *fw_path = guc_fw->guc_fw_path;
> > -	int retries, ret, err;
> > +	int ret;
> >  
> >  	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
> >  		fw_path,
> >  		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> >  		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> >  
> > -	/* Loading forbidden, or no firmware to load? */
> > -	if (!i915.enable_guc_loading) {
> > -		err = 0;
> > -		goto fail;
> > -	} else if (fw_path == NULL) {
> > +	if (fw_path == NULL) {
> >  		/* Device is known to have no uCode (e.g. no GuC) */
> > -		err = -ENXIO;
> > -		goto fail;
> > +		return -ENXIO;
> >  	} else if (*fw_path == '\0') {
> >  		/* Device has a GuC but we don't know what f/w to load? */
> >  		WARN(1, "No GuC firmware known for this platform!\n");
> > -		err = -ENODEV;
> > -		goto fail;
> > +		return -ENODEV;
> >  	}
> >  
> >  	/* Fetch failed, or already fetched but failed to load? */
> >  	if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) {
> > -		err = -EIO;
> > -		goto fail;
> > +		return -EIO;
> >  	} else if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) {
> > -		err = -ENOEXEC;
> > -		goto fail;
> > +		return -ENOEXEC;
> >  	}
> >  
> > -	guc_interrupts_release(dev_priv);
> > -	gen9_reset_guc_interrupts(dev_priv);
> > -
> >  	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
> >  
> > -	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> > -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> > -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> > -
> > -	err = i915_guc_submission_init(dev_priv);
> > -	if (err)
> > -		goto fail;
> > -
> > -	/*
> > -	 * WaEnableuKernelHeaderValidFix:skl,bxt
> > -	 * For BXT, this is only upto B0 but below WA is required for later
> > -	 * steppings also so this is extended as well.
> > -	 */
> >  	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> > -	for (retries = 3; ; ) {
> > -		/*
> > -		 * Always reset the GuC just before (re)loading, so
> > -		 * that the state and timing are fairly predictable
> > -		 */
> > -		err = guc_hw_reset(dev_priv);
> > -		if (err)
> > -			goto fail;
> > +	/* we may want to retry guc ucode transfer */
> > +	ret = guc_ucode_xfer(dev_priv);
> >  
> > -		err = guc_ucode_xfer(dev_priv);
> > -		if (!err)
> > -			break;
> > -
> > -		if (--retries == 0)
> > -			goto fail;
> > -
> > -		DRM_INFO("GuC fw load failed: %d; will reset and "
> > -			 "retry %d more time(s)\n", err, retries);
> > -	}
> > +	if (ret)
> > +		return -EAGAIN;
> >  
> >  	guc_fw->guc_fw_load_status = GUC_FIRMWARE_SUCCESS;
> >  
> > @@ -528,63 +490,7 @@ int intel_guc_load(struct drm_i915_private *dev_priv)
> >  		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> >  		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> >  
> > -	if (i915.enable_guc_submission) {
> > -		if (i915.guc_log_level >= 0)
> > -			gen9_enable_guc_interrupts(dev_priv);
> > -
> > -		err = i915_guc_submission_enable(dev_priv);
> > -		if (err)
> > -			goto fail;
> > -		guc_interrupts_capture(dev_priv);
> > -	}
> > -
> >  	return 0;
> > -
> > -fail:
> > -	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
> > -		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;
> > -
> > -	guc_interrupts_release(dev_priv);
> > -	i915_guc_submission_disable(dev_priv);
> > -	i915_guc_submission_fini(dev_priv);
> > -
> > -	/*
> > -	 * We've failed to load the firmware :(
> > -	 *
> > -	 * Decide whether to disable GuC submission and fall back to
> > -	 * execlist mode, and whether to hide the error by returning
> > -	 * zero or to return -EIO, which the caller will treat as a
> > -	 * nonfatal error (i.e. it doesn't prevent driver load, but
> > -	 * marks the GPU as wedged until reset).
> > -	 */
> > -	if (i915.enable_guc_loading > 1) {
> > -		ret = -EIO;
> > -	} else if (i915.enable_guc_submission > 1) {
> > -		ret = -EIO;
> > -	} else {
> > -		ret = 0;
> > -	}
> > -
> > -	if (err == 0 && !HAS_GUC_UCODE(dev_priv))
> > -		;	/* Don't mention the GuC! */
> > -	else if (err == 0)
> > -		DRM_INFO("GuC firmware load skipped\n");
> > -	else if (ret != -EIO)
> > -		DRM_NOTE("GuC firmware load failed: %d\n", err);
> > -	else
> > -		DRM_WARN("GuC firmware load failed: %d\n", err);
> > -
> > -	if (i915.enable_guc_submission) {
> > -		if (fw_path == NULL)
> > -			DRM_INFO("GuC submission without firmware not supported\n");
> > -		if (ret == 0)
> > -			DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> > -		else
> > -			DRM_ERROR("GuC init failed: %d\n", ret);
> > -	}
> > -	i915.enable_guc_submission = 0;
> > -
> > -	return ret;
> >  }
> >  
> >  static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> > @@ -757,6 +663,10 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
> >  	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
> >  	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
> >  
> > +	/* can't enable guc submission without guc */
> > +	if (!i915.enable_guc_loading)
> > +		i915.enable_guc_submission = 0;
> > +
> >  	/* Early (and silent) return if GuC loading is disabled */
> >  	if (!i915.enable_guc_loading)
> >  		return;
> > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> > index 8eec035..4e184edb 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > @@ -35,6 +35,89 @@ void intel_uc_init(struct drm_i915_private *dev_priv)
> >  	intel_guc_init(dev_priv);
> >  }
> >  
> > +int intel_uc_load(struct drm_i915_private *dev_priv)
> > +{
> > +	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > +	int ret, retries;
> > +
> > +	/* guc not enabled, nothing to do */
> > +	if (!i915.enable_guc_loading)
> > +		return 0;
> > +
> > +	guc_interrupts_release(dev_priv);
> > +	gen9_reset_guc_interrupts(dev_priv);
> > +
> > +	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
> > +
> > +	if (i915.enable_guc_submission) {
> > +		ret = i915_guc_submission_init(dev_priv);
> > +		if (ret)
> > +			goto fail;
> > +	}
> > +
> > +	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> > +	retries = GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS;
> > +	while (retries--) {
> > +		/*
> > +		 * Always reset the GuC just before (re)loading, so
> > +		 * that the state and timing are fairly predictable
> > +		 */
> > +		ret = guc_hw_reset(dev_priv);
> > +		if (ret)
> > +			goto fail;
> > +
> > +		ret = intel_guc_load(dev_priv);
> > +		if (ret == 0 || ret != -EAGAIN)
> > +			break;
> > +
> > +		DRM_INFO("GuC fw load failed: %d; will reset and "
> > +			 "retry %d more time(s)\n", ret, retries);
> > +	}
> > +
> > +	/* did we succeded or run out of retries? */
> > +	if (ret)
> > +		goto fail;
> > +
> > +	if (i915.enable_guc_submission) {
> > +		if (i915.guc_log_level >= 0)
> > +			gen9_enable_guc_interrupts(dev_priv);
> > +
> > +		ret = i915_guc_submission_enable(dev_priv);
> 
> Hmm, enabling guc submission in this function is still something more
> than one can expect from function name suggesting pure "load"...
> Do you plan to offload this function in the future ?

Yes. With vol. 1 I wanted to push things in the right direction without
really finalizing them.

I could have spend month and grow the series to dozens of patches, but I
think it's better to improve things incrementally, with smaller,
digestible patches and series.

I also anticipate a lot of valuable feedback that I can incorporate in
the future vols.

> > +		if (ret)
> > +			goto fail;
> > +		guc_interrupts_capture(dev_priv);
> > +	}
> > +
> > +	return 0;
> > +
> > +fail:
> > +	/*
> > +	 * We've failed to load the firmware :(
> > +	 *
> > +	 * Decide whether to disable GuC submission and fall back to
> > +	 * execlist mode, and whether to hide the error by returning
> > +	 * zero or to return -EIO, which the caller will treat as a
> > +	 * nonfatal error (i.e. it doesn't prevent driver load, but
> > +	 * marks the GPU as wedged until reset).
> > +	 */
> > +	if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1)
> > +		ret = -EIO;
> > +	else
> > +		ret = 0;
> > +
> > +	if (i915.enable_guc_submission) {
> > +		i915.enable_guc_submission = 0;
> > +		DRM_INFO("GuC submission without firmware not supported\n");
> > +		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> > +	}
> > +
> > +	guc_interrupts_release(dev_priv);
> > +	i915_guc_submission_disable(dev_priv);
> > +	i915_guc_submission_fini(dev_priv);
> > +
> > +	return ret;
> > +}
> > +
> >  /*
> >   * Read GuC command/status register (SOFT_SCRATCH_0)
> >   * Return true if it contains a response rather than a command
> > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> > index ec1a5b2..89c7e1f 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.h
> > +++ b/drivers/gpu/drm/i915/intel_uc.h
> > @@ -28,6 +28,9 @@
> >  #include "i915_guc_reg.h"
> >  #include "intel_ringbuffer.h"
> >  
> > +/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> > +#define GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS 3
> > +
> >  struct drm_i915_gem_request;
> >  
> >  /*
> > @@ -171,6 +174,7 @@ struct intel_guc {
> >  /* intel_uc.c */
> >  void intel_uc_init_early(struct drm_i915_private *dev_priv);
> >  void intel_uc_init(struct drm_i915_private *dev_priv);
> > +int intel_uc_load(struct drm_i915_private *dev_priv);
> >  bool intel_guc_recv(struct drm_i915_private *dev_priv, u32 *status);
> >  int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
> >  int intel_guc_sample_forcewake(struct intel_guc *guc);
> > @@ -185,6 +189,10 @@ extern void intel_guc_fini(struct drm_i915_private *dev_priv);
> >  extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status);
> >  extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
> >  extern int intel_guc_resume(struct drm_i915_private *dev_priv);
> > +void guc_interrupts_release(struct drm_i915_private *dev_priv);
> > +void guc_interrupts_capture(struct drm_i915_private *dev_priv);
> > +int guc_hw_reset(struct drm_i915_private *dev_priv);
> > +u32 guc_wopcm_size(struct drm_i915_private *dev_priv);
> >  
> >  /* i915_guc_submission.c */
> >  int i915_guc_submission_init(struct drm_i915_private *dev_priv);
> > -- 
> > 2.9.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Cheers,
Arek
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915/guc: Simplify intel_guc_load()

[Daniele Ceraolo Spurio]

On 15/12/16 07:47, Arkadiusz Hiler wrote:
> Current version of intel_guc_load() does a lot:
>  - cares about submission
>  - loads huc
>  - implement WA
>
> This change offloads some of the logic to intel_uc_load(), which now
> cares about the above.
>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Jeff McGee <jeff.mcgee@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c         |   2 +-
>  drivers/gpu/drm/i915/intel_guc_loader.c | 126 +++++---------------------------
>  drivers/gpu/drm/i915/intel_uc.c         |  83 +++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_uc.h         |   8 ++
>  4 files changed, 110 insertions(+), 109 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6af4e85..76b52c6 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4412,7 +4412,7 @@ i915_gem_init_hw(struct drm_i915_private *dev_priv)
>  	intel_mocs_init_l3cc_table(dev_priv);
>
>  	/* We can't enable contexts until all firmware is loaded */
> -	ret = intel_guc_load(dev_priv);
> +	ret = intel_uc_load(dev_priv);
>  	if (ret)
>  		goto out;
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index f8b28b1..b76b556 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -97,7 +97,7 @@ const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status)
>  	}
>  };
>
> -static void guc_interrupts_release(struct drm_i915_private *dev_priv)
> +void guc_interrupts_release(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_engine_cs *engine;
>  	enum intel_engine_id id;
> @@ -115,7 +115,7 @@ static void guc_interrupts_release(struct drm_i915_private *dev_priv)
>  	I915_WRITE(GUC_WD_VECS_IER, 0);
>  }
>
> -static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
> +void guc_interrupts_capture(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_engine_cs *engine;
>  	enum intel_engine_id id;
> @@ -334,7 +334,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
>  	return ret;
>  }
>
> -static u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
> +u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
>  {
>  	u32 wopcm_size = GUC_WOPCM_TOP;
>
> @@ -417,7 +417,7 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
>  	return ret;
>  }
>
> -static int guc_hw_reset(struct drm_i915_private *dev_priv)
> +int guc_hw_reset(struct drm_i915_private *dev_priv)

If I haven't missed anything, guc_hw_reset is only called in 1 place, so 
we could keep the function static and move it to intel_uc.c.

>  {
>  	int ret;
>  	u32 guc_status;
> @@ -452,75 +452,37 @@ int intel_guc_load(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>  	const char *fw_path = guc_fw->guc_fw_path;
> -	int retries, ret, err;
> +	int ret;
>
>  	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
>  		fw_path,
>  		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
>  		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
>
> -	/* Loading forbidden, or no firmware to load? */
> -	if (!i915.enable_guc_loading) {
> -		err = 0;
> -		goto fail;
> -	} else if (fw_path == NULL) {
> +	if (fw_path == NULL) {
>  		/* Device is known to have no uCode (e.g. no GuC) */
> -		err = -ENXIO;
> -		goto fail;
> +		return -ENXIO;
>  	} else if (*fw_path == '\0') {
>  		/* Device has a GuC but we don't know what f/w to load? */
>  		WARN(1, "No GuC firmware known for this platform!\n");
> -		err = -ENODEV;
> -		goto fail;
> +		return -ENODEV;
>  	}
>
>  	/* Fetch failed, or already fetched but failed to load? */
>  	if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) {
> -		err = -EIO;
> -		goto fail;
> +		return -EIO;
>  	} else if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) {
> -		err = -ENOEXEC;
> -		goto fail;
> +		return -ENOEXEC;
>  	}
>
> -	guc_interrupts_release(dev_priv);
> -	gen9_reset_guc_interrupts(dev_priv);
> -
>  	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
>
> -	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> -
> -	err = i915_guc_submission_init(dev_priv);
> -	if (err)
> -		goto fail;
> -
> -	/*
> -	 * WaEnableuKernelHeaderValidFix:skl,bxt
> -	 * For BXT, this is only upto B0 but below WA is required for later
> -	 * steppings also so this is extended as well.
> -	 */

This comment is removed, but the WA is applicable to all SKL steppings 
and is also applicable to HuC according to the specs so I suggest to 
retain the comment and move it to intel_uc_load().

>  	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */

The implementation of this WA is now outside this function and it is 
marked as such there. I'd personally prefer to remove this comment from 
here as it might cause confusion, but no strong feelings either way.

> -	for (retries = 3; ; ) {
> -		/*
> -		 * Always reset the GuC just before (re)loading, so
> -		 * that the state and timing are fairly predictable
> -		 */
> -		err = guc_hw_reset(dev_priv);
> -		if (err)
> -			goto fail;
> +	/* we may want to retry guc ucode transfer */
> +	ret = guc_ucode_xfer(dev_priv);
>
> -		err = guc_ucode_xfer(dev_priv);
> -		if (!err)
> -			break;
> -
> -		if (--retries == 0)
> -			goto fail;
> -
> -		DRM_INFO("GuC fw load failed: %d; will reset and "
> -			 "retry %d more time(s)\n", err, retries);
> -	}
> +	if (ret)
> +		return -EAGAIN;
>
>  	guc_fw->guc_fw_load_status = GUC_FIRMWARE_SUCCESS;
>
> @@ -528,63 +490,7 @@ int intel_guc_load(struct drm_i915_private *dev_priv)
>  		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
>  		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
>
> -	if (i915.enable_guc_submission) {
> -		if (i915.guc_log_level >= 0)
> -			gen9_enable_guc_interrupts(dev_priv);
> -
> -		err = i915_guc_submission_enable(dev_priv);
> -		if (err)
> -			goto fail;
> -		guc_interrupts_capture(dev_priv);
> -	}
> -
>  	return 0;
> -
> -fail:
> -	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
> -		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;
> -
> -	guc_interrupts_release(dev_priv);
> -	i915_guc_submission_disable(dev_priv);
> -	i915_guc_submission_fini(dev_priv);
> -
> -	/*
> -	 * We've failed to load the firmware :(
> -	 *
> -	 * Decide whether to disable GuC submission and fall back to
> -	 * execlist mode, and whether to hide the error by returning
> -	 * zero or to return -EIO, which the caller will treat as a
> -	 * nonfatal error (i.e. it doesn't prevent driver load, but
> -	 * marks the GPU as wedged until reset).
> -	 */
> -	if (i915.enable_guc_loading > 1) {
> -		ret = -EIO;
> -	} else if (i915.enable_guc_submission > 1) {
> -		ret = -EIO;
> -	} else {
> -		ret = 0;
> -	}
> -
> -	if (err == 0 && !HAS_GUC_UCODE(dev_priv))
> -		;	/* Don't mention the GuC! */
> -	else if (err == 0)
> -		DRM_INFO("GuC firmware load skipped\n");
> -	else if (ret != -EIO)
> -		DRM_NOTE("GuC firmware load failed: %d\n", err);
> -	else
> -		DRM_WARN("GuC firmware load failed: %d\n", err);
> -
> -	if (i915.enable_guc_submission) {
> -		if (fw_path == NULL)
> -			DRM_INFO("GuC submission without firmware not supported\n");
> -		if (ret == 0)
> -			DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> -		else
> -			DRM_ERROR("GuC init failed: %d\n", ret);
> -	}
> -	i915.enable_guc_submission = 0;
> -
> -	return ret;
>  }
>
>  static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> @@ -757,6 +663,10 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
>  	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
>  	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
>
> +	/* can't enable guc submission without guc */
> +	if (!i915.enable_guc_loading)
> +		i915.enable_guc_submission = 0;
> +
>  	/* Early (and silent) return if GuC loading is disabled */
>  	if (!i915.enable_guc_loading)
>  		return;
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 8eec035..4e184edb 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -35,6 +35,89 @@ void intel_uc_init(struct drm_i915_private *dev_priv)
>  	intel_guc_init(dev_priv);
>  }
>
> +int intel_uc_load(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> +	int ret, retries;
> +
> +	/* guc not enabled, nothing to do */
> +	if (!i915.enable_guc_loading)
> +		return 0;
> +
> +	guc_interrupts_release(dev_priv);
> +	gen9_reset_guc_interrupts(dev_priv);
> +
> +	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
> +
> +	if (i915.enable_guc_submission) {
> +		ret = i915_guc_submission_init(dev_priv);
> +		if (ret)
> +			goto fail;
> +	}
> +
> +	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> +	retries = GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS;
> +	while (retries--) {
> +		/*
> +		 * Always reset the GuC just before (re)loading, so
> +		 * that the state and timing are fairly predictable
> +		 */
> +		ret = guc_hw_reset(dev_priv);
> +		if (ret)
> +			goto fail;
> +
> +		ret = intel_guc_load(dev_priv);
> +		if (ret == 0 || ret != -EAGAIN)
> +			break;
> +
> +		DRM_INFO("GuC fw load failed: %d; will reset and "
> +			 "retry %d more time(s)\n", ret, retries);
> +	}
> +
> +	/* did we succeded or run out of retries? */
> +	if (ret)
> +		goto fail;
> +
> +	if (i915.enable_guc_submission) {
> +		if (i915.guc_log_level >= 0)
> +			gen9_enable_guc_interrupts(dev_priv);
> +
> +		ret = i915_guc_submission_enable(dev_priv);
> +		if (ret)
> +			goto fail;
> +		guc_interrupts_capture(dev_priv);
> +	}
> +
> +	return 0;
> +
> +fail:
> +	/*
> +	 * We've failed to load the firmware :(
> +	 *
> +	 * Decide whether to disable GuC submission and fall back to
> +	 * execlist mode, and whether to hide the error by returning
> +	 * zero or to return -EIO, which the caller will treat as a
> +	 * nonfatal error (i.e. it doesn't prevent driver load, but
> +	 * marks the GPU as wedged until reset).
> +	 */
> +	if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1)
> +		ret = -EIO;
> +	else
> +		ret = 0;
> +
> +	if (i915.enable_guc_submission) {
> +		i915.enable_guc_submission = 0;
> +		DRM_INFO("GuC submission without firmware not supported\n");
> +		DRM_NOTE("Falling back from GuC submission to execlist mode\n");

If i915.enable_guc_submission > 1 we will mark the GPU as wedged so it 
might be worth retaining an error level message here in that scenario.

Apart from the minor comments above, the code re-org looks sensible (and 
required :)) and the patch lgtm.

Thanks,
Daniele

> +	}
> +
> +	guc_interrupts_release(dev_priv);
> +	i915_guc_submission_disable(dev_priv);
> +	i915_guc_submission_fini(dev_priv);
> +
> +	return ret;
> +}
> +
>  /*
>   * Read GuC command/status register (SOFT_SCRATCH_0)
>   * Return true if it contains a response rather than a command
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index ec1a5b2..89c7e1f 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -28,6 +28,9 @@
>  #include "i915_guc_reg.h"
>  #include "intel_ringbuffer.h"
>
> +/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> +#define GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS 3
> +
>  struct drm_i915_gem_request;
>
>  /*
> @@ -171,6 +174,7 @@ struct intel_guc {
>  /* intel_uc.c */
>  void intel_uc_init_early(struct drm_i915_private *dev_priv);
>  void intel_uc_init(struct drm_i915_private *dev_priv);
> +int intel_uc_load(struct drm_i915_private *dev_priv);
>  bool intel_guc_recv(struct drm_i915_private *dev_priv, u32 *status);
>  int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
>  int intel_guc_sample_forcewake(struct intel_guc *guc);
> @@ -185,6 +189,10 @@ extern void intel_guc_fini(struct drm_i915_private *dev_priv);
>  extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status);
>  extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
>  extern int intel_guc_resume(struct drm_i915_private *dev_priv);
> +void guc_interrupts_release(struct drm_i915_private *dev_priv);
> +void guc_interrupts_capture(struct drm_i915_private *dev_priv);
> +int guc_hw_reset(struct drm_i915_private *dev_priv);
> +u32 guc_wopcm_size(struct drm_i915_private *dev_priv);
>
>  /* i915_guc_submission.c */
>  int i915_guc_submission_init(struct drm_i915_private *dev_priv);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915/guc: Simplify intel_guc_load()

[Arkadiusz Hiler]

On Thu, Dec 15, 2016 at 02:26:29PM -0800, Daniele Ceraolo Spurio wrote:
> 
> 
> On 15/12/16 07:47, Arkadiusz Hiler wrote:
> > Current version of intel_guc_load() does a lot:
> >  - cares about submission
> >  - loads huc
> >  - implement WA
> > 
> > This change offloads some of the logic to intel_uc_load(), which now
> > cares about the above.
> > 
> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Cc: Jeff McGee <jeff.mcgee@intel.com>
> > Cc: Michal Winiarski <michal.winiarski@intel.com>
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c         |   2 +-
> >  drivers/gpu/drm/i915/intel_guc_loader.c | 126 +++++---------------------------
> >  drivers/gpu/drm/i915/intel_uc.c         |  83 +++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_uc.h         |   8 ++
> >  4 files changed, 110 insertions(+), 109 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 6af4e85..76b52c6 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -417,7 +417,7 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
> >  	return ret;
> >  }
> > 
> > -static int guc_hw_reset(struct drm_i915_private *dev_priv)
> > +int guc_hw_reset(struct drm_i915_private *dev_priv)
> If I haven't missed anything, guc_hw_reset is only called in 1 place, so we
> could keep the function static and move it to intel_uc.c.

Okay.

> >  {
> >  	int ret;
> >  	u32 guc_status;
> > @@ -452,75 +452,37 @@ int intel_guc_load(struct drm_i915_private *dev_priv)
> >  {
> >  	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> >  	const char *fw_path = guc_fw->guc_fw_path;
> > -	int retries, ret, err;
> > +	int ret;
> > 
> >  	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
> >  		fw_path,
> >  		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> >  		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> > 
> > -	/* Loading forbidden, or no firmware to load? */
> > -	if (!i915.enable_guc_loading) {
> > -		err = 0;
> > -		goto fail;
> > -	} else if (fw_path == NULL) {
> > +	if (fw_path == NULL) {
> >  		/* Device is known to have no uCode (e.g. no GuC) */
> > -		err = -ENXIO;
> > -		goto fail;
> > +		return -ENXIO;
> >  	} else if (*fw_path == '\0') {
> >  		/* Device has a GuC but we don't know what f/w to load? */
> >  		WARN(1, "No GuC firmware known for this platform!\n");
> > -		err = -ENODEV;
> > -		goto fail;
> > +		return -ENODEV;
> >  	}
> > 
> >  	/* Fetch failed, or already fetched but failed to load? */
> >  	if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) {
> > -		err = -EIO;
> > -		goto fail;
> > +		return -EIO;
> >  	} else if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) {
> > -		err = -ENOEXEC;
> > -		goto fail;
> > +		return -ENOEXEC;
> >  	}
> > 
> > -	guc_interrupts_release(dev_priv);
> > -	gen9_reset_guc_interrupts(dev_priv);
> > -
> >  	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
> > 
> > -	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> > -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> > -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> > -
> > -	err = i915_guc_submission_init(dev_priv);
> > -	if (err)
> > -		goto fail;
> > -
> > -	/*
> > -	 * WaEnableuKernelHeaderValidFix:skl,bxt
> > -	 * For BXT, this is only upto B0 but below WA is required for later
> > -	 * steppings also so this is extended as well.
> > -	 */
> 
> This comment is removed, but the WA is applicable to all SKL steppings and
> is also applicable to HuC according to the specs so I suggest to retain the
> comment and move it to intel_uc_load().

I missread the commend. I'll leave this as a
WaEnableuKernelHeaderValidFix:skl
since it is fixed for BXT.


> >  	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> 
> The implementation of this WA is now outside this function and it is marked
> as such there. I'd personally prefer to remove this comment from here as it
> might cause confusion, but no strong feelings either way.

The implementation is twofold now - the the function which returns
-EAGAIN if we failed at the step we know may fail and we may want to
retry on it.

Then you have functions that handles the actuall three attempts.

So I prefer to keep the note on the WA in both places.

> > -	for (retries = 3; ; ) {
> > -		/*
> > -		 * Always reset the GuC just before (re)loading, so
> > -		 * that the state and timing are fairly predictable
> > -		 */
> > -		err = guc_hw_reset(dev_priv);
> > -		if (err)
> > -			goto fail;
> > +	/* we may want to retry guc ucode transfer */
> > +	ret = guc_ucode_xfer(dev_priv);
> > 
> > -		err = guc_ucode_xfer(dev_priv);
> > -		if (!err)
> > -			break;
> > -
> > -		if (--retries == 0)
> > -			goto fail;
> > -
> > -		DRM_INFO("GuC fw load failed: %d; will reset and "
> > -			 "retry %d more time(s)\n", err, retries);
> > -	}
> > +	if (ret)
> > +		return -EAGAIN;
> > 
> >  	guc_fw->guc_fw_load_status = GUC_FIRMWARE_SUCCESS;
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> > index 8eec035..4e184edb 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > @@ -35,6 +35,89 @@ void intel_uc_init(struct drm_i915_private *dev_priv)
> >  	intel_guc_init(dev_priv);
> >  }
> > 
> > +int intel_uc_load(struct drm_i915_private *dev_priv)
> > +{
> > +	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> > +	int ret, retries;
> > +
> > +	/* guc not enabled, nothing to do */
> > +	if (!i915.enable_guc_loading)
> > +		return 0;
> > +
> > +	guc_interrupts_release(dev_priv);
> > +	gen9_reset_guc_interrupts(dev_priv);
> > +
> > +	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
> > +
> > +	if (i915.enable_guc_submission) {
> > +		ret = i915_guc_submission_init(dev_priv);
> > +		if (ret)
> > +			goto fail;
> > +	}
> > +
> > +	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> > +	retries = GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS;
> > +	while (retries--) {
> > +		/*
> > +		 * Always reset the GuC just before (re)loading, so
> > +		 * that the state and timing are fairly predictable
> > +		 */
> > +		ret = guc_hw_reset(dev_priv);
> > +		if (ret)
> > +			goto fail;
> > +
> > +		ret = intel_guc_load(dev_priv);
> > +		if (ret == 0 || ret != -EAGAIN)
> > +			break;
> > +
> > +		DRM_INFO("GuC fw load failed: %d; will reset and "
> > +			 "retry %d more time(s)\n", ret, retries);
> > +	}
> > +
> > +	/* did we succeded or run out of retries? */
> > +	if (ret)
> > +		goto fail;
> > +
> > +	if (i915.enable_guc_submission) {
> > +		if (i915.guc_log_level >= 0)
> > +			gen9_enable_guc_interrupts(dev_priv);
> > +
> > +		ret = i915_guc_submission_enable(dev_priv);
> > +		if (ret)
> > +			goto fail;
> > +		guc_interrupts_capture(dev_priv);
> > +	}
> > +
> > +	return 0;
> > +
> > +fail:
> > +	/*
> > +	 * We've failed to load the firmware :(
> > +	 *
> > +	 * Decide whether to disable GuC submission and fall back to
> > +	 * execlist mode, and whether to hide the error by returning
> > +	 * zero or to return -EIO, which the caller will treat as a
> > +	 * nonfatal error (i.e. it doesn't prevent driver load, but
> > +	 * marks the GPU as wedged until reset).
> > +	 */
> > +	if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1)
> > +		ret = -EIO;
> > +	else
> > +		ret = 0;
> > +
> > +	if (i915.enable_guc_submission) {
> > +		i915.enable_guc_submission = 0;
> > +		DRM_INFO("GuC submission without firmware not supported\n");
> > +		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> 
> If i915.enable_guc_submission > 1 we will mark the GPU as wedged so it might
> be worth retaining an error level message here in that scenario.

If we are wedging the GPU you do not really care about the fallback, so
theres no real use in having that promoted + those are the original
levels that were already here.

Anyway, it seems like the `enable_guc_* > 1` are likely to be gone. I've
discussed that on IRC yesterday and no one seems to really remember why
we've got it in the first place.

Anusha posted similar concern here with her HuC series as well.

> Apart from the minor comments above, the code re-org looks sensible (and
> required :)) and the patch lgtm.
> 
> Thanks,
> Daniele
> 
> > +	}
> > +
> > +	guc_interrupts_release(dev_priv);
> > +	i915_guc_submission_disable(dev_priv);
> > +	i915_guc_submission_fini(dev_priv);
> > +
> > +	return ret;
> > +}
> > +
> >  /*
> >   * Read GuC command/status register (SOFT_SCRATCH_0)
> >   * Return true if it contains a response rather than a command

-- 
Cheers,
Arek
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915/guc: Simplify intel_guc_load()

[Daniele Ceraolo Spurio]

<snip>

>>> +
>>> +fail:
>>> +	/*
>>> +	 * We've failed to load the firmware :(
>>> +	 *
>>> +	 * Decide whether to disable GuC submission and fall back to
>>> +	 * execlist mode, and whether to hide the error by returning
>>> +	 * zero or to return -EIO, which the caller will treat as a
>>> +	 * nonfatal error (i.e. it doesn't prevent driver load, but
>>> +	 * marks the GPU as wedged until reset).
>>> +	 */
>>> +	if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1)
>>> +		ret = -EIO;
>>> +	else
>>> +		ret = 0;
>>> +
>>> +	if (i915.enable_guc_submission) {
>>> +		i915.enable_guc_submission = 0;
>>> +		DRM_INFO("GuC submission without firmware not supported\n");
>>> +		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
>>
>> If i915.enable_guc_submission > 1 we will mark the GPU as wedged so it might
>> be worth retaining an error level message here in that scenario.
>
> If we are wedging the GPU you do not really care about the fallback, so
> theres no real use in having that promoted + those are the original
> levels that were already here.
>
> Anyway, it seems like the `enable_guc_* > 1` are likely to be gone. I've
> discussed that on IRC yesterday and no one seems to really remember why
> we've got it in the first place.
>
> Anusha posted similar concern here with her HuC series as well.
>

Just to clarify (because as you said the case will probably go away), 
what I meant was an extra log for the > 1 case like we had in the 
original code, i.e:

	DRM_ERROR("GuC init failed: %d\n", ret);

as otherwise we would have declared the GPU wedged without printing any 
error-level message to explain why.

Daniele
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915/guc: Simplify intel_guc_load()

[Tvrtko Ursulin]

On 15/12/2016 15:47, Arkadiusz Hiler wrote:
> Current version of intel_guc_load() does a lot:
>  - cares about submission
>  - loads huc

Not yet, no? So instead you could say that you are preparing the 
groundworks to make adding in the HuC fit better.

>  - implement WA
>
> This change offloads some of the logic to intel_uc_load(), which now
> cares about the above.
>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Jeff McGee <jeff.mcgee@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c         |   2 +-
>  drivers/gpu/drm/i915/intel_guc_loader.c | 126 +++++---------------------------
>  drivers/gpu/drm/i915/intel_uc.c         |  83 +++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_uc.h         |   8 ++
>  4 files changed, 110 insertions(+), 109 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6af4e85..76b52c6 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4412,7 +4412,7 @@ i915_gem_init_hw(struct drm_i915_private *dev_priv)
>  	intel_mocs_init_l3cc_table(dev_priv);
>
>  	/* We can't enable contexts until all firmware is loaded */
> -	ret = intel_guc_load(dev_priv);
> +	ret = intel_uc_load(dev_priv);
>  	if (ret)
>  		goto out;
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index f8b28b1..b76b556 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -97,7 +97,7 @@ const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status)
>  	}
>  };
>
> -static void guc_interrupts_release(struct drm_i915_private *dev_priv)
> +void guc_interrupts_release(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_engine_cs *engine;
>  	enum intel_engine_id id;
> @@ -115,7 +115,7 @@ static void guc_interrupts_release(struct drm_i915_private *dev_priv)
>  	I915_WRITE(GUC_WD_VECS_IER, 0);
>  }
>
> -static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
> +void guc_interrupts_capture(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_engine_cs *engine;
>  	enum intel_engine_id id;
> @@ -334,7 +334,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
>  	return ret;
>  }
>
> -static u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
> +u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
>  {
>  	u32 wopcm_size = GUC_WOPCM_TOP;
>
> @@ -417,7 +417,7 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
>  	return ret;
>  }
>
> -static int guc_hw_reset(struct drm_i915_private *dev_priv)
> +int guc_hw_reset(struct drm_i915_private *dev_priv)
>  {
>  	int ret;
>  	u32 guc_status;
> @@ -452,75 +452,37 @@ int intel_guc_load(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>  	const char *fw_path = guc_fw->guc_fw_path;
> -	int retries, ret, err;
> +	int ret;
>
>  	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
>  		fw_path,
>  		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
>  		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
>
> -	/* Loading forbidden, or no firmware to load? */
> -	if (!i915.enable_guc_loading) {
> -		err = 0;
> -		goto fail;
> -	} else if (fw_path == NULL) {
> +	if (fw_path == NULL) {
>  		/* Device is known to have no uCode (e.g. no GuC) */
> -		err = -ENXIO;
> -		goto fail;
> +		return -ENXIO;
>  	} else if (*fw_path == '\0') {
>  		/* Device has a GuC but we don't know what f/w to load? */
>  		WARN(1, "No GuC firmware known for this platform!\n");
> -		err = -ENODEV;
> -		goto fail;
> +		return -ENODEV;
>  	}
>
>  	/* Fetch failed, or already fetched but failed to load? */
>  	if (guc_fw->guc_fw_fetch_status != GUC_FIRMWARE_SUCCESS) {
> -		err = -EIO;
> -		goto fail;
> +		return -EIO;
>  	} else if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_FAIL) {
> -		err = -ENOEXEC;
> -		goto fail;
> +		return -ENOEXEC;
>  	}
>
> -	guc_interrupts_release(dev_priv);
> -	gen9_reset_guc_interrupts(dev_priv);
> -
>  	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
>
> -	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> -		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
> -		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
> -
> -	err = i915_guc_submission_init(dev_priv);
> -	if (err)
> -		goto fail;
> -
> -	/*
> -	 * WaEnableuKernelHeaderValidFix:skl,bxt
> -	 * For BXT, this is only upto B0 but below WA is required for later
> -	 * steppings also so this is extended as well.
> -	 */
>  	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> -	for (retries = 3; ; ) {
> -		/*
> -		 * Always reset the GuC just before (re)loading, so
> -		 * that the state and timing are fairly predictable
> -		 */
> -		err = guc_hw_reset(dev_priv);
> -		if (err)
> -			goto fail;
> +	/* we may want to retry guc ucode transfer */
> +	ret = guc_ucode_xfer(dev_priv);
>
> -		err = guc_ucode_xfer(dev_priv);
> -		if (!err)
> -			break;
> -
> -		if (--retries == 0)
> -			goto fail;
> -
> -		DRM_INFO("GuC fw load failed: %d; will reset and "
> -			 "retry %d more time(s)\n", err, retries);
> -	}
> +	if (ret)
> +		return -EAGAIN;
>
>  	guc_fw->guc_fw_load_status = GUC_FIRMWARE_SUCCESS;
>
> @@ -528,63 +490,7 @@ int intel_guc_load(struct drm_i915_private *dev_priv)
>  		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
>  		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
>
> -	if (i915.enable_guc_submission) {
> -		if (i915.guc_log_level >= 0)
> -			gen9_enable_guc_interrupts(dev_priv);
> -
> -		err = i915_guc_submission_enable(dev_priv);
> -		if (err)
> -			goto fail;
> -		guc_interrupts_capture(dev_priv);
> -	}
> -
>  	return 0;
> -
> -fail:
> -	if (guc_fw->guc_fw_load_status == GUC_FIRMWARE_PENDING)
> -		guc_fw->guc_fw_load_status = GUC_FIRMWARE_FAIL;

You seem to have lost this state transition at least in the 
enable_guc_loading = 0 case. Was that deliberate? I have no idea if that 
is important so just an observation.

> -
> -	guc_interrupts_release(dev_priv);
> -	i915_guc_submission_disable(dev_priv);
> -	i915_guc_submission_fini(dev_priv);
> -
> -	/*
> -	 * We've failed to load the firmware :(
> -	 *
> -	 * Decide whether to disable GuC submission and fall back to
> -	 * execlist mode, and whether to hide the error by returning
> -	 * zero or to return -EIO, which the caller will treat as a
> -	 * nonfatal error (i.e. it doesn't prevent driver load, but
> -	 * marks the GPU as wedged until reset).
> -	 */
> -	if (i915.enable_guc_loading > 1) {
> -		ret = -EIO;
> -	} else if (i915.enable_guc_submission > 1) {
> -		ret = -EIO;
> -	} else {
> -		ret = 0;
> -	}
> -
> -	if (err == 0 && !HAS_GUC_UCODE(dev_priv))
> -		;	/* Don't mention the GuC! */
> -	else if (err == 0)
> -		DRM_INFO("GuC firmware load skipped\n");
> -	else if (ret != -EIO)
> -		DRM_NOTE("GuC firmware load failed: %d\n", err);
> -	else
> -		DRM_WARN("GuC firmware load failed: %d\n", err);
> -
> -	if (i915.enable_guc_submission) {
> -		if (fw_path == NULL)
> -			DRM_INFO("GuC submission without firmware not supported\n");
> -		if (ret == 0)
> -			DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> -		else
> -			DRM_ERROR("GuC init failed: %d\n", ret);
> -	}
> -	i915.enable_guc_submission = 0;
> -
> -	return ret;
>  }
>
>  static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> @@ -757,6 +663,10 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
>  	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
>  	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
>
> +	/* can't enable guc submission without guc */
> +	if (!i915.enable_guc_loading)
> +		i915.enable_guc_submission = 0;
> +
>  	/* Early (and silent) return if GuC loading is disabled */
>  	if (!i915.enable_guc_loading)
>  		return;

You got two same conditions one after another, maybe merge them?

> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 8eec035..4e184edb 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -35,6 +35,89 @@ void intel_uc_init(struct drm_i915_private *dev_priv)
>  	intel_guc_init(dev_priv);
>  }
>
> +int intel_uc_load(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> +	int ret, retries;
> +
> +	/* guc not enabled, nothing to do */
> +	if (!i915.enable_guc_loading)
> +		return 0;
> +
> +	guc_interrupts_release(dev_priv);
> +	gen9_reset_guc_interrupts(dev_priv);
> +
> +	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
> +
> +	if (i915.enable_guc_submission) {
> +		ret = i915_guc_submission_init(dev_priv);
> +		if (ret)
> +			goto fail;
> +	}
> +
> +	/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> +	retries = GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS;
> +	while (retries--) {
> +		/*
> +		 * Always reset the GuC just before (re)loading, so
> +		 * that the state and timing are fairly predictable
> +		 */
> +		ret = guc_hw_reset(dev_priv);
> +		if (ret)
> +			goto fail;
> +
> +		ret = intel_guc_load(dev_priv);
> +		if (ret == 0 || ret != -EAGAIN)
> +			break;
> +
> +		DRM_INFO("GuC fw load failed: %d; will reset and "
> +			 "retry %d more time(s)\n", ret, retries);
> +	}
> +
> +	/* did we succeded or run out of retries? */

"Have we succeeded" I think.

> +	if (ret)
> +		goto fail;

There was a debug message round about here I think which logged that the 
firmware was successfully loaded. I think it is good to have it. I think 
even logging the major and minor would be good and maybe even at 
informational level?

> +
> +	if (i915.enable_guc_submission) {
> +		if (i915.guc_log_level >= 0)
> +			gen9_enable_guc_interrupts(dev_priv);
> +
> +		ret = i915_guc_submission_enable(dev_priv);
> +		if (ret)
> +			goto fail;
> +		guc_interrupts_capture(dev_priv);
> +	}
> +
> +	return 0;
> +
> +fail:
> +	/*
> +	 * We've failed to load the firmware :(
> +	 *
> +	 * Decide whether to disable GuC submission and fall back to
> +	 * execlist mode, and whether to hide the error by returning
> +	 * zero or to return -EIO, which the caller will treat as a
> +	 * nonfatal error (i.e. it doesn't prevent driver load, but
> +	 * marks the GPU as wedged until reset).
> +	 */
> +	if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1)
> +		ret = -EIO;
> +	else
> +		ret = 0;
> +
> +	if (i915.enable_guc_submission) {
> +		i915.enable_guc_submission = 0;
> +		DRM_INFO("GuC submission without firmware not supported\n");
> +		DRM_NOTE("Falling back from GuC submission to execlist mode\n");
> +	}
> +
> +	guc_interrupts_release(dev_priv);
> +	i915_guc_submission_disable(dev_priv);
> +	i915_guc_submission_fini(dev_priv);
> +
> +	return ret;
> +}
> +
>  /*
>   * Read GuC command/status register (SOFT_SCRATCH_0)
>   * Return true if it contains a response rather than a command
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index ec1a5b2..89c7e1f 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -28,6 +28,9 @@
>  #include "i915_guc_reg.h"
>  #include "intel_ringbuffer.h"
>
> +/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> +#define GUC_WA_HASH_CHECK_NOT_SET_ATTEPMTS 3
> +
>  struct drm_i915_gem_request;
>
>  /*
> @@ -171,6 +174,7 @@ struct intel_guc {
>  /* intel_uc.c */
>  void intel_uc_init_early(struct drm_i915_private *dev_priv);
>  void intel_uc_init(struct drm_i915_private *dev_priv);
> +int intel_uc_load(struct drm_i915_private *dev_priv);
>  bool intel_guc_recv(struct drm_i915_private *dev_priv, u32 *status);
>  int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len);
>  int intel_guc_sample_forcewake(struct intel_guc *guc);
> @@ -185,6 +189,10 @@ extern void intel_guc_fini(struct drm_i915_private *dev_priv);
>  extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status);
>  extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
>  extern int intel_guc_resume(struct drm_i915_private *dev_priv);
> +void guc_interrupts_release(struct drm_i915_private *dev_priv);
> +void guc_interrupts_capture(struct drm_i915_private *dev_priv);
> +int guc_hw_reset(struct drm_i915_private *dev_priv);
> +u32 guc_wopcm_size(struct drm_i915_private *dev_priv);
>
>  /* i915_guc_submission.c */
>  int i915_guc_submission_init(struct drm_i915_private *dev_priv);
>

I was just reading it so I can review 5/5 as you have asked me to look 
at that one.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] drm/i915/guc: Extract param logic form guc_init

[Arkadiusz Hiler]

Let intel_guc_init() focus on determining and fetching the correct
firmware.

This patch introduces intel_sanitize_uc_params() that is called from
intel_uc_init().

Then, if we have GuC, we can call intel_guc_init() conditionally and we
do not have to do internal checks.

It can be easily extended to support HuC case as well.

Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Jeff McGee <jeff.mcgee@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_loader.c | 19 +------------------
 drivers/gpu/drm/i915/intel_uc.c         | 23 ++++++++++++++++++++++-
 2 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index b76b556..0bb5fd1 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -617,7 +617,7 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
 }
 
 /**
- * intel_guc_init() - define parameters and fetch firmware
+ * intel_guc_init() - determine and fetch firmware
  * @dev_priv:	i915 device private
  *
  * Called early during driver load, but after GEM is initialised.
@@ -630,17 +630,6 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 	const char *fw_path;
 
-	if (!HAS_GUC(dev_priv)) {
-		i915.enable_guc_loading = 0;
-		i915.enable_guc_submission = 0;
-	} else {
-		/* A negative value means "use platform default" */
-		if (i915.enable_guc_loading < 0)
-			i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
-		if (i915.enable_guc_submission < 0)
-			i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
-	}
-
 	if (!HAS_GUC_UCODE(dev_priv)) {
 		fw_path = NULL;
 	} else if (IS_SKYLAKE(dev_priv)) {
@@ -663,13 +652,7 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
 	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
 	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
 
-	/* can't enable guc submission without guc */
-	if (!i915.enable_guc_loading)
-		i915.enable_guc_submission = 0;
-
 	/* Early (and silent) return if GuC loading is disabled */
-	if (!i915.enable_guc_loading)
-		return;
 	if (fw_path == NULL)
 		return;
 	if (*fw_path == '\0')
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 4e184edb..e72f784 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -25,6 +25,24 @@
 #include "i915_drv.h"
 #include "intel_uc.h"
 
+static void intel_sanitize_uc_params(struct drm_i915_private *dev_priv)
+{
+	if (!HAS_GUC(dev_priv)) {
+		i915.enable_guc_loading = 0;
+		i915.enable_guc_submission = 0;
+	} else {
+		/* A negative value means "use platform default" */
+		if (i915.enable_guc_loading < 0)
+			i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
+		if (i915.enable_guc_submission < 0)
+			i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
+	}
+
+	/* can't enable guc submission without guc */
+	if (!i915.enable_guc_loading)
+		i915.enable_guc_submission = 0;
+}
+
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
 {
 	mutex_init(&dev_priv->guc.send_mutex);
@@ -32,7 +50,10 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
 
 void intel_uc_init(struct drm_i915_private *dev_priv)
 {
-	intel_guc_init(dev_priv);
+	intel_sanitize_uc_params(dev_priv);
+
+	if (i915.enable_guc_loading)
+		intel_guc_init(dev_priv);
 }
 
 int intel_uc_load(struct drm_i915_private *dev_priv)
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915/guc: Extract param logic form guc_init

[Daniele Ceraolo Spurio]

On 15/12/16 07:47, Arkadiusz Hiler wrote:
> Let intel_guc_init() focus on determining and fetching the correct
> firmware.
>
> This patch introduces intel_sanitize_uc_params() that is called from
> intel_uc_init().
>
> Then, if we have GuC, we can call intel_guc_init() conditionally and we
> do not have to do internal checks.
>
> It can be easily extended to support HuC case as well.
>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Jeff McGee <jeff.mcgee@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_loader.c | 19 +------------------
>  drivers/gpu/drm/i915/intel_uc.c         | 23 ++++++++++++++++++++++-
>  2 files changed, 23 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index b76b556..0bb5fd1 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -617,7 +617,7 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
>  }
>
>  /**
> - * intel_guc_init() - define parameters and fetch firmware
> + * intel_guc_init() - determine and fetch firmware
>   * @dev_priv:	i915 device private
>   *
>   * Called early during driver load, but after GEM is initialised.
> @@ -630,17 +630,6 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
>  	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>  	const char *fw_path;
>
> -	if (!HAS_GUC(dev_priv)) {
> -		i915.enable_guc_loading = 0;
> -		i915.enable_guc_submission = 0;
> -	} else {
> -		/* A negative value means "use platform default" */
> -		if (i915.enable_guc_loading < 0)
> -			i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
> -		if (i915.enable_guc_submission < 0)
> -			i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
> -	}
> -
>  	if (!HAS_GUC_UCODE(dev_priv)) {
>  		fw_path = NULL;
>  	} else if (IS_SKYLAKE(dev_priv)) {
> @@ -663,13 +652,7 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
>  	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
>  	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
>
> -	/* can't enable guc submission without guc */
> -	if (!i915.enable_guc_loading)
> -		i915.enable_guc_submission = 0;
> -
>  	/* Early (and silent) return if GuC loading is disabled */
> -	if (!i915.enable_guc_loading)
> -		return;
>  	if (fw_path == NULL)
>  		return;
>  	if (*fw_path == '\0')
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 4e184edb..e72f784 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -25,6 +25,24 @@
>  #include "i915_drv.h"
>  #include "intel_uc.h"
>
> +static void intel_sanitize_uc_params(struct drm_i915_private *dev_priv)
> +{
> +	if (!HAS_GUC(dev_priv)) {
> +		i915.enable_guc_loading = 0;
> +		i915.enable_guc_submission = 0;
> +	} else {
> +		/* A negative value means "use platform default" */
> +		if (i915.enable_guc_loading < 0)
> +			i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
> +		if (i915.enable_guc_submission < 0)
> +			i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
> +	}
> +
> +	/* can't enable guc submission without guc */
> +	if (!i915.enable_guc_loading)
> +		i915.enable_guc_submission = 0;
> +}
> +
>  void intel_uc_init_early(struct drm_i915_private *dev_priv)
>  {
>  	mutex_init(&dev_priv->guc.send_mutex);
> @@ -32,7 +50,10 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
>
>  void intel_uc_init(struct drm_i915_private *dev_priv)
>  {
> -	intel_guc_init(dev_priv);
> +	intel_sanitize_uc_params(dev_priv);

If we place intel_sanitize_uc_params in intel_sanitize_options we can 
have the parameters resolved earlier and we should therefore be able to 
use them to better handle GuC-related differences during driver load.

Daniele

> +
> +	if (i915.enable_guc_loading)
> +		intel_guc_init(dev_priv);
>  }
>
>  int intel_uc_load(struct drm_i915_private *dev_priv)
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915/guc: Extract param logic form guc_init

[Arkadiusz Hiler]

On Fri, Dec 23, 2016 at 01:19:18PM -0800, Daniele Ceraolo Spurio wrote:
> 
> 
> On 15/12/16 07:47, Arkadiusz Hiler wrote:
> > Let intel_guc_init() focus on determining and fetching the correct
> > firmware.
> > 
> > This patch introduces intel_sanitize_uc_params() that is called from
> > intel_uc_init().
> > 
> > Then, if we have GuC, we can call intel_guc_init() conditionally and we
> > do not have to do internal checks.
> > 
> > It can be easily extended to support HuC case as well.
> > 
> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Cc: Jeff McGee <jeff.mcgee@intel.com>
> > Cc: Michal Winiarski <michal.winiarski@intel.com>
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_guc_loader.c | 19 +------------------
> >  drivers/gpu/drm/i915/intel_uc.c         | 23 ++++++++++++++++++++++-
> >  2 files changed, 23 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> > index b76b556..0bb5fd1 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> > @@ -617,7 +617,7 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
> >  }
> > 
> >  /**
> > - * intel_guc_init() - define parameters and fetch firmware
> > + * intel_guc_init() - determine and fetch firmware
> >   * @dev_priv:	i915 device private
> >   *
> >   * Called early during driver load, but after GEM is initialised.
> > @@ -630,17 +630,6 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
> >  	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> >  	const char *fw_path;
> > 
> > -	if (!HAS_GUC(dev_priv)) {
> > -		i915.enable_guc_loading = 0;
> > -		i915.enable_guc_submission = 0;
> > -	} else {
> > -		/* A negative value means "use platform default" */
> > -		if (i915.enable_guc_loading < 0)
> > -			i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
> > -		if (i915.enable_guc_submission < 0)
> > -			i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
> > -	}
> > -
> >  	if (!HAS_GUC_UCODE(dev_priv)) {
> >  		fw_path = NULL;
> >  	} else if (IS_SKYLAKE(dev_priv)) {
> > @@ -663,13 +652,7 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
> >  	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
> >  	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
> > 
> > -	/* can't enable guc submission without guc */
> > -	if (!i915.enable_guc_loading)
> > -		i915.enable_guc_submission = 0;
> > -
> >  	/* Early (and silent) return if GuC loading is disabled */
> > -	if (!i915.enable_guc_loading)
> > -		return;
> >  	if (fw_path == NULL)
> >  		return;
> >  	if (*fw_path == '\0')
> > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> > index 4e184edb..e72f784 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > @@ -25,6 +25,24 @@
> >  #include "i915_drv.h"
> >  #include "intel_uc.h"
> > 
> > +static void intel_sanitize_uc_params(struct drm_i915_private *dev_priv)
> > +{
> > +	if (!HAS_GUC(dev_priv)) {
> > +		i915.enable_guc_loading = 0;
> > +		i915.enable_guc_submission = 0;
> > +	} else {
> > +		/* A negative value means "use platform default" */
> > +		if (i915.enable_guc_loading < 0)
> > +			i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
> > +		if (i915.enable_guc_submission < 0)
> > +			i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
> > +	}
> > +
> > +	/* can't enable guc submission without guc */
> > +	if (!i915.enable_guc_loading)
> > +		i915.enable_guc_submission = 0;
> > +}
> > +
> >  void intel_uc_init_early(struct drm_i915_private *dev_priv)
> >  {
> >  	mutex_init(&dev_priv->guc.send_mutex);
> > @@ -32,7 +50,10 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
> > 
> >  void intel_uc_init(struct drm_i915_private *dev_priv)
> >  {
> > -	intel_guc_init(dev_priv);
> > +	intel_sanitize_uc_params(dev_priv);
> 
> If we place intel_sanitize_uc_params in intel_sanitize_options we can have
> the parameters resolved earlier and we should therefore be able to use them
> to better handle GuC-related differences during driver load.
> 
> Daniele

Agree. I haven't noticed the intel_sanitize_options().

> 
> > +
> > +	if (i915.enable_guc_loading)
> > +		intel_guc_init(dev_priv);
> >  }
> > 
> >  int intel_uc_load(struct drm_i915_private *dev_priv)
> > 

-- 
Cheers,
Arek
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/5] drm/i915/guc: Simplify guc_fw_path

[Arkadiusz Hiler]

Currently guc_fw_path values can represent one of three possible states:

 1) NULL - device without GuC
 2) '\0' - device with GuC but no known firmware
 3) else - device with GuC and known firmware

Second case is used only to WARN at the later stage.

We can WARN right away and merge cases 1 and 2 for later handling.

Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Jeff McGee <jeff.mcgee@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_loader.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 0bb5fd1..075a103 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -460,12 +460,8 @@ int intel_guc_load(struct drm_i915_private *dev_priv)
 		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
 
 	if (fw_path == NULL) {
-		/* Device is known to have no uCode (e.g. no GuC) */
+		/* We do not have uCode for the device */
 		return -ENXIO;
-	} else if (*fw_path == '\0') {
-		/* Device has a GuC but we don't know what f/w to load? */
-		WARN(1, "No GuC firmware known for this platform!\n");
-		return -ENODEV;
 	}
 
 	/* Fetch failed, or already fetched but failed to load? */
@@ -628,11 +624,9 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
 void intel_guc_init(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
-	const char *fw_path;
+	const char *fw_path = NULL;
 
-	if (!HAS_GUC_UCODE(dev_priv)) {
-		fw_path = NULL;
-	} else if (IS_SKYLAKE(dev_priv)) {
+	if (IS_SKYLAKE(dev_priv)) {
 		fw_path = I915_SKL_GUC_UCODE;
 		guc_fw->guc_fw_major_wanted = SKL_FW_MAJOR;
 		guc_fw->guc_fw_minor_wanted = SKL_FW_MINOR;
@@ -644,24 +638,22 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
 		fw_path = I915_KBL_GUC_UCODE;
 		guc_fw->guc_fw_major_wanted = KBL_FW_MAJOR;
 		guc_fw->guc_fw_minor_wanted = KBL_FW_MINOR;
-	} else {
-		fw_path = "";	/* unknown device */
+	} else if (HAS_GUC_UCODE(dev_priv)) {
+		WARN(1, "No GuC firmware known for platform with GuC!\n");
 	}
 
 	guc_fw->guc_fw_path = fw_path;
 	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
 	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
 
-	/* Early (and silent) return if GuC loading is disabled */
+	/* Early return if we do not have firmware to fetch */
 	if (fw_path == NULL)
 		return;
-	if (*fw_path == '\0')
-		return;
 
 	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_PENDING;
 	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
+
 	guc_fw_fetch(dev_priv, guc_fw);
-	/* status must now be FAIL or SUCCESS */
 }
 
 /**
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915/guc: Simplify guc_fw_path

[Tvrtko Ursulin]

On 15/12/2016 15:47, Arkadiusz Hiler wrote:
> Currently guc_fw_path values can represent one of three possible states:
>
>  1) NULL - device without GuC
>  2) '\0' - device with GuC but no known firmware
>  3) else - device with GuC and known firmware
>
> Second case is used only to WARN at the later stage.
>
> We can WARN right away and merge cases 1 and 2 for later handling.
>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Jeff McGee <jeff.mcgee@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_loader.c | 22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 0bb5fd1..075a103 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -460,12 +460,8 @@ int intel_guc_load(struct drm_i915_private *dev_priv)
>  		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
>
>  	if (fw_path == NULL) {
> -		/* Device is known to have no uCode (e.g. no GuC) */
> +		/* We do not have uCode for the device */
>  		return -ENXIO;
> -	} else if (*fw_path == '\0') {
> -		/* Device has a GuC but we don't know what f/w to load? */
> -		WARN(1, "No GuC firmware known for this platform!\n");
> -		return -ENODEV;
>  	}
>
>  	/* Fetch failed, or already fetched but failed to load? */
> @@ -628,11 +624,9 @@ static void guc_fw_fetch(struct drm_i915_private *dev_priv,
>  void intel_guc_init(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> -	const char *fw_path;
> +	const char *fw_path = NULL;
>
> -	if (!HAS_GUC_UCODE(dev_priv)) {
> -		fw_path = NULL;
> -	} else if (IS_SKYLAKE(dev_priv)) {
> +	if (IS_SKYLAKE(dev_priv)) {
>  		fw_path = I915_SKL_GUC_UCODE;
>  		guc_fw->guc_fw_major_wanted = SKL_FW_MAJOR;
>  		guc_fw->guc_fw_minor_wanted = SKL_FW_MINOR;
> @@ -644,24 +638,22 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
>  		fw_path = I915_KBL_GUC_UCODE;
>  		guc_fw->guc_fw_major_wanted = KBL_FW_MAJOR;
>  		guc_fw->guc_fw_minor_wanted = KBL_FW_MINOR;
> -	} else {
> -		fw_path = "";	/* unknown device */
> +	} else if (HAS_GUC_UCODE(dev_priv)) {
> +		WARN(1, "No GuC firmware known for platform with GuC!\n");

I think you should also set i915.enable_guc_loading to zero here to 
avoid the later stages bothering with partial setup and cleanup.

intel_uc_load specifically I think would with this patch run some setup 
then immediately fail and have to unwind.

Regards,

Tvrtko

>  	}
>
>  	guc_fw->guc_fw_path = fw_path;
>  	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
>  	guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
>
> -	/* Early (and silent) return if GuC loading is disabled */
> +	/* Early return if we do not have firmware to fetch */
>  	if (fw_path == NULL)
>  		return;
> -	if (*fw_path == '\0')
> -		return;
>
>  	guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_PENDING;
>  	DRM_DEBUG_DRIVER("GuC firmware pending, path %s\n", fw_path);
> +
>  	guc_fw_fetch(dev_priv, guc_fw);
> -	/* status must now be FAIL or SUCCESS */
>  }
>
>  /**
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for GuC Scrub vol. 1

[Patchwork]

== Series Details ==

Series: GuC Scrub vol. 1
URL   : https://patchwork.freedesktop.org/series/16856/
State : failure

== Summary ==

Series 16856v1 GuC Scrub vol. 1
https://patchwork.freedesktop.org/api/1.0/series/16856/revisions/1/mbox/

Test drv_module_reload:
        Subgroup basic-reload-inject:
                pass       -> INCOMPLETE (fi-kbl-7500u)
Test kms_pipe_crc_basic:
        Subgroup bad-nb-words-1:
                dmesg-warn -> PASS       (fi-snb-2520m)
        Subgroup suspend-read-crc-pipe-a:
                skip       -> PASS       (fi-bxt-j4205)

fi-bdw-5557u     total:247  pass:233  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:247  pass:208  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:247  pass:223  dwarn:0   dfail:0   fail:0   skip:24 
fi-bxt-t5700     total:247  pass:220  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-j1900     total:247  pass:220  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-ilk-650       total:247  pass:195  dwarn:0   dfail:0   fail:0   skip:52 
fi-ivb-3520m     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:7    pass:6    dwarn:0   dfail:0   fail:0   skip:0  
fi-skl-6260u     total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:247  pass:227  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:247  pass:224  dwarn:3   dfail:0   fail:0   skip:20 
fi-skl-6770hq    total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:247  pass:215  dwarn:0   dfail:0   fail:0   skip:32 

a16fc233af5c80d1ed23178c94b7fbb77e48ffe0 drm-tip: 2016y-12m-15d-15h-21m-31s UTC integration manifest
056221b drm/i915/guc: Simplify guc_fw_path
6b29b79 drm/i915/guc: Extract param logic form guc_init
16d3647 drm/i915/guc: Simplify intel_guc_load()
b4d985a drm/i915/guc: Introduce intel_uc_init()
7d2e815 drm/i915/guc: Rename _setup() to _load()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3298/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx