| 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: <0e5f029b-ee63-4e44-a75e-877fbae042b3@quicinc.com>
Date: Thu, 11 Jul 2024 19:54:07 +0800
From: Depeng Shao <quic_depengs@...cinc.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>, <rfoss@...nel.org>,
<todor.too@...il.com>, <mchehab@...nel.org>, <robh@...nel.org>,
<krzk+dt@...nel.org>, <conor+dt@...nel.org>
CC: <quic_eberman@...cinc.com>, <linux-media@...r.kernel.org>,
<linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <kernel@...cinc.com>
Subject: Re: [PATCH 11/13] media: qcom: camss: Add notify interface in camss
driver
Hi Bryan,
On 7/10/2024 7:54 PM, Bryan O'Donoghue wrote:
> On 09/07/2024 17:06, Depeng Shao wrote:
>> The main v4l2 process logic in camss vfe subdev driver, so the vfe driver
>> handles the buf done irq and register update configuration. But the buf
>> done irq and register update configuration have been moved CSID HW in
>> Titan 780 and other new platform, so vfe driver needs to call into CSID
>> driver to configure the register update. And CSID driver also needs to
>> call into vfe driver to notify of the buf done irq.
>>
>> Adding this notify interface in camss structure to do the subdevs cross
>> /*
>> * csid_isr - CSID module interrupt service routine
>> * @irq: Interrupt line
>> @@ -341,6 +353,14 @@ static irqreturn_t csid_isr(int irq, void *dev)
>> if (csid->phy.en_vc & BIT(i)) {
>> val = readl_relaxed(csid->base +
>> CSID_CSI2_RDIN_IRQ_STATUS(i));
>> writel_relaxed(val, csid->base +
>> CSID_CSI2_RDIN_IRQ_CLEAR(i));
>> +
>> + if (buf_done_val & BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET +
>> i)) {
>> + /* For Titan 780, Buf Done IRQ® has been moved to
>> CSID from VFE.
>> + * Once CSID received Buf Done, need notify this
>> event to VFE.
>> + * Trigger VFE to handle Buf Done process.
>> + */
>> + csid->camss->notify(&csid->subdev,
>> CAMSS_MSG_BUF_DONE, (void *)&i);
>
> Instead of a generic notify function with an enum passed to a switch
> lets just have a dedicated function for each functional callback.
>
> csid->camss->reg_update_cmd();
> csid->camss->reg_clear_cmd();
>
> We can get rid of a switch and an additional enum that way, plus
> redundant "event not supported" error checking.
>
Sure, will update the code based on suggestion.
>> + }
>> }
>> val = 1 << IRQ_CMD_CLEAR;
>> @@ -434,6 +454,23 @@ static void csid_subdev_init(struct csid_device
>> *csid)
>> csid->testgen.nmodes = CSID_PAYLOAD_MODE_NUM_SUPPORTED_GEN2;
>> }
>> +static void csid_subdev_process_msg(struct csid_device *csid,
>> unsigned int msg_type, void *arg)
>> +{
>> + int msg_data = *(int *)arg;
>> +
>> + switch (msg_type) {
>> + case CAMSS_MSG_RUP:
>> + csid_reg_update(csid, msg_data);
>> + break;
>> + case CAMSS_MSG_RUP_CLEAR:
>> + csid_reg_update_clear(csid, msg_data);
>> + break;
>> + default:
>> + dev_err(csid->camss->dev, "NOT Supported EVT Type\n");
>> + break;
>> + }
>> +}
>> +
>> const struct csid_hw_ops csid_ops_gen3 = {
>> .configure_stream = csid_configure_stream,
>> .configure_testgen_pattern = csid_configure_testgen_pattern,
>> @@ -442,4 +479,5 @@ const struct csid_hw_ops csid_ops_gen3 = {
>> .reset = csid_reset,
>> .src_pad_code = csid_src_pad_code,
>> .subdev_init = csid_subdev_init,
>> + .process_msg = csid_subdev_process_msg,
>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h
>> b/drivers/media/platform/qcom/camss/camss-vfe.h
>> index 10e2cc3c0b83..a8b09ce9941b 100644
>> --- a/drivers/media/platform/qcom/camss/camss-vfe.h
>> +++ b/drivers/media/platform/qcom/camss/camss-vfe.h
>> @@ -115,6 +115,7 @@ struct vfe_hw_ops {
>> int (*vfe_halt)(struct vfe_device *vfe);
>> void (*violation_read)(struct vfe_device *vfe);
>> void (*vfe_wm_stop)(struct vfe_device *vfe, u8 wm);
>> + void (*process_msg)(struct vfe_device *vfe, unsigned int
>> msg_type, void *arg);
>> };
>> struct vfe_isr_ops {
>> diff --git a/drivers/media/platform/qcom/camss/camss.c
>> b/drivers/media/platform/qcom/camss/camss.c
>> index 1f1f44f6fbb2..abeb0918e47d 100644
>> --- a/drivers/media/platform/qcom/camss/camss.c
>> +++ b/drivers/media/platform/qcom/camss/camss.c
>> @@ -2202,6 +2202,52 @@ static void camss_genpd_cleanup(struct camss
>> *camss)
>> dev_pm_domain_detach(camss->genpd, true);
>> }
>> +static void camss_notify_msg(struct v4l2_subdev *sd,
>> + unsigned int msg_type, void *arg)
>> +{
>> + struct v4l2_device *v4l2_dev = sd->v4l2_dev;
>> + struct camss *camss = to_camss(v4l2_dev);
>> + struct vfe_device *vfe;
>> + struct vfe_line *vfe_line;
>> + struct csid_device *csid;
>> + int evt_data = *(int *)arg;
>> +
>> + switch (msg_type) {
>> + case CAMSS_MSG_BUF_DONE:
>> + csid = v4l2_get_subdevdata(sd);
>> + vfe = &(camss->vfe[csid->id]);
>> + if (vfe->res->hw_ops->process_msg)
>> + vfe->res->hw_ops->process_msg(vfe,
>> + CAMSS_MSG_BUF_DONE, (void *)&evt_data);
>> + break;
>> +
>> + case CAMSS_MSG_RUP:
>> + vfe_line = v4l2_get_subdevdata(sd);
>> + vfe = to_vfe(vfe_line);
>> + csid = &(camss->csid[vfe->id]);
>> +
>> + if (csid->res->hw_ops->process_msg)
>> + csid->res->hw_ops->process_msg(csid,
>> + CAMSS_MSG_RUP, (void *)&evt_data);
>> + break;
>> +
>> + case CAMSS_MSG_RUP_CLEAR:
>> + vfe_line = v4l2_get_subdevdata(sd);
>> + vfe = to_vfe(vfe_line);
>> + csid = &(camss->csid[vfe->id]);
>> +
>> + if (csid->res->hw_ops->process_msg)
>> + csid->res->hw_ops->process_msg(csid,
>> + CAMSS_MSG_RUP_CLEAR, (void *)&evt_data);
>> +
>> + break;
>> +
>> + default:
>> + dev_err(camss->dev, "Not supported evt type\n");
>> + break;
>> + }
>> +}
>
> Instead of having a central swiss army knife notify() callback these
> should be CSID or VFE specific callbacks vfe->buf_done() csid->msg_rup()
> etc.
>
I will try to add the callback in CSID and VFE driver.
Thanks,
Depeng
Powered by blists - more mailing lists