| Message ID | 1479948758-4553-1-git-send-email-anusha.srivatsa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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.
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
>-----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
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
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;
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(-)