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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
