Message ID | 20210303180817.12285-2-andrey.konovalov@linaro.org (mailing list archive) |
---|---|
State | RFC, archived |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1lHbvS-000Pbs-QC; Thu, 04 Mar 2021 00:33:11 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1355650AbhCDAcR (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Wed, 3 Mar 2021 19:32:17 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53840 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S243233AbhCCSJl (ORCPT <rfc822;linux-media@vger.kernel.org>); Wed, 3 Mar 2021 13:09:41 -0500 Received: from mail-lf1-x12f.google.com (mail-lf1-x12f.google.com [IPv6:2a00:1450:4864:20::12f]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 120D8C06175F for <linux-media@vger.kernel.org>; Wed, 3 Mar 2021 10:08:54 -0800 (PST) Received: by mail-lf1-x12f.google.com with SMTP id v5so38563918lft.13 for <linux-media@vger.kernel.org>; Wed, 03 Mar 2021 10:08:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=bSsDnS6dbLhvTy+AVlfdF3LogW8QXbBSHC6yrMiBxcc=; b=eubOFCkwtk73peY4EPbEunGR/uk+5JCUWi3g2QGv85tqPZ3PNrVG8gXXANyHv0NmcI K8W/I2nmnxLYCZwal7WUloHZciQ0TZBa+YkM51io8pPygVXfN4iEHPbEIkcloO1M+QdE yp6KZrQ8JJvCWbb+a/iyFgVAQ5h/ZeAAnLpo4Lr8JwiQMJXyqU6ps1S4lYXkoo4SDcbY SU3lDszVeY8JmB96TjpUU9ViKu8ZYRCHJboG9K7JKj5cw1kPbKWkxp/1BL+UUZxD4Mbw w6jJiBjzRM4lnYSLrC05WxSwhXiUNQsQXGBfnFG5Qmgba/+sWnheE9s8JOl+zj6CcTzV sB2g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=bSsDnS6dbLhvTy+AVlfdF3LogW8QXbBSHC6yrMiBxcc=; b=Zom6fkbTeYr4bhRrJCyPkzgqIyBsf79qXOndmhxg2Rj/xqP2nGBoDrY4/8r96tX/kn bUvbM8MF03EIcbGNm28r/aSDsQWsY6WsiLbDNCssxc7I5zp4kfsc7OcFLRRA7uYTx6yp jZn3NRQWcj1DpJNM0JRSo+pOYOvQyFO5QnzTUhCIFhzl9OEaTjkGRbcBB8EM3i77/HZM h54XTt+xhTGWhD+QSRSnWFuzleO8CexCIwl+ytDZ/u82aiqVLU57EH1VrYeePO3Z28op 0izSY0P1deEqcOFmdIFbl7qIQKQMDzq42FbKB+gM6ruJ6qRuHuM/mC3sEx+tPRpGbBRV Q6RA== X-Gm-Message-State: AOAM532tf4YSd4J4VfInlM36oTbRrLdIcgej7vysnEON5NZP8QWmi1Lc sQaQumb4lC50fNsTqBrH4lodhQ== X-Google-Smtp-Source: ABdhPJy0ovoxjIHjD8PCduT0ojbEEFGT4iXC21oduORhhszKoXbyOpWTh6E4hh/eMLxk+A997Kk40Q== X-Received: by 2002:a05:6512:2208:: with SMTP id h8mr16310190lfu.636.1614794932537; Wed, 03 Mar 2021 10:08:52 -0800 (PST) Received: from localhost.localdomain ([85.249.43.69]) by smtp.googlemail.com with ESMTPSA id j15sm2220990lfm.138.2021.03.03.10.08.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 03 Mar 2021 10:08:51 -0800 (PST) From: Andrey Konovalov <andrey.konovalov@linaro.org> To: sakari.ailus@linux.intel.com, linux-media@vger.kernel.org, laurent.pinchart@ideasonboard.com Cc: mchehab@kernel.org, niklas.soderlund@ragnatech.se, bparrot@ti.com, mickael.guene@st.com, Andrey Konovalov <andrey.konovalov@linaro.org> Subject: [RFC PATCH 1/4] media: rcar-vin: use v4l2_get_link_freq() to calculate phypll frequency Date: Wed, 3 Mar 2021 21:08:14 +0300 Message-Id: <20210303180817.12285-2-andrey.konovalov@linaro.org> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20210303180817.12285-1-andrey.konovalov@linaro.org> References: <20210303180817.12285-1-andrey.konovalov@linaro.org> Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -2.5 (--) X-LSpam-Report: No, score=-2.5 required=5.0 tests=BAYES_00=-1.9,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1 autolearn=ham autolearn_force=no |
Series |
use v4l2_get_link_freq() in CSI receiver drivers
|
|
Commit Message
Andrey Konovalov
March 3, 2021, 6:08 p.m. UTC
To get the link frequency value, or to calculate a parameter depending on
it the receiver driver should use V4L2_CID_LINK_FREQ. If V4L2_CID_LINK_FREQ
control is not implemented in the remote subdevice, the link frequency
can be calculated from V4L2_CID_PIXEL_RATE control value. But the latter
may not give the correct link frequency, and should only be used as the
last resort. v4l2_get_link_freq() does exactly that, so use it instead
of reading V4L2_CID_PIXEL_RATE directly.
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
drivers/media/platform/rcar-vin/rcar-csi2.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
Comments
Hi Andrey, Thanks for your patch. On 2021-03-03 21:08:14 +0300, Andrey Konovalov wrote: > To get the link frequency value, or to calculate a parameter depending on > it the receiver driver should use V4L2_CID_LINK_FREQ. If V4L2_CID_LINK_FREQ > control is not implemented in the remote subdevice, the link frequency > can be calculated from V4L2_CID_PIXEL_RATE control value. But the latter > may not give the correct link frequency, and should only be used as the > last resort. v4l2_get_link_freq() does exactly that, so use it instead > of reading V4L2_CID_PIXEL_RATE directly. I like the direction this patch is taking, but I'm a bit concerned about that V4L2_CID_LINK_FREQ is not able to replace V4L2_CID_PIXEL_RATE as it is designed today. Maybe my concern is unfounded and only reflects my own misunderstanding of the API. When I wrote this code I tried to first do it using V4L2_CID_LINK_FREQ but I found no way to be able to express the wide rang of values needed for my use-case given that V4L2_CID_LINK_FREQ is a menu control. I had to use V4L2_CID_PIXEL_RATE as it allowed me to at runtime calculate and report the link speed based on input formats. The Use-cases I need to address are where CSI-2 transmitter themself are a bridge in the video pipeline, for example * Case 1 - HDMI video source HDMI source -> ADV748x (HDMI-to-CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver The R-Car CSI-2 receiver needs to know the CSI-2 link frequency and queries the ADV748x using V4L2_CID_PIXEL_RATE. The ADV748x reports the pixel rate based on the HDMI format detected on its sink pad. This could be done using V4L2_CID_LINK_FREQ, but as it's a menu control it becomes rather tricky to populate it with all possible values, but I guess it could be doable? * Case 2 - Multiple video streams over a CSI-2 bus (GMSL) Camera 1 -| Camera 2 -| Camera 3 -|---> MAX9286 (GMSL-to CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver Camera 4 -| The MAX9286 has 4 sink pads each connected to an independent camera and a single CSI-2 source pad. When streaming starts the MAX9286 computes the total CSI-2 link speed as V4L2_CID_PIXEL_RATE based on the format on each of it's 4 sink pads. As in case 1 this could be reported by V4L2_CID_LINK_FREQ but I don't see it as feasible to populate the menu control with all possible frequencies before hand. Hopefully this is all easily solvable and I have only misunderstood how menu controls work. If not I think this needs to be considered as part of this series as otherwise it could leave the CSI-2 bridge drivers without a path forward. > > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> I tested this and it works as expected. Also as expected it prints lots of warnings about the usage of V4L2_CID_PIXEL_RATE :-) Once I understand how I can fix the CSI-2 transmitters used as bridges in the R-Car boards I will be happy to add my tag to this series as well as fix the bridge drivers. > --- > drivers/media/platform/rcar-vin/rcar-csi2.c | 18 +++++++----------- > 1 file changed, 7 insertions(+), 11 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c > index e06cd512aba2..eec8f9dd9060 100644 > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > @@ -455,29 +455,25 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp, > unsigned int lanes) > { > struct v4l2_subdev *source; > - struct v4l2_ctrl *ctrl; > - u64 mbps; > + s64 mbps; > > if (!priv->remote) > return -ENODEV; > > source = priv->remote; > > - /* Read the pixel rate control from remote. */ > - ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE); > - if (!ctrl) { > - dev_err(priv->dev, "no pixel rate control in subdev %s\n", > + /* Read the link frequency from the remote subdev. */ > + mbps = v4l2_get_link_freq(source->ctrl_handler, bpp, 2 * lanes); > + if (mbps < 0) { > + dev_err(priv->dev, "failed to get link rate from subdev %s\n", > source->name); > - return -EINVAL; > + return mbps; > } > - > /* > * Calculate the phypll in mbps. > - * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes) > * bps = link_freq * 2 > */ > - mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; > - do_div(mbps, lanes * 1000000); > + do_div(mbps, 1000000 / 2); > > return mbps; > } > -- > 2.17.1 >
Hi Niklas, On Mon, Mar 08, 2021 at 02:46:52PM +0100, Niklas Söderlund wrote: > Hi Andrey, > > Thanks for your patch. > > On 2021-03-03 21:08:14 +0300, Andrey Konovalov wrote: > > To get the link frequency value, or to calculate a parameter depending on > > it the receiver driver should use V4L2_CID_LINK_FREQ. If V4L2_CID_LINK_FREQ > > control is not implemented in the remote subdevice, the link frequency > > can be calculated from V4L2_CID_PIXEL_RATE control value. But the latter > > may not give the correct link frequency, and should only be used as the > > last resort. v4l2_get_link_freq() does exactly that, so use it instead > > of reading V4L2_CID_PIXEL_RATE directly. > > I like the direction this patch is taking, but I'm a bit concerned about > that V4L2_CID_LINK_FREQ is not able to replace V4L2_CID_PIXEL_RATE as it > is designed today. Maybe my concern is unfounded and only reflects my > own misunderstanding of the API. > > When I wrote this code I tried to first do it using V4L2_CID_LINK_FREQ > but I found no way to be able to express the wide rang of values needed > for my use-case given that V4L2_CID_LINK_FREQ is a menu control. I had I think we could make it alternatively a 64-bit integer control if that helps. The helper needs to be adjusted accordingly. > to use V4L2_CID_PIXEL_RATE as it allowed me to at runtime calculate and > report the link speed based on input formats. The Use-cases I need to > address are where CSI-2 transmitter themself are a bridge in the video > pipeline, for example Is the actual bus frequency changed based on this? Depending on the system where this chip is being used, only certain frequencies may be allowed on that bus. It would be most straightforward to use only those, but on the other hand, if any frequency can be used and that is certain, then I have no objections to allowing that either. We simply would make the link-frequencies property optional. > > * Case 1 - HDMI video source > > HDMI source -> ADV748x (HDMI-to-CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver > > The R-Car CSI-2 receiver needs to know the CSI-2 link frequency and > queries the ADV748x using V4L2_CID_PIXEL_RATE. The ADV748x reports the > pixel rate based on the HDMI format detected on its sink pad. > > This could be done using V4L2_CID_LINK_FREQ, but as it's a menu control > it becomes rather tricky to populate it with all possible values, but I > guess it could be doable? > > * Case 2 - Multiple video streams over a CSI-2 bus (GMSL) > > Camera 1 -| > Camera 2 -| > Camera 3 -|---> MAX9286 (GMSL-to CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver > Camera 4 -| > > The MAX9286 has 4 sink pads each connected to an independent camera and > a single CSI-2 source pad. When streaming starts the MAX9286 computes > the total CSI-2 link speed as V4L2_CID_PIXEL_RATE based on the format on > each of it's 4 sink pads. > > As in case 1 this could be reported by V4L2_CID_LINK_FREQ but I don't > see it as feasible to populate the menu control with all possible > frequencies before hand. > > Hopefully this is all easily solvable and I have only misunderstood how > menu controls work. If not I think this needs to be considered as part > of this series as otherwise it could leave the CSI-2 bridge drivers > without a path forward. > > > > > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> > > I tested this and it works as expected. Also as expected it prints lots > of warnings about the usage of V4L2_CID_PIXEL_RATE :-) Once I understand > how I can fix the CSI-2 transmitters used as bridges in the R-Car boards > I will be happy to add my tag to this series as well as fix the bridge > drivers. > > > --- > > drivers/media/platform/rcar-vin/rcar-csi2.c | 18 +++++++----------- > > 1 file changed, 7 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c > > index e06cd512aba2..eec8f9dd9060 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > > @@ -455,29 +455,25 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp, > > unsigned int lanes) > > { > > struct v4l2_subdev *source; > > - struct v4l2_ctrl *ctrl; > > - u64 mbps; > > + s64 mbps; > > > > if (!priv->remote) > > return -ENODEV; > > > > source = priv->remote; > > > > - /* Read the pixel rate control from remote. */ > > - ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE); > > - if (!ctrl) { > > - dev_err(priv->dev, "no pixel rate control in subdev %s\n", > > + /* Read the link frequency from the remote subdev. */ > > + mbps = v4l2_get_link_freq(source->ctrl_handler, bpp, 2 * lanes); > > + if (mbps < 0) { > > + dev_err(priv->dev, "failed to get link rate from subdev %s\n", > > source->name); > > - return -EINVAL; > > + return mbps; > > } > > - > > /* > > * Calculate the phypll in mbps. > > - * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes) > > * bps = link_freq * 2 > > */ > > - mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; > > - do_div(mbps, lanes * 1000000); > > + do_div(mbps, 1000000 / 2); > > > > return mbps; > > } > > -- > > 2.17.1 > >
Hi Sakari, On 2021-03-23 15:10:41 +0200, Sakari Ailus wrote: > Hi Niklas, > > On Mon, Mar 08, 2021 at 02:46:52PM +0100, Niklas Söderlund wrote: > > Hi Andrey, > > > > Thanks for your patch. > > > > On 2021-03-03 21:08:14 +0300, Andrey Konovalov wrote: > > > To get the link frequency value, or to calculate a parameter depending on > > > it the receiver driver should use V4L2_CID_LINK_FREQ. If V4L2_CID_LINK_FREQ > > > control is not implemented in the remote subdevice, the link frequency > > > can be calculated from V4L2_CID_PIXEL_RATE control value. But the latter > > > may not give the correct link frequency, and should only be used as the > > > last resort. v4l2_get_link_freq() does exactly that, so use it instead > > > of reading V4L2_CID_PIXEL_RATE directly. > > > > I like the direction this patch is taking, but I'm a bit concerned about > > that V4L2_CID_LINK_FREQ is not able to replace V4L2_CID_PIXEL_RATE as it > > is designed today. Maybe my concern is unfounded and only reflects my > > own misunderstanding of the API. > > > > When I wrote this code I tried to first do it using V4L2_CID_LINK_FREQ > > but I found no way to be able to express the wide rang of values needed > > for my use-case given that V4L2_CID_LINK_FREQ is a menu control. I had > > I think we could make it alternatively a 64-bit integer control if that > helps. The helper needs to be adjusted accordingly. That would solve my concern. > > > to use V4L2_CID_PIXEL_RATE as it allowed me to at runtime calculate and > > report the link speed based on input formats. The Use-cases I need to > > address are where CSI-2 transmitter themself are a bridge in the video > > pipeline, for example > > Is the actual bus frequency changed based on this? Yes > > Depending on the system where this chip is being used, only certain > frequencies may be allowed on that bus. It would be most straightforward to > use only those, but on the other hand, if any frequency can be used and > that is certain, then I have no objections to allowing that either. We > simply would make the link-frequencies property optional. The transmitter is a ADV748x and depending on the video input source (HDMI or CVBS) the output frequency changes. Failing to negotiate this of course results in the CSI-2 receiver never detecting LP-11. > > > > > * Case 1 - HDMI video source > > > > HDMI source -> ADV748x (HDMI-to-CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver > > > > The R-Car CSI-2 receiver needs to know the CSI-2 link frequency and > > queries the ADV748x using V4L2_CID_PIXEL_RATE. The ADV748x reports the > > pixel rate based on the HDMI format detected on its sink pad. > > > > This could be done using V4L2_CID_LINK_FREQ, but as it's a menu control > > it becomes rather tricky to populate it with all possible values, but I > > guess it could be doable? > > > > * Case 2 - Multiple video streams over a CSI-2 bus (GMSL) > > > > Camera 1 -| > > Camera 2 -| > > Camera 3 -|---> MAX9286 (GMSL-to CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver > > Camera 4 -| > > > > The MAX9286 has 4 sink pads each connected to an independent camera and > > a single CSI-2 source pad. When streaming starts the MAX9286 computes > > the total CSI-2 link speed as V4L2_CID_PIXEL_RATE based on the format on > > each of it's 4 sink pads. > > > > As in case 1 this could be reported by V4L2_CID_LINK_FREQ but I don't > > see it as feasible to populate the menu control with all possible > > frequencies before hand. > > > > Hopefully this is all easily solvable and I have only misunderstood how > > menu controls work. If not I think this needs to be considered as part > > of this series as otherwise it could leave the CSI-2 bridge drivers > > without a path forward. > > > > > > > > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> > > > > I tested this and it works as expected. Also as expected it prints lots > > of warnings about the usage of V4L2_CID_PIXEL_RATE :-) Once I understand > > how I can fix the CSI-2 transmitters used as bridges in the R-Car boards > > I will be happy to add my tag to this series as well as fix the bridge > > drivers. > > > > > --- > > > drivers/media/platform/rcar-vin/rcar-csi2.c | 18 +++++++----------- > > > 1 file changed, 7 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c > > > index e06cd512aba2..eec8f9dd9060 100644 > > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c > > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > > > @@ -455,29 +455,25 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp, > > > unsigned int lanes) > > > { > > > struct v4l2_subdev *source; > > > - struct v4l2_ctrl *ctrl; > > > - u64 mbps; > > > + s64 mbps; > > > > > > if (!priv->remote) > > > return -ENODEV; > > > > > > source = priv->remote; > > > > > > - /* Read the pixel rate control from remote. */ > > > - ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE); > > > - if (!ctrl) { > > > - dev_err(priv->dev, "no pixel rate control in subdev %s\n", > > > + /* Read the link frequency from the remote subdev. */ > > > + mbps = v4l2_get_link_freq(source->ctrl_handler, bpp, 2 * lanes); > > > + if (mbps < 0) { > > > + dev_err(priv->dev, "failed to get link rate from subdev %s\n", > > > source->name); > > > - return -EINVAL; > > > + return mbps; > > > } > > > - > > > /* > > > * Calculate the phypll in mbps. > > > - * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes) > > > * bps = link_freq * 2 > > > */ > > > - mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; > > > - do_div(mbps, lanes * 1000000); > > > + do_div(mbps, 1000000 / 2); > > > > > > return mbps; > > > } > > > -- > > > 2.17.1 > > > > > -- > Kind regards, > > Sakari Ailus
Hi Niklas, On Tue, Mar 23, 2021 at 02:57:05PM +0100, Niklas Söderlund wrote: > On 2021-03-23 15:10:41 +0200, Sakari Ailus wrote: > > On Mon, Mar 08, 2021 at 02:46:52PM +0100, Niklas Söderlund wrote: > > > On 2021-03-03 21:08:14 +0300, Andrey Konovalov wrote: > > > > To get the link frequency value, or to calculate a parameter depending on > > > > it the receiver driver should use V4L2_CID_LINK_FREQ. If V4L2_CID_LINK_FREQ > > > > control is not implemented in the remote subdevice, the link frequency > > > > can be calculated from V4L2_CID_PIXEL_RATE control value. But the latter > > > > may not give the correct link frequency, and should only be used as the > > > > last resort. v4l2_get_link_freq() does exactly that, so use it instead > > > > of reading V4L2_CID_PIXEL_RATE directly. > > > > > > I like the direction this patch is taking, but I'm a bit concerned about > > > that V4L2_CID_LINK_FREQ is not able to replace V4L2_CID_PIXEL_RATE as it > > > is designed today. Maybe my concern is unfounded and only reflects my > > > own misunderstanding of the API. > > > > > > When I wrote this code I tried to first do it using V4L2_CID_LINK_FREQ > > > but I found no way to be able to express the wide rang of values needed > > > for my use-case given that V4L2_CID_LINK_FREQ is a menu control. I had > > > > I think we could make it alternatively a 64-bit integer control if that > > helps. The helper needs to be adjusted accordingly. > > That would solve my concern. > > > > to use V4L2_CID_PIXEL_RATE as it allowed me to at runtime calculate and > > > report the link speed based on input formats. The Use-cases I need to > > > address are where CSI-2 transmitter themself are a bridge in the video > > > pipeline, for example > > > > Is the actual bus frequency changed based on this? > > Yes > > > Depending on the system where this chip is being used, only certain > > frequencies may be allowed on that bus. It would be most straightforward to > > use only those, but on the other hand, if any frequency can be used and > > that is certain, then I have no objections to allowing that either. We > > simply would make the link-frequencies property optional. > > The transmitter is a ADV748x and depending on the video input source > (HDMI or CVBS) the output frequency changes. Failing to negotiate this > of course results in the CSI-2 receiver never detecting LP-11. > > > > * Case 1 - HDMI video source > > > > > > HDMI source -> ADV748x (HDMI-to-CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver > > > > > > The R-Car CSI-2 receiver needs to know the CSI-2 link frequency and > > > queries the ADV748x using V4L2_CID_PIXEL_RATE. The ADV748x reports the > > > pixel rate based on the HDMI format detected on its sink pad. > > > > > > This could be done using V4L2_CID_LINK_FREQ, but as it's a menu control > > > it becomes rather tricky to populate it with all possible values, but I > > > guess it could be doable? There are, generally speaking, two different uses for this information on the receiver side. We need to configure the DPHY timings that depend on the link frequency, and we need to configure the functional clock of the receiver and downstream IP cores to ensure they have enough bandwidth to absorb all pixels. Those are two fundamentally different issues: - The DPHY timings depend on the link frequency, which is a well-defined physical concept. We currently compute it from the pixel rate, which is a more loosely defined concept (see below). Assuming the V4L2_CID_LINK_FREQ control can be made to report the actual link frequency (and given that this is the control's purpose, there's no other option than making it work, otherwise the control would be entirely pointless), possibly by turning it into an INT64 control, then that's the right control to use for this purpose. - The functional clock of the video pipeline need to be able to absord the incoming pixels. If the clock is configurable, it means that it differs from the CSI-2 receiver clock (derived from the bus), which normally implies a FIFO between the CSI-2 receiver and the downstream blocks. The main constraint is that the FIFO shouldn't overflow, which in practice means that the effective average pixel rate per line on the input needs to be smaller or equal than on the output. This however doesn't mean that the input clock needs to be higher than the output clock, given that not only input and output bus widths can be different, but horizontal blanking can also be used to perform timing adjustements. For instance, if the input video stream has 1000 active and 3000 blanking pixels per line, assuming identical bus widths on the input and output side of the FIFO, we could have an output clock frequency equal to half of the input clock frequency, as long as the FIFO depth is at least 500 pixels. The output side would have a horizontal blanking of 1000 pixels. The same applies on the transmitter side, as there's often a FIFO between the pixel source (the pixel array for a sensor for instance, with a sampling clock rate) and the CSI-2 transmitter (running at the bus rate). The pixel rate is thus a much more fuzzy concept, isn't well-defined in V4L2, and can lead to all kind of interoperability issues. It should only be used along with blanking information, in order to perform rate adaptation calculations. > > > * Case 2 - Multiple video streams over a CSI-2 bus (GMSL) > > > > > > Camera 1 -| > > > Camera 2 -| > > > Camera 3 -|---> MAX9286 (GMSL-to CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver > > > Camera 4 -| > > > > > > The MAX9286 has 4 sink pads each connected to an independent camera and > > > a single CSI-2 source pad. When streaming starts the MAX9286 computes > > > the total CSI-2 link speed as V4L2_CID_PIXEL_RATE based on the format on > > > each of it's 4 sink pads. > > > > > > As in case 1 this could be reported by V4L2_CID_LINK_FREQ but I don't > > > see it as feasible to populate the menu control with all possible > > > frequencies before hand. As explained above, the CSI-2 frequency doesn't have to match the pixel rate of the sensor(s). I haven't checked exactly how the MAX9286 handles clock domains, but in general there source rate and the bus rate can be different. That's why the link frequency is often a menu control with a limited set of values (carefully selected by the system designer to accommodate EMC constraints), while the source rate can vary more freely. As long as the link frequency provides enough bandwidth, it doesn't have to be tightly coupled with the pixel rate. For source devices that have a single clock domain, and adjust the link frequency to follow the source rate, then turning the link frequency control into an INT64 would make sense. > > > Hopefully this is all easily solvable and I have only misunderstood how > > > menu controls work. If not I think this needs to be considered as part > > > of this series as otherwise it could leave the CSI-2 bridge drivers > > > without a path forward. > > > > > > > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> > > > > > > I tested this and it works as expected. Also as expected it prints lots > > > of warnings about the usage of V4L2_CID_PIXEL_RATE :-) Once I understand > > > how I can fix the CSI-2 transmitters used as bridges in the R-Car boards > > > I will be happy to add my tag to this series as well as fix the bridge > > > drivers. > > > > > > > --- > > > > drivers/media/platform/rcar-vin/rcar-csi2.c | 18 +++++++----------- > > > > 1 file changed, 7 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c > > > > index e06cd512aba2..eec8f9dd9060 100644 > > > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c > > > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > > > > @@ -455,29 +455,25 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp, > > > > unsigned int lanes) > > > > { > > > > struct v4l2_subdev *source; > > > > - struct v4l2_ctrl *ctrl; > > > > - u64 mbps; > > > > + s64 mbps; > > > > > > > > if (!priv->remote) > > > > return -ENODEV; > > > > > > > > source = priv->remote; > > > > > > > > - /* Read the pixel rate control from remote. */ > > > > - ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE); > > > > - if (!ctrl) { > > > > - dev_err(priv->dev, "no pixel rate control in subdev %s\n", > > > > + /* Read the link frequency from the remote subdev. */ > > > > + mbps = v4l2_get_link_freq(source->ctrl_handler, bpp, 2 * lanes); > > > > + if (mbps < 0) { > > > > + dev_err(priv->dev, "failed to get link rate from subdev %s\n", > > > > source->name); > > > > - return -EINVAL; > > > > + return mbps; > > > > } > > > > - > > > > /* > > > > * Calculate the phypll in mbps. > > > > - * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes) > > > > * bps = link_freq * 2 > > > > */ > > > > - mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; > > > > - do_div(mbps, lanes * 1000000); > > > > + do_div(mbps, 1000000 / 2); > > > > > > > > return mbps; > > > > }
Hi Laurent, Thanks for the good write-up of the two different uses for this type of information, it make the situation more clear for me. I really think this series moves in the right direction as the current usage of V4L2_CID_PIXEL_RATE is then wrong in the rcar-vin driver. The reason being to work around the fact that the V4L2_CID_LINK_FREQ is a menu and not an INT64 control. I'm looking at the ADV748x driver sources and it seems it too adjusts the link frequency bases on the source rate. With this background do you think the right move is to turn V4L2_CID_LINK_FREQ into a INT64 and try to remove or redefine V4L2_CID_PIXEL_RATE to better describe to more complex parameters you outline below? On 2021-03-23 23:24:40 +0200, Laurent Pinchart wrote: > Hi Niklas, > > On Tue, Mar 23, 2021 at 02:57:05PM +0100, Niklas Söderlund wrote: > > On 2021-03-23 15:10:41 +0200, Sakari Ailus wrote: > > > On Mon, Mar 08, 2021 at 02:46:52PM +0100, Niklas Söderlund wrote: > > > > On 2021-03-03 21:08:14 +0300, Andrey Konovalov wrote: > > > > > To get the link frequency value, or to calculate a parameter depending on > > > > > it the receiver driver should use V4L2_CID_LINK_FREQ. If V4L2_CID_LINK_FREQ > > > > > control is not implemented in the remote subdevice, the link frequency > > > > > can be calculated from V4L2_CID_PIXEL_RATE control value. But the latter > > > > > may not give the correct link frequency, and should only be used as the > > > > > last resort. v4l2_get_link_freq() does exactly that, so use it instead > > > > > of reading V4L2_CID_PIXEL_RATE directly. > > > > > > > > I like the direction this patch is taking, but I'm a bit concerned about > > > > that V4L2_CID_LINK_FREQ is not able to replace V4L2_CID_PIXEL_RATE as it > > > > is designed today. Maybe my concern is unfounded and only reflects my > > > > own misunderstanding of the API. > > > > > > > > When I wrote this code I tried to first do it using V4L2_CID_LINK_FREQ > > > > but I found no way to be able to express the wide rang of values needed > > > > for my use-case given that V4L2_CID_LINK_FREQ is a menu control. I had > > > > > > I think we could make it alternatively a 64-bit integer control if that > > > helps. The helper needs to be adjusted accordingly. > > > > That would solve my concern. > > > > > > to use V4L2_CID_PIXEL_RATE as it allowed me to at runtime calculate and > > > > report the link speed based on input formats. The Use-cases I need to > > > > address are where CSI-2 transmitter themself are a bridge in the video > > > > pipeline, for example > > > > > > Is the actual bus frequency changed based on this? > > > > Yes > > > > > Depending on the system where this chip is being used, only certain > > > frequencies may be allowed on that bus. It would be most straightforward to > > > use only those, but on the other hand, if any frequency can be used and > > > that is certain, then I have no objections to allowing that either. We > > > simply would make the link-frequencies property optional. > > > > The transmitter is a ADV748x and depending on the video input source > > (HDMI or CVBS) the output frequency changes. Failing to negotiate this > > of course results in the CSI-2 receiver never detecting LP-11. > > > > > > * Case 1 - HDMI video source > > > > > > > > HDMI source -> ADV748x (HDMI-to-CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver > > > > > > > > The R-Car CSI-2 receiver needs to know the CSI-2 link frequency and > > > > queries the ADV748x using V4L2_CID_PIXEL_RATE. The ADV748x reports the > > > > pixel rate based on the HDMI format detected on its sink pad. > > > > > > > > This could be done using V4L2_CID_LINK_FREQ, but as it's a menu control > > > > it becomes rather tricky to populate it with all possible values, but I > > > > guess it could be doable? > > There are, generally speaking, two different uses for this information > on the receiver side. We need to configure the DPHY timings that depend > on the link frequency, and we need to configure the functional clock of > the receiver and downstream IP cores to ensure they have enough > bandwidth to absorb all pixels. Those are two fundamentally different > issues: > > - The DPHY timings depend on the link frequency, which is a well-defined > physical concept. We currently compute it from the pixel rate, which > is a more loosely defined concept (see below). Assuming the > V4L2_CID_LINK_FREQ control can be made to report the actual link > frequency (and given that this is the control's purpose, there's no > other option than making it work, otherwise the control would be > entirely pointless), possibly by turning it into an INT64 control, > then that's the right control to use for this purpose. > > - The functional clock of the video pipeline need to be able to absord > the incoming pixels. If the clock is configurable, it means that it > differs from the CSI-2 receiver clock (derived from the bus), which > normally implies a FIFO between the CSI-2 receiver and the downstream > blocks. The main constraint is that the FIFO shouldn't overflow, which > in practice means that the effective average pixel rate per line on > the input needs to be smaller or equal than on the output. This > however doesn't mean that the input clock needs to be higher than the > output clock, given that not only input and output bus widths can be > different, but horizontal blanking can also be used to perform timing > adjustements. For instance, if the input video stream has 1000 active > and 3000 blanking pixels per line, assuming identical bus widths on > the input and output side of the FIFO, we could have an output clock > frequency equal to half of the input clock frequency, as long as the > FIFO depth is at least 500 pixels. The output side would have a > horizontal blanking of 1000 pixels. The same applies on the > transmitter side, as there's often a FIFO between the pixel source > (the pixel array for a sensor for instance, with a sampling clock > rate) and the CSI-2 transmitter (running at the bus rate). The pixel > rate is thus a much more fuzzy concept, isn't well-defined in V4L2, > and can lead to all kind of interoperability issues. It should only be > used along with blanking information, in order to perform rate > adaptation calculations. > > > > > * Case 2 - Multiple video streams over a CSI-2 bus (GMSL) > > > > > > > > Camera 1 -| > > > > Camera 2 -| > > > > Camera 3 -|---> MAX9286 (GMSL-to CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver > > > > Camera 4 -| > > > > > > > > The MAX9286 has 4 sink pads each connected to an independent camera and > > > > a single CSI-2 source pad. When streaming starts the MAX9286 computes > > > > the total CSI-2 link speed as V4L2_CID_PIXEL_RATE based on the format on > > > > each of it's 4 sink pads. > > > > > > > > As in case 1 this could be reported by V4L2_CID_LINK_FREQ but I don't > > > > see it as feasible to populate the menu control with all possible > > > > frequencies before hand. > > As explained above, the CSI-2 frequency doesn't have to match the pixel > rate of the sensor(s). I haven't checked exactly how the MAX9286 handles > clock domains, but in general there source rate and the bus rate can be > different. That's why the link frequency is often a menu control with a > limited set of values (carefully selected by the system designer to > accommodate EMC constraints), while the source rate can vary more > freely. As long as the link frequency provides enough bandwidth, it > doesn't have to be tightly coupled with the pixel rate. For source > devices that have a single clock domain, and adjust the link frequency > to follow the source rate, then turning the link frequency control into > an INT64 would make sense. > > > > > Hopefully this is all easily solvable and I have only misunderstood how > > > > menu controls work. If not I think this needs to be considered as part > > > > of this series as otherwise it could leave the CSI-2 bridge drivers > > > > without a path forward. > > > > > > > > > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> > > > > > > > > I tested this and it works as expected. Also as expected it prints lots > > > > of warnings about the usage of V4L2_CID_PIXEL_RATE :-) Once I understand > > > > how I can fix the CSI-2 transmitters used as bridges in the R-Car boards > > > > I will be happy to add my tag to this series as well as fix the bridge > > > > drivers. > > > > > > > > > --- > > > > > drivers/media/platform/rcar-vin/rcar-csi2.c | 18 +++++++----------- > > > > > 1 file changed, 7 insertions(+), 11 deletions(-) > > > > > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c > > > > > index e06cd512aba2..eec8f9dd9060 100644 > > > > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c > > > > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > > > > > @@ -455,29 +455,25 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp, > > > > > unsigned int lanes) > > > > > { > > > > > struct v4l2_subdev *source; > > > > > - struct v4l2_ctrl *ctrl; > > > > > - u64 mbps; > > > > > + s64 mbps; > > > > > > > > > > if (!priv->remote) > > > > > return -ENODEV; > > > > > > > > > > source = priv->remote; > > > > > > > > > > - /* Read the pixel rate control from remote. */ > > > > > - ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE); > > > > > - if (!ctrl) { > > > > > - dev_err(priv->dev, "no pixel rate control in subdev %s\n", > > > > > + /* Read the link frequency from the remote subdev. */ > > > > > + mbps = v4l2_get_link_freq(source->ctrl_handler, bpp, 2 * lanes); > > > > > + if (mbps < 0) { > > > > > + dev_err(priv->dev, "failed to get link rate from subdev %s\n", > > > > > source->name); > > > > > - return -EINVAL; > > > > > + return mbps; > > > > > } > > > > > - > > > > > /* > > > > > * Calculate the phypll in mbps. > > > > > - * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes) > > > > > * bps = link_freq * 2 > > > > > */ > > > > > - mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; > > > > > - do_div(mbps, lanes * 1000000); > > > > > + do_div(mbps, 1000000 / 2); > > > > > > > > > > return mbps; > > > > > } > > -- > Regards, > > Laurent Pinchart
Hi Niklas, Laurent, and Sakari, Thank you for your comments, that's a good discussion! So it looks like changing V4L2_CID_LINK_FREQ control from an integer menu to an INT64 one works for everyone. What is the best way to do this change? It could be single patch which turns V4L2_CID_LINK_FREQ into an INT64, changes the v4l2_get_link_freq() helper accordingly, and updates all the drivers implementing or using V4L2_CID_LINK_FREQ (twenty-something drivers). The change is simple, but the diff is quite large. Doing it as a set of patches would break git bisecting, so isn't an option. Or we could make the INT64 variant of the link frequency control a separate new control (e.g. V4L2_CID_LINK_FREQ_NEW or V4L2_CID_LINK_FREQ_INT64), switch V4L2_CID_LINK_FREQ users to the new version one by one, then delete the older version and rename the new one back to V4L2_CID_LINK_FREQ. This change might affect the userland, but it looks like this control isn't the one widely used by the applications. E.g. V4L2_CID_LINK_FREQ definition is present in libcamera (include/linux/v4l2-controls.h), but is not currently used. One more question below. On 25.03.2021 12:39, Niklas Söderlund wrote: > Hi Laurent, > > Thanks for the good write-up of the two different uses for this type of > information, it make the situation more clear for me. I really think > this series moves in the right direction as the current usage of > V4L2_CID_PIXEL_RATE is then wrong in the rcar-vin driver. The reason > being to work around the fact that the V4L2_CID_LINK_FREQ is a menu and > not an INT64 control. > > I'm looking at the ADV748x driver sources and it seems it too adjusts > the link frequency bases on the source rate. With this background do you > think the right move is to turn V4L2_CID_LINK_FREQ into a INT64 and try > to remove or redefine V4L2_CID_PIXEL_RATE to better describe to more > complex parameters you outline below? > > On 2021-03-23 23:24:40 +0200, Laurent Pinchart wrote: >> Hi Niklas, >> >> On Tue, Mar 23, 2021 at 02:57:05PM +0100, Niklas Söderlund wrote: >>> On 2021-03-23 15:10:41 +0200, Sakari Ailus wrote: >>>> On Mon, Mar 08, 2021 at 02:46:52PM +0100, Niklas Söderlund wrote: >>>>> On 2021-03-03 21:08:14 +0300, Andrey Konovalov wrote: >>>>>> To get the link frequency value, or to calculate a parameter depending on >>>>>> it the receiver driver should use V4L2_CID_LINK_FREQ. If V4L2_CID_LINK_FREQ >>>>>> control is not implemented in the remote subdevice, the link frequency >>>>>> can be calculated from V4L2_CID_PIXEL_RATE control value. But the latter >>>>>> may not give the correct link frequency, and should only be used as the >>>>>> last resort. v4l2_get_link_freq() does exactly that, so use it instead >>>>>> of reading V4L2_CID_PIXEL_RATE directly. >>>>> >>>>> I like the direction this patch is taking, but I'm a bit concerned about >>>>> that V4L2_CID_LINK_FREQ is not able to replace V4L2_CID_PIXEL_RATE as it >>>>> is designed today. Maybe my concern is unfounded and only reflects my >>>>> own misunderstanding of the API. >>>>> >>>>> When I wrote this code I tried to first do it using V4L2_CID_LINK_FREQ >>>>> but I found no way to be able to express the wide rang of values needed >>>>> for my use-case given that V4L2_CID_LINK_FREQ is a menu control. I had >>>> >>>> I think we could make it alternatively a 64-bit integer control if that >>>> helps. The helper needs to be adjusted accordingly. >>> >>> That would solve my concern. >>> >>>>> to use V4L2_CID_PIXEL_RATE as it allowed me to at runtime calculate and >>>>> report the link speed based on input formats. The Use-cases I need to >>>>> address are where CSI-2 transmitter themself are a bridge in the video >>>>> pipeline, for example >>>> >>>> Is the actual bus frequency changed based on this? >>> >>> Yes >>> >>>> Depending on the system where this chip is being used, only certain >>>> frequencies may be allowed on that bus. It would be most straightforward to >>>> use only those, but on the other hand, if any frequency can be used and >>>> that is certain, then I have no objections to allowing that either. We >>>> simply would make the link-frequencies property optional. Sakari, Could you please elaborate a bit more on the last sentence ("make the link-frequencies property optional")? How should the CSI-2 receiver get the link frequency value if this optional property is missing? If the transmitter driver doesn't implement V4L2_CID_LINK_FREQ then it must guarantee that the link frequency calculated from V4L2_CID_PIXEL_RATE gives the correct value - something like that? Should the warning message [1] in the v4l2_get_link_freq() helper be rephrased if V4L2_CID_LINK_FREQ is optional? Thanks, Andrey [1] https://git.linuxtv.org/media_tree.git/commit/drivers/media/v4l2-core/v4l2-common.c?id=67012d97df931b1be24efa0cac06f20d5be062eb >>> The transmitter is a ADV748x and depending on the video input source >>> (HDMI or CVBS) the output frequency changes. Failing to negotiate this >>> of course results in the CSI-2 receiver never detecting LP-11. >>> >>>>> * Case 1 - HDMI video source >>>>> >>>>> HDMI source -> ADV748x (HDMI-to-CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver >>>>> >>>>> The R-Car CSI-2 receiver needs to know the CSI-2 link frequency and >>>>> queries the ADV748x using V4L2_CID_PIXEL_RATE. The ADV748x reports the >>>>> pixel rate based on the HDMI format detected on its sink pad. >>>>> >>>>> This could be done using V4L2_CID_LINK_FREQ, but as it's a menu control >>>>> it becomes rather tricky to populate it with all possible values, but I >>>>> guess it could be doable? >> >> There are, generally speaking, two different uses for this information >> on the receiver side. We need to configure the DPHY timings that depend >> on the link frequency, and we need to configure the functional clock of >> the receiver and downstream IP cores to ensure they have enough >> bandwidth to absorb all pixels. Those are two fundamentally different >> issues: >> >> - The DPHY timings depend on the link frequency, which is a well-defined >> physical concept. We currently compute it from the pixel rate, which >> is a more loosely defined concept (see below). Assuming the >> V4L2_CID_LINK_FREQ control can be made to report the actual link >> frequency (and given that this is the control's purpose, there's no >> other option than making it work, otherwise the control would be >> entirely pointless), possibly by turning it into an INT64 control, >> then that's the right control to use for this purpose. >> >> - The functional clock of the video pipeline need to be able to absord >> the incoming pixels. If the clock is configurable, it means that it >> differs from the CSI-2 receiver clock (derived from the bus), which >> normally implies a FIFO between the CSI-2 receiver and the downstream >> blocks. The main constraint is that the FIFO shouldn't overflow, which >> in practice means that the effective average pixel rate per line on >> the input needs to be smaller or equal than on the output. This >> however doesn't mean that the input clock needs to be higher than the >> output clock, given that not only input and output bus widths can be >> different, but horizontal blanking can also be used to perform timing >> adjustements. For instance, if the input video stream has 1000 active >> and 3000 blanking pixels per line, assuming identical bus widths on >> the input and output side of the FIFO, we could have an output clock >> frequency equal to half of the input clock frequency, as long as the >> FIFO depth is at least 500 pixels. The output side would have a >> horizontal blanking of 1000 pixels. The same applies on the >> transmitter side, as there's often a FIFO between the pixel source >> (the pixel array for a sensor for instance, with a sampling clock >> rate) and the CSI-2 transmitter (running at the bus rate). The pixel >> rate is thus a much more fuzzy concept, isn't well-defined in V4L2, >> and can lead to all kind of interoperability issues. It should only be >> used along with blanking information, in order to perform rate >> adaptation calculations. >> >>>>> * Case 2 - Multiple video streams over a CSI-2 bus (GMSL) >>>>> >>>>> Camera 1 -| >>>>> Camera 2 -| >>>>> Camera 3 -|---> MAX9286 (GMSL-to CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver >>>>> Camera 4 -| >>>>> >>>>> The MAX9286 has 4 sink pads each connected to an independent camera and >>>>> a single CSI-2 source pad. When streaming starts the MAX9286 computes >>>>> the total CSI-2 link speed as V4L2_CID_PIXEL_RATE based on the format on >>>>> each of it's 4 sink pads. >>>>> >>>>> As in case 1 this could be reported by V4L2_CID_LINK_FREQ but I don't >>>>> see it as feasible to populate the menu control with all possible >>>>> frequencies before hand. >> >> As explained above, the CSI-2 frequency doesn't have to match the pixel >> rate of the sensor(s). I haven't checked exactly how the MAX9286 handles >> clock domains, but in general there source rate and the bus rate can be >> different. That's why the link frequency is often a menu control with a >> limited set of values (carefully selected by the system designer to >> accommodate EMC constraints), while the source rate can vary more >> freely. As long as the link frequency provides enough bandwidth, it >> doesn't have to be tightly coupled with the pixel rate. For source >> devices that have a single clock domain, and adjust the link frequency >> to follow the source rate, then turning the link frequency control into >> an INT64 would make sense. >> >>>>> Hopefully this is all easily solvable and I have only misunderstood how >>>>> menu controls work. If not I think this needs to be considered as part >>>>> of this series as otherwise it could leave the CSI-2 bridge drivers >>>>> without a path forward. >>>>> >>>>>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> >>>>> >>>>> I tested this and it works as expected. Also as expected it prints lots >>>>> of warnings about the usage of V4L2_CID_PIXEL_RATE :-) Once I understand >>>>> how I can fix the CSI-2 transmitters used as bridges in the R-Car boards >>>>> I will be happy to add my tag to this series as well as fix the bridge >>>>> drivers. >>>>> >>>>>> --- >>>>>> drivers/media/platform/rcar-vin/rcar-csi2.c | 18 +++++++----------- >>>>>> 1 file changed, 7 insertions(+), 11 deletions(-) >>>>>> >>>>>> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c >>>>>> index e06cd512aba2..eec8f9dd9060 100644 >>>>>> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c >>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c >>>>>> @@ -455,29 +455,25 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp, >>>>>> unsigned int lanes) >>>>>> { >>>>>> struct v4l2_subdev *source; >>>>>> - struct v4l2_ctrl *ctrl; >>>>>> - u64 mbps; >>>>>> + s64 mbps; >>>>>> >>>>>> if (!priv->remote) >>>>>> return -ENODEV; >>>>>> >>>>>> source = priv->remote; >>>>>> >>>>>> - /* Read the pixel rate control from remote. */ >>>>>> - ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE); >>>>>> - if (!ctrl) { >>>>>> - dev_err(priv->dev, "no pixel rate control in subdev %s\n", >>>>>> + /* Read the link frequency from the remote subdev. */ >>>>>> + mbps = v4l2_get_link_freq(source->ctrl_handler, bpp, 2 * lanes); >>>>>> + if (mbps < 0) { >>>>>> + dev_err(priv->dev, "failed to get link rate from subdev %s\n", >>>>>> source->name); >>>>>> - return -EINVAL; >>>>>> + return mbps; >>>>>> } >>>>>> - >>>>>> /* >>>>>> * Calculate the phypll in mbps. >>>>>> - * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes) >>>>>> * bps = link_freq * 2 >>>>>> */ >>>>>> - mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; >>>>>> - do_div(mbps, lanes * 1000000); >>>>>> + do_div(mbps, 1000000 / 2); >>>>>> >>>>>> return mbps; >>>>>> } >> >> -- >> Regards, >> >> Laurent Pinchart >
diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c index e06cd512aba2..eec8f9dd9060 100644 --- a/drivers/media/platform/rcar-vin/rcar-csi2.c +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c @@ -455,29 +455,25 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp, unsigned int lanes) { struct v4l2_subdev *source; - struct v4l2_ctrl *ctrl; - u64 mbps; + s64 mbps; if (!priv->remote) return -ENODEV; source = priv->remote; - /* Read the pixel rate control from remote. */ - ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE); - if (!ctrl) { - dev_err(priv->dev, "no pixel rate control in subdev %s\n", + /* Read the link frequency from the remote subdev. */ + mbps = v4l2_get_link_freq(source->ctrl_handler, bpp, 2 * lanes); + if (mbps < 0) { + dev_err(priv->dev, "failed to get link rate from subdev %s\n", source->name); - return -EINVAL; + return mbps; } - /* * Calculate the phypll in mbps. - * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes) * bps = link_freq * 2 */ - mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; - do_div(mbps, lanes * 1000000); + do_div(mbps, 1000000 / 2); return mbps; }