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

[Jai Luthra]

Hi,

Thanks for the review.

On Jul 10, 2024 at 11:55:59 +0100, Kieran Bingham wrote:
> 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?

No, the raw bayer formats are not exposed to the user with SoftISP 
enabled. Only the SwISP post-processed BGR/RGB output.

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

I am not sure where exactly that is happening in the code, but I do see 
these logs:

[0:19:22.063346150] [1269]  INFO Debayer debayer_cpu.cpp:298 Unsupported input format XBGR8888
[0:19:22.425591115] [1269]  INFO Debayer debayer_cpu.cpp:298 Unsupported input format RGB565
[0:19:22.426594635] [1269]  INFO Debayer debayer_cpu.cpp:298 Unsupported input format UYVY

So I assume the pipeline handler passes through any RGB/YUV pixfmt that 
the SwISP does not support as an input.


> 
> 
> > > Here are the logs of `cam -c1 -I`: https://0x0.st/XmOS.txt

Full logs with OV5640 are here ^

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

Ah okay, yes that seems fine for now.

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

-- 
Thanks,
Jai

GPG Fingerprint: 4DE0 D818 E5D5 75E8 D45A AFC5 43DE 91F9 249A 7145