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

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

Hi Jack,

On Thu, Aug 03, 2023 at 10:41:08AM +0800, Jack Zhu wrote:
> On 2023/8/2 18:48, Laurent Pinchart wrote:
> > 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 ?
> 
> Yes, OK, I will add a block diagram and a more detailed description in
> the starfive_camss.rst file in the next version.
> 
> >> >> +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 ?
> 
> 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 ?

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