On 02/12/15 13:08, tiffany lin wrote:
We need MTK_STATE_ABORT to inform encoder thread (mtk_venc_worker) that
stop encodeing job from stopped ctx instance.
When user space qbuf, we need to make sure everything is ready to sent
buf to encode.
Agree that you need a flag here. In fact currently you have two,
MTK_STATE_ABORT and an unused one called aborting.
You need to be very careful with these flags though. They are a magnet
for data race bugs (especially combined with SMP).
For example at present I can't see any locking in the worker code. This
means there is nothing to make all those read-modify-write sequences
that manage the state atomic (thus risking state corruption).
We prevent that one function set the flag and others clear the flag.
So there is no special lock to protect state.
What prevents concurrent access from different calling contexts? It
looks to me like the work on the work queue may run concurrently with
the ioctl calls.
+static void vb2ops_venc_stop_streaming(struct vb2_queue *q)
+{
+ struct mtk_vcodec_ctx *ctx = vb2_get_drv_priv(q);
+ struct v4l2_device *v4l2_dev = &ctx->dev->v4l2_dev;
+ struct vb2_buffer *src_buf, *dst_buf;
+ int retry;
+ int ret;
+
+ mtk_v4l2_debug(2, "[%d]-> type=%d", ctx->idx, q->type);
+
+ retry = 0;
+ while ((ctx->state & MTK_STATE_RUNNING) && (retry < 10)) {
+ mtk_vcodec_clean_ctx_int_flags(ctx);
+ ctx->state |= MTK_STATE_ABORT;
As a simple example I think the above line can run concurrently with the
following code near the end of the worker code.
+ ctx->state &= ~MTK_STATE_RUNNING;
+ v4l2_m2m_job_finish(ctx->dev->m2m_dev_enc, ctx->m2m_ctx);
If I'm right then the state of the flags can definitely get clobbered
due to the read-modify-write actions on the state.
Daniel.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
