[Intel-gfx] [PATCH] drm/i915/skl: replace csr_mutex by completion in csr firmware loading

[Daniel Vetter]

On Thu, Jun 04, 2015 at 03:36:32PM +0100, Dave Gordon wrote:
> On 04/06/15 06:59, Sagar Arun Kamble wrote:
> > 
> > Hi Daniel,
> > 
> > We already are grabbing RPM reference before start of DMC FW load and
> > release post load completion.
> > 
> > DC5/6 can happen without Runtime PM as well. So we need to wait for CSR
> > FW load for some time once we disable PW2.
> > 
> > Having completion instead of csr.lock+csr.state is  correct thing to do.
> > 
> > Thanks,
> > Sagar
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> Hi guys,
> 
> we have a new unified loader for GuC and HuC, which we intend should be
> usable for any other microcontrollers that might be added in future. So
> it would be good to consider using it for the DMC/CSR/etc.
> 
> The most-recently posted version was Alex's on 2015-04-29, but we're
> just about to re-post an updated patchset for command submission via the
> GuC, which will include the both unified firmware loading module and the
> GuC-specific part of the loader.
> 
> Please have a look at these patches and let me know whether there's
> anything that would help with using the unified code for your uC(s).
> 
> http://lists.freedesktop.org/archives/intel-gfx/2015-April/065575.html
> http://lists.freedesktop.org/archives/intel-gfx/2015-April/065583.html
> 
> One thing that may not be obvious from the posted patches is that the
> uC-specific code can override the way the f/w image is saved e.g. if the
> loaded file image contains sections that are only needed once, they need
> not be saved; more generally, if the way that the data in the image file
> is organised doesn't match the way the data is used, the loader can
> shuffle the content around for the convenience of the final consumer.
> (We needed that at one stage, as the GuC firmware format didn't match
> the constraints of the GuC DMA engine; but the format has been fixed
> since, so the data-shuffler is not part of the public patch sequence).

Hm just looked at the generic firmware loader and not convinced this is a
good idea at all. As this discussion here shows synchronization here is a
tricky business. And in general driver load ordering is full of peril.
Trying to force a common abstraction for something rather trivial (we have
completions for the most basic case already) will only result in pains
trying to work around the not-quite-fitting infrastructure.

In my experience trying to extract common code at all costs is harmful
way too often.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch