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

[Martin Peres <martin.peres@linux.intel.com>]

On 19/08/2019 21:22, Lucas De Marchi wrote:
> On Mon, Aug 19, l we need is to add2019 at 4:15 AM Michal Wajdeczko
> <michal.wajdeczko@intel.com> wrote:
>>
>> On Mon, 19 Aug 2019 10:09:15 +0200, Martin Peres
>> <martin.peres@linux.intel.com> 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
>>
>> Yep, this is right time to double check that.
>>
>>>  - add fault injection to the FW loading path
>>
>> About 40 new failure points were added to firmware loading path by
>> recent commits like [1] ("drm/i915/uc: Hardening firmware fetch")
>> and [2] ("drm/i915/uc: Inject probe errors into intel_uc_init_hw")
>> so I assume we could mark this item as done.
>>
>>>  - add IGT tests to make sure driver behaves well on different FW
>>> loading errors
>>
>> Not sure which if we need another set of tests beyond existing and
>> already used igt@i915_module_load@reload-with-fault-injection.
>>
>> Maybe all we need is to add dedicated CI machines that will try to
>> use GuC but without valid firmware on the system? Let CI team speak
>> here. Note I was already trying to mimic that scenario with [3] and
>> [4] and they we all good.
> 
> I don't think we should use dedicated CI machines for that. We could
> make the test to bind mount an empty
> directory before executing in order to hide the firmware, which would
> be an easy solution.
> 
> In kmod testsuite I do something similar for modules, but I trap all
> fs-related calls (I had to do that for other reasons):
> https://git.kernel.org/pub/scm/utils/kernel/kmod/kmod.git/tree/testsuite/path.c

Let's not forget that IGT is meant to be run by anyone, and the more
testing we can do on a developer's machine, the better.

So thank you Lucas for being faster than me to suggest that relying on
dedicated machines is a bad idea, especially when it is such an easy
solution :)

Martin

> 
> Lucas De Marchi
> 
> 
>>
>> Thanks,
>> Michal
>>
>> [1]
>> https://cgit.freedesktop.org/drm/drm-tip/commit/?id=5e0a809af2a2b577934ec54c0e50897b806f0732
>> [2]
>> https://cgit.freedesktop.org/drm/drm-tip/commit/?id=5d1ef2b4270de45e1b1b40a00838e3b6196eefd7
>> [3] https://patchwork.freedesktop.org/series/65378/#rev1
>> [4] https://patchwork.freedesktop.org/series/65379/#rev1
>>
>>>
>>> Martin
>>>
>>>>
>>>> References: commit f774f0964919 ("drm/i915/guc: Turn on GuC/HuC auto
>>>> mode")
>>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/i915_params.c | 2 +-
>>>>  drivers/gpu/drm/i915/i915_params.h | 2 +-
>>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_params.c
>>>> b/drivers/gpu/drm/i915/i915_params.c
>>>> index 296452f9efe4..b4f481e1e6b6 100644
>>>> --- a/drivers/gpu/drm/i915/i915_params.c
>>>> +++ b/drivers/gpu/drm/i915/i915_params.c
>>>> @@ -146,7 +146,7 @@ i915_param_named_unsafe(edp_vswing, int, 0400,
>>>>  i915_param_named_unsafe(enable_guc, int, 0400,
>>>>      "Enable GuC load for GuC submission and/or HuC load. "
>>>>      "Required functionality can be selected using bitmask values. "
>>>> -    "(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)");
>>>> +    "(-1=auto [default], 0=disable, 1=GuC submission, 2=HuC load)");
>>>>
>>>>  i915_param_named(guc_log_level, int, 0400,
>>>>      "GuC firmware logging level. Requires GuC to be loaded. "
>>>> diff --git a/drivers/gpu/drm/i915/i915_params.h
>>>> b/drivers/gpu/drm/i915/i915_params.h
>>>> index d29ade3b7de6..5736c55694fe 100644
>>>> --- a/drivers/gpu/drm/i915/i915_params.h
>>>> +++ b/drivers/gpu/drm/i915/i915_params.h
>>>> @@ -54,7 +54,7 @@ struct drm_printer;
>>>>      param(int, disable_power_well, -1) \
>>>>      param(int, enable_ips, 1) \
>>>>      param(int, invert_brightness, 0) \
>>>> -    param(int, enable_guc, 0) \
>>>> +    param(int, enable_guc, -1) \
>>>>      param(int, guc_log_level, -1) \
>>>>      param(char *, guc_firmware_path, NULL) \
>>>>      param(char *, huc_firmware_path, NULL) \
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx