On Thu, 18 Jul 2024 at 05:06, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote: > > On 11-07-24, 17:25, Ulf Hansson wrote: > > Right, I get your point. > > > > Although, it seems to me that just limiting required-opps to > > performance-levels, could avoid us from having to enforce the OPPs for > > genpd. In other words, doing something along the lines of $subject > > patch should work fine. > > I really don't want to design the code that way. Required OPPs don't > have anything to do with a genpd. Genpd is just one of the possible > use cases and I would like the code to reflect it, even if we don't > have any other users for this kind of stuff for now, but we surely > can. Just that those problems are solved differently for now. For > example, cache DVFS along with CPUs, etc. > > And as I said earlier, it is entirely possible that the genpd OPP > table wants to configure few more things apart from just level, and > hence a full fledged set-opp is a better design choice. I understand your point, but we don't need to call dev_pm_opp_set_opp() from _set_required_opps() to accomplish this. In fact, I have realized that calling dev_pm_opp_set_opp() from there doesn't work the way we expected. More precisely, at each recursive call to dev_pm_opp_set_opp() we are changing the OPP for a genpd's OPP table for a device that has been attached to it. The problem with this, is that we may have several devices being attached to the same genpd, thus sharing the same OPP-table that is being used for their required OPPs. So, we may have several active requests for different OPPs for a genpd's OPP table simultaneously. It seems wrong from the OPP library point of view. To me, it's would be better to leave any kind of aggregation to be managed by genpd itself. For the reason explained above, it still looks correct to call _set_opp_level() from _set_required_opps(), as $subject patch proposes (but clarifications of why is certainly needed in the commit message). Moreover, when/if we see a need for additonal resourses but the level, to be managed through a genpd's OPP table, we can extend _set_required_opps() to cover that too. > > > In fact, it looks to me that the required-opps handling for the > > *single* PM domain case, is already limited to solely > > performance-levels (opp-level), as there are no required_devs being > > assigned for it. Or did I get that wrong? > > That's why the API for setting required-opps was introduced, to make > it a central point of entry for all use cases where we want to attach > a device. The API as such isn't the problem, but rather that we are recursivly calling dev_pm_opp_set_opp() for the required-devs. In the single PM domain case, this would simply not work, as there is not a separate virtual device we can assign to the required-dev to. Kind regards Uffe
