| Message ID | 20201119131331.30847-2-sebastian.fricke.linux@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Sebastian, Thanks for your patch. On 11/19/20 10:13 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 | 36 ++++++++++++++++++++++-- > 1 file changed, 33 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 1b1922a..621e9bf 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -671,6 +671,9 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > RkISP1CameraData *data = cameraData(camera); > CameraSensor *sensor = data->sensor_; > int ret; > + Size original_format_size = Size(0, 0); I believe you can declare this when you need it (when assigning format.size), and you don't need to call the constructor to init to zero. (at least this is the style I see being used in the project). > + Size max_size = Size(0, 0); You can declare this inside the if block, since you only use this there. > + > > ret = initLinks(camera, sensor, *config); > if (ret) > @@ -682,17 +685,44 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > */ > V4L2SubdeviceFormat format = config->sensorFormat(); > LOG(RkISP1, Debug) << "Configuring sensor with " << format.toString(); This message should be updated, since you are configuring the isp and not the sensor. > + // format is changed in setFormat, keep the resolution for comparison > + original_format_size = 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 (original_format_size > format.size) { > + 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 <= format.size && size > max_size) I'm not sure if comparing Size objects is enough. According to the docs: * Sizes are compared on three criteria, in the following order. * * - A size with smaller width and smaller height is smaller. * - A size with smaller area is smaller. * - A size with smaller width is smaller. If it reaches the 3rd case, we can have a resolution that is smaller in width but bigger in height that could go over isp limitation leading to a non-matching configuration. Regards, Helen > + max_size = size; > + } > + if (max_size == 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(), max_size); > > - 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) >
Hi Sebastian, On Thu, Nov 19, 2020 at 02:13:31PM +0100, 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 | 36 ++++++++++++++++++++++-- > 1 file changed, 33 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 1b1922a..621e9bf 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -671,6 +671,9 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > RkISP1CameraData *data = cameraData(camera); > CameraSensor *sensor = data->sensor_; > int ret; > + Size original_format_size = Size(0, 0); > + Size max_size = Size(0, 0); > + > > ret = initLinks(camera, sensor, *config); > if (ret) > @@ -682,17 +685,44 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > */ > V4L2SubdeviceFormat format = config->sensorFormat(); > LOG(RkISP1, Debug) << "Configuring sensor with " << format.toString(); > + // format is changed in setFormat, keep the resolution for comparison Just a style note skimming through the patch. libcamera enforces a common code style http://libcamera.org/coding-style.html#coding-style-guidelines and provides a style checker tool in utils/checkstyle.py Make sure to use it before submitting patches. Thanks j > + original_format_size = 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 (original_format_size > format.size) { > + 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 <= format.size && size > max_size) > + max_size = size; > + } > + if (max_size == 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(), max_size); > > - 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) > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
On 19.11.2020 15:58, Jacopo Mondi wrote: >Hi Sebastian, Hey Jacopo, > >On Thu, Nov 19, 2020 at 02:13:31PM +0100, 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 | 36 ++++++++++++++++++++++-- >> 1 file changed, 33 insertions(+), 3 deletions(-) >> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> index 1b1922a..621e9bf 100644 >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> @@ -671,6 +671,9 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) >> RkISP1CameraData *data = cameraData(camera); >> CameraSensor *sensor = data->sensor_; >> int ret; >> + Size original_format_size = Size(0, 0); >> + Size max_size = Size(0, 0); >> + >> >> ret = initLinks(camera, sensor, *config); >> if (ret) >> @@ -682,17 +685,44 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) >> */ >> V4L2SubdeviceFormat format = config->sensorFormat(); >> LOG(RkISP1, Debug) << "Configuring sensor with " << format.toString(); >> + // format is changed in setFormat, keep the resolution for comparison > >Just a style note skimming through the patch. > >libcamera enforces a common code style >http://libcamera.org/coding-style.html#coding-style-guidelines I think you refer to the '//' comment, should I use a '/* */' comment block for this comment? I read through both the style guidelines and kernel guidelines and I couldn't spot a sentence where it states that '//' comments are not allowed. Or do you not like that explain why I do it? > >and provides a style checker tool in utils/checkstyle.py > >Make sure to use it before submitting patches. That is odd, here is the result of the run I did before submitting the patch: ``` basti@basti:~/Kernel/libcamera$ ./utils/checkstyle.py ----------------------------------------------------------------------------------------- 4cfb814796f2f27bfa4057b2a69618b01eb1de93 pipeline: rkisp1: Fix sensor-ISP format mismatch ----------------------------------------------------------------------------------------- No style issue detected ``` Do you get a different result? And should we change the the style checker in order to detect this mistake? > >Thanks > j Thanks for taking a look! Sebastian > >> + original_format_size = 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 (original_format_size > format.size) { >> + 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 <= format.size && size > max_size) >> + max_size = size; >> + } >> + if (max_size == 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(), max_size); >> >> - 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) >> -- >> 2.20.1 >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel
Hi Sebastian, On Thu, Nov 19, 2020 at 04:11:01PM +0100, Sebastian Fricke wrote: > On 19.11.2020 15:58, Jacopo Mondi wrote: > > Hi Sebastian, > > Hey Jacopo, > > > > > On Thu, Nov 19, 2020 at 02:13:31PM +0100, 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 | 36 ++++++++++++++++++++++-- > > > 1 file changed, 33 insertions(+), 3 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > index 1b1922a..621e9bf 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > @@ -671,6 +671,9 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > > > RkISP1CameraData *data = cameraData(camera); > > > CameraSensor *sensor = data->sensor_; > > > int ret; > > > + Size original_format_size = Size(0, 0); > > > + Size max_size = Size(0, 0); > > > + > > > > > > ret = initLinks(camera, sensor, *config); > > > if (ret) > > > @@ -682,17 +685,44 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > > > */ > > > V4L2SubdeviceFormat format = config->sensorFormat(); > > > LOG(RkISP1, Debug) << "Configuring sensor with " << format.toString(); > > > + // format is changed in setFormat, keep the resolution for comparison > > > > Just a style note skimming through the patch. > > > > libcamera enforces a common code style > > http://libcamera.org/coding-style.html#coding-style-guidelines > > I think you refer to the '//' comment, should I use a '/* */' comment > block for this comment? I read through both the style guidelines and > kernel guidelines and I couldn't spot a sentence where it states that > '//' comments are not allowed. Or do you not like that explain why I do > it? Well, kernel is C, so it does not disallow C++ commenting explicitely in facts :) But yes, we use /* */ Also we use camelCase and not snake_case for variables > > > > > and provides a style checker tool in utils/checkstyle.py > > > > Make sure to use it before submitting patches. > > That is odd, here is the result of the run I did before submitting the > patch: > > ``` > basti@basti:~/Kernel/libcamera$ ./utils/checkstyle.py > ----------------------------------------------------------------------------------------- > 4cfb814796f2f27bfa4057b2a69618b01eb1de93 pipeline: rkisp1: Fix sensor-ISP format mismatch > ----------------------------------------------------------------------------------------- > No style issue detected > ``` > > Do you get a different result? > And should we change the the style checker in order to detect this > mistake? I thought checkstyle checks for // and snake_case, I'm sorry... > > > > > > Thanks > > j > > Thanks for taking a look! > Sebastian > > > > > > + original_format_size = 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 (original_format_size > format.size) { > > > + 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 <= format.size && size > max_size) > > > + max_size = size; > > > + } > > > + if (max_size == 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(), max_size); > > > > > > - 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) > > > -- > > > 2.20.1 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel
On 19.11.2020 16:25, Jacopo Mondi wrote: >Hi Sebastian, > >On Thu, Nov 19, 2020 at 04:11:01PM +0100, Sebastian Fricke wrote: >> On 19.11.2020 15:58, Jacopo Mondi wrote: >> > Hi Sebastian, >> >> Hey Jacopo, >> >> > >> > On Thu, Nov 19, 2020 at 02:13:31PM +0100, 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 | 36 ++++++++++++++++++++++-- >> > > 1 file changed, 33 insertions(+), 3 deletions(-) >> > > >> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> > > index 1b1922a..621e9bf 100644 >> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> > > @@ -671,6 +671,9 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) >> > > RkISP1CameraData *data = cameraData(camera); >> > > CameraSensor *sensor = data->sensor_; >> > > int ret; >> > > + Size original_format_size = Size(0, 0); >> > > + Size max_size = Size(0, 0); > >> > > + >> > > >> > > ret = initLinks(camera, sensor, *config); >> > > if (ret) >> > > @@ -682,17 +685,44 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) >> > > */ >> > > V4L2SubdeviceFormat format = config->sensorFormat(); >> > > LOG(RkISP1, Debug) << "Configuring sensor with " << format.toString(); >> > > + // format is changed in setFormat, keep the resolution for comparison >> > >> > Just a style note skimming through the patch. >> > >> > libcamera enforces a common code style >> > http://libcamera.org/coding-style.html#coding-style-guidelines >> >> I think you refer to the '//' comment, should I use a '/* */' comment >> block for this comment? I read through both the style guidelines and >> kernel guidelines and I couldn't spot a sentence where it states that >> '//' comments are not allowed. Or do you not like that explain why I do >> it? > >Well, kernel is C, so it does not disallow C++ commenting explicitely >in facts :) > >But yes, we use /* */ > >Also we use camelCase and not snake_case for variables > Thanks I take that into consideration for v2. >> >> > >> > and provides a style checker tool in utils/checkstyle.py >> > >> > Make sure to use it before submitting patches. >> >> That is odd, here is the result of the run I did before submitting the >> patch: >> >> ``` >> basti@basti:~/Kernel/libcamera$ ./utils/checkstyle.py >> ----------------------------------------------------------------------------------------- >> 4cfb814796f2f27bfa4057b2a69618b01eb1de93 pipeline: rkisp1: Fix sensor-ISP format mismatch >> ----------------------------------------------------------------------------------------- >> No style issue detected >> ``` >> >> Do you get a different result? >> And should we change the the style checker in order to detect this >> mistake? > >I thought checkstyle checks for // and snake_case, I'm sorry... > Do we want to add that to the ToDo list? >> >> >> > >> > Thanks >> > j >> >> Thanks for taking a look! >> Sebastian >> >> > >> > > + original_format_size = 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 (original_format_size > format.size) { >> > > + 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 <= format.size && size > max_size) >> > > + max_size = size; >> > > + } >> > > + if (max_size == 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(), max_size); >> > > >> > > - 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) >> > > -- >> > > 2.20.1 >> > > >> > > _______________________________________________ >> > > libcamera-devel mailing list >> > > libcamera-devel@lists.libcamera.org >> > > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 1b1922a..621e9bf 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -671,6 +671,9 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) RkISP1CameraData *data = cameraData(camera); CameraSensor *sensor = data->sensor_; int ret; + Size original_format_size = Size(0, 0); + Size max_size = Size(0, 0); + ret = initLinks(camera, sensor, *config); if (ret) @@ -682,17 +685,44 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) */ V4L2SubdeviceFormat format = config->sensorFormat(); LOG(RkISP1, Debug) << "Configuring sensor with " << format.toString(); + // format is changed in setFormat, keep the resolution for comparison + original_format_size = 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 (original_format_size > format.size) { + 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 <= format.size && size > max_size) + max_size = size; + } + if (max_size == 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(), max_size); - 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 | 36 ++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 3 deletions(-)