| Message ID | 20250428-qcom-iris-hevc-vp9-v2-1-3a6013ecb8a5@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for HEVC and VP9 codecs in decoder | expand |
On 4/28/2025 2:58 PM, 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. > Also, make sure that all buffers are released during streamoff. > > 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 | 37 +++++++++++++++++++++++++- > drivers/media/platform/qcom/iris/iris_buffer.h | 3 ++- > drivers/media/platform/qcom/iris/iris_vdec.c | 4 +-- > drivers/media/platform/qcom/iris/iris_vidc.c | 6 +++-- > 4 files changed, 44 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/platform/qcom/iris/iris_buffer.c b/drivers/media/platform/qcom/iris/iris_buffer.c > index e5c5a564fcb8..606d76b10be2 100644 > --- a/drivers/media/platform/qcom/iris/iris_buffer.c > +++ b/drivers/media/platform/qcom/iris/iris_buffer.c > @@ -376,7 +376,7 @@ int iris_destroy_internal_buffer(struct iris_inst *inst, struct iris_buffer *buf > return 0; > } > > -int iris_destroy_internal_buffers(struct iris_inst *inst, u32 plane) > +int iris_destroy_internal_buffers(struct iris_inst *inst, u32 plane, bool force) > { > const struct iris_platform_data *platform_data = inst->core->iris_platform_data; > struct iris_buffer *buf, *next; > @@ -396,6 +396,14 @@ 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) { > + /* > + * during stream on, skip destroying internal(DPB) buffer > + * if firmware did not return it. > + * during close, destroy all buffers irrespectively. > + */ > + if (!force && buf->attr & BUF_ATTR_QUEUED) > + continue; > + > ret = iris_destroy_internal_buffer(inst, buf); > if (ret) > return ret; > @@ -446,6 +454,33 @@ static int iris_release_input_internal_buffers(struct iris_inst *inst) > return 0; > } > > +void iris_get_num_queued_internal_buffers(struct iris_inst *inst, u32 plane) name this iris_check_num_queued_internal_buffers.. > +{ > + const struct iris_platform_data *platform_data = inst->core->iris_platform_data; > + struct iris_buffer *buf, *next; > + struct iris_buffers *buffers; > + const u32 *internal_buf_type; > + u32 internal_buffer_count, i; > + u32 count = 0; > + > + if (V4L2_TYPE_IS_OUTPUT(plane)) { > + internal_buf_type = platform_data->dec_ip_int_buf_tbl; > + internal_buffer_count = platform_data->dec_ip_int_buf_tbl_size; > + } else { > + internal_buf_type = platform_data->dec_op_int_buf_tbl; > + internal_buffer_count = platform_data->dec_op_int_buf_tbl_size; > + } > + > + for (i = 0; i < internal_buffer_count; i++) { > + buffers = &inst->buffers[internal_buf_type[i]]; > + list_for_each_entry_safe(buf, next, &buffers->list, list) > + count++; > + if (count) > + dev_err(inst->core->dev, "%d buffer of type %d not released", > + count, internal_buf_type[i]); > + } > +} > + > int iris_alloc_and_queue_persist_bufs(struct iris_inst *inst) > { > struct iris_buffers *buffers = &inst->buffers[BUF_PERSIST]; > diff --git a/drivers/media/platform/qcom/iris/iris_buffer.h b/drivers/media/platform/qcom/iris/iris_buffer.h > index c36b6347b077..03a32b91cf21 100644 > --- a/drivers/media/platform/qcom/iris/iris_buffer.h > +++ b/drivers/media/platform/qcom/iris/iris_buffer.h > @@ -106,7 +106,8 @@ void iris_get_internal_buffers(struct iris_inst *inst, u32 plane); > int iris_create_internal_buffers(struct iris_inst *inst, u32 plane); > int iris_queue_internal_buffers(struct iris_inst *inst, u32 plane); > int iris_destroy_internal_buffer(struct iris_inst *inst, struct iris_buffer *buffer); > -int iris_destroy_internal_buffers(struct iris_inst *inst, u32 plane); > +int iris_destroy_internal_buffers(struct iris_inst *inst, u32 plane, bool force); > +void iris_get_num_queued_internal_buffers(struct iris_inst *inst, u32 plane); make this static Regards, Vikash > int iris_alloc_and_queue_persist_bufs(struct iris_inst *inst); > int iris_alloc_and_queue_input_int_bufs(struct iris_inst *inst); > int iris_queue_buffer(struct iris_inst *inst, struct iris_buffer *buf); > diff --git a/drivers/media/platform/qcom/iris/iris_vdec.c b/drivers/media/platform/qcom/iris/iris_vdec.c > index 4143acedfc57..2c1a7162d2da 100644 > --- a/drivers/media/platform/qcom/iris/iris_vdec.c > +++ b/drivers/media/platform/qcom/iris/iris_vdec.c > @@ -408,7 +408,7 @@ int iris_vdec_streamon_input(struct iris_inst *inst) > > iris_get_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE); > > - ret = iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE); > + ret = iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, false); > if (ret) > return ret; > > @@ -496,7 +496,7 @@ int iris_vdec_streamon_output(struct iris_inst *inst) > > iris_get_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); > > - ret = iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); > + ret = iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE, false); > if (ret) > return ret; > > diff --git a/drivers/media/platform/qcom/iris/iris_vidc.c b/drivers/media/platform/qcom/iris/iris_vidc.c > index ca0f4e310f77..56531a7f0dfe 100644 > --- a/drivers/media/platform/qcom/iris/iris_vidc.c > +++ b/drivers/media/platform/qcom/iris/iris_vidc.c > @@ -233,8 +233,10 @@ int iris_close(struct file *filp) > iris_session_close(inst); > iris_inst_change_state(inst, IRIS_INST_DEINIT); > iris_v4l2_fh_deinit(inst); > - iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE); > - iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); > + iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, true); > + iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE, true); > + iris_get_num_queued_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE); > + iris_get_num_queued_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); > iris_remove_session(inst); > mutex_unlock(&inst->lock); > mutex_destroy(&inst->ctx_q_lock); >
On 4/29/2025 2:54 PM, Vikash Garodia wrote: > > On 4/28/2025 2:58 PM, 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. >> Also, make sure that all buffers are released during streamoff. >> >> 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 | 37 +++++++++++++++++++++++++- >> drivers/media/platform/qcom/iris/iris_buffer.h | 3 ++- >> drivers/media/platform/qcom/iris/iris_vdec.c | 4 +-- >> drivers/media/platform/qcom/iris/iris_vidc.c | 6 +++-- >> 4 files changed, 44 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/iris/iris_buffer.c b/drivers/media/platform/qcom/iris/iris_buffer.c >> index e5c5a564fcb8..606d76b10be2 100644 >> --- a/drivers/media/platform/qcom/iris/iris_buffer.c >> +++ b/drivers/media/platform/qcom/iris/iris_buffer.c >> @@ -376,7 +376,7 @@ int iris_destroy_internal_buffer(struct iris_inst *inst, struct iris_buffer *buf >> return 0; >> } >> >> -int iris_destroy_internal_buffers(struct iris_inst *inst, u32 plane) >> +int iris_destroy_internal_buffers(struct iris_inst *inst, u32 plane, bool force) >> { >> const struct iris_platform_data *platform_data = inst->core->iris_platform_data; >> struct iris_buffer *buf, *next; >> @@ -396,6 +396,14 @@ 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) { >> + /* >> + * during stream on, skip destroying internal(DPB) buffer >> + * if firmware did not return it. >> + * during close, destroy all buffers irrespectively. >> + */ >> + if (!force && buf->attr & BUF_ATTR_QUEUED) >> + continue; >> + >> ret = iris_destroy_internal_buffer(inst, buf); >> if (ret) >> return ret; >> @@ -446,6 +454,33 @@ static int iris_release_input_internal_buffers(struct iris_inst *inst) >> return 0; >> } >> >> +void iris_get_num_queued_internal_buffers(struct iris_inst *inst, u32 plane) > name this iris_check_num_queued_internal_buffers.. > >> +{ >> + const struct iris_platform_data *platform_data = inst->core->iris_platform_data; >> + struct iris_buffer *buf, *next; >> + struct iris_buffers *buffers; >> + const u32 *internal_buf_type; >> + u32 internal_buffer_count, i; >> + u32 count = 0; >> + >> + if (V4L2_TYPE_IS_OUTPUT(plane)) { >> + internal_buf_type = platform_data->dec_ip_int_buf_tbl; >> + internal_buffer_count = platform_data->dec_ip_int_buf_tbl_size; >> + } else { >> + internal_buf_type = platform_data->dec_op_int_buf_tbl; >> + internal_buffer_count = platform_data->dec_op_int_buf_tbl_size; >> + } >> + >> + for (i = 0; i < internal_buffer_count; i++) { >> + buffers = &inst->buffers[internal_buf_type[i]]; >> + list_for_each_entry_safe(buf, next, &buffers->list, list) >> + count++; >> + if (count) >> + dev_err(inst->core->dev, "%d buffer of type %d not released", >> + count, internal_buf_type[i]); >> + } >> +} >> + >> int iris_alloc_and_queue_persist_bufs(struct iris_inst *inst) >> { >> struct iris_buffers *buffers = &inst->buffers[BUF_PERSIST]; >> diff --git a/drivers/media/platform/qcom/iris/iris_buffer.h b/drivers/media/platform/qcom/iris/iris_buffer.h >> index c36b6347b077..03a32b91cf21 100644 >> --- a/drivers/media/platform/qcom/iris/iris_buffer.h >> +++ b/drivers/media/platform/qcom/iris/iris_buffer.h >> @@ -106,7 +106,8 @@ void iris_get_internal_buffers(struct iris_inst *inst, u32 plane); >> int iris_create_internal_buffers(struct iris_inst *inst, u32 plane); >> int iris_queue_internal_buffers(struct iris_inst *inst, u32 plane); >> int iris_destroy_internal_buffer(struct iris_inst *inst, struct iris_buffer *buffer); >> -int iris_destroy_internal_buffers(struct iris_inst *inst, u32 plane); >> +int iris_destroy_internal_buffers(struct iris_inst *inst, u32 plane, bool force); >> +void iris_get_num_queued_internal_buffers(struct iris_inst *inst, u32 plane); > make this static With the above changes, you can mark it Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com> > > Regards, > Vikash >> int iris_alloc_and_queue_persist_bufs(struct iris_inst *inst); >> int iris_alloc_and_queue_input_int_bufs(struct iris_inst *inst); >> int iris_queue_buffer(struct iris_inst *inst, struct iris_buffer *buf); >> diff --git a/drivers/media/platform/qcom/iris/iris_vdec.c b/drivers/media/platform/qcom/iris/iris_vdec.c >> index 4143acedfc57..2c1a7162d2da 100644 >> --- a/drivers/media/platform/qcom/iris/iris_vdec.c >> +++ b/drivers/media/platform/qcom/iris/iris_vdec.c >> @@ -408,7 +408,7 @@ int iris_vdec_streamon_input(struct iris_inst *inst) >> >> iris_get_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE); >> >> - ret = iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE); >> + ret = iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, false); >> if (ret) >> return ret; >> >> @@ -496,7 +496,7 @@ int iris_vdec_streamon_output(struct iris_inst *inst) >> >> iris_get_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); >> >> - ret = iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); >> + ret = iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE, false); >> if (ret) >> return ret; >> >> diff --git a/drivers/media/platform/qcom/iris/iris_vidc.c b/drivers/media/platform/qcom/iris/iris_vidc.c >> index ca0f4e310f77..56531a7f0dfe 100644 >> --- a/drivers/media/platform/qcom/iris/iris_vidc.c >> +++ b/drivers/media/platform/qcom/iris/iris_vidc.c >> @@ -233,8 +233,10 @@ int iris_close(struct file *filp) >> iris_session_close(inst); >> iris_inst_change_state(inst, IRIS_INST_DEINIT); >> iris_v4l2_fh_deinit(inst); >> - iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE); >> - iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); >> + iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, true); >> + iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE, true); >> + iris_get_num_queued_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE); >> + iris_get_num_queued_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); >> iris_remove_session(inst); >> mutex_unlock(&inst->lock); >> mutex_destroy(&inst->ctx_q_lock); >>
On 28/04/2025 10:28, Dikshita Agarwal wrote: > + /* > + * during stream on, skip destroying internal(DPB) buffer > + * if firmware did not return it. > + * during close, destroy all buffers irrespectively. > + */ > + if (!force && buf->attr & BUF_ATTR_QUEUED) > + continue; > + What's the effect of the firmware not having dequeued the buffer though ? My main concern here is APSS and firmware have a different view of DMA memory. We release on the APSS side but firmware has not. Surely failure to release buffers by the time we get to Linux::close() is a failure of the software contract sufficient to require resetting the firmware ? i.e. we release memory on the APSS side but firmware writes into it anyway ... ? --- bod
On 4/29/2025 3:13 PM, Bryan O'Donoghue wrote: > On 28/04/2025 10:28, Dikshita Agarwal wrote: >> + /* >> + * during stream on, skip destroying internal(DPB) buffer >> + * if firmware did not return it. >> + * during close, destroy all buffers irrespectively. >> + */ >> + if (!force && buf->attr & BUF_ATTR_QUEUED) >> + continue; >> + > > What's the effect of the firmware not having dequeued the buffer though ? > > My main concern here is APSS and firmware have a different view of DMA memory. > > We release on the APSS side but firmware has not. > > Surely failure to release buffers by the time we get to Linux::close() is a > failure of the software contract sufficient to require resetting the > firmware ? > > i.e. we release memory on the APSS side but firmware writes into it anyway ... > As I mentioned earlier as well, during STOP issued before close, firmware is expected to release all these internal buffers and the check I added now in this patch will also ensures that. Firmware also has a check to make sure all buffers queued are released as part of STOP and then only firmware sends the STOP_DONE response to driver. STOP is a synchronous call and driver doesn't proceed without receiving the response for the same from firmware. So we will never run into this issue where driver release buffers which are still being accessed by firmware. Please note, these are internal buffers which are allocated and managed by driver only. Thanks, Dikshita > ? > > --- > bod
On 4/29/2025 2:54 PM, Vikash Garodia wrote: > > On 4/28/2025 2:58 PM, 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. >> Also, make sure that all buffers are released during streamoff. >> >> 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 | 37 +++++++++++++++++++++++++- >> drivers/media/platform/qcom/iris/iris_buffer.h | 3 ++- >> drivers/media/platform/qcom/iris/iris_vdec.c | 4 +-- >> drivers/media/platform/qcom/iris/iris_vidc.c | 6 +++-- >> 4 files changed, 44 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/iris/iris_buffer.c b/drivers/media/platform/qcom/iris/iris_buffer.c >> index e5c5a564fcb8..606d76b10be2 100644 >> --- a/drivers/media/platform/qcom/iris/iris_buffer.c >> +++ b/drivers/media/platform/qcom/iris/iris_buffer.c >> @@ -376,7 +376,7 @@ int iris_destroy_internal_buffer(struct iris_inst *inst, struct iris_buffer *buf >> return 0; >> } >> >> -int iris_destroy_internal_buffers(struct iris_inst *inst, u32 plane) >> +int iris_destroy_internal_buffers(struct iris_inst *inst, u32 plane, bool force) >> { >> const struct iris_platform_data *platform_data = inst->core->iris_platform_data; >> struct iris_buffer *buf, *next; >> @@ -396,6 +396,14 @@ 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) { >> + /* >> + * during stream on, skip destroying internal(DPB) buffer >> + * if firmware did not return it. >> + * during close, destroy all buffers irrespectively. >> + */ >> + if (!force && buf->attr & BUF_ATTR_QUEUED) >> + continue; >> + >> ret = iris_destroy_internal_buffer(inst, buf); >> if (ret) >> return ret; >> @@ -446,6 +454,33 @@ static int iris_release_input_internal_buffers(struct iris_inst *inst) >> return 0; >> } >> >> +void iris_get_num_queued_internal_buffers(struct iris_inst *inst, u32 plane) > name this iris_check_num_queued_internal_buffers.. > Ack. Thanks, Dikshita >> +{ >> + const struct iris_platform_data *platform_data = inst->core->iris_platform_data; >> + struct iris_buffer *buf, *next; >> + struct iris_buffers *buffers; >> + const u32 *internal_buf_type; >> + u32 internal_buffer_count, i; >> + u32 count = 0; >> + >> + if (V4L2_TYPE_IS_OUTPUT(plane)) { >> + internal_buf_type = platform_data->dec_ip_int_buf_tbl; >> + internal_buffer_count = platform_data->dec_ip_int_buf_tbl_size; >> + } else { >> + internal_buf_type = platform_data->dec_op_int_buf_tbl; >> + internal_buffer_count = platform_data->dec_op_int_buf_tbl_size; >> + } >> + >> + for (i = 0; i < internal_buffer_count; i++) { >> + buffers = &inst->buffers[internal_buf_type[i]]; >> + list_for_each_entry_safe(buf, next, &buffers->list, list) >> + count++; >> + if (count) >> + dev_err(inst->core->dev, "%d buffer of type %d not released", >> + count, internal_buf_type[i]); >> + } >> +} >> + >> int iris_alloc_and_queue_persist_bufs(struct iris_inst *inst) >> { >> struct iris_buffers *buffers = &inst->buffers[BUF_PERSIST]; >> diff --git a/drivers/media/platform/qcom/iris/iris_buffer.h b/drivers/media/platform/qcom/iris/iris_buffer.h >> index c36b6347b077..03a32b91cf21 100644 >> --- a/drivers/media/platform/qcom/iris/iris_buffer.h >> +++ b/drivers/media/platform/qcom/iris/iris_buffer.h >> @@ -106,7 +106,8 @@ void iris_get_internal_buffers(struct iris_inst *inst, u32 plane); >> int iris_create_internal_buffers(struct iris_inst *inst, u32 plane); >> int iris_queue_internal_buffers(struct iris_inst *inst, u32 plane); >> int iris_destroy_internal_buffer(struct iris_inst *inst, struct iris_buffer *buffer); >> -int iris_destroy_internal_buffers(struct iris_inst *inst, u32 plane); >> +int iris_destroy_internal_buffers(struct iris_inst *inst, u32 plane, bool force); >> +void iris_get_num_queued_internal_buffers(struct iris_inst *inst, u32 plane); > make this static > > Regards, > Vikash >> int iris_alloc_and_queue_persist_bufs(struct iris_inst *inst); >> int iris_alloc_and_queue_input_int_bufs(struct iris_inst *inst); >> int iris_queue_buffer(struct iris_inst *inst, struct iris_buffer *buf); >> diff --git a/drivers/media/platform/qcom/iris/iris_vdec.c b/drivers/media/platform/qcom/iris/iris_vdec.c >> index 4143acedfc57..2c1a7162d2da 100644 >> --- a/drivers/media/platform/qcom/iris/iris_vdec.c >> +++ b/drivers/media/platform/qcom/iris/iris_vdec.c >> @@ -408,7 +408,7 @@ int iris_vdec_streamon_input(struct iris_inst *inst) >> >> iris_get_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE); >> >> - ret = iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE); >> + ret = iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, false); >> if (ret) >> return ret; >> >> @@ -496,7 +496,7 @@ int iris_vdec_streamon_output(struct iris_inst *inst) >> >> iris_get_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); >> >> - ret = iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); >> + ret = iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE, false); >> if (ret) >> return ret; >> >> diff --git a/drivers/media/platform/qcom/iris/iris_vidc.c b/drivers/media/platform/qcom/iris/iris_vidc.c >> index ca0f4e310f77..56531a7f0dfe 100644 >> --- a/drivers/media/platform/qcom/iris/iris_vidc.c >> +++ b/drivers/media/platform/qcom/iris/iris_vidc.c >> @@ -233,8 +233,10 @@ int iris_close(struct file *filp) >> iris_session_close(inst); >> iris_inst_change_state(inst, IRIS_INST_DEINIT); >> iris_v4l2_fh_deinit(inst); >> - iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE); >> - iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); >> + iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, true); >> + iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE, true); >> + iris_get_num_queued_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE); >> + iris_get_num_queued_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); >> iris_remove_session(inst); >> mutex_unlock(&inst->lock); >> mutex_destroy(&inst->ctx_q_lock); >>
Not mine to review, but wanted to highlight some best practices, comment below... Le lundi 28 avril 2025 à 14:58 +0530, Dikshita Agarwal a écrit : > Firmware might hold the DPB buffers for reference in case of sequence > change, so skip destroying buffers for which QUEUED flag is not removed. > Also, make sure that all buffers are released during streamoff. > > 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 | 37 +++++++++++++++++++++++++- > drivers/media/platform/qcom/iris/iris_buffer.h | 3 ++- > drivers/media/platform/qcom/iris/iris_vdec.c | 4 +-- > drivers/media/platform/qcom/iris/iris_vidc.c | 6 +++-- > 4 files changed, 44 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/platform/qcom/iris/iris_buffer.c b/drivers/media/platform/qcom/iris/iris_buffer.c > index e5c5a564fcb8..606d76b10be2 100644 > --- a/drivers/media/platform/qcom/iris/iris_buffer.c > +++ b/drivers/media/platform/qcom/iris/iris_buffer.c > @@ -376,7 +376,7 @@ int iris_destroy_internal_buffer(struct iris_inst *inst, struct iris_buffer *buf > return 0; > } > > -int iris_destroy_internal_buffers(struct iris_inst *inst, u32 plane) > +int iris_destroy_internal_buffers(struct iris_inst *inst, u32 plane, bool force) Its always tempting to just glue a boolean at the end of a parameter list. But this has huge downside in code readability, see below... > { > const struct iris_platform_data *platform_data = inst->core->iris_platform_data; > struct iris_buffer *buf, *next; > @@ -396,6 +396,14 @@ 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) { > + /* > + * during stream on, skip destroying internal(DPB) buffer > + * if firmware did not return it. > + * during close, destroy all buffers irrespectively. > + */ > + if (!force && buf->attr & BUF_ATTR_QUEUED) > + continue; > + > ret = iris_destroy_internal_buffer(inst, buf); > if (ret) > return ret; > @@ -446,6 +454,33 @@ static int iris_release_input_internal_buffers(struct iris_inst *inst) > return 0; > } > > +void iris_get_num_queued_internal_buffers(struct iris_inst *inst, u32 plane) > +{ > + const struct iris_platform_data *platform_data = inst->core->iris_platform_data; > + struct iris_buffer *buf, *next; > + struct iris_buffers *buffers; > + const u32 *internal_buf_type; > + u32 internal_buffer_count, i; > + u32 count = 0; > + > + if (V4L2_TYPE_IS_OUTPUT(plane)) { > + internal_buf_type = platform_data->dec_ip_int_buf_tbl; > + internal_buffer_count = platform_data->dec_ip_int_buf_tbl_size; > + } else { > + internal_buf_type = platform_data->dec_op_int_buf_tbl; > + internal_buffer_count = platform_data->dec_op_int_buf_tbl_size; > + } > + > + for (i = 0; i < internal_buffer_count; i++) { > + buffers = &inst->buffers[internal_buf_type[i]]; > + list_for_each_entry_safe(buf, next, &buffers->list, list) > + count++; > + if (count) > + dev_err(inst->core->dev, "%d buffer of type %d not released", > + count, internal_buf_type[i]); > + } > +} > + > int iris_alloc_and_queue_persist_bufs(struct iris_inst *inst) > { > struct iris_buffers *buffers = &inst->buffers[BUF_PERSIST]; > diff --git a/drivers/media/platform/qcom/iris/iris_buffer.h b/drivers/media/platform/qcom/iris/iris_buffer.h > index c36b6347b077..03a32b91cf21 100644 > --- a/drivers/media/platform/qcom/iris/iris_buffer.h > +++ b/drivers/media/platform/qcom/iris/iris_buffer.h > @@ -106,7 +106,8 @@ void iris_get_internal_buffers(struct iris_inst *inst, u32 plane); > int iris_create_internal_buffers(struct iris_inst *inst, u32 plane); > int iris_queue_internal_buffers(struct iris_inst *inst, u32 plane); > int iris_destroy_internal_buffer(struct iris_inst *inst, struct iris_buffer *buffer); > -int iris_destroy_internal_buffers(struct iris_inst *inst, u32 plane); > +int iris_destroy_internal_buffers(struct iris_inst *inst, u32 plane, bool force); > +void iris_get_num_queued_internal_buffers(struct iris_inst *inst, u32 plane); > int iris_alloc_and_queue_persist_bufs(struct iris_inst *inst); > int iris_alloc_and_queue_input_int_bufs(struct iris_inst *inst); > int iris_queue_buffer(struct iris_inst *inst, struct iris_buffer *buf); > diff --git a/drivers/media/platform/qcom/iris/iris_vdec.c b/drivers/media/platform/qcom/iris/iris_vdec.c > index 4143acedfc57..2c1a7162d2da 100644 > --- a/drivers/media/platform/qcom/iris/iris_vdec.c > +++ b/drivers/media/platform/qcom/iris/iris_vdec.c > @@ -408,7 +408,7 @@ int iris_vdec_streamon_input(struct iris_inst *inst) > > iris_get_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE); > > - ret = iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE); > + ret = iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, false); > if (ret) > return ret; > > @@ -496,7 +496,7 @@ int iris_vdec_streamon_output(struct iris_inst *inst) > > iris_get_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); > > - ret = iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); > + ret = iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE, false); If I was reviewing some changes (or even debugging) this specific C file, I would not be able to understanding what this "false" means. I would have to spend extra time, opening the declaration, going back and forth, and breaking the flow. An alternative approach is to keep the boolean parameter in a static function (c local), and then add two function wrappers that have explicit names. regards, Nicolas > if (ret) > return ret; > > diff --git a/drivers/media/platform/qcom/iris/iris_vidc.c b/drivers/media/platform/qcom/iris/iris_vidc.c > index ca0f4e310f77..56531a7f0dfe 100644 > --- a/drivers/media/platform/qcom/iris/iris_vidc.c > +++ b/drivers/media/platform/qcom/iris/iris_vidc.c > @@ -233,8 +233,10 @@ int iris_close(struct file *filp) > iris_session_close(inst); > iris_inst_change_state(inst, IRIS_INST_DEINIT); > iris_v4l2_fh_deinit(inst); > - iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE); > - iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); > + iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, true); > + iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE, true); > + iris_get_num_queued_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE); > + iris_get_num_queued_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); > iris_remove_session(inst); > mutex_unlock(&inst->lock); > mutex_destroy(&inst->ctx_q_lock);
diff --git a/drivers/media/platform/qcom/iris/iris_buffer.c b/drivers/media/platform/qcom/iris/iris_buffer.c index e5c5a564fcb8..606d76b10be2 100644 --- a/drivers/media/platform/qcom/iris/iris_buffer.c +++ b/drivers/media/platform/qcom/iris/iris_buffer.c @@ -376,7 +376,7 @@ int iris_destroy_internal_buffer(struct iris_inst *inst, struct iris_buffer *buf return 0; } -int iris_destroy_internal_buffers(struct iris_inst *inst, u32 plane) +int iris_destroy_internal_buffers(struct iris_inst *inst, u32 plane, bool force) { const struct iris_platform_data *platform_data = inst->core->iris_platform_data; struct iris_buffer *buf, *next; @@ -396,6 +396,14 @@ 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) { + /* + * during stream on, skip destroying internal(DPB) buffer + * if firmware did not return it. + * during close, destroy all buffers irrespectively. + */ + if (!force && buf->attr & BUF_ATTR_QUEUED) + continue; + ret = iris_destroy_internal_buffer(inst, buf); if (ret) return ret; @@ -446,6 +454,33 @@ static int iris_release_input_internal_buffers(struct iris_inst *inst) return 0; } +void iris_get_num_queued_internal_buffers(struct iris_inst *inst, u32 plane) +{ + const struct iris_platform_data *platform_data = inst->core->iris_platform_data; + struct iris_buffer *buf, *next; + struct iris_buffers *buffers; + const u32 *internal_buf_type; + u32 internal_buffer_count, i; + u32 count = 0; + + if (V4L2_TYPE_IS_OUTPUT(plane)) { + internal_buf_type = platform_data->dec_ip_int_buf_tbl; + internal_buffer_count = platform_data->dec_ip_int_buf_tbl_size; + } else { + internal_buf_type = platform_data->dec_op_int_buf_tbl; + internal_buffer_count = platform_data->dec_op_int_buf_tbl_size; + } + + for (i = 0; i < internal_buffer_count; i++) { + buffers = &inst->buffers[internal_buf_type[i]]; + list_for_each_entry_safe(buf, next, &buffers->list, list) + count++; + if (count) + dev_err(inst->core->dev, "%d buffer of type %d not released", + count, internal_buf_type[i]); + } +} + int iris_alloc_and_queue_persist_bufs(struct iris_inst *inst) { struct iris_buffers *buffers = &inst->buffers[BUF_PERSIST]; diff --git a/drivers/media/platform/qcom/iris/iris_buffer.h b/drivers/media/platform/qcom/iris/iris_buffer.h index c36b6347b077..03a32b91cf21 100644 --- a/drivers/media/platform/qcom/iris/iris_buffer.h +++ b/drivers/media/platform/qcom/iris/iris_buffer.h @@ -106,7 +106,8 @@ void iris_get_internal_buffers(struct iris_inst *inst, u32 plane); int iris_create_internal_buffers(struct iris_inst *inst, u32 plane); int iris_queue_internal_buffers(struct iris_inst *inst, u32 plane); int iris_destroy_internal_buffer(struct iris_inst *inst, struct iris_buffer *buffer); -int iris_destroy_internal_buffers(struct iris_inst *inst, u32 plane); +int iris_destroy_internal_buffers(struct iris_inst *inst, u32 plane, bool force); +void iris_get_num_queued_internal_buffers(struct iris_inst *inst, u32 plane); int iris_alloc_and_queue_persist_bufs(struct iris_inst *inst); int iris_alloc_and_queue_input_int_bufs(struct iris_inst *inst); int iris_queue_buffer(struct iris_inst *inst, struct iris_buffer *buf); diff --git a/drivers/media/platform/qcom/iris/iris_vdec.c b/drivers/media/platform/qcom/iris/iris_vdec.c index 4143acedfc57..2c1a7162d2da 100644 --- a/drivers/media/platform/qcom/iris/iris_vdec.c +++ b/drivers/media/platform/qcom/iris/iris_vdec.c @@ -408,7 +408,7 @@ int iris_vdec_streamon_input(struct iris_inst *inst) iris_get_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE); - ret = iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE); + ret = iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, false); if (ret) return ret; @@ -496,7 +496,7 @@ int iris_vdec_streamon_output(struct iris_inst *inst) iris_get_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); - ret = iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); + ret = iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE, false); if (ret) return ret; diff --git a/drivers/media/platform/qcom/iris/iris_vidc.c b/drivers/media/platform/qcom/iris/iris_vidc.c index ca0f4e310f77..56531a7f0dfe 100644 --- a/drivers/media/platform/qcom/iris/iris_vidc.c +++ b/drivers/media/platform/qcom/iris/iris_vidc.c @@ -233,8 +233,10 @@ int iris_close(struct file *filp) iris_session_close(inst); iris_inst_change_state(inst, IRIS_INST_DEINIT); iris_v4l2_fh_deinit(inst); - iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE); - iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); + iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, true); + iris_destroy_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE, true); + iris_get_num_queued_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE); + iris_get_num_queued_internal_buffers(inst, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); iris_remove_session(inst); mutex_unlock(&inst->lock); mutex_destroy(&inst->ctx_q_lock);
Firmware might hold the DPB buffers for reference in case of sequence change, so skip destroying buffers for which QUEUED flag is not removed. Also, make sure that all buffers are released during streamoff. 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 | 37 +++++++++++++++++++++++++- drivers/media/platform/qcom/iris/iris_buffer.h | 3 ++- drivers/media/platform/qcom/iris/iris_vdec.c | 4 +-- drivers/media/platform/qcom/iris/iris_vidc.c | 6 +++-- 4 files changed, 44 insertions(+), 6 deletions(-)