Re: [PATCH v7 5/6] media: starfive: camss: Add ISP driver

Hi Jack,

On Wed, Aug 02, 2023 at 05:57:57PM +0800, Jack Zhu wrote:

On 2023/7/28 4:41, Laurent Pinchart wrote:

quoted

On Mon, Jun 19, 2023 at 07:28:37PM +0800, Jack Zhu wrote:

quoted

Add ISP driver for StarFive Camera Subsystem.

Signed-off-by: Jack Zhu <jack.zhu@starfivetech.com>
---
 .../media/platform/starfive/camss/Makefile    |   2 +
 .../media/platform/starfive/camss/stf_camss.c |  76 ++-
 .../media/platform/starfive/camss/stf_camss.h |   3 +
 .../media/platform/starfive/camss/stf_isp.c   | 519 ++++++++++++++++++
 .../media/platform/starfive/camss/stf_isp.h   | 479 ++++++++++++++++
 .../platform/starfive/camss/stf_isp_hw_ops.c  | 468 ++++++++++++++++
 6 files changed, 1544 insertions(+), 3 deletions(-)
 create mode 100644 drivers/media/platform/starfive/camss/stf_isp.c
 create mode 100644 drivers/media/platform/starfive/camss/stf_isp.h
 create mode 100644 drivers/media/platform/starfive/camss/stf_isp_hw_ops.c

[snip]

quoted

quoted

diff --git a/drivers/media/platform/starfive/camss/stf_isp.c b/drivers/media/platform/starfive/camss/stf_isp.c
new file mode 100644
index 000000000000..933a583b398c
--- /dev/null
+++ b/drivers/media/platform/starfive/camss/stf_isp.c

@@ -0,0 +1,519 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * stf_isp.c
+ *
+ * StarFive Camera Subsystem - ISP Module
+ *
+ * Copyright (C) 2021-2023 StarFive Technology Co., Ltd.
+ */
+#include <linux/firmware.h>

This doesn't seem needed.

quoted

+#include <media/v4l2-event.h>
+
+#include "stf_camss.h"
+
+#define SINK_FORMATS_INDEX    0
+#define UO_FORMATS_INDEX      1

What does "UO" stand for ?

"UO" is Usual Out, just represents output. :-)

Maybe "out", "output" or "source" would make the code easier to read
then ?

quoted

quoted

+
+static int isp_set_selection(struct v4l2_subdev *sd,
+			     struct v4l2_subdev_state *state,
+			     struct v4l2_subdev_selection *sel);
+
+static const struct isp_format isp_formats_sink[] = {
+	{ MEDIA_BUS_FMT_SRGGB10_1X10, 10 },
+	{ MEDIA_BUS_FMT_SGRBG10_1X10, 10 },
+	{ MEDIA_BUS_FMT_SGBRG10_1X10, 10 },
+	{ MEDIA_BUS_FMT_SBGGR10_1X10, 10 },
+};

[snip]

quoted

quoted

diff --git a/drivers/media/platform/starfive/camss/stf_isp.h b/drivers/media/platform/starfive/camss/stf_isp.h
new file mode 100644
index 000000000000..1e5c98482350
--- /dev/null
+++ b/drivers/media/platform/starfive/camss/stf_isp.h

@@ -0,0 +1,479 @@

[snip]

quoted

quoted

+/* The output line of ISP */

What is an ISP "line" ?

A pipeline contains ISP.

Patch 6/6 uses STF_ISP_LINE_MAX to iterate over the ISP lines. This
makes the code somehow generic, but you only support a single line at
the moment. Does this or other SoCs in your product line integrate the
same ISP with multiple lines ? If so, would it be possible to share a
block diagram, to better understand the other hardware architectures
that this driver will need to support in the future ?

quoted

quoted

+enum isp_line_id {
+	STF_ISP_LINE_INVALID = -1,
+	STF_ISP_LINE_SRC = 1,
+	STF_ISP_LINE_MAX = STF_ISP_LINE_SRC
+};

[snip]

quoted

quoted

+void stf_isp_init_cfg(struct stf_isp_dev *isp_dev)
+{
+	stf_isp_reg_write(isp_dev->stfcamss, ISP_REG_DC_CFG_1, DC_AXI_ID(0x0));
+	stf_isp_reg_write(isp_dev->stfcamss, ISP_REG_DEC_CFG,
+			  DEC_V_KEEP(0x0) |
+			  DEC_V_PERIOD(0x0) |
+			  DEC_H_KEEP(0x0) |
+			  DEC_H_PERIOD(0x0));
+
+	stf_isp_config_obc(isp_dev->stfcamss);
+	stf_isp_config_oecf(isp_dev->stfcamss);
+	stf_isp_config_lccf(isp_dev->stfcamss);
+	stf_isp_config_awb(isp_dev->stfcamss);
+	stf_isp_config_grgb(isp_dev->stfcamss);
+	stf_isp_config_cfa(isp_dev->stfcamss);
+	stf_isp_config_ccm(isp_dev->stfcamss);
+	stf_isp_config_gamma(isp_dev->stfcamss);
+	stf_isp_config_r2y(isp_dev->stfcamss);
+	stf_isp_config_y_curve(isp_dev->stfcamss);
+	stf_isp_config_sharpen(isp_dev->stfcamss);
+	stf_isp_config_dnyuv(isp_dev->stfcamss);
+	stf_isp_config_sat(isp_dev->stfcamss);

All these parameters are hardcoded, why are they not exposed to
userspace ?

Here is a basic startup configuration for the ISP registers. The
function name is confusing, as if it is configuring a specific
function. In fact, it is just a basic init configuration.

Did I miss a place in the patch series where all these parameters can be
configured by userspace, or is that not possible at the moment ? If it
isn't possible, do you plan to implement that ?

quoted

quoted

+
+	stf_isp_reg_write(isp_dev->stfcamss, ISP_REG_CSI_MODULE_CFG,
+			  CSI_DUMP_EN | CSI_SC_EN | CSI_AWB_EN |
+			  CSI_LCCF_EN | CSI_OECF_EN | CSI_OBC_EN | CSI_DEC_EN);
+	stf_isp_reg_write(isp_dev->stfcamss, ISP_REG_ISP_CTRL_1,
+			  CTRL_SAT(1) | CTRL_DBC | CTRL_CTC | CTRL_YHIST |
+			  CTRL_YCURVE | CTRL_BIYUV | CTRL_SCE | CTRL_EE |
+			  CTRL_CCE | CTRL_RGE | CTRL_CME | CTRL_AE | CTRL_CE);
+}

[snip]

-- 
Regards,

Laurent Pinchart