| Message ID | 20201120124503.22718-2-sebastian.fricke.linux@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
Hi Sebastian, Thank you for your patch. On 11/20/20 9:45 AM, Sebastian Fricke wrote: > Make sure to use a sensor format resolution that is smaller or equal to > the maximum allowed resolution for the RkISP1. The maximum resolution > is defined within the `rkisp1-common.h` file as: > define RKISP1_ISP_MAX_WIDTH 4032 > define RKISP1_ISP_MAX_HEIGHT 3024 > > Change the order of setting the formats, in order to first check if the > requested resolution exceeds the maximum and search for the next smaller > available sensor resolution if that is the case. > Fail if no viable sensor format was located. > > This means that some camera sensors can never operate with their maximum > resolution, for example on my OV13850 camera sensor, there are two > possible resolutions: 4224x3136 & 2112x1568, the first of those two will > never be picked as it surpasses the maximum of the ISP. > > Signed-off-by: Sebastian Fricke <sebastian.fricke.linux@gmail.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 46 ++++++++++++++++++++---- > 1 file changed, 40 insertions(+), 6 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 1b1922a..3ef8acd 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -677,22 +677,56 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > return ret; > > /* > - * Configure the format on the sensor output and propagate it through > - * the pipeline. > + * Configure the format at the ISP input and pass it on through > + * the pipeline after checking that the maximum resolution allowed > + * for the ISP is not exceeded. > */ > V4L2SubdeviceFormat format = config->sensorFormat(); > - LOG(RkISP1, Debug) << "Configuring sensor with " << format.toString(); > + LOG(RkISP1, Debug) << "Configuring ISP with " << format.toString(); > + /* > + * format is changed in setFormat, keep the resolution for comparison > + */ > + Size originalFormatSize = format.size; > > - ret = sensor->setFormat(&format); > + ret = isp_->setFormat(0, &format); > if (ret < 0) > return ret; > + LOG(RkISP1, Debug) << "ISP configured with " << format.toString(); Could you please make it clear it is the input pad? LOG(RkISP1, Debug) << "ISP input pad configured with " << format.toString(); Actually, I have another suggestion to include the crop rectangle and be less confusing, please check my comments at the end. > + > + if (originalFormatSize != format.size) { > + Size maxSize = Size(0, 0); It seems you don't need this assignment, the default constructor already sets it to zero. It is enough to do: Size maxSize This is how I see CameraConfiguration::Status RkISP1CameraConfiguration::validate() doing it. > + LOG(RkISP1, Info) << "Configured resolution is greater than " > + "the maximum resolution for the ISP, " > + "trying to re-configure to a smaller " > + "valid sensor format"; I think "Configured resolution" is confusing, since I'm not sure where this was configured (since this refers to the sensor and not the ISP, and we didn't print the sensor resolution before). I would change this message to something like: "Sensor format (%dx%d) is not supported by the ISP (%dx%d), trying to re-configure to a smaller valid sensor format" What do you think? > + > + for (const Size &size : sensor->sizes()) { > + if (size.width > format.size.width || > + size.height > format.size.height) > + continue; > + maxSize = std::max(maxSize, size); > + } > + if (maxSize == Size(0, 0)) { > + LOG(RkISP1, Error) << "No available sensor resolution" > + "that is smaller or equal to " > + << format.toString(); > + return -1; > + } > + format = sensor->getFormat(sensor->mbusCodes(), maxSize); > > - LOG(RkISP1, Debug) << "Sensor configured with " << format.toString(); > + ret = isp_->setFormat(0, &format); > + if (ret < 0) > + return ret; > + LOG(RkISP1, Debug) << "ISP re-configured with " > + << format.toString(); Make if clear it is the input pad. > + } > > - ret = isp_->setFormat(0, &format); > + ret = sensor->setFormat(&format); I may be wrong, but it seems this sensor->setFormat() can be moved to inside the if statement above, since we only need to set it if it is different from the original config->sensorFormat() > if (ret < 0) > return ret; > > + LOG(RkISP1, Debug) << "Sensor configured with " << format.toString(); > + > Rectangle rect(0, 0, format.size); > ret = isp_->setSelection(0, V4L2_SEL_TGT_CROP, &rect); > if (ret < 0) > There is a message after this line that prints the ISP format, it should be deleted or moved or updated, but then it will conflict with this patch that wasn't merged yet: https://lists.libcamera.org/pipermail/libcamera-devel/2020-November/014866.html Which adds a print to the crop configuration. Could you rebase your change on top of this patch? I think it would make things easier. Maybe instead of printing "ISP configured with" and "ISP re-configured with ", you can print "Configuring ISP input pad with " and "Re-configuring ISP input pad with " And then, after the negotiation: LOG(RkISP1, Debug) << "ISP input pad configured with " << format.toString() << " crop " << rect.toString(); The isp_->setSelection() could be moved just before sensor->setFormat() to finish configuring the isp, keeping the order of things. Thanks, Helen
On 20.11.2020 10:40, Helen Koike wrote: >Hi Sebastian, > >Thank you for your patch. Hey Helen, I have been working on an alternative way to handle the format matching, I have just sent a patch to the media mailing list, which adds the enum_frame_size ioctl to the RkISP1. My thought is, that this will make this whole patch a bit cleaner. Depending on the feedback, I will maybe put this version on hold and instead work on the version that enumerates the maximum frame size of the ISP before deciding the frame resolution for the sensor. Additionally, I left some comments to your feedback. I am happy to hear your thoughts on this! > >On 11/20/20 9:45 AM, Sebastian Fricke wrote: >> Make sure to use a sensor format resolution that is smaller or equal to >> the maximum allowed resolution for the RkISP1. The maximum resolution >> is defined within the `rkisp1-common.h` file as: >> define RKISP1_ISP_MAX_WIDTH 4032 >> define RKISP1_ISP_MAX_HEIGHT 3024 >> >> Change the order of setting the formats, in order to first check if the >> requested resolution exceeds the maximum and search for the next smaller >> available sensor resolution if that is the case. >> Fail if no viable sensor format was located. >> >> This means that some camera sensors can never operate with their maximum >> resolution, for example on my OV13850 camera sensor, there are two >> possible resolutions: 4224x3136 & 2112x1568, the first of those two will >> never be picked as it surpasses the maximum of the ISP. >> >> Signed-off-by: Sebastian Fricke <sebastian.fricke.linux@gmail.com> >> --- >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 46 ++++++++++++++++++++---- >> 1 file changed, 40 insertions(+), 6 deletions(-) >> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> index 1b1922a..3ef8acd 100644 >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> @@ -677,22 +677,56 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) >> return ret; >> >> /* >> - * Configure the format on the sensor output and propagate it through >> - * the pipeline. >> + * Configure the format at the ISP input and pass it on through >> + * the pipeline after checking that the maximum resolution allowed >> + * for the ISP is not exceeded. >> */ >> V4L2SubdeviceFormat format = config->sensorFormat(); > >> - LOG(RkISP1, Debug) << "Configuring sensor with " << format.toString(); >> + LOG(RkISP1, Debug) << "Configuring ISP with " << format.toString(); >> + /* >> + * format is changed in setFormat, keep the resolution for comparison >> + */ >> + Size originalFormatSize = format.size; >> >> - ret = sensor->setFormat(&format); >> + ret = isp_->setFormat(0, &format); >> if (ret < 0) >> return ret; >> + LOG(RkISP1, Debug) << "ISP configured with " << format.toString(); > >Could you please make it clear it is the input pad? > > LOG(RkISP1, Debug) << "ISP input pad configured with " << format.toString(); > >Actually, I have another suggestion to include the crop rectangle and be less confusing, >please check my comments at the end. > > >> + >> + if (originalFormatSize != format.size) { >> + Size maxSize = Size(0, 0); > >It seems you don't need this assignment, the default constructor already sets it >to zero. >It is enough to do: > > Size maxSize > >This is how I see CameraConfiguration::Status RkISP1CameraConfiguration::validate() >doing it. > >> + LOG(RkISP1, Info) << "Configured resolution is greater than " >> + "the maximum resolution for the ISP, " >> + "trying to re-configure to a smaller " >> + "valid sensor format"; > >I think "Configured resolution" is confusing, since I'm not sure where >this was configured (since this refers to the sensor and not the ISP, and we didn't >print the sensor resolution before). > >I would change this message to something like: > >"Sensor format (%dx%d) is not supported by the ISP (%dx%d), trying to re-configure >to a smaller valid sensor format" > >What do you think? Yes that sounds way better. > >> + >> + for (const Size &size : sensor->sizes()) { >> + if (size.width > format.size.width || >> + size.height > format.size.height) >> + continue; >> + maxSize = std::max(maxSize, size); >> + } >> + if (maxSize == Size(0, 0)) { >> + LOG(RkISP1, Error) << "No available sensor resolution" >> + "that is smaller or equal to " >> + << format.toString(); >> + return -1; >> + } >> + format = sensor->getFormat(sensor->mbusCodes(), maxSize); >> >> - LOG(RkISP1, Debug) << "Sensor configured with " << format.toString(); >> + ret = isp_->setFormat(0, &format); >> + if (ret < 0) >> + return ret; >> + LOG(RkISP1, Debug) << "ISP re-configured with " >> + << format.toString(); > >Make if clear it is the input pad. > >> + } >> >> - ret = isp_->setFormat(0, &format); >> + ret = sensor->setFormat(&format); > >I may be wrong, but it seems this sensor->setFormat() can be moved to >inside the if statement above, since we only need to set it if it is >different from the original config->sensorFormat() > >> if (ret < 0) >> return ret; >> >> + LOG(RkISP1, Debug) << "Sensor configured with " << format.toString(); >> + >> Rectangle rect(0, 0, format.size); >> ret = isp_->setSelection(0, V4L2_SEL_TGT_CROP, &rect); >> if (ret < 0) >> > >There is a message after this line that prints the ISP format, it should be deleted >or moved or updated, but then it will conflict with this patch that wasn't merged yet: > > https://lists.libcamera.org/pipermail/libcamera-devel/2020-November/014866.html > >Which adds a print to the crop configuration. Could you rebase your change on top >of this patch? I think it would make things easier. I see the patch is now merged, so the next version will based on it. > >Maybe instead of printing "ISP configured with" and "ISP re-configured with ", you can >print "Configuring ISP input pad with " and "Re-configuring ISP input pad with " > >And then, after the negotiation: > > LOG(RkISP1, Debug) > << "ISP input pad configured with " << format.toString() > << " crop " << rect.toString(); > >The isp_->setSelection() could be moved just before sensor->setFormat() to finish >configuring the isp, keeping the order of things. > >Thanks, >Helen Thank you for your feedback. Sebastian
Hi Sebastian, On 12/5/20 3:32 PM, Sebastian Fricke wrote: > On 20.11.2020 10:40, Helen Koike wrote: >> Hi Sebastian, >> >> Thank you for your patch. > > Hey Helen, > > I have been working on an alternative way to handle the format matching, > I have just sent a patch to the media mailing list, which adds the > enum_frame_size ioctl to the RkISP1. My thought is, that this will make > this whole patch a bit cleaner.> Depending on the feedback, I will maybe put this version on hold and > instead work on the version that enumerates the maximum frame size of > the ISP before deciding the frame resolution for the sensor. Sure, feel free to send the code to make it easier for us to see how you plan to use the enum, you don't need to wait for the enum patch to be accepted to request comments regarding this approach on the libcamera side. You can submit the patch and write in the comment section (after the 3 dashes) that it depends on the enum kernel patch. Regards, Helen > > Additionally, I left some comments to your feedback. > I am happy to hear your thoughts on this! > >> >> On 11/20/20 9:45 AM, Sebastian Fricke wrote: >>> Make sure to use a sensor format resolution that is smaller or equal to >>> the maximum allowed resolution for the RkISP1. The maximum resolution >>> is defined within the `rkisp1-common.h` file as: >>> define RKISP1_ISP_MAX_WIDTH 4032 >>> define RKISP1_ISP_MAX_HEIGHT 3024 >>> >>> Change the order of setting the formats, in order to first check if the >>> requested resolution exceeds the maximum and search for the next smaller >>> available sensor resolution if that is the case. >>> Fail if no viable sensor format was located. >>> >>> This means that some camera sensors can never operate with their maximum >>> resolution, for example on my OV13850 camera sensor, there are two >>> possible resolutions: 4224x3136 & 2112x1568, the first of those two will >>> never be picked as it surpasses the maximum of the ISP. >>> >>> Signed-off-by: Sebastian Fricke <sebastian.fricke.linux@gmail.com> >>> --- >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 46 ++++++++++++++++++++---- >>> 1 file changed, 40 insertions(+), 6 deletions(-) >>> >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >>> index 1b1922a..3ef8acd 100644 >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >>> @@ -677,22 +677,56 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) >>> return ret; >>> >>> /* >>> - * Configure the format on the sensor output and propagate it through >>> - * the pipeline. >>> + * Configure the format at the ISP input and pass it on through >>> + * the pipeline after checking that the maximum resolution allowed >>> + * for the ISP is not exceeded. >>> */ >>> V4L2SubdeviceFormat format = config->sensorFormat(); >> >>> - LOG(RkISP1, Debug) << "Configuring sensor with " << format.toString(); >>> + LOG(RkISP1, Debug) << "Configuring ISP with " << format.toString(); >>> + /* >>> + * format is changed in setFormat, keep the resolution for comparison >>> + */ >>> + Size originalFormatSize = format.size; >>> >>> - ret = sensor->setFormat(&format); >>> + ret = isp_->setFormat(0, &format); >>> if (ret < 0) >>> return ret; >>> + LOG(RkISP1, Debug) << "ISP configured with " << format.toString(); >> >> Could you please make it clear it is the input pad? >> >> LOG(RkISP1, Debug) << "ISP input pad configured with " << format.toString(); >> >> Actually, I have another suggestion to include the crop rectangle and be less confusing, >> please check my comments at the end. >> >> >>> + >>> + if (originalFormatSize != format.size) { >>> + Size maxSize = Size(0, 0); >> >> It seems you don't need this assignment, the default constructor already sets it >> to zero. >> It is enough to do: >> >> Size maxSize >> >> This is how I see CameraConfiguration::Status RkISP1CameraConfiguration::validate() >> doing it. >> >>> + LOG(RkISP1, Info) << "Configured resolution is greater than " >>> + "the maximum resolution for the ISP, " >>> + "trying to re-configure to a smaller " >>> + "valid sensor format"; >> >> I think "Configured resolution" is confusing, since I'm not sure where >> this was configured (since this refers to the sensor and not the ISP, and we didn't >> print the sensor resolution before). >> >> I would change this message to something like: >> >> "Sensor format (%dx%d) is not supported by the ISP (%dx%d), trying to re-configure >> to a smaller valid sensor format" >> >> What do you think? > > Yes that sounds way better. > >> >>> + >>> + for (const Size &size : sensor->sizes()) { >>> + if (size.width > format.size.width || >>> + size.height > format.size.height) >>> + continue; >>> + maxSize = std::max(maxSize, size); >>> + } >>> + if (maxSize == Size(0, 0)) { >>> + LOG(RkISP1, Error) << "No available sensor resolution" >>> + "that is smaller or equal to " >>> + << format.toString(); >>> + return -1; >>> + } >>> + format = sensor->getFormat(sensor->mbusCodes(), maxSize); >>> >>> - LOG(RkISP1, Debug) << "Sensor configured with " << format.toString(); >>> + ret = isp_->setFormat(0, &format); >>> + if (ret < 0) >>> + return ret; >>> + LOG(RkISP1, Debug) << "ISP re-configured with " >>> + << format.toString(); >> >> Make if clear it is the input pad. >> >>> + } >>> >>> - ret = isp_->setFormat(0, &format); >>> + ret = sensor->setFormat(&format); >> >> I may be wrong, but it seems this sensor->setFormat() can be moved to >> inside the if statement above, since we only need to set it if it is >> different from the original config->sensorFormat() >> >>> if (ret < 0) >>> return ret; >>> >>> + LOG(RkISP1, Debug) << "Sensor configured with " << format.toString(); >>> + >>> Rectangle rect(0, 0, format.size); >>> ret = isp_->setSelection(0, V4L2_SEL_TGT_CROP, &rect); >>> if (ret < 0) >>> >> >> There is a message after this line that prints the ISP format, it should be deleted >> or moved or updated, but then it will conflict with this patch that wasn't merged yet: >> >> https://lists.libcamera.org/pipermail/libcamera-devel/2020-November/014866.html >> >> Which adds a print to the crop configuration. Could you rebase your change on top >> of this patch? I think it would make things easier. > > I see the patch is now merged, so the next version will based on it. > >> >> Maybe instead of printing "ISP configured with" and "ISP re-configured with ", you can >> print "Configuring ISP input pad with " and "Re-configuring ISP input pad with " >> >> And then, after the negotiation: >> >> LOG(RkISP1, Debug) >> << "ISP input pad configured with " << format.toString() >> << " crop " << rect.toString(); >> >> The isp_->setSelection() could be moved just before sensor->setFormat() to finish >> configuring the isp, keeping the order of things. >> >> Thanks, >> Helen > > Thank you for your feedback. > Sebastian
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 1b1922a..3ef8acd 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -677,22 +677,56 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) return ret; /* - * Configure the format on the sensor output and propagate it through - * the pipeline. + * Configure the format at the ISP input and pass it on through + * the pipeline after checking that the maximum resolution allowed + * for the ISP is not exceeded. */ V4L2SubdeviceFormat format = config->sensorFormat(); - LOG(RkISP1, Debug) << "Configuring sensor with " << format.toString(); + LOG(RkISP1, Debug) << "Configuring ISP with " << format.toString(); + /* + * format is changed in setFormat, keep the resolution for comparison + */ + Size originalFormatSize = format.size; - ret = sensor->setFormat(&format); + ret = isp_->setFormat(0, &format); if (ret < 0) return ret; + LOG(RkISP1, Debug) << "ISP configured with " << format.toString(); + + if (originalFormatSize != format.size) { + Size maxSize = Size(0, 0); + LOG(RkISP1, Info) << "Configured resolution is greater than " + "the maximum resolution for the ISP, " + "trying to re-configure to a smaller " + "valid sensor format"; + + for (const Size &size : sensor->sizes()) { + if (size.width > format.size.width || + size.height > format.size.height) + continue; + maxSize = std::max(maxSize, size); + } + if (maxSize == Size(0, 0)) { + LOG(RkISP1, Error) << "No available sensor resolution" + "that is smaller or equal to " + << format.toString(); + return -1; + } + format = sensor->getFormat(sensor->mbusCodes(), maxSize); - LOG(RkISP1, Debug) << "Sensor configured with " << format.toString(); + ret = isp_->setFormat(0, &format); + if (ret < 0) + return ret; + LOG(RkISP1, Debug) << "ISP re-configured with " + << format.toString(); + } - ret = isp_->setFormat(0, &format); + ret = sensor->setFormat(&format); if (ret < 0) return ret; + LOG(RkISP1, Debug) << "Sensor configured with " << format.toString(); + Rectangle rect(0, 0, format.size); ret = isp_->setSelection(0, V4L2_SEL_TGT_CROP, &rect); if (ret < 0)
Make sure to use a sensor format resolution that is smaller or equal to the maximum allowed resolution for the RkISP1. The maximum resolution is defined within the `rkisp1-common.h` file as: define RKISP1_ISP_MAX_WIDTH 4032 define RKISP1_ISP_MAX_HEIGHT 3024 Change the order of setting the formats, in order to first check if the requested resolution exceeds the maximum and search for the next smaller available sensor resolution if that is the case. Fail if no viable sensor format was located. This means that some camera sensors can never operate with their maximum resolution, for example on my OV13850 camera sensor, there are two possible resolutions: 4224x3136 & 2112x1568, the first of those two will never be picked as it surpasses the maximum of the ISP. Signed-off-by: Sebastian Fricke <sebastian.fricke.linux@gmail.com> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 46 ++++++++++++++++++++---- 1 file changed, 40 insertions(+), 6 deletions(-)