[Intel-gfx] [PATCH] drm/i915/GuC: Combine the two kernel parameter into one

[Tvrtko Ursulin]

On 15/11/2016 22:46, Jeff McGee wrote:
> On Tue, Nov 15, 2016 at 10:06:47AM -0800, Srivatsa, Anusha wrote:
>>> -----Original Message-----
>>> From: Tvrtko Ursulin [mailto:tvrtko.ursulin at linux.intel.com]
>>> Sent: Tuesday, November 15, 2016 2:31 AM
>>> To: Srivatsa, Anusha <anusha.srivatsa at intel.com>; Mcgee, Jeff
>>> <jeff.mcgee at intel.com>
>>> Cc: Ursulin, Tvrtko <tvrtko.ursulin at intel.com>; intel-gfx at lists.freedesktop.org;
>>> Vivi, Rodrigo <rodrigo.vivi at intel.com>
>>> Subject: Re: [Intel-gfx] [PATCH] drm/i915/GuC: Combine the two kernel
>>> parameter into one
>>>
>>>
>>> On 14/11/2016 17:34, Srivatsa, Anusha wrote:
>>>
>>> [snip]
>>>
>>>>> One idea could be to hide the guc loading form the user altogether
>>>>> and hence improve usability (decrease exposed complexity) by having
>>>>> only two parameters; i915.enable_guc_scheduling and i915.enable_huc.
>>>> That's a good point. But with this we will have two parameters (which kills the
>>> point of why the patch was written in the first place), then we can rather leave it
>>> the way it is. Right?
>>>
>>> For some reason I thought the HuC patch series add a another module
>>> parameter.
>>>
>>> What is the failure mode for HuC is GuC firmware loading is disabled btw?
>>
>>
>> Hi Tvrtko, in the function intel_guc_auth_huc() there is a check to see if GuC is loaded or not. If GuC loading has failed or loading is disabled, then HuC authentication does not happen.
>>
> Yes, GuC must authenticate HuC firmware.

I was wondering about the failure mode - or in other words does it 
suggest in the error message what might be the problem so it is helpful 
for the user. Let me check.. I did not find anything - what it will say? 
Just generic firmware load fetch messages with a timeout / error?

> I am in favor of Tvrtko's suggestion for dropping i915.enable_guc_loading,
> keeping i915.enable_guc_submission, and adding i915.enable_huc. If either of
> the last two are enabled, GuC loading is implied. So kernel parameters are tied
> to enabling specific functionality. I think the specific parameter for loading
> is legacy from the first hurdle for GuC long ago. I assume we are not bound by
> ABI to keep that around if it is no longer needed, yes?

I think it is OK to change them. But there would have to be wider 
agreement on the end result which I think means soliciting opinions from 
maintainers and domain owners.

> The other thing I would want to reconsider is the "casual" enable vs. "force"
> enable options. Does anyone remember why these 2 levels of enable have been
> used. Maybe this is also a legacy. Can we just do a auto (-1), disable (0),
> and enable (1)?

Yes I was wondering myself. Don't know really. Dave is not around any 
more so all we could do is trawl the mailing list archives in case there 
is something there.

Regards,

Tvrtko