On Thu, Aug 22, 2019 at 09:31:07AM +0200, Michal Wajdeczko wrote: > 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/ Oh, that's a good start. It would be good to land this along the default enabling of HuC and have the agreement on the "best error codes" by then. > > 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? My bad. coffee++ would have helped _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
