DRC events can happen virtually at anytime, including when we are
starting a seek. Should this happen, we must make sure to return to the
DRC state, otherwise the firmware will expect buffers of the new
resolution whereas userspace will still work with the old one.
Returning to the DRC state upon resume for seeking makes sure that the
client will get the DRC event and will reallocate the buffers to fit the
firmware's expectations.
Signed-off-by: Alexandre Courbot <[email protected]>
---
drivers/media/platform/qcom/venus/vdec.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 8488411204c3..e3d0df7fd765 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -972,7 +972,10 @@ static int vdec_start_output(struct venus_inst *inst)
if (inst->codec_state == VENUS_DEC_STATE_SEEK) {
ret = venus_helper_process_initial_out_bufs(inst);
- inst->codec_state = VENUS_DEC_STATE_DECODING;
+ if (inst->next_buf_last)
+ inst->codec_state = VENUS_DEC_STATE_DRC;
+ else
+ inst->codec_state = VENUS_DEC_STATE_DECODING;
goto done;
}
@@ -1077,8 +1080,10 @@ static int vdec_stop_capture(struct venus_inst *inst)
ret = hfi_session_flush(inst, HFI_FLUSH_ALL, true);
fallthrough;
case VENUS_DEC_STATE_DRAIN:
- vdec_cancel_dst_buffers(inst);
inst->codec_state = VENUS_DEC_STATE_STOPPED;
+ fallthrough;
+ case VENUS_DEC_STATE_SEEK:
+ vdec_cancel_dst_buffers(inst);
break;
case VENUS_DEC_STATE_DRC:
WARN_ON(1);
@@ -1102,6 +1107,7 @@ static int vdec_stop_output(struct venus_inst *inst)
case VENUS_DEC_STATE_DECODING:
case VENUS_DEC_STATE_DRAIN:
case VENUS_DEC_STATE_STOPPED:
+ case VENUS_DEC_STATE_DRC:
ret = hfi_session_flush(inst, HFI_FLUSH_ALL, true);
inst->codec_state = VENUS_DEC_STATE_SEEK;
break;
@@ -1371,6 +1377,7 @@ static void vdec_event_change(struct venus_inst *inst,
dev_dbg(dev, VDBGH "flush output error %d\n", ret);
}
+ inst->next_buf_last = true;
inst->reconfig = true;
v4l2_event_queue_fh(&inst->fh, &ev);
wake_up(&inst->reconf_wait);
--
2.29.2.454.gaff20da3a2-goog
Hi Alexandre,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v5.10-rc6 next-20201201]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Alexandre-Courbot/media-venus-preserve-DRC-state-across-seeks/20201202-133933
base: git://linuxtv.org/media_tree.git master
config: m68k-randconfig-r003-20201201 (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/8ca3c23e59a3bb6c00e6d70a1de927c1558b6b32
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Alexandre-Courbot/media-venus-preserve-DRC-state-across-seeks/20201202-133933
git checkout 8ca3c23e59a3bb6c00e6d70a1de927c1558b6b32
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers/media/platform/qcom/venus/vdec.c: In function 'vdec_start_output':
>> drivers/media/platform/qcom/venus/vdec.c:975:11: error: 'struct venus_inst' has no member named 'next_buf_last'
975 | if (inst->next_buf_last)
| ^~
drivers/media/platform/qcom/venus/vdec.c: In function 'vdec_event_change':
drivers/media/platform/qcom/venus/vdec.c:1380:6: error: 'struct venus_inst' has no member named 'next_buf_last'
1380 | inst->next_buf_last = true;
| ^~
vim +975 drivers/media/platform/qcom/venus/vdec.c
968
969 static int vdec_start_output(struct venus_inst *inst)
970 {
971 int ret;
972
973 if (inst->codec_state == VENUS_DEC_STATE_SEEK) {
974 ret = venus_helper_process_initial_out_bufs(inst);
> 975 if (inst->next_buf_last)
976 inst->codec_state = VENUS_DEC_STATE_DRC;
977 else
978 inst->codec_state = VENUS_DEC_STATE_DECODING;
979 goto done;
980 }
981
982 if (inst->codec_state == VENUS_DEC_STATE_INIT ||
983 inst->codec_state == VENUS_DEC_STATE_CAPTURE_SETUP) {
984 ret = venus_helper_process_initial_out_bufs(inst);
985 goto done;
986 }
987
988 if (inst->codec_state != VENUS_DEC_STATE_DEINIT)
989 return -EINVAL;
990
991 venus_helper_init_instance(inst);
992 inst->sequence_out = 0;
993 inst->reconfig = false;
994
995 ret = vdec_set_properties(inst);
996 if (ret)
997 return ret;
998
999 ret = vdec_output_conf(inst);
1000 if (ret)
1001 return ret;
1002
1003 ret = vdec_verify_conf(inst);
1004 if (ret)
1005 return ret;
1006
1007 ret = venus_helper_set_num_bufs(inst, inst->num_input_bufs,
1008 VB2_MAX_FRAME, VB2_MAX_FRAME);
1009 if (ret)
1010 return ret;
1011
1012 ret = venus_helper_vb2_start_streaming(inst);
1013 if (ret)
1014 return ret;
1015
1016 ret = venus_helper_process_initial_out_bufs(inst);
1017 if (ret)
1018 return ret;
1019
1020 inst->codec_state = VENUS_DEC_STATE_INIT;
1021
1022 done:
1023 inst->streamon_out = 1;
1024 return ret;
1025 }
1026
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Wed, Dec 2, 2020 at 2:34 PM Alexandre Courbot <[email protected]> wrote:
>
> DRC events can happen virtually at anytime, including when we are
> starting a seek. Should this happen, we must make sure to return to the
> DRC state, otherwise the firmware will expect buffers of the new
> resolution whereas userspace will still work with the old one.
>
> Returning to the DRC state upon resume for seeking makes sure that the
> client will get the DRC event and will reallocate the buffers to fit the
> firmware's expectations.
Oops, please ignore as this seems to depend on another patch... I will
repost once I can figure out the correct dependency chain, unless
Stanimir can find a better way to handle DRC during seek and flush.
>
> Signed-off-by: Alexandre Courbot <[email protected]>
> ---
> drivers/media/platform/qcom/venus/vdec.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 8488411204c3..e3d0df7fd765 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -972,7 +972,10 @@ static int vdec_start_output(struct venus_inst *inst)
>
> if (inst->codec_state == VENUS_DEC_STATE_SEEK) {
> ret = venus_helper_process_initial_out_bufs(inst);
> - inst->codec_state = VENUS_DEC_STATE_DECODING;
> + if (inst->next_buf_last)
> + inst->codec_state = VENUS_DEC_STATE_DRC;
> + else
> + inst->codec_state = VENUS_DEC_STATE_DECODING;
> goto done;
> }
>
> @@ -1077,8 +1080,10 @@ static int vdec_stop_capture(struct venus_inst *inst)
> ret = hfi_session_flush(inst, HFI_FLUSH_ALL, true);
> fallthrough;
> case VENUS_DEC_STATE_DRAIN:
> - vdec_cancel_dst_buffers(inst);
> inst->codec_state = VENUS_DEC_STATE_STOPPED;
> + fallthrough;
> + case VENUS_DEC_STATE_SEEK:
> + vdec_cancel_dst_buffers(inst);
> break;
> case VENUS_DEC_STATE_DRC:
> WARN_ON(1);
> @@ -1102,6 +1107,7 @@ static int vdec_stop_output(struct venus_inst *inst)
> case VENUS_DEC_STATE_DECODING:
> case VENUS_DEC_STATE_DRAIN:
> case VENUS_DEC_STATE_STOPPED:
> + case VENUS_DEC_STATE_DRC:
> ret = hfi_session_flush(inst, HFI_FLUSH_ALL, true);
> inst->codec_state = VENUS_DEC_STATE_SEEK;
> break;
> @@ -1371,6 +1377,7 @@ static void vdec_event_change(struct venus_inst *inst,
> dev_dbg(dev, VDBGH "flush output error %d\n", ret);
> }
>
> + inst->next_buf_last = true;
> inst->reconfig = true;
> v4l2_event_queue_fh(&inst->fh, &ev);
> wake_up(&inst->reconf_wait);
> --
> 2.29.2.454.gaff20da3a2-goog
>
Hi,
On 12/2/20 12:24 PM, Alexandre Courbot wrote:
> On Wed, Dec 2, 2020 at 2:34 PM Alexandre Courbot <[email protected]> wrote:
>>
>> DRC events can happen virtually at anytime, including when we are
>> starting a seek. Should this happen, we must make sure to return to the
>> DRC state, otherwise the firmware will expect buffers of the new
>> resolution whereas userspace will still work with the old one.
>>
>> Returning to the DRC state upon resume for seeking makes sure that the
>> client will get the DRC event and will reallocate the buffers to fit the
>> firmware's expectations.
>
> Oops, please ignore as this seems to depend on another patch... I will
> repost once I can figure out the correct dependency chain, unless
> Stanimir can find a better way to handle DRC during seek and flush.
This patch depends on [1] series which is still under review.
[1] https://www.spinics.net/lists/linux-media/msg177801.html
>
>>
>> Signed-off-by: Alexandre Courbot <[email protected]>
>> ---
>> drivers/media/platform/qcom/venus/vdec.c | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
>> index 8488411204c3..e3d0df7fd765 100644
>> --- a/drivers/media/platform/qcom/venus/vdec.c
>> +++ b/drivers/media/platform/qcom/venus/vdec.c
>> @@ -972,7 +972,10 @@ static int vdec_start_output(struct venus_inst *inst)
>>
>> if (inst->codec_state == VENUS_DEC_STATE_SEEK) {
>> ret = venus_helper_process_initial_out_bufs(inst);
>> - inst->codec_state = VENUS_DEC_STATE_DECODING;
>> + if (inst->next_buf_last)
>> + inst->codec_state = VENUS_DEC_STATE_DRC;
>> + else
>> + inst->codec_state = VENUS_DEC_STATE_DECODING;
>> goto done;
>> }
>>
>> @@ -1077,8 +1080,10 @@ static int vdec_stop_capture(struct venus_inst *inst)
>> ret = hfi_session_flush(inst, HFI_FLUSH_ALL, true);
>> fallthrough;
>> case VENUS_DEC_STATE_DRAIN:
>> - vdec_cancel_dst_buffers(inst);
>> inst->codec_state = VENUS_DEC_STATE_STOPPED;
>> + fallthrough;
>> + case VENUS_DEC_STATE_SEEK:
>> + vdec_cancel_dst_buffers(inst);
>> break;
>> case VENUS_DEC_STATE_DRC:
>> WARN_ON(1);
>> @@ -1102,6 +1107,7 @@ static int vdec_stop_output(struct venus_inst *inst)
>> case VENUS_DEC_STATE_DECODING:
>> case VENUS_DEC_STATE_DRAIN:
>> case VENUS_DEC_STATE_STOPPED:
>> + case VENUS_DEC_STATE_DRC:
>> ret = hfi_session_flush(inst, HFI_FLUSH_ALL, true);
>> inst->codec_state = VENUS_DEC_STATE_SEEK;
>> break;
>> @@ -1371,6 +1377,7 @@ static void vdec_event_change(struct venus_inst *inst,
>> dev_dbg(dev, VDBGH "flush output error %d\n", ret);
>> }
>>
>> + inst->next_buf_last = true;
>> inst->reconfig = true;
>> v4l2_event_queue_fh(&inst->fh, &ev);
>> wake_up(&inst->reconf_wait);
>> --
>> 2.29.2.454.gaff20da3a2-goog
>>
--
regards,
Stan
Hi Alex,
On 12/2/20 7:34 AM, Alexandre Courbot wrote:
> DRC events can happen virtually at anytime, including when we are
> starting a seek. Should this happen, we must make sure to return to the
> DRC state, otherwise the firmware will expect buffers of the new
> resolution whereas userspace will still work with the old one.
>
> Returning to the DRC state upon resume for seeking makes sure that the
> client will get the DRC event and will reallocate the buffers to fit the
> firmware's expectations.
>
> Signed-off-by: Alexandre Courbot <[email protected]>
> ---
> drivers/media/platform/qcom/venus/vdec.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 8488411204c3..e3d0df7fd765 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -972,7 +972,10 @@ static int vdec_start_output(struct venus_inst *inst)
>
> if (inst->codec_state == VENUS_DEC_STATE_SEEK) {
> ret = venus_helper_process_initial_out_bufs(inst);
> - inst->codec_state = VENUS_DEC_STATE_DECODING;
> + if (inst->next_buf_last)
> + inst->codec_state = VENUS_DEC_STATE_DRC;
> + else
> + inst->codec_state = VENUS_DEC_STATE_DECODING;
> goto done;
> }
>
> @@ -1077,8 +1080,10 @@ static int vdec_stop_capture(struct venus_inst *inst)
> ret = hfi_session_flush(inst, HFI_FLUSH_ALL, true);
> fallthrough;
> case VENUS_DEC_STATE_DRAIN:
> - vdec_cancel_dst_buffers(inst);
> inst->codec_state = VENUS_DEC_STATE_STOPPED;
> + fallthrough;
> + case VENUS_DEC_STATE_SEEK:
> + vdec_cancel_dst_buffers(inst);
> break;
> case VENUS_DEC_STATE_DRC:
> WARN_ON(1);
> @@ -1102,6 +1107,7 @@ static int vdec_stop_output(struct venus_inst *inst)
> case VENUS_DEC_STATE_DECODING:
> case VENUS_DEC_STATE_DRAIN:
> case VENUS_DEC_STATE_STOPPED:
> + case VENUS_DEC_STATE_DRC:
> ret = hfi_session_flush(inst, HFI_FLUSH_ALL, true);
> inst->codec_state = VENUS_DEC_STATE_SEEK;
> break;
> @@ -1371,6 +1377,7 @@ static void vdec_event_change(struct venus_inst *inst,
> dev_dbg(dev, VDBGH "flush output error %d\n", ret);
> }
>
> + inst->next_buf_last = true;
Setting next_buf_last unconditionally makes me think can we reuse
inst->reconfig instead?
> inst->reconfig = true;
> v4l2_event_queue_fh(&inst->fh, &ev);
> wake_up(&inst->reconf_wait);
>
--
regards,
Stan