| Message ID | 20170307152500.6760-11-arkadiusz.hiler@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>-----Original Message----- >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of >Arkadiusz Hiler >Sent: Tuesday, March 7, 2017 7:25 AM >To: intel-gfx@lists.freedesktop.org >Subject: [Intel-gfx] [PATCH 10/10] drm/i915/uc: Add params for specifying >firmware > >`guc_firmware_path` and `huc_firmware_path` module parameters are added. > >Using the parameter disables version checks and loads desired firmware instead >of the default one. I see that the effort of this patch makes us test with different firmware versions and not just the default one. But is it worth introducing two new params ? We already have 3 parameters that are guc and huc related. Anusha >v2: make params unsafe && notice about disabled fw check (J. Lahtinen) > >Cc: Chris Wilson <chris@chris-wilson.co.uk> >Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >Cc: Michal Winiarski <michal.winiarski@intel.com> >Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> >--- > drivers/gpu/drm/i915/i915_params.c | 10 ++++++++++ > drivers/gpu/drm/i915/i915_params.h | 2 ++ > drivers/gpu/drm/i915/intel_guc_loader.c | 6 +++++- > drivers/gpu/drm/i915/intel_huc.c | 6 +++++- > drivers/gpu/drm/i915/intel_uc.c | 6 ++++-- > 5 files changed, 26 insertions(+), 4 deletions(-) > >diff --git a/drivers/gpu/drm/i915/i915_params.c >b/drivers/gpu/drm/i915/i915_params.c >index 2e9645e..b6a7e36 100644 >--- a/drivers/gpu/drm/i915/i915_params.c >+++ b/drivers/gpu/drm/i915/i915_params.c >@@ -59,6 +59,8 @@ struct i915_params i915 __read_mostly = { > .enable_guc_loading = 0, > .enable_guc_submission = 0, > .guc_log_level = -1, >+ .guc_firmware_path = NULL, >+ .huc_firmware_path = NULL, > .enable_dp_mst = true, > .inject_load_failure = 0, > .enable_dpcd_backlight = false, >@@ -230,6 +232,14 @@ module_param_named(guc_log_level, >i915.guc_log_level, int, 0400); MODULE_PARM_DESC(guc_log_level, > "GuC firmware logging level (-1:disabled (default), 0-3:enabled)"); > >+module_param_named_unsafe(guc_firmware_path, i915.guc_firmware_path, >+charp, 0400); MODULE_PARM_DESC(guc_firmware_path, >+ "GuC firmware path to use instead of the default one"); >+ >+module_param_named_unsafe(huc_firmware_path, i915.huc_firmware_path, >+charp, 0400); MODULE_PARM_DESC(huc_firmware_path, >+ "HuC firmware path to use instead of the default one"); >+ > module_param_named_unsafe(enable_dp_mst, i915.enable_dp_mst, bool, >0600); MODULE_PARM_DESC(enable_dp_mst, > "Enable multi-stream transport (MST) for new DisplayPort sinks. (default: >true)"); diff --git a/drivers/gpu/drm/i915/i915_params.h >b/drivers/gpu/drm/i915/i915_params.h >index 55d47ee..34148cc 100644 >--- a/drivers/gpu/drm/i915/i915_params.h >+++ b/drivers/gpu/drm/i915/i915_params.h >@@ -46,6 +46,8 @@ > func(int, enable_guc_loading); \ > func(int, enable_guc_submission); \ > func(int, guc_log_level); \ >+ func(char *, guc_firmware_path); \ >+ func(char *, huc_firmware_path); \ > func(int, use_mmio_flip); \ > func(int, mmio_debug); \ > func(int, edp_vswing); \ >diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c >b/drivers/gpu/drm/i915/intel_guc_loader.c >index 0478483..283b0ca 100644 >--- a/drivers/gpu/drm/i915/intel_guc_loader.c >+++ b/drivers/gpu/drm/i915/intel_guc_loader.c >@@ -474,7 +474,11 @@ int intel_guc_select_fw(struct intel_guc *guc) > guc->fw.load_status = INTEL_UC_FIRMWARE_NONE; > guc->fw.fw = INTEL_UC_FW_TYPE_GUC; > >- if (IS_SKYLAKE(dev_priv)) { >+ if (i915.guc_firmware_path) { >+ guc->fw.path = i915.guc_firmware_path; >+ guc->fw.major_ver_wanted = 0; >+ guc->fw.minor_ver_wanted = 0; >+ } else if (IS_SKYLAKE(dev_priv)) { > guc->fw.path = I915_SKL_GUC_UCODE; > guc->fw.major_ver_wanted = SKL_FW_MAJOR; > guc->fw.minor_ver_wanted = SKL_FW_MINOR; diff --git >a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c >index ea67abc..ab4ee08 100644 >--- a/drivers/gpu/drm/i915/intel_huc.c >+++ b/drivers/gpu/drm/i915/intel_huc.c >@@ -153,7 +153,11 @@ void intel_huc_select_fw(struct intel_huc *huc) > huc->fw.load_status = INTEL_UC_FIRMWARE_NONE; > huc->fw.fw = INTEL_UC_FW_TYPE_HUC; > >- if (IS_SKYLAKE(dev_priv)) { >+ if (i915.huc_firmware_path) { >+ huc->fw.path = i915.huc_firmware_path; >+ huc->fw.major_ver_wanted = 0; >+ huc->fw.minor_ver_wanted = 0; >+ } else if (IS_SKYLAKE(dev_priv)) { > huc->fw.path = I915_SKL_HUC_UCODE; > huc->fw.major_ver_wanted = SKL_HUC_FW_MAJOR; > huc->fw.minor_ver_wanted = SKL_HUC_FW_MINOR; diff --git >a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index >8875647..1cf4590 100644 >--- a/drivers/gpu/drm/i915/intel_uc.c >+++ b/drivers/gpu/drm/i915/intel_uc.c >@@ -359,8 +359,10 @@ void intel_uc_prepare_fw(struct drm_i915_private >*dev_priv, > goto fail; > } > >- if (uc_fw->major_ver_found != uc_fw->major_ver_wanted || >- uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) { >+ if (uc_fw->major_ver_wanted == 0 && uc_fw->minor_ver_wanted == 0) { >+ DRM_NOTE("Skipping uC firmware version check\n"); >+ } else if (uc_fw->major_ver_found != uc_fw->major_ver_wanted || >+ uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) { > DRM_NOTE("uC firmware version %d.%d, required %d.%d\n", > uc_fw->major_ver_found, uc_fw->minor_ver_found, > uc_fw->major_ver_wanted, uc_fw- >>minor_ver_wanted); >-- >2.9.3 > >_______________________________________________ >Intel-gfx mailing list >Intel-gfx@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 08 Mar 2017, "Srivatsa, Anusha" <anusha.srivatsa@intel.com> wrote: >>-----Original Message----- >>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of >>Arkadiusz Hiler >>Sent: Tuesday, March 7, 2017 7:25 AM >>To: intel-gfx@lists.freedesktop.org >>Subject: [Intel-gfx] [PATCH 10/10] drm/i915/uc: Add params for specifying >>firmware >> >>`guc_firmware_path` and `huc_firmware_path` module parameters are added. >> >>Using the parameter disables version checks and loads desired firmware instead >>of the default one. > > I see that the effort of this patch makes us test with different > firmware versions and not just the default one. But is it worth > introducing two new params ? We already have 3 parameters that are guc > and huc related. Obviously I'd prefer there were fewer module parameters, but looks like they multiply like rabbits... Back when we decided that we only accept one firmware version, there were complaints about it becoming hard to test various firmware versions or to bisect the kernel while keeping the firmware constant. This addresses those issues. If you decide those are non-issues and the patch is not needed, I'll point whoever complains about the issues to this discussion. >>- if (uc_fw->major_ver_found != uc_fw->major_ver_wanted || >>- uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) { >>+ if (uc_fw->major_ver_wanted == 0 && uc_fw->minor_ver_wanted == 0) { >>+ DRM_NOTE("Skipping uC firmware version check\n"); Log the version found in the firmware? Or does that happens somewhere else already? BR, Jani.
On Wed, Mar 08, 2017 at 02:19:48AM +0100, Srivatsa, Anusha wrote: > > > >-----Original Message----- > >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of > >Arkadiusz Hiler > >Sent: Tuesday, March 7, 2017 7:25 AM > >To: intel-gfx@lists.freedesktop.org > >Subject: [Intel-gfx] [PATCH 10/10] drm/i915/uc: Add params for specifying > >firmware > > > >`guc_firmware_path` and `huc_firmware_path` module parameters are added. > > > >Using the parameter disables version checks and loads desired firmware instead > >of the default one. > > I see that the effort of this patch makes us test with different > firmware versions and not just the default one. But is it worth > introducing two new params ? We already have 3 parameters that are guc > and huc related. > > Anusha Hey, Having a mean to easily point to any binary you want to try out helps with testing and verification, without the need to do in-kernel changes. This param was suggested by Chris, and I've seen couple of similar patches by different people in their trees - I've used one myself. Since it seem so common, why not have it in the mainline? It's _unsafe anyway.
On Wed, Mar 08, 2017 at 11:23:36AM +0200, Jani Nikula wrote: > On Wed, 08 Mar 2017, "Srivatsa, Anusha" <anusha.srivatsa@intel.com> wrote: > >>-----Original Message----- > >>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of > >>Arkadiusz Hiler > >>Sent: Tuesday, March 7, 2017 7:25 AM > >>To: intel-gfx@lists.freedesktop.org > >>Subject: [Intel-gfx] [PATCH 10/10] drm/i915/uc: Add params for specifying > >>firmware > >> > >>`guc_firmware_path` and `huc_firmware_path` module parameters are added. > >> > >>Using the parameter disables version checks and loads desired firmware instead > >>of the default one. > > > > I see that the effort of this patch makes us test with different > > firmware versions and not just the default one. But is it worth > > introducing two new params ? We already have 3 parameters that are guc > > and huc related. > > Obviously I'd prefer there were fewer module parameters, but looks like > they multiply like rabbits... > > Back when we decided that we only accept one firmware version, there > were complaints about it becoming hard to test various firmware versions > or to bisect the kernel while keeping the firmware constant. This > addresses those issues. If you decide those are non-issues and the patch > is not needed, I'll point whoever complains about the issues to this > discussion. > > >>- if (uc_fw->major_ver_found != uc_fw->major_ver_wanted || > >>- uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) { > >>+ if (uc_fw->major_ver_wanted == 0 && uc_fw->minor_ver_wanted == 0) { > >>+ DRM_NOTE("Skipping uC firmware version check\n"); > > Log the version found in the firmware? Or does that happens somewhere > else already? It's logging the version couple lines down using DRM_DEBUG_DRIVER(). The log in the "else if" is there because we are reporting error and then returning. This function requires general cleanup, especially when it comes to the log messages. I'll do it after this series lands (it's starting to go out of hand already), along with other fixes, using targeted and independent patches.
On ke, 2017-03-08 at 11:02 +0100, Arkadiusz Hiler wrote: > > Having a mean to easily point to any binary you want to try out helps > with testing and verification, without the need to do in-kernel changes. > > This param was suggested by Chris, and I've seen couple of similar > patches by different people in their trees - I've used one myself. Since > it seem so common, why not have it in the mainline? We definitely need this, there's no doubt at this point. We'll have to be able to provide test versions of the GuC firmware so we can triage problems during our own or customer debug sessions. Regards, Joonas
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 2e9645e..b6a7e36 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -59,6 +59,8 @@ struct i915_params i915 __read_mostly = { .enable_guc_loading = 0, .enable_guc_submission = 0, .guc_log_level = -1, + .guc_firmware_path = NULL, + .huc_firmware_path = NULL, .enable_dp_mst = true, .inject_load_failure = 0, .enable_dpcd_backlight = false, @@ -230,6 +232,14 @@ module_param_named(guc_log_level, i915.guc_log_level, int, 0400); MODULE_PARM_DESC(guc_log_level, "GuC firmware logging level (-1:disabled (default), 0-3:enabled)"); +module_param_named_unsafe(guc_firmware_path, i915.guc_firmware_path, charp, 0400); +MODULE_PARM_DESC(guc_firmware_path, + "GuC firmware path to use instead of the default one"); + +module_param_named_unsafe(huc_firmware_path, i915.huc_firmware_path, charp, 0400); +MODULE_PARM_DESC(huc_firmware_path, + "HuC firmware path to use instead of the default one"); + module_param_named_unsafe(enable_dp_mst, i915.enable_dp_mst, bool, 0600); MODULE_PARM_DESC(enable_dp_mst, "Enable multi-stream transport (MST) for new DisplayPort sinks. (default: true)"); diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index 55d47ee..34148cc 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -46,6 +46,8 @@ func(int, enable_guc_loading); \ func(int, enable_guc_submission); \ func(int, guc_log_level); \ + func(char *, guc_firmware_path); \ + func(char *, huc_firmware_path); \ func(int, use_mmio_flip); \ func(int, mmio_debug); \ func(int, edp_vswing); \ diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 0478483..283b0ca 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -474,7 +474,11 @@ int intel_guc_select_fw(struct intel_guc *guc) guc->fw.load_status = INTEL_UC_FIRMWARE_NONE; guc->fw.fw = INTEL_UC_FW_TYPE_GUC; - if (IS_SKYLAKE(dev_priv)) { + if (i915.guc_firmware_path) { + guc->fw.path = i915.guc_firmware_path; + guc->fw.major_ver_wanted = 0; + guc->fw.minor_ver_wanted = 0; + } else if (IS_SKYLAKE(dev_priv)) { guc->fw.path = I915_SKL_GUC_UCODE; guc->fw.major_ver_wanted = SKL_FW_MAJOR; guc->fw.minor_ver_wanted = SKL_FW_MINOR; diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c index ea67abc..ab4ee08 100644 --- a/drivers/gpu/drm/i915/intel_huc.c +++ b/drivers/gpu/drm/i915/intel_huc.c @@ -153,7 +153,11 @@ void intel_huc_select_fw(struct intel_huc *huc) huc->fw.load_status = INTEL_UC_FIRMWARE_NONE; huc->fw.fw = INTEL_UC_FW_TYPE_HUC; - if (IS_SKYLAKE(dev_priv)) { + if (i915.huc_firmware_path) { + huc->fw.path = i915.huc_firmware_path; + huc->fw.major_ver_wanted = 0; + huc->fw.minor_ver_wanted = 0; + } else if (IS_SKYLAKE(dev_priv)) { huc->fw.path = I915_SKL_HUC_UCODE; huc->fw.major_ver_wanted = SKL_HUC_FW_MAJOR; huc->fw.minor_ver_wanted = SKL_HUC_FW_MINOR; diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 8875647..1cf4590 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -359,8 +359,10 @@ void intel_uc_prepare_fw(struct drm_i915_private *dev_priv, goto fail; } - if (uc_fw->major_ver_found != uc_fw->major_ver_wanted || - uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) { + if (uc_fw->major_ver_wanted == 0 && uc_fw->minor_ver_wanted == 0) { + DRM_NOTE("Skipping uC firmware version check\n"); + } else if (uc_fw->major_ver_found != uc_fw->major_ver_wanted || + uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) { DRM_NOTE("uC firmware version %d.%d, required %d.%d\n", uc_fw->major_ver_found, uc_fw->minor_ver_found, uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
`guc_firmware_path` and `huc_firmware_path` module parameters are added. Using the parameter disables version checks and loads desired firmware instead of the default one. v2: make params unsafe && notice about disabled fw check (J. Lahtinen) Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Michal Winiarski <michal.winiarski@intel.com> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> --- drivers/gpu/drm/i915/i915_params.c | 10 ++++++++++ drivers/gpu/drm/i915/i915_params.h | 2 ++ drivers/gpu/drm/i915/intel_guc_loader.c | 6 +++++- drivers/gpu/drm/i915/intel_huc.c | 6 +++++- drivers/gpu/drm/i915/intel_uc.c | 6 ++++-- 5 files changed, 26 insertions(+), 4 deletions(-)