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

[Milan Zamazal]

Kieran Bingham <kieran.bingham at ideasonboard.com> writes:

> 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.

Yes, what I meant was just that with making it configurable we don't
have to decide which boolean value should be unconditionally imposed on
a pipeline.

> 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.

Is anybody going to work on this (maybe Andrei IIRC?)?

> 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
>>