linux-kernel - Re: [PATCH] media: stm32-dcmi: Set minimum cpufreq requirement

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <jhjpnahizkm.mognet@arm.com>
Date:   Tue, 02 Jun 2020 10:31:21 +0100
From:   Valentin Schneider <valentin.schneider@....com>
To:     Benjamin Gaignard <benjamin.gaignard@...com>
Cc:     hugues.fruchet@...com, mchehab@...nel.org,
        mcoquelin.stm32@...il.com, alexandre.torgue@...com,
        linux-media@...r.kernel.org,
        linux-stm32@...md-mailman.stormreply.com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        vincent.guittot@...aro.org, rjw@...ysocki.net
Subject: Re: [PATCH] media: stm32-dcmi: Set minimum cpufreq requirement


Hi Benjamin,

On 27/05/20 16:16, Benjamin Gaignard wrote:
> Before start streaming set cpufreq minimum frequency requirement.
> The cpufreq governor will adapt the frequencies and we will have
> no latency for handling interrupts.
>

Few comments below from someone oblivious to your platform, they may not
be all that relevant but I figured I'd pitch in anyway.

> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@...com>
> ---
>  drivers/media/platform/stm32/stm32-dcmi.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> index b8931490b83b..97c342351569 100644
> --- a/drivers/media/platform/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> @@ -13,6 +13,7 @@
>
>  #include <linux/clk.h>
>  #include <linux/completion.h>
> +#include <linux/cpufreq.h>
>  #include <linux/delay.h>
>  #include <linux/dmaengine.h>
>  #include <linux/init.h>
> @@ -99,6 +100,8 @@ enum state {
>
>  #define OVERRUN_ERROR_THRESHOLD	3
>
> +#define DCMI_MIN_FREQ	650000 /* in KHz */
> +

This assumes the handling part is guaranteed to always run on the same CPU
with the same performance profile (regardless of the platform). If that's
not guaranteed, it feels like you'd want this to be configurable in some
way.

>  struct dcmi_graph_entity {
>       struct v4l2_async_subdev asd;
>
[...]
> @@ -2020,6 +2042,8 @@ static int dcmi_probe(struct platform_device *pdev)
>               goto err_cleanup;
>       }
>
> +	dcmi->policy = cpufreq_cpu_get(0);
> +

Ideally you'd want to fetch the policy of the CPU your IRQ (and handling
thread) is affined to; The only compatible DTS I found describes a single
A7, which is somewhat limited in the affinity area...

>       dev_info(&pdev->dev, "Probe done\n");
>
>       platform_set_drvdata(pdev, dcmi);
> @@ -2049,6 +2073,9 @@ static int dcmi_remove(struct platform_device *pdev)
>
>       pm_runtime_disable(&pdev->dev);
>
> +	if (dcmi->policy)
> +		cpufreq_cpu_put(dcmi->policy);
> +
>       v4l2_async_notifier_unregister(&dcmi->notifier);
>       v4l2_async_notifier_cleanup(&dcmi->notifier);
>       media_entity_cleanup(&dcmi->vdev->entity);