| Message ID | 20241004045338.18443-1-umang.jain@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang On Fri, Oct 04, 2024 at 10:23:38AM GMT, Umang Jain wrote: > Commit 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not > supported by the pipeline") introduced a mechanism to determine maximum > supported sensor resolution and filter out resolutions that cannot be > supported by the ISP. > > However, it missed to update the raw stream configuration path, where > it should have clamped the raw stream configuration size to the maximum > sensor supported resolution. > > This patch fixes the above issue and can be confirmed with IMX283 > on i.MX8MP: > > From: > ($) cam -c1 -srole=raw,width=5472,height=3072 > INFO Camera camera.cpp:1197 configuring streams: (0) 5472x3648-SRGGB12 > ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12 > Failed to configure camera > Failed to start camera session > > To: > ($) cam -c1 -srole=raw,width=5472,height=3072 > INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12 > cam0: Capture until user interrupts by SIGINT > 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824 > 536.182378 (10.00 fps) cam0-stream0 seq: 000001 bytesused: 25165824 > 536.282375 (10.00 fps) cam0-stream0 seq: 000002 bytesused: 25165824 > ... > > Fixes: 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline") > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > index da8d25c3..feb6d89f 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > @@ -316,9 +316,11 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor, > if (isRaw) { > /* > * Use the sensor output size closest to the requested stream > - * size. > + * size while ensuring the output size doesn't exceed ISP limits. > */ > uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat); > + cfg->size.boundTo(resolution); > + > V4L2SubdeviceFormat sensorFormat = > sensor->getFormat({ mbusCode }, cfg->size); CameraSensor::getFormat() returns a sensor resolution large enough to accomodate the requested size, at least that's how I read the following part of the documentation * \a size indicates the desired size at the output of the sensor. This function * selects the best media bus code and size supported by the sensor according * to the following criteria. * * - The desired \a size shall fit in the sensor output size to avoid the need * to up-scale. And this part of the code if (sz.width < size.width || sz.height < size.height) continue; So, at least in my understanding "sensorFormat" could represent a size larger than cfg->size. Is this your understanding as well ? In the lines below this function we have minResolution = sensorFormat.size; maxResolution = sensorFormat.size; cfg->size.boundTo(maxResolution); cfg->size.expandTo(minResolution); So if ((maxResolution = minResolution) > cfg->size) will the calls to boundTo() followed by expandTo() enlarge cfg->size ? Wouldn't it be better to use 'resolution' in V4L2SubdeviceFormat sensorFormat = sensor->getFormat({ mbusCode }, cfg->size); instead of cfg->size ? Thanks j > > -- > 2.45.2 >
Hi Jacopo On 04/10/24 3:07 pm, Jacopo Mondi wrote: > Hi Umang > > On Fri, Oct 04, 2024 at 10:23:38AM GMT, Umang Jain wrote: >> Commit 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not >> supported by the pipeline") introduced a mechanism to determine maximum >> supported sensor resolution and filter out resolutions that cannot be >> supported by the ISP. >> >> However, it missed to update the raw stream configuration path, where >> it should have clamped the raw stream configuration size to the maximum >> sensor supported resolution. >> >> This patch fixes the above issue and can be confirmed with IMX283 >> on i.MX8MP: >> >> From: >> ($) cam -c1 -srole=raw,width=5472,height=3072 >> INFO Camera camera.cpp:1197 configuring streams: (0) 5472x3648-SRGGB12 >> ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12 >> Failed to configure camera >> Failed to start camera session >> >> To: >> ($) cam -c1 -srole=raw,width=5472,height=3072 >> INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12 >> cam0: Capture until user interrupts by SIGINT >> 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824 >> 536.182378 (10.00 fps) cam0-stream0 seq: 000001 bytesused: 25165824 >> 536.282375 (10.00 fps) cam0-stream0 seq: 000002 bytesused: 25165824 >> ... >> >> Fixes: 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline") >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >> index da8d25c3..feb6d89f 100644 >> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >> @@ -316,9 +316,11 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor, >> if (isRaw) { >> /* >> * Use the sensor output size closest to the requested stream >> - * size. >> + * size while ensuring the output size doesn't exceed ISP limits. >> */ >> uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat); >> + cfg->size.boundTo(resolution); >> + >> V4L2SubdeviceFormat sensorFormat = >> sensor->getFormat({ mbusCode }, cfg->size); > CameraSensor::getFormat() returns a sensor resolution large enough to > accomodate the requested size, at least that's how I read the > following part of the documentation > > * \a size indicates the desired size at the output of the sensor. This function > * selects the best media bus code and size supported by the sensor according > * to the following criteria. > * > * - The desired \a size shall fit in the sensor output size to avoid the need > * to up-scale. > > And this part of the code > > if (sz.width < size.width || sz.height < size.height) > continue; > > So, at least in my understanding "sensorFormat" could represent a size > larger than cfg->size. Is this your understanding as well ? "sensorFormat" could represent a size larger than cfg->size and could also represent a size large than max supported resolution (i.e. 'resolution' variable in this case) For e.g. in IMX283 case it would, 5472x3648 > > In the lines below this function we have > > minResolution = sensorFormat.size; > maxResolution = sensorFormat.size; From the above e.g. of IMX283 minResolution = maxResolution = 5472x3648 > > cfg->size.boundTo(maxResolution); > cfg->size.expandTo(minResolution); > > So if ((maxResolution = minResolution) > cfg->size) will the calls to > boundTo() followed by expandTo() enlarge cfg->size ? so cfg->size is 5472x3648 here, which can't be supported. Have you noticed the failure case I mentioned in the commit message? > > Wouldn't it be better to use 'resolution' in > > V4L2SubdeviceFormat sensorFormat = > sensor->getFormat({ mbusCode }, cfg->size); > > instead of cfg->size ? No, otherwise it will always pick the highest sensor output size. For e.g. I request -srole=raw,width=1920,height=1080 "cfg->size" will pick 2736x1824 for IMX283 "resolution" will always pick 4096x3072. > > Thanks > j > >> -- >> 2.45.2 >>
Hi Umang On Fri, Oct 04, 2024 at 03:17:34PM GMT, Umang Jain wrote: > Hi Jacopo > > On 04/10/24 3:07 pm, Jacopo Mondi wrote: > > Hi Umang > > > > On Fri, Oct 04, 2024 at 10:23:38AM GMT, Umang Jain wrote: > > > Commit 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not > > > supported by the pipeline") introduced a mechanism to determine maximum > > > supported sensor resolution and filter out resolutions that cannot be > > > supported by the ISP. > > > > > > However, it missed to update the raw stream configuration path, where > > > it should have clamped the raw stream configuration size to the maximum > > > sensor supported resolution. > > > > > > This patch fixes the above issue and can be confirmed with IMX283 > > > on i.MX8MP: > > > > > > From: > > > ($) cam -c1 -srole=raw,width=5472,height=3072 > > > INFO Camera camera.cpp:1197 configuring streams: (0) 5472x3648-SRGGB12 > > > ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12 > > > Failed to configure camera > > > Failed to start camera session > > > > > > To: > > > ($) cam -c1 -srole=raw,width=5472,height=3072 > > > INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12 > > > cam0: Capture until user interrupts by SIGINT > > > 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824 > > > 536.182378 (10.00 fps) cam0-stream0 seq: 000001 bytesused: 25165824 > > > 536.282375 (10.00 fps) cam0-stream0 seq: 000002 bytesused: 25165824 > > > ... > > > > > > Fixes: 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline") > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > --- > > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > index da8d25c3..feb6d89f 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > @@ -316,9 +316,11 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor, > > > if (isRaw) { > > > /* > > > * Use the sensor output size closest to the requested stream > > > - * size. > > > + * size while ensuring the output size doesn't exceed ISP limits. > > > */ > > > uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat); > > > + cfg->size.boundTo(resolution); > > > + > > > V4L2SubdeviceFormat sensorFormat = > > > sensor->getFormat({ mbusCode }, cfg->size); > > CameraSensor::getFormat() returns a sensor resolution large enough to > > accomodate the requested size, at least that's how I read the > > following part of the documentation > > > > * \a size indicates the desired size at the output of the sensor. This function > > * selects the best media bus code and size supported by the sensor according > > * to the following criteria. > > * > > * - The desired \a size shall fit in the sensor output size to avoid the need > > * to up-scale. > > > > And this part of the code > > > > if (sz.width < size.width || sz.height < size.height) > > continue; > > > > So, at least in my understanding "sensorFormat" could represent a size > > larger than cfg->size. Is this your understanding as well ? > > "sensorFormat" could represent a size larger than cfg->size > > and > > could also represent a size large than max supported resolution (i.e. > 'resolution' variable in this case) > > For e.g. in IMX283 case it would, 5472x3648 > > > > > In the lines below this function we have > > > > minResolution = sensorFormat.size; > > maxResolution = sensorFormat.size; > > From the above e.g. of IMX283 minResolution = maxResolution = 5472x3648 > > > > cfg->size.boundTo(maxResolution); > > cfg->size.expandTo(minResolution); > > > > So if ((maxResolution = minResolution) > cfg->size) will the calls to > > boundTo() followed by expandTo() enlarge cfg->size ? > > so cfg->size is 5472x3648 here, which can't be supported. Sorry, I'm confused. Aren't you confirming me that the resulting cfg->size cannot be supported (with this patch I mean) ? > > Have you noticed the failure case I mentioned in the commit message? While the commmit message suggests this patch makes it valid ? What am I missing ? :) > > > > Wouldn't it be better to use 'resolution' in > > > > V4L2SubdeviceFormat sensorFormat = > > sensor->getFormat({ mbusCode }, cfg->size); > > > > instead of cfg->size ? > > No, otherwise it will always pick the highest sensor output size. > > For e.g. I request -srole=raw,width=1920,height=1080 > > "cfg->size" will pick 2736x1824 for IMX283 > > "resolution" will always pick 4096x3072. > > > > Thanks > > j > > > > > -- > > > 2.45.2 > > > >
Hi Jacopo, On 04/10/24 3:58 pm, Jacopo Mondi wrote: > Hi Umang > > On Fri, Oct 04, 2024 at 03:17:34PM GMT, Umang Jain wrote: >> Hi Jacopo >> >> On 04/10/24 3:07 pm, Jacopo Mondi wrote: >>> Hi Umang >>> >>> On Fri, Oct 04, 2024 at 10:23:38AM GMT, Umang Jain wrote: >>>> Commit 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not >>>> supported by the pipeline") introduced a mechanism to determine maximum >>>> supported sensor resolution and filter out resolutions that cannot be >>>> supported by the ISP. >>>> >>>> However, it missed to update the raw stream configuration path, where >>>> it should have clamped the raw stream configuration size to the maximum >>>> sensor supported resolution. >>>> >>>> This patch fixes the above issue and can be confirmed with IMX283 >>>> on i.MX8MP: >>>> >>>> From: >>>> ($) cam -c1 -srole=raw,width=5472,height=3072 >>>> INFO Camera camera.cpp:1197 configuring streams: (0) 5472x3648-SRGGB12 >>>> ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12 >>>> Failed to configure camera >>>> Failed to start camera session >>>> >>>> To: >>>> ($) cam -c1 -srole=raw,width=5472,height=3072 >>>> INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12 >>>> cam0: Capture until user interrupts by SIGINT >>>> 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824 >>>> 536.182378 (10.00 fps) cam0-stream0 seq: 000001 bytesused: 25165824 >>>> 536.282375 (10.00 fps) cam0-stream0 seq: 000002 bytesused: 25165824 >>>> ... >>>> >>>> Fixes: 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline") >>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>>> --- >>>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >>>> index da8d25c3..feb6d89f 100644 >>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >>>> @@ -316,9 +316,11 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor, >>>> if (isRaw) { >>>> /* >>>> * Use the sensor output size closest to the requested stream >>>> - * size. >>>> + * size while ensuring the output size doesn't exceed ISP limits. >>>> */ >>>> uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat); >>>> + cfg->size.boundTo(resolution); >>>> + >>>> V4L2SubdeviceFormat sensorFormat = >>>> sensor->getFormat({ mbusCode }, cfg->size); >>> CameraSensor::getFormat() returns a sensor resolution large enough to >>> accomodate the requested size, at least that's how I read the >>> following part of the documentation >>> >>> * \a size indicates the desired size at the output of the sensor. This function >>> * selects the best media bus code and size supported by the sensor according >>> * to the following criteria. >>> * >>> * - The desired \a size shall fit in the sensor output size to avoid the need >>> * to up-scale. >>> >>> And this part of the code >>> >>> if (sz.width < size.width || sz.height < size.height) >>> continue; >>> >>> So, at least in my understanding "sensorFormat" could represent a size >>> larger than cfg->size. Is this your understanding as well ? >> "sensorFormat" could represent a size larger than cfg->size >> >> and >> >> could also represent a size large than max supported resolution (i.e. >> 'resolution' variable in this case) >> >> For e.g. in IMX283 case it would, 5472x3648 >> >>> In the lines below this function we have >>> >>> minResolution = sensorFormat.size; >>> maxResolution = sensorFormat.size; >> From the above e.g. of IMX283 minResolution = maxResolution = 5472x3648 >>> cfg->size.boundTo(maxResolution); >>> cfg->size.expandTo(minResolution); >>> >>> So if ((maxResolution = minResolution) > cfg->size) will the calls to >>> boundTo() followed by expandTo() enlarge cfg->size ? >> so cfg->size is 5472x3648 here, which can't be supported. > Sorry, I'm confused. Aren't you confirming me that the resulting > cfg->size cannot be supported (with this patch I mean) ? So user passes a relatively high resolution, supported by the sensor (but not with ISP) cam -c1 -srole=raw,width=5472,height=3648 so cfg-size here is 5472x3648. So without this patch, you would right now get: ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12 Failed to configure camera Failed to start camera session With this patch patched, you would get: INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12 cam0: Capture until user interrupts by SIGINT 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824 .... So What has happened: User has requested a raw stream with 5472x3648. It was found that this can't be supported hence, the camera configuration is adjusted to a (lower) but maximum supported resolution and delivers raw frames with 4096x3072. One can argue that we should never adjust camera configuration to a lower size than cfg->size (i.e. what the user asked). But then, * \var CameraConfiguration::Adjusted * The configuration has been adjusted to a valid configuration and * \retval CameraConfigutation::Adjusted The configuration has been adjusted * and is now valid. Parameters may have changed for any stream, and stream * configurations may have been removed. The caller shall check the * configuration carefully. > >> Have you noticed the failure case I mentioned in the commit message? > While the commmit message suggests this patch makes it valid ? > > What am I missing ? :) > >>> Wouldn't it be better to use 'resolution' in >>> >>> V4L2SubdeviceFormat sensorFormat = >>> sensor->getFormat({ mbusCode }, cfg->size); >>> >>> instead of cfg->size ? >> No, otherwise it will always pick the highest sensor output size. >> >> For e.g. I request -srole=raw,width=1920,height=1080 >> >> "cfg->size" will pick 2736x1824 for IMX283 >> >> "resolution" will always pick 4096x3072. >>> Thanks >>> j >>> >>>> -- >>>> 2.45.2 >>>>
Umang, seems like I'm failing to make my point across, sorry about this On Fri, Oct 04, 2024 at 04:45:01PM GMT, Umang Jain wrote: > Hi Jacopo, > > On 04/10/24 3:58 pm, Jacopo Mondi wrote: > > Hi Umang > > > > On Fri, Oct 04, 2024 at 03:17:34PM GMT, Umang Jain wrote: > > > Hi Jacopo > > > > > > On 04/10/24 3:07 pm, Jacopo Mondi wrote: > > > > Hi Umang > > > > > > > > On Fri, Oct 04, 2024 at 10:23:38AM GMT, Umang Jain wrote: > > > > > Commit 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not > > > > > supported by the pipeline") introduced a mechanism to determine maximum > > > > > supported sensor resolution and filter out resolutions that cannot be > > > > > supported by the ISP. > > > > > > > > > > However, it missed to update the raw stream configuration path, where > > > > > it should have clamped the raw stream configuration size to the maximum > > > > > sensor supported resolution. > > > > > > > > > > This patch fixes the above issue and can be confirmed with IMX283 > > > > > on i.MX8MP: > > > > > > > > > > From: > > > > > ($) cam -c1 -srole=raw,width=5472,height=3072 > > > > > INFO Camera camera.cpp:1197 configuring streams: (0) 5472x3648-SRGGB12 > > > > > ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12 > > > > > Failed to configure camera > > > > > Failed to start camera session > > > > > > > > > > To: > > > > > ($) cam -c1 -srole=raw,width=5472,height=3072 > > > > > INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12 > > > > > cam0: Capture until user interrupts by SIGINT > > > > > 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824 > > > > > 536.182378 (10.00 fps) cam0-stream0 seq: 000001 bytesused: 25165824 > > > > > 536.282375 (10.00 fps) cam0-stream0 seq: 000002 bytesused: 25165824 > > > > > ... > > > > > > > > > > Fixes: 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline") > > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > > > --- > > > > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++- > > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > > > index da8d25c3..feb6d89f 100644 > > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > > > @@ -316,9 +316,11 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor, > > > > > if (isRaw) { > > > > > /* > > > > > * Use the sensor output size closest to the requested stream > > > > > - * size. > > > > > + * size while ensuring the output size doesn't exceed ISP limits. > > > > > */ > > > > > uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat); > > > > > + cfg->size.boundTo(resolution); > > > > > + > > > > > V4L2SubdeviceFormat sensorFormat = > > > > > sensor->getFormat({ mbusCode }, cfg->size); > > > > CameraSensor::getFormat() returns a sensor resolution large enough to > > > > accomodate the requested size, at least that's how I read the > > > > following part of the documentation > > > > > > > > * \a size indicates the desired size at the output of the sensor. This function > > > > * selects the best media bus code and size supported by the sensor according > > > > * to the following criteria. > > > > * > > > > * - The desired \a size shall fit in the sensor output size to avoid the need > > > > * to up-scale. > > > > > > > > And this part of the code > > > > > > > > if (sz.width < size.width || sz.height < size.height) > > > > continue; > > > > > > > > So, at least in my understanding "sensorFormat" could represent a size > > > > larger than cfg->size. Is this your understanding as well ? > > > "sensorFormat" could represent a size larger than cfg->size > > > > > > and > > > > > > could also represent a size large than max supported resolution (i.e. > > > 'resolution' variable in this case) > > > > > > For e.g. in IMX283 case it would, 5472x3648 > > > > > > > In the lines below this function we have > > > > > > > > minResolution = sensorFormat.size; > > > > maxResolution = sensorFormat.size; > > > From the above e.g. of IMX283 minResolution = maxResolution = 5472x3648 > > > > cfg->size.boundTo(maxResolution); > > > > cfg->size.expandTo(minResolution); > > > > > > > > So if ((maxResolution = minResolution) > cfg->size) will the calls to > > > > boundTo() followed by expandTo() enlarge cfg->size ? > > > so cfg->size is 5472x3648 here, which can't be supported. > > Sorry, I'm confused. Aren't you confirming me that the resulting > > cfg->size cannot be supported (with this patch I mean) ? > > So user passes a relatively high resolution, supported by the sensor (but > not with ISP) > > cam -c1 -srole=raw,width=5472,height=3648 > > so cfg-size here is 5472x3648. > > So without this patch, you would right now get: > > ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12 > Failed to configure camera > Failed to start camera session > > With this patch patched, you would get: > > INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12 > cam0: Capture until user interrupts by SIGINT > 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824 > .... > > So What has happened: > > User has requested a raw stream with 5472x3648. It was found that this can't > be supported hence, > the camera configuration is adjusted to a (lower) but maximum supported > resolution and delivers raw > frames with 4096x3072. I understand this fixes your use case but my concern, as explained in he previous email is that cfg->size.boundTo(resolution); V4L2SubdeviceFormat sensorFormat = sensor->getFormat({ mbusCode }, cfg->size); minResolution = sensorFormat.size; maxResolution = sensorFormat.size; cfg->size.boundTo(maxResolution); cfg->size.expandTo(minResolution); as CameraSensor::getFormat() can return a resolution -higher- than the requested one (cfg->size) there is a risk that cfg->size is later expanded to a larger value. Now, if I get this right "resolution" is returned by the newly introduced RkISP1Path::filterSensorResolution() and it's guaranteed to be the "higher sensor's resolution supported by the ISP". if that's the case, then CameraSensor::getFormat() will never return a size larger than "cfg->size.boundedTo(resolution)" as "resolution" is known to be a size supported by the sensor itself. If my understanding is correct, then please add my tag Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > One can argue that we should never adjust camera configuration to a lower > size than cfg->size (i.e. what the user asked). > I don't think that's a problem > But then, > > * \var CameraConfiguration::Adjusted > * The configuration has been adjusted to a valid configuration > > and > > * \retval CameraConfigutation::Adjusted The configuration has been adjusted > * and is now valid. Parameters may have changed for any stream, and stream > * configurations may have been removed. The caller shall check the > * configuration carefully. > > > > > > > Have you noticed the failure case I mentioned in the commit message? > > While the commmit message suggests this patch makes it valid ? > > > > What am I missing ? :) > > > > > > Wouldn't it be better to use 'resolution' in > > > > > > > > V4L2SubdeviceFormat sensorFormat = > > > > sensor->getFormat({ mbusCode }, cfg->size); > > > > > > > > instead of cfg->size ? > > > No, otherwise it will always pick the highest sensor output size. > > > > > > For e.g. I request -srole=raw,width=1920,height=1080 > > > > > > "cfg->size" will pick 2736x1824 for IMX283 > > > > > > "resolution" will always pick 4096x3072. > > > > Thanks > > > > j > > > > > > > > > -- > > > > > 2.45.2 > > > > > >
Hi Jacopo, On 08/10/24 1:18 pm, Jacopo Mondi wrote: > Umang, > seems like I'm failing to make my point across, sorry about this > > On Fri, Oct 04, 2024 at 04:45:01PM GMT, Umang Jain wrote: >> Hi Jacopo, >> >> On 04/10/24 3:58 pm, Jacopo Mondi wrote: >>> Hi Umang >>> >>> On Fri, Oct 04, 2024 at 03:17:34PM GMT, Umang Jain wrote: >>>> Hi Jacopo >>>> >>>> On 04/10/24 3:07 pm, Jacopo Mondi wrote: >>>>> Hi Umang >>>>> >>>>> On Fri, Oct 04, 2024 at 10:23:38AM GMT, Umang Jain wrote: >>>>>> Commit 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not >>>>>> supported by the pipeline") introduced a mechanism to determine maximum >>>>>> supported sensor resolution and filter out resolutions that cannot be >>>>>> supported by the ISP. >>>>>> >>>>>> However, it missed to update the raw stream configuration path, where >>>>>> it should have clamped the raw stream configuration size to the maximum >>>>>> sensor supported resolution. >>>>>> >>>>>> This patch fixes the above issue and can be confirmed with IMX283 >>>>>> on i.MX8MP: >>>>>> >>>>>> From: >>>>>> ($) cam -c1 -srole=raw,width=5472,height=3072 >>>>>> INFO Camera camera.cpp:1197 configuring streams: (0) 5472x3648-SRGGB12 >>>>>> ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12 >>>>>> Failed to configure camera >>>>>> Failed to start camera session >>>>>> >>>>>> To: >>>>>> ($) cam -c1 -srole=raw,width=5472,height=3072 >>>>>> INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12 >>>>>> cam0: Capture until user interrupts by SIGINT >>>>>> 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824 >>>>>> 536.182378 (10.00 fps) cam0-stream0 seq: 000001 bytesused: 25165824 >>>>>> 536.282375 (10.00 fps) cam0-stream0 seq: 000002 bytesused: 25165824 >>>>>> ... >>>>>> >>>>>> Fixes: 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline") >>>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>>>>> --- >>>>>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++- >>>>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >>>>>> index da8d25c3..feb6d89f 100644 >>>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >>>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >>>>>> @@ -316,9 +316,11 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor, >>>>>> if (isRaw) { >>>>>> /* >>>>>> * Use the sensor output size closest to the requested stream >>>>>> - * size. >>>>>> + * size while ensuring the output size doesn't exceed ISP limits. >>>>>> */ >>>>>> uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat); >>>>>> + cfg->size.boundTo(resolution); >>>>>> + >>>>>> V4L2SubdeviceFormat sensorFormat = >>>>>> sensor->getFormat({ mbusCode }, cfg->size); >>>>> CameraSensor::getFormat() returns a sensor resolution large enough to >>>>> accomodate the requested size, at least that's how I read the >>>>> following part of the documentation >>>>> >>>>> * \a size indicates the desired size at the output of the sensor. This function >>>>> * selects the best media bus code and size supported by the sensor according >>>>> * to the following criteria. >>>>> * >>>>> * - The desired \a size shall fit in the sensor output size to avoid the need >>>>> * to up-scale. >>>>> >>>>> And this part of the code >>>>> >>>>> if (sz.width < size.width || sz.height < size.height) >>>>> continue; >>>>> >>>>> So, at least in my understanding "sensorFormat" could represent a size >>>>> larger than cfg->size. Is this your understanding as well ? >>>> "sensorFormat" could represent a size larger than cfg->size >>>> >>>> and >>>> >>>> could also represent a size large than max supported resolution (i.e. >>>> 'resolution' variable in this case) >>>> >>>> For e.g. in IMX283 case it would, 5472x3648 >>>> >>>>> In the lines below this function we have >>>>> >>>>> minResolution = sensorFormat.size; >>>>> maxResolution = sensorFormat.size; >>>> From the above e.g. of IMX283 minResolution = maxResolution = 5472x3648 >>>>> cfg->size.boundTo(maxResolution); >>>>> cfg->size.expandTo(minResolution); >>>>> >>>>> So if ((maxResolution = minResolution) > cfg->size) will the calls to >>>>> boundTo() followed by expandTo() enlarge cfg->size ? >>>> so cfg->size is 5472x3648 here, which can't be supported. >>> Sorry, I'm confused. Aren't you confirming me that the resulting >>> cfg->size cannot be supported (with this patch I mean) ? >> So user passes a relatively high resolution, supported by the sensor (but >> not with ISP) >> >> cam -c1 -srole=raw,width=5472,height=3648 >> >> so cfg-size here is 5472x3648. >> >> So without this patch, you would right now get: >> >> ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12 >> Failed to configure camera >> Failed to start camera session >> >> With this patch patched, you would get: >> >> INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12 >> cam0: Capture until user interrupts by SIGINT >> 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824 >> .... >> >> So What has happened: >> >> User has requested a raw stream with 5472x3648. It was found that this can't >> be supported hence, >> the camera configuration is adjusted to a (lower) but maximum supported >> resolution and delivers raw >> frames with 4096x3072. > I understand this fixes your use case but my concern, as explained in > he previous email is that > > cfg->size.boundTo(resolution); > > V4L2SubdeviceFormat sensorFormat = > sensor->getFormat({ mbusCode }, cfg->size); > > minResolution = sensorFormat.size; > maxResolution = sensorFormat.size; > > > cfg->size.boundTo(maxResolution); > cfg->size.expandTo(minResolution); > > as CameraSensor::getFormat() can return a resolution -higher- than the > requested one (cfg->size) there is a risk that cfg->size is later > expanded to a larger value. > > Now, if I get this right "resolution" is returned by the newly introduced > RkISP1Path::filterSensorResolution() and it's guaranteed to be the > "higher sensor's resolution supported by the ISP". yes, resolution is is the 'highest sensor's resolution supported by the ISP' hence, we never allow cfg->size to go beyond that. > > if that's the case, then CameraSensor::getFormat() will never return > a size larger than "cfg->size.boundedTo(resolution)" as "resolution" > is known to be a size supported by the sensor itself. Indeed, that's what's the intent of the patch. > > If my understanding is correct, then please add my tag > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> You're understanding is correct. Do you think this understand needs to be documented in form of comments ? > >> One can argue that we should never adjust camera configuration to a lower >> size than cfg->size (i.e. what the user asked). >> > I don't think that's a problem > >> But then, >> >> * \var CameraConfiguration::Adjusted >> * The configuration has been adjusted to a valid configuration >> >> and >> >> * \retval CameraConfigutation::Adjusted The configuration has been adjusted >> * and is now valid. Parameters may have changed for any stream, and stream >> * configurations may have been removed. The caller shall check the >> * configuration carefully. >> >> >>>> Have you noticed the failure case I mentioned in the commit message? >>> While the commmit message suggests this patch makes it valid ? >>> >>> What am I missing ? :) >>> >>>>> Wouldn't it be better to use 'resolution' in >>>>> >>>>> V4L2SubdeviceFormat sensorFormat = >>>>> sensor->getFormat({ mbusCode }, cfg->size); >>>>> >>>>> instead of cfg->size ? >>>> No, otherwise it will always pick the highest sensor output size. >>>> >>>> For e.g. I request -srole=raw,width=1920,height=1080 >>>> >>>> "cfg->size" will pick 2736x1824 for IMX283 >>>> >>>> "resolution" will always pick 4096x3072. >>>>> Thanks >>>>> j >>>>> >>>>>> -- >>>>>> 2.45.2 >>>>>>
Hi Umang On Wed, Oct 09, 2024 at 12:10:37AM GMT, Umang Jain wrote: > Hi Jacopo, > > On 08/10/24 1:18 pm, Jacopo Mondi wrote: > > Umang, > > seems like I'm failing to make my point across, sorry about this > > > > On Fri, Oct 04, 2024 at 04:45:01PM GMT, Umang Jain wrote: > > > Hi Jacopo, > > > > > > On 04/10/24 3:58 pm, Jacopo Mondi wrote: > > > > Hi Umang > > > > > > > > On Fri, Oct 04, 2024 at 03:17:34PM GMT, Umang Jain wrote: > > > > > Hi Jacopo > > > > > > > > > > On 04/10/24 3:07 pm, Jacopo Mondi wrote: > > > > > > Hi Umang > > > > > > > > > > > > On Fri, Oct 04, 2024 at 10:23:38AM GMT, Umang Jain wrote: > > > > > > > Commit 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not > > > > > > > supported by the pipeline") introduced a mechanism to determine maximum > > > > > > > supported sensor resolution and filter out resolutions that cannot be > > > > > > > supported by the ISP. > > > > > > > > > > > > > > However, it missed to update the raw stream configuration path, where > > > > > > > it should have clamped the raw stream configuration size to the maximum > > > > > > > sensor supported resolution. > > > > > > > > > > > > > > This patch fixes the above issue and can be confirmed with IMX283 > > > > > > > on i.MX8MP: > > > > > > > > > > > > > > From: > > > > > > > ($) cam -c1 -srole=raw,width=5472,height=3072 > > > > > > > INFO Camera camera.cpp:1197 configuring streams: (0) 5472x3648-SRGGB12 > > > > > > > ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12 > > > > > > > Failed to configure camera > > > > > > > Failed to start camera session > > > > > > > > > > > > > > To: > > > > > > > ($) cam -c1 -srole=raw,width=5472,height=3072 > > > > > > > INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12 > > > > > > > cam0: Capture until user interrupts by SIGINT > > > > > > > 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824 > > > > > > > 536.182378 (10.00 fps) cam0-stream0 seq: 000001 bytesused: 25165824 > > > > > > > 536.282375 (10.00 fps) cam0-stream0 seq: 000002 bytesused: 25165824 > > > > > > > ... > > > > > > > > > > > > > > Fixes: 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline") > > > > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > > > > > --- > > > > > > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++- > > > > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > > > > > index da8d25c3..feb6d89f 100644 > > > > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > > > > > @@ -316,9 +316,11 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor, > > > > > > > if (isRaw) { > > > > > > > /* > > > > > > > * Use the sensor output size closest to the requested stream > > > > > > > - * size. > > > > > > > + * size while ensuring the output size doesn't exceed ISP limits. > > > > > > > */ > > > > > > > uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat); > > > > > > > + cfg->size.boundTo(resolution); > > > > > > > + > > > > > > > V4L2SubdeviceFormat sensorFormat = > > > > > > > sensor->getFormat({ mbusCode }, cfg->size); > > > > > > CameraSensor::getFormat() returns a sensor resolution large enough to > > > > > > accomodate the requested size, at least that's how I read the > > > > > > following part of the documentation > > > > > > > > > > > > * \a size indicates the desired size at the output of the sensor. This function > > > > > > * selects the best media bus code and size supported by the sensor according > > > > > > * to the following criteria. > > > > > > * > > > > > > * - The desired \a size shall fit in the sensor output size to avoid the need > > > > > > * to up-scale. > > > > > > > > > > > > And this part of the code > > > > > > > > > > > > if (sz.width < size.width || sz.height < size.height) > > > > > > continue; > > > > > > > > > > > > So, at least in my understanding "sensorFormat" could represent a size > > > > > > larger than cfg->size. Is this your understanding as well ? > > > > > "sensorFormat" could represent a size larger than cfg->size > > > > > > > > > > and > > > > > > > > > > could also represent a size large than max supported resolution (i.e. > > > > > 'resolution' variable in this case) > > > > > > > > > > For e.g. in IMX283 case it would, 5472x3648 > > > > > > > > > > > In the lines below this function we have > > > > > > > > > > > > minResolution = sensorFormat.size; > > > > > > maxResolution = sensorFormat.size; > > > > > From the above e.g. of IMX283 minResolution = maxResolution = 5472x3648 > > > > > > cfg->size.boundTo(maxResolution); > > > > > > cfg->size.expandTo(minResolution); > > > > > > > > > > > > So if ((maxResolution = minResolution) > cfg->size) will the calls to > > > > > > boundTo() followed by expandTo() enlarge cfg->size ? > > > > > so cfg->size is 5472x3648 here, which can't be supported. > > > > Sorry, I'm confused. Aren't you confirming me that the resulting > > > > cfg->size cannot be supported (with this patch I mean) ? > > > So user passes a relatively high resolution, supported by the sensor (but > > > not with ISP) > > > > > > cam -c1 -srole=raw,width=5472,height=3648 > > > > > > so cfg-size here is 5472x3648. > > > > > > So without this patch, you would right now get: > > > > > > ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12 > > > Failed to configure camera > > > Failed to start camera session > > > > > > With this patch patched, you would get: > > > > > > INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12 > > > cam0: Capture until user interrupts by SIGINT > > > 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824 > > > .... > > > > > > So What has happened: > > > > > > User has requested a raw stream with 5472x3648. It was found that this can't > > > be supported hence, > > > the camera configuration is adjusted to a (lower) but maximum supported > > > resolution and delivers raw > > > frames with 4096x3072. > > I understand this fixes your use case but my concern, as explained in > > he previous email is that > > > > cfg->size.boundTo(resolution); > > > > V4L2SubdeviceFormat sensorFormat = > > sensor->getFormat({ mbusCode }, cfg->size); > > > > minResolution = sensorFormat.size; > > maxResolution = sensorFormat.size; > > > > > > cfg->size.boundTo(maxResolution); > > cfg->size.expandTo(minResolution); > > > > as CameraSensor::getFormat() can return a resolution -higher- than the > > requested one (cfg->size) there is a risk that cfg->size is later > > expanded to a larger value. > > > > Now, if I get this right "resolution" is returned by the newly introduced > > RkISP1Path::filterSensorResolution() and it's guaranteed to be the > > "higher sensor's resolution supported by the ISP". > > yes, resolution is is the 'highest sensor's resolution supported by the > ISP' hence, we never allow cfg->size to go beyond that. > > > > if that's the case, then CameraSensor::getFormat() will never return > > a size larger than "cfg->size.boundedTo(resolution)" as "resolution" > > is known to be a size supported by the sensor itself. > > Indeed, that's what's the intent of the patch. > > > > > If my understanding is correct, then please add my tag > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > You're understanding is correct. > > Do you think this understand needs to be documented in form of comments ? > Maybe yes.. Just a small note along the lines of - * size. + * size while ensuring the output size doesn't exceed ISP limits. + * + * As 'resolution' is the largest sensor resolution + * supported by the ISP, CameraSensor::getFormat() will never + * return a V4L2SubdeviceFormat with a larger size. */ Or whatever you like the most. Thanks j > > > > > One can argue that we should never adjust camera configuration to a lower > > > size than cfg->size (i.e. what the user asked). > > > > > I don't think that's a problem > > > > > But then, > > > > > > * \var CameraConfiguration::Adjusted > > > * The configuration has been adjusted to a valid configuration > > > > > > and > > > > > > * \retval CameraConfigutation::Adjusted The configuration has been adjusted > > > * and is now valid. Parameters may have changed for any stream, and stream > > > * configurations may have been removed. The caller shall check the > > > * configuration carefully. > > > > > > > > > > > Have you noticed the failure case I mentioned in the commit message? > > > > While the commmit message suggests this patch makes it valid ? > > > > > > > > What am I missing ? :) > > > > > > > > > > Wouldn't it be better to use 'resolution' in > > > > > > > > > > > > V4L2SubdeviceFormat sensorFormat = > > > > > > sensor->getFormat({ mbusCode }, cfg->size); > > > > > > > > > > > > instead of cfg->size ? > > > > > No, otherwise it will always pick the highest sensor output size. > > > > > > > > > > For e.g. I request -srole=raw,width=1920,height=1080 > > > > > > > > > > "cfg->size" will pick 2736x1824 for IMX283 > > > > > > > > > > "resolution" will always pick 4096x3072. > > > > > > Thanks > > > > > > j > > > > > > > > > > > > > -- > > > > > > > 2.45.2 > > > > > > > >
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp index da8d25c3..feb6d89f 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp @@ -316,9 +316,11 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor, if (isRaw) { /* * Use the sensor output size closest to the requested stream - * size. + * size while ensuring the output size doesn't exceed ISP limits. */ uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat); + cfg->size.boundTo(resolution); + V4L2SubdeviceFormat sensorFormat = sensor->getFormat({ mbusCode }, cfg->size);
Commit 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline") introduced a mechanism to determine maximum supported sensor resolution and filter out resolutions that cannot be supported by the ISP. However, it missed to update the raw stream configuration path, where it should have clamped the raw stream configuration size to the maximum sensor supported resolution. This patch fixes the above issue and can be confirmed with IMX283 on i.MX8MP: From: ($) cam -c1 -srole=raw,width=5472,height=3072 INFO Camera camera.cpp:1197 configuring streams: (0) 5472x3648-SRGGB12 ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12 Failed to configure camera Failed to start camera session To: ($) cam -c1 -srole=raw,width=5472,height=3072 INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12 cam0: Capture until user interrupts by SIGINT 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824 536.182378 (10.00 fps) cam0-stream0 seq: 000001 bytesused: 25165824 536.282375 (10.00 fps) cam0-stream0 seq: 000002 bytesused: 25165824 ... Fixes: 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline") Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)