diff mbox

[10/10] drm/i915/uc: Add params for specifying firmware

Message ID 20170224154004.17821-11-arkadiusz.hiler@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Arkadiusz Hiler Feb. 24, 2017, 3:40 p.m. UTC 
`guc_firmware_path` and `huc_firmware_path` module parameters are added.

Using the parameter disabled version checks and loads desired firmware
instead of the default one.

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         |  5 +++--
 5 files changed, 25 insertions(+), 4 deletions(-)

Comments

Michal Wajdeczko Feb. 24, 2017, 6:44 p.m. UTC | #1 
On Fri, Feb 24, 2017 at 04:40:04PM +0100, Arkadiusz Hiler wrote:
> `guc_firmware_path` and `huc_firmware_path` module parameters are added.
> 
> Using the parameter disabled version checks and loads desired firmware
> instead of the default one.
> 
> 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         |  5 +++--
>  5 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 2e9645e..9c3ff3c 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(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(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 8ccd832..8a03836 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -479,7 +479,11 @@ void 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 d073a68..b2067bf 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 89681b37..bbfb5b6 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -357,8 +357,9 @@ 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_found == 0 && uc_fw->minor_ver_found == 0) &&

Hmm, is this correct condition ?


-Michal

> +	    (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
Joonas Lahtinen Feb. 27, 2017, 12:39 p.m. UTC | #2 
On pe, 2017-02-24 at 16:40 +0100, Arkadiusz Hiler wrote:
> `guc_firmware_path` and `huc_firmware_path` module parameters are added.
> 
> Using the parameter disabled version checks and loads desired firmware
> instead of the default one.
> 
> 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>

<SNIP>

> @@ -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(guc_firmware_path, i915.guc_firmware_path, charp, 0400);

I'm pretty sure this should be _unsafe, because it overrides the
version checks. Cc'd Jani for this.

> @@ -479,7 +479,11 @@ void 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;

Or, we could keep the wanted version number, only replace the path, and
spit out WARN/taint kernel if some other version was detected?

But I guess the main purpose is to override version (not provide
request_firmware workarounds), so my vote is to make the param _unsafe.

Regards, Joonas
Jani Nikula Feb. 27, 2017, 1:30 p.m. UTC | #3 
On Mon, 27 Feb 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote:
> On pe, 2017-02-24 at 16:40 +0100, Arkadiusz Hiler wrote:
>> `guc_firmware_path` and `huc_firmware_path` module parameters are added.
>> 
>> Using the parameter disabled version checks and loads desired firmware
>> instead of the default one.
>> 
>> 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>
>
> <SNIP>
>
>> @@ -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(guc_firmware_path, i915.guc_firmware_path, charp, 0400);
>
> I'm pretty sure this should be _unsafe, because it overrides the
> version checks. Cc'd Jani for this.

Yes, please. I replied the same thing to Chris' patches adding the same
thing.

BR,
Jani.

>
>> @@ -479,7 +479,11 @@ void 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;
>
> Or, we could keep the wanted version number, only replace the path, and
> spit out WARN/taint kernel if some other version was detected?
>
> But I guess the main purpose is to override version (not provide
> request_firmware workarounds), so my vote is to make the param _unsafe.
>
> Regards, Joonas
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 2e9645e..9c3ff3c 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(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(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 8ccd832..8a03836 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -479,7 +479,11 @@  void 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 d073a68..b2067bf 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 89681b37..bbfb5b6 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -357,8 +357,9 @@  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_found == 0 && uc_fw->minor_ver_found == 0) &&
+	    (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);