| Message ID | 20200611161651.264633-10-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | v4l2-subdev: Introduce [g|s]et_mbus_format pad op | expand |
Hi Jacopo, On Thu, Jun 11, 2020 at 06:16:51PM +0200, Jacopo Mondi wrote: > Use the newly introduced get_mbus_config() subdevice pad operation to > retrieve the remote subdevice MIPI CSI-2 bus configuration and configure > the number of active data lanes accordingly. > > In order to be able to call the remote subdevice operation cache the > index of the remote pad connected to the single CSI-2 input port. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/platform/rcar-vin/rcar-csi2.c | 61 ++++++++++++++++++++- > 1 file changed, 58 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c > index 151e6a90c5fb..11769f004fd8 100644 > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > @@ -363,6 +363,7 @@ struct rcar_csi2 { > struct v4l2_async_notifier notifier; > struct v4l2_async_subdev asd; > struct v4l2_subdev *remote; > + unsigned int remote_pad; > > struct v4l2_mbus_framefmt mf; > > @@ -371,6 +372,7 @@ struct rcar_csi2 { > > unsigned short lanes; > unsigned char lane_swap[4]; > + unsigned short active_lanes; Do you need this? I.e. should you not always request this from the transmitter device? > }; > > static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd) > @@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv) > > /* Wait for the clock and data lanes to enter LP-11 state. */ > for (timeout = 0; timeout <= 20; timeout++) { > - const u32 lane_mask = (1 << priv->lanes) - 1; > + const u32 lane_mask = (1 << priv->active_lanes) - 1; > > if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL) && > (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask) > @@ -471,11 +473,57 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp) > * bps = link_freq * 2 > */ > mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; > - do_div(mbps, priv->lanes * 1000000); > + do_div(mbps, priv->active_lanes * 1000000); > > return mbps; > } > > +static int rcsi2_config_active_lanes(struct rcar_csi2 *priv) > +{ > + struct v4l2_mbus_config mbus_config = { 0 }; > + unsigned int num_lanes = (-1U); > + int ret; > + > + priv->active_lanes = priv->lanes; > + ret = v4l2_subdev_call(priv->remote, pad, get_mbus_config, > + priv->remote_pad, &mbus_config); > + if (ret == -ENOIOCTLCMD) { > + dev_dbg(priv->dev, "No remote mbus configuration available\n"); > + return 0; > + } > + > + if (ret) { > + dev_err(priv->dev, "Failed to get remote mbus configuration\n"); > + return ret; > + } > + > + if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) { > + dev_err(priv->dev, "Unsupported media bus type %u\n", > + mbus_config.type); > + return -EINVAL; > + } > + > + if (mbus_config.flags & V4L2_MBUS_CSI2_1_LANE) > + num_lanes = 1; > + else if (mbus_config.flags & V4L2_MBUS_CSI2_2_LANE) > + num_lanes = 2; > + else if (mbus_config.flags & V4L2_MBUS_CSI2_3_LANE) > + num_lanes = 3; > + else if (mbus_config.flags & V4L2_MBUS_CSI2_4_LANE) > + num_lanes = 4; This is the downside of using flags... Anyway, I guess this is certainly fine now. > + > + if (num_lanes > priv->lanes) { > + dev_err(priv->dev, > + "Unsupported mbus config: too many data lanes %u\n", > + num_lanes); > + return -EINVAL; > + } > + > + priv->active_lanes = num_lanes; > + > + return 0; > +} > + > static int rcsi2_start_receiver(struct rcar_csi2 *priv) > { > const struct rcar_csi2_format *format; > @@ -490,6 +538,11 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv) > /* Code is validated in set_fmt. */ > format = rcsi2_code_to_fmt(priv->mf.code); > > + /* Get the remote mbus config to get the number of enabled lanes. */ > + ret = rcsi2_config_active_lanes(priv); > + if (ret) > + return ret; > + > /* > * Enable all supported CSI-2 channels with virtual channel and > * data type matching. > @@ -522,7 +575,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv) > } > > phycnt = PHYCNT_ENABLECLK; > - phycnt |= (1 << priv->lanes) - 1; > + phycnt |= (1 << priv->active_lanes) - 1; > > mbps = rcsi2_calc_mbps(priv, format->bpp); > if (mbps < 0) > @@ -748,6 +801,7 @@ static int rcsi2_notify_bound(struct v4l2_async_notifier *notifier, > } > > priv->remote = subdev; > + priv->remote_pad = pad; > > dev_dbg(priv->dev, "Bound %s pad: %d\n", subdev->name, pad); > > @@ -793,6 +847,7 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv, > priv->lanes); > return -EINVAL; > } > + priv->active_lanes = priv->lanes; > > for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) { > priv->lane_swap[i] = i < priv->lanes ?
Hi Jacopo, Sakari, On 15/06/2020 09:34, Sakari Ailus wrote: > Hi Jacopo, > > On Thu, Jun 11, 2020 at 06:16:51PM +0200, Jacopo Mondi wrote: >> Use the newly introduced get_mbus_config() subdevice pad operation to >> retrieve the remote subdevice MIPI CSI-2 bus configuration and configure >> the number of active data lanes accordingly. >> >> In order to be able to call the remote subdevice operation cache the >> index of the remote pad connected to the single CSI-2 input port. >> >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >> --- >> drivers/media/platform/rcar-vin/rcar-csi2.c | 61 ++++++++++++++++++++- >> 1 file changed, 58 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c >> index 151e6a90c5fb..11769f004fd8 100644 >> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c >> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c >> @@ -363,6 +363,7 @@ struct rcar_csi2 { >> struct v4l2_async_notifier notifier; >> struct v4l2_async_subdev asd; >> struct v4l2_subdev *remote; >> + unsigned int remote_pad; >> >> struct v4l2_mbus_framefmt mf; >> >> @@ -371,6 +372,7 @@ struct rcar_csi2 { >> >> unsigned short lanes; >> unsigned char lane_swap[4]; >> + unsigned short active_lanes; > > Do you need this? I.e. should you not always request this from the > transmitter device? > >> }; >> >> static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd) >> @@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv) >> >> /* Wait for the clock and data lanes to enter LP-11 state. */ >> for (timeout = 0; timeout <= 20; timeout++) { >> - const u32 lane_mask = (1 << priv->lanes) - 1; >> + const u32 lane_mask = (1 << priv->active_lanes) - 1; >> >> if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL) && >> (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask) >> @@ -471,11 +473,57 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp) >> * bps = link_freq * 2 >> */ >> mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; >> - do_div(mbps, priv->lanes * 1000000); >> + do_div(mbps, priv->active_lanes * 1000000); >> >> return mbps; >> } >> >> +static int rcsi2_config_active_lanes(struct rcar_csi2 *priv) >> +{ >> + struct v4l2_mbus_config mbus_config = { 0 }; >> + unsigned int num_lanes = (-1U); >> + int ret; >> + >> + priv->active_lanes = priv->lanes; >> + ret = v4l2_subdev_call(priv->remote, pad, get_mbus_config, >> + priv->remote_pad, &mbus_config); >> + if (ret == -ENOIOCTLCMD) { >> + dev_dbg(priv->dev, "No remote mbus configuration available\n"); >> + return 0; >> + } >> + >> + if (ret) { >> + dev_err(priv->dev, "Failed to get remote mbus configuration\n"); >> + return ret; >> + } >> + >> + if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) { >> + dev_err(priv->dev, "Unsupported media bus type %u\n", >> + mbus_config.type); >> + return -EINVAL; >> + } >> + >> + if (mbus_config.flags & V4L2_MBUS_CSI2_1_LANE) >> + num_lanes = 1; >> + else if (mbus_config.flags & V4L2_MBUS_CSI2_2_LANE) >> + num_lanes = 2; >> + else if (mbus_config.flags & V4L2_MBUS_CSI2_3_LANE) >> + num_lanes = 3; >> + else if (mbus_config.flags & V4L2_MBUS_CSI2_4_LANE) >> + num_lanes = 4; > > This is the downside of using flags... Anyway, I guess this is certainly > fine now. Given that the mbus_config has a .type, can't we easily extend struct v4l2_mbus_config to contain a type specific union ? Or is that something to do on top of this series instead? I mean, it could even go after the flags, and keep the flags as a common attribute - so that no other changes are needed to other components... -- Kieran > >> + >> + if (num_lanes > priv->lanes) { >> + dev_err(priv->dev, >> + "Unsupported mbus config: too many data lanes %u\n", >> + num_lanes); >> + return -EINVAL; >> + } >> + >> + priv->active_lanes = num_lanes; >> + >> + return 0; >> +} >> + >> static int rcsi2_start_receiver(struct rcar_csi2 *priv) >> { >> const struct rcar_csi2_format *format; >> @@ -490,6 +538,11 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv) >> /* Code is validated in set_fmt. */ >> format = rcsi2_code_to_fmt(priv->mf.code); >> >> + /* Get the remote mbus config to get the number of enabled lanes. */ >> + ret = rcsi2_config_active_lanes(priv); >> + if (ret) >> + return ret; >> + >> /* >> * Enable all supported CSI-2 channels with virtual channel and >> * data type matching. >> @@ -522,7 +575,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv) >> } >> >> phycnt = PHYCNT_ENABLECLK; >> - phycnt |= (1 << priv->lanes) - 1; >> + phycnt |= (1 << priv->active_lanes) - 1; >> >> mbps = rcsi2_calc_mbps(priv, format->bpp); >> if (mbps < 0) >> @@ -748,6 +801,7 @@ static int rcsi2_notify_bound(struct v4l2_async_notifier *notifier, >> } >> >> priv->remote = subdev; >> + priv->remote_pad = pad; >> >> dev_dbg(priv->dev, "Bound %s pad: %d\n", subdev->name, pad); >> >> @@ -793,6 +847,7 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv, >> priv->lanes); >> return -EINVAL; >> } >> + priv->active_lanes = priv->lanes; >> >> for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) { >> priv->lane_swap[i] = i < priv->lanes ? >
Hi Jacopo, On 15/06/2020 09:43, Jacopo Mondi wrote: > Hi Sakari, > > On Mon, Jun 15, 2020 at 11:34:06AM +0300, Sakari Ailus wrote: >> Hi Jacopo, >> >> On Thu, Jun 11, 2020 at 06:16:51PM +0200, Jacopo Mondi wrote: >>> Use the newly introduced get_mbus_config() subdevice pad operation to >>> retrieve the remote subdevice MIPI CSI-2 bus configuration and configure >>> the number of active data lanes accordingly. >>> >>> In order to be able to call the remote subdevice operation cache the >>> index of the remote pad connected to the single CSI-2 input port. >>> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >>> --- >>> drivers/media/platform/rcar-vin/rcar-csi2.c | 61 ++++++++++++++++++++- >>> 1 file changed, 58 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c >>> index 151e6a90c5fb..11769f004fd8 100644 >>> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c >>> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c >>> @@ -363,6 +363,7 @@ struct rcar_csi2 { >>> struct v4l2_async_notifier notifier; >>> struct v4l2_async_subdev asd; >>> struct v4l2_subdev *remote; >>> + unsigned int remote_pad; >>> >>> struct v4l2_mbus_framefmt mf; >>> >>> @@ -371,6 +372,7 @@ struct rcar_csi2 { >>> >>> unsigned short lanes; >>> unsigned char lane_swap[4]; >>> + unsigned short active_lanes; >> >> Do you need this? I.e. should you not always request this from the >> transmitter device? > > It's actually the other way around. The receiver queries the > transmitter to know how many data lanes it intends to use and adjusts > its configuration to accommodate it. > >> >>> }; >>> >>> static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd) >>> @@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv) >>> >>> /* Wait for the clock and data lanes to enter LP-11 state. */ >>> for (timeout = 0; timeout <= 20; timeout++) { >>> - const u32 lane_mask = (1 << priv->lanes) - 1; >>> + const u32 lane_mask = (1 << priv->active_lanes) - 1; >>> >>> if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL) && >>> (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask) >>> @@ -471,11 +473,57 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp) >>> * bps = link_freq * 2 >>> */ >>> mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; >>> - do_div(mbps, priv->lanes * 1000000); >>> + do_div(mbps, priv->active_lanes * 1000000); >>> >>> return mbps; >>> } >>> >>> +static int rcsi2_config_active_lanes(struct rcar_csi2 *priv) >>> +{ >>> + struct v4l2_mbus_config mbus_config = { 0 }; >>> + unsigned int num_lanes = (-1U); >>> + int ret; >>> + >>> + priv->active_lanes = priv->lanes; >>> + ret = v4l2_subdev_call(priv->remote, pad, get_mbus_config, >>> + priv->remote_pad, &mbus_config); >>> + if (ret == -ENOIOCTLCMD) { >>> + dev_dbg(priv->dev, "No remote mbus configuration available\n"); >>> + return 0; >>> + } >>> + >>> + if (ret) { >>> + dev_err(priv->dev, "Failed to get remote mbus configuration\n"); >>> + return ret; >>> + } >>> + >>> + if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) { >>> + dev_err(priv->dev, "Unsupported media bus type %u\n", >>> + mbus_config.type); >>> + return -EINVAL; >>> + } >>> + >>> + if (mbus_config.flags & V4L2_MBUS_CSI2_1_LANE) >>> + num_lanes = 1; >>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_2_LANE) >>> + num_lanes = 2; >>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_3_LANE) >>> + num_lanes = 3; >>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_4_LANE) >>> + num_lanes = 4; >> >> This is the downside of using flags... Anyway, I guess this is certainly >> fine now. >> > > Let's change this on top, if we like to (and I would like to :) That answers my question then ;-) -- Kieran > > Thanks > j > >>> + >>> + if (num_lanes > priv->lanes) { >>> + dev_err(priv->dev, >>> + "Unsupported mbus config: too many data lanes %u\n", >>> + num_lanes); >>> + return -EINVAL; >>> + } >>> + >>> + priv->active_lanes = num_lanes; >>> + >>> + return 0; >>> +} >>> + >>> static int rcsi2_start_receiver(struct rcar_csi2 *priv) >>> { >>> const struct rcar_csi2_format *format; >>> @@ -490,6 +538,11 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv) >>> /* Code is validated in set_fmt. */ >>> format = rcsi2_code_to_fmt(priv->mf.code); >>> >>> + /* Get the remote mbus config to get the number of enabled lanes. */ >>> + ret = rcsi2_config_active_lanes(priv); >>> + if (ret) >>> + return ret; >>> + >>> /* >>> * Enable all supported CSI-2 channels with virtual channel and >>> * data type matching. >>> @@ -522,7 +575,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv) >>> } >>> >>> phycnt = PHYCNT_ENABLECLK; >>> - phycnt |= (1 << priv->lanes) - 1; >>> + phycnt |= (1 << priv->active_lanes) - 1; >>> >>> mbps = rcsi2_calc_mbps(priv, format->bpp); >>> if (mbps < 0) >>> @@ -748,6 +801,7 @@ static int rcsi2_notify_bound(struct v4l2_async_notifier *notifier, >>> } >>> >>> priv->remote = subdev; >>> + priv->remote_pad = pad; >>> >>> dev_dbg(priv->dev, "Bound %s pad: %d\n", subdev->name, pad); >>> >>> @@ -793,6 +847,7 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv, >>> priv->lanes); >>> return -EINVAL; >>> } >>> + priv->active_lanes = priv->lanes; >>> >>> for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) { >>> priv->lane_swap[i] = i < priv->lanes ? >> >> -- >> Kind regards, >> >> Sakari Ailus
Hi Sakari, On Mon, Jun 15, 2020 at 11:34:06AM +0300, Sakari Ailus wrote: > Hi Jacopo, > > On Thu, Jun 11, 2020 at 06:16:51PM +0200, Jacopo Mondi wrote: > > Use the newly introduced get_mbus_config() subdevice pad operation to > > retrieve the remote subdevice MIPI CSI-2 bus configuration and configure > > the number of active data lanes accordingly. > > > > In order to be able to call the remote subdevice operation cache the > > index of the remote pad connected to the single CSI-2 input port. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > drivers/media/platform/rcar-vin/rcar-csi2.c | 61 ++++++++++++++++++++- > > 1 file changed, 58 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c > > index 151e6a90c5fb..11769f004fd8 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > > @@ -363,6 +363,7 @@ struct rcar_csi2 { > > struct v4l2_async_notifier notifier; > > struct v4l2_async_subdev asd; > > struct v4l2_subdev *remote; > > + unsigned int remote_pad; > > > > struct v4l2_mbus_framefmt mf; > > > > @@ -371,6 +372,7 @@ struct rcar_csi2 { > > > > unsigned short lanes; > > unsigned char lane_swap[4]; > > + unsigned short active_lanes; > > Do you need this? I.e. should you not always request this from the > transmitter device? It's actually the other way around. The receiver queries the transmitter to know how many data lanes it intends to use and adjusts its configuration to accommodate it. > > > }; > > > > static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd) > > @@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv) > > > > /* Wait for the clock and data lanes to enter LP-11 state. */ > > for (timeout = 0; timeout <= 20; timeout++) { > > - const u32 lane_mask = (1 << priv->lanes) - 1; > > + const u32 lane_mask = (1 << priv->active_lanes) - 1; > > > > if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL) && > > (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask) > > @@ -471,11 +473,57 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp) > > * bps = link_freq * 2 > > */ > > mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; > > - do_div(mbps, priv->lanes * 1000000); > > + do_div(mbps, priv->active_lanes * 1000000); > > > > return mbps; > > } > > > > +static int rcsi2_config_active_lanes(struct rcar_csi2 *priv) > > +{ > > + struct v4l2_mbus_config mbus_config = { 0 }; > > + unsigned int num_lanes = (-1U); > > + int ret; > > + > > + priv->active_lanes = priv->lanes; > > + ret = v4l2_subdev_call(priv->remote, pad, get_mbus_config, > > + priv->remote_pad, &mbus_config); > > + if (ret == -ENOIOCTLCMD) { > > + dev_dbg(priv->dev, "No remote mbus configuration available\n"); > > + return 0; > > + } > > + > > + if (ret) { > > + dev_err(priv->dev, "Failed to get remote mbus configuration\n"); > > + return ret; > > + } > > + > > + if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) { > > + dev_err(priv->dev, "Unsupported media bus type %u\n", > > + mbus_config.type); > > + return -EINVAL; > > + } > > + > > + if (mbus_config.flags & V4L2_MBUS_CSI2_1_LANE) > > + num_lanes = 1; > > + else if (mbus_config.flags & V4L2_MBUS_CSI2_2_LANE) > > + num_lanes = 2; > > + else if (mbus_config.flags & V4L2_MBUS_CSI2_3_LANE) > > + num_lanes = 3; > > + else if (mbus_config.flags & V4L2_MBUS_CSI2_4_LANE) > > + num_lanes = 4; > > This is the downside of using flags... Anyway, I guess this is certainly > fine now. > Let's change this on top, if we like to (and I would like to :) Thanks j > > + > > + if (num_lanes > priv->lanes) { > > + dev_err(priv->dev, > > + "Unsupported mbus config: too many data lanes %u\n", > > + num_lanes); > > + return -EINVAL; > > + } > > + > > + priv->active_lanes = num_lanes; > > + > > + return 0; > > +} > > + > > static int rcsi2_start_receiver(struct rcar_csi2 *priv) > > { > > const struct rcar_csi2_format *format; > > @@ -490,6 +538,11 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv) > > /* Code is validated in set_fmt. */ > > format = rcsi2_code_to_fmt(priv->mf.code); > > > > + /* Get the remote mbus config to get the number of enabled lanes. */ > > + ret = rcsi2_config_active_lanes(priv); > > + if (ret) > > + return ret; > > + > > /* > > * Enable all supported CSI-2 channels with virtual channel and > > * data type matching. > > @@ -522,7 +575,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv) > > } > > > > phycnt = PHYCNT_ENABLECLK; > > - phycnt |= (1 << priv->lanes) - 1; > > + phycnt |= (1 << priv->active_lanes) - 1; > > > > mbps = rcsi2_calc_mbps(priv, format->bpp); > > if (mbps < 0) > > @@ -748,6 +801,7 @@ static int rcsi2_notify_bound(struct v4l2_async_notifier *notifier, > > } > > > > priv->remote = subdev; > > + priv->remote_pad = pad; > > > > dev_dbg(priv->dev, "Bound %s pad: %d\n", subdev->name, pad); > > > > @@ -793,6 +847,7 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv, > > priv->lanes); > > return -EINVAL; > > } > > + priv->active_lanes = priv->lanes; > > > > for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) { > > priv->lane_swap[i] = i < priv->lanes ? > > -- > Kind regards, > > Sakari Ailus
Hi Kieran, On Mon, Jun 15, 2020 at 09:40:42AM +0100, Kieran Bingham wrote: > Hi Jacopo, > > On 15/06/2020 09:43, Jacopo Mondi wrote: > > Hi Sakari, > > > > On Mon, Jun 15, 2020 at 11:34:06AM +0300, Sakari Ailus wrote: > >> Hi Jacopo, > >> > >> On Thu, Jun 11, 2020 at 06:16:51PM +0200, Jacopo Mondi wrote: > >>> Use the newly introduced get_mbus_config() subdevice pad operation to > >>> retrieve the remote subdevice MIPI CSI-2 bus configuration and configure > >>> the number of active data lanes accordingly. > >>> > >>> In order to be able to call the remote subdevice operation cache the > >>> index of the remote pad connected to the single CSI-2 input port. > >>> > >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > >>> --- > >>> drivers/media/platform/rcar-vin/rcar-csi2.c | 61 ++++++++++++++++++++- > >>> 1 file changed, 58 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c > >>> index 151e6a90c5fb..11769f004fd8 100644 > >>> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c > >>> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > >>> @@ -363,6 +363,7 @@ struct rcar_csi2 { > >>> struct v4l2_async_notifier notifier; > >>> struct v4l2_async_subdev asd; > >>> struct v4l2_subdev *remote; > >>> + unsigned int remote_pad; > >>> > >>> struct v4l2_mbus_framefmt mf; > >>> > >>> @@ -371,6 +372,7 @@ struct rcar_csi2 { > >>> > >>> unsigned short lanes; > >>> unsigned char lane_swap[4]; > >>> + unsigned short active_lanes; > >> > >> Do you need this? I.e. should you not always request this from the > >> transmitter device? > > > > It's actually the other way around. The receiver queries the > > transmitter to know how many data lanes it intends to use and adjusts > > its configuration to accommodate it. > > > >> > >>> }; > >>> > >>> static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd) > >>> @@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv) > >>> > >>> /* Wait for the clock and data lanes to enter LP-11 state. */ > >>> for (timeout = 0; timeout <= 20; timeout++) { > >>> - const u32 lane_mask = (1 << priv->lanes) - 1; > >>> + const u32 lane_mask = (1 << priv->active_lanes) - 1; > >>> > >>> if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL) && > >>> (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask) > >>> @@ -471,11 +473,57 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp) > >>> * bps = link_freq * 2 > >>> */ > >>> mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; > >>> - do_div(mbps, priv->lanes * 1000000); > >>> + do_div(mbps, priv->active_lanes * 1000000); > >>> > >>> return mbps; > >>> } > >>> > >>> +static int rcsi2_config_active_lanes(struct rcar_csi2 *priv) > >>> +{ > >>> + struct v4l2_mbus_config mbus_config = { 0 }; > >>> + unsigned int num_lanes = (-1U); > >>> + int ret; > >>> + > >>> + priv->active_lanes = priv->lanes; > >>> + ret = v4l2_subdev_call(priv->remote, pad, get_mbus_config, > >>> + priv->remote_pad, &mbus_config); > >>> + if (ret == -ENOIOCTLCMD) { > >>> + dev_dbg(priv->dev, "No remote mbus configuration available\n"); > >>> + return 0; > >>> + } > >>> + > >>> + if (ret) { > >>> + dev_err(priv->dev, "Failed to get remote mbus configuration\n"); > >>> + return ret; > >>> + } > >>> + > >>> + if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) { > >>> + dev_err(priv->dev, "Unsupported media bus type %u\n", > >>> + mbus_config.type); > >>> + return -EINVAL; > >>> + } > >>> + > >>> + if (mbus_config.flags & V4L2_MBUS_CSI2_1_LANE) > >>> + num_lanes = 1; > >>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_2_LANE) > >>> + num_lanes = 2; > >>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_3_LANE) > >>> + num_lanes = 3; > >>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_4_LANE) > >>> + num_lanes = 4; > >> > >> This is the downside of using flags... Anyway, I guess this is certainly > >> fine now. > >> > > > > Let's change this on top, if we like to (and I would like to :) > > That answers my question then ;-) > There's been a discussion on one of the first version of the series where I tried replacing flags and introduced a union of structures with fields, most likely similar to what you suggested. Based on the received comments I decided to use the existing V4L2_MBUS_* flags to ease the replacement of the video ops g|s_mbus_config() to minimize the changes. We could then on top replace flags. Thanks j > -- > Kieran > > > > > > Thanks > > j > > > >>> + > >>> + if (num_lanes > priv->lanes) { > >>> + dev_err(priv->dev, > >>> + "Unsupported mbus config: too many data lanes %u\n", > >>> + num_lanes); > >>> + return -EINVAL; > >>> + } > >>> + > >>> + priv->active_lanes = num_lanes; > >>> + > >>> + return 0; > >>> +} > >>> + > >>> static int rcsi2_start_receiver(struct rcar_csi2 *priv) > >>> { > >>> const struct rcar_csi2_format *format; > >>> @@ -490,6 +538,11 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv) > >>> /* Code is validated in set_fmt. */ > >>> format = rcsi2_code_to_fmt(priv->mf.code); > >>> > >>> + /* Get the remote mbus config to get the number of enabled lanes. */ > >>> + ret = rcsi2_config_active_lanes(priv); > >>> + if (ret) > >>> + return ret; > >>> + > >>> /* > >>> * Enable all supported CSI-2 channels with virtual channel and > >>> * data type matching. > >>> @@ -522,7 +575,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv) > >>> } > >>> > >>> phycnt = PHYCNT_ENABLECLK; > >>> - phycnt |= (1 << priv->lanes) - 1; > >>> + phycnt |= (1 << priv->active_lanes) - 1; > >>> > >>> mbps = rcsi2_calc_mbps(priv, format->bpp); > >>> if (mbps < 0) > >>> @@ -748,6 +801,7 @@ static int rcsi2_notify_bound(struct v4l2_async_notifier *notifier, > >>> } > >>> > >>> priv->remote = subdev; > >>> + priv->remote_pad = pad; > >>> > >>> dev_dbg(priv->dev, "Bound %s pad: %d\n", subdev->name, pad); > >>> > >>> @@ -793,6 +847,7 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv, > >>> priv->lanes); > >>> return -EINVAL; > >>> } > >>> + priv->active_lanes = priv->lanes; > >>> > >>> for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) { > >>> priv->lane_swap[i] = i < priv->lanes ? > >> > >> -- > >> Kind regards, > >> > >> Sakari Ailus > > -- > Regards > -- > Kieran
Hi Jacopo, On 15/06/2020 09:47, Jacopo Mondi wrote: > Hi Kieran, > > On Mon, Jun 15, 2020 at 09:40:42AM +0100, Kieran Bingham wrote: >> Hi Jacopo, >> >> On 15/06/2020 09:43, Jacopo Mondi wrote: >>> Hi Sakari, >>> >>> On Mon, Jun 15, 2020 at 11:34:06AM +0300, Sakari Ailus wrote: >>>> Hi Jacopo, >>>> >>>> On Thu, Jun 11, 2020 at 06:16:51PM +0200, Jacopo Mondi wrote: >>>>> Use the newly introduced get_mbus_config() subdevice pad operation to >>>>> retrieve the remote subdevice MIPI CSI-2 bus configuration and configure >>>>> the number of active data lanes accordingly. >>>>> >>>>> In order to be able to call the remote subdevice operation cache the >>>>> index of the remote pad connected to the single CSI-2 input port. >>>>> >>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >>>>> --- >>>>> drivers/media/platform/rcar-vin/rcar-csi2.c | 61 ++++++++++++++++++++- >>>>> 1 file changed, 58 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c >>>>> index 151e6a90c5fb..11769f004fd8 100644 >>>>> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c >>>>> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c >>>>> @@ -363,6 +363,7 @@ struct rcar_csi2 { >>>>> struct v4l2_async_notifier notifier; >>>>> struct v4l2_async_subdev asd; >>>>> struct v4l2_subdev *remote; >>>>> + unsigned int remote_pad; >>>>> >>>>> struct v4l2_mbus_framefmt mf; >>>>> >>>>> @@ -371,6 +372,7 @@ struct rcar_csi2 { >>>>> >>>>> unsigned short lanes; >>>>> unsigned char lane_swap[4]; >>>>> + unsigned short active_lanes; >>>> >>>> Do you need this? I.e. should you not always request this from the >>>> transmitter device? >>> >>> It's actually the other way around. The receiver queries the >>> transmitter to know how many data lanes it intends to use and adjusts >>> its configuration to accommodate it. >>> >>>> >>>>> }; >>>>> >>>>> static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd) >>>>> @@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv) >>>>> >>>>> /* Wait for the clock and data lanes to enter LP-11 state. */ >>>>> for (timeout = 0; timeout <= 20; timeout++) { >>>>> - const u32 lane_mask = (1 << priv->lanes) - 1; >>>>> + const u32 lane_mask = (1 << priv->active_lanes) - 1; >>>>> >>>>> if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL) && >>>>> (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask) >>>>> @@ -471,11 +473,57 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp) >>>>> * bps = link_freq * 2 >>>>> */ >>>>> mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; >>>>> - do_div(mbps, priv->lanes * 1000000); >>>>> + do_div(mbps, priv->active_lanes * 1000000); >>>>> >>>>> return mbps; >>>>> } >>>>> >>>>> +static int rcsi2_config_active_lanes(struct rcar_csi2 *priv) >>>>> +{ >>>>> + struct v4l2_mbus_config mbus_config = { 0 }; >>>>> + unsigned int num_lanes = (-1U); >>>>> + int ret; >>>>> + >>>>> + priv->active_lanes = priv->lanes; >>>>> + ret = v4l2_subdev_call(priv->remote, pad, get_mbus_config, >>>>> + priv->remote_pad, &mbus_config); >>>>> + if (ret == -ENOIOCTLCMD) { >>>>> + dev_dbg(priv->dev, "No remote mbus configuration available\n"); >>>>> + return 0; >>>>> + } >>>>> + >>>>> + if (ret) { >>>>> + dev_err(priv->dev, "Failed to get remote mbus configuration\n"); >>>>> + return ret; >>>>> + } >>>>> + >>>>> + if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) { >>>>> + dev_err(priv->dev, "Unsupported media bus type %u\n", >>>>> + mbus_config.type); >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> + if (mbus_config.flags & V4L2_MBUS_CSI2_1_LANE) >>>>> + num_lanes = 1; >>>>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_2_LANE) >>>>> + num_lanes = 2; >>>>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_3_LANE) >>>>> + num_lanes = 3; >>>>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_4_LANE) >>>>> + num_lanes = 4; >>>> >>>> This is the downside of using flags... Anyway, I guess this is certainly >>>> fine now. >>>> >>> >>> Let's change this on top, if we like to (and I would like to :) >> >> That answers my question then ;-) >> > > There's been a discussion on one of the first version of the series > where I tried replacing flags and introduced a union of structures > with fields, most likely similar to what you suggested. > > Based on the received comments I decided to use the existing > V4L2_MBUS_* flags to ease the replacement of the video ops > g|s_mbus_config() to minimize the changes. We could then on top > replace flags. That sound fine to me. When I first saw this patch, I thought that this series was introducing the V4L2_MBUS_CSI2_4_LANE flag types, but I see they are pre-existing. I think it's perfectly reasonable to use the existing infrastructure for this series, and adapt after. -- Kieran > > Thanks > j > >> -- >> Kieran >> >> >>> >>> Thanks >>> j >>> >>>>> + >>>>> + if (num_lanes > priv->lanes) { >>>>> + dev_err(priv->dev, >>>>> + "Unsupported mbus config: too many data lanes %u\n", >>>>> + num_lanes); >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> + priv->active_lanes = num_lanes; >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> static int rcsi2_start_receiver(struct rcar_csi2 *priv) >>>>> { >>>>> const struct rcar_csi2_format *format; >>>>> @@ -490,6 +538,11 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv) >>>>> /* Code is validated in set_fmt. */ >>>>> format = rcsi2_code_to_fmt(priv->mf.code); >>>>> >>>>> + /* Get the remote mbus config to get the number of enabled lanes. */ >>>>> + ret = rcsi2_config_active_lanes(priv); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> /* >>>>> * Enable all supported CSI-2 channels with virtual channel and >>>>> * data type matching. >>>>> @@ -522,7 +575,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv) >>>>> } >>>>> >>>>> phycnt = PHYCNT_ENABLECLK; >>>>> - phycnt |= (1 << priv->lanes) - 1; >>>>> + phycnt |= (1 << priv->active_lanes) - 1; >>>>> >>>>> mbps = rcsi2_calc_mbps(priv, format->bpp); >>>>> if (mbps < 0) >>>>> @@ -748,6 +801,7 @@ static int rcsi2_notify_bound(struct v4l2_async_notifier *notifier, >>>>> } >>>>> >>>>> priv->remote = subdev; >>>>> + priv->remote_pad = pad; >>>>> >>>>> dev_dbg(priv->dev, "Bound %s pad: %d\n", subdev->name, pad); >>>>> >>>>> @@ -793,6 +847,7 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv, >>>>> priv->lanes); >>>>> return -EINVAL; >>>>> } >>>>> + priv->active_lanes = priv->lanes; >>>>> >>>>> for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) { >>>>> priv->lane_swap[i] = i < priv->lanes ? >>>> >>>> -- >>>> Kind regards, >>>> >>>> Sakari Ailus >> >> -- >> Regards >> -- >> Kieran
Jacopo, On Mon, Jun 15, 2020 at 10:43:06AM +0200, Jacopo Mondi wrote: > Hi Sakari, > > On Mon, Jun 15, 2020 at 11:34:06AM +0300, Sakari Ailus wrote: > > Hi Jacopo, > > > > On Thu, Jun 11, 2020 at 06:16:51PM +0200, Jacopo Mondi wrote: > > > Use the newly introduced get_mbus_config() subdevice pad operation to > > > retrieve the remote subdevice MIPI CSI-2 bus configuration and configure > > > the number of active data lanes accordingly. > > > > > > In order to be able to call the remote subdevice operation cache the > > > index of the remote pad connected to the single CSI-2 input port. > > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > --- > > > drivers/media/platform/rcar-vin/rcar-csi2.c | 61 ++++++++++++++++++++- > > > 1 file changed, 58 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c > > > index 151e6a90c5fb..11769f004fd8 100644 > > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c > > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > > > @@ -363,6 +363,7 @@ struct rcar_csi2 { > > > struct v4l2_async_notifier notifier; > > > struct v4l2_async_subdev asd; > > > struct v4l2_subdev *remote; > > > + unsigned int remote_pad; > > > > > > struct v4l2_mbus_framefmt mf; > > > > > > @@ -371,6 +372,7 @@ struct rcar_csi2 { > > > > > > unsigned short lanes; > > > unsigned char lane_swap[4]; > > > + unsigned short active_lanes; > > > > Do you need this? I.e. should you not always request this from the > > transmitter device? > > It's actually the other way around. The receiver queries the > transmitter to know how many data lanes it intends to use and adjusts > its configuration to accommodate it. I think we didn't have a different view on the process. But you basically need this when you're starting streaming, so why is the information stored here? > > > > > > }; > > > > > > static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd) > > > @@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv) > > > > > > /* Wait for the clock and data lanes to enter LP-11 state. */ > > > for (timeout = 0; timeout <= 20; timeout++) { > > > - const u32 lane_mask = (1 << priv->lanes) - 1; > > > + const u32 lane_mask = (1 << priv->active_lanes) - 1; > > > > > > if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL) && > > > (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask) > > > @@ -471,11 +473,57 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp) > > > * bps = link_freq * 2 > > > */ > > > mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; > > > - do_div(mbps, priv->lanes * 1000000); > > > + do_div(mbps, priv->active_lanes * 1000000); > > > > > > return mbps; > > > } > > > > > > +static int rcsi2_config_active_lanes(struct rcar_csi2 *priv) > > > +{ > > > + struct v4l2_mbus_config mbus_config = { 0 }; > > > + unsigned int num_lanes = (-1U); > > > + int ret; > > > + > > > + priv->active_lanes = priv->lanes; > > > + ret = v4l2_subdev_call(priv->remote, pad, get_mbus_config, > > > + priv->remote_pad, &mbus_config); > > > + if (ret == -ENOIOCTLCMD) { > > > + dev_dbg(priv->dev, "No remote mbus configuration available\n"); > > > + return 0; > > > + } > > > + > > > + if (ret) { > > > + dev_err(priv->dev, "Failed to get remote mbus configuration\n"); > > > + return ret; > > > + } > > > + > > > + if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) { > > > + dev_err(priv->dev, "Unsupported media bus type %u\n", > > > + mbus_config.type); > > > + return -EINVAL; > > > + } > > > + > > > + if (mbus_config.flags & V4L2_MBUS_CSI2_1_LANE) > > > + num_lanes = 1; > > > + else if (mbus_config.flags & V4L2_MBUS_CSI2_2_LANE) > > > + num_lanes = 2; > > > + else if (mbus_config.flags & V4L2_MBUS_CSI2_3_LANE) > > > + num_lanes = 3; > > > + else if (mbus_config.flags & V4L2_MBUS_CSI2_4_LANE) > > > + num_lanes = 4; > > > > This is the downside of using flags... Anyway, I guess this is certainly > > fine now. > > > > Let's change this on top, if we like to (and I would like to :) Agreed.
Hi Sakari, On Mon, Jun 15, 2020 at 11:53:35AM +0300, Sakari Ailus wrote: > Jacopo, > > On Mon, Jun 15, 2020 at 10:43:06AM +0200, Jacopo Mondi wrote: > > Hi Sakari, > > > > On Mon, Jun 15, 2020 at 11:34:06AM +0300, Sakari Ailus wrote: > > > Hi Jacopo, > > > > > > On Thu, Jun 11, 2020 at 06:16:51PM +0200, Jacopo Mondi wrote: > > > > Use the newly introduced get_mbus_config() subdevice pad operation to > > > > retrieve the remote subdevice MIPI CSI-2 bus configuration and configure > > > > the number of active data lanes accordingly. > > > > > > > > In order to be able to call the remote subdevice operation cache the > > > > index of the remote pad connected to the single CSI-2 input port. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > > --- > > > > drivers/media/platform/rcar-vin/rcar-csi2.c | 61 ++++++++++++++++++++- > > > > 1 file changed, 58 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c > > > > index 151e6a90c5fb..11769f004fd8 100644 > > > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c > > > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > > > > @@ -363,6 +363,7 @@ struct rcar_csi2 { > > > > struct v4l2_async_notifier notifier; > > > > struct v4l2_async_subdev asd; > > > > struct v4l2_subdev *remote; > > > > + unsigned int remote_pad; > > > > > > > > struct v4l2_mbus_framefmt mf; > > > > > > > > @@ -371,6 +372,7 @@ struct rcar_csi2 { > > > > > > > > unsigned short lanes; > > > > unsigned char lane_swap[4]; > > > > + unsigned short active_lanes; > > > > > > Do you need this? I.e. should you not always request this from the > > > transmitter device? > > > > It's actually the other way around. The receiver queries the > > transmitter to know how many data lanes it intends to use and adjusts > > its configuration to accommodate it. > > I think we didn't have a different view on the process. But you basically > need this when you're starting streaming, so why is the information stored > here? > Still not sure I got your question, so pardon me if the reply is something obvious to you already, and I'm missing the real point. But, basically what happens is there at probe time the number of 'available' data lanes is parsed from firmware and stored in priv->lanes. At stream start time, the remote end gets queried and the number of 'active' lanes adjusted according to what it's reported. Then, during the whole CSI-2 interface startup process, the number of 'active' lanes is used in a few different places (ie. rcsi2_wait_phy_start() and rcsi2_calc_mbps()) so I had to store it somewhere. Now that I wrote that, I realize you might be wondering why a parameter that is valid for a single streaming session is stored in the main driver structure. This might not be optimal, but the driver struct already contains, in example, a v4l2_mbus_framefmt entry which is a session specific paramter as well, so I thought it was no harm adding the number of active 'lanes' there. Is this what's bothering you ? Thanks j > > > > > > > > > }; > > > > > > > > static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd) > > > > @@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv) > > > > > > > > /* Wait for the clock and data lanes to enter LP-11 state. */ > > > > for (timeout = 0; timeout <= 20; timeout++) { > > > > - const u32 lane_mask = (1 << priv->lanes) - 1; > > > > + const u32 lane_mask = (1 << priv->active_lanes) - 1; > > > > > > > > if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL) && > > > > (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask) > > > > @@ -471,11 +473,57 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp) > > > > * bps = link_freq * 2 > > > > */ > > > > mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; > > > > - do_div(mbps, priv->lanes * 1000000); > > > > + do_div(mbps, priv->active_lanes * 1000000); > > > > > > > > return mbps; > > > > } > > > > > > > > +static int rcsi2_config_active_lanes(struct rcar_csi2 *priv) > > > > +{ > > > > + struct v4l2_mbus_config mbus_config = { 0 }; > > > > + unsigned int num_lanes = (-1U); > > > > + int ret; > > > > + > > > > + priv->active_lanes = priv->lanes; > > > > + ret = v4l2_subdev_call(priv->remote, pad, get_mbus_config, > > > > + priv->remote_pad, &mbus_config); > > > > + if (ret == -ENOIOCTLCMD) { > > > > + dev_dbg(priv->dev, "No remote mbus configuration available\n"); > > > > + return 0; > > > > + } > > > > + > > > > + if (ret) { > > > > + dev_err(priv->dev, "Failed to get remote mbus configuration\n"); > > > > + return ret; > > > > + } > > > > + > > > > + if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) { > > > > + dev_err(priv->dev, "Unsupported media bus type %u\n", > > > > + mbus_config.type); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + if (mbus_config.flags & V4L2_MBUS_CSI2_1_LANE) > > > > + num_lanes = 1; > > > > + else if (mbus_config.flags & V4L2_MBUS_CSI2_2_LANE) > > > > + num_lanes = 2; > > > > + else if (mbus_config.flags & V4L2_MBUS_CSI2_3_LANE) > > > > + num_lanes = 3; > > > > + else if (mbus_config.flags & V4L2_MBUS_CSI2_4_LANE) > > > > + num_lanes = 4; > > > > > > This is the downside of using flags... Anyway, I guess this is certainly > > > fine now. > > > > > > > Let's change this on top, if we like to (and I would like to :) > > Agreed. > > -- > Sakari Ailus
Hi Jacopo, On Mon, Jun 15, 2020 at 11:19:49AM +0200, Jacopo Mondi wrote: > Hi Sakari, > > On Mon, Jun 15, 2020 at 11:53:35AM +0300, Sakari Ailus wrote: > > Jacopo, > > > > On Mon, Jun 15, 2020 at 10:43:06AM +0200, Jacopo Mondi wrote: > > > Hi Sakari, > > > > > > On Mon, Jun 15, 2020 at 11:34:06AM +0300, Sakari Ailus wrote: > > > > Hi Jacopo, > > > > > > > > On Thu, Jun 11, 2020 at 06:16:51PM +0200, Jacopo Mondi wrote: > > > > > Use the newly introduced get_mbus_config() subdevice pad operation to > > > > > retrieve the remote subdevice MIPI CSI-2 bus configuration and configure > > > > > the number of active data lanes accordingly. > > > > > > > > > > In order to be able to call the remote subdevice operation cache the > > > > > index of the remote pad connected to the single CSI-2 input port. > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > > > --- > > > > > drivers/media/platform/rcar-vin/rcar-csi2.c | 61 ++++++++++++++++++++- > > > > > 1 file changed, 58 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c > > > > > index 151e6a90c5fb..11769f004fd8 100644 > > > > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c > > > > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > > > > > @@ -363,6 +363,7 @@ struct rcar_csi2 { > > > > > struct v4l2_async_notifier notifier; > > > > > struct v4l2_async_subdev asd; > > > > > struct v4l2_subdev *remote; > > > > > + unsigned int remote_pad; > > > > > > > > > > struct v4l2_mbus_framefmt mf; > > > > > > > > > > @@ -371,6 +372,7 @@ struct rcar_csi2 { > > > > > > > > > > unsigned short lanes; > > > > > unsigned char lane_swap[4]; > > > > > + unsigned short active_lanes; > > > > > > > > Do you need this? I.e. should you not always request this from the > > > > transmitter device? > > > > > > It's actually the other way around. The receiver queries the > > > transmitter to know how many data lanes it intends to use and adjusts > > > its configuration to accommodate it. > > > > I think we didn't have a different view on the process. But you basically > > need this when you're starting streaming, so why is the information stored > > here? > > > > Still not sure I got your question, so pardon me if the reply is > something obvious to you already, and I'm missing the real point. > > But, basically what happens is there at probe time the number of > 'available' data lanes is parsed from firmware and stored in > priv->lanes. At stream start time, the remote end gets queried and the > number of 'active' lanes adjusted according to what it's reported. > > Then, during the whole CSI-2 interface startup process, the number of > 'active' lanes is used in a few different places (ie. > rcsi2_wait_phy_start() and rcsi2_calc_mbps()) so I had to store it > somewhere. > > Now that I wrote that, I realize you might be wondering why a > parameter that is valid for a single streaming session is stored in > the main driver structure. This might not be optimal, but the driver > struct already contains, in example, a v4l2_mbus_framefmt entry which > is a session specific paramter as well, so I thought it was no harm > adding the number of active 'lanes' there. Is this what's bothering > you ? The frame format is needed there as that's set by the user outside the s_stream call. The number of active lanes is not needed elsewhere, so therefore I wouldn't store in the device context either. It can be a local variable which you may pass to another function.
diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c index 151e6a90c5fb..11769f004fd8 100644 --- a/drivers/media/platform/rcar-vin/rcar-csi2.c +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c @@ -363,6 +363,7 @@ struct rcar_csi2 { struct v4l2_async_notifier notifier; struct v4l2_async_subdev asd; struct v4l2_subdev *remote; + unsigned int remote_pad; struct v4l2_mbus_framefmt mf; @@ -371,6 +372,7 @@ struct rcar_csi2 { unsigned short lanes; unsigned char lane_swap[4]; + unsigned short active_lanes; }; static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd) @@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv) /* Wait for the clock and data lanes to enter LP-11 state. */ for (timeout = 0; timeout <= 20; timeout++) { - const u32 lane_mask = (1 << priv->lanes) - 1; + const u32 lane_mask = (1 << priv->active_lanes) - 1; if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL) && (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask) @@ -471,11 +473,57 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp) * bps = link_freq * 2 */ mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; - do_div(mbps, priv->lanes * 1000000); + do_div(mbps, priv->active_lanes * 1000000); return mbps; } +static int rcsi2_config_active_lanes(struct rcar_csi2 *priv) +{ + struct v4l2_mbus_config mbus_config = { 0 }; + unsigned int num_lanes = (-1U); + int ret; + + priv->active_lanes = priv->lanes; + ret = v4l2_subdev_call(priv->remote, pad, get_mbus_config, + priv->remote_pad, &mbus_config); + if (ret == -ENOIOCTLCMD) { + dev_dbg(priv->dev, "No remote mbus configuration available\n"); + return 0; + } + + if (ret) { + dev_err(priv->dev, "Failed to get remote mbus configuration\n"); + return ret; + } + + if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) { + dev_err(priv->dev, "Unsupported media bus type %u\n", + mbus_config.type); + return -EINVAL; + } + + if (mbus_config.flags & V4L2_MBUS_CSI2_1_LANE) + num_lanes = 1; + else if (mbus_config.flags & V4L2_MBUS_CSI2_2_LANE) + num_lanes = 2; + else if (mbus_config.flags & V4L2_MBUS_CSI2_3_LANE) + num_lanes = 3; + else if (mbus_config.flags & V4L2_MBUS_CSI2_4_LANE) + num_lanes = 4; + + if (num_lanes > priv->lanes) { + dev_err(priv->dev, + "Unsupported mbus config: too many data lanes %u\n", + num_lanes); + return -EINVAL; + } + + priv->active_lanes = num_lanes; + + return 0; +} + static int rcsi2_start_receiver(struct rcar_csi2 *priv) { const struct rcar_csi2_format *format; @@ -490,6 +538,11 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv) /* Code is validated in set_fmt. */ format = rcsi2_code_to_fmt(priv->mf.code); + /* Get the remote mbus config to get the number of enabled lanes. */ + ret = rcsi2_config_active_lanes(priv); + if (ret) + return ret; + /* * Enable all supported CSI-2 channels with virtual channel and * data type matching. @@ -522,7 +575,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv) } phycnt = PHYCNT_ENABLECLK; - phycnt |= (1 << priv->lanes) - 1; + phycnt |= (1 << priv->active_lanes) - 1; mbps = rcsi2_calc_mbps(priv, format->bpp); if (mbps < 0) @@ -748,6 +801,7 @@ static int rcsi2_notify_bound(struct v4l2_async_notifier *notifier, } priv->remote = subdev; + priv->remote_pad = pad; dev_dbg(priv->dev, "Bound %s pad: %d\n", subdev->name, pad); @@ -793,6 +847,7 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv, priv->lanes); return -EINVAL; } + priv->active_lanes = priv->lanes; for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) { priv->lane_swap[i] = i < priv->lanes ?
Use the newly introduced get_mbus_config() subdevice pad operation to retrieve the remote subdevice MIPI CSI-2 bus configuration and configure the number of active data lanes accordingly. In order to be able to call the remote subdevice operation cache the index of the remote pad connected to the single CSI-2 input port. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- drivers/media/platform/rcar-vin/rcar-csi2.c | 61 ++++++++++++++++++++- 1 file changed, 58 insertions(+), 3 deletions(-)