Re: [PATCH 3/6] drm/i915/huc: Add HuC fw loading support

[Peter Antoine <peter.antoine@intel.com>]

On Wed, 13 Jul 2016, Daniel Vetter wrote:

> On Thu, Jun 23, 2016 at 02:52:41PM +0100, Peter Antoine wrote:
>> On Thu, 23 Jun 2016, Dave Gordon wrote:
>>> On 22/06/16 09:31, Daniel Vetter wrote:
>>> No, the *correct* fix is to unify all the firmware loaders we have.
>>> There should just be ONE piece of code that can be used to fetch and
>>> load ANy firmware into ANY auxiliary microcontroller. NOT one per
>>> microcontroller, all different -- that way lies madness.
>>>
>>> We already had a unified loader for the HuC and GuC a year ago, but IIRC
>>> the party line then was "just make it (GuC) specific, then copypaste it
>>> for the second uC, and when we've got three versions we'll have learnt
>>> how we really want a unified loader to behave."
>>>
>>> Well. here's the copypaste, and we already have a different loader for
>>> the DMC/CSR, so it must be time for (re-)unification.
>>>
>>> .Dave.
>>>
>>
>> Just to add, if you uc_fw_fetch() has an error code you will still have to
>> remember the state of the fetch or at each reset/resume/etc... or you will
>> have to try the firmware load again and that can take a long time. So the
>> state will have to be re-instated.
>>
>> Seeing this code was written with the given goals and were written in the
>> same vane as code that was deemed acceptable, it seems weird at this late
>> stage to change the design goals.
>>
>> Note: this is the third time that these patches have been posted and were
>> only rejected (as far as I know) due to no open-source user. Which there is
>> now, and is why I have reposted these patches.
>
> I never liked the guc firmware code, but figure for one copy it's not
> worth fighting over. Adding more copies (or perpetuating the design by
> making it generic) isn't what I'm looking for. Firmware loading shouldn't
> be that complicated, really.
>
> The unified firmware loader is called request_firmware. If that's not good
> enough, pls fix the core function, not paper code over in i915. In that
> regard DMC/CSR is unified, everything else isn't yet.
>
> Iirc the big issue is delayed firmware loading for built-in i915 and fw
> only available later on. This is an open issue in request_firmware() since
> years, and there's various patches floating around. If the problem is that
> Greg KH doesn't consider those patches, I can help with that. But not
> pushing the core fix forward isn't acceptable imo. Once that fix is landed
> we can treat request_firmware as reliable (it might take a while, hence
> must be run in an async work like DMC loading), with no need to ever retry
> anything. If fw loading fails we can just mark the entire render part of
> the gpu as dead by injecting the equivalent of a non-recoverable hang
> (async setup) or failing engine init with -EIO (if this is still
> synchronous, which I don't expect really).
>
> If there's another reason for this complexity, please explain since I'd
> like to understand why we need this.
> -Daniel
>

I was not involved at the start and I am porting code that others have 
written, but as far as I understand this, you requested that the code be 
duplicated. See Dave's comment above as he was involved.

The code uses request_firmware() to handle the load of the firmware into 
memory and the rest of this code manages the loading of that memory into 
the HuC's SRAM. This needs extra setup that should not really go into the 
generic firmware loader (one loader for uIA's make sense as this will 
futureproof the code). Also the SRAM is write-once memory and needs to 
be handled correctly. Also, the GuC needs to verify the HuC some this 
becomes a little be more fun.

Also, again you are ignoring the point that not having firmware is not 
fatal. We remember the state of the load as this is required to save time 
when we come out of reset to not waste time trying to reload the firmware, 
if it has failed already. They are other mechinums to do this, but they 
will always need some form of history.

Also, if request_firmware() is broken why has this not been fixed? If it 
has been broken for "years" why and how do you expect to to be fixed now? If 
the "not pushing the core fix is not acceptable" why has that not been done?

Peter.

--
    Peter Antoine (Android Graphics Driver Software Engineer)
    ---------------------------------------------------------------------
    Intel Corporation (UK) Limited
    Registered No. 1134945 (England)
    Registered Office: Pipers Way, Swindon SN3 1RJ
    VAT No: 860 2173 47
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx