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

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

[Benjamin Gaignard]

Before start streaming set cpufreq minimum frequency requirement.
The cpufreq governor will adapt the frequencies and we will have
no latency for handling interrupts.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.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 */
+
 struct dcmi_graph_entity {
 	struct v4l2_async_subdev asd;
 
@@ -173,6 +176,10 @@ struct stm32_dcmi {
 	struct media_device		mdev;
 	struct media_pad		vid_cap_pad;
 	struct media_pipeline		pipeline;
+
+	/* CPU freq contraint */
+	struct cpufreq_policy		*policy;
+	struct freq_qos_request		qos_req;
 };
 
 static inline struct stm32_dcmi *notifier_to_dcmi(struct v4l2_async_notifier *n)
@@ -736,11 +743,20 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
 		goto err_release_buffers;
 	}
 
+	if (dcmi->policy) {
+		ret = freq_qos_add_request(&dcmi->policy->constraints,
+					   &dcmi->qos_req, FREQ_QOS_MIN,
+					   DCMI_MIN_FREQ);
+
+		if (ret < 0)
+			goto err_pm_put;
+	}
+
 	ret = media_pipeline_start(&dcmi->vdev->entity, &dcmi->pipeline);
 	if (ret < 0) {
 		dev_err(dcmi->dev, "%s: Failed to start streaming, media pipeline start error (%d)\n",
 			__func__, ret);
-		goto err_pm_put;
+		goto err_drop_qos;
 	}
 
 	ret = dcmi_pipeline_start(dcmi);
@@ -835,6 +851,9 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
 err_media_pipeline_stop:
 	media_pipeline_stop(&dcmi->vdev->entity);
 
+err_drop_qos:
+	if (dcmi->policy)
+		freq_qos_remove_request(&dcmi->qos_req);
 err_pm_put:
 	pm_runtime_put(dcmi->dev);
 
@@ -863,6 +882,9 @@ static void dcmi_stop_streaming(struct vb2_queue *vq)
 
 	media_pipeline_stop(&dcmi->vdev->entity);
 
+	if (dcmi->policy)
+		freq_qos_remove_request(&dcmi->qos_req);
+
 	spin_lock_irq(&dcmi->irqlock);
 
 	/* Disable interruptions */
@@ -2020,6 +2042,8 @@ static int dcmi_probe(struct platform_device *pdev)
 		goto err_cleanup;
 	}
 
+	dcmi->policy = cpufreq_cpu_get(0);
+
 	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);
-- 
2.15.0

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

[Hugues FRUCHET]

Acked-by: Hugues Fruchet <hugues.fruchet@st.com>

On 5/27/20 5:16 PM, 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.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.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 */
> +
>   struct dcmi_graph_entity {
>   	struct v4l2_async_subdev asd;
>   
> @@ -173,6 +176,10 @@ struct stm32_dcmi {
>   	struct media_device		mdev;
>   	struct media_pad		vid_cap_pad;
>   	struct media_pipeline		pipeline;
> +
> +	/* CPU freq contraint */
> +	struct cpufreq_policy		*policy;
> +	struct freq_qos_request		qos_req;
>   };
>   
>   static inline struct stm32_dcmi *notifier_to_dcmi(struct v4l2_async_notifier *n)
> @@ -736,11 +743,20 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
>   		goto err_release_buffers;
>   	}
>   
> +	if (dcmi->policy) {
> +		ret = freq_qos_add_request(&dcmi->policy->constraints,
> +					   &dcmi->qos_req, FREQ_QOS_MIN,
> +					   DCMI_MIN_FREQ);
> +
> +		if (ret < 0)
> +			goto err_pm_put;
> +	}
> +
>   	ret = media_pipeline_start(&dcmi->vdev->entity, &dcmi->pipeline);
>   	if (ret < 0) {
>   		dev_err(dcmi->dev, "%s: Failed to start streaming, media pipeline start error (%d)\n",
>   			__func__, ret);
> -		goto err_pm_put;
> +		goto err_drop_qos;
>   	}
>   
>   	ret = dcmi_pipeline_start(dcmi);
> @@ -835,6 +851,9 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
>   err_media_pipeline_stop:
>   	media_pipeline_stop(&dcmi->vdev->entity);
>   
> +err_drop_qos:
> +	if (dcmi->policy)
> +		freq_qos_remove_request(&dcmi->qos_req);
>   err_pm_put:
>   	pm_runtime_put(dcmi->dev);
>   
> @@ -863,6 +882,9 @@ static void dcmi_stop_streaming(struct vb2_queue *vq)
>   
>   	media_pipeline_stop(&dcmi->vdev->entity);
>   
> +	if (dcmi->policy)
> +		freq_qos_remove_request(&dcmi->qos_req);
> +
>   	spin_lock_irq(&dcmi->irqlock);
>   
>   	/* Disable interruptions */
> @@ -2020,6 +2042,8 @@ static int dcmi_probe(struct platform_device *pdev)
>   		goto err_cleanup;
>   	}
>   
> +	dcmi->policy = cpufreq_cpu_get(0);
> +
>   	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);
> 

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

[Valentin Schneider]

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@st.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);

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

[Benjamin GAIGNARD]

On 6/2/20 11:31 AM, Valentin Schneider wrote:
> 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@st.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.
Yes I could add a st,stm32-dcmi-min-frequency (in KHz) parameter the 
device tree node.

>
>>   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...
If I move this code just before start streaming and use get_cpu(), would 
it works ?

Benjamin
>
>>        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);

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

[Valentin Schneider]

On 02/06/20 12:37, Benjamin GAIGNARD wrote:
> On 6/2/20 11:31 AM, Valentin Schneider wrote:
>>> @@ -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.
> Yes I could add a st,stm32-dcmi-min-frequency (in KHz) parameter the
> device tree node.
>

Something like that - I'm not sure how well this fits with the DT
landscape, as you could argue it isn't really a description of the
hardware, more of a description of the performance expectations of the
software. I won't really argue here.

>>
>>>   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...
> If I move this code just before start streaming and use get_cpu(), would
> it works ?
>

AFAIA streaming_start() is not necessarily executing on the same CPU as the
one that will handle the interrupt. I was thinking you could use the IRQ's
effective affinity as a hint of which CPU(s) to boost, i.e. something like:

---
    struct cpumask_var_t visited;
    struct irq_data *d = irq_get_irq_data(irq);

    err = alloc_cpumask_var(visited, GFP_KERNEL);
    /* ... */
    for_each_cpu(cpu, irq_data_get_effective_affinity_mask(d)) {
            /* check if not already spanned */
            if (cpumask_test_cpu(cpu, visited))
                    continue;

            policy = cpufreq_cpu_get(cpu);
            cpumask_or(visited, visited, policy->cpus);
            /* do the boost for that policy here */
            /* ... */
            cpufreq_cpu_put(policy);
    }
---

That of course falls apart when hotplug gets involved, and the effective
affinity changes... There's irq_set_affinity_notifier() out there, but it
seems it's only about the affinity, not the effective_affinity, I'm not
sure how valid it would be to query the effective_affinity in that
notifier.

> Benjamin
>>
>>>        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);

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

[Benjamin GAIGNARD]

On 6/2/20 3:35 PM, Valentin Schneider wrote:
> On 02/06/20 12:37, Benjamin GAIGNARD wrote:
>> On 6/2/20 11:31 AM, Valentin Schneider wrote:
>>>> @@ -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.
>> Yes I could add a st,stm32-dcmi-min-frequency (in KHz) parameter the
>> device tree node.
>>
> Something like that - I'm not sure how well this fits with the DT
> landscape, as you could argue it isn't really a description of the
> hardware, more of a description of the performance expectations of the
> software. I won't really argue here.
>
>>>>    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...
>> If I move this code just before start streaming and use get_cpu(), would
>> it works ?
>>
> AFAIA streaming_start() is not necessarily executing on the same CPU as the
> one that will handle the interrupt. I was thinking you could use the IRQ's
> effective affinity as a hint of which CPU(s) to boost, i.e. something like:
>
> ---
>      struct cpumask_var_t visited;
>      struct irq_data *d = irq_get_irq_data(irq);
>
>      err = alloc_cpumask_var(visited, GFP_KERNEL);
>      /* ... */
>      for_each_cpu(cpu, irq_data_get_effective_affinity_mask(d)) {
>              /* check if not already spanned */
>              if (cpumask_test_cpu(cpu, visited))
>                      continue;
>
>              policy = cpufreq_cpu_get(cpu);
>              cpumask_or(visited, visited, policy->cpus);
>              /* do the boost for that policy here */
>              /* ... */
>              cpufreq_cpu_put(policy);
>      }
> ---
>
> That of course falls apart when hotplug gets involved, and the effective
> affinity changes... There's irq_set_affinity_notifier() out there, but it
> seems it's only about the affinity, not the effective_affinity, I'm not
> sure how valid it would be to query the effective_affinity in that
> notifier.
If I wait to be in the irq it will be too late so I think I will do a 
loop over all possible CPUs
before start the streaming to change the policies.

>
>> Benjamin
>>>>         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);

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

[Vincent Guittot]

On Wed, 3 Jun 2020 at 09:34, Benjamin GAIGNARD <benjamin.gaignard@st.com> wrote:
>
>
>
> On 6/2/20 3:35 PM, Valentin Schneider wrote:
> > On 02/06/20 12:37, Benjamin GAIGNARD wrote:
> >> On 6/2/20 11:31 AM, Valentin Schneider wrote:
> >>>> @@ -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.
> >> Yes I could add a st,stm32-dcmi-min-frequency (in KHz) parameter the
> >> device tree node.
> >>
> > Something like that - I'm not sure how well this fits with the DT
> > landscape, as you could argue it isn't really a description of the
> > hardware, more of a description of the performance expectations of the
> > software. I won't really argue here.
> >
> >>>>    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...
> >> If I move this code just before start streaming and use get_cpu(), would
> >> it works ?
> >>
> > AFAIA streaming_start() is not necessarily executing on the same CPU as the
> > one that will handle the interrupt. I was thinking you could use the IRQ's
> > effective affinity as a hint of which CPU(s) to boost, i.e. something like:
> >
> > ---
> >      struct cpumask_var_t visited;
> >      struct irq_data *d = irq_get_irq_data(irq);
> >
> >      err = alloc_cpumask_var(visited, GFP_KERNEL);
> >      /* ... */
> >      for_each_cpu(cpu, irq_data_get_effective_affinity_mask(d)) {
> >              /* check if not already spanned */
> >              if (cpumask_test_cpu(cpu, visited))
> >                      continue;
> >
> >              policy = cpufreq_cpu_get(cpu);
> >              cpumask_or(visited, visited, policy->cpus);
> >              /* do the boost for that policy here */
> >              /* ... */
> >              cpufreq_cpu_put(policy);
> >      }
> > ---
> >
> > That of course falls apart when hotplug gets involved, and the effective
> > affinity changes... There's irq_set_affinity_notifier() out there, but it
> > seems it's only about the affinity, not the effective_affinity, I'm not
> > sure how valid it would be to query the effective_affinity in that
> > notifier.
> If I wait to be in the irq it will be too late so I think I will do a
> loop over all possible CPUs
> before start the streaming to change the policies.

Can't you use irq_get_affinity_mask  and loop over it ?

Also You should better use freq_qos_add/remove_request during probe
and remove of the driver and use freq_qos_update_request in
dcmi_start/stop_streaming to set/unset your constraint.

>
> >
> >> Benjamin
> >>>>         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);

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

[Valentin Schneider]

On 03/06/20 08:50, Vincent Guittot wrote:
> On Wed, 3 Jun 2020 at 09:34, Benjamin GAIGNARD <benjamin.gaignard@st.com> wrote:
>> On 6/2/20 3:35 PM, Valentin Schneider wrote:
>> > AFAIA streaming_start() is not necessarily executing on the same CPU as the
>> > one that will handle the interrupt. I was thinking you could use the IRQ's
>> > effective affinity as a hint of which CPU(s) to boost, i.e. something like:
>> >
>> > ---
>> >      struct cpumask_var_t visited;
>> >      struct irq_data *d = irq_get_irq_data(irq);
>> >
>> >      err = alloc_cpumask_var(visited, GFP_KERNEL);
>> >      /* ... */
>> >      for_each_cpu(cpu, irq_data_get_effective_affinity_mask(d)) {
>> >              /* check if not already spanned */
>> >              if (cpumask_test_cpu(cpu, visited))
>> >                      continue;
>> >
>> >              policy = cpufreq_cpu_get(cpu);
>> >              cpumask_or(visited, visited, policy->cpus);
>> >              /* do the boost for that policy here */
>> >              /* ... */
>> >              cpufreq_cpu_put(policy);
>> >      }
>> > ---
>> >
>> > That of course falls apart when hotplug gets involved, and the effective
>> > affinity changes... There's irq_set_affinity_notifier() out there, but it
>> > seems it's only about the affinity, not the effective_affinity, I'm not
>> > sure how valid it would be to query the effective_affinity in that
>> > notifier.

>> If I wait to be in the irq it will be too late so I think I will do a
>> loop over all possible CPUs
>> before start the streaming to change the policies.
>

Yes, that's what I was thinking as well.

> Can't you use irq_get_affinity_mask  and loop over it ?
>

In the end that's the only usable option, I think.

I was looking at alternatives because on arm64 (and AFAICT that applies
to arm too; see irq-gic.c::gic_set_affinity()) the affinity mask spans
all CPUs by default, while the effective affinity mask spans only the
CPU that will actually handle the IRQ (+ where its thread should run).

That said, using the effective mask that way does feel like an
implementation leak. Sadly I couldn't find any better way to minimize
the number of boosted frequency domains.

> Also You should better use freq_qos_add/remove_request during probe
> and remove of the driver and use freq_qos_update_request in
> dcmi_start/stop_streaming to set/unset your constraint.
>