* [RFC PATCH] media: rcar-vin: Allow independent VIN link enablement @ 2018-12-25 23:27 Steve Longerbeam 2018-12-27 0:51 ` Niklas Söderlund 0 siblings, 1 reply; 4+ messages in thread From: Steve Longerbeam @ 2018-12-25 23:27 UTC (permalink / raw) To: linux-media Cc: Steve Longerbeam, Niklas Söderlund, Mauro Carvalho Chehab, open list:MEDIA DRIVERS FOR RENESAS - VIN, open list There is a block of code in rvin_group_link_notify() that prevents enabling a link to a VIN node if any entity in the media graph 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 block 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 block is meant to prevent modifying a link if the link is actively involved in streaming, 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. Remove the code block so that VIN node links can be enabled even if there are other independent in-use entities. Fixes: c0cc5aef31 ("media: rcar-vin: add link notify for Gen3") Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> --- drivers/media/platform/rcar-vin/rcar-core.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c index f0719ce24b97..b2c9a876969e 100644 --- a/drivers/media/platform/rcar-vin/rcar-core.c +++ b/drivers/media/platform/rcar-vin/rcar-core.c @@ -116,7 +116,6 @@ static int rvin_group_link_notify(struct media_link *link, u32 flags, struct rvin_group, mdev); unsigned int master_id, channel, mask_new, i; unsigned int mask = ~0; - struct media_entity *entity; struct video_device *vdev; struct media_pad *csi_pad; struct rvin_dev *vin = NULL; @@ -131,11 +130,6 @@ 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. */ - media_device_for_each_entity(entity, &group->mdev) - if (entity->use_count) - return -EBUSY; - mutex_lock(&group->lock); /* Find the master VIN that controls the routes. */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] media: rcar-vin: Allow independent VIN link enablement 2018-12-25 23:27 [RFC PATCH] media: rcar-vin: Allow independent VIN link enablement Steve Longerbeam @ 2018-12-27 0:51 ` Niklas Söderlund 2018-12-29 23:37 ` Steve Longerbeam 0 siblings, 1 reply; 4+ messages in thread From: Niklas Söderlund @ 2018-12-27 0:51 UTC (permalink / raw) To: Steve Longerbeam Cc: linux-media, Mauro Carvalho Chehab, open list:MEDIA DRIVERS FOR RENESAS - VIN, open list Hi Steve, Thanks for your patch. On 2018-12-25 15:27:25 -0800, Steve Longerbeam wrote: > There is a block of code in rvin_group_link_notify() that prevents > enabling a link to a VIN node if any entity in the media graph 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 block 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. Unfortunately this is by design and needed due to the hardware design. The different VIN endpoints shares a configuration register which controls the routing from the CSI-2 receivers to the VIN (register name CHSEL). Modifying the CHSEL register which is what happens when a link is enabled/disabled can have side effects on running streams even if they are not shown to be dependent in the media graph. There is a CHSEL register in VIN0 which controls the routing from all CSI-2 receivers to VIN0-3 and a CHSEL register in VIN4 which controls the same for VIN4-7. > > If this code block is meant to prevent modifying a link if the > link is actively involved in streaming, 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. For the reason above the check in __media_entity_setup_link() is not enough :-( This register sharing is my least favorite thing about the VIN on Gen3 and forces the driver to become more complex as all VIN instances needs to know about each other and interact. > > Remove the code block so that VIN node links can be enabled even if > there are other independent in-use entities. There is room for some improvement in this area disregarding the odd hardware design. It *could* be allowed to change a link terminating in VIN4-7 even if there is a stream running for one or more in VIN0-3. I would be interested to test such a patch but to allow any link change which is allowed by __media_entity_setup_link() is unfortunately not possible, as I understand it. Maybe someone more clever then me can find ways to unlock even more then just the split between VIN0-3 and VIn4-7. In essence the CHSEL register can not be changed if it's involved in a running pipeline even if the end result would be that the running pipeline would look the same. This is possible as there are multiple CHSEL settings where the same source is connected to a specific VIN while other members of the "subgroup of VINs" (e.g. VIN0-3) is routed to something else for the two CHSEL settings. > > Fixes: c0cc5aef31 ("media: rcar-vin: add link notify for Gen3") > > Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> > --- > drivers/media/platform/rcar-vin/rcar-core.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > index f0719ce24b97..b2c9a876969e 100644 > --- a/drivers/media/platform/rcar-vin/rcar-core.c > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > @@ -116,7 +116,6 @@ static int rvin_group_link_notify(struct media_link *link, u32 flags, > struct rvin_group, mdev); > unsigned int master_id, channel, mask_new, i; > unsigned int mask = ~0; > - struct media_entity *entity; > struct video_device *vdev; > struct media_pad *csi_pad; > struct rvin_dev *vin = NULL; > @@ -131,11 +130,6 @@ 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. */ > - media_device_for_each_entity(entity, &group->mdev) > - if (entity->use_count) > - return -EBUSY; > - > mutex_lock(&group->lock); > > /* Find the master VIN that controls the routes. */ > -- > 2.17.1 > -- Regards, Niklas Söderlund ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] media: rcar-vin: Allow independent VIN link enablement 2018-12-27 0:51 ` Niklas Söderlund @ 2018-12-29 23:37 ` Steve Longerbeam 2019-01-05 0:48 ` Steve Longerbeam 0 siblings, 1 reply; 4+ messages in thread From: Steve Longerbeam @ 2018-12-29 23:37 UTC (permalink / raw) To: Niklas Söderlund Cc: linux-media, Mauro Carvalho Chehab, open list:MEDIA DRIVERS FOR RENESAS - VIN, open list Hi Niklas, On 12/26/18 4:51 PM, Niklas Söderlund wrote: > Hi Steve, > > Thanks for your patch. > > On 2018-12-25 15:27:25 -0800, Steve Longerbeam wrote: >> There is a block of code in rvin_group_link_notify() that prevents >> enabling a link to a VIN node if any entity in the media graph 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 block 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. > Unfortunately this is by design and needed due to the hardware design. > The different VIN endpoints shares a configuration register which > controls the routing from the CSI-2 receivers to the VIN (register name > CHSEL). Modifying the CHSEL register which is what happens when a link > is enabled/disabled can have side effects on running streams even if > they are not shown to be dependent in the media graph. Ok, understood, modifying CHSEL register can adversely affect running streams. > > There is a CHSEL register in VIN0 which controls the routing from all > CSI-2 receivers to VIN0-3 and a CHSEL register in VIN4 which controls > the same for VIN4-7. > >> If this code block is meant to prevent modifying a link if the >> link is actively involved in streaming, 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. > For the reason above the check in __media_entity_setup_link() is not > enough :-( This register sharing is my least favorite thing about the > VIN on Gen3 and forces the driver to become more complex as all VIN > instances needs to know about each other and interact. > Given above I understand why the stream count checks in __media_entity_setup_link() are insufficient, because only the requested link's source stream count is checked, and not the other CSI-2 receiver for example. But why check the use counts of every entity upstream from the VIN sources? Why not check only the VIN source entities stream counts (both CSI-2 receivers and/or parallel devices), and ignore entities upstream from those? And why are the use counts checked, it seems it should be the stream counts that should be checked. >> Remove the code block so that VIN node links can be enabled even if >> there are other independent in-use entities. > There is room for some improvement in this area disregarding the odd > hardware design. It *could* be allowed to change a link terminating in > VIN4-7 even if there is a stream running for one or more in VIN0-3. > > I would be interested to test such a patch but to allow any link change > which is allowed by __media_entity_setup_link() is unfortunately not > possible, as I understand it. Maybe someone more clever then me can find > ways to unlock even more then just the split between VIN0-3 and VIn4-7. > > In essence the CHSEL register can not be changed if it's involved in a > running pipeline even if the end result would be that the running > pipeline would look the same. This is possible as there are multiple > CHSEL settings where the same source is connected to a specific VIN > while other members of the "subgroup of VINs" (e.g. VIN0-3) is routed to > something else for the two CHSEL settings. Right, so rvin_group_link_notify() determines whether the requested VIN link enable will result in a valid set of CSI2->VIN links for the given hardware, using the CHSEL bitmask tables. Which is why it seems it is the stream counts that should be checked as mentioned above, rather than the use counts, because the CHSEL bitmask checks are validating the set of enabled links, and the only remaining checks are to verify no streams are running on either CSI-2 receiver. Steve >> Fixes: c0cc5aef31 ("media: rcar-vin: add link notify for Gen3") >> >> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> >> --- >> drivers/media/platform/rcar-vin/rcar-core.c | 6 ------ >> 1 file changed, 6 deletions(-) >> >> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c >> index f0719ce24b97..b2c9a876969e 100644 >> --- a/drivers/media/platform/rcar-vin/rcar-core.c >> +++ b/drivers/media/platform/rcar-vin/rcar-core.c >> @@ -116,7 +116,6 @@ static int rvin_group_link_notify(struct media_link *link, u32 flags, >> struct rvin_group, mdev); >> unsigned int master_id, channel, mask_new, i; >> unsigned int mask = ~0; >> - struct media_entity *entity; >> struct video_device *vdev; >> struct media_pad *csi_pad; >> struct rvin_dev *vin = NULL; >> @@ -131,11 +130,6 @@ 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. */ >> - media_device_for_each_entity(entity, &group->mdev) >> - if (entity->use_count) >> - return -EBUSY; >> - >> mutex_lock(&group->lock); >> >> /* Find the master VIN that controls the routes. */ >> -- >> 2.17.1 >> ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] media: rcar-vin: Allow independent VIN link enablement 2018-12-29 23:37 ` Steve Longerbeam @ 2019-01-05 0:48 ` Steve Longerbeam 0 siblings, 0 replies; 4+ messages in thread From: Steve Longerbeam @ 2019-01-05 0:48 UTC (permalink / raw) To: Steve Longerbeam, Niklas Söderlund Cc: linux-media, Mauro Carvalho Chehab, open list:MEDIA DRIVERS FOR RENESAS - VIN, open list Hi Niklas, How about a patch that simply replaces the entity use_count check with a stream_count check? As in: diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c index f0719ce24b97..aef8d8dab6ab 100644 --- a/drivers/media/platform/rcar-vin/rcar-core.c +++ b/drivers/media/platform/rcar-vin/rcar-core.c @@ -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, 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); And that might be overkilll, maybe only the stream_count's of the VIN entities need to be checked. Steve On 12/29/18 3:37 PM, Steve Longerbeam wrote: > Hi Niklas, > > On 12/26/18 4:51 PM, Niklas Söderlund wrote: >> Hi Steve, >> >> Thanks for your patch. >> >> On 2018-12-25 15:27:25 -0800, Steve Longerbeam wrote: >>> There is a block of code in rvin_group_link_notify() that prevents >>> enabling a link to a VIN node if any entity in the media graph 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 block 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. >> Unfortunately this is by design and needed due to the hardware design. >> The different VIN endpoints shares a configuration register which >> controls the routing from the CSI-2 receivers to the VIN (register name >> CHSEL). Modifying the CHSEL register which is what happens when a link >> is enabled/disabled can have side effects on running streams even if >> they are not shown to be dependent in the media graph. > > Ok, understood, modifying CHSEL register can adversely affect running > streams. > >> >> There is a CHSEL register in VIN0 which controls the routing from all >> CSI-2 receivers to VIN0-3 and a CHSEL register in VIN4 which controls >> the same for VIN4-7. >> >>> If this code block is meant to prevent modifying a link if the >>> link is actively involved in streaming, 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. >> For the reason above the check in __media_entity_setup_link() is not >> enough :-( This register sharing is my least favorite thing about the >> VIN on Gen3 and forces the driver to become more complex as all VIN >> instances needs to know about each other and interact. >> > > Given above I understand why the stream count checks in > __media_entity_setup_link() are insufficient, because only the > requested link's source stream count is checked, and not the other > CSI-2 receiver for example. > > But why check the use counts of every entity upstream from the VIN > sources? Why not check only the VIN source entities stream counts > (both CSI-2 receivers and/or parallel devices), and ignore entities > upstream from those? > > And why are the use counts checked, it seems it should be the stream > counts that should be checked. > >>> Remove the code block so that VIN node links can be enabled even if >>> there are other independent in-use entities. >> There is room for some improvement in this area disregarding the odd >> hardware design. It *could* be allowed to change a link terminating in >> VIN4-7 even if there is a stream running for one or more in VIN0-3. >> >> I would be interested to test such a patch but to allow any link change >> which is allowed by __media_entity_setup_link() is unfortunately not >> possible, as I understand it. Maybe someone more clever then me can find >> ways to unlock even more then just the split between VIN0-3 and VIn4-7. >> >> In essence the CHSEL register can not be changed if it's involved in a >> running pipeline even if the end result would be that the running >> pipeline would look the same. This is possible as there are multiple >> CHSEL settings where the same source is connected to a specific VIN >> while other members of the "subgroup of VINs" (e.g. VIN0-3) is routed to >> something else for the two CHSEL settings. > > Right, so rvin_group_link_notify() determines whether the requested > VIN link enable will result in a valid set of CSI2->VIN links for the > given hardware, using the CHSEL bitmask tables. Which is why it seems > it is the stream counts that should be checked as mentioned above, > rather than the use counts, because the CHSEL bitmask checks are > validating the set of enabled links, and the only remaining checks are > to verify no streams are running on either CSI-2 receiver. > > Steve > > >>> Fixes: c0cc5aef31 ("media: rcar-vin: add link notify for Gen3") >>> >>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> >>> --- >>> drivers/media/platform/rcar-vin/rcar-core.c | 6 ------ >>> 1 file changed, 6 deletions(-) >>> >>> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c >>> b/drivers/media/platform/rcar-vin/rcar-core.c >>> index f0719ce24b97..b2c9a876969e 100644 >>> --- a/drivers/media/platform/rcar-vin/rcar-core.c >>> +++ b/drivers/media/platform/rcar-vin/rcar-core.c >>> @@ -116,7 +116,6 @@ static int rvin_group_link_notify(struct >>> media_link *link, u32 flags, >>> struct rvin_group, mdev); >>> unsigned int master_id, channel, mask_new, i; >>> unsigned int mask = ~0; >>> - struct media_entity *entity; >>> struct video_device *vdev; >>> struct media_pad *csi_pad; >>> struct rvin_dev *vin = NULL; >>> @@ -131,11 +130,6 @@ 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. */ >>> - media_device_for_each_entity(entity, &group->mdev) >>> - if (entity->use_count) >>> - return -EBUSY; >>> - >>> mutex_lock(&group->lock); >>> /* Find the master VIN that controls the routes. */ >>> -- >>> 2.17.1 >>> > ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-01-05 0:49 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-25 23:27 [RFC PATCH] media: rcar-vin: Allow independent VIN link enablement Steve Longerbeam 2018-12-27 0:51 ` Niklas Söderlund 2018-12-29 23:37 ` Steve Longerbeam 2019-01-05 0:48 ` Steve Longerbeam
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).