* [PATCH v2 0/3] media: qcom: camss: V4L2_CID_PIXEL_RATE/LINK_FREQ fixes
@ 2021-02-17 22:11 Andrey Konovalov
2021-02-17 22:11 ` [PATCH v2 1/3] v4l: common: v4l2_get_link_freq: add printing a warning Andrey Konovalov
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Andrey Konovalov @ 2021-02-17 22:11 UTC (permalink / raw)
To: junak.pub, robert.foss, sakari.ailus
Cc: todor.too, agross, bjorn.andersson, mchehab, laurent.pinchart,
jacopo, linux-media, linux-arm-msm, linux-kernel
The first patch adds printing a warning in v4l2_get_link_freq() if
V4L2_CID_LINK_FREQ isn't implemented (this is a mandatory control for
CSI-2 transmitter drivers [1], but many sensor drivers don't have it
currently).
The second patch is the start of the work discussed in the "[RFC] Repurpose
V4L2_CID_PIXEL_RATE for the sampling rate in the pixel array" thread [2].
I plan to send a few other similar patches for other CSI receiver drivers,
and if the current patchset needs to wait for those before it can be merged,
that's fine for me.
The reason I decided to post the camss patch first is the patch [3] by
Vladimir Lypak. The third patch in this series is the Vladimir's patch
rebased onto the changes done by the second patch. By replacing getting
the pixel clock with v4l2_get_link_freq() the second patch also fixes the
integer overflow which Vladimir's patch addresses. So the third patch
only needs to fix drivers/media/platform/qcom/camss/camss-vfe.c which
the second patch doesn't touch.
The resulting patchset is free from the "undefined reference to `__udivdi3'"
issue [4] as the u64 value is only divided by a power of 2, which doesn't
need do_div().
[1] https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/csi2.html
[2] https://www.spinics.net/lists/linux-media/msg183183.html
[3] https://www.spinics.net/lists/linux-media/msg186875.html
[4] https://www.spinics.net/lists/linux-media/msg186918.html
Changes in v2:
* Added [PATCH 1/3] v4l: common: v4l2_get_link_freq: add printing a warning
* camss_get_link_freq() changed to take the actual number of lanes as the
third arg vs the number of lanes multiplied by 2 in the first version
* Fixed checkpatch warnings and bad indentation
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v2 1/3] v4l: common: v4l2_get_link_freq: add printing a warning 2021-02-17 22:11 [PATCH v2 0/3] media: qcom: camss: V4L2_CID_PIXEL_RATE/LINK_FREQ fixes Andrey Konovalov @ 2021-02-17 22:11 ` Andrey Konovalov 2021-02-18 7:55 ` Jacopo Mondi 2021-02-17 22:11 ` [PATCH v2 2/3] media: camss: use v4l2_get_link_freq() to calculate the relevant clocks Andrey Konovalov 2021-02-17 22:11 ` [PATCH v2 3/3] media: qcom: camss: Fix overflows in clock rate calculations Andrey Konovalov 2 siblings, 1 reply; 9+ messages in thread From: Andrey Konovalov @ 2021-02-17 22:11 UTC (permalink / raw) To: junak.pub, robert.foss, sakari.ailus Cc: todor.too, agross, bjorn.andersson, mchehab, laurent.pinchart, jacopo, linux-media, linux-arm-msm, linux-kernel, Andrey Konovalov Print a warning if V4L2_CID_LINK_FREQ control is not implemented. Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> --- drivers/media/v4l2-core/v4l2-common.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c index 133d20e40f82..f1abdf2ab4ec 100644 --- a/drivers/media/v4l2-core/v4l2-common.c +++ b/drivers/media/v4l2-core/v4l2-common.c @@ -461,6 +461,8 @@ s64 v4l2_get_link_freq(struct v4l2_ctrl_handler *handler, unsigned int mul, freq = qm.value; } else { + pr_warn("%s: V4L2_CID_LINK_FREQ not implemented\n", __func__); + if (!mul || !div) return -ENOENT; -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] v4l: common: v4l2_get_link_freq: add printing a warning 2021-02-17 22:11 ` [PATCH v2 1/3] v4l: common: v4l2_get_link_freq: add printing a warning Andrey Konovalov @ 2021-02-18 7:55 ` Jacopo Mondi 2021-02-18 17:14 ` Andrey Konovalov 0 siblings, 1 reply; 9+ messages in thread From: Jacopo Mondi @ 2021-02-18 7:55 UTC (permalink / raw) To: Andrey Konovalov Cc: junak.pub, robert.foss, sakari.ailus, todor.too, agross, bjorn.andersson, mchehab, laurent.pinchart, linux-media, linux-arm-msm, linux-kernel Hi Andrey, On Thu, Feb 18, 2021 at 01:11:32AM +0300, Andrey Konovalov wrote: > Print a warning if V4L2_CID_LINK_FREQ control is not implemented. > > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> > --- > drivers/media/v4l2-core/v4l2-common.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c > index 133d20e40f82..f1abdf2ab4ec 100644 > --- a/drivers/media/v4l2-core/v4l2-common.c > +++ b/drivers/media/v4l2-core/v4l2-common.c > @@ -461,6 +461,8 @@ s64 v4l2_get_link_freq(struct v4l2_ctrl_handler *handler, unsigned int mul, > > freq = qm.value; > } else { > + pr_warn("%s: V4L2_CID_LINK_FREQ not implemented\n", __func__); > + It's a shame we can't access a struct device * somehow :( Also, nitpicking (please bear with me here) it is absolutely correct that V4L2_CID_LINK_FREQ is not implemented, but I think the real deal here is that the link rate is estimanted from PIXEL_RATE and that might be wrong. What about (insipired from the error message in match_fwnode() which I find useful) pr_warn("%s: Link frequency estimanted using pixel rate: result might be inaccurate\n", __func__); pr_warn("%s: Consider implementing support for V4L2_CID_LINK_FREQ in the transmitter driver\n", __func___); Anyway, whatever works Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > if (!mul || !div) > return -ENOENT; > > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] v4l: common: v4l2_get_link_freq: add printing a warning 2021-02-18 7:55 ` Jacopo Mondi @ 2021-02-18 17:14 ` Andrey Konovalov 0 siblings, 0 replies; 9+ messages in thread From: Andrey Konovalov @ 2021-02-18 17:14 UTC (permalink / raw) To: Jacopo Mondi Cc: junak.pub, robert.foss, sakari.ailus, todor.too, agross, bjorn.andersson, mchehab, laurent.pinchart, linux-media, linux-arm-msm, linux-kernel Hi Jacopo, On 18.02.2021 10:55, Jacopo Mondi wrote: > Hi Andrey, > > On Thu, Feb 18, 2021 at 01:11:32AM +0300, Andrey Konovalov wrote: >> Print a warning if V4L2_CID_LINK_FREQ control is not implemented. >> >> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> >> --- >> drivers/media/v4l2-core/v4l2-common.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c >> index 133d20e40f82..f1abdf2ab4ec 100644 >> --- a/drivers/media/v4l2-core/v4l2-common.c >> +++ b/drivers/media/v4l2-core/v4l2-common.c >> @@ -461,6 +461,8 @@ s64 v4l2_get_link_freq(struct v4l2_ctrl_handler *handler, unsigned int mul, >> >> freq = qm.value; >> } else { >> + pr_warn("%s: V4L2_CID_LINK_FREQ not implemented\n", __func__); >> + > > It's a shame we can't access a struct device * somehow :( Right. Otherwise the message could be even more useful. > Also, nitpicking (please bear with me here) it is absolutely correct > that V4L2_CID_LINK_FREQ is not implemented, but I think the real deal > here is that the link rate is estimanted from PIXEL_RATE and that > might be wrong. Correct. > What about (insipired from the error message in match_fwnode() which I > find useful) > > pr_warn("%s: Link frequency estimanted using pixel rate: result might be inaccurate\n", > __func__); > pr_warn("%s: Consider implementing support for V4L2_CID_LINK_FREQ in the transmitter driver\n", > __func___); This text is fine for me. Just the both of these pr_warn's wouldn't fit into the 80 characters limit. I tried to think out something more descriptive within this limit, but gave up soon. Let me post the v2.1 of this single patch with the longer warning message. Then the maintainers could probably merge whatever they prefer. Or I could post the v3 of the whole patchset if the [PATCH v2.1 1/3] gets positive feedback. Thanks, Andrey > Anyway, whatever works > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Thanks > j > >> if (!mul || !div) >> return -ENOENT; >> >> -- >> 2.17.1 >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 2/3] media: camss: use v4l2_get_link_freq() to calculate the relevant clocks 2021-02-17 22:11 [PATCH v2 0/3] media: qcom: camss: V4L2_CID_PIXEL_RATE/LINK_FREQ fixes Andrey Konovalov 2021-02-17 22:11 ` [PATCH v2 1/3] v4l: common: v4l2_get_link_freq: add printing a warning Andrey Konovalov @ 2021-02-17 22:11 ` Andrey Konovalov 2021-02-18 8:07 ` Jacopo Mondi 2021-02-17 22:11 ` [PATCH v2 3/3] media: qcom: camss: Fix overflows in clock rate calculations Andrey Konovalov 2 siblings, 1 reply; 9+ messages in thread From: Andrey Konovalov @ 2021-02-17 22:11 UTC (permalink / raw) To: junak.pub, robert.foss, sakari.ailus Cc: todor.too, agross, bjorn.andersson, mchehab, laurent.pinchart, jacopo, linux-media, linux-arm-msm, linux-kernel, Andrey Konovalov There are places in the camss driver where camss_get_pixel_clock() is called to get the pixel rate (using V4L2_CID_PIXEL_RATE control) and to calculate the link frequency from it. There is a case when this would not work: when V4L2_CID_PIXEL_RATE gets the rate at which the pixels are read (sampled) from the sensor's pixel array, and this rate is different from the pixel transmission rate over the CSI link, the link frequency value can't be calculated from the pixel rate. One needs to use V4L2_CID_LINK_FREQ to get the link frequency in this case. Replace such calls to camss_get_pixel_clock() with calls to a wrapper around v4l2_get_link_freq(). v4l2_get_link_freq() tries V4L2_CID_LINK_FREQ first, and if it is not implemented by the camera sensor driver, falls back to V4L2_CID_PIXEL_RATE to calculate the link frequency value from. Calls to camss_get_pixel_clock() from vfe_[check,set]_clock_rates() are left intact as it looks like this VFE clock does depend on the rate the pixel samples comes out of the camera sensor, not on the frequency at which the link between the sensor and the CSI receiver operates. Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> Acked-by: Robert Foss <robert.foss@linaro.org> --- .../media/platform/qcom/camss/camss-csid.c | 20 +++++------ .../qcom/camss/camss-csiphy-2ph-1-0.c | 22 ++++++------ .../qcom/camss/camss-csiphy-3ph-1-0.c | 22 ++++++------ .../media/platform/qcom/camss/camss-csiphy.c | 36 +++++++++---------- .../media/platform/qcom/camss/camss-csiphy.h | 2 +- drivers/media/platform/qcom/camss/camss.c | 23 ++++++++++++ drivers/media/platform/qcom/camss/camss.h | 2 ++ 7 files changed, 71 insertions(+), 56 deletions(-) diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c index be3fe76f3dc3..cff9759c9158 100644 --- a/drivers/media/platform/qcom/camss/camss-csid.c +++ b/drivers/media/platform/qcom/camss/camss-csid.c @@ -462,13 +462,17 @@ static irqreturn_t csid_isr(int irq, void *dev) static int csid_set_clock_rates(struct csid_device *csid) { struct device *dev = csid->camss->dev; - u32 pixel_clock; + const struct csid_format *fmt; + s64 link_freq; int i, j; int ret; - ret = camss_get_pixel_clock(&csid->subdev.entity, &pixel_clock); - if (ret) - pixel_clock = 0; + fmt = csid_get_fmt_entry(csid->formats, csid->nformats, + csid->fmt[MSM_CSIPHY_PAD_SINK].code); + link_freq = camss_get_link_freq(&csid->subdev.entity, fmt->bpp, + csid->phy.lane_cnt); + if (link_freq < 0) + link_freq = 0; for (i = 0; i < csid->nclocks; i++) { struct camss_clock *clock = &csid->clock[i]; @@ -477,13 +481,7 @@ static int csid_set_clock_rates(struct csid_device *csid) !strcmp(clock->name, "csi1") || !strcmp(clock->name, "csi2") || !strcmp(clock->name, "csi3")) { - const struct csid_format *f = csid_get_fmt_entry( - csid->formats, - csid->nformats, - csid->fmt[MSM_CSIPHY_PAD_SINK].code); - u8 num_lanes = csid->phy.lane_cnt; - u64 min_rate = pixel_clock * f->bpp / - (2 * num_lanes * 4); + u64 min_rate = link_freq / 4; long rate; camss_add_clock_margin(&min_rate); diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c index 12bce391d71f..30b454c369ab 100644 --- a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c +++ b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c @@ -51,16 +51,13 @@ static void csiphy_reset(struct csiphy_device *csiphy) * * Helper function to calculate settle count value. This is * based on the CSI2 T_hs_settle parameter which in turn - * is calculated based on the CSI2 transmitter pixel clock - * frequency. + * is calculated based on the CSI2 transmitter link frequency. * - * Return settle count value or 0 if the CSI2 pixel clock - * frequency is not available + * Return settle count value or 0 if the CSI2 link frequency + * is not available */ -static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes, - u32 timer_clk_rate) +static u8 csiphy_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate) { - u32 mipi_clock; /* Hz */ u32 ui; /* ps */ u32 timer_period; /* ps */ u32 t_hs_prepare_max; /* ps */ @@ -68,8 +65,10 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes, u32 t_hs_settle; /* ps */ u8 settle_cnt; - mipi_clock = pixel_clock * bpp / (2 * num_lanes); - ui = div_u64(1000000000000LL, mipi_clock); + if (link_freq <= 0) + return 0; + + ui = div_u64(1000000000000LL, link_freq); ui /= 2; t_hs_prepare_max = 85000 + 6 * ui; t_hs_prepare_zero_min = 145000 + 10 * ui; @@ -83,15 +82,14 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes, static void csiphy_lanes_enable(struct csiphy_device *csiphy, struct csiphy_config *cfg, - u32 pixel_clock, u8 bpp, u8 lane_mask) + s64 link_freq, u8 lane_mask) { struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg; u8 settle_cnt; u8 val, l = 0; int i = 0; - settle_cnt = csiphy_settle_cnt_calc(pixel_clock, bpp, c->num_data, - csiphy->timer_clk_rate); + settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate); writel_relaxed(0x1, csiphy->base + CAMSS_CSI_PHY_GLBL_T_INIT_CFG0); diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c index 97cb9de85031..da7c3d3f9a10 100644 --- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c +++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c @@ -107,24 +107,23 @@ static irqreturn_t csiphy_isr(int irq, void *dev) * * Helper function to calculate settle count value. This is * based on the CSI2 T_hs_settle parameter which in turn - * is calculated based on the CSI2 transmitter pixel clock - * frequency. + * is calculated based on the CSI2 transmitter link frequency. * - * Return settle count value or 0 if the CSI2 pixel clock - * frequency is not available + * Return settle count value or 0 if the CSI2 link frequency + * is not available */ -static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes, - u32 timer_clk_rate) +static u8 csiphy_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate) { - u32 mipi_clock; /* Hz */ u32 ui; /* ps */ u32 timer_period; /* ps */ u32 t_hs_prepare_max; /* ps */ u32 t_hs_settle; /* ps */ u8 settle_cnt; - mipi_clock = pixel_clock * bpp / (2 * num_lanes); - ui = div_u64(1000000000000LL, mipi_clock); + if (link_freq <= 0) + return 0; + + ui = div_u64(1000000000000LL, link_freq); ui /= 2; t_hs_prepare_max = 85000 + 6 * ui; t_hs_settle = t_hs_prepare_max; @@ -137,15 +136,14 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes, static void csiphy_lanes_enable(struct csiphy_device *csiphy, struct csiphy_config *cfg, - u32 pixel_clock, u8 bpp, u8 lane_mask) + s64 link_freq, u8 lane_mask) { struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg; u8 settle_cnt; u8 val, l = 0; int i; - settle_cnt = csiphy_settle_cnt_calc(pixel_clock, bpp, c->num_data, - csiphy->timer_clk_rate); + settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate); val = BIT(c->clk.pos); for (i = 0; i < c->num_data; i++) diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c index 509c9a59c09c..40384d7ca78c 100644 --- a/drivers/media/platform/qcom/camss/camss-csiphy.c +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c @@ -102,23 +102,23 @@ static u8 csiphy_get_bpp(const struct csiphy_format *formats, static int csiphy_set_clock_rates(struct csiphy_device *csiphy) { struct device *dev = csiphy->camss->dev; - u32 pixel_clock; + s64 link_freq; int i, j; int ret; - ret = camss_get_pixel_clock(&csiphy->subdev.entity, &pixel_clock); - if (ret) - pixel_clock = 0; + u8 bpp = csiphy_get_bpp(csiphy->formats, csiphy->nformats, + csiphy->fmt[MSM_CSIPHY_PAD_SINK].code); + u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data; + + link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp, num_lanes); + if (link_freq < 0) + link_freq = 0; for (i = 0; i < csiphy->nclocks; i++) { struct camss_clock *clock = &csiphy->clock[i]; if (csiphy->rate_set[i]) { - u8 bpp = csiphy_get_bpp(csiphy->formats, - csiphy->nformats, - csiphy->fmt[MSM_CSIPHY_PAD_SINK].code); - u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data; - u64 min_rate = pixel_clock * bpp / (2 * num_lanes * 4); + u64 min_rate = link_freq / 4; long round_rate; camss_add_clock_margin(&min_rate); @@ -238,22 +238,18 @@ static u8 csiphy_get_lane_mask(struct csiphy_lanes_cfg *lane_cfg) static int csiphy_stream_on(struct csiphy_device *csiphy) { struct csiphy_config *cfg = &csiphy->cfg; - u32 pixel_clock; + s64 link_freq; u8 lane_mask = csiphy_get_lane_mask(&cfg->csi2->lane_cfg); u8 bpp = csiphy_get_bpp(csiphy->formats, csiphy->nformats, csiphy->fmt[MSM_CSIPHY_PAD_SINK].code); + u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data; u8 val; - int ret; - ret = camss_get_pixel_clock(&csiphy->subdev.entity, &pixel_clock); - if (ret) { - dev_err(csiphy->camss->dev, - "Cannot get CSI2 transmitter's pixel clock\n"); - return -EINVAL; - } - if (!pixel_clock) { + link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp, num_lanes); + + if (link_freq < 0) { dev_err(csiphy->camss->dev, - "Got pixel clock == 0, cannot continue\n"); + "Cannot get CSI2 transmitter's link frequency\n"); return -EINVAL; } @@ -268,7 +264,7 @@ static int csiphy_stream_on(struct csiphy_device *csiphy) writel_relaxed(val, csiphy->base_clk_mux); wmb(); - csiphy->ops->lanes_enable(csiphy, cfg, pixel_clock, bpp, lane_mask); + csiphy->ops->lanes_enable(csiphy, cfg, link_freq, lane_mask); return 0; } diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h index f7967ef836dc..d71b8bc6ec00 100644 --- a/drivers/media/platform/qcom/camss/camss-csiphy.h +++ b/drivers/media/platform/qcom/camss/camss-csiphy.h @@ -50,7 +50,7 @@ struct csiphy_hw_ops { void (*reset)(struct csiphy_device *csiphy); void (*lanes_enable)(struct csiphy_device *csiphy, struct csiphy_config *cfg, - u32 pixel_clock, u8 bpp, u8 lane_mask); + s64 link_freq, u8 lane_mask); void (*lanes_disable)(struct csiphy_device *csiphy, struct csiphy_config *cfg); irqreturn_t (*isr)(int irq, void *dev); diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c index 7c0f669f8aa6..eb8fb8c34acd 100644 --- a/drivers/media/platform/qcom/camss/camss.c +++ b/drivers/media/platform/qcom/camss/camss.c @@ -548,6 +548,29 @@ struct media_entity *camss_find_sensor(struct media_entity *entity) } } +/** + * camss_get_link_freq - Get link frequency from sensor + * @entity: Media entity in the current pipeline + * @bpp: Number of bits per pixel for the current format + * @lanes: Number of lanes in the link to the sensor + * + * Return link frequency on success or a negative error code otherwise + */ +s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp, + unsigned int lanes) +{ + struct media_entity *sensor; + struct v4l2_subdev *subdev; + + sensor = camss_find_sensor(entity); + if (!sensor) + return -ENODEV; + + subdev = media_entity_to_v4l2_subdev(sensor); + + return v4l2_get_link_freq(subdev->ctrl_handler, bpp, 2 * lanes); +} + /* * camss_get_pixel_clock - Get pixel clock rate from sensor * @entity: Media entity in the current pipeline diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h index 3a0484683cd6..86cdc25189eb 100644 --- a/drivers/media/platform/qcom/camss/camss.h +++ b/drivers/media/platform/qcom/camss/camss.h @@ -108,6 +108,8 @@ int camss_enable_clocks(int nclocks, struct camss_clock *clock, struct device *dev); void camss_disable_clocks(int nclocks, struct camss_clock *clock); struct media_entity *camss_find_sensor(struct media_entity *entity); +s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp, + unsigned int lanes); int camss_get_pixel_clock(struct media_entity *entity, u32 *pixel_clock); int camss_pm_domain_on(struct camss *camss, int id); void camss_pm_domain_off(struct camss *camss, int id); -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] media: camss: use v4l2_get_link_freq() to calculate the relevant clocks 2021-02-17 22:11 ` [PATCH v2 2/3] media: camss: use v4l2_get_link_freq() to calculate the relevant clocks Andrey Konovalov @ 2021-02-18 8:07 ` Jacopo Mondi 2021-02-18 9:45 ` Andrey Konovalov 0 siblings, 1 reply; 9+ messages in thread From: Jacopo Mondi @ 2021-02-18 8:07 UTC (permalink / raw) To: Andrey Konovalov Cc: junak.pub, robert.foss, sakari.ailus, todor.too, agross, bjorn.andersson, mchehab, laurent.pinchart, linux-media, linux-arm-msm, linux-kernel Hi Andrey, On Thu, Feb 18, 2021 at 01:11:33AM +0300, Andrey Konovalov wrote: > There are places in the camss driver where camss_get_pixel_clock() is > called to get the pixel rate (using V4L2_CID_PIXEL_RATE control) and to > calculate the link frequency from it. There is a case when this would > not work: when V4L2_CID_PIXEL_RATE gets the rate at which the pixels are > read (sampled) from the sensor's pixel array, and this rate is different > from the pixel transmission rate over the CSI link, the link frequency > value can't be calculated from the pixel rate. One needs to use > V4L2_CID_LINK_FREQ to get the link frequency in this case. > > Replace such calls to camss_get_pixel_clock() with calls to a wrapper > around v4l2_get_link_freq(). v4l2_get_link_freq() tries V4L2_CID_LINK_FREQ > first, and if it is not implemented by the camera sensor driver, falls > back to V4L2_CID_PIXEL_RATE to calculate the link frequency value from. That's nice > > Calls to camss_get_pixel_clock() from vfe_[check,set]_clock_rates() > are left intact as it looks like this VFE clock does depend on the > rate the pixel samples comes out of the camera sensor, not on the > frequency at which the link between the sensor and the CSI receiver > operates. Out of curiosity, you have any idea why this happens ? The patch looks good, fwiw given I don't know the platform: Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> > Acked-by: Robert Foss <robert.foss@linaro.org> > --- > .../media/platform/qcom/camss/camss-csid.c | 20 +++++------ > .../qcom/camss/camss-csiphy-2ph-1-0.c | 22 ++++++------ > .../qcom/camss/camss-csiphy-3ph-1-0.c | 22 ++++++------ > .../media/platform/qcom/camss/camss-csiphy.c | 36 +++++++++---------- > .../media/platform/qcom/camss/camss-csiphy.h | 2 +- > drivers/media/platform/qcom/camss/camss.c | 23 ++++++++++++ > drivers/media/platform/qcom/camss/camss.h | 2 ++ > 7 files changed, 71 insertions(+), 56 deletions(-) > > diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c > index be3fe76f3dc3..cff9759c9158 100644 > --- a/drivers/media/platform/qcom/camss/camss-csid.c > +++ b/drivers/media/platform/qcom/camss/camss-csid.c > @@ -462,13 +462,17 @@ static irqreturn_t csid_isr(int irq, void *dev) > static int csid_set_clock_rates(struct csid_device *csid) > { > struct device *dev = csid->camss->dev; > - u32 pixel_clock; > + const struct csid_format *fmt; > + s64 link_freq; > int i, j; > int ret; > > - ret = camss_get_pixel_clock(&csid->subdev.entity, &pixel_clock); > - if (ret) > - pixel_clock = 0; > + fmt = csid_get_fmt_entry(csid->formats, csid->nformats, > + csid->fmt[MSM_CSIPHY_PAD_SINK].code); > + link_freq = camss_get_link_freq(&csid->subdev.entity, fmt->bpp, > + csid->phy.lane_cnt); > + if (link_freq < 0) > + link_freq = 0; > > for (i = 0; i < csid->nclocks; i++) { > struct camss_clock *clock = &csid->clock[i]; > @@ -477,13 +481,7 @@ static int csid_set_clock_rates(struct csid_device *csid) > !strcmp(clock->name, "csi1") || > !strcmp(clock->name, "csi2") || > !strcmp(clock->name, "csi3")) { > - const struct csid_format *f = csid_get_fmt_entry( > - csid->formats, > - csid->nformats, > - csid->fmt[MSM_CSIPHY_PAD_SINK].code); > - u8 num_lanes = csid->phy.lane_cnt; > - u64 min_rate = pixel_clock * f->bpp / > - (2 * num_lanes * 4); > + u64 min_rate = link_freq / 4; > long rate; > > camss_add_clock_margin(&min_rate); > diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c > index 12bce391d71f..30b454c369ab 100644 > --- a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c > +++ b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c > @@ -51,16 +51,13 @@ static void csiphy_reset(struct csiphy_device *csiphy) > * > * Helper function to calculate settle count value. This is > * based on the CSI2 T_hs_settle parameter which in turn > - * is calculated based on the CSI2 transmitter pixel clock > - * frequency. > + * is calculated based on the CSI2 transmitter link frequency. > * > - * Return settle count value or 0 if the CSI2 pixel clock > - * frequency is not available > + * Return settle count value or 0 if the CSI2 link frequency > + * is not available > */ > -static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes, > - u32 timer_clk_rate) > +static u8 csiphy_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate) > { > - u32 mipi_clock; /* Hz */ > u32 ui; /* ps */ > u32 timer_period; /* ps */ > u32 t_hs_prepare_max; /* ps */ > @@ -68,8 +65,10 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes, > u32 t_hs_settle; /* ps */ > u8 settle_cnt; > > - mipi_clock = pixel_clock * bpp / (2 * num_lanes); > - ui = div_u64(1000000000000LL, mipi_clock); > + if (link_freq <= 0) > + return 0; > + > + ui = div_u64(1000000000000LL, link_freq); > ui /= 2; > t_hs_prepare_max = 85000 + 6 * ui; > t_hs_prepare_zero_min = 145000 + 10 * ui; > @@ -83,15 +82,14 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes, > > static void csiphy_lanes_enable(struct csiphy_device *csiphy, > struct csiphy_config *cfg, > - u32 pixel_clock, u8 bpp, u8 lane_mask) > + s64 link_freq, u8 lane_mask) > { > struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg; > u8 settle_cnt; > u8 val, l = 0; > int i = 0; > > - settle_cnt = csiphy_settle_cnt_calc(pixel_clock, bpp, c->num_data, > - csiphy->timer_clk_rate); > + settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate); > > writel_relaxed(0x1, csiphy->base + > CAMSS_CSI_PHY_GLBL_T_INIT_CFG0); > diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c > index 97cb9de85031..da7c3d3f9a10 100644 > --- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c > +++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c > @@ -107,24 +107,23 @@ static irqreturn_t csiphy_isr(int irq, void *dev) > * > * Helper function to calculate settle count value. This is > * based on the CSI2 T_hs_settle parameter which in turn > - * is calculated based on the CSI2 transmitter pixel clock > - * frequency. > + * is calculated based on the CSI2 transmitter link frequency. > * > - * Return settle count value or 0 if the CSI2 pixel clock > - * frequency is not available > + * Return settle count value or 0 if the CSI2 link frequency > + * is not available > */ > -static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes, > - u32 timer_clk_rate) > +static u8 csiphy_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate) > { > - u32 mipi_clock; /* Hz */ > u32 ui; /* ps */ > u32 timer_period; /* ps */ > u32 t_hs_prepare_max; /* ps */ > u32 t_hs_settle; /* ps */ > u8 settle_cnt; > > - mipi_clock = pixel_clock * bpp / (2 * num_lanes); > - ui = div_u64(1000000000000LL, mipi_clock); > + if (link_freq <= 0) > + return 0; > + > + ui = div_u64(1000000000000LL, link_freq); > ui /= 2; > t_hs_prepare_max = 85000 + 6 * ui; > t_hs_settle = t_hs_prepare_max; > @@ -137,15 +136,14 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes, > > static void csiphy_lanes_enable(struct csiphy_device *csiphy, > struct csiphy_config *cfg, > - u32 pixel_clock, u8 bpp, u8 lane_mask) > + s64 link_freq, u8 lane_mask) > { > struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg; > u8 settle_cnt; > u8 val, l = 0; > int i; > > - settle_cnt = csiphy_settle_cnt_calc(pixel_clock, bpp, c->num_data, > - csiphy->timer_clk_rate); > + settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate); > > val = BIT(c->clk.pos); > for (i = 0; i < c->num_data; i++) > diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c > index 509c9a59c09c..40384d7ca78c 100644 > --- a/drivers/media/platform/qcom/camss/camss-csiphy.c > +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c > @@ -102,23 +102,23 @@ static u8 csiphy_get_bpp(const struct csiphy_format *formats, > static int csiphy_set_clock_rates(struct csiphy_device *csiphy) > { > struct device *dev = csiphy->camss->dev; > - u32 pixel_clock; > + s64 link_freq; > int i, j; > int ret; > > - ret = camss_get_pixel_clock(&csiphy->subdev.entity, &pixel_clock); > - if (ret) > - pixel_clock = 0; > + u8 bpp = csiphy_get_bpp(csiphy->formats, csiphy->nformats, > + csiphy->fmt[MSM_CSIPHY_PAD_SINK].code); > + u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data; > + > + link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp, num_lanes); > + if (link_freq < 0) > + link_freq = 0; > > for (i = 0; i < csiphy->nclocks; i++) { > struct camss_clock *clock = &csiphy->clock[i]; > > if (csiphy->rate_set[i]) { > - u8 bpp = csiphy_get_bpp(csiphy->formats, > - csiphy->nformats, > - csiphy->fmt[MSM_CSIPHY_PAD_SINK].code); > - u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data; > - u64 min_rate = pixel_clock * bpp / (2 * num_lanes * 4); > + u64 min_rate = link_freq / 4; > long round_rate; > > camss_add_clock_margin(&min_rate); > @@ -238,22 +238,18 @@ static u8 csiphy_get_lane_mask(struct csiphy_lanes_cfg *lane_cfg) > static int csiphy_stream_on(struct csiphy_device *csiphy) > { > struct csiphy_config *cfg = &csiphy->cfg; > - u32 pixel_clock; > + s64 link_freq; > u8 lane_mask = csiphy_get_lane_mask(&cfg->csi2->lane_cfg); > u8 bpp = csiphy_get_bpp(csiphy->formats, csiphy->nformats, > csiphy->fmt[MSM_CSIPHY_PAD_SINK].code); > + u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data; > u8 val; > - int ret; > > - ret = camss_get_pixel_clock(&csiphy->subdev.entity, &pixel_clock); > - if (ret) { > - dev_err(csiphy->camss->dev, > - "Cannot get CSI2 transmitter's pixel clock\n"); > - return -EINVAL; > - } > - if (!pixel_clock) { > + link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp, num_lanes); > + > + if (link_freq < 0) { > dev_err(csiphy->camss->dev, > - "Got pixel clock == 0, cannot continue\n"); > + "Cannot get CSI2 transmitter's link frequency\n"); > return -EINVAL; > } > > @@ -268,7 +264,7 @@ static int csiphy_stream_on(struct csiphy_device *csiphy) > writel_relaxed(val, csiphy->base_clk_mux); > wmb(); > > - csiphy->ops->lanes_enable(csiphy, cfg, pixel_clock, bpp, lane_mask); > + csiphy->ops->lanes_enable(csiphy, cfg, link_freq, lane_mask); > > return 0; > } > diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h > index f7967ef836dc..d71b8bc6ec00 100644 > --- a/drivers/media/platform/qcom/camss/camss-csiphy.h > +++ b/drivers/media/platform/qcom/camss/camss-csiphy.h > @@ -50,7 +50,7 @@ struct csiphy_hw_ops { > void (*reset)(struct csiphy_device *csiphy); > void (*lanes_enable)(struct csiphy_device *csiphy, > struct csiphy_config *cfg, > - u32 pixel_clock, u8 bpp, u8 lane_mask); > + s64 link_freq, u8 lane_mask); > void (*lanes_disable)(struct csiphy_device *csiphy, > struct csiphy_config *cfg); > irqreturn_t (*isr)(int irq, void *dev); > diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c > index 7c0f669f8aa6..eb8fb8c34acd 100644 > --- a/drivers/media/platform/qcom/camss/camss.c > +++ b/drivers/media/platform/qcom/camss/camss.c > @@ -548,6 +548,29 @@ struct media_entity *camss_find_sensor(struct media_entity *entity) > } > } > > +/** > + * camss_get_link_freq - Get link frequency from sensor > + * @entity: Media entity in the current pipeline > + * @bpp: Number of bits per pixel for the current format > + * @lanes: Number of lanes in the link to the sensor > + * > + * Return link frequency on success or a negative error code otherwise > + */ > +s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp, > + unsigned int lanes) > +{ > + struct media_entity *sensor; > + struct v4l2_subdev *subdev; > + > + sensor = camss_find_sensor(entity); > + if (!sensor) > + return -ENODEV; > + > + subdev = media_entity_to_v4l2_subdev(sensor); > + > + return v4l2_get_link_freq(subdev->ctrl_handler, bpp, 2 * lanes); > +} > + > /* > * camss_get_pixel_clock - Get pixel clock rate from sensor > * @entity: Media entity in the current pipeline > diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h > index 3a0484683cd6..86cdc25189eb 100644 > --- a/drivers/media/platform/qcom/camss/camss.h > +++ b/drivers/media/platform/qcom/camss/camss.h > @@ -108,6 +108,8 @@ int camss_enable_clocks(int nclocks, struct camss_clock *clock, > struct device *dev); > void camss_disable_clocks(int nclocks, struct camss_clock *clock); > struct media_entity *camss_find_sensor(struct media_entity *entity); > +s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp, > + unsigned int lanes); > int camss_get_pixel_clock(struct media_entity *entity, u32 *pixel_clock); > int camss_pm_domain_on(struct camss *camss, int id); > void camss_pm_domain_off(struct camss *camss, int id); > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] media: camss: use v4l2_get_link_freq() to calculate the relevant clocks 2021-02-18 8:07 ` Jacopo Mondi @ 2021-02-18 9:45 ` Andrey Konovalov 0 siblings, 0 replies; 9+ messages in thread From: Andrey Konovalov @ 2021-02-18 9:45 UTC (permalink / raw) To: Jacopo Mondi Cc: junak.pub, robert.foss, sakari.ailus, todor.too, agross, bjorn.andersson, mchehab, laurent.pinchart, linux-media, linux-arm-msm, linux-kernel Hi Jacopo, On 18.02.2021 11:07, Jacopo Mondi wrote: > Hi Andrey, > > On Thu, Feb 18, 2021 at 01:11:33AM +0300, Andrey Konovalov wrote: >> There are places in the camss driver where camss_get_pixel_clock() is >> called to get the pixel rate (using V4L2_CID_PIXEL_RATE control) and to >> calculate the link frequency from it. There is a case when this would >> not work: when V4L2_CID_PIXEL_RATE gets the rate at which the pixels are >> read (sampled) from the sensor's pixel array, and this rate is different >> from the pixel transmission rate over the CSI link, the link frequency >> value can't be calculated from the pixel rate. One needs to use >> V4L2_CID_LINK_FREQ to get the link frequency in this case. >> >> Replace such calls to camss_get_pixel_clock() with calls to a wrapper >> around v4l2_get_link_freq(). v4l2_get_link_freq() tries V4L2_CID_LINK_FREQ >> first, and if it is not implemented by the camera sensor driver, falls >> back to V4L2_CID_PIXEL_RATE to calculate the link frequency value from. > > That's nice > >> >> Calls to camss_get_pixel_clock() from vfe_[check,set]_clock_rates() >> are left intact as it looks like this VFE clock does depend on the >> rate the pixel samples comes out of the camera sensor, not on the >> frequency at which the link between the sensor and the CSI receiver >> operates. > > Out of curiosity, you have any idea why this happens ? Unfortunately no. There are no any details in the public datasheet on that - just the formula itself (less than 1 line of text). > The patch looks good, fwiw given I don't know the platform: > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks! -- Andrey > Thanks > j >> >> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> >> Acked-by: Robert Foss <robert.foss@linaro.org> >> --- >> .../media/platform/qcom/camss/camss-csid.c | 20 +++++------ >> .../qcom/camss/camss-csiphy-2ph-1-0.c | 22 ++++++------ >> .../qcom/camss/camss-csiphy-3ph-1-0.c | 22 ++++++------ >> .../media/platform/qcom/camss/camss-csiphy.c | 36 +++++++++---------- >> .../media/platform/qcom/camss/camss-csiphy.h | 2 +- >> drivers/media/platform/qcom/camss/camss.c | 23 ++++++++++++ >> drivers/media/platform/qcom/camss/camss.h | 2 ++ >> 7 files changed, 71 insertions(+), 56 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c >> index be3fe76f3dc3..cff9759c9158 100644 >> --- a/drivers/media/platform/qcom/camss/camss-csid.c >> +++ b/drivers/media/platform/qcom/camss/camss-csid.c >> @@ -462,13 +462,17 @@ static irqreturn_t csid_isr(int irq, void *dev) >> static int csid_set_clock_rates(struct csid_device *csid) >> { >> struct device *dev = csid->camss->dev; >> - u32 pixel_clock; >> + const struct csid_format *fmt; >> + s64 link_freq; >> int i, j; >> int ret; >> >> - ret = camss_get_pixel_clock(&csid->subdev.entity, &pixel_clock); >> - if (ret) >> - pixel_clock = 0; >> + fmt = csid_get_fmt_entry(csid->formats, csid->nformats, >> + csid->fmt[MSM_CSIPHY_PAD_SINK].code); >> + link_freq = camss_get_link_freq(&csid->subdev.entity, fmt->bpp, >> + csid->phy.lane_cnt); >> + if (link_freq < 0) >> + link_freq = 0; >> >> for (i = 0; i < csid->nclocks; i++) { >> struct camss_clock *clock = &csid->clock[i]; >> @@ -477,13 +481,7 @@ static int csid_set_clock_rates(struct csid_device *csid) >> !strcmp(clock->name, "csi1") || >> !strcmp(clock->name, "csi2") || >> !strcmp(clock->name, "csi3")) { >> - const struct csid_format *f = csid_get_fmt_entry( >> - csid->formats, >> - csid->nformats, >> - csid->fmt[MSM_CSIPHY_PAD_SINK].code); >> - u8 num_lanes = csid->phy.lane_cnt; >> - u64 min_rate = pixel_clock * f->bpp / >> - (2 * num_lanes * 4); >> + u64 min_rate = link_freq / 4; >> long rate; >> >> camss_add_clock_margin(&min_rate); >> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c >> index 12bce391d71f..30b454c369ab 100644 >> --- a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c >> +++ b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c >> @@ -51,16 +51,13 @@ static void csiphy_reset(struct csiphy_device *csiphy) >> * >> * Helper function to calculate settle count value. This is >> * based on the CSI2 T_hs_settle parameter which in turn >> - * is calculated based on the CSI2 transmitter pixel clock >> - * frequency. >> + * is calculated based on the CSI2 transmitter link frequency. >> * >> - * Return settle count value or 0 if the CSI2 pixel clock >> - * frequency is not available >> + * Return settle count value or 0 if the CSI2 link frequency >> + * is not available >> */ >> -static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes, >> - u32 timer_clk_rate) >> +static u8 csiphy_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate) >> { >> - u32 mipi_clock; /* Hz */ >> u32 ui; /* ps */ >> u32 timer_period; /* ps */ >> u32 t_hs_prepare_max; /* ps */ >> @@ -68,8 +65,10 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes, >> u32 t_hs_settle; /* ps */ >> u8 settle_cnt; >> >> - mipi_clock = pixel_clock * bpp / (2 * num_lanes); >> - ui = div_u64(1000000000000LL, mipi_clock); >> + if (link_freq <= 0) >> + return 0; >> + >> + ui = div_u64(1000000000000LL, link_freq); >> ui /= 2; >> t_hs_prepare_max = 85000 + 6 * ui; >> t_hs_prepare_zero_min = 145000 + 10 * ui; >> @@ -83,15 +82,14 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes, >> >> static void csiphy_lanes_enable(struct csiphy_device *csiphy, >> struct csiphy_config *cfg, >> - u32 pixel_clock, u8 bpp, u8 lane_mask) >> + s64 link_freq, u8 lane_mask) >> { >> struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg; >> u8 settle_cnt; >> u8 val, l = 0; >> int i = 0; >> >> - settle_cnt = csiphy_settle_cnt_calc(pixel_clock, bpp, c->num_data, >> - csiphy->timer_clk_rate); >> + settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate); >> >> writel_relaxed(0x1, csiphy->base + >> CAMSS_CSI_PHY_GLBL_T_INIT_CFG0); >> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c >> index 97cb9de85031..da7c3d3f9a10 100644 >> --- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c >> +++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c >> @@ -107,24 +107,23 @@ static irqreturn_t csiphy_isr(int irq, void *dev) >> * >> * Helper function to calculate settle count value. This is >> * based on the CSI2 T_hs_settle parameter which in turn >> - * is calculated based on the CSI2 transmitter pixel clock >> - * frequency. >> + * is calculated based on the CSI2 transmitter link frequency. >> * >> - * Return settle count value or 0 if the CSI2 pixel clock >> - * frequency is not available >> + * Return settle count value or 0 if the CSI2 link frequency >> + * is not available >> */ >> -static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes, >> - u32 timer_clk_rate) >> +static u8 csiphy_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate) >> { >> - u32 mipi_clock; /* Hz */ >> u32 ui; /* ps */ >> u32 timer_period; /* ps */ >> u32 t_hs_prepare_max; /* ps */ >> u32 t_hs_settle; /* ps */ >> u8 settle_cnt; >> >> - mipi_clock = pixel_clock * bpp / (2 * num_lanes); >> - ui = div_u64(1000000000000LL, mipi_clock); >> + if (link_freq <= 0) >> + return 0; >> + >> + ui = div_u64(1000000000000LL, link_freq); >> ui /= 2; >> t_hs_prepare_max = 85000 + 6 * ui; >> t_hs_settle = t_hs_prepare_max; >> @@ -137,15 +136,14 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes, >> >> static void csiphy_lanes_enable(struct csiphy_device *csiphy, >> struct csiphy_config *cfg, >> - u32 pixel_clock, u8 bpp, u8 lane_mask) >> + s64 link_freq, u8 lane_mask) >> { >> struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg; >> u8 settle_cnt; >> u8 val, l = 0; >> int i; >> >> - settle_cnt = csiphy_settle_cnt_calc(pixel_clock, bpp, c->num_data, >> - csiphy->timer_clk_rate); >> + settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate); >> >> val = BIT(c->clk.pos); >> for (i = 0; i < c->num_data; i++) >> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c >> index 509c9a59c09c..40384d7ca78c 100644 >> --- a/drivers/media/platform/qcom/camss/camss-csiphy.c >> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c >> @@ -102,23 +102,23 @@ static u8 csiphy_get_bpp(const struct csiphy_format *formats, >> static int csiphy_set_clock_rates(struct csiphy_device *csiphy) >> { >> struct device *dev = csiphy->camss->dev; >> - u32 pixel_clock; >> + s64 link_freq; >> int i, j; >> int ret; >> >> - ret = camss_get_pixel_clock(&csiphy->subdev.entity, &pixel_clock); >> - if (ret) >> - pixel_clock = 0; >> + u8 bpp = csiphy_get_bpp(csiphy->formats, csiphy->nformats, >> + csiphy->fmt[MSM_CSIPHY_PAD_SINK].code); >> + u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data; >> + >> + link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp, num_lanes); >> + if (link_freq < 0) >> + link_freq = 0; >> >> for (i = 0; i < csiphy->nclocks; i++) { >> struct camss_clock *clock = &csiphy->clock[i]; >> >> if (csiphy->rate_set[i]) { >> - u8 bpp = csiphy_get_bpp(csiphy->formats, >> - csiphy->nformats, >> - csiphy->fmt[MSM_CSIPHY_PAD_SINK].code); >> - u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data; >> - u64 min_rate = pixel_clock * bpp / (2 * num_lanes * 4); >> + u64 min_rate = link_freq / 4; >> long round_rate; >> >> camss_add_clock_margin(&min_rate); >> @@ -238,22 +238,18 @@ static u8 csiphy_get_lane_mask(struct csiphy_lanes_cfg *lane_cfg) >> static int csiphy_stream_on(struct csiphy_device *csiphy) >> { >> struct csiphy_config *cfg = &csiphy->cfg; >> - u32 pixel_clock; >> + s64 link_freq; >> u8 lane_mask = csiphy_get_lane_mask(&cfg->csi2->lane_cfg); >> u8 bpp = csiphy_get_bpp(csiphy->formats, csiphy->nformats, >> csiphy->fmt[MSM_CSIPHY_PAD_SINK].code); >> + u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data; >> u8 val; >> - int ret; >> >> - ret = camss_get_pixel_clock(&csiphy->subdev.entity, &pixel_clock); >> - if (ret) { >> - dev_err(csiphy->camss->dev, >> - "Cannot get CSI2 transmitter's pixel clock\n"); >> - return -EINVAL; >> - } >> - if (!pixel_clock) { >> + link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp, num_lanes); >> + >> + if (link_freq < 0) { >> dev_err(csiphy->camss->dev, >> - "Got pixel clock == 0, cannot continue\n"); >> + "Cannot get CSI2 transmitter's link frequency\n"); >> return -EINVAL; >> } >> >> @@ -268,7 +264,7 @@ static int csiphy_stream_on(struct csiphy_device *csiphy) >> writel_relaxed(val, csiphy->base_clk_mux); >> wmb(); >> >> - csiphy->ops->lanes_enable(csiphy, cfg, pixel_clock, bpp, lane_mask); >> + csiphy->ops->lanes_enable(csiphy, cfg, link_freq, lane_mask); >> >> return 0; >> } >> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h >> index f7967ef836dc..d71b8bc6ec00 100644 >> --- a/drivers/media/platform/qcom/camss/camss-csiphy.h >> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.h >> @@ -50,7 +50,7 @@ struct csiphy_hw_ops { >> void (*reset)(struct csiphy_device *csiphy); >> void (*lanes_enable)(struct csiphy_device *csiphy, >> struct csiphy_config *cfg, >> - u32 pixel_clock, u8 bpp, u8 lane_mask); >> + s64 link_freq, u8 lane_mask); >> void (*lanes_disable)(struct csiphy_device *csiphy, >> struct csiphy_config *cfg); >> irqreturn_t (*isr)(int irq, void *dev); >> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c >> index 7c0f669f8aa6..eb8fb8c34acd 100644 >> --- a/drivers/media/platform/qcom/camss/camss.c >> +++ b/drivers/media/platform/qcom/camss/camss.c >> @@ -548,6 +548,29 @@ struct media_entity *camss_find_sensor(struct media_entity *entity) >> } >> } >> >> +/** >> + * camss_get_link_freq - Get link frequency from sensor >> + * @entity: Media entity in the current pipeline >> + * @bpp: Number of bits per pixel for the current format >> + * @lanes: Number of lanes in the link to the sensor >> + * >> + * Return link frequency on success or a negative error code otherwise >> + */ >> +s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp, >> + unsigned int lanes) >> +{ >> + struct media_entity *sensor; >> + struct v4l2_subdev *subdev; >> + >> + sensor = camss_find_sensor(entity); >> + if (!sensor) >> + return -ENODEV; >> + >> + subdev = media_entity_to_v4l2_subdev(sensor); >> + >> + return v4l2_get_link_freq(subdev->ctrl_handler, bpp, 2 * lanes); >> +} >> + >> /* >> * camss_get_pixel_clock - Get pixel clock rate from sensor >> * @entity: Media entity in the current pipeline >> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h >> index 3a0484683cd6..86cdc25189eb 100644 >> --- a/drivers/media/platform/qcom/camss/camss.h >> +++ b/drivers/media/platform/qcom/camss/camss.h >> @@ -108,6 +108,8 @@ int camss_enable_clocks(int nclocks, struct camss_clock *clock, >> struct device *dev); >> void camss_disable_clocks(int nclocks, struct camss_clock *clock); >> struct media_entity *camss_find_sensor(struct media_entity *entity); >> +s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp, >> + unsigned int lanes); >> int camss_get_pixel_clock(struct media_entity *entity, u32 *pixel_clock); >> int camss_pm_domain_on(struct camss *camss, int id); >> void camss_pm_domain_off(struct camss *camss, int id); >> -- >> 2.17.1 >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 3/3] media: qcom: camss: Fix overflows in clock rate calculations 2021-02-17 22:11 [PATCH v2 0/3] media: qcom: camss: V4L2_CID_PIXEL_RATE/LINK_FREQ fixes Andrey Konovalov 2021-02-17 22:11 ` [PATCH v2 1/3] v4l: common: v4l2_get_link_freq: add printing a warning Andrey Konovalov 2021-02-17 22:11 ` [PATCH v2 2/3] media: camss: use v4l2_get_link_freq() to calculate the relevant clocks Andrey Konovalov @ 2021-02-17 22:11 ` Andrey Konovalov 2021-02-18 8:15 ` Jacopo Mondi 2 siblings, 1 reply; 9+ messages in thread From: Andrey Konovalov @ 2021-02-17 22:11 UTC (permalink / raw) To: junak.pub, robert.foss, sakari.ailus Cc: todor.too, agross, bjorn.andersson, mchehab, laurent.pinchart, jacopo, linux-media, linux-arm-msm, linux-kernel, Andrey Konovalov From: Vladimir Lypak <junak.pub@gmail.com> Because of u32 type being used to store pixel clock rate, expression used to calculate pipeline clocks (pixel_clock * bpp) produces wrong value due to integer overflow. This patch changes data type used to store, pass and retrieve pixel_clock from u32 to u64 to make this mistake less likely to be repeated in the future. Signed-off-by: Vladimir Lypak <junak.pub@gmail.com> Acked-by: Robert Foss <robert.foss@linaro.org> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> --- drivers/media/platform/qcom/camss/camss-vfe.c | 4 ++-- drivers/media/platform/qcom/camss/camss.c | 2 +- drivers/media/platform/qcom/camss/camss.h | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c index fae2b513b2f9..b2c95b46ce66 100644 --- a/drivers/media/platform/qcom/camss/camss-vfe.c +++ b/drivers/media/platform/qcom/camss/camss-vfe.c @@ -1112,7 +1112,7 @@ static inline void vfe_isr_halt_ack(struct vfe_device *vfe) static int vfe_set_clock_rates(struct vfe_device *vfe) { struct device *dev = vfe->camss->dev; - u32 pixel_clock[MSM_VFE_LINE_NUM]; + u64 pixel_clock[MSM_VFE_LINE_NUM]; int i, j; int ret; @@ -1194,7 +1194,7 @@ static int vfe_set_clock_rates(struct vfe_device *vfe) */ static int vfe_check_clock_rates(struct vfe_device *vfe) { - u32 pixel_clock[MSM_VFE_LINE_NUM]; + u64 pixel_clock[MSM_VFE_LINE_NUM]; int i, j; int ret; diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c index eb8fb8c34acd..d82bbc2213a6 100644 --- a/drivers/media/platform/qcom/camss/camss.c +++ b/drivers/media/platform/qcom/camss/camss.c @@ -578,7 +578,7 @@ s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp, * * Return 0 on success or a negative error code otherwise */ -int camss_get_pixel_clock(struct media_entity *entity, u32 *pixel_clock) +int camss_get_pixel_clock(struct media_entity *entity, u64 *pixel_clock) { struct media_entity *sensor; struct v4l2_subdev *subdev; diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h index 86cdc25189eb..e29466d07ad2 100644 --- a/drivers/media/platform/qcom/camss/camss.h +++ b/drivers/media/platform/qcom/camss/camss.h @@ -110,7 +110,7 @@ void camss_disable_clocks(int nclocks, struct camss_clock *clock); struct media_entity *camss_find_sensor(struct media_entity *entity); s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp, unsigned int lanes); -int camss_get_pixel_clock(struct media_entity *entity, u32 *pixel_clock); +int camss_get_pixel_clock(struct media_entity *entity, u64 *pixel_clock); int camss_pm_domain_on(struct camss *camss, int id); void camss_pm_domain_off(struct camss *camss, int id); void camss_delete(struct camss *camss); -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 3/3] media: qcom: camss: Fix overflows in clock rate calculations 2021-02-17 22:11 ` [PATCH v2 3/3] media: qcom: camss: Fix overflows in clock rate calculations Andrey Konovalov @ 2021-02-18 8:15 ` Jacopo Mondi 0 siblings, 0 replies; 9+ messages in thread From: Jacopo Mondi @ 2021-02-18 8:15 UTC (permalink / raw) To: Andrey Konovalov Cc: junak.pub, robert.foss, sakari.ailus, todor.too, agross, bjorn.andersson, mchehab, laurent.pinchart, linux-media, linux-arm-msm, linux-kernel Hi Andrey, On Thu, Feb 18, 2021 at 01:11:34AM +0300, Andrey Konovalov wrote: > From: Vladimir Lypak <junak.pub@gmail.com> > > Because of u32 type being used to store pixel clock rate, expression used > to calculate pipeline clocks (pixel_clock * bpp) produces wrong value due > to integer overflow. This patch changes data type used to store, pass and > retrieve pixel_clock from u32 to u64 to make this mistake less likely to > be repeated in the future. I see this might be the cause for the overflow tmp = pixel_clock[j] * bpp / 64; and anyway PIXEL_RATE is an u64 control, so this seems correct Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > > Signed-off-by: Vladimir Lypak <junak.pub@gmail.com> > Acked-by: Robert Foss <robert.foss@linaro.org> > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> > --- > drivers/media/platform/qcom/camss/camss-vfe.c | 4 ++-- > drivers/media/platform/qcom/camss/camss.c | 2 +- > drivers/media/platform/qcom/camss/camss.h | 2 +- > 3 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c > index fae2b513b2f9..b2c95b46ce66 100644 > --- a/drivers/media/platform/qcom/camss/camss-vfe.c > +++ b/drivers/media/platform/qcom/camss/camss-vfe.c > @@ -1112,7 +1112,7 @@ static inline void vfe_isr_halt_ack(struct vfe_device *vfe) > static int vfe_set_clock_rates(struct vfe_device *vfe) > { > struct device *dev = vfe->camss->dev; > - u32 pixel_clock[MSM_VFE_LINE_NUM]; > + u64 pixel_clock[MSM_VFE_LINE_NUM]; > int i, j; > int ret; > > @@ -1194,7 +1194,7 @@ static int vfe_set_clock_rates(struct vfe_device *vfe) > */ > static int vfe_check_clock_rates(struct vfe_device *vfe) > { > - u32 pixel_clock[MSM_VFE_LINE_NUM]; > + u64 pixel_clock[MSM_VFE_LINE_NUM]; > int i, j; > int ret; > > diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c > index eb8fb8c34acd..d82bbc2213a6 100644 > --- a/drivers/media/platform/qcom/camss/camss.c > +++ b/drivers/media/platform/qcom/camss/camss.c > @@ -578,7 +578,7 @@ s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp, > * > * Return 0 on success or a negative error code otherwise > */ > -int camss_get_pixel_clock(struct media_entity *entity, u32 *pixel_clock) > +int camss_get_pixel_clock(struct media_entity *entity, u64 *pixel_clock) > { > struct media_entity *sensor; > struct v4l2_subdev *subdev; > diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h > index 86cdc25189eb..e29466d07ad2 100644 > --- a/drivers/media/platform/qcom/camss/camss.h > +++ b/drivers/media/platform/qcom/camss/camss.h > @@ -110,7 +110,7 @@ void camss_disable_clocks(int nclocks, struct camss_clock *clock); > struct media_entity *camss_find_sensor(struct media_entity *entity); > s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp, > unsigned int lanes); > -int camss_get_pixel_clock(struct media_entity *entity, u32 *pixel_clock); > +int camss_get_pixel_clock(struct media_entity *entity, u64 *pixel_clock); > int camss_pm_domain_on(struct camss *camss, int id); > void camss_pm_domain_off(struct camss *camss, int id); > void camss_delete(struct camss *camss); > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-02-18 18:58 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-17 22:11 [PATCH v2 0/3] media: qcom: camss: V4L2_CID_PIXEL_RATE/LINK_FREQ fixes Andrey Konovalov 2021-02-17 22:11 ` [PATCH v2 1/3] v4l: common: v4l2_get_link_freq: add printing a warning Andrey Konovalov 2021-02-18 7:55 ` Jacopo Mondi 2021-02-18 17:14 ` Andrey Konovalov 2021-02-17 22:11 ` [PATCH v2 2/3] media: camss: use v4l2_get_link_freq() to calculate the relevant clocks Andrey Konovalov 2021-02-18 8:07 ` Jacopo Mondi 2021-02-18 9:45 ` Andrey Konovalov 2021-02-17 22:11 ` [PATCH v2 3/3] media: qcom: camss: Fix overflows in clock rate calculations Andrey Konovalov 2021-02-18 8:15 ` Jacopo Mondi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).