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@xxxxxxxxx> > > >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@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
