Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: mansur=codeaurora.org@mg.codeaurora.org
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Tue, 28 Sep 2021 09:55:38 +0530
From:   mansur@codeaurora.org
To:     Stanimir Varbanov <stanimir.varbanov@linaro.org>
Cc:     linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, vgarodia@codeaurora.org,
        dikshita@codeaurora.org
Subject: Re: [V3] venus: vdec: decoded picture buffer handling during reconfig
 sequence
In-Reply-To: <78dec463-5e75-18d7-b74e-154f00b8a7b2@linaro.org>
References: <20210825110841.12815-1-mansur@codeaurora.org>
 <78dec463-5e75-18d7-b74e-154f00b8a7b2@linaro.org>
Message-ID: <4db580aea0ddfc6092fd86b51e67802f@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Precedence: bulk

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@codeaurora.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