Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2] venus: vdec: Handle DRC after drain
To:     Fritz Koenig <frkoenig@chromium.org>, linux-media@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        vgarodia@codeaurora.org, dikshita@codeaurora.org,
        acourbot@chromium.org
References: <20201201042322.3346817-1-frkoenig@chromium.org>
From:   Stanimir Varbanov <stanimir.varbanov@linaro.org>
Message-ID: <57cc5999-e54c-219c-812b-71b214dbe760@linaro.org>
Date:   Wed, 2 Dec 2020 00:34:51 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20201201042322.3346817-1-frkoenig@chromium.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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:
> 

-- 
regards,
Stan