| 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
| ||
|
Date: Tue, 28 Sep 2021 09:55:38 +0530
From: mansur@...eaurora.org
To: Stanimir Varbanov <stanimir.varbanov@...aro.org>
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org, vgarodia@...eaurora.org,
dikshita@...eaurora.org
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. I will change while posting next version.
>> +{
>> + 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