On 27/09/2021 03:48, Laurent Pinchart wrote:
Hi Tomi,
On Thu, Sep 16, 2021 at 04:24:19PM +0300, Tomi Valkeinen wrote:
On 16/09/2021 16:07, Jacopo Mondi wrote:
Also note that operations like s_stream do not take a state as
parameter. The driver has to fetch it from the subdev anyway
(this in reply to the idea of having the active state as parameter vs
retrieving it from the subdev if ACTIVE)
While porting the R-Car drivers on top of this series I found myself
in the need to (in the s_stream call chain)
static int rcsi2_start_receiver(struct rcar_csi2 *priv)
{
const struct v4l2_subdev_state *state = priv->subdev.state;
const struct v4l2_subdev_stream_configs *configs = &state->stream_configs;
...
/*
* Configure field handling inspecting the formats of the
* single sink pad streams.
*/
for (i = 0; i < configs->num_configs; ++i) {
const struct v4l2_subdev_stream_config *config = configs->configs;
if (config->pad != RCAR_CSI2_SINK)
continue;
if (config->fmt.field != V4L2_FIELD_ALTERNATE)
continue;
fld |= FLD_DET_SEL(1);
fld |= FLD_FLD_EN(config->stream);
/* PAL vs NTSC. */
if (config->fmt.height == 240)
fld |= FLD_FLD_NUM(0);
else
fld |= FLD_FLD_NUM(1);
}
...
}
Am I doing it wrong, or is this a case for the subdev to have to
directly access sd->state ?
In s_stream path you should:
state = v4l2_subdev_lock_active_state(sd);
<do the work with the state>
v4l2_subdev_unlock_state(state);
If you already have the state, e.g. in set_fmt:
state = v4l2_subdev_validate_and_lock_state(sd, state);
<do the work with the state>
v4l2_subdev_unlock_state(state);
Accessing the stream_configs directly is fine but not that nice. I did
think about some helpers, perhaps for_each_stream_config(), but I didn't
add that as I didn't have the need.
There's v4l2_state_get_stream_format() which can be used in many cases,
but we probably need something else if you need to iterate over all the
configs.
I really like forcing drivers to call functions that will lock the
state, at least until we can move the locks to the core (if ever). We
should move the fields of v4l2_subdev that drivers are not supposed to
access directly under a big PRIVATE comment.
Can you clarify what you mean here? Did you mean you like functions that
will _check_ the lock? Or did you just mean that you like
v4l2_subdev_lock_state() better than mutex_lock(state->lock)?
Well, in any case, I think my series does both =). I can add the private
comment to subdev. In fact, at one time I did not have sd->state, but
sd->_state, just to make sure no one accesses it.
Tomi
