回复: [PATCH v5 12/14] staging: media: starfive: Add ISP parameters hardware configure

From: Changhuang Liang
Date: Fri Jul 12 2024 - 09:15:33 EST

Hi Jacopo

Thanks for your comments.

> Hi Changhuang
>
> On Tue, Jul 09, 2024 at 01:38:22AM GMT, Changhuang Liang wrote:
> > Add ISP parameters hardware configure.
> >
> > Signed-off-by: Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> > ---
> > .../staging/media/starfive/camss/stf-camss.h | 8 +
> > .../media/starfive/camss/stf-isp-hw-ops.c | 573
> ++++++++++++++++++
> > .../staging/media/starfive/camss/stf-isp.h | 135 +++++
> > 3 files changed, 716 insertions(+)
> >
> > diff --git a/drivers/staging/media/starfive/camss/stf-camss.h
> > b/drivers/staging/media/starfive/camss/stf-camss.h
> > index 3f84f1a1e997..328318d61c6b 100644
> > --- a/drivers/staging/media/starfive/camss/stf-camss.h
> > +++ b/drivers/staging/media/starfive/camss/stf-camss.h
> > @@ -106,6 +106,14 @@ static inline void stf_isp_reg_set(struct stfcamss
> *stfcamss, u32 reg, u32 mask)
> > stfcamss->isp_base + reg);
> > }
> >
> > +static inline void stf_isp_reg_fill_zero(struct stfcamss *stfcamss,
> > +u32 reg, u32 size) {
> > + u32 i;
> > +
> > + for (i = 0; i < size; i++, reg += 4)
> > + iowrite32(0, stfcamss->isp_base + reg); }
> > +
>
> Why in the header file ? Also could memset_io() replace this ?
>

OK, I will try this.

> > static inline u32 stf_syscon_reg_read(struct stfcamss *stfcamss, u32
> > reg) {
> > return ioread32(stfcamss->syscon_base + reg); diff --git
> > a/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
> > b/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
> > index 0bc5e36f952e..e54368910906 100644
> > --- a/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
> > +++ b/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
> > @@ -10,6 +10,25 @@
> >
> > #include "stf-camss.h"
> >
> > +static const struct stf_isp_module_info mod_info[] = {
> > + { ISP_REG_CSI_MODULE_CFG, 2 },
> > + { ISP_REG_CSI_MODULE_CFG, 4 },
> > + { ISP_REG_CSI_MODULE_CFG, 6 },
> > + { ISP_REG_CSI_MODULE_CFG, 7 },
> > + { ISP_REG_CSI_MODULE_CFG, 17 },
> > + { ISP_REG_ISP_CTRL_1, 1 },
> > + { ISP_REG_ISP_CTRL_1, 2 },
> > + { ISP_REG_ISP_CTRL_1, 3 },
> > + { ISP_REG_ISP_CTRL_1, 4 },
> > + { ISP_REG_ISP_CTRL_1, 5 },
> > + { ISP_REG_ISP_CTRL_1, 7 },
> > + { ISP_REG_ISP_CTRL_1, 8 },
> > + { ISP_REG_ISP_CTRL_1, 17 },
> > + { ISP_REG_ISP_CTRL_1, 19 },
> > + { ISP_REG_ISP_CTRL_1, 21 },
> > + { ISP_REG_ISP_CTRL_1, 22 },
> > +};
> > +
> > static void stf_isp_config_obc(struct stfcamss *stfcamss) {
> > u32 reg_val, reg_add;
> > @@ -517,6 +536,59 @@ static void stf_isp_fill_flag(struct stfcamss
> *stfcamss, void *vaddr,
> > }
> > }
> >
> > +static void stf_isp_set_params(struct stfcamss *stfcamss, void
> > +*vaddr) {
>
> Shouldn't all these operations be in the -params.c file ?
>
> > + struct jh7110_isp_params_buffer *params = (struct
> > +jh7110_isp_params_buffer *)vaddr;
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_WB_SETTING)
> > + isp_set_ctrl_wb(stfcamss, &params->wb_setting);
>
> Also this function is exported in stf-isp.h but are only called from within this
> file ? Same for all the other ones I guess ?
>

Yes, but we have a file stf-isp-hw-ops.c which store the ISP hardware operations.
Should these operations move into -params.c?

> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_CAR_SETTING)
> > + isp_set_ctrl_car(stfcamss, &params->car_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_CCM_SETTING)
> > + isp_set_ctrl_ccm(stfcamss, &params->ccm_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_CFA_SETTING)
> > + isp_set_ctrl_cfa(stfcamss, &params->cfa_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_CTC_SETTING)
> > + isp_set_ctrl_ctc(stfcamss, &params->ctc_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_DBC_SETTING)
> > + isp_set_ctrl_dbc(stfcamss, &params->dbc_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_DNYUV_SETTING)
> > + isp_set_ctrl_dnyuv(stfcamss, &params->dnyuv_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_GMARGB_SETTING)
> > + isp_set_ctrl_gmargb(stfcamss, &params->gmargb_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_LCCF_SETTING)
> > + isp_set_ctrl_lccf(stfcamss, &params->lccf_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_OBC_SETTING)
> > + isp_set_ctrl_obc(stfcamss, &params->obc_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_OECF_SETTING)
> > + isp_set_ctrl_oecf(stfcamss, &params->oecf_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_R2Y_SETTING)
> > + isp_set_ctrl_r2y(stfcamss, &params->r2y_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_SAT_SETTING)
> > + isp_set_ctrl_sat(stfcamss, &params->sat_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_SHARP_SETTING)
> > + isp_set_ctrl_sharp(stfcamss, &params->sharp_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_YCRV_SETTING)
> > + isp_set_ctrl_ycrv(stfcamss, &params->ycrv_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_SC_SETTING)
> > + isp_set_ctrl_sc(stfcamss, &params->sc_setting); }
> > +
> > irqreturn_t stf_line_irq_handler(int irq, void *priv) {
> > struct stfcamss *stfcamss = priv;
> > @@ -566,11 +638,20 @@ irqreturn_t stf_isp_irq_handler(int irq, void *priv)
> > struct stfcamss *stfcamss = priv;
> > struct stf_capture *cap = &stfcamss->captures[STF_CAPTURE_YUV];
> > struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
> > + struct stf_output *output = &stfcamss->output;
> > struct stfcamss_buffer *ready_buf;
> > u32 status;
> >
> > status = stf_isp_reg_read(stfcamss, ISP_REG_ISP_CTRL_0);
> > if (status & ISPC_ISP) {
> > + ready_buf = stf_buf_get_ready(&output->buffers);
> > + if (ready_buf) {
> > + stf_isp_set_params(stfcamss, ready_buf->vaddr);
> > + ready_buf->vb.vb2_buf.timestamp = ktime_get_ns();
> > + ready_buf->vb.sequence = output->buffers.sequence++;
> > + vb2_buffer_done(&ready_buf->vb.vb2_buf,
> VB2_BUF_STATE_DONE);
> > + }
> > +
> > if (status & ISPC_ENUO) {
> > ready_buf = stf_buf_done(&cap->buffers);
> > if (ready_buf)
> > @@ -591,4 +672,496 @@ irqreturn_t stf_isp_irq_handler(int irq, void *priv)
> > }
> >
> > return IRQ_HANDLED;
> > +};
> > +
> > +int isp_set_ctrl_wb(struct stfcamss *stfcamss, const void *value)
>
> This function is generic, there's no need to pass a const void * here and cast it
> below, you can use const struct jh7110_isp_wb_setting * as the second
> argument type.
>

OK

Regards,
Changhuang