Hi, Tomasz Thanks for your review.
On 8/16/19 2:10 PM, Tomasz Figa wrote: > 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? Let me try to move the check into imgu_powerup(). > >> 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. 2048 *1152 is the smallest common 16:9/4:3 resolution that larger than 1080p, we try to use this resolution as the start point to make imgu work on high frequency. > >> + 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? We want to guarantee 2 pipes can always work on each frequency, there is no precise calculation which can be used to make more finer granularity. > > Best regards, > Tomasz >