diff mbox series

[1/1] drm/i915/uc: Use platform specific defaults for GuC/HuC enabling

Message ID 20210603164812.19045-2-matthew.brost@intel.com (mailing list archive)
State New, archived
Headers show
Series Use platform specific defaults for GuC/HuC enabling | expand

Commit Message

Matthew Brost June 3, 2021, 4:48 p.m. UTC 
From: John Harrison <John.C.Harrison@Intel.com>

The meaning of 'default' for the enable_guc module parameter has been
updated to accurately reflect what is supported on current platforms.
So start using the defaults instead of forcing everything off.
Although, note that right now, the default is for everything to be off
anyway. So this is not a change for current platforms.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/i915_params.c | 2 +-
 drivers/gpu/drm/i915/i915_params.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Daniele Ceraolo Spurio June 9, 2021, 6:31 p.m. UTC | #1 
On 6/3/2021 9:48 AM, Matthew Brost wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> The meaning of 'default' for the enable_guc module parameter has been
> updated to accurately reflect what is supported on current platforms.
> So start using the defaults instead of forcing everything off.
> Although, note that right now, the default is for everything to be off
> anyway. So this is not a change for current platforms.
>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Double checked the CI results and the 2 errors are unrelated.
Pushed to gt-next.

Daniele

> ---
>   drivers/gpu/drm/i915/i915_params.c | 2 +-
>   drivers/gpu/drm/i915/i915_params.h | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 0320878d96b0..e07f4cfea63a 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -160,7 +160,7 @@ i915_param_named_unsafe(edp_vswing, int, 0400,
>   i915_param_named_unsafe(enable_guc, int, 0400,
>   	"Enable GuC load for GuC submission and/or HuC load. "
>   	"Required functionality can be selected using bitmask values. "
> -	"(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)");
> +	"(-1=auto [default], 0=disable, 1=GuC submission, 2=HuC load)");
>   
>   i915_param_named(guc_log_level, int, 0400,
>   	"GuC firmware logging level. Requires GuC to be loaded. "
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index 4a114a5ad000..f27eceb82c0f 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -59,7 +59,7 @@ struct drm_printer;
>   	param(int, disable_power_well, -1, 0400) \
>   	param(int, enable_ips, 1, 0600) \
>   	param(int, invert_brightness, 0, 0600) \
> -	param(int, enable_guc, 0, 0400) \
> +	param(int, enable_guc, -1, 0400) \
>   	param(int, guc_log_level, -1, 0400) \
>   	param(char *, guc_firmware_path, NULL, 0400) \
>   	param(char *, huc_firmware_path, NULL, 0400) \
Tvrtko Ursulin April 7, 2022, 3:49 p.m. UTC | #2 
On 03/06/2021 17:48, Matthew Brost wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> The meaning of 'default' for the enable_guc module parameter has been
> updated to accurately reflect what is supported on current platforms.
> So start using the defaults instead of forcing everything off.
> Although, note that right now, the default is for everything to be off
> anyway. So this is not a change for current platforms.
> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_params.c | 2 +-
>   drivers/gpu/drm/i915/i915_params.h | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 0320878d96b0..e07f4cfea63a 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -160,7 +160,7 @@ i915_param_named_unsafe(edp_vswing, int, 0400,
>   i915_param_named_unsafe(enable_guc, int, 0400,
>   	"Enable GuC load for GuC submission and/or HuC load. "
>   	"Required functionality can be selected using bitmask values. "
> -	"(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)");
> +	"(-1=auto [default], 0=disable, 1=GuC submission, 2=HuC load)");
>   
>   i915_param_named(guc_log_level, int, 0400,
>   	"GuC firmware logging level. Requires GuC to be loaded. "
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index 4a114a5ad000..f27eceb82c0f 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -59,7 +59,7 @@ struct drm_printer;
>   	param(int, disable_power_well, -1, 0400) \
>   	param(int, enable_ips, 1, 0600) \
>   	param(int, invert_brightness, 0, 0600) \
> -	param(int, enable_guc, 0, 0400) \
> +	param(int, enable_guc, -1, 0400) \
>   	param(int, guc_log_level, -1, 0400) \
>   	param(char *, guc_firmware_path, NULL, 0400) \
>   	param(char *, huc_firmware_path, NULL, 0400) \

What is the BKM to use this with multi-GPU setups? Specifically I have a 
TGL+DG1 laptop (off the shelf) and want to have GuC with DG1 only. If I 
pass i915.enable_guc=3 it seems it wants to enable it for TGL as well 
and wedges the GPU if it can't?

Regards,

Tvrtko
John Harrison April 7, 2022, 8:49 p.m. UTC | #3 
On 4/7/2022 08:49, Tvrtko Ursulin wrote:
> On 03/06/2021 17:48, Matthew Brost wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> The meaning of 'default' for the enable_guc module parameter has been
>> updated to accurately reflect what is supported on current platforms.
>> So start using the defaults instead of forcing everything off.
>> Although, note that right now, the default is for everything to be off
>> anyway. So this is not a change for current platforms.
>>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_params.c | 2 +-
>>   drivers/gpu/drm/i915/i915_params.h | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_params.c 
>> b/drivers/gpu/drm/i915/i915_params.c
>> index 0320878d96b0..e07f4cfea63a 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -160,7 +160,7 @@ i915_param_named_unsafe(edp_vswing, int, 0400,
>>   i915_param_named_unsafe(enable_guc, int, 0400,
>>       "Enable GuC load for GuC submission and/or HuC load. "
>>       "Required functionality can be selected using bitmask values. "
>> -    "(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)");
>> +    "(-1=auto [default], 0=disable, 1=GuC submission, 2=HuC load)");
>>     i915_param_named(guc_log_level, int, 0400,
>>       "GuC firmware logging level. Requires GuC to be loaded. "
>> diff --git a/drivers/gpu/drm/i915/i915_params.h 
>> b/drivers/gpu/drm/i915/i915_params.h
>> index 4a114a5ad000..f27eceb82c0f 100644
>> --- a/drivers/gpu/drm/i915/i915_params.h
>> +++ b/drivers/gpu/drm/i915/i915_params.h
>> @@ -59,7 +59,7 @@ struct drm_printer;
>>       param(int, disable_power_well, -1, 0400) \
>>       param(int, enable_ips, 1, 0600) \
>>       param(int, invert_brightness, 0, 0600) \
>> -    param(int, enable_guc, 0, 0400) \
>> +    param(int, enable_guc, -1, 0400) \
>>       param(int, guc_log_level, -1, 0400) \
>>       param(char *, guc_firmware_path, NULL, 0400) \
>>       param(char *, huc_firmware_path, NULL, 0400) \
>
> What is the BKM to use this with multi-GPU setups? Specifically I have 
> a TGL+DG1 laptop (off the shelf) and want to have GuC with DG1 only. 
> If I pass i915.enable_guc=3 it seems it wants to enable it for TGL as 
> well and wedges the GPU if it can't?
>
I don't think there is one.

Module parameters are driver global and therefore apply to all cards in 
the system, both discrete and integrated. The '-1' default can and does 
have different meanings for each card. So in the TGL+DG1 case, TGL 
should default to execlist and DG1 should already be defaulting to GuC. 
So the -1 setting should do what you want. But if you did need to 
override for one specific card only then I think you would need to do 
that with a code change and rebuild.

John.


> Regards,
>
> Tvrtko
Tvrtko Ursulin April 8, 2022, 9:44 a.m. UTC | #4 
On 07/04/2022 21:49, John Harrison wrote:
> On 4/7/2022 08:49, Tvrtko Ursulin wrote:
>> On 03/06/2021 17:48, Matthew Brost wrote:
>>> From: John Harrison <John.C.Harrison@Intel.com>
>>>
>>> The meaning of 'default' for the enable_guc module parameter has been
>>> updated to accurately reflect what is supported on current platforms.
>>> So start using the defaults instead of forcing everything off.
>>> Although, note that right now, the default is for everything to be off
>>> anyway. So this is not a change for current platforms.
>>>
>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_params.c | 2 +-
>>>   drivers/gpu/drm/i915/i915_params.h | 2 +-
>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_params.c 
>>> b/drivers/gpu/drm/i915/i915_params.c
>>> index 0320878d96b0..e07f4cfea63a 100644
>>> --- a/drivers/gpu/drm/i915/i915_params.c
>>> +++ b/drivers/gpu/drm/i915/i915_params.c
>>> @@ -160,7 +160,7 @@ i915_param_named_unsafe(edp_vswing, int, 0400,
>>>   i915_param_named_unsafe(enable_guc, int, 0400,
>>>       "Enable GuC load for GuC submission and/or HuC load. "
>>>       "Required functionality can be selected using bitmask values. "
>>> -    "(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)");
>>> +    "(-1=auto [default], 0=disable, 1=GuC submission, 2=HuC load)");
>>>     i915_param_named(guc_log_level, int, 0400,
>>>       "GuC firmware logging level. Requires GuC to be loaded. "
>>> diff --git a/drivers/gpu/drm/i915/i915_params.h 
>>> b/drivers/gpu/drm/i915/i915_params.h
>>> index 4a114a5ad000..f27eceb82c0f 100644
>>> --- a/drivers/gpu/drm/i915/i915_params.h
>>> +++ b/drivers/gpu/drm/i915/i915_params.h
>>> @@ -59,7 +59,7 @@ struct drm_printer;
>>>       param(int, disable_power_well, -1, 0400) \
>>>       param(int, enable_ips, 1, 0600) \
>>>       param(int, invert_brightness, 0, 0600) \
>>> -    param(int, enable_guc, 0, 0400) \
>>> +    param(int, enable_guc, -1, 0400) \
>>>       param(int, guc_log_level, -1, 0400) \
>>>       param(char *, guc_firmware_path, NULL, 0400) \
>>>       param(char *, huc_firmware_path, NULL, 0400) \
>>
>> What is the BKM to use this with multi-GPU setups? Specifically I have 
>> a TGL+DG1 laptop (off the shelf) and want to have GuC with DG1 only. 
>> If I pass i915.enable_guc=3 it seems it wants to enable it for TGL as 
>> well and wedges the GPU if it can't?
>>
> I don't think there is one.
> 
> Module parameters are driver global and therefore apply to all cards in 
> the system, both discrete and integrated. The '-1' default can and does 
> have different meanings for each card. So in the TGL+DG1 case, TGL 
> should default to execlist and DG1 should already be defaulting to GuC. 
> So the -1 setting should do what you want. But if you did need to 
> override for one specific card only then I think you would need to do 
> that with a code change and rebuild.

You are right, I was on a kernel where DG1 wasn't yet defaulting to GuC 
hence the confusion when I tried to pass enable_guc=3 that broke TGL as 
well, but because I had no up to date firmware for it.

We really need per device modparams, as long as we have modparams..

Regards,

Tvrtko
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 0320878d96b0..e07f4cfea63a 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -160,7 +160,7 @@  i915_param_named_unsafe(edp_vswing, int, 0400,
 i915_param_named_unsafe(enable_guc, int, 0400,
 	"Enable GuC load for GuC submission and/or HuC load. "
 	"Required functionality can be selected using bitmask values. "
-	"(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)");
+	"(-1=auto [default], 0=disable, 1=GuC submission, 2=HuC load)");
 
 i915_param_named(guc_log_level, int, 0400,
 	"GuC firmware logging level. Requires GuC to be loaded. "
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 4a114a5ad000..f27eceb82c0f 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -59,7 +59,7 @@  struct drm_printer;
 	param(int, disable_power_well, -1, 0400) \
 	param(int, enable_ips, 1, 0600) \
 	param(int, invert_brightness, 0, 0600) \
-	param(int, enable_guc, 0, 0400) \
+	param(int, enable_guc, -1, 0400) \
 	param(int, guc_log_level, -1, 0400) \
 	param(char *, guc_firmware_path, NULL, 0400) \
 	param(char *, huc_firmware_path, NULL, 0400) \