From mboxrd@z Thu Jan 1 00:00:00 1970 From: Doug Anderson Subject: Re: [PATCH v8 09/10] drivers: qcom: rpmh: add support for batch RPMH request Date: Tue, 15 May 2018 09:50:22 -0700 Message-ID: References: <20180509170159.29682-1-ilina@codeaurora.org> <20180509170159.29682-10-ilina@codeaurora.org> <20180514195929.GA22950@codeaurora.org> <20180515162326.GA28489@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: <20180515162326.GA28489@codeaurora.org> Sender: linux-kernel-owner@vger.kernel.org To: Lina Iyer Cc: Andy Gross , David Brown , linux-arm-msm@vger.kernel.org, "open list:ARM/QUALCOMM SUPPORT" , Rajendra Nayak , Bjorn Andersson , LKML , Stephen Boyd , Evan Green , Matthias Kaehlcke , rplsssn@codeaurora.org List-Id: linux-arm-msm@vger.kernel.org Hi, On Tue, May 15, 2018 at 9:23 AM, Lina Iyer wrote: > On Tue, May 15 2018 at 09:52 -0600, Doug Anderson wrote: >> >> Hi, >> >> On Mon, May 14, 2018 at 12:59 PM, Lina Iyer wrote: >> >>>>> /** >>>>> @@ -77,12 +82,14 @@ struct rpmh_request { >>>>> * @cache: the list of cached requests >>>>> * @lock: synchronize access to the controller data >>>>> * @dirty: was the cache updated since flush >>>>> + * @batch_cache: Cache sleep and wake requests sent as batch >>>>> */ >>>>> struct rpmh_ctrlr { >>>>> struct rsc_drv *drv; >>>>> struct list_head cache; >>>>> spinlock_t lock; >>>>> bool dirty; >>>>> + const struct rpmh_request *batch_cache[RPMH_MAX_BATCH_CACHE]; >>>> >>>> >>>> >>>> I'm pretty confused about why the "batch_cache" is separate from the >>>> normal cache. As far as I can tell the purpose of the two is the same >>>> but you have two totally separate code paths and data structures. >>>> >>> Due to a hardware limitation, requests made by bus drivers must be set >>> up in the sleep and wake TCS first before setting up the requests from >>> other drivers. Bus drivers use batch mode for any and all RPMH >>> communication. Hence their request are the only ones in the batch_cache. >> >> >> This is totally not obvious and not commented. Why not rename to >> "priority" instead of "batch"? >> >> If the only requirement is that these come first, that's still no >> reason to use totally separate code paths and mechanisms. These >> requests can still use the same data structures / functions and just >> be ordered to come first, can't they? ...or given a boolean >> "priority" and you can do two passes through your queue: one to do the >> priority ones and one to do the secondary ones. >> >> > The bus requests have a certain order and cannot be mutated by the > RPMH driver. It has to be maintained in the TCS in the same order. Please document this requirement in the code. > Also, the bus requests have quite a churn during the course of an > usecase. They are setup and invalidated often. > It is faster to have them separate and invalidate the whole lot of the > batch_cache instead of intertwining them with requests from other > drivers and then figuring out which all must be invalidated and rebuilt > when the next batch requests comes in. Remember, that requests may come > from any driver any time and therefore will be mangled if we use the > same data structure. So to be faster and to avoid having mangled requests > in the TCS, I have kept the data structures separate. If you need to find a certain group of requests then can't you just tag them and it's easy to find them? I'm still not seeing the need for a whole separate code path and data structure. >>>>> + spin_unlock_irqrestore(&ctrlr->lock, flags); >>>>> + >>>>> + return ret; >>>>> +} >>>> >>>> >>>> >>>> As part of my overall confusion about why the batch cache is different >>>> than the normal one: for the normal use case you still call >>>> rpmh_rsc_write_ctrl_data() for things you put in your cache, but you >>>> don't for the batch cache. I still haven't totally figured out what >>>> rpmh_rsc_write_ctrl_data() does, but it seems strange that you don't >>>> do it for the batch cache but you do for the other one. >>>> >>>> >>> flush_batch does write to the controller using >>> rpmh_rsc_write_ctrl_data() >> >> >> My confusion is that they happen at different times. As I understand it: >> >> * For the normal case, you immediately calls >> rpmh_rsc_write_ctrl_data() and then later do the rest of the work. >> >> * For the batch case, you call both later. >> >> Is there a good reason for this, or is it just an arbitrary >> difference? If there's a good reason, it should be documented. >> >> > In both the cases, the requests are cached in the rpmh library and are > only sent to the controller only when the flushed. I am not sure the > work is any different. The rpmh_flush() flushes out batch requests and > then the requests from other drivers. OK then, here are the code paths I see: rpmh_write => __rpmh_write => cache_rpm_request() => (state != RPMH_ACTIVE_ONLY_STATE): rpmh_rsc_send_data() rpmh_write_batch => (state != RPMH_ACTIVE_ONLY_STATE): cache_batch() => No call to rpmh_rsc_send_data() Said another way: * if you call rpmh_write() for something you're going to defer you will still call cache_rpm_request() _before_ rpmh_write() returns. * if you call rpmh_write_batch() for something you're going to defer then you _don't_ call cache_rpm_request() before rpmh_write_batch() returns. Do you find a fault in my analysis of the current code? If you see a fault then please point it out. If you don't see a fault, please say why the behaviors are different. I certainly understand that eventually you will call cache_rpm_request() for both cases. It's just that in one case the call happens right away and the other case it is deferred.