[Intel-gfx] [PATCH 02/15] drm/i915: Embedded microcontroller (uC) firmware loading support

[Dave Gordon]

On 24/06/15 11:29, Daniel Vetter wrote:
> On Fri, Jun 19, 2015 at 09:43:11AM +0100, Dave Gordon wrote:
>> On 18/06/15 15:49, Daniel Vetter wrote:
>>> On Thu, Jun 18, 2015 at 01:11:34PM +0100, Dave Gordon wrote:
>>>> On 17/06/15 13:05, Daniel Vetter wrote:
>>>>> On Mon, Jun 15, 2015 at 07:36:20PM +0100, Dave Gordon wrote:
>>>>>> Current devices may contain one or more programmable microcontrollers
>>>>>> that need to have a firmware image (aka "binary blob") loaded from an
>>>>>> external medium and transferred to the device's memory.
>>>>>>
>>>>>> This file provides generic support functions for doing this; they can
>>>>>> then be used by each uC-specific loader, thus reducing code duplication
>>>>>> and testing effort.
>>>>>>
>>>>>> Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
>>>>>> Signed-off-by: Alex Dai <yu.dai at intel.com>
>>>>>
>>>>> Given that I'm just shredding the synchronization used by the dmc loader
>>>>> I'm not convinced this is a good idea. Abstraction has cost, and a bit of
>>>>> copy-paste for similar sounding but slightly different things doesn't
>>>>> sound awful to me. And the critical bit in all the firmware loading I've
>>>>> seen thus far is in synchronizing the loading with other operations,
>>>>> hiding that isn't a good idea. Worse if we enforce stuff like requiring
>>>>> dev->struct_mutex.
>>>>> -Daniel
>>>>
>>>> It's precisely because it's in some sense "trivial-but-tricky" that we
>>>> should write it once, get it right, and use it everywhere. Copypaste
>>>> /does/ sound awful; I've seen how the code this was derived from had
>>>> already been cloned into three flavours, all different and all wrong.
>>>>
>>>> It's a very simple abstraction: one early call to kick things off as
>>>> early as possible, no locking required. One late call with the
>>>> struct_mutex held to complete the synchronisation and actually do the
>>>> work, thus guaranteeing that the transfer to the target uC is done in a
>>>> controlled fashion, at a time of the caller's choice, and by the
>>>> driver's mainline thread, NOT by an asynchronous thread racing with
>>>> other activity (which was one of the things wrong with the original
>>>> version).
>>>
>>> Yeah I've seen the origins of this in the display code, and that code gets
>>> the syncing wrong. The only thing that one has do to is grab a runtime pm
>>> reference for the appropriate power well to prevent dc5 entry, and release
>>> it when the firmware is loaded and initialized.
>>
>> Agreed.
>>
>>> Which means any kind of firmware loader which requires/uses
>>> dev->struct_mutex get stuff wrong and is not appropriate everywhere.
>>
>> BUT, the loading of the firmware into any uC MUST be done in a
>> controlled manner i.e. at a time when no other thread is touching the
>> h/w. Otherwise the f/w load and whatever else is concurrently accessing
>> the h/w could in some cases interfere disastrously. Examples of
>> interference might be:
>>
>> * interleaved accesses to the ELSP (in the case of the GuC)
>> * incorrect handover of power management (DMC, GuC)
>> * erroneous management of forcewake state
>>
>> In general the f/w that is just starting on the uC may have certain
>> expectations about the initial state of the h/w, which may not be met if
>> other threads are accessing various bits of h/w while the uC is booting up.
>>
>> So we absolutely need to guarantee that the f/w load is done by a thread
>> which has exclusive ownership of any bit of the h/w that the f/w is
>> going to make assumptions about. With the current locking structure of
>> the driver, that means holding the struct_mutex (it shouldn't really,
>> there should be a separate mutex for h/w register access vs.
>> driver-private data structures, but there isn't).
>
> If you really need this guarantee (and I seriously hope not) then the only
> option is a synchronous firmware load at driver init _before_ we launch
> any of the asynchronous setup code. And there is already a lot of that,
> and we're adding more all the time.
>
> What I expect we need is synchronization of just the revelant part with
> the firmware loading, which necessarily needs to be somewhat async to be
> able to support cros/android requirements. And yes that needs to be done
> in a controlled manner, but most likely we need very specific solutions
> for the problem at hand. Unconditionally holding dev->struct_mutex isn't
> that solution.
>
> The other problem with dev->struct_mutex is that it's a giantic lock with
> ill defined coverage and semantics. It's imo the biggest piece of
> technical debt we carry around in i915.ko, and we pay the price for that
> dearly&daily. Which means that since a few years any kind of code
> which extended dev->struct_mutex to anything not clearly core gem data
> structures was rejected.

Oh, I quite agree that the struct_mutex is an abomination and would 
certainly like to eliminate it. But at the moment it's the only 
sufficiently large-scale synchronisation operation available to ensure 
that (for example) we don't try to load the f/w at the same time that 
another thread is trying to reset the h/w.

None of this loader code really needs the struct_mutex specifically; the 
WARN_ON macros were just there to help callers know what degree of 
synchronisation they need to organise before calling these functions.

[snip]

>> BTW, the existing DMC loader probably won't work on Android :(
>
> Yeah I completely missed out on this fun since I presumed that firmware
> loading is easy and simple. And if you look around on other drm drivers it
> indeed is, they all use a synchronous request_firmware and if the firmware
> isn't there, they just fall over (fully in the case of radeon.ko,
> partially in the case of nouveau.ko since they have all the support in
> place for handling a kms-only accel-less gpu in userspace anyway, like we
> do). But for a bunch of reasons (afaik it's "you can't include a blob in a
> gpl-ed kernel image" we need async firmware loading for cros&android).
>
> That leaves us with a situation where we should have done a special design
> discussion about asynchronous firmware, but somehow failed do to that.
> Which leaves us in a very ugly position.
>
> I talked with a bunch of people over the past few days to figure out how
> this is supposed to work and also figure out why it's being done like that
> today. I think I have a reasonable good plan for moving forward too. I'll
> start a new top-level thread here to discuss this.
>
> Thanks, Daniel

It really isn't "asynchronous", it's just "deferred" -- but implying 
that everything that relies on having the firmware available also has to 
be deferred. For the DMC, that means we can't have full PM; and for the 
GuC, we can't submit any batches at all to any engine until the f/w load 
is done.

Really, it would be simpler if we didn't support automatic firmware 
loading in the kernel at all, and had a userland startup process whose 
job was to locate and transfer the required firmware before the device 
could be used. But that would just give us the opposite problem, because 
the display device is one of the things that /should/ be usable even 
before the rootfs is mounted.

Anyway, I've posted a simplified v3 which only supports synchronous 
fetch, without the prefetch capability. See separate thread ...

.Dave.