| Message ID | 20250220092036.6757-1-demonsingur@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | media: add v4l2_subdev_state_xlate_streams() | expand |
Hi Cosmin 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 ? Is this the first step for a larger rework or is this a drive-by beautification ? 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(-) > > -- > 2.48.1 > >
On 2/20/25 1:38 PM, Jacopo Mondi wrote: > Hi Cosmin > > 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. > 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(-) >> >> -- >> 2.48.1 >> >>
Hi Cosmin, On Thu, Feb 20, 2025 at 03:01:41PM +0200, Cosmin Tanislav wrote: > > > On 2/20/25 1:38 PM, Jacopo Mondi wrote: > > Hi Cosmin > > > > 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. > > > 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(-) > > > > > > -- > > > 2.48.1 > > > > > > >
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(-)
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(-) >