| 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: <aHD2h9/LqSZ4ru6K@trex>
Date: Fri, 11 Jul 2025 13:33:27 +0200
From: Jorge Ramirez <jorge.ramirez@....qualcomm.com>
To: Dikshita Agarwal <quic_dikshita@...cinc.com>
Cc: Jorge Ramirez <jorge.ramirez@....qualcomm.com>, krzk+dt@...nel.org,
bryan.odonoghue@...aro.org, quic_vgarodia@...cinc.com,
mchehab@...nel.org, robh@...nel.org, conor+dt@...nel.org,
konradybcio@...nel.org, andersson@...nel.org,
linux-arm-msm@...r.kernel.org, linux-media@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/5] media: venus: vdec: AR50_LITE video core support
On 10/07/25 14:25:33, Dikshita Agarwal wrote:
>
>
> On 7/9/2025 12:44 AM, Jorge Ramirez wrote:
> > On 30/06/25 12:17:32, Dikshita Agarwal wrote:
> >>
> >>
> >> On 6/27/2025 8:48 PM, Jorge Ramirez wrote:
> >>> On 27/06/25 18:17:27, Dikshita Agarwal wrote:
> >>>>
> >>>>
> >>>> On 6/26/2025 7:29 PM, Jorge Ramirez-Ortiz wrote:
> >>>>> The AR50_LITE is a streamlined variant of the AR50 video core, designed
> >>>>> for power and cost-efficient platforms.
> >>>>>
> >>>>> It supports hardware-accelerated decoding of H.264, HEVC, and VP9
> >>>>> formats.
> >>>>>
> >>>>> Co-developed-by: Loic Poulain <loic.poulain@....qualcomm.com>
> >>>>> Signed-off-by: Loic Poulain <loic.poulain@....qualcomm.com>
> >>>>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@....qualcomm.com>
> >>>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
> >>>>> ---
> >>>>> drivers/media/platform/qcom/venus/core.c | 11 ++-
> >>>>> drivers/media/platform/qcom/venus/core.h | 11 ++-
> >>>>> drivers/media/platform/qcom/venus/firmware.c | 8 +-
> >>>>> drivers/media/platform/qcom/venus/helpers.c | 80 +++++++++++++++++++
> >>>>> drivers/media/platform/qcom/venus/helpers.h | 2 +
> >>>>> .../media/platform/qcom/venus/hfi_helper.h | 10 ++-
> >>>>> drivers/media/platform/qcom/venus/hfi_venus.c | 14 ++--
> >>>>> .../media/platform/qcom/venus/pm_helpers.c | 1 +
> >>>>> drivers/media/platform/qcom/venus/vdec.c | 15 ++--
> >>>>> 9 files changed, 128 insertions(+), 24 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> >>>>> index d305d74bb152..736ef53d988d 100644
> >>>>> --- a/drivers/media/platform/qcom/venus/core.c
> >>>>> +++ b/drivers/media/platform/qcom/venus/core.c
> >>>>> @@ -254,14 +254,19 @@ static int venus_enumerate_codecs(struct venus_core *core, u32 type)
> >>>>>
> >>>>> static void venus_assign_register_offsets(struct venus_core *core)
> >>>>> {
> >>>>> - if (IS_IRIS2(core) || IS_IRIS2_1(core)) {
> >>>>> - core->vbif_base = core->base + VBIF_BASE;
> >>>>> + if (IS_IRIS2(core) || IS_IRIS2_1(core) || IS_AR50_LITE(core)) {
> >>>>> core->cpu_base = core->base + CPU_BASE_V6;
> >>>>> core->cpu_cs_base = core->base + CPU_CS_BASE_V6;
> >>>>> core->cpu_ic_base = core->base + CPU_IC_BASE_V6;
> >>>>> core->wrapper_base = core->base + WRAPPER_BASE_V6;
> >>>>> core->wrapper_tz_base = core->base + WRAPPER_TZ_BASE_V6;
> >>>>> - core->aon_base = core->base + AON_BASE_V6;
> >>>>> + if (IS_AR50_LITE(core)) {
> >>>>> + core->vbif_base = NULL;
> >>>>> + core->aon_base = NULL;
> >>>>> + } else {
> >>>>> + core->vbif_base = core->base + VBIF_BASE;
> >>>>> + core->aon_base = core->base + AON_BASE_V6;
> >>>>> + }
> >>>>> } else {
> >>>>> core->vbif_base = core->base + VBIF_BASE;
> >>>>> core->cpu_base = core->base + CPU_BASE;
> >>>>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> >>>>> index b412e0c5515a..e755a28e919b 100644
> >>>>> --- a/drivers/media/platform/qcom/venus/core.h
> >>>>> +++ b/drivers/media/platform/qcom/venus/core.h
> >>>>> @@ -382,6 +382,7 @@ enum venus_inst_modes {
> >>>>> * @lock: instance lock
> >>>>> * @core: a reference to the core struct
> >>>>> * @clk_data: clock data per core ID
> >>>>> + * @eosbufs: a lit of EOS buffers
> >>>>> * @dpbbufs: a list of decoded picture buffers
> >>>>> * @internalbufs: a list of internal bufferes
> >>>>> * @registeredbufs: a list of registered capture bufferes
> >>>>> @@ -450,6 +451,7 @@ struct venus_inst {
> >>>>> struct mutex lock;
> >>>>> struct venus_core *core;
> >>>>> struct clock_data clk_data;
> >>>>> + struct list_head eosbufs;
> >>>>> struct list_head dpbbufs;
> >>>>> struct list_head internalbufs;
> >>>>> struct list_head registeredbufs;
> >>>>> @@ -520,7 +522,14 @@ struct venus_inst {
> >>>>> #define IS_V1(core) ((core)->res->hfi_version == HFI_VERSION_1XX)
> >>>>> #define IS_V3(core) ((core)->res->hfi_version == HFI_VERSION_3XX)
> >>>>> #define IS_V4(core) ((core)->res->hfi_version == HFI_VERSION_4XX)
> >>>>> -#define IS_V6(core) ((core)->res->hfi_version == HFI_VERSION_6XX)
> >>>>> +static inline bool IS_V6(struct venus_core *core)
> >>>>> +{
> >>>>> + if (WARN_ON_ONCE(!core))
> >>>>> + return false;
> >>>>> +
> >>>>> + return core->res->hfi_version == HFI_VERSION_6XX ||
> >>>>> + core->res->hfi_version == HFI_VERSION_6XX_LITE;
> >>>>> +}
> >>>> It should be HFI_VERSION_4XX_LITE for AR50_LITE. 4XX represents SC7280 and
> >>>> SDM845 which are AR50.
> >>>
> >>> ah good information - where is this documented? I never found this
> >>> information... I'd appreciate if you could confirm with some document
> >>> for future reference.
> >>>
> >>>>>
> >>>>> #define IS_AR50(core) ((core)->res->vpu_version == VPU_VERSION_AR50)
> >>>>> #define IS_AR50_LITE(core) ((core)->res->vpu_version == VPU_VERSION_AR50_LITE)
> >>>>> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> >>>>> index 66a18830e66d..f8dcef0426ac 100644
> >>>>> --- a/drivers/media/platform/qcom/venus/firmware.c
> >>>>> +++ b/drivers/media/platform/qcom/venus/firmware.c
> >>>>> @@ -30,7 +30,7 @@ static void venus_reset_cpu(struct venus_core *core)
> >>>>> u32 fw_size = core->fw.mapped_mem_size;
> >>>>> void __iomem *wrapper_base;
> >>>>>
> >>>>> - if (IS_IRIS2_1(core))
> >>>>> + if (IS_IRIS2_1(core) || IS_AR50_LITE(core))
> >>>>> wrapper_base = core->wrapper_tz_base;
> >>>>> else
> >>>>> wrapper_base = core->wrapper_base;
> >>>>> @@ -42,7 +42,7 @@ static void venus_reset_cpu(struct venus_core *core)
> >>>>> writel(fw_size, wrapper_base + WRAPPER_NONPIX_START_ADDR);
> >>>>> writel(fw_size, wrapper_base + WRAPPER_NONPIX_END_ADDR);
> >>>>>
> >>>>> - if (IS_IRIS2_1(core)) {
> >>>>> + if (IS_IRIS2_1(core) || IS_AR50_LITE(core)) {
> >>>>> /* Bring XTSS out of reset */
> >>>>> writel(0, wrapper_base + WRAPPER_TZ_XTSS_SW_RESET);
> >>>>> } else {
> >>>>> @@ -68,7 +68,7 @@ int venus_set_hw_state(struct venus_core *core, bool resume)
> >>>>> if (resume) {
> >>>>> venus_reset_cpu(core);
> >>>>> } else {
> >>>>> - if (IS_IRIS2_1(core))
> >>>>> + if (IS_IRIS2_1(core) || IS_AR50_LITE(core))
> >>>>> writel(WRAPPER_XTSS_SW_RESET_BIT,
> >>>>> core->wrapper_tz_base + WRAPPER_TZ_XTSS_SW_RESET);
> >>>>> else
> >>>>> @@ -181,7 +181,7 @@ static int venus_shutdown_no_tz(struct venus_core *core)
> >>>>> void __iomem *wrapper_base = core->wrapper_base;
> >>>>> void __iomem *wrapper_tz_base = core->wrapper_tz_base;
> >>>>>
> >>>>> - if (IS_IRIS2_1(core)) {
> >>>>> + if (IS_IRIS2_1(core) || IS_AR50_LITE(core)) {
> >>>>> /* Assert the reset to XTSS */
> >>>>> reg = readl(wrapper_tz_base + WRAPPER_TZ_XTSS_SW_RESET);
> >>>> No need to handle no-tz case. Pls drop.
> >>>
> >>> ok
> >>>
> >>>>> reg |= WRAPPER_XTSS_SW_RESET_BIT;
> >>>>> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
> >>>>> index 8295542e1a7c..812bec9a05be 100644
> >>>>> --- a/drivers/media/platform/qcom/venus/helpers.c
> >>>>> +++ b/drivers/media/platform/qcom/venus/helpers.c
> >>>>> @@ -230,6 +230,79 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *inst)
> >>>>> }
> >>>>> EXPORT_SYMBOL_GPL(venus_helper_alloc_dpb_bufs);
> >>>>>
> >>>>> +static void free_eos_buf(struct venus_inst *inst, struct intbuf *buf)
> >>>>> +{
> >>>>> + list_del_init(&buf->list);
> >>>>> + dma_free_attrs(inst->core->dev, buf->size, buf->va, buf->da,
> >>>>> + buf->attrs);
> >>>>> + kfree(buf);
> >>>>> +}
> >>>>> +
> >>>>> +int venus_helper_free_eos_bufs(struct venus_inst *inst)
> >>>>> +{
> >>>>> + struct intbuf *buf, *n;
> >>>>> +
> >>>>> + list_for_each_entry_safe(buf, n, &inst->eosbufs, list) {
> >>>>> + free_eos_buf(inst, buf);
> >>>>> + }
> >>>>> +
> >>>>> + if (list_empty(&inst->eosbufs))
> >>>>> + INIT_LIST_HEAD(&inst->eosbufs);
> >>>>> +
> >>>>> + return 0;
> >>>>> +}
> >>>>> +EXPORT_SYMBOL_GPL(venus_helper_free_eos_bufs);
> >>>>> +
> >>>>> +int venus_helper_alloc_eos_buf(struct venus_inst *inst,
> >>>>> + struct hfi_frame_data *data)
> >>>>> +{
> >>>>> + struct venus_core *core = inst->core;
> >>>>> + struct device *dev = core->dev;
> >>>>> + struct intbuf *buf;
> >>>>> + int ret = 0;
> >>>>> +
> >>>>> + memset(data, 0, sizeof(*data));
> >>>>> +
> >>>>> + data->buffer_type = HFI_BUFFER_INPUT;
> >>>>> + data->flags = HFI_BUFFERFLAG_EOS;
> >>>>> +
> >>>>> + if (IS_AR50_LITE(inst->core)) {
> >>>>> + /* We must send valid sizes and addresses */
> >>>>> + buf = kzalloc(sizeof(*buf), GFP_KERNEL);
> >>>>> + if (!buf) {
> >>>>> + ret = -ENOMEM;
> >>>>> + goto fail;
> >>>>> + }
> >>>>> +
> >>>>> + buf->type = HFI_BUFFER_INPUT;
> >>>>> + buf->size = SZ_4K;
> >>>>> + buf->attrs = DMA_ATTR_NO_KERNEL_MAPPING;
> >>>>> + buf->va = dma_alloc_attrs(dev, buf->size, &buf->da, GFP_KERNEL,
> >>>>> + buf->attrs);
> >>>>> + if (!buf->va) {
> >>>>> + ret = -ENOMEM;
> >>>>> + goto fail;
> >>>>> + }
> >>>>> +
> >>>>> + list_add_tail(&buf->list, &inst->eosbufs);
> >>>>> +
> >>>>> + data->alloc_len = buf->size;
> >>>>> + data->device_addr = buf->da;
> >>>>> +
> >>>> why this special handling for eos buffer is needed for AR50_LITE?
> >>>
> >>> this _fix_ was develope through testing: without it there is no EOS and
> >>> errors are reporting upon killing the player
> >>>
> >> Would be better to see why there is no EOS from firmware,
> >> there shouldn't be the need to have a dma allocation for this dummy
> >> buffers, as there is no useful info in the buffer. Having the device
> >> address as 0 or 0xdeadb000 should be enough.
> >>
> >
> > hi dikshita,
> >
> > I am still keeping this on v6 as per our internal discussions and
> > because v6 is quite different from v5 so wanted to provide early access
> > to users.
> >
> > if the firwmare is fixed to address this issue on time, I might revert
> > the EOS page buffer.
> >
> I'd prefer to resolve this via correct EOS handling or gain clarity on why
> AR50_LITE requires special treatment, instead of proceeding with new patch
> sets built around this design.
>
Fully agree.
However this patch is the actual proper implementation - it follows the
HFI spec - while the current code upstream is not.
We should revert over time the current implementation to avoid hitting
issues when the firmware stops checking for things like 0xdeadb000.
Powered by blists - more mailing lists