| 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: <55125121-5349-3b8b-2e81-29eec95d8337@quicinc.com>
Date: Thu, 10 Jul 2025 14:25:33 +0530
From: Dikshita Agarwal <quic_dikshita@...cinc.com>
To: Jorge Ramirez <jorge.ramirez@....qualcomm.com>
CC: <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 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.
Thanks,
Dikshita
> TIA
> jorge
Powered by blists - more mailing lists