| Message ID | 1632370218-5508-1-git-send-email-bingbu.cao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ipu3-cio2: Update high watermark to support higher data rate camera sensors | expand |
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
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(-)