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

From: Daniel Vetter <daniel@ffwll.ch>
To: Peter Antoine <peter.antoine@intel.com>
Cc: lawrence.t.li@intel.com, sean.v.kelley@intel.com,
	intel-gfx@lists.freedesktop.org, rodrigo.vivi@intel.com
Subject: Re: [PATCH 3/6] drm/i915/huc: Add HuC fw loading support
Date: Wed, 13 Jul 2016 14:48:10 +0200	[thread overview]
Message-ID: <20160713124810.GO23520@phenom.ffwll.local> (raw)
In-Reply-To: <alpine.DEB.2.10.1606231440410.8449@peterant-linux2>

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx