| Message ID | 20230524135737.2557837-1-senozhatsky@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCHv2] media: venus: provide video device lock | expand |
On 5/24/2023 8:14 PM, Hans Verkuil wrote: > On 24/05/2023 16:29, Bryan O'Donoghue wrote: >> On 24/05/2023 15:12, Sergey Senozhatsky wrote: >>> Video device has to provide ->lock so that __video_do_ioctl() >>> can serialize IOCTL calls. Provided dedicated enc/dec mutex-s >>> for that purpose. Why do we need to serialize at device context ? Please share some details on the issue faced leading to the serialization. This may impact performance, let say, when we have multiple concurrent video sessions running at the same time and the ioctl for one session have to wait if the lock is taken by another session ioctl. >>> >>> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > > Since these are m2m devices, I think this should set vfh->m2m_ctx->q_lock > instead. > > The vb2_queue is per filehandle for such devices, so by just setting > vdev->lock you will have all vb2_queues use the same mutex. > > Instead the struct v4l2_m2m_ctx q_lock pointer, if set, will use that > mutex for all vb2 operations. > > I think you can set it to the 'lock' mutex in struct venus_inst. IIUC, the suggestion is to use the 'lock' in struct venus_inst while initializing the queue. This might lead to deadlock as the same lock is used during vb2 operations in driver. Might be introducing a new lock for this purpose in struct venus_inst would do, unless we are trying to serialize at video device (or core) context. > > Regards, > > Hans > >>> --- >>> drivers/media/platform/qcom/venus/core.h | 4 ++++ >>> drivers/media/platform/qcom/venus/vdec.c | 2 ++ >>> drivers/media/platform/qcom/venus/venc.c | 2 ++ >>> 3 files changed, 8 insertions(+) >>> >>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h >>> index 4f81669986ba..b6c9a653a007 100644 >>> --- a/drivers/media/platform/qcom/venus/core.h >>> +++ b/drivers/media/platform/qcom/venus/core.h >>> @@ -113,7 +113,9 @@ struct venus_format { >>> * @opp_pmdomain: an OPP power-domain >>> * @resets: an array of reset signals >>> * @vdev_dec: a reference to video device structure for decoder instances >>> + * @vdev_dec_lock: decoder instance video device ioctl lock >>> * @vdev_enc: a reference to video device structure for encoder instances >>> + * @vdev_enc_lock: encoder instance video device ioctl lock >>> * @v4l2_dev: a holder for v4l2 device structure >>> * @res: a reference to venus resources structure >>> * @dev: convenience struct device pointer >>> @@ -165,7 +167,9 @@ struct venus_core { >>> struct device *opp_pmdomain; >>> struct reset_control *resets[VIDC_RESETS_NUM_MAX]; >>> struct video_device *vdev_dec; >>> + struct mutex vdev_dec_lock; >>> struct video_device *vdev_enc; >>> + struct mutex vdev_enc_lock; >>> struct v4l2_device v4l2_dev; >>> const struct venus_resources *res; >>> struct device *dev; >>> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c >>> index 51a53bf82bd3..7e9363714bfb 100644 >>> --- a/drivers/media/platform/qcom/venus/vdec.c >>> +++ b/drivers/media/platform/qcom/venus/vdec.c >>> @@ -1760,6 +1760,7 @@ static int vdec_probe(struct platform_device *pdev) >>> if (!vdev) >>> return -ENOMEM; >>> + mutex_init(&core->vdev_dec_lock); >>> strscpy(vdev->name, "qcom-venus-decoder", sizeof(vdev->name)); >>> vdev->release = video_device_release; >>> vdev->fops = &vdec_fops; >>> @@ -1767,6 +1768,7 @@ static int vdec_probe(struct platform_device *pdev) >>> vdev->vfl_dir = VFL_DIR_M2M; >>> vdev->v4l2_dev = &core->v4l2_dev; >>> vdev->device_caps = V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING; >>> + vdev->lock = &core->vdev_dec_lock; >>> ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1); >>> if (ret) >>> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c >>> index 4666f42feea3..8522ed339d5d 100644 >>> --- a/drivers/media/platform/qcom/venus/venc.c >>> +++ b/drivers/media/platform/qcom/venus/venc.c >>> @@ -1558,6 +1558,7 @@ static int venc_probe(struct platform_device *pdev) >>> if (!vdev) >>> return -ENOMEM; >>> + mutex_init(&core->vdev_enc_lock); >>> strscpy(vdev->name, "qcom-venus-encoder", sizeof(vdev->name)); >>> vdev->release = video_device_release; >>> vdev->fops = &venc_fops; >>> @@ -1565,6 +1566,7 @@ static int venc_probe(struct platform_device *pdev) >>> vdev->vfl_dir = VFL_DIR_M2M; >>> vdev->v4l2_dev = &core->v4l2_dev; >>> vdev->device_caps = V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING; >>> + vdev->lock = &core->vdev_enc_lock; >>> ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1); >>> if (ret) >> >> LGTM >> >> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >
On (23/05/25 09:22), Hans Verkuil wrote: > >> Since these are m2m devices, I think this should set vfh->m2m_ctx->q_lock > >> instead. > >> > >> The vb2_queue is per filehandle for such devices, so by just setting > >> vdev->lock you will have all vb2_queues use the same mutex. > >> > >> Instead the struct v4l2_m2m_ctx q_lock pointer, if set, will use that > >> mutex for all vb2 operations. > >> > >> I think you can set it to the 'lock' mutex in struct venus_inst. > > > > IIUC, the suggestion is to use the 'lock' in struct venus_inst while > > initializing the queue. This might lead to deadlock as the same lock is used > > during vb2 operations in driver. Might be introducing a new lock for this > > purpose in struct venus_inst would do, unless we are trying to serialize at > > video device (or core) context. > > For the record, I have not analyzed how that lock is used in the driver, > so if a new mutex has to be added to venus_inst rather than reusing the > existing one, then that's fine by me. > > But it should be a instance-specific mutex, not one at the device level. Thanks for your help Hans. I added a per-instance queue mutex [1], so if no one sees any problems with it then I can send a formal patch later on. [1] https://lore.kernel.org/linux-media/20230525005312.GC30543@google.com
On 5/25/2023 6:23 AM, Sergey Senozhatsky wrote: > On (23/05/24 22:06), Vikash Garodia wrote: >>> Instead the struct v4l2_m2m_ctx q_lock pointer, if set, will use that >>> mutex for all vb2 operations. >>> >>> I think you can set it to the 'lock' mutex in struct venus_inst. >> >> IIUC, the suggestion is to use the 'lock' in struct venus_inst while >> initializing the queue. This might lead to deadlock as the same lock is used >> during vb2 operations in driver. Might be introducing a new lock for this >> purpose in struct venus_inst would do, unless we are trying to serialize at >> video device (or core) context. > > Something like this? > > Video device has to provide a lock so that __video_do_ioctl() > can serialize IOCTL calls. Introduce a dedicated venus_inst > mutex (which is set a ctx ->q_lock) for the purpose of vb2 > operations synchronization. > > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > --- > drivers/media/platform/qcom/venus/core.h | 2 ++ > drivers/media/platform/qcom/venus/vdec.c | 4 ++++ > drivers/media/platform/qcom/venus/venc.c | 3 +++ > 3 files changed, 9 insertions(+) > > diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > index 4f81669986ba..6ac5236d6888 100644 > --- a/drivers/media/platform/qcom/venus/core.h > +++ b/drivers/media/platform/qcom/venus/core.h > @@ -389,6 +389,7 @@ enum venus_inst_modes { > * @sequence_out: a sequence counter for output queue > * @m2m_dev: a reference to m2m device structure > * @m2m_ctx: a reference to m2m context structure > + * @ctx_queue_lock: a lock to serialize video device ioctl calls suggestion: we can keep this as ctx_q_lock. > * @state: current state of the instance > * @done: a completion for sync HFI operation > * @error: an error returned during last HFI sync operation > @@ -460,6 +461,7 @@ struct venus_inst { > u32 sequence_out; > struct v4l2_m2m_dev *m2m_dev; > struct v4l2_m2m_ctx *m2m_ctx; > + struct mutex ctx_queue_lock; > unsigned int state; > struct completion done; > unsigned int error; > diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c > index 51a53bf82bd3..2caeba5b6378 100644 > --- a/drivers/media/platform/qcom/venus/vdec.c > +++ b/drivers/media/platform/qcom/venus/vdec.c > @@ -1641,6 +1641,7 @@ static int vdec_open(struct file *file) > INIT_LIST_HEAD(&inst->internalbufs); > INIT_LIST_HEAD(&inst->list); > mutex_init(&inst->lock); > + mutex_init(&inst->ctx_queue_lock); > > inst->core = core; > inst->session_type = VIDC_SESSION_TYPE_DEC; > @@ -1684,8 +1685,10 @@ static int vdec_open(struct file *file) > goto err_m2m_release; > } > > + > v4l2_fh_init(&inst->fh, core->vdev_dec); > > + inst->m2m_ctx->q_lock = &inst->ctx_queue_lock; Better to do this in queue_init callback i.e "m2m_queue_init" in vdec.c. src_vq->lock = &inst->ctx_q_lock; ... dst_vq->lock = src_vq->lock; > inst->fh.ctrl_handler = &inst->ctrl_handler; > v4l2_fh_add(&inst->fh); > inst->fh.m2m_ctx = inst->m2m_ctx; > @@ -1716,6 +1719,7 @@ static int vdec_close(struct file *file) > ida_destroy(&inst->dpb_ids); > hfi_session_destroy(inst); > mutex_destroy(&inst->lock); > + mutex_destroy(&inst->ctx_queue_lock); > v4l2_fh_del(&inst->fh); > v4l2_fh_exit(&inst->fh); > > diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c > index 4666f42feea3..4292b299f014 100644 > --- a/drivers/media/platform/qcom/venus/venc.c > +++ b/drivers/media/platform/qcom/venus/venc.c > @@ -1443,6 +1443,7 @@ static int venc_open(struct file *file) > INIT_LIST_HEAD(&inst->internalbufs); > INIT_LIST_HEAD(&inst->list); > mutex_init(&inst->lock); > + mutex_init(&inst->ctx_queue_lock); > > inst->core = core; > inst->session_type = VIDC_SESSION_TYPE_ENC; > @@ -1483,6 +1484,7 @@ static int venc_open(struct file *file) > > v4l2_fh_init(&inst->fh, core->vdev_enc); > > + inst->m2m_ctx->q_lock = &inst->ctx_queue_lock; Same comment applies here. This can be moved to "m2m_queue_init" in venc.c. > inst->fh.ctrl_handler = &inst->ctrl_handler; > v4l2_fh_add(&inst->fh); > inst->fh.m2m_ctx = inst->m2m_ctx; > @@ -1512,6 +1514,7 @@ static int venc_close(struct file *file) > venc_ctrl_deinit(inst); > hfi_session_destroy(inst); > mutex_destroy(&inst->lock); > + mutex_destroy(&inst->ctx_queue_lock); > v4l2_fh_del(&inst->fh); > v4l2_fh_exit(&inst->fh); >
On (23/05/25 16:32), Vikash Garodia wrote: > > * @sequence_out: a sequence counter for output queue > > * @m2m_dev: a reference to m2m device structure > > * @m2m_ctx: a reference to m2m context structure > > + * @ctx_queue_lock: a lock to serialize video device ioctl calls > suggestion: we can keep this as ctx_q_lock. Ack > > v4l2_fh_init(&inst->fh, core->vdev_dec); > > > > + inst->m2m_ctx->q_lock = &inst->ctx_queue_lock; > Better to do this in queue_init callback i.e "m2m_queue_init" in vdec.c. > src_vq->lock = &inst->ctx_q_lock; > ... > dst_vq->lock = src_vq->lock; Ack > > @@ -1483,6 +1484,7 @@ static int venc_open(struct file *file) > > > > v4l2_fh_init(&inst->fh, core->vdev_enc); > > > > + inst->m2m_ctx->q_lock = &inst->ctx_queue_lock; > Same comment applies here. This can be moved to "m2m_queue_init" in venc.c. Ack
Hi Hans, On 5/24/2023 8:14 PM, Hans Verkuil wrote: > On 24/05/2023 16:29, Bryan O'Donoghue wrote: >> On 24/05/2023 15:12, Sergey Senozhatsky wrote: >>> Video device has to provide ->lock so that __video_do_ioctl() >>> can serialize IOCTL calls. Provided dedicated enc/dec mutex-s >>> for that purpose. >>> >>> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > > Since these are m2m devices, I think this should set vfh->m2m_ctx->q_lock > instead. > > The vb2_queue is per filehandle for such devices, so by just setting > vdev->lock you will have all vb2_queues use the same mutex. > > Instead the struct v4l2_m2m_ctx q_lock pointer, if set, will use that > mutex for all vb2 operations. Recently we came across a race between queryctrl and s_fmt. Above lock would synchronize the operations for IOCTL with flag INFO_FL_QUEUE. Any suggestion on how other IOCTLs can be serialized as well, for ex s_fmt and queryctrl which are of type INFO_FL_PRIO and INFO_FL_CTRL. Thanks, Vikash > I think you can set it to the 'lock' mutex in struct venus_inst. > > Regards, > > Hans
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h index 4f81669986ba..23259fa114c7 100644 --- a/drivers/media/platform/qcom/venus/core.h +++ b/drivers/media/platform/qcom/venus/core.h @@ -113,7 +113,9 @@ struct venus_format { * @opp_pmdomain: an OPP power-domain * @resets: an array of reset signals * @vdev_dec: a reference to video device structure for decoder instances + * @@vdev_dec_lock: decoder instance video device ioctl lock * @vdev_enc: a reference to video device structure for encoder instances + * @@vdev_enc_lock: encoder instance video device ioctl lock * @v4l2_dev: a holder for v4l2 device structure * @res: a reference to venus resources structure * @dev: convenience struct device pointer @@ -165,7 +167,9 @@ struct venus_core { struct device *opp_pmdomain; struct reset_control *resets[VIDC_RESETS_NUM_MAX]; struct video_device *vdev_dec; + struct mutex vdev_dec_lock; struct video_device *vdev_enc; + struct mutex vdev_enc_lock; struct v4l2_device v4l2_dev; const struct venus_resources *res; struct device *dev; diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c index 51a53bf82bd3..7e9363714bfb 100644 --- a/drivers/media/platform/qcom/venus/vdec.c +++ b/drivers/media/platform/qcom/venus/vdec.c @@ -1760,6 +1760,7 @@ static int vdec_probe(struct platform_device *pdev) if (!vdev) return -ENOMEM; + mutex_init(&core->vdev_dec_lock); strscpy(vdev->name, "qcom-venus-decoder", sizeof(vdev->name)); vdev->release = video_device_release; vdev->fops = &vdec_fops; @@ -1767,6 +1768,7 @@ static int vdec_probe(struct platform_device *pdev) vdev->vfl_dir = VFL_DIR_M2M; vdev->v4l2_dev = &core->v4l2_dev; vdev->device_caps = V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING; + vdev->lock = &core->vdev_dec_lock; ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1); if (ret) diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c index 4666f42feea3..8522ed339d5d 100644 --- a/drivers/media/platform/qcom/venus/venc.c +++ b/drivers/media/platform/qcom/venus/venc.c @@ -1558,6 +1558,7 @@ static int venc_probe(struct platform_device *pdev) if (!vdev) return -ENOMEM; + mutex_init(&core->vdev_enc_lock); strscpy(vdev->name, "qcom-venus-encoder", sizeof(vdev->name)); vdev->release = video_device_release; vdev->fops = &venc_fops; @@ -1565,6 +1566,7 @@ static int venc_probe(struct platform_device *pdev) vdev->vfl_dir = VFL_DIR_M2M; vdev->v4l2_dev = &core->v4l2_dev; vdev->device_caps = V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING; + vdev->lock = &core->vdev_enc_lock; ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1); if (ret)
Video device has to provide ->lock so that __video_do_ioctl() can serialize IOCTL calls. Provided dedicated enc/dec mutex-s for that purpose. Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> --- drivers/media/platform/qcom/venus/core.h | 4 ++++ drivers/media/platform/qcom/venus/vdec.c | 2 ++ drivers/media/platform/qcom/venus/venc.c | 2 ++ 3 files changed, 8 insertions(+)