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

[Daniel Vetter]

On Thu, May 21, 2015 at 10:35:07PM +0530, Animesh Manna wrote:
> 
> 
> On 5/21/2015 5:41 PM, Daniel Vetter wrote:
> >On Thu, May 21, 2015 at 03:49:52PM +0530, Animesh Manna wrote:
> >>Before enabling dc5/dc6, used wait for completion instead of busy waiting.
> >>
> >>v1:
> >>- Based on review comment from Daniel replaced mutex and related
> >>implementation with completion. In current patch not used power
> >>domain lock, don't want to block runtime power management if dmc
> >>firmware failed to load. Will analyzing further and possibly send
> >>as a incremental patch.
> >>- Based on review comment from Damien, warning for firmware loading
> >>failure is removed.
> >>
> >>Signed-off-by: Animesh Manna <animesh.manna at intel.com>
> >Sorry if this cross with my comments, but upon further digging into this
> >code we don't even need a completion at all. As long as we prevent the
> >power well from getting disabled by holding a reference for it before we
> >launch the firmware loader it'll all be fine. And the async work can then
> >just drop that reference when everything is set up.
> >
> >Yes this means runtime D3 requires the firmware to be loaded, but that's
> >imo not a problem.
> >
> >Also doing it this way is more in-line with how we do all the other async
> >setup. But there's another serious issue with this design here, see below.
> >
> Ok, previously I was thinking more on how to pass the firmware loading status
> before enabling low power states (dc5/dc6). Now while thinking about power
> management framework I have a fundamental doubt - if got request to disable a power-well
> and firmware loading failed for some reason, should we disable the power-well(option1) or
> keep it enable(option2). Both the cases will not trigger for dc5/dc6.
> 
> I understood from your comment to follow option2, I will change the design accordingly
> as the current implementation based on option1.
> 
> To implement option2, I can see there is one mutex present for a power domain, do not have
> mutex for each power-well, which need to be added and can be used as you mentioned above.
> 
> If we want to follow option1, I can think a better way of managing firmware loading.
> As firmware is required when display engine goes to low power state, so we can only parse
> the packaged firmware during driver initialization. Firmware loading (writing to registers)
> can be done in skl_enable_dc6/gen9_enable_dc5 function itself and then trigger for dc5/dc6.
> 
> Planning to implement option2 and will send for review, please correct me if I understood wrongly.

power wells are reference counted, which means the only thing you need to
do is grab such a reference. Then the corresponding power well won't ever
get disabled until the async firmware loader has done it's job and
released the power well reference again.

See e.g. how the async rps setup grabs a runtime_pm reference (works the
same for power wells, they simply nest within the runtime pm stuff). So
for option2 no fiddling with mutexes or power well internals needed at all
(well the wait_for can be removed afterwards ofc). And we don't need any
other mutex, the csr_mutex and crs.state can be removed afterwards too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch