| Message ID | 20201201042322.3346817-1-frkoenig@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] venus: vdec: Handle DRC after drain | expand |
Hi Fritz, On 12/1/20 6:23 AM, Fritz Koenig 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> > --- > > v2: remove TODO > > drivers/media/platform/qcom/venus/core.h | 1 + > drivers/media/platform/qcom/venus/vdec.c | 17 +++++++++++++++-- > 2 files changed, 16 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; Could you introduce a new codec state instead of adding a flag for such corner case. I think that we will need to handle at least one more corner case (DRC during seek operation), so then we have to introduce another flag, and this is not a good solution to me. These additional flags will complicate the state handling and will make the code readability worst I'd introduce: VENUS_DEC_STATE_DRC_DURING_DRAIN or something similar. > }; > > #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..1d551b4d651a8 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,9 +1438,13 @@ 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; Could you move this state transition into vdec_event_change(). That way we will do it only once. > 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; ditto > vdec_event_change(inst, data, false); > break; > case HFI_EVENT_RELEASE_BUFFER_REFERENCE: >
On Wed, Dec 2, 2020 at 7:34 AM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > Hi Fritz, > > On 12/1/20 6:23 AM, Fritz Koenig 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> > > --- > > > > v2: remove TODO > > > > drivers/media/platform/qcom/venus/core.h | 1 + > > drivers/media/platform/qcom/venus/vdec.c | 17 +++++++++++++++-- > > 2 files changed, 16 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; > > Could you introduce a new codec state instead of adding a flag for such > corner case. > > I think that we will need to handle at least one more corner case (DRC > during seek operation), Just happen to have posted a patch for that. :) https://lkml.org/lkml/2020/12/2/24 > so then we have to introduce another flag, and > this is not a good solution to me. These additional flags will > complicate the state handling and will make the code readability worst > > I'd introduce: VENUS_DEC_STATE_DRC_DURING_DRAIN or something similar. I'm wondering what is the best approach here. As you see in my patch, I did not have to introduce a new state but relied instead on the state of next_buf_last (which may or may not be correct - maybe we can think of a way to merge both patches into one?). Flushes, either explicit or implicitly triggered by a DRC, are more than a state by themselves but rather an extra dimension from which other states can still apply. I'm afraid we already have many states as it is and adding more might add complexity. A lot of the recent issues we had came from that number of states, notably from the fact that not all states are always tested when they should (and fall back to the default: branch of a switch case that does nothing). I think we could improve the robustness of this driver if we mandate that each state check must be done using a switch statement without a default: branch. That would force us to ensure that each newly introduced state is considered in every situation where it might be relevant. > > > }; > > > > #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..1d551b4d651a8 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,9 +1438,13 @@ 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; > > Could you move this state transition into vdec_event_change(). That way > we will do it only once. > > > 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; > > ditto > > > vdec_event_change(inst, data, false); > > break; > > case HFI_EVENT_RELEASE_BUFFER_REFERENCE: > > > > -- > regards, > Stan
On Tue, Dec 1, 2020 at 9:58 PM Alexandre Courbot <acourbot@chromium.org> wrote: > > On Wed, Dec 2, 2020 at 7:34 AM Stanimir Varbanov > <stanimir.varbanov@linaro.org> wrote: > > > > Hi Fritz, > > > > On 12/1/20 6:23 AM, Fritz Koenig 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> > > > --- > > > > > > v2: remove TODO > > > > > > drivers/media/platform/qcom/venus/core.h | 1 + > > > drivers/media/platform/qcom/venus/vdec.c | 17 +++++++++++++++-- > > > 2 files changed, 16 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; > > > > Could you introduce a new codec state instead of adding a flag for such > > corner case. > > > > I think that we will need to handle at least one more corner case (DRC > > during seek operation), > > Just happen to have posted a patch for that. :) > > https://lkml.org/lkml/2020/12/2/24 > > > so then we have to introduce another flag, and > > this is not a good solution to me. These additional flags will > > complicate the state handling and will make the code readability worst > > > > I'd introduce: VENUS_DEC_STATE_DRC_DURING_DRAIN or something similar. > > I'm wondering what is the best approach here. As you see in my patch, > I did not have to introduce a new state but relied instead on the > state of next_buf_last (which may or may not be correct - maybe we can > think of a way to merge both patches into one?). Flushes, either > explicit or implicitly triggered by a DRC, are more than a state by > themselves but rather an extra dimension from which other states can > still apply. I'm afraid we already have many states as it is and > adding more might add complexity. > > A lot of the recent issues we had came from that number of states, > notably from the fact that not all states are always tested when they > should (and fall back to the default: branch of a switch case that > does nothing). I think we could improve the robustness of this driver > if we mandate that each state check must be done using a switch > statement without a default: branch. That would force us to ensure > that each newly introduced state is considered in every situation > where it might be relevant. > I'm finding it hard to just add an extra state. The DRC nominally goes something like this: VENUS_DEC_STATE_DECODING received HFI_EVENT_DATA_SEQUENCE_CHANGED : transition to VENUS_DEC_STATE_DRAIN received stop_capture: transition to VENUS_DEC_STATE_STOPPED received start_capture: transition to VENUS_DEC_STATE_DECODING The problematic one: VENUS_DEC_STATE_DECODING received V4L2_DEC_CMD_STOP : transition to VENUS_DEC_STATE_DRAIN received HFI_EVENT_DATA_SEQUENCE_CHANGED : transition to VENUS_DEC_STATE_DRC_DURING_DRAIN received stop_capture: transition to VENUS_DEC_STATE_DRC_DURING_DRAIN received start_capture: transition to VENUS_DEC_STATE_DECODING So it looks like I would need to add another state VENUS_DEC_STATE_STOPPED_DURING_DRC_DURING_DRAIN so that transitioning back to VENUS_DEC_STATE_DECODING would be smooth. Otherwise VENUS_DEC_STATE_DRC_DURING_DRAIN and VENUS_DEC_STATE_STOPPED will mean the same thing. This is why I had originally added the flag instead of states. I'm still working on getting the states to work. My first implementation only added VENUS_DEC_STATE_DRC_DURING_DRAIN state and I haven't totally gotten it working yet because of trying to work out the logic around VENUS_DEC_STATE_STOPPED. Please let me know if I have overlooked anything. I'm going to try adding two states and see if the logic is clearer. -Fritz > > > > > }; > > > > > > #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..1d551b4d651a8 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,9 +1438,13 @@ 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; > > > > Could you move this state transition into vdec_event_change(). That way > > we will do it only once. > > > > > 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; > > > > ditto > > > > > vdec_event_change(inst, data, false); > > > break; > > > case HFI_EVENT_RELEASE_BUFFER_REFERENCE: > > > > > > > -- > > regards, > > Stan
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..1d551b4d651a8 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,9 +1438,13 @@ 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; case HFI_EVENT_RELEASE_BUFFER_REFERENCE:
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> --- v2: remove TODO drivers/media/platform/qcom/venus/core.h | 1 + drivers/media/platform/qcom/venus/vdec.c | 17 +++++++++++++++-- 2 files changed, 16 insertions(+), 2 deletions(-)