* [PATCH 0/3] suns6-csi changes to support libcamera
@ 2023-01-06 19:40 adam
2023-01-06 19:40 ` [PATCH 1/3] media: sun6i-csi: merge sun6i_csi_formats and sun6i_csi_formats_match adam
` (3 more replies)
0 siblings, 4 replies; 18+ messages in thread
From: adam @ 2023-01-06 19:40 UTC (permalink / raw)
To: linux-media
Cc: yong.deng, mchehab, linux-sunxi, paul.kocialkowski,
laurent.pinchart, Adam Pigg
From: Adam Pigg <adam@piggz.co.uk>
This 3 patch set adds V4L2_CAP_IO_MC and vidioc_enum_framesizes support
to the sun6i-csi driver, allowing the Pinephone rear camera (ov5640) to
be supported.
These have been developed with the guidance of Laurent Pinchart, and
been tested by capturing frames from the libcamera cam utility.
In addition to these, a patch to change the v4l gain mode in the ov5640
driver, and the 2 format propagation patches all by Laurnet are
required.
Adam Pigg (3):
media: sun6i-csi: merge sun6i_csi_formats and sun6i_csi_formats_match
media: sun6i-csi: implement V4L2_CAP_IO_MC
media: sun6i-csi: implement vidioc_enum_framesizes
.../sunxi/sun6i-csi/sun6i_csi_capture.c | 215 +++++++++---------
.../sunxi/sun6i-csi/sun6i_csi_capture.h | 6 +-
2 files changed, 103 insertions(+), 118 deletions(-)
--
2.39.0
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/3] media: sun6i-csi: merge sun6i_csi_formats and sun6i_csi_formats_match
2023-01-06 19:40 [PATCH 0/3] suns6-csi changes to support libcamera adam
@ 2023-01-06 19:40 ` adam
2023-01-06 23:31 ` Laurent Pinchart
2023-01-06 19:40 ` [PATCH 2/3] media: sun6i-csi: implement V4L2_CAP_IO_MC adam
` (2 subsequent siblings)
3 siblings, 1 reply; 18+ messages in thread
From: adam @ 2023-01-06 19:40 UTC (permalink / raw)
To: linux-media
Cc: yong.deng, mchehab, linux-sunxi, paul.kocialkowski,
laurent.pinchart, Adam Pigg
From: Adam Pigg <adam@piggz.co.uk>
Merged the two format arrays into sun6i_csi_capture_formats as a
pre-requisite to implementing V4L2_CAP_IO_MC
Signed-off-by: Adam Pigg <adam@piggz.co.uk>
---
.../sunxi/sun6i-csi/sun6i_csi_capture.c | 155 +++++-------------
.../sunxi/sun6i-csi/sun6i_csi_capture.h | 6 +-
2 files changed, 46 insertions(+), 115 deletions(-)
diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
index 03d4adec054c..69578075421c 100644
--- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
+++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
@@ -22,6 +22,8 @@
/* Helpers */
+#define SUN6I_BUS_FMTS(fmt...) (const u32[]) {fmt, 0}
+
void sun6i_csi_capture_dimensions(struct sun6i_csi_device *csi_dev,
unsigned int *width, unsigned int *height)
{
@@ -49,72 +51,86 @@ static const struct sun6i_csi_capture_format sun6i_csi_capture_formats[] = {
.pixelformat = V4L2_PIX_FMT_SBGGR8,
.output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
+ .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SBGGR8_1X8),
},
{
.pixelformat = V4L2_PIX_FMT_SGBRG8,
.output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
+ .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGBRG8_1X8),
},
{
.pixelformat = V4L2_PIX_FMT_SGRBG8,
.output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
+ .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGRBG8_1X8),
},
{
.pixelformat = V4L2_PIX_FMT_SRGGB8,
.output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
+ .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SRGGB8_1X8),
},
{
.pixelformat = V4L2_PIX_FMT_SBGGR10,
.output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_10,
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_10,
+ .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SBGGR10_1X10),
},
{
.pixelformat = V4L2_PIX_FMT_SGBRG10,
.output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_10,
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_10,
+ .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGBRG10_1X10),
},
{
.pixelformat = V4L2_PIX_FMT_SGRBG10,
.output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_10,
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_10,
+ .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGRBG10_1X10),
},
{
.pixelformat = V4L2_PIX_FMT_SRGGB10,
.output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_10,
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_10,
+ .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SRGGB10_1X10),
},
{
.pixelformat = V4L2_PIX_FMT_SBGGR12,
.output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_12,
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_12,
+ .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SBGGR12_1X12),
},
{
.pixelformat = V4L2_PIX_FMT_SGBRG12,
.output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_12,
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_12,
+ .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGBRG12_1X12),
},
{
.pixelformat = V4L2_PIX_FMT_SGRBG12,
.output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_12,
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_12,
+ .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGRBG12_1X12),
},
{
.pixelformat = V4L2_PIX_FMT_SRGGB12,
.output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_12,
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_12,
+ .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SRGGB12_1X12),
},
/* RGB */
{
.pixelformat = V4L2_PIX_FMT_RGB565,
.output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RGB565,
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RGB565,
+ .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_RGB565_2X8_LE),
},
{
.pixelformat = V4L2_PIX_FMT_RGB565X,
.output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RGB565,
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RGB565,
+ .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_RGB565_2X8_BE),
},
/* YUV422 */
{
@@ -123,6 +139,8 @@ static const struct sun6i_csi_capture_format sun6i_csi_capture_formats[] = {
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
.input_format_raw = true,
.hsize_len_factor = 2,
+ .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_YUYV8_2X8,
+ MEDIA_BUS_FMT_YUYV8_1X16),
},
{
.pixelformat = V4L2_PIX_FMT_YVYU,
@@ -130,6 +148,8 @@ static const struct sun6i_csi_capture_format sun6i_csi_capture_formats[] = {
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
.input_format_raw = true,
.hsize_len_factor = 2,
+ .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_YVYU8_2X8,
+ MEDIA_BUS_FMT_YVYU8_1X16),
},
{
.pixelformat = V4L2_PIX_FMT_UYVY,
@@ -137,6 +157,8 @@ static const struct sun6i_csi_capture_format sun6i_csi_capture_formats[] = {
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
.input_format_raw = true,
.hsize_len_factor = 2,
+ .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_UYVY8_2X8,
+ MEDIA_BUS_FMT_UYVY8_1X16),
},
{
.pixelformat = V4L2_PIX_FMT_VYUY,
@@ -144,57 +166,68 @@ static const struct sun6i_csi_capture_format sun6i_csi_capture_formats[] = {
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
.input_format_raw = true,
.hsize_len_factor = 2,
+ .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_VYUY8_2X8,
+ MEDIA_BUS_FMT_VYUY8_1X16),
},
{
.pixelformat = V4L2_PIX_FMT_NV16,
.output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422SP,
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422SP,
+ .mbus_codes = 0,
},
{
.pixelformat = V4L2_PIX_FMT_NV61,
.output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422SP,
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422SP,
.input_yuv_seq_invert = true,
+ .mbus_codes = 0,
},
{
.pixelformat = V4L2_PIX_FMT_YUV422P,
.output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422P,
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422P,
+ .mbus_codes = 0,
},
/* YUV420 */
{
.pixelformat = V4L2_PIX_FMT_NV12_16L16,
.output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420MB,
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420MB,
+ .mbus_codes = 0,
},
{
.pixelformat = V4L2_PIX_FMT_NV12,
.output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420SP,
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420SP,
+ .mbus_codes = 0,
},
{
.pixelformat = V4L2_PIX_FMT_NV21,
.output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420SP,
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420SP,
.input_yuv_seq_invert = true,
+ .mbus_codes = 0,
},
{
.pixelformat = V4L2_PIX_FMT_YUV420,
.output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420P,
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420P,
+ .mbus_codes = 0,
},
{
.pixelformat = V4L2_PIX_FMT_YVU420,
.output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420P,
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420P,
.input_yuv_seq_invert = true,
+ .mbus_codes = 0,
},
/* Compressed */
{
.pixelformat = V4L2_PIX_FMT_JPEG,
.output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
.output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
+ .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_JPEG_1X8),
},
};
@@ -210,118 +243,20 @@ struct sun6i_csi_capture_format *sun6i_csi_capture_format_find(u32 pixelformat)
return NULL;
}
-/* RAW formats need an exact match between pixel and mbus formats. */
-static const
-struct sun6i_csi_capture_format_match sun6i_csi_capture_format_matches[] = {
- /* YUV420 */
- {
- .pixelformat = V4L2_PIX_FMT_YUYV,
- .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8,
- },
- {
- .pixelformat = V4L2_PIX_FMT_YUYV,
- .mbus_code = MEDIA_BUS_FMT_YUYV8_1X16,
- },
- {
- .pixelformat = V4L2_PIX_FMT_YVYU,
- .mbus_code = MEDIA_BUS_FMT_YVYU8_2X8,
- },
- {
- .pixelformat = V4L2_PIX_FMT_YVYU,
- .mbus_code = MEDIA_BUS_FMT_YVYU8_1X16,
- },
- {
- .pixelformat = V4L2_PIX_FMT_UYVY,
- .mbus_code = MEDIA_BUS_FMT_UYVY8_2X8,
- },
- {
- .pixelformat = V4L2_PIX_FMT_UYVY,
- .mbus_code = MEDIA_BUS_FMT_UYVY8_1X16,
- },
- {
- .pixelformat = V4L2_PIX_FMT_VYUY,
- .mbus_code = MEDIA_BUS_FMT_VYUY8_2X8,
- },
- {
- .pixelformat = V4L2_PIX_FMT_VYUY,
- .mbus_code = MEDIA_BUS_FMT_VYUY8_1X16,
- },
- /* RGB */
- {
- .pixelformat = V4L2_PIX_FMT_RGB565,
- .mbus_code = MEDIA_BUS_FMT_RGB565_2X8_LE,
- },
- {
- .pixelformat = V4L2_PIX_FMT_RGB565X,
- .mbus_code = MEDIA_BUS_FMT_RGB565_2X8_BE,
- },
- /* Bayer */
- {
- .pixelformat = V4L2_PIX_FMT_SBGGR8,
- .mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8,
- },
- {
- .pixelformat = V4L2_PIX_FMT_SGBRG8,
- .mbus_code = MEDIA_BUS_FMT_SGBRG8_1X8,
- },
- {
- .pixelformat = V4L2_PIX_FMT_SGRBG8,
- .mbus_code = MEDIA_BUS_FMT_SGRBG8_1X8,
- },
- {
- .pixelformat = V4L2_PIX_FMT_SRGGB8,
- .mbus_code = MEDIA_BUS_FMT_SRGGB8_1X8,
- },
- {
- .pixelformat = V4L2_PIX_FMT_SBGGR10,
- .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10,
- },
- {
- .pixelformat = V4L2_PIX_FMT_SGBRG10,
- .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10,
- },
- {
- .pixelformat = V4L2_PIX_FMT_SGRBG10,
- .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10,
- },
- {
- .pixelformat = V4L2_PIX_FMT_SRGGB10,
- .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10,
- },
- {
- .pixelformat = V4L2_PIX_FMT_SBGGR12,
- .mbus_code = MEDIA_BUS_FMT_SBGGR12_1X12,
- },
- {
- .pixelformat = V4L2_PIX_FMT_SGBRG12,
- .mbus_code = MEDIA_BUS_FMT_SGBRG12_1X12,
- },
- {
- .pixelformat = V4L2_PIX_FMT_SGRBG12,
- .mbus_code = MEDIA_BUS_FMT_SGRBG12_1X12,
- },
- {
- .pixelformat = V4L2_PIX_FMT_SRGGB12,
- .mbus_code = MEDIA_BUS_FMT_SRGGB12_1X12,
- },
- /* Compressed */
- {
- .pixelformat = V4L2_PIX_FMT_JPEG,
- .mbus_code = MEDIA_BUS_FMT_JPEG_1X8,
- },
-};
-
static bool sun6i_csi_capture_format_match(u32 pixelformat, u32 mbus_code)
{
- unsigned int i;
-
- for (i = 0; i < ARRAY_SIZE(sun6i_csi_capture_format_matches); i++) {
- const struct sun6i_csi_capture_format_match *match =
- &sun6i_csi_capture_format_matches[i];
-
- if (match->pixelformat == pixelformat &&
- match->mbus_code == mbus_code)
- return true;
+ unsigned int i, j;
+
+ for (i = 0; i < ARRAY_SIZE(sun6i_csi_capture_formats); i++) {
+ const struct sun6i_csi_capture_format *format =
+ &sun6i_csi_capture_formats[i];
+
+ if (format->pixelformat == pixelformat) {
+ for (j = 0; format->mbus_codes[j]; j++) {
+ if (mbus_code == format->mbus_codes[j])
+ return true;
+ }
+ }
}
return false;
diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
index 3ee5ccefbd10..0484942834e3 100644
--- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
+++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
@@ -27,11 +27,7 @@ struct sun6i_csi_capture_format {
bool input_yuv_seq_invert;
bool input_format_raw;
u32 hsize_len_factor;
-};
-
-struct sun6i_csi_capture_format_match {
- u32 pixelformat;
- u32 mbus_code;
+ const u32 *mbus_codes;
};
#undef current
--
2.39.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/3] media: sun6i-csi: implement V4L2_CAP_IO_MC
2023-01-06 19:40 [PATCH 0/3] suns6-csi changes to support libcamera adam
2023-01-06 19:40 ` [PATCH 1/3] media: sun6i-csi: merge sun6i_csi_formats and sun6i_csi_formats_match adam
@ 2023-01-06 19:40 ` adam
2023-01-06 23:34 ` Laurent Pinchart
2023-01-06 19:40 ` [PATCH 3/3] media: sun6i-csi: implement vidioc_enum_framesizes adam
2023-01-06 23:10 ` [PATCH 0/3] suns6-csi changes to support libcamera piggz1
3 siblings, 1 reply; 18+ messages in thread
From: adam @ 2023-01-06 19:40 UTC (permalink / raw)
To: linux-media
Cc: yong.deng, mchehab, linux-sunxi, paul.kocialkowski,
laurent.pinchart, Adam Pigg
From: Adam Pigg <adam@piggz.co.uk>
modify sun6i_csi_capture_enum_fmt to check the mbus_codes from the
pixel format array for the supplied format
Signed-off-by: Adam Pigg <adam@piggz.co.uk>
---
.../sunxi/sun6i-csi/sun6i_csi_capture.c | 36 +++++++++++++++++--
1 file changed, 33 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
index 69578075421c..54beba4d2b2f 100644
--- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
+++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
@@ -666,13 +666,43 @@ static int sun6i_csi_capture_enum_fmt(struct file *file, void *private,
struct v4l2_fmtdesc *fmtdesc)
{
u32 index = fmtdesc->index;
+ unsigned int i;
if (index >= ARRAY_SIZE(sun6i_csi_capture_formats))
return -EINVAL;
- fmtdesc->pixelformat = sun6i_csi_capture_formats[index].pixelformat;
+ for (i = 0; i < ARRAY_SIZE(sun6i_csi_capture_formats); i++) {
+ const struct sun6i_csi_capture_format *format =
+ &sun6i_csi_capture_formats[i];
- return 0;
+ /*
+ * If a media bus code is specified, only consider formats that
+ * match it.
+ */
+ if (fmtdesc->mbus_code) {
+ unsigned int j;
+
+ if (!format->mbus_codes)
+ continue;
+
+ for (j = 0; format->mbus_codes[j]; j++) {
+ if (fmtdesc->mbus_code == format->mbus_codes[j])
+ break;
+ }
+
+ if (!format->mbus_codes[j])
+ continue;
+ }
+
+ if (index == 0) {
+ fmtdesc->pixelformat = format->pixelformat;
+ return 0;
+ }
+
+ index--;
+ }
+
+ return -EINVAL;
}
static int sun6i_csi_capture_g_fmt(struct file *file, void *private,
@@ -978,7 +1008,7 @@ int sun6i_csi_capture_setup(struct sun6i_csi_device *csi_dev)
strscpy(video_dev->name, SUN6I_CSI_CAPTURE_NAME,
sizeof(video_dev->name));
- video_dev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
+ video_dev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING | V4L2_CAP_IO_MC;
video_dev->vfl_dir = VFL_DIR_RX;
video_dev->release = video_device_release_empty;
video_dev->fops = &sun6i_csi_capture_fops;
--
2.39.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/3] media: sun6i-csi: implement vidioc_enum_framesizes
2023-01-06 19:40 [PATCH 0/3] suns6-csi changes to support libcamera adam
2023-01-06 19:40 ` [PATCH 1/3] media: sun6i-csi: merge sun6i_csi_formats and sun6i_csi_formats_match adam
2023-01-06 19:40 ` [PATCH 2/3] media: sun6i-csi: implement V4L2_CAP_IO_MC adam
@ 2023-01-06 19:40 ` adam
2023-01-06 23:35 ` Laurent Pinchart
2023-01-06 23:10 ` [PATCH 0/3] suns6-csi changes to support libcamera piggz1
3 siblings, 1 reply; 18+ messages in thread
From: adam @ 2023-01-06 19:40 UTC (permalink / raw)
To: linux-media
Cc: yong.deng, mchehab, linux-sunxi, paul.kocialkowski,
laurent.pinchart, Adam Pigg
From: Adam Pigg <adam@piggz.co.uk>
Create sun6i_csi_capture_enum_framesizes which defines the min
and max frame sizes
Signed-off-by: Adam Pigg <adam@piggz.co.uk>
---
.../sunxi/sun6i-csi/sun6i_csi_capture.c | 24 +++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
index 54beba4d2b2f..2be761e6b650 100644
--- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
+++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
@@ -739,6 +739,29 @@ static int sun6i_csi_capture_try_fmt(struct file *file, void *private,
return 0;
}
+static int sun6i_csi_capture_enum_framesizes(struct file *file, void *fh,
+ struct v4l2_frmsizeenum *fsize)
+{
+ const struct sun6i_csi_capture_format *format;
+
+ if (fsize->index > 0)
+ return -EINVAL;
+
+ format = sun6i_csi_capture_format_find(fsize->pixel_format);
+ if (!format)
+ return -EINVAL;
+
+ fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS;
+ fsize->stepwise.min_width = SUN6I_CSI_CAPTURE_WIDTH_MIN;
+ fsize->stepwise.max_width = SUN6I_CSI_CAPTURE_WIDTH_MAX;
+ fsize->stepwise.min_height = SUN6I_CSI_CAPTURE_HEIGHT_MIN;
+ fsize->stepwise.max_height = SUN6I_CSI_CAPTURE_HEIGHT_MAX;
+ fsize->stepwise.step_width = 1;
+ fsize->stepwise.step_height = 1;
+
+ return 0;
+}
+
static int sun6i_csi_capture_enum_input(struct file *file, void *private,
struct v4l2_input *input)
{
@@ -775,6 +798,7 @@ static const struct v4l2_ioctl_ops sun6i_csi_capture_ioctl_ops = {
.vidioc_g_fmt_vid_cap = sun6i_csi_capture_g_fmt,
.vidioc_s_fmt_vid_cap = sun6i_csi_capture_s_fmt,
.vidioc_try_fmt_vid_cap = sun6i_csi_capture_try_fmt,
+ .vidioc_enum_framesizes = sun6i_csi_capture_enum_framesizes,
.vidioc_enum_input = sun6i_csi_capture_enum_input,
.vidioc_g_input = sun6i_csi_capture_g_input,
--
2.39.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] suns6-csi changes to support libcamera
2023-01-06 19:40 [PATCH 0/3] suns6-csi changes to support libcamera adam
` (2 preceding siblings ...)
2023-01-06 19:40 ` [PATCH 3/3] media: sun6i-csi: implement vidioc_enum_framesizes adam
@ 2023-01-06 23:10 ` piggz1
2023-01-07 8:43 ` Adam Pigg
3 siblings, 1 reply; 18+ messages in thread
From: piggz1 @ 2023-01-06 23:10 UTC (permalink / raw)
To: adam, linux-media
Cc: mchehab, linux-sunxi, paul.kocialkowski, laurent.pinchart, adam,
yong.deng
Apologies, accidentally sent this set twice. please ignore this set.
Adam
On Friday, 6 January 2023, adam@piggz.co.uk wrote:
> From: Adam Pigg <adam@piggz.co.uk>
>
> This 3 patch set adds V4L2_CAP_IO_MC and vidioc_enum_framesizes support
> to the sun6i-csi driver, allowing the Pinephone rear camera (ov5640) to
> be supported.
>
> These have been developed with the guidance of Laurent Pinchart, and
> been tested by capturing frames from the libcamera cam utility.
>
> In addition to these, a patch to change the v4l gain mode in the ov5640
> driver, and the 2 format propagation patches all by Laurnet are
> required.
>
> Adam Pigg (3):
> media: sun6i-csi: merge sun6i_csi_formats and sun6i_csi_formats_match
> media: sun6i-csi: implement V4L2_CAP_IO_MC
> media: sun6i-csi: implement vidioc_enum_framesizes
>
> .../sunxi/sun6i-csi/sun6i_csi_capture.c | 215 +++++++++---------
> .../sunxi/sun6i-csi/sun6i_csi_capture.h | 6 +-
> 2 files changed, 103 insertions(+), 118 deletions(-)
>
> --
> 2.39.0
>
>
--
Sent from my Sailfish device
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] media: sun6i-csi: merge sun6i_csi_formats and sun6i_csi_formats_match
2023-01-06 19:40 ` [PATCH 1/3] media: sun6i-csi: merge sun6i_csi_formats and sun6i_csi_formats_match adam
@ 2023-01-06 23:31 ` Laurent Pinchart
2023-01-08 18:23 ` Adam Pigg
0 siblings, 1 reply; 18+ messages in thread
From: Laurent Pinchart @ 2023-01-06 23:31 UTC (permalink / raw)
To: adam; +Cc: linux-media, yong.deng, mchehab, linux-sunxi, paul.kocialkowski
Hi Adam,
Thank you for the patch.
On Fri, Jan 06, 2023 at 07:40:36PM +0000, adam@piggz.co.uk wrote:
> From: Adam Pigg <adam@piggz.co.uk>
>
> Merged the two format arrays into sun6i_csi_capture_formats as a
> pre-requisite to implementing V4L2_CAP_IO_MC
I'll point to https://cbea.ms/git-commit/ if you haven't read it yet.
You can ignore the "Limit the subject line to 50 characters" rule and go
up to the normal 72 characters limit for commit messages, as 50 doesn't
include the prefixes commonly used in kernel commit messages.
For this specific patch, I would write
Information about media bus formats and pixel formats supported by the
driver is split between the sun6i_csi_capture_formats and
sun6i_csi_capture_format_matches arrays. This makes it difficult to map
media bus formats to pixel formats when enumerating the supported pixel
formats by walking the sun6i_csi_capture_formats array. To prepare for
support of media bus format support in sun6i_csi_capture_enum_fmt(),
merge the two arrays toegether.
An extra paragraph could be added to explain *how* this is being done,
but the implementation is straightforward enough to not require that.
> Signed-off-by: Adam Pigg <adam@piggz.co.uk>
> ---
> .../sunxi/sun6i-csi/sun6i_csi_capture.c | 155 +++++-------------
> .../sunxi/sun6i-csi/sun6i_csi_capture.h | 6 +-
> 2 files changed, 46 insertions(+), 115 deletions(-)
>
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> index 03d4adec054c..69578075421c 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> @@ -22,6 +22,8 @@
>
> /* Helpers */
>
> +#define SUN6I_BUS_FMTS(fmt...) (const u32[]) {fmt, 0}
> +
> void sun6i_csi_capture_dimensions(struct sun6i_csi_device *csi_dev,
> unsigned int *width, unsigned int *height)
> {
> @@ -49,72 +51,86 @@ static const struct sun6i_csi_capture_format sun6i_csi_capture_formats[] = {
> .pixelformat = V4L2_PIX_FMT_SBGGR8,
> .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SBGGR8_1X8),
> },
> {
> .pixelformat = V4L2_PIX_FMT_SGBRG8,
> .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGBRG8_1X8),
> },
> {
> .pixelformat = V4L2_PIX_FMT_SGRBG8,
> .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGRBG8_1X8),
> },
> {
> .pixelformat = V4L2_PIX_FMT_SRGGB8,
> .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SRGGB8_1X8),
> },
> {
> .pixelformat = V4L2_PIX_FMT_SBGGR10,
> .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_10,
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_10,
> + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SBGGR10_1X10),
> },
> {
> .pixelformat = V4L2_PIX_FMT_SGBRG10,
> .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_10,
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_10,
> + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGBRG10_1X10),
> },
> {
> .pixelformat = V4L2_PIX_FMT_SGRBG10,
> .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_10,
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_10,
> + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGRBG10_1X10),
> },
> {
> .pixelformat = V4L2_PIX_FMT_SRGGB10,
> .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_10,
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_10,
> + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SRGGB10_1X10),
> },
> {
> .pixelformat = V4L2_PIX_FMT_SBGGR12,
> .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_12,
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_12,
> + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SBGGR12_1X12),
> },
> {
> .pixelformat = V4L2_PIX_FMT_SGBRG12,
> .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_12,
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_12,
> + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGBRG12_1X12),
> },
> {
> .pixelformat = V4L2_PIX_FMT_SGRBG12,
> .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_12,
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_12,
> + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGRBG12_1X12),
> },
> {
> .pixelformat = V4L2_PIX_FMT_SRGGB12,
> .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_12,
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_12,
> + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SRGGB12_1X12),
> },
> /* RGB */
> {
> .pixelformat = V4L2_PIX_FMT_RGB565,
> .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RGB565,
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RGB565,
> + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_RGB565_2X8_LE),
> },
> {
> .pixelformat = V4L2_PIX_FMT_RGB565X,
> .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RGB565,
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RGB565,
> + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_RGB565_2X8_BE),
> },
> /* YUV422 */
> {
> @@ -123,6 +139,8 @@ static const struct sun6i_csi_capture_format sun6i_csi_capture_formats[] = {
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> .input_format_raw = true,
> .hsize_len_factor = 2,
> + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_YUYV8_2X8,
> + MEDIA_BUS_FMT_YUYV8_1X16),
> },
> {
> .pixelformat = V4L2_PIX_FMT_YVYU,
> @@ -130,6 +148,8 @@ static const struct sun6i_csi_capture_format sun6i_csi_capture_formats[] = {
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> .input_format_raw = true,
> .hsize_len_factor = 2,
> + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_YVYU8_2X8,
> + MEDIA_BUS_FMT_YVYU8_1X16),
> },
> {
> .pixelformat = V4L2_PIX_FMT_UYVY,
> @@ -137,6 +157,8 @@ static const struct sun6i_csi_capture_format sun6i_csi_capture_formats[] = {
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> .input_format_raw = true,
> .hsize_len_factor = 2,
> + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_UYVY8_2X8,
> + MEDIA_BUS_FMT_UYVY8_1X16),
> },
> {
> .pixelformat = V4L2_PIX_FMT_VYUY,
> @@ -144,57 +166,68 @@ static const struct sun6i_csi_capture_format sun6i_csi_capture_formats[] = {
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> .input_format_raw = true,
> .hsize_len_factor = 2,
> + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_VYUY8_2X8,
> + MEDIA_BUS_FMT_VYUY8_1X16),
> },
> {
> .pixelformat = V4L2_PIX_FMT_NV16,
> .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422SP,
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422SP,
> + .mbus_codes = 0,
I don't think this is correct. To produce semi-planar or multi-planar
YUV formats, I believe the CSI needs YUV input. This should thus be
(unless I'm mistaken)
.mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_UYVY8_2X8,
MEDIA_BUS_FMT_UYVY8_1X16,
MEDIA_BUS_FMT_VYUY8_2X8,
MEDIA_BUS_FMT_VYUY8_1X16,
MEDIA_BUS_FMT_YUYV8_2X8,
MEDIA_BUS_FMT_YUYV8_1X16,
MEDIA_BUS_FMT_YVYU8_2X8,
MEDIA_BUS_FMT_YVYU8_1X16),
and same below.
Paul, could you confirm this ?
I'm a bit surprised that the CSI can't shuffle the YUV components for
packed YUYV formats, but so be it if that's a hardware limitation.
I'm also thinking that a subsequent patch could drop the raw check from
sun6i_csi_capture_link_validate():
- /* With raw input mode, we need a 1:1 match between input and output. */
- if (bridge_format->input_format == SUN6I_CSI_INPUT_FMT_RAW ||
- capture_format->input_format_raw) {
- match = sun6i_csi_capture_format_match(pixelformat,
- fmt.format.code);
- if (!match)
- goto invalid;
- }
+ /* Make sure the media bus code and pixel format are compatible. */
+ match = sun6i_csi_capture_format_match(pixelformat, fmt.format.code);
+ if (!match)
+ goto invalid;
> },
> {
> .pixelformat = V4L2_PIX_FMT_NV61,
> .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422SP,
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422SP,
> .input_yuv_seq_invert = true,
> + .mbus_codes = 0,
> },
> {
> .pixelformat = V4L2_PIX_FMT_YUV422P,
> .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422P,
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422P,
> + .mbus_codes = 0,
> },
> /* YUV420 */
> {
> .pixelformat = V4L2_PIX_FMT_NV12_16L16,
> .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420MB,
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420MB,
> + .mbus_codes = 0,
> },
> {
> .pixelformat = V4L2_PIX_FMT_NV12,
> .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420SP,
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420SP,
> + .mbus_codes = 0,
> },
> {
> .pixelformat = V4L2_PIX_FMT_NV21,
> .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420SP,
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420SP,
> .input_yuv_seq_invert = true,
> + .mbus_codes = 0,
> },
>
> {
> .pixelformat = V4L2_PIX_FMT_YUV420,
> .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420P,
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420P,
> + .mbus_codes = 0,
> },
> {
> .pixelformat = V4L2_PIX_FMT_YVU420,
> .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420P,
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420P,
> .input_yuv_seq_invert = true,
> + .mbus_codes = 0,
> },
> /* Compressed */
> {
> .pixelformat = V4L2_PIX_FMT_JPEG,
> .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_JPEG_1X8),
> },
> };
>
> @@ -210,118 +243,20 @@ struct sun6i_csi_capture_format *sun6i_csi_capture_format_find(u32 pixelformat)
> return NULL;
> }
>
> -/* RAW formats need an exact match between pixel and mbus formats. */
> -static const
> -struct sun6i_csi_capture_format_match sun6i_csi_capture_format_matches[] = {
> - /* YUV420 */
> - {
> - .pixelformat = V4L2_PIX_FMT_YUYV,
> - .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8,
> - },
> - {
> - .pixelformat = V4L2_PIX_FMT_YUYV,
> - .mbus_code = MEDIA_BUS_FMT_YUYV8_1X16,
> - },
> - {
> - .pixelformat = V4L2_PIX_FMT_YVYU,
> - .mbus_code = MEDIA_BUS_FMT_YVYU8_2X8,
> - },
> - {
> - .pixelformat = V4L2_PIX_FMT_YVYU,
> - .mbus_code = MEDIA_BUS_FMT_YVYU8_1X16,
> - },
> - {
> - .pixelformat = V4L2_PIX_FMT_UYVY,
> - .mbus_code = MEDIA_BUS_FMT_UYVY8_2X8,
> - },
> - {
> - .pixelformat = V4L2_PIX_FMT_UYVY,
> - .mbus_code = MEDIA_BUS_FMT_UYVY8_1X16,
> - },
> - {
> - .pixelformat = V4L2_PIX_FMT_VYUY,
> - .mbus_code = MEDIA_BUS_FMT_VYUY8_2X8,
> - },
> - {
> - .pixelformat = V4L2_PIX_FMT_VYUY,
> - .mbus_code = MEDIA_BUS_FMT_VYUY8_1X16,
> - },
> - /* RGB */
> - {
> - .pixelformat = V4L2_PIX_FMT_RGB565,
> - .mbus_code = MEDIA_BUS_FMT_RGB565_2X8_LE,
> - },
> - {
> - .pixelformat = V4L2_PIX_FMT_RGB565X,
> - .mbus_code = MEDIA_BUS_FMT_RGB565_2X8_BE,
> - },
> - /* Bayer */
> - {
> - .pixelformat = V4L2_PIX_FMT_SBGGR8,
> - .mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8,
> - },
> - {
> - .pixelformat = V4L2_PIX_FMT_SGBRG8,
> - .mbus_code = MEDIA_BUS_FMT_SGBRG8_1X8,
> - },
> - {
> - .pixelformat = V4L2_PIX_FMT_SGRBG8,
> - .mbus_code = MEDIA_BUS_FMT_SGRBG8_1X8,
> - },
> - {
> - .pixelformat = V4L2_PIX_FMT_SRGGB8,
> - .mbus_code = MEDIA_BUS_FMT_SRGGB8_1X8,
> - },
> - {
> - .pixelformat = V4L2_PIX_FMT_SBGGR10,
> - .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10,
> - },
> - {
> - .pixelformat = V4L2_PIX_FMT_SGBRG10,
> - .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10,
> - },
> - {
> - .pixelformat = V4L2_PIX_FMT_SGRBG10,
> - .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10,
> - },
> - {
> - .pixelformat = V4L2_PIX_FMT_SRGGB10,
> - .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10,
> - },
> - {
> - .pixelformat = V4L2_PIX_FMT_SBGGR12,
> - .mbus_code = MEDIA_BUS_FMT_SBGGR12_1X12,
> - },
> - {
> - .pixelformat = V4L2_PIX_FMT_SGBRG12,
> - .mbus_code = MEDIA_BUS_FMT_SGBRG12_1X12,
> - },
> - {
> - .pixelformat = V4L2_PIX_FMT_SGRBG12,
> - .mbus_code = MEDIA_BUS_FMT_SGRBG12_1X12,
> - },
> - {
> - .pixelformat = V4L2_PIX_FMT_SRGGB12,
> - .mbus_code = MEDIA_BUS_FMT_SRGGB12_1X12,
> - },
> - /* Compressed */
> - {
> - .pixelformat = V4L2_PIX_FMT_JPEG,
> - .mbus_code = MEDIA_BUS_FMT_JPEG_1X8,
> - },
> -};
> -
> static bool sun6i_csi_capture_format_match(u32 pixelformat, u32 mbus_code)
> {
> - unsigned int i;
> -
> - for (i = 0; i < ARRAY_SIZE(sun6i_csi_capture_format_matches); i++) {
> - const struct sun6i_csi_capture_format_match *match =
> - &sun6i_csi_capture_format_matches[i];
> -
> - if (match->pixelformat == pixelformat &&
> - match->mbus_code == mbus_code)
> - return true;
> + unsigned int i, j;
> +
> + for (i = 0; i < ARRAY_SIZE(sun6i_csi_capture_formats); i++) {
> + const struct sun6i_csi_capture_format *format =
> + &sun6i_csi_capture_formats[i];
> +
> + if (format->pixelformat == pixelformat) {
> + for (j = 0; format->mbus_codes[j]; j++) {
> + if (mbus_code == format->mbus_codes[j])
> + return true;
> + }
> + }
> }
>
> return false;
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> index 3ee5ccefbd10..0484942834e3 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> @@ -27,11 +27,7 @@ struct sun6i_csi_capture_format {
> bool input_yuv_seq_invert;
> bool input_format_raw;
> u32 hsize_len_factor;
> -};
> -
> -struct sun6i_csi_capture_format_match {
> - u32 pixelformat;
> - u32 mbus_code;
> + const u32 *mbus_codes;
> };
>
> #undef current
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] media: sun6i-csi: implement V4L2_CAP_IO_MC
2023-01-06 19:40 ` [PATCH 2/3] media: sun6i-csi: implement V4L2_CAP_IO_MC adam
@ 2023-01-06 23:34 ` Laurent Pinchart
0 siblings, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2023-01-06 23:34 UTC (permalink / raw)
To: adam; +Cc: linux-media, yong.deng, mchehab, linux-sunxi, paul.kocialkowski
Hi Adam,
Thank you for the patch.
On Fri, Jan 06, 2023 at 07:40:37PM +0000, adam@piggz.co.uk wrote:
> From: Adam Pigg <adam@piggz.co.uk>
>
> modify sun6i_csi_capture_enum_fmt to check the mbus_codes from the
> pixel format array for the supplied format
The commit message should explain why. Please also start sentences with
a capital letter, and end them with a period.
> Signed-off-by: Adam Pigg <adam@piggz.co.uk>
> ---
> .../sunxi/sun6i-csi/sun6i_csi_capture.c | 36 +++++++++++++++++--
> 1 file changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> index 69578075421c..54beba4d2b2f 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> @@ -666,13 +666,43 @@ static int sun6i_csi_capture_enum_fmt(struct file *file, void *private,
> struct v4l2_fmtdesc *fmtdesc)
> {
> u32 index = fmtdesc->index;
> + unsigned int i;
>
> if (index >= ARRAY_SIZE(sun6i_csi_capture_formats))
> return -EINVAL;
>
> - fmtdesc->pixelformat = sun6i_csi_capture_formats[index].pixelformat;
> + for (i = 0; i < ARRAY_SIZE(sun6i_csi_capture_formats); i++) {
> + const struct sun6i_csi_capture_format *format =
> + &sun6i_csi_capture_formats[i];
>
> - return 0;
> + /*
> + * If a media bus code is specified, only consider formats that
> + * match it.
> + */
> + if (fmtdesc->mbus_code) {
> + unsigned int j;
> +
> + if (!format->mbus_codes)
> + continue;
> +
> + for (j = 0; format->mbus_codes[j]; j++) {
> + if (fmtdesc->mbus_code == format->mbus_codes[j])
> + break;
> + }
> +
> + if (!format->mbus_codes[j])
> + continue;
> + }
> +
> + if (index == 0) {
> + fmtdesc->pixelformat = format->pixelformat;
> + return 0;
> + }
> +
> + index--;
> + }
> +
> + return -EINVAL;
> }
>
> static int sun6i_csi_capture_g_fmt(struct file *file, void *private,
> @@ -978,7 +1008,7 @@ int sun6i_csi_capture_setup(struct sun6i_csi_device *csi_dev)
>
> strscpy(video_dev->name, SUN6I_CSI_CAPTURE_NAME,
> sizeof(video_dev->name));
> - video_dev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> + video_dev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING | V4L2_CAP_IO_MC;
Let's avoid long lines:
video_dev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING
| V4L2_CAP_IO_MC;
With this change and with an updated commit message you can add my
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
in the next version of this patch.
> video_dev->vfl_dir = VFL_DIR_RX;
> video_dev->release = video_device_release_empty;
> video_dev->fops = &sun6i_csi_capture_fops;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] media: sun6i-csi: implement vidioc_enum_framesizes
2023-01-06 19:40 ` [PATCH 3/3] media: sun6i-csi: implement vidioc_enum_framesizes adam
@ 2023-01-06 23:35 ` Laurent Pinchart
2023-02-22 10:43 ` Paul Kocialkowski
0 siblings, 1 reply; 18+ messages in thread
From: Laurent Pinchart @ 2023-01-06 23:35 UTC (permalink / raw)
To: adam; +Cc: linux-media, yong.deng, mchehab, linux-sunxi, paul.kocialkowski
Hi Adam,
Thank you for the patch.
On Fri, Jan 06, 2023 at 07:40:38PM +0000, adam@piggz.co.uk wrote:
> From: Adam Pigg <adam@piggz.co.uk>
>
> Create sun6i_csi_capture_enum_framesizes which defines the min
> and max frame sizes
With the commit message updated (see review of 1/3),
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Adam Pigg <adam@piggz.co.uk>
> ---
> .../sunxi/sun6i-csi/sun6i_csi_capture.c | 24 +++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> index 54beba4d2b2f..2be761e6b650 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> @@ -739,6 +739,29 @@ static int sun6i_csi_capture_try_fmt(struct file *file, void *private,
> return 0;
> }
>
> +static int sun6i_csi_capture_enum_framesizes(struct file *file, void *fh,
> + struct v4l2_frmsizeenum *fsize)
> +{
> + const struct sun6i_csi_capture_format *format;
> +
> + if (fsize->index > 0)
> + return -EINVAL;
> +
> + format = sun6i_csi_capture_format_find(fsize->pixel_format);
> + if (!format)
> + return -EINVAL;
> +
> + fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS;
> + fsize->stepwise.min_width = SUN6I_CSI_CAPTURE_WIDTH_MIN;
> + fsize->stepwise.max_width = SUN6I_CSI_CAPTURE_WIDTH_MAX;
> + fsize->stepwise.min_height = SUN6I_CSI_CAPTURE_HEIGHT_MIN;
> + fsize->stepwise.max_height = SUN6I_CSI_CAPTURE_HEIGHT_MAX;
> + fsize->stepwise.step_width = 1;
> + fsize->stepwise.step_height = 1;
> +
> + return 0;
> +}
> +
> static int sun6i_csi_capture_enum_input(struct file *file, void *private,
> struct v4l2_input *input)
> {
> @@ -775,6 +798,7 @@ static const struct v4l2_ioctl_ops sun6i_csi_capture_ioctl_ops = {
> .vidioc_g_fmt_vid_cap = sun6i_csi_capture_g_fmt,
> .vidioc_s_fmt_vid_cap = sun6i_csi_capture_s_fmt,
> .vidioc_try_fmt_vid_cap = sun6i_csi_capture_try_fmt,
> + .vidioc_enum_framesizes = sun6i_csi_capture_enum_framesizes,
>
> .vidioc_enum_input = sun6i_csi_capture_enum_input,
> .vidioc_g_input = sun6i_csi_capture_g_input,
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] suns6-csi changes to support libcamera
2023-01-06 23:10 ` [PATCH 0/3] suns6-csi changes to support libcamera piggz1
@ 2023-01-07 8:43 ` Adam Pigg
0 siblings, 0 replies; 18+ messages in thread
From: Adam Pigg @ 2023-01-07 8:43 UTC (permalink / raw)
To: linux-media, piggz1
Cc: mchehab, linux-sunxi, paul.kocialkowski, laurent.pinchart, yong.deng
[-- Attachment #1: Type: text/plain, Size: 1513 bytes --]
On Friday, 6 January 2023 23:10:46 GMT piggz1@gmail.com wrote:
> Apologies, accidentally sent this set twice. please ignore this set.
>
Ignore this, the first set I sent myself, and I didnt actually send it twice to
the list!
Also, if you see 2 email addresses, adam@piggz.co.uk is a forwarding address
to GMail. I could see how to make git-send-email set the preferred sender
address, only the from address.
> Adam
>
> On Friday, 6 January 2023, adam@piggz.co.uk wrote:
>
> > From: Adam Pigg <adam@piggz.co.uk>
> >
> > This 3 patch set adds V4L2_CAP_IO_MC and vidioc_enum_framesizes support
> > to the sun6i-csi driver, allowing the Pinephone rear camera (ov5640) to
> > be supported.
> >
> > These have been developed with the guidance of Laurent Pinchart, and
> > been tested by capturing frames from the libcamera cam utility.
> >
> > In addition to these, a patch to change the v4l gain mode in the ov5640
> > driver, and the 2 format propagation patches all by Laurnet are
> > required.
> >
> > Adam Pigg (3):
> >
> > media: sun6i-csi: merge sun6i_csi_formats and sun6i_csi_formats_match
> > media: sun6i-csi: implement V4L2_CAP_IO_MC
> > media: sun6i-csi: implement vidioc_enum_framesizes
> >
> >
> >
> > .../sunxi/sun6i-csi/sun6i_csi_capture.c | 215 +++++++++---------
> > .../sunxi/sun6i-csi/sun6i_csi_capture.h | 6 +-
> > 2 files changed, 103 insertions(+), 118 deletions(-)
> >
> >
> > --
> > 2.39.0
> >
> >
> >
>
>
> --
> Sent from my Sailfish device
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] media: sun6i-csi: merge sun6i_csi_formats and sun6i_csi_formats_match
2023-01-06 23:31 ` Laurent Pinchart
@ 2023-01-08 18:23 ` Adam Pigg
2023-01-08 20:26 ` Laurent Pinchart
0 siblings, 1 reply; 18+ messages in thread
From: Adam Pigg @ 2023-01-08 18:23 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, yong.deng, mchehab, linux-sunxi, paul.kocialkowski
[-- Attachment #1: Type: text/plain, Size: 16262 bytes --]
On Friday, 6 January 2023 23:31:48 GMT Laurent Pinchart wrote:
> Hi Adam,
>
> Thank you for the patch.
>
> On Fri, Jan 06, 2023 at 07:40:36PM +0000, adam@piggz.co.uk wrote:
> > From: Adam Pigg <adam@piggz.co.uk>
> >
> > Merged the two format arrays into sun6i_csi_capture_formats as a
> > pre-requisite to implementing V4L2_CAP_IO_MC
>
> I'll point to https://cbea.ms/git-commit/ if you haven't read it yet.
> You can ignore the "Limit the subject line to 50 characters" rule and go
> up to the normal 72 characters limit for commit messages, as 50 doesn't
> include the prefixes commonly used in kernel commit messages.
>
> For this specific patch, I would write
>
> Information about media bus formats and pixel formats supported by the
> driver is split between the sun6i_csi_capture_formats and
> sun6i_csi_capture_format_matches arrays. This makes it difficult to map
> media bus formats to pixel formats when enumerating the supported pixel
> formats by walking the sun6i_csi_capture_formats array. To prepare for
> support of media bus format support in sun6i_csi_capture_enum_fmt(),
> merge the two arrays toegether.
>
>
> An extra paragraph could be added to explain *how* this is being done,
> but the implementation is straightforward enough to not require that.
>
> > Signed-off-by: Adam Pigg <adam@piggz.co.uk>
> > ---
> >
> > .../sunxi/sun6i-csi/sun6i_csi_capture.c | 155 +++++-------------
> > .../sunxi/sun6i-csi/sun6i_csi_capture.h | 6 +-
> > 2 files changed, 46 insertions(+), 115 deletions(-)
> >
> > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> > b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c index
> > 03d4adec054c..69578075421c 100644
> > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> > @@ -22,6 +22,8 @@
> >
> > /* Helpers */
> >
> > +#define SUN6I_BUS_FMTS(fmt...) (const u32[]) {fmt, 0}
> > +
> >
> > void sun6i_csi_capture_dimensions(struct sun6i_csi_device *csi_dev,
> >
> > unsigned int *width, unsigned
int *height)
> >
> > {
> >
> > @@ -49,72 +51,86 @@ static const struct sun6i_csi_capture_format
> > sun6i_csi_capture_formats[] = {>
> > .pixelformat = V4L2_PIX_FMT_SBGGR8,
> > .output_format_frame =
SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> >
> > + .mbus_codes =
SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SBGGR8_1X8),
> >
> > },
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_SGBRG8,
> > .output_format_frame =
SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> >
> > + .mbus_codes =
SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGBRG8_1X8),
> >
> > },
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_SGRBG8,
> > .output_format_frame =
SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> >
> > + .mbus_codes =
SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGRBG8_1X8),
> >
> > },
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_SRGGB8,
> > .output_format_frame =
SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> >
> > + .mbus_codes =
SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SRGGB8_1X8),
> >
> > },
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_SBGGR10,
> > .output_format_frame =
SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_10,
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_10,
> >
> > + .mbus_codes =
SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SBGGR10_1X10),
> >
> > },
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_SGBRG10,
> > .output_format_frame =
SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_10,
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_10,
> >
> > + .mbus_codes =
SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGBRG10_1X10),
> >
> > },
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_SGRBG10,
> > .output_format_frame =
SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_10,
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_10,
> >
> > + .mbus_codes =
SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGRBG10_1X10),
> >
> > },
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_SRGGB10,
> > .output_format_frame =
SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_10,
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_10,
> >
> > + .mbus_codes =
SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SRGGB10_1X10),
> >
> > },
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_SBGGR12,
> > .output_format_frame =
SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_12,
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_12,
> >
> > + .mbus_codes =
SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SBGGR12_1X12),
> >
> > },
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_SGBRG12,
> > .output_format_frame =
SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_12,
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_12,
> >
> > + .mbus_codes =
SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGBRG12_1X12),
> >
> > },
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_SGRBG12,
> > .output_format_frame =
SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_12,
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_12,
> >
> > + .mbus_codes =
SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGRBG12_1X12),
> >
> > },
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_SRGGB12,
> > .output_format_frame =
SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_12,
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_12,
> >
> > + .mbus_codes =
SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SRGGB12_1X12),
> >
> > },
> > /* RGB */
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_RGB565,
> > .output_format_frame =
SUN6I_CSI_OUTPUT_FMT_FRAME_RGB565,
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_RGB565,
> >
> > + .mbus_codes =
SUN6I_BUS_FMTS(MEDIA_BUS_FMT_RGB565_2X8_LE),
> >
> > },
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_RGB565X,
> > .output_format_frame =
SUN6I_CSI_OUTPUT_FMT_FRAME_RGB565,
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_RGB565,
> >
> > + .mbus_codes =
SUN6I_BUS_FMTS(MEDIA_BUS_FMT_RGB565_2X8_BE),
> >
> > },
> > /* YUV422 */
> > {
> >
> > @@ -123,6 +139,8 @@ static const struct sun6i_csi_capture_format
> > sun6i_csi_capture_formats[] = {>
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > .input_format_raw = true,
> > .hsize_len_factor = 2,
> >
> > + .mbus_codes =
SUN6I_BUS_FMTS(MEDIA_BUS_FMT_YUYV8_2X8,
> > +
MEDIA_BUS_FMT_YUYV8_1X16),
> >
> > },
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_YVYU,
> >
> > @@ -130,6 +148,8 @@ static const struct sun6i_csi_capture_format
> > sun6i_csi_capture_formats[] = {>
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > .input_format_raw = true,
> > .hsize_len_factor = 2,
> >
> > + .mbus_codes =
SUN6I_BUS_FMTS(MEDIA_BUS_FMT_YVYU8_2X8,
> > +
MEDIA_BUS_FMT_YVYU8_1X16),
> >
> > },
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_UYVY,
> >
> > @@ -137,6 +157,8 @@ static const struct sun6i_csi_capture_format
> > sun6i_csi_capture_formats[] = {>
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > .input_format_raw = true,
> > .hsize_len_factor = 2,
> >
> > + .mbus_codes =
SUN6I_BUS_FMTS(MEDIA_BUS_FMT_UYVY8_2X8,
> > +
MEDIA_BUS_FMT_UYVY8_1X16),
> >
> > },
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_VYUY,
> >
> > @@ -144,57 +166,68 @@ static const struct sun6i_csi_capture_format
> > sun6i_csi_capture_formats[] = {>
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > .input_format_raw = true,
> > .hsize_len_factor = 2,
> >
> > + .mbus_codes =
SUN6I_BUS_FMTS(MEDIA_BUS_FMT_VYUY8_2X8,
> > +
MEDIA_BUS_FMT_VYUY8_1X16),
> >
> > },
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_NV16,
> > .output_format_frame =
SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422SP,
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422SP,
> >
> > + .mbus_codes = 0,
>
> I don't think this is correct. To produce semi-planar or multi-planar
> YUV formats, I believe the CSI needs YUV input. This should thus be
> (unless I'm mistaken)
>
> .mbus_codes =
SUN6I_BUS_FMTS(MEDIA_BUS_FMT_UYVY8_2X8,
>
MEDIA_BUS_FMT_UYVY8_1X16,
>
MEDIA_BUS_FMT_VYUY8_2X8,
>
MEDIA_BUS_FMT_VYUY8_1X16,
>
MEDIA_BUS_FMT_YUYV8_2X8,
>
MEDIA_BUS_FMT_YUYV8_1X16,
>
MEDIA_BUS_FMT_YVYU8_2X8,
>
MEDIA_BUS_FMT_YVYU8_1X16),
>
> and same below.
>
Hi Laurent
Thanks for the help and tips. Ive made all the other changes, which can be
viewed here until i resubmit them https://github.com/sailfish-on-dontbeevil/
kernel-megi/commits/pinephone-libcamera
Im just not quite sure on this one. I think my implementation of merging the
arrays keeps the previous mapping right? In sun6i_csi_capture_format_matches
there is no mapping for the *NV formats, and the remaining ones ive set to 0?
Thanks
> Paul, could you confirm this ?
>
> I'm a bit surprised that the CSI can't shuffle the YUV components for
> packed YUYV formats, but so be it if that's a hardware limitation.
>
> I'm also thinking that a subsequent patch could drop the raw check from
> sun6i_csi_capture_link_validate():
>
> - /* With raw input mode, we need a 1:1 match between input and
output. */
> - if (bridge_format->input_format == SUN6I_CSI_INPUT_FMT_RAW ||
> - capture_format->input_format_raw) {
> - match = sun6i_csi_capture_format_match(pixelformat,
> -
fmt.format.code);
> - if (!match)
> - goto invalid;
> - }
> + /* Make sure the media bus code and pixel format are compatible. */
> + match = sun6i_csi_capture_format_match(pixelformat,
fmt.format.code);
> + if (!match)
> + goto invalid;
>
> > },
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_NV61,
> > .output_format_frame =
SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422SP,
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422SP,
> > .input_yuv_seq_invert = true,
> >
> > + .mbus_codes = 0,
> >
> > },
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_YUV422P,
> > .output_format_frame =
SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422P,
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422P,
> >
> > + .mbus_codes = 0,
> >
> > },
> > /* YUV420 */
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_NV12_16L16,
> > .output_format_frame =
SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420MB,
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420MB,
> >
> > + .mbus_codes = 0,
> >
> > },
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_NV12,
> > .output_format_frame =
SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420SP,
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420SP,
> >
> > + .mbus_codes = 0,
> >
> > },
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_NV21,
> > .output_format_frame =
SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420SP,
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420SP,
> > .input_yuv_seq_invert = true,
> >
> > + .mbus_codes = 0,
> >
> > },
> >
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_YUV420,
> > .output_format_frame =
SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420P,
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420P,
> >
> > + .mbus_codes = 0,
> >
> > },
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_YVU420,
> > .output_format_frame =
SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420P,
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420P,
> > .input_yuv_seq_invert = true,
> >
> > + .mbus_codes = 0,
> >
> > },
> > /* Compressed */
> > {
> >
> > .pixelformat = V4L2_PIX_FMT_JPEG,
> > .output_format_frame =
SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> > .output_format_field =
SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> >
> > + .mbus_codes =
SUN6I_BUS_FMTS(MEDIA_BUS_FMT_JPEG_1X8),
> >
> > },
> >
> > };
> >
> > @@ -210,118 +243,20 @@ struct sun6i_csi_capture_format
> > *sun6i_csi_capture_format_find(u32 pixelformat)>
> > return NULL;
> >
> > }
> >
> > -/* RAW formats need an exact match between pixel and mbus formats. */
> > -static const
> > -struct sun6i_csi_capture_format_match sun6i_csi_capture_format_matches[]
> > = { - /* YUV420 */
> > - {
> > - .pixelformat = V4L2_PIX_FMT_YUYV,
> > - .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8,
> > - },
> > - {
> > - .pixelformat = V4L2_PIX_FMT_YUYV,
> > - .mbus_code = MEDIA_BUS_FMT_YUYV8_1X16,
> > - },
> > - {
> > - .pixelformat = V4L2_PIX_FMT_YVYU,
> > - .mbus_code = MEDIA_BUS_FMT_YVYU8_2X8,
> > - },
> > - {
> > - .pixelformat = V4L2_PIX_FMT_YVYU,
> > - .mbus_code = MEDIA_BUS_FMT_YVYU8_1X16,
> > - },
> > - {
> > - .pixelformat = V4L2_PIX_FMT_UYVY,
> > - .mbus_code = MEDIA_BUS_FMT_UYVY8_2X8,
> > - },
> > - {
> > - .pixelformat = V4L2_PIX_FMT_UYVY,
> > - .mbus_code = MEDIA_BUS_FMT_UYVY8_1X16,
> > - },
> > - {
> > - .pixelformat = V4L2_PIX_FMT_VYUY,
> > - .mbus_code = MEDIA_BUS_FMT_VYUY8_2X8,
> > - },
> > - {
> > - .pixelformat = V4L2_PIX_FMT_VYUY,
> > - .mbus_code = MEDIA_BUS_FMT_VYUY8_1X16,
> > - },
> > - /* RGB */
> > - {
> > - .pixelformat = V4L2_PIX_FMT_RGB565,
> > - .mbus_code = MEDIA_BUS_FMT_RGB565_2X8_LE,
> > - },
> > - {
> > - .pixelformat = V4L2_PIX_FMT_RGB565X,
> > - .mbus_code = MEDIA_BUS_FMT_RGB565_2X8_BE,
> > - },
> > - /* Bayer */
> > - {
> > - .pixelformat = V4L2_PIX_FMT_SBGGR8,
> > - .mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8,
> > - },
> > - {
> > - .pixelformat = V4L2_PIX_FMT_SGBRG8,
> > - .mbus_code = MEDIA_BUS_FMT_SGBRG8_1X8,
> > - },
> > - {
> > - .pixelformat = V4L2_PIX_FMT_SGRBG8,
> > - .mbus_code = MEDIA_BUS_FMT_SGRBG8_1X8,
> > - },
> > - {
> > - .pixelformat = V4L2_PIX_FMT_SRGGB8,
> > - .mbus_code = MEDIA_BUS_FMT_SRGGB8_1X8,
> > - },
> > - {
> > - .pixelformat = V4L2_PIX_FMT_SBGGR10,
> > - .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10,
> > - },
> > - {
> > - .pixelformat = V4L2_PIX_FMT_SGBRG10,
> > - .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10,
> > - },
> > - {
> > - .pixelformat = V4L2_PIX_FMT_SGRBG10,
> > - .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10,
> > - },
> > - {
> > - .pixelformat = V4L2_PIX_FMT_SRGGB10,
> > - .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10,
> > - },
> > - {
> > - .pixelformat = V4L2_PIX_FMT_SBGGR12,
> > - .mbus_code = MEDIA_BUS_FMT_SBGGR12_1X12,
> > - },
> > - {
> > - .pixelformat = V4L2_PIX_FMT_SGBRG12,
> > - .mbus_code = MEDIA_BUS_FMT_SGBRG12_1X12,
> > - },
> > - {
> > - .pixelformat = V4L2_PIX_FMT_SGRBG12,
> > - .mbus_code = MEDIA_BUS_FMT_SGRBG12_1X12,
> > - },
> > - {
> > - .pixelformat = V4L2_PIX_FMT_SRGGB12,
> > - .mbus_code = MEDIA_BUS_FMT_SRGGB12_1X12,
> > - },
> > - /* Compressed */
> > - {
> > - .pixelformat = V4L2_PIX_FMT_JPEG,
> > - .mbus_code = MEDIA_BUS_FMT_JPEG_1X8,
> > - },
> > -};
> > -
> >
> > static bool sun6i_csi_capture_format_match(u32 pixelformat, u32
> > mbus_code)
> > {
> >
> > - unsigned int i;
> > -
> > - for (i = 0; i < ARRAY_SIZE(sun6i_csi_capture_format_matches); i++)
{
> > - const struct sun6i_csi_capture_format_match *match =
> > - &sun6i_csi_capture_format_matches[i];
> > -
> > - if (match->pixelformat == pixelformat &&
> > - match->mbus_code == mbus_code)
> > - return true;
> > + unsigned int i, j;
> > +
> > + for (i = 0; i < ARRAY_SIZE(sun6i_csi_capture_formats); i++) {
> > + const struct sun6i_csi_capture_format *format =
> > + &sun6i_csi_capture_formats[i];
> > +
> > + if (format->pixelformat == pixelformat) {
> > + for (j = 0; format->mbus_codes[j]; j++) {
> > + if (mbus_code == format-
>mbus_codes[j])
> > + return true;
> > + }
> > + }
> >
> > }
> >
> > return false;
> >
> > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h index
> > 3ee5ccefbd10..0484942834e3 100644
> > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > @@ -27,11 +27,7 @@ struct sun6i_csi_capture_format {
> >
> > bool input_yuv_seq_invert;
> > bool input_format_raw;
> > u32 hsize_len_factor;
> >
> > -};
> > -
> > -struct sun6i_csi_capture_format_match {
> > - u32 pixelformat;
> > - u32 mbus_code;
> > + const u32 *mbus_codes;
> >
> > };
> >
> > #undef current
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] media: sun6i-csi: merge sun6i_csi_formats and sun6i_csi_formats_match
2023-01-08 18:23 ` Adam Pigg
@ 2023-01-08 20:26 ` Laurent Pinchart
2023-02-17 18:20 ` Laurent Pinchart
0 siblings, 1 reply; 18+ messages in thread
From: Laurent Pinchart @ 2023-01-08 20:26 UTC (permalink / raw)
To: Adam Pigg; +Cc: linux-media, yong.deng, mchehab, linux-sunxi, paul.kocialkowski
Hi Adam,
On Sun, Jan 08, 2023 at 06:23:56PM +0000, Adam Pigg wrote:
> On Friday, 6 January 2023 23:31:48 GMT Laurent Pinchart wrote:
> > On Fri, Jan 06, 2023 at 07:40:36PM +0000, adam@piggz.co.uk wrote:
> > > From: Adam Pigg <adam@piggz.co.uk>
> > >
> > > Merged the two format arrays into sun6i_csi_capture_formats as a
> > > pre-requisite to implementing V4L2_CAP_IO_MC
> >
> > I'll point to https://cbea.ms/git-commit/ if you haven't read it yet.
> > You can ignore the "Limit the subject line to 50 characters" rule and go
> > up to the normal 72 characters limit for commit messages, as 50 doesn't
> > include the prefixes commonly used in kernel commit messages.
> >
> > For this specific patch, I would write
> >
> > Information about media bus formats and pixel formats supported by the
> > driver is split between the sun6i_csi_capture_formats and
> > sun6i_csi_capture_format_matches arrays. This makes it difficult to map
> > media bus formats to pixel formats when enumerating the supported pixel
> > formats by walking the sun6i_csi_capture_formats array. To prepare for
> > support of media bus format support in sun6i_csi_capture_enum_fmt(),
> > merge the two arrays toegether.
> >
> >
> > An extra paragraph could be added to explain *how* this is being done,
> > but the implementation is straightforward enough to not require that.
> >
> > > Signed-off-by: Adam Pigg <adam@piggz.co.uk>
> > > ---
> > >
> > > .../sunxi/sun6i-csi/sun6i_csi_capture.c | 155 +++++-------------
> > > .../sunxi/sun6i-csi/sun6i_csi_capture.h | 6 +-
> > > 2 files changed, 46 insertions(+), 115 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> > > b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c index
> > > 03d4adec054c..69578075421c 100644
> > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> > > @@ -22,6 +22,8 @@
> > >
> > > /* Helpers */
> > >
> > > +#define SUN6I_BUS_FMTS(fmt...) (const u32[]) {fmt, 0}
> > > +
> > >
> > > void sun6i_csi_capture_dimensions(struct sun6i_csi_device *csi_dev,
> > >
> > > unsigned int *width, unsigned int *height)
> > >
> > > {
> > >
> > > @@ -49,72 +51,86 @@ static const struct sun6i_csi_capture_format
> > > sun6i_csi_capture_formats[] = {>
> > > .pixelformat = V4L2_PIX_FMT_SBGGR8,
> > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > >
> > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SBGGR8_1X8),
> > >
> > > },
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_SGBRG8,
> > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > >
> > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGBRG8_1X8),
> > >
> > > },
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_SGRBG8,
> > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > >
> > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGRBG8_1X8),
> > >
> > > },
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_SRGGB8,
> > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > >
> > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SRGGB8_1X8),
> > >
> > > },
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_SBGGR10,
> > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_10,
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_10,
> > >
> > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SBGGR10_1X10),
> > >
> > > },
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_SGBRG10,
> > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_10,
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_10,
> > >
> > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGBRG10_1X10),
> > >
> > > },
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_SGRBG10,
> > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_10,
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_10,
> > >
> > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGRBG10_1X10),
> > >
> > > },
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_SRGGB10,
> > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_10,
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_10,
> > >
> > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SRGGB10_1X10),
> > >
> > > },
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_SBGGR12,
> > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_12,
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_12,
> > >
> > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SBGGR12_1X12),
> > >
> > > },
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_SGBRG12,
> > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_12,
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_12,
> > >
> > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGBRG12_1X12),
> > >
> > > },
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_SGRBG12,
> > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_12,
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_12,
> > >
> > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGRBG12_1X12),
> > >
> > > },
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_SRGGB12,
> > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_12,
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_12,
> > >
> > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SRGGB12_1X12),
> > >
> > > },
> > > /* RGB */
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_RGB565,
> > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RGB565,
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RGB565,
> > >
> > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_RGB565_2X8_LE),
> > >
> > > },
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_RGB565X,
> > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RGB565,
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RGB565,
> > >
> > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_RGB565_2X8_BE),
> > >
> > > },
> > > /* YUV422 */
> > > {
> > >
> > > @@ -123,6 +139,8 @@ static const struct sun6i_csi_capture_format
> > > sun6i_csi_capture_formats[] = {>
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > .input_format_raw = true,
> > > .hsize_len_factor = 2,
> > >
> > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_YUYV8_2X8,
> > > + MEDIA_BUS_FMT_YUYV8_1X16),
> > >
> > > },
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_YVYU,
> > >
> > > @@ -130,6 +148,8 @@ static const struct sun6i_csi_capture_format
> > > sun6i_csi_capture_formats[] = {>
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > .input_format_raw = true,
> > > .hsize_len_factor = 2,
> > >
> > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_YVYU8_2X8,
> > > + MEDIA_BUS_FMT_YVYU8_1X16),
> > >
> > > },
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_UYVY,
> > >
> > > @@ -137,6 +157,8 @@ static const struct sun6i_csi_capture_format
> > > sun6i_csi_capture_formats[] = {
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > .input_format_raw = true,
> > > .hsize_len_factor = 2,
> > >
> > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_UYVY8_2X8,
> > > + MEDIA_BUS_FMT_UYVY8_1X16),
> > >
> > > },
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_VYUY,
> > >
> > > @@ -144,57 +166,68 @@ static const struct sun6i_csi_capture_format
> > > sun6i_csi_capture_formats[] = {>
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > .input_format_raw = true,
> > > .hsize_len_factor = 2,
> > >
> > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_VYUY8_2X8,
> > > + MEDIA_BUS_FMT_VYUY8_1X16),
> > >
> > > },
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_NV16,
> > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422SP,
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422SP,
> > >
> > > + .mbus_codes = 0,
> >
> > I don't think this is correct. To produce semi-planar or multi-planar
> > YUV formats, I believe the CSI needs YUV input. This should thus be
> > (unless I'm mistaken)
> >
> > .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_UYVY8_2X8,
> > MEDIA_BUS_FMT_UYVY8_1X16,
> > MEDIA_BUS_FMT_VYUY8_2X8,
> > MEDIA_BUS_FMT_VYUY8_1X16,
> > MEDIA_BUS_FMT_YUYV8_2X8,
> > MEDIA_BUS_FMT_YUYV8_1X16,
> > MEDIA_BUS_FMT_YVYU8_2X8,
> > MEDIA_BUS_FMT_YVYU8_1X16),
> >
> > and same below.
> >
> Hi Laurent
>
> Thanks for the help and tips. Ive made all the other changes, which can be
> viewed here until i resubmit them https://github.com/sailfish-on-dontbeevil/
> kernel-megi/commits/pinephone-libcamera
>
> Im just not quite sure on this one. I think my implementation of merging the
> arrays keeps the previous mapping right? In sun6i_csi_capture_format_matches
> there is no mapping for the *NV formats, and the remaining ones ive set to 0?
The current implementation allows writing multi-planar formats (e.g.
NV12) to memory when the input of the CSI is a YUV media bus format
(e.g. YUYV8_1X16). This patch doesn't change that, but it will prevent
NV12 from being enumerated when using media bus-based enumeration of
pixel formats, so userspace won't see NV12 as being available.
It would be fine fixing that issue in a separate patch on top of this
one, but I though you could as well do both in one go.
> > Paul, could you confirm this ?
> >
> > I'm a bit surprised that the CSI can't shuffle the YUV components for
> > packed YUYV formats, but so be it if that's a hardware limitation.
> >
> > I'm also thinking that a subsequent patch could drop the raw check from
> > sun6i_csi_capture_link_validate():
> >
> > - /* With raw input mode, we need a 1:1 match between input and output. */
> > - if (bridge_format->input_format == SUN6I_CSI_INPUT_FMT_RAW ||
> > - capture_format->input_format_raw) {
> > - match = sun6i_csi_capture_format_match(pixelformat,
> > - fmt.format.code);
> > - if (!match)
> > - goto invalid;
> > - }
> > + /* Make sure the media bus code and pixel format are compatible. */
> > + match = sun6i_csi_capture_format_match(pixelformat, fmt.format.code);
> > + if (!match)
> > + goto invalid;
> >
> > > },
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_NV61,
> > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422SP,
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422SP,
> > > .input_yuv_seq_invert = true,
> > >
> > > + .mbus_codes = 0,
> > >
> > > },
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_YUV422P,
> > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422P,
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422P,
> > >
> > > + .mbus_codes = 0,
> > >
> > > },
> > > /* YUV420 */
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_NV12_16L16,
> > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420MB,
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420MB,
> > >
> > > + .mbus_codes = 0,
> > >
> > > },
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_NV12,
> > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420SP,
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420SP,
> > >
> > > + .mbus_codes = 0,
> > >
> > > },
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_NV21,
> > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420SP,
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420SP,
> > > .input_yuv_seq_invert = true,
> > >
> > > + .mbus_codes = 0,
> > >
> > > },
> > >
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_YUV420,
> > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420P,
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420P,
> > >
> > > + .mbus_codes = 0,
> > >
> > > },
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_YVU420,
> > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420P,
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420P,
> > > .input_yuv_seq_invert = true,
> > >
> > > + .mbus_codes = 0,
> > >
> > > },
> > > /* Compressed */
> > > {
> > >
> > > .pixelformat = V4L2_PIX_FMT_JPEG,
> > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > >
> > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_JPEG_1X8),
> > >
> > > },
> > >
> > > };
> > >
> > > @@ -210,118 +243,20 @@ struct sun6i_csi_capture_format
> > > *sun6i_csi_capture_format_find(u32 pixelformat)>
> > > return NULL;
> > >
> > > }
> > >
> > > -/* RAW formats need an exact match between pixel and mbus formats. */
> > > -static const
> > > -struct sun6i_csi_capture_format_match sun6i_csi_capture_format_matches[]
> > > = { - /* YUV420 */
> > > - {
> > > - .pixelformat = V4L2_PIX_FMT_YUYV,
> > > - .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8,
> > > - },
> > > - {
> > > - .pixelformat = V4L2_PIX_FMT_YUYV,
> > > - .mbus_code = MEDIA_BUS_FMT_YUYV8_1X16,
> > > - },
> > > - {
> > > - .pixelformat = V4L2_PIX_FMT_YVYU,
> > > - .mbus_code = MEDIA_BUS_FMT_YVYU8_2X8,
> > > - },
> > > - {
> > > - .pixelformat = V4L2_PIX_FMT_YVYU,
> > > - .mbus_code = MEDIA_BUS_FMT_YVYU8_1X16,
> > > - },
> > > - {
> > > - .pixelformat = V4L2_PIX_FMT_UYVY,
> > > - .mbus_code = MEDIA_BUS_FMT_UYVY8_2X8,
> > > - },
> > > - {
> > > - .pixelformat = V4L2_PIX_FMT_UYVY,
> > > - .mbus_code = MEDIA_BUS_FMT_UYVY8_1X16,
> > > - },
> > > - {
> > > - .pixelformat = V4L2_PIX_FMT_VYUY,
> > > - .mbus_code = MEDIA_BUS_FMT_VYUY8_2X8,
> > > - },
> > > - {
> > > - .pixelformat = V4L2_PIX_FMT_VYUY,
> > > - .mbus_code = MEDIA_BUS_FMT_VYUY8_1X16,
> > > - },
> > > - /* RGB */
> > > - {
> > > - .pixelformat = V4L2_PIX_FMT_RGB565,
> > > - .mbus_code = MEDIA_BUS_FMT_RGB565_2X8_LE,
> > > - },
> > > - {
> > > - .pixelformat = V4L2_PIX_FMT_RGB565X,
> > > - .mbus_code = MEDIA_BUS_FMT_RGB565_2X8_BE,
> > > - },
> > > - /* Bayer */
> > > - {
> > > - .pixelformat = V4L2_PIX_FMT_SBGGR8,
> > > - .mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8,
> > > - },
> > > - {
> > > - .pixelformat = V4L2_PIX_FMT_SGBRG8,
> > > - .mbus_code = MEDIA_BUS_FMT_SGBRG8_1X8,
> > > - },
> > > - {
> > > - .pixelformat = V4L2_PIX_FMT_SGRBG8,
> > > - .mbus_code = MEDIA_BUS_FMT_SGRBG8_1X8,
> > > - },
> > > - {
> > > - .pixelformat = V4L2_PIX_FMT_SRGGB8,
> > > - .mbus_code = MEDIA_BUS_FMT_SRGGB8_1X8,
> > > - },
> > > - {
> > > - .pixelformat = V4L2_PIX_FMT_SBGGR10,
> > > - .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10,
> > > - },
> > > - {
> > > - .pixelformat = V4L2_PIX_FMT_SGBRG10,
> > > - .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10,
> > > - },
> > > - {
> > > - .pixelformat = V4L2_PIX_FMT_SGRBG10,
> > > - .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10,
> > > - },
> > > - {
> > > - .pixelformat = V4L2_PIX_FMT_SRGGB10,
> > > - .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10,
> > > - },
> > > - {
> > > - .pixelformat = V4L2_PIX_FMT_SBGGR12,
> > > - .mbus_code = MEDIA_BUS_FMT_SBGGR12_1X12,
> > > - },
> > > - {
> > > - .pixelformat = V4L2_PIX_FMT_SGBRG12,
> > > - .mbus_code = MEDIA_BUS_FMT_SGBRG12_1X12,
> > > - },
> > > - {
> > > - .pixelformat = V4L2_PIX_FMT_SGRBG12,
> > > - .mbus_code = MEDIA_BUS_FMT_SGRBG12_1X12,
> > > - },
> > > - {
> > > - .pixelformat = V4L2_PIX_FMT_SRGGB12,
> > > - .mbus_code = MEDIA_BUS_FMT_SRGGB12_1X12,
> > > - },
> > > - /* Compressed */
> > > - {
> > > - .pixelformat = V4L2_PIX_FMT_JPEG,
> > > - .mbus_code = MEDIA_BUS_FMT_JPEG_1X8,
> > > - },
> > > -};
> > > -
> > >
> > > static bool sun6i_csi_capture_format_match(u32 pixelformat, u32
> > > mbus_code)
> > > {
> > >
> > > - unsigned int i;
> > > -
> > > - for (i = 0; i < ARRAY_SIZE(sun6i_csi_capture_format_matches); i++) {
> > > - const struct sun6i_csi_capture_format_match *match =
> > > - &sun6i_csi_capture_format_matches[i];
> > > -
> > > - if (match->pixelformat == pixelformat &&
> > > - match->mbus_code == mbus_code)
> > > - return true;
> > > + unsigned int i, j;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(sun6i_csi_capture_formats); i++) {
> > > + const struct sun6i_csi_capture_format *format =
> > > + &sun6i_csi_capture_formats[i];
> > > +
> > > + if (format->pixelformat == pixelformat) {
> > > + for (j = 0; format->mbus_codes[j]; j++) {
> > > + if (mbus_code == format->mbus_codes[j])
> > > + return true;
> > > + }
> > > + }
> > >
> > > }
> > >
> > > return false;
> > >
> > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > > b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h index
> > > 3ee5ccefbd10..0484942834e3 100644
> > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > > @@ -27,11 +27,7 @@ struct sun6i_csi_capture_format {
> > >
> > > bool input_yuv_seq_invert;
> > > bool input_format_raw;
> > > u32 hsize_len_factor;
> > >
> > > -};
> > > -
> > > -struct sun6i_csi_capture_format_match {
> > > - u32 pixelformat;
> > > - u32 mbus_code;
> > > + const u32 *mbus_codes;
> > >
> > > };
> > >
> > > #undef current
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] media: sun6i-csi: merge sun6i_csi_formats and sun6i_csi_formats_match
2023-01-08 20:26 ` Laurent Pinchart
@ 2023-02-17 18:20 ` Laurent Pinchart
2023-02-22 10:39 ` Paul Kocialkowski
0 siblings, 1 reply; 18+ messages in thread
From: Laurent Pinchart @ 2023-02-17 18:20 UTC (permalink / raw)
To: paul.kocialkowski; +Cc: Adam Pigg, linux-media, yong.deng, mchehab, linux-sunxi
Hi Paul,
Could you share your opinion on the question below ?
On Sun, Jan 08, 2023 at 10:26:09PM +0200, Laurent Pinchart wrote:
> On Sun, Jan 08, 2023 at 06:23:56PM +0000, Adam Pigg wrote:
> > On Friday, 6 January 2023 23:31:48 GMT Laurent Pinchart wrote:
> > > On Fri, Jan 06, 2023 at 07:40:36PM +0000, adam@piggz.co.uk wrote:
> > > > From: Adam Pigg <adam@piggz.co.uk>
> > > >
> > > > Merged the two format arrays into sun6i_csi_capture_formats as a
> > > > pre-requisite to implementing V4L2_CAP_IO_MC
> > >
> > > I'll point to https://cbea.ms/git-commit/ if you haven't read it yet.
> > > You can ignore the "Limit the subject line to 50 characters" rule and go
> > > up to the normal 72 characters limit for commit messages, as 50 doesn't
> > > include the prefixes commonly used in kernel commit messages.
> > >
> > > For this specific patch, I would write
> > >
> > > Information about media bus formats and pixel formats supported by the
> > > driver is split between the sun6i_csi_capture_formats and
> > > sun6i_csi_capture_format_matches arrays. This makes it difficult to map
> > > media bus formats to pixel formats when enumerating the supported pixel
> > > formats by walking the sun6i_csi_capture_formats array. To prepare for
> > > support of media bus format support in sun6i_csi_capture_enum_fmt(),
> > > merge the two arrays toegether.
> > >
> > >
> > > An extra paragraph could be added to explain *how* this is being done,
> > > but the implementation is straightforward enough to not require that.
> > >
> > > > Signed-off-by: Adam Pigg <adam@piggz.co.uk>
> > > > ---
> > > >
> > > > .../sunxi/sun6i-csi/sun6i_csi_capture.c | 155 +++++-------------
> > > > .../sunxi/sun6i-csi/sun6i_csi_capture.h | 6 +-
> > > > 2 files changed, 46 insertions(+), 115 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> > > > b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c index
> > > > 03d4adec054c..69578075421c 100644
> > > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> > > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> > > > @@ -22,6 +22,8 @@
> > > >
> > > > /* Helpers */
> > > >
> > > > +#define SUN6I_BUS_FMTS(fmt...) (const u32[]) {fmt, 0}
> > > > +
> > > >
> > > > void sun6i_csi_capture_dimensions(struct sun6i_csi_device *csi_dev,
> > > >
> > > > unsigned int *width, unsigned int *height)
> > > >
> > > > {
> > > >
> > > > @@ -49,72 +51,86 @@ static const struct sun6i_csi_capture_format
> > > > sun6i_csi_capture_formats[] = {>
> > > > .pixelformat = V4L2_PIX_FMT_SBGGR8,
> > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > >
> > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SBGGR8_1X8),
> > > >
> > > > },
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_SGBRG8,
> > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > >
> > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGBRG8_1X8),
> > > >
> > > > },
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_SGRBG8,
> > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > >
> > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGRBG8_1X8),
> > > >
> > > > },
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_SRGGB8,
> > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > >
> > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SRGGB8_1X8),
> > > >
> > > > },
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_SBGGR10,
> > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_10,
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_10,
> > > >
> > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SBGGR10_1X10),
> > > >
> > > > },
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_SGBRG10,
> > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_10,
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_10,
> > > >
> > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGBRG10_1X10),
> > > >
> > > > },
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_SGRBG10,
> > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_10,
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_10,
> > > >
> > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGRBG10_1X10),
> > > >
> > > > },
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_SRGGB10,
> > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_10,
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_10,
> > > >
> > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SRGGB10_1X10),
> > > >
> > > > },
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_SBGGR12,
> > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_12,
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_12,
> > > >
> > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SBGGR12_1X12),
> > > >
> > > > },
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_SGBRG12,
> > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_12,
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_12,
> > > >
> > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGBRG12_1X12),
> > > >
> > > > },
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_SGRBG12,
> > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_12,
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_12,
> > > >
> > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SGRBG12_1X12),
> > > >
> > > > },
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_SRGGB12,
> > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_12,
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_12,
> > > >
> > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_SRGGB12_1X12),
> > > >
> > > > },
> > > > /* RGB */
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_RGB565,
> > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RGB565,
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RGB565,
> > > >
> > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_RGB565_2X8_LE),
> > > >
> > > > },
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_RGB565X,
> > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RGB565,
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RGB565,
> > > >
> > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_RGB565_2X8_BE),
> > > >
> > > > },
> > > > /* YUV422 */
> > > > {
> > > >
> > > > @@ -123,6 +139,8 @@ static const struct sun6i_csi_capture_format
> > > > sun6i_csi_capture_formats[] = {>
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > .input_format_raw = true,
> > > > .hsize_len_factor = 2,
> > > >
> > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_YUYV8_2X8,
> > > > + MEDIA_BUS_FMT_YUYV8_1X16),
> > > >
> > > > },
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_YVYU,
> > > >
> > > > @@ -130,6 +148,8 @@ static const struct sun6i_csi_capture_format
> > > > sun6i_csi_capture_formats[] = {>
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > .input_format_raw = true,
> > > > .hsize_len_factor = 2,
> > > >
> > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_YVYU8_2X8,
> > > > + MEDIA_BUS_FMT_YVYU8_1X16),
> > > >
> > > > },
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_UYVY,
> > > >
> > > > @@ -137,6 +157,8 @@ static const struct sun6i_csi_capture_format
> > > > sun6i_csi_capture_formats[] = {
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > .input_format_raw = true,
> > > > .hsize_len_factor = 2,
> > > >
> > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_UYVY8_2X8,
> > > > + MEDIA_BUS_FMT_UYVY8_1X16),
> > > >
> > > > },
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_VYUY,
> > > >
> > > > @@ -144,57 +166,68 @@ static const struct sun6i_csi_capture_format
> > > > sun6i_csi_capture_formats[] = {>
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > .input_format_raw = true,
> > > > .hsize_len_factor = 2,
> > > >
> > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_VYUY8_2X8,
> > > > + MEDIA_BUS_FMT_VYUY8_1X16),
> > > >
> > > > },
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_NV16,
> > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422SP,
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422SP,
> > > >
> > > > + .mbus_codes = 0,
> > >
> > > I don't think this is correct. To produce semi-planar or multi-planar
> > > YUV formats, I believe the CSI needs YUV input. This should thus be
> > > (unless I'm mistaken)
> > >
> > > .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_UYVY8_2X8,
> > > MEDIA_BUS_FMT_UYVY8_1X16,
> > > MEDIA_BUS_FMT_VYUY8_2X8,
> > > MEDIA_BUS_FMT_VYUY8_1X16,
> > > MEDIA_BUS_FMT_YUYV8_2X8,
> > > MEDIA_BUS_FMT_YUYV8_1X16,
> > > MEDIA_BUS_FMT_YVYU8_2X8,
> > > MEDIA_BUS_FMT_YVYU8_1X16),
> > >
> > > and same below.
> > >
> > Hi Laurent
> >
> > Thanks for the help and tips. Ive made all the other changes, which can be
> > viewed here until i resubmit them https://github.com/sailfish-on-dontbeevil/
> > kernel-megi/commits/pinephone-libcamera
> >
> > Im just not quite sure on this one. I think my implementation of merging the
> > arrays keeps the previous mapping right? In sun6i_csi_capture_format_matches
> > there is no mapping for the *NV formats, and the remaining ones ive set to 0?
>
> The current implementation allows writing multi-planar formats (e.g.
> NV12) to memory when the input of the CSI is a YUV media bus format
> (e.g. YUYV8_1X16). This patch doesn't change that, but it will prevent
> NV12 from being enumerated when using media bus-based enumeration of
> pixel formats, so userspace won't see NV12 as being available.
>
> It would be fine fixing that issue in a separate patch on top of this
> one, but I though you could as well do both in one go.
Adam, you mentioned that NV12 and NV16 "don't work". Could you elaborate
and explain what you've tried exactly ?
> > > Paul, could you confirm this ?
> > >
> > > I'm a bit surprised that the CSI can't shuffle the YUV components for
> > > packed YUYV formats, but so be it if that's a hardware limitation.
> > >
> > > I'm also thinking that a subsequent patch could drop the raw check from
> > > sun6i_csi_capture_link_validate():
> > >
> > > - /* With raw input mode, we need a 1:1 match between input and output. */
> > > - if (bridge_format->input_format == SUN6I_CSI_INPUT_FMT_RAW ||
> > > - capture_format->input_format_raw) {
> > > - match = sun6i_csi_capture_format_match(pixelformat,
> > > - fmt.format.code);
> > > - if (!match)
> > > - goto invalid;
> > > - }
> > > + /* Make sure the media bus code and pixel format are compatible. */
> > > + match = sun6i_csi_capture_format_match(pixelformat, fmt.format.code);
> > > + if (!match)
> > > + goto invalid;
> > >
> > > > },
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_NV61,
> > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422SP,
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422SP,
> > > > .input_yuv_seq_invert = true,
> > > >
> > > > + .mbus_codes = 0,
> > > >
> > > > },
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_YUV422P,
> > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422P,
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422P,
> > > >
> > > > + .mbus_codes = 0,
> > > >
> > > > },
> > > > /* YUV420 */
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_NV12_16L16,
> > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420MB,
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420MB,
> > > >
> > > > + .mbus_codes = 0,
> > > >
> > > > },
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_NV12,
> > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420SP,
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420SP,
> > > >
> > > > + .mbus_codes = 0,
> > > >
> > > > },
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_NV21,
> > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420SP,
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420SP,
> > > > .input_yuv_seq_invert = true,
> > > >
> > > > + .mbus_codes = 0,
> > > >
> > > > },
> > > >
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_YUV420,
> > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420P,
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420P,
> > > >
> > > > + .mbus_codes = 0,
> > > >
> > > > },
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_YVU420,
> > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420P,
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420P,
> > > > .input_yuv_seq_invert = true,
> > > >
> > > > + .mbus_codes = 0,
> > > >
> > > > },
> > > > /* Compressed */
> > > > {
> > > >
> > > > .pixelformat = V4L2_PIX_FMT_JPEG,
> > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > >
> > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_JPEG_1X8),
> > > >
> > > > },
> > > >
> > > > };
> > > >
> > > > @@ -210,118 +243,20 @@ struct sun6i_csi_capture_format
> > > > *sun6i_csi_capture_format_find(u32 pixelformat)>
> > > > return NULL;
> > > >
> > > > }
> > > >
> > > > -/* RAW formats need an exact match between pixel and mbus formats. */
> > > > -static const
> > > > -struct sun6i_csi_capture_format_match sun6i_csi_capture_format_matches[]
> > > > = { - /* YUV420 */
> > > > - {
> > > > - .pixelformat = V4L2_PIX_FMT_YUYV,
> > > > - .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8,
> > > > - },
> > > > - {
> > > > - .pixelformat = V4L2_PIX_FMT_YUYV,
> > > > - .mbus_code = MEDIA_BUS_FMT_YUYV8_1X16,
> > > > - },
> > > > - {
> > > > - .pixelformat = V4L2_PIX_FMT_YVYU,
> > > > - .mbus_code = MEDIA_BUS_FMT_YVYU8_2X8,
> > > > - },
> > > > - {
> > > > - .pixelformat = V4L2_PIX_FMT_YVYU,
> > > > - .mbus_code = MEDIA_BUS_FMT_YVYU8_1X16,
> > > > - },
> > > > - {
> > > > - .pixelformat = V4L2_PIX_FMT_UYVY,
> > > > - .mbus_code = MEDIA_BUS_FMT_UYVY8_2X8,
> > > > - },
> > > > - {
> > > > - .pixelformat = V4L2_PIX_FMT_UYVY,
> > > > - .mbus_code = MEDIA_BUS_FMT_UYVY8_1X16,
> > > > - },
> > > > - {
> > > > - .pixelformat = V4L2_PIX_FMT_VYUY,
> > > > - .mbus_code = MEDIA_BUS_FMT_VYUY8_2X8,
> > > > - },
> > > > - {
> > > > - .pixelformat = V4L2_PIX_FMT_VYUY,
> > > > - .mbus_code = MEDIA_BUS_FMT_VYUY8_1X16,
> > > > - },
> > > > - /* RGB */
> > > > - {
> > > > - .pixelformat = V4L2_PIX_FMT_RGB565,
> > > > - .mbus_code = MEDIA_BUS_FMT_RGB565_2X8_LE,
> > > > - },
> > > > - {
> > > > - .pixelformat = V4L2_PIX_FMT_RGB565X,
> > > > - .mbus_code = MEDIA_BUS_FMT_RGB565_2X8_BE,
> > > > - },
> > > > - /* Bayer */
> > > > - {
> > > > - .pixelformat = V4L2_PIX_FMT_SBGGR8,
> > > > - .mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8,
> > > > - },
> > > > - {
> > > > - .pixelformat = V4L2_PIX_FMT_SGBRG8,
> > > > - .mbus_code = MEDIA_BUS_FMT_SGBRG8_1X8,
> > > > - },
> > > > - {
> > > > - .pixelformat = V4L2_PIX_FMT_SGRBG8,
> > > > - .mbus_code = MEDIA_BUS_FMT_SGRBG8_1X8,
> > > > - },
> > > > - {
> > > > - .pixelformat = V4L2_PIX_FMT_SRGGB8,
> > > > - .mbus_code = MEDIA_BUS_FMT_SRGGB8_1X8,
> > > > - },
> > > > - {
> > > > - .pixelformat = V4L2_PIX_FMT_SBGGR10,
> > > > - .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10,
> > > > - },
> > > > - {
> > > > - .pixelformat = V4L2_PIX_FMT_SGBRG10,
> > > > - .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10,
> > > > - },
> > > > - {
> > > > - .pixelformat = V4L2_PIX_FMT_SGRBG10,
> > > > - .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10,
> > > > - },
> > > > - {
> > > > - .pixelformat = V4L2_PIX_FMT_SRGGB10,
> > > > - .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10,
> > > > - },
> > > > - {
> > > > - .pixelformat = V4L2_PIX_FMT_SBGGR12,
> > > > - .mbus_code = MEDIA_BUS_FMT_SBGGR12_1X12,
> > > > - },
> > > > - {
> > > > - .pixelformat = V4L2_PIX_FMT_SGBRG12,
> > > > - .mbus_code = MEDIA_BUS_FMT_SGBRG12_1X12,
> > > > - },
> > > > - {
> > > > - .pixelformat = V4L2_PIX_FMT_SGRBG12,
> > > > - .mbus_code = MEDIA_BUS_FMT_SGRBG12_1X12,
> > > > - },
> > > > - {
> > > > - .pixelformat = V4L2_PIX_FMT_SRGGB12,
> > > > - .mbus_code = MEDIA_BUS_FMT_SRGGB12_1X12,
> > > > - },
> > > > - /* Compressed */
> > > > - {
> > > > - .pixelformat = V4L2_PIX_FMT_JPEG,
> > > > - .mbus_code = MEDIA_BUS_FMT_JPEG_1X8,
> > > > - },
> > > > -};
> > > > -
> > > >
> > > > static bool sun6i_csi_capture_format_match(u32 pixelformat, u32
> > > > mbus_code)
> > > > {
> > > >
> > > > - unsigned int i;
> > > > -
> > > > - for (i = 0; i < ARRAY_SIZE(sun6i_csi_capture_format_matches); i++) {
> > > > - const struct sun6i_csi_capture_format_match *match =
> > > > - &sun6i_csi_capture_format_matches[i];
> > > > -
> > > > - if (match->pixelformat == pixelformat &&
> > > > - match->mbus_code == mbus_code)
> > > > - return true;
> > > > + unsigned int i, j;
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(sun6i_csi_capture_formats); i++) {
> > > > + const struct sun6i_csi_capture_format *format =
> > > > + &sun6i_csi_capture_formats[i];
> > > > +
> > > > + if (format->pixelformat == pixelformat) {
> > > > + for (j = 0; format->mbus_codes[j]; j++) {
> > > > + if (mbus_code == format->mbus_codes[j])
> > > > + return true;
> > > > + }
> > > > + }
> > > >
> > > > }
> > > >
> > > > return false;
> > > >
> > > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > > > b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h index
> > > > 3ee5ccefbd10..0484942834e3 100644
> > > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > > > @@ -27,11 +27,7 @@ struct sun6i_csi_capture_format {
> > > >
> > > > bool input_yuv_seq_invert;
> > > > bool input_format_raw;
> > > > u32 hsize_len_factor;
> > > >
> > > > -};
> > > > -
> > > > -struct sun6i_csi_capture_format_match {
> > > > - u32 pixelformat;
> > > > - u32 mbus_code;
> > > > + const u32 *mbus_codes;
> > > >
> > > > };
> > > >
> > > > #undef current
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] media: sun6i-csi: merge sun6i_csi_formats and sun6i_csi_formats_match
2023-02-17 18:20 ` Laurent Pinchart
@ 2023-02-22 10:39 ` Paul Kocialkowski
2023-03-21 17:50 ` Adam Pigg
2023-03-25 23:17 ` Laurent Pinchart
0 siblings, 2 replies; 18+ messages in thread
From: Paul Kocialkowski @ 2023-02-22 10:39 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Adam Pigg, linux-media, yong.deng, mchehab, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 15871 bytes --]
Hi Adam, Laurent,
On Fri 17 Feb 23, 20:20, Laurent Pinchart wrote:
> Could you share your opinion on the question below ?
Yeah sorry for the long delay here. It's taken me a while to dive back into
this topic.
My first impression is that I would rather be in favor of keeping a dynamic
approach like what's already in sun6i_csi_capture_link_validate and extract
the mbus/pixfmt matching logic from there.
I would be happy to craft a patch in that direction, but maybe you'd like
to have a try at it (since it's your series). Just let me know, I think I can
do it pretty quickly now that I have everything back in mind. I could also
add some comment about the general hardware mechanism/limitations.
The hardware is a bit odd in a few ways and I found that the explicit
combinatory approach wasn't a very good fit (but obviously that's an open topic
for discussions).
The general idea is that the SUN6I_CSI_INPUT_FMT_YUV420/422 can only be used
to produce outputs on 2 or 3 planes, but not packed YUV. There's also no
explicit hardware reordering of the chroma components, so we need to lie about
the input order (input_yuv_seq_invert) to achieve inverted chroma components
on the output format.
In order to produce packed data, we can only rely on SUN6I_CSI_INPUT_FMT_RAW
which provides no reordering mechanism. This is why it made good sense to me
to only have an explicit matching table for this case and rely on more general
logic that reflects the hardware capabilities otherwise.
> On Sun, Jan 08, 2023 at 10:26:09PM +0200, Laurent Pinchart wrote:
> > On Sun, Jan 08, 2023 at 06:23:56PM +0000, Adam Pigg wrote:
> > > On Friday, 6 January 2023 23:31:48 GMT Laurent Pinchart wrote:
> > > > On Fri, Jan 06, 2023 at 07:40:36PM +0000, adam@piggz.co.uk wrote:
[...]
> > > > > +#define SUN6I_BUS_FMTS(fmt...) (const u32[]) {fmt, 0}
Cosmetic suggestion to stay consistent with the rest:
#define SUN6I_CSI_CAPTURE_MBUS_CODES(mbus_codes...) \
(const u32[]) { mbus_codes, 0 }
Also it would look better in sun6i_csi_capture.h.
But really I would be happier with a dynamic approach.
[...]
> > > > > /* YUV422 */
> > > > > {
> > > > >
> > > > > @@ -123,6 +139,8 @@ static const struct sun6i_csi_capture_format
> > > > > sun6i_csi_capture_formats[] = {>
> > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > > .input_format_raw = true,
> > > > > .hsize_len_factor = 2,
> > > > >
> > > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_YUYV8_2X8,
> > > > > + MEDIA_BUS_FMT_YUYV8_1X16),
> > > > >
> > > > > },
> > > > > {
> > > > >
> > > > > .pixelformat = V4L2_PIX_FMT_YVYU,
> > > > >
> > > > > @@ -130,6 +148,8 @@ static const struct sun6i_csi_capture_format
> > > > > sun6i_csi_capture_formats[] = {>
> > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > > .input_format_raw = true,
> > > > > .hsize_len_factor = 2,
> > > > >
> > > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_YVYU8_2X8,
> > > > > + MEDIA_BUS_FMT_YVYU8_1X16),
> > > > >
> > > > > },
> > > > > {
> > > > >
> > > > > .pixelformat = V4L2_PIX_FMT_UYVY,
> > > > >
> > > > > @@ -137,6 +157,8 @@ static const struct sun6i_csi_capture_format
> > > > > sun6i_csi_capture_formats[] = {
> > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > > .input_format_raw = true,
> > > > > .hsize_len_factor = 2,
> > > > >
> > > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_UYVY8_2X8,
> > > > > + MEDIA_BUS_FMT_UYVY8_1X16),
> > > > >
> > > > > },
> > > > > {
> > > > >
> > > > > .pixelformat = V4L2_PIX_FMT_VYUY,
> > > > >
> > > > > @@ -144,57 +166,68 @@ static const struct sun6i_csi_capture_format
> > > > > sun6i_csi_capture_formats[] = {>
> > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > > .input_format_raw = true,
> > > > > .hsize_len_factor = 2,
> > > > >
> > > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_VYUY8_2X8,
> > > > > + MEDIA_BUS_FMT_VYUY8_1X16),
> > > > >
> > > > > },
> > > > > {
> > > > >
> > > > > .pixelformat = V4L2_PIX_FMT_NV16,
> > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422SP,
> > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422SP,
> > > > >
> > > > > + .mbus_codes = 0,
> > > >
> > > > I don't think this is correct. To produce semi-planar or multi-planar
> > > > YUV formats, I believe the CSI needs YUV input. This should thus be
> > > > (unless I'm mistaken)
You are correct.
> > > >
> > > > .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_UYVY8_2X8,
> > > > MEDIA_BUS_FMT_UYVY8_1X16,
> > > > MEDIA_BUS_FMT_VYUY8_2X8,
> > > > MEDIA_BUS_FMT_VYUY8_1X16,
> > > > MEDIA_BUS_FMT_YUYV8_2X8,
> > > > MEDIA_BUS_FMT_YUYV8_1X16,
> > > > MEDIA_BUS_FMT_YVYU8_2X8,
> > > > MEDIA_BUS_FMT_YVYU8_1X16),
> > > >
> > > > and same below.
All of the YUV420/422 pixel formats on 2 or 3 planes can take all of the
supports packed 16-bit YUV bus formats, which is why it doesn't seem very
graceful to have an explicit list.
> > > >
> > > Hi Laurent
> > >
> > > Thanks for the help and tips. Ive made all the other changes, which can be
> > > viewed here until i resubmit them https://github.com/sailfish-on-dontbeevil/
> > > kernel-megi/commits/pinephone-libcamera
> > >
> > > Im just not quite sure on this one. I think my implementation of merging the
> > > arrays keeps the previous mapping right? In sun6i_csi_capture_format_matches
> > > there is no mapping for the *NV formats, and the remaining ones ive set to 0?
> >
> > The current implementation allows writing multi-planar formats (e.g.
> > NV12) to memory when the input of the CSI is a YUV media bus format
> > (e.g. YUYV8_1X16). This patch doesn't change that, but it will prevent
> > NV12 from being enumerated when using media bus-based enumeration of
> > pixel formats, so userspace won't see NV12 as being available.
> >
> > It would be fine fixing that issue in a separate patch on top of this
> > one, but I though you could as well do both in one go.
>
> Adam, you mentioned that NV12 and NV16 "don't work". Could you elaborate
> and explain what you've tried exactly ?
We definitely need to keep the ability to produce NV12, NV16 and friends from
YUV bus formats.
> > > > Paul, could you confirm this ?
> > > >
> > > > I'm a bit surprised that the CSI can't shuffle the YUV components for
> > > > packed YUYV formats, but so be it if that's a hardware limitation.
Yep that is correct, it's a hardware limitation.
Cheers,
Paul
> > > > I'm also thinking that a subsequent patch could drop the raw check from
> > > > sun6i_csi_capture_link_validate():
> > > >
> > > > - /* With raw input mode, we need a 1:1 match between input and output. */
> > > > - if (bridge_format->input_format == SUN6I_CSI_INPUT_FMT_RAW ||
> > > > - capture_format->input_format_raw) {
> > > > - match = sun6i_csi_capture_format_match(pixelformat,
> > > > - fmt.format.code);
> > > > - if (!match)
> > > > - goto invalid;
> > > > - }
> > > > + /* Make sure the media bus code and pixel format are compatible. */
> > > > + match = sun6i_csi_capture_format_match(pixelformat, fmt.format.code);
> > > > + if (!match)
> > > > + goto invalid;
> > > >
> > > > > },
> > > > > {
> > > > >
> > > > > .pixelformat = V4L2_PIX_FMT_NV61,
> > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422SP,
> > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422SP,
> > > > > .input_yuv_seq_invert = true,
> > > > >
> > > > > + .mbus_codes = 0,
> > > > >
> > > > > },
> > > > > {
> > > > >
> > > > > .pixelformat = V4L2_PIX_FMT_YUV422P,
> > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422P,
> > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422P,
> > > > >
> > > > > + .mbus_codes = 0,
> > > > >
> > > > > },
> > > > > /* YUV420 */
> > > > > {
> > > > >
> > > > > .pixelformat = V4L2_PIX_FMT_NV12_16L16,
> > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420MB,
> > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420MB,
> > > > >
> > > > > + .mbus_codes = 0,
> > > > >
> > > > > },
> > > > > {
> > > > >
> > > > > .pixelformat = V4L2_PIX_FMT_NV12,
> > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420SP,
> > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420SP,
> > > > >
> > > > > + .mbus_codes = 0,
> > > > >
> > > > > },
> > > > > {
> > > > >
> > > > > .pixelformat = V4L2_PIX_FMT_NV21,
> > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420SP,
> > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420SP,
> > > > > .input_yuv_seq_invert = true,
> > > > >
> > > > > + .mbus_codes = 0,
> > > > >
> > > > > },
> > > > >
> > > > > {
> > > > >
> > > > > .pixelformat = V4L2_PIX_FMT_YUV420,
> > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420P,
> > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420P,
> > > > >
> > > > > + .mbus_codes = 0,
> > > > >
> > > > > },
> > > > > {
> > > > >
> > > > > .pixelformat = V4L2_PIX_FMT_YVU420,
> > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420P,
> > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420P,
> > > > > .input_yuv_seq_invert = true,
> > > > >
> > > > > + .mbus_codes = 0,
> > > > >
> > > > > },
> > > > > /* Compressed */
> > > > > {
> > > > >
> > > > > .pixelformat = V4L2_PIX_FMT_JPEG,
> > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > >
> > > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_JPEG_1X8),
> > > > >
> > > > > },
> > > > >
> > > > > };
> > > > >
> > > > > @@ -210,118 +243,20 @@ struct sun6i_csi_capture_format
> > > > > *sun6i_csi_capture_format_find(u32 pixelformat)>
> > > > > return NULL;
> > > > >
> > > > > }
> > > > >
> > > > > -/* RAW formats need an exact match between pixel and mbus formats. */
> > > > > -static const
> > > > > -struct sun6i_csi_capture_format_match sun6i_csi_capture_format_matches[]
> > > > > = { - /* YUV420 */
> > > > > - {
> > > > > - .pixelformat = V4L2_PIX_FMT_YUYV,
> > > > > - .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8,
> > > > > - },
> > > > > - {
> > > > > - .pixelformat = V4L2_PIX_FMT_YUYV,
> > > > > - .mbus_code = MEDIA_BUS_FMT_YUYV8_1X16,
> > > > > - },
> > > > > - {
> > > > > - .pixelformat = V4L2_PIX_FMT_YVYU,
> > > > > - .mbus_code = MEDIA_BUS_FMT_YVYU8_2X8,
> > > > > - },
> > > > > - {
> > > > > - .pixelformat = V4L2_PIX_FMT_YVYU,
> > > > > - .mbus_code = MEDIA_BUS_FMT_YVYU8_1X16,
> > > > > - },
> > > > > - {
> > > > > - .pixelformat = V4L2_PIX_FMT_UYVY,
> > > > > - .mbus_code = MEDIA_BUS_FMT_UYVY8_2X8,
> > > > > - },
> > > > > - {
> > > > > - .pixelformat = V4L2_PIX_FMT_UYVY,
> > > > > - .mbus_code = MEDIA_BUS_FMT_UYVY8_1X16,
> > > > > - },
> > > > > - {
> > > > > - .pixelformat = V4L2_PIX_FMT_VYUY,
> > > > > - .mbus_code = MEDIA_BUS_FMT_VYUY8_2X8,
> > > > > - },
> > > > > - {
> > > > > - .pixelformat = V4L2_PIX_FMT_VYUY,
> > > > > - .mbus_code = MEDIA_BUS_FMT_VYUY8_1X16,
> > > > > - },
> > > > > - /* RGB */
> > > > > - {
> > > > > - .pixelformat = V4L2_PIX_FMT_RGB565,
> > > > > - .mbus_code = MEDIA_BUS_FMT_RGB565_2X8_LE,
> > > > > - },
> > > > > - {
> > > > > - .pixelformat = V4L2_PIX_FMT_RGB565X,
> > > > > - .mbus_code = MEDIA_BUS_FMT_RGB565_2X8_BE,
> > > > > - },
> > > > > - /* Bayer */
> > > > > - {
> > > > > - .pixelformat = V4L2_PIX_FMT_SBGGR8,
> > > > > - .mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8,
> > > > > - },
> > > > > - {
> > > > > - .pixelformat = V4L2_PIX_FMT_SGBRG8,
> > > > > - .mbus_code = MEDIA_BUS_FMT_SGBRG8_1X8,
> > > > > - },
> > > > > - {
> > > > > - .pixelformat = V4L2_PIX_FMT_SGRBG8,
> > > > > - .mbus_code = MEDIA_BUS_FMT_SGRBG8_1X8,
> > > > > - },
> > > > > - {
> > > > > - .pixelformat = V4L2_PIX_FMT_SRGGB8,
> > > > > - .mbus_code = MEDIA_BUS_FMT_SRGGB8_1X8,
> > > > > - },
> > > > > - {
> > > > > - .pixelformat = V4L2_PIX_FMT_SBGGR10,
> > > > > - .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10,
> > > > > - },
> > > > > - {
> > > > > - .pixelformat = V4L2_PIX_FMT_SGBRG10,
> > > > > - .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10,
> > > > > - },
> > > > > - {
> > > > > - .pixelformat = V4L2_PIX_FMT_SGRBG10,
> > > > > - .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10,
> > > > > - },
> > > > > - {
> > > > > - .pixelformat = V4L2_PIX_FMT_SRGGB10,
> > > > > - .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10,
> > > > > - },
> > > > > - {
> > > > > - .pixelformat = V4L2_PIX_FMT_SBGGR12,
> > > > > - .mbus_code = MEDIA_BUS_FMT_SBGGR12_1X12,
> > > > > - },
> > > > > - {
> > > > > - .pixelformat = V4L2_PIX_FMT_SGBRG12,
> > > > > - .mbus_code = MEDIA_BUS_FMT_SGBRG12_1X12,
> > > > > - },
> > > > > - {
> > > > > - .pixelformat = V4L2_PIX_FMT_SGRBG12,
> > > > > - .mbus_code = MEDIA_BUS_FMT_SGRBG12_1X12,
> > > > > - },
> > > > > - {
> > > > > - .pixelformat = V4L2_PIX_FMT_SRGGB12,
> > > > > - .mbus_code = MEDIA_BUS_FMT_SRGGB12_1X12,
> > > > > - },
> > > > > - /* Compressed */
> > > > > - {
> > > > > - .pixelformat = V4L2_PIX_FMT_JPEG,
> > > > > - .mbus_code = MEDIA_BUS_FMT_JPEG_1X8,
> > > > > - },
> > > > > -};
> > > > > -
> > > > >
> > > > > static bool sun6i_csi_capture_format_match(u32 pixelformat, u32
> > > > > mbus_code)
> > > > > {
> > > > >
> > > > > - unsigned int i;
> > > > > -
> > > > > - for (i = 0; i < ARRAY_SIZE(sun6i_csi_capture_format_matches); i++) {
> > > > > - const struct sun6i_csi_capture_format_match *match =
> > > > > - &sun6i_csi_capture_format_matches[i];
> > > > > -
> > > > > - if (match->pixelformat == pixelformat &&
> > > > > - match->mbus_code == mbus_code)
> > > > > - return true;
> > > > > + unsigned int i, j;
> > > > > +
> > > > > + for (i = 0; i < ARRAY_SIZE(sun6i_csi_capture_formats); i++) {
> > > > > + const struct sun6i_csi_capture_format *format =
> > > > > + &sun6i_csi_capture_formats[i];
> > > > > +
> > > > > + if (format->pixelformat == pixelformat) {
> > > > > + for (j = 0; format->mbus_codes[j]; j++) {
> > > > > + if (mbus_code == format->mbus_codes[j])
> > > > > + return true;
> > > > > + }
> > > > > + }
> > > > >
> > > > > }
> > > > >
> > > > > return false;
> > > > >
> > > > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > > > > b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h index
> > > > > 3ee5ccefbd10..0484942834e3 100644
> > > > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > > > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > > > > @@ -27,11 +27,7 @@ struct sun6i_csi_capture_format {
> > > > >
> > > > > bool input_yuv_seq_invert;
> > > > > bool input_format_raw;
> > > > > u32 hsize_len_factor;
> > > > >
> > > > > -};
> > > > > -
> > > > > -struct sun6i_csi_capture_format_match {
> > > > > - u32 pixelformat;
> > > > > - u32 mbus_code;
> > > > > + const u32 *mbus_codes;
> > > > >
> > > > > };
> > > > >
> > > > > #undef current
>
> --
> Regards,
>
> Laurent Pinchart
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] media: sun6i-csi: implement vidioc_enum_framesizes
2023-01-06 23:35 ` Laurent Pinchart
@ 2023-02-22 10:43 ` Paul Kocialkowski
2023-03-25 23:12 ` Laurent Pinchart
0 siblings, 1 reply; 18+ messages in thread
From: Paul Kocialkowski @ 2023-02-22 10:43 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: adam, linux-media, yong.deng, mchehab, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 3042 bytes --]
Hi,
On Sat 07 Jan 23, 01:35, Laurent Pinchart wrote:
> Hi Adam,
>
> Thank you for the patch.
>
> On Fri, Jan 06, 2023 at 07:40:38PM +0000, adam@piggz.co.uk wrote:
> > From: Adam Pigg <adam@piggz.co.uk>
> >
> > Create sun6i_csi_capture_enum_framesizes which defines the min
> > and max frame sizes
>
> With the commit message updated (see review of 1/3),
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
I'm always a bit confused regarding how such an ioctl's behavior should depend
on the attached subdev. Is it well-defined behavior that this here is only
about the receiver part and has nothing to do with what the connected sensor?
> > Signed-off-by: Adam Pigg <adam@piggz.co.uk>
> > ---
> > .../sunxi/sun6i-csi/sun6i_csi_capture.c | 24 +++++++++++++++++++
> > 1 file changed, 24 insertions(+)
> >
> > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> > index 54beba4d2b2f..2be761e6b650 100644
> > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> > @@ -739,6 +739,29 @@ static int sun6i_csi_capture_try_fmt(struct file *file, void *private,
> > return 0;
> > }
> >
> > +static int sun6i_csi_capture_enum_framesizes(struct file *file, void *fh,
> > + struct v4l2_frmsizeenum *fsize)
A cosmetic/consistency suggestion would be to name this variable "frmsize" to
reuse part of the name of the structure, which is what I've done in other places
of the driver.
Cheers,
Paul
> > +{
> > + const struct sun6i_csi_capture_format *format;
> > +
> > + if (fsize->index > 0)
> > + return -EINVAL;
> > +
> > + format = sun6i_csi_capture_format_find(fsize->pixel_format);
> > + if (!format)
> > + return -EINVAL;
> > +
> > + fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS;
> > + fsize->stepwise.min_width = SUN6I_CSI_CAPTURE_WIDTH_MIN;
> > + fsize->stepwise.max_width = SUN6I_CSI_CAPTURE_WIDTH_MAX;
> > + fsize->stepwise.min_height = SUN6I_CSI_CAPTURE_HEIGHT_MIN;
> > + fsize->stepwise.max_height = SUN6I_CSI_CAPTURE_HEIGHT_MAX;
> > + fsize->stepwise.step_width = 1;
> > + fsize->stepwise.step_height = 1;
> > +
> > + return 0;
> > +}
> > +
> > static int sun6i_csi_capture_enum_input(struct file *file, void *private,
> > struct v4l2_input *input)
> > {
> > @@ -775,6 +798,7 @@ static const struct v4l2_ioctl_ops sun6i_csi_capture_ioctl_ops = {
> > .vidioc_g_fmt_vid_cap = sun6i_csi_capture_g_fmt,
> > .vidioc_s_fmt_vid_cap = sun6i_csi_capture_s_fmt,
> > .vidioc_try_fmt_vid_cap = sun6i_csi_capture_try_fmt,
> > + .vidioc_enum_framesizes = sun6i_csi_capture_enum_framesizes,
> >
> > .vidioc_enum_input = sun6i_csi_capture_enum_input,
> > .vidioc_g_input = sun6i_csi_capture_g_input,
>
> --
> Regards,
>
> Laurent Pinchart
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] media: sun6i-csi: merge sun6i_csi_formats and sun6i_csi_formats_match
2023-02-22 10:39 ` Paul Kocialkowski
@ 2023-03-21 17:50 ` Adam Pigg
2023-03-23 12:55 ` Paul Kocialkowski
2023-03-25 23:17 ` Laurent Pinchart
1 sibling, 1 reply; 18+ messages in thread
From: Adam Pigg @ 2023-03-21 17:50 UTC (permalink / raw)
To: Paul Kocialkowski
Cc: Laurent Pinchart, linux-media, yong.deng, mchehab, linux-sunxi
On Wed, 22 Feb 2023 at 10:39, Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi Adam, Laurent,
>
> On Fri 17 Feb 23, 20:20, Laurent Pinchart wrote:
> > Could you share your opinion on the question below ?
>
> Yeah sorry for the long delay here. It's taken me a while to dive back into
> this topic.
>
> My first impression is that I would rather be in favor of keeping a dynamic
> approach like what's already in sun6i_csi_capture_link_validate and extract
> the mbus/pixfmt matching logic from there.
>
> I would be happy to craft a patch in that direction, but maybe you'd like
> to have a try at it (since it's your series). Just let me know, I think I can
> do it pretty quickly now that I have everything back in mind. I could also
> add some comment about the general hardware mechanism/limitations.
>
Hi Paul
Apologies for missing this email!
As you seem to know the hardware much better, and I was only getting
it working together with
help from others, im more than happy for you to work up the changes,
and it will likely result in
a much shorter change cycle!
Would that be for this patch only in the series, or the other 2 also?
Thanks
> The hardware is a bit odd in a few ways and I found that the explicit
> combinatory approach wasn't a very good fit (but obviously that's an open topic
> for discussions).
>
> The general idea is that the SUN6I_CSI_INPUT_FMT_YUV420/422 can only be used
> to produce outputs on 2 or 3 planes, but not packed YUV. There's also no
> explicit hardware reordering of the chroma components, so we need to lie about
> the input order (input_yuv_seq_invert) to achieve inverted chroma components
> on the output format.
>
> In order to produce packed data, we can only rely on SUN6I_CSI_INPUT_FMT_RAW
> which provides no reordering mechanism. This is why it made good sense to me
> to only have an explicit matching table for this case and rely on more general
> logic that reflects the hardware capabilities otherwise.
>
> > On Sun, Jan 08, 2023 at 10:26:09PM +0200, Laurent Pinchart wrote:
> > > On Sun, Jan 08, 2023 at 06:23:56PM +0000, Adam Pigg wrote:
> > > > On Friday, 6 January 2023 23:31:48 GMT Laurent Pinchart wrote:
> > > > > On Fri, Jan 06, 2023 at 07:40:36PM +0000, adam@piggz.co.uk wrote:
>
> [...]
>
> > > > > > +#define SUN6I_BUS_FMTS(fmt...) (const u32[]) {fmt, 0}
>
> Cosmetic suggestion to stay consistent with the rest:
> #define SUN6I_CSI_CAPTURE_MBUS_CODES(mbus_codes...) \
> (const u32[]) { mbus_codes, 0 }
>
> Also it would look better in sun6i_csi_capture.h.
>
> But really I would be happier with a dynamic approach.
>
> [...]
>
> > > > > > /* YUV422 */
> > > > > > {
> > > > > >
> > > > > > @@ -123,6 +139,8 @@ static const struct sun6i_csi_capture_format
> > > > > > sun6i_csi_capture_formats[] = {>
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > > > .input_format_raw = true,
> > > > > > .hsize_len_factor = 2,
> > > > > >
> > > > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_YUYV8_2X8,
> > > > > > + MEDIA_BUS_FMT_YUYV8_1X16),
> > > > > >
> > > > > > },
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_YVYU,
> > > > > >
> > > > > > @@ -130,6 +148,8 @@ static const struct sun6i_csi_capture_format
> > > > > > sun6i_csi_capture_formats[] = {>
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > > > .input_format_raw = true,
> > > > > > .hsize_len_factor = 2,
> > > > > >
> > > > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_YVYU8_2X8,
> > > > > > + MEDIA_BUS_FMT_YVYU8_1X16),
> > > > > >
> > > > > > },
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_UYVY,
> > > > > >
> > > > > > @@ -137,6 +157,8 @@ static const struct sun6i_csi_capture_format
> > > > > > sun6i_csi_capture_formats[] = {
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > > > .input_format_raw = true,
> > > > > > .hsize_len_factor = 2,
> > > > > >
> > > > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_UYVY8_2X8,
> > > > > > + MEDIA_BUS_FMT_UYVY8_1X16),
> > > > > >
> > > > > > },
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_VYUY,
> > > > > >
> > > > > > @@ -144,57 +166,68 @@ static const struct sun6i_csi_capture_format
> > > > > > sun6i_csi_capture_formats[] = {>
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > > > .input_format_raw = true,
> > > > > > .hsize_len_factor = 2,
> > > > > >
> > > > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_VYUY8_2X8,
> > > > > > + MEDIA_BUS_FMT_VYUY8_1X16),
> > > > > >
> > > > > > },
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_NV16,
> > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422SP,
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422SP,
> > > > > >
> > > > > > + .mbus_codes = 0,
> > > > >
> > > > > I don't think this is correct. To produce semi-planar or multi-planar
> > > > > YUV formats, I believe the CSI needs YUV input. This should thus be
> > > > > (unless I'm mistaken)
>
> You are correct.
>
> > > > >
> > > > > .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_UYVY8_2X8,
> > > > > MEDIA_BUS_FMT_UYVY8_1X16,
> > > > > MEDIA_BUS_FMT_VYUY8_2X8,
> > > > > MEDIA_BUS_FMT_VYUY8_1X16,
> > > > > MEDIA_BUS_FMT_YUYV8_2X8,
> > > > > MEDIA_BUS_FMT_YUYV8_1X16,
> > > > > MEDIA_BUS_FMT_YVYU8_2X8,
> > > > > MEDIA_BUS_FMT_YVYU8_1X16),
> > > > >
> > > > > and same below.
>
> All of the YUV420/422 pixel formats on 2 or 3 planes can take all of the
> supports packed 16-bit YUV bus formats, which is why it doesn't seem very
> graceful to have an explicit list.
>
> > > > >
> > > > Hi Laurent
> > > >
> > > > Thanks for the help and tips. Ive made all the other changes, which can be
> > > > viewed here until i resubmit them https://github.com/sailfish-on-dontbeevil/
> > > > kernel-megi/commits/pinephone-libcamera
> > > >
> > > > Im just not quite sure on this one. I think my implementation of merging the
> > > > arrays keeps the previous mapping right? In sun6i_csi_capture_format_matches
> > > > there is no mapping for the *NV formats, and the remaining ones ive set to 0?
> > >
> > > The current implementation allows writing multi-planar formats (e.g.
> > > NV12) to memory when the input of the CSI is a YUV media bus format
> > > (e.g. YUYV8_1X16). This patch doesn't change that, but it will prevent
> > > NV12 from being enumerated when using media bus-based enumeration of
> > > pixel formats, so userspace won't see NV12 as being available.
> > >
> > > It would be fine fixing that issue in a separate patch on top of this
> > > one, but I though you could as well do both in one go.
> >
> > Adam, you mentioned that NV12 and NV16 "don't work". Could you elaborate
> > and explain what you've tried exactly ?
>
> We definitely need to keep the ability to produce NV12, NV16 and friends from
> YUV bus formats.
>
> > > > > Paul, could you confirm this ?
> > > > >
> > > > > I'm a bit surprised that the CSI can't shuffle the YUV components for
> > > > > packed YUYV formats, but so be it if that's a hardware limitation.
>
> Yep that is correct, it's a hardware limitation.
>
> Cheers,
>
> Paul
>
> > > > > I'm also thinking that a subsequent patch could drop the raw check from
> > > > > sun6i_csi_capture_link_validate():
> > > > >
> > > > > - /* With raw input mode, we need a 1:1 match between input and output. */
> > > > > - if (bridge_format->input_format == SUN6I_CSI_INPUT_FMT_RAW ||
> > > > > - capture_format->input_format_raw) {
> > > > > - match = sun6i_csi_capture_format_match(pixelformat,
> > > > > - fmt.format.code);
> > > > > - if (!match)
> > > > > - goto invalid;
> > > > > - }
> > > > > + /* Make sure the media bus code and pixel format are compatible. */
> > > > > + match = sun6i_csi_capture_format_match(pixelformat, fmt.format.code);
> > > > > + if (!match)
> > > > > + goto invalid;
> > > > >
> > > > > > },
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_NV61,
> > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422SP,
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422SP,
> > > > > > .input_yuv_seq_invert = true,
> > > > > >
> > > > > > + .mbus_codes = 0,
> > > > > >
> > > > > > },
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_YUV422P,
> > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422P,
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422P,
> > > > > >
> > > > > > + .mbus_codes = 0,
> > > > > >
> > > > > > },
> > > > > > /* YUV420 */
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_NV12_16L16,
> > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420MB,
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420MB,
> > > > > >
> > > > > > + .mbus_codes = 0,
> > > > > >
> > > > > > },
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_NV12,
> > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420SP,
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420SP,
> > > > > >
> > > > > > + .mbus_codes = 0,
> > > > > >
> > > > > > },
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_NV21,
> > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420SP,
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420SP,
> > > > > > .input_yuv_seq_invert = true,
> > > > > >
> > > > > > + .mbus_codes = 0,
> > > > > >
> > > > > > },
> > > > > >
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_YUV420,
> > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420P,
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420P,
> > > > > >
> > > > > > + .mbus_codes = 0,
> > > > > >
> > > > > > },
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_YVU420,
> > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420P,
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420P,
> > > > > > .input_yuv_seq_invert = true,
> > > > > >
> > > > > > + .mbus_codes = 0,
> > > > > >
> > > > > > },
> > > > > > /* Compressed */
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_JPEG,
> > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > > >
> > > > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_JPEG_1X8),
> > > > > >
> > > > > > },
> > > > > >
> > > > > > };
> > > > > >
> > > > > > @@ -210,118 +243,20 @@ struct sun6i_csi_capture_format
> > > > > > *sun6i_csi_capture_format_find(u32 pixelformat)>
> > > > > > return NULL;
> > > > > >
> > > > > > }
> > > > > >
> > > > > > -/* RAW formats need an exact match between pixel and mbus formats. */
> > > > > > -static const
> > > > > > -struct sun6i_csi_capture_format_match sun6i_csi_capture_format_matches[]
> > > > > > = { - /* YUV420 */
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_YUYV,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_YUYV,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_YUYV8_1X16,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_YVYU,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_YVYU8_2X8,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_YVYU,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_YVYU8_1X16,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_UYVY,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_UYVY8_2X8,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_UYVY,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_UYVY8_1X16,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_VYUY,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_VYUY8_2X8,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_VYUY,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_VYUY8_1X16,
> > > > > > - },
> > > > > > - /* RGB */
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_RGB565,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_RGB565_2X8_LE,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_RGB565X,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_RGB565_2X8_BE,
> > > > > > - },
> > > > > > - /* Bayer */
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SBGGR8,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SGBRG8,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SGBRG8_1X8,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SGRBG8,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SGRBG8_1X8,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SRGGB8,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SRGGB8_1X8,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SBGGR10,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SGBRG10,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SGRBG10,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SRGGB10,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SBGGR12,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SBGGR12_1X12,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SGBRG12,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SGBRG12_1X12,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SGRBG12,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SGRBG12_1X12,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SRGGB12,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SRGGB12_1X12,
> > > > > > - },
> > > > > > - /* Compressed */
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_JPEG,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_JPEG_1X8,
> > > > > > - },
> > > > > > -};
> > > > > > -
> > > > > >
> > > > > > static bool sun6i_csi_capture_format_match(u32 pixelformat, u32
> > > > > > mbus_code)
> > > > > > {
> > > > > >
> > > > > > - unsigned int i;
> > > > > > -
> > > > > > - for (i = 0; i < ARRAY_SIZE(sun6i_csi_capture_format_matches); i++) {
> > > > > > - const struct sun6i_csi_capture_format_match *match =
> > > > > > - &sun6i_csi_capture_format_matches[i];
> > > > > > -
> > > > > > - if (match->pixelformat == pixelformat &&
> > > > > > - match->mbus_code == mbus_code)
> > > > > > - return true;
> > > > > > + unsigned int i, j;
> > > > > > +
> > > > > > + for (i = 0; i < ARRAY_SIZE(sun6i_csi_capture_formats); i++) {
> > > > > > + const struct sun6i_csi_capture_format *format =
> > > > > > + &sun6i_csi_capture_formats[i];
> > > > > > +
> > > > > > + if (format->pixelformat == pixelformat) {
> > > > > > + for (j = 0; format->mbus_codes[j]; j++) {
> > > > > > + if (mbus_code == format->mbus_codes[j])
> > > > > > + return true;
> > > > > > + }
> > > > > > + }
> > > > > >
> > > > > > }
> > > > > >
> > > > > > return false;
> > > > > >
> > > > > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > > > > > b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h index
> > > > > > 3ee5ccefbd10..0484942834e3 100644
> > > > > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > > > > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > > > > > @@ -27,11 +27,7 @@ struct sun6i_csi_capture_format {
> > > > > >
> > > > > > bool input_yuv_seq_invert;
> > > > > > bool input_format_raw;
> > > > > > u32 hsize_len_factor;
> > > > > >
> > > > > > -};
> > > > > > -
> > > > > > -struct sun6i_csi_capture_format_match {
> > > > > > - u32 pixelformat;
> > > > > > - u32 mbus_code;
> > > > > > + const u32 *mbus_codes;
> > > > > >
> > > > > > };
> > > > > >
> > > > > > #undef current
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
> --
> Paul Kocialkowski, Bootlin
> Embedded Linux and kernel engineering
> https://bootlin.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] media: sun6i-csi: merge sun6i_csi_formats and sun6i_csi_formats_match
2023-03-21 17:50 ` Adam Pigg
@ 2023-03-23 12:55 ` Paul Kocialkowski
0 siblings, 0 replies; 18+ messages in thread
From: Paul Kocialkowski @ 2023-03-23 12:55 UTC (permalink / raw)
To: Adam Pigg; +Cc: Laurent Pinchart, linux-media, yong.deng, mchehab, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 21559 bytes --]
Hi Adam,
On Tue 21 Mar 23, 17:50, Adam Pigg wrote:
> On Wed, 22 Feb 2023 at 10:39, Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> >
> > Hi Adam, Laurent,
> >
> > On Fri 17 Feb 23, 20:20, Laurent Pinchart wrote:
> > > Could you share your opinion on the question below ?
> >
> > Yeah sorry for the long delay here. It's taken me a while to dive back into
> > this topic.
> >
> > My first impression is that I would rather be in favor of keeping a dynamic
> > approach like what's already in sun6i_csi_capture_link_validate and extract
> > the mbus/pixfmt matching logic from there.
> >
> > I would be happy to craft a patch in that direction, but maybe you'd like
> > to have a try at it (since it's your series). Just let me know, I think I can
> > do it pretty quickly now that I have everything back in mind. I could also
> > add some comment about the general hardware mechanism/limitations.
> >
>
> Hi Paul
>
> Apologies for missing this email!
>
> As you seem to know the hardware much better, and I was only getting
> it working together with
> help from others, im more than happy for you to work up the changes,
> and it will likely result in
> a much shorter change cycle!
Glad to hear! I have some patches working already :)
> Would that be for this patch only in the series, or the other 2 also?
That would be for the enum_fmt mbus_code support as well as enum_framesizes.
For the latter, I've made little change to your code so I will add your
Co-authored-by and Signed-off-by lines.
The same changes also apply to the sun6i-isp driver, which I try to keep
in sync with sun6i-csi.
Will send the patches soon (probably tomorrow).
Cheers!
Paul
> Thanks
>
> > The hardware is a bit odd in a few ways and I found that the explicit
> > combinatory approach wasn't a very good fit (but obviously that's an open topic
> > for discussions).
> >
> > The general idea is that the SUN6I_CSI_INPUT_FMT_YUV420/422 can only be used
> > to produce outputs on 2 or 3 planes, but not packed YUV. There's also no
> > explicit hardware reordering of the chroma components, so we need to lie about
> > the input order (input_yuv_seq_invert) to achieve inverted chroma components
> > on the output format.
> >
> > In order to produce packed data, we can only rely on SUN6I_CSI_INPUT_FMT_RAW
> > which provides no reordering mechanism. This is why it made good sense to me
> > to only have an explicit matching table for this case and rely on more general
> > logic that reflects the hardware capabilities otherwise.
> >
> > > On Sun, Jan 08, 2023 at 10:26:09PM +0200, Laurent Pinchart wrote:
> > > > On Sun, Jan 08, 2023 at 06:23:56PM +0000, Adam Pigg wrote:
> > > > > On Friday, 6 January 2023 23:31:48 GMT Laurent Pinchart wrote:
> > > > > > On Fri, Jan 06, 2023 at 07:40:36PM +0000, adam@piggz.co.uk wrote:
> >
> > [...]
> >
> > > > > > > +#define SUN6I_BUS_FMTS(fmt...) (const u32[]) {fmt, 0}
> >
> > Cosmetic suggestion to stay consistent with the rest:
> > #define SUN6I_CSI_CAPTURE_MBUS_CODES(mbus_codes...) \
> > (const u32[]) { mbus_codes, 0 }
> >
> > Also it would look better in sun6i_csi_capture.h.
> >
> > But really I would be happier with a dynamic approach.
> >
> > [...]
> >
> > > > > > > /* YUV422 */
> > > > > > > {
> > > > > > >
> > > > > > > @@ -123,6 +139,8 @@ static const struct sun6i_csi_capture_format
> > > > > > > sun6i_csi_capture_formats[] = {>
> > > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > > > > .input_format_raw = true,
> > > > > > > .hsize_len_factor = 2,
> > > > > > >
> > > > > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_YUYV8_2X8,
> > > > > > > + MEDIA_BUS_FMT_YUYV8_1X16),
> > > > > > >
> > > > > > > },
> > > > > > > {
> > > > > > >
> > > > > > > .pixelformat = V4L2_PIX_FMT_YVYU,
> > > > > > >
> > > > > > > @@ -130,6 +148,8 @@ static const struct sun6i_csi_capture_format
> > > > > > > sun6i_csi_capture_formats[] = {>
> > > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > > > > .input_format_raw = true,
> > > > > > > .hsize_len_factor = 2,
> > > > > > >
> > > > > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_YVYU8_2X8,
> > > > > > > + MEDIA_BUS_FMT_YVYU8_1X16),
> > > > > > >
> > > > > > > },
> > > > > > > {
> > > > > > >
> > > > > > > .pixelformat = V4L2_PIX_FMT_UYVY,
> > > > > > >
> > > > > > > @@ -137,6 +157,8 @@ static const struct sun6i_csi_capture_format
> > > > > > > sun6i_csi_capture_formats[] = {
> > > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > > > > .input_format_raw = true,
> > > > > > > .hsize_len_factor = 2,
> > > > > > >
> > > > > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_UYVY8_2X8,
> > > > > > > + MEDIA_BUS_FMT_UYVY8_1X16),
> > > > > > >
> > > > > > > },
> > > > > > > {
> > > > > > >
> > > > > > > .pixelformat = V4L2_PIX_FMT_VYUY,
> > > > > > >
> > > > > > > @@ -144,57 +166,68 @@ static const struct sun6i_csi_capture_format
> > > > > > > sun6i_csi_capture_formats[] = {>
> > > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > > > > .input_format_raw = true,
> > > > > > > .hsize_len_factor = 2,
> > > > > > >
> > > > > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_VYUY8_2X8,
> > > > > > > + MEDIA_BUS_FMT_VYUY8_1X16),
> > > > > > >
> > > > > > > },
> > > > > > > {
> > > > > > >
> > > > > > > .pixelformat = V4L2_PIX_FMT_NV16,
> > > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422SP,
> > > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422SP,
> > > > > > >
> > > > > > > + .mbus_codes = 0,
> > > > > >
> > > > > > I don't think this is correct. To produce semi-planar or multi-planar
> > > > > > YUV formats, I believe the CSI needs YUV input. This should thus be
> > > > > > (unless I'm mistaken)
> >
> > You are correct.
> >
> > > > > >
> > > > > > .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_UYVY8_2X8,
> > > > > > MEDIA_BUS_FMT_UYVY8_1X16,
> > > > > > MEDIA_BUS_FMT_VYUY8_2X8,
> > > > > > MEDIA_BUS_FMT_VYUY8_1X16,
> > > > > > MEDIA_BUS_FMT_YUYV8_2X8,
> > > > > > MEDIA_BUS_FMT_YUYV8_1X16,
> > > > > > MEDIA_BUS_FMT_YVYU8_2X8,
> > > > > > MEDIA_BUS_FMT_YVYU8_1X16),
> > > > > >
> > > > > > and same below.
> >
> > All of the YUV420/422 pixel formats on 2 or 3 planes can take all of the
> > supports packed 16-bit YUV bus formats, which is why it doesn't seem very
> > graceful to have an explicit list.
> >
> > > > > >
> > > > > Hi Laurent
> > > > >
> > > > > Thanks for the help and tips. Ive made all the other changes, which can be
> > > > > viewed here until i resubmit them https://github.com/sailfish-on-dontbeevil/
> > > > > kernel-megi/commits/pinephone-libcamera
> > > > >
> > > > > Im just not quite sure on this one. I think my implementation of merging the
> > > > > arrays keeps the previous mapping right? In sun6i_csi_capture_format_matches
> > > > > there is no mapping for the *NV formats, and the remaining ones ive set to 0?
> > > >
> > > > The current implementation allows writing multi-planar formats (e.g.
> > > > NV12) to memory when the input of the CSI is a YUV media bus format
> > > > (e.g. YUYV8_1X16). This patch doesn't change that, but it will prevent
> > > > NV12 from being enumerated when using media bus-based enumeration of
> > > > pixel formats, so userspace won't see NV12 as being available.
> > > >
> > > > It would be fine fixing that issue in a separate patch on top of this
> > > > one, but I though you could as well do both in one go.
> > >
> > > Adam, you mentioned that NV12 and NV16 "don't work". Could you elaborate
> > > and explain what you've tried exactly ?
> >
> > We definitely need to keep the ability to produce NV12, NV16 and friends from
> > YUV bus formats.
> >
> > > > > > Paul, could you confirm this ?
> > > > > >
> > > > > > I'm a bit surprised that the CSI can't shuffle the YUV components for
> > > > > > packed YUYV formats, but so be it if that's a hardware limitation.
> >
> > Yep that is correct, it's a hardware limitation.
> >
> > Cheers,
> >
> > Paul
> >
> > > > > > I'm also thinking that a subsequent patch could drop the raw check from
> > > > > > sun6i_csi_capture_link_validate():
> > > > > >
> > > > > > - /* With raw input mode, we need a 1:1 match between input and output. */
> > > > > > - if (bridge_format->input_format == SUN6I_CSI_INPUT_FMT_RAW ||
> > > > > > - capture_format->input_format_raw) {
> > > > > > - match = sun6i_csi_capture_format_match(pixelformat,
> > > > > > - fmt.format.code);
> > > > > > - if (!match)
> > > > > > - goto invalid;
> > > > > > - }
> > > > > > + /* Make sure the media bus code and pixel format are compatible. */
> > > > > > + match = sun6i_csi_capture_format_match(pixelformat, fmt.format.code);
> > > > > > + if (!match)
> > > > > > + goto invalid;
> > > > > >
> > > > > > > },
> > > > > > > {
> > > > > > >
> > > > > > > .pixelformat = V4L2_PIX_FMT_NV61,
> > > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422SP,
> > > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422SP,
> > > > > > > .input_yuv_seq_invert = true,
> > > > > > >
> > > > > > > + .mbus_codes = 0,
> > > > > > >
> > > > > > > },
> > > > > > > {
> > > > > > >
> > > > > > > .pixelformat = V4L2_PIX_FMT_YUV422P,
> > > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422P,
> > > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422P,
> > > > > > >
> > > > > > > + .mbus_codes = 0,
> > > > > > >
> > > > > > > },
> > > > > > > /* YUV420 */
> > > > > > > {
> > > > > > >
> > > > > > > .pixelformat = V4L2_PIX_FMT_NV12_16L16,
> > > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420MB,
> > > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420MB,
> > > > > > >
> > > > > > > + .mbus_codes = 0,
> > > > > > >
> > > > > > > },
> > > > > > > {
> > > > > > >
> > > > > > > .pixelformat = V4L2_PIX_FMT_NV12,
> > > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420SP,
> > > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420SP,
> > > > > > >
> > > > > > > + .mbus_codes = 0,
> > > > > > >
> > > > > > > },
> > > > > > > {
> > > > > > >
> > > > > > > .pixelformat = V4L2_PIX_FMT_NV21,
> > > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420SP,
> > > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420SP,
> > > > > > > .input_yuv_seq_invert = true,
> > > > > > >
> > > > > > > + .mbus_codes = 0,
> > > > > > >
> > > > > > > },
> > > > > > >
> > > > > > > {
> > > > > > >
> > > > > > > .pixelformat = V4L2_PIX_FMT_YUV420,
> > > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420P,
> > > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420P,
> > > > > > >
> > > > > > > + .mbus_codes = 0,
> > > > > > >
> > > > > > > },
> > > > > > > {
> > > > > > >
> > > > > > > .pixelformat = V4L2_PIX_FMT_YVU420,
> > > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420P,
> > > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420P,
> > > > > > > .input_yuv_seq_invert = true,
> > > > > > >
> > > > > > > + .mbus_codes = 0,
> > > > > > >
> > > > > > > },
> > > > > > > /* Compressed */
> > > > > > > {
> > > > > > >
> > > > > > > .pixelformat = V4L2_PIX_FMT_JPEG,
> > > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> > > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > > > >
> > > > > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_JPEG_1X8),
> > > > > > >
> > > > > > > },
> > > > > > >
> > > > > > > };
> > > > > > >
> > > > > > > @@ -210,118 +243,20 @@ struct sun6i_csi_capture_format
> > > > > > > *sun6i_csi_capture_format_find(u32 pixelformat)>
> > > > > > > return NULL;
> > > > > > >
> > > > > > > }
> > > > > > >
> > > > > > > -/* RAW formats need an exact match between pixel and mbus formats. */
> > > > > > > -static const
> > > > > > > -struct sun6i_csi_capture_format_match sun6i_csi_capture_format_matches[]
> > > > > > > = { - /* YUV420 */
> > > > > > > - {
> > > > > > > - .pixelformat = V4L2_PIX_FMT_YUYV,
> > > > > > > - .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8,
> > > > > > > - },
> > > > > > > - {
> > > > > > > - .pixelformat = V4L2_PIX_FMT_YUYV,
> > > > > > > - .mbus_code = MEDIA_BUS_FMT_YUYV8_1X16,
> > > > > > > - },
> > > > > > > - {
> > > > > > > - .pixelformat = V4L2_PIX_FMT_YVYU,
> > > > > > > - .mbus_code = MEDIA_BUS_FMT_YVYU8_2X8,
> > > > > > > - },
> > > > > > > - {
> > > > > > > - .pixelformat = V4L2_PIX_FMT_YVYU,
> > > > > > > - .mbus_code = MEDIA_BUS_FMT_YVYU8_1X16,
> > > > > > > - },
> > > > > > > - {
> > > > > > > - .pixelformat = V4L2_PIX_FMT_UYVY,
> > > > > > > - .mbus_code = MEDIA_BUS_FMT_UYVY8_2X8,
> > > > > > > - },
> > > > > > > - {
> > > > > > > - .pixelformat = V4L2_PIX_FMT_UYVY,
> > > > > > > - .mbus_code = MEDIA_BUS_FMT_UYVY8_1X16,
> > > > > > > - },
> > > > > > > - {
> > > > > > > - .pixelformat = V4L2_PIX_FMT_VYUY,
> > > > > > > - .mbus_code = MEDIA_BUS_FMT_VYUY8_2X8,
> > > > > > > - },
> > > > > > > - {
> > > > > > > - .pixelformat = V4L2_PIX_FMT_VYUY,
> > > > > > > - .mbus_code = MEDIA_BUS_FMT_VYUY8_1X16,
> > > > > > > - },
> > > > > > > - /* RGB */
> > > > > > > - {
> > > > > > > - .pixelformat = V4L2_PIX_FMT_RGB565,
> > > > > > > - .mbus_code = MEDIA_BUS_FMT_RGB565_2X8_LE,
> > > > > > > - },
> > > > > > > - {
> > > > > > > - .pixelformat = V4L2_PIX_FMT_RGB565X,
> > > > > > > - .mbus_code = MEDIA_BUS_FMT_RGB565_2X8_BE,
> > > > > > > - },
> > > > > > > - /* Bayer */
> > > > > > > - {
> > > > > > > - .pixelformat = V4L2_PIX_FMT_SBGGR8,
> > > > > > > - .mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8,
> > > > > > > - },
> > > > > > > - {
> > > > > > > - .pixelformat = V4L2_PIX_FMT_SGBRG8,
> > > > > > > - .mbus_code = MEDIA_BUS_FMT_SGBRG8_1X8,
> > > > > > > - },
> > > > > > > - {
> > > > > > > - .pixelformat = V4L2_PIX_FMT_SGRBG8,
> > > > > > > - .mbus_code = MEDIA_BUS_FMT_SGRBG8_1X8,
> > > > > > > - },
> > > > > > > - {
> > > > > > > - .pixelformat = V4L2_PIX_FMT_SRGGB8,
> > > > > > > - .mbus_code = MEDIA_BUS_FMT_SRGGB8_1X8,
> > > > > > > - },
> > > > > > > - {
> > > > > > > - .pixelformat = V4L2_PIX_FMT_SBGGR10,
> > > > > > > - .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10,
> > > > > > > - },
> > > > > > > - {
> > > > > > > - .pixelformat = V4L2_PIX_FMT_SGBRG10,
> > > > > > > - .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10,
> > > > > > > - },
> > > > > > > - {
> > > > > > > - .pixelformat = V4L2_PIX_FMT_SGRBG10,
> > > > > > > - .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10,
> > > > > > > - },
> > > > > > > - {
> > > > > > > - .pixelformat = V4L2_PIX_FMT_SRGGB10,
> > > > > > > - .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10,
> > > > > > > - },
> > > > > > > - {
> > > > > > > - .pixelformat = V4L2_PIX_FMT_SBGGR12,
> > > > > > > - .mbus_code = MEDIA_BUS_FMT_SBGGR12_1X12,
> > > > > > > - },
> > > > > > > - {
> > > > > > > - .pixelformat = V4L2_PIX_FMT_SGBRG12,
> > > > > > > - .mbus_code = MEDIA_BUS_FMT_SGBRG12_1X12,
> > > > > > > - },
> > > > > > > - {
> > > > > > > - .pixelformat = V4L2_PIX_FMT_SGRBG12,
> > > > > > > - .mbus_code = MEDIA_BUS_FMT_SGRBG12_1X12,
> > > > > > > - },
> > > > > > > - {
> > > > > > > - .pixelformat = V4L2_PIX_FMT_SRGGB12,
> > > > > > > - .mbus_code = MEDIA_BUS_FMT_SRGGB12_1X12,
> > > > > > > - },
> > > > > > > - /* Compressed */
> > > > > > > - {
> > > > > > > - .pixelformat = V4L2_PIX_FMT_JPEG,
> > > > > > > - .mbus_code = MEDIA_BUS_FMT_JPEG_1X8,
> > > > > > > - },
> > > > > > > -};
> > > > > > > -
> > > > > > >
> > > > > > > static bool sun6i_csi_capture_format_match(u32 pixelformat, u32
> > > > > > > mbus_code)
> > > > > > > {
> > > > > > >
> > > > > > > - unsigned int i;
> > > > > > > -
> > > > > > > - for (i = 0; i < ARRAY_SIZE(sun6i_csi_capture_format_matches); i++) {
> > > > > > > - const struct sun6i_csi_capture_format_match *match =
> > > > > > > - &sun6i_csi_capture_format_matches[i];
> > > > > > > -
> > > > > > > - if (match->pixelformat == pixelformat &&
> > > > > > > - match->mbus_code == mbus_code)
> > > > > > > - return true;
> > > > > > > + unsigned int i, j;
> > > > > > > +
> > > > > > > + for (i = 0; i < ARRAY_SIZE(sun6i_csi_capture_formats); i++) {
> > > > > > > + const struct sun6i_csi_capture_format *format =
> > > > > > > + &sun6i_csi_capture_formats[i];
> > > > > > > +
> > > > > > > + if (format->pixelformat == pixelformat) {
> > > > > > > + for (j = 0; format->mbus_codes[j]; j++) {
> > > > > > > + if (mbus_code == format->mbus_codes[j])
> > > > > > > + return true;
> > > > > > > + }
> > > > > > > + }
> > > > > > >
> > > > > > > }
> > > > > > >
> > > > > > > return false;
> > > > > > >
> > > > > > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > > > > > > b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h index
> > > > > > > 3ee5ccefbd10..0484942834e3 100644
> > > > > > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > > > > > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > > > > > > @@ -27,11 +27,7 @@ struct sun6i_csi_capture_format {
> > > > > > >
> > > > > > > bool input_yuv_seq_invert;
> > > > > > > bool input_format_raw;
> > > > > > > u32 hsize_len_factor;
> > > > > > >
> > > > > > > -};
> > > > > > > -
> > > > > > > -struct sun6i_csi_capture_format_match {
> > > > > > > - u32 pixelformat;
> > > > > > > - u32 mbus_code;
> > > > > > > + const u32 *mbus_codes;
> > > > > > >
> > > > > > > };
> > > > > > >
> > > > > > > #undef current
> > >
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart
> >
> > --
> > Paul Kocialkowski, Bootlin
> > Embedded Linux and kernel engineering
> > https://bootlin.com
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] media: sun6i-csi: implement vidioc_enum_framesizes
2023-02-22 10:43 ` Paul Kocialkowski
@ 2023-03-25 23:12 ` Laurent Pinchart
0 siblings, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2023-03-25 23:12 UTC (permalink / raw)
To: Paul Kocialkowski; +Cc: adam, linux-media, yong.deng, mchehab, linux-sunxi
Hi Paul,
On Wed, Feb 22, 2023 at 11:43:06AM +0100, Paul Kocialkowski wrote:
> On Sat 07 Jan 23, 01:35, Laurent Pinchart wrote:
> > On Fri, Jan 06, 2023 at 07:40:38PM +0000, adam@piggz.co.uk wrote:
> > > From: Adam Pigg <adam@piggz.co.uk>
> > >
> > > Create sun6i_csi_capture_enum_framesizes which defines the min
> > > and max frame sizes
> >
> > With the commit message updated (see review of 1/3),
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> I'm always a bit confused regarding how such an ioctl's behavior should depend
> on the attached subdev. Is it well-defined behavior that this here is only
> about the receiver part and has nothing to do with what the connected sensor?
That's right. For MC-based drivers, enumerating pixel formats and frame
sizes on a video node should return the formats and sizes intrinsically
supported by the video node (in most cases, that maps to the DMA engine
at the hardware level) without considering connected subdevs.
> > > Signed-off-by: Adam Pigg <adam@piggz.co.uk>
> > > ---
> > > .../sunxi/sun6i-csi/sun6i_csi_capture.c | 24 +++++++++++++++++++
> > > 1 file changed, 24 insertions(+)
> > >
> > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> > > index 54beba4d2b2f..2be761e6b650 100644
> > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> > > @@ -739,6 +739,29 @@ static int sun6i_csi_capture_try_fmt(struct file *file, void *private,
> > > return 0;
> > > }
> > >
> > > +static int sun6i_csi_capture_enum_framesizes(struct file *file, void *fh,
> > > + struct v4l2_frmsizeenum *fsize)
>
> A cosmetic/consistency suggestion would be to name this variable "frmsize" to
> reuse part of the name of the structure, which is what I've done in other places
> of the driver.
>
> > > +{
> > > + const struct sun6i_csi_capture_format *format;
> > > +
> > > + if (fsize->index > 0)
> > > + return -EINVAL;
> > > +
> > > + format = sun6i_csi_capture_format_find(fsize->pixel_format);
> > > + if (!format)
> > > + return -EINVAL;
> > > +
> > > + fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS;
> > > + fsize->stepwise.min_width = SUN6I_CSI_CAPTURE_WIDTH_MIN;
> > > + fsize->stepwise.max_width = SUN6I_CSI_CAPTURE_WIDTH_MAX;
> > > + fsize->stepwise.min_height = SUN6I_CSI_CAPTURE_HEIGHT_MIN;
> > > + fsize->stepwise.max_height = SUN6I_CSI_CAPTURE_HEIGHT_MAX;
> > > + fsize->stepwise.step_width = 1;
> > > + fsize->stepwise.step_height = 1;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static int sun6i_csi_capture_enum_input(struct file *file, void *private,
> > > struct v4l2_input *input)
> > > {
> > > @@ -775,6 +798,7 @@ static const struct v4l2_ioctl_ops sun6i_csi_capture_ioctl_ops = {
> > > .vidioc_g_fmt_vid_cap = sun6i_csi_capture_g_fmt,
> > > .vidioc_s_fmt_vid_cap = sun6i_csi_capture_s_fmt,
> > > .vidioc_try_fmt_vid_cap = sun6i_csi_capture_try_fmt,
> > > + .vidioc_enum_framesizes = sun6i_csi_capture_enum_framesizes,
> > >
> > > .vidioc_enum_input = sun6i_csi_capture_enum_input,
> > > .vidioc_g_input = sun6i_csi_capture_g_input,
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] media: sun6i-csi: merge sun6i_csi_formats and sun6i_csi_formats_match
2023-02-22 10:39 ` Paul Kocialkowski
2023-03-21 17:50 ` Adam Pigg
@ 2023-03-25 23:17 ` Laurent Pinchart
1 sibling, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2023-03-25 23:17 UTC (permalink / raw)
To: Paul Kocialkowski; +Cc: Adam Pigg, linux-media, yong.deng, mchehab, linux-sunxi
Hi Paul,
On Wed, Feb 22, 2023 at 11:39:42AM +0100, Paul Kocialkowski wrote:
> On Fri 17 Feb 23, 20:20, Laurent Pinchart wrote:
> > Could you share your opinion on the question below ?
>
> Yeah sorry for the long delay here. It's taken me a while to dive back into
> this topic.
>
> My first impression is that I would rather be in favor of keeping a dynamic
> approach like what's already in sun6i_csi_capture_link_validate and extract
> the mbus/pixfmt matching logic from there.
>
> I would be happy to craft a patch in that direction, but maybe you'd like
> to have a try at it (since it's your series). Just let me know, I think I can
> do it pretty quickly now that I have everything back in mind. I could also
> add some comment about the general hardware mechanism/limitations.
>
> The hardware is a bit odd in a few ways and I found that the explicit
> combinatory approach wasn't a very good fit (but obviously that's an open topic
> for discussions).
>
> The general idea is that the SUN6I_CSI_INPUT_FMT_YUV420/422 can only be used
> to produce outputs on 2 or 3 planes, but not packed YUV. There's also no
> explicit hardware reordering of the chroma components, so we need to lie about
> the input order (input_yuv_seq_invert) to achieve inverted chroma components
> on the output format.
>
> In order to produce packed data, we can only rely on SUN6I_CSI_INPUT_FMT_RAW
> which provides no reordering mechanism. This is why it made good sense to me
> to only have an explicit matching table for this case and rely on more general
> logic that reflects the hardware capabilities otherwise.
The explicit logic is more CPU-intensive so I have a small preference
for adding mbus_codes to the sun6i_csi_capture_format structure. It's
true that there will be some duplication of data, but I don't think it
will consume lots of memory. As mentioned in the review of the series
you've recently sent, I'm fine with both approaches, so you can decide.
> > On Sun, Jan 08, 2023 at 10:26:09PM +0200, Laurent Pinchart wrote:
> > > On Sun, Jan 08, 2023 at 06:23:56PM +0000, Adam Pigg wrote:
> > > > On Friday, 6 January 2023 23:31:48 GMT Laurent Pinchart wrote:
> > > > > On Fri, Jan 06, 2023 at 07:40:36PM +0000, adam@piggz.co.uk wrote:
>
> [...]
>
> > > > > > +#define SUN6I_BUS_FMTS(fmt...) (const u32[]) {fmt, 0}
>
> Cosmetic suggestion to stay consistent with the rest:
> #define SUN6I_CSI_CAPTURE_MBUS_CODES(mbus_codes...) \
> (const u32[]) { mbus_codes, 0 }
>
> Also it would look better in sun6i_csi_capture.h.
>
> But really I would be happier with a dynamic approach.
>
> [...]
>
> > > > > > /* YUV422 */
> > > > > > {
> > > > > >
> > > > > > @@ -123,6 +139,8 @@ static const struct sun6i_csi_capture_format
> > > > > > sun6i_csi_capture_formats[] = {>
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > > > .input_format_raw = true,
> > > > > > .hsize_len_factor = 2,
> > > > > >
> > > > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_YUYV8_2X8,
> > > > > > + MEDIA_BUS_FMT_YUYV8_1X16),
> > > > > >
> > > > > > },
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_YVYU,
> > > > > >
> > > > > > @@ -130,6 +148,8 @@ static const struct sun6i_csi_capture_format
> > > > > > sun6i_csi_capture_formats[] = {>
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > > > .input_format_raw = true,
> > > > > > .hsize_len_factor = 2,
> > > > > >
> > > > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_YVYU8_2X8,
> > > > > > + MEDIA_BUS_FMT_YVYU8_1X16),
> > > > > >
> > > > > > },
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_UYVY,
> > > > > >
> > > > > > @@ -137,6 +157,8 @@ static const struct sun6i_csi_capture_format
> > > > > > sun6i_csi_capture_formats[] = {
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > > > .input_format_raw = true,
> > > > > > .hsize_len_factor = 2,
> > > > > >
> > > > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_UYVY8_2X8,
> > > > > > + MEDIA_BUS_FMT_UYVY8_1X16),
> > > > > >
> > > > > > },
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_VYUY,
> > > > > >
> > > > > > @@ -144,57 +166,68 @@ static const struct sun6i_csi_capture_format
> > > > > > sun6i_csi_capture_formats[] = {>
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > > > .input_format_raw = true,
> > > > > > .hsize_len_factor = 2,
> > > > > >
> > > > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_VYUY8_2X8,
> > > > > > + MEDIA_BUS_FMT_VYUY8_1X16),
> > > > > >
> > > > > > },
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_NV16,
> > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422SP,
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422SP,
> > > > > >
> > > > > > + .mbus_codes = 0,
> > > > >
> > > > > I don't think this is correct. To produce semi-planar or multi-planar
> > > > > YUV formats, I believe the CSI needs YUV input. This should thus be
> > > > > (unless I'm mistaken)
>
> You are correct.
>
> > > > >
> > > > > .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_UYVY8_2X8,
> > > > > MEDIA_BUS_FMT_UYVY8_1X16,
> > > > > MEDIA_BUS_FMT_VYUY8_2X8,
> > > > > MEDIA_BUS_FMT_VYUY8_1X16,
> > > > > MEDIA_BUS_FMT_YUYV8_2X8,
> > > > > MEDIA_BUS_FMT_YUYV8_1X16,
> > > > > MEDIA_BUS_FMT_YVYU8_2X8,
> > > > > MEDIA_BUS_FMT_YVYU8_1X16),
> > > > >
> > > > > and same below.
>
> All of the YUV420/422 pixel formats on 2 or 3 planes can take all of the
> supports packed 16-bit YUV bus formats, which is why it doesn't seem very
> graceful to have an explicit list.
>
> > > > >
> > > > Hi Laurent
> > > >
> > > > Thanks for the help and tips. Ive made all the other changes, which can be
> > > > viewed here until i resubmit them https://github.com/sailfish-on-dontbeevil/
> > > > kernel-megi/commits/pinephone-libcamera
> > > >
> > > > Im just not quite sure on this one. I think my implementation of merging the
> > > > arrays keeps the previous mapping right? In sun6i_csi_capture_format_matches
> > > > there is no mapping for the *NV formats, and the remaining ones ive set to 0?
> > >
> > > The current implementation allows writing multi-planar formats (e.g.
> > > NV12) to memory when the input of the CSI is a YUV media bus format
> > > (e.g. YUYV8_1X16). This patch doesn't change that, but it will prevent
> > > NV12 from being enumerated when using media bus-based enumeration of
> > > pixel formats, so userspace won't see NV12 as being available.
> > >
> > > It would be fine fixing that issue in a separate patch on top of this
> > > one, but I though you could as well do both in one go.
> >
> > Adam, you mentioned that NV12 and NV16 "don't work". Could you elaborate
> > and explain what you've tried exactly ?
>
> We definitely need to keep the ability to produce NV12, NV16 and friends from
> YUV bus formats.
>
> > > > > Paul, could you confirm this ?
> > > > >
> > > > > I'm a bit surprised that the CSI can't shuffle the YUV components for
> > > > > packed YUYV formats, but so be it if that's a hardware limitation.
>
> Yep that is correct, it's a hardware limitation.
>
> Cheers,
>
> Paul
>
> > > > > I'm also thinking that a subsequent patch could drop the raw check from
> > > > > sun6i_csi_capture_link_validate():
> > > > >
> > > > > - /* With raw input mode, we need a 1:1 match between input and output. */
> > > > > - if (bridge_format->input_format == SUN6I_CSI_INPUT_FMT_RAW ||
> > > > > - capture_format->input_format_raw) {
> > > > > - match = sun6i_csi_capture_format_match(pixelformat,
> > > > > - fmt.format.code);
> > > > > - if (!match)
> > > > > - goto invalid;
> > > > > - }
> > > > > + /* Make sure the media bus code and pixel format are compatible. */
> > > > > + match = sun6i_csi_capture_format_match(pixelformat, fmt.format.code);
> > > > > + if (!match)
> > > > > + goto invalid;
> > > > >
> > > > > > },
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_NV61,
> > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422SP,
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422SP,
> > > > > > .input_yuv_seq_invert = true,
> > > > > >
> > > > > > + .mbus_codes = 0,
> > > > > >
> > > > > > },
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_YUV422P,
> > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV422P,
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV422P,
> > > > > >
> > > > > > + .mbus_codes = 0,
> > > > > >
> > > > > > },
> > > > > > /* YUV420 */
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_NV12_16L16,
> > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420MB,
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420MB,
> > > > > >
> > > > > > + .mbus_codes = 0,
> > > > > >
> > > > > > },
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_NV12,
> > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420SP,
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420SP,
> > > > > >
> > > > > > + .mbus_codes = 0,
> > > > > >
> > > > > > },
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_NV21,
> > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420SP,
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420SP,
> > > > > > .input_yuv_seq_invert = true,
> > > > > >
> > > > > > + .mbus_codes = 0,
> > > > > >
> > > > > > },
> > > > > >
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_YUV420,
> > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420P,
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420P,
> > > > > >
> > > > > > + .mbus_codes = 0,
> > > > > >
> > > > > > },
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_YVU420,
> > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_YUV420P,
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_YUV420P,
> > > > > > .input_yuv_seq_invert = true,
> > > > > >
> > > > > > + .mbus_codes = 0,
> > > > > >
> > > > > > },
> > > > > > /* Compressed */
> > > > > > {
> > > > > >
> > > > > > .pixelformat = V4L2_PIX_FMT_JPEG,
> > > > > > .output_format_frame = SUN6I_CSI_OUTPUT_FMT_FRAME_RAW_8,
> > > > > > .output_format_field = SUN6I_CSI_OUTPUT_FMT_FIELD_RAW_8,
> > > > > >
> > > > > > + .mbus_codes = SUN6I_BUS_FMTS(MEDIA_BUS_FMT_JPEG_1X8),
> > > > > >
> > > > > > },
> > > > > >
> > > > > > };
> > > > > >
> > > > > > @@ -210,118 +243,20 @@ struct sun6i_csi_capture_format
> > > > > > *sun6i_csi_capture_format_find(u32 pixelformat)>
> > > > > > return NULL;
> > > > > >
> > > > > > }
> > > > > >
> > > > > > -/* RAW formats need an exact match between pixel and mbus formats. */
> > > > > > -static const
> > > > > > -struct sun6i_csi_capture_format_match sun6i_csi_capture_format_matches[]
> > > > > > = { - /* YUV420 */
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_YUYV,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_YUYV,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_YUYV8_1X16,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_YVYU,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_YVYU8_2X8,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_YVYU,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_YVYU8_1X16,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_UYVY,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_UYVY8_2X8,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_UYVY,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_UYVY8_1X16,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_VYUY,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_VYUY8_2X8,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_VYUY,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_VYUY8_1X16,
> > > > > > - },
> > > > > > - /* RGB */
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_RGB565,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_RGB565_2X8_LE,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_RGB565X,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_RGB565_2X8_BE,
> > > > > > - },
> > > > > > - /* Bayer */
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SBGGR8,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SGBRG8,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SGBRG8_1X8,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SGRBG8,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SGRBG8_1X8,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SRGGB8,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SRGGB8_1X8,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SBGGR10,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SGBRG10,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SGRBG10,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SRGGB10,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SBGGR12,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SBGGR12_1X12,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SGBRG12,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SGBRG12_1X12,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SGRBG12,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SGRBG12_1X12,
> > > > > > - },
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_SRGGB12,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_SRGGB12_1X12,
> > > > > > - },
> > > > > > - /* Compressed */
> > > > > > - {
> > > > > > - .pixelformat = V4L2_PIX_FMT_JPEG,
> > > > > > - .mbus_code = MEDIA_BUS_FMT_JPEG_1X8,
> > > > > > - },
> > > > > > -};
> > > > > > -
> > > > > >
> > > > > > static bool sun6i_csi_capture_format_match(u32 pixelformat, u32
> > > > > > mbus_code)
> > > > > > {
> > > > > >
> > > > > > - unsigned int i;
> > > > > > -
> > > > > > - for (i = 0; i < ARRAY_SIZE(sun6i_csi_capture_format_matches); i++) {
> > > > > > - const struct sun6i_csi_capture_format_match *match =
> > > > > > - &sun6i_csi_capture_format_matches[i];
> > > > > > -
> > > > > > - if (match->pixelformat == pixelformat &&
> > > > > > - match->mbus_code == mbus_code)
> > > > > > - return true;
> > > > > > + unsigned int i, j;
> > > > > > +
> > > > > > + for (i = 0; i < ARRAY_SIZE(sun6i_csi_capture_formats); i++) {
> > > > > > + const struct sun6i_csi_capture_format *format =
> > > > > > + &sun6i_csi_capture_formats[i];
> > > > > > +
> > > > > > + if (format->pixelformat == pixelformat) {
> > > > > > + for (j = 0; format->mbus_codes[j]; j++) {
> > > > > > + if (mbus_code == format->mbus_codes[j])
> > > > > > + return true;
> > > > > > + }
> > > > > > + }
> > > > > >
> > > > > > }
> > > > > >
> > > > > > return false;
> > > > > >
> > > > > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > > > > > b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h index
> > > > > > 3ee5ccefbd10..0484942834e3 100644
> > > > > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > > > > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > > > > > @@ -27,11 +27,7 @@ struct sun6i_csi_capture_format {
> > > > > >
> > > > > > bool input_yuv_seq_invert;
> > > > > > bool input_format_raw;
> > > > > > u32 hsize_len_factor;
> > > > > >
> > > > > > -};
> > > > > > -
> > > > > > -struct sun6i_csi_capture_format_match {
> > > > > > - u32 pixelformat;
> > > > > > - u32 mbus_code;
> > > > > > + const u32 *mbus_codes;
> > > > > >
> > > > > > };
> > > > > >
> > > > > > #undef current
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2023-03-25 23:17 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-06 19:40 [PATCH 0/3] suns6-csi changes to support libcamera adam
2023-01-06 19:40 ` [PATCH 1/3] media: sun6i-csi: merge sun6i_csi_formats and sun6i_csi_formats_match adam
2023-01-06 23:31 ` Laurent Pinchart
2023-01-08 18:23 ` Adam Pigg
2023-01-08 20:26 ` Laurent Pinchart
2023-02-17 18:20 ` Laurent Pinchart
2023-02-22 10:39 ` Paul Kocialkowski
2023-03-21 17:50 ` Adam Pigg
2023-03-23 12:55 ` Paul Kocialkowski
2023-03-25 23:17 ` Laurent Pinchart
2023-01-06 19:40 ` [PATCH 2/3] media: sun6i-csi: implement V4L2_CAP_IO_MC adam
2023-01-06 23:34 ` Laurent Pinchart
2023-01-06 19:40 ` [PATCH 3/3] media: sun6i-csi: implement vidioc_enum_framesizes adam
2023-01-06 23:35 ` Laurent Pinchart
2023-02-22 10:43 ` Paul Kocialkowski
2023-03-25 23:12 ` Laurent Pinchart
2023-01-06 23:10 ` [PATCH 0/3] suns6-csi changes to support libcamera piggz1
2023-01-07 8:43 ` Adam Pigg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).