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

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

On Thu, 14 Jul 2016, 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
>
Daniel,

I won't have to time to do this. So I'll leave the HuC/GuC rewrite for 
you to find someone else to take them on as this is likely to take more 
time than I will be employed by Intel.

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