[libcamera-devel] [PATCH v2 3/6] pipeline: raspberrypi: Set packing formats for the Unicam image node

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Fri, Oct 22, 2021 at 04:30:45PM +0100, Naushir Patuck wrote:
> On Fri, 22 Oct 2021 at 16:22, David Plowman wrote:
> > On Fri, 22 Oct 2021 at 15:40, Naushir Patuck wrote:
> > >
> > > Default to using CSI2 packed formats when setting up the Unicam image format,
> > > but use an unpacked format if the user requests one through StreamConfiguration.
> >
> > Yes, I think this is the right way to do it!
> >
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > ---
> > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++++--
> > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index a31b0f81eba7..45725527d66e 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -61,11 +61,14 @@ SensorFormats populateSensorFormats(std::unique_ptr<CameraSensor> &sensor)
> > >         return formats;
> > >  }
> > >
> > > -inline V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &mode)
> > > +inline V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &mode, bool unpacked = false)

By the way, I'd drop the inline keyword and let the compiler decide.

> > I'm not totally sure I like the default value here - I kind of feel
> > people should be forced to think about it. But that's a teeny thing so
> > I wouldn't worry.
> 
> I can remove the default value in the next version, and switch the argument
> to be "packed".

How about also replacing the bool argument with a BayerFormat::Packing ?

> > And also ignoring the very minor brain contortion involved in passing
> > *un*-packed which gets flipped to set the *packed* field!
> >
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> >
> > >  {
> > >         V4L2DeviceFormat deviceFormat;
> > >         BayerFormat bayer = BayerFormat::fromMbusCode(mode.mbus_code);
> > >
> > > +       bayer.packing = unpacked ? BayerFormat::Packing::None
> > > +                                : BayerFormat::Packing::CSI2Packed;
> > > +
> > >         deviceFormat.fourcc = bayer.toV4L2PixelFormat();
> > >         deviceFormat.size = mode.size;
> > >         return deviceFormat;
> > > @@ -598,6 +601,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> > >         Size maxSize, sensorSize;
> > >         unsigned int maxIndex = 0;
> > >         bool rawStream = false;
> > > +       bool unpacked = false;
> > >
> > >         /*
> > >          * Look for the RAW stream (if given) size as well as the largest
> > > @@ -613,6 +617,10 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> > >                          */
> > >                         sensorSize = cfg.size;
> > >                         rawStream = true;
> > > +                       /* Check if the user has explicitly set an unpacked format. */
> > > +                       V4L2PixelFormat fourcc = V4L2PixelFormat::fromPixelFormat(cfg.pixelFormat);
> > > +                       BayerFormat bayer = BayerFormat::fromV4L2PixelFormat(fourcc);

This should go to a BayerFormat::fromPixelFormat() function, given that
you'd added BayerFormat::toPixelFormat() in 1/6.

> > > +                       unpacked = bayer.packing == BayerFormat::Packing::None;
> > >                 } else {
> > >                         if (cfg.size > maxSize) {
> > >                                 maxSize = config->at(i).size;
> > > @@ -623,7 +631,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> > >
> > >         /* First calculate the best sensor mode we can use based on the user request. */
> > >         V4L2SubdeviceFormat sensorFormat = findBestMode(data->sensorFormats_, rawStream ? sensorSize : maxSize);
> > > -       V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat);
> > > +       V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, unpacked);
> > >
> > >         ret = data->sensor_->setFormat(&sensorFormat);
> > >         if (ret)

-- 
Regards,

Laurent Pinchart