| Message ID | 20230130135418.1604455-1-mk@semmihalf.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] media: venus: dec: Fix handling of the start cmd | expand |
pon., 30 sty 2023 o 14:55 Michał Krawczyk <mk@semihalf.com> napisał(a): > > From: Michał Krawczyk <mk@semihalf.com> > > The decoder driver should clear the last_buffer_dequeued flag of the > capture queue upon receiving V4L2_DEC_CMD_START. > > The last_buffer_dequeued flag is set upon receiving EOS (which always > happens upon receiving V4L2_DEC_CMD_STOP). > > Without this patch, after issuing the V4L2_DEC_CMD_STOP and > V4L2_DEC_CMD_START, the vb2_dqbuf() function will always fail, even if > the buffers are completed by the hardware. > > Fixes: beac82904a87 ("media: venus: make decoder compliant with stateful codec API") > > Signed-off-by: Michał Krawczyk <mk@semihalf.com> Hello, Did anyone have a chance to take a look at this patch? It's fairly simple, but lack of this fix can have a big impact on the V4L2 applications which implement the flush mechanism using the stop/start commands, especially in the middle of the video. Thank you, Michał
Hello Michal, Thank you for raising a fix in video driver. > -----Original Message----- > From: Michał Krawczyk <mk@semihalf.com> > Sent: Tuesday, February 7, 2023 2:48 PM > To: Stanimir Varbanov <stanimir.k.varbanov@gmail.com>; Vikash Garodia > (QUIC) <quic_vgarodia@quicinc.com> > Cc: Andy Gross <agross@kernel.org>; Bjorn Andersson > <andersson@kernel.org>; Konrad Dybcio <konrad.dybcio@linaro.org>; Mauro > Carvalho Chehab <mchehab@kernel.org>; linux-media@vger.kernel.org; linux- > arm-msm@vger.kernel.org; linux-kernel@vger.kernel.org; mw@semihalf.com > Subject: Re: [PATCH v2] media: venus: dec: Fix handling of the start cmd > > WARNING: This email originated from outside of Qualcomm. Please be wary of > any links or attachments, and do not enable macros. > > pon., 30 sty 2023 o 14:55 Michał Krawczyk <mk@semihalf.com> napisał(a): > > > > From: Michał Krawczyk <mk@semihalf.com> > > > > The decoder driver should clear the last_buffer_dequeued flag of the > > capture queue upon receiving V4L2_DEC_CMD_START. > > > > The last_buffer_dequeued flag is set upon receiving EOS (which always > > happens upon receiving V4L2_DEC_CMD_STOP). > > > > Without this patch, after issuing the V4L2_DEC_CMD_STOP and > > V4L2_DEC_CMD_START, the vb2_dqbuf() function will always fail, even if > > the buffers are completed by the hardware. > > > > Fixes: beac82904a87 ("media: venus: make decoder compliant with > > stateful codec API") > > > > Signed-off-by: Michał Krawczyk <mk@semihalf.com> > > Hello, > > Did anyone have a chance to take a look at this patch? It's fairly simple, but lack > of this fix can have a big impact on the V4L2 applications which implement the > flush mechanism using the stop/start commands, especially in the middle of the > video. I have reviewed the patch, and the drain sequence handling looks good to me. Could you share some details on the test client which you are using to catch this issue ? > Thank you, > Michał Thanks, Vikash
wt., 7 lut 2023 o 10:54 Vikash Garodia <vgarodia@qti.qualcomm.com> napisał(a): > I have reviewed the patch, and the drain sequence handling looks good to me. > Could you share some details on the test client which you are using to catch this issue ? Hi Vikash, Thank you for looking at the code! I've been testing it using the Chromium implementation of the V4L2 codec [1]. Meanwhile, we were running a test suite which changes the encryption method in the middle of the video decoding. This triggers the flush behavior and the Chromium sends the stop/start cmd to the V4L2 kernel component, and the test expects the video to continue the playback normally. Unfortunately, it was causing a stall of the video at the same time. [1] https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/v4l2/ > > > Thank you, > > Michał > > Thanks, > Vikash
Hi, I'm wondering if there are any more comments for this patch? I would be happy to clarify anything that's unclear or improve the code if needed. I know it's pretty late, but it would be really great if this fix could land before v6.2 is released, so I'd appreciate your help and review. Thank you, Michał wt., 7 lut 2023 o 12:15 Michał Krawczyk <mk@semihalf.com> napisał(a): > > wt., 7 lut 2023 o 10:54 Vikash Garodia <vgarodia@qti.qualcomm.com> napisał(a): > > I have reviewed the patch, and the drain sequence handling looks good to me. > > Could you share some details on the test client which you are using to catch this issue ? > > Hi Vikash, > > Thank you for looking at the code! > > I've been testing it using the Chromium implementation of the V4L2 > codec [1]. Meanwhile, we were running a test suite which changes the > encryption method in the middle of the video decoding. This triggers > the flush behavior and the Chromium sends the stop/start cmd to the > V4L2 kernel component, and the test expects the video to continue the > playback normally. Unfortunately, it was causing a stall of the video > at the same time. > > [1] https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/v4l2/ > > > > > > Thank you, > > > Michał > > > > Thanks, > > Vikash
Hi, Any update on this patch? It would be great if we could make some progress there (and, hopefully, finally merge it :)) Thanks, Michał pt., 10 lut 2023 o 16:18 Michał Krawczyk <mk@semihalf.com> napisał(a): > > Hi, > > I'm wondering if there are any more comments for this patch? I would > be happy to clarify anything that's unclear or improve the code if > needed. > > I know it's pretty late, but it would be really great if this fix > could land before v6.2 is released, so I'd appreciate your help and > review. > > Thank you, > Michał > > wt., 7 lut 2023 o 12:15 Michał Krawczyk <mk@semihalf.com> napisał(a): > > > > wt., 7 lut 2023 o 10:54 Vikash Garodia <vgarodia@qti.qualcomm.com> napisał(a): > > > I have reviewed the patch, and the drain sequence handling looks good to me. > > > Could you share some details on the test client which you are using to catch this issue ? > > > > Hi Vikash, > > > > Thank you for looking at the code! > > > > I've been testing it using the Chromium implementation of the V4L2 > > codec [1]. Meanwhile, we were running a test suite which changes the > > encryption method in the middle of the video decoding. This triggers > > the flush behavior and the Chromium sends the stop/start cmd to the > > V4L2 kernel component, and the test expects the video to continue the > > playback normally. Unfortunately, it was causing a stall of the video > > at the same time. > > > > [1] https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/v4l2/ > > > > > > > > > Thank you, > > > > Michał > > > > > > Thanks, > > > Vikash
Hi, just a kindly reminder about the patch. Thanks, Michał pt., 10 mar 2023 o 16:05 Michał Krawczyk <mk@semihalf.com> napisał(a): > > Hi, > > Any update on this patch? It would be great if we could make some > progress there (and, hopefully, finally merge it :)) > > Thanks, > Michał > > pt., 10 lut 2023 o 16:18 Michał Krawczyk <mk@semihalf.com> napisał(a): > > > > Hi, > > > > I'm wondering if there are any more comments for this patch? I would > > be happy to clarify anything that's unclear or improve the code if > > needed. > > > > I know it's pretty late, but it would be really great if this fix > > could land before v6.2 is released, so I'd appreciate your help and > > review. > > > > Thank you, > > Michał > > > > wt., 7 lut 2023 o 12:15 Michał Krawczyk <mk@semihalf.com> napisał(a): > > > > > > wt., 7 lut 2023 o 10:54 Vikash Garodia <vgarodia@qti.qualcomm.com> napisał(a): > > > > I have reviewed the patch, and the drain sequence handling looks good to me. > > > > Could you share some details on the test client which you are using to catch this issue ? > > > > > > Hi Vikash, > > > > > > Thank you for looking at the code! > > > > > > I've been testing it using the Chromium implementation of the V4L2 > > > codec [1]. Meanwhile, we were running a test suite which changes the > > > encryption method in the middle of the video decoding. This triggers > > > the flush behavior and the Chromium sends the stop/start cmd to the > > > V4L2 kernel component, and the test expects the video to continue the > > > playback normally. Unfortunately, it was causing a stall of the video > > > at the same time. > > > > > > [1] https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/v4l2/ > > > > > > > > > > > > Thank you, > > > > > Michał > > > > > > > > Thanks, > > > > Vikash
On 4/5/2023 2:59 PM, Michał Krawczyk wrote: > Hi, > > just a kindly reminder about the patch. > > Thanks, > Michał Hi Michal, this patch is part of latest PR https://patchwork.linuxtv.org/project/linux-media/patch/20230404192722.144496-1-stanimir.k.varbanov@gmail.com/ Thanks, Dikshita > pt., 10 mar 2023 o 16:05 Michał Krawczyk <mk@semihalf.com> napisał(a): >> Hi, >> >> Any update on this patch? It would be great if we could make some >> progress there (and, hopefully, finally merge it :)) >> >> Thanks, >> Michał >> >> pt., 10 lut 2023 o 16:18 Michał Krawczyk <mk@semihalf.com> napisał(a): >>> Hi, >>> >>> I'm wondering if there are any more comments for this patch? I would >>> be happy to clarify anything that's unclear or improve the code if >>> needed. >>> >>> I know it's pretty late, but it would be really great if this fix >>> could land before v6.2 is released, so I'd appreciate your help and >>> review. >>> >>> Thank you, >>> Michał >>> >>> wt., 7 lut 2023 o 12:15 Michał Krawczyk <mk@semihalf.com> napisał(a): >>>> wt., 7 lut 2023 o 10:54 Vikash Garodia <vgarodia@qti.qualcomm.com> napisał(a): >>>>> I have reviewed the patch, and the drain sequence handling looks good to me. >>>>> Could you share some details on the test client which you are using to catch this issue ? >>>> Hi Vikash, >>>> >>>> Thank you for looking at the code! >>>> >>>> I've been testing it using the Chromium implementation of the V4L2 >>>> codec [1]. Meanwhile, we were running a test suite which changes the >>>> encryption method in the middle of the video decoding. This triggers >>>> the flush behavior and the Chromium sends the stop/start cmd to the >>>> V4L2 kernel component, and the test expects the video to continue the >>>> playback normally. Unfortunately, it was causing a stall of the video >>>> at the same time. >>>> >>>> [1] https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/v4l2/ >>>> >>>>>> Thank you, >>>>>> Michał >>>>> Thanks, >>>>> Vikash
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c index 4ceaba37e2e5..9d26587716bf 100644 --- a/drivers/media/platform/qcom/venus/vdec.c +++ b/drivers/media/platform/qcom/venus/vdec.c @@ -526,6 +526,7 @@ static int vdec_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd) { struct venus_inst *inst = to_inst(file); + struct vb2_queue *dst_vq; struct hfi_frame_data fdata = {0}; int ret; @@ -556,6 +557,13 @@ vdec_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd) inst->codec_state = VENUS_DEC_STATE_DRAIN; inst->drain_active = true; } + } else if (cmd->cmd == V4L2_DEC_CMD_START && + inst->codec_state == VENUS_DEC_STATE_STOPPED) { + dst_vq = v4l2_m2m_get_vq(inst->fh.m2m_ctx, + V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE); + vb2_clear_last_buffer_dequeued(dst_vq); + + inst->codec_state = VENUS_DEC_STATE_DECODING; } unlock: