pipeline: rpi: pisp: Fix colours for NV21 and NV61 pixel formats
diff mbox series

Message ID 20260702095903.79640-1-david.plowman@raspberrypi.com
State New
Headers show
Series
  • pipeline: rpi: pisp: Fix colours for NV21 and NV61 pixel formats
Related show

Commit Message

David Plowman July 2, 2026, 9:57 a.m. UTC 
Code was previously passing the "ORDER_SWAPPED" flag to swap the U and
V components, but our hardware doesn't actually support this for
semi-planar formats.

Instead swap over the 2nd and 3rd rows of the output colour conversion
matrices to achieve the same effect.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/libcamera/pipeline/rpi/pisp/pisp.cpp | 27 ++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

Comments

Naushir Patuck July 2, 2026, 10:46 a.m. UTC | #1 
Hi David,

Thanks for the fix.

On Thu, 2 Jul 2026 at 10:59, David Plowman
<david.plowman@raspberrypi.com> wrote:
>
> Code was previously passing the "ORDER_SWAPPED" flag to swap the U and
> V components, but our hardware doesn't actually support this for
> semi-planar formats.
>
> Instead swap over the 2nd and 3rd rows of the output colour conversion
> matrices to achieve the same effect.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>

> ---
>  src/libcamera/pipeline/rpi/pisp/pisp.cpp | 27 ++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> index 23f4c14d..5ea57399 100644
> --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> @@ -172,11 +172,14 @@ pisp_image_format_config toPiSPImageFormat(V4L2DeviceFormat &format)
>                 image.stride2 = image.stride;
>                 break;
>         case formats::NV21:
> +               /*
> +                * ORDER_SWAPPED does not work with semi-planar formats, so
> +                * we going to have swap rows in the output CSC matrix.
> +                */
>                 image.format = PISP_IMAGE_FORMAT_THREE_CHANNEL +
>                                PISP_IMAGE_FORMAT_BPS_8 +
>                                PISP_IMAGE_FORMAT_SAMPLING_420 +
> -                              PISP_IMAGE_FORMAT_PLANARITY_SEMI_PLANAR +
> -                              PISP_IMAGE_FORMAT_ORDER_SWAPPED;
> +                              PISP_IMAGE_FORMAT_PLANARITY_SEMI_PLANAR;
>                 image.stride2 = image.stride;
>                 break;
>         case formats::YUYV:
> @@ -200,11 +203,14 @@ pisp_image_format_config toPiSPImageFormat(V4L2DeviceFormat &format)
>                 image.stride2 = image.stride;
>                 break;
>         case formats::NV61:
> +               /*
> +                * ORDER_SWAPPED does not work with semi-planar formats, so
> +                * we going to have swap rows in the output CSC matrix.
> +                */
>                 image.format = PISP_IMAGE_FORMAT_THREE_CHANNEL +
>                                PISP_IMAGE_FORMAT_BPS_8 +
>                                PISP_IMAGE_FORMAT_SAMPLING_422 +
> -                              PISP_IMAGE_FORMAT_PLANARITY_SEMI_PLANAR +
> -                              PISP_IMAGE_FORMAT_ORDER_SWAPPED;
> +                              PISP_IMAGE_FORMAT_PLANARITY_SEMI_PLANAR;
>                 image.stride2 = image.stride;
>                 break;
>         case formats::RGB888:
> @@ -1964,6 +1970,19 @@ bool PiSPCameraData::calculateCscConfiguration(const V4L2DeviceFormat &v4l2Forma
>                                 << ", defaulting to sYCC";
>                         be_->InitialiseYcbcr(csc, "jpeg");
>                 }
> +
> +               if (pixFormat == formats::NV21 || pixFormat == formats::NV61) {
> +                       /*
> +                        * The ORDER_SWAPPED flag doesn't work with semi-planar formats,
> +                        * so instead we have to swap 2 matrix rows.
> +                        */
> +                       pisp_be_ccm_config copy = csc;
> +                       memcpy(&csc.coeffs[3], &copy.coeffs[6], 3 * sizeof(csc.coeffs[0]));
> +                       memcpy(&csc.coeffs[6], &copy.coeffs[3], 3 * sizeof(csc.coeffs[0]));
> +                       csc.offsets[1] = copy.offsets[2];
> +                       csc.offsets[2] = copy.offsets[1];
> +               }
> +
>                 return true;
>         }
>         /* There will be more formats to check for in due course. */
> --
> 2.47.3
>
David Plowman July 2, 2026, 11:25 a.m. UTC | #2 
Hi Naush

Thanks for the review, but actually there is a problem.

I'm inclined to drop this patch for the moment, I want to have another
think about it.

David

On Thu, 2 Jul 2026 at 11:47, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> Hi David,
>
> Thanks for the fix.
>
> On Thu, 2 Jul 2026 at 10:59, David Plowman
> <david.plowman@raspberrypi.com> wrote:
> >
> > Code was previously passing the "ORDER_SWAPPED" flag to swap the U and
> > V components, but our hardware doesn't actually support this for
> > semi-planar formats.
> >
> > Instead swap over the 2nd and 3rd rows of the output colour conversion
> > matrices to achieve the same effect.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
>
> > ---
> >  src/libcamera/pipeline/rpi/pisp/pisp.cpp | 27 ++++++++++++++++++++----
> >  1 file changed, 23 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> > index 23f4c14d..5ea57399 100644
> > --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> > +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> > @@ -172,11 +172,14 @@ pisp_image_format_config toPiSPImageFormat(V4L2DeviceFormat &format)
> >                 image.stride2 = image.stride;
> >                 break;
> >         case formats::NV21:
> > +               /*
> > +                * ORDER_SWAPPED does not work with semi-planar formats, so
> > +                * we going to have swap rows in the output CSC matrix.
> > +                */
> >                 image.format = PISP_IMAGE_FORMAT_THREE_CHANNEL +
> >                                PISP_IMAGE_FORMAT_BPS_8 +
> >                                PISP_IMAGE_FORMAT_SAMPLING_420 +
> > -                              PISP_IMAGE_FORMAT_PLANARITY_SEMI_PLANAR +
> > -                              PISP_IMAGE_FORMAT_ORDER_SWAPPED;
> > +                              PISP_IMAGE_FORMAT_PLANARITY_SEMI_PLANAR;
> >                 image.stride2 = image.stride;
> >                 break;
> >         case formats::YUYV:
> > @@ -200,11 +203,14 @@ pisp_image_format_config toPiSPImageFormat(V4L2DeviceFormat &format)
> >                 image.stride2 = image.stride;
> >                 break;
> >         case formats::NV61:
> > +               /*
> > +                * ORDER_SWAPPED does not work with semi-planar formats, so
> > +                * we going to have swap rows in the output CSC matrix.
> > +                */
> >                 image.format = PISP_IMAGE_FORMAT_THREE_CHANNEL +
> >                                PISP_IMAGE_FORMAT_BPS_8 +
> >                                PISP_IMAGE_FORMAT_SAMPLING_422 +
> > -                              PISP_IMAGE_FORMAT_PLANARITY_SEMI_PLANAR +
> > -                              PISP_IMAGE_FORMAT_ORDER_SWAPPED;
> > +                              PISP_IMAGE_FORMAT_PLANARITY_SEMI_PLANAR;
> >                 image.stride2 = image.stride;
> >                 break;
> >         case formats::RGB888:
> > @@ -1964,6 +1970,19 @@ bool PiSPCameraData::calculateCscConfiguration(const V4L2DeviceFormat &v4l2Forma
> >                                 << ", defaulting to sYCC";
> >                         be_->InitialiseYcbcr(csc, "jpeg");
> >                 }
> > +
> > +               if (pixFormat == formats::NV21 || pixFormat == formats::NV61) {
> > +                       /*
> > +                        * The ORDER_SWAPPED flag doesn't work with semi-planar formats,
> > +                        * so instead we have to swap 2 matrix rows.
> > +                        */
> > +                       pisp_be_ccm_config copy = csc;
> > +                       memcpy(&csc.coeffs[3], &copy.coeffs[6], 3 * sizeof(csc.coeffs[0]));
> > +                       memcpy(&csc.coeffs[6], &copy.coeffs[3], 3 * sizeof(csc.coeffs[0]));
> > +                       csc.offsets[1] = copy.offsets[2];
> > +                       csc.offsets[2] = copy.offsets[1];
> > +               }
> > +
> >                 return true;
> >         }
> >         /* There will be more formats to check for in due course. */
> > --
> > 2.47.3
> >

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
index 23f4c14d..5ea57399 100644
--- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp
+++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
@@ -172,11 +172,14 @@  pisp_image_format_config toPiSPImageFormat(V4L2DeviceFormat &format)
 		image.stride2 = image.stride;
 		break;
 	case formats::NV21:
+		/*
+		 * ORDER_SWAPPED does not work with semi-planar formats, so
+		 * we going to have swap rows in the output CSC matrix.
+		 */
 		image.format = PISP_IMAGE_FORMAT_THREE_CHANNEL +
 			       PISP_IMAGE_FORMAT_BPS_8 +
 			       PISP_IMAGE_FORMAT_SAMPLING_420 +
-			       PISP_IMAGE_FORMAT_PLANARITY_SEMI_PLANAR +
-			       PISP_IMAGE_FORMAT_ORDER_SWAPPED;
+			       PISP_IMAGE_FORMAT_PLANARITY_SEMI_PLANAR;
 		image.stride2 = image.stride;
 		break;
 	case formats::YUYV:
@@ -200,11 +203,14 @@  pisp_image_format_config toPiSPImageFormat(V4L2DeviceFormat &format)
 		image.stride2 = image.stride;
 		break;
 	case formats::NV61:
+		/*
+		 * ORDER_SWAPPED does not work with semi-planar formats, so
+		 * we going to have swap rows in the output CSC matrix.
+		 */
 		image.format = PISP_IMAGE_FORMAT_THREE_CHANNEL +
 			       PISP_IMAGE_FORMAT_BPS_8 +
 			       PISP_IMAGE_FORMAT_SAMPLING_422 +
-			       PISP_IMAGE_FORMAT_PLANARITY_SEMI_PLANAR +
-			       PISP_IMAGE_FORMAT_ORDER_SWAPPED;
+			       PISP_IMAGE_FORMAT_PLANARITY_SEMI_PLANAR;
 		image.stride2 = image.stride;
 		break;
 	case formats::RGB888:
@@ -1964,6 +1970,19 @@  bool PiSPCameraData::calculateCscConfiguration(const V4L2DeviceFormat &v4l2Forma
 				<< ", defaulting to sYCC";
 			be_->InitialiseYcbcr(csc, "jpeg");
 		}
+
+		if (pixFormat == formats::NV21 || pixFormat == formats::NV61) {
+			/*
+			 * The ORDER_SWAPPED flag doesn't work with semi-planar formats,
+			 * so instead we have to swap 2 matrix rows.
+			 */
+			pisp_be_ccm_config copy = csc;
+			memcpy(&csc.coeffs[3], &copy.coeffs[6], 3 * sizeof(csc.coeffs[0]));
+			memcpy(&csc.coeffs[6], &copy.coeffs[3], 3 * sizeof(csc.coeffs[0]));
+			csc.offsets[1] = copy.offsets[2];
+			csc.offsets[2] = copy.offsets[1];
+		}
+
 		return true;
 	}
 	/* There will be more formats to check for in due course. */