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
