On Sat, Oct 16, 2021 at 11:54 AM Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> 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@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). > 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
