From: "Niklas Söderlund" <niklas.soderlund@ragnatech.se>
To: Steve Longerbeam <slongerbeam@gmail.com>
Cc: linux-media@vger.kernel.org,
Mauro Carvalho Chehab <mchehab@kernel.org>,
"open list:MEDIA DRIVERS FOR RENESAS - VIN"
<linux-renesas-soc@vger.kernel.org>,
open list <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH] media: rcar-vin: Allow independent VIN link enablement
Date: Thu, 27 Dec 2018 01:51:25 +0100 [thread overview]
Message-ID: <20181227005125.GK19796@bigcity.dyn.berto.se> (raw)
In-Reply-To: <20181225232725.15935-1-slongerbeam@gmail.com>
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
next prev parent reply other threads:[~2018-12-27 0:51 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2018-12-29 23:37 ` Steve Longerbeam
2019-01-05 0:48 ` Steve Longerbeam
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181227005125.GK19796@bigcity.dyn.berto.se \
--to=niklas.soderlund@ragnatech.se \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=slongerbeam@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).