| 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: <642d1669-32e1-624c-287a-7f51c45d48b8@marek.ca>
Date: Tue, 8 Jun 2021 19:08:19 -0400
From: Jonathan Marek <jonathan@...ek.ca>
To: Robert Foss <robert.foss@...aro.org>
Cc: MSM <linux-arm-msm@...r.kernel.org>,
Andrey Konovalov <andrey.konovalov@...aro.org>,
Todor Tomov <todor.too@...il.com>,
Andy Gross <agross@...nel.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
open list <linux-kernel@...r.kernel.org>,
"open list:QUALCOMM CAMERA SUBSYSTEM DRIVER"
<linux-media@...r.kernel.org>
Subject: Re: [PATCH 14/17] media: camss: Add initial support for VFE hardware
version Titan 480
On 5/31/21 8:13 AM, Robert Foss wrote:
> Hey Jonathan,
>
> Thanks for sending this out.
>
> There are a few checkpatch --strict warnings/etc. in this patch. I
> won't cover them individually below.
>
> On Tue, 11 May 2021 at 20:08, Jonathan Marek <jonathan@...ek.ca> wrote:
>>
>> Add support for VFE found on SM8250 (Titan 480). This implementation is
>> based on the titan 170 implementation. It supports the normal and lite VFE,
>> and only supports the RDI0 capture path.
>>
>> Signed-off-by: Jonathan Marek <jonathan@...ek.ca>
>> ---
>> drivers/media/platform/qcom/camss/Makefile | 1 +
>> .../media/platform/qcom/camss/camss-vfe-480.c | 554 ++++++++++++++++++
>> drivers/media/platform/qcom/camss/camss-vfe.h | 1 +
>> 3 files changed, 556 insertions(+)
>> create mode 100644 drivers/media/platform/qcom/camss/camss-vfe-480.c
>>
>> diff --git a/drivers/media/platform/qcom/camss/Makefile b/drivers/media/platform/qcom/camss/Makefile
>> index 0752c46ea37b..81dd56aff0f2 100644
>> --- a/drivers/media/platform/qcom/camss/Makefile
>> +++ b/drivers/media/platform/qcom/camss/Makefile
>> @@ -15,6 +15,7 @@ qcom-camss-objs += \
>> camss-vfe-4-7.o \
>> camss-vfe-4-8.o \
>> camss-vfe-170.o \
>> + camss-vfe-480.o \
>> camss-vfe-gen1.o \
>> camss-vfe.o \
>> camss-video.o \
>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-480.c b/drivers/media/platform/qcom/camss/camss-vfe-480.c
>> new file mode 100644
>> index 000000000000..79210fabbc2a
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/camss/camss-vfe-480.c
>> @@ -0,0 +1,554 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * camss-vfe-480.c
>> + *
>> + * Qualcomm MSM Camera Subsystem - VFE (Video Front End) Module v480 (SM8250)
>> + *
>> + * Copyright (C) 2020-2021 Linaro Ltd.
>> + * Copyright (C) 2021 Jonathan Marek
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +
>> +#include "camss.h"
>> +#include "camss-vfe.h"
>> +
>> +/* VFE 2/3 are lite and have a different register layout */
>> +#define IS_LITE (vfe->id >= 2 ? 1 : 0)
>> +
>> +#define VFE_HW_VERSION (0x00)
>> +
>> +#define VFE_GLOBAL_RESET_CMD (IS_LITE ? 0x0c : 0x1c)
>> +#define GLOBAL_RESET_HW_AND_REG (IS_LITE ? BIT(1) : BIT(0))
>> +
>> +#define VFE_REG_UPDATE_CMD (IS_LITE ? 0x20 : 0x34)
>> +#define REG_UPDATE_RDI(n) (IS_LITE ? BIT(n) : BIT(1 + (n)))
>> +#define VFE_IRQ_CMD (IS_LITE ? 0x24 : 0x38)
>> +#define IRQ_CMD_GLOBAL_CLEAR BIT(0)
>> +
>> +#define VFE_IRQ_MASK(n) ((IS_LITE ? 0x28 : 0x3c) + (n) * 4)
>> +#define IRQ_MASK_0_RESET_ACK (IS_LITE ? BIT(17) : BIT(0))
>> +#define IRQ_MASK_0_BUS_TOP_IRQ (IS_LITE ? BIT(4) : BIT(7))
>> +#define VFE_IRQ_CLEAR(n) ((IS_LITE ? 0x34 : 0x48) + (n) * 4)
>> +#define VFE_IRQ_STATUS(n) ((IS_LITE ? 0x40 : 0x54) + (n) * 4)
>> +
>> +#define BUS_REG_BASE (IS_LITE ? 0x1a00 : 0xaa00)
>> +
>> +#define VFE_BUS_WM_CGC_OVERRIDE (BUS_REG_BASE + 0x08)
>> +#define WM_CGC_OVERRIDE_ALL (0x3FFFFFF)
>> +
>> +#define VFE_BUS_WM_TEST_BUS_CTRL (BUS_REG_BASE + 0xdc)
>> +
>> +#define VFE_BUS_IRQ_MASK(n) (BUS_REG_BASE + 0x18 + (n) * 4)
>> +#define BUS_IRQ_MASK_0_RDI_RUP(n) (IS_LITE ? BIT(n) : BIT(3 + (n)))
>> +#define BUS_IRQ_MASK_0_COMP_DONE(n) (IS_LITE ? BIT(4 + (n)) : BIT(6 + (n)))
>> +#define VFE_BUS_IRQ_CLEAR(n) (BUS_REG_BASE + 0x20 + (n) * 4)
>> +#define VFE_BUS_IRQ_STATUS(n) (BUS_REG_BASE + 0x28 + (n) * 4)
>> +#define VFE_BUS_IRQ_CLEAR_GLOBAL (BUS_REG_BASE + 0x30)
>> +
>> +#define VFE_BUS_WM_CFG(n) (BUS_REG_BASE + 0x200 + (n) * 0x100)
>> +#define WM_CFG_EN (0)
>> +#define WM_CFG_MODE (16)
>> +#define MODE_QCOM_PLAIN (0)
>> +#define MODE_MIPI_RAW (1)
>> +#define VFE_BUS_WM_IMAGE_ADDR(n) (BUS_REG_BASE + 0x204 + (n) * 0x100)
>> +#define VFE_BUS_WM_FRAME_INCR(n) (BUS_REG_BASE + 0x208 + (n) * 0x100)
>> +#define VFE_BUS_WM_IMAGE_CFG_0(n) (BUS_REG_BASE + 0x20c + (n) * 0x100)
>> +#define WM_IMAGE_CFG_0_DEFAULT_WIDTH (0xFFFF)
>> +#define VFE_BUS_WM_IMAGE_CFG_1(n) (BUS_REG_BASE + 0x210 + (n) * 0x100)
>> +#define VFE_BUS_WM_IMAGE_CFG_2(n) (BUS_REG_BASE + 0x214 + (n) * 0x100)
>> +#define VFE_BUS_WM_PACKER_CFG(n) (BUS_REG_BASE + 0x218 + (n) * 0x100)
>> +#define VFE_BUS_WM_HEADER_ADDR(n) (BUS_REG_BASE + 0x220 + (n) * 0x100)
>> +#define VFE_BUS_WM_HEADER_INCR(n) (BUS_REG_BASE + 0x224 + (n) * 0x100)
>> +#define VFE_BUS_WM_HEADER_CFG(n) (BUS_REG_BASE + 0x228 + (n) * 0x100)
>> +
>> +#define VFE_BUS_WM_IRQ_SUBSAMPLE_PERIOD(n) (BUS_REG_BASE + 0x230 + (n) * 0x100)
>> +#define VFE_BUS_WM_IRQ_SUBSAMPLE_PATTERN(n) (BUS_REG_BASE + 0x234 + (n) * 0x100)
>> +#define VFE_BUS_WM_FRAMEDROP_PERIOD(n) (BUS_REG_BASE + 0x238 + (n) * 0x100)
>> +#define VFE_BUS_WM_FRAMEDROP_PATTERN(n) (BUS_REG_BASE + 0x23c + (n) * 0x100)
>> +
>> +#define VFE_BUS_WM_SYSTEM_CACHE_CFG(n) (BUS_REG_BASE + 0x260 + (n) * 0x100)
>> +#define VFE_BUS_WM_BURST_LIMIT(n) (BUS_REG_BASE + 0x264 + (n) * 0x100)
>> +
>> +/* for titan 480, each bus client is hardcoded to a specific path
>> + * and each bus client is part of a hardcoded "comp group"
>> + */
>> +#define RDI_WM(n) ((IS_LITE ? 0 : 23) + n)
>> +#define RDI_COMP_GROUP(n) ((IS_LITE ? 0 : 11) + n)
>
> The indentation of the different types of defines above differ from
> vfe170, but I kind of prefer this style. Feel free to change either.
>
Its not just the indentation, the naming style is a bit different too.
Not sure its worth trying to make them fully consistent, but I wouldn't
mind just changing the identation for 170.
>> +
>> +static void vfe_hw_version_read(struct vfe_device *vfe, struct device *dev)
>> +{
>> + 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_dbg(dev, "VFE HW Version = %u.%u.%u\n", gen, rev, step);
>> +}
>> +
>> +static void vfe_global_reset(struct vfe_device *vfe)
>> +{
>> + writel_relaxed(IRQ_MASK_0_RESET_ACK, vfe->base + VFE_IRQ_MASK(0));
>> + writel_relaxed(GLOBAL_RESET_HW_AND_REG, vfe->base + VFE_GLOBAL_RESET_CMD);
>> +}
>> +
>> +static void vfe_wm_start(struct vfe_device *vfe, u8 wm, struct vfe_line *line)
>> +{
>> + struct v4l2_pix_format_mplane *pix =
>> + &line->video_out.active_fmt.fmt.pix_mp;
>> +
>> + wm = RDI_WM(wm); /* map to actual WM used (from wm=RDI index) */
>> +
>> + /* no clock gating at bus input */
>> + writel_relaxed(WM_CGC_OVERRIDE_ALL, vfe->base + VFE_BUS_WM_CGC_OVERRIDE);
>> +
>> + writel_relaxed(0x0, vfe->base + VFE_BUS_WM_TEST_BUS_CTRL);
>> +
>> + writel_relaxed(pix->plane_fmt[0].bytesperline * pix->height,
>> + vfe->base + VFE_BUS_WM_FRAME_INCR(wm));
>> + writel_relaxed(0xf, vfe->base + VFE_BUS_WM_BURST_LIMIT(wm));
>> + writel_relaxed(WM_IMAGE_CFG_0_DEFAULT_WIDTH,
>> + vfe->base + VFE_BUS_WM_IMAGE_CFG_0(wm));
>> + writel_relaxed(pix->plane_fmt[0].bytesperline,
>> + vfe->base + VFE_BUS_WM_IMAGE_CFG_2(wm));
>> + writel_relaxed(0, vfe->base + VFE_BUS_WM_PACKER_CFG(wm));
>
> ^^^ using more of the 130char line length is probably better for legibility.
>
More than 100 line length is still a warning from checkpatch, and this
is IMO more readable since all the "vfe->base + X" are more or less at
the same indentation, and removing the line breaks would make it harder
to see which registers are being updated.
>> +
>> + /* no dropped frames, one irq per frame */
>> + writel_relaxed(0, vfe->base + VFE_BUS_WM_FRAMEDROP_PERIOD(wm));
>> + writel_relaxed(1, vfe->base + VFE_BUS_WM_FRAMEDROP_PATTERN(wm));
>> + writel_relaxed(0, vfe->base + VFE_BUS_WM_IRQ_SUBSAMPLE_PERIOD(wm));
>> + writel_relaxed(1, vfe->base + VFE_BUS_WM_IRQ_SUBSAMPLE_PATTERN(wm));
>> +
>> + writel_relaxed(1 << WM_CFG_EN | MODE_MIPI_RAW << WM_CFG_MODE,
>> + vfe->base + VFE_BUS_WM_CFG(wm));
>> +}
>> +
>> +static void vfe_wm_stop(struct vfe_device *vfe, u8 wm)
>> +{
>> + wm = RDI_WM(wm); /* map to actual WM used (from wm=RDI index) */
>> + writel_relaxed(0, vfe->base + VFE_BUS_WM_CFG(wm));
>> +}
>> +
>> +static void vfe_wm_update(struct vfe_device *vfe, u8 wm, u32 addr,
>> + struct vfe_line *line)
>> +{
>> + wm = RDI_WM(wm); /* map to actual WM used (from wm=RDI index) */
>> + writel_relaxed(addr, vfe->base + VFE_BUS_WM_IMAGE_ADDR(wm));
>> +}
>> +
>> +static void vfe_reg_update(struct vfe_device *vfe, enum vfe_line_id line_id)
>> +{
>> + vfe->reg_update |= REG_UPDATE_RDI(line_id);
>> + writel_relaxed(vfe->reg_update, vfe->base + VFE_REG_UPDATE_CMD);
>> +}
>> +
>> +static inline void vfe_reg_update_clear(struct vfe_device *vfe,
>> + enum vfe_line_id line_id)
>> +{
>> + vfe->reg_update &= ~REG_UPDATE_RDI(line_id);
>> +}
>> +
>> +static void vfe_enable_irq_common(struct vfe_device *vfe)
>> +{
>> + /* enable only the IRQs used: rup and comp_done irqs for RDI0 */
>> + writel_relaxed(IRQ_MASK_0_RESET_ACK | IRQ_MASK_0_BUS_TOP_IRQ,
>> + vfe->base + VFE_IRQ_MASK(0));
>> + writel_relaxed(BUS_IRQ_MASK_0_RDI_RUP(0) |
>> + BUS_IRQ_MASK_0_COMP_DONE(RDI_COMP_GROUP(0)),
>> + vfe->base + VFE_BUS_IRQ_MASK(0));
>> +}
>> +
>> +/*
>> + * 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)
>> +{
>> + struct vfe_device *vfe = dev;
>> + u32 status;
>> +
>> + status = readl_relaxed(vfe->base + VFE_IRQ_STATUS(0));
>> + writel_relaxed(status, vfe->base + VFE_IRQ_CLEAR(0));
>> + writel_relaxed(IRQ_CMD_GLOBAL_CLEAR, vfe->base + VFE_IRQ_CMD);
>> +
>> + if (status & IRQ_MASK_0_RESET_ACK)
>> + vfe->isr_ops.reset_ack(vfe);
>> +
>> + if (status & IRQ_MASK_0_BUS_TOP_IRQ) {
>> + u32 status = readl_relaxed(vfe->base + VFE_BUS_IRQ_STATUS(0));
>> + writel_relaxed(status, vfe->base + VFE_BUS_IRQ_CLEAR(0));
>> + writel_relaxed(1, vfe->base + VFE_BUS_IRQ_CLEAR_GLOBAL);
>> +
>> + if (status & BUS_IRQ_MASK_0_RDI_RUP(0))
>> + vfe->isr_ops.reg_update(vfe, 0);
>> +
>> + if (status & BUS_IRQ_MASK_0_COMP_DONE(RDI_COMP_GROUP(0)))
>> + vfe->isr_ops.wm_done(vfe, 0);
>
> COMP_DONE is signalled in the status register, but wm_done() is
> called. comp_done() seems to never be called.
>
The current "vfe_isr_comp_done" is not relevant to RDI capture. Titan
480 (unlike 170) now uses comp done IRQs for RDI too.
This is a bit of a hack. wm_done is called with wm=0, but the
implementation passes it through RDI_WM(0) to map it to wm=23 (which is
the WM for RDI0 - with titan 480 the WM paths are fixed).
...
>> +const struct vfe_hw_ops vfe_ops_480 = {
>> + .global_reset = vfe_global_reset,
>> + .hw_version_read = vfe_hw_version_read,
>> + .isr = vfe_isr,
>> + .pm_domain_off = vfe_pm_domain_off,
>> + .pm_domain_on = vfe_pm_domain_on,
>> + .reg_update_clear = vfe_reg_update_clear,
>> + .reg_update = vfe_reg_update,
>> + .subdev_init = vfe_subdev_init,
>> + .vfe_disable = vfe_disable,
>> + .vfe_enable = vfe_enable,
>> + .vfe_halt = vfe_halt,
>> +};
>
> Again there are some functions that could be refactored out to a vfe
> gen2 parent struct & object. This time I think it's worth refactoring
> out the common code, especially since we know more platforms based on
> this architecture are coming.
>
> vfe_queue_buffer
> vfe_pm_domain_on
> vfe_pm_domain_off
> vfe_isr_wm_done
> vfe_isr_reg_update
> vfe_get_output (although vfe170 contains a frame_skip chunk that
> should be removed)
> vfe_halt
>
In the current minimal implementation of both vfe-170 and vfe-480, most
of these could be shared, but I think duplicating these few functions
for now is fine, it can easily be resolved later. (there might be less
shared code than expected if the titan 480 WMs are implementated fully
instead of the "hack" I've used to support only RDI cases).
>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h
>> index 844b9275031d..83b11ae1572d 100644
>> --- a/drivers/media/platform/qcom/camss/camss-vfe.h
>> +++ b/drivers/media/platform/qcom/camss/camss-vfe.h
>> @@ -201,5 +201,6 @@ extern const struct vfe_hw_ops vfe_ops_4_1;
>> extern const struct vfe_hw_ops vfe_ops_4_7;
>> extern const struct vfe_hw_ops vfe_ops_4_8;
>> extern const struct vfe_hw_ops vfe_ops_170;
>> +extern const struct vfe_hw_ops vfe_ops_480;
>>
>> #endif /* QC_MSM_CAMSS_VFE_H */
>> --
>> 2.26.1
>>
Powered by blists - more mailing lists