Message-ID: <3e9277bc-75dd-b408-4ab8-1da4c43d72d0@quicinc.com>
Date: Wed, 13 Nov 2024 18:35:49 +0530
From: Dikshita Agarwal <quic_dikshita@...cinc.com>
To: Hans Verkuil <hverkuil@...all.nl>,
        Vikash Garodia
	<quic_vgarodia@...cinc.com>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        "Mauro Carvalho Chehab" <mchehab@...nel.org>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>
CC: Sebastian Fricke <sebastian.fricke@...labora.com>,
        Bryan O'Donoghue
	<bryan.odonoghue@...aro.org>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Neil Armstrong <neil.armstrong@...aro.org>,
        Nicolas Dufresne
	<nicolas@...fresne.ca>,
        Uwe Kleine-König
	<u.kleine-koenig@...libre.com>,
        Jianhua Lu <lujianhua000@...il.com>, <linux-media@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 09/28] media: iris: implement reqbuf ioctl with
 vb2_queue_setup



On 11/13/2024 6:20 PM, Hans Verkuil wrote:
> On 11/13/24 13:44, Dikshita Agarwal wrote:
>>
>>
>> On 11/13/2024 5:44 PM, Hans Verkuil wrote:
>>> On 11/13/24 12:20, Dikshita Agarwal wrote:
>>>>
>>>>
>>>> On 11/13/2024 4:45 PM, Hans Verkuil wrote:
>>>>> On 11/13/24 11:32, Dikshita Agarwal wrote:
>>>>>>
>>>>>>
>>>>>> On 11/13/2024 2:52 PM, Hans Verkuil wrote:
>>>>>>> On 13/11/2024 10:00, Dikshita Agarwal wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 11/13/2024 1:18 PM, Hans Verkuil wrote:
>>>>>>>>> On 13/11/2024 07:19, Dikshita Agarwal wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 11/12/2024 3:20 PM, Hans Verkuil wrote:
>>>>>>>>>>> On 05/11/2024 07:55, Dikshita Agarwal wrote:
>>>>>>>>>>>> Implement reqbuf IOCTL op and vb2_queue_setup vb2 op in the driver with
>>>>>>>>>>>> necessary hooks.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Dikshita Agarwal <quic_dikshita@...cinc.com>
>>>>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> <snip>
>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/media/platform/qcom/iris/iris_vb2.c b/drivers/media/platform/qcom/iris/iris_vb2.c
>>>>>>>>>>>> new file mode 100644
>>>>>>>>>>>> index 000000000000..61033f95cdba
>>>>>>>>>>>> --- /dev/null
>>>>>>>>>>>> +++ b/drivers/media/platform/qcom/iris/iris_vb2.c
>>>>>>>>>>>> @@ -0,0 +1,74 @@
>>>>>>>>>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>>>>>>>>>> +/*
>>>>>>>>>>>> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>>>>>>>>>>>> + */
>>>>>>>>>>>> +
>>>>>>>>>>>> +#include "iris_buffer.h"
>>>>>>>>>>>> +#include "iris_instance.h"
>>>>>>>>>>>> +#include "iris_vb2.h"
>>>>>>>>>>>> +#include "iris_vpu_buffer.h"
>>>>>>>>>>>> +
>>>>>>>>>>>> +int iris_vb2_queue_setup(struct vb2_queue *q,
>>>>>>>>>>>> +			 unsigned int *num_buffers, unsigned int *num_planes,
>>>>>>>>>>>> +			 unsigned int sizes[], struct device *alloc_devs[])
>>>>>>>>>>>> +{
>>>>>>>>>>>> +	enum iris_buffer_type buffer_type = 0;
>>>>>>>>>>>> +	struct iris_buffers *buffers;
>>>>>>>>>>>> +	struct iris_inst *inst;
>>>>>>>>>>>> +	struct iris_core *core;
>>>>>>>>>>>> +	struct v4l2_format *f;
>>>>>>>>>>>> +	int ret = 0;
>>>>>>>>>>>> +
>>>>>>>>>>>> +	inst = vb2_get_drv_priv(q);
>>>>>>>>>>>> +
>>>>>>>>>>>> +	mutex_lock(&inst->lock);
>>>>>>>>>>>> +
>>>>>>>>>>>> +	core = inst->core;
>>>>>>>>>>>> +	f = V4L2_TYPE_IS_OUTPUT(q->type) ? inst->fmt_src : inst->fmt_dst;
>>>>>>>>>>>> +
>>>>>>>>>>>> +	if (*num_planes) {
>>>>>>>>>>>> +		if (*num_planes != f->fmt.pix_mp.num_planes ||
>>>>>>>>>>>> +			sizes[0] < f->fmt.pix_mp.plane_fmt[0].sizeimage)
>>>>>>>>>>>> +			ret = -EINVAL;
>>>>>>>>>>>> +		goto unlock;
>>>>>>>>>>>> +	}
>>>>>>>>>>>> +
>>>>>>>>>>>> +	buffer_type = iris_v4l2_type_to_driver(q->type);
>>>>>>>>>>>> +	if (buffer_type == -EINVAL) {
>>>>>>>>>>>
>>>>>>>>>>> Can this ever fail?
>>>>>>>>>>>
>>>>>>>>>> If the q->type passed is not supported by driver then it can fail.
>>>>>>>>>
>>>>>>>>> But it is the driver that sets q->type when the vb2_queue is initialized.
>>>>>>>>> So it makes no sense to test it here, it would be a driver bug if this fails.
>>>>>>>>>
>>>>>>>> Ok, Will remove this check.
>>>>>>>>>>>> +		ret = -EINVAL;
>>>>>>>>>>>> +		goto unlock;
>>>>>>>>>>>> +	}
>>>>>>>>>>>> +
>>>>>>>>>>>> +	if (!inst->once_per_session_set) {
>>>>>>>>>>>> +		inst->once_per_session_set = true;
>>>>>>>>>>>> +
>>>>>>>>>>>> +		ret = core->hfi_ops->session_open(inst);
>>>>>>>>>>>> +		if (ret) {
>>>>>>>>>>>> +			ret = -EINVAL;
>>>>>>>>>>>> +			dev_err(core->dev, "session open failed\n");
>>>>>>>>>>>> +			goto unlock;
>>>>>>>>>>>> +		}
>>>>>>>>>>>> +	}
>>>>>>>>>>>> +
>>>>>>>>>>>> +	buffers = &inst->buffers[buffer_type];
>>>>>>>>>>>> +	if (!buffers) {
>>>>>>>>>>>
>>>>>>>>>>> This definitely can never fail.
>>>>>>>>>>>
>>>>>>>>>> Right, will remove the check.
>>>>>>>>>>>> +		ret = -EINVAL;
>>>>>>>>>>>> +		goto unlock;
>>>>>>>>>>>> +	}
>>>>>>>>>>>> +
>>>>>>>>>>>> +	buffers->min_count = iris_vpu_buf_count(inst, buffer_type);
>>>>>>>>>>>> +	buffers->actual_count = *num_buffers;
>>>>>>>>>>>
>>>>>>>>>>> Don't mirror the number of buffers in actual_count, instead just always
>>>>>>>>>>> ask for the number of buffers using vb2_get_num_buffers().
>>>>>>>>>>>
>>>>>>>>>>> This code is wrong anyway, since actual_count isn't updated if more
>>>>>>>>>>> buffers are added using VIDIOC_CREATEBUFS.
>>>>>>>>>>>
>>>>>>>>>> Ok, so below would fix the VIDIOC_CREATEBUFS as well, right?
>>>>>>>>>> - buffers->actual_count = *num_buffers;
>>>>>>>>>> + buffers->actual_count = vb2_get_num_buffers();
>>>>>>>> Does this look good?
>>>>>>>
>>>>>>> No. You shouldn't have the actual_count field at all, especially since I see that
>>>>>>> you set it in several places. vb2_get_num_buffers() reports the current number of
>>>>>>> buffers, which can change if userspace calls VIDIOC_CREATE_BUFS or REMOVE_BUFS.
>>>>>>>
>>>>>>> You shouldn't try to mirror that value yourself. If you need that information,
>>>>>>> then call vb2_get_num_buffers().
>>>>>>>
>>>>>>> There are weird things going on in your driver w.r.t. actual_count and also min_count
>>>>>>> (and I saw a count_actual as well, very confusing).
>>>>>>>
>>>>>>> I'm not sure what you are trying to achieve, but it doesn't look right.
>>>>>>>
>>>>>> We need to set the value of actual buffers being queued to firmware via a
>>>>>> property, for that we are caching the value in actual_count so that we can
>>>>>> set it to fw when needed.
>>>>>
>>>>> So do you need to know the number of allocated buffers, or the number of
>>>>> buffers queued to the device instance?
>>>>>
>>>>> The first is reported by vb2_get_num_buffers(), the second is something
>>>>> you can keep track of yourself: a buffer is queued in the buf_queue op and
>>>>> dequeued when vb2_buffer_done is called. But this has nothing to do with
>>>>> what happens in queue_setup.
>>>>>
>>>> We need to know the number of allocated buffers, hence using
>>>> vb2_get_num_buffers() is fine as you said.
>>>
>>> Why do you need this? Are the buffer addresses also passed to the fw?
>>>
>>> Remember that buffer memory is only allocated when using V4L2_MEMORY_MMAP.
>>> In the DMABUF case it just allocates vb2_buffer structs, not the actual
>>> buffer memory. So a buffer can be dequeued and the corresponding dmabuf
>>> closed (so the memory is freed) by the application.
>>>
>>> In other words, vb2_get_num_buffers() reports the number of allocated
>>> vb2_buffer structs, but not the actual number of buffers in memory, that
>>> might be different in the DMABUF case.
>>>
>>> What exactly is the firmware using this number for? What does it expect
>>> it contains?
>>>
>> Firmware uses this number to create its internal buffer pool of
>> input/output buffers. It should be the maximum numbers of buffers that can
>> be queued by client, which is same as the number of allocated buffers.
>> it doesn't need to match the number of buffers actually being queued.
> 
> Since with CREATE_BUFS userspace can add buffers on the fly, and with
> REMOVE_BUFS it can remove buffers on the fly, the number of buffers can
> fluctuate. Can the fw be updated whenever that number changes?
> 
> And if the fw just needs the max, can't you just set it to VIDEO_MAX_FRAME
> and be done with it? Or does the fw allocate a lot of internal memory for
> each buffer, so you want it to be a precise number?
> 
yeah, it should be fine to set it to VIDEO_MAX_FRAME.
Will make the respective changes and remove setting min_count or
actual_count in queue_setup.

Thanks,
Dikshita
> Regards,
> 
> 	Hans
> 
>>
>> Thanks,
>> Dikshita
>>> Regards,
>>>
>>> 	Hans
>>>
>>>>
>>>> But would want to cache this in internal buffer strcuture in queue_setup,
>>>> to be able to use later while setting to firmware.
>>>>
>>>> Thanks,
>>>> Dikshita
>>>>> Regards,
>>>>>
>>>>> 	Hans
>>>>>
>>>>>>
>>>>>> count_actual is the variable of the hfi struture being filled to set the
>>>>>> property to fw,
>>>>>> ---
>>>>>> u32 ptype = HFI_PROPERTY_PARAM_BUFFER_COUNT_ACTUAL;
>>>>>> struct hfi_buffer_count_actual buf_count;
>>>>>> int ret;
>>>>>>
>>>>>> buf_count.type = HFI_BUFFER_INPUT;
>>>>>> buf_count.count_actual = inst->buffers[BUF_INPUT].actual_count;
>>>>>> ---
>>>>>>
>>>>>> Calling vb2_get_num_buffers from HFI layer will violate the current design
>>>>>> of driver so will need to cache this info in upper layer, best place to do
>>>>>> that seems to be queue_setup which is called from both VIDIOC_REQBUFS and
>>>>>> VIDIOC_CREATE_BUFS.
>>>>>> Any other suggestions for the same?
>>>>>>
>>>>>> To avoid the confusion, I can rename the actual_count to count_actual to
>>>>>> match with hfi structure.
>>>>>> Also, I can cleanup some part of driver where this variable is being
>>>>>> updated un-necessarily.
>>>>>> This is only needed to set the property to firmware as explained above.
>>>>>>
>>>>>> min_count holds the min numbers of buffer needed by firmware for the
>>>>>> particluar session, it can be changed by firmware if source changes.
>>>>>>
>>>>>> Thanks,
>>>>>> Dikshita
>>>>>>> Regards,
>>>>>>>
>>>>>>> 	Hans
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Dikshita
>>>
> 

Powered by blists - more mailing lists