Re: [PATCH v8 05/14] media: rkisp1: add Rockchip ISP1 subdev driver
From: Dafna Hirschfeld
Date: Fri Jul 17 2020 - 03:46:37 EST
Hi,
On 11.07.20 13:04, Dafna Hirschfeld wrote:
Hi Laurent,
On 16.08.19 02:13, Laurent Pinchart wrote:
Hello Helen,
Thank you for the patch.
On Tue, Jul 30, 2019 at 03:42:47PM -0300, Helen Koike wrote:
From: Jacob Chen <jacob2.chen@xxxxxxxxxxxxxx>
Add the subdev driver for rockchip isp1.
Signed-off-by: Jacob Chen <jacob2.chen@xxxxxxxxxxxxxx>
Signed-off-by: Shunqian Zheng <zhengsq@xxxxxxxxxxxxxx>
Signed-off-by: Yichong Zhong <zyc@xxxxxxxxxxxxxx>
Signed-off-by: Jacob Chen <cc@xxxxxxxxxxxxxx>
Signed-off-by: Eddie Cai <eddie.cai.linux@xxxxxxxxx>
Signed-off-by: Jeffy Chen <jeffy.chen@xxxxxxxxxxxxxx>
Signed-off-by: Allon Huang <allon.huang@xxxxxxxxxxxxxx>
Signed-off-by: Tomasz Figa <tfiga@xxxxxxxxxxxx>
[fixed unknown entity type / switched to PIXEL_RATE]
Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
[update for upstream]
Signed-off-by: Helen Koike <helen.koike@xxxxxxxxxxxxx>
---
Changes in v8: None
Changes in v7:
- fixed warning because of unknown entity type
- fixed v4l2-compliance errors regarding rkisp1 formats, try formats
and default values
- fix typo riksp1/rkisp1
- redesign: remove mipi/csi subdevice, sensors connect directly to the
isp subdevice in the media topology now. As a consequence, remove the
hack in mipidphy_g_mbus_config() where information from the sensor was
being propagated through the topology.
- From the old dphy:
ÂÂÂÂÂÂÂÂ * cache get_remote_sensor() in s_stream
ÂÂÂÂÂÂÂÂ * use V4L2_CID_PIXEL_RATE instead of V4L2_CID_LINK_FREQ
- Replace stream state with a boolean
- code styling and checkpatch fixes
- fix stop_stream (return after calling stop, do not reenable the stream)
- fix rkisp1_isp_sd_get_selection when V4L2_SUBDEV_FORMAT_TRY is set
- fix get format in output (isp_sd->out_fmt.mbus_code was being ignored)
- s/intput/input
- remove #define sd_to_isp_sd(_sd), add a static inline as it will be
reused by the capture
 drivers/media/platform/rockchip/isp1/rkisp1.c | 1286 +++++++++++++++++
 drivers/media/platform/rockchip/isp1/rkisp1.h | 111 ++
 2 files changed, 1397 insertions(+)
 create mode 100644 drivers/media/platform/rockchip/isp1/rkisp1.c
 create mode 100644 drivers/media/platform/rockchip/isp1/rkisp1.h
diff --git a/drivers/media/platform/rockchip/isp1/rkisp1.c b/drivers/media/platform/rockchip/isp1/rkisp1.c
new file mode 100644
index 000000000000..6d0c0ffb5e03
--- /dev/null
+++ b/drivers/media/platform/rockchip/isp1/rkisp1.c
@@ -0,0 +1,1286 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Rockchip isp1 driver
Shouldn't each file describe what it contains ? Maybe
 * Rockchip ISP1 Driver - ISP Core
for this one ? Same for other .c or .h files.
+ *
+ * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
+ */
+
+#include <linux/iopoll.h>
+#include <linux/phy/phy.h>
+#include <linux/phy/phy-mipi-dphy.h>
+#include <linux/pm_runtime.h>
+#include <linux/videodev2.h>
+#include <linux/vmalloc.h>
+#include <media/v4l2-event.h>
+
+#include "common.h"
+#include "regs.h"
common.h and regs.h aren't available yet. This won't break bisection as
this file isn't referenced from the makefile yet, but it makes it a bit
annoying when reviewing patches in order :-S
+
+#define CIF_ISP_INPUT_W_MAXÂÂÂÂÂÂÂ 4032
+#define CIF_ISP_INPUT_H_MAXÂÂÂÂÂÂÂ 3024
+#define CIF_ISP_INPUT_W_MINÂÂÂÂÂÂÂ 32
+#define CIF_ISP_INPUT_H_MINÂÂÂÂÂÂÂ 32
+#define CIF_ISP_OUTPUT_W_MAXÂÂÂÂÂÂÂ CIF_ISP_INPUT_W_MAX
+#define CIF_ISP_OUTPUT_H_MAXÂÂÂÂÂÂÂ CIF_ISP_INPUT_H_MAX
+#define CIF_ISP_OUTPUT_W_MINÂÂÂÂÂÂÂ CIF_ISP_INPUT_W_MIN
+#define CIF_ISP_OUTPUT_H_MINÂÂÂÂÂÂÂ CIF_ISP_INPUT_H_MIN
+
+/*
+ * NOTE: MIPI controller and input MUX are also configured in this file,
+ * because ISP Subdev is not only describe ISP submodule(input size,format,
+ * output size, format), but also a virtual route device.
+ */
+
+/*
+ * There are many variables named with format/frame in below code,
+ * please see here for their meaning.
+ *
+ * Cropping regions of ISP
+ *
+ * +---------------------------------------------------------+
+ * | Sensor imageÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |
+ * | +---------------------------------------------------+ÂÂ |
+ * | | ISP_ACQ (for black level)ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |ÂÂ |
+ * | | in_frmÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |ÂÂ |
+ * | | +--------------------------------------------+ÂÂÂ |ÂÂ |
+ * | | |ÂÂÂ ISP_OUTÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |ÂÂÂ |ÂÂ |
+ * | | |ÂÂÂ in_cropÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |ÂÂÂ |ÂÂ |
in_crop at the ISP output ? That seems a bit weird. I'm guessing that
this is really the ISP output, while ISP_IS is related to the resizer ?
+ * | | |ÂÂÂ +---------------------------------+ÂÂÂÂ |ÂÂÂ |ÂÂ |
+ * | | |ÂÂÂ |ÂÂ ISP_ISÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |ÂÂÂÂ |ÂÂÂ |ÂÂ |
+ * | | |ÂÂÂ |ÂÂ rkisp1_isp_subdev: out_cropÂÂ |ÂÂÂÂ |ÂÂÂ |ÂÂ |
+ * | | |ÂÂÂ +---------------------------------+ÂÂÂÂ |ÂÂÂ |ÂÂ |
+ * | | +--------------------------------------------+ÂÂÂ |ÂÂ |
+ * | +---------------------------------------------------+ÂÂ |
+ * +---------------------------------------------------------+
+ */
+
+static inline struct rkisp1_device *sd_to_isp_dev(struct v4l2_subdev *sd)
+{
+ÂÂÂ return container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev);
+}
+
+/* Get sensor by enabled media link */
+static struct v4l2_subdev *get_remote_sensor(struct v4l2_subdev *sd)
+{
+ÂÂÂ struct media_pad *local, *remote;
+ÂÂÂ struct media_entity *sensor_me;
+
+ÂÂÂ local = &sd->entity.pads[RKISP1_ISP_PAD_SINK];
+ÂÂÂ remote = media_entity_remote_pad(local);
+ÂÂÂ if (!remote) {
+ÂÂÂÂÂÂÂ v4l2_warn(sd, "No link between isp and sensor\n");
+ÂÂÂÂÂÂÂ return NULL;
+ÂÂÂ }
+
+ÂÂÂ sensor_me = media_entity_remote_pad(local)->entity;
+ÂÂÂ return media_entity_to_v4l2_subdev(sensor_me);
+}
+
+static struct rkisp1_sensor *sd_to_sensor(struct rkisp1_device *dev,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_subdev *sd)
+{
+ÂÂÂ struct rkisp1_sensor *sensor;
+
+ÂÂÂ list_for_each_entry(sensor, &dev->sensors, list)
+ÂÂÂÂÂÂÂ if (sensor->sd == sd)
+ÂÂÂÂÂÂÂÂÂÂÂ return sensor;
+
+ÂÂÂ return NULL;
+}
+
+/****************Â register operations ****************/
+
+/*
+ * Image Stabilization.
+ * This should only be called when configuring CIF
+ * or at the frame end interrupt
+ */
+static void rkisp1_config_ism(struct rkisp1_device *dev)
+{
+ÂÂÂ void __iomem *base = dev->base_addr;
+ÂÂÂ struct v4l2_rect *out_crop = &dev->isp_sdev.out_crop;
+ÂÂÂ u32 val;
+
+ÂÂÂ writel(0, base + CIF_ISP_IS_RECENTER);
How about read/write wrappers that take a rkisp1_device pointer, a
register offset and a value (for the write wrapper) and compute
dev->base_addr + offset internally ? That would make the code easier to
read.
+ÂÂÂ writel(0, base + CIF_ISP_IS_MAX_DX);
+ÂÂÂ writel(0, base + CIF_ISP_IS_MAX_DY);
+ÂÂÂ writel(0, base + CIF_ISP_IS_DISPLACE);
+ÂÂÂ writel(out_crop->left, base + CIF_ISP_IS_H_OFFS);
+ÂÂÂ writel(out_crop->top, base + CIF_ISP_IS_V_OFFS);
+ÂÂÂ writel(out_crop->width, base + CIF_ISP_IS_H_SIZE);
+ÂÂÂ writel(out_crop->height, base + CIF_ISP_IS_V_SIZE);
+
+ÂÂÂ /* IS(Image Stabilization) is always on, working as output crop */
+ÂÂÂ writel(1, base + CIF_ISP_IS_CTRL);
+ÂÂÂ val = readl(base + CIF_ISP_CTRL);
+ÂÂÂ val |= CIF_ISP_CTRL_ISP_CFG_UPD;
+ÂÂÂ writel(val, base + CIF_ISP_CTRL);
+}
+
+/*
+ * configure isp blocks with input format, size......
+ */
+static int rkisp1_config_isp(struct rkisp1_device *dev)
+{
+ÂÂÂ u32 isp_ctrl = 0, irq_mask = 0, acq_mult = 0, signal = 0;
+ÂÂÂ struct v4l2_rect *out_crop, *in_crop;
+ÂÂÂ void __iomem *base = dev->base_addr;
+ÂÂÂ struct v4l2_mbus_framefmt *in_frm;
+ÂÂÂ struct ispsd_out_fmt *out_fmt;
+ÂÂÂ struct rkisp1_sensor *sensor;
+ÂÂÂ struct ispsd_in_fmt *in_fmt;
+
+ÂÂÂ sensor = dev->active_sensor;
+ÂÂÂ in_frm = &dev->isp_sdev.in_frm;
+ÂÂÂ in_fmt = &dev->isp_sdev.in_fmt;
+ÂÂÂ out_fmt = &dev->isp_sdev.out_fmt;
+ÂÂÂ out_crop = &dev->isp_sdev.out_crop;
+ÂÂÂ in_crop = &dev->isp_sdev.in_crop;
+
+ÂÂÂ if (in_fmt->fmt_type == FMT_BAYER) {
+ÂÂÂÂÂÂÂ acq_mult = 1;
+ÂÂÂÂÂÂÂ if (out_fmt->fmt_type == FMT_BAYER) {
+ÂÂÂÂÂÂÂÂÂÂÂ if (sensor->mbus.type == V4L2_MBUS_BT656)
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ isp_ctrl =
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ CIF_ISP_CTRL_ISP_MODE_RAW_PICT_ITU656;
+ÂÂÂÂÂÂÂÂÂÂÂ else
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ isp_ctrl =
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ CIF_ISP_CTRL_ISP_MODE_RAW_PICT;
+ÂÂÂÂÂÂÂ } else {
+ÂÂÂÂÂÂÂÂÂÂÂ writel(CIF_ISP_DEMOSAIC_TH(0xc),
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ base + CIF_ISP_DEMOSAIC);
+
+ÂÂÂÂÂÂÂÂÂÂÂ if (sensor->mbus.type == V4L2_MBUS_BT656)
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ isp_ctrl = CIF_ISP_CTRL_ISP_MODE_BAYER_ITU656;
+ÂÂÂÂÂÂÂÂÂÂÂ else
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ isp_ctrl = CIF_ISP_CTRL_ISP_MODE_BAYER_ITU601;
+ÂÂÂÂÂÂÂ }
+ÂÂÂ } else if (in_fmt->fmt_type == FMT_YUV) {
+ÂÂÂÂÂÂÂ acq_mult = 2;
+ÂÂÂÂÂÂÂ if (sensor->mbus.type == V4L2_MBUS_CSI2_DPHY) {
+ÂÂÂÂÂÂÂÂÂÂÂ isp_ctrl = CIF_ISP_CTRL_ISP_MODE_ITU601;
+ÂÂÂÂÂÂÂ } else {
+ÂÂÂÂÂÂÂÂÂÂÂ if (sensor->mbus.type == V4L2_MBUS_BT656)
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ isp_ctrl = CIF_ISP_CTRL_ISP_MODE_ITU656;
+ÂÂÂÂÂÂÂÂÂÂÂ else
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ isp_ctrl = CIF_ISP_CTRL_ISP_MODE_ITU601;
+
+ÂÂÂÂÂÂÂ }
+
+ÂÂÂÂÂÂÂ irq_mask |= CIF_ISP_DATA_LOSS;
+ÂÂÂ }
+
+ÂÂÂ /* Set up input acquisition properties */
+ÂÂÂ if (sensor->mbus.type == V4L2_MBUS_BT656 ||
+ÂÂÂÂÂÂÂ sensor->mbus.type == V4L2_MBUS_PARALLEL) {
+ÂÂÂÂÂÂÂ if (sensor->mbus.flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
+ÂÂÂÂÂÂÂÂÂÂÂ signal = CIF_ISP_ACQ_PROP_POS_EDGE;
+ÂÂÂ }
+
+ÂÂÂ if (sensor->mbus.type == V4L2_MBUS_PARALLEL) {
+ÂÂÂÂÂÂÂ if (sensor->mbus.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
+ÂÂÂÂÂÂÂÂÂÂÂ signal |= CIF_ISP_ACQ_PROP_VSYNC_LOW;
+
+ÂÂÂÂÂÂÂ if (sensor->mbus.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
+ÂÂÂÂÂÂÂÂÂÂÂ signal |= CIF_ISP_ACQ_PROP_HSYNC_LOW;
+ÂÂÂ }
+
+ÂÂÂ writel(isp_ctrl, base + CIF_ISP_CTRL);
+ÂÂÂ writel(signal | in_fmt->yuv_seq |
+ÂÂÂÂÂÂÂÂÂÂ CIF_ISP_ACQ_PROP_BAYER_PAT(in_fmt->bayer_pat) |
+ÂÂÂÂÂÂÂÂÂÂ CIF_ISP_ACQ_PROP_FIELD_SEL_ALL, base + CIF_ISP_ACQ_PROP);
+ÂÂÂ writel(0, base + CIF_ISP_ACQ_NR_FRAMES);
+
+ÂÂÂ /* Acquisition Size */
+ÂÂÂ writel(0, base + CIF_ISP_ACQ_H_OFFS);
+ÂÂÂ writel(0, base + CIF_ISP_ACQ_V_OFFS);
+ÂÂÂ writel(acq_mult * in_frm->width, base + CIF_ISP_ACQ_H_SIZE);
+ÂÂÂ writel(in_frm->height, base + CIF_ISP_ACQ_V_SIZE);
+
+ÂÂÂ /* ISP Out Area */
+ÂÂÂ writel(in_crop->left, base + CIF_ISP_OUT_H_OFFS);
+ÂÂÂ writel(in_crop->top, base + CIF_ISP_OUT_V_OFFS);
+ÂÂÂ writel(in_crop->width, base + CIF_ISP_OUT_H_SIZE);
+ÂÂÂ writel(in_crop->height, base + CIF_ISP_OUT_V_SIZE);
+
+ÂÂÂ /* interrupt mask */
+ÂÂÂ irq_mask |= CIF_ISP_FRAME | CIF_ISP_V_START | CIF_ISP_PIC_SIZE_ERROR |
+ÂÂÂÂÂÂÂÂÂÂÂ CIF_ISP_FRAME_IN;
+ÂÂÂ writel(irq_mask, base + CIF_ISP_IMSC);
+
+ÂÂÂ if (out_fmt->fmt_type == FMT_BAYER)
+ÂÂÂÂÂÂÂ rkisp1_params_disable_isp(&dev->params_vdev);
+ÂÂÂ else
+ÂÂÂÂÂÂÂ rkisp1_params_configure_isp(&dev->params_vdev, in_fmt,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ dev->isp_sdev.quantization);
+
+ÂÂÂ return 0;
+}
+
+static int rkisp1_config_dvp(struct rkisp1_device *dev)
+{
+ÂÂÂ struct ispsd_in_fmt *in_fmt = &dev->isp_sdev.in_fmt;
+ÂÂÂ void __iomem *base = dev->base_addr;
+ÂÂÂ u32 val, input_sel;
+
+ÂÂÂ switch (in_fmt->bus_width) {
+ÂÂÂ case 8:
+ÂÂÂÂÂÂÂ input_sel = CIF_ISP_ACQ_PROP_IN_SEL_8B_ZERO;
+ÂÂÂÂÂÂÂ break;
+ÂÂÂ case 10:
+ÂÂÂÂÂÂÂ input_sel = CIF_ISP_ACQ_PROP_IN_SEL_10B_ZERO;
+ÂÂÂÂÂÂÂ break;
+ÂÂÂ case 12:
+ÂÂÂÂÂÂÂ input_sel = CIF_ISP_ACQ_PROP_IN_SEL_12B;
+ÂÂÂÂÂÂÂ break;
+ÂÂÂ default:
+ÂÂÂÂÂÂÂ v4l2_err(&dev->v4l2_dev, "Invalid bus width\n");
+ÂÂÂÂÂÂÂ return -EINVAL;
+ÂÂÂ }
+
+ÂÂÂ val = readl(base + CIF_ISP_ACQ_PROP);
+ÂÂÂ writel(val | input_sel, base + CIF_ISP_ACQ_PROP);
+
+ÂÂÂ return 0;
+}
+
+static int rkisp1_config_mipi(struct rkisp1_device *dev)
+{
+ÂÂÂ struct ispsd_in_fmt *in_fmt = &dev->isp_sdev.in_fmt;
+ÂÂÂ struct rkisp1_sensor *sensor = dev->active_sensor;
+ÂÂÂ void __iomem *base = dev->base_addr;
+ÂÂÂ unsigned int lanes;
+ÂÂÂ u32 mipi_ctrl;
+
+ÂÂÂ /*
+ÂÂÂÂ * sensor->mbus is set in isp or d-phy notifier_bound function
+ÂÂÂÂ */
+ÂÂÂ switch (sensor->mbus.flags & V4L2_MBUS_CSI2_LANES) {
+ÂÂÂ case V4L2_MBUS_CSI2_4_LANE:
+ÂÂÂÂÂÂÂ lanes = 4;
+ÂÂÂÂÂÂÂ break;
+ÂÂÂ case V4L2_MBUS_CSI2_3_LANE:
+ÂÂÂÂÂÂÂ lanes = 3;
+ÂÂÂÂÂÂÂ break;
+ÂÂÂ case V4L2_MBUS_CSI2_2_LANE:
+ÂÂÂÂÂÂÂ lanes = 2;
+ÂÂÂÂÂÂÂ break;
+ÂÂÂ case V4L2_MBUS_CSI2_1_LANE:
+ÂÂÂÂÂÂÂ lanes = 1;
+ÂÂÂÂÂÂÂ break;
+ÂÂÂ default:
+ÂÂÂÂÂÂÂ return -EINVAL;
+ÂÂÂ }
+
+ÂÂÂ mipi_ctrl = CIF_MIPI_CTRL_NUM_LANES(lanes - 1) |
+ÂÂÂÂÂÂÂÂÂÂÂ CIF_MIPI_CTRL_SHUTDOWNLANES(0xf) |
+ÂÂÂÂÂÂÂÂÂÂÂ CIF_MIPI_CTRL_ERR_SOT_SYNC_HS_SKIP |
+ÂÂÂÂÂÂÂÂÂÂÂ CIF_MIPI_CTRL_CLOCKLANE_ENA;
+
+ÂÂÂ writel(mipi_ctrl, base + CIF_MIPI_CTRL);
+
+ÂÂÂ /* Configure Data Type and Virtual Channel */
+ÂÂÂ writel(CIF_MIPI_DATA_SEL_DT(in_fmt->mipi_dt) | CIF_MIPI_DATA_SEL_VC(0),
+ÂÂÂÂÂÂÂÂÂÂ base + CIF_MIPI_IMG_DATA_SEL);
+
+ÂÂÂ /* Clear MIPI interrupts */
+ÂÂÂ writel(~0, base + CIF_MIPI_ICR);
+ÂÂÂ /*
+ÂÂÂÂ * Disable CIF_MIPI_ERR_DPHY interrupt here temporary for
+ÂÂÂÂ * isp bus may be dead when switch isp.
+ÂÂÂÂ */
+ÂÂÂ writel(CIF_MIPI_FRAME_END | CIF_MIPI_ERR_CSI | CIF_MIPI_ERR_DPHY |
+ÂÂÂÂÂÂÂÂÂÂ CIF_MIPI_SYNC_FIFO_OVFLW(0x03) | CIF_MIPI_ADD_DATA_OVFLW,
+ÂÂÂÂÂÂÂÂÂÂ base + CIF_MIPI_IMSC);
+
+ v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev, "\n MIPI_CTRL 0x%08x\n"
+ÂÂÂÂÂÂÂÂ "Â MIPI_IMG_DATA_SEL 0x%08x\n"
+ÂÂÂÂÂÂÂÂ "Â MIPI_STATUS 0x%08x\n"
+ÂÂÂÂÂÂÂÂ "Â MIPI_IMSC 0x%08x\n",
+ÂÂÂÂÂÂÂÂ readl(base + CIF_MIPI_CTRL),
+ÂÂÂÂÂÂÂÂ readl(base + CIF_MIPI_IMG_DATA_SEL),
+ÂÂÂÂÂÂÂÂ readl(base + CIF_MIPI_STATUS),
+ÂÂÂÂÂÂÂÂ readl(base + CIF_MIPI_IMSC));
+
+ÂÂÂ return 0;
+}
+
+/* Configure MUX */
+static int rkisp1_config_path(struct rkisp1_device *dev)
+{
+ÂÂÂ struct rkisp1_sensor *sensor = dev->active_sensor;
+ÂÂÂ u32 dpcl = readl(dev->base_addr + CIF_VI_DPCL);
+ÂÂÂ int ret = 0;
+
+ÂÂÂ if (sensor->mbus.type == V4L2_MBUS_BT656 ||
+ÂÂÂÂÂÂÂ sensor->mbus.type == V4L2_MBUS_PARALLEL) {
+ÂÂÂÂÂÂÂ ret = rkisp1_config_dvp(dev);
+ÂÂÂÂÂÂÂ dpcl |= CIF_VI_DPCL_IF_SEL_PARALLEL;
+ÂÂÂ } else if (sensor->mbus.type == V4L2_MBUS_CSI2_DPHY) {
+ÂÂÂÂÂÂÂ ret = rkisp1_config_mipi(dev);
+ÂÂÂÂÂÂÂ dpcl |= CIF_VI_DPCL_IF_SEL_MIPI;
+ÂÂÂ }
+
+ÂÂÂ writel(dpcl, dev->base_addr + CIF_VI_DPCL);
+
+ÂÂÂ return ret;
+}
+
+/* Hareware configure Entry */
s/Hareware/Hardware/
+static int rkisp1_config_cif(struct rkisp1_device *dev)
+{
+ÂÂÂ u32 cif_id;
+ÂÂÂ int ret;
+
+ÂÂÂ v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev,
+ÂÂÂÂÂÂÂÂ "SP streaming = %d, MP streaming = %d\n",
+ÂÂÂÂÂÂÂÂ dev->stream[RKISP1_STREAM_SP].streaming,
+ÂÂÂÂÂÂÂÂ dev->stream[RKISP1_STREAM_MP].streaming);
+
+ÂÂÂ cif_id = readl(dev->base_addr + CIF_VI_ID);
+ÂÂÂ v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev, "CIF_ID 0x%08x\n", cif_id);
+
+ÂÂÂ ret = rkisp1_config_isp(dev);
+ÂÂÂ if (ret < 0)
+ÂÂÂÂÂÂÂ return ret;
+ÂÂÂ ret = rkisp1_config_path(dev);
+ÂÂÂ if (ret < 0)
+ÂÂÂÂÂÂÂ return ret;
+ÂÂÂ rkisp1_config_ism(dev);
+
+ÂÂÂ return 0;
+}
+
+/* Mess register operations to stop isp */
Is it such a mess ? :-)
I would capitalise ISP in all comments.
+static int rkisp1_isp_stop(struct rkisp1_device *dev)
+{
+ÂÂÂ void __iomem *base = dev->base_addr;
+ÂÂÂ u32 val;
+
+ÂÂÂ v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev,
+ÂÂÂÂÂÂÂÂ "SP streaming = %d, MP streaming = %d\n",
+ÂÂÂÂÂÂÂÂ dev->stream[RKISP1_STREAM_SP].streaming,
+ÂÂÂÂÂÂÂÂ dev->stream[RKISP1_STREAM_MP].streaming);
+
+ÂÂÂ /*
+ÂÂÂÂ * ISP(mi) stop in mi frame end -> Stop ISP(mipi) ->
+ÂÂÂÂ * Stop ISP(isp) ->wait for ISP isp off
+ÂÂÂÂ */
+ÂÂÂ /* stop and clear MI, MIPI, and ISP interrupts */
+ÂÂÂ writel(0, base + CIF_MIPI_IMSC);
+ÂÂÂ writel(~0, base + CIF_MIPI_ICR);
+
+ÂÂÂ writel(0, base + CIF_ISP_IMSC);
+ÂÂÂ writel(~0, base + CIF_ISP_ICR);
+
+ÂÂÂ writel(0, base + CIF_MI_IMSC);
+ÂÂÂ writel(~0, base + CIF_MI_ICR);
+ÂÂÂ val = readl(base + CIF_MIPI_CTRL);
+ÂÂÂ writel(val & (~CIF_MIPI_CTRL_OUTPUT_ENA), base + CIF_MIPI_CTRL);
+ÂÂÂ /* stop ISP */
+ÂÂÂ val = readl(base + CIF_ISP_CTRL);
+ÂÂÂ val &= ~(CIF_ISP_CTRL_ISP_INFORM_ENABLE | CIF_ISP_CTRL_ISP_ENABLE);
+ÂÂÂ writel(val, base + CIF_ISP_CTRL);
+
+ÂÂÂ val = readl(base + CIF_ISP_CTRL);
+ÂÂÂ writel(val | CIF_ISP_CTRL_ISP_CFG_UPD, base + CIF_ISP_CTRL);
+
+ÂÂÂ readx_poll_timeout(readl, base + CIF_ISP_RIS,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ val, val & CIF_ISP_OFF, 20, 100);
+ÂÂÂ v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev,
+ÂÂÂÂÂÂÂ "streaming(MP:%d, SP:%d), MI_CTRL:%x, ISP_CTRL:%x, MIPI_CTRL:%x\n",
+ÂÂÂÂÂÂÂÂ dev->stream[RKISP1_STREAM_SP].streaming,
+ÂÂÂÂÂÂÂÂ dev->stream[RKISP1_STREAM_MP].streaming,
+ÂÂÂÂÂÂÂÂ readl(base + CIF_MI_CTRL),
+ÂÂÂÂÂÂÂÂ readl(base + CIF_ISP_CTRL),
+ÂÂÂÂÂÂÂÂ readl(base + CIF_MIPI_CTRL));
+
+ÂÂÂ writel(CIF_IRCL_MIPI_SW_RST | CIF_IRCL_ISP_SW_RST, base + CIF_IRCL);
+ÂÂÂ writel(0x0, base + CIF_IRCL);
+
+ÂÂÂ return 0;
+}
+
+/* Mess register operations to start isp */
+static int rkisp1_isp_start(struct rkisp1_device *dev)
+{
+ÂÂÂ struct rkisp1_sensor *sensor = dev->active_sensor;
+ÂÂÂ void __iomem *base = dev->base_addr;
+ÂÂÂ u32 val;
+
+ÂÂÂ v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev,
+ÂÂÂÂÂÂÂÂ "SP streaming = %d, MP streaming = %d\n",
+ÂÂÂÂÂÂÂÂ dev->stream[RKISP1_STREAM_SP].streaming,
+ÂÂÂÂÂÂÂÂ dev->stream[RKISP1_STREAM_MP].streaming);
+
+ÂÂÂ /* Activate MIPI */
+ÂÂÂ if (sensor->mbus.type == V4L2_MBUS_CSI2_DPHY) {
+ÂÂÂÂÂÂÂ val = readl(base + CIF_MIPI_CTRL);
+ÂÂÂÂÂÂÂ writel(val | CIF_MIPI_CTRL_OUTPUT_ENA, base + CIF_MIPI_CTRL);
+ÂÂÂ }
+ÂÂÂ /* Activate ISP */
+ÂÂÂ val = readl(base + CIF_ISP_CTRL);
+ÂÂÂ val |= CIF_ISP_CTRL_ISP_CFG_UPD | CIF_ISP_CTRL_ISP_ENABLE |
+ÂÂÂÂÂÂÂÂÂÂ CIF_ISP_CTRL_ISP_INFORM_ENABLE;
+ÂÂÂ writel(val, base + CIF_ISP_CTRL);
+
+ÂÂÂ /* XXX: Is the 1000us too long?
+ÂÂÂÂ * CIF spec says to wait for sufficient time after enabling
+ÂÂÂÂ * the MIPI interface and before starting the sensor output.
+ÂÂÂÂ */
+ÂÂÂ usleep_range(1000, 1200);
+
+ÂÂÂ v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev,
+ÂÂÂÂÂÂÂÂ "SP streaming = %d, MP streaming = %d MI_CTRL 0x%08x\n"
+ÂÂÂÂÂÂÂÂ "Â ISP_CTRL 0x%08x MIPI_CTRL 0x%08x\n",
+ÂÂÂÂÂÂÂÂ dev->stream[RKISP1_STREAM_SP].streaming,
+ÂÂÂÂÂÂÂÂ dev->stream[RKISP1_STREAM_MP].streaming,
+ÂÂÂÂÂÂÂÂ readl(base + CIF_MI_CTRL),
+ÂÂÂÂÂÂÂÂ readl(base + CIF_ISP_CTRL),
+ÂÂÂÂÂÂÂÂ readl(base + CIF_MIPI_CTRL));
+
+ÂÂÂ return 0;
+}
+
+static void rkisp1_config_clk(struct rkisp1_device *dev)
+{
+ÂÂÂ u32 val = CIF_ICCL_ISP_CLK | CIF_ICCL_CP_CLK | CIF_ICCL_MRSZ_CLK |
+ÂÂÂÂÂÂÂÂÂ CIF_ICCL_SRSZ_CLK | CIF_ICCL_JPEG_CLK | CIF_ICCL_MI_CLK |
+ÂÂÂÂÂÂÂÂÂ CIF_ICCL_IE_CLK | CIF_ICCL_MIPI_CLK | CIF_ICCL_DCROP_CLK;
+
+ÂÂÂ writel(val, dev->base_addr + CIF_ICCL);
+}
+
+/***************************** isp sub-devs *******************************/
+
+static const struct ispsd_in_fmt rkisp1_isp_input_formats[] = {
+ÂÂÂ {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SBGGR10_1X10,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂÂÂÂÂ .mipi_dtÂÂÂ = CIF_CSI2_DT_RAW10,
+ÂÂÂÂÂÂÂ .bayer_patÂÂÂ = RAW_BGGR,
+ÂÂÂÂÂÂÂ .bus_widthÂÂÂ = 10,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SRGGB10_1X10,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂÂÂÂÂ .mipi_dtÂÂÂ = CIF_CSI2_DT_RAW10,
+ÂÂÂÂÂÂÂ .bayer_patÂÂÂ = RAW_RGGB,
+ÂÂÂÂÂÂÂ .bus_widthÂÂÂ = 10,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SGBRG10_1X10,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂÂÂÂÂ .mipi_dtÂÂÂ = CIF_CSI2_DT_RAW10,
+ÂÂÂÂÂÂÂ .bayer_patÂÂÂ = RAW_GBRG,
+ÂÂÂÂÂÂÂ .bus_widthÂÂÂ = 10,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SGRBG10_1X10,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂÂÂÂÂ .mipi_dtÂÂÂ = CIF_CSI2_DT_RAW10,
+ÂÂÂÂÂÂÂ .bayer_patÂÂÂ = RAW_GRBG,
+ÂÂÂÂÂÂÂ .bus_widthÂÂÂ = 10,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SRGGB12_1X12,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂÂÂÂÂ .mipi_dtÂÂÂ = CIF_CSI2_DT_RAW12,
+ÂÂÂÂÂÂÂ .bayer_patÂÂÂ = RAW_RGGB,
+ÂÂÂÂÂÂÂ .bus_widthÂÂÂ = 12,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SBGGR12_1X12,
Is there a reason why the 10-bit Bayer format start with BGGR while the
12-bit formats start with RGGB ? Not a big deal, just OCD kicking in :-)
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂÂÂÂÂ .mipi_dtÂÂÂ = CIF_CSI2_DT_RAW12,
+ÂÂÂÂÂÂÂ .bayer_patÂÂÂ = RAW_BGGR,
+ÂÂÂÂÂÂÂ .bus_widthÂÂÂ = 12,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SGBRG12_1X12,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂÂÂÂÂ .mipi_dtÂÂÂ = CIF_CSI2_DT_RAW12,
+ÂÂÂÂÂÂÂ .bayer_patÂÂÂ = RAW_GBRG,
+ÂÂÂÂÂÂÂ .bus_widthÂÂÂ = 12,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SGRBG12_1X12,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂÂÂÂÂ .mipi_dtÂÂÂ = CIF_CSI2_DT_RAW12,
+ÂÂÂÂÂÂÂ .bayer_patÂÂÂ = RAW_GRBG,
+ÂÂÂÂÂÂÂ .bus_widthÂÂÂ = 12,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SRGGB8_1X8,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂÂÂÂÂ .mipi_dtÂÂÂ = CIF_CSI2_DT_RAW8,
+ÂÂÂÂÂÂÂ .bayer_patÂÂÂ = RAW_RGGB,
+ÂÂÂÂÂÂÂ .bus_widthÂÂÂ = 8,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SBGGR8_1X8,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂÂÂÂÂ .mipi_dtÂÂÂ = CIF_CSI2_DT_RAW8,
+ÂÂÂÂÂÂÂ .bayer_patÂÂÂ = RAW_BGGR,
+ÂÂÂÂÂÂÂ .bus_widthÂÂÂ = 8,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SGBRG8_1X8,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂÂÂÂÂ .mipi_dtÂÂÂ = CIF_CSI2_DT_RAW8,
+ÂÂÂÂÂÂÂ .bayer_patÂÂÂ = RAW_GBRG,
+ÂÂÂÂÂÂÂ .bus_widthÂÂÂ = 8,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SGRBG8_1X8,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂÂÂÂÂ .mipi_dtÂÂÂ = CIF_CSI2_DT_RAW8,
+ÂÂÂÂÂÂÂ .bayer_patÂÂÂ = RAW_GRBG,
+ÂÂÂÂÂÂÂ .bus_widthÂÂÂ = 8,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_YUYV8_1X16,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_YUV,
+ÂÂÂÂÂÂÂ .mipi_dtÂÂÂ = CIF_CSI2_DT_YUV422_8b,
+ÂÂÂÂÂÂÂ .yuv_seqÂÂÂ = CIF_ISP_ACQ_PROP_YCBYCR,
+ÂÂÂÂÂÂÂ .bus_widthÂÂÂ = 16,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_YVYU8_1X16,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_YUV,
+ÂÂÂÂÂÂÂ .mipi_dtÂÂÂ = CIF_CSI2_DT_YUV422_8b,
+ÂÂÂÂÂÂÂ .yuv_seqÂÂÂ = CIF_ISP_ACQ_PROP_YCRYCB,
+ÂÂÂÂÂÂÂ .bus_widthÂÂÂ = 16,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_UYVY8_1X16,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_YUV,
+ÂÂÂÂÂÂÂ .mipi_dtÂÂÂ = CIF_CSI2_DT_YUV422_8b,
+ÂÂÂÂÂÂÂ .yuv_seqÂÂÂ = CIF_ISP_ACQ_PROP_CBYCRY,
+ÂÂÂÂÂÂÂ .bus_widthÂÂÂ = 16,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_VYUY8_1X16,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_YUV,
+ÂÂÂÂÂÂÂ .mipi_dtÂÂÂ = CIF_CSI2_DT_YUV422_8b,
+ÂÂÂÂÂÂÂ .yuv_seqÂÂÂ = CIF_ISP_ACQ_PROP_CRYCBY,
+ÂÂÂÂÂÂÂ .bus_widthÂÂÂ = 16,
+ÂÂÂ },
+};
+
+static const struct ispsd_out_fmt rkisp1_isp_output_formats[] = {
+ÂÂÂ {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_YUYV8_2X8,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_YUV,
+ÂÂÂ }, {
This is the only entry not present in the previous table, so I'm
wondering if it would make sense to merge the two tables and rename
ispsd_in_fmt to rkisp1_format_info. You would need to add a field that
tells, for each format, if it's valid as an input format, and output
format, or both. Hmmmm and also make the enum logic a bit more complex.
Maybe it's not worth it after all, but it bothers me a bit to have two
tables :-) I'll let you decide what's best.
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SRGGB12_1X12,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SBGGR12_1X12,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SGBRG12_1X12,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SGRBG12_1X12,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SRGGB10_1X10,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SBGGR10_1X10,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SGBRG10_1X10,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SGRBG10_1X10,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SRGGB8_1X8,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SBGGR8_1X8,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SGBRG8_1X8,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂ }, {
+ÂÂÂÂÂÂÂ .mbus_codeÂÂÂ = MEDIA_BUS_FMT_SGRBG8_1X8,
+ÂÂÂÂÂÂÂ .fmt_typeÂÂÂ = FMT_BAYER,
+ÂÂÂ },
+};
+
+static const struct ispsd_in_fmt *find_in_fmt(u32 mbus_code)
+{
+ÂÂÂ unsigned int i, array_size = ARRAY_SIZE(rkisp1_isp_input_formats);
+ÂÂÂ const struct ispsd_in_fmt *fmt;
You can move this variable inside the loop.
+
+ÂÂÂ for (i = 0; i < array_size; i++) {
I would remove the array_size local variable, it doesn't improve
readability. Same for the next function.
+ÂÂÂÂÂÂÂ fmt = &rkisp1_isp_input_formats[i];
+ÂÂÂÂÂÂÂ if (fmt->mbus_code == mbus_code)
+ÂÂÂÂÂÂÂÂÂÂÂ return fmt;
+ÂÂÂ }
+
+ÂÂÂ return NULL;
+}
+
+static const struct ispsd_out_fmt *find_out_fmt(u32 mbus_code)
+{
+ÂÂÂ unsigned int i, array_size = ARRAY_SIZE(rkisp1_isp_output_formats);
+ÂÂÂ const struct ispsd_out_fmt *fmt;
+
+ÂÂÂ for (i = 0; i < array_size; i++) {
+ÂÂÂÂÂÂÂ fmt = &rkisp1_isp_output_formats[i];
+ÂÂÂÂÂÂÂ if (fmt->mbus_code == mbus_code)
+ÂÂÂÂÂÂÂÂÂÂÂ return fmt;
+ÂÂÂ }
+
+ÂÂÂ return NULL;
+}
+
+static int rkisp1_isp_sd_enum_mbus_code(struct v4l2_subdev *sd,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_subdev_pad_config *cfg,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_subdev_mbus_code_enum *code)
+{
+ÂÂÂ unsigned int i = code->index;
+
+ÂÂÂ if ((code->pad != RKISP1_ISP_PAD_SINK) &&
+ÂÂÂÂÂÂÂ (code->pad != RKISP1_ISP_PAD_SOURCE_PATH)) {
+ÂÂÂÂÂÂÂ if (i > 0)
+ÂÂÂÂÂÂÂÂÂÂÂ return -EINVAL;
+ÂÂÂÂÂÂÂ code->code = MEDIA_BUS_FMT_FIXED;
+ÂÂÂÂÂÂÂ return 0;
+ÂÂÂ }
+
+ÂÂÂ if (code->pad == RKISP1_ISP_PAD_SINK) {
+ÂÂÂÂÂÂÂ if (i >= ARRAY_SIZE(rkisp1_isp_input_formats))
+ÂÂÂÂÂÂÂÂÂÂÂ return -EINVAL;
+ÂÂÂÂÂÂÂ code->code = rkisp1_isp_input_formats[i].mbus_code;
+ÂÂÂ } else {
+ÂÂÂÂÂÂÂ if (i >= ARRAY_SIZE(rkisp1_isp_output_formats))
+ÂÂÂÂÂÂÂÂÂÂÂ return -EINVAL;
+ÂÂÂÂÂÂÂ code->code = rkisp1_isp_output_formats[i].mbus_code;
+ÂÂÂ }
On the other hand, merging the two tables above into one, you could
merge the two branches here and only consider formats indicated as valid
for to the pad you want. Maybe the code would be cleaner in the end.
+
+ÂÂÂ return 0;
+}
+
+static int rkisp1_isp_sd_init_config(struct v4l2_subdev *sd,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_subdev_pad_config *cfg)
+{
+ÂÂÂ struct v4l2_rect *mf_in_crop, *mf_out_crop;
+ÂÂÂ struct v4l2_mbus_framefmt *mf_in, *mf_out;
+
+ÂÂÂ mf_in = v4l2_subdev_get_try_format(sd, cfg, RKISP1_ISP_PAD_SINK);
+ÂÂÂ mf_in->width = RKISP1_DEFAULT_WIDTH;
+ÂÂÂ mf_in->height = RKISP1_DEFAULT_HEIGHT;
+ÂÂÂ mf_in->field = V4L2_FIELD_NONE;
+ÂÂÂ mf_in->code = rkisp1_isp_input_formats[0].mbus_code;
+
+ÂÂÂ mf_in_crop = v4l2_subdev_get_try_crop(sd, cfg, RKISP1_ISP_PAD_SINK);
+ÂÂÂ mf_in_crop->width = RKISP1_DEFAULT_WIDTH;
+ÂÂÂ mf_in_crop->height = RKISP1_DEFAULT_HEIGHT;
+ÂÂÂ mf_in_crop->left = 0;
+ÂÂÂ mf_in_crop->top = 0;
+
+ÂÂÂ mf_out = v4l2_subdev_get_try_format(sd, cfg,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ RKISP1_ISP_PAD_SOURCE_PATH);
+ÂÂÂ *mf_out = *mf_in;
+ÂÂÂ mf_out->code = rkisp1_isp_output_formats[0].mbus_code;
+ÂÂÂ mf_out->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+
+ÂÂÂ mf_out_crop = v4l2_subdev_get_try_crop(sd, cfg,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ RKISP1_ISP_PAD_SOURCE_PATH);
+ÂÂÂ *mf_out_crop = *mf_in_crop;
+
+ÂÂÂ return 0;
+}
+
+static int rkisp1_isp_sd_get_fmt(struct v4l2_subdev *sd,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_subdev_pad_config *cfg,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_subdev_format *fmt)
+{
+ÂÂÂ struct rkisp1_isp_subdev *isp_sd = sd_to_isp_sd(sd);
+ÂÂÂ struct v4l2_mbus_framefmt *mf = &fmt->format;
+
+ÂÂÂ if ((fmt->pad != RKISP1_ISP_PAD_SINK) &&
+ÂÂÂÂÂÂÂ (fmt->pad != RKISP1_ISP_PAD_SOURCE_PATH)) {
+ÂÂÂÂÂÂÂ fmt->format.code = MEDIA_BUS_FMT_FIXED;
+ÂÂÂÂÂÂÂ /*
+ÂÂÂÂÂÂÂÂ * NOTE: setting a format here doesn't make much sense
+ÂÂÂÂÂÂÂÂ * but v4l2-compliance complains
+ÂÂÂÂÂÂÂÂ */
For the params pad I agreed it makes no sense, and I think
v4l2-compliance is at fault, so I'd set width and height to 0. For the
stats pad we *could* use the size of the image from which stats are
computed, but because v4l2_meta_format has no width/height, I think 0
would also be appropriate.
+ÂÂÂÂÂÂÂ fmt->format.width = RKISP1_DEFAULT_WIDTH;
+ÂÂÂÂÂÂÂ fmt->format.height = RKISP1_DEFAULT_HEIGHT;
+ÂÂÂÂÂÂÂ fmt->format.field = V4L2_FIELD_NONE;
+ÂÂÂÂÂÂÂ return 0;
+ÂÂÂ }
+
+ÂÂÂ if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
+ÂÂÂÂÂÂÂ mf = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
+ÂÂÂÂÂÂÂ fmt->format = *mf;
+ÂÂÂÂÂÂÂ return 0;
+ÂÂÂ }
+
+ÂÂÂ if (fmt->pad == RKISP1_ISP_PAD_SINK) {
+ÂÂÂÂÂÂÂ *mf = isp_sd->in_frm;
+ÂÂÂ } else if (fmt->pad == RKISP1_ISP_PAD_SOURCE_PATH) {
+ÂÂÂÂÂÂÂ /* format of source pad */
+ÂÂÂÂÂÂÂ *mf = isp_sd->in_frm;
+ÂÂÂÂÂÂÂ mf->code = isp_sd->out_fmt.mbus_code;
+ÂÂÂÂÂÂÂ /* window size of source pad */
+ÂÂÂÂÂÂÂ mf->width = isp_sd->out_crop.width;
+ÂÂÂÂÂÂÂ mf->height = isp_sd->out_crop.height;
+ÂÂÂÂÂÂÂ mf->quantization = isp_sd->quantization;
+ÂÂÂ }
+ÂÂÂ mf->field = V4L2_FIELD_NONE;
This can be simplified, please read through, or jump to the review of
rkisp1_isp_subdev at the end.
+
+ÂÂÂ return 0;
+}
+
+static void rkisp1_isp_sd_try_fmt(struct v4l2_subdev *sd,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned int pad,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_mbus_framefmt *fmt)
+{
+ÂÂÂ struct rkisp1_device *isp_dev = sd_to_isp_dev(sd);
+ÂÂÂ struct rkisp1_isp_subdev *isp_sd = &isp_dev->isp_sdev;
+ÂÂÂ const struct ispsd_out_fmt *out_fmt;
+ÂÂÂ const struct ispsd_in_fmt *in_fmt;
+
+ÂÂÂ switch (pad) {
+ÂÂÂ case RKISP1_ISP_PAD_SINK:
+ÂÂÂÂÂÂÂ in_fmt = find_in_fmt(fmt->code);
+ÂÂÂÂÂÂÂ if (in_fmt)
+ÂÂÂÂÂÂÂÂÂÂÂ fmt->code = in_fmt->mbus_code;
+ÂÂÂÂÂÂÂ else
+ÂÂÂÂÂÂÂÂÂÂÂ fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
You write MEDIA_BUS_FMT_SRGGB10_1X10 explicitly here, while you use
rkisp1_isp_output_formats[0].mbus_code below (and in other places). I
would standardise on one of the two (explicit formats or array[0]), with
a preference for the first as that would allow merging the input and
output arrays more easily. I would then create two #define,
RKISP1_DEF_INPUT_FORMAT and RKISP2_DEF_OUTPUT_FORMAT (or similar).
Similar macros for the default width and height could also be useful, to
make it easier to change them.
+ fmt->width = clamp_t(u32, fmt->width, CIF_ISP_INPUT_W_MIN,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ CIF_ISP_INPUT_W_MAX);
+ÂÂÂÂÂÂÂ fmt->height = clamp_t(u32, fmt->height, CIF_ISP_INPUT_H_MIN,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ CIF_ISP_INPUT_H_MAX);
+ÂÂÂÂÂÂÂ break;
+ÂÂÂ case RKISP1_ISP_PAD_SOURCE_PATH:
+ÂÂÂÂÂÂÂ out_fmt = find_out_fmt(fmt->code);
+ÂÂÂÂÂÂÂ if (out_fmt)
+ÂÂÂÂÂÂÂÂÂÂÂ fmt->code = out_fmt->mbus_code;
+ÂÂÂÂÂÂÂ else
+ÂÂÂÂÂÂÂÂÂÂÂ fmt->code = rkisp1_isp_output_formats[0].mbus_code;
+ÂÂÂÂÂÂÂ /* window size is set in s_selection */
+ fmt->width = isp_sd->out_crop.width;
+ÂÂÂÂÂÂÂ fmt->height = isp_sd->out_crop.height;
This function operates on the TRY configuration too, in which case you
should use the TRY crop rectangle here, not the ACTIVE one. If you've
already jumped to the review of rkisp1_isp_subdev you know my proposal
to simplify this. Otherwise now may be a good time to do so :-)
+ÂÂÂÂÂÂÂ /* full range by default */
+ÂÂÂÂÂÂÂ if (!fmt->quantization)
+ÂÂÂÂÂÂÂÂÂÂÂ fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+ÂÂÂÂÂÂÂ break;
+ÂÂÂ }
+
+ÂÂÂ fmt->field = V4L2_FIELD_NONE;
+}
+
+static int rkisp1_isp_sd_set_fmt(struct v4l2_subdev *sd,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_subdev_pad_config *cfg,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_subdev_format *fmt)
+{
+ÂÂÂ struct rkisp1_device *isp_dev = sd_to_isp_dev(sd);
+ÂÂÂ struct rkisp1_isp_subdev *isp_sd = &isp_dev->isp_sdev;
+ÂÂÂ struct v4l2_mbus_framefmt *mf = &fmt->format;
+
+ÂÂÂ if ((fmt->pad != RKISP1_ISP_PAD_SINK) &&
+ÂÂÂÂÂÂÂ (fmt->pad != RKISP1_ISP_PAD_SOURCE_PATH))
+ÂÂÂÂÂÂÂ return rkisp1_isp_sd_get_fmt(sd, cfg, fmt);
+
+ÂÂÂ rkisp1_isp_sd_try_fmt(sd, fmt->pad, mf);
+
+ÂÂÂ if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
+ÂÂÂÂÂÂÂ struct v4l2_mbus_framefmt *try_mf;
+
+ÂÂÂÂÂÂÂ try_mf = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
+ÂÂÂÂÂÂÂ *try_mf = *mf;
+ÂÂÂÂÂÂÂ return 0;
When setting the format on the sink pad the crop rectangles need to be
reset (here, and for the ACTIVE format below too).
+ÂÂÂ }
+
+ÂÂÂ if (fmt->pad == RKISP1_ISP_PAD_SINK) {
+ÂÂÂÂÂÂÂ const struct ispsd_in_fmt *in_fmt;
+
+ÂÂÂÂÂÂÂ in_fmt = find_in_fmt(mf->code);
+ÂÂÂÂÂÂÂ isp_sd->in_fmt = *in_fmt;
+ÂÂÂÂÂÂÂ isp_sd->in_frm = *mf;
+ÂÂÂ } else if (fmt->pad == RKISP1_ISP_PAD_SOURCE_PATH) {
+ÂÂÂÂÂÂÂ const struct ispsd_out_fmt *out_fmt;
+
+ÂÂÂÂÂÂÂ /* Ignore width/height */
+ÂÂÂÂÂÂÂ out_fmt = find_out_fmt(mf->code);
+ÂÂÂÂÂÂÂ isp_sd->out_fmt = *out_fmt;
I would return the in_fmt and out_fmt from rkisp1_isp_sd_try_fmt() as it
already looks them up. If you merge the input and output tables, you'll
have a single format info structure type, and rkisp1_isp_sd_try_fmt()
could return the entry for the pad it operates on.
+ÂÂÂÂÂÂÂ /*
+ÂÂÂÂÂÂÂÂ * It is quantization for output,
+ÂÂÂÂÂÂÂÂ * isp use bt601 limit-range in internal
+ÂÂÂÂÂÂÂÂ */
+ÂÂÂÂÂÂÂ isp_sd->quantization = mf->quantization;
+ÂÂÂ }
+
+ÂÂÂ return 0;
+}
+
+static void rkisp1_isp_sd_try_crop(struct v4l2_subdev *sd,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_subdev_pad_config *cfg,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_subdev_selection *sel)
+{
+ÂÂÂ struct rkisp1_isp_subdev *isp_sd = sd_to_isp_sd(sd);
+ÂÂÂ struct v4l2_mbus_framefmt in_frm = isp_sd->in_frm;
+ÂÂÂ struct v4l2_rect in_crop = isp_sd->in_crop;
+ÂÂÂ struct v4l2_rect *input = &sel->r;
+
+ÂÂÂ if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
+ÂÂÂÂÂÂÂ in_frm = *v4l2_subdev_get_try_format(sd, cfg,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ RKISP1_ISP_PAD_SINK);
+ÂÂÂÂÂÂÂ in_crop = *v4l2_subdev_get_try_crop(sd, cfg,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ RKISP1_ISP_PAD_SINK);
+ÂÂÂ }
+
+ÂÂÂ input->left = ALIGN(input->left, 2);
+ÂÂÂ input->width = ALIGN(input->width, 2);
+
+ÂÂÂ if (sel->pad == RKISP1_ISP_PAD_SINK) {
+ÂÂÂÂÂÂÂ input->left = clamp_t(u32, input->left, 0, in_frm.width);
+ÂÂÂÂÂÂÂ input->top = clamp_t(u32, input->top, 0, in_frm.height);
+ÂÂÂÂÂÂÂ input->width = clamp_t(u32, input->width, CIF_ISP_INPUT_W_MIN,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ in_frm.width - input->left);
+ÂÂÂÂÂÂÂ input->height = clamp_t(u32, input->height,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ CIF_ISP_INPUT_H_MIN,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ in_frm.height - input->top);
+ÂÂÂ } else if (sel->pad == RKISP1_ISP_PAD_SOURCE_PATH) {
+ÂÂÂÂÂÂÂ input->left = clamp_t(u32, input->left, 0, in_crop.width);
+ÂÂÂÂÂÂÂ input->top = clamp_t(u32, input->top, 0, in_crop.height);
+ÂÂÂÂÂÂÂ input->width = clamp_t(u32, input->width, CIF_ISP_OUTPUT_W_MIN,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ in_crop.width - input->left);
+ÂÂÂÂÂÂÂ input->height = clamp_t(u32, input->height,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ CIF_ISP_OUTPUT_H_MIN,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ in_crop.height - input->top);
+ÂÂÂ }
+}
+
+static int rkisp1_isp_sd_get_selection(struct v4l2_subdev *sd,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_subdev_pad_config *cfg,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_subdev_selection *sel)
+{
+ÂÂÂ struct rkisp1_isp_subdev *isp_sd = sd_to_isp_sd(sd);
+ÂÂÂ struct v4l2_mbus_framefmt *frm;
+ÂÂÂ struct v4l2_rect *rect;
+
+ÂÂÂ if (sel->pad != RKISP1_ISP_PAD_SOURCE_PATH &&
+ÂÂÂÂÂÂÂ sel->pad != RKISP1_ISP_PAD_SINK)
+ÂÂÂÂÂÂÂ return -EINVAL;
+
+ÂÂÂ switch (sel->target) {
+ÂÂÂ case V4L2_SEL_TGT_CROP_BOUNDS:
+ÂÂÂÂÂÂÂ if (sel->pad == RKISP1_ISP_PAD_SINK) {
+ÂÂÂÂÂÂÂÂÂÂÂ if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ frm = v4l2_subdev_get_try_format(sd, cfg,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ sel->pad);
+ÂÂÂÂÂÂÂÂÂÂÂ else
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ frm = &isp_sd->in_frm;
+
+ÂÂÂÂÂÂÂÂÂÂÂ sel->r.height = frm->height;
+ÂÂÂÂÂÂÂÂÂÂÂ sel->r.width = frm->width;
+ÂÂÂÂÂÂÂÂÂÂÂ sel->r.left = 0;
+ÂÂÂÂÂÂÂÂÂÂÂ sel->r.top = 0;
+ÂÂÂÂÂÂÂ } else {
+ÂÂÂÂÂÂÂÂÂÂÂ if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ rect = v4l2_subdev_get_try_crop(sd, cfg,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ RKISP1_ISP_PAD_SINK);
+ÂÂÂÂÂÂÂÂÂÂÂ else
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ rect = &isp_sd->in_crop;
+ÂÂÂÂÂÂÂÂÂÂÂ sel->r = *rect;
+ÂÂÂÂÂÂÂ }
+ÂÂÂÂÂÂÂ break;
+ÂÂÂ case V4L2_SEL_TGT_CROP:
+ÂÂÂÂÂÂÂ if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
+ÂÂÂÂÂÂÂÂÂÂÂ rect = v4l2_subdev_get_try_crop(sd, cfg, sel->pad);
+ÂÂÂÂÂÂÂ else if (sel->pad == RKISP1_ISP_PAD_SINK)
+ÂÂÂÂÂÂÂÂÂÂÂ rect = &isp_sd->in_crop;
+ÂÂÂÂÂÂÂ else
+ÂÂÂÂÂÂÂÂÂÂÂ rect = &isp_sd->out_crop;
+ÂÂÂÂÂÂÂ sel->r = *rect;
+ÂÂÂÂÂÂÂ break;
+ÂÂÂ default:
+ÂÂÂÂÂÂÂ return -EINVAL;
+ÂÂÂ }
+
+ÂÂÂ return 0;
+}
+
+static int rkisp1_isp_sd_set_selection(struct v4l2_subdev *sd,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_subdev_pad_config *cfg,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_subdev_selection *sel)
+{
+ÂÂÂ struct rkisp1_isp_subdev *isp_sd = sd_to_isp_sd(sd);
+ÂÂÂ struct rkisp1_device *dev = sd_to_isp_dev(sd);
+
+ÂÂÂ if (sel->pad != RKISP1_ISP_PAD_SOURCE_PATH &&
+ÂÂÂÂÂÂÂ sel->pad != RKISP1_ISP_PAD_SINK)
+ÂÂÂÂÂÂÂ return -EINVAL;
+ÂÂÂ if (sel->target != V4L2_SEL_TGT_CROP)
+ÂÂÂÂÂÂÂ return -EINVAL;
+
+ÂÂÂ v4l2_dbg(1, rkisp1_debug, &dev->v4l2_dev,
+ÂÂÂÂÂÂÂÂ "%s: pad: %d sel(%d,%d)/%dx%d\n", __func__, sel->pad,
+ÂÂÂÂÂÂÂÂ sel->r.left, sel->r.top, sel->r.width, sel->r.height);
+ÂÂÂ rkisp1_isp_sd_try_crop(sd, cfg, sel);
+
+ÂÂÂ if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
+ÂÂÂÂÂÂÂ struct v4l2_rect *try_sel =
+ÂÂÂÂÂÂÂÂÂÂÂ v4l2_subdev_get_try_crop(sd, cfg, sel->pad);
+
+ÂÂÂÂÂÂÂ *try_sel = sel->r;
+ÂÂÂÂÂÂÂ return 0;
When setting the crop rectangle on the sink pad the rectangle on the
source pad should be reset (and below for the ACTIVE format too).
The resizer logic, through selection rectangles, seems to be missing. I
assume it's configured through the video nodes, but that's not correct
I'm afraid, scaling should be configured on subdevs, see [1]. I'm afraid
this means that we'll need separate subdevs for the resizers :-S
[1] https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/dev-subdev.html#order-of-configuration-and-format-propagation
+ÂÂÂ }
+
+ÂÂÂ if (sel->pad == RKISP1_ISP_PAD_SINK)
+ÂÂÂÂÂÂÂ isp_sd->in_crop = sel->r;
+ÂÂÂ else
+ÂÂÂÂÂÂÂ isp_sd->out_crop = sel->r;
+
+ÂÂÂ return 0;
+}
+
I'll skip the part related to power on/off below as Sakari requested it
to be handled through runtime PM.
+static int mipi_csi2_s_stream_start(struct rkisp1_isp_subdev *isp_sd,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct rkisp1_sensor *sensor)
+{
+ÂÂÂ union phy_configure_opts opts = { 0 };
+ÂÂÂ struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy;
+ÂÂÂ struct v4l2_ctrl *pixel_rate;
+ÂÂÂ s64 pixel_clock;
+
+ÂÂÂ pixel_rate = v4l2_ctrl_find(sensor->sd->ctrl_handler,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ V4L2_CID_PIXEL_RATE);
+ÂÂÂ if (!pixel_rate) {
+ÂÂÂÂÂÂÂ v4l2_warn(sensor->sd, "No pixel rate control in subdev\n");
+ÂÂÂÂÂÂÂ return -EPIPE;
+ÂÂÂ }
Would it make sense to retrieve (and cache) the control pointer (not its
value of course) at probe time already ?
+
+ÂÂÂ pixel_clock = v4l2_ctrl_g_ctrl_int64(pixel_rate);
+ÂÂÂ if (!pixel_clock) {
+ÂÂÂÂÂÂÂ v4l2_err(sensor->sd, "Invalid pixel rate value\n");
+ÂÂÂÂÂÂÂ return -EINVAL;
+ÂÂÂ }
+
+ÂÂÂ phy_mipi_dphy_get_default_config(pixel_clock, isp_sd->in_fmt.bus_width,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ sensor->lanes, cfg);
+ÂÂÂ phy_set_mode(sensor->dphy, PHY_MODE_MIPI_DPHY);
+ÂÂÂ phy_configure(sensor->dphy, &opts);
+ÂÂÂ phy_power_on(sensor->dphy);
+
+ÂÂÂ return 0;
+}
+
+static void mipi_csi2_s_stream_stop(struct rkisp1_sensor *sensor)
+{
+ÂÂÂ phy_power_off(sensor->dphy);
+}
+
+static int rkisp1_isp_sd_s_stream(struct v4l2_subdev *sd, int on)
+{
+ÂÂÂ struct rkisp1_device *isp_dev = sd_to_isp_dev(sd);
+ÂÂÂ struct v4l2_subdev *sensor_sd;
+ÂÂÂ int ret = 0;
+
+ÂÂÂ if (!on) {
+ÂÂÂÂÂÂÂ ret = rkisp1_isp_stop(isp_dev);
+ÂÂÂÂÂÂÂ if (ret < 0)
+ÂÂÂÂÂÂÂÂÂÂÂ return ret;
+ÂÂÂÂÂÂÂ mipi_csi2_s_stream_stop(isp_dev->active_sensor);
+ÂÂÂÂÂÂÂ return 0;
+ÂÂÂ }
+
+ÂÂÂ sensor_sd = get_remote_sensor(sd);
+ÂÂÂ if (!sensor_sd)
+ÂÂÂÂÂÂÂ return -ENODEV;
+
+ÂÂÂ isp_dev->active_sensor = sd_to_sensor(isp_dev, sensor_sd);
+ÂÂÂ if (!isp_dev->active_sensor)
+ÂÂÂÂÂÂÂ return -ENODEV;
+
+ÂÂÂ atomic_set(&isp_dev->isp_sdev.frm_sync_seq, 0);
+ÂÂÂ ret = rkisp1_config_cif(isp_dev);
+ÂÂÂ if (ret < 0)
+ÂÂÂÂÂÂÂ return ret;
+
+ÂÂÂ /* TODO: support other interfaces */
+ÂÂÂ if (isp_dev->active_sensor->mbus.type != V4L2_MBUS_CSI2_DPHY)
+ÂÂÂÂÂÂÂ return -EINVAL;
+
+ÂÂÂ ret = mipi_csi2_s_stream_start(&isp_dev->isp_sdev,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ isp_dev->active_sensor);
+ÂÂÂ if (ret < 0)
+ÂÂÂÂÂÂÂ return ret;
+
+ÂÂÂ ret = rkisp1_isp_start(isp_dev);
+ÂÂÂ if (ret)
+ÂÂÂÂÂÂÂ mipi_csi2_s_stream_stop(isp_dev->active_sensor);
+
+ÂÂÂ return ret;
+}
+
+static int rkisp1_isp_sd_s_power(struct v4l2_subdev *sd, int on)
+{
+ÂÂÂ struct rkisp1_device *isp_dev = sd_to_isp_dev(sd);
+ÂÂÂ int ret;
+
+ÂÂÂ v4l2_dbg(1, rkisp1_debug, &isp_dev->v4l2_dev, "s_power: %d\n", on);
+
+ÂÂÂ if (on) {
+ÂÂÂÂÂÂÂ ret = pm_runtime_get_sync(isp_dev->dev);
+ÂÂÂÂÂÂÂ if (ret < 0)
+ÂÂÂÂÂÂÂÂÂÂÂ return ret;
+
+ÂÂÂÂÂÂÂ rkisp1_config_clk(isp_dev);
+ÂÂÂ } else {
+ÂÂÂÂÂÂÂ ret = pm_runtime_put(isp_dev->dev);
+ÂÂÂÂÂÂÂ if (ret < 0)
+ÂÂÂÂÂÂÂÂÂÂÂ return ret;
+ÂÂÂ }
+
+ÂÂÂ return 0;
+}
+
+static int rkisp1_subdev_link_validate(struct media_link *link)
+{
+ÂÂÂ if (link->source->index == RKISP1_ISP_PAD_SINK_PARAMS)
+ÂÂÂÂÂÂÂ return 0;
+
+ÂÂÂ return v4l2_subdev_link_validate(link);
+}
+
+static int rkisp1_subdev_fmt_link_validate(struct v4l2_subdev *sd,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct media_link *link,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_subdev_format *source_fmt,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_subdev_format *sink_fmt)
+{
+ÂÂÂ if (source_fmt->format.code != sink_fmt->format.code)
+ÂÂÂÂÂÂÂ return -EINVAL;
+
+ÂÂÂ /* Crop is available */
+ÂÂÂ if (source_fmt->format.width < sink_fmt->format.width ||
+ÂÂÂÂÂÂÂ source_fmt->format.height < sink_fmt->format.height)
+ÂÂÂÂÂÂÂ return -EINVAL;
Crop should be handled on pads, not on links. The size at the ends of
the link should match. This will likely require a bit of a redesign of
the format and selection rectangles handling, but I think it's also an
opportunity to simplify the code.
+
+ÂÂÂ return 0;
+}
+
+static void rkisp1_isp_queue_event_sof(struct rkisp1_isp_subdev *isp)
+{
+ÂÂÂ struct v4l2_event event = {
+ÂÂÂÂÂÂÂ .type = V4L2_EVENT_FRAME_SYNC,
+ÂÂÂÂÂÂÂ .u.frame_sync.frame_sequence =
+ÂÂÂÂÂÂÂÂÂÂÂ atomic_inc_return(&isp->frm_sync_seq) - 1,
I would move the increment to the caller, hiding it in this function is
error-prone (and if you look at the caller I'm pointing out one possible
error :-)).
In general usage of frm_sync_seq through the driver seems to be very
race-prone. It's read in various IRQ handling functions, all coming from
the same IRQ, so that part is fine (and wouldn't require an atomic
variable), but when read from the buffer queue handlers I really get a
red light flashing in my head. I'll try to investigate more when
reviewing the next patches.
I see that the only place were 'frame_sequence' is read outside of the irq
handlers is in the capture in 'rkisp1_vb2_buf_queue':
ÂÂÂÂ/*
ÂÂÂÂÂÂÂÂ * If there's no next buffer assigned, queue this buffer directly
ÂÂÂÂÂÂÂÂ * as the next buffer, and update the memory interface.
ÂÂÂÂÂÂÂÂ */
ÂÂÂÂÂÂÂ if (cap->is_streaming && !cap->buf.next &&
ÂÂÂÂÂÂÂÂÂÂÂ atomic_read(&cap->rkisp1->isp.frame_sequence) == -1) {
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ cap->buf.next = ispbuf;
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ rkisp1_set_next_buf(cap);
ÂÂÂÂÂÂÂ } else {
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ list_add_tail(&ispbuf->queue, &cap->buf.queue);
ÂÂÂÂÂÂÂ }
This "if" condition seems very specific, a case where we already stream but v-start was not yet received.
I think it is possible to remove the test 'atomic_read(&cap->rkisp1->isp.frame_sequence) == -1'
from the above condition so that the next buffer is updated in case it is null not just before the first
v-start signal.
+ÂÂÂ };
+ÂÂÂ v4l2_event_queue(isp->sd.devnode, &event);
+}
+
+static int rkisp1_isp_sd_subs_evt(struct v4l2_subdev *sd, struct v4l2_fh *fh,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_event_subscription *sub)
+{
+ÂÂÂ if (sub->type != V4L2_EVENT_FRAME_SYNC)
+ÂÂÂÂÂÂÂ return -EINVAL;
+
+ÂÂÂ /* Line number. For now only zero accepted. */
Is the id a line number for V4L2_EVENT_FRAME_SYNC ? It's not mentioned
for V4L2_EVENT_FRAME_SYNC in the V4L2 spec, so I think this check is
correct, but the comment doesn't seem to be.
+ÂÂÂ if (sub->id != 0)
+ÂÂÂÂÂÂÂ return -EINVAL;
+
+ÂÂÂ return v4l2_event_subscribe(fh, sub, 0, NULL);
+}
+
+static const struct v4l2_subdev_pad_ops rkisp1_isp_sd_pad_ops = {
+ÂÂÂ .enum_mbus_code = rkisp1_isp_sd_enum_mbus_code,
+ÂÂÂ .get_selection = rkisp1_isp_sd_get_selection,
+ÂÂÂ .set_selection = rkisp1_isp_sd_set_selection,
+ÂÂÂ .init_cfg = rkisp1_isp_sd_init_config,
+ÂÂÂ .get_fmt = rkisp1_isp_sd_get_fmt,
+ÂÂÂ .set_fmt = rkisp1_isp_sd_set_fmt,
+ÂÂÂ .link_validate = rkisp1_subdev_fmt_link_validate,
+};
+
+static const struct media_entity_operations rkisp1_isp_sd_media_ops = {
+ÂÂÂ .link_validate = rkisp1_subdev_link_validate,
+};
+
+static const struct v4l2_subdev_video_ops rkisp1_isp_sd_video_ops = {
+ÂÂÂ .s_stream = rkisp1_isp_sd_s_stream,
+};
+
+static const struct v4l2_subdev_core_ops rkisp1_isp_core_ops = {
+ÂÂÂ .subscribe_event = rkisp1_isp_sd_subs_evt,
+ÂÂÂ .unsubscribe_event = v4l2_event_subdev_unsubscribe,
+ÂÂÂ .s_power = rkisp1_isp_sd_s_power,
+};
+
+static struct v4l2_subdev_ops rkisp1_isp_sd_ops = {
static const
+ÂÂÂ .core = &rkisp1_isp_core_ops,
+ÂÂÂ .video = &rkisp1_isp_sd_video_ops,
+ÂÂÂ .pad = &rkisp1_isp_sd_pad_ops,
+};
+
+static void rkisp1_isp_sd_init_default_fmt(struct rkisp1_isp_subdev *isp_sd)
+{
+ÂÂÂ struct v4l2_mbus_framefmt *in_frm = &isp_sd->in_frm;
+ÂÂÂ struct v4l2_rect *in_crop = &isp_sd->in_crop;
+ÂÂÂ struct v4l2_rect *out_crop = &isp_sd->out_crop;
+ÂÂÂ struct ispsd_in_fmt *in_fmt = &isp_sd->in_fmt;
+ÂÂÂ struct ispsd_out_fmt *out_fmt = &isp_sd->out_fmt;
+
+ÂÂÂ *in_fmt = rkisp1_isp_input_formats[0];
+ÂÂÂ in_frm->width = RKISP1_DEFAULT_WIDTH;
+ÂÂÂ in_frm->height = RKISP1_DEFAULT_HEIGHT;
+ÂÂÂ in_frm->code = in_fmt->mbus_code;
+
+ÂÂÂ in_crop->width = in_frm->width;
+ÂÂÂ in_crop->height = in_frm->height;
+ÂÂÂ in_crop->left = 0;
+ÂÂÂ in_crop->top = 0;
+
+ÂÂÂ /* propagate to source */
+ÂÂÂ *out_crop = *in_crop;
+ÂÂÂ *out_fmt = rkisp1_isp_output_formats[0];
+ÂÂÂ isp_sd->quantization = V4L2_QUANTIZATION_FULL_RANGE;
I wonder, could this be implemented by sharing code with .init_cfg() if
you store the active configuration in v4l2_subdev_pad_config instances
(please see the comments about the rkisp1_isp_subdev structure below) ?
+}
+
+int rkisp1_register_isp_subdev(struct rkisp1_device *isp_dev,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_device *v4l2_dev)
+{
+ÂÂÂ struct rkisp1_isp_subdev *isp_sdev = &isp_dev->isp_sdev;
+ÂÂÂ struct v4l2_subdev *sd = &isp_sdev->sd;
+ÂÂÂ int ret;
+
+ÂÂÂ v4l2_subdev_init(sd, &rkisp1_isp_sd_ops);
+ÂÂÂ sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
+ÂÂÂ sd->entity.ops = &rkisp1_isp_sd_media_ops;
+ÂÂÂ snprintf(sd->name, sizeof(sd->name), "rkisp1-isp-subdev");
+
+ÂÂÂ isp_sdev->pads[RKISP1_ISP_PAD_SINK].flags =
+ÂÂÂÂÂÂÂ MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_MUST_CONNECT;
+ÂÂÂ isp_sdev->pads[RKISP1_ISP_PAD_SINK_PARAMS].flags = MEDIA_PAD_FL_SINK;
+ÂÂÂ isp_sdev->pads[RKISP1_ISP_PAD_SOURCE_PATH].flags = MEDIA_PAD_FL_SOURCE;
+ÂÂÂ isp_sdev->pads[RKISP1_ISP_PAD_SOURCE_STATS].flags = MEDIA_PAD_FL_SOURCE;
+ÂÂÂ sd->entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
+ÂÂÂ ret = media_entity_pads_init(&sd->entity, RKISP1_ISP_PAD_MAX,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ isp_sdev->pads);
+ÂÂÂ if (ret < 0)
+ÂÂÂÂÂÂÂ return ret;
+
+ÂÂÂ sd->owner = THIS_MODULE;
+ÂÂÂ v4l2_set_subdevdata(sd, isp_dev);
+
+ÂÂÂ sd->grp_id = GRP_ID_ISP;
I think you can drop this, as well as all the GRP_ID_* macros, they're
not used.
+ÂÂÂ ret = v4l2_device_register_subdev(v4l2_dev, sd);
+ÂÂÂ if (ret < 0) {
+ÂÂÂÂÂÂÂ v4l2_err(sd, "Failed to register isp subdev\n");
+ÂÂÂÂÂÂÂ goto err_cleanup_media_entity;
+ÂÂÂ }
+
+ÂÂÂ rkisp1_isp_sd_init_default_fmt(isp_sdev);
+
+ÂÂÂ return 0;
+err_cleanup_media_entity:
+ÂÂÂ media_entity_cleanup(&sd->entity);
+ÂÂÂ return ret;
+}
+
+void rkisp1_unregister_isp_subdev(struct rkisp1_device *isp_dev)
+{
+ÂÂÂ struct v4l2_subdev *sd = &isp_dev->isp_sdev.sd;
+
+ÂÂÂ v4l2_device_unregister_subdev(sd);
+ÂÂÂ media_entity_cleanup(&sd->entity);
+}
+
+/****************Â Interrupter Handler ****************/
s/Handler/Handlers/
+
+void rkisp1_mipi_isr(unsigned int mis, struct rkisp1_device *dev)
I would make the mis field an u32 as it contains register values. Should
it also be renamed to status ? Same for rkisp1_isp_isr() below.
+{
+ÂÂÂ struct v4l2_device *v4l2_dev = &dev->v4l2_dev;
+ÂÂÂ void __iomem *base = dev->base_addr;
+ÂÂÂ u32 val;
+
How about moving the CIF_MIPI_MIS read here and removing the mis
argument to the function ? It would be more logical to group all
register access related to interrupts in a single place. Same for the
other interrupt handler functions.
+ÂÂÂ writel(~0, base + CIF_MIPI_ICR);
+
+ÂÂÂ /*
+ÂÂÂÂ * Disable DPHY errctrl interrupt, because this dphy
+ÂÂÂÂ * erctrl signal is asserted until the next changes
+ÂÂÂÂ * of line state. This time is may be too long and cpu
+ÂÂÂÂ * is hold in this interrupt.
+ÂÂÂÂ */
+ÂÂÂ if (mis & CIF_MIPI_ERR_CTRL(0x0f)) {
+ÂÂÂÂÂÂÂ val = readl(base + CIF_MIPI_IMSC);
+ÂÂÂÂÂÂÂ writel(val & ~CIF_MIPI_ERR_CTRL(0x0f), base + CIF_MIPI_IMSC);
+ÂÂÂÂÂÂÂ dev->isp_sdev.dphy_errctrl_disabled = true;
+ÂÂÂ }
+
+ÂÂÂ /*
+ÂÂÂÂ * Enable DPHY errctrl interrupt again, if mipi have receive
+ÂÂÂÂ * the whole frame without any error.
+ÂÂÂÂ */
+ÂÂÂ if (mis == CIF_MIPI_FRAME_END) {
+ÂÂÂÂÂÂÂ /*
+ÂÂÂÂÂÂÂÂ * Enable DPHY errctrl interrupt again, if mipi have receive
+ÂÂÂÂÂÂÂÂ * the whole frame without any error.
+ÂÂÂÂÂÂÂÂ */
+ÂÂÂÂÂÂÂ if (dev->isp_sdev.dphy_errctrl_disabled) {
+ÂÂÂÂÂÂÂÂÂÂÂ val = readl(base + CIF_MIPI_IMSC);
+ÂÂÂÂÂÂÂÂÂÂÂ val |= CIF_MIPI_ERR_CTRL(0x0f);
+ÂÂÂÂÂÂÂÂÂÂÂ writel(val, base + CIF_MIPI_IMSC);
+ÂÂÂÂÂÂÂÂÂÂÂ dev->isp_sdev.dphy_errctrl_disabled = false;
+ÂÂÂÂÂÂÂ }
+ÂÂÂ } else {
+ÂÂÂÂÂÂÂ v4l2_warn(v4l2_dev, "MIPI mis error: 0x%08x\n", mis);
+ÂÂÂ }
+}
+
+void rkisp1_isp_isr(unsigned int isp_mis, struct rkisp1_device *dev)
+{
+ÂÂÂ void __iomem *base = dev->base_addr;
+ÂÂÂ unsigned int isp_mis_tmp = 0;
_tmp are never good names :-S
+ÂÂÂ unsigned int isp_err = 0;
Neither of these variable need to be initialised to 0.
+
+ÂÂÂ /* start edge of v_sync */
+ÂÂÂ if (isp_mis & CIF_ISP_V_START) {
+ÂÂÂÂÂÂÂ rkisp1_isp_queue_event_sof(&dev->isp_sdev);
This will increment the frame sequence number. What if the interrupt is
slightly delayed and the next frame starts before we get a change to
copy the sequence number to the buffers (before they will complete
below) ?
Do you mean that we get two sequental v-start signals and then the next
frame-end signal in MI_MIS belongs to the first v-start signal of the two?
How can this be solved? I wonder if any v-start signal has a later signal
that correspond to the same frame so that we can follow it?
Maybe we should have one counter that is incremented on v-start signal,
and another counter that is incremented uppon some other signal?
+
+ÂÂÂÂÂÂÂ writel(CIF_ISP_V_START, base + CIF_ISP_ICR);
Do you need to clear all interrupt bits individually, can't you write
isp_mis to CIF_ISP_ICR at the beginning of the function to clear them
all in one go ?
+ÂÂÂÂÂÂÂ isp_mis_tmp = readl(base + CIF_ISP_MIS);
+ÂÂÂÂÂÂÂ if (isp_mis_tmp & CIF_ISP_V_START)
+ÂÂÂÂÂÂÂÂÂÂÂ v4l2_err(&dev->v4l2_dev, "isp icr v_statr err: 0x%x\n",
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ isp_mis_tmp);
This require some explanation. It looks like a naive way to protect
against something, but I think it could trigger under normal
circumstances if IRQ handling is delayed, and wouldn't do much anyway.
Same for the similar constructs below.
+ÂÂÂ }
+
+ÂÂÂ if ((isp_mis & CIF_ISP_PIC_SIZE_ERROR)) {
+ÂÂÂÂÂÂÂ /* Clear pic_size_error */
+ÂÂÂÂÂÂÂ writel(CIF_ISP_PIC_SIZE_ERROR, base + CIF_ISP_ICR);
+ÂÂÂÂÂÂÂ isp_err = readl(base + CIF_ISP_ERR);
+ÂÂÂÂÂÂÂ v4l2_err(&dev->v4l2_dev,
+ÂÂÂÂÂÂÂÂÂÂÂÂ "CIF_ISP_PIC_SIZE_ERROR (0x%08x)", isp_err);
What does this mean ?
+ÂÂÂÂÂÂÂ writel(isp_err, base + CIF_ISP_ERR_CLR);
+ÂÂÂ } else if ((isp_mis & CIF_ISP_DATA_LOSS)) {
Are CIF_ISP_PIC_SIZE_ERROR and CIF_ISP_DATA_LOSS mutually exclusive ?
+ÂÂÂÂÂÂÂ /* Clear data_loss */
+ÂÂÂÂÂÂÂ writel(CIF_ISP_DATA_LOSS, base + CIF_ISP_ICR);
+ÂÂÂÂÂÂÂ v4l2_err(&dev->v4l2_dev, "CIF_ISP_DATA_LOSS\n");
+ÂÂÂÂÂÂÂ writel(CIF_ISP_DATA_LOSS, base + CIF_ISP_ICR);
+ÂÂÂ }
+
+ÂÂÂ /* sampled input frame is complete */
+ÂÂÂ if (isp_mis & CIF_ISP_FRAME_IN) {
+ÂÂÂÂÂÂÂ writel(CIF_ISP_FRAME_IN, base + CIF_ISP_ICR);
+ÂÂÂÂÂÂÂ isp_mis_tmp = readl(base + CIF_ISP_MIS);
+ÂÂÂÂÂÂÂ if (isp_mis_tmp & CIF_ISP_FRAME_IN)
+ÂÂÂÂÂÂÂÂÂÂÂ v4l2_err(&dev->v4l2_dev, "isp icr frame_in err: 0x%x\n",
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ isp_mis_tmp);
+ÂÂÂ }
+
+ÂÂÂ /* frame was completely put out */
"put out" ? :-) What's the difference between ISP_FRAME_IN and ISP_FRAME
? The two comments could do with a bit of brush up, and I think the
ISP_FRAME_IN interrupt could be disabled as it doesn't perform any
action.
Those two oneline comments are just copy-paste from the datasheet.
""
5 MIS_FRAME_IN sampled input frame is complete
1 MIS_FRAME frame was completely put out
""
Unfrotunately, the datasheet does not add any further explanation about those signals.
+ÂÂÂ if (isp_mis & CIF_ISP_FRAME) {
+ÂÂÂÂÂÂÂ u32 isp_ris = 0;
No need to initialise this to 0.
+ÂÂÂÂÂÂÂ /* Clear Frame In (ISP) */
+ÂÂÂÂÂÂÂ writel(CIF_ISP_FRAME, base + CIF_ISP_ICR);
+ÂÂÂÂÂÂÂ isp_mis_tmp = readl(base + CIF_ISP_MIS);
+ÂÂÂÂÂÂÂ if (isp_mis_tmp & CIF_ISP_FRAME)
+ÂÂÂÂÂÂÂÂÂÂÂ v4l2_err(&dev->v4l2_dev,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "isp icr frame end err: 0x%x\n", isp_mis_tmp);
+
+ÂÂÂÂÂÂÂ isp_ris = readl(base + CIF_ISP_RIS);
+ÂÂÂÂÂÂÂ if (isp_ris & (CIF_ISP_AWB_DONE | CIF_ISP_AFM_FIN |
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ CIF_ISP_EXP_END | CIF_ISP_HIST_MEASURE_RDY))
+ÂÂÂÂÂÂÂÂÂÂÂ rkisp1_stats_isr(&dev->stats_vdev, isp_ris);
Is there a guarantee that the statistics will be fully written out
before the video frame itself ? And doesn't this test if any of the
statistics is complete, not all of them ? I think the logic is wrong, it
The datasheet does not add any explanation of what is expected to come first.
Should we wait until all statistics measurements are done? In the struct
sent to userspace there is a bitmaks for which of the statistics are read.
I think that if only part of the statistics are ready, we can already send the once
that are ready to userspace.
seems it should be moved out of the CIF_ISP_FRAME test, to a test of its
own. It's hard to tell for sure without extra information though (for
instance why are the stats-related bits read from CIF_ISP_RIS, when
they seem to be documented as valid in CIF_ISP_ISR), but this should be
validated, and most probably fixed. Care should be taken to keep
synchronisation of sequence number between the different queues.
I see that the capture buffers are done before incrementing the frame_sequence with
the following explanation:
ÂÂÂÂ/*
ÂÂÂÂÂÂÂÂ * Call rkisp1_capture_isr() first to handle the frame that
ÂÂÂÂÂÂÂÂ * potentially completed using the current frame_sequence number before
ÂÂÂÂÂÂÂÂ * it is potentially incremented by rkisp1_isp_isr() in the vertical
ÂÂÂÂÂÂÂÂ * sync.
ÂÂÂÂÂÂÂÂ */
I think reading the stats/params should also be done before calling rkisp1_capture_isr
for the same reason. (so to match the correct frame_sequence)
Thanks,
Dafna
I am not sure what should be the meaning of the sequence field for parameters buffers.
Currently the rkisp1-params entity returns the first buffer queued right away with the frame_sequence that it reads.
If streaming did not yet started then the first buffer is used as the initial configuration that overrides the driver's default.
In that case the first params buffer is not associated with any captured frame and has a sequence value '-1'
Is this a valid behavior?
Thanks,
Dafna
+ÂÂÂ }
+
+ÂÂÂ /*
+ÂÂÂÂ * Then update changed configs. Some of them involve
+ÂÂÂÂ * lot of register writes. Do those only one per frame.
+ÂÂÂÂ * Do the updates in the order of the processing flow.
+ÂÂÂÂ */
+ÂÂÂ rkisp1_params_isr(&dev->params_vdev, isp_mis);
You should pass dev to this function, and use it internally for the I/O
accessors instead of going through the vdev.
+}
diff --git a/drivers/media/platform/rockchip/isp1/rkisp1.h b/drivers/media/platform/rockchip/isp1/rkisp1.h
new file mode 100644
index 000000000000..b0366e354ec2
--- /dev/null
+++ b/drivers/media/platform/rockchip/isp1/rkisp1.h
@@ -0,0 +1,111 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * Rockchip isp1 driver
+ *
+ * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
+ */
+
+#ifndef _RKISP1_H
+#define _RKISP1_H
+
+#include <linux/platform_device.h>
+#include <media/v4l2-fwnode.h>
I don't think you need these headers. You however need
#include <media/v4l2-ctrls.h>
#include <media/v4l2-subdev.h>
to make this header self-contained. Other headers could be added too
(videodev2.h, v4l2-mediabus.h and media-entity.h), they're included
through the above two headers at the moment.
+
+#include "common.h"
+
+struct rkisp1_stream;
+
+/*
+ * struct ispsd_in_fmt - ISP intput-pad format
+ *
+ * Translate mbus_code to hardware format values
+ *
+ * @bus_width: used for parallel
+ */
+struct ispsd_in_fmt {
+ÂÂÂ u32 mbus_code;
+ÂÂÂ u8 fmt_type;
+ÂÂÂ u32 mipi_dt;
+ÂÂÂ u32 yuv_seq;
+ÂÂÂ enum rkisp1_fmt_raw_pat_type bayer_pat;
+ÂÂÂ u8 bus_width;
+};
Some of the structures are prefixed with ispsd_, some with rkisp1_. I
think we should standardise on a single prefix.
+
+struct ispsd_out_fmt {
+ÂÂÂ u32 mbus_code;
+ÂÂÂ u8 fmt_type;
+};
+
+struct rkisp1_ie_config {
+ÂÂÂ unsigned int effect;
+};
This structure isn't used.
+
+enum rkisp1_isp_pad {
+ÂÂÂ RKISP1_ISP_PAD_SINK,
Should this be named SINK_VIDEO to match SINK_PARAMS ?
+ÂÂÂ RKISP1_ISP_PAD_SINK_PARAMS,
+ÂÂÂ RKISP1_ISP_PAD_SOURCE_PATH,
And SOURCE_VIDEO ?
+ÂÂÂ RKISP1_ISP_PAD_SOURCE_STATS,
+ÂÂÂ RKISP1_ISP_PAD_MAX
+};
+
+/*
+ * struct rkisp1_isp_subdev - ISP sub-device
+ *
+ * See Cropping regions of ISP in rkisp1.c for details
Could you please document all fields from the structure ?
+ * @in_frm: input size, don't have to be equal to sensor size
+ * @in_fmt: input format
+ * @in_crop: crop for sink pad
+ * @out_fmt: output format
+ * @out_crop: output size
+ *
+ * @dphy_errctrl_disabled: if dphy errctrl is disabled(avoid endless interrupt)
s/disabled/disabled /
+ * @frm_sync_seq: frame sequence, to sync frame_id between video devices.
+ * @quantization: output quantization
+ */
+struct rkisp1_isp_subdev {
+ÂÂÂ struct v4l2_subdev sd;
+ÂÂÂ struct media_pad pads[RKISP1_ISP_PAD_MAX];
+ÂÂÂ struct v4l2_ctrl_handler ctrl_handler;
+ÂÂÂ struct v4l2_mbus_framefmt in_frm;
+ÂÂÂ struct ispsd_in_fmt in_fmt;
I think this (and out_fmt) could be stored as pointers.
+ÂÂÂ struct v4l2_rect in_crop;
+ÂÂÂ struct ispsd_out_fmt out_fmt;
+ÂÂÂ struct v4l2_rect out_crop;
I think this could be greatly simplified if you stored all the data as
v4l2_subdev_pad_config instances for both the sink and source video pads
instead of using ad-hoc structures. In particular your get and set
format handlers would become much simpler. I recommend looking at the
mt9p031 sensor driver for an example. In a nutshell, the get handler
would retrieve the format from the v4l2_subdev_pad_config instance
either from rkisp1_isp_subdev (for V4L2_SUBDEV_FORMAT_ACTIVE) or from
the passed cfg argument (for V4L2_SUBDEV_FORMAT_TRY). The set handler
would operate similarly, with just a test one ACTIVE/TRY at the
beginning, followed by common code that only operates on the cfg, and
finally storing the format in rkisp1_isp_subdev for
V4L2_SUBDEV_FORMAT_ACTIVE.
+ÂÂÂ bool dphy_errctrl_disabled;
+ÂÂÂ atomic_t frm_sync_seq;
+ÂÂÂ enum v4l2_quantization quantization;
The quantization would then be stored in the v4l2_subdev_pad_config too.
+};
+
+int rkisp1_register_isp_subdev(struct rkisp1_device *isp_dev,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct v4l2_device *v4l2_dev);
+
+void rkisp1_unregister_isp_subdev(struct rkisp1_device *isp_dev);
+
+void rkisp1_mipi_isr(unsigned int mipi_mis, struct rkisp1_device *dev);
+
+void rkisp1_isp_isr(unsigned int isp_mis, struct rkisp1_device *dev);
+
+static inline
+struct ispsd_out_fmt *rkisp1_get_ispsd_out_fmt(struct rkisp1_isp_subdev *isp_sdev)
+{
+ÂÂÂ return &isp_sdev->out_fmt;
+}
+
+static inline
+struct ispsd_in_fmt *rkisp1_get_ispsd_in_fmt(struct rkisp1_isp_subdev *isp_sdev)
+{
+ÂÂÂ return &isp_sdev->in_fmt;
+}
+
+static inline
+struct v4l2_rect *rkisp1_get_isp_sd_win(struct rkisp1_isp_subdev *isp_sdev)
+{
+ÂÂÂ return &isp_sdev->out_crop;
+}
I agree with Sakari that accessing fields directly would be more
readable.
+
+static inline struct rkisp1_isp_subdev *sd_to_isp_sd(struct v4l2_subdev *sd)
+{
+ÂÂÂ return container_of(sd, struct rkisp1_isp_subdev, sd);
+}
+
+#endif /* _RKISP1_H */