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@xxxxxxxxxxx>
>>>>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> <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
>>>
>
