[v2,1/1] pipeline: rpi: pisp: Ensure we don't run our SW correction with packed
diff mbox series

Message ID 20260630090215.813311-2-naush@raspberrypi.com
State Accepted
Headers show
Series
  • RPi pipeline handler fixes
Related show

Commit Message

Naushir Patuck June 30, 2026, 9 a.m. UTC 
Due to hardware bugs in the CSI-2 RX, we need to unpack 14-bit CSI-2
packed samples and swap endian order in 16-bit samples both in software.
These SW routines only work on unpacked/uncompresssed samples.

For this reason, ensure the user cannot configure 14/16-bit RAW streams
with PiSP Bayer compression in platformValidate().

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/libcamera/pipeline/rpi/pisp/pisp.cpp | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Paul Elder July 6, 2026, 9:03 a.m. UTC | #1 
Hi Naush,

Thanks for the patch.

Quoting Naushir Patuck (2026-06-30 18:00:20)
> Due to hardware bugs in the CSI-2 RX, we need to unpack 14-bit CSI-2
> packed samples and swap endian order in 16-bit samples both in software.
> These SW routines only work on unpacked/uncompresssed samples.
> 
> For this reason, ensure the user cannot configure 14/16-bit RAW streams
> with PiSP Bayer compression in platformValidate().
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Looks good to me.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  src/libcamera/pipeline/rpi/pisp/pisp.cpp | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> index b744c901f751..8e8da116ef6f 100644
> --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> @@ -1223,6 +1223,17 @@ PiSPCameraData::platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const
>                         status = CameraConfiguration::Adjusted;
>                 }
>  
> +               unsigned bpp = MediaBusFormatInfo::info(rpiConfig->sensorFormat_.code).bitsPerPixel;
> +               if ((bpp == 16 || bpp == 14) &&
> +                   bayer.packing != BayerFormat::Packing::None) {
> +                       LOG(RPI, Info)
> +                               << "The sensor is configured for a 16/14-bit output, "
> +                               << "compression is not available for SW fixups needed.";
> +                       bayer.packing = BayerFormat::Packing::None;
> +                       rawStream->pixelFormat = bayer.toPixelFormat();
> +                       status = CameraConfiguration::Adjusted;
> +               }
> +
>                 rawStreams[0].format =
>                         RPi::PipelineHandlerBase::toV4L2DeviceFormat(cfe_[Cfe::Output0].dev(), rawStream);
>  
> -- 
> 2.53.0
>
Laurent Pinchart July 6, 2026, 11 a.m. UTC | #2 
On Tue, Jun 30, 2026 at 10:00:20AM +0100, Naushir Patuck wrote:
> Due to hardware bugs in the CSI-2 RX, we need to unpack 14-bit CSI-2
> packed samples and swap endian order in 16-bit samples both in software.
> These SW routines only work on unpacked/uncompresssed samples.
> 
> For this reason, ensure the user cannot configure 14/16-bit RAW streams
> with PiSP Bayer compression in platformValidate().
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/libcamera/pipeline/rpi/pisp/pisp.cpp | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> index b744c901f751..8e8da116ef6f 100644
> --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> @@ -1223,6 +1223,17 @@ PiSPCameraData::platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const
>  			status = CameraConfiguration::Adjusted;
>  		}
>  
> +		unsigned bpp = MediaBusFormatInfo::info(rpiConfig->sensorFormat_.code).bitsPerPixel;
> +		if ((bpp == 16 || bpp == 14) &&
> +		    bayer.packing != BayerFormat::Packing::None) {
> +			LOG(RPI, Info)
> +				<< "The sensor is configured for a 16/14-bit output, "
> +				<< "compression is not available for SW fixups needed.";

I still can't parse the message:

"The sensor is configured for a 16/14-bit output, compression is not available for SW fixups needed."

> +			bayer.packing = BayerFormat::Packing::None;
> +			rawStream->pixelFormat = bayer.toPixelFormat();
> +			status = CameraConfiguration::Adjusted;
> +		}
> +
>  		rawStreams[0].format =
>  			RPi::PipelineHandlerBase::toV4L2DeviceFormat(cfe_[Cfe::Output0].dev(), rawStream);
>
Naushir Patuck July 6, 2026, 11:06 a.m. UTC | #3 
On Mon, 6 Jul 2026 at 12:00, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Tue, Jun 30, 2026 at 10:00:20AM +0100, Naushir Patuck wrote:
> > Due to hardware bugs in the CSI-2 RX, we need to unpack 14-bit CSI-2
> > packed samples and swap endian order in 16-bit samples both in software.
> > These SW routines only work on unpacked/uncompresssed samples.
> >
> > For this reason, ensure the user cannot configure 14/16-bit RAW streams
> > with PiSP Bayer compression in platformValidate().
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  src/libcamera/pipeline/rpi/pisp/pisp.cpp | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> > index b744c901f751..8e8da116ef6f 100644
> > --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> > +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> > @@ -1223,6 +1223,17 @@ PiSPCameraData::platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const
> >                       status = CameraConfiguration::Adjusted;
> >               }
> >
> > +             unsigned bpp = MediaBusFormatInfo::info(rpiConfig->sensorFormat_.code).bitsPerPixel;
> > +             if ((bpp == 16 || bpp == 14) &&
> > +                 bayer.packing != BayerFormat::Packing::None) {
> > +                     LOG(RPI, Info)
> > +                             << "The sensor is configured for a 16/14-bit output, "
> > +                             << "compression is not available for SW fixups needed.";
>
> I still can't parse the message:
>
> "The sensor is configured for a 16/14-bit output, compression is not available for SW fixups needed."

How about:

LOG(RPI, Info)
        << "The sensor is configured for "
        << bpp
        << "-bit pixel readout. PISP compression will be unavilable
because of the"
        << " software fixups necessary in this format."


>
> > +                     bayer.packing = BayerFormat::Packing::None;
> > +                     rawStream->pixelFormat = bayer.toPixelFormat();
> > +                     status = CameraConfiguration::Adjusted;
> > +             }
> > +
> >               rawStreams[0].format =
> >                       RPi::PipelineHandlerBase::toV4L2DeviceFormat(cfe_[Cfe::Output0].dev(), rawStream);
> >
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart July 6, 2026, 11:59 a.m. UTC | #4 
On Mon, Jul 06, 2026 at 12:06:11PM +0100, Naushir Patuck wrote:
> On Mon, 6 Jul 2026 at 12:00, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > On Tue, Jun 30, 2026 at 10:00:20AM +0100, Naushir Patuck wrote:
> > > Due to hardware bugs in the CSI-2 RX, we need to unpack 14-bit CSI-2
> > > packed samples and swap endian order in 16-bit samples both in software.
> > > These SW routines only work on unpacked/uncompresssed samples.
> > >
> > > For this reason, ensure the user cannot configure 14/16-bit RAW streams
> > > with PiSP Bayer compression in platformValidate().
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > > ---
> > >  src/libcamera/pipeline/rpi/pisp/pisp.cpp | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> > > index b744c901f751..8e8da116ef6f 100644
> > > --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> > > +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> > > @@ -1223,6 +1223,17 @@ PiSPCameraData::platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const
> > >                       status = CameraConfiguration::Adjusted;
> > >               }
> > >
> > > +             unsigned bpp = MediaBusFormatInfo::info(rpiConfig->sensorFormat_.code).bitsPerPixel;
> > > +             if ((bpp == 16 || bpp == 14) &&
> > > +                 bayer.packing != BayerFormat::Packing::None) {
> > > +                     LOG(RPI, Info)
> > > +                             << "The sensor is configured for a 16/14-bit output, "
> > > +                             << "compression is not available for SW fixups needed.";
> >
> > I still can't parse the message:
> >
> > "The sensor is configured for a 16/14-bit output, compression is not available for SW fixups needed."
> 
> How about:
> 
> LOG(RPI, Info)
>         << "The sensor is configured for "
>         << bpp
>         << "-bit pixel readout. PISP compression will be unavilable because of the"
>         << " software fixups necessary in this format."

s/unavilable/unvailable/

I can understand that :-)

I'd write "Disabling PISP compression" instead of "PISP compression will
be unavailable", to be clear.

			LOG(RPI, Info)
 				<< "The sensor is configured for " << bpp
				<< "-bit pixel readout. Disabling PISP compression because of the"
				<< " software fixups necessary in this format."

We could also shorten it:

			LOG(RPI, Info)
			        << "Sensor configured in RAW" << bpp
			        << ", disabling PISP compression incompatible with required software fixups."

or

			LOG(RPI, Info)
			        << "PISP compression incompatible with software fixups required for RAW"
				<< bpp << ", disabling"

With one of those messages,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > > +                     bayer.packing = BayerFormat::Packing::None;
> > > +                     rawStream->pixelFormat = bayer.toPixelFormat();
> > > +                     status = CameraConfiguration::Adjusted;
> > > +             }
> > > +
> > >               rawStreams[0].format =
> > >                       RPi::PipelineHandlerBase::toV4L2DeviceFormat(cfe_[Cfe::Output0].dev(), rawStream);
> > >
Naushir Patuck July 6, 2026, 1:51 p.m. UTC | #5 
On Mon, 6 Jul 2026 at 12:59, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> On Mon, Jul 06, 2026 at 12:06:11PM +0100, Naushir Patuck wrote:
> > On Mon, 6 Jul 2026 at 12:00, Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > >
> > > On Tue, Jun 30, 2026 at 10:00:20AM +0100, Naushir Patuck wrote:
> > > > Due to hardware bugs in the CSI-2 RX, we need to unpack 14-bit CSI-2
> > > > packed samples and swap endian order in 16-bit samples both in
> software.
> > > > These SW routines only work on unpacked/uncompresssed samples.
> > > >
> > > > For this reason, ensure the user cannot configure 14/16-bit RAW
> streams
> > > > with PiSP Bayer compression in platformValidate().
> > > >
> > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > > > ---
> > > >  src/libcamera/pipeline/rpi/pisp/pisp.cpp | 11 +++++++++++
> > > >  1 file changed, 11 insertions(+)
> > > >
> > > > diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> > > > index b744c901f751..8e8da116ef6f 100644
> > > > --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> > > > +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> > > > @@ -1223,6 +1223,17 @@
> PiSPCameraData::platformValidate(RPi::RPiCameraConfiguration *rpiConfig)
> const
> > > >                       status = CameraConfiguration::Adjusted;
> > > >               }
> > > >
> > > > +             unsigned bpp =
> MediaBusFormatInfo::info(rpiConfig->sensorFormat_.code).bitsPerPixel;
> > > > +             if ((bpp == 16 || bpp == 14) &&
> > > > +                 bayer.packing != BayerFormat::Packing::None) {
> > > > +                     LOG(RPI, Info)
> > > > +                             << "The sensor is configured for a
> 16/14-bit output, "
> > > > +                             << "compression is not available for
> SW fixups needed.";
> > >
> > > I still can't parse the message:
> > >
> > > "The sensor is configured for a 16/14-bit output, compression is not
> available for SW fixups needed."
> >
> > How about:
> >
> > LOG(RPI, Info)
> >         << "The sensor is configured for "
> >         << bpp
> >         << "-bit pixel readout. PISP compression will be unavilable
> because of the"
> >         << " software fixups necessary in this format."
>
> s/unavilable/unvailable/
>
> I can understand that :-)
>
> I'd write "Disabling PISP compression" instead of "PISP compression will
> be unavailable", to be clear.
>
>                         LOG(RPI, Info)
>                                 << "The sensor is configured for " << bpp
>                                 << "-bit pixel readout. Disabling PISP
> compression because of the"
>                                 << " software fixups necessary in this
> format."
>
> We could also shorten it:
>
>                         LOG(RPI, Info)
>                                 << "Sensor configured in RAW" << bpp
>                                 << ", disabling PISP compression
> incompatible with required software fixups."
>
> or
>
>                         LOG(RPI, Info)
>                                 << "PISP compression incompatible with
> software fixups required for RAW"
>                                 << bpp << ", disabling"
>
>
I'm good with either.  Would you be able to make the change before merging,
or should I post a follow-on patch?

Naush




> With one of those messages,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > > > +                     bayer.packing = BayerFormat::Packing::None;
> > > > +                     rawStream->pixelFormat = bayer.toPixelFormat();
> > > > +                     status = CameraConfiguration::Adjusted;
> > > > +             }
> > > > +
> > > >               rawStreams[0].format =
> > > >
>  RPi::PipelineHandlerBase::toV4L2DeviceFormat(cfe_[Cfe::Output0].dev(),
> rawStream);
> > > >
>
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart July 8, 2026, 9:52 p.m. UTC | #6 
On Mon, Jul 06, 2026 at 02:51:58PM +0100, Naushir Patuck wrote:
> On Mon, 6 Jul 2026 at 12:59, Laurent Pinchart wrote:
> > On Mon, Jul 06, 2026 at 12:06:11PM +0100, Naushir Patuck wrote:
> > > On Mon, 6 Jul 2026 at 12:00, Laurent Pinchart wrote:
> > > > On Tue, Jun 30, 2026 at 10:00:20AM +0100, Naushir Patuck wrote:
> > > > > Due to hardware bugs in the CSI-2 RX, we need to unpack 14-bit CSI-2
> > > > > packed samples and swap endian order in 16-bit samples both in software.
> > > > > These SW routines only work on unpacked/uncompresssed samples.
> > > > >
> > > > > For this reason, ensure the user cannot configure 14/16-bit RAW streams
> > > > > with PiSP Bayer compression in platformValidate().
> > > > >
> > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > > > > ---
> > > > >  src/libcamera/pipeline/rpi/pisp/pisp.cpp | 11 +++++++++++
> > > > >  1 file changed, 11 insertions(+)
> > > > >
> > > > > diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> > > > > index b744c901f751..8e8da116ef6f 100644
> > > > > --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> > > > > +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> > > > > @@ -1223,6 +1223,17 @@ PiSPCameraData::platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const
> > > > >                       status = CameraConfiguration::Adjusted;
> > > > >               }
> > > > >
> > > > > +             unsigned bpp = MediaBusFormatInfo::info(rpiConfig->sensorFormat_.code).bitsPerPixel;
> > > > > +             if ((bpp == 16 || bpp == 14) &&
> > > > > +                 bayer.packing != BayerFormat::Packing::None) {
> > > > > +                     LOG(RPI, Info)
> > > > > +                             << "The sensor is configured for a 16/14-bit output, "
> > > > > +                             << "compression is not available for SW fixups needed.";
> > > >
> > > > I still can't parse the message:
> > > >
> > > > "The sensor is configured for a 16/14-bit output, compression is not available for SW fixups needed."
> > >
> > > How about:
> > >
> > > LOG(RPI, Info)
> > >         << "The sensor is configured for "
> > >         << bpp
> > >         << "-bit pixel readout. PISP compression will be unavilable because of the"
> > >         << " software fixups necessary in this format."
> >
> > s/unavilable/unvailable/
> >
> > I can understand that :-)
> >
> > I'd write "Disabling PISP compression" instead of "PISP compression will
> > be unavailable", to be clear.
> >
> >                         LOG(RPI, Info)
> >                                 << "The sensor is configured for " << bpp
> >                                 << "-bit pixel readout. Disabling PISP compression because of the"
> >                                 << " software fixups necessary in this format."
> >
> > We could also shorten it:
> >
> >                         LOG(RPI, Info)
> >                                 << "Sensor configured in RAW" << bpp
> >                                 << ", disabling PISP compression incompatible with required software fixups."
> >
> > or
> >
> >                         LOG(RPI, Info)
> >                                 << "PISP compression incompatible with software fixups required for RAW"
> >                                 << bpp << ", disabling"
> >
> >
> 
> I'm good with either.  Would you be able to make the change before merging,
> or should I post a follow-on patch?

Kieran handled it. Thank you Kieran.

> > With one of those messages,
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > > > > +                     bayer.packing = BayerFormat::Packing::None;
> > > > > +                     rawStream->pixelFormat = bayer.toPixelFormat();
> > > > > +                     status = CameraConfiguration::Adjusted;
> > > > > +             }
> > > > > +
> > > > >               rawStreams[0].format = RPi::PipelineHandlerBase::toV4L2DeviceFormat(cfe_[Cfe::Output0].dev(), rawStream);

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
index b744c901f751..8e8da116ef6f 100644
--- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp
+++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
@@ -1223,6 +1223,17 @@  PiSPCameraData::platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const
 			status = CameraConfiguration::Adjusted;
 		}
 
+		unsigned bpp = MediaBusFormatInfo::info(rpiConfig->sensorFormat_.code).bitsPerPixel;
+		if ((bpp == 16 || bpp == 14) &&
+		    bayer.packing != BayerFormat::Packing::None) {
+			LOG(RPI, Info)
+				<< "The sensor is configured for a 16/14-bit output, "
+				<< "compression is not available for SW fixups needed.";
+			bayer.packing = BayerFormat::Packing::None;
+			rawStream->pixelFormat = bayer.toPixelFormat();
+			status = CameraConfiguration::Adjusted;
+		}
+
 		rawStreams[0].format =
 			RPi::PipelineHandlerBase::toV4L2DeviceFormat(cfe_[Cfe::Output0].dev(), rawStream);