[Intel-gfx] [PATCH 3/6] drm/i915/huc: Add HuC fw loading support

[Dave Gordon]

On 14/07/16 15:16, Daniel Vetter wrote:
> On Wed, Jul 13, 2016 at 03:52:39PM +0100, Peter Antoine wrote:
>> 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?
>
> Apparently because I'm a too nice maintainer and allowed half-solutions to
> get landed, under the expecations that people would indeed follow up and
> fix things.
>
> And yes, you care, you fix it, is how this works, whether you like it or
> not.
> -Daniel

AFAIK request_firmware() is *not* broken. It does what it says i.e. 
fetches a blob from the internal bucket or from the filesystem; and 
fails if it's not found. So it doesn't need fixing.

What's broken (from the Android POV) is i915 needing to do too much too 
early i.e. before all filesystems are mounted. The answer to *that* is 
to defer *all* engine initialisation until (much) later; but that's 
nothing at all to do with Peter's patches here.

.Dave.