| Message ID | 20210601112456.118755-3-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. On Tue, Jun 01, 2021 at 04:54:56PM +0530, Umang Jain wrote: > We report a hard-coded frame duration value for CTS. However, when the > CTS is tested on nautilus, which has a 'imx258' sensor, the frame > duration was found to be out-of-range. Since different sensors have > different frame durations range-limits, we need to bump up our value > to match the lower end of the 'imx258' frame duration range-limit > (rounded up to the nearest micro-second). > > This allows the following CTS test to pass on nautilus platform: > - android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/ipa/ipu3/ipu3.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index 700a5660..e51b0401 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -271,7 +271,7 @@ void IPAIPU3::parseStatistics(unsigned int frame, > > /* \todo Populate this with real values */ > ctrls.set(controls::FrameDuration, > - static_cast<int64_t>(33334)); > + static_cast<int64_t>(34483)); I think it's time to fix this properly by calculating the actual frame duration. It should hopefully not be more complicated than duration = (width + hblank) * (height + vblank) / pixel_rate; > > IPU3Action op; > op.op = ActionMetadataReady;
Hi Laurent, On 6/2/21 6:38 AM, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > On Tue, Jun 01, 2021 at 04:54:56PM +0530, Umang Jain wrote: >> We report a hard-coded frame duration value for CTS. However, when the >> CTS is tested on nautilus, which has a 'imx258' sensor, the frame >> duration was found to be out-of-range. Since different sensors have >> different frame durations range-limits, we need to bump up our value >> to match the lower end of the 'imx258' frame duration range-limit >> (rounded up to the nearest micro-second). >> >> This allows the following CTS test to pass on nautilus platform: >> - android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> src/ipa/ipu3/ipu3.cpp | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >> index 700a5660..e51b0401 100644 >> --- a/src/ipa/ipu3/ipu3.cpp >> +++ b/src/ipa/ipu3/ipu3.cpp >> @@ -271,7 +271,7 @@ void IPAIPU3::parseStatistics(unsigned int frame, >> >> /* \todo Populate this with real values */ >> ctrls.set(controls::FrameDuration, >> - static_cast<int64_t>(33334)); >> + static_cast<int64_t>(34483)); > I think it's time to fix this properly by calculating the actual frame > duration. It should hopefully not be more complicated than > > duration = (width + hblank) * (height + vblank) / pixel_rate; We need to get vblank, for each frame's exposure I think. We need to plumb this into IPAIPU3 and JM seem to have the base helper class in-progress. For now, I have slightly made an improvement by using the minVBlank_ value instead. It doesn't makes CTS on nautilus pass, but anyway is an improvement over hard-coded value. I'll check with Paul, to see if it doesn't regress CTS on soraka. Let me know you thoughts! > >> >> IPU3Action op; >> op.op = ActionMetadataReady;
Hi Umang, On Wed, Jun 02, 2021 at 03:57:37PM +0530, Umang Jain wrote: > On 6/2/21 6:38 AM, Laurent Pinchart wrote: > > On Tue, Jun 01, 2021 at 04:54:56PM +0530, Umang Jain wrote: > >> We report a hard-coded frame duration value for CTS. However, when the > >> CTS is tested on nautilus, which has a 'imx258' sensor, the frame > >> duration was found to be out-of-range. Since different sensors have > >> different frame durations range-limits, we need to bump up our value > >> to match the lower end of the 'imx258' frame duration range-limit > >> (rounded up to the nearest micro-second). > >> > >> This allows the following CTS test to pass on nautilus platform: > >> - android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl > >> > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >> --- > >> src/ipa/ipu3/ipu3.cpp | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > >> index 700a5660..e51b0401 100644 > >> --- a/src/ipa/ipu3/ipu3.cpp > >> +++ b/src/ipa/ipu3/ipu3.cpp > >> @@ -271,7 +271,7 @@ void IPAIPU3::parseStatistics(unsigned int frame, > >> > >> /* \todo Populate this with real values */ > >> ctrls.set(controls::FrameDuration, > >> - static_cast<int64_t>(33334)); > >> + static_cast<int64_t>(34483)); > > > > I think it's time to fix this properly by calculating the actual frame > > duration. It should hopefully not be more complicated than > > > > duration = (width + hblank) * (height + vblank) / pixel_rate; > > We need to get vblank, for each frame's exposure I think. We need to > plumb this into IPAIPU3 and JM seem to have the base helper class > in-progress. For now, I have slightly made an improvement by using the > minVBlank_ value instead. It doesn't makes CTS on nautilus pass, but > anyway is an improvement over hard-coded value. I'll check with Paul, to > see if it doesn't regress CTS on soraka. Let me know you thoughts! V4L2_CID_VBLANK is set by userspace. It can be updated by drivers when the format and/or selection rectangles are modified, but will not otherwise be changed automatically at runtime. The IPA should compute the desired vblank value and pass it to the pipeline handler to configure the sensor. I don't think that's related to the work that JM is doing. > >> > >> IPU3Action op; > >> op.op = ActionMetadataReady;
Hi Laurent, On 6/2/21 11:40 PM, Laurent Pinchart wrote: > Hi Umang, > > On Wed, Jun 02, 2021 at 03:57:37PM +0530, Umang Jain wrote: >> On 6/2/21 6:38 AM, Laurent Pinchart wrote: >>> On Tue, Jun 01, 2021 at 04:54:56PM +0530, Umang Jain wrote: >>>> We report a hard-coded frame duration value for CTS. However, when the >>>> CTS is tested on nautilus, which has a 'imx258' sensor, the frame >>>> duration was found to be out-of-range. Since different sensors have >>>> different frame durations range-limits, we need to bump up our value >>>> to match the lower end of the 'imx258' frame duration range-limit >>>> (rounded up to the nearest micro-second). >>>> >>>> This allows the following CTS test to pass on nautilus platform: >>>> - android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl >>>> >>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>>> --- >>>> src/ipa/ipu3/ipu3.cpp | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >>>> index 700a5660..e51b0401 100644 >>>> --- a/src/ipa/ipu3/ipu3.cpp >>>> +++ b/src/ipa/ipu3/ipu3.cpp >>>> @@ -271,7 +271,7 @@ void IPAIPU3::parseStatistics(unsigned int frame, >>>> >>>> /* \todo Populate this with real values */ >>>> ctrls.set(controls::FrameDuration, >>>> - static_cast<int64_t>(33334)); >>>> + static_cast<int64_t>(34483)); >>> I think it's time to fix this properly by calculating the actual frame >>> duration. It should hopefully not be more complicated than >>> >>> duration = (width + hblank) * (height + vblank) / pixel_rate; >> We need to get vblank, for each frame's exposure I think. We need to >> plumb this into IPAIPU3 and JM seem to have the base helper class >> in-progress. For now, I have slightly made an improvement by using the >> minVBlank_ value instead. It doesn't makes CTS on nautilus pass, but >> anyway is an improvement over hard-coded value. I'll check with Paul, to >> see if it doesn't regress CTS on soraka. Let me know you thoughts! > V4L2_CID_VBLANK is set by userspace. It can be updated by drivers when > the format and/or selection rectangles are modified, but will not > otherwise be changed automatically at runtime. The IPA should compute > the desired vblank value and pass it to the pipeline handler to > configure the sensor. I don't think that's related to the work that JM > is doing. Ok, I will check in with JM for that, Meanwhile, I saw and JM also mentioned that, for getting the vblank - we need 'frameIntegrationDiff' (referencing CamHelper::GetVBlanking from RPi IPA). Looking into it, it's seems to a value tied to a particular sensor, so that's something. Not sure if we are going to put in some specific sensor-related parameters into IPU3 IPA. CC-ing JM, Would like your thoughts on the above discussion. > >>>> >>>> IPU3Action op; >>>> op.op = ActionMetadataReady;
Hi Laurent and Umang, On Wed, 2 Jun 2021 at 19:10, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Umang, > > On Wed, Jun 02, 2021 at 03:57:37PM +0530, Umang Jain wrote: > > On 6/2/21 6:38 AM, Laurent Pinchart wrote: > > > On Tue, Jun 01, 2021 at 04:54:56PM +0530, Umang Jain wrote: > > >> We report a hard-coded frame duration value for CTS. However, when the > > >> CTS is tested on nautilus, which has a 'imx258' sensor, the frame > > >> duration was found to be out-of-range. Since different sensors have > > >> different frame durations range-limits, we need to bump up our value > > >> to match the lower end of the 'imx258' frame duration range-limit > > >> (rounded up to the nearest micro-second). > > >> > > >> This allows the following CTS test to pass on nautilus platform: > > >> - > android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl > > >> > > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > >> --- > > >> src/ipa/ipu3/ipu3.cpp | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > > >> index 700a5660..e51b0401 100644 > > >> --- a/src/ipa/ipu3/ipu3.cpp > > >> +++ b/src/ipa/ipu3/ipu3.cpp > > >> @@ -271,7 +271,7 @@ void IPAIPU3::parseStatistics(unsigned int frame, > > >> > > >> /* \todo Populate this with real values */ > > >> ctrls.set(controls::FrameDuration, > > >> - static_cast<int64_t>(33334)); > > >> + static_cast<int64_t>(34483)); > > > > > > I think it's time to fix this properly by calculating the actual frame > > > duration. It should hopefully not be more complicated than > > > > > > duration = (width + hblank) * (height + vblank) / pixel_rate; > > > > We need to get vblank, for each frame's exposure I think. We need to > > plumb this into IPAIPU3 and JM seem to have the base helper class > > in-progress. For now, I have slightly made an improvement by using the > > minVBlank_ value instead. It doesn't makes CTS on nautilus pass, but > > anyway is an improvement over hard-coded value. I'll check with Paul, to > > see if it doesn't regress CTS on soraka. Let me know you thoughts! > > V4L2_CID_VBLANK is set by userspace. It can be updated by drivers when > the format and/or selection rectangles are modified, but will not > otherwise be changed automatically at runtime. The IPA should compute > the desired vblank value and pass it to the pipeline handler to > configure the sensor. I don't think that's related to the work that JM > is doing. > Sorry for jumping in on this topic, but for RPi IPA, it is possible for VBLANK to change at runtime when we are running in variable framerate mode. This decision is down to the AGC and what exposure it decides to use. This is also where we run into the problem with V4L2 control limits not allowing us to update VBLANK and EXPOSURE at the same time without using priority writes. Naush > > >> > > >> IPU3Action op; > > >> op.op = ActionMetadataReady; > > -- > Regards, > > Laurent Pinchart >
Hi Umang, On Thu, Jun 03, 2021 at 08:41:25AM +0530, Umang Jain wrote: > On 6/2/21 11:40 PM, Laurent Pinchart wrote: > > On Wed, Jun 02, 2021 at 03:57:37PM +0530, Umang Jain wrote: > >> On 6/2/21 6:38 AM, Laurent Pinchart wrote: > >>> On Tue, Jun 01, 2021 at 04:54:56PM +0530, Umang Jain wrote: > >>>> We report a hard-coded frame duration value for CTS. However, when the > >>>> CTS is tested on nautilus, which has a 'imx258' sensor, the frame > >>>> duration was found to be out-of-range. Since different sensors have > >>>> different frame durations range-limits, we need to bump up our value > >>>> to match the lower end of the 'imx258' frame duration range-limit > >>>> (rounded up to the nearest micro-second). > >>>> > >>>> This allows the following CTS test to pass on nautilus platform: > >>>> - android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl > >>>> > >>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >>>> --- > >>>> src/ipa/ipu3/ipu3.cpp | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > >>>> index 700a5660..e51b0401 100644 > >>>> --- a/src/ipa/ipu3/ipu3.cpp > >>>> +++ b/src/ipa/ipu3/ipu3.cpp > >>>> @@ -271,7 +271,7 @@ void IPAIPU3::parseStatistics(unsigned int frame, > >>>> > >>>> /* \todo Populate this with real values */ > >>>> ctrls.set(controls::FrameDuration, > >>>> - static_cast<int64_t>(33334)); > >>>> + static_cast<int64_t>(34483)); > >>> > >>> I think it's time to fix this properly by calculating the actual frame > >>> duration. It should hopefully not be more complicated than > >>> > >>> duration = (width + hblank) * (height + vblank) / pixel_rate; > >> > >> We need to get vblank, for each frame's exposure I think. We need to > >> plumb this into IPAIPU3 and JM seem to have the base helper class > >> in-progress. For now, I have slightly made an improvement by using the > >> minVBlank_ value instead. It doesn't makes CTS on nautilus pass, but > >> anyway is an improvement over hard-coded value. I'll check with Paul, to > >> see if it doesn't regress CTS on soraka. Let me know you thoughts! > > > > V4L2_CID_VBLANK is set by userspace. It can be updated by drivers when > > the format and/or selection rectangles are modified, but will not > > otherwise be changed automatically at runtime. The IPA should compute > > the desired vblank value and pass it to the pipeline handler to > > configure the sensor. I don't think that's related to the work that JM > > is doing. > > Ok, I will check in with JM for that, > > Meanwhile, I saw and JM also mentioned that, for getting the vblank - we > need 'frameIntegrationDiff' (referencing CamHelper::GetVBlanking from > RPi IPA). Looking into it, it's seems to a value tied to a particular > sensor, so that's something. Not sure if we are going to put in some > specific sensor-related parameters into IPU3 IPA. We will, Jean-Michel is working on that. The frame integration diff value will be used by the IPA to compute the desired exposure time and/or vertical blanking, but you won't need to care about it to compute the frame duration. Only the vblank value is needed for that. > CC-ing JM, Would like your thoughts on the above discussion. > > >>>> > >>>> IPU3Action op; > >>>> op.op = ActionMetadataReady;
Hi Naush, On Thu, Jun 03, 2021 at 11:33:54AM +0100, Naushir Patuck wrote: > On Wed, 2 Jun 2021 at 19:10, Laurent Pinchart wrote: > > On Wed, Jun 02, 2021 at 03:57:37PM +0530, Umang Jain wrote: > > > On 6/2/21 6:38 AM, Laurent Pinchart wrote: > > > > On Tue, Jun 01, 2021 at 04:54:56PM +0530, Umang Jain wrote: > > > >> We report a hard-coded frame duration value for CTS. However, when the > > > >> CTS is tested on nautilus, which has a 'imx258' sensor, the frame > > > >> duration was found to be out-of-range. Since different sensors have > > > >> different frame durations range-limits, we need to bump up our value > > > >> to match the lower end of the 'imx258' frame duration range-limit > > > >> (rounded up to the nearest micro-second). > > > >> > > > >> This allows the following CTS test to pass on nautilus platform: > > > >> - android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl > > > >> > > > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > >> --- > > > >> src/ipa/ipu3/ipu3.cpp | 2 +- > > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > > >> > > > >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > > > >> index 700a5660..e51b0401 100644 > > > >> --- a/src/ipa/ipu3/ipu3.cpp > > > >> +++ b/src/ipa/ipu3/ipu3.cpp > > > >> @@ -271,7 +271,7 @@ void IPAIPU3::parseStatistics(unsigned int frame, > > > >> > > > >> /* \todo Populate this with real values */ > > > >> ctrls.set(controls::FrameDuration, > > > >> - static_cast<int64_t>(33334)); > > > >> + static_cast<int64_t>(34483)); > > > > > > > > I think it's time to fix this properly by calculating the actual frame > > > > duration. It should hopefully not be more complicated than > > > > > > > > duration = (width + hblank) * (height + vblank) / pixel_rate; > > > > > > We need to get vblank, for each frame's exposure I think. We need to > > > plumb this into IPAIPU3 and JM seem to have the base helper class > > > in-progress. For now, I have slightly made an improvement by using the > > > minVBlank_ value instead. It doesn't makes CTS on nautilus pass, but > > > anyway is an improvement over hard-coded value. I'll check with Paul, to > > > see if it doesn't regress CTS on soraka. Let me know you thoughts! > > > > V4L2_CID_VBLANK is set by userspace. It can be updated by drivers when > > the format and/or selection rectangles are modified, but will not > > otherwise be changed automatically at runtime. The IPA should compute > > the desired vblank value and pass it to the pipeline handler to > > configure the sensor. I don't think that's related to the work that JM > > is doing. > > Sorry for jumping in on this topic, but for RPi IPA, it is possible for VBLANK to > change at runtime when we are running in variable framerate mode. This > decision is down to the AGC and what exposure it decides to use. Sure, my point was that the IPA controls the vblank value, the driver doesn't adjust it automatically at runtime. > This is also where we run into the problem with V4L2 control limits not allowing > us to update VBLANK and EXPOSURE at the same time without using > priority writes. > > > > >> > > > >> IPU3Action op; > > > >> op.op = ActionMetadataReady;
Hi Laurent, On 6/7/21 5:35 AM, Laurent Pinchart wrote: > Hi Umang, > > On Thu, Jun 03, 2021 at 08:41:25AM +0530, Umang Jain wrote: >> On 6/2/21 11:40 PM, Laurent Pinchart wrote: >>> On Wed, Jun 02, 2021 at 03:57:37PM +0530, Umang Jain wrote: >>>> On 6/2/21 6:38 AM, Laurent Pinchart wrote: >>>>> On Tue, Jun 01, 2021 at 04:54:56PM +0530, Umang Jain wrote: >>>>>> We report a hard-coded frame duration value for CTS. However, when the >>>>>> CTS is tested on nautilus, which has a 'imx258' sensor, the frame >>>>>> duration was found to be out-of-range. Since different sensors have >>>>>> different frame durations range-limits, we need to bump up our value >>>>>> to match the lower end of the 'imx258' frame duration range-limit >>>>>> (rounded up to the nearest micro-second). >>>>>> >>>>>> This allows the following CTS test to pass on nautilus platform: >>>>>> - android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl >>>>>> >>>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>>>>> --- >>>>>> src/ipa/ipu3/ipu3.cpp | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >>>>>> index 700a5660..e51b0401 100644 >>>>>> --- a/src/ipa/ipu3/ipu3.cpp >>>>>> +++ b/src/ipa/ipu3/ipu3.cpp >>>>>> @@ -271,7 +271,7 @@ void IPAIPU3::parseStatistics(unsigned int frame, >>>>>> >>>>>> /* \todo Populate this with real values */ >>>>>> ctrls.set(controls::FrameDuration, >>>>>> - static_cast<int64_t>(33334)); >>>>>> + static_cast<int64_t>(34483)); >>>>> I think it's time to fix this properly by calculating the actual frame >>>>> duration. It should hopefully not be more complicated than >>>>> >>>>> duration = (width + hblank) * (height + vblank) / pixel_rate; >>>> We need to get vblank, for each frame's exposure I think. We need to >>>> plumb this into IPAIPU3 and JM seem to have the base helper class >>>> in-progress. For now, I have slightly made an improvement by using the >>>> minVBlank_ value instead. It doesn't makes CTS on nautilus pass, but >>>> anyway is an improvement over hard-coded value. I'll check with Paul, to >>>> see if it doesn't regress CTS on soraka. Let me know you thoughts! >>> V4L2_CID_VBLANK is set by userspace. It can be updated by drivers when >>> the format and/or selection rectangles are modified, but will not >>> otherwise be changed automatically at runtime. The IPA should compute >>> the desired vblank value and pass it to the pipeline handler to >>> configure the sensor. I don't think that's related to the work that JM >>> is doing. >> Ok, I will check in with JM for that, >> >> Meanwhile, I saw and JM also mentioned that, for getting the vblank - we >> need 'frameIntegrationDiff' (referencing CamHelper::GetVBlanking from >> RPi IPA). Looking into it, it's seems to a value tied to a particular >> sensor, so that's something. Not sure if we are going to put in some >> specific sensor-related parameters into IPU3 IPA. > We will, Jean-Michel is working on that. > > The frame integration diff value will be used by the IPA to compute the > desired exposure time and/or vertical blanking, but you won't need to > care about it to compute the frame duration. Only the vblank value is > needed for that. Yes, the vblank value is needed and currently I am using the minimum vblank value queried from sensor controls in the " ipa: ipu3: Calculate frame duration from minimum VBLANK value" patch. It also contains the \todo stating that it can be improved on, by using vblank from each frame's exposure. Can you please take a look at that series too, "IPAIPU3 drive-by improvements" > >> CC-ing JM, Would like your thoughts on the above discussion. >> >>>>>> >>>>>> IPU3Action op; >>>>>> op.op = ActionMetadataReady;
Hi Umang, On Mon, Jun 07, 2021 at 06:59:57PM +0530, Umang Jain wrote: > On 6/7/21 5:35 AM, Laurent Pinchart wrote: > > On Thu, Jun 03, 2021 at 08:41:25AM +0530, Umang Jain wrote: > >> On 6/2/21 11:40 PM, Laurent Pinchart wrote: > >>> On Wed, Jun 02, 2021 at 03:57:37PM +0530, Umang Jain wrote: > >>>> On 6/2/21 6:38 AM, Laurent Pinchart wrote: > >>>>> On Tue, Jun 01, 2021 at 04:54:56PM +0530, Umang Jain wrote: > >>>>>> We report a hard-coded frame duration value for CTS. However, when the > >>>>>> CTS is tested on nautilus, which has a 'imx258' sensor, the frame > >>>>>> duration was found to be out-of-range. Since different sensors have > >>>>>> different frame durations range-limits, we need to bump up our value > >>>>>> to match the lower end of the 'imx258' frame duration range-limit > >>>>>> (rounded up to the nearest micro-second). > >>>>>> > >>>>>> This allows the following CTS test to pass on nautilus platform: > >>>>>> - android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl > >>>>>> > >>>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >>>>>> --- > >>>>>> src/ipa/ipu3/ipu3.cpp | 2 +- > >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > >>>>>> index 700a5660..e51b0401 100644 > >>>>>> --- a/src/ipa/ipu3/ipu3.cpp > >>>>>> +++ b/src/ipa/ipu3/ipu3.cpp > >>>>>> @@ -271,7 +271,7 @@ void IPAIPU3::parseStatistics(unsigned int frame, > >>>>>> > >>>>>> /* \todo Populate this with real values */ > >>>>>> ctrls.set(controls::FrameDuration, > >>>>>> - static_cast<int64_t>(33334)); > >>>>>> + static_cast<int64_t>(34483)); > >>>>> > >>>>> I think it's time to fix this properly by calculating the actual frame > >>>>> duration. It should hopefully not be more complicated than > >>>>> > >>>>> duration = (width + hblank) * (height + vblank) / pixel_rate; > >>>> > >>>> We need to get vblank, for each frame's exposure I think. We need to > >>>> plumb this into IPAIPU3 and JM seem to have the base helper class > >>>> in-progress. For now, I have slightly made an improvement by using the > >>>> minVBlank_ value instead. It doesn't makes CTS on nautilus pass, but > >>>> anyway is an improvement over hard-coded value. I'll check with Paul, to > >>>> see if it doesn't regress CTS on soraka. Let me know you thoughts! > >>> > >>> V4L2_CID_VBLANK is set by userspace. It can be updated by drivers when > >>> the format and/or selection rectangles are modified, but will not > >>> otherwise be changed automatically at runtime. The IPA should compute > >>> the desired vblank value and pass it to the pipeline handler to > >>> configure the sensor. I don't think that's related to the work that JM > >>> is doing. > >> > >> Ok, I will check in with JM for that, > >> > >> Meanwhile, I saw and JM also mentioned that, for getting the vblank - we > >> need 'frameIntegrationDiff' (referencing CamHelper::GetVBlanking from > >> RPi IPA). Looking into it, it's seems to a value tied to a particular > >> sensor, so that's something. Not sure if we are going to put in some > >> specific sensor-related parameters into IPU3 IPA. > > > > We will, Jean-Michel is working on that. > > > > The frame integration diff value will be used by the IPA to compute the > > desired exposure time and/or vertical blanking, but you won't need to > > care about it to compute the frame duration. Only the vblank value is > > needed for that. > > Yes, the vblank value is needed and currently I am using the minimum > vblank value > queried from sensor controls in the " ipa: ipu3: Calculate frame > duration from minimum VBLANK value" > patch. It also contains the \todo stating that it can be improved on, by > using vblank from each frame's exposure. Any chance we could address this \todo now already ? I fear it may be a cause for regressions. > Can you please take a look at that series too, "IPAIPU3 drive-by > improvements" Next on my list :-) > >> CC-ing JM, Would like your thoughts on the above discussion. > >> > >>>>>> > >>>>>> IPU3Action op; > >>>>>> op.op = ActionMetadataReady;
diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index 700a5660..e51b0401 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -271,7 +271,7 @@ void IPAIPU3::parseStatistics(unsigned int frame, /* \todo Populate this with real values */ ctrls.set(controls::FrameDuration, - static_cast<int64_t>(33334)); + static_cast<int64_t>(34483)); IPU3Action op; op.op = ActionMetadataReady;
We report a hard-coded frame duration value for CTS. However, when the CTS is tested on nautilus, which has a 'imx258' sensor, the frame duration was found to be out-of-range. Since different sensors have different frame durations range-limits, we need to bump up our value to match the lower end of the 'imx258' frame duration range-limit (rounded up to the nearest micro-second). This allows the following CTS test to pass on nautilus platform: - android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/ipa/ipu3/ipu3.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)