Hi Bingbu, On Fri, Aug 16, 2019 at 12:25 PM <bingbu....@intel.com> wrote: > > From: Bingbu Cao <bingbu....@intel.com> > > 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....@intel.com> > --- > 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