Re: [PATCH v2 0/3] media: add v4l2_subdev_state_xlate_streams()

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

On Thu, Feb 20, 2025 at 02:16:51PM +0000, Sakari Ailus wrote:
> On Thu, Feb 20, 2025 at 03:01:41PM +0200, Cosmin Tanislav wrote:
> > On 2/20/25 1:38 PM, Jacopo Mondi wrote:
> > > On Thu, Feb 20, 2025 at 11:20:32AM +0200, Cosmin Tanislav wrote:
> > > > Currently, the v4l2_subdev_state_xlate_streams() function is used
> > > > to translate streams from one pad to another.
> > > > This function takes the entire subdev state as argument, but only makes
> > > > use of the routing.
> > > 
> > > Correct, but is this a problem ?
> > > 
> > 
> > No, it's not a problem.
> 
> I think I have a slight preference to keep the pattern of referring to the
> state as other functions do.
> 
> I wonder what Laurent and Hans think, too.

I agree, I think the state should be passed everywhere. This lowers the
risk of subsystem-wide refactoring if a function that receives a pointer
to part of a state (such as v4l2_subdev_routing_xlate_streams()) is
later found to need more information from the state.

The situation would be different if the states were not monolithic, for
instance if the routing table could be locked separatly from other parts
of the state, but that's not the case and I don't foresee moving to
finer-grained objects.

> > > Is this the first step for a larger rework or is this a drive-by
> > > beautification ?
> > 
> > Mostly a drive-by beautification to avoid passing the whole state around
> > where we only need the routing. I'm planning to submit drivers for more
> > GMSL2/3 chips and we're using this just to not pass the whole state
> > around. I think I can just use v4l2_subdev_state_xlate_streams(),
> > but I had these patches in my tree and it would have been good to get
> > them upstream, in preparations for submitting the GMSL2/3 drivers.
> > 
> > > I'm asking because (and I know it's hard to strike a balance) this
> > > kind of changes tend to make back-porting a more painful, and if
> > > only justified by "it looks better" I would be a bit hesitant in
> > > taking them.
> > > 
> > > > Introduce a v4l2_subdev_routing_xlate_streams() function which can be
> > > > used without the entire subdev state, to avoid passing the entire state
> > > > around when not needed.
> > > > 
> > > > Convert all usages of v4l2_subdev_state_xlate_streams() to
> > > > v4l2_subdev_routing_xlate_streams().
> > > > 
> > > > Remove v4l2_subdev_state_xlate_streams().
> > > > 
> > > > V2:
> > > >    * Fix description of parameters
> > > > 
> > > > Cosmin Tanislav (3):
> > > >    media: v4l: subdev: add v4l2_subdev_routing_xlate_streams()
> > > >    media: use v4l2_subdev_routing_xlate_streams()
> > > >    media: v4l: subdev: remove v4l2_subdev_state_xlate_streams()
> > > > 
> > > >   drivers/media/i2c/ds90ub913.c                 | 14 ++++++-----
> > > >   drivers/media/i2c/ds90ub953.c                 | 14 ++++++-----
> > > >   drivers/media/i2c/max96714.c                  | 16 ++++++-------
> > > >   drivers/media/i2c/max96717.c                  | 23 ++++++++++---------
> > > >   drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 14 ++++++-----
> > > >   .../platform/nxp/imx8-isi/imx8-isi-crossbar.c |  2 +-
> > > >   drivers/media/v4l2-core/v4l2-subdev.c         |  7 +++---
> > > >   include/media/v4l2-subdev.h                   | 10 ++++----
> > > >   8 files changed, 53 insertions(+), 47 deletions(-)

-- 
Regards,

Laurent Pinchart