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

[Arkadiusz Hiler]

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 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/

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