| Message ID | 20211022143907.3089419-4-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for the update. On Fri, 22 Oct 2021 at 15:40, Naushir Patuck <naush@raspberrypi.com> 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@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) 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. 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@raspberrypi.com> Thanks! David > { > 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); > + 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) > -- > 2.25.1 >
Hi David, Thank you for your feedback. On Fri, 22 Oct 2021 at 16:22, David Plowman <david.plowman@raspberrypi.com> wrote: > Hi Naush > > Thanks for the update. > > On Fri, 22 Oct 2021 at 15:40, Naushir Patuck <naush@raspberrypi.com> > 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@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) > > 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". Regards, Naush > > 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@raspberrypi.com> > > Thanks! > David > > > { > > 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); > > + 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) > > -- > > 2.25.1 > > >
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@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@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)
Hi Laurent, Thank you for your feedback. On Mon, 25 Oct 2021 at 17:59, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > 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@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 ? > That seems reasonable, and would really force the caller to make a decision. > > > > 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@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. > Will do! Thanks, Naush > > > > > + 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 >
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) { 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); + 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)
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. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)