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@xxxxxxxxx> > >> --- > >> 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 -- Regards, Laurent Pinchart
