| Message ID | 20201128063629.1830702-1-frkoenig@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | venus: vdec: Handle DRC after drain | expand |
On Sat, Nov 28, 2020 at 3:40 PM Fritz Koenig <frkoenig@chromium.org> wrote: > > If the DRC is near the end of the stream the client > may send a V4L2_DEC_CMD_STOP before the DRC occurs. > V4L2_DEC_CMD_STOP puts the driver into the > VENUS_DEC_STATE_DRAIN state. DRC must be aware so > that after the DRC event the state can be restored > correctly. > > Signed-off-by: Fritz Koenig <frkoenig@chromium.org> > > --- > > This is an attempt to fix the logic for when a DRC occurs > after the driver is in VENUS_DEC_STATE_DRAIN state. This > works for me, but I'm not sure if I covered all the cases. > Specifically I'm not sure if I reset |drain_active| in all > the right places. > > drivers/media/platform/qcom/venus/core.h | 1 + > drivers/media/platform/qcom/venus/vdec.c | 19 +++++++++++++++++-- > 2 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > index 2b0899bf5b05f..1680c936c06fb 100644 > --- a/drivers/media/platform/qcom/venus/core.h > +++ b/drivers/media/platform/qcom/venus/core.h > @@ -406,6 +406,7 @@ struct venus_inst { > unsigned int core_acquired: 1; > unsigned int bit_depth; > bool next_buf_last; > + bool drain_active; > }; > > #define IS_V1(core) ((core)->res->hfi_version == HFI_VERSION_1XX) > diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c > index 5671cf3458a68..7edbefbd75210 100644 > --- a/drivers/media/platform/qcom/venus/vdec.c > +++ b/drivers/media/platform/qcom/venus/vdec.c > @@ -523,8 +523,10 @@ vdec_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd) > > ret = hfi_session_process_buf(inst, &fdata); > > - if (!ret && inst->codec_state == VENUS_DEC_STATE_DECODING) > + if (!ret && inst->codec_state == VENUS_DEC_STATE_DECODING) { > inst->codec_state = VENUS_DEC_STATE_DRAIN; > + inst->drain_active = true; > + } > } > > unlock: > @@ -976,10 +978,14 @@ static int vdec_start_capture(struct venus_inst *inst) > > inst->codec_state = VENUS_DEC_STATE_DECODING; > > + if (inst->drain_active) > + inst->codec_state = VENUS_DEC_STATE_DRAIN; > + > inst->streamon_cap = 1; > inst->sequence_cap = 0; > inst->reconfig = false; > inst->next_buf_last = false; > + inst->drain_active = false; > > return 0; > > @@ -1105,6 +1111,7 @@ static int vdec_stop_capture(struct venus_inst *inst) > /* fallthrough */ > case VENUS_DEC_STATE_DRAIN: > inst->codec_state = VENUS_DEC_STATE_STOPPED; > + inst->drain_active = false; > /* fallthrough */ > case VENUS_DEC_STATE_SEEK: > vdec_cancel_dst_buffers(inst); > @@ -1304,8 +1311,10 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type, > > v4l2_event_queue_fh(&inst->fh, &ev); > > - if (inst->codec_state == VENUS_DEC_STATE_DRAIN) > + if (inst->codec_state == VENUS_DEC_STATE_DRAIN) { > + inst->drain_active = false; > inst->codec_state = VENUS_DEC_STATE_STOPPED; > + } > } > > if (!bytesused) > @@ -1429,11 +1438,17 @@ static void vdec_event_notify(struct venus_inst *inst, u32 event, > case EVT_SYS_EVENT_CHANGE: > switch (data->event_type) { > case HFI_EVENT_DATA_SEQUENCE_CHANGED_SUFFICIENT_BUF_RESOURCES: > + if (inst->codec_state == VENUS_DEC_STATE_DRAIN) > + inst->codec_state = VENUS_DEC_STATE_DECODING; > vdec_event_change(inst, data, true); > break; > case HFI_EVENT_DATA_SEQUENCE_CHANGED_INSUFFICIENT_BUF_RESOURCES: > + if (inst->codec_state == VENUS_DEC_STATE_DRAIN) > + inst->codec_state = VENUS_DEC_STATE_DECODING; > vdec_event_change(inst, data, false); > break; > + // TODO(fritz) : does HFI_EVENT_RELEASE_BUFFER_REFERENCE also need to > + // change the codec_state to VENUS_DEC_STATE_DECODING? I don't think it does, but Stanimir can confirm probably. In any case we should remove this TODO in the next version. :) > case HFI_EVENT_RELEASE_BUFFER_REFERENCE: > venus_helper_release_buf_ref(inst, data->tag); > break; > -- > 2.29.2.454.gaff20da3a2-goog >
On 12/1/20 5:09 AM, Alexandre Courbot wrote: > On Sat, Nov 28, 2020 at 3:40 PM Fritz Koenig <frkoenig@chromium.org> wrote: >> >> If the DRC is near the end of the stream the client >> may send a V4L2_DEC_CMD_STOP before the DRC occurs. >> V4L2_DEC_CMD_STOP puts the driver into the >> VENUS_DEC_STATE_DRAIN state. DRC must be aware so >> that after the DRC event the state can be restored >> correctly. >> >> Signed-off-by: Fritz Koenig <frkoenig@chromium.org> >> >> --- >> >> This is an attempt to fix the logic for when a DRC occurs >> after the driver is in VENUS_DEC_STATE_DRAIN state. This >> works for me, but I'm not sure if I covered all the cases. >> Specifically I'm not sure if I reset |drain_active| in all >> the right places. >> >> drivers/media/platform/qcom/venus/core.h | 1 + >> drivers/media/platform/qcom/venus/vdec.c | 19 +++++++++++++++++-- >> 2 files changed, 18 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h >> index 2b0899bf5b05f..1680c936c06fb 100644 >> --- a/drivers/media/platform/qcom/venus/core.h >> +++ b/drivers/media/platform/qcom/venus/core.h >> @@ -406,6 +406,7 @@ struct venus_inst { >> unsigned int core_acquired: 1; >> unsigned int bit_depth; >> bool next_buf_last; >> + bool drain_active; >> }; >> >> #define IS_V1(core) ((core)->res->hfi_version == HFI_VERSION_1XX) >> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c >> index 5671cf3458a68..7edbefbd75210 100644 >> --- a/drivers/media/platform/qcom/venus/vdec.c >> +++ b/drivers/media/platform/qcom/venus/vdec.c >> @@ -523,8 +523,10 @@ vdec_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd) >> >> ret = hfi_session_process_buf(inst, &fdata); >> >> - if (!ret && inst->codec_state == VENUS_DEC_STATE_DECODING) >> + if (!ret && inst->codec_state == VENUS_DEC_STATE_DECODING) { >> inst->codec_state = VENUS_DEC_STATE_DRAIN; >> + inst->drain_active = true; >> + } >> } >> >> unlock: >> @@ -976,10 +978,14 @@ static int vdec_start_capture(struct venus_inst *inst) >> >> inst->codec_state = VENUS_DEC_STATE_DECODING; >> >> + if (inst->drain_active) >> + inst->codec_state = VENUS_DEC_STATE_DRAIN; >> + >> inst->streamon_cap = 1; >> inst->sequence_cap = 0; >> inst->reconfig = false; >> inst->next_buf_last = false; >> + inst->drain_active = false; >> >> return 0; >> >> @@ -1105,6 +1111,7 @@ static int vdec_stop_capture(struct venus_inst *inst) >> /* fallthrough */ >> case VENUS_DEC_STATE_DRAIN: >> inst->codec_state = VENUS_DEC_STATE_STOPPED; >> + inst->drain_active = false; >> /* fallthrough */ >> case VENUS_DEC_STATE_SEEK: >> vdec_cancel_dst_buffers(inst); >> @@ -1304,8 +1311,10 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type, >> >> v4l2_event_queue_fh(&inst->fh, &ev); >> >> - if (inst->codec_state == VENUS_DEC_STATE_DRAIN) >> + if (inst->codec_state == VENUS_DEC_STATE_DRAIN) { >> + inst->drain_active = false; >> inst->codec_state = VENUS_DEC_STATE_STOPPED; >> + } >> } >> >> if (!bytesused) >> @@ -1429,11 +1438,17 @@ static void vdec_event_notify(struct venus_inst *inst, u32 event, >> case EVT_SYS_EVENT_CHANGE: >> switch (data->event_type) { >> case HFI_EVENT_DATA_SEQUENCE_CHANGED_SUFFICIENT_BUF_RESOURCES: >> + if (inst->codec_state == VENUS_DEC_STATE_DRAIN) >> + inst->codec_state = VENUS_DEC_STATE_DECODING; >> vdec_event_change(inst, data, true); >> break; >> case HFI_EVENT_DATA_SEQUENCE_CHANGED_INSUFFICIENT_BUF_RESOURCES: >> + if (inst->codec_state == VENUS_DEC_STATE_DRAIN) >> + inst->codec_state = VENUS_DEC_STATE_DECODING; >> vdec_event_change(inst, data, false); >> break; >> + // TODO(fritz) : does HFI_EVENT_RELEASE_BUFFER_REFERENCE also need to >> + // change the codec_state to VENUS_DEC_STATE_DECODING? > > I don't think it does, but Stanimir can confirm probably. In any case > we should remove this TODO in the next version. :) No, we don't need state change for release buffer reference. > >> case HFI_EVENT_RELEASE_BUFFER_REFERENCE: >> venus_helper_release_buf_ref(inst, data->tag); >> break; >> -- >> 2.29.2.454.gaff20da3a2-goog >>
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h index 2b0899bf5b05f..1680c936c06fb 100644 --- a/drivers/media/platform/qcom/venus/core.h +++ b/drivers/media/platform/qcom/venus/core.h @@ -406,6 +406,7 @@ struct venus_inst { unsigned int core_acquired: 1; unsigned int bit_depth; bool next_buf_last; + bool drain_active; }; #define IS_V1(core) ((core)->res->hfi_version == HFI_VERSION_1XX) diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c index 5671cf3458a68..7edbefbd75210 100644 --- a/drivers/media/platform/qcom/venus/vdec.c +++ b/drivers/media/platform/qcom/venus/vdec.c @@ -523,8 +523,10 @@ vdec_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd) ret = hfi_session_process_buf(inst, &fdata); - if (!ret && inst->codec_state == VENUS_DEC_STATE_DECODING) + if (!ret && inst->codec_state == VENUS_DEC_STATE_DECODING) { inst->codec_state = VENUS_DEC_STATE_DRAIN; + inst->drain_active = true; + } } unlock: @@ -976,10 +978,14 @@ static int vdec_start_capture(struct venus_inst *inst) inst->codec_state = VENUS_DEC_STATE_DECODING; + if (inst->drain_active) + inst->codec_state = VENUS_DEC_STATE_DRAIN; + inst->streamon_cap = 1; inst->sequence_cap = 0; inst->reconfig = false; inst->next_buf_last = false; + inst->drain_active = false; return 0; @@ -1105,6 +1111,7 @@ static int vdec_stop_capture(struct venus_inst *inst) /* fallthrough */ case VENUS_DEC_STATE_DRAIN: inst->codec_state = VENUS_DEC_STATE_STOPPED; + inst->drain_active = false; /* fallthrough */ case VENUS_DEC_STATE_SEEK: vdec_cancel_dst_buffers(inst); @@ -1304,8 +1311,10 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type, v4l2_event_queue_fh(&inst->fh, &ev); - if (inst->codec_state == VENUS_DEC_STATE_DRAIN) + if (inst->codec_state == VENUS_DEC_STATE_DRAIN) { + inst->drain_active = false; inst->codec_state = VENUS_DEC_STATE_STOPPED; + } } if (!bytesused) @@ -1429,11 +1438,17 @@ static void vdec_event_notify(struct venus_inst *inst, u32 event, case EVT_SYS_EVENT_CHANGE: switch (data->event_type) { case HFI_EVENT_DATA_SEQUENCE_CHANGED_SUFFICIENT_BUF_RESOURCES: + if (inst->codec_state == VENUS_DEC_STATE_DRAIN) + inst->codec_state = VENUS_DEC_STATE_DECODING; vdec_event_change(inst, data, true); break; case HFI_EVENT_DATA_SEQUENCE_CHANGED_INSUFFICIENT_BUF_RESOURCES: + if (inst->codec_state == VENUS_DEC_STATE_DRAIN) + inst->codec_state = VENUS_DEC_STATE_DECODING; vdec_event_change(inst, data, false); break; + // TODO(fritz) : does HFI_EVENT_RELEASE_BUFFER_REFERENCE also need to + // change the codec_state to VENUS_DEC_STATE_DECODING? case HFI_EVENT_RELEASE_BUFFER_REFERENCE: venus_helper_release_buf_ref(inst, data->tag); break;
If the DRC is near the end of the stream the client may send a V4L2_DEC_CMD_STOP before the DRC occurs. V4L2_DEC_CMD_STOP puts the driver into the VENUS_DEC_STATE_DRAIN state. DRC must be aware so that after the DRC event the state can be restored correctly. Signed-off-by: Fritz Koenig <frkoenig@chromium.org> --- This is an attempt to fix the logic for when a DRC occurs after the driver is in VENUS_DEC_STATE_DRAIN state. This works for me, but I'm not sure if I covered all the cases. Specifically I'm not sure if I reset |drain_active| in all the right places. drivers/media/platform/qcom/venus/core.h | 1 + drivers/media/platform/qcom/venus/vdec.c | 19 +++++++++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-)