drm/i915: Always load guc by default.
diff mbox

Message ID 1479948758-4553-1-git-send-email-anusha.srivatsa@intel.com
State New
Headers show

Commit Message

Srivatsa, Anusha Nov. 24, 2016, 12:52 a.m. UTC
Remove the enable_guc_loading parameter. Load the GuC on
plaforms that have GuC. All issues we found so far are related
to GuC features like the command submission, but no bug is related
to the guc loading itself.

This addresses the case when we need GuC loaded even with no GuC feature
in use, like - GuC  authenticating HuC loading.

If we need to debug GuC we can do so by removing the firmware from
the rootfs instead of disabling with a parameter. So besides enabling
guc by default this patch also kill the use of the parameter for
loading.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/i915_params.c      |  6 ------
 drivers/gpu/drm/i915/i915_params.h      |  1 -
 drivers/gpu/drm/i915/intel_guc_loader.c | 19 ++++++-------------
 3 files changed, 6 insertions(+), 20 deletions(-)

Comments

Chris Wilson Nov. 24, 2016, 7:13 a.m. UTC | #1
On Wed, Nov 23, 2016 at 04:52:38PM -0800, Anusha Srivatsa wrote:
> Remove the enable_guc_loading parameter. Load the GuC on
> plaforms that have GuC. All issues we found so far are related
> to GuC features like the command submission, but no bug is related
> to the guc loading itself.
> 
> This addresses the case when we need GuC loaded even with no GuC feature
> in use, like - GuC  authenticating HuC loading.

Why not just load the firmware if it may be used?
-Chris
Tvrtko Ursulin Nov. 24, 2016, 8:15 a.m. UTC | #2
On 24/11/2016 07:13, Chris Wilson wrote:
> On Wed, Nov 23, 2016 at 04:52:38PM -0800, Anusha Srivatsa wrote:
>> Remove the enable_guc_loading parameter. Load the GuC on
>> plaforms that have GuC. All issues we found so far are related
>> to GuC features like the command submission, but no bug is related
>> to the guc loading itself.
>>
>> This addresses the case when we need GuC loaded even with no GuC feature
>> in use, like - GuC  authenticating HuC loading.
>
> Why not just load the firmware if it may be used?

It was discussed briefly in the other thread, but I suppose as soon as 
the HuC patches go in that would be always so it may not be that useful.

Unless there is a reason to add a HuC enable/disable parameter in 
general. I have no idea on that one.

Regards,

Tvrtko
Chris Wilson Nov. 24, 2016, 8:21 a.m. UTC | #3
On Thu, Nov 24, 2016 at 08:15:31AM +0000, Tvrtko Ursulin wrote:
> 
> On 24/11/2016 07:13, Chris Wilson wrote:
> >On Wed, Nov 23, 2016 at 04:52:38PM -0800, Anusha Srivatsa wrote:
> >>Remove the enable_guc_loading parameter. Load the GuC on
> >>plaforms that have GuC. All issues we found so far are related
> >>to GuC features like the command submission, but no bug is related
> >>to the guc loading itself.
> >>
> >>This addresses the case when we need GuC loaded even with no GuC feature
> >>in use, like - GuC  authenticating HuC loading.
> >
> >Why not just load the firmware if it may be used?
> 
> It was discussed briefly in the other thread, but I suppose as soon
> as the HuC patches go in that would be always so it may not be that
> useful.
> 
> Unless there is a reason to add a HuC enable/disable parameter in
> general. I have no idea on that one.

In hindsight, we should have had i915.enable_dmc to easily protect users
against failures. History says we will regret enabling a new piece of
hw/fw without a feature option.
-chris
Tvrtko Ursulin Nov. 24, 2016, 8:31 a.m. UTC | #4
On 24/11/2016 08:21, Chris Wilson wrote:
> On Thu, Nov 24, 2016 at 08:15:31AM +0000, Tvrtko Ursulin wrote:
>>
>> On 24/11/2016 07:13, Chris Wilson wrote:
>>> On Wed, Nov 23, 2016 at 04:52:38PM -0800, Anusha Srivatsa wrote:
>>>> Remove the enable_guc_loading parameter. Load the GuC on
>>>> plaforms that have GuC. All issues we found so far are related
>>>> to GuC features like the command submission, but no bug is related
>>>> to the guc loading itself.
>>>>
>>>> This addresses the case when we need GuC loaded even with no GuC feature
>>>> in use, like - GuC  authenticating HuC loading.
>>>
>>> Why not just load the firmware if it may be used?
>>
>> It was discussed briefly in the other thread, but I suppose as soon
>> as the HuC patches go in that would be always so it may not be that
>> useful.
>>
>> Unless there is a reason to add a HuC enable/disable parameter in
>> general. I have no idea on that one.
>
> In hindsight, we should have had i915.enable_dmc to easily protect users
> against failures. History says we will regret enabling a new piece of
> hw/fw without a feature option.

So..

  1. Add i915.enable_huc, default to enabled
  2. Unexport i915.enable_guc_loading
  3. Gate enable_guc_loading by i915.enable_huc and 
i915.enable_guc_submission

?

Regards,

Tvrtko
Jani Nikula Nov. 24, 2016, 8:31 a.m. UTC | #5
On Thu, 24 Nov 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> History says we will regret enabling a new piece of hw/fw without a
> feature option.

And history says we'll regret adding new module parameters for
everything. Lose-lose. :(

BR,
Jani.
Chris Wilson Nov. 24, 2016, 8:41 a.m. UTC | #6
On Thu, Nov 24, 2016 at 08:31:22AM +0000, Tvrtko Ursulin wrote:
> 
> On 24/11/2016 08:21, Chris Wilson wrote:
> >On Thu, Nov 24, 2016 at 08:15:31AM +0000, Tvrtko Ursulin wrote:
> >>
> >>On 24/11/2016 07:13, Chris Wilson wrote:
> >>>On Wed, Nov 23, 2016 at 04:52:38PM -0800, Anusha Srivatsa wrote:
> >>>>Remove the enable_guc_loading parameter. Load the GuC on
> >>>>plaforms that have GuC. All issues we found so far are related
> >>>>to GuC features like the command submission, but no bug is related
> >>>>to the guc loading itself.
> >>>>
> >>>>This addresses the case when we need GuC loaded even with no GuC feature
> >>>>in use, like - GuC  authenticating HuC loading.
> >>>
> >>>Why not just load the firmware if it may be used?
> >>
> >>It was discussed briefly in the other thread, but I suppose as soon
> >>as the HuC patches go in that would be always so it may not be that
> >>useful.
> >>
> >>Unless there is a reason to add a HuC enable/disable parameter in
> >>general. I have no idea on that one.
> >
> >In hindsight, we should have had i915.enable_dmc to easily protect users
> >against failures. History says we will regret enabling a new piece of
> >hw/fw without a feature option.
> 
> So..
> 
>  1. Add i915.enable_huc, default to enabled
>  2. Unexport i915.enable_guc_loading
>  3. Gate enable_guc_loading by i915.enable_huc and
> i915.enable_guc_submission

Aye, that would be my preference.
-Chris
Srivatsa, Anusha Dec. 2, 2016, 6:11 p.m. UTC | #7
>-----Original Message-----
>From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
>Sent: Thursday, November 24, 2016 12:41 AM
>To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel-
>gfx@lists.freedesktop.org; Mcgee, Jeff <jeff.mcgee@intel.com>
>Subject: Re: [Intel-gfx] [PATCH] drm/i915: Always load guc by default.
>
>On Thu, Nov 24, 2016 at 08:31:22AM +0000, Tvrtko Ursulin wrote:
>>
>> On 24/11/2016 08:21, Chris Wilson wrote:
>> >On Thu, Nov 24, 2016 at 08:15:31AM +0000, Tvrtko Ursulin wrote:
>> >>
>> >>On 24/11/2016 07:13, Chris Wilson wrote:
>> >>>On Wed, Nov 23, 2016 at 04:52:38PM -0800, Anusha Srivatsa wrote:
>> >>>>Remove the enable_guc_loading parameter. Load the GuC on plaforms
>> >>>>that have GuC. All issues we found so far are related to GuC
>> >>>>features like the command submission, but no bug is related to the
>> >>>>guc loading itself.
>> >>>>
>> >>>>This addresses the case when we need GuC loaded even with no GuC
>> >>>>feature in use, like - GuC  authenticating HuC loading.
>> >>>
>> >>>Why not just load the firmware if it may be used?
>> >>
>> >>It was discussed briefly in the other thread, but I suppose as soon
>> >>as the HuC patches go in that would be always so it may not be that
>> >>useful.
>> >>
>> >>Unless there is a reason to add a HuC enable/disable parameter in
>> >>general. I have no idea on that one.
>> >
>> >In hindsight, we should have had i915.enable_dmc to easily protect
>> >users against failures. History says we will regret enabling a new
>> >piece of hw/fw without a feature option.
>>
>> So..
>>
>>  1. Add i915.enable_huc, default to enabled  2. Unexport
>> i915.enable_guc_loading  3. Gate enable_guc_loading by i915.enable_huc
>> and i915.enable_guc_submission
>
>Aye, that would be my preference.
>-Chris

So, basically control the guc loads depending on huc_enable or guc_submission parameter. If either or both are enabled then load guc.
No need to load guc if a platform has one? 
Any thought on having 0,-1,1 and 2 values for submission? 1 is load if guc is available (but do not error out if not present) and 2 is load and fail if not present. I am thinking what if we need that differentiation.....

-Anusha
>--
>Chris Wilson, Intel Open Source Technology Centre
Chris Wilson Dec. 3, 2016, 4:39 p.m. UTC | #8
On Fri, Dec 02, 2016 at 06:11:12PM +0000, Srivatsa, Anusha wrote:
> 
> 
> >-----Original Message-----
> >From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> >Sent: Thursday, November 24, 2016 12:41 AM
> >To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> >Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel-
> >gfx@lists.freedesktop.org; Mcgee, Jeff <jeff.mcgee@intel.com>
> >Subject: Re: [Intel-gfx] [PATCH] drm/i915: Always load guc by default.
> >
> >On Thu, Nov 24, 2016 at 08:31:22AM +0000, Tvrtko Ursulin wrote:
> >>
> >> On 24/11/2016 08:21, Chris Wilson wrote:
> >> >On Thu, Nov 24, 2016 at 08:15:31AM +0000, Tvrtko Ursulin wrote:
> >> >>
> >> >>On 24/11/2016 07:13, Chris Wilson wrote:
> >> >>>On Wed, Nov 23, 2016 at 04:52:38PM -0800, Anusha Srivatsa wrote:
> >> >>>>Remove the enable_guc_loading parameter. Load the GuC on plaforms
> >> >>>>that have GuC. All issues we found so far are related to GuC
> >> >>>>features like the command submission, but no bug is related to the
> >> >>>>guc loading itself.
> >> >>>>
> >> >>>>This addresses the case when we need GuC loaded even with no GuC
> >> >>>>feature in use, like - GuC  authenticating HuC loading.
> >> >>>
> >> >>>Why not just load the firmware if it may be used?
> >> >>
> >> >>It was discussed briefly in the other thread, but I suppose as soon
> >> >>as the HuC patches go in that would be always so it may not be that
> >> >>useful.
> >> >>
> >> >>Unless there is a reason to add a HuC enable/disable parameter in
> >> >>general. I have no idea on that one.
> >> >
> >> >In hindsight, we should have had i915.enable_dmc to easily protect
> >> >users against failures. History says we will regret enabling a new
> >> >piece of hw/fw without a feature option.
> >>
> >> So..
> >>
> >>  1. Add i915.enable_huc, default to enabled  2. Unexport
> >> i915.enable_guc_loading  3. Gate enable_guc_loading by i915.enable_huc
> >> and i915.enable_guc_submission
> >
> >Aye, that would be my preference.
> >-Chris
> 
> So, basically control the guc loads depending on huc_enable or guc_submission parameter. If either or both are enabled then load guc.
> No need to load guc if a platform has one? 
> Any thought on having 0,-1,1 and 2 values for submission? 1 is load if guc is available (but do not error out if not present) and 2 is load and fail if not present. I am thinking what if we need that differentiation.....

I have no idea what value 2 has for anybody. Userspace can check whether
or not guc submission is enabled and fail validation without resorting to
the kernel wedging the machine.
-Chris

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index d46ffe7..a8011f2 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -56,7 +56,6 @@  struct i915_params i915 __read_mostly = {
 	.verbose_state_checks = 1,
 	.nuclear_pageflip = 0,
 	.edp_vswing = 0,
-	.enable_guc_loading = 0,
 	.enable_guc_submission = 0,
 	.guc_log_level = -1,
 	.enable_dp_mst = true,
@@ -216,11 +215,6 @@  MODULE_PARM_DESC(edp_vswing,
 		 "(0=use value from vbt [default], 1=low power swing(200mV),"
 		 "2=default swing(400mV))");
 
-module_param_named_unsafe(enable_guc_loading, i915.enable_guc_loading, int, 0400);
-MODULE_PARM_DESC(enable_guc_loading,
-		"Enable GuC firmware loading "
-		"(-1=auto, 0=never [default], 1=if available, 2=required)");
-
 module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, int, 0400);
 MODULE_PARM_DESC(enable_guc_submission,
 		"Enable GuC submission "
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 817ad95..4b7529a 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -45,7 +45,6 @@  struct i915_params {
 	int enable_ips;
 	int invert_brightness;
 	int enable_cmd_parser;
-	int enable_guc_loading;
 	int enable_guc_submission;
 	int guc_log_level;
 	int use_mmio_flip;
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 6946311..d48dc73 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -460,11 +460,9 @@  int intel_guc_setup(struct drm_device *dev)
 		fw_path,
 		intel_uc_fw_status_repr(guc_fw->fetch_status),
 		intel_uc_fw_status_repr(guc_fw->load_status));
-
-	/* Loading forbidden, or no firmware to load? */
-	if (!i915.enable_guc_loading) {
-		err = 0;
-		goto fail;
+	if (!HAS_GUC(dev_priv)) { 
+		/* Platform does not have a GuC */	
+		return;
 	} else if (fw_path == NULL) {
 		/* Device is known to have no uCode (e.g. no GuC) */
 		err = -ENXIO;
@@ -562,9 +560,8 @@  int intel_guc_setup(struct drm_device *dev)
 	 * 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) {
+
+	if (i915.enable_guc_submission > 1) {
 		ret = -EIO;
 	} else {
 		ret = 0;
@@ -745,12 +742,9 @@  void intel_guc_init(struct drm_device *dev)
 	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);
 	}
@@ -778,8 +772,7 @@  void intel_guc_init(struct drm_device *dev)
 	guc_fw->fetch_status = UC_FIRMWARE_NONE;
 	guc_fw->load_status = UC_FIRMWARE_NONE;
 
-	/* Early (and silent) return if GuC loading is disabled */
-	if (!i915.enable_guc_loading)
+	if(!HAS_GUC(dev_priv))
 		return;
 	if (fw_path == NULL)
 		return;