On 2/28/22 09:59, Hans Verkuil wrote:
>
>
> On 2/16/22 14:00, Tomi Valkeinen wrote:
>> All suitable subdev ops are now passed either the TRY or the ACTIVE
>> state by the v4l2 core. However, other subdev drivers can still call the
>> ops passing NULL as the state, implying the active case.
>>
>> For all current upstream drivers this doesn't matter, as they do not
>> expect to get a valid state for ACTIVE case. But future drivers which
>> support multiplexed streaming and routing will depend on getting a state
>> for both active and try cases.
>>
>> For new drivers we can mandate that the pipelines where the drivers are
>> used need to pass the state properly, or preferably, not call such
>> subdev ops at all.
>>
>> However, if an existing subdev driver is changed to support multiplexed
>> streams, the driver has to consider cases where its ops will be called
>> with NULL state. The problem can easily be solved by using the
>> v4l2_subdev_lock_and_return_state() helper, introduced here.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
>
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
Ignore this, I should have read Laurent's comments first. I agree with him.
Regards,
Hans
>
> Regards,
>
> Hans
>
>> ---
>> include/media/v4l2-subdev.h | 38 +++++++++++++++++++++++++++++++++++++
>> 1 file changed, 38 insertions(+)
>>
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index 8d089a2dbd32..84c02488d53f 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -1278,6 +1278,44 @@ void v4l2_subdev_lock_state(struct v4l2_subdev_state *state);
>> */
>> void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state);
>>
>> +/**
>> + * v4l2_subdev_lock_and_return_state() - Gets locked try or active subdev state
>> + * @sd: subdevice
>> + * @state: subdevice state as passed to the subdev op
>> + *
>> + * Due to legacy reasons, when subdev drivers call ops in other subdevs they use
>> + * NULL as the state parameter, as subdevs always used to have their active
>> + * state stored privately.
>> + *
>> + * However, newer state-aware subdev drivers, which store their active state in
>> + * a common place, subdev->active_state, expect to always get a proper state as
>> + * a parameter.
>> + *
>> + * These state-aware drivers can use v4l2_subdev_lock_and_return_state() instead
>> + * of v4l2_subdev_lock_state(). v4l2_subdev_lock_and_return_state() solves the
>> + * issue by using subdev->active_state in case the passed state is NULL.
>> + *
>> + * This is a temporary helper function, and should be removed when we can ensure
>> + * that all drivers pass proper state when calling other subdevs.
>> + */
>> +static inline struct v4l2_subdev_state *
>> +v4l2_subdev_lock_and_return_state(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *state)
>> +{
>> + if (state)
>> + return state;
>> +
>> + dev_notice_once(sd->dev,
>> + "The provided state is NULL. Adjusting to the subdev active state.\n"
>> + "Please consider porting your driver to the new state management API.\n");
>> +
>> + state = sd->active_state;
>> +
>> + v4l2_subdev_lock_state(state);
>> +
>> + return state;
>> +}
>> +
>> #endif /* CONFIG_MEDIA_CONTROLLER */
>>
>> /**
