Hi Tomi,
On Mon, Oct 11, 2021 at 11:13:40AM +0300, Tomi Valkeinen wrote:
> On 10/10/2021 13:21, Laurent Pinchart wrote:
> > 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@xxxxxxxxxxxxxxxx>
> >> ---
> >> 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.
That's right. I was mostly concern about driver conversion, it's likely
easy to forget to add V4L2_SUBDEV_FL_MULTIPLEXED and then spend time
debugging issues.
I'm fine either way for the WARN_ON, but would still like to capture
this in the documentation.
> >> +
> >> + 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.
I mean an integer overflow in the size * nelems calculation. The array
allocation functions pay special attention to this.
> >> + if (!dst->routes)
> >> + return -ENOMEM;
> >> +
> >> + memcpy(dst->routes, src->routes,
> >> + src->num_routes * sizeof(*src->routes));
> >> + dst->num_routes = src->num_routes;
> >> + }
> >> +
> >> + return 0;
--
Regards,
Laurent Pinchart
