Hi Paul,
Thank you for the patch.
On Fri, Mar 24, 2023 at 04:12:24PM +0100, Paul Kocialkowski wrote:
> Introduce a new sun6i_csi_capture_format_check helper to indicate
> whether a set of pixel format and mbus code are compatible.
> Most of the logic is taken from sun6i_csi_capture_link_validate,
> with extra checks added along the way.
>
> Note that v4l2_format_info is now used for all pixel formats
> since they should all be listed in the helper at this point.
>
> The motivation behind this change is to pave the way for supporting
> the mc-style enum_fmt.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> ---
> .../sunxi/sun6i-csi/sun6i_csi_capture.c | 95 ++++++++++---------
> 1 file changed, 49 insertions(+), 46 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 cf6aadbc130b..6ce7f1d3ed57 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> @@ -327,6 +327,52 @@ static bool sun6i_csi_capture_format_match(u32 pixelformat, u32 mbus_code)
> return false;
> }
>
> +static bool sun6i_csi_capture_format_check(u32 pixelformat, u32 mbus_code)
> +{
> + const struct sun6i_csi_capture_format *capture_format;
> + const struct sun6i_csi_bridge_format *bridge_format;
I think I would have named those output_format and input_format
respectively, as "capture" and "bridge" take a bit more mental
processing when reading the code. That may be because I'm not familiar
with the driver though, so feel free to ignore this.
> + const struct v4l2_format_info *format_info;
> +
> + format_info = v4l2_format_info(pixelformat);
> + if (WARN_ON(!format_info))
> + return false;
> +
> + capture_format = sun6i_csi_capture_format_find(pixelformat);
> + if (WARN_ON(!capture_format))
> + return false;
> +
> + bridge_format = sun6i_csi_bridge_format_find(mbus_code);
> + if (WARN_ON(!bridge_format))
> + return false;
> +
> + /* Raw input is required for non-YUV formats. */
> + if (bridge_format->input_format != SUN6I_CSI_INPUT_FMT_RAW &&
> + (format_info->pixel_enc == V4L2_PIXEL_ENC_BAYER ||
> + format_info->pixel_enc == V4L2_PIXEL_ENC_RGB ||
> + format_info->pixel_enc == V4L2_PIXEL_ENC_COMPRESSED))
Unless I'm mistaken, the compressed format check is new. You could split
it to a separate patch, or at least mention it in the commit message.
> + return false;
> +
> + if (format_info->pixel_enc == V4L2_PIXEL_ENC_YUV) {
> + /* YUV input is required for YUV pixels. */
> + if (bridge_format->input_format != SUN6I_CSI_INPUT_FMT_YUV420 &&
> + bridge_format->input_format != SUN6I_CSI_INPUT_FMT_YUV422)
> + return false;
> +
> + /* YUV420 input can't produce (upsampled) YUV422 output. */
> + if (bridge_format->input_format == SUN6I_CSI_INPUT_FMT_YUV420 &&
> + format_info->vdiv == 1)
> + return false;
> + }
> +
> + /* Raw input requires a 1:1 match between input and output. */
> + if ((bridge_format->input_format == SUN6I_CSI_INPUT_FMT_RAW ||
> + capture_format->input_format_raw) &&
> + !sun6i_csi_capture_format_match(pixelformat, mbus_code))
> + return false;
Wrong indentation.
With this fixed,
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> +
> + return true;
> +}
> +
> /* Capture */
>
> static void
> @@ -890,28 +936,16 @@ static int sun6i_csi_capture_link_validate(struct media_link *link)
> media_entity_to_video_device(link->sink->entity);
> struct sun6i_csi_device *csi_dev = video_get_drvdata(video_dev);
> struct v4l2_device *v4l2_dev = csi_dev->v4l2_dev;
> - const struct sun6i_csi_capture_format *capture_format;
> - const struct sun6i_csi_bridge_format *bridge_format;
> unsigned int capture_width, capture_height;
> unsigned int bridge_width, bridge_height;
> - const struct v4l2_format_info *format_info;
> u32 pixelformat, capture_field;
> u32 mbus_code, bridge_field;
> - bool match;
>
> sun6i_csi_capture_dimensions(csi_dev, &capture_width, &capture_height);
> -
> sun6i_csi_capture_format(csi_dev, &pixelformat, &capture_field);
> - capture_format = sun6i_csi_capture_format_find(pixelformat);
> - if (WARN_ON(!capture_format))
> - return -EINVAL;
>
> sun6i_csi_bridge_dimensions(csi_dev, &bridge_width, &bridge_height);
> -
> sun6i_csi_bridge_format(csi_dev, &mbus_code, &bridge_field);
> - bridge_format = sun6i_csi_bridge_format_find(mbus_code);
> - if (WARN_ON(!bridge_format))
> - return -EINVAL;
>
> /* No cropping/scaling is supported. */
> if (capture_width != bridge_width || capture_height != bridge_height) {
> @@ -922,43 +956,12 @@ static int sun6i_csi_capture_link_validate(struct media_link *link)
> return -EINVAL;
> }
>
> - format_info = v4l2_format_info(pixelformat);
> - /* Some formats are not listed. */
> - if (!format_info)
> - return 0;
> -
> - if (format_info->pixel_enc == V4L2_PIXEL_ENC_BAYER &&
> - bridge_format->input_format != SUN6I_CSI_INPUT_FMT_RAW)
> - goto invalid;
> -
> - if (format_info->pixel_enc == V4L2_PIXEL_ENC_RGB &&
> - bridge_format->input_format != SUN6I_CSI_INPUT_FMT_RAW)
> - goto invalid;
> -
> - if (format_info->pixel_enc == V4L2_PIXEL_ENC_YUV) {
> - if (bridge_format->input_format != SUN6I_CSI_INPUT_FMT_YUV420 &&
> - bridge_format->input_format != SUN6I_CSI_INPUT_FMT_YUV422)
> - goto invalid;
> -
> - /* YUV420 input can't produce YUV422 output. */
> - if (bridge_format->input_format == SUN6I_CSI_INPUT_FMT_YUV420 &&
> - format_info->vdiv == 1)
> - goto invalid;
> - }
> -
> - /* 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, mbus_code);
> - if (!match)
> - goto invalid;
> + if (!sun6i_csi_capture_format_check(pixelformat, mbus_code)) {
> + v4l2_err(v4l2_dev, "invalid input/output format combination\n");
> + return -EINVAL;
> }
>
> return 0;
> -
> -invalid:
> - v4l2_err(v4l2_dev, "invalid input/output format combination\n");
> - return -EINVAL;
> }
>
> static const struct media_entity_operations sun6i_csi_capture_media_ops = {
--
Regards,
Laurent Pinchart
