Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Hugues FRUCHET <hugues.fruchet@st.com>
To: Benjamin GAIGNARD <benjamin.gaignard@st.com>,
	"mchehab@kernel.org" <mchehab@kernel.org>,
	"mcoquelin.stm32@gmail.com" <mcoquelin.stm32@gmail.com>,
	Alexandre TORGUE <alexandre.torgue@st.com>
Cc: "linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
	"linux-stm32@st-md-mailman.stormreply.com" 
	<linux-stm32@st-md-mailman.stormreply.com>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"vincent.guittot@linaro.org" <vincent.guittot@linaro.org>,
	"valentin.schneider@arm.com" <valentin.schneider@arm.com>,
	"rjw@rjwysocki.net" <rjw@rjwysocki.net>
Subject: Re: [PATCH v6 2/3] media: stm32-dcmi: Set minimum cpufreq requirement
Date: Wed, 24 Jun 2020 09:24:17 +0000
Message-ID: <b40a7312-9ea8-098f-8ba8-f080b3f57b5f@st.com> (raw)
In-Reply-To: <20200610122500.4304-3-benjamin.gaignard@st.com>


Hi Benjamin,

test condition in set_min_frequency() to fix, appart from that:
Acked-by: Hugues Fruchet <hugues.fruchet@st.com>

BR,
Hugues.

On 6/10/20 2:24 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.
> The frequency requirement is retrieved from the device-tree node.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> ---
> version 6:
> - come back to version 4 and follow Valentin's suggestions about notifier
> - add Valentin's comment about notifier set/unset
> 
> version 5:
> - add a mutex to protect dcmi_irq_notifier_notify()
> - register notifier a probe time
> 
> version 4:
> - simplify irq affinity handling by using only dcmi_irq_notifier_notify()
> 
> version 3:
> - add a cpumask field to track boosted CPUs
> - add irq_affinity_notify callback
> - protect cpumask field with a mutex
> 
>   drivers/media/platform/stm32/stm32-dcmi.c | 138 ++++++++++++++++++++++++++++--
>   1 file changed, 130 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> index b8931490b83b..382df6e7c864 100644
> --- a/drivers/media/platform/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> @@ -13,10 +13,13 @@
>   
>   #include <linux/clk.h>
>   #include <linux/completion.h>
> +#include <linux/cpufreq.h>
> +#include <linux/cpumask.h>
>   #include <linux/delay.h>
>   #include <linux/dmaengine.h>
>   #include <linux/init.h>
>   #include <linux/interrupt.h>
> +#include <linux/irq.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/of.h>
> @@ -99,6 +102,8 @@ enum state {
>   
>   #define OVERRUN_ERROR_THRESHOLD	3
>   
> +static DEFINE_PER_CPU(struct freq_qos_request, qos_req);
> +
>   struct dcmi_graph_entity {
>   	struct v4l2_async_subdev asd;
>   
> @@ -133,6 +138,7 @@ struct stm32_dcmi {
>   	struct resource			*res;
>   	struct reset_control		*rstc;
>   	int				sequence;
> +	int				irq;
>   	struct list_head		buffers;
>   	struct dcmi_buf			*active;
>   
> @@ -173,6 +179,11 @@ struct stm32_dcmi {
>   	struct media_device		mdev;
>   	struct media_pad		vid_cap_pad;
>   	struct media_pipeline		pipeline;
> +
> +	struct mutex			freq_lock;
> +	u32				min_frequency;
> +	cpumask_var_t			boosted;
> +	struct irq_affinity_notify	notify;
>   };
>   
>   static inline struct stm32_dcmi *notifier_to_dcmi(struct v4l2_async_notifier *n)
> @@ -722,6 +733,99 @@ static void dcmi_pipeline_stop(struct stm32_dcmi *dcmi)
>   	dcmi_pipeline_s_stream(dcmi, 0);
>   }
>   
> +static void dcmi_get_min_frequency(struct stm32_dcmi *dcmi)
> +{
> +	struct device_node *np = dcmi->mdev.dev->of_node;
> +
> +	dcmi->min_frequency = FREQ_QOS_MIN_DEFAULT_VALUE;
> +
> +	of_property_read_u32(np, "st,stm32-dcmi-min-frequency",
> +			     &dcmi->min_frequency);
> +}
> +
> +static void dcmi_irq_notifier_notify(struct irq_affinity_notify *notify,
> +				     const cpumask_t *mask)
> +{
> +	struct stm32_dcmi *dcmi = container_of(notify,
> +					       struct stm32_dcmi,
> +					       notify);
> +	struct cpufreq_policy *p;
> +	int cpu;
> +
> +	mutex_lock(&dcmi->freq_lock);
> +	/*
> +	 * For all boosted CPUs check if it is still the case
> +	 * if not remove the request
> +	 */
> +	for_each_cpu(cpu, dcmi->boosted) {
> +		if (cpumask_test_cpu(cpu, mask))
> +			continue;
> +
> +		p = cpufreq_cpu_get(cpu);
> +		if (!p)
> +			continue;
> +
> +		freq_qos_remove_request(&per_cpu(qos_req, cpu));
> +		cpumask_andnot(dcmi->boosted, dcmi->boosted, p->cpus);
> +
> +		cpufreq_cpu_put(p);
> +	}
> +
> +	/*
> +	 * For CPUs in the mask check if they are boosted if not add
> +	 * a request
> +	 */
> +	for_each_cpu(cpu, mask) {
> +		if (cpumask_test_cpu(cpu, dcmi->boosted))
> +			continue;
> +
> +		p = cpufreq_cpu_get(cpu);
> +		if (!p)
> +			continue;
> +
> +		freq_qos_add_request(&p->constraints, &per_cpu(qos_req, cpu),
> +				     FREQ_QOS_MIN, dcmi->min_frequency);
> +		cpumask_or(dcmi->boosted, dcmi->boosted, p->cpus);
> +		cpufreq_cpu_put(p);
> +	}
> +
> +	mutex_unlock(&dcmi->freq_lock);
> +}
> +
> +static void dcmi_irq_notifier_release(struct kref *ref)
> +{
> +	/*
> +	 * This is required by affinity notifier. We don't have anything to
> +	 * free here.
> +	 */
> +}
> +
> +static void dcmi_set_min_frequency(struct stm32_dcmi *dcmi, s32 freq)
> +{
> +	struct irq_affinity_notify *notify = &dcmi->notify;
> +	struct cpumask clear;
> +
> +	if (freq) {

if (freq != FREQ_QOS_MIN_DEFAULT_VALUE)

> +		/*
> +		 * Register the notifier before doing any change, so the
> +		 * callback can be queued if an affinity change happens *while*
> +		 * we are requesting the boosts.
> +		 */
> +		irq_set_affinity_notifier(dcmi->irq, notify);
> +		dcmi_irq_notifier_notify(notify,
> +					 irq_get_affinity_mask(dcmi->irq));
> +	} else {
> +		/*
> +		 * Unregister the notifier before clearing the boost requests,
> +		 * as we don't want to boost again if an affinity change happens
> +		 * *while* we are clearing the requests
> +		 */
> +		irq_set_affinity_notifier(dcmi->irq, NULL);
> +		cpumask_clear(&clear);
> +		dcmi_irq_notifier_notify(notify, &clear);
> +	}
> +}
> +
>   static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
>   {
>   	struct stm32_dcmi *dcmi = vb2_get_drv_priv(vq);
> @@ -736,11 +840,13 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
>   		goto err_release_buffers;
>   	}
>   
> +	dcmi_set_min_frequency(dcmi, dcmi->min_frequency);
> +
>   	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,7 +941,8 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
>   err_media_pipeline_stop:
>   	media_pipeline_stop(&dcmi->vdev->entity);
>   
> -err_pm_put:
> +err_drop_qos:
> +	dcmi_set_min_frequency(dcmi, FREQ_QOS_MIN_DEFAULT_VALUE) >   	pm_runtime_put(dcmi->dev);
>   
>   err_release_buffers:
> @@ -863,6 +970,8 @@ static void dcmi_stop_streaming(struct vb2_queue *vq)
>   
>   	media_pipeline_stop(&dcmi->vdev->entity);
>   
> +	dcmi_set_min_frequency(dcmi, FREQ_QOS_MIN_DEFAULT_VALUE);
> +
>   	spin_lock_irq(&dcmi->irqlock);
>   
>   	/* Disable interruptions */
> @@ -1834,11 +1943,11 @@ static int dcmi_probe(struct platform_device *pdev)
>   	struct device_node *np = pdev->dev.of_node;
>   	const struct of_device_id *match = NULL;
>   	struct v4l2_fwnode_endpoint ep = { .bus_type = 0 };
> +	struct irq_affinity_notify *notify;
>   	struct stm32_dcmi *dcmi;
>   	struct vb2_queue *q;
>   	struct dma_chan *chan;
>   	struct clk *mclk;
> -	int irq;
>   	int ret = 0;
>   
>   	match = of_match_device(of_match_ptr(stm32_dcmi_of_match), &pdev->dev);
> @@ -1879,9 +1988,9 @@ static int dcmi_probe(struct platform_device *pdev)
>   	dcmi->bus.bus_width = ep.bus.parallel.bus_width;
>   	dcmi->bus.data_shift = ep.bus.parallel.data_shift;
>   
> -	irq = platform_get_irq(pdev, 0);
> -	if (irq <= 0)
> -		return irq ? irq : -ENXIO;
> +	dcmi->irq = platform_get_irq(pdev, 0);
> +	if (dcmi->irq <= 0)
> +		return dcmi->irq ? dcmi->irq : -ENXIO;
>   
>   	dcmi->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   	if (!dcmi->res) {
> @@ -1895,11 +2004,12 @@ static int dcmi_probe(struct platform_device *pdev)
>   		return PTR_ERR(dcmi->regs);
>   	}
>   
> -	ret = devm_request_threaded_irq(&pdev->dev, irq, dcmi_irq_callback,
> +	ret = devm_request_threaded_irq(&pdev->dev, dcmi->irq,
> +					dcmi_irq_callback,
>   					dcmi_irq_thread, IRQF_ONESHOT,
>   					dev_name(&pdev->dev), dcmi);
>   	if (ret) {
> -		dev_err(&pdev->dev, "Unable to request irq %d\n", irq);
> +		dev_err(&pdev->dev, "Unable to request irq %d\n", dcmi->irq);
>   		return ret;
>   	}
>   
> @@ -1922,6 +2032,7 @@ static int dcmi_probe(struct platform_device *pdev)
>   	spin_lock_init(&dcmi->irqlock);
>   	mutex_init(&dcmi->lock);
>   	mutex_init(&dcmi->dma_lock);
> +	mutex_init(&dcmi->freq_lock);
>   	init_completion(&dcmi->complete);
>   	INIT_LIST_HEAD(&dcmi->buffers);
>   
> @@ -1930,6 +2041,13 @@ static int dcmi_probe(struct platform_device *pdev)
>   	dcmi->state = STOPPED;
>   	dcmi->dma_chan = chan;
>   
> +	if (!alloc_cpumask_var(&dcmi->boosted, GFP_KERNEL))
> +		return -ENODEV;
> +
> +	notify = &dcmi->notify;
> +	notify->notify = dcmi_irq_notifier_notify;
> +	notify->release = dcmi_irq_notifier_release;
> +
>   	q = &dcmi->queue;
>   
>   	dcmi->v4l2_dev.mdev = &dcmi->mdev;
> @@ -2022,6 +2140,8 @@ static int dcmi_probe(struct platform_device *pdev)
>   
>   	dev_info(&pdev->dev, "Probe done\n");
>   
> +	dcmi_get_min_frequency(dcmi);
> +
>   	platform_set_drvdata(pdev, dcmi);
>   
>   	pm_runtime_enable(&pdev->dev);
> @@ -2049,6 +2169,8 @@ static int dcmi_remove(struct platform_device *pdev)
>   
>   	pm_runtime_disable(&pdev->dev);
>   
> +	free_cpumask_var(dcmi->boosted);
> +
>   	v4l2_async_notifier_unregister(&dcmi->notifier);
>   	v4l2_async_notifier_cleanup(&dcmi->notifier);
>   	media_entity_cleanup(&dcmi->vdev->entity);
> 

  parent reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-10 12:24 [PATCH v6 0/3] DCMI set " Benjamin Gaignard
2020-06-10 12:24 ` [PATCH v6 1/3] dt-bindings: media: stm32-dcmi: Add DCMI min frequency property Benjamin Gaignard
2020-06-23 11:48   ` Benjamin GAIGNARD
2020-06-10 12:24 ` [PATCH v6 2/3] media: stm32-dcmi: Set minimum cpufreq requirement Benjamin Gaignard
2020-06-10 17:16   ` Valentin Schneider
2020-06-24  9:24   ` Hugues FRUCHET [this message]
2020-06-24 11:27     ` Benjamin GAIGNARD
2020-06-10 12:25 ` [PATCH v6 3/3] ARM: dts: stm32: Set DCMI frequency requirement for stm32mp15x Benjamin Gaignard
2020-07-01 12:59 [PATCH v6 0/3] DCMI set minimum cpufreq requirement Benjamin Gaignard
2020-07-01 12:59 ` [PATCH v6 2/3] media: stm32-dcmi: Set " Benjamin Gaignard

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b40a7312-9ea8-098f-8ba8-f080b3f57b5f@st.com \
    --to=hugues.fruchet@st.com \
    --cc=alexandre.torgue@st.com \
    --cc=benjamin.gaignard@st.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-stm32@st-md-mailman.stormreply.com \
    --cc=mchehab@kernel.org \
    --cc=mcoquelin.stm32@gmail.com \
    --cc=rjw@rjwysocki.net \
    --cc=valentin.schneider@arm.com \
    --cc=vincent.guittot@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git