| Message ID | 1632370218-5508-1-git-send-email-bingbu.cao@intel.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Sakari Ailus |
| Headers |
Received: from vger.kernel.org ([23.128.96.18])
by www.linuxtv.org with esmtp (Exim 4.92)
(envelope-from <linux-media-owner@vger.kernel.org>)
id 1mTG4x-0049tE-I6; Thu, 23 Sep 2021 04:11:23 +0000
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
id S229700AbhIWEMw (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other);
Thu, 23 Sep 2021 00:12:52 -0400
Received: from mga01.intel.com ([192.55.52.88]:31700 "EHLO mga01.intel.com"
rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
id S229436AbhIWEMv (ORCPT <rfc822;linux-media@vger.kernel.org>);
Thu, 23 Sep 2021 00:12:51 -0400
X-IronPort-AV: E=McAfee;i="6200,9189,10115"; a="246206099"
X-IronPort-AV: E=Sophos;i="5.85,315,1624345200";
d="scan'208";a="246206099"
Received: from fmsmga008.fm.intel.com ([10.253.24.58])
by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;
22 Sep 2021 21:11:20 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.85,315,1624345200";
d="scan'208";a="513543287"
Received: from ipu5-build.bj.intel.com ([10.238.232.202])
by fmsmga008.fm.intel.com with ESMTP; 22 Sep 2021 21:11:18 -0700
From: Bingbu Cao <bingbu.cao@intel.com>
To: linux-media@vger.kernel.org, sakari.ailus@linux.intel.com
Cc: tian.shu.qiu@intel.com, laurent.pinchart@ideasonboard.com,
tfiga@chromium.org, bingbu.cao@intel.com,
bingbu.cao@linux.intel.com
Subject: [PATCH] media: ipu3-cio2: Update high watermark to support higher
data rate camera sensors
Date: Thu, 23 Sep 2021 12:10:18 +0800
Message-Id: <1632370218-5508-1-git-send-email-bingbu.cao@intel.com>
X-Mailer: git-send-email 2.7.4
Precedence: bulk
List-ID: <linux-media.vger.kernel.org>
X-Mailing-List: linux-media@vger.kernel.org
X-LSpam-Score: -4.7 (----)
X-LSpam-Report: No,
score=-4.7 required=5.0 tests=BAYES_00=-1.9,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1,RCVD_IN_DNSWL_MED=-2.3
autolearn=ham autolearn_force=no
|
| Series |
media: ipu3-cio2: Update high watermark to support higher data rate camera sensors
|
|
Commit Message
Cao, Bingbu
Sept. 23, 2021, 4:10 a.m. UTC
CIO2 worked well with most camera sensors so far, but CIO2 will meet SRAM
overflow when working with higher data rate camera sensors such as 13M@30fps.
We must set lower high watermark value to trigger the DRAM write to support
such camera sensors.
Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
---
drivers/media/pci/intel/ipu3/ipu3-cio2.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
Hi Bingbu, On Thu, Sep 23, 2021 at 1:11 PM Bingbu Cao <bingbu.cao@intel.com> wrote: > > CIO2 worked well with most camera sensors so far, but CIO2 will meet SRAM > overflow when working with higher data rate camera sensors such as 13M@30fps. > We must set lower high watermark value to trigger the DRAM write to support > such camera sensors. > > Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > --- > drivers/media/pci/intel/ipu3/ipu3-cio2.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Thanks for the patch. Would this have any implications for other (lower) operating modes, such as increased power consumption, or it's harmless? If so, what's the reason we didn't use the value from the very beginning? Best regards, Tomasz > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h > index 3806d7f04d69..fde80d48533b 100644 > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h > @@ -181,7 +181,7 @@ struct pci_dev; > #define CIO2_PBM_WMCTRL1_MID1_2CK (16 << CIO2_PBM_WMCTRL1_MID1_2CK_SHIFT) > #define CIO2_PBM_WMCTRL1_MID2_2CK (21 << CIO2_PBM_WMCTRL1_MID2_2CK_SHIFT) > #define CIO2_REG_PBM_WMCTRL2 0x1468 > -#define CIO2_PBM_WMCTRL2_HWM_2CK 40U > +#define CIO2_PBM_WMCTRL2_HWM_2CK 30U > #define CIO2_PBM_WMCTRL2_HWM_2CK_SHIFT 0U > #define CIO2_PBM_WMCTRL2_LWM_2CK 22U > #define CIO2_PBM_WMCTRL2_LWM_2CK_SHIFT 8U > -- > 2.7.4 >
Tomasz, On 10/6/21 1:03 PM, Tomasz Figa wrote: > Hi Bingbu, > > On Thu, Sep 23, 2021 at 1:11 PM Bingbu Cao <bingbu.cao@intel.com> wrote: >> >> CIO2 worked well with most camera sensors so far, but CIO2 will meet SRAM >> overflow when working with higher data rate camera sensors such as 13M@30fps. >> We must set lower high watermark value to trigger the DRAM write to support >> such camera sensors. >> >> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> >> --- >> drivers/media/pci/intel/ipu3/ipu3-cio2.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> > > Thanks for the patch. Would this have any implications for other > (lower) operating modes, such as increased power consumption, or it's > harmless? If so, what's the reason we didn't use the value from the > very beginning? Yes, we meet that the frame data corruption for some high data rate camera sensors like imx258 (13M@30fps) with current watermark settings. The higher watermark potentially has power concern as it request DMA transfer more than before. To keep the old settings for low data rate camera sensor, I am thinking the rationality to determine the HWM value based on the link_frequency? Apparently, it is not reliable to determine by the format. > > Best regards, > Tomasz > >> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h >> index 3806d7f04d69..fde80d48533b 100644 >> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h >> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h >> @@ -181,7 +181,7 @@ struct pci_dev; >> #define CIO2_PBM_WMCTRL1_MID1_2CK (16 << CIO2_PBM_WMCTRL1_MID1_2CK_SHIFT) >> #define CIO2_PBM_WMCTRL1_MID2_2CK (21 << CIO2_PBM_WMCTRL1_MID2_2CK_SHIFT) >> #define CIO2_REG_PBM_WMCTRL2 0x1468 >> -#define CIO2_PBM_WMCTRL2_HWM_2CK 40U >> +#define CIO2_PBM_WMCTRL2_HWM_2CK 30U >> #define CIO2_PBM_WMCTRL2_HWM_2CK_SHIFT 0U >> #define CIO2_PBM_WMCTRL2_LWM_2CK 22U >> #define CIO2_PBM_WMCTRL2_LWM_2CK_SHIFT 8U >> -- >> 2.7.4 >>
Hi Bingbu, On Thu, Oct 14, 2021 at 02:49:19PM +0800, Bingbu Cao wrote: > On 10/6/21 1:03 PM, Tomasz Figa wrote: > > On Thu, Sep 23, 2021 at 1:11 PM Bingbu Cao wrote: > >> > >> CIO2 worked well with most camera sensors so far, but CIO2 will meet SRAM > >> overflow when working with higher data rate camera sensors such as 13M@30fps. > >> We must set lower high watermark value to trigger the DRAM write to support > >> such camera sensors. > >> > >> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > >> --- > >> drivers/media/pci/intel/ipu3/ipu3-cio2.h | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > > > > Thanks for the patch. Would this have any implications for other > > (lower) operating modes, such as increased power consumption, or it's > > harmless? If so, what's the reason we didn't use the value from the > > very beginning? > > Yes, we meet that the frame data corruption for some high data rate camera sensors like > imx258 (13M@30fps) with current watermark settings. The higher watermark potentially has > power concern as it request DMA transfer more than before. > > To keep the old settings for low data rate camera sensor, I am thinking the rationality > to determine the HWM value based on the link_frequency? Apparently, it is not reliable > to determine by the format. It depends on the SRAM buffer size, on the image width, the horizontal blanking, and the link frequency. If you can store a full line of data, you'll have time during horizontal blanking to finish the DMA transfer, so you can trigger it later. I don't know how the hardware works exactly so I can't provide an exact formula (and I suppose you'll need to reserve some margin to account for other traffic to the DRAM). > >> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h > >> index 3806d7f04d69..fde80d48533b 100644 > >> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h > >> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h > >> @@ -181,7 +181,7 @@ struct pci_dev; > >> #define CIO2_PBM_WMCTRL1_MID1_2CK (16 << CIO2_PBM_WMCTRL1_MID1_2CK_SHIFT) > >> #define CIO2_PBM_WMCTRL1_MID2_2CK (21 << CIO2_PBM_WMCTRL1_MID2_2CK_SHIFT) > >> #define CIO2_REG_PBM_WMCTRL2 0x1468 > >> -#define CIO2_PBM_WMCTRL2_HWM_2CK 40U > >> +#define CIO2_PBM_WMCTRL2_HWM_2CK 30U > >> #define CIO2_PBM_WMCTRL2_HWM_2CK_SHIFT 0U > >> #define CIO2_PBM_WMCTRL2_LWM_2CK 22U > >> #define CIO2_PBM_WMCTRL2_LWM_2CK_SHIFT 8U
On Sat, Oct 16, 2021 at 11:54 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Bingbu, > > On Thu, Oct 14, 2021 at 02:49:19PM +0800, Bingbu Cao wrote: > > On 10/6/21 1:03 PM, Tomasz Figa wrote: > > > On Thu, Sep 23, 2021 at 1:11 PM Bingbu Cao wrote: > > >> > > >> CIO2 worked well with most camera sensors so far, but CIO2 will meet SRAM > > >> overflow when working with higher data rate camera sensors such as 13M@30fps. > > >> We must set lower high watermark value to trigger the DRAM write to support > > >> such camera sensors. > > >> > > >> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > > >> --- > > >> drivers/media/pci/intel/ipu3/ipu3-cio2.h | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > > > > > Thanks for the patch. Would this have any implications for other > > > (lower) operating modes, such as increased power consumption, or it's > > > harmless? If so, what's the reason we didn't use the value from the > > > very beginning? > > > > Yes, we meet that the frame data corruption for some high data rate camera sensors like > > imx258 (13M@30fps) with current watermark settings. The higher watermark potentially has > > power concern as it request DMA transfer more than before. > > > > To keep the old settings for low data rate camera sensor, I am thinking the rationality > > to determine the HWM value based on the link_frequency? Apparently, it is not reliable > > to determine by the format. > > It depends on the SRAM buffer size, on the image width, the horizontal > blanking, and the link frequency. If you can store a full line of data, > you'll have time during horizontal blanking to finish the DMA transfer, > so you can trigger it later. I don't know how the hardware works exactly > so I can't provide an exact formula (and I suppose you'll need to > reserve some margin to account for other traffic to the DRAM). > Yes, it would be good to have a formula. It doesn't have to be exact if it would make it overly complex, just care should be taken so that it doesn't undershoot, as it would cause overflows. > > >> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h > > >> index 3806d7f04d69..fde80d48533b 100644 > > >> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h > > >> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h > > >> @@ -181,7 +181,7 @@ struct pci_dev; > > >> #define CIO2_PBM_WMCTRL1_MID1_2CK (16 << CIO2_PBM_WMCTRL1_MID1_2CK_SHIFT) > > >> #define CIO2_PBM_WMCTRL1_MID2_2CK (21 << CIO2_PBM_WMCTRL1_MID2_2CK_SHIFT) > > >> #define CIO2_REG_PBM_WMCTRL2 0x1468 > > >> -#define CIO2_PBM_WMCTRL2_HWM_2CK 40U > > >> +#define CIO2_PBM_WMCTRL2_HWM_2CK 30U > > >> #define CIO2_PBM_WMCTRL2_HWM_2CK_SHIFT 0U > > >> #define CIO2_PBM_WMCTRL2_LWM_2CK 22U > > >> #define CIO2_PBM_WMCTRL2_LWM_2CK_SHIFT 8U > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h index 3806d7f04d69..fde80d48533b 100644 --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h @@ -181,7 +181,7 @@ struct pci_dev; #define CIO2_PBM_WMCTRL1_MID1_2CK (16 << CIO2_PBM_WMCTRL1_MID1_2CK_SHIFT) #define CIO2_PBM_WMCTRL1_MID2_2CK (21 << CIO2_PBM_WMCTRL1_MID2_2CK_SHIFT) #define CIO2_REG_PBM_WMCTRL2 0x1468 -#define CIO2_PBM_WMCTRL2_HWM_2CK 40U +#define CIO2_PBM_WMCTRL2_HWM_2CK 30U #define CIO2_PBM_WMCTRL2_HWM_2CK_SHIFT 0U #define CIO2_PBM_WMCTRL2_LWM_2CK 22U #define CIO2_PBM_WMCTRL2_LWM_2CK_SHIFT 8U