| Message ID | 20250305104335.3629945-9-quic_dikshita@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for HEVC and VP9 codecs in decoder | expand |
On 05/03/2025 10:43, Dikshita Agarwal wrote: > During the reconfig, firmware sends the resolution aligned by 8 byte, > if driver set the same resoluton to firmware, it will be aligned to 16 > byte causing another sequence change which would be incorrect. During reconfig the firmware sends the resolution aligned to 8 bytes. If the driver sends the same resolution back to the firmware the resolution will be aligned to 16 bytes not 8. The alignment mismatch would then subsequently cause the firmware to send another redundant sequence change. > Fix this by not setting the updated resolution to firmware during > reconfig. Fix this by not setting the resolution property during reconfig. > > Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> > --- > .../platform/qcom/iris/iris_hfi_gen1_command.c | 15 ++++++++------- > .../platform/qcom/iris/iris_hfi_gen1_response.c | 1 + > drivers/media/platform/qcom/iris/iris_instance.h | 2 ++ > drivers/media/platform/qcom/iris/iris_vdec.c | 4 ++++ > 4 files changed, 15 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c > index a160ae915886..d5e81049d37e 100644 > --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c > +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c > @@ -562,14 +562,15 @@ static int iris_hfi_gen1_set_resolution(struct iris_inst *inst) > struct hfi_framesize fs; > int ret; > > - fs.buffer_type = HFI_BUFFER_INPUT; > - fs.width = inst->fmt_src->fmt.pix_mp.width; > - fs.height = inst->fmt_src->fmt.pix_mp.height; > - > - ret = hfi_gen1_set_property(inst, ptype, &fs, sizeof(fs)); > - if (ret) > - return ret; > + if (!inst->in_reconfig) { > + fs.buffer_type = HFI_BUFFER_INPUT; > + fs.width = inst->fmt_src->fmt.pix_mp.width; > + fs.height = inst->fmt_src->fmt.pix_mp.height; > > + ret = hfi_gen1_set_property(inst, ptype, &fs, sizeof(fs)); > + if (ret) > + return ret; > + } > fs.buffer_type = HFI_BUFFER_OUTPUT2; > fs.width = inst->fmt_dst->fmt.pix_mp.width; > fs.height = inst->fmt_dst->fmt.pix_mp.height; > diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c b/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c > index 91d95eed68aa..6576496fdbdf 100644 > --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c > +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c > @@ -155,6 +155,7 @@ static void iris_hfi_gen1_read_changed_params(struct iris_inst *inst, > inst->crop.height = event.height; > } > > + inst->in_reconfig = true; This flag can be changed by iris_hfi_isr_handler() down the chain. > @@ -453,6 +453,8 @@ static int iris_vdec_process_streamon_input(struct iris_inst *inst) > if (ret) > return ret; > > + inst->in_reconfig = false; > + > return iris_inst_change_sub_state(inst, 0, set_sub_state); > } > > @@ -544,6 +546,8 @@ static int iris_vdec_process_streamon_output(struct iris_inst *inst) > if (ret) > return ret; > > + inst->in_reconfig = false; > + Are these usages of the in_reconfig flag then thread-safe ? i.e. are both iris_vdec_process_streamon_input() and iris_vdec_process_streamon_output() guaranteed not to run @ the same time ? I don't see any obvious locking here. --- bod
On 3/6/2025 6:56 AM, Bryan O'Donoghue wrote: > On 05/03/2025 10:43, Dikshita Agarwal wrote: >> During the reconfig, firmware sends the resolution aligned by 8 byte, >> if driver set the same resoluton to firmware, it will be aligned to 16 >> byte causing another sequence change which would be incorrect. > > During reconfig the firmware sends the resolution aligned to 8 bytes. If > the driver sends the same resolution back to the firmware the resolution > will be aligned to 16 bytes not 8. > > The alignment mismatch would then subsequently cause the firmware to send > another redundant sequence change. > >> Fix this by not setting the updated resolution to firmware during >> reconfig. > > Fix this by not setting the resolution property during reconfig. Ack. >> >> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> >> --- >> .../platform/qcom/iris/iris_hfi_gen1_command.c | 15 ++++++++------- >> .../platform/qcom/iris/iris_hfi_gen1_response.c | 1 + >> drivers/media/platform/qcom/iris/iris_instance.h | 2 ++ >> drivers/media/platform/qcom/iris/iris_vdec.c | 4 ++++ >> 4 files changed, 15 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c >> b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c >> index a160ae915886..d5e81049d37e 100644 >> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c >> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c >> @@ -562,14 +562,15 @@ static int iris_hfi_gen1_set_resolution(struct >> iris_inst *inst) >> struct hfi_framesize fs; >> int ret; >> - fs.buffer_type = HFI_BUFFER_INPUT; >> - fs.width = inst->fmt_src->fmt.pix_mp.width; >> - fs.height = inst->fmt_src->fmt.pix_mp.height; >> - >> - ret = hfi_gen1_set_property(inst, ptype, &fs, sizeof(fs)); >> - if (ret) >> - return ret; >> + if (!inst->in_reconfig) { >> + fs.buffer_type = HFI_BUFFER_INPUT; >> + fs.width = inst->fmt_src->fmt.pix_mp.width; >> + fs.height = inst->fmt_src->fmt.pix_mp.height; >> + ret = hfi_gen1_set_property(inst, ptype, &fs, sizeof(fs)); >> + if (ret) >> + return ret; >> + } >> fs.buffer_type = HFI_BUFFER_OUTPUT2; >> fs.width = inst->fmt_dst->fmt.pix_mp.width; >> fs.height = inst->fmt_dst->fmt.pix_mp.height; >> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c >> b/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c >> index 91d95eed68aa..6576496fdbdf 100644 >> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c >> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c >> @@ -155,6 +155,7 @@ static void iris_hfi_gen1_read_changed_params(struct >> iris_inst *inst, >> inst->crop.height = event.height; >> } >> + inst->in_reconfig = true; > > This flag can be changed by iris_hfi_isr_handler() down the chain. > > >> @@ -453,6 +453,8 @@ static int iris_vdec_process_streamon_input(struct >> iris_inst *inst) >> if (ret) >> return ret; >> + inst->in_reconfig = false; >> + >> return iris_inst_change_sub_state(inst, 0, set_sub_state); >> } >> @@ -544,6 +546,8 @@ static int iris_vdec_process_streamon_output(struct >> iris_inst *inst) >> if (ret) >> return ret; >> + inst->in_reconfig = false; >> + > > Are these usages of the in_reconfig flag then thread-safe ? > > i.e. are both iris_vdec_process_streamon_input() and > iris_vdec_process_streamon_output() guaranteed not to run @ the same time ? > > I don't see any obvious locking here. > Since reconfig handling is only relevant to capture port, the usage of in_reconfig flag in output port is unnecessary. I'll remove the redundant flag from output stream_on to simplify the code. Thanks, Dikshita > --- > bod
diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c index a160ae915886..d5e81049d37e 100644 --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c @@ -562,14 +562,15 @@ static int iris_hfi_gen1_set_resolution(struct iris_inst *inst) struct hfi_framesize fs; int ret; - fs.buffer_type = HFI_BUFFER_INPUT; - fs.width = inst->fmt_src->fmt.pix_mp.width; - fs.height = inst->fmt_src->fmt.pix_mp.height; - - ret = hfi_gen1_set_property(inst, ptype, &fs, sizeof(fs)); - if (ret) - return ret; + if (!inst->in_reconfig) { + fs.buffer_type = HFI_BUFFER_INPUT; + fs.width = inst->fmt_src->fmt.pix_mp.width; + fs.height = inst->fmt_src->fmt.pix_mp.height; + ret = hfi_gen1_set_property(inst, ptype, &fs, sizeof(fs)); + if (ret) + return ret; + } fs.buffer_type = HFI_BUFFER_OUTPUT2; fs.width = inst->fmt_dst->fmt.pix_mp.width; fs.height = inst->fmt_dst->fmt.pix_mp.height; diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c b/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c index 91d95eed68aa..6576496fdbdf 100644 --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c @@ -155,6 +155,7 @@ static void iris_hfi_gen1_read_changed_params(struct iris_inst *inst, inst->crop.height = event.height; } + inst->in_reconfig = true; inst->fw_min_count = event.buf_count; inst->buffers[BUF_OUTPUT].min_count = iris_vpu_buf_count(inst, BUF_OUTPUT); inst->buffers[BUF_OUTPUT].size = pixmp_op->plane_fmt[0].sizeimage; diff --git a/drivers/media/platform/qcom/iris/iris_instance.h b/drivers/media/platform/qcom/iris/iris_instance.h index d8f076936c2b..625325f7f9b0 100644 --- a/drivers/media/platform/qcom/iris/iris_instance.h +++ b/drivers/media/platform/qcom/iris/iris_instance.h @@ -43,6 +43,7 @@ * @tss: timestamp metadata * @metadata_idx: index for metadata buffer * @codec: codec type + * @in_reconfig: a flag raised by decoder when the stream resolution changes */ struct iris_inst { @@ -74,6 +75,7 @@ struct iris_inst { struct iris_ts_metadata tss[VIDEO_MAX_FRAME]; u32 metadata_idx; u32 codec; + bool in_reconfig; }; #endif diff --git a/drivers/media/platform/qcom/iris/iris_vdec.c b/drivers/media/platform/qcom/iris/iris_vdec.c index 16ae83948076..d5127d88d103 100644 --- a/drivers/media/platform/qcom/iris/iris_vdec.c +++ b/drivers/media/platform/qcom/iris/iris_vdec.c @@ -453,6 +453,8 @@ static int iris_vdec_process_streamon_input(struct iris_inst *inst) if (ret) return ret; + inst->in_reconfig = false; + return iris_inst_change_sub_state(inst, 0, set_sub_state); } @@ -544,6 +546,8 @@ static int iris_vdec_process_streamon_output(struct iris_inst *inst) if (ret) return ret; + inst->in_reconfig = false; + return iris_inst_change_sub_state(inst, clear_sub_state, 0); }
During the reconfig, firmware sends the resolution aligned by 8 byte, if driver set the same resoluton to firmware, it will be aligned to 16 byte causing another sequence change which would be incorrect. Fix this by not setting the updated resolution to firmware during reconfig. Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> --- .../platform/qcom/iris/iris_hfi_gen1_command.c | 15 ++++++++------- .../platform/qcom/iris/iris_hfi_gen1_response.c | 1 + drivers/media/platform/qcom/iris/iris_instance.h | 2 ++ drivers/media/platform/qcom/iris/iris_vdec.c | 4 ++++ 4 files changed, 15 insertions(+), 7 deletions(-)