diff mbox series

media: venus: preserve DRC state across seeks

Message ID 20201202053424.3286774-1-acourbot@chromium.org (mailing list archive)
State New, archived
Headers show
Series media: venus: preserve DRC state across seeks | expand

Commit Message

Alexandre Courbot Dec. 2, 2020, 5:34 a.m. UTC 
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 <acourbot@chromium.org>
---
 drivers/media/platform/qcom/venus/vdec.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

kernel test robot Dec. 2, 2020, 9:28 a.m. UTC | #1 
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 <lkp@intel.com>

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/kbuild-all@lists.01.org
Alexandre Courbot Dec. 2, 2020, 10:24 a.m. UTC | #2 
On Wed, Dec 2, 2020 at 2:34 PM Alexandre Courbot <acourbot@chromium.org> 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 <acourbot@chromium.org>
> ---
>  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
>
Stanimir Varbanov Dec. 2, 2020, 10:28 a.m. UTC | #3 
Hi,

On 12/2/20 12:24 PM, Alexandre Courbot wrote:
> On Wed, Dec 2, 2020 at 2:34 PM Alexandre Courbot <acourbot@chromium.org> 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 <acourbot@chromium.org>
>> ---
>>  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
>>
Stanimir Varbanov Dec. 17, 2020, 9:46 a.m. UTC | #4 
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 <acourbot@chromium.org>
> ---
>  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);
>
diff mbox series

Patch

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);