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
From: Vladimir Lypak <[email protected]>
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 <[email protected]>
Acked-by: Robert Foss <[email protected]>
Signed-off-by: Andrey Konovalov <[email protected]>
---
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
Print a warning if V4L2_CID_LINK_FREQ control is not implemented.
Signed-off-by: Andrey Konovalov <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
Thanks
j
> if (!mul || !div)
> return -ENOENT;
>
> --
> 2.17.1
>
Hi Andrey,
On Thu, Feb 18, 2021 at 01:11:34AM +0300, Andrey Konovalov wrote:
> From: Vladimir Lypak <[email protected]>
>
> 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 <[email protected]>
Thanks
j
>
> Signed-off-by: Vladimir Lypak <[email protected]>
> Acked-by: Robert Foss <[email protected]>
> Signed-off-by: Andrey Konovalov <[email protected]>
> ---
> 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
>
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 <[email protected]>
>> ---
>> 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 <[email protected]>
>
> Thanks
> j
>
>> if (!mul || !div)
>> return -ENOENT;
>>
>> --
>> 2.17.1
>>