Re: [PATCH v9 06/13] media: atmel: atmel-isc-base: use mutex to lock awb workqueue from streaming

[Hans Verkuil]

On 29/04/2022 10:08, Eugen.Hristev@xxxxxxxxxxxxx wrote:
> On 4/29/22 10:41 AM, Hans Verkuil wrote:
>> Hi Eugen,
>>
>> On 10/03/2022 10:51, Eugen Hristev wrote:
>>> The AWB workqueue runs in a kernel thread and needs to be synchronized
>>> w.r.t. the streaming status.
>>> It is possible that streaming is stopped while the AWB workq is running.
>>> In this case it is likely that the check for vb2_start_streaming_called is done
>>> at one point in time, but the AWB computations are done later, including a call
>>> to isc_update_profile, which requires streaming to be started.
>>> Thus , isc_update_profile will fail if during this operation sequence the
>>> streaming was stopped.
>>> To solve this issue, a mutex is added, that will serialize the awb work and
>>> streaming stopping, with the mention that either streaming is stopped
>>> completely including termination of the last frame is done, and after that
>>> the AWB work can check stream status and stop; either first AWB work is
>>> completed and after that the streaming can stop correctly.
>>> The awb spin lock cannot be used since this spinlock is taken in the same
>>> context and using it in the stop streaming will result in a recursion BUG.
>>
>> Please keep the line length in a commit log to no more than 75.
>>
>>>
>>> Signed-off-by: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>
>>> Reviewed-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
>>> ---
>>>   drivers/media/platform/atmel/atmel-isc-base.c | 29 ++++++++++++++++---
>>>   drivers/media/platform/atmel/atmel-isc.h      |  2 ++
>>>   2 files changed, 27 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
>>
>> <snip>
>>
>>> @@ -1548,6 +1564,7 @@ static int isc_s_awb_ctrl(struct v4l2_ctrl *ctrl)
>>>                         */
>>>                        v4l2_ctrl_activate(isc->do_wb_ctrl, false);
>>>                }
>>> +             mutex_unlock(&isc->awb_mutex);
>>
>> Huh? What is this unlock doing here? Am I missing something?
> 
> Hello Hans,
> 
> Sorry. It looks like the corresponding mutex_lock was lost when I 
> rebased the series over the patch to use 'is_streaming' instead of 
> 'stop' variable.
> In version 3, the mutex_lock was there :
> 
> https://www.spinics.net/lists/linux-media/msg204059.html
> 
> Somehow a race problem did not occur in this specific critical area in 
> my tests.
> 
> I will resend this patch as a v10 , or the whole series if you have 
> other comments on the other patches? How would you prefer ?

I have more comments (just finished the full review), so a v10 is definitely
needed.

Regards,

	Hans

> 
> Thanks,
> Eugen
> 
>>
>> Regards,
>>
>>          Hans
>>
>>>
>>>                /* if we have autowhitebalance on, start histogram procedure */
>>>                if (ctrls->awb == ISC_WB_AUTO &&
>>> @@ -1740,6 +1757,7 @@ static void isc_async_unbind(struct v4l2_async_notifier *notifier,
>>>   {
>>>        struct isc_device *isc = container_of(notifier->v4l2_dev,
>>>                                              struct isc_device, v4l2_dev);
>>> +     mutex_destroy(&isc->awb_mutex);
>>>        cancel_work_sync(&isc->awb_work);
>>>        video_unregister_device(&isc->video_dev);
>>>        v4l2_ctrl_handler_free(&isc->ctrls.handler);
>>> @@ -1850,6 +1868,8 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
>>>        isc->current_subdev = container_of(notifier,
>>>                                           struct isc_subdev_entity, notifier);
>>>        mutex_init(&isc->lock);
>>> +     mutex_init(&isc->awb_mutex);
>>> +
>>>        init_completion(&isc->comp);
>>>
>>>        /* Initialize videobuf2 queue */
>>> @@ -1930,6 +1950,7 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier)
>>>        video_unregister_device(vdev);
>>>
>>>   isc_async_complete_err:
>>> +     mutex_destroy(&isc->awb_mutex);
>>>        mutex_destroy(&isc->lock);
>>>        return ret;
>>>   }
>>> diff --git a/drivers/media/platform/atmel/atmel-isc.h b/drivers/media/platform/atmel/atmel-isc.h
>>> index 9cc69c3ae26d..f98f25a55e73 100644
>>> --- a/drivers/media/platform/atmel/atmel-isc.h
>>> +++ b/drivers/media/platform/atmel/atmel-isc.h
>>> @@ -229,6 +229,7 @@ enum isc_scaler_pads {
>>>    *
>>>    * @lock:            lock for serializing userspace file operations
>>>    *                   with ISC operations
>>> + * @awb_mutex:               serialize access to streaming status from awb work queue
>>>    * @awb_lock:                lock for serializing awb work queue operations
>>>    *                   with DMA/buffer operations
>>>    *
>>> @@ -307,6 +308,7 @@ struct isc_device {
>>>        struct work_struct      awb_work;
>>>
>>>        struct mutex            lock;
>>> +     struct mutex            awb_mutex;
>>>        spinlock_t              awb_lock;
>>>
>>>        struct regmap_field     *pipeline[ISC_PIPE_LINE_NODE_NUM];
>>
>