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

[Sagar Arun Kamble <sagar.a.kamble@intel.com>]

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.

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

Thanks,
Sagar


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx