Hi Hans,
On Tue, Nov 28, 2023 at 09:55:06AM +0100, Hans Verkuil wrote:
> On 27/11/2023 10:07, Laurent Pinchart wrote:
> > The subdev .init_cfg() operation is affected by two issues:
> >
> > - It has long been extended to initialize a whole v4l2_subdev_state
> > instead of just a v4l2_subdev_pad_config, but its name has stuck
> > around.
> >
> > - Despite operating on a whole subdev state and not being directly
> > exposed to the subdev users (either in-kernel or through the userspace
> > API), .init_cfg() is categorized as a subdev pad operation.
> >
> > This participates in making the subdev API confusing for new developers.
> > Fix it by renaming the operation to .init_state(), and make it a subdev
> > internal operation.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > Acked-by: Michael Riesch <michael.riesch@xxxxxxxxxxxxxx> # for imx415
> > Acked-by: Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx> # for vimc
> > ---
> > Changes since v3:
> >
> > - Rebase on top of the new stm32-dcmipp driver
> > - Fix uninitialized variable in __v4l2_subdev_state_alloc() due to bad
> > rebase
> >
> > Changes since v2:
> >
> > - Rebase on top of the latest media tree
> >
> > Changes since v1:
> >
> > - Fix compilation of the vsp1 driver
> > ---
>
> <snip>
>
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_entity.c b/drivers/media/platform/renesas/vsp1/vsp1_entity.c
> > index 0280b8ff7ba9..0a5a7f9cc870 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_entity.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_entity.c
> > @@ -170,33 +170,6 @@ vsp1_entity_get_pad_selection(struct vsp1_entity *entity,
> > }
> > }
> >
> > -/*
> > - * vsp1_entity_init_cfg - Initialize formats on all pads
> > - * @subdev: V4L2 subdevice
> > - * @sd_state: V4L2 subdev state
> > - *
> > - * Initialize all pad formats with default values in the given subdev state.
> > - * This function can be used as a handler for the subdev pad::init_cfg
> > - * operation.
> > - */
> > -int vsp1_entity_init_cfg(struct v4l2_subdev *subdev,
> > - struct v4l2_subdev_state *sd_state)
> > -{
> > - unsigned int pad;
> > -
> > - for (pad = 0; pad < subdev->entity.num_pads - 1; ++pad) {
> > - struct v4l2_subdev_format format = {
> > - .pad = pad,
> > - .which = sd_state ? V4L2_SUBDEV_FORMAT_TRY
> > - : V4L2_SUBDEV_FORMAT_ACTIVE,
> > - };
> > -
> > - v4l2_subdev_call(subdev, pad, set_fmt, sd_state, &format);
> > - }
> > -
> > - return 0;
> > -}
> > -
> > /*
> > * vsp1_subdev_get_pad_format - Subdev pad get_fmt handler
> > * @subdev: V4L2 subdevice
> > @@ -424,6 +397,29 @@ int vsp1_subdev_set_pad_format(struct v4l2_subdev *subdev,
> > return ret;
> > }
> >
> > +static int vsp1_entity_init_state(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_state *sd_state)
> > +{
> > + unsigned int pad;
> > +
> > + /* Initialize all pad formats with default values. */
> > + for (pad = 0; pad < subdev->entity.num_pads - 1; ++pad) {
> > + struct v4l2_subdev_format format = {
> > + .pad = pad,
> > + .which = sd_state ? V4L2_SUBDEV_FORMAT_TRY
> > + : V4L2_SUBDEV_FORMAT_ACTIVE,
> > + };
> > +
> > + v4l2_subdev_call(subdev, pad, set_fmt, sd_state, &format);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct v4l2_subdev_internal_ops vsp1_entity_internal_ops = {
> > + .init_state = vsp1_entity_init_state,
> > +};
> > +
> > /* -----------------------------------------------------------------------------
> > * Media Operations
> > */
> > @@ -658,6 +654,7 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity,
> > /* Initialize the V4L2 subdev. */
> > subdev = &entity->subdev;
> > v4l2_subdev_init(subdev, ops);
> > + subdev->internal_ops = &vsp1_entity_internal_ops;
> >
> > subdev->entity.function = function;
> > subdev->entity.ops = &vsp1->media_ops;
> > @@ -666,7 +663,7 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity,
> > snprintf(subdev->name, sizeof(subdev->name), "%s %s",
> > dev_name(vsp1->dev), name);
> >
> > - vsp1_entity_init_cfg(subdev, NULL);
> > + vsp1_entity_init_state(subdev, NULL);
>
> This is the only media driver that calls init_cfg/state directly.
> That's a bit unexpected, and perhaps this could use a comment. That
> can be a follow-up patch as well.
I have already posted a patch series to drop all this and use the V4L2
subdev active state API. That is hopefully better than a comment :-)
There are actually quite a few sensor drivers that don't use the active
state, and call the .init_cfg() handler at probe time to initialize
their private copy of the active state. All this should eventually go
away as drivers are converter. I try to keep an eye when reviewing new
code and push all new drivers to use the subdev active state.
> >
> > /*
> > * Allocate the subdev state to store formats and selection
>
> In any case, you can add my:
>
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>
> to this patch.
--
Regards,
Laurent Pinchart
