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

[Benjamin GAIGNARD <benjamin.gaignard@st.com>]

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