On 08/02/2022 04:33, Laurent Pinchart wrote:
Hi Tomi,
On Mon, Feb 07, 2022 at 06:11:00PM +0200, Tomi Valkeinen wrote:
Hi,
This is v3 of the subdev active state series. Changes since v2:
- Doc improvements
- Allow state->lock to be set by the driver (similarly to v4l2_ctrl_handler)
While I think we need better in the longer term, this seems like a
reasonable compromise to land this series and continue building on top.
- Rename fields in 'struct v4l2_subdev_pad_config' and drop the try_ prefix.
- Add v4l2_subdev_get_locked_active_state(), which calls lockdep_assert_locked() and returns the state.
- Changed v4l2_subdev_get_active_state() to call lockdep_assert_not_locked()
The idea with the v4l2_subdev_get_active_state /
v4l2_subdev_get_locked_active_state change is to have a lockdep_assert
called. Roughly I think there are two cases where the
v4l2_subdev_get_active_state could be called:
- With the intention of just passing it forward to another subdev, in
which case the state must _not_ be locked. Here
v4l2_subdev_get_active_state() can be called.
- With the intention of using the state in a case where the state is
known to be already locked. Here v4l2_subdev_get_locked_active_state()
can be called.
I'm not sure how this will work out, but it seems fine to me to start
with.
So, a bit longer explanation:
After I added the state->lock change, and used it in a driver so that I
share the ctrl and the state locks, I had a piece of code in a control
handler function which needs the state. I had this in the function:
state = v4l2_subdev_lock_active_state()
...
v4l2_subdev_unlock_state(state);
After sharing the lock with ctrls, I had to change it to:
state = v4l2_subdev_get_active_state()
And my immediate thought was, is it really locked? So I added a
lockdep_assert. Then I similarly added lockdep_assert in a few other
drivers, after which I thought that this needs a helper. Then I realized
that all the old places where v4l2_subdev_get_active_state() is used
require that the state is _not_locked, so I added
lockdep_assert_not_locked().
So, when you need the state, you must use one of:
- v4l2_subdev_lock_active_state() (lock the state and get it)
- v4l2_subdev_get_active_state() (get the currently unlocked state)
- v4l2_subdev_get_locked_active_state() (get the currently locked state)
which cover the possible cases and can implicitly do lockdep_assert. The
first and last functions sound a bit similar, though, but I'm not sure
how to name them better.
Tomi
