* [PATCH 0/2] media: mc: Simplify media pipeline start/stop
@ 2022-01-13 15:00 Laurent Pinchart
2022-01-13 15:00 ` [PATCH 1/2] media: media-entity: Add media_pad_is_streaming() helper function Laurent Pinchart
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Laurent Pinchart @ 2022-01-13 15:00 UTC (permalink / raw)
To: linux-media
Cc: linux-renesas-soc, Sakari Ailus, Niklas Söderlund,
Tomi Valkeinen, Jacopo Mondi
Hello,
This small series simplifies the implementation of the
media_pipeline_start() function (and to a lesser extend, the
media_pipeline_stop() function as well) by avoiding unnecessary graph
walks.
Patch 1/2 adds a small inline helper function to avoid direct access to
the media_entity.stream_count field in drivers, making it easier in
patch 2/2 to rework the implementation (dropping the stream_count field)
without disturbing drivers. Please see patch 2/2 for a detailed
explanation of the simplification.
Laurent Pinchart (2):
media: media-entity: Add media_pad_is_streaming() helper function
media: media-entity: Simplify media_pipeline_start()
drivers/media/mc/mc-entity.c | 55 ++++++++-----------
drivers/media/platform/exynos4-is/common.c | 5 +-
drivers/media/platform/exynos4-is/fimc-isp.c | 2 +-
drivers/media/platform/exynos4-is/fimc-lite.c | 6 +-
drivers/media/platform/rcar-vin/rcar-core.c | 2 +-
include/media/media-entity.h | 21 +++++--
6 files changed, 48 insertions(+), 43 deletions(-)
base-commit: 68b9bcc8a534cd11fe55f8bc82f948aae7d81b3c
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] media: media-entity: Add media_pad_is_streaming() helper function
2022-01-13 15:00 [PATCH 0/2] media: mc: Simplify media pipeline start/stop Laurent Pinchart
@ 2022-01-13 15:00 ` Laurent Pinchart
2022-01-13 15:00 ` [PATCH 2/2] media: media-entity: Simplify media_pipeline_start() Laurent Pinchart
2022-01-13 15:28 ` [PATCH 0/2] media: mc: Simplify media pipeline start/stop Laurent Pinchart
2 siblings, 0 replies; 8+ messages in thread
From: Laurent Pinchart @ 2022-01-13 15:00 UTC (permalink / raw)
To: linux-media
Cc: linux-renesas-soc, Sakari Ailus, Niklas Söderlund,
Tomi Valkeinen, Jacopo Mondi, Laurent Pinchart
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Add a function to test if a pad is part of a pipeline currently
streaming, and use it through drivers to replace direct access to the
stream_count field. This will help reworking pipeline start/stop without
disturbing drivers.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/mc/mc-entity.c | 3 ++-
drivers/media/platform/exynos4-is/common.c | 5 ++++-
drivers/media/platform/exynos4-is/fimc-isp.c | 2 +-
drivers/media/platform/exynos4-is/fimc-lite.c | 6 +++---
drivers/media/platform/rcar-vin/rcar-core.c | 2 +-
include/media/media-entity.h | 12 ++++++++++++
6 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index b411f9796191..f83e043f0f3b 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -834,7 +834,8 @@ int __media_entity_setup_link(struct media_link *link, u32 flags)
sink = link->sink->entity;
if (!(link->flags & MEDIA_LNK_FL_DYNAMIC) &&
- (source->stream_count || sink->stream_count))
+ (media_entity_is_streaming(source) ||
+ media_entity_is_streaming(sink)))
return -EBUSY;
mdev = source->graph_obj.mdev;
diff --git a/drivers/media/platform/exynos4-is/common.c b/drivers/media/platform/exynos4-is/common.c
index 944b224eb621..023f624d29d5 100644
--- a/drivers/media/platform/exynos4-is/common.c
+++ b/drivers/media/platform/exynos4-is/common.c
@@ -10,7 +10,10 @@
#include <media/drv-intf/exynos-fimc.h>
#include "common.h"
-/* Called with the media graph mutex held or entity->stream_count > 0. */
+/*
+ * Called with the media graph mutex held or media_entity_is_streaming(entity)
+ * true.
+ */
struct v4l2_subdev *fimc_find_remote_sensor(struct media_entity *entity)
{
struct media_pad *pad = &entity->pads[0];
diff --git a/drivers/media/platform/exynos4-is/fimc-isp.c b/drivers/media/platform/exynos4-is/fimc-isp.c
index 855235bea46d..b85986e50f46 100644
--- a/drivers/media/platform/exynos4-is/fimc-isp.c
+++ b/drivers/media/platform/exynos4-is/fimc-isp.c
@@ -226,7 +226,7 @@ static int fimc_isp_subdev_set_fmt(struct v4l2_subdev *sd,
}
}
} else {
- if (sd->entity.stream_count == 0) {
+ if (!media_entity_is_streaming(&sd->entity)) {
if (fmt->pad == FIMC_ISP_SD_PAD_SINK) {
struct v4l2_subdev_format format = *fmt;
diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c
index aaa3af0493ce..386ee3948ac6 100644
--- a/drivers/media/platform/exynos4-is/fimc-lite.c
+++ b/drivers/media/platform/exynos4-is/fimc-lite.c
@@ -1073,7 +1073,7 @@ static int fimc_lite_subdev_set_fmt(struct v4l2_subdev *sd,
mutex_lock(&fimc->lock);
if ((atomic_read(&fimc->out_path) == FIMC_IO_ISP &&
- sd->entity.stream_count > 0) ||
+ media_entity_is_streaming(&sd->entity)) ||
(atomic_read(&fimc->out_path) == FIMC_IO_DMA &&
vb2_is_busy(&fimc->vb_queue))) {
mutex_unlock(&fimc->lock);
@@ -1197,8 +1197,8 @@ static int fimc_lite_subdev_s_stream(struct v4l2_subdev *sd, int on)
* Find sensor subdev linked to FIMC-LITE directly or through
* MIPI-CSIS. This is required for configuration where FIMC-LITE
* is used as a subdev only and feeds data internally to FIMC-IS.
- * The pipeline links are protected through entity.stream_count
- * so there is no need to take the media graph mutex here.
+ * The pipeline links are protected through entity.pipe so there is no
+ * need to take the media graph mutex here.
*/
fimc->sensor = fimc_find_remote_sensor(&sd->entity);
diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 0186ae235113..5117a7a3b5ec 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -816,7 +816,7 @@ static int rvin_csi2_link_notify(struct media_link *link, u32 flags,
* running streams.
*/
media_device_for_each_entity(entity, &group->mdev)
- if (entity->stream_count)
+ if (media_entity_is_streaming(entity))
return -EBUSY;
mutex_lock(&group->lock);
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index fea489f03d57..8546f13c42a9 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -858,6 +858,18 @@ struct media_link *media_entity_find_link(struct media_pad *source,
*/
struct media_pad *media_entity_remote_pad(const struct media_pad *pad);
+/**
+ * media_entity_is_streaming - Test if an entity is part of a streaming pipeline
+ * @entity: The entity
+ *
+ * Return: True if the entity is part of a pipeline started with the
+ * media_pipeline_start() function, false otherwise.
+ */
+static inline bool media_entity_is_streaming(const struct media_entity *entity)
+{
+ return entity->stream_count > 0;
+}
+
/**
* media_entity_get_fwnode_pad - Get pad number from fwnode
*
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] media: media-entity: Simplify media_pipeline_start()
2022-01-13 15:00 [PATCH 0/2] media: mc: Simplify media pipeline start/stop Laurent Pinchart
2022-01-13 15:00 ` [PATCH 1/2] media: media-entity: Add media_pad_is_streaming() helper function Laurent Pinchart
@ 2022-01-13 15:00 ` Laurent Pinchart
2022-01-15 17:39 ` Sakari Ailus
2022-01-13 15:28 ` [PATCH 0/2] media: mc: Simplify media pipeline start/stop Laurent Pinchart
2 siblings, 1 reply; 8+ messages in thread
From: Laurent Pinchart @ 2022-01-13 15:00 UTC (permalink / raw)
To: linux-media
Cc: linux-renesas-soc, Sakari Ailus, Niklas Söderlund,
Tomi Valkeinen, Jacopo Mondi, Laurent Pinchart
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
The media_pipeline_start() function has two purposes: it constructs a
pipeline by recording the entities that are part of it, gathered from a
graph walk, and validate the media links. The pipeline pointer is stored
in the media_entity structure as part of this process, and the entity's
stream count is increased, to record that the entity is streaming.
When multiple video nodes are present in a pipeline,
media_pipeline_start() is typically called on all of them, with the same
pipeline pointer. This is taken into account in media_pipeline_start()
by skipping validation for entities that are already part of the
pipeline, while returning an error if an entity is part of a different
pipeline.
It turns out that this process is overly complicated. When
media_pipeline_start() is called for the first time, it constructs the
full pipeline, adding all entities and validating all the links.
Subsequent calls to media_pipeline_start() are then nearly no-ops, they
only increase the stream count on the pipeline and on all entities.
The media_entity stream_count field is used for two purposes: checking
if the entity is streaming, and detecting when a call to
media_pipeline_stop() balances needs to reset the entity pipe pointer to
NULL. The former can easily be replaced by a check of the pipe pointer.
Simplify media_pipeline_start() by avoiding the pipeline walk on all
calls but the first one, and drop the media_entity stream_count field.
media_pipeline_stop() is updated accordingly.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/mc/mc-entity.c | 52 +++++++++++++++---------------------
include/media/media-entity.h | 11 +++-----
2 files changed, 26 insertions(+), 37 deletions(-)
diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index f83e043f0f3b..8ab0913d8d82 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -396,20 +396,21 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
struct media_link *link;
int ret;
- if (!pipe->streaming_count++) {
- ret = media_graph_walk_init(&pipe->graph, mdev);
- if (ret)
- goto error_graph_walk_start;
+ if (pipe->streaming_count) {
+ pipe->streaming_count++;
+ return 0;
}
+ ret = media_graph_walk_init(&pipe->graph, mdev);
+ if (ret)
+ return ret;
+
media_graph_walk_start(&pipe->graph, entity);
while ((entity = media_graph_walk_next(graph))) {
DECLARE_BITMAP(active, MEDIA_ENTITY_MAX_PADS);
DECLARE_BITMAP(has_no_links, MEDIA_ENTITY_MAX_PADS);
- entity->stream_count++;
-
if (entity->pipe && entity->pipe != pipe) {
pr_err("Pipe active for %s. Can't start for %s\n",
entity->name,
@@ -418,12 +419,12 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
goto error;
}
- entity->pipe = pipe;
-
/* Already streaming --- no need to check. */
- if (entity->stream_count > 1)
+ if (entity->pipe)
continue;
+ entity->pipe = pipe;
+
if (!entity->ops || !entity->ops->link_validate)
continue;
@@ -479,6 +480,8 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
}
}
+ pipe->streaming_count++;
+
return 0;
error:
@@ -489,24 +492,17 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
media_graph_walk_start(graph, entity_err);
while ((entity_err = media_graph_walk_next(graph))) {
- /* Sanity check for negative stream_count */
- if (!WARN_ON_ONCE(entity_err->stream_count <= 0)) {
- entity_err->stream_count--;
- if (entity_err->stream_count == 0)
- entity_err->pipe = NULL;
- }
+ entity_err->pipe = NULL;
/*
- * We haven't increased stream_count further than this
- * so we quit here.
+ * We haven't started entities further than this so we quit
+ * here.
*/
if (entity_err == entity)
break;
}
-error_graph_walk_start:
- if (!--pipe->streaming_count)
- media_graph_walk_cleanup(graph);
+ media_graph_walk_cleanup(graph);
return ret;
}
@@ -537,19 +533,15 @@ void __media_pipeline_stop(struct media_entity *entity)
if (WARN_ON(!pipe))
return;
+ if (--pipe->streaming_count)
+ return;
+
media_graph_walk_start(graph, entity);
- while ((entity = media_graph_walk_next(graph))) {
- /* Sanity check for negative stream_count */
- if (!WARN_ON_ONCE(entity->stream_count <= 0)) {
- entity->stream_count--;
- if (entity->stream_count == 0)
- entity->pipe = NULL;
- }
- }
+ while ((entity = media_graph_walk_next(graph)))
+ entity->pipe = NULL;
- if (!--pipe->streaming_count)
- media_graph_walk_cleanup(graph);
+ media_graph_walk_cleanup(graph);
}
EXPORT_SYMBOL_GPL(__media_pipeline_stop);
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 8546f13c42a9..e3c4fd1e3623 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -268,7 +268,6 @@ enum media_entity_type {
* @pads: Pads array with the size defined by @num_pads.
* @links: List of data links.
* @ops: Entity operations.
- * @stream_count: Stream count for the entity.
* @use_count: Use count for the entity.
* @pipe: Pipeline this entity belongs to.
* @info: Union with devnode information. Kept just for backward
@@ -283,10 +282,9 @@ enum media_entity_type {
*
* .. note::
*
- * @stream_count and @use_count reference counts must never be
- * negative, but are signed integers on purpose: a simple ``WARN_ON(<0)``
- * check can be used to detect reference count bugs that would make them
- * negative.
+ * The @use_count reference count must never be negative, but is a signed
+ * integer on purpose: a simple ``WARN_ON(<0)`` check can be used to detect
+ * reference count bugs that would make it negative.
*/
struct media_entity {
struct media_gobj graph_obj; /* must be first field in struct */
@@ -305,7 +303,6 @@ struct media_entity {
const struct media_entity_operations *ops;
- int stream_count;
int use_count;
struct media_pipeline *pipe;
@@ -867,7 +864,7 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad);
*/
static inline bool media_entity_is_streaming(const struct media_entity *entity)
{
- return entity->stream_count > 0;
+ return entity->pipe != NULL;
}
/**
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] media: mc: Simplify media pipeline start/stop
2022-01-13 15:00 [PATCH 0/2] media: mc: Simplify media pipeline start/stop Laurent Pinchart
2022-01-13 15:00 ` [PATCH 1/2] media: media-entity: Add media_pad_is_streaming() helper function Laurent Pinchart
2022-01-13 15:00 ` [PATCH 2/2] media: media-entity: Simplify media_pipeline_start() Laurent Pinchart
@ 2022-01-13 15:28 ` Laurent Pinchart
2 siblings, 0 replies; 8+ messages in thread
From: Laurent Pinchart @ 2022-01-13 15:28 UTC (permalink / raw)
To: linux-media
Cc: linux-renesas-soc, Sakari Ailus, Niklas Söderlund,
Tomi Valkeinen, Jacopo Mondi
On Thu, Jan 13, 2022 at 05:00:40PM +0200, Laurent Pinchart wrote:
> Hello,
>
> This small series simplifies the implementation of the
> media_pipeline_start() function (and to a lesser extend, the
> media_pipeline_stop() function as well) by avoiding unnecessary graph
> walks.
>
> Patch 1/2 adds a small inline helper function to avoid direct access to
> the media_entity.stream_count field in drivers, making it easier in
> patch 2/2 to rework the implementation (dropping the stream_count field)
> without disturbing drivers. Please see patch 2/2 for a detailed
> explanation of the simplification.
I forgot to mention that I've tested this with the Renesas R-Car VSP1
test suite that has pipeline with multiple output and capture video
nodes, as well on a Xilinx platform with a pipeline that has two capture
video nodes.
> Laurent Pinchart (2):
> media: media-entity: Add media_pad_is_streaming() helper function
> media: media-entity: Simplify media_pipeline_start()
>
> drivers/media/mc/mc-entity.c | 55 ++++++++-----------
> drivers/media/platform/exynos4-is/common.c | 5 +-
> drivers/media/platform/exynos4-is/fimc-isp.c | 2 +-
> drivers/media/platform/exynos4-is/fimc-lite.c | 6 +-
> drivers/media/platform/rcar-vin/rcar-core.c | 2 +-
> include/media/media-entity.h | 21 +++++--
> 6 files changed, 48 insertions(+), 43 deletions(-)
>
>
> base-commit: 68b9bcc8a534cd11fe55f8bc82f948aae7d81b3c
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] media: media-entity: Simplify media_pipeline_start()
2022-01-13 15:00 ` [PATCH 2/2] media: media-entity: Simplify media_pipeline_start() Laurent Pinchart
@ 2022-01-15 17:39 ` Sakari Ailus
2022-01-15 20:02 ` Laurent Pinchart
0 siblings, 1 reply; 8+ messages in thread
From: Sakari Ailus @ 2022-01-15 17:39 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, linux-renesas-soc, Niklas Söderlund,
Tomi Valkeinen, Jacopo Mondi, Laurent Pinchart
Moi,
Thanks for the set.
On Thu, Jan 13, 2022 at 05:00:42PM +0200, Laurent Pinchart wrote:
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> The media_pipeline_start() function has two purposes: it constructs a
> pipeline by recording the entities that are part of it, gathered from a
> graph walk, and validate the media links. The pipeline pointer is stored
> in the media_entity structure as part of this process, and the entity's
> stream count is increased, to record that the entity is streaming.
>
> When multiple video nodes are present in a pipeline,
> media_pipeline_start() is typically called on all of them, with the same
> pipeline pointer. This is taken into account in media_pipeline_start()
> by skipping validation for entities that are already part of the
> pipeline, while returning an error if an entity is part of a different
> pipeline.
>
> It turns out that this process is overly complicated. When
> media_pipeline_start() is called for the first time, it constructs the
> full pipeline, adding all entities and validating all the links.
> Subsequent calls to media_pipeline_start() are then nearly no-ops, they
> only increase the stream count on the pipeline and on all entities.
>
> The media_entity stream_count field is used for two purposes: checking
> if the entity is streaming, and detecting when a call to
> media_pipeline_stop() balances needs to reset the entity pipe pointer to
> NULL. The former can easily be replaced by a check of the pipe pointer.
>
> Simplify media_pipeline_start() by avoiding the pipeline walk on all
> calls but the first one, and drop the media_entity stream_count field.
> media_pipeline_stop() is updated accordingly.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/mc/mc-entity.c | 52 +++++++++++++++---------------------
> include/media/media-entity.h | 11 +++-----
> 2 files changed, 26 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index f83e043f0f3b..8ab0913d8d82 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -396,20 +396,21 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
> struct media_link *link;
> int ret;
>
> - if (!pipe->streaming_count++) {
> - ret = media_graph_walk_init(&pipe->graph, mdev);
> - if (ret)
> - goto error_graph_walk_start;
> + if (pipe->streaming_count) {
> + pipe->streaming_count++;
> + return 0;
> }
>
> + ret = media_graph_walk_init(&pipe->graph, mdev);
> + if (ret)
> + return ret;
> +
> media_graph_walk_start(&pipe->graph, entity);
>
> while ((entity = media_graph_walk_next(graph))) {
> DECLARE_BITMAP(active, MEDIA_ENTITY_MAX_PADS);
> DECLARE_BITMAP(has_no_links, MEDIA_ENTITY_MAX_PADS);
>
> - entity->stream_count++;
> -
> if (entity->pipe && entity->pipe != pipe) {
> pr_err("Pipe active for %s. Can't start for %s\n",
> entity->name,
> @@ -418,12 +419,12 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
> goto error;
> }
>
> - entity->pipe = pipe;
> -
> /* Already streaming --- no need to check. */
> - if (entity->stream_count > 1)
> + if (entity->pipe)
> continue;
>
> + entity->pipe = pipe;
> +
> if (!entity->ops || !entity->ops->link_validate)
> continue;
>
> @@ -479,6 +480,8 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
> }
> }
>
> + pipe->streaming_count++;
> +
> return 0;
>
> error:
> @@ -489,24 +492,17 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
> media_graph_walk_start(graph, entity_err);
>
> while ((entity_err = media_graph_walk_next(graph))) {
> - /* Sanity check for negative stream_count */
> - if (!WARN_ON_ONCE(entity_err->stream_count <= 0)) {
> - entity_err->stream_count--;
> - if (entity_err->stream_count == 0)
> - entity_err->pipe = NULL;
> - }
> + entity_err->pipe = NULL;
>
> /*
> - * We haven't increased stream_count further than this
> - * so we quit here.
> + * We haven't started entities further than this so we quit
> + * here.
> */
> if (entity_err == entity)
> break;
> }
>
> -error_graph_walk_start:
> - if (!--pipe->streaming_count)
> - media_graph_walk_cleanup(graph);
> + media_graph_walk_cleanup(graph);
>
> return ret;
> }
> @@ -537,19 +533,15 @@ void __media_pipeline_stop(struct media_entity *entity)
> if (WARN_ON(!pipe))
> return;
>
> + if (--pipe->streaming_count)
> + return;
> +
> media_graph_walk_start(graph, entity);
>
> - while ((entity = media_graph_walk_next(graph))) {
> - /* Sanity check for negative stream_count */
> - if (!WARN_ON_ONCE(entity->stream_count <= 0)) {
> - entity->stream_count--;
> - if (entity->stream_count == 0)
> - entity->pipe = NULL;
> - }
> - }
> + while ((entity = media_graph_walk_next(graph)))
> + entity->pipe = NULL;
>
> - if (!--pipe->streaming_count)
> - media_graph_walk_cleanup(graph);
> + media_graph_walk_cleanup(graph);
>
> }
> EXPORT_SYMBOL_GPL(__media_pipeline_stop);
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 8546f13c42a9..e3c4fd1e3623 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -268,7 +268,6 @@ enum media_entity_type {
> * @pads: Pads array with the size defined by @num_pads.
> * @links: List of data links.
> * @ops: Entity operations.
> - * @stream_count: Stream count for the entity.
> * @use_count: Use count for the entity.
> * @pipe: Pipeline this entity belongs to.
> * @info: Union with devnode information. Kept just for backward
> @@ -283,10 +282,9 @@ enum media_entity_type {
> *
> * .. note::
> *
> - * @stream_count and @use_count reference counts must never be
> - * negative, but are signed integers on purpose: a simple ``WARN_ON(<0)``
> - * check can be used to detect reference count bugs that would make them
> - * negative.
> + * The @use_count reference count must never be negative, but is a signed
> + * integer on purpose: a simple ``WARN_ON(<0)`` check can be used to detect
> + * reference count bugs that would make it negative.
> */
> struct media_entity {
> struct media_gobj graph_obj; /* must be first field in struct */
> @@ -305,7 +303,6 @@ struct media_entity {
>
> const struct media_entity_operations *ops;
>
> - int stream_count;
> int use_count;
>
> struct media_pipeline *pipe;
> @@ -867,7 +864,7 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad);
> */
> static inline bool media_entity_is_streaming(const struct media_entity *entity)
> {
> - return entity->stream_count > 0;
> + return entity->pipe != NULL;
I'd drop "!= NULL" part; it's redundant.
I'll do that when applying if that's fine.
> }
>
> /**
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] media: media-entity: Simplify media_pipeline_start()
2022-01-15 17:39 ` Sakari Ailus
@ 2022-01-15 20:02 ` Laurent Pinchart
2022-02-11 21:27 ` Laurent Pinchart
0 siblings, 1 reply; 8+ messages in thread
From: Laurent Pinchart @ 2022-01-15 20:02 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, linux-renesas-soc, Niklas Söderlund,
Tomi Valkeinen, Jacopo Mondi
Hi Sakari,
On Sat, Jan 15, 2022 at 07:39:33PM +0200, Sakari Ailus wrote:
> Moi,
>
> Thanks for the set.
Ole hyvä.
> On Thu, Jan 13, 2022 at 05:00:42PM +0200, Laurent Pinchart wrote:
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > The media_pipeline_start() function has two purposes: it constructs a
> > pipeline by recording the entities that are part of it, gathered from a
> > graph walk, and validate the media links. The pipeline pointer is stored
> > in the media_entity structure as part of this process, and the entity's
> > stream count is increased, to record that the entity is streaming.
> >
> > When multiple video nodes are present in a pipeline,
> > media_pipeline_start() is typically called on all of them, with the same
> > pipeline pointer. This is taken into account in media_pipeline_start()
> > by skipping validation for entities that are already part of the
> > pipeline, while returning an error if an entity is part of a different
> > pipeline.
> >
> > It turns out that this process is overly complicated. When
> > media_pipeline_start() is called for the first time, it constructs the
> > full pipeline, adding all entities and validating all the links.
> > Subsequent calls to media_pipeline_start() are then nearly no-ops, they
> > only increase the stream count on the pipeline and on all entities.
> >
> > The media_entity stream_count field is used for two purposes: checking
> > if the entity is streaming, and detecting when a call to
> > media_pipeline_stop() balances needs to reset the entity pipe pointer to
> > NULL. The former can easily be replaced by a check of the pipe pointer.
> >
> > Simplify media_pipeline_start() by avoiding the pipeline walk on all
> > calls but the first one, and drop the media_entity stream_count field.
> > media_pipeline_stop() is updated accordingly.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > drivers/media/mc/mc-entity.c | 52 +++++++++++++++---------------------
> > include/media/media-entity.h | 11 +++-----
> > 2 files changed, 26 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> > index f83e043f0f3b..8ab0913d8d82 100644
> > --- a/drivers/media/mc/mc-entity.c
> > +++ b/drivers/media/mc/mc-entity.c
> > @@ -396,20 +396,21 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
> > struct media_link *link;
> > int ret;
> >
> > - if (!pipe->streaming_count++) {
> > - ret = media_graph_walk_init(&pipe->graph, mdev);
> > - if (ret)
> > - goto error_graph_walk_start;
> > + if (pipe->streaming_count) {
> > + pipe->streaming_count++;
> > + return 0;
> > }
> >
> > + ret = media_graph_walk_init(&pipe->graph, mdev);
> > + if (ret)
> > + return ret;
> > +
> > media_graph_walk_start(&pipe->graph, entity);
> >
> > while ((entity = media_graph_walk_next(graph))) {
> > DECLARE_BITMAP(active, MEDIA_ENTITY_MAX_PADS);
> > DECLARE_BITMAP(has_no_links, MEDIA_ENTITY_MAX_PADS);
> >
> > - entity->stream_count++;
> > -
> > if (entity->pipe && entity->pipe != pipe) {
> > pr_err("Pipe active for %s. Can't start for %s\n",
> > entity->name,
> > @@ -418,12 +419,12 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
> > goto error;
> > }
> >
> > - entity->pipe = pipe;
> > -
> > /* Already streaming --- no need to check. */
> > - if (entity->stream_count > 1)
> > + if (entity->pipe)
> > continue;
> >
> > + entity->pipe = pipe;
> > +
> > if (!entity->ops || !entity->ops->link_validate)
> > continue;
> >
> > @@ -479,6 +480,8 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
> > }
> > }
> >
> > + pipe->streaming_count++;
> > +
> > return 0;
> >
> > error:
> > @@ -489,24 +492,17 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
> > media_graph_walk_start(graph, entity_err);
> >
> > while ((entity_err = media_graph_walk_next(graph))) {
> > - /* Sanity check for negative stream_count */
> > - if (!WARN_ON_ONCE(entity_err->stream_count <= 0)) {
> > - entity_err->stream_count--;
> > - if (entity_err->stream_count == 0)
> > - entity_err->pipe = NULL;
> > - }
> > + entity_err->pipe = NULL;
> >
> > /*
> > - * We haven't increased stream_count further than this
> > - * so we quit here.
> > + * We haven't started entities further than this so we quit
> > + * here.
> > */
> > if (entity_err == entity)
> > break;
> > }
> >
> > -error_graph_walk_start:
> > - if (!--pipe->streaming_count)
> > - media_graph_walk_cleanup(graph);
> > + media_graph_walk_cleanup(graph);
> >
> > return ret;
> > }
> > @@ -537,19 +533,15 @@ void __media_pipeline_stop(struct media_entity *entity)
> > if (WARN_ON(!pipe))
> > return;
> >
> > + if (--pipe->streaming_count)
> > + return;
> > +
> > media_graph_walk_start(graph, entity);
> >
> > - while ((entity = media_graph_walk_next(graph))) {
> > - /* Sanity check for negative stream_count */
> > - if (!WARN_ON_ONCE(entity->stream_count <= 0)) {
> > - entity->stream_count--;
> > - if (entity->stream_count == 0)
> > - entity->pipe = NULL;
> > - }
> > - }
> > + while ((entity = media_graph_walk_next(graph)))
> > + entity->pipe = NULL;
> >
> > - if (!--pipe->streaming_count)
> > - media_graph_walk_cleanup(graph);
> > + media_graph_walk_cleanup(graph);
> >
> > }
> > EXPORT_SYMBOL_GPL(__media_pipeline_stop);
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index 8546f13c42a9..e3c4fd1e3623 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -268,7 +268,6 @@ enum media_entity_type {
> > * @pads: Pads array with the size defined by @num_pads.
> > * @links: List of data links.
> > * @ops: Entity operations.
> > - * @stream_count: Stream count for the entity.
> > * @use_count: Use count for the entity.
> > * @pipe: Pipeline this entity belongs to.
> > * @info: Union with devnode information. Kept just for backward
> > @@ -283,10 +282,9 @@ enum media_entity_type {
> > *
> > * .. note::
> > *
> > - * @stream_count and @use_count reference counts must never be
> > - * negative, but are signed integers on purpose: a simple ``WARN_ON(<0)``
> > - * check can be used to detect reference count bugs that would make them
> > - * negative.
> > + * The @use_count reference count must never be negative, but is a signed
> > + * integer on purpose: a simple ``WARN_ON(<0)`` check can be used to detect
> > + * reference count bugs that would make it negative.
> > */
> > struct media_entity {
> > struct media_gobj graph_obj; /* must be first field in struct */
> > @@ -305,7 +303,6 @@ struct media_entity {
> >
> > const struct media_entity_operations *ops;
> >
> > - int stream_count;
> > int use_count;
> >
> > struct media_pipeline *pipe;
> > @@ -867,7 +864,7 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad);
> > */
> > static inline bool media_entity_is_streaming(const struct media_entity *entity)
> > {
> > - return entity->stream_count > 0;
> > + return entity->pipe != NULL;
>
> I'd drop "!= NULL" part; it's redundant.
I usually use "if (pointer)" or "if (!pointer)" without comparing to
NULL, but for some reason, when returning a bool, it feels more explicit
to me to use a comparison. I'm not sure why, maybe because, unlike with
if (), the implicit cast is only apparent when you read the signature of
the function ? Not that it's far away in this case, it's only two lines
up.
> I'll do that when applying if that's fine.
Fine with me, I don't mind either way. Thanks.
> > }
> >
> > /**
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] media: media-entity: Simplify media_pipeline_start()
2022-01-15 20:02 ` Laurent Pinchart
@ 2022-02-11 21:27 ` Laurent Pinchart
2022-02-15 21:28 ` Sakari Ailus
0 siblings, 1 reply; 8+ messages in thread
From: Laurent Pinchart @ 2022-02-11 21:27 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, linux-renesas-soc, Niklas Söderlund,
Tomi Valkeinen, Jacopo Mondi
Hi Sakari,
On Sat, Jan 15, 2022 at 10:02:21PM +0200, Laurent Pinchart wrote:
> On Sat, Jan 15, 2022 at 07:39:33PM +0200, Sakari Ailus wrote:
> > On Thu, Jan 13, 2022 at 05:00:42PM +0200, Laurent Pinchart wrote:
> > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >
> > > The media_pipeline_start() function has two purposes: it constructs a
> > > pipeline by recording the entities that are part of it, gathered from a
> > > graph walk, and validate the media links. The pipeline pointer is stored
> > > in the media_entity structure as part of this process, and the entity's
> > > stream count is increased, to record that the entity is streaming.
> > >
> > > When multiple video nodes are present in a pipeline,
> > > media_pipeline_start() is typically called on all of them, with the same
> > > pipeline pointer. This is taken into account in media_pipeline_start()
> > > by skipping validation for entities that are already part of the
> > > pipeline, while returning an error if an entity is part of a different
> > > pipeline.
> > >
> > > It turns out that this process is overly complicated. When
> > > media_pipeline_start() is called for the first time, it constructs the
> > > full pipeline, adding all entities and validating all the links.
> > > Subsequent calls to media_pipeline_start() are then nearly no-ops, they
> > > only increase the stream count on the pipeline and on all entities.
> > >
> > > The media_entity stream_count field is used for two purposes: checking
> > > if the entity is streaming, and detecting when a call to
> > > media_pipeline_stop() balances needs to reset the entity pipe pointer to
> > > NULL. The former can easily be replaced by a check of the pipe pointer.
> > >
> > > Simplify media_pipeline_start() by avoiding the pipeline walk on all
> > > calls but the first one, and drop the media_entity stream_count field.
> > > media_pipeline_stop() is updated accordingly.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > drivers/media/mc/mc-entity.c | 52 +++++++++++++++---------------------
> > > include/media/media-entity.h | 11 +++-----
> > > 2 files changed, 26 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> > > index f83e043f0f3b..8ab0913d8d82 100644
> > > --- a/drivers/media/mc/mc-entity.c
> > > +++ b/drivers/media/mc/mc-entity.c
> > > @@ -396,20 +396,21 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
> > > struct media_link *link;
> > > int ret;
> > >
> > > - if (!pipe->streaming_count++) {
> > > - ret = media_graph_walk_init(&pipe->graph, mdev);
> > > - if (ret)
> > > - goto error_graph_walk_start;
> > > + if (pipe->streaming_count) {
> > > + pipe->streaming_count++;
> > > + return 0;
> > > }
> > >
> > > + ret = media_graph_walk_init(&pipe->graph, mdev);
> > > + if (ret)
> > > + return ret;
> > > +
> > > media_graph_walk_start(&pipe->graph, entity);
> > >
> > > while ((entity = media_graph_walk_next(graph))) {
> > > DECLARE_BITMAP(active, MEDIA_ENTITY_MAX_PADS);
> > > DECLARE_BITMAP(has_no_links, MEDIA_ENTITY_MAX_PADS);
> > >
> > > - entity->stream_count++;
> > > -
> > > if (entity->pipe && entity->pipe != pipe) {
> > > pr_err("Pipe active for %s. Can't start for %s\n",
> > > entity->name,
> > > @@ -418,12 +419,12 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
> > > goto error;
> > > }
> > >
> > > - entity->pipe = pipe;
> > > -
> > > /* Already streaming --- no need to check. */
> > > - if (entity->stream_count > 1)
> > > + if (entity->pipe)
> > > continue;
> > >
> > > + entity->pipe = pipe;
> > > +
> > > if (!entity->ops || !entity->ops->link_validate)
> > > continue;
> > >
> > > @@ -479,6 +480,8 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
> > > }
> > > }
> > >
> > > + pipe->streaming_count++;
> > > +
> > > return 0;
> > >
> > > error:
> > > @@ -489,24 +492,17 @@ __must_check int __media_pipeline_start(struct media_entity *entity,
> > > media_graph_walk_start(graph, entity_err);
> > >
> > > while ((entity_err = media_graph_walk_next(graph))) {
> > > - /* Sanity check for negative stream_count */
> > > - if (!WARN_ON_ONCE(entity_err->stream_count <= 0)) {
> > > - entity_err->stream_count--;
> > > - if (entity_err->stream_count == 0)
> > > - entity_err->pipe = NULL;
> > > - }
> > > + entity_err->pipe = NULL;
> > >
> > > /*
> > > - * We haven't increased stream_count further than this
> > > - * so we quit here.
> > > + * We haven't started entities further than this so we quit
> > > + * here.
> > > */
> > > if (entity_err == entity)
> > > break;
> > > }
> > >
> > > -error_graph_walk_start:
> > > - if (!--pipe->streaming_count)
> > > - media_graph_walk_cleanup(graph);
> > > + media_graph_walk_cleanup(graph);
> > >
> > > return ret;
> > > }
> > > @@ -537,19 +533,15 @@ void __media_pipeline_stop(struct media_entity *entity)
> > > if (WARN_ON(!pipe))
> > > return;
> > >
> > > + if (--pipe->streaming_count)
> > > + return;
> > > +
> > > media_graph_walk_start(graph, entity);
> > >
> > > - while ((entity = media_graph_walk_next(graph))) {
> > > - /* Sanity check for negative stream_count */
> > > - if (!WARN_ON_ONCE(entity->stream_count <= 0)) {
> > > - entity->stream_count--;
> > > - if (entity->stream_count == 0)
> > > - entity->pipe = NULL;
> > > - }
> > > - }
> > > + while ((entity = media_graph_walk_next(graph)))
> > > + entity->pipe = NULL;
> > >
> > > - if (!--pipe->streaming_count)
> > > - media_graph_walk_cleanup(graph);
> > > + media_graph_walk_cleanup(graph);
> > >
> > > }
> > > EXPORT_SYMBOL_GPL(__media_pipeline_stop);
> > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > > index 8546f13c42a9..e3c4fd1e3623 100644
> > > --- a/include/media/media-entity.h
> > > +++ b/include/media/media-entity.h
> > > @@ -268,7 +268,6 @@ enum media_entity_type {
> > > * @pads: Pads array with the size defined by @num_pads.
> > > * @links: List of data links.
> > > * @ops: Entity operations.
> > > - * @stream_count: Stream count for the entity.
> > > * @use_count: Use count for the entity.
> > > * @pipe: Pipeline this entity belongs to.
> > > * @info: Union with devnode information. Kept just for backward
> > > @@ -283,10 +282,9 @@ enum media_entity_type {
> > > *
> > > * .. note::
> > > *
> > > - * @stream_count and @use_count reference counts must never be
> > > - * negative, but are signed integers on purpose: a simple ``WARN_ON(<0)``
> > > - * check can be used to detect reference count bugs that would make them
> > > - * negative.
> > > + * The @use_count reference count must never be negative, but is a signed
> > > + * integer on purpose: a simple ``WARN_ON(<0)`` check can be used to detect
> > > + * reference count bugs that would make it negative.
> > > */
> > > struct media_entity {
> > > struct media_gobj graph_obj; /* must be first field in struct */
> > > @@ -305,7 +303,6 @@ struct media_entity {
> > >
> > > const struct media_entity_operations *ops;
> > >
> > > - int stream_count;
> > > int use_count;
> > >
> > > struct media_pipeline *pipe;
> > > @@ -867,7 +864,7 @@ struct media_pad *media_entity_remote_pad(const struct media_pad *pad);
> > > */
> > > static inline bool media_entity_is_streaming(const struct media_entity *entity)
> > > {
> > > - return entity->stream_count > 0;
> > > + return entity->pipe != NULL;
> >
> > I'd drop "!= NULL" part; it's redundant.
>
> I usually use "if (pointer)" or "if (!pointer)" without comparing to
> NULL, but for some reason, when returning a bool, it feels more explicit
> to me to use a comparison. I'm not sure why, maybe because, unlike with
> if (), the implicit cast is only apparent when you read the signature of
> the function ? Not that it's far away in this case, it's only two lines
> up.
>
> > I'll do that when applying if that's fine.
>
> Fine with me, I don't mind either way. Thanks.
Will you take this series for v5.18 ?
> > > }
> > >
> > > /**
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] media: media-entity: Simplify media_pipeline_start()
2022-02-11 21:27 ` Laurent Pinchart
@ 2022-02-15 21:28 ` Sakari Ailus
0 siblings, 0 replies; 8+ messages in thread
From: Sakari Ailus @ 2022-02-15 21:28 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, linux-renesas-soc, Niklas Söderlund,
Tomi Valkeinen, Jacopo Mondi
On Fri, Feb 11, 2022 at 11:27:22PM +0200, Laurent Pinchart wrote:
> > > I'll do that when applying if that's fine.
> >
> > Fine with me, I don't mind either way. Thanks.
>
> Will you take this series for v5.18 ?
Yes.
--
Sakari Ailus
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-02-15 21:28 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-13 15:00 [PATCH 0/2] media: mc: Simplify media pipeline start/stop Laurent Pinchart
2022-01-13 15:00 ` [PATCH 1/2] media: media-entity: Add media_pad_is_streaming() helper function Laurent Pinchart
2022-01-13 15:00 ` [PATCH 2/2] media: media-entity: Simplify media_pipeline_start() Laurent Pinchart
2022-01-15 17:39 ` Sakari Ailus
2022-01-15 20:02 ` Laurent Pinchart
2022-02-11 21:27 ` Laurent Pinchart
2022-02-15 21:28 ` Sakari Ailus
2022-01-13 15:28 ` [PATCH 0/2] media: mc: Simplify media pipeline start/stop Laurent Pinchart
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.