[Intel-gfx] [PATCH 1/2] drm/i915/huc: Introduce enable_huc parameter

[Srivatsa, Anusha]

>-----Original Message-----
>From: Wajdeczko, Michal
>Sent: Thursday, December 15, 2016 10:32 AM
>To: Srivatsa, Anusha <anusha.srivatsa at intel.com>
>Cc: intel-gfx at lists.freedesktop.org; Nikula, Jani <jani.nikula at intel.com>
>Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/huc: Introduce enable_huc
>parameter
>
>On Thu, Dec 15, 2016 at 10:24:56AM -0800, anushasr wrote:
>> From: Anusha Srivatsa <anusha.srivatsa at intel.com>
>>
>> Add a new kernel parameter: enable_huc. This parameter controls HuC
>> functionality. If this parameter is set to 1, it suggests that HuC
>> functionality is being used and also that the GuC has to be loaded.
>> GuC has to be loaded in order to authenticate the HuC.
>>
>> Cc: Jani Nikula <jani.nikula at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
>> Cc: Arek <arkadiusz.hiler at intel.com>
>> Signed-off-by: Anusha Srivatsa <anusha.srivatsa at intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_params.c      | 5 +++++
>>  drivers/gpu/drm/i915/i915_params.h      | 1 +
>>  drivers/gpu/drm/i915/intel_huc_loader.c | 4 ++++
>>  3 files changed, 10 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_params.c
>> b/drivers/gpu/drm/i915/i915_params.c
>> index 0e280fb..1d9c306 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -56,6 +56,7 @@ struct i915_params i915 __read_mostly = {
>>  	.verbose_state_checks = 1,
>>  	.nuclear_pageflip = 0,
>>  	.edp_vswing = 0,
>> +	.enable_huc = 1,
>>  	.enable_guc_loading = 0,
>>  	.enable_guc_submission = 0,
>>  	.guc_log_level = -1,
>> @@ -216,6 +217,10 @@ MODULE_PARM_DESC(edp_vswing,
>>  		 "(0=use value from vbt [default], 1=low power swing(200mV),"
>>  		 "2=default swing(400mV))");
>>
>> +module_param_named(enable_huc, i915.enable_huc, int, 0400);
>> +MODULE_PARM_DESC(enable_huc,
>> +		"Enable HuC usage. If enabled,load GuC (1:enabled (default),
>> +0:disabled)");
>> +
>>  module_param_named_unsafe(enable_guc_loading,
>> i915.enable_guc_loading, int, 0400);
>MODULE_PARM_DESC(enable_guc_loading,
>>  		"Enable GuC firmware loading "
>> diff --git a/drivers/gpu/drm/i915/i915_params.h
>> b/drivers/gpu/drm/i915/i915_params.h
>> index 8e433de..7b0523b 100644
>> --- a/drivers/gpu/drm/i915/i915_params.h
>> +++ b/drivers/gpu/drm/i915/i915_params.h
>> @@ -44,6 +44,7 @@ struct i915_params {
>>  	int disable_power_well;
>>  	int enable_ips;
>>  	int invert_brightness;
>> +	int enable_huc;
>>  	int enable_guc_loading;
>>  	int enable_guc_submission;
>>  	int guc_log_level;
>> diff --git a/drivers/gpu/drm/i915/intel_huc_loader.c
>> b/drivers/gpu/drm/i915/intel_huc_loader.c
>> index 8137979..a545f76 100644
>> --- a/drivers/gpu/drm/i915/intel_huc_loader.c
>> +++ b/drivers/gpu/drm/i915/intel_huc_loader.c
>> @@ -166,6 +166,10 @@ void intel_huc_init(struct drm_i915_private *dev_priv)
>>  	huc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
>>  	huc_fw->fw_type = INTEL_UC_FW_TYPE_HUC;
>>
>> +	if (!i915.enable_huc)
>> +		DRM_ERROR("HuC not enabled. Will not be loaded\n");
>
>Is this really an error ? Maybe DRM_INFO ?
>What if value of this param conflicts with HAS_HUC_UCODE ?
>Shouldn't we have some 'sanitize' code for this ?
>Or maybe we should check this param after HAS_HUC check below ?
>
DRM_Info might be a good idea. But I feel this is the right place for this. Since we first check if HuC is there or not, if not there then return immediately. If present then go ahead and do the check below.

Anusha
>Michal
>
>> +		return;
>> +
>>  	if (!HAS_HUC_UCODE(dev_priv))
>>  		return;
>>
>> --
>> 2.7.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx