Hi Laurent,
Thanks for the review.
On Thu, Dec 13, 2018 at 12:41:36PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Thursday, 13 December 2018 11:38:26 EET Sakari Ailus wrote:
> > Hi Laurent,
> >
> > I'm sending a separate patch to address the comments.
> >
> > On Fri, Nov 30, 2018 at 12:50:36AM +0200, Laurent Pinchart wrote:
> > > On Tuesday, 30 October 2018 00:22:56 EET Yong Zhi wrote:
> > >> From: Rajmohan Mani <rajmohan.mani@xxxxxxxxx>
> > >>
> > >> This patch adds the details about the IPU3 Imaging Unit driver.
> > >
> > > Strictly speaking this documents both the CIO2 and the IMGU. As they're
> > > handled by two separate drivers, should they be split in two separate
> > > files ? If you prefer keeping them together you should update the commit
> > > message accordingly. I would in that case also split the documentation in
> > > a CIO2 and a IMGU section in the file, instead of mixing them.
> >
> > I'm keeping it in a single document for now. In practice these devices
> > always come together as neither is really usable without the other.
>
> But they're still two separate devices. Splitting the documentation would
> clarify which part is associated with each device. CIO2 and ImgU instructions
> are currently interleaved and that's very confusing. If you really want to
> keep everything in one file, the CIO2 and ImgU parts should be deinterleaved,
> and the CIO2 should come first.
That's implemented in the patch I submitted.
>
> > >> Change-Id: I560cecf673df2dcc3ec72767cf8077708d649656
> > >
> > > The Change-Id: tag isn't suitable for mainline, you can drop it.
> >
> > Fixed.
> >
> > >> Signed-off-by: Rajmohan Mani <rajmohan.mani@xxxxxxxxx>
> > >> ---
> > >>
> > >> Documentation/media/v4l-drivers/index.rst | 1 +
> > >> Documentation/media/v4l-drivers/ipu3.rst | 326 ++++++++++++++++++++++++
> > >> 2 files changed, 327 insertions(+)
> > >> create mode 100644 Documentation/media/v4l-drivers/ipu3.rst
>
> [snip]
>
> > >> diff --git a/Documentation/media/v4l-drivers/ipu3.rst
> > >> b/Documentation/media/v4l-drivers/ipu3.rst new file mode 100644
> > >> index 0000000..045bf42
> > >> --- /dev/null
> > >> +++ b/Documentation/media/v4l-drivers/ipu3.rst
>
> [snip]
>
> > >> +Media controller
> > >> +----------------
> > >> +
> > >> +The media device interface allows to configure the ImgU links, which
> > >> defines +the behavior of the IPU3 firmware.
> > >
> > > s/defines/define/ or possibly better s/defines/control/
> >
> > I'm removing the section as the binary is selected using a control in the
> > current version. I'm adding this instead:
> >
> > Firmware binary selection
> > -------------------------
> >
> > The firmware binary is selected using the V4L2_CID_INTEL_IPU3_MODE,
> > currently defined in drivers/staging/media/ipu3/include/intel-ipu3.h .
> > "VIDEO" and "STILL" modes are available.
>
> Do we need to expose the fact that they use different firmwares ? Or should we
> instead document the two modes of operation, and explain that they need to be
> selected before anything else ? In any case modes need to be documented.
I agree. I'll add this to the TODO file.
> Interestingly enough, the driver code includes
>
> enum ipu3_css_pipe_id {
> IPU3_CSS_PIPE_ID_PREVIEW,
> IPU3_CSS_PIPE_ID_COPY,
> IPU3_CSS_PIPE_ID_VIDEO,
> IPU3_CSS_PIPE_ID_CAPTURE,
> IPU3_CSS_PIPE_ID_YUVPP,
> IPU3_CSS_PIPE_ID_ACC,
> IPU3_CSS_PIPE_ID_NUM
> };
>
> but seems to only support the video and capture modes.
I wonder if the others exist in the firmware binary. Other devices with
e.g. integrated CSI-2 receivers have been supported using the same source
code base I presume.
>
> > >> +
> > >> +Device operation
> > >> +----------------
> > >> +
> > >> +With IPU3, once the input video node ("ipu3-imgu 0/1":0,
> > >> +in <entity>:<pad-number> format) is queued with buffer (in packed raw
> > >> bayer
> > >
> > > s/bayer/Bayer/
> >
> > Fixed in the entire file.
> >
> > >> +format), IPU3 ISP starts processing the buffer and produces the video
> > >
> > > s/IPU3 ISP/the IPU3 ISP/
> > >
> > > This is the first time you mention an ISP. Should the term ISP be replaced
> > > by ImgU here and below ? I'm fine keeping it, but it should then be
> > > defined in the introduction, in particular with an explanation of the
> > > difference between ImgU and ISP.
> >
> > I replaced IPU3 with ImgU in this section.
> >
> > >> output +in YUV format and statistics output on respective output nodes.
> > >> The driver +is expected to have buffers ready for all of parameter,
> > >> output and +statistics nodes, when input video node is queued with
> > >> buffer.
> > >
> > > Why is that, shouldn't the driver wait for all necessary buffers to be
> > > ready before processing ?
> >
> > Not all are mandatory.
>
> Which ones are mandatory, and which ones are not ? V4L2 doesn't enforce buffer
> queuing order for M2M devices, it's a very bad idea to do so here. It would
> make usage of the driver impossible with separate unsynchronized processes for
> different queues (which is typically the case when using command line test
> tools).
How about this:
- Document different operation modes, and which buffer queues are relevant
in each mode. To process an image, which queues require a buffer an in
which ones is it optional?
>
> > >> +At a minimum, all of input, main output, 3A statistics and viewfinder
> > >> +video nodes should be enabled for IPU3 to start image processing.
> > >
> > > If they all need to be enabled, shouldn't the respective links be ENABLED
> > > and IMMUTABLE ?
> >
> > Yes.
>
> Could you please capture this in the TODO file ?
>
> [snip]
>
> > >> +Configuring the Intel IPU3
> > >> +==========================
> > >> +
> > >> +The Intel IPU3 ImgU driver supports V4L2 interface. Using V4L2 ioctl
> > >> calls, +the ISP can be configured and enabled.
> > >> +
> > >> +The IPU3 ImgU pipelines can be configured using media controller APIs,
> > >> +defined at :ref:`media_controller`.
> > >> +
> > >> +Capturing frames in raw bayer format
> > >> +------------------------------------
> > >> +
> > >> +IPU3 MIPI CSI2 receiver is used to capture frames (in packed raw bayer
> > >> +format) from the raw sensors connected to the CSI2 ports. The captured
> > >> +frames are used as input to the ImgU driver.
> > >> +
> > >> +Image processing using IPU3 ImgU requires tools such as v4l2n [#f1]_,
> > >
> > > I would drop v4l2n from the documentation as it's not maintained and is
> > > not functional (in particular it doesn't implement MPLANE support which
> > > the driver requires).
> >
> > Ack. I'm leaving the command plus the reference to v4l2n, this might
> > contain information useful still.
>
> Who will address this TODO item ? :-)
I don't think we need to name anyone at this point. Presumably someone from
Intel.
>
> > >> +raw2pnm [#f1]_, and yavta [#f2]_ due to the following unique
> > >> requirements
> > >> +and / or features specific to IPU3.
>
> [snip]
>
> > >> +Processing the image in raw bayer format
> > >> +----------------------------------------
> > >> +
> > >> +Configuring ImgU V4L2 subdev for image processing
> > >> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> [snip]
>
> > >> +RAW bayer frames go through the following ISP pipeline HW blocks to
> > >> +have the processed image output to the DDR memory.
> > >> +
> > >> +RAW bayer frame -> Input Feeder -> Bayer Down Scaling (BDS) ->
> > >> Geometric +Distortion Correction (GDC) -> DDR
> > >
> > > A more detailed block diagram, with the other blocks included, should be
> > > added to the ImgU description above. Each block should have a short
> > > description of its purpose.
> >
> > I think we have one in conjunction with the parameter format description.
>
> You're right. As mentioned in my review for that patch, I think it should be
> moved to this document.
Fair enough. Let's polish this more later.
--
Regards,
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx
