On 05/14/2014 08:29 AM, Arun Kumar K wrote:
> MFCv6 encoder needs specific minimum number of buffers to
> be queued in the CAPTURE plane. This minimum number will
> be known only when the sequence header is generated.
> So we used to allow STREAMON on the CAPTURE plane only after
> sequence header is generated and checked with the minimum
> buffer requirement.
>
> But this causes a problem that we call a vb2_buffer_done
> for the sequence header buffer before doing a STREAON on the
> CAPTURE plane.
How could this ever have worked? Buffers aren't queued to the driver until
STREAMON is called, and calling vb2_buffer_done for a buffer that is not queued
first to the driver will mess up internal data (q->queued_count for one).
> This used to still work fine until this patch
> was merged -
> b3379c6 : vb2: only call start_streaming if sufficient buffers are queued
Are you testing with CONFIG_VIDEO_ADV_DEBUG set? If not, you should do so. That
will check whether all the vb2 calls are balanced.
BTW, that's a small typo in s5p_mfc_enc.c (search for 'inavlid').
> This problem should also come in earlier MFC firmware versions
> if the application calls STREAMON on CAPTURE with some delay
> after doing STREAMON on OUTPUT.
You can also play around with the min_buffers_needed field. My rule-of-thumb is
that
when start_streaming is called everything should be ready to stream. It is
painful
for drivers to have to keep track of the 'do I have enough buffers' status.
For that reason I introduced the min_buffers_needed field. What I believe you
can
do here is to set it initially to a large value, preventing start_streaming from
being called, and once you really know the minimum number of buffers that you
need
it can be updated again to the actual value.
I don't know this driver well enough to tell whether that works here or not, but
it is worth looking at IMHO.
Regards,
Hans
> So this patch keeps the header buffer until the other frame
> buffers are ready and dequeues it just before the first frame
> is ready.
>
> Signed-off-by: Arun Kumar K <[email protected]>
> ---
> Changes from v1
> - Addressed review comments from Sachin
> https://patchwork.linuxtv.org/patch/23839/
> ---
> drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 ++
> drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 6 +++++-
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> index f5404a6..cc2b96e 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> @@ -523,6 +523,7 @@ struct s5p_mfc_codec_ops {
> * @output_state: state of the output buffers queue
> * @src_bufs: information on allocated source buffers
> * @dst_bufs: information on allocated destination buffers
> + * @header_mb: buffer pointer of the encoded sequence header
> * @sequence: counter for the sequence number for v4l2
> * @dec_dst_flag: flags for buffers queued in the hardware
> * @dec_src_buf_size: size of the buffer for source buffers in
> decoding
> @@ -607,6 +608,7 @@ struct s5p_mfc_ctx {
> int src_bufs_cnt;
> struct s5p_mfc_buf dst_bufs[MFC_MAX_BUFFERS];
> int dst_bufs_cnt;
> + struct s5p_mfc_buf *header_mb;
>
> unsigned int sequence;
> unsigned long dec_dst_flag;
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> index f8c7053..0c8d593e 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
> @@ -787,7 +787,7 @@ static int enc_post_seq_start(struct s5p_mfc_ctx *ctx)
> ctx->dst_queue_cnt--;
> vb2_set_plane_payload(dst_mb->b, 0,
> s5p_mfc_hw_call(dev->mfc_ops, get_enc_strm_size, dev));
> - vb2_buffer_done(dst_mb->b, VB2_BUF_STATE_DONE);
> + ctx->header_mb = dst_mb;
> spin_unlock_irqrestore(&dev->irqlock, flags);
> }
>
> @@ -845,6 +845,10 @@ static int enc_post_frame_start(struct s5p_mfc_ctx *ctx)
> unsigned int strm_size;
> unsigned long flags;
>
> + if (ctx->header_mb) {
> + vb2_buffer_done(ctx->header_mb->b, VB2_BUF_STATE_DONE);
> + ctx->header_mb = NULL;
> + }
> slice_type = s5p_mfc_hw_call(dev->mfc_ops, get_enc_slice_type, dev);
> strm_size = s5p_mfc_hw_call(dev->mfc_ops, get_enc_strm_size, dev);
> mfc_debug(2, "Encoded slice type: %d\n", slice_type);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
