Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API

[Stanimir Varbanov]

Hi Malathi,

On 1/21/19 1:20 PM, mgottam@xxxxxxxxxxxxxx wrote:
> On 2019-01-17 21:50, Stanimir Varbanov wrote:
>> This refactored code for start/stop streaming vb2 operations and
>> adds a state machine handling similar to the one in stateful codec
>> API documentation. One major change is that now the HFI session is
>> started on STREAMON(OUTPUT) and stopped on REQBUF(OUTPUT,count=0),
>> during that time streamoff(cap,out) just flush buffers but doesn't
>> stop the session. The other major change is that now the capture
>> and output queues are completely separated.
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@xxxxxxxxxx>
>> ---
>> Âdrivers/media/platform/qcom/venus/core.hÂÂÂ |Â 20 +-
>> Âdrivers/media/platform/qcom/venus/helpers.c |Â 23 +-
>> Âdrivers/media/platform/qcom/venus/helpers.h |ÂÂ 5 +
>> Âdrivers/media/platform/qcom/venus/vdec.cÂÂÂ | 449 ++++++++++++++++----
>> Â4 files changed, 389 insertions(+), 108 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/core.h
>> b/drivers/media/platform/qcom/venus/core.h
>> index 79c7e816c706..5a133c203455 100644
>> --- a/drivers/media/platform/qcom/venus/core.h
>> +++ b/drivers/media/platform/qcom/venus/core.h
>> @@ -218,6 +218,15 @@ struct venus_buffer {
>>
>> Â#define to_venus_buffer(ptr)ÂÂÂ container_of(ptr, struct
>> venus_buffer, vb)
>>
>> +#define DEC_STATE_UNINITÂÂÂÂÂÂÂ 0
>> +#define DEC_STATE_INITÂÂÂÂÂÂÂÂÂÂÂ 1
>> +#define DEC_STATE_CAPTURE_SETUPÂÂÂÂÂÂÂ 2
>> +#define DEC_STATE_STOPPEDÂÂÂÂÂÂÂ 3
>> +#define DEC_STATE_SEEKÂÂÂÂÂÂÂÂÂÂÂ 4
>> +#define DEC_STATE_DRAINÂÂÂÂÂÂÂÂÂÂÂ 5
>> +#define DEC_STATE_DECODINGÂÂÂÂÂÂÂ 6
>> +#define DEC_STATE_DRCÂÂÂÂÂÂÂÂÂÂÂ 7
>> +
>> Â/**
>> Â * struct venus_inst - holds per instance paramerters
>> Â *
>> @@ -241,6 +250,10 @@ struct venus_buffer {
>> Â * @colorspace:ÂÂÂ current color space
>> Â * @quantization:ÂÂÂ current quantization
>> Â * @xfer_func:ÂÂÂ current xfer function
>> + * @codec_state:ÂÂÂ current codec API state (see DEC/ENC_STATE_)
>> + * @reconf_wait:ÂÂÂ wait queue for resolution change event
>> + * @ten_bits:ÂÂÂÂÂÂÂ does new stream is 10bits depth
>> + * @buf_count:ÂÂÂÂÂÂÂ used to count number number of buffers (reqbuf(0))
>> Â * @fps:ÂÂÂÂÂÂÂ holds current FPS
>> Â * @timeperframe:ÂÂÂ holds current time per frame structure
>> Â * @fmt_out:ÂÂÂ a reference to output format structure
>> @@ -255,8 +268,6 @@ struct venus_buffer {
>> Â * @opb_buftype:ÂÂÂ output picture buffer type
>> Â * @opb_fmt:ÂÂÂÂÂÂÂ output picture buffer raw format
>> Â * @reconfig:ÂÂÂ a flag raised by decoder when the stream resolution
>> changed
>> - * @reconfig_width:ÂÂÂ holds the new width
>> - * @reconfig_height:ÂÂÂ holds the new height
>> Â * @hfi_codec:ÂÂÂÂÂÂÂ current codec for this instance in HFI space
>> Â * @sequence_cap:ÂÂÂ a sequence counter for capture queue
>> Â * @sequence_out:ÂÂÂ a sequence counter for output queue
>> @@ -296,6 +307,9 @@ struct venus_inst {
>> ÂÂÂÂ u8 ycbcr_enc;
>> ÂÂÂÂ u8 quantization;
>> ÂÂÂÂ u8 xfer_func;
>> +ÂÂÂ unsigned int codec_state;
>> +ÂÂÂ wait_queue_head_t reconf_wait;
>> +ÂÂÂ int buf_count;
>> ÂÂÂÂ u64 fps;
>> ÂÂÂÂ struct v4l2_fract timeperframe;
>> ÂÂÂÂ const struct venus_format *fmt_out;
>> @@ -310,8 +324,6 @@ struct venus_inst {
>> ÂÂÂÂ u32 opb_buftype;
>> ÂÂÂÂ u32 opb_fmt;
>> ÂÂÂÂ bool reconfig;
>> -ÂÂÂ u32 reconfig_width;
>> -ÂÂÂ u32 reconfig_height;
>> ÂÂÂÂ u32 hfi_codec;
>> ÂÂÂÂ u32 sequence_cap;
>> ÂÂÂÂ u32 sequence_out;
>> diff --git a/drivers/media/platform/qcom/venus/helpers.c
>> b/drivers/media/platform/qcom/venus/helpers.c
>> index 637ce7b82d94..25d8cceccae4 100644
>> --- a/drivers/media/platform/qcom/venus/helpers.c
>> +++ b/drivers/media/platform/qcom/venus/helpers.c
>> @@ -1030,16 +1030,15 @@ void venus_helper_vb2_buf_queue(struct
>> vb2_buffer *vb)
>>
>> ÂÂÂÂ v4l2_m2m_buf_queue(m2m_ctx, vbuf);
>>
>> -ÂÂÂ if (!(inst->streamon_out & inst->streamon_cap))
>> -ÂÂÂÂÂÂÂ goto unlock;
>> -
>> -ÂÂÂ ret = is_buf_refed(inst, vbuf);
>> -ÂÂÂ if (ret)
>> -ÂÂÂÂÂÂÂ goto unlock;
>> +ÂÂÂ if (IS_OUT(vb->vb2_queue, inst) || IS_CAP(vb->vb2_queue, inst)) {
>> +ÂÂÂÂÂÂÂ ret = is_buf_refed(inst, vbuf);
>> +ÂÂÂÂÂÂÂ if (ret)
>> +ÂÂÂÂÂÂÂÂÂÂÂ goto unlock;
>>
>> -ÂÂÂ ret = session_process_buf(inst, vbuf);
>> -ÂÂÂ if (ret)
>> -ÂÂÂÂÂÂÂ return_buf_error(inst, vbuf);
>> +ÂÂÂÂÂÂÂ ret = session_process_buf(inst, vbuf);
>> +ÂÂÂÂÂÂÂ if (ret)
>> +ÂÂÂÂÂÂÂÂÂÂÂ return_buf_error(inst, vbuf);
>> +ÂÂÂ }
>>
>> Âunlock:
>> ÂÂÂÂ mutex_unlock(&inst->lock);
> 
> Hi Stan,
> 
> In case of encoder, we are queuing buffers only after both planes are
> streamed on.
> As we donât have any reconfig event in case of encoder,
> itâs better if we stick to the earlier implementation of queuing buffers.
> 
> So I would recommend to add a check for the same in the below way :
> 
> diff --git a/drivers/media/platform/qcom/venus/helpers.c
> b/drivers/media/platform/qcom/venus/helpers.c
> index 25d8cce..cc490fe2 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -1029,6 +1029,8 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer
> *vb)
> ÂÂÂÂÂÂÂ mutex_lock(&inst->lock);
> 
> ÂÂÂÂÂÂÂ v4l2_m2m_buf_queue(m2m_ctx, vbuf);
> +ÂÂÂÂÂÂ if (inst->session_type == VIDC_SESSION_TYPE_ENC &&
> !(inst->streamon_out & inst->streamon_cap))
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ goto unlock;
> 
> ÂÂÂÂÂÂÂ if (IS_OUT(vb->vb2_queue, inst) || IS_CAP(vb->vb2_queue, inst)) {
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ret = is_buf_refed(inst, vbuf);
> 
> Please provide your view.

I agree that this change is needed for encoder and will incorporate such
a change in next version.

-- 
regards,
Stan