| 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: <9f09bc40e52021f89b29cfaef8e314d8@codeaurora.org>
Date: Mon, 27 Sep 2021 10:54:57 +0530
From: mansur@...eaurora.org
To: undisclosed-recipients:;
Subject: Re: [V3] venus: vdec: decoded picture buffer handling during reconfig
sequence
> On 2021-09-21 19:17, Stanimir Varbanov wrote:
>> Hi Mansur,
>>
>> On 8/25/21 2:08 PM, Mansur Alisha Shaik wrote:
>>> In existing implementation, driver is freeing and un-mapping all the
>>> decoded picture buffers(DPB) as part of dynamic resolution
>>> change(DRC)
>>> handling. As a result, when firmware try to access the DPB buffer,
>>> from
>>> previous sequence, SMMU context fault is seen due to the buffer being
>>> already unmapped.
>>>
>>> With this change, driver defines ownership of each DPB buffer. If a
>>> buffer
>>> is owned by firmware, driver would skip from un-mapping the same.
>>>
>>> Signed-off-by: Mansur Alisha Shaik <mansur@...eaurora.org>
>>>
>>> Changes in V3:
>>> - Migrated id allocation using kernel API ida_alloc_min()
>>>
>>> ---
>>> drivers/media/platform/qcom/venus/helpers.c | 50
>>> ++++++++++++++++++++-
>>> drivers/media/platform/qcom/venus/helpers.h | 2 +
>>> drivers/media/platform/qcom/venus/vdec.c | 7 ++-
>>> 3 files changed, 57 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/qcom/venus/helpers.c
>>> b/drivers/media/platform/qcom/venus/helpers.c
>>> index 8012f5c7bf34..f36361d346b2 100644
>>> --- a/drivers/media/platform/qcom/venus/helpers.c
>>> +++ b/drivers/media/platform/qcom/venus/helpers.c
>>> @@ -3,6 +3,7 @@
>>> * Copyright (c) 2012-2016, The Linux Foundation. All rights
>>> reserved.
>>> * Copyright (C) 2017 Linaro Ltd.
>>> */
>>> +#include <linux/idr.h>
>>> #include <linux/list.h>
>>> #include <linux/mutex.h>
>>> #include <linux/slab.h>
>>> @@ -21,6 +22,13 @@
>>> #define NUM_MBS_720P (((1280 + 15) >> 4) * ((720 + 15) >> 4))
>>> #define NUM_MBS_4K (((4096 + 15) >> 4) * ((2304 + 15) >> 4))
>>>
>>> +static DEFINE_IDA(dpb_out_tag_ida);
>>
>> No global static variables please. Make it part of venus_inst
>> structure.
> As per my understanding it is not just static global variable.
> We are defining the ida structure and assign to name when pass as
> param as follows
> struct ida {
> struct idr idr;
> struct ida_bitmap *free_bitmap;
> };
> #define IDA_INIT(name) { .idr = IDR_INIT((name).idr), .free_bitmap =
> NULL, }
> #define DEFINE_IDA(name) struct ida name = IDA_INIT(name)
>
> Any ida related API's expect pointer to this structure.
> If we move the variable then it might be bit difficult use ida_xxx()
> API'same
>>> +
>>> +enum dpb_buf_owner {
>>> + DRIVER,
>>> + FIRMWARE,
>>> +};
>>> +
>>> struct intbuf {
>>> struct list_head list;
>>> u32 type;
>>> @@ -28,6 +36,8 @@ struct intbuf {
>>> void *va;
>>> dma_addr_t da;
>>> unsigned long attrs;
>>> + enum dpb_buf_owner owned_by;
>>> + u32 dpb_out_tag;
>>> };
>>>
>>> bool venus_helper_check_codec(struct venus_inst *inst, u32
>>> v4l2_pixfmt)
>>> @@ -95,9 +105,16 @@ int venus_helper_queue_dpb_bufs(struct venus_inst
>>> *inst)
>>> fdata.device_addr = buf->da;
>>> fdata.buffer_type = buf->type;
>>>
>>> + if (buf->owned_by == FIRMWARE)
>>> + continue;
>>> +
>>> + fdata.clnt_data = buf->dpb_out_tag;
>>> +
>>> ret = hfi_session_process_buf(inst, &fdata);
>>> if (ret)
>>> goto fail;
>>> +
>>> + buf->owned_by = FIRMWARE;
>>> }
>>>
>>> fail:
>>> @@ -110,13 +127,19 @@ int venus_helper_free_dpb_bufs(struct
>>> venus_inst *inst)
>>> struct intbuf *buf, *n;
>>>
>>> list_for_each_entry_safe(buf, n, &inst->dpbbufs, list) {
>>> + if (buf->owned_by == FIRMWARE)
>>> + continue;
>>> +
>>> + ida_free(&dpb_out_tag_ida, buf->dpb_out_tag);
>>> +
>>> list_del_init(&buf->list);
>>> dma_free_attrs(inst->core->dev, buf->size, buf->va, buf->da,
>>> buf->attrs);
>>> kfree(buf);
>>> }
>>>
>>> - INIT_LIST_HEAD(&inst->dpbbufs);
>>> + if (list_empty(&inst->dpbbufs))
>>> + INIT_LIST_HEAD(&inst->dpbbufs);
>>>
>>> return 0;
>>> }
>>> @@ -134,6 +157,7 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst
>>> *inst)
>>> unsigned int i;
>>> u32 count;
>>> int ret;
>>> + int id;
>>>
>>> /* no need to allocate dpb buffers */
>>> if (!inst->dpb_fmt)
>>> @@ -171,6 +195,15 @@ int venus_helper_alloc_dpb_bufs(struct
>>> venus_inst *inst)
>>> ret = -ENOMEM;
>>> goto fail;
>>> }
>>> + buf->owned_by = DRIVER;
>>> +
>>> + id = ida_alloc_min(&dpb_out_tag_ida, VB2_MAX_FRAME, GFP_KERNEL);
>>> + if (id < 0) {
>>> + ret = id;
>>> + goto fail;
>>> + }
>>> +
>>> + buf->dpb_out_tag = id;
>>>
>>> list_add_tail(&buf->list, &inst->dpbbufs);
>>> }
>>> @@ -1365,6 +1398,21 @@ venus_helper_find_buf(struct venus_inst *inst,
>>> unsigned int type, u32 idx)
>>> }
>>> EXPORT_SYMBOL_GPL(venus_helper_find_buf);
>>>
>>> +void venus_helper_find_dpb_buf(struct venus_inst *inst, struct
>>> vb2_v4l2_buffer *vbuf,
>>> + unsigned int type, unsigned int buf_type, u32 tag)
>>
>> If this helper will return void then it should be renamed to something
>> like venus_helper_dpb_buf_change_owner().
> Ok.
>>> +{
>>> + struct intbuf *dpb_buf;
>>> +
>>> + if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE &&
>>> + buf_type == HFI_BUFFER_OUTPUT)
>>> + list_for_each_entry(dpb_buf, &inst->dpbbufs, list)
>>> + if (dpb_buf->dpb_out_tag == tag) {
>>> + dpb_buf->owned_by = DRIVER;
>>> + break;
>>> + }
>>> +}
>>> +EXPORT_SYMBOL_GPL(venus_helper_find_dpb_buf);
>>> +
>>> int venus_helper_vb2_buf_init(struct vb2_buffer *vb)
>>> {
>>> struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue);
>>> diff --git a/drivers/media/platform/qcom/venus/helpers.h
>>> b/drivers/media/platform/qcom/venus/helpers.h
>>> index e6269b4be3af..17c5aadaec82 100644
>>> --- a/drivers/media/platform/qcom/venus/helpers.h
>>> +++ b/drivers/media/platform/qcom/venus/helpers.h
>>> @@ -14,6 +14,8 @@ struct venus_core;
>>> bool venus_helper_check_codec(struct venus_inst *inst, u32
>>> v4l2_pixfmt);
>>> struct vb2_v4l2_buffer *venus_helper_find_buf(struct venus_inst
>>> *inst,
>>> unsigned int type, u32 idx);
>>> +void venus_helper_find_dpb_buf(struct venus_inst *inst, struct
>>> vb2_v4l2_buffer *vbuf,
>>> + unsigned int type, unsigned int buf_type, u32 idx);
>>> void venus_helper_buffers_done(struct venus_inst *inst, unsigned int
>>> type,
>>> enum vb2_buffer_state state);
>>> int venus_helper_vb2_buf_init(struct vb2_buffer *vb);
>>> diff --git a/drivers/media/platform/qcom/venus/vdec.c
>>> b/drivers/media/platform/qcom/venus/vdec.c
>>> index 198e47eb63f4..cafdc3d8e473 100644
>>> --- a/drivers/media/platform/qcom/venus/vdec.c
>>> +++ b/drivers/media/platform/qcom/venus/vdec.c
>>> @@ -1297,6 +1297,7 @@ static void vdec_buf_done(struct venus_inst
>>> *inst, unsigned int buf_type,
>>> struct vb2_v4l2_buffer *vbuf;
>>> struct vb2_buffer *vb;
>>> unsigned int type;
>>> + struct intbuf *dpb_buf;
>>>
>>> vdec_pm_touch(inst);
>>>
>>> @@ -1306,8 +1307,10 @@ static void vdec_buf_done(struct venus_inst
>>> *inst, unsigned int buf_type,
>>> type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
>>>
>>> vbuf = venus_helper_find_buf(inst, type, tag);
>>> - if (!vbuf)
>>> + if (!vbuf) {
>>> + venus_helper_find_dpb_buf(inst, vbuf, type, buf_type, tag);
>>> return;
>>> + }
>>>
>>> vbuf->flags = flags;
>>> vbuf->field = V4L2_FIELD_NONE;
>>> @@ -1622,6 +1625,8 @@ static int vdec_close(struct file *file)
>>>
>>> vdec_pm_get(inst);
>>>
>>> + venus_helper_free_dpb_bufs(inst);
>>> + INIT_LIST_HEAD(&inst->dpbbufs);
>>
>> This belongs to venus_helper_free_dpb_bufs not here.
> Ok, I will remove it from here, since in free_dpb() api
> INIT_LIST_HEAD() when list is free.
>>> v4l2_m2m_ctx_release(inst->m2m_ctx);
>>> v4l2_m2m_release(inst->m2m_dev);
>>> vdec_ctrl_deinit(inst);
>>>
>>
>> --
>> regards,
>> Stan
>
> --
> Thanks,
> Mansur
Powered by blists - more mailing lists