From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9314CC3A59C for ; Fri, 16 Aug 2019 07:07:49 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 5DEAA2133F for ; Fri, 16 Aug 2019 07:07:49 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="QdEGFXeJ" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726958AbfHPHHs (ORCPT ); Fri, 16 Aug 2019 03:07:48 -0400 Received: from mail-ed1-f65.google.com ([209.85.208.65]:35607 "EHLO mail-ed1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726866AbfHPHHs (ORCPT ); Fri, 16 Aug 2019 03:07:48 -0400 Received: by mail-ed1-f65.google.com with SMTP id w20so4301190edd.2 for ; Fri, 16 Aug 2019 00:07:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=8eRWyc41c5Q881PBnau4gPoJpN+PILBi3u8esjZiR2k=; b=QdEGFXeJlObF7YeQxuj2bS0XubLL1l3qCyGlHboNVtKQTC0lkLuqFwgiwNLBgVdr/W mp/oYBG4RAtgg/Qz8MaYPgQ6I58RG2q2WZGlbtTzQKqUMdmH3cX8YFXpIMkubiLsChGc 6uQMo17F9aIn15IAm1+H+/4hFlslam9XoO7hQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=8eRWyc41c5Q881PBnau4gPoJpN+PILBi3u8esjZiR2k=; b=Q+N7XiweF/NObXb4fkC95PJRinWmxyBkEUBS5nj0nKelKMHL8G/z2tXj+vNqBcWeuQ vtPKR7ey2h4x4fp9ZRchkKMDr0i508udZMueIJzGs4IrXX+Lf90dQTxMjE9bS1S5uXOG xgCTalGKOAhO2TMYF69Bmqz/8KBwrtdBlHsnUVQqMNNhHpI7+ygHZVMkJp4OnOe34KRr H6S2dq32UH43BIbkWveQqEVmqjYoQisMwCvqE8veIhFh7fR0YMVVXLCYChy1achlA6il 10CxhyEWXUpRkRq5M6vX+Icw6fcbaRpmelsai7ZgQAo2yd/3rUz5kYJdV1sozN6Sj3t+ 5fdg== X-Gm-Message-State: APjAAAUBJ28nPF1H9TU50Zt6ypmuFkrQ7Z9VlYJejv7VQnuM4xHAjE/q VoO+Gs5IBm2XhA3g4Tv26HBTxFEQthAu/g== X-Google-Smtp-Source: APXvYqyR+eqS6fhv7sTaBtGaNZdMEmqhjGDppn8Y/W/0Pq9ojG97PB6TyoKfTbcTQxMJxlArJKlcUA== X-Received: by 2002:a17:906:a98b:: with SMTP id jr11mr7740422ejb.224.1565939265894; Fri, 16 Aug 2019 00:07:45 -0700 (PDT) Received: from mail-wr1-f44.google.com (mail-wr1-f44.google.com. [209.85.221.44]) by smtp.gmail.com with ESMTPSA id s4sm694173ejq.72.2019.08.16.00.07.45 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 16 Aug 2019 00:07:45 -0700 (PDT) Received: by mail-wr1-f44.google.com with SMTP id k2so566196wrq.2 for ; Fri, 16 Aug 2019 00:07:45 -0700 (PDT) X-Received: by 2002:a5d:5543:: with SMTP id g3mr9024920wrw.166.1565939264766; Fri, 16 Aug 2019 00:07:44 -0700 (PDT) MIME-Version: 1.0 References: <1565926419-2228-1-git-send-email-bingbu.cao@intel.com> In-Reply-To: From: Tomasz Figa Date: Fri, 16 Aug 2019 16:07:32 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH] media: staging: imgu: make imgu work on low frequency for low input To: Bingbu Cao Cc: Cao Bing Bu , Linux Media Mailing List , Sakari Ailus , "Yeh, Andy" , "Qiu, Tian Shu" Content-Type: text/plain; charset="UTF-8" Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org On Fri, Aug 16, 2019 at 3:52 PM Bingbu Cao 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 wrote: > >> > >> From: Bingbu Cao > >> > >> 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 > >> --- > >> 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