| Message ID | 1405613112-22442-7-git-send-email-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/17/2014 06:05 PM, Philipp Zabel wrote: > From: Michael Olbrich <m.olbrich@pengutronix.de> > > coda_fill_bitstream() calls v4l2_m2m_buf_done() which is no longer allowed > before streaming was started. > Delay coda_fill_bitstream() until coda_start_streaming() and explicitly set > 'start_streaming_called' before calling coda_fill_bitstream() > > Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/media/platform/coda.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/coda.c b/drivers/media/platform/coda.c > index 141ec29..3d57986 100644 > --- a/drivers/media/platform/coda.c > +++ b/drivers/media/platform/coda.c > @@ -1682,7 +1682,8 @@ static void coda_buf_queue(struct vb2_buffer *vb) > } > mutex_lock(&ctx->bitstream_mutex); > v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, vb); > - coda_fill_bitstream(ctx); > + if (vb2_is_streaming(vb->vb2_queue)) > + coda_fill_bitstream(ctx); > mutex_unlock(&ctx->bitstream_mutex); > } else { > v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, vb); > @@ -2272,6 +2273,15 @@ static int coda_start_streaming(struct vb2_queue *q, unsigned int count) > q_data_src = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT); > if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) { > if (q_data_src->fourcc == V4L2_PIX_FMT_H264) { > + struct vb2_queue *vq; > + /* start_streaming_called must be set, for v4l2_m2m_buf_done() */ > + vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT); > + vq->start_streaming_called = 1; Why set start_streaming_called to 1? It is set before calling start_streaming. This is a recent change in videobuf2-core.c though. BTW, you should test with CONFIG_VIDEO_ADV_DEBUG on and force start_streaming errors to check whether vb2_buffer_done(buf, VB2_BUF_STATE_DEQUEUED) is called for the queued buffers in case of start_streaming failure. With that config option vb2 will complain about unbalanced vb2 operations. I strongly suspect this code does not play well with this. BTW, isn't it time to split up the coda driver? Over 3000 lines... Regards, Hans > + /* copy the buffers that where queued before streamon */ > + mutex_lock(&ctx->bitstream_mutex); > + coda_fill_bitstream(ctx); > + mutex_unlock(&ctx->bitstream_mutex); > + > if (coda_get_bitstream_payload(ctx) < 512) > return -EINVAL; > } else { > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, thank you for the review. Am Donnerstag, den 17.07.2014, 18:17 +0200 schrieb Hans Verkuil: > > @@ -2272,6 +2273,15 @@ static int coda_start_streaming(struct vb2_queue *q, unsigned int count) > > q_data_src = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT); > > if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) { > > if (q_data_src->fourcc == V4L2_PIX_FMT_H264) { > > + struct vb2_queue *vq; > > + /* start_streaming_called must be set, for v4l2_m2m_buf_done() */ > > + vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT); > > + vq->start_streaming_called = 1; > > Why set start_streaming_called to 1? It is set before calling start_streaming. > This is a recent change in videobuf2-core.c though. Because I hadn't seen "[media] v4l: vb2: Fix stream start and buffer completion race" yet. I'll update this patch. > BTW, you should test with CONFIG_VIDEO_ADV_DEBUG on and force start_streaming > errors to check whether vb2_buffer_done(buf, VB2_BUF_STATE_DEQUEUED) is called > for the queued buffers in case of start_streaming failure. > > With that config option vb2 will complain about unbalanced vb2 operations. > > I strongly suspect this code does not play well with this. Yes, I will fix this. > BTW, isn't it time to split up the coda driver? Over 3000 lines... Indeed. This is also on my list. regards Philipp -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/platform/coda.c b/drivers/media/platform/coda.c index 141ec29..3d57986 100644 --- a/drivers/media/platform/coda.c +++ b/drivers/media/platform/coda.c @@ -1682,7 +1682,8 @@ static void coda_buf_queue(struct vb2_buffer *vb) } mutex_lock(&ctx->bitstream_mutex); v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, vb); - coda_fill_bitstream(ctx); + if (vb2_is_streaming(vb->vb2_queue)) + coda_fill_bitstream(ctx); mutex_unlock(&ctx->bitstream_mutex); } else { v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, vb); @@ -2272,6 +2273,15 @@ static int coda_start_streaming(struct vb2_queue *q, unsigned int count) q_data_src = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT); if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) { if (q_data_src->fourcc == V4L2_PIX_FMT_H264) { + struct vb2_queue *vq; + /* start_streaming_called must be set, for v4l2_m2m_buf_done() */ + vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT); + vq->start_streaming_called = 1; + /* copy the buffers that where queued before streamon */ + mutex_lock(&ctx->bitstream_mutex); + coda_fill_bitstream(ctx); + mutex_unlock(&ctx->bitstream_mutex); + if (coda_get_bitstream_payload(ctx) < 512) return -EINVAL; } else {