| 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: <26eb0d02-06a5-4743-b828-01206c65e9dc@quicinc.com>
Date: Wed, 7 Aug 2024 23:10:23 +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>,
Yongsheng Li
<quic_yon@...cinc.com>
Subject: Re: [PATCH 09/13] media: qcom: camss: Add CSID Gen3 support for
SM8550
Hi Bryan,
Looks like you missed this mail, Could you please check again?
On 7/11/2024 11:33 PM, Depeng Shao wrote:
> Hi Bryan,
>
> On 7/10/2024 7:28 PM, Bryan O'Donoghue wrote:
>> On 09/07/2024 17:06, Depeng Shao wrote:
>>> The CSID in SM8550 is gen3, it has new register offset and new
>>> functionality. The buf done irq,register update and reset are
>>> moved to CSID gen3. And CSID gen3 has a new register block which
>>> is named as CSID top, it controls the output of CSID, since the
>>> CSID can connect to Sensor Front End (SFE) or original VFE, the
>>> register in top block is used to control the HW connection.
>>>
>>> Co-developed-by: Yongsheng Li <quic_yon@...cinc.com>
>>> Signed-off-by: Yongsheng Li <quic_yon@...cinc.com>
>>> Signed-off-by: Depeng Shao <quic_depengs@...cinc.com>
>>> ---
>>> drivers/media/platform/qcom/camss/Makefile | 1 +
>>> .../platform/qcom/camss/camss-csid-gen3.c | 445 ++++++++++++++++++
>>> .../platform/qcom/camss/camss-csid-gen3.h | 26 +
>>> .../media/platform/qcom/camss/camss-csid.h | 2 +
>>> 4 files changed, 474 insertions(+)
>>> create mode 100644 drivers/media/platform/qcom/camss/camss-csid-gen3.c
>>> create mode 100644 drivers/media/platform/qcom/camss/camss-csid-gen3.h
>>>
>
>>> +
>>> +#define REG_UPDATE_RDI reg_update_rdi
>>> +
>>> +static void __csid_configure_rx(struct csid_device *csid,
>>> + struct csid_phy_config *phy, int vc)
>>> +{
>>> + int val;
>>> +
>>> + val = (phy->lane_cnt - 1) << CSI2_RX_CFG0_NUM_ACTIVE_LANES;
>>> + val |= phy->lane_assign << CSI2_RX_CFG0_DL0_INPUT_SEL;
>>> + val |= (phy->csiphy_id + CSI2_RX_CFG0_PHY_SEL_BASE_IDX) <<
>>> CSI2_RX_CFG0_PHY_NUM_SEL;
>>> +
>>> + writel_relaxed(val, csid->base + CSID_CSI2_RX_CFG0);
>>> +
>>> + val = 1 << CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN;
>>> + if (vc > 3)
>>> + val |= 1 << CSI2_RX_CFG1_VC_MODE;
>>
>> I realise downstream does these shifts but, I think in upstream we
>> should just define a BIT(x)
>>
>> #define CSI2_RX_CFG1_VC_MODE BIT(2)
>>
>> val |= CSI2_RX_CFG1_VC_MODE;
>>
>
> Here CSI2_RX_CFG1_VC_MODE just means a register bit offset, not a
> register configuration.
>
> Some fixed configuration can do this, but some register bits value are
> configured based on actual parameter.
> e.g.; val = (phy->lane_cnt - 1) << CSI2_RX_CFG0_NUM_ACTIVE_LANES;
>
> If we want to use macro definition, maybe we can do like below.
>
> #define CSI2_RX_CFG1_VC_MODE(n) ((n) << 2)
> val |= CSI2_RX_CFG1_VC_MODE(1);
>
>
> #define CSI2_RX_CFG0_DL0_INPUT_SEL(n) ((n) << 4)
> val |= CSI2_RX_CFG0_DL0_INPUT_SEL(phy->lane_assign)
>
> Could you please comment if we really need to do like this?
>
>
>>> + writel_relaxed(val, csid->base + CSID_CSI2_RX_CFG1);
>>> +}
>>> +
>>> +static void __csid_ctrl_rdi(struct csid_device *csid, int enable, u8
>>> rdi)
>>> +{
>>> + int val;
>>> +
>>> + if (enable)
>>> + val = 1 << RDI_CTRL_START_CMD;
>>> + else
>>> + val = 0 << RDI_CTRL_START_CMD;
>>
>> Here is an example of how the bit shifting is weird
>>
>> (0 << anything) is still zero
>>
>
> Understood, the value is same, but we can know the configuration clearly
> on this register bit. If we do like above way, then it likes below.
>
> #define RDI_CTRL_START_CMD(n) ((n) << 0) --> it is same with (n), but
> we don't know the register bit offset clearly if we use (n).
>
> if (enable)
> val = RDI_CTRL_START_CMD(1);
> else
> val = RDI_CTRL_START_CMD(0);
>
>>> + writel_relaxed(val, csid->base + CSID_RDI_CTRL(rdi));
>>> +}
>>> +
>>> +static void __csid_configure_top(struct csid_device *csid)
>>> +{
>>> + u32 val;
>>> +
>>> + /* CSID "top" is a new function in Titan780.
>>> + * CSID can connect to VFE & SFE(Sensor Front End).
>>> + * This connection is ontrolled by CSID "top".
>>> + * Only enable VFE path in current driver.
>>> + */
>>> + if (csid->top_base) {
>>
>> When is csid->top_base NULL ?
>>
>> Its required in your yaml.
>>
>
> csid->top_base is NULL when it is csid lite, I will add this info in yaml.
>
>
>>> +
>>> +static void csid_configure_stream(struct csid_device *csid, u8 enable)
>>> +{
>>> + u8 i;
>>> +
>>> + /* Loop through all enabled VCs and configure stream for each */
>>> + for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
>>> + if (csid->phy.en_vc & BIT(i)) {
>>> + /* Configure CSID "top" */
>>> + __csid_configure_top(csid);
>>> + __csid_configure_rdi_stream(csid, enable, i);
>>> + __csid_configure_rx(csid, &csid->phy, i);
>>> + __csid_ctrl_rdi(csid, enable, i);
>>> + }
>>> +}
>>
>> I really like this breakdown
>
> Sorry, I don't get it, do you mean you like that configuring the
> different block use different functions, and no other meaning?
>
>>> +
>>> +static int csid_configure_testgen_pattern(struct csid_device *csid,
>>> s32 val)
>>> +{
>>> + if (val > 0 && val <= csid->testgen.nmodes)
>>> + csid->testgen.mode = val;
>>
>> Surely you should just set the val parameter directly ?
>>
>> Also why is this a signed integer and how does it get assigned a
>> negative value which we need to mitigate against here >
>
> This was copied from csid-gen2 driver, they are same, so we can move to
> common file.
>
> And the val comes from ctrl->val, so I guess this is the reason why this
> agrument is signed integer.
>
> struct v4l2_ctrl {
> ...
> s32 val;
> ...
> };
>
>
>
>>> +
>>> +static u32 csid_src_pad_code(struct csid_device *csid, u32 sink_code,
>>> + unsigned int match_format_idx, u32 match_code)
>>> +{
>>> + switch (sink_code) {
>>> + case MEDIA_BUS_FMT_SBGGR10_1X10:
>>> + {
>>> + u32 src_code[] = {
>>> + MEDIA_BUS_FMT_SBGGR10_1X10,
>>> + MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE,
>>> + };
>>> +
>>> + return csid_find_code(src_code, ARRAY_SIZE(src_code),
>>> + match_format_idx, match_code);
>>> + }
>>> + case MEDIA_BUS_FMT_Y10_1X10:
>>> + {
>>> + u32 src_code[] = {
>>> + MEDIA_BUS_FMT_Y10_1X10,
>>> + MEDIA_BUS_FMT_Y10_2X8_PADHI_LE,
>>> + };
>>> +
>>> + return csid_find_code(src_code, ARRAY_SIZE(src_code),
>>> + match_format_idx, match_code);
>>> + }
>>> + default:
>>> + if (match_format_idx > 0)
>>> + return 0;
>>> +
>>> + return sink_code;
>>> + }
>>> +}
>>
>> Same code as in gen2.
>>
>> You could move the gen2 version of this into camss-csid.c and just
>> reuse in both.
>>
>
> Sure, it is same with the comments in vfe driver, I will try to move
> same code to camss-csid.c
>
> Thanks,
> Depeng
Thanks,
Depeng
Powered by blists - more mailing lists