From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stanimir Varbanov Subject: Re: [PATCH 10/10] venus: dec: make decoder compliant with stateful codec API Date: Thu, 24 Jan 2019 12:24:00 +0200 Message-ID: <1663ea51-1fa0-6e29-715d-9d67aabb5ca9@linaro.org> References: <20190117162008.25217-1-stanimir.varbanov@linaro.org> <20190117162008.25217-11-stanimir.varbanov@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: mgottam@codeaurora.org, Stanimir Varbanov Cc: linux-media@vger.kernel.org, Mauro Carvalho Chehab , Hans Verkuil , linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, Vikash Garodia , Tomasz Figa , Alexandre Courbot List-Id: linux-arm-msm@vger.kernel.org Hi Malathi, On 1/21/19 1:20 PM, mgottam@codeaurora.org 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 >> --- >>  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