| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<ZQ0PR01MB130232FA373BB3C284D876A9F2A62@ZQ0PR01MB1302.CHNPR01.prod.partner.outlook.cn>
Date: Fri, 12 Jul 2024 13:14:54 +0000
From: Changhuang Liang <changhuang.liang@...rfivetech.com>
To: Jacopo Mondi <jacopo.mondi@...asonboard.com>
CC: Mauro Carvalho Chehab <mchehab@...nel.org>, Greg Kroah-Hartman
<gregkh@...uxfoundation.org>, Sakari Ailus <sakari.ailus@...ux.intel.com>,
Hans Verkuil <hverkuil-cisco@...all.nl>, Laurent Pinchart
<laurent.pinchart@...asonboard.com>, Jean-Michel Hautbois
<jeanmichel.hautbois@...asonboard.com>, Benjamin Gaignard
<benjamin.gaignard@...labora.com>, Tomi Valkeinen
<tomi.valkeinen+renesas@...asonboard.com>, Mingjia Zhang
<mingjia.zhang@...iatek.com>, Jack Zhu <jack.zhu@...rfivetech.com>, Keith
Zhao <keith.zhao@...rfivetech.com>, "linux-media@...r.kernel.org"
<linux-media@...r.kernel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "linux-staging@...ts.linux.dev"
<linux-staging@...ts.linux.dev>
Subject:
回复: [PATCH v5 12/14] staging: media: starfive: Add ISP parameters hardware configure
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@...rfivetech.com>
> > ---
> > .../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, ¶ms->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, ¶ms->car_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_CCM_SETTING)
> > + isp_set_ctrl_ccm(stfcamss, ¶ms->ccm_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_CFA_SETTING)
> > + isp_set_ctrl_cfa(stfcamss, ¶ms->cfa_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_CTC_SETTING)
> > + isp_set_ctrl_ctc(stfcamss, ¶ms->ctc_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_DBC_SETTING)
> > + isp_set_ctrl_dbc(stfcamss, ¶ms->dbc_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_DNYUV_SETTING)
> > + isp_set_ctrl_dnyuv(stfcamss, ¶ms->dnyuv_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_GMARGB_SETTING)
> > + isp_set_ctrl_gmargb(stfcamss, ¶ms->gmargb_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_LCCF_SETTING)
> > + isp_set_ctrl_lccf(stfcamss, ¶ms->lccf_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_OBC_SETTING)
> > + isp_set_ctrl_obc(stfcamss, ¶ms->obc_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_OECF_SETTING)
> > + isp_set_ctrl_oecf(stfcamss, ¶ms->oecf_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_R2Y_SETTING)
> > + isp_set_ctrl_r2y(stfcamss, ¶ms->r2y_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_SAT_SETTING)
> > + isp_set_ctrl_sat(stfcamss, ¶ms->sat_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_SHARP_SETTING)
> > + isp_set_ctrl_sharp(stfcamss, ¶ms->sharp_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_YCRV_SETTING)
> > + isp_set_ctrl_ycrv(stfcamss, ¶ms->ycrv_setting);
> > +
> > + if (params->enable_setting & JH7110_ISP_MODULE_SC_SETTING)
> > + isp_set_ctrl_sc(stfcamss, ¶ms->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
Powered by blists - more mailing lists