[libcamera-devel] [PATCH 1/1] pipeline: rkisp1: Fix sensor-ISP format mismatch
Jacopo Mondi
jacopo at jmondi.org
Thu Nov 19 16:25:24 CET 2020
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 at 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 at 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 at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list