| Message ID | 20241105105445.1468954-2-chenghaoyang@chromium.org |
|---|---|
| State | Rejected |
| Headers | show |
| Series |
|
| Related | show |
Hi Harvey, Han-Lin On Tue, Nov 05, 2024 at 10:49:21AM +0000, Harvey Yang wrote: > From: Han-Lin Chen <hanlinchen@chromium.org> > > Add v4l2_subdev_format in V4L2SubdeviceFormat and set the value when > setFormat() and getFormat(). > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > --- > include/libcamera/internal/v4l2_subdevice.h | 2 ++ > src/libcamera/pipeline/simple/simple.cpp | 2 +- > src/libcamera/sensor/camera_sensor.cpp | 1 + > src/libcamera/v4l2_subdevice.cpp | 10 ++++++++++ > 4 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h > index 194382f84..d81296ee6 100644 > --- a/include/libcamera/internal/v4l2_subdevice.h > +++ b/include/libcamera/internal/v4l2_subdevice.h > @@ -66,6 +66,8 @@ struct V4L2SubdeviceFormat { > std::optional<ColorSpace> colorSpace; > > const std::string toString() const; > + > + struct v4l2_subdev_format subdevFmt; > }; > > std::ostream &operator<<(std::ostream &out, const V4L2SubdeviceFormat &f); > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 3ddce71d3..6ec055596 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -1063,7 +1063,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > LOG(SimplePipeline, Debug) > << "Picked " > - << V4L2SubdeviceFormat{ pipeConfig_->code, pipeConfig_->sensorSize, {} } > + << V4L2SubdeviceFormat{ pipeConfig_->code, pipeConfig_->sensorSize, {}, {} } > << " -> " << pipeConfig_->captureSize > << "-" << pipeConfig_->captureFormat > << " for max stream size " << maxStreamSize; > diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp > index 1b224f198..ac96b4843 100644 > --- a/src/libcamera/sensor/camera_sensor.cpp > +++ b/src/libcamera/sensor/camera_sensor.cpp > @@ -744,6 +744,7 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbu > .code = bestCode, > .size = *bestSize, > .colorSpace = ColorSpace::Raw, > + .subdevFmt = {}, > }; > > return format; > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > index 9f2ec4798..677714890 100644 > --- a/src/libcamera/v4l2_subdevice.cpp > +++ b/src/libcamera/v4l2_subdevice.cpp > @@ -808,6 +808,14 @@ const MediaBusFormatInfo &MediaBusFormatInfo::info(uint32_t code) > * resulting color space is acceptable. > */ > > +/** > + * \var V4L2SubdeviceFormat::subdevFmt > + * \brief The whole v4l2_subdev_format after calling setFormat()/getFormat() > + * > + * It's used in some pipeline handlers that need extra information apart from > + * the existing fields. > + */ > + > /** > * \brief Assemble and return a string describing the format > * \return A string describing the V4L2SubdeviceFormat > @@ -1266,6 +1274,7 @@ int V4L2Subdevice::getFormat(const Stream &stream, V4L2SubdeviceFormat *format, > format->size.height = subdevFmt.format.height; > format->code = subdevFmt.format.code; > format->colorSpace = toColorSpace(subdevFmt.format); > + format->subdevFmt = subdevFmt; > > return 0; > } > @@ -1324,6 +1333,7 @@ int V4L2Subdevice::setFormat(const Stream &stream, V4L2SubdeviceFormat *format, > format->size.height = subdevFmt.format.height; > format->code = subdevFmt.format.code; > format->colorSpace = toColorSpace(subdevFmt.format); > + format->subdevFmt = subdevFmt; I really don't like this last two bits. The getFormat and setFormat functions already use an instance of 'struct v4l2_subdev_format' and copying it back and forth defeats the purpose of using the V4L2SubdeviceFormat libcamera types and the abstractions built around them. Looking at V4L2SubdeviceFormat, most of the information contained in 'struct v4l2_subdev_format' are already there (apart from field, if I'm seeing it right). The thing is that they get translated into the corresponding libcamera representations and, if for some translating them back to v4l2 fields is trivial (code and sizes in example) the translation of certain fields happens inside V4L2VideoDevice, particularly the colorspace related fields that happens in V4L2Device::fromColorSpace()/V4L2Device::toColorSpace(). If you have to re-create a v4l2_subdev_format instance to pass it to the MTK kernel interface through the V4L2_CID_MTK_CAM_RAW_RESOURCE_CALC control, you would have to reimplement V4L2Device::fromColorSpace()/ V4L2Device::toColorSpace() in the pipeline handler. Not ideal indeed. Let alone the fact, ideally at least, v4l2 abstractions should not be present in the pipeline handlers. One option could be to add and std::optional<v4l2_subdev_format> to V4L2SubdevFormat. If the caller of getFormat/setFormat populates it, it will be used in place of the local subdevFmt in those two functions. It will need to be properly documented, and explain it is only needed in the rare case the pipeline handler needs to access the v4l2 subdev interface. I would like to hear what others think as well. Thanks j > > return 0; > } > -- > 2.47.0.199.ga7371fff76-goog >
On Tue, Nov 05, 2024 at 05:13:04PM +0100, Jacopo Mondi wrote: > On Tue, Nov 05, 2024 at 10:49:21AM +0000, Harvey Yang wrote: > > From: Han-Lin Chen <hanlinchen@chromium.org> > > > > Add v4l2_subdev_format in V4L2SubdeviceFormat and set the value when > > setFormat() and getFormat(). > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > --- > > include/libcamera/internal/v4l2_subdevice.h | 2 ++ > > src/libcamera/pipeline/simple/simple.cpp | 2 +- > > src/libcamera/sensor/camera_sensor.cpp | 1 + > > src/libcamera/v4l2_subdevice.cpp | 10 ++++++++++ > > 4 files changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h > > index 194382f84..d81296ee6 100644 > > --- a/include/libcamera/internal/v4l2_subdevice.h > > +++ b/include/libcamera/internal/v4l2_subdevice.h > > @@ -66,6 +66,8 @@ struct V4L2SubdeviceFormat { > > std::optional<ColorSpace> colorSpace; > > > > const std::string toString() const; > > + > > + struct v4l2_subdev_format subdevFmt; > > }; > > > > std::ostream &operator<<(std::ostream &out, const V4L2SubdeviceFormat &f); > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > > index 3ddce71d3..6ec055596 100644 > > --- a/src/libcamera/pipeline/simple/simple.cpp > > +++ b/src/libcamera/pipeline/simple/simple.cpp > > @@ -1063,7 +1063,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > > > LOG(SimplePipeline, Debug) > > << "Picked " > > - << V4L2SubdeviceFormat{ pipeConfig_->code, pipeConfig_->sensorSize, {} } > > + << V4L2SubdeviceFormat{ pipeConfig_->code, pipeConfig_->sensorSize, {}, {} } > > << " -> " << pipeConfig_->captureSize > > << "-" << pipeConfig_->captureFormat > > << " for max stream size " << maxStreamSize; > > diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp > > index 1b224f198..ac96b4843 100644 > > --- a/src/libcamera/sensor/camera_sensor.cpp > > +++ b/src/libcamera/sensor/camera_sensor.cpp > > @@ -744,6 +744,7 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbu > > .code = bestCode, > > .size = *bestSize, > > .colorSpace = ColorSpace::Raw, > > + .subdevFmt = {}, > > }; > > > > return format; > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > > index 9f2ec4798..677714890 100644 > > --- a/src/libcamera/v4l2_subdevice.cpp > > +++ b/src/libcamera/v4l2_subdevice.cpp > > @@ -808,6 +808,14 @@ const MediaBusFormatInfo &MediaBusFormatInfo::info(uint32_t code) > > * resulting color space is acceptable. > > */ > > > > +/** > > + * \var V4L2SubdeviceFormat::subdevFmt > > + * \brief The whole v4l2_subdev_format after calling setFormat()/getFormat() > > + * > > + * It's used in some pipeline handlers that need extra information apart from > > + * the existing fields. > > + */ > > + > > /** > > * \brief Assemble and return a string describing the format > > * \return A string describing the V4L2SubdeviceFormat > > @@ -1266,6 +1274,7 @@ int V4L2Subdevice::getFormat(const Stream &stream, V4L2SubdeviceFormat *format, > > format->size.height = subdevFmt.format.height; > > format->code = subdevFmt.format.code; > > format->colorSpace = toColorSpace(subdevFmt.format); > > + format->subdevFmt = subdevFmt; > > > > return 0; > > } > > @@ -1324,6 +1333,7 @@ int V4L2Subdevice::setFormat(const Stream &stream, V4L2SubdeviceFormat *format, > > format->size.height = subdevFmt.format.height; > > format->code = subdevFmt.format.code; > > format->colorSpace = toColorSpace(subdevFmt.format); > > + format->subdevFmt = subdevFmt; > > I really don't like this last two bits. The getFormat and setFormat > functions already use an instance of 'struct v4l2_subdev_format' and > copying it back and forth defeats the purpose of using the > V4L2SubdeviceFormat libcamera types and the abstractions built around > them. > > Looking at V4L2SubdeviceFormat, most of the information contained in > 'struct v4l2_subdev_format' are already there (apart from field, if > I'm seeing it right). The thing is that they get translated into the > corresponding libcamera representations and, if for some translating > them back to v4l2 fields is trivial (code and sizes in example) the > translation of certain fields happens inside V4L2VideoDevice, > particularly the colorspace related fields that happens in > V4L2Device::fromColorSpace()/V4L2Device::toColorSpace(). > > If you have to re-create a v4l2_subdev_format instance to pass it to > the MTK kernel interface through the V4L2_CID_MTK_CAM_RAW_RESOURCE_CALC > control, you would have to reimplement V4L2Device::fromColorSpace()/ > V4L2Device::toColorSpace() in the pipeline handler. Not ideal indeed. > > Let alone the fact, ideally at least, v4l2 abstractions should not > be present in the pipeline handlers. > > One option could be to add and std::optional<v4l2_subdev_format> to > V4L2SubdevFormat. If the caller of getFormat/setFormat populates it, > it will be used in place of the local subdevFmt in those two functions. > > It will need to be properly documented, and explain it is only needed > in the rare case the pipeline handler needs to access the v4l2 subdev > interface. > > I would like to hear what others think as well. Unsurprisingly, I'm not thrilled. V4L2_CID_MTK_CAM_RAW_RESOURCE_CALC seems to be a big hack (not even commenting on the fact that the control *value* contains userspace pointers). I think the driver will need to be substantially reworked, and APIs will need to be cleaned up. We certainly shouldn't merge this in the libcamera core before the situation on the kernel side improves. > > > > return 0; > > }
Hi Laurent and Jacopo, On Wed, Nov 6, 2024 at 8:54 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Tue, Nov 05, 2024 at 05:13:04PM +0100, Jacopo Mondi wrote: > > On Tue, Nov 05, 2024 at 10:49:21AM +0000, Harvey Yang wrote: > > > From: Han-Lin Chen <hanlinchen@chromium.org> > > > > > > Add v4l2_subdev_format in V4L2SubdeviceFormat and set the value when > > > setFormat() and getFormat(). > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > > --- > > > include/libcamera/internal/v4l2_subdevice.h | 2 ++ > > > src/libcamera/pipeline/simple/simple.cpp | 2 +- > > > src/libcamera/sensor/camera_sensor.cpp | 1 + > > > src/libcamera/v4l2_subdevice.cpp | 10 ++++++++++ > > > 4 files changed, 14 insertions(+), 1 deletion(-) > > > > > > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h > > > index 194382f84..d81296ee6 100644 > > > --- a/include/libcamera/internal/v4l2_subdevice.h > > > +++ b/include/libcamera/internal/v4l2_subdevice.h > > > @@ -66,6 +66,8 @@ struct V4L2SubdeviceFormat { > > > std::optional<ColorSpace> colorSpace; > > > > > > const std::string toString() const; > > > + > > > + struct v4l2_subdev_format subdevFmt; > > > }; > > > > > > std::ostream &operator<<(std::ostream &out, const V4L2SubdeviceFormat &f); > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > > > index 3ddce71d3..6ec055596 100644 > > > --- a/src/libcamera/pipeline/simple/simple.cpp > > > +++ b/src/libcamera/pipeline/simple/simple.cpp > > > @@ -1063,7 +1063,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > > > > > LOG(SimplePipeline, Debug) > > > << "Picked " > > > - << V4L2SubdeviceFormat{ pipeConfig_->code, pipeConfig_->sensorSize, {} } > > > + << V4L2SubdeviceFormat{ pipeConfig_->code, pipeConfig_->sensorSize, {}, {} } > > > << " -> " << pipeConfig_->captureSize > > > << "-" << pipeConfig_->captureFormat > > > << " for max stream size " << maxStreamSize; > > > diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp > > > index 1b224f198..ac96b4843 100644 > > > --- a/src/libcamera/sensor/camera_sensor.cpp > > > +++ b/src/libcamera/sensor/camera_sensor.cpp > > > @@ -744,6 +744,7 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbu > > > .code = bestCode, > > > .size = *bestSize, > > > .colorSpace = ColorSpace::Raw, > > > + .subdevFmt = {}, > > > }; > > > > > > return format; > > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > > > index 9f2ec4798..677714890 100644 > > > --- a/src/libcamera/v4l2_subdevice.cpp > > > +++ b/src/libcamera/v4l2_subdevice.cpp > > > @@ -808,6 +808,14 @@ const MediaBusFormatInfo &MediaBusFormatInfo::info(uint32_t code) > > > * resulting color space is acceptable. > > > */ > > > > > > +/** > > > + * \var V4L2SubdeviceFormat::subdevFmt > > > + * \brief The whole v4l2_subdev_format after calling setFormat()/getFormat() > > > + * > > > + * It's used in some pipeline handlers that need extra information apart from > > > + * the existing fields. > > > + */ > > > + > > > /** > > > * \brief Assemble and return a string describing the format > > > * \return A string describing the V4L2SubdeviceFormat > > > @@ -1266,6 +1274,7 @@ int V4L2Subdevice::getFormat(const Stream &stream, V4L2SubdeviceFormat *format, > > > format->size.height = subdevFmt.format.height; > > > format->code = subdevFmt.format.code; > > > format->colorSpace = toColorSpace(subdevFmt.format); > > > + format->subdevFmt = subdevFmt; > > > > > > return 0; > > > } > > > @@ -1324,6 +1333,7 @@ int V4L2Subdevice::setFormat(const Stream &stream, V4L2SubdeviceFormat *format, > > > format->size.height = subdevFmt.format.height; > > > format->code = subdevFmt.format.code; > > > format->colorSpace = toColorSpace(subdevFmt.format); > > > + format->subdevFmt = subdevFmt; > > > > I really don't like this last two bits. The getFormat and setFormat > > functions already use an instance of 'struct v4l2_subdev_format' and > > copying it back and forth defeats the purpose of using the > > V4L2SubdeviceFormat libcamera types and the abstractions built around > > them. > > > > Looking at V4L2SubdeviceFormat, most of the information contained in > > 'struct v4l2_subdev_format' are already there (apart from field, if > > I'm seeing it right). The thing is that they get translated into the > > corresponding libcamera representations and, if for some translating > > them back to v4l2 fields is trivial (code and sizes in example) the > > translation of certain fields happens inside V4L2VideoDevice, > > particularly the colorspace related fields that happens in > > V4L2Device::fromColorSpace()/V4L2Device::toColorSpace(). > > > > If you have to re-create a v4l2_subdev_format instance to pass it to > > the MTK kernel interface through the V4L2_CID_MTK_CAM_RAW_RESOURCE_CALC > > control, you would have to reimplement V4L2Device::fromColorSpace()/ > > V4L2Device::toColorSpace() in the pipeline handler. Not ideal indeed. > > > > Let alone the fact, ideally at least, v4l2 abstractions should not > > be present in the pipeline handlers. > > > > One option could be to add and std::optional<v4l2_subdev_format> to > > V4L2SubdevFormat. If the caller of getFormat/setFormat populates it, > > it will be used in place of the local subdevFmt in those two functions. > > > > It will need to be properly documented, and explain it is only needed > > in the rare case the pipeline handler needs to access the v4l2 subdev > > interface. > > > > I would like to hear what others think as well. > > Unsurprisingly, I'm not thrilled. V4L2_CID_MTK_CAM_RAW_RESOURCE_CALC > seems to be a big hack (not even commenting on the fact that the control > *value* contains userspace pointers). I think the driver will need to be > substantially reworked, and APIs will need to be cleaned up. We > certainly shouldn't merge this in the libcamera core before the > situation on the kernel side improves. Yeah, I agree. Also, I've tried to provide only width & height in struct v4l2_mbus_framefmt, and the kernel still works [1]. Let's abandon this CL for now. I'll wait for MTK's comments though. BR, Harvey [1]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/v6.1/drivers/media/platform/mediatek/isp/isp_7x/camsys/mtk_cam-raw.c;l=607 > > > > > > > return 0; > > > } > > -- > Regards, > > Laurent Pinchart
Hi folks, On Thu, Nov 7, 2024 at 11:48 PM Cheng-Hao Yang <chenghaoyang@chromium.org> wrote: > > Hi Laurent and Jacopo, > > On Wed, Nov 6, 2024 at 8:54 PM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > On Tue, Nov 05, 2024 at 05:13:04PM +0100, Jacopo Mondi wrote: > > > On Tue, Nov 05, 2024 at 10:49:21AM +0000, Harvey Yang wrote: > > > > From: Han-Lin Chen <hanlinchen@chromium.org> > > > > > > > > Add v4l2_subdev_format in V4L2SubdeviceFormat and set the value when > > > > setFormat() and getFormat(). > > > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > > > --- > > > > include/libcamera/internal/v4l2_subdevice.h | 2 ++ > > > > src/libcamera/pipeline/simple/simple.cpp | 2 +- > > > > src/libcamera/sensor/camera_sensor.cpp | 1 + > > > > src/libcamera/v4l2_subdevice.cpp | 10 ++++++++++ > > > > 4 files changed, 14 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h > > > > index 194382f84..d81296ee6 100644 > > > > --- a/include/libcamera/internal/v4l2_subdevice.h > > > > +++ b/include/libcamera/internal/v4l2_subdevice.h > > > > @@ -66,6 +66,8 @@ struct V4L2SubdeviceFormat { > > > > std::optional<ColorSpace> colorSpace; > > > > > > > > const std::string toString() const; > > > > + > > > > + struct v4l2_subdev_format subdevFmt; > > > > }; > > > > > > > > std::ostream &operator<<(std::ostream &out, const V4L2SubdeviceFormat &f); > > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > > > > index 3ddce71d3..6ec055596 100644 > > > > --- a/src/libcamera/pipeline/simple/simple.cpp > > > > +++ b/src/libcamera/pipeline/simple/simple.cpp > > > > @@ -1063,7 +1063,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > > > > > > > > LOG(SimplePipeline, Debug) > > > > << "Picked " > > > > - << V4L2SubdeviceFormat{ pipeConfig_->code, pipeConfig_->sensorSize, {} } > > > > + << V4L2SubdeviceFormat{ pipeConfig_->code, pipeConfig_->sensorSize, {}, {} } > > > > << " -> " << pipeConfig_->captureSize > > > > << "-" << pipeConfig_->captureFormat > > > > << " for max stream size " << maxStreamSize; > > > > diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp > > > > index 1b224f198..ac96b4843 100644 > > > > --- a/src/libcamera/sensor/camera_sensor.cpp > > > > +++ b/src/libcamera/sensor/camera_sensor.cpp > > > > @@ -744,6 +744,7 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbu > > > > .code = bestCode, > > > > .size = *bestSize, > > > > .colorSpace = ColorSpace::Raw, > > > > + .subdevFmt = {}, > > > > }; > > > > > > > > return format; > > > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > > > > index 9f2ec4798..677714890 100644 > > > > --- a/src/libcamera/v4l2_subdevice.cpp > > > > +++ b/src/libcamera/v4l2_subdevice.cpp > > > > @@ -808,6 +808,14 @@ const MediaBusFormatInfo &MediaBusFormatInfo::info(uint32_t code) > > > > * resulting color space is acceptable. > > > > */ > > > > > > > > +/** > > > > + * \var V4L2SubdeviceFormat::subdevFmt > > > > + * \brief The whole v4l2_subdev_format after calling setFormat()/getFormat() > > > > + * > > > > + * It's used in some pipeline handlers that need extra information apart from > > > > + * the existing fields. > > > > + */ > > > > + > > > > /** > > > > * \brief Assemble and return a string describing the format > > > > * \return A string describing the V4L2SubdeviceFormat > > > > @@ -1266,6 +1274,7 @@ int V4L2Subdevice::getFormat(const Stream &stream, V4L2SubdeviceFormat *format, > > > > format->size.height = subdevFmt.format.height; > > > > format->code = subdevFmt.format.code; > > > > format->colorSpace = toColorSpace(subdevFmt.format); > > > > + format->subdevFmt = subdevFmt; > > > > > > > > return 0; > > > > } > > > > @@ -1324,6 +1333,7 @@ int V4L2Subdevice::setFormat(const Stream &stream, V4L2SubdeviceFormat *format, > > > > format->size.height = subdevFmt.format.height; > > > > format->code = subdevFmt.format.code; > > > > format->colorSpace = toColorSpace(subdevFmt.format); > > > > + format->subdevFmt = subdevFmt; > > > > > > I really don't like this last two bits. The getFormat and setFormat > > > functions already use an instance of 'struct v4l2_subdev_format' and > > > copying it back and forth defeats the purpose of using the > > > V4L2SubdeviceFormat libcamera types and the abstractions built around > > > them. > > > > > > Looking at V4L2SubdeviceFormat, most of the information contained in > > > 'struct v4l2_subdev_format' are already there (apart from field, if > > > I'm seeing it right). The thing is that they get translated into the > > > corresponding libcamera representations and, if for some translating > > > them back to v4l2 fields is trivial (code and sizes in example) the > > > translation of certain fields happens inside V4L2VideoDevice, > > > particularly the colorspace related fields that happens in > > > V4L2Device::fromColorSpace()/V4L2Device::toColorSpace(). > > > > > > If you have to re-create a v4l2_subdev_format instance to pass it to > > > the MTK kernel interface through the V4L2_CID_MTK_CAM_RAW_RESOURCE_CALC > > > control, you would have to reimplement V4L2Device::fromColorSpace()/ > > > V4L2Device::toColorSpace() in the pipeline handler. Not ideal indeed. > > > > > > Let alone the fact, ideally at least, v4l2 abstractions should not > > > be present in the pipeline handlers. > > > > > > One option could be to add and std::optional<v4l2_subdev_format> to > > > V4L2SubdevFormat. If the caller of getFormat/setFormat populates it, > > > it will be used in place of the local subdevFmt in those two functions. > > > > > > It will need to be properly documented, and explain it is only needed > > > in the rare case the pipeline handler needs to access the v4l2 subdev > > > interface. > > > > > > I would like to hear what others think as well. > > > > Unsurprisingly, I'm not thrilled. V4L2_CID_MTK_CAM_RAW_RESOURCE_CALC > > seems to be a big hack (not even commenting on the fact that the control > > *value* contains userspace pointers). I think the driver will need to be > > substantially reworked, and APIs will need to be cleaned up. We > > certainly shouldn't merge this in the libcamera core before the > > situation on the kernel side improves. > > Yeah, I agree. > Also, I've tried to provide only width & height in struct v4l2_mbus_framefmt, > and the kernel still works [1]. > > Let's abandon this CL for now. I'll wait for MTK's comments though. Confirmed with MTK: the kernel only needs width and height. We should update the kernel that takes the two values only. BR, Harvey > > BR, > Harvey > > [1]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/v6.1/drivers/media/platform/mediatek/isp/isp_7x/camsys/mtk_cam-raw.c;l=607 > > > > > > > > > > > return 0; > > > > } > > > > -- > > Regards, > > > > Laurent Pinchart
diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h index 194382f84..d81296ee6 100644 --- a/include/libcamera/internal/v4l2_subdevice.h +++ b/include/libcamera/internal/v4l2_subdevice.h @@ -66,6 +66,8 @@ struct V4L2SubdeviceFormat { std::optional<ColorSpace> colorSpace; const std::string toString() const; + + struct v4l2_subdev_format subdevFmt; }; std::ostream &operator<<(std::ostream &out, const V4L2SubdeviceFormat &f); diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 3ddce71d3..6ec055596 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -1063,7 +1063,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() LOG(SimplePipeline, Debug) << "Picked " - << V4L2SubdeviceFormat{ pipeConfig_->code, pipeConfig_->sensorSize, {} } + << V4L2SubdeviceFormat{ pipeConfig_->code, pipeConfig_->sensorSize, {}, {} } << " -> " << pipeConfig_->captureSize << "-" << pipeConfig_->captureFormat << " for max stream size " << maxStreamSize; diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp index 1b224f198..ac96b4843 100644 --- a/src/libcamera/sensor/camera_sensor.cpp +++ b/src/libcamera/sensor/camera_sensor.cpp @@ -744,6 +744,7 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbu .code = bestCode, .size = *bestSize, .colorSpace = ColorSpace::Raw, + .subdevFmt = {}, }; return format; diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp index 9f2ec4798..677714890 100644 --- a/src/libcamera/v4l2_subdevice.cpp +++ b/src/libcamera/v4l2_subdevice.cpp @@ -808,6 +808,14 @@ const MediaBusFormatInfo &MediaBusFormatInfo::info(uint32_t code) * resulting color space is acceptable. */ +/** + * \var V4L2SubdeviceFormat::subdevFmt + * \brief The whole v4l2_subdev_format after calling setFormat()/getFormat() + * + * It's used in some pipeline handlers that need extra information apart from + * the existing fields. + */ + /** * \brief Assemble and return a string describing the format * \return A string describing the V4L2SubdeviceFormat @@ -1266,6 +1274,7 @@ int V4L2Subdevice::getFormat(const Stream &stream, V4L2SubdeviceFormat *format, format->size.height = subdevFmt.format.height; format->code = subdevFmt.format.code; format->colorSpace = toColorSpace(subdevFmt.format); + format->subdevFmt = subdevFmt; return 0; } @@ -1324,6 +1333,7 @@ int V4L2Subdevice::setFormat(const Stream &stream, V4L2SubdeviceFormat *format, format->size.height = subdevFmt.format.height; format->code = subdevFmt.format.code; format->colorSpace = toColorSpace(subdevFmt.format); + format->subdevFmt = subdevFmt; return 0; }