[PATCH] libcamera: pipeline: simple: Enable Soft ISP for TI CSI-RX

[Kieran Bingham]

Quoting Milan Zamazal (2024-07-10 11:11:12)
> Hi Jai,
> 
> thank you for the patch.
> 
> Jai Luthra <j-luthra at ti.com> writes:
> 
> > Hi Laurent, Umang,
> >
> > On Jun 27, 2024 at 17:40:27 +0300, Laurent Pinchart wrote:
> >> On Thu, Jun 27, 2024 at 07:11:22PM +0530, Umang Jain wrote:
> >> > On 27/06/24 2:09 pm, Jai Luthra wrote:
> >> > > The j721e-csi2rx driver pipeline uses no converters, so enable the
> >> > > software ISP plugin support. This is handy for boards with AM62 SoC
> >> > > (like BeaglePlay) that have no HW ISP.
> >> > >
> >> > > Tested with IMX519 on SK-AM62 running a kernel built with dmabuf heap
> >> > > support.
> >> > >
> >> > > Signed-off-by: Jai Luthra <j-luthra at ti.com>
> >> > 
> >> > Probably worth having a Tested-by: Tag here as well. Just provide one on 
> >> > the thread, if you have tested it.
> >
> > Ah okay. I thought those tags are for others and not for patch 
> > submitter. But yes I have tested it so,
> >
> > Tested-by: Jai Luthra <j-luthra at ti.com>
> 
> I have tested it with SK-AM69 + IMX219:
> 
> Tested-by: Milan Zamazal <mzamazal at redhat.com>
> 
> >> I generally assume that, unless otherwise noted, patch submitters will
> >> have arranged for code to be tested :-)
> >> 
> >> Is there a risk of regression for working use cases by enabling the soft
> >> ISP ? I'm thinking in particular of losing the ability to capture images
> >> from YUV sensors (if the simple pipeline handler doesn't skip the soft
> >> ISP in that case),
> >
> > I have tested this with OV5640, which supports ending both raw bayer 
> > formats, and converted YUV/RGB using the in-sensor ISP.
> >
> > With the SwISP enabled, cam tool lets me capture both the 
> > YUV/XBGR/RGB565 directly coming from sensor, or RGB/BGR converted from 
> > the raw bayer.

Did I miss the raw stream being added? Does this already work that the
raw can be configured and accessed while the SoftISP is enabled?

On the X13s I do not see unprocessed streams exposed, only the SoftISP
formats.


How do YUV formats get passed through without being handled in the
SoftISP here?


> > Here are the logs of `cam -c1 -I`: https://0x0.st/XmOS.txt
> >
> >
> >> and the ability to capture raw images from Bayer
> >> sensors. The latter is likely less of an issue, it could probably be
> >> implemented a bit later if it's not there yet.
> >
> > This is true, and I personally do use cam tool to capture raw bayer 
> > data, as it is more convenient compared to using v4l2-ctl/yavta - given 
> > it sets the routes and formats on all subdevs for me.
> >
> > It would be great to be able to be able to choose that through either 
> > CLI options or the config file support Milan proposed. But I wouldn't 
> > call missing that a regression, as actual end-users of a camera almost 
> > always want a usable RGB image :)
> 
> I'm inclined to this opinion too.  And yes, my config file patches are
> posted, updated and solving the true/false problem for all the
> pipelines, just waiting for reviews. :-)

I don't object to merging this patch - I do think it's probably more
demonstrative of the purpose of libcamera, and the raw captures can
already be captured through other tools (with some additional
configuration) - but I don't actually think the configuration options
remove the bool enablement for the soft-isp - it just 'moves' it.

The only way we can remove the softisp enable flag is if the
simple-pipeline handler exposes the raw stream which doesn't process
through the SoftISP.


Anyway, for this patch:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


> 
> >> > Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> >> > 
> >> > > ---
> >> > >   src/libcamera/pipeline/simple/simple.cpp | 2 +-
> >> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> >> > >
> >> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> >> > > index eb36578e..812c492e 100644
> >> > > --- a/src/libcamera/pipeline/simple/simple.cpp
> >> > > +++ b/src/libcamera/pipeline/simple/simple.cpp
> >> > > @@ -198,7 +198,7 @@ namespace {
> >> > >   static const SimplePipelineInfo supportedDevices[] = {
> >> > >          { "dcmipp", {}, false },
> >> > >          { "imx7-csi", { { "pxp", 1 } }, false },
> >> > > -        { "j721e-csi2rx", {}, false },
> >> > > +        { "j721e-csi2rx", {}, true },
> >> > >          { "mtk-seninf", { { "mtk-mdp", 3 } }, false },
> >> > >          { "mxc-isi", {}, false },
> >> > >          { "qcom-camss", {}, true },
> >> 
> >> -- 
> >> Regards,
> >> 
> >> Laurent Pinchart
>