On 13/07/2020 18:05, Robert Foss wrote:
> On Mon, 13 Jul 2020 at 13:30, Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
>>
>> Use vb2_video_unregister_device() to automatically stop streaming
>> at unregister time.
>>
>> This avoids the use of vb2_queue_release() which should not be
>> called by drivers that set vdev->queue.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>> Cc: Robert Foss <robert.foss@xxxxxxxxxx>
>> Cc: Andrey Konovalov <andrey.konovalov@xxxxxxxxxx>
>> ---
>> drivers/media/platform/qcom/camss/camss-vfe.c | 8 --------
>> drivers/media/platform/qcom/camss/camss-vfe.h | 2 --
>> drivers/media/platform/qcom/camss/camss-video.c | 12 ++----------
>> drivers/media/platform/qcom/camss/camss-video.h | 2 --
>> drivers/media/platform/qcom/camss/camss.c | 5 -----
>> 5 files changed, 2 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
>> index fc31c2c169cd..b7d2293a5004 100644
>> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
>> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
>> @@ -2205,14 +2205,6 @@ static const struct camss_video_ops camss_vfe_video_ops = {
>> .flush_buffers = vfe_flush_buffers,
>> };
>>
>> -void msm_vfe_stop_streaming(struct vfe_device *vfe)
>> -{
>> - int i;
>> -
>> - for (i = 0; i < ARRAY_SIZE(vfe->line); i++)
>> - msm_video_stop_streaming(&vfe->line[i].video_out);
>> -}
>> -
>> /*
>> * msm_vfe_register_entities - Register subdev node for VFE module
>> * @vfe: VFE device
>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h
>> index 0d10071ae881..a90b0d2cc6de 100644
>> --- a/drivers/media/platform/qcom/camss/camss-vfe.h
>> +++ b/drivers/media/platform/qcom/camss/camss-vfe.h
>> @@ -178,8 +178,6 @@ void msm_vfe_unregister_entities(struct vfe_device *vfe);
>> void msm_vfe_get_vfe_id(struct media_entity *entity, u8 *id);
>> void msm_vfe_get_vfe_line_id(struct media_entity *entity, enum vfe_line_id *id);
>>
>> -void msm_vfe_stop_streaming(struct vfe_device *vfe);
>> -
>> extern const struct vfe_hw_ops vfe_ops_4_1;
>> extern const struct vfe_hw_ops vfe_ops_4_7;
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
>> index cdbd6dba1122..0e2fcee97eeb 100644
>> --- a/drivers/media/platform/qcom/camss/camss-video.c
>> +++ b/drivers/media/platform/qcom/camss/camss-video.c
>> @@ -879,7 +879,7 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>> if (ret < 0) {
>> dev_err(v4l2_dev->dev, "Failed to init video entity: %d\n",
>> ret);
>> - goto error_media_init;
>> + goto error_vb2_init;
>> }
>>
>> mutex_init(&video->lock);
>> @@ -936,23 +936,15 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>> error_video_register:
>> media_entity_cleanup(&vdev->entity);
>> mutex_destroy(&video->lock);
>> -error_media_init:
>> - vb2_queue_release(&video->vb2_q);
>> error_vb2_init:
>> mutex_destroy(&video->q_lock);
>>
>> return ret;
>> }
>>
>> -void msm_video_stop_streaming(struct camss_video *video)
>> -{
>> - if (vb2_is_streaming(&video->vb2_q))
>> - vb2_queue_release(&video->vb2_q);
>> -}
>> -
>> void msm_video_unregister(struct camss_video *video)
>> {
>> atomic_inc(&video->camss->ref_count);
>> - video_unregister_device(&video->vdev);
>> + vb2_video_unregister_device(&video->vdev);
>> atomic_dec(&video->camss->ref_count);
>> }
>> diff --git a/drivers/media/platform/qcom/camss/camss-video.h b/drivers/media/platform/qcom/camss/camss-video.h
>> index aa35e8cc6fd5..bdbae8424140 100644
>> --- a/drivers/media/platform/qcom/camss/camss-video.h
>> +++ b/drivers/media/platform/qcom/camss/camss-video.h
>> @@ -52,8 +52,6 @@ struct camss_video {
>> unsigned int nformats;
>> };
>>
>> -void msm_video_stop_streaming(struct camss_video *video);
>> -
>> int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>> const char *name, int is_pix);
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
>> index 3fdc9f964a3c..d0f4360eb9a0 100644
>> --- a/drivers/media/platform/qcom/camss/camss.c
>> +++ b/drivers/media/platform/qcom/camss/camss.c
>> @@ -964,13 +964,8 @@ void camss_delete(struct camss *camss)
>> */
>> static int camss_remove(struct platform_device *pdev)
>> {
>> - unsigned int i;
>> -
>> struct camss *camss = platform_get_drvdata(pdev);
>>
>> - for (i = 0; i < camss->vfe_num; i++)
>> - msm_vfe_stop_streaming(&camss->vfe[i]);
>> -
>
> I'm trying to understand this change, with msm_vfe_stop_streaming
> removed, are we relying on vb2_video_unregister_device being called
> through some other path?
>
>> v4l2_async_notifier_unregister(&camss->notifier);
>> v4l2_async_notifier_cleanup(&camss->notifier);
>> camss_unregister_entities(camss);
Yes: camss_unregister_entities -> camss_unregister_entities ->
msm_vfe_unregister_entities -> msm_video_unregister -> vb2_video_unregister_device
I have to say that this is a pretty convoluted call sequence, it took me a
bit of time to unravel it.
That said, I think I want a 'Tested-by' before I'll apply this specific patch.
Unfortunately, I don't have hardware to test this. Are you or Andrey able to test it?
Regards,
Hans
