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.
--
Regards,
Laurent Pinchart
