| Message ID | 1580796831-18996-2-git-send-email-mkshah@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Misc stability fixes and optimization for rpmh driver | expand |
On Mon, Feb 3, 2020 at 10:14 PM 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 it when data is updated to new values. > > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > --- > drivers/soc/qcom/rpmh.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c > index 035091f..c3d6f00 100644 > --- a/drivers/soc/qcom/rpmh.c > +++ b/drivers/soc/qcom/rpmh.c > @@ -139,20 +139,27 @@ static struct cache_req *cache_rpm_request(struct rpmh_ctrlr *ctrlr, > 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; > + } Don't you need to set dirty = true for ACTIVE_ONLY state always? The conditional is just saying "if nobody set a sleep vote, then maintain this vote when we wake back up".
On 2/5/2020 6:05 AM, Evan Green wrote: > On Mon, Feb 3, 2020 at 10:14 PM 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 it when data is updated to new values. >> >> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> >> --- >> drivers/soc/qcom/rpmh.c | 15 +++++++++++---- >> 1 file changed, 11 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c >> index 035091f..c3d6f00 100644 >> --- a/drivers/soc/qcom/rpmh.c >> +++ b/drivers/soc/qcom/rpmh.c >> @@ -139,20 +139,27 @@ static struct cache_req *cache_rpm_request(struct rpmh_ctrlr *ctrlr, >> 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; >> + } > Don't you need to set dirty = true for ACTIVE_ONLY state always? The > conditional is just saying "if nobody set a sleep vote, then maintain > this vote when we wake back up". The ACTIVE_ONLY vote is cached as wake_val to be apply when wakeup happens. In case value didn't change,wake_val is still same as older value and there is no need to mark the entire cache as dirty.
On Tue, Feb 4, 2020 at 8:14 PM Maulik Shah <mkshah@codeaurora.org> wrote: > > > On 2/5/2020 6:05 AM, Evan Green wrote: > > On Mon, Feb 3, 2020 at 10:14 PM 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 it when data is updated to new values. > >> > >> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > >> --- > >> drivers/soc/qcom/rpmh.c | 15 +++++++++++---- > >> 1 file changed, 11 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c > >> index 035091f..c3d6f00 100644 > >> --- a/drivers/soc/qcom/rpmh.c > >> +++ b/drivers/soc/qcom/rpmh.c > >> @@ -139,20 +139,27 @@ static struct cache_req *cache_rpm_request(struct rpmh_ctrlr *ctrlr, > >> 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; > >> + } > > Don't you need to set dirty = true for ACTIVE_ONLY state always? The > > conditional is just saying "if nobody set a sleep vote, then maintain > > this vote when we wake back up". > > The ACTIVE_ONLY vote is cached as wake_val to be apply when wakeup happens. > > In case value didn't change,wake_val is still same as older value and > there is no need to mark the entire cache as dirty. > Ah, I see it now. We don't actually cache active_only votes anywhere, since they're one time requests. The sleep/wake votes seem to be the only thing that gets cached. I was thinking it might be safer to also set dirty = true just after list_add_tail, since in the non-existing case this is a new batch that RPMh has never seen before and should always be written. But I suppose your checks here should cover that case, since sleep_val and wake_val are initialized to UINT_MAX. If you think the code might evolve, it might still be nice to add it. While I'm looking at that, why do we have this needless INIT_LIST_HEAD? INIT_LIST_HEAD(&req->list); list_add_tail(&req->list, &ctrlr->cache); -Evan > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
On 2/5/2020 11:37 PM, Evan Green wrote: > On Tue, Feb 4, 2020 at 8:14 PM Maulik Shah <mkshah@codeaurora.org> wrote: >> >> On 2/5/2020 6:05 AM, Evan Green wrote: >>> On Mon, Feb 3, 2020 at 10:14 PM 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 it when data is updated to new values. >>>> >>>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> >>>> --- >>>> drivers/soc/qcom/rpmh.c | 15 +++++++++++---- >>>> 1 file changed, 11 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c >>>> index 035091f..c3d6f00 100644 >>>> --- a/drivers/soc/qcom/rpmh.c >>>> +++ b/drivers/soc/qcom/rpmh.c >>>> @@ -139,20 +139,27 @@ static struct cache_req *cache_rpm_request(struct rpmh_ctrlr *ctrlr, >>>> 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; >>>> + } >>> Don't you need to set dirty = true for ACTIVE_ONLY state always? The >>> conditional is just saying "if nobody set a sleep vote, then maintain >>> this vote when we wake back up". >> The ACTIVE_ONLY vote is cached as wake_val to be apply when wakeup happens. >> >> In case value didn't change,wake_val is still same as older value and >> there is no need to mark the entire cache as dirty. >> > Ah, I see it now. We don't actually cache active_only votes anywhere, > since they're one time requests. The sleep/wake votes seem to be the > only thing that gets cached. > > I was thinking it might be safer to also set dirty = true just after > list_add_tail, since in the non-existing case this is a new batch that > RPMh has never seen before and should always be written. But I suppose > your checks here should cover that case, since sleep_val and wake_val > are initialized to UINT_MAX. If you think the code might evolve, it > might still be nice to add it. current change seems good. > > While I'm looking at that, why do we have this needless INIT_LIST_HEAD? > INIT_LIST_HEAD(&req->list); > list_add_tail(&req->list, &ctrlr->cache); > > -Evan Thanks for pointing this, i will remove this unnecessary INIT in next revision. > >> -- >> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c index 035091f..c3d6f00 100644 --- a/drivers/soc/qcom/rpmh.c +++ b/drivers/soc/qcom/rpmh.c @@ -139,20 +139,27 @@ static struct cache_req *cache_rpm_request(struct rpmh_ctrlr *ctrlr, 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; + if (req->sleep_val != cmd->data) { + req->sleep_val = cmd->data; + ctrlr->dirty = true; + } break; default: break; } - ctrlr->dirty = true; unlock: spin_unlock_irqrestore(&ctrlr->cache_lock, flags);
Currently rpmh ctrlr dirty flag is set for all cases regardless of data is really changed or not. Add changes to update it when data is updated to new values. Signed-off-by: Maulik Shah <mkshah@codeaurora.org> --- drivers/soc/qcom/rpmh.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)