Hi, On 15/10/2021 15:13, Jacopo Mondi wrote:
+/** + * struct v4l2_subdev_krouting - subdev routing table + * + * @num_routes: number of routes + * @routes: &struct v4l2_subdev_route + * + * This structure contains the routing table for a subdev. + */ +struct v4l2_subdev_krouting { + unsigned int num_routes; + struct v4l2_subdev_route *routes; +};Re-reading v8 https://patchwork.linuxtv.org/project/linux-media/patch/20210830110116.488338-3-tomi.valkeinen@xxxxxxxxxxxxxxxx/#131630 my understanding was that your intention was to pass to the set_routing() op the num_routes, *routes and which and drop v4l2_subdev_krouting
No, that was not what I meant. But I did write it a bit unclearly, I admit...
I think I meant "drop v4l2_subdev_krouting, and pass v4l2_subdev_routing_table to the ops".
+ /** * struct v4l2_subdev_state - Used for storing subdev state information. * * @lock: mutex for the state * @pads: &struct v4l2_subdev_pad_config array + * @routing: routing table for the subdev * * This structure only needs to be passed to the pad op if the 'which' field * of the main argument is set to %V4L2_SUBDEV_FORMAT_TRY. For @@ -707,6 +721,7 @@ struct v4l2_subdev_pad_config { struct v4l2_subdev_state { struct mutex lock; struct v4l2_subdev_pad_config *pads; + struct v4l2_subdev_krouting routing; }; /** @@ -770,6 +785,9 @@ struct v4l2_subdev_state { * applied to the hardware. The operation shall fail if the * pad index it has been called on is not valid or in case of * unrecoverable failures. + * + * @set_routing: enable or disable data connection routes described in the + * subdevice routing table. */ struct v4l2_subdev_pad_ops { int (*init_cfg)(struct v4l2_subdev *sd, @@ -814,6 +832,10 @@ struct v4l2_subdev_pad_ops { struct v4l2_mbus_config *config); int (*set_mbus_config)(struct v4l2_subdev *sd, unsigned int pad, struct v4l2_mbus_config *config); + int (*set_routing)(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state, + enum v4l2_subdev_format_whence which, + struct v4l2_subdev_krouting *route);But it seems you've instead coalesced v4l2_subdev_routing_table into v4l2_subdev_krouting (which I like!) But why keep 'which' as a standalone parameter instead of adding it to v4l2_subdev_krouting ? This operation signature is a bit of an outliner as other ops have 'which' embedded in op-specific structures.
The whole point here was to remove 'which'. Otherwise the routing table contains 'which', and the drivers somehow need to come up with a correct 'which' value, which was causing trouble.
E.g. in init_cfg, you want to set up routing. But init_cfg is supposed to be 'which-less', the init should be the same for both active and try states.
The only use of 'which' in the routing table is when calling set_routing(). It's never needed after that. So it's pointless to store the 'which' value in the state.
The other ops have a struct as a parameter that comes directly from the userspace, and thus contains the 'which'. But any data they store to the state does not contain 'which'.
So, we could do the same, have the structs as they are in the mail you linked. But then we have the struct v4l2_subdev_krouting, which is only used when calling set_routing, and the only purpose of that struct would be to avoid passing 'which' as a parameter to set_routing...
Tomi
