| 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: <b73f1117-580b-46cf-8c56-9e78974b6e45@quicinc.com>
Date: Thu, 11 Jul 2024 21:29:53 +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 10/13] media: qcom: camss: Add support for VFE hardware
version Titan 780
Hi Bryan,
On 7/10/2024 7:47 PM, Bryan O'Donoghue wrote:
> On 09/07/2024 17:06, Depeng Shao wrote:
>> Add support for VFE found on SM8550 (Titan 780). This implementation is
>> based on the titan 480 implementation. It supports the normal and lite
>> VFE.
>>
>> 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 +
>> .../media/platform/qcom/camss/camss-vfe-780.c | 404 ++++++++++++++++++
>> 2 files changed, 405 insertions(+)
>> create mode 100644 drivers/media/platform/qcom/camss/camss-vfe-780.c
>>
>> diff --git a/drivers/media/platform/qcom/camss/Makefile
>> b/drivers/media/platform/qcom/camss/Makefile
>> index c336e4c1a399..a83b7a8dcef7 100644
>> --- a/drivers/media/platform/qcom/camss/Makefile
>> +++ b/drivers/media/platform/qcom/camss/Makefile
>> @@ -17,6 +17,7 @@ qcom-camss-objs += \
>> camss-vfe-4-8.o \
>> camss-vfe-17x.o \
>> camss-vfe-480.o \
>> + camss-vfe-780.o \
>> camss-vfe-gen1.o \
>> camss-vfe.o \
>> camss-video.o \
>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-780.c
>> b/drivers/media/platform/qcom/camss/camss-vfe-780.c
>> new file mode 100644
>> index 000000000000..abef2d5b9c2e
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/camss/camss-vfe-780.c
>> @@ -0,0 +1,404 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * camss-vfe-780.c
>> +
>> +static u32 vfe_hw_version(struct vfe_device *vfe)
>> +{
>> + u32 hw_version = readl_relaxed(vfe->base + VFE_HW_VERSION);
>> +
>> + u32 gen = (hw_version >> 28) & 0xF;
>> + u32 rev = (hw_version >> 16) & 0xFFF;
>> + u32 step = hw_version & 0xFFFF;
>> +
>> + dev_info(vfe->camss->dev, "VFE HW Version = %u.%u.%u\n", gen,
>> rev, step);
>> +
>> + return hw_version;
>> +}
>
> This could be functionally decomposed into vfe_hw_version_v2() or
> similar and exported by camss-vfe.c
>
>> +
Yes, same with below comments, I will try to figure out which functions
can be moved to common files.
>> +
>> +/*
>> + * vfe_isr - VFE module interrupt handler
>> + * @irq: Interrupt line
>> + * @dev: VFE device
>> + *
>> + * Return IRQ_HANDLED on success
>> + */
>> +static irqreturn_t vfe_isr(int irq, void *dev)
>> +{
>> + /* Buf Done has beem moved to CSID in Titan 780.
>> + * Disable VFE related IRQ.
>> + * Clear the contents of this function.
>> + * Return IRQ_HANDLED.
>> + */
>> + return IRQ_HANDLED;
>> +}
>
> What's the point of this ISR at all if it never fires and just returns
> done ?
>
> Since it takes no action - it can't do anything useful and therefore if
> it ever did fire, would fire ad infinitum.
>
> Please drop
Sure, will drop it.
>> +
>> +static int vfe_get_output(struct vfe_line *line)
>> +{
>> + struct vfe_device *vfe = to_vfe(line);
>> + struct vfe_output *output;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&vfe->output_lock, flags);
>> +
>> + output = &line->output;
>> + if (output->state > VFE_OUTPUT_RESERVED) {
>> + dev_err(vfe->camss->dev, "Output is running\n");
>> + goto error;
>> + }
>> +
>> + output->wm_num = 1;
>> +
>> + /* Correspondence between VFE line number and WM number.
>> + * line 0 -> RDI 0, line 1 -> RDI1, line 2 -> RDI2, line 3 ->
>> PIX/RDI3
>> + * Note this 1:1 mapping will not work for PIX streams.
>> + */
>> + output->wm_idx[0] = line->id;
>> + vfe->wm_output_map[line->id] = line->id;
>> +
>> + output->drop_update_idx = 0;
>> +
>> + spin_unlock_irqrestore(&vfe->output_lock, flags);
>> +
>> + return 0;
>> +
>> +error:
>> + spin_unlock_irqrestore(&vfe->output_lock, flags);
>> + output->state = VFE_OUTPUT_OFF;
>> +
>> + return -EINVAL;
>> +}
>
> This is copy/paste from vfe480 and should be functionally decomposed
> into a common function in camss-vfe.
Sure, the flow of some functions are same with other platform, and don't
read/write registers, this can be moved to a common file and reused by
all platform.
I will think about this.
>> +
>> +static int vfe_enable_output(struct vfe_line *line)
>> +{
>> + struct vfe_device *vfe = to_vfe(line);
>> + struct vfe_output *output = &line->output;
>> + unsigned long flags;
>> + unsigned int i;
>> +
>> + spin_lock_irqsave(&vfe->output_lock, flags);
>> +
>> + vfe_reg_update_clear(vfe, line->id);
>> +
>> + if (output->state > VFE_OUTPUT_RESERVED) {
>> + dev_err(vfe->camss->dev, "Output is not in reserved state %d\n",
>> + output->state);
>> + spin_unlock_irqrestore(&vfe->output_lock, flags);
>> + return -EINVAL;
>> + }
>> +
>> + WARN_ON(output->gen2.active_num);
>> +
>> + output->state = VFE_OUTPUT_ON;
>> +
>> + output->sequence = 0;
>> +
>> + vfe_wm_start(vfe, output->wm_idx[0], line);
>> +
>> + for (i = 0; i < MAX_VFE_ACT_BUF; i++) {
>> + output->buf[i] = vfe_buf_get_pending(output);
>> + if (!output->buf[i])
>> + break;
>> + output->gen2.active_num++;
>> + vfe_wm_update(vfe, output->wm_idx[0],
>> output->buf[i]->addr[0], line);
>> +
>> + vfe_reg_update(vfe, line->id);
>
> I see this differs from vfe480 in that vfe_reg_update(vfe, line-id); is
> done on each iteration of this loop whereas in 480 it is done directly
> after the loop, seems to me this would be a valid fix for 480 too
> leading to my next comment
>
Yes, vfe-480 also need this.
>> + }
>> +
>> + spin_unlock_irqrestore(&vfe->output_lock, flags);
>> +
>> + return 0;
>> +}
>
> This function is so similar across different SoCs with very minor
> differences that instead of copy/pasting and very slightly tweaking, we
> should be functionally decomposing and using a flag of some kind to
> differentaite between wait_reg_update logic in 480 and not in 780.
>
> Again I think we should functionally decompose into camss-vfe.c and use
> a flag to branch the logic for the very slight logical difference
> between the two
>
> vfe-480.c
>
> output->sequence = 0;
> output->wait_reg_update = 0;
> reinit_completion(&output->reg_update);
>
> As a result your fix for line->id would be useful across SoCs instead of
> isolated to vfe 780.
>
Yes, some functions are same code flow, and don't read/write register,
this can be moved to a common file and reused by all platform.
I will think about this.
>> +
>> +/*
>> + * vfe_enable - Enable streaming on VFE line
>> + * @line: VFE line
>> + *
>> + * Return 0 on success or a negative error code otherwise
>> + */
>> +static int vfe_enable(struct vfe_line *line)
>> +{
>> + struct vfe_device *vfe = to_vfe(line);
>> + int ret;
>> +
>> + mutex_lock(&vfe->stream_lock);
>> +
>> + vfe->stream_count++;
>> +
>> + mutex_unlock(&vfe->stream_lock);
>> +
>> + ret = vfe_get_output(line);
>> + if (ret < 0)
>> + goto error_get_output;
>> +
>> + ret = vfe_enable_output(line);
>> + if (ret < 0)
>> + goto error_enable_output;
>> +
>> + vfe->was_streaming = 1;
>> +
>> + return 0;
>> +
>> +error_enable_output:
>> + vfe_put_output(line);
>> +
>> +error_get_output:
>> + mutex_lock(&vfe->stream_lock);
>> +
>> + vfe->stream_count--;
>> +
>> + mutex_unlock(&vfe->stream_lock);
>> +
>> + return ret;
>> +}
>
> Same thesis on functional decomposition - this should be moved to
> camss-vfe.c and made common - its only a minor difference betwen the
> required logic on different SoCs so there's not a compelling reason to
> have largely identical functions living in difference .c files in the
> same driver.
>
Sure, I will check which functions can be moved to camss-vfe.c.
Thanks,
Depeng
Powered by blists - more mailing lists