media: rcar-vin: Allow independent VIN link enablement

RFC PATCH Superseded Steve Longerbeam 2 participants 4 messages Jan 6, 2019 – Jan 15, 2019

Subsystems (2)

Media (V4L/DVB) Arm/RISC-V/Renesas architecture

lore

Share & tools

lore

Steve Longerbeam

author

2019-01-06 21:20 UTC #0 lore raw

RFC PATCH v2 1/1 media: rcar-vin: Allow independent VIN link enablement

There is a block of code in rvin_group_link_notify() that loops through all entities in the media graph, and prevents enabling a link to a VIN node if any entity is in use. This prevents enabling a VIN link even if there is an in-use entity somewhere in the graph that is independent of the link's pipeline.

For example, the code will prevent enabling a link from the first rcar-csi2 receiver to a VIN node even if there is an enabled link somewhere far upstream on the second independent rcar-csi2 receiver pipeline.

If this code is meant to prevent modifying a link if any entity in the graph is actively involved in streaming (because modifying the CHSEL register fields can disrupt any/all running streams), then the entities stream counts should be checked rather than the use counts.

(There is already such a check in __media_entity_setup_link() that verifies the stream_count of the link's source and sink entities are both zero, but that is insufficient, since there should be no running streams in the entire graph).

Modify the media_device_for_each_entity() loop to check the entity stream_count instead of the use_count, and elaborate on the comment. VIN node links can now be enabled even if there are other independent in-use entities that are not streaming.

Fixes c0cc5aef31 ("media: rcar-vin: add link notify for Gen3")

Signed-off-by Steve Longerbeam <[email protected]>

---
Changes in v2:
- bring back the media_device_for_each_entity() loop but check the stream_count not the use_count.

---
 drivers/media/platform/rcar-vin/rcar-core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

drivers/media/platform/rcar-vin/rcar-core.c +6 -2

index f0719ce24b97..6dd6b11c1b2b 100644
@@ -131,9 +131,13 @@ static int rvin_group_link_notify(struct media_link *link, u32 flags,
 	    !is_media_entity_v4l2_video_device(link->sink->entity))
 		return 0;
 
-	/* If any entity is in use don't allow link changes. */
+	/*
+	 * Don't allow link changes if any entity in the graph is
+	 * streaming, because modifying the CHSEL register fields
+	 * can disrupt running streams.
+	 */
 	media_device_for_each_entity(entity, &group->mdev)
-		if (entity->use_count)
+		if (entity->stream_count)
 			return -EBUSY;
 
 	mutex_lock(&group->lock);

Signature · 1 line

2.17.1

Niklas Söderlund

Reviewed-by on v2 1/1

2019-01-09 22:40 UTC #1 lore raw

Hi Steve,

Thanks for your patch, I think it looks good.

Quoted 21 lines · Steve Longerbeam

There is a block of code in rvin_group_link_notify() that loops through all entities in the media graph, and prevents enabling a link to a VIN node if any entity is in use. This prevents enabling a VIN link even if there is an in-use entity somewhere in the graph that is independent of the link's pipeline.
For example, the code will prevent enabling a link from the first rcar-csi2 receiver to a VIN node even if there is an enabled link somewhere far upstream on the second independent rcar-csi2 receiver pipeline.
If this code is meant to prevent modifying a link if any entity in the graph is actively involved in streaming (because modifying the CHSEL register fields can disrupt any/all running streams), then the entities stream counts should be checked rather than the use counts.
(There is already such a check in __media_entity_setup_link() that verifies the stream_count of the link's source and sink entities are both zero, but that is insufficient, since there should be no running streams in the entire graph).

Quoted 8 lines

Modify the media_device_for_each_entity() loop to check the entity stream_count instead of the use_count, and elaborate on the comment. VIN node links can now be enabled even if there are other independent in-use entities that are not streaming.
Fixes c0cc5aef31 ("media: rcar-vin: add link notify for Gen3")
Signed-off-by Steve Longerbeam <[email protected]>

Reviewed-by Niklas Söderlund <[email protected]>

Quoted 31 lines

---
Changes in v2:
- bring back the media_device_for_each_entity() loop but check the stream_count not the use_count.

---
 drivers/media/platform/rcar-vin/rcar-core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

drivers/media/platform/rcar-vin/rcar-core.c +6 -2

index f0719ce24b97..6dd6b11c1b2b 100644
@@ -131,9 +131,13 @@ static int rvin_group_link_notify(struct media_link *link, u32 flags,
 	    !is_media_entity_v4l2_video_device(link->sink->entity))
 		return 0;
 
-	/* If any entity is in use don't allow link changes. */
+	/*
+	 * Don't allow link changes if any entity in the graph is
+	 * streaming, because modifying the CHSEL register fields
+	 * can disrupt running streams.
+	 */
 	media_device_for_each_entity(entity, &group->mdev)
-		if (entity->use_count)
+		if (entity->stream_count)
 			return -EBUSY;
 
 	mutex_lock(&group->lock);

Signature · 1 line

2.17.1

Signature · 2 lines

Regards,
Niklas Söderlund

Steve Longerbeam

author

2019-01-10 01:19 UTC #2 lore raw

Quoted 3 lines · Niklas Söderlund

Hi Steve,
Thanks for your patch, I think it looks good.

Thanks for the Ack! I'm not real familiar with the RFC patch process. Should this be submitted again with RFC stripped from the subject line?

Steve

Quoted 63 lines · Steve Longerbeam

Quoted 21 lines
There is a block of code in rvin_group_link_notify() that loops through all entities in the media graph, and prevents enabling a link to a VIN node if any entity is in use. This prevents enabling a VIN link even if there is an in-use entity somewhere in the graph that is independent of the link's pipeline.
For example, the code will prevent enabling a link from the first rcar-csi2 receiver to a VIN node even if there is an enabled link somewhere far upstream on the second independent rcar-csi2 receiver pipeline.
If this code is meant to prevent modifying a link if any entity in the graph is actively involved in streaming (because modifying the CHSEL register fields can disrupt any/all running streams), then the entities stream counts should be checked rather than the use counts.
(There is already such a check in __media_entity_setup_link() that verifies the stream_count of the link's source and sink entities are both zero, but that is insufficient, since there should be no running streams in the entire graph).
Quoted 8 lines
Modify the media_device_for_each_entity() loop to check the entity stream_count instead of the use_count, and elaborate on the comment. VIN node links can now be enabled even if there are other independent in-use entities that are not streaming.
Fixes c0cc5aef31 ("media: rcar-vin: add link notify for Gen3")
Signed-off-by Steve Longerbeam <[email protected]>
Reviewed-by Niklas Söderlund <[email protected]>
Quoted 31 lines
---
Changes in v2:
- bring back the media_device_for_each_entity() loop but check the stream_count not the use_count.
---
  drivers/media/platform/rcar-vin/rcar-core.c | 8 ++++++--
  1 file changed, 6 insertions(+), 2 deletions(-)
drivers/media/platform/rcar-vin/rcar-core.c +6 -2
index f0719ce24b97..6dd6b11c1b2b 100644
@@ -131,9 +131,13 @@ static int rvin_group_link_notify(struct media_link *link, u32 flags,
  	    !is_media_entity_v4l2_video_device(link->sink->entity))
  		return 0;
 
-	/* If any entity is in use don't allow link changes. */
+	/*
+	 * Don't allow link changes if any entity in the graph is
+	 * streaming, because modifying the CHSEL register fields
+	 * can disrupt running streams.
+	 */
  	media_device_for_each_entity(entity, &group->mdev)
-		if (entity->use_count)
+		if (entity->stream_count)
  			return -EBUSY;
 
  	mutex_lock(&group->lock);
Signature · 1 line
2.17.1

Steve Longerbeam

author

2019-01-15 01:03 UTC #3 lore raw

Quoted 8 lines · Steve Longerbeam

Quoted 3 lines · Niklas Söderlund
Hi Steve,
Thanks for your patch, I think it looks good.
Thanks for the Ack! I'm not real familiar with the RFC patch process. Should this be submitted again with RFC stripped from the subject line?

I've been told there isn't really a process for RFC patch submission. I will re-submit and strip off the RFC and add your Reviewed-by.

Steve

Quoted 65 lines

Quoted 64 lines · Steve Longerbeam
Quoted 21 lines
There is a block of code in rvin_group_link_notify() that loops through all entities in the media graph, and prevents enabling a link to a VIN node if any entity is in use. This prevents enabling a VIN link even if there is an in-use entity somewhere in the graph that is independent of the link's pipeline.
For example, the code will prevent enabling a link from the first rcar-csi2 receiver to a VIN node even if there is an enabled link somewhere far upstream on the second independent rcar-csi2 receiver pipeline.
If this code is meant to prevent modifying a link if any entity in the graph is actively involved in streaming (because modifying the CHSEL register fields can disrupt any/all running streams), then the entities stream counts should be checked rather than the use counts.
(There is already such a check in __media_entity_setup_link() that verifies the stream_count of the link's source and sink entities are both zero, but that is insufficient, since there should be no running streams in the entire graph).
Quoted 8 lines
Modify the media_device_for_each_entity() loop to check the entity stream_count instead of the use_count, and elaborate on the comment. VIN node links can now be enabled even if there are other independent in-use entities that are not streaming.
Fixes c0cc5aef31 ("media: rcar-vin: add link notify for Gen3")
Signed-off-by Steve Longerbeam <[email protected]>
Reviewed-by Niklas Söderlund <[email protected]>
Quoted 30 lines
---
Changes in v2:
- bring back the media_device_for_each_entity() loop but check the stream_count not the use_count.
---
drivers/media/platform/rcar-vin/rcar-core.c | 8 ++++++--
  1 file changed, 6 insertions(+), 2 deletions(-)
drivers/media/platform/rcar-vin/rcar-core.c +0 -0
index f0719ce24b97..6dd6b11c1b2b 100644
@@ -131,9 +131,13 @@ static int rvin_group_link_notify(struct 
 media_link *link, u32 flags,
 !is_media_entity_v4l2_video_device(link->sink->entity))
           return 0;
-    /* If any entity is in use don't allow link changes. */ +    /*
+     * Don't allow link changes if any entity in the graph is +     * streaming, because modifying the CHSEL register fields +     * can disrupt running streams.
+     */
      media_device_for_each_entity(entity, &group->mdev) -        if (entity->use_count)
+        if (entity->stream_count)
              return -EBUSY; mutex_lock(&group->lock);
Signature · 1 line
2.17.1