Hi Bingbu,
On Fri, Aug 16, 2019 at 12:25 PM <bingbu.cao@xxxxxxxxx> wrote:
>
> From: Bingbu Cao <bingbu.cao@xxxxxxxxx>
>
> Currently, imgu is working on 450MHz for all cases, however
> in some cases (input frame less than 2.3MP), the imgu
> did not need work in high frequency.
> This patch make imgu work on 200MHz if the imgu input
> frame is less than 2.3MP to save power.
>
Thanks for the patch! Please see my comments inline.
> Signed-off-by: Bingbu Cao <bingbu.cao@xxxxxxxxx>
> ---
> drivers/staging/media/ipu3/ipu3-css.c | 7 ++++---
> drivers/staging/media/ipu3/ipu3-css.h | 3 ++-
> drivers/staging/media/ipu3/ipu3-v4l2.c | 6 ++++++
> drivers/staging/media/ipu3/ipu3.c | 6 ++++--
> drivers/staging/media/ipu3/ipu3.h | 1 +
> 5 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/media/ipu3/ipu3-css.c b/drivers/staging/media/ipu3/ipu3-css.c
> index fd1ed84c400c..590ed7e182a6 100644
> --- a/drivers/staging/media/ipu3/ipu3-css.c
> +++ b/drivers/staging/media/ipu3/ipu3-css.c
> @@ -210,12 +210,13 @@ static int imgu_hw_wait(void __iomem *base, int reg, u32 mask, u32 cmp)
>
> /* Initialize the IPU3 CSS hardware and associated h/w blocks */
>
> -int imgu_css_set_powerup(struct device *dev, void __iomem *base)
> +int imgu_css_set_powerup(struct device *dev, void __iomem *base, bool low_power)
> {
> - static const unsigned int freq = 450;
> + unsigned int freq;
How about making freq the argument to this function rather than
introducing some artificial boolean?
> u32 pm_ctrl, state, val;
>
> - dev_dbg(dev, "%s\n", __func__);
> + freq = low_power ? 200 : 450;
> + dev_dbg(dev, "%s with freq %u\n", __func__, freq);
> /* Clear the CSS busy signal */
> readl(base + IMGU_REG_GP_BUSY);
> writel(0, base + IMGU_REG_GP_BUSY);
> diff --git a/drivers/staging/media/ipu3/ipu3-css.h b/drivers/staging/media/ipu3/ipu3-css.h
> index 6b8bab27ab1f..882936a9dae9 100644
> --- a/drivers/staging/media/ipu3/ipu3-css.h
> +++ b/drivers/staging/media/ipu3/ipu3-css.h
> @@ -187,7 +187,8 @@ bool imgu_css_is_streaming(struct imgu_css *css);
> bool imgu_css_pipe_queue_empty(struct imgu_css *css, unsigned int pipe);
>
> /******************* css hw *******************/
> -int imgu_css_set_powerup(struct device *dev, void __iomem *base);
> +int imgu_css_set_powerup(struct device *dev, void __iomem *base,
> + bool low_power);
> void imgu_css_set_powerdown(struct device *dev, void __iomem *base);
> int imgu_css_irq_ack(struct imgu_css *css);
>
> diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
> index 3c7ad1eed434..dcc2a0476e49 100644
> --- a/drivers/staging/media/ipu3/ipu3-v4l2.c
> +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
> @@ -182,6 +182,12 @@ static int imgu_subdev_set_fmt(struct v4l2_subdev *sd,
> fmt->format.height = clamp(fmt->format.height,
> IPU3_INPUT_MIN_HEIGHT,
> IPU3_INPUT_MAX_HEIGHT);
> +
> + /* input less than 2.3MP, ask imgu to work with low freq */
> + if ((fmt->format.width * fmt->format.height) < (2048 * 1152))
Why 2048 * 1152 specifically if we just care about the number of
pixels? Also it's slightly more than 2.3Mpix (2.359296 Mpix) making
the comment inaccurate.
> + imgu->low_power = true;
> + else
> + imgu->low_power = false;
There should be no need to store this, as we should have access to the
exact format when we start streaming. Could you move the check there?
Also, we have 2 pipes. How do they play together?
Best regards,
Tomasz
