libcamera: pipeline: simple: Enable Soft ISP for TI CSI-RX
diff mbox series

Message ID 20240627083938.554370-1-j-luthra@ti.com
State New
Headers show
Series
  • libcamera: pipeline: simple: Enable Soft ISP for TI CSI-RX
Related show

Commit Message

Jai Luthra June 27, 2024, 8:39 a.m. UTC
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@ti.com>
---
 src/libcamera/pipeline/simple/simple.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Umang Jain June 27, 2024, 1:41 p.m. UTC | #1
Hi Jai,

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@ti.com>

Probably worth having a Tested-by: Tag here as well. Just provide one on 
the thread, if you have tested it.

Reviewed-by: Umang Jain <umang.jain@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 },
Laurent Pinchart June 27, 2024, 2:40 p.m. UTC | #2
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@ti.com>
> 
> Probably worth having a Tested-by: Tag here as well. Just provide one on 
> the thread, if you have tested it.

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

> Reviewed-by: Umang Jain <umang.jain@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 },
Robert Mader June 27, 2024, 2:43 p.m. UTC | #3
On 27.06.24 16:40, Laurent Pinchart wrote:
> I'm thinking in particular of losing the ability to capture images
> from YUV sensors

That shouldn't be the case - only capturing raw bayer should be affected 
(I had to explicitly disable non-bayer formats on the PP for postmarket 
to enforce usage of the softISP).
Jai Luthra June 28, 2024, 5:42 a.m. UTC | #4
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@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@ti.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 :)

> 
> > Reviewed-by: Umang Jain <umang.jain@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
Milan Zamazal July 10, 2024, 10:11 a.m. UTC | #5
Hi Jai,

thank you for the patch.

Jai Luthra <j-luthra@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@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@ti.com>

I have tested it with SK-AM69 + IMX219:

Tested-by: Milan Zamazal <mzamazal@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@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
Kieran Bingham July 10, 2024, 10:55 a.m. UTC | #6
Quoting Milan Zamazal (2024-07-10 11:11:12)
> Hi Jai,
> 
> thank you for the patch.
> 
> Jai Luthra <j-luthra@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@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@ti.com>
> 
> I have tested it with SK-AM69 + IMX219:
> 
> Tested-by: Milan Zamazal <mzamazal@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@ideasonboard.com>


> 
> >> > Reviewed-by: Umang Jain <umang.jain@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
>
Jai Luthra July 10, 2024, 11:44 a.m. UTC | #7
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@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@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@ti.com>
> > 
> > I have tested it with SK-AM69 + IMX219:
> > 
> > Tested-by: Milan Zamazal <mzamazal@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@ideasonboard.com>
> 
> 
> > 
> > >> > Reviewed-by: Umang Jain <umang.jain@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
> >
Milan Zamazal July 10, 2024, 12:19 p.m. UTC | #8
Kieran Bingham <kieran.bingham@ideasonboard.com> writes:

> Quoting Milan Zamazal (2024-07-10 11:11:12)
>> Hi Jai,
>> 
>
>> thank you for the patch.
>> 
>> Jai Luthra <j-luthra@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@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@ti.com>
>> 
>> I have tested it with SK-AM69 + IMX219:
>> 
>> Tested-by: Milan Zamazal <mzamazal@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@ideasonboard.com>
>
>
>> 
>> >> > Reviewed-by: Umang Jain <umang.jain@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
>>

Patch
diff mbox series

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