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

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jun 04, 2015 at 11:29:35AM +0530, Sagar Arun Kamble wrote:
> On Thu, 2015-05-21 at 23:29 +0200, Daniel Vetter wrote:
> > 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@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
> 
> 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.

If dc5/6 can happen despite you holding a runtime pm reference, then
you're holding the wrong runtime pm reference. They nest, hence just walk
up the chain of power domains until you have one that does prevent dc5/6.

I guess there's a confusion between runtime pm as the overall concept
(which I'm talking about here, implemented in intel_rpm.c) and the linux
runtime pm api which is only used for the very last level of runtime pm in
i915 to control entry/exit to D3. On top of that we stack the power wells
i915 specific runtime pm api and the corresponding busyness tracking on
the render side.

> Having completion instead of csr.lock+csr.state is  correct thing to do.

I hope the above explanation helps. I can follow up with a sketch patch
too if you want.

Cheers, 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