| Message ID | 20250408-iris-dec-hevc-vp9-v1-1-acd258778bd6@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for HEVC and VP9 codecs in decoder | expand |
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@vger.kernel.org > Fixes: 73702f45db81 ("media: iris: allocate, initialize and queue internal buffers") > Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> > --- > 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 ? --- bod
On 15/04/2025 05:58, Dikshita Agarwal wrote: > 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. Shouldn't we reset iris in this case ? i.e. its a breaking of the software contract to have failed to have returned a buffer by - close. Its not enough to free the memory on the APSS side as the remote end could still assume ownership of a buffer... right ? --- bod
On 4/16/2025 5:40 PM, Bryan O'Donoghue wrote: > On 15/04/2025 05:58, Dikshita Agarwal wrote: >> 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. > > Shouldn't we reset iris in this case ? > Not required. > i.e. its a breaking of the software contract to have failed to have > returned a buffer by - close. > > Its not enough to free the memory on the APSS side as the remote end could > still assume ownership of a buffer... right ? > Before close, Stop will be called to firmware and firmware will return all the buffers to driver, which will transfer the ownership to driver, so no issue with freeing these buffers in close. Thanks, Dikshita > --- > bod
On 16/04/2025 17:40, Dikshita Agarwal wrote: > > On 4/16/2025 5:40 PM, Bryan O'Donoghue wrote: >> On 15/04/2025 05:58, Dikshita Agarwal wrote: >>> 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. >> Shouldn't we reset iris in this case ? >> > Not required. OK sure. Could you at least add an error message on close() if any buffer is not released ? That way we can "trust but verify". What makes me suspicious is that we have one instance where a buffer hasn't been released which we expected to have been released - that may be reasons for that which we can't interrogate from APSS - fine but, then how can we be sure the software contract on close() is respected ? So yes, I accept what you say that its not required but for peace of mind we should at the very least be noisy on close() about unreleased buffers and if we start to see kernel logs about unreleased bufs we should revisit resetting firmware. --- bod
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;
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@vger.kernel.org Fixes: 73702f45db81 ("media: iris: allocate, initialize and queue internal buffers") Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> --- drivers/media/platform/qcom/iris/iris_buffer.c | 7 +++++++ 1 file changed, 7 insertions(+)