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

[Jack Zhu]

Hi Laurent,

On 2023/8/2 18:48, Laurent Pinchart wrote:
> 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@xxxxxxxxxxxxxxxx>
>> >> ---
>> >>  .../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.

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

Jack Zhu