[libcamera-devel] [PATCH] libcamera: pipeline: Add IMX8 ISI pipeline

[Jacopo Mondi]

Hi Laurent,

[snip]

On Fri, Nov 11, 2022 at 11:39:15AM +0200, Laurent Pinchart wrote:
> > > > > +                       .sink_stream = 0,
> > > > > +                       .source_pad = static_cast<unsigned int>(3 + idx),
> > >
> > > The source pad number should be computed dynamically to avoid hardcoding
> > > the number of sink pads. One option is crossbar_->pads().size() / 2 + 1
> > > + idx, another one is to base the computation on the number of streams.
> >
> > So we now have 2 CSI-2 sinks and one sink for the memory interface.
> > We have 2 sources for each ISI instances.
> >
> > Do we know what are the combinations in other SoCs ? I understand
> > there might be more ISI instances, but will there be more sinks too ?
>
> All the ones I've seen have N live inputs, one memory input, and N
> processing pipelines, with N being 1, 2 or 6 (if I recall correctly).
> The crossbar switch always orders pads as live inputs, memory input,
> outputs, and the memory input can only be used with the first processing
> pipeline.
>

We could also store these numbers in the per-SoC info structures, even
if autodiscovery would make supporting new SoCs easier, once
implemented..

> > I'll add a todo
> >
> > > Pipeline allocation will need to be made dynamic, but that's for later.
> > >
> > > > > +                       .source_stream = 0,
> > > > > +                       .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> > > > > +                       .reserved = {}
> > > > > +               };
> > > > > +
> > > > > +               routing.push_back(route);
> > > > > +       }
> > > > > +
> > > > > +       ret = crossbar_->setRouting(&routing, V4L2Subdevice::ActiveFormat);
> > > > > +       if (ret)
> > > > > +               return ret;
> > > > > +
> > > > > +       /* Apply format to the sensor and CSIS receiver. */
> > > > > +       V4L2SubdeviceFormat sensorFmt = camConfig->sensorFormat_;
> > >
> > > s/sensorFmt/format/ as it's used through the function for all formats.
> > >
> > > > > +       ret = sensor->setFormat(&sensorFmt);
> > > > > +       if (ret)
> > > > > +               return ret;
> > > > > +
> > > > > +       ret = data->csis->setFormat(0, &sensorFmt);
> > > > > +       if (ret)
> > > > > +               return ret;
> > > > > +
> > > > > +       ret = data->csis->setFormat(1, &sensorFmt);
> > >
> > > This could be skipped, as the CSIS driver will propagate the format
> > > internally.
> > >
> > > > > +       if (ret)
> > > > > +               return ret;
> > > > > +
> > > > > +       ret = crossbar_->setFormat(data->id_, &sensorFmt);
> > > > > +       if (ret)
> > > > > +               return ret;
> > > > > +
> > > >
> > > > This makes me wonder about Naush's MediaWalk proposal earlier this year.
> > > > Seems we could likely do with some helpers for all the media graph
> > > > options in each pipeline handler.
> > >
> > > Fully agreed. That's an area where libcamera can really help, by
> > > providing good helpers.
> > >
> > > > But ... that's not something that will happen for this patch.
> > > >
> > > > > +       /* Now configure the ISI and video node instances, one per stream. */
> > > > > +       data->enabledStreams_.clear();
> > > > > +       for (const auto &config : *c) {
> > > > > +               Pipe *pipe = pipeFromStream(camera, config.stream());
> > > > > +
> > > > > +               /*
> > > > > +                * Set the format on the ISI sink pad: it must match what is
> > > > > +                * received by the CSIS.
> > > > > +                */
> > > > > +               ret = pipe->isi->setFormat(0, &sensorFmt);
> > > > > +               if (ret)
> > > > > +                       return ret;
> > > > > +
> > > > > +               /*
> > > > > +                * Configure the ISI sink compose rectangle to downscale the
> > > > > +                * image.
> > > > > +                *
> > > > > +                * \todo Additional cropping could be applied on the ISI source
> > > > > +                * pad to further downscale the image.
> > > >
> > > > Cropping ? or downscaling?
> > > >
> > > > Aside from the comments, I don't have any specific objections - and I
> > > > expect more things can develop on top.
> > > >
> > > >
> > > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > >
> > > > > +                */
> > > > > +               Rectangle isiScale = {};
> > > > > +               isiScale.x = 0;
> > > > > +               isiScale.y = 0;
> > > > > +               isiScale.width = config.size.width;
> > > > > +               isiScale.height = config.size.height;
> > >
> > > 			Rectangle isiScale{ config.size };
> > >
> > > > > +
> > > > > +               ret = pipe->isi->setSelection(0, V4L2_SEL_TGT_COMPOSE, &isiScale);
> > > > > +               if (ret)
> > > > > +                       return ret;
> > > > > +
> > > > > +               /*
> > > > > +                * Set the format on ISI source pad: only the media bus code
> > > > > +                * is relevant as it configures format conversion, while the
> > > > > +                * size is taken from the sink's COMPOSE (or source's CROP,
> > > > > +                * if any) rectangles.
> > > > > +                */
> > > > > +               auto fmtMap = ISICameraConfiguration::formatsMap.find(config.pixelFormat);
> > > > > +               ISICameraConfiguration::PipeFormat pipeFormat = fmtMap->second;
> > >
> > > 		const ISICameraConfiguration::PipeFormat &pipeFormat =
> > > 			ISICameraConfiguration::formatsMap[config.pixelFormat];
> > >
> > > > > +
> > > > > +               V4L2SubdeviceFormat isiFormat{};
> > > > > +               isiFormat.mbus_code = pipeFormat.isiCode;
> > > > > +               isiFormat.size = config.size;
> > > > > +
> > > > > +               ret = pipe->isi->setFormat(1, &isiFormat);
> > > > > +               if (ret)
> > > > > +                       return ret;
> > > > > +
> > > > > +               V4L2DeviceFormat captureFmt{};
> > > > > +               captureFmt.fourcc = pipeFormat.fourcc;
> >
> > I've also noticed there's no need to store the 4cc code associated
> > with a pixelformat as we can rely on the V4L2 video device doing the
> > conversion for us
> >
> > 		captureFmt.fourcc = pipe->capture->toV4L2PixelFormat(fmtMap->first);
>
> Storing the V4L2 pixel format in formatsMap would avoid another lookup,
> but I don't mind using toV4L2PixelFormat().
>

I went for poking the V4L2 device when I realized I didn't know if I
should have used the single-planar or multiplanar version of YUV422
for still capture. I assumed asking the video device would have
returned the correct one.

I should have looked in the driver and realize I should have gone for
NVxyM or YUV444M as YUV422 is not supported.

> > > > > +               captureFmt.size = config.size;
> > >
> > > You should also set the stride and image size. A todo comment will do
> > > for now.
> >