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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 22 Aug 2019 08:40:33 +0200, Arkadiusz Hiler <arkadiusz.hiler@xxxxxxxxx> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux