[PATCH 1/1] libcamera: Add v4l2_subdev_format in V4L2SubdeviceFormat
Cheng-Hao Yang
chenghaoyang at chromium.org
Thu Nov 14 06:33:20 CET 2024
Hi folks,
On Thu, Nov 7, 2024 at 11:48 PM Cheng-Hao Yang
<chenghaoyang at chromium.org> wrote:
>
> Hi Laurent and Jacopo,
>
> On Wed, Nov 6, 2024 at 8:54 PM Laurent Pinchart
> <laurent.pinchart at 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 at chromium.org>
> > > >
> > > > Add v4l2_subdev_format in V4L2SubdeviceFormat and set the value when
> > > > setFormat() and getFormat().
> > > >
> > > > Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> > > > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > > > Signed-off-by: Harvey Yang <chenghaoyang at 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
More information about the libcamera-devel
mailing list