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

[Daniel Vetter <daniel@ffwll.ch>]

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@intel.com>
> >> Signed-off-by: Alex Dai <yu.dai@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.

Which means any kind of firmware loader which requires/uses
dev->struct_mutex get stuff wrong and is not appropriate everywhere.

> We should convert the DMC loader to use this too, so there need be only
> one bit of code in the whole driver that needs to understand how to use
> completions to get correct handover from a free-running no-locks-held
> thread to the properly disciplined environment of driver mainline for
> purposes of programming the h/w.

Nack on using this for dmc, since I want them to convert it to the above
synchronization, since that's how all the other async power initialization
is done.

Guc is different since we really must have it ready for execbuf, and for
that usecase a completion at drm_open time sounds like the right thing.

As a rule of thumb for refactoring and share infastructure we use the
following recipe in drm:
- first driver implements things as straightforward as possible
- 2nd user copypastes
- 3rd one has the duty to figure out whether some refactoring is in order
  or not.

Imo that approach leads a really good balance between avoiding
overengineering and having maintainable code.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx