From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-17.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5047EC47087 for ; Fri, 28 May 2021 11:35:02 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 2EB48613DA for ; Fri, 28 May 2021 11:35:02 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229641AbhE1Lgf (ORCPT ); Fri, 28 May 2021 07:36:35 -0400 Received: from perceval.ideasonboard.com ([213.167.242.64]:55028 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229608AbhE1Lgd (ORCPT ); Fri, 28 May 2021 07:36:33 -0400 Received: from [192.168.1.111] (91-157-208-71.elisa-laajakaista.fi [91.157.208.71]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 7980D580; Fri, 28 May 2021 13:34:56 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1622201697; bh=aT/AmQrykMHSfwPfgRZaq+68OkP8U5sR4avORes0PE0=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=qJfkhMcuArhlZWI2JFyI4xBrQy6mZHE8WdAaQ9B9cjO9OWdy9LhSg2gl4pvQzZT53 P81s0MnzOUexF5oGLkEmuvFJuyVGQaHtRnFQPNSuVByFHcpxQZPg2BZPjiOqLwfKDb tKCtkpl4CgXAOHndoa/gtzVnG4GZQ9ET4AWteCYU= Subject: Re: [PATCH v7 24/27] v4l: subdev: use streams in v4l2_subdev_link_validate() To: linux-media@vger.kernel.org, sakari.ailus@linux.intel.com, Jacopo Mondi , Laurent Pinchart , niklas.soderlund+renesas@ragnatech.se Cc: Mauro Carvalho Chehab , Hans Verkuil , Pratyush Yadav , Lokesh Vutla References: <20210524104408.599645-1-tomi.valkeinen@ideasonboard.com> <20210524104408.599645-25-tomi.valkeinen@ideasonboard.com> From: Tomi Valkeinen Message-ID: Date: Fri, 28 May 2021 14:34:55 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: <20210524104408.599645-25-tomi.valkeinen@ideasonboard.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org On 24/05/2021 13:44, Tomi Valkeinen wrote: > Update v4l2_subdev_link_validate() to use routing and streams for > validation. > > Signed-off-by: Tomi Valkeinen > --- > drivers/media/v4l2-core/v4l2-subdev.c | 184 +++++++++++++++++++++++--- > 1 file changed, 166 insertions(+), 18 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index da6ea9b14631..b30b456d8d99 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -16,6 +16,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -894,6 +895,7 @@ EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate_default); > > static int > v4l2_subdev_link_validate_get_format(struct media_pad *pad, > + u32 stream, > struct v4l2_subdev_format *fmt) > { > if (is_media_entity_v4l2_subdev(pad->entity)) { > @@ -902,6 +904,7 @@ v4l2_subdev_link_validate_get_format(struct media_pad *pad, > > fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE; > fmt->pad = pad->index; > + fmt->stream = stream; > return v4l2_subdev_call(sd, pad, get_fmt, NULL, fmt); > } > > @@ -1012,31 +1015,176 @@ bool v4l2_subdev_has_route(struct v4l2_subdev_krouting *routing, > } > EXPORT_SYMBOL_GPL(v4l2_subdev_has_route); > > +static int cmp_u32(const void *a, const void *b) > +{ > + u32 a32 = *(u32 *)a; > + u32 b32 = *(u32 *)b; > + > + return a32 > b32 ? 1 : (a32 < b32 ? -1 : 0); > +} > + > int v4l2_subdev_link_validate(struct media_link *link) > { > - struct v4l2_subdev *sink; > - struct v4l2_subdev_format sink_fmt, source_fmt; > - int rval; > + int ret; > + unsigned int i; > > - rval = v4l2_subdev_link_validate_get_format( > - link->source, &source_fmt); > - if (rval < 0) > - return 0; > + struct v4l2_subdev *source_subdev = > + media_entity_to_v4l2_subdev(link->source->entity); > + struct v4l2_subdev *sink_subdev = > + media_entity_to_v4l2_subdev(link->sink->entity); > + struct device *dev = sink_subdev->entity.graph_obj.mdev->dev; > > - rval = v4l2_subdev_link_validate_get_format( > - link->sink, &sink_fmt); > - if (rval < 0) > - return 0; > + struct v4l2_subdev_krouting routing; > > - sink = media_entity_to_v4l2_subdev(link->sink->entity); > + static const u32 default_streams[] = { 0 }; > > - rval = v4l2_subdev_call(sink, pad, link_validate, link, > - &source_fmt, &sink_fmt); > - if (rval != -ENOIOCTLCMD) > - return rval; > + u32 num_source_streams = 0; > + const u32 *source_streams; > + u32 num_sink_streams = 0; > + const u32 *sink_streams; > + > + dev_dbg(dev, "validating link \"%s\":%u -> \"%s\":%u\n", > + link->source->entity->name, link->source->index, > + link->sink->entity->name, link->sink->index); > + > + /* Get source streams */ > + > + memset(&routing, 0, sizeof(routing)); > + > + ret = v4l2_subdev_get_routing(source_subdev, NULL, &routing); > + > + if (ret && ret != -ENOIOCTLCMD) > + return ret; > + > + if (ret == -ENOIOCTLCMD) { > + num_source_streams = 1; > + source_streams = default_streams; > + } else { > + u32 *streams; > + > + streams = kmalloc_array(routing.num_routes, sizeof(u32), > + GFP_KERNEL); > + > + for (i = 0; i < routing.num_routes; ++i) { > + struct v4l2_subdev_route *route = &routing.routes[i]; > + > + if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE)) > + continue; > + > + if (route->source_pad == link->source->index) > + streams[num_source_streams++] = > + route->source_stream; > + } > + > + sort(streams, num_source_streams, sizeof(u32), &cmp_u32, NULL); > + > + source_streams = streams; > + > + v4l2_subdev_free_routing(&routing); > + } > + > + /* Get sink streams */ > + > + memset(&routing, 0, sizeof(routing)); > + > + ret = v4l2_subdev_get_routing(sink_subdev, NULL, &routing); > + > + if (ret && ret != -ENOIOCTLCMD) > + return ret; > + > + if (ret == -ENOIOCTLCMD) { > + num_sink_streams = 1; > + sink_streams = default_streams; > + } else { > + u32 *streams; > > - return v4l2_subdev_link_validate_default( > - sink, link, &source_fmt, &sink_fmt); > + streams = kmalloc_array(routing.num_routes, sizeof(u32), > + GFP_KERNEL); > + > + for (i = 0; i < routing.num_routes; ++i) { > + struct v4l2_subdev_route *route = &routing.routes[i]; > + > + if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE)) > + continue; > + > + if (route->sink_pad == link->sink->index) > + streams[num_sink_streams++] = > + route->sink_stream; > + } > + > + sort(streams, num_sink_streams, sizeof(u32), &cmp_u32, NULL); > + > + sink_streams = streams; > + > + v4l2_subdev_free_routing(&routing); > + } > + > + if (num_source_streams != num_sink_streams) { > + dev_err(dev, > + "Sink and source stream count mismatch: %d vs %d\n", > + num_source_streams, num_sink_streams); > + return -EINVAL; > + } > + > + /* Validate source and sink stream formats */ > + > + for (i = 0; i < num_source_streams; ++i) { > + struct v4l2_subdev_format sink_fmt, source_fmt; > + u32 stream; > + int ret; > + > + if (source_streams[i] != sink_streams[i]) { > + dev_err(dev, "Sink and source streams do not match\n"); > + return -EINVAL; > + } > + > + stream = source_streams[i]; > + > + dev_dbg(dev, "validating stream \"%s\":%u:%u -> \"%s\":%u:%u\n", > + link->source->entity->name, link->source->index, stream, > + link->sink->entity->name, link->sink->index, stream); > + > + ret = v4l2_subdev_link_validate_get_format(link->source, stream, > + &source_fmt); > + if (ret < 0) { > + dev_err(dev, "Failed to get format for \"%s\":%u:%u\n", > + link->source->entity->name, link->source->index, > + stream); > + return ret; > + } > + > + ret = v4l2_subdev_link_validate_get_format(link->sink, stream, > + &sink_fmt); > + if (ret < 0) { > + dev_err(dev, "Failed to get format for \"%s\":%u:%u\n", > + link->sink->entity->name, link->sink->index, > + stream); > + return ret; > + } > + > + /* TODO: add stream number to link_validate() */ > + ret = v4l2_subdev_call(sink_subdev, pad, link_validate, link, > + &source_fmt, &sink_fmt); > + if (!ret) > + continue; > + > + if (ret != -ENOIOCTLCMD) > + return ret; > + > + ret = v4l2_subdev_link_validate_default(sink_subdev, link, > + &source_fmt, &sink_fmt); > + > + if (ret) > + return ret; > + } > + > + if (source_streams != default_streams) > + kfree(source_streams); > + > + if (sink_streams != default_streams) > + kfree(sink_streams); > + > + return 0; > } > EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate); > I noticed two issues with this patch: - It leaks memory on error cases. - The previous behavior silently ignored failures to get format from subdevs, and returned 0. This one fails. I'll fix those. Tomi