Re: [linux-sunxi] [PATCH v3 1/3] media: V3s: Add support for Allwinner CSI.

[Chen-Yu Tsai]

On Mon, Nov 13, 2017 at 3:30 PM, Yong Deng <yong.deng@xxxxxxxxxxxx> wrote:
> Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> and CSI1 is used for parallel interface. This is not documented in
> datasheet but by testing and guess.
>
> This patch implement a v4l2 framework driver for it.
>
> Currently, the driver only support the parallel interface. MIPI-CSI2,
> ISP's support are not included in this patch.
>
> Signed-off-by: Yong Deng <yong.deng@xxxxxxxxxxxx>
> ---
>  drivers/media/platform/Kconfig                     |   1 +
>  drivers/media/platform/Makefile                    |   2 +
>  drivers/media/platform/sunxi/sun6i-csi/Kconfig     |   9 +
>  drivers/media/platform/sunxi/sun6i-csi/Makefile    |   3 +
>  drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c | 918 +++++++++++++++++++++
>  drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h | 146 ++++
>  .../media/platform/sunxi/sun6i-csi/sun6i_csi_reg.h | 203 +++++
>  .../media/platform/sunxi/sun6i-csi/sun6i_video.c   | 722 ++++++++++++++++
>  .../media/platform/sunxi/sun6i-csi/sun6i_video.h   |  61 ++
>  9 files changed, 2065 insertions(+)
>  create mode 100644 drivers/media/platform/sunxi/sun6i-csi/Kconfig
>  create mode 100644 drivers/media/platform/sunxi/sun6i-csi/Makefile
>  create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
>  create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h
>  create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_reg.h
>  create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c
>  create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_video.h
>

[...]

> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c
> new file mode 100644
> index 0000000..0cebcbd
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c

[...]

> +/* -----------------------------------------------------------------------------
> + * Media Operations
> + */
> +static int sun6i_video_formats_init(struct sun6i_video *video)
> +{
> +       struct v4l2_subdev_mbus_code_enum mbus_code = { 0 };
> +       struct sun6i_csi *csi = video->csi;
> +       struct v4l2_format format;
> +       struct v4l2_subdev *subdev;
> +       u32 pad;
> +       const u32 *pixformats;
> +       int pixformat_count = 0;
> +       u32 subdev_codes[32]; /* subdev format codes, 32 should be enough */
> +       int codes_count = 0;
> +       int num_fmts = 0;
> +       int i, j;
> +
> +       subdev = sun6i_video_remote_subdev(video, &pad);
> +       if (subdev == NULL)
> +               return -ENXIO;
> +
> +       /* Get supported pixformats of CSI */
> +       pixformat_count = sun6i_csi_get_supported_pixformats(csi, &pixformats);
> +       if (pixformat_count <= 0)
> +               return -ENXIO;
> +
> +       /* Get subdev formats codes */
> +       mbus_code.pad = pad;
> +       mbus_code.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +       while (!v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL,
> +                                &mbus_code)) {
> +               if (codes_count >= ARRAY_SIZE(subdev_codes)) {
> +                       dev_warn(video->csi->dev,
> +                                "subdev_codes array is full!\n");
> +                       break;
> +               }
> +               subdev_codes[codes_count] = mbus_code.code;
> +               codes_count++;
> +               mbus_code.index++;
> +       }
> +
> +       if (!codes_count)
> +               return -ENXIO;
> +
> +       /* Get supported formats count */
> +       for (i = 0; i < codes_count; i++) {
> +               for (j = 0; j < pixformat_count; j++) {
> +                       if (!sun6i_csi_is_format_support(csi, pixformats[j],
> +                                       mbus_code.code)) {

Bug here. You are testing against mbus_code.code, which would be whatever
was leftover from the previous section. Instead you should be testing
against subdev_codes[i], the list you just built.

Without it, it won't get past this part of the code if the last enumerated
media bus format isn't supported.

> +                               continue;
> +                       }
> +                       num_fmts++;
> +               }
> +       }
> +
> +       if (!num_fmts)
> +               return -ENXIO;
> +
> +       video->num_formats = num_fmts;
> +       video->formats = devm_kcalloc(video->csi->dev, num_fmts,
> +                       sizeof(struct sun6i_csi_format), GFP_KERNEL);
> +       if (!video->formats)
> +               return -ENOMEM;
> +
> +       /* Get supported formats */
> +       num_fmts = 0;
> +       for (i = 0; i < codes_count; i++) {
> +               for (j = 0; j < pixformat_count; j++) {
> +                       if (!sun6i_csi_is_format_support(csi, pixformats[j],
> +                                       mbus_code.code)) {

Same here.

This gets me past the enumeration part of things...

> +                               continue;
> +                       }
> +
> +                       video->formats[num_fmts].fourcc = pixformats[j];
> +                       video->formats[num_fmts].mbus_code =
> +                                       mbus_code.code;
> +                       video->formats[num_fmts].bpp =
> +                                       v4l2_pixformat_get_bpp(pixformats[j]);
> +                       num_fmts++;
> +               }
> +       }
> +
> +       /* setup default format */
> +       format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +       format.fmt.pix.width = 1280;
> +       format.fmt.pix.height = 720;
> +       format.fmt.pix.pixelformat = video->formats[0].fourcc;
> +       sun6i_video_set_fmt(video, &format);

But my system crashes here within the OV5640 driver.
So no tests about the actual functionality. This was on the Bananapi M3,
which has an A83T SoC.


In general I think you should make your driver much more noisy than it
currently is. I spent the whole afternoon adding error messages and
debug traces to narrow down the issue.

ChenYu