Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Tomasz Figa <tfiga@chromium.org>
To: Bingbu Cao <bingbu.cao@linux.intel.com>
Cc: Cao Bing Bu <bingbu.cao@intel.com>,
	Linux Media Mailing List <linux-media@vger.kernel.org>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	"Yeh, Andy" <andy.yeh@intel.com>,
	"Qiu, Tian Shu" <tian.shu.qiu@intel.com>
Subject: Re: [PATCH] media: staging: imgu: make imgu work on low frequency for low input
Date: Fri, 16 Aug 2019 16:07:32 +0900
Message-ID: <CAAFQd5DYjLFzj1ZS5v0zvr=61MxbZmOO4Dtgjs=hZT20YbHk+g@mail.gmail.com> (raw)
In-Reply-To: <f8f4e40a-b3ae-d75e-d2c2-0c667b0e8b52@linux.intel.com>

On Fri, Aug 16, 2019 at 3:52 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote:
>
> 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.cao@intel.com> wrote:
> >>
> >> From: Bingbu Cao <bingbu.cao@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.cao@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.

I'm still not sure what's the reason for this particular value. What's
exactly the hardware specification for 200 MHz?

> >
> >> +                       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.

i'm not sure I follow. imgu->low_power is a variable common to both
pipes, but it can be changed from subdev_s_fmt() of each pipe
separately.

Consider this example:
- pipe 0 s_fmt 2048x1536 -> low_power = false;
- pipe 1 s_fmt 1024x768 -> low_power = truel

We end up with low_power despite the resolution on pipe 0 being higher
than the thershold.

Best regards,
Tomasz

  reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-16  3:33 bingbu.cao
2019-08-16  6:10 ` Tomasz Figa
2019-08-16  7:00   ` Bingbu Cao
2019-08-16  7:07     ` Tomasz Figa [this message]
2019-08-16 11:02       ` Bingbu Cao

Reply instructions:

You may reply publically 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='CAAFQd5DYjLFzj1ZS5v0zvr=61MxbZmOO4Dtgjs=hZT20YbHk+g@mail.gmail.com' \
    --to=tfiga@chromium.org \
    --cc=andy.yeh@intel.com \
    --cc=bingbu.cao@intel.com \
    --cc=bingbu.cao@linux.intel.com \
    --cc=linux-media@vger.kernel.org \
    --cc=sakari.ailus@linux.intel.com \
    --cc=tian.shu.qiu@intel.com \
    /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 linux-media@archiver.kernel.org
	public-inbox-index linux-media


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