Re: [PATCH v4 1/1] media: v4l2-subdev: Rename .init_cfg() operation to .init_state()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@ideasonboard.com>
> > Acked-by: Michael Riesch <michael.riesch@wolfvision.net> # for imx415
> > Acked-by: Shuah Khan <skhan@linuxfoundation.org> # 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@xs4all.nl>
> 
> to this patch.

-- 
Regards,

Laurent Pinchart