| Message ID | 20240116084240.14228-2-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: Fix mishandling of MEDIA_PAD_FL_MUST_CONNECT flag | expand |
Hi Laurent, On Tue, Jan 16, 2024 at 10:42:34AM +0200, Laurent Pinchart wrote: > When building pipelines by following links, the > media_pipeline_explore_next_link() function only traverses enabled > links. The remote pad of a disabled link is not added to the pipeline, > and neither is the local pad. While the former is correct as disabled > links should not be followed, not adding the local pad breaks processing > of the MEDIA_PAD_FL_MUST_CONNECT flag. > > The MEDIA_PAD_FL_MUST_CONNECT flag is checked in the > __media_pipeline_start() function that iterates over all pads after > populating the pipeline. If the pad is not present, the check gets > skipped, rendering it useless. > > Fix this by adding the local pad of all links regardless of their state, > only skipping the remote pad for disabled links. > > Cc: Alexander Shiyan <eagle.alexander923@gmail.com> > Fixes: ae219872834a ("media: mc: entity: Rewrite media_pipeline_start()") > Reported-by: Frieder Schrempf <frieder.schrempf@kontron.de> > Closes: https://lore.kernel.org/linux-media/7658a15a-80c5-219f-2477-2a94ba6c6ba1@kontron.de > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> Could you add Cc: stable when sending the PR? This should be applied to 6.1 and later.
Hi Sakari, On Tue, Jan 16, 2024 at 11:27:29AM +0000, Sakari Ailus wrote: > On Tue, Jan 16, 2024 at 10:42:34AM +0200, Laurent Pinchart wrote: > > When building pipelines by following links, the > > media_pipeline_explore_next_link() function only traverses enabled > > links. The remote pad of a disabled link is not added to the pipeline, > > and neither is the local pad. While the former is correct as disabled > > links should not be followed, not adding the local pad breaks processing > > of the MEDIA_PAD_FL_MUST_CONNECT flag. > > > > The MEDIA_PAD_FL_MUST_CONNECT flag is checked in the > > __media_pipeline_start() function that iterates over all pads after > > populating the pipeline. If the pad is not present, the check gets > > skipped, rendering it useless. > > > > Fix this by adding the local pad of all links regardless of their state, > > only skipping the remote pad for disabled links. > > > > Cc: Alexander Shiyan <eagle.alexander923@gmail.com> > > Fixes: ae219872834a ("media: mc: entity: Rewrite media_pipeline_start()") > > Reported-by: Frieder Schrempf <frieder.schrempf@kontron.de> > > Closes: https://lore.kernel.org/linux-media/7658a15a-80c5-219f-2477-2a94ba6c6ba1@kontron.de > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Could you add Cc: stable when sending the PR? This should be applied to 6.1 > and later. I'll do so, far all patches in the series.
On Tue, Jan 16, 2024 at 03:35:16PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > On Tue, Jan 16, 2024 at 11:27:29AM +0000, Sakari Ailus wrote: > > On Tue, Jan 16, 2024 at 10:42:34AM +0200, Laurent Pinchart wrote: > > > When building pipelines by following links, the > > > media_pipeline_explore_next_link() function only traverses enabled > > > links. The remote pad of a disabled link is not added to the pipeline, > > > and neither is the local pad. While the former is correct as disabled > > > links should not be followed, not adding the local pad breaks processing > > > of the MEDIA_PAD_FL_MUST_CONNECT flag. > > > > > > The MEDIA_PAD_FL_MUST_CONNECT flag is checked in the > > > __media_pipeline_start() function that iterates over all pads after > > > populating the pipeline. If the pad is not present, the check gets > > > skipped, rendering it useless. > > > > > > Fix this by adding the local pad of all links regardless of their state, > > > only skipping the remote pad for disabled links. > > > > > > Cc: Alexander Shiyan <eagle.alexander923@gmail.com> > > > Fixes: ae219872834a ("media: mc: entity: Rewrite media_pipeline_start()") > > > Reported-by: Frieder Schrempf <frieder.schrempf@kontron.de> > > > Closes: https://lore.kernel.org/linux-media/7658a15a-80c5-219f-2477-2a94ba6c6ba1@kontron.de > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > Could you add Cc: stable when sending the PR? This should be applied to 6.1 > > and later. > > I'll do so, far all patches in the series. Thanks!
diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c index 543a392f8635..a6f28366106f 100644 --- a/drivers/media/mc/mc-entity.c +++ b/drivers/media/mc/mc-entity.c @@ -620,13 +620,6 @@ static int media_pipeline_explore_next_link(struct media_pipeline *pipe, link->source->entity->name, link->source->index, link->sink->entity->name, link->sink->index); - /* Skip links that are not enabled. */ - if (!(link->flags & MEDIA_LNK_FL_ENABLED)) { - dev_dbg(walk->mdev->dev, - "media pipeline: skipping link (disabled)\n"); - return 0; - } - /* Get the local pad and remote pad. */ if (link->source->entity == pad->entity) { local = link->source; @@ -648,13 +641,20 @@ static int media_pipeline_explore_next_link(struct media_pipeline *pipe, } /* - * Add the local and remote pads of the link to the pipeline and push - * them to the stack, if they're not already present. + * Add the local pad of the link to the pipeline and push it to the + * stack, if not already present. */ ret = media_pipeline_add_pad(pipe, walk, local); if (ret) return ret; + /* Similarly, add the remote pad, but only if the link is enabled. */ + if (!(link->flags & MEDIA_LNK_FL_ENABLED)) { + dev_dbg(walk->mdev->dev, + "media pipeline: skipping link (disabled)\n"); + return 0; + } + ret = media_pipeline_add_pad(pipe, walk, remote); if (ret) return ret;
When building pipelines by following links, the media_pipeline_explore_next_link() function only traverses enabled links. The remote pad of a disabled link is not added to the pipeline, and neither is the local pad. While the former is correct as disabled links should not be followed, not adding the local pad breaks processing of the MEDIA_PAD_FL_MUST_CONNECT flag. The MEDIA_PAD_FL_MUST_CONNECT flag is checked in the __media_pipeline_start() function that iterates over all pads after populating the pipeline. If the pad is not present, the check gets skipped, rendering it useless. Fix this by adding the local pad of all links regardless of their state, only skipping the remote pad for disabled links. Cc: Alexander Shiyan <eagle.alexander923@gmail.com> Fixes: ae219872834a ("media: mc: entity: Rewrite media_pipeline_start()") Reported-by: Frieder Schrempf <frieder.schrempf@kontron.de> Closes: https://lore.kernel.org/linux-media/7658a15a-80c5-219f-2477-2a94ba6c6ba1@kontron.de Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/mc/mc-entity.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)