| Message ID | 1583238415-18686-3-git-send-email-mkshah@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Invoke rpmh_flush for non OSI targets | expand |
Hi, On Tue, Mar 3, 2020 at 4:27 AM Maulik Shah <mkshah@codeaurora.org> wrote: > > Currently rpmh ctrlr dirty flag is set for all cases regardless of data > is really changed or not. Add changes to update dirty flag when data is > changed to newer values. Update dirty flag everytime when data in batch > cache is updated since rpmh_flush() may get invoked from any CPU instead > of only last CPU going to low power mode. > > Also move dirty flag updates to happen from within cache_lock and remove > unnecessary INIT_LIST_HEAD() call and a default case from switch. > > Fixes: 600513dfeef3 ("drivers: qcom: rpmh: cache sleep/wake state requests") > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > Reviewed-by: Srinivas Rao L <lsrao@codeaurora.org> > Reviewed-by: Evan Green <evgreen@chromium.org> > --- > drivers/soc/qcom/rpmh.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c > index eb0ded0..f28afe4 100644 > --- a/drivers/soc/qcom/rpmh.c > +++ b/drivers/soc/qcom/rpmh.c > @@ -133,26 +133,30 @@ static struct cache_req *cache_rpm_request(struct rpmh_ctrlr *ctrlr, > > req->addr = cmd->addr; > req->sleep_val = req->wake_val = UINT_MAX; > - INIT_LIST_HEAD(&req->list); > list_add_tail(&req->list, &ctrlr->cache); > > existing: > switch (state) { > case RPMH_ACTIVE_ONLY_STATE: > - if (req->sleep_val != UINT_MAX) > + if (req->sleep_val != UINT_MAX) { > req->wake_val = cmd->data; > + ctrlr->dirty = true; > + } You could maybe avoid a few additional "dirty" cases by changing the above "if" to: if (req->sleep_val != UINT_MAX && (req->wake_val != cmd->data) ...since otherwise writing an "ACTIVE_ONLY" thing over and over again with the same value would keep saying "dirty". Looking at this code makes me wonder a bit about how it's supposed to work, though. Let's look at a sequence of 3 commands called in two different orders: rpmh_write(RPMH_WAKE_ONLY_STATE, addr=0x10, data=0xaa); rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0xbb); ==> End result will be a cache entry (addr=0x10, wake=0xaa, sleep=0xbb) rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0xbb); rpmh_write(RPMH_WAKE_ONLY_STATE, addr=0x10, data=0xaa); rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); ==> End result will be a cache entry (addr=0x10, wake=0x99, sleep=0xbb) Said another way, it seems weird that a vote for "active" counts as a vote for "wake", but only if a sleep vote was made beforehand? Howzat? Maybe at one point in time it was assumed that wake's point was just to undo sleep? That is, if: state_orig = /* the state before sleep happens */ state_sleep = apply(state_orig, sleep_actions) state_wake = apply(state_sleep, wake_actions) The code is assuming "state_orig == state_wake". ...it sorta makes sense that "state_orig == state_wake" would be true, but if we were really making that requirement we really should have structured RPMH's APIs differently. We shouldn't have even allowed the callers to specify "WAKE_ONLY" state and we should have just constructed it from the "ACTIVE_ONLY" state. To summarize: a) If the only allowable use of "WAKE_ONLY" is to undo "SLEEP_ONLY" then we should re-think the API and stop letting callers to rpmh_write(), rpmh_write_async(), or rpmh_write_batch() ever specify "WAKE_ONLY". The code should just assume that "wake_only = active_only if (active_only != sleep_only)". In other words, RPMH should programmatically figure out the "wake" state based on the sleep/active state and not force callers to do this. b) If "WAKE_ONLY" is allowed to do other things (or if it's not RPMH's job to enforce/assume this) then we should fully skip calling cache_rpm_request() for RPMH_ACTIVE_ONLY_STATE. NOTE: this discussion also makes me wonder about the is_req_valid() function. That will skip sending a sleep/wake entry if the sleep and wake entries are equal to each other. ...but if sleep and wake are both different than "active" it'll be a problem. > break; > case RPMH_WAKE_ONLY_STATE: > - req->wake_val = cmd->data; > + if (req->wake_val != cmd->data) { > + req->wake_val = cmd->data; > + ctrlr->dirty = true; As far as I can tell from the code, you can also avoid dirty if req->sleep_val == UINT_MAX since nothing will be sent if either sleep_val or wake_val are UINT_MAX. Same in the sleep case where we can avoid dirty if wake_val == UINT_MAX. > + } > break; > case RPMH_SLEEP_STATE: > - req->sleep_val = cmd->data; > - break; > - default: > + if (req->sleep_val != cmd->data) { > + req->sleep_val = cmd->data; > + ctrlr->dirty = true; > + } > break; > } I wonder if instead of putting the dirty everywhere above it's better to cache the old value before the switch, then do: ctrl->dirty = (req->sleep_val != old_sleep_val || req->wake_val != old_wake_val) && req->sleep_val != UINT_MAX && req->wake_val != UINT_MAX; -Doug
On 3/5/2020 4:51 AM, Doug Anderson wrote: > Hi, > > On Tue, Mar 3, 2020 at 4:27 AM Maulik Shah <mkshah@codeaurora.org> wrote: >> Currently rpmh ctrlr dirty flag is set for all cases regardless of data >> is really changed or not. Add changes to update dirty flag when data is >> changed to newer values. Update dirty flag everytime when data in batch >> cache is updated since rpmh_flush() may get invoked from any CPU instead >> of only last CPU going to low power mode. >> >> Also move dirty flag updates to happen from within cache_lock and remove >> unnecessary INIT_LIST_HEAD() call and a default case from switch. >> >> Fixes: 600513dfeef3 ("drivers: qcom: rpmh: cache sleep/wake state requests") >> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> >> Reviewed-by: Srinivas Rao L <lsrao@codeaurora.org> >> Reviewed-by: Evan Green <evgreen@chromium.org> >> --- >> drivers/soc/qcom/rpmh.c | 21 +++++++++++++-------- >> 1 file changed, 13 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c >> index eb0ded0..f28afe4 100644 >> --- a/drivers/soc/qcom/rpmh.c >> +++ b/drivers/soc/qcom/rpmh.c >> @@ -133,26 +133,30 @@ static struct cache_req *cache_rpm_request(struct rpmh_ctrlr *ctrlr, >> >> req->addr = cmd->addr; >> req->sleep_val = req->wake_val = UINT_MAX; >> - INIT_LIST_HEAD(&req->list); >> list_add_tail(&req->list, &ctrlr->cache); >> >> existing: >> switch (state) { >> case RPMH_ACTIVE_ONLY_STATE: >> - if (req->sleep_val != UINT_MAX) >> + if (req->sleep_val != UINT_MAX) { >> req->wake_val = cmd->data; >> + ctrlr->dirty = true; >> + } > You could maybe avoid a few additional "dirty" cases by changing the > above "if" to: > > if (req->sleep_val != UINT_MAX && > (req->wake_val != cmd->data) > > ...since otherwise writing an "ACTIVE_ONLY" thing over and over again > with the same value would keep saying "dirty". > > > Looking at this code makes me wonder a bit about how it's supposed to > work, though. Let's look at a sequence of 3 commands called in two > different orders: > > rpmh_write(RPMH_WAKE_ONLY_STATE, addr=0x10, data=0xaa); > rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); > rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0xbb); > > ==> End result will be a cache entry (addr=0x10, wake=0xaa, sleep=0xbb) > > > rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0xbb); > rpmh_write(RPMH_WAKE_ONLY_STATE, addr=0x10, data=0xaa); > rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); > > ==> End result will be a cache entry (addr=0x10, wake=0x99, sleep=0xbb) > > > Said another way, it seems weird that a vote for "active" counts as a > vote for "wake", but only if a sleep vote was made beforehand? > Howzat? > > > Maybe at one point in time it was assumed that wake's point was just > to undo sleep? That is, if: > > state_orig = /* the state before sleep happens */ > state_sleep = apply(state_orig, sleep_actions) > state_wake = apply(state_sleep, wake_actions) > > The code is assuming "state_orig == state_wake". > > ...it sorta makes sense that "state_orig == state_wake" would be true, > but if we were really making that requirement we really should have > structured RPMH's APIs differently. We shouldn't have even allowed > the callers to specify "WAKE_ONLY" state and we should have just > constructed it from the "ACTIVE_ONLY" state. > > > To summarize: > > a) If the only allowable use of "WAKE_ONLY" is to undo "SLEEP_ONLY" > then we should re-think the API and stop letting callers to > rpmh_write(), rpmh_write_async(), or rpmh_write_batch() ever specify > "WAKE_ONLY". The code should just assume that "wake_only = > active_only if (active_only != sleep_only)". In other words, RPMH > should programmatically figure out the "wake" state based on the > sleep/active state and not force callers to do this. > > b) If "WAKE_ONLY" is allowed to do other things (or if it's not RPMH's > job to enforce/assume this) then we should fully skip calling > cache_rpm_request() for RPMH_ACTIVE_ONLY_STATE. > > > NOTE: this discussion also makes me wonder about the is_req_valid() > function. That will skip sending a sleep/wake entry if the sleep and > wake entries are equal to each other. ...but if sleep and wake are > both different than "active" it'll be a problem. Hi Doug, To answer above points, yes in general it’s the understanding that wake is almost always need to be equal to active. However, there can be valid reasons for which the callers are enforced to call them differently in the first place. At present caller send 3 types of request. rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0x0); rpmh_write(RPMH_WAKE_ONLY_STATE, addr=0x10, data=0x99); Now, Lets assume we handle this in rpmh driver since wake=active and the caller send only 2 type of request (lets call it active and sleep, since we have assumption of wake=active, and we don’t want 3rd request as its handled in rpmh driver) So callers will now invoke 2 types of request. rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0x0); with first type request, it now needs to serve 2 purpose (a) cache ACTIVE request votes as WAKE votes (b) trigger it out immediately (in ACTIVE TCS) as it need to be also complete immediately. For SLEEP, nothing changes. Now when entering to sleep we do rpmh_flush() to program all WAKE and SLEEP request…so far so good… Now consider a corner case, There is something called a solver mode in RSC where HW could be in autonomous mode executing low power modes. For this it may want to “only” program WAKE and SLEEP votes and then controller would be in solver mode entering and exiting sleep autonomously. There is no ACTIVE set request and hence no requirement to send it right away as ACTIVE vote. If we have only 2 type of request, caller again need to differentiate to tell rpmh driver that when it invoke rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); with this caching it as WAKE is fine ((a) in above) but do not trigger it ((b) in above) so we need to again modify this API and pass another argument saying whether to do (a + b) or only (a). but caller can already differentiate by using RPMH_ACTIVE_ONLY_STATE or RPMH_WAKE_ONLY_STATE. i think at least for now, leave it as it is, unless we really see any impact by caller invoking all 3 types of request and take in account all such corner cases before i make any such change. we can take it separate if needed along with optimization pointed in v9 series discussions. > >> break; >> case RPMH_WAKE_ONLY_STATE: >> - req->wake_val = cmd->data; >> + if (req->wake_val != cmd->data) { >> + req->wake_val = cmd->data; >> + ctrlr->dirty = true; > As far as I can tell from the code, you can also avoid dirty if > req->sleep_val == UINT_MAX since nothing will be sent if either > sleep_val or wake_val are UINT_MAX. Same in the sleep case where we > can avoid dirty if wake_val == UINT_MAX. > > >> + } >> break; >> case RPMH_SLEEP_STATE: >> - req->sleep_val = cmd->data; >> - break; >> - default: >> + if (req->sleep_val != cmd->data) { >> + req->sleep_val = cmd->data; >> + ctrlr->dirty = true; >> + } >> break; >> } > I wonder if instead of putting the dirty everywhere above it's better > to cache the old value before the switch, then do: > > ctrl->dirty = (req->sleep_val != old_sleep_val || > req->wake_val != old_wake_val) && > req->sleep_val != UINT_MAX && > req->wake_val != UINT_MAX; > > > -Doug Thanks, this seems good. v11 on the way. Thanks, Maulik
Hi, On Thu, Mar 5, 2020 at 3:10 AM Maulik Shah <mkshah@codeaurora.org> wrote: > > > To summarize: > > > > a) If the only allowable use of "WAKE_ONLY" is to undo "SLEEP_ONLY" > > then we should re-think the API and stop letting callers to > > rpmh_write(), rpmh_write_async(), or rpmh_write_batch() ever specify > > "WAKE_ONLY". The code should just assume that "wake_only = > > active_only if (active_only != sleep_only)". In other words, RPMH > > should programmatically figure out the "wake" state based on the > > sleep/active state and not force callers to do this. > > > > b) If "WAKE_ONLY" is allowed to do other things (or if it's not RPMH's > > job to enforce/assume this) then we should fully skip calling > > cache_rpm_request() for RPMH_ACTIVE_ONLY_STATE. > > > > > > NOTE: this discussion also makes me wonder about the is_req_valid() > > function. That will skip sending a sleep/wake entry if the sleep and > > wake entries are equal to each other. ...but if sleep and wake are > > both different than "active" it'll be a problem. > > Hi Doug, > > To answer above points, yes in general it’s the understanding that wake is > almost always need to be equal to active. However, there can be valid reasons > for which the callers are enforced to call them differently in the first place. > > At present caller send 3 types of request. > rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); > rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0x0); > rpmh_write(RPMH_WAKE_ONLY_STATE, addr=0x10, data=0x99); > > Now, Lets assume we handle this in rpmh driver since wake=active and the caller > send only 2 type of request (lets call it active and sleep, since we have assumption > of wake=active, and we don’t want 3rd request as its handled in rpmh driver) > So callers will now invoke 2 types of request. > > rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); > rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0x0); > > with first type request, it now needs to serve 2 purpose > (a) cache ACTIVE request votes as WAKE votes > (b) trigger it out immediately (in ACTIVE TCS) as it need to be also complete immediately. > > For SLEEP, nothing changes. Now when entering to sleep we do rpmh_flush() to program all > WAKE and SLEEP request…so far so good… > > Now consider a corner case, > > There is something called a solver mode in RSC where HW could be in autonomous mode executing > low power modes. For this it may want to “only” program WAKE and SLEEP votes and then controller > would be in solver mode entering and exiting sleep autonomously. > > There is no ACTIVE set request and hence no requirement to send it right away as ACTIVE vote. > > If we have only 2 type of request, caller again need to differentiate to tell rpmh driver that > when it invoke > > rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); > > with this caching it as WAKE is fine ((a) in above) but do not trigger it ((b) in above) > > so we need to again modify this API and pass another argument saying whether to do (a + b) or only (a). > but caller can already differentiate by using RPMH_ACTIVE_ONLY_STATE or RPMH_WAKE_ONLY_STATE. > > i think at least for now, leave it as it is, unless we really see any impact by caller invoking all > 3 types of request and take in account all such corner cases before i make any such change. > we can take it separate if needed along with optimization pointed in v9 series discussions. I totally don't understand what solver mode is for and when it's used, but I'm willing to set that aside for now I guess. From looking at what you did for v12 it looks like the way you're expecting things to function is this: * ACTIVE: set wake state and trigger active change right away. * SLEEP: set only sleep state * WAKE: set only wake state, will take effect after next sleep/wake unless changed again before that happens. ...I'll look at the code with this understanding, now. Presumably also: * We should document this. * If we see clients that are explicitly setting _both_ active and wake to the same thing then we can change the clients. That means the only people using "WAKE" mode would be those clients that explicitly want the deferred action (presumably those using "solver" mode). Do those seem correct? If that's correct, I guess one subtle corner-case bug in is_req_valid(). Specifically if it's ever valid to do this: rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0x0); rpmh_write(RPMH_WAKE_ONLY_STATE, addr=0x10, data=0x0); ...then it won't work. You'll transition between sleep/wake and stay with "data=0x99". Since "sleep == wake" then is_req_valid() will return "false" and we won't bother programming the commands for sleep/wake. One simple way to solve this is remove the "req->sleep_val != req->wake_val" optimization in is_req_valid(). I guess we should also document that "batch" doesn't work like this. The "batch" API is really designed around having exactly one "batch" caller (the interconnect code) and we assume that the batch code will be sending us pre-optimized commands I guess? Specifically there doesn't seem to be anything trying to catch batch writes to "active" and also applying them to "wake". -Doug
On 3/6/2020 3:52 AM, Doug Anderson wrote: > Hi, > > On Thu, Mar 5, 2020 at 3:10 AM Maulik Shah <mkshah@codeaurora.org> wrote: >>> To summarize: >>> >>> a) If the only allowable use of "WAKE_ONLY" is to undo "SLEEP_ONLY" >>> then we should re-think the API and stop letting callers to >>> rpmh_write(), rpmh_write_async(), or rpmh_write_batch() ever specify >>> "WAKE_ONLY". The code should just assume that "wake_only = >>> active_only if (active_only != sleep_only)". In other words, RPMH >>> should programmatically figure out the "wake" state based on the >>> sleep/active state and not force callers to do this. >>> >>> b) If "WAKE_ONLY" is allowed to do other things (or if it's not RPMH's >>> job to enforce/assume this) then we should fully skip calling >>> cache_rpm_request() for RPMH_ACTIVE_ONLY_STATE. >>> >>> >>> NOTE: this discussion also makes me wonder about the is_req_valid() >>> function. That will skip sending a sleep/wake entry if the sleep and >>> wake entries are equal to each other. ...but if sleep and wake are >>> both different than "active" it'll be a problem. >> Hi Doug, >> >> To answer above points, yes in general it’s the understanding that wake is >> almost always need to be equal to active. However, there can be valid reasons >> for which the callers are enforced to call them differently in the first place. >> >> At present caller send 3 types of request. >> rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); >> rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0x0); >> rpmh_write(RPMH_WAKE_ONLY_STATE, addr=0x10, data=0x99); >> >> Now, Lets assume we handle this in rpmh driver since wake=active and the caller >> send only 2 type of request (lets call it active and sleep, since we have assumption >> of wake=active, and we don’t want 3rd request as its handled in rpmh driver) >> So callers will now invoke 2 types of request. >> >> rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); >> rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0x0); >> >> with first type request, it now needs to serve 2 purpose >> (a) cache ACTIVE request votes as WAKE votes >> (b) trigger it out immediately (in ACTIVE TCS) as it need to be also complete immediately. >> >> For SLEEP, nothing changes. Now when entering to sleep we do rpmh_flush() to program all >> WAKE and SLEEP request…so far so good… >> >> Now consider a corner case, >> >> There is something called a solver mode in RSC where HW could be in autonomous mode executing >> low power modes. For this it may want to “only” program WAKE and SLEEP votes and then controller >> would be in solver mode entering and exiting sleep autonomously. >> >> There is no ACTIVE set request and hence no requirement to send it right away as ACTIVE vote. >> >> If we have only 2 type of request, caller again need to differentiate to tell rpmh driver that >> when it invoke >> >> rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); >> >> with this caching it as WAKE is fine ((a) in above) but do not trigger it ((b) in above) >> >> so we need to again modify this API and pass another argument saying whether to do (a + b) or only (a). >> but caller can already differentiate by using RPMH_ACTIVE_ONLY_STATE or RPMH_WAKE_ONLY_STATE. >> >> i think at least for now, leave it as it is, unless we really see any impact by caller invoking all >> 3 types of request and take in account all such corner cases before i make any such change. >> we can take it separate if needed along with optimization pointed in v9 series discussions. > I totally don't understand what solver mode is for and when it's used, > but I'm willing to set that aside for now I guess. From looking at > what you did for v12 it looks like the way you're expecting things to > function is this: > > * ACTIVE: set wake state and trigger active change right away. > > * SLEEP: set only sleep state > > * WAKE: set only wake state, will take effect after next sleep/wake > unless changed again before that happens. > > > ...I'll look at the code with this understanding, now. Presumably also: > > * We should document this. Okay, i will document above. > * If we see clients that are explicitly setting _both_ active and wake > to the same thing then we can change the clients. That means the only > people using "WAKE" mode would be those clients that explicitly want > the deferred action (presumably those using "solver" mode). > > Do those seem correct? Correct. but i suggest to change clients only once solver mode changes go in. until then leave clients to call ACTIVE and WAKE request separately (even with same data) > > If that's correct, I guess one subtle corner-case bug in > is_req_valid(). Specifically if it's ever valid to do this: > > rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); > rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0x0); > rpmh_write(RPMH_WAKE_ONLY_STATE, addr=0x10, data=0x0); This scenario will never hit in solver mode. when in solver, only WAKE and SLEEP requests are allowed to go through. > ...then it won't work. will work out just fine, as said above. > You'll transition between sleep/wake and stay > with "data=0x99". Since "sleep == wake" then is_req_valid() will > return "false" and we won't bother programming the commands for > sleep/wake. One simple way to solve this is remove the > "req->sleep_val != req->wake_val" optimization in is_req_valid(). This will still need to keep check. the clients may invoke with below example data... rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0x99); rpmh_write(RPMH_WAKE_ONLY_STATE, addr=0x10, data=0x99); (we assume wake=active) while ACTIVE is immediatly sent, and resource already came to 0x99 level. Now while flushing, there is no point in programming in SLEEP TCS as such when cmd triggers from SLEEP TCS then it won't make any real level transition since its already brought up to 0x99 level with previous ACTIVE cmd. same reason goes for not programming it in WAKE TCS. > > I guess we should also document that "batch" doesn't work like this. > The "batch" API is really designed around having exactly one "batch" > caller (the interconnect code) and we assume that the batch code will > be sending us pre-optimized commands I guess? Specifically there > doesn't seem to be anything trying to catch batch writes to "active" > and also applying them to "wake". Okay, i will document above. Thanks, Maulik > -Doug
Hi, On Tue, Mar 10, 2020 at 4:03 AM Maulik Shah <mkshah@codeaurora.org> wrote: > > > On 3/6/2020 3:52 AM, Doug Anderson wrote: > > Hi, > > > > On Thu, Mar 5, 2020 at 3:10 AM Maulik Shah <mkshah@codeaurora.org> wrote: > >>> To summarize: > >>> > >>> a) If the only allowable use of "WAKE_ONLY" is to undo "SLEEP_ONLY" > >>> then we should re-think the API and stop letting callers to > >>> rpmh_write(), rpmh_write_async(), or rpmh_write_batch() ever specify > >>> "WAKE_ONLY". The code should just assume that "wake_only = > >>> active_only if (active_only != sleep_only)". In other words, RPMH > >>> should programmatically figure out the "wake" state based on the > >>> sleep/active state and not force callers to do this. > >>> > >>> b) If "WAKE_ONLY" is allowed to do other things (or if it's not RPMH's > >>> job to enforce/assume this) then we should fully skip calling > >>> cache_rpm_request() for RPMH_ACTIVE_ONLY_STATE. > >>> > >>> > >>> NOTE: this discussion also makes me wonder about the is_req_valid() > >>> function. That will skip sending a sleep/wake entry if the sleep and > >>> wake entries are equal to each other. ...but if sleep and wake are > >>> both different than "active" it'll be a problem. > >> Hi Doug, > >> > >> To answer above points, yes in general it’s the understanding that wake is > >> almost always need to be equal to active. However, there can be valid reasons > >> for which the callers are enforced to call them differently in the first place. > >> > >> At present caller send 3 types of request. > >> rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); > >> rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0x0); > >> rpmh_write(RPMH_WAKE_ONLY_STATE, addr=0x10, data=0x99); > >> > >> Now, Lets assume we handle this in rpmh driver since wake=active and the caller > >> send only 2 type of request (lets call it active and sleep, since we have assumption > >> of wake=active, and we don’t want 3rd request as its handled in rpmh driver) > >> So callers will now invoke 2 types of request. > >> > >> rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); > >> rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0x0); > >> > >> with first type request, it now needs to serve 2 purpose > >> (a) cache ACTIVE request votes as WAKE votes > >> (b) trigger it out immediately (in ACTIVE TCS) as it need to be also complete immediately. > >> > >> For SLEEP, nothing changes. Now when entering to sleep we do rpmh_flush() to program all > >> WAKE and SLEEP request…so far so good… > >> > >> Now consider a corner case, > >> > >> There is something called a solver mode in RSC where HW could be in autonomous mode executing > >> low power modes. For this it may want to “only” program WAKE and SLEEP votes and then controller > >> would be in solver mode entering and exiting sleep autonomously. > >> > >> There is no ACTIVE set request and hence no requirement to send it right away as ACTIVE vote. > >> > >> If we have only 2 type of request, caller again need to differentiate to tell rpmh driver that > >> when it invoke > >> > >> rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); > >> > >> with this caching it as WAKE is fine ((a) in above) but do not trigger it ((b) in above) > >> > >> so we need to again modify this API and pass another argument saying whether to do (a + b) or only (a). > >> but caller can already differentiate by using RPMH_ACTIVE_ONLY_STATE or RPMH_WAKE_ONLY_STATE. > >> > >> i think at least for now, leave it as it is, unless we really see any impact by caller invoking all > >> 3 types of request and take in account all such corner cases before i make any such change. > >> we can take it separate if needed along with optimization pointed in v9 series discussions. > > I totally don't understand what solver mode is for and when it's used, > > but I'm willing to set that aside for now I guess. From looking at > > what you did for v12 it looks like the way you're expecting things to > > function is this: > > > > * ACTIVE: set wake state and trigger active change right away. > > > > * SLEEP: set only sleep state > > > > * WAKE: set only wake state, will take effect after next sleep/wake > > unless changed again before that happens. > > > > > > ...I'll look at the code with this understanding, now. Presumably also: > > > > * We should document this. > Okay, i will document above. > > * If we see clients that are explicitly setting _both_ active and wake > > to the same thing then we can change the clients. That means the only > > people using "WAKE" mode would be those clients that explicitly want > > the deferred action (presumably those using "solver" mode). > > > > Do those seem correct? > Correct. but i suggest to change clients only once solver mode changes go in. > until then leave clients to call ACTIVE and WAKE request separately (even with same data) If you want to leave clients to call ACTIVE and WAKE requests separately for now, then please change your patch ("soc: qcom: rpmh: Update dirty flag only when data changes"), which (from v13) has this in cache_rpm_request(): switch (state) { case RPMH_ACTIVE_ONLY_STATE: case RPMH_WAKE_ONLY_STATE: req->wake_val = cmd->data; ...that bit of code is what led me to request that you document that setting the active-only state also sets the wake-only state, so either change that code or add the documentation. > > If that's correct, I guess one subtle corner-case bug in > > is_req_valid(). Specifically if it's ever valid to do this: > > > > rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); > > rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0x0); > > rpmh_write(RPMH_WAKE_ONLY_STATE, addr=0x10, data=0x0); > This scenario will never hit in solver mode. > when in solver, only WAKE and SLEEP requests are allowed to go through. OK, I guess this stems from me not understanding solver mode. I guess for now it's unlikely to cause problems given the current usage, so we can ignore for now. > > ...then it won't work. > will work out just fine, as said above. > > You'll transition between sleep/wake and stay > > with "data=0x99". Since "sleep == wake" then is_req_valid() will > > return "false" and we won't bother programming the commands for > > sleep/wake. One simple way to solve this is remove the > > "req->sleep_val != req->wake_val" optimization in is_req_valid(). > > This will still need to keep check. > > the clients may invoke with below example data... > > rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); > rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0x99); > rpmh_write(RPMH_WAKE_ONLY_STATE, addr=0x10, data=0x99); (we assume wake=active) > > while ACTIVE is immediatly sent, and resource already came to 0x99 level. > > Now while flushing, there is no point in programming in SLEEP TCS as such when cmd triggers > from SLEEP TCS then it won't make any real level transition since its already brought up to > 0x99 level with previous ACTIVE cmd. same reason goes for not programming it in WAKE TCS. "Need" is perhaps too strong of a word if I understand things. The example above would still work even if we didn't check "sleep == wake" it would just program an extra (but pointless) command. Right? ...but I guess we can debate about it later. For now I'm not aware of anyone setting WAKE to something other than ACTIVE. -Doug
Hi, On 3/10/2020 9:16 PM, Doug Anderson wrote: > Hi, > > > On Tue, Mar 10, 2020 at 4:03 AM Maulik Shah <mkshah@codeaurora.org> wrote: >> >> On 3/6/2020 3:52 AM, Doug Anderson wrote: >>> Hi, >>> >>> On Thu, Mar 5, 2020 at 3:10 AM Maulik Shah <mkshah@codeaurora.org> wrote: >>>>> To summarize: >>>>> >>>>> a) If the only allowable use of "WAKE_ONLY" is to undo "SLEEP_ONLY" >>>>> then we should re-think the API and stop letting callers to >>>>> rpmh_write(), rpmh_write_async(), or rpmh_write_batch() ever specify >>>>> "WAKE_ONLY". The code should just assume that "wake_only = >>>>> active_only if (active_only != sleep_only)". In other words, RPMH >>>>> should programmatically figure out the "wake" state based on the >>>>> sleep/active state and not force callers to do this. >>>>> >>>>> b) If "WAKE_ONLY" is allowed to do other things (or if it's not RPMH's >>>>> job to enforce/assume this) then we should fully skip calling >>>>> cache_rpm_request() for RPMH_ACTIVE_ONLY_STATE. >>>>> >>>>> >>>>> NOTE: this discussion also makes me wonder about the is_req_valid() >>>>> function. That will skip sending a sleep/wake entry if the sleep and >>>>> wake entries are equal to each other. ...but if sleep and wake are >>>>> both different than "active" it'll be a problem. >>>> Hi Doug, >>>> >>>> To answer above points, yes in general it’s the understanding that wake is >>>> almost always need to be equal to active. However, there can be valid reasons >>>> for which the callers are enforced to call them differently in the first place. >>>> >>>> At present caller send 3 types of request. >>>> rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); >>>> rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0x0); >>>> rpmh_write(RPMH_WAKE_ONLY_STATE, addr=0x10, data=0x99); >>>> >>>> Now, Lets assume we handle this in rpmh driver since wake=active and the caller >>>> send only 2 type of request (lets call it active and sleep, since we have assumption >>>> of wake=active, and we don’t want 3rd request as its handled in rpmh driver) >>>> So callers will now invoke 2 types of request. >>>> >>>> rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); >>>> rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0x0); >>>> >>>> with first type request, it now needs to serve 2 purpose >>>> (a) cache ACTIVE request votes as WAKE votes >>>> (b) trigger it out immediately (in ACTIVE TCS) as it need to be also complete immediately. >>>> >>>> For SLEEP, nothing changes. Now when entering to sleep we do rpmh_flush() to program all >>>> WAKE and SLEEP request…so far so good… >>>> >>>> Now consider a corner case, >>>> >>>> There is something called a solver mode in RSC where HW could be in autonomous mode executing >>>> low power modes. For this it may want to “only” program WAKE and SLEEP votes and then controller >>>> would be in solver mode entering and exiting sleep autonomously. >>>> >>>> There is no ACTIVE set request and hence no requirement to send it right away as ACTIVE vote. >>>> >>>> If we have only 2 type of request, caller again need to differentiate to tell rpmh driver that >>>> when it invoke >>>> >>>> rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); >>>> >>>> with this caching it as WAKE is fine ((a) in above) but do not trigger it ((b) in above) >>>> >>>> so we need to again modify this API and pass another argument saying whether to do (a + b) or only (a). >>>> but caller can already differentiate by using RPMH_ACTIVE_ONLY_STATE or RPMH_WAKE_ONLY_STATE. >>>> >>>> i think at least for now, leave it as it is, unless we really see any impact by caller invoking all >>>> 3 types of request and take in account all such corner cases before i make any such change. >>>> we can take it separate if needed along with optimization pointed in v9 series discussions. >>> I totally don't understand what solver mode is for and when it's used, >>> but I'm willing to set that aside for now I guess. From looking at >>> what you did for v12 it looks like the way you're expecting things to >>> function is this: >>> >>> * ACTIVE: set wake state and trigger active change right away. >>> >>> * SLEEP: set only sleep state >>> >>> * WAKE: set only wake state, will take effect after next sleep/wake >>> unless changed again before that happens. >>> >>> >>> ...I'll look at the code with this understanding, now. Presumably also: >>> >>> * We should document this. >> Okay, i will document above. >>> * If we see clients that are explicitly setting _both_ active and wake >>> to the same thing then we can change the clients. That means the only >>> people using "WAKE" mode would be those clients that explicitly want >>> the deferred action (presumably those using "solver" mode). >>> >>> Do those seem correct? >> Correct. but i suggest to change clients only once solver mode changes go in. >> until then leave clients to call ACTIVE and WAKE request separately (even with same data) > If you want to leave clients to call ACTIVE and WAKE requests > separately for now, then please change your patch ("soc: qcom: rpmh: > Update dirty flag only when data changes"), which (from v13) has this > in cache_rpm_request(): > > switch (state) { > case RPMH_ACTIVE_ONLY_STATE: > case RPMH_WAKE_ONLY_STATE: > req->wake_val = cmd->data; > > ...that bit of code is what led me to request that you document that > setting the active-only state also sets the wake-only state, so either > change that code or add the documentation. yes, i will add the documentation. > > >>> If that's correct, I guess one subtle corner-case bug in >>> is_req_valid(). Specifically if it's ever valid to do this: >>> >>> rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); >>> rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0x0); >>> rpmh_write(RPMH_WAKE_ONLY_STATE, addr=0x10, data=0x0); >> This scenario will never hit in solver mode. >> when in solver, only WAKE and SLEEP requests are allowed to go through. > OK, I guess this stems from me not understanding solver mode. I guess > for now it's unlikely to cause problems given the current usage, so we > can ignore for now. > > >>> ...then it won't work. >> will work out just fine, as said above. >>> You'll transition between sleep/wake and stay >>> with "data=0x99". Since "sleep == wake" then is_req_valid() will >>> return "false" and we won't bother programming the commands for >>> sleep/wake. One simple way to solve this is remove the >>> "req->sleep_val != req->wake_val" optimization in is_req_valid(). >> This will still need to keep check. >> >> the clients may invoke with below example data... >> >> rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99); >> rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0x99); >> rpmh_write(RPMH_WAKE_ONLY_STATE, addr=0x10, data=0x99); (we assume wake=active) >> >> while ACTIVE is immediatly sent, and resource already came to 0x99 level. >> >> Now while flushing, there is no point in programming in SLEEP TCS as such when cmd triggers >> from SLEEP TCS then it won't make any real level transition since its already brought up to >> 0x99 level with previous ACTIVE cmd. same reason goes for not programming it in WAKE TCS. > "Need" is perhaps too strong of a word if I understand things. The > example above would still work even if we didn't check "sleep == wake" > it would just program an extra (but pointless) command. Right? > > ...but I guess we can debate about it later. For now I'm not aware of > anyone setting WAKE to something other than ACTIVE. > > > -Doug its not just programming an extra command, neither it is pointless. its needed to keep it that way. let me make explain effect of this command... the resource for which this extra command will be sent is already at required level. so below might happen in my understanding. Resource may be in its own low power mode, when this extra command is sent, it needs to come out of low power mode and apply the newly requested level in sleep command but it is already at that level, so without doing any real level transition, it will again go back to sleep. same happens for wake command. so there can be a power hit. so this extra command need not sent. Thanks, Maulik
diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c index eb0ded0..f28afe4 100644 --- a/drivers/soc/qcom/rpmh.c +++ b/drivers/soc/qcom/rpmh.c @@ -133,26 +133,30 @@ static struct cache_req *cache_rpm_request(struct rpmh_ctrlr *ctrlr, req->addr = cmd->addr; req->sleep_val = req->wake_val = UINT_MAX; - INIT_LIST_HEAD(&req->list); list_add_tail(&req->list, &ctrlr->cache); existing: switch (state) { case RPMH_ACTIVE_ONLY_STATE: - if (req->sleep_val != UINT_MAX) + if (req->sleep_val != UINT_MAX) { req->wake_val = cmd->data; + ctrlr->dirty = true; + } break; case RPMH_WAKE_ONLY_STATE: - req->wake_val = cmd->data; + if (req->wake_val != cmd->data) { + req->wake_val = cmd->data; + ctrlr->dirty = true; + } break; case RPMH_SLEEP_STATE: - req->sleep_val = cmd->data; - break; - default: + if (req->sleep_val != cmd->data) { + req->sleep_val = cmd->data; + ctrlr->dirty = true; + } break; } - ctrlr->dirty = true; unlock: spin_unlock_irqrestore(&ctrlr->cache_lock, flags); @@ -287,6 +291,7 @@ static void cache_batch(struct rpmh_ctrlr *ctrlr, struct batch_cache_req *req) spin_lock_irqsave(&ctrlr->cache_lock, flags); list_add_tail(&req->list, &ctrlr->batch_cache); + ctrlr->dirty = true; spin_unlock_irqrestore(&ctrlr->cache_lock, flags); } @@ -323,6 +328,7 @@ static void invalidate_batch(struct rpmh_ctrlr *ctrlr) list_for_each_entry_safe(req, tmp, &ctrlr->batch_cache, list) kfree(req); INIT_LIST_HEAD(&ctrlr->batch_cache); + ctrlr->dirty = true; spin_unlock_irqrestore(&ctrlr->cache_lock, flags); } @@ -507,7 +513,6 @@ int rpmh_invalidate(const struct device *dev) int ret; invalidate_batch(ctrlr); - ctrlr->dirty = true; do { ret = rpmh_rsc_invalidate(ctrlr_to_drv(ctrlr));
Currently rpmh ctrlr dirty flag is set for all cases regardless of data is really changed or not. Add changes to update dirty flag when data is changed to newer values. Update dirty flag everytime when data in batch cache is updated since rpmh_flush() may get invoked from any CPU instead of only last CPU going to low power mode. Also move dirty flag updates to happen from within cache_lock and remove unnecessary INIT_LIST_HEAD() call and a default case from switch. Fixes: 600513dfeef3 ("drivers: qcom: rpmh: cache sleep/wake state requests") Signed-off-by: Maulik Shah <mkshah@codeaurora.org> Reviewed-by: Srinivas Rao L <lsrao@codeaurora.org> Reviewed-by: Evan Green <evgreen@chromium.org> --- drivers/soc/qcom/rpmh.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)