| Message ID | 20190425183546.16244-5-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Assorted CODA fixes | expand |
On Thu, 2019-04-25 at 15:35 -0300, Ezequiel Garcia wrote: > There isn't any reason to run the mem2mem job on a separate worker, > because the mem2mem framework guarantees that device_run will never > run in interrupt context. The purpose of the workqueue is to serialize BIT processor commands, currently the PIC_RUN commands issued by the mem2mem framework (as well as SEQ_INIT, SET_FRAME_BUF, and ENCODE_HEADER) against the SEQ_END command issued directly from the STREAMOFF ioctl. Further, to fully support the stateful decoder API we'll have to move SEQ_INIT out of the mem2mem device_run as well, since that should be called on queued OUTPUT buffers before the CAPTURE side is even streaming. regards Philipp
On Fri, 2019-04-26 at 10:11 +0200, Philipp Zabel wrote: > On Thu, 2019-04-25 at 15:35 -0300, Ezequiel Garcia wrote: > > There isn't any reason to run the mem2mem job on a separate worker, > > because the mem2mem framework guarantees that device_run will never > > run in interrupt context. > > The purpose of the workqueue is to serialize BIT processor commands, > currently the PIC_RUN commands issued by the mem2mem framework (as well > as SEQ_INIT, SET_FRAME_BUF, and ENCODE_HEADER) against the SEQ_END > command issued directly from the STREAMOFF ioctl. Right, but that's serialized by the coda_mutex, not by the worker, right? > Further, to fully support the stateful decoder API we'll have to move > SEQ_INIT out of the mem2mem device_run as well, since that should be > called on queued OUTPUT buffers before the CAPTURE side is even > streaming. > Isn't that already done? I see SEQ_INIT being issued in start_streaming. In what sense is this driver currently violating the decoder spec? So, returning to the serialization. I believe commands are still serialized after this change. The pic_run_work worker is only queued from .device_run. Only one job can run on a context at any given time, but multiple contexts can run in parallel. Inside the worker, the coda_mutex serializes the commands. The worker waits until the command has finished execution. So, with this commit, there is no longer a worker, but commands are still serialized by the mutex and the fact that the command is completed in .device_run. That being said, the worker does makes the serialization more clear, so I think dropping this patch is better. Perhaps adding a small comment so the purpose of pic_run_work is clear. Thanks, Eze
On Fri, 2019-04-26 at 15:19 -0300, Ezequiel Garcia wrote: > On Fri, 2019-04-26 at 10:11 +0200, Philipp Zabel wrote: > > On Thu, 2019-04-25 at 15:35 -0300, Ezequiel Garcia wrote: > > > There isn't any reason to run the mem2mem job on a separate worker, > > > because the mem2mem framework guarantees that device_run will never > > > run in interrupt context. > > > > The purpose of the workqueue is to serialize BIT processor commands, > > currently the PIC_RUN commands issued by the mem2mem framework (as well > > as SEQ_INIT, SET_FRAME_BUF, and ENCODE_HEADER) against the SEQ_END > > command issued directly from the STREAMOFF ioctl. > > Right, but that's serialized by the coda_mutex, not by the worker, right? Hmm, coda_start_decoding as called from .start_streaming is indeed just serialized by the device coda_mutex. I would prefer this to be scheduled as a work item as well though, those are way easier to reason about. As an aside, while SEQ_INIT on its own is fast, technically .start_streaming (or .buf_queue, see below) wouldn't even have to wait until the SEQ_INIT command has finished, in case the BIT processor is busy decoding for another context. > > Further, to fully support the stateful decoder API we'll have to move > > SEQ_INIT out of the mem2mem device_run as well, since that should be > > called on queued OUTPUT buffers before the CAPTURE side is even > > streaming. > > Isn't that already done? I see SEQ_INIT being issued in start_streaming. > In what sense is this driver currently violating the decoder spec? I suppose it should try SEQ_INIT on every OUTPUT .buf_queue as long as initialization hasn't succeeded, not only on .start_streaming of the second queue (whichever is last). Right now, the buffers fed into the output queue before STREAMON on both OUTPUT and CAPTURE must already contain headers, or stream start will fail. > So, returning to the serialization. I believe commands are still > serialized after this change. While I'm not sure if any odd corner cases could still arise with this change, I think you are right. I remember having a few deadlock issues with the locking before adding the workqueue, but that was before we could just wait for the device to finish in .device_run. > The pic_run_work worker is only queued from .device_run. > Only one job can run on a context at any given time, > but multiple contexts can run in parallel. > > Inside the worker, the coda_mutex serializes the commands. > The worker waits until the command has finished execution. > > So, with this commit, there is no longer a worker, but commands > are still serialized by the mutex and the fact that the command > is completed in .device_run. > > That being said, the worker does makes the serialization > more clear, so I think dropping this patch is better. I agree with both analysis and conclusion. > Perhaps adding a small comment so the purpose of pic_run_work > is clear. Yes, that is a good idea. regards Philipp
diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c index 617d4547ec82..e0227593a649 100644 --- a/drivers/media/platform/coda/coda-common.c +++ b/drivers/media/platform/coda/coda-common.c @@ -1306,14 +1306,6 @@ static void coda_device_run(void *m2m_priv) { struct coda_ctx *ctx = m2m_priv; struct coda_dev *dev = ctx->dev; - - queue_work(dev->workqueue, &ctx->pic_run_work); -} - -static void coda_pic_run_work(struct work_struct *work) -{ - struct coda_ctx *ctx = container_of(work, struct coda_ctx, pic_run_work); - struct coda_dev *dev = ctx->dev; int ret; mutex_lock(&ctx->buffer_mutex); @@ -2233,7 +2225,6 @@ static int coda_open(struct file *file) ctx->ops = ctx->cvd->ops; ctx->use_bit = !ctx->cvd->direct; init_completion(&ctx->completion); - INIT_WORK(&ctx->pic_run_work, coda_pic_run_work); if (ctx->ops->seq_end_work) INIT_WORK(&ctx->seq_end_work, ctx->ops->seq_end_work); v4l2_fh_init(&ctx->fh, video_devdata(file)); diff --git a/drivers/media/platform/coda/coda.h b/drivers/media/platform/coda/coda.h index 31c80bda2c0b..6870edeb6324 100644 --- a/drivers/media/platform/coda/coda.h +++ b/drivers/media/platform/coda/coda.h @@ -194,7 +194,6 @@ struct coda_context_ops { struct coda_ctx { struct coda_dev *dev; struct mutex buffer_mutex; - struct work_struct pic_run_work; struct work_struct seq_end_work; struct completion completion; const struct coda_video_device *cvd;
There isn't any reason to run the mem2mem job on a separate worker, because the mem2mem framework guarantees that device_run will never run in interrupt context. Therefore, get rid of the extra pic_run_work worker, and instead run the job in the context of .device_run itself. Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- drivers/media/platform/coda/coda-common.c | 9 --------- drivers/media/platform/coda/coda.h | 1 - 2 files changed, 10 deletions(-)