Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 12 Jun 2020 10:24:54 +0100
From:   Quentin Perret <qperret@google.com>
To:     Doug Anderson <dianders@chromium.org>
Cc:     Benson Leung <bleung@chromium.org>,
        Enric Balletbo i Serra <enric.balletbo@collabora.com>,
        hsinyi@chromium.org, Joel Fernandes <joelaf@google.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Nicolas Boichat <drinkcat@chromium.org>,
        Gwendal Grignou <gwendal@chromium.org>,
        ctheegal@codeaurora.org, Guenter Roeck <groeck@chromium.org>,
        LKML <linux-kernel@vger.kernel.org>, qais.yousef@arm.com
Subject: Re: [PATCH] cros_ec_spi: Even though we're RT priority, don't bump
 cpu freq
Message-ID: <20200612092454.GA94228@google.com>
References: <20200610151818.1.I666ecd9c6f3c6405bd75831a21001b8109b6438c@changeid>
 <20200611110301.GA132747@google.com>
 <CAD=FV=V2FvFcYrghxUSdHNxmcS3DKpnBbk2oL64w7hh=tV-DfQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAD=FV=V2FvFcYrghxUSdHNxmcS3DKpnBbk2oL64w7hh=tV-DfQ@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

+CC Qais [FYI]

On Thursday 11 Jun 2020 at 10:48:40 (-0700), Doug Anderson wrote:
> Hrm.  I guess my first instinct is to say that we still want this
> patch even if we have something that is applied more globally.

Fair enough.

> Specifically it sounds as if the patch you point at is suggesting that
> we'd tweak the boost value to something other than max but we'd still
> have a boost value.  In the case of cros_ec_spi I don't believe we
> need any boost value at all, so my patch would still be useful.  The
> computational needs of cros_ec_spi are very modest and it can do its
> work at lower CPU frequencies just fine.  It just can't be interrupted
> for large swaths of time.
> 
> 
> > IOW, how do you feel about 20200511154053.7822-1-qais.yousef@arm.com ?
> 
> I'm not totally a fan, but I'm definitely not an expert in this area
> (I've also only read the patch description and not the patch or the
> whole thread).  I really don't want yet another value that I need to
> tune from board to board.  Even worse, this tuning value isn't
> board-specific but a combination of board and software specific.  By
> this, I'd imagine a scenario where you're using a real-time task to
> get audio decoding done within a certain latency.  I guess you'd tune
> this value to make sure that you can get all your audio decoding done
> in time but also not burn extra power.  Now, imagine that the OS
> upgrades and the audio task suddenly has to decode more complex
> streams.  You've got to go across all of your boards and re-tune every
> one?  ...or, nobody thinks about it and older boards start getting
> stuttery audio?  Perhaps the opposite happens and someone comes up
> with a newer lower-cpu-intensive codec and you could save power.
> Sounds like a bit of a nightmare.
> 
> I'd rather have a boolean value: boost all RT threads to max vs. don't
> boost all RT threads to max.  Someone that just wanted RT stuff to run
> as fast as possible without any hassle on their system and didn't care
> about power efficiency could turn this on.  Anyone who really cared
> about power could turn this off and then could find a more targeted
> way to boost things, hopefully in a way that doesn't require tuning.
> One option would be to still boost the CPU to max but only for certain
> tasks known to be really latency sensitive.  Another might be to
> somehow measure whether or not the task is making its deadlines and
> boost the CPU frequency up if deadlines are not being met.  I'm sure
> there are fancier ways.
> 
> ...of course, I believe your patch allows me to do what I want: I can
> just set the default boost to 0.  It just leaves in the temptation for
> others to require a default boost of something else and then I'm stuck
> in my tuning nightmare.

Right, so I am not disagreeing at all with the above. FWIW, I expect
Android to set this default value to 0 as you mentioned, so that uclamp
basically becomes 'opt-in' for _all_ tasks, including RT.

Now, given that Qais' patch is commiting something to userspace, I think
it makes sense to expose the full range rather than a boolean value, as
it's probably more future-proof. That is, if we expose a boolean knob
now, and somebody wants to be able to set a default value in the middle
in a few years, we'll have to add another knob, and maintain both (which
sucks). But it's just my personal opinion. Feel free to jump in the
other thread if you feel differently :)

Cheers,
Quentin