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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jack,

On Wed, Aug 02, 2023 at 05:57:57PM +0800, Jack Zhu wrote:
> On 2023/7/28 4:41, Laurent Pinchart wrote:
> > On Mon, Jun 19, 2023 at 07:28:37PM +0800, Jack Zhu wrote:
> >> 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]

> >> 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.
> > 
> >> +#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 ?

> >> +
> >> +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]

> >> 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]

> >> +/* 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 ?

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

[snip]

> >> +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 ?

> >> +
> >> +	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