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

[Milan Zamazal]

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

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