[libcamera-devel,00/11] Support OV7251 in IPU3 pipeline and qcam
mbox series

Message ID 20230318234014.29506-1-dan.scally@ideasonboard.com
Headers show
Series
  • Support OV7251 in IPU3 pipeline and qcam
Related show

Message

Dan Scally March 18, 2023, 11:40 p.m. UTC
Hello all

This series is an attempt at adding support for the OV7251 camera on Microsoft
Surface platforms to libcamera. The sensor outputs data in MEDIA_BUS_FMT_Y10_1X10
format, which is packed by the CIO2 CSI-2 reciever into V4L2_PIX_FMT_IPU3_Y10. As
far as I know, these data are not intended to run through the Imgu at all. Certainly
the kernel's ipu3-imgu driver does not support that format, but given the simplicity
of the format we can simply use the CIO2 device's capture-capable devnode as the
source of the image data.

To facilitate that, I'm identifying sensors which don't have a format that needs
the Imgu and simply skipping the Imgu/IPA processing in the IPU3 pipeline for
them.

What I don't particularly like about the way that I've done things here is that
the stream only works for the Raw role, which feels wrong. The advantage is that
it's contained within the existing IPU3 pipeline without too much disruption,
but I do wonder whether a separate CIO2-only pipeline might not be better. In
that case PipelineHandlerIPU3::match() would need amending to check whether the
sensor for each "ipu3-csi-2 n" instance needed hooking into the Imgu and not adding
those to the device match if it found that they didn't need it, so that a new
CIO2-only pipeline could claim the media devices. I'd love to hear other people's
thoughts about the best way to handle this.

Thanks
Dan

Daniel Scally (11):
  pipeline: ipu3: Check if sensor supports test pattern control
  include: drm_fourcc: Add Y10 format
  libcamera/formats: Add IPU3_Y10 format
  pipeline: ipu3: Identify sensors that do not need the Imgu
  pipeline: ipu3: Add needsImgu flag to IPU3Frames
  pipeline: ipu3: Support sensors using only CIO2
  pipeline: ipu3: Allow raw-only streams in IPU3 pipeline
  pipeline: ipu3: Support IPU3_Y10 format
  apps: qcam: Add support for IPU3_Y10
  apps: qcam: Remove restriction on raw-only streams
  pipeline: ipu3: Increase maximum connected cameras

 include/linux/drm_fourcc.h             |   3 +
 src/apps/qcam/format_converter.cpp     |  50 +++++
 src/apps/qcam/format_converter.h       |   2 +
 src/apps/qcam/main_window.cpp          |   6 -
 src/libcamera/formats.cpp              |  10 +
 src/libcamera/formats.yaml             |   3 +
 src/libcamera/pipeline/ipu3/cio2.cpp   |  21 +++
 src/libcamera/pipeline/ipu3/cio2.h     |   4 +
 src/libcamera/pipeline/ipu3/frames.cpp |  45 +++--
 src/libcamera/pipeline/ipu3/frames.h   |   3 +-
 src/libcamera/pipeline/ipu3/ipu3.cpp   | 242 ++++++++++++++-----------
 11 files changed, 255 insertions(+), 134 deletions(-)

Comments

Jacopo Mondi March 20, 2023, 7:56 a.m. UTC | #1
Hi Dan

On Sat, Mar 18, 2023 at 11:40:03PM +0000, Daniel Scally via libcamera-devel wrote:
> Hello all
>
> This series is an attempt at adding support for the OV7251 camera on Microsoft
> Surface platforms to libcamera. The sensor outputs data in MEDIA_BUS_FMT_Y10_1X10
> format, which is packed by the CIO2 CSI-2 reciever into V4L2_PIX_FMT_IPU3_Y10. As
> far as I know, these data are not intended to run through the Imgu at all. Certainly

Aren't they ? How can an application generically display Y10 if it
receives data packed in a platform-specific format ?

> the kernel's ipu3-imgu driver does not support that format, but given the simplicity

Maybe it is still just missing from the driver ? Should we ask the
maintainers of the driver if it is just missing or it is not supported
at all ? From a quick glance I have't find a trivial way to add Y10
support to imgu_css_formats[]

> of the format we can simply use the CIO2 device's capture-capable devnode as the
> source of the image data.
>
> To facilitate that, I'm identifying sensors which don't have a format that needs
> the Imgu and simply skipping the Imgu/IPA processing in the IPU3 pipeline for
> them.
>
> What I don't particularly like about the way that I've done things here is that
> the stream only works for the Raw role, which feels wrong. The advantage is that
> it's contained within the existing IPU3 pipeline without too much disruption,
> but I do wonder whether a separate CIO2-only pipeline might not be better. In
> that case PipelineHandlerIPU3::match() would need amending to check whether the
> sensor for each "ipu3-csi-2 n" instance needed hooking into the Imgu and not adding
> those to the device match if it found that they didn't need it, so that a new
> CIO2-only pipeline could claim the media devices. I'd love to hear other people's
> thoughts about the best way to handle this.
>
> Thanks
> Dan
>
> Daniel Scally (11):
>   pipeline: ipu3: Check if sensor supports test pattern control
>   include: drm_fourcc: Add Y10 format
>   libcamera/formats: Add IPU3_Y10 format
>   pipeline: ipu3: Identify sensors that do not need the Imgu
>   pipeline: ipu3: Add needsImgu flag to IPU3Frames
>   pipeline: ipu3: Support sensors using only CIO2
>   pipeline: ipu3: Allow raw-only streams in IPU3 pipeline
>   pipeline: ipu3: Support IPU3_Y10 format
>   apps: qcam: Add support for IPU3_Y10
>   apps: qcam: Remove restriction on raw-only streams
>   pipeline: ipu3: Increase maximum connected cameras
>
>  include/linux/drm_fourcc.h             |   3 +
>  src/apps/qcam/format_converter.cpp     |  50 +++++
>  src/apps/qcam/format_converter.h       |   2 +
>  src/apps/qcam/main_window.cpp          |   6 -
>  src/libcamera/formats.cpp              |  10 +
>  src/libcamera/formats.yaml             |   3 +
>  src/libcamera/pipeline/ipu3/cio2.cpp   |  21 +++
>  src/libcamera/pipeline/ipu3/cio2.h     |   4 +
>  src/libcamera/pipeline/ipu3/frames.cpp |  45 +++--
>  src/libcamera/pipeline/ipu3/frames.h   |   3 +-
>  src/libcamera/pipeline/ipu3/ipu3.cpp   | 242 ++++++++++++++-----------
>  11 files changed, 255 insertions(+), 134 deletions(-)
>
> --
> 2.34.1
>
Laurent Pinchart April 18, 2023, 4:46 p.m. UTC | #2
On Mon, Mar 20, 2023 at 08:56:00AM +0100, Jacopo Mondi via libcamera-devel wrote:
> Hi Dan
> 
> On Sat, Mar 18, 2023 at 11:40:03PM +0000, Daniel Scally via libcamera-devel wrote:
> > Hello all
> >
> > This series is an attempt at adding support for the OV7251 camera on Microsoft
> > Surface platforms to libcamera. The sensor outputs data in MEDIA_BUS_FMT_Y10_1X10
> > format, which is packed by the CIO2 CSI-2 reciever into V4L2_PIX_FMT_IPU3_Y10. As
> > far as I know, these data are not intended to run through the Imgu at all. Certainly
> 
> Aren't they ? How can an application generically display Y10 if it
> receives data packed in a platform-specific format ?

This will require platform-specific applications :-(

> > the kernel's ipu3-imgu driver does not support that format, but given the simplicity
> 
> Maybe it is still just missing from the driver ? Should we ask the
> maintainers of the driver if it is just missing or it is not supported
> at all ? From a quick glance I have't find a trivial way to add Y10
> support to imgu_css_formats[]

It's most likely possible at the hardware level, but the information we
have is that at least the firmware doesn't support skipping the CFA
interpolation.

> > of the format we can simply use the CIO2 device's capture-capable devnode as the
> > source of the image data.
> >
> > To facilitate that, I'm identifying sensors which don't have a format that needs
> > the Imgu and simply skipping the Imgu/IPA processing in the IPU3 pipeline for
> > them.
> >
> > What I don't particularly like about the way that I've done things here is that
> > the stream only works for the Raw role, which feels wrong. The advantage is that
> > it's contained within the existing IPU3 pipeline without too much disruption,
> > but I do wonder whether a separate CIO2-only pipeline might not be better. In
> > that case PipelineHandlerIPU3::match() would need amending to check whether the
> > sensor for each "ipu3-csi-2 n" instance needed hooking into the Imgu and not adding
> > those to the device match if it found that they didn't need it, so that a new
> > CIO2-only pipeline could claim the media devices. I'd love to hear other people's
> > thoughts about the best way to handle this.
> >
> > Thanks
> > Dan
> >
> > Daniel Scally (11):
> >   pipeline: ipu3: Check if sensor supports test pattern control
> >   include: drm_fourcc: Add Y10 format
> >   libcamera/formats: Add IPU3_Y10 format
> >   pipeline: ipu3: Identify sensors that do not need the Imgu
> >   pipeline: ipu3: Add needsImgu flag to IPU3Frames
> >   pipeline: ipu3: Support sensors using only CIO2
> >   pipeline: ipu3: Allow raw-only streams in IPU3 pipeline
> >   pipeline: ipu3: Support IPU3_Y10 format
> >   apps: qcam: Add support for IPU3_Y10
> >   apps: qcam: Remove restriction on raw-only streams
> >   pipeline: ipu3: Increase maximum connected cameras
> >
> >  include/linux/drm_fourcc.h             |   3 +
> >  src/apps/qcam/format_converter.cpp     |  50 +++++
> >  src/apps/qcam/format_converter.h       |   2 +
> >  src/apps/qcam/main_window.cpp          |   6 -
> >  src/libcamera/formats.cpp              |  10 +
> >  src/libcamera/formats.yaml             |   3 +
> >  src/libcamera/pipeline/ipu3/cio2.cpp   |  21 +++
> >  src/libcamera/pipeline/ipu3/cio2.h     |   4 +
> >  src/libcamera/pipeline/ipu3/frames.cpp |  45 +++--
> >  src/libcamera/pipeline/ipu3/frames.h   |   3 +-
> >  src/libcamera/pipeline/ipu3/ipu3.cpp   | 242 ++++++++++++++-----------
> >  11 files changed, 255 insertions(+), 134 deletions(-)