| Message ID | 20260702095903.79640-1-david.plowman@raspberrypi.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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], ©.coeffs[6], 3 * sizeof(csc.coeffs[0])); > + memcpy(&csc.coeffs[6], ©.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 >
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], ©.coeffs[6], 3 * sizeof(csc.coeffs[0])); > > + memcpy(&csc.coeffs[6], ©.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 > >
Sorry, wrong patch! No, this one is good! David On Thu, 2 Jul 2026 at 12:25, David Plowman <david.plowman@raspberrypi.com> wrote: > > 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], ©.coeffs[6], 3 * sizeof(csc.coeffs[0])); > > > + memcpy(&csc.coeffs[6], ©.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 > > >
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], ©.coeffs[6], 3 * sizeof(csc.coeffs[0])); + memcpy(&csc.coeffs[6], ©.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. */
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(-)