Re: [PATCH 01/20] media: iris: Skip destroying internal buffer if not dequeued

[Dikshita Agarwal]

On 4/14/2025 3:56 PM, Bryan O'Donoghue wrote:
> On 11/04/2025 13:10, Bryan O'Donoghue wrote:
>> On 08/04/2025 16:54, Dikshita Agarwal wrote:
>>> Firmware might hold the DPB buffers for reference in case of sequence
>>> change, so skip destroying buffers for which QUEUED flag is not removed.
>>>
>>> Cc: stable@xxxxxxxxxxxxxxx
>>> Fixes: 73702f45db81 ("media: iris: allocate, initialize and queue
>>> internal buffers")
>>> Signed-off-by: Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx>
>>> ---
>>>   drivers/media/platform/qcom/iris/iris_buffer.c | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/qcom/iris/iris_buffer.c b/drivers/
>>> media/platform/qcom/iris/iris_buffer.c
>>> index e5c5a564fcb8..75fe63cc2327 100644
>>> --- a/drivers/media/platform/qcom/iris/iris_buffer.c
>>> +++ b/drivers/media/platform/qcom/iris/iris_buffer.c
>>> @@ -396,6 +396,13 @@ int iris_destroy_internal_buffers(struct iris_inst
>>> *inst, u32 plane)
>>>       for (i = 0; i < len; i++) {
>>>           buffers = &inst->buffers[internal_buf_type[i]];
>>>           list_for_each_entry_safe(buf, next, &buffers->list, list) {
>>> +            /*
>>> +             * skip destroying internal(DPB) buffer if firmware
>>> +             * did not return it.
>>> +             */
>>> +            if (buf->attr & BUF_ATTR_QUEUED)
>>> +                continue;
>>> +
>>>               ret = iris_destroy_internal_buffer(inst, buf);
>>>               if (ret)
>>>                   return ret;
>>>
>>
>> iris_destroy_internal_buffers() is called from
>>
>> - iris_vdec_streamon_output
>> - iris_venc_streamon_output
>> - iris_close
>>
>> So if we skip releasing the buffer here, when will the memory be released ?
>>
>> Particularly the kfree() in iris_destroy_internal_buffer() ?
>>
>> iris_close -> iris_destroy_internal_buffers ! -> iris_destroy_buffer
>>
>> Is a leak right ?
Good catch, Thanks!
>>
>> ---
>> bod
> 
> Thinking about this some more, I believe we should have some sort of
> reaping routine.
> 
> - The firmware fails to release a buffer, it is up to APSS/Linux
>   to run some kind of reaping routine.
>   We can debate when is the right time to reset.
>   Perhaps instead of ignoring the buffer as you have done here
>   we schedule work with a timeout and if the timeout expires then
>   this triggers a reset/reap routine.
> 
> - Since Linux allocates a buffer on the APSS side, you can't have a
>   situation where firmware can indefinitely hold memory.
> 
> - APSS is in effect the bus master here since it can assert/deassert
>   RESET lines to the firmware, can control regulators and clocks.
> 
> So we should have some kind of watchdog logic here.
> 
> As alluded to above, what exactly do you do if firmware never returns a
> buffer ? Accept memory leak on the APSS side ?
> 
> Rather we should agree when it is appropriate to run a watchdog routine to
> 
> 1. Timeout firmware not returning a buffer
> 2. Put the iris/venus hardware into reset
> 3. Reap leaked memory
> 4. Restart
> 
> I see we have IRQ based watchdog logic but, I don't see that it reaps memory.
> 
> In any case we should have the ability to reset iris and reclaim/reap
> memory in this type of situation.
> 
> Perhaps I'm off on a rant here but, this seems like a problem we should
> address with a more comprehensive solution.
> 
That's right, but we don't need to over complicate this.

We need to skip destroying these buffers for a running session so that the
DPB buffers which are still being referenced by firmware are not lost.
But these should be freed during session close which is missing in current
code.
Although firmware makes sure that during session close, all buffers are
returned to driver and driver will release them but still we shouldn't rely
for this on firmware and should handle in driver.
Will fix this in next patch set.

Thanks,
Dikshita
> ---
> bod