Re: [PATCHv3] media: venus: provide video device lock

[Vikash Garodia <quic_vgarodia@quicinc.com>]

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>
>