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

[Cosmin Tanislav <demonsingur@gmail.com>]

On 2/20/25 4:39 PM, Laurent Pinchart wrote:
> 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.
> 

Got it. We can drop these patches then.

>>>> 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(-)
>