[Intel-gfx] [PATCH 1/1] drm/i915/uc: Turn on GuC/HuC auto mode again

[Michal Wajdeczko]

On Thu, 22 Aug 2019 08:40:33 +0200, Arkadiusz Hiler  
<arkadiusz.hiler at intel.com> wrote:

> On Mon, Aug 19, 2019 at 11:09:15AM +0300, Martin Peres wrote:
>> On 18/08/2019 18:51, Michal Wajdeczko wrote:
>> > We hope that now all issues related to missing uC firmwares
>> > are fixed and that driver can continue to load without GuC
>> > or HuC firmware available and running:
>> >
>> >  - missing or corrupted HuC firmware has no impact to driver
>> >    load (clients should continue to use HUC_STATUS to check
>> >    if HuC firmware was loaded and authenticated)
>> >
>> >  - missing or corrupted GuC firmware has no impact to driver
>> >    load (unless GuC firmware blob was overridden by the user
>> >    or GuC submission was requested or GuC was previously
>> >    enabled on this system without reboot - then we will mark
>> >    GPU as wedged and continue with KMS only)
>>
>> Please hold merging this patch, as many more items need to be crossed
>> off before such a patch can land.
>>
>> Such items include:
>>
>>  - Assess all the existing GUC-related bugs, and prove they won't
>> suddenly get seen by more users
>>  - add fault injection to the FW loading path
>>  - add IGT tests to make sure driver behaves well on different FW
>> loading errors
>
> If this is going to get enabled by default we should add some tests to
> verify that HuC is indeed loaded and operational. Otherwise this may
> degrade without anyone noticing.

we were discussing such test some time ago [1], but we couldn't get
into final agreement.

[1] https://patchwork.freedesktop.org/series/60800/


>
> Something along those lines:
>     int huc_status = getparam(I915_PARAM_HUC_STATUS);
>
>     assert(MI_PREDICATE(HUC_STATUS) == !!huc_status);
>     skip_on_f(huc_status == 0, "HuC disabled\n");
>
>     assert_f(huc_status == 1, "HuC status is not enabled: %d\n",  
> huc_status);
>     assert(submit_commands_to_check_that_huc_is_operational());
>
>
>
> The issue is that the PARAM_HUC_STATUS won't even work right now because:
>
>     case I915_PARAM_HUC_STATUS:
>     	value = intel_huc_check_status(&i915->gt.uc.huc);
>     	if (value < 0)
>     		return value;
>     	break;
>     /* ... */
>     return 0;

Please note that this return if for ioctl() call status

>
>
>     /**
>      * intel_huc_check_status() - check HuC status
>      * @huc: intel_huc structure
>      *
>      * This function reads status register to verify if HuC
>      * firmware was successfully loaded.
>      *
>      * Returns: 1 if HuC firmware is loaded and verified,
>      * 0 if HuC firmware is not loaded and -ENODEV if HuC
>      * is not present on this platform.
>      */
>
> This is broken - we will get 0 whether it's enabled or disabled.

I don't think so. Negative values returned by this function are simply
used as ioctl() errors, while 0/1 values are returned as 'value' field
that holds reply with actual HuC status:

	if (put_user(value, param->value))
		return -EFAULT;

More coffee?

Michal