[PATCH] media: ipu3-cio2: Update high watermark to support higher data rate camera sensors

[PATCH] media: ipu3-cio2: Update high watermark to support higher data rate camera sensors

[Bingbu Cao]

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(-)

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

Re: [PATCH] media: ipu3-cio2: Update high watermark to support higher data rate camera sensors

[Tomasz Figa]

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
>

Re: [PATCH] media: ipu3-cio2: Update high watermark to support higher data rate camera sensors

[Bingbu Cao]

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
>>

-- 
Best regards,
Bingbu Cao

Re: [PATCH] media: ipu3-cio2: Update high watermark to support higher data rate camera sensors

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: ipu3-cio2: Update high watermark to support higher data rate camera sensors

[Tomasz Figa]

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