Re: [PATCH v9 29/36] media: subdev: add v4l2_subdev_set_routing helper()

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

On 10/10/2021 13:21, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Oct 05, 2021 at 11:57:43AM +0300, Tomi Valkeinen wrote:
>> Add a helper function to set the subdev routing. The helper can be used
>> from subdev driver's set_routing op to store the routing table.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/media/v4l2-core/v4l2-subdev.c | 28 +++++++++++++++++++++++++++
>>   include/media/v4l2-subdev.h           | 16 +++++++++++++++
>>   2 files changed, 44 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 14b8282fe45b..af53f827ec27 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -1151,3 +1151,31 @@ void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state)
>>   	mutex_unlock(&state->lock);
>>   }
>>   EXPORT_SYMBOL_GPL(v4l2_subdev_unlock_state);
>> +
>> +int v4l2_subdev_set_routing(struct v4l2_subdev *sd,
>> +			    struct v4l2_subdev_state *state,
>> +			    struct v4l2_subdev_krouting *routing)
>> +{
>> +	struct v4l2_subdev_krouting *dst = &state->routing;
>> +	const struct v4l2_subdev_krouting *src = routing;
>> +
>> +	lockdep_assert_held(&state->lock);
> 
> Calling this function doesn't make much sense if the subdev doesn't have
> the V4L2_SUBDEV_FL_MULTIPLEXED set, right ? If that's correct, should
> this be documented below, and/or possibly checked here ?
> 
> 	if (WARN_ON(!(sd->flags & V4L2_SUBDEV_FL_MULTIPLEXED)))
> 		return -E???;

That is true, but it's true for all the functions introduced in the 
series. Do we want to add the check everywhere? In many cases this 
should be quite obvious: if you haven't ported the subdev driver to the 
new API, you don't have routing table, and can't sensibly call this 
function. Or you don't have stream ID, so you can't sensibly call 
functions that take a stream number.

>> +
>> +	kvfree(dst->routes);
>> +	dst->routes = NULL;
>> +	dst->num_routes = 0;
>> +
>> +	if (src->num_routes > 0) {
>> +		dst->routes = kvmalloc_array(src->num_routes,
>> +					     sizeof(*src->routes), GFP_KERNEL);
> 
> How many routes do we typically expect, is it worth a vmalloc ? If not,

I have to say I have no clue if it's worth it. vmalloc is used for the 
pads, and was used in the original routing patches. I did wonder about 
this but just went with what's already there.

It's not easy to guess the expected number of routes. I'd say... 4 pads 
on each side and 8 streams per pad-pair sounds kind of a lot but still 
sounds feasible. That would be 32 routes. But that would be on the 
maximum side. On average, I guess we'll have... 1 or 2 routes? That's 
wha the sensors will have.

> kmemdup() could be a candidate (as I think by the time we get here,
> num_routes should have been validated to not overflow, but please tell
> me if I got this part wrong).

I'm not sure what you mean here. We are allocating (enough) new memory, 
it will never overflow.

>> +		if (!dst->routes)
>> +			return -ENOMEM;
>> +
>> +		memcpy(dst->routes, src->routes,
>> +		       src->num_routes * sizeof(*src->routes));
>> +		dst->num_routes = src->num_routes;
>> +	}
>> +
>> +	return 0;

  Tomi