From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0C556134BD for ; Mon, 4 Aug 2025 11:47:00 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=213.167.242.64 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1754308024; cv=none; b=uhlIqvySfs1TW4UAo45DLWxGKAfU1IsD6Nf50NAsmSGSiJJ5E159fd2O0wkKW3KEHqEbzKZ57hhvyXkEEyifL+VBEzB4/NxF+UvhR7mXlou4fUAUG18KT+JOlc620ItUwwtgCp0uq9n3xfWrKspj1KIC1vdEm2J3yZXbRqfrQ+I= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1754308024; c=relaxed/simple; bh=ptlDNL6IJLRpCqJTznKm73NMu1OQbM+Jr/f0pa/497M=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=udOHGlOvE+fy1uG65taFEkCstGOgjwrI2P3dRxHKzUmCgUqxpWZhGaruXuyRLu4IgtoT+fc1t1YEZcm3rPKRKXjQyJwVflpK/c2Bekxu0XN07Lo2C0h0u7wApiRk96YKlg9DVPZhywJRTbNK4GjumIMfUKF9ploX1bdDJeDMTNQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=ideasonboard.com; spf=pass smtp.mailfrom=ideasonboard.com; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b=bQcxDadG; arc=none smtp.client-ip=213.167.242.64 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="bQcxDadG" Received: from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi [81.175.209.231]) by perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 084809CE; Mon, 4 Aug 2025 13:46:09 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1754307970; bh=ptlDNL6IJLRpCqJTznKm73NMu1OQbM+Jr/f0pa/497M=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=bQcxDadGcTUnAMf0mZxzTT0jQaAk4BqY2q7OhnZyPBZ/kGL1XeuNklUz35t93D7n6 R47JAwCyy2cgAkNybxHc70dw2axXc1hFfdLyQONk7mw3USJTc8iRKwwfnDbWRXnPvO 9URIHo3VekvzU/jsbacLCpi7oPIecLp4DF/KUHGM= Date: Mon, 4 Aug 2025 14:46:43 +0300 From: Laurent Pinchart To: Sakari Ailus Cc: linux-media@vger.kernel.org, bingbu.cao@linux.intel.com, stanislaw.gruszka@linux.intel.com, tian.shu.qiu@intel.com, tomi.valkeinen@ideasonboard.com Subject: Re: [PATCH 12/13] media: v4l2-mc: Introduce v4l2_mc_pipeline_enabled() Message-ID: <20250804114643.GB9262@pendragon.ideasonboard.com> References: <20250619081546.1582969-1-sakari.ailus@linux.intel.com> <20250619081546.1582969-13-sakari.ailus@linux.intel.com> <20250626230710.GA31209@pendragon.ideasonboard.com> Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: On Mon, Aug 04, 2025 at 11:32:17AM +0000, Sakari Ailus wrote: > On Fri, Jun 27, 2025 at 02:07:10AM +0300, Laurent Pinchart wrote: > > On Thu, Jun 19, 2025 at 11:15:45AM +0300, Sakari Ailus wrote: > > > v4l2_mc_pipeline_enabled() helps solving a problem known for long but > > > lacked any sort of general solution: with multiple streams, when streaming > > > is started on video nodes one by one, when should streaming be started in > > > the source? > > > > > > v4l2_mc_pipeline_enabled() traverses the pipeline towards the source, > > > queries the streams generated by the source and traces them back to the > > > video nodes. > > > > > > Signed-off-by: Sakari Ailus > > > --- > > > drivers/media/v4l2-core/v4l2-mc.c | 243 ++++++++++++++++++++++++++++++ > > > include/media/v4l2-mc.h | 44 ++++++ > > > 2 files changed, 287 insertions(+) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c > > > index 937d358697e1..1731088ad436 100644 > > > --- a/drivers/media/v4l2-core/v4l2-mc.c > > > +++ b/drivers/media/v4l2-core/v4l2-mc.c > > > @@ -612,3 +612,246 @@ int v4l2_pipeline_link_notify(struct media_link *link, u32 flags, > > > return ret; > > > } > > > EXPORT_SYMBOL_GPL(v4l2_pipeline_link_notify); > > > + > > > +static int > > > +__v4l2_mc_pipeline_enabled(struct v4l2_subdev_state *state, > > > + struct media_pad *src_pad, u64 __src_streams, > > > + struct media_pad **__sink_pad, u64 *__sink_streams) > > > +{ > > > + struct v4l2_subdev_route *route; > > > + u64 src_streams = 0, sink_streams = 0; > > > + bool has_sink_pad = false; > > > + unsigned int sink_pad; > > > + > > > + dev_dbg(state->sd->dev, "%s: source enabled, pad/streams %u/%#llx\n", > > > + state->sd->entity.name, src_pad->index, __src_streams); > > > + for_each_active_route(&state->routing, route) { > > > + dev_dbg(state->sd->dev, "%s: %u/%u -> %u/%u, flags %x\n", > > > + state->sd->entity.name, > > > + route->sink_pad, route->sink_stream, route->source_pad, > > > + route->source_stream, route->flags); > > > + if (route->source_pad != src_pad->index) > > > + continue; > > > + > > > + if (!(BIT_ULL(route->source_stream) & __src_streams)) > > > + continue; > > > + > > > + if (!has_sink_pad) { > > > + has_sink_pad = true; > > > + sink_pad = route->sink_pad; > > > + } > > > + > > > + if (route->sink_pad != sink_pad) { > > > + dev_dbg(state->sd->dev, > > > + "sink pads (%u vs. %u) differ\n", > > > + route->sink_pad, sink_pad); > > > + return -EMLINK; > > > + } > > > + > > > + sink_streams |= BIT_ULL(route->sink_stream); > > > + src_streams |= BIT_ULL(route->source_stream); > > > + } > > > + > > > + *__sink_pad = has_sink_pad ? &state->sd->entity.pads[sink_pad] : NULL; > > > + *__sink_streams = sink_streams; > > > + > > > + return 0; > > > +} > > > + > > > +static int v4l2_mc_downpath_enabled(struct media_pad *sink_pad, > > > + unsigned int sink_stream, > > > + bool (*func)(struct video_device *vdev), > > > + struct media_pad **__sink_pad, > > > + u64 *__sink_streams) > > > +{ > > > + struct v4l2_subdev_state *state; > > > + struct v4l2_subdev_route *route; > > > + struct v4l2_subdev *sd; > > > + struct media_pad *source_pad, *tmp_pad; > > > + u32 source_stream; > > > + > > > + if (!is_media_entity_v4l2_subdev(sink_pad->entity)) > > > + return -ENXIO; > > > + > > > + sd = media_entity_to_v4l2_subdev(sink_pad->entity); > > > + dev_dbg(sd->dev, "path_enabled: found sub-device %s\n", > > > + sd->entity.name); > > > + > > > + state = v4l2_subdev_lock_and_get_active_state(sd); > > > + route = v4l2_subdev_find_route(&state->routing, sink_pad->index, > > > + sink_stream, true, 0); > > > + if (IS_ERR(route)) { > > > + v4l2_subdev_unlock_state(state); > > > + dev_dbg(sd->dev, > > > + "path_enabled: can't find opposite route for %s:%u/%u", > > > + sd->entity.name, sink_pad->index, sink_stream); > > > + return 2; > > > + } > > > + > > > + source_pad = &sd->entity.pads[route->source_pad]; > > > + v4l2_subdev_unlock_state(state); > > > + > > > + tmp_pad = sink_pad; > > > + sink_pad = media_pad_remote_pad_unique(source_pad); > > > + if (IS_ERR(sink_pad)) { > > > + dev_dbg(sd->dev, > > > + "path_enabled: can't find remote source for %s:%u\n", > > > + source_pad->entity->name, source_pad->index); > > > + return PTR_ERR(sink_pad); > > > + } > > > + > > > + if (is_media_entity_v4l2_video_device(sink_pad->entity)) { > > > + struct video_device *vdev; > > > + > > > + vdev = media_entity_to_video_device(sink_pad->entity); > > > + if (!vdev) > > > + return -ENXIO; > > > + > > > + dev_dbg(vdev->dev_parent, > > > + "path_enabled: found video device %s\n", > > > + vdev->name); > > > + > > > + if (!*__sink_pad) { > > > + *__sink_pad = tmp_pad; > > > + dev_dbg(sd->dev, "path_enabled: sink %u/%u\n", > > > + tmp_pad->index, sink_stream); > > > + } else if (tmp_pad != *__sink_pad) { > > > + dev_dbg(sd->dev, > > > + "path_enabled: pads %s/%u and %s/%u differ\n", > > > + tmp_pad->entity->name, tmp_pad->index, > > > + (*__sink_pad)->entity->name, > > > + (*__sink_pad)->index); > > > + return -EXDEV; > > > + } > > > + > > > + *__sink_streams |= BIT_ULL(sink_stream); > > > + > > > + return func(vdev); > > > + } > > > + > > > + return v4l2_mc_downpath_enabled(sink_pad, source_stream, func, > > > + __sink_pad, __sink_streams); > > > +} > > > + > > > +static int v4l2_mc_source_get_streams(struct v4l2_subdev *sd, unsigned int pad, > > > + u64 *__streams) > > > +{ > > > + struct v4l2_mbus_frame_desc desc; > > > + u64 streams = 0; > > > + int ret; > > > + > > > + if (!__streams) > > > + return -EINVAL; > > > + > > > + ret = v4l2_subdev_call(sd, pad, get_frame_desc, pad, &desc); > > > + if (ret == -ENOIOCTLCMD) { > > > + *__streams = 1ULL; > > > + return 0; > > > + } > > > + if (ret) > > > + return ret; > > > + > > > + for (unsigned int i = 0; i < desc.num_entries; i++) { > > > + if (streams & BIT_ULL(desc.entry[i].stream)) > > > + return -EINVAL; > > > + > > > + streams |= BIT_ULL(desc.entry[i].stream); > > > + } > > > + > > > + dev_dbg(sd->dev, "found streams %#llx based on streams %#llx\n", > > > + *__streams, streams); > > > + if (*__streams & ~streams) > > > + return -EINVAL; > > > + > > > + *__streams = streams; > > > + > > > + return 0; > > > +} > > > + > > > +int v4l2_mc_pipeline_enabled(struct video_device *vdev, > > > + bool (*func)(struct video_device *vdev), > > > + struct media_pad **__sink_pad, u64 *__sink_streams) > > > +{ > > > + u64 sink_streams = 1U; > > > + struct media_pad *src_pad; > > > + u64 src_streams; > > > + struct v4l2_subdev_state *state; > > > + struct media_pad *sink_pad = vdev->entity.pads; > > > + struct v4l2_subdev *sd = NULL; > > > + bool streaming = true; > > > + struct media_pad *tmp_pad; > > > + u64 tmp_streams; > > > + int ret; > > > + > > > + if (!__sink_pad) > > > + __sink_pad = &tmp_pad; > > > + if (!__sink_streams) > > > + __sink_streams = &tmp_streams; > > > + *__sink_pad = NULL; > > > + *__sink_streams = 0; > > > + > > > + do { > > > + src_pad = media_pad_remote_pad_unique(sink_pad); > > > + if (IS_ERR(src_pad)) { > > > + dev_dbg(sd ? sd->dev : vdev->dev_parent, > > > + "no unique remote pad found from %s:%u\n", > > > + sink_pad->entity->name, sink_pad->index); > > > + return PTR_ERR(src_pad); > > > + } > > > + > > > + sd = media_entity_to_v4l2_subdev(src_pad->entity); > > > + if (!sd) { > > > + dev_dbg(sd->dev, > > > + "media entity %s is not a V4L2 sub-device\n", > > > + src_pad->entity->name); > > > + return -ENXIO; > > > + } > > > + > > > + /* Source streams match sink. */ > > > + src_streams = sink_streams; > > > + > > > + state = v4l2_subdev_lock_and_get_active_state(sd); > > > + ret = __v4l2_mc_pipeline_enabled(state, src_pad, > > > + src_streams, &sink_pad, > > > + &sink_streams); > > > + v4l2_subdev_unlock_state(state); > > > + if (ret) > > > + return ret; > > > + } while (sink_pad); > > > + > > > + ret = v4l2_mc_source_get_streams(sd, src_pad->index, &src_streams); > > > + if (ret) > > > + return ret; > > > + > > > + sd = media_entity_to_v4l2_subdev(src_pad->entity); > > > + > > > + dev_dbg(sd->dev, "following %s:%u/%#llx\n", sd->entity.name, > > > + src_pad->index, src_streams); > > > + > > > + for (unsigned int i = __ffs(src_streams); src_streams; > > > + src_streams &= ~BIT_ULL(i), i = __ffs(src_streams)) { > > > + sink_pad = media_pad_remote_pad_unique(src_pad); > > > + if (IS_ERR(src_pad)) { > > > + dev_dbg(sd->dev, > > > + "no unique remote pad found from %s:%u\n", > > > + sink_pad->entity->name, sink_pad->index); > > > + return PTR_ERR(src_pad); > > > + } > > > + > > > + ret = v4l2_mc_downpath_enabled(sink_pad, i, func, __sink_pad, > > > + __sink_streams); > > > + if (ret == 2) > > > + continue; > > > + if (ret < 0) > > > + return ret; > > > + if (!ret) > > > + streaming = false; > > > + } > > > + > > > + dev_dbg(media_entity_to_v4l2_subdev((*__sink_pad)->entity)->dev, > > > + "sink pad %s:%u/%#llx\n", (*__sink_pad)->entity->name, > > > + (*__sink_pad)->index, *__sink_streams); > > > + > > > + return streaming; > > > +} > > > +EXPORT_SYMBOL_GPL(v4l2_mc_pipeline_enabled); > > > diff --git a/include/media/v4l2-mc.h b/include/media/v4l2-mc.h > > > index 1837c9fd78cf..e72c0f62fa34 100644 > > > --- a/include/media/v4l2-mc.h > > > +++ b/include/media/v4l2-mc.h > > > @@ -193,6 +193,50 @@ void v4l2_pipeline_pm_put(struct media_entity *entity); > > > int v4l2_pipeline_link_notify(struct media_link *link, u32 flags, > > > unsigned int notification); > > > > > > +/** > > > + * v4l2_mc_pipeline_enabled - Tell when to start streaming > > > + * @vdev: The video device > > > + * @func: Caller-provided function to tell a video device's streaming state > > > + * @__sink_pad: sink pad at the root of the local pipeline > > > + * @__sink_streams: streams to start > > > > Any reason for the double underscore ? > > The function used internally a variable without the underscores for a > different purpose. Then it's better to use the __ for internal variables instead of the publicly documented API. Or rename internal variables so they don't clash. > > > + * > > > + * Use to tell whether streaming should start on a video node. @func returns > > > + * true if streaming has been started on a given video node. @__sink_pad and > > > + * @__sink_streams are filled with pad and streams on the sub-device closest to > > > + * the video nodes, to be used for calling v4l2_subdev_enable_streams() and > > > + * v4l2_subdev_disable_streams(). > > > + * > > > + * Using v4l2_mc_pipeline_enabled() has a few limitations currently (consider it > > > + * a to-do list): > > > + * * only unbranched streams can be supported albeit adding support for > > > + * downstream branches would be fairly trivial, > > > > I can't tell from the documentation here what you mean exactly by > > "unbranched streams". > > These may have been referred to as "linear" streams elsewhere. I'll address > this in the next version. > > > > + * * streams within a single source sub-device are considered to start at the > > > + * same time, more control could be added in two ways: 1) for sources to > > > + * determine stream starting, a control could be added to UAPI and 2) sources > > > + * could tell which streams start at the same time using a sub-device > > > + * operation, > > > + * * CSI-2 VC framing is ignored currently, but VC-based stream starting could > > > + * be implemented by letting the caller to provide a function to determine > > > + * which streams are of interest and > > > + * * routes leading to nowhere are ignored, on some hardware this is a problem, > > > + * but this can also be rather trivially addressed. > > > > I'm afraid this function looks like a hack, to solve a problem that is > > not even explicitly described. You don't explain the issue in the cover > > letter or in Documentation/, the cover letter merely states that this is > > a "partial solution". The documentation of the function doesn't explain > > what criteria the decision is based on. We need a proper explanation of > > the problem in Documentation/, with a description of the behaviour (or > > behaviours) drivers are expected to implement. > > I believe this is discussed in the cover letter, not in detail though. I'm > fine with adding more documentation, there isn't much as I also wanted to > get feedback on the approach itself. But that's my point, I have trouble giving feedback on the approach if you don't explain what problem you're trying to solve, and why you picked this particular solution. It may be all clear in your head, but it isn't for me. > No alternatives have been proposed so > far either. > > > Furthermore, on the implementation side, things are fairly inefficient. > > We already traverse the whole pipeline in media_pipeline_start(), based > > on links and routes, and populate the media_pipeline structure. We > > shouldn't do the same here, but instead inspect media_pipeline to > > extract the information we need. If you're missing information there, > > let's add it. > > The media pipeline is created and traversed, yes, but the media pipeline > does not include streams which are strictly a V4L2 concept. I agree there > is some overlap between the two but as long as MC remains separate from > V4L2, we can't reasonably access streams from the pipeline traversal. > > > > + * > > > + * Return: > > > + * * 0: Success, but don't start streaming yet > > > + * * 1: Success, now it's time to start streaming > > > + * * -ENXIO: Route traversal encountered a non-video device/sub-device entity > > > + * * -ENOTUNIQ: No unique remote pad > > > + * * -ENOLINK: No remote pad found > > > + * * -ENOENT: Enabled upstream route not found > > > + * * -EMLINK: No unique downstream route found > > > + * * -EINVAL: Stream could not be followed to source or was not produced by > > > + * the source > > > + */ > > > +int v4l2_mc_pipeline_enabled(struct video_device *vdev, > > > + bool (*func)(struct video_device *vdev), > > > + struct media_pad **__sink_pad, > > > + u64 *__sink_streams); > > > + > > > #else /* CONFIG_MEDIA_CONTROLLER */ > > > > > > static inline int v4l2_mc_create_media_graph(struct media_device *mdev) -- Regards, Laurent Pinchart