Re: [PATCH v9 27/36] media: subdev: Add [GS]_ROUTING subdev ioctls and operations

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

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@ideasonboard.com/#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