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

Hi Jack,

On Thu, Aug 03, 2023 at 10:41:08AM +0800, Jack Zhu wrote:

On 2023/8/2 18:48, Laurent Pinchart wrote:

quoted

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

quoted

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

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

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

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

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 ?

Yes, OK, I will add a block diagram and a more detailed description in
the starfive_camss.rst file in the next version.

quoted

quoted

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

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 ?

Yes, we are doing related development internally.

That's nice to hear :-) Without the ability to control the ISP from
userspace, the driver will have very limited usefulness. Still, I
understand that incremental development will be easier to handle, so I'm
not against merging this patch series with hardcoded parameters first,
and adding support for ISP control on top. It may however make sense to
merge the driver in drivers/staging/media/ first, and move it to
drivers/media/ once support for ISP control will be available. That
would give you more room to change the userspace API exposed by the
driver when implementing support for ISP control without having to keep
backward compatibility. Sakari, Hans, do you have any opinion on this ?

Do you have an estimated time frame for when ISP control will be ready ?

quoted

quoted

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