* [PATCH] media: venus: provide video device lock @ 2023-05-24 1:36 Sergey Senozhatsky 2023-05-24 12:40 ` Sergey Senozhatsky 2023-05-24 13:56 ` [PATCHv2] " Sergey Senozhatsky 0 siblings, 2 replies; 14+ messages in thread From: Sergey Senozhatsky @ 2023-05-24 1:36 UTC (permalink / raw) To: Stanimir Varbanov, Vikash Garodia, Andy Gross, Bjorn Andersson, Konrad Dybcio Cc: Tomasz Figa, Hans Verkuil, linux-media, linux-arm-msm, Sergey Senozhatsky Video device has to provide ->lock so that __video_do_ioctl() can serialize IOCTL calls. Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> --- drivers/media/platform/qcom/venus/vdec.c | 1 + drivers/media/platform/qcom/venus/venc.c | 1 + 2 files changed, 2 insertions(+) diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c index 51a53bf82bd3..90b359074c35 100644 --- a/drivers/media/platform/qcom/venus/vdec.c +++ b/drivers/media/platform/qcom/venus/vdec.c @@ -1767,6 +1767,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->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..e6b63ff5bc0e 100644 --- a/drivers/media/platform/qcom/venus/venc.c +++ b/drivers/media/platform/qcom/venus/venc.c @@ -1565,6 +1565,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->lock; ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1); if (ret) -- 2.40.1.698.g37aff9b760-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] media: venus: provide video device lock 2023-05-24 1:36 [PATCH] media: venus: provide video device lock Sergey Senozhatsky @ 2023-05-24 12:40 ` Sergey Senozhatsky 2023-05-24 13:56 ` [PATCHv2] " Sergey Senozhatsky 1 sibling, 0 replies; 14+ messages in thread From: Sergey Senozhatsky @ 2023-05-24 12:40 UTC (permalink / raw) To: Stanimir Varbanov, Vikash Garodia, Andy Gross Cc: Bjorn Andersson, Konrad Dybcio, Tomasz Figa, Hans Verkuil, linux-media, linux-arm-msm, Sergey Senozhatsky On (23/05/24 10:36), Sergey Senozhatsky wrote: > Video device has to provide ->lock so that __video_do_ioctl() > can serialize IOCTL calls. > > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > --- > drivers/media/platform/qcom/venus/vdec.c | 1 + > drivers/media/platform/qcom/venus/venc.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c > index 51a53bf82bd3..90b359074c35 100644 > --- a/drivers/media/platform/qcom/venus/vdec.c > +++ b/drivers/media/platform/qcom/venus/vdec.c > @@ -1767,6 +1767,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->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..e6b63ff5bc0e 100644 > --- a/drivers/media/platform/qcom/venus/venc.c > +++ b/drivers/media/platform/qcom/venus/venc.c > @@ -1565,6 +1565,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->lock; Or should it use a dedicated mutex to serialize video_do_ioctl()? ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCHv2] media: venus: provide video device lock 2023-05-24 1:36 [PATCH] media: venus: provide video device lock Sergey Senozhatsky 2023-05-24 12:40 ` Sergey Senozhatsky @ 2023-05-24 13:56 ` Sergey Senozhatsky 2023-05-24 14:12 ` [PATCHv3] " Sergey Senozhatsky 1 sibling, 1 reply; 14+ messages in thread From: Sergey Senozhatsky @ 2023-05-24 13:56 UTC (permalink / raw) To: Stanimir Varbanov, Vikash Garodia, Andy Gross, Bjorn Andersson, Konrad Dybcio Cc: Tomasz Figa, Hans Verkuil, linux-media, linux-arm-msm, Sergey Senozhatsky 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(+) 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) -- 2.40.1.698.g37aff9b760-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCHv3] media: venus: provide video device lock 2023-05-24 13:56 ` [PATCHv2] " Sergey Senozhatsky @ 2023-05-24 14:12 ` Sergey Senozhatsky 2023-05-24 14:29 ` Bryan O'Donoghue 0 siblings, 1 reply; 14+ messages in thread From: Sergey Senozhatsky @ 2023-05-24 14:12 UTC (permalink / raw) To: Stanimir Varbanov, Vikash Garodia, Andy Gross, Bjorn Andersson, Konrad Dybcio Cc: Tomasz Figa, Hans Verkuil, linux-media, linux-arm-msm, Sergey Senozhatsky 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(+) 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) -- 2.40.1.698.g37aff9b760-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCHv3] media: venus: provide video device lock 2023-05-24 14:12 ` [PATCHv3] " Sergey Senozhatsky @ 2023-05-24 14:29 ` Bryan O'Donoghue 2023-05-24 14:44 ` Hans Verkuil 0 siblings, 1 reply; 14+ messages in thread From: Bryan O'Donoghue @ 2023-05-24 14:29 UTC (permalink / raw) To: Sergey Senozhatsky, Stanimir Varbanov, Vikash Garodia, Andy Gross, Bjorn Andersson, Konrad Dybcio Cc: Tomasz Figa, Hans Verkuil, linux-media, linux-arm-msm 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> > --- > 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> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv3] media: venus: provide video device lock 2023-05-24 14:29 ` Bryan O'Donoghue @ 2023-05-24 14:44 ` Hans Verkuil 2023-05-24 16:36 ` Vikash Garodia 2023-06-01 10:16 ` Vikash Garodia 0 siblings, 2 replies; 14+ messages in thread From: Hans Verkuil @ 2023-05-24 14:44 UTC (permalink / raw) To: Bryan O'Donoghue, Sergey Senozhatsky, Stanimir Varbanov, Vikash Garodia, Andy Gross, Bjorn Andersson, Konrad Dybcio Cc: Tomasz Figa, linux-media, linux-arm-msm 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. I think you can set it to the 'lock' mutex in struct venus_inst. 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> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv3] media: venus: provide video device lock 2023-05-24 14:44 ` Hans Verkuil @ 2023-05-24 16:36 ` Vikash Garodia 2023-05-25 0:53 ` Sergey Senozhatsky 2023-05-25 7:22 ` Hans Verkuil 2023-06-01 10:16 ` Vikash Garodia 1 sibling, 2 replies; 14+ messages in thread From: Vikash Garodia @ 2023-05-24 16:36 UTC (permalink / raw) To: Hans Verkuil, Bryan O'Donoghue, Sergey Senozhatsky, Stanimir Varbanov, Andy Gross, Bjorn Andersson, Konrad Dybcio Cc: Tomasz Figa, linux-media, linux-arm-msm 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> > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv3] media: venus: provide video device lock 2023-05-24 16:36 ` Vikash Garodia @ 2023-05-25 0:53 ` Sergey Senozhatsky 2023-05-25 11:02 ` Vikash Garodia 2023-05-25 7:22 ` Hans Verkuil 1 sibling, 1 reply; 14+ messages in thread From: Sergey Senozhatsky @ 2023-05-25 0:53 UTC (permalink / raw) To: Vikash Garodia Cc: Hans Verkuil, Bryan O'Donoghue, Sergey Senozhatsky, Stanimir Varbanov, Andy Gross, Bjorn Andersson, Konrad Dybcio, Tomasz Figa, linux-media, linux-arm-msm 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 * @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; 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; 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); -- 2.40.1.698.g37aff9b760-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCHv3] media: venus: provide video device lock 2023-05-25 0:53 ` Sergey Senozhatsky @ 2023-05-25 11:02 ` Vikash Garodia 2023-05-26 3:35 ` Sergey Senozhatsky 0 siblings, 1 reply; 14+ messages in thread From: Vikash Garodia @ 2023-05-25 11:02 UTC (permalink / raw) To: Sergey Senozhatsky Cc: Hans Verkuil, Bryan O'Donoghue, Stanimir Varbanov, Andy Gross, Bjorn Andersson, Konrad Dybcio, Tomasz Figa, linux-media, linux-arm-msm 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); > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv3] media: venus: provide video device lock 2023-05-25 11:02 ` Vikash Garodia @ 2023-05-26 3:35 ` Sergey Senozhatsky 0 siblings, 0 replies; 14+ messages in thread From: Sergey Senozhatsky @ 2023-05-26 3:35 UTC (permalink / raw) To: Vikash Garodia Cc: Sergey Senozhatsky, Hans Verkuil, Bryan O'Donoghue, Stanimir Varbanov, Andy Gross, Bjorn Andersson, Konrad Dybcio, Tomasz Figa, linux-media, linux-arm-msm 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv3] media: venus: provide video device lock 2023-05-24 16:36 ` Vikash Garodia 2023-05-25 0:53 ` Sergey Senozhatsky @ 2023-05-25 7:22 ` Hans Verkuil 2023-05-25 8:59 ` Sergey Senozhatsky 1 sibling, 1 reply; 14+ messages in thread From: Hans Verkuil @ 2023-05-25 7:22 UTC (permalink / raw) To: Vikash Garodia, Bryan O'Donoghue, Sergey Senozhatsky, Stanimir Varbanov, Andy Gross, Bjorn Andersson, Konrad Dybcio Cc: Tomasz Figa, linux-media, linux-arm-msm On 24/05/2023 18:36, Vikash Garodia wrote: > > 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. 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. Regards, Hans > >> >> 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> >> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv3] media: venus: provide video device lock 2023-05-25 7:22 ` Hans Verkuil @ 2023-05-25 8:59 ` Sergey Senozhatsky 2023-05-25 9:03 ` Hans Verkuil 0 siblings, 1 reply; 14+ messages in thread From: Sergey Senozhatsky @ 2023-05-25 8:59 UTC (permalink / raw) To: Hans Verkuil Cc: Vikash Garodia, Bryan O'Donoghue, Sergey Senozhatsky, Stanimir Varbanov, Andy Gross, Bjorn Andersson, Konrad Dybcio, Tomasz Figa, linux-media, linux-arm-msm 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv3] media: venus: provide video device lock 2023-05-25 8:59 ` Sergey Senozhatsky @ 2023-05-25 9:03 ` Hans Verkuil 0 siblings, 0 replies; 14+ messages in thread From: Hans Verkuil @ 2023-05-25 9:03 UTC (permalink / raw) To: Sergey Senozhatsky Cc: Vikash Garodia, Bryan O'Donoghue, Stanimir Varbanov, Andy Gross, Bjorn Andersson, Konrad Dybcio, Tomasz Figa, linux-media, linux-arm-msm On 25/05/2023 10:59, Sergey Senozhatsky wrote: > 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 Note that I would like to see 'Tested-by' and 'Reviewed/Acked-by' tags before merging, preferably from the driver maintainer(s). Regards, Hans ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv3] media: venus: provide video device lock 2023-05-24 14:44 ` Hans Verkuil 2023-05-24 16:36 ` Vikash Garodia @ 2023-06-01 10:16 ` Vikash Garodia 1 sibling, 0 replies; 14+ messages in thread From: Vikash Garodia @ 2023-06-01 10:16 UTC (permalink / raw) To: Hans Verkuil, Bryan O'Donoghue, Sergey Senozhatsky, Stanimir Varbanov, Andy Gross, Bjorn Andersson, Konrad Dybcio Cc: Tomasz Figa, linux-media, linux-arm-msm 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-06-01 10:21 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-05-24 1:36 [PATCH] media: venus: provide video device lock Sergey Senozhatsky 2023-05-24 12:40 ` Sergey Senozhatsky 2023-05-24 13:56 ` [PATCHv2] " Sergey Senozhatsky 2023-05-24 14:12 ` [PATCHv3] " Sergey Senozhatsky 2023-05-24 14:29 ` Bryan O'Donoghue 2023-05-24 14:44 ` Hans Verkuil 2023-05-24 16:36 ` Vikash Garodia 2023-05-25 0:53 ` Sergey Senozhatsky 2023-05-25 11:02 ` Vikash Garodia 2023-05-26 3:35 ` Sergey Senozhatsky 2023-05-25 7:22 ` Hans Verkuil 2023-05-25 8:59 ` Sergey Senozhatsky 2023-05-25 9:03 ` Hans Verkuil 2023-06-01 10:16 ` Vikash Garodia
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox