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