| Message ID | 20200416104052.2643098-15-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | GMSL: max9286-v8-rc1 + RDAMC20-v8 | expand |
Hi Jacopo, There we go - looks like a viable solution to me. I wonder/suspect if this is the issue the BSP team were having some weeks ago too? Anyway, I'm certainly happy with this. Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> On 16/04/2020 11:40, Jacopo Mondi wrote: > Calculate the CSI-2 transmitter pixel rate using the one reported from > sources. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/i2c/max9286.c | 50 +++++++++++++++++++++++++++++-------- > 1 file changed, 40 insertions(+), 10 deletions(-) > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > index 2d71205a1aad..3ef74ba10074 100644 > --- a/drivers/media/i2c/max9286.c > +++ b/drivers/media/i2c/max9286.c > @@ -430,9 +430,46 @@ static int max9286_check_config_link(struct max9286_priv *priv, > * V4L2 Subdev > */ > > -static int max9286_set_pixelrate(struct max9286_priv *priv, s64 rate) > +static int max9286_set_pixelrate(struct max9286_priv *priv) > { > - return v4l2_ctrl_s_ctrl_int64(priv->pixelrate, rate); > + struct max9286_source *source = NULL; > + u64 pixelrate = 0; > + > + for_each_source(priv, source) { > + struct v4l2_ctrl *ctrl; > + u64 source_rate = 0; > + > + /* Pixel rate is mandatory to be reported by sources. */ > + ctrl = v4l2_ctrl_find(source->sd->ctrl_handler, > + V4L2_CID_PIXEL_RATE); > + if (!ctrl) { > + pixelrate = 0; > + break; > + } > + > + /* All source must report the same pixel rate. */ > + source_rate = v4l2_ctrl_g_ctrl_int64(ctrl); > + if (!pixelrate) { > + pixelrate = source_rate; > + } else if (pixelrate != source_rate) { > + dev_err(&priv->client->dev, > + "Unable to calculate pixel rate\n"); Hrm ... I would report "Source pixel rates are different\n" ... but I don't think that's a big deal. > + return -EINVAL; > + } > + } > + > + if (!pixelrate) { > + dev_err(&priv->client->dev, > + "No pixel rate control available in sources\n"); > + return -EINVAL; > + } > + > + /* > + * The CSI-2 transmitter pixel rate is the single source rate multiplied > + * by the number of available sources. > + */ > + return v4l2_ctrl_s_ctrl_int64(priv->pixelrate, > + pixelrate * priv->nsources); > } > > static int max9286_notify_bound(struct v4l2_async_notifier *notifier, > @@ -496,7 +533,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier, > */ > max9286_configure_i2c(priv, false); > > - return 0; > + return max9286_set_pixelrate(priv); Aha - at first I thought this would call for every bind event, but we already filter so this tail end of the function only executes on the last bind :-) > } > > static void max9286_notify_unbind(struct v4l2_async_notifier *notifier, > @@ -670,7 +707,6 @@ static int max9286_set_fmt(struct v4l2_subdev *sd, > { > struct max9286_priv *priv = sd_to_max9286(sd); > struct v4l2_mbus_framefmt *cfg_fmt; > - s64 pixelrate; > > if (format->pad >= MAX9286_SRC_PAD) > return -EINVAL; > @@ -695,12 +731,6 @@ static int max9286_set_fmt(struct v4l2_subdev *sd, > *cfg_fmt = format->format; > mutex_unlock(&priv->mutex); > > - /* Update pixel rate for the CSI2 receiver */ > - pixelrate = cfg_fmt->width * cfg_fmt->height > - * priv->nsources * 30 /*FPS*/; > - > - max9286_set_pixelrate(priv, pixelrate); > - > return 0; > } > >
Hi Kieran, On Thu, Apr 16, 2020 at 11:45:27AM +0100, Kieran Bingham wrote: > Hi Jacopo, > > There we go - looks like a viable solution to me. > > I wonder/suspect if this is the issue the BSP team were having some > weeks ago too? Could be. Let's revive the thread once we get this sent to linux-media > > Anyway, I'm certainly happy with this. > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > > On 16/04/2020 11:40, Jacopo Mondi wrote: > > Calculate the CSI-2 transmitter pixel rate using the one reported from > > sources. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > drivers/media/i2c/max9286.c | 50 +++++++++++++++++++++++++++++-------- > > 1 file changed, 40 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > > index 2d71205a1aad..3ef74ba10074 100644 > > --- a/drivers/media/i2c/max9286.c > > +++ b/drivers/media/i2c/max9286.c > > @@ -430,9 +430,46 @@ static int max9286_check_config_link(struct max9286_priv *priv, > > * V4L2 Subdev > > */ > > > > -static int max9286_set_pixelrate(struct max9286_priv *priv, s64 rate) > > +static int max9286_set_pixelrate(struct max9286_priv *priv) > > { > > - return v4l2_ctrl_s_ctrl_int64(priv->pixelrate, rate); > > + struct max9286_source *source = NULL; > > + u64 pixelrate = 0; > > + > > + for_each_source(priv, source) { > > + struct v4l2_ctrl *ctrl; > > + u64 source_rate = 0; > > + > > + /* Pixel rate is mandatory to be reported by sources. */ > > + ctrl = v4l2_ctrl_find(source->sd->ctrl_handler, > > + V4L2_CID_PIXEL_RATE); > > + if (!ctrl) { > > + pixelrate = 0; > > + break; > > + } > > + > > + /* All source must report the same pixel rate. */ > > + source_rate = v4l2_ctrl_g_ctrl_int64(ctrl); > > + if (!pixelrate) { > > + pixelrate = source_rate; > > + } else if (pixelrate != source_rate) { > > + dev_err(&priv->client->dev, > > + "Unable to calculate pixel rate\n"); > > Hrm ... I would report "Source pixel rates are different\n" > ... but I don't think that's a big deal. That's fine, as you wish :) > > > > + return -EINVAL; > > + } > > + } > > + > > + if (!pixelrate) { > > + dev_err(&priv->client->dev, > > + "No pixel rate control available in sources\n"); > > + return -EINVAL; > > + } > > + > > + /* > > + * The CSI-2 transmitter pixel rate is the single source rate multiplied > > + * by the number of available sources. > > + */ > > + return v4l2_ctrl_s_ctrl_int64(priv->pixelrate, > > + pixelrate * priv->nsources); > > } > > > > static int max9286_notify_bound(struct v4l2_async_notifier *notifier, > > @@ -496,7 +533,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier, > > */ > > max9286_configure_i2c(priv, false); > > > > - return 0; > > + return max9286_set_pixelrate(priv); > > Aha - at first I thought this would call for every bind event, but we > already filter so this tail end of the function only executes on the > last bind :-) Yes, we get there only when all remotes have been bound. I wonder what happens though if something goes wrong and we end up returning an error from _bound(). I assume the media graph won't complete and that's desirable, so this should be enough > > > > > } > > > > static void max9286_notify_unbind(struct v4l2_async_notifier *notifier, > > @@ -670,7 +707,6 @@ static int max9286_set_fmt(struct v4l2_subdev *sd, > > { > > struct max9286_priv *priv = sd_to_max9286(sd); > > struct v4l2_mbus_framefmt *cfg_fmt; > > - s64 pixelrate; > > > > if (format->pad >= MAX9286_SRC_PAD) > > return -EINVAL; > > @@ -695,12 +731,6 @@ static int max9286_set_fmt(struct v4l2_subdev *sd, > > *cfg_fmt = format->format; > > mutex_unlock(&priv->mutex); > > > > - /* Update pixel rate for the CSI2 receiver */ > > - pixelrate = cfg_fmt->width * cfg_fmt->height > > - * priv->nsources * 30 /*FPS*/; > > - > > - max9286_set_pixelrate(priv, pixelrate); > > - > > return 0; > > } > > > > >
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c index 2d71205a1aad..3ef74ba10074 100644 --- a/drivers/media/i2c/max9286.c +++ b/drivers/media/i2c/max9286.c @@ -430,9 +430,46 @@ static int max9286_check_config_link(struct max9286_priv *priv, * V4L2 Subdev */ -static int max9286_set_pixelrate(struct max9286_priv *priv, s64 rate) +static int max9286_set_pixelrate(struct max9286_priv *priv) { - return v4l2_ctrl_s_ctrl_int64(priv->pixelrate, rate); + struct max9286_source *source = NULL; + u64 pixelrate = 0; + + for_each_source(priv, source) { + struct v4l2_ctrl *ctrl; + u64 source_rate = 0; + + /* Pixel rate is mandatory to be reported by sources. */ + ctrl = v4l2_ctrl_find(source->sd->ctrl_handler, + V4L2_CID_PIXEL_RATE); + if (!ctrl) { + pixelrate = 0; + break; + } + + /* All source must report the same pixel rate. */ + source_rate = v4l2_ctrl_g_ctrl_int64(ctrl); + if (!pixelrate) { + pixelrate = source_rate; + } else if (pixelrate != source_rate) { + dev_err(&priv->client->dev, + "Unable to calculate pixel rate\n"); + return -EINVAL; + } + } + + if (!pixelrate) { + dev_err(&priv->client->dev, + "No pixel rate control available in sources\n"); + return -EINVAL; + } + + /* + * The CSI-2 transmitter pixel rate is the single source rate multiplied + * by the number of available sources. + */ + return v4l2_ctrl_s_ctrl_int64(priv->pixelrate, + pixelrate * priv->nsources); } static int max9286_notify_bound(struct v4l2_async_notifier *notifier, @@ -496,7 +533,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier, */ max9286_configure_i2c(priv, false); - return 0; + return max9286_set_pixelrate(priv); } static void max9286_notify_unbind(struct v4l2_async_notifier *notifier, @@ -670,7 +707,6 @@ static int max9286_set_fmt(struct v4l2_subdev *sd, { struct max9286_priv *priv = sd_to_max9286(sd); struct v4l2_mbus_framefmt *cfg_fmt; - s64 pixelrate; if (format->pad >= MAX9286_SRC_PAD) return -EINVAL; @@ -695,12 +731,6 @@ static int max9286_set_fmt(struct v4l2_subdev *sd, *cfg_fmt = format->format; mutex_unlock(&priv->mutex); - /* Update pixel rate for the CSI2 receiver */ - pixelrate = cfg_fmt->width * cfg_fmt->height - * priv->nsources * 30 /*FPS*/; - - max9286_set_pixelrate(priv, pixelrate); - return 0; }
Calculate the CSI-2 transmitter pixel rate using the one reported from sources. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- drivers/media/i2c/max9286.c | 50 +++++++++++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 10 deletions(-)