[v2] media: ov5640: add support of V4L2_CID_LINK_FREQ

Message ID	1561640224-24062-1-git-send-email-hugues.fruchet@st.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <linux-media-owner@kernel.org> From: Hugues Fruchet <hugues.fruchet@st.com> To: Steve Longerbeam <slongerbeam@gmail.com>, Sakari Ailus <sakari.ailus@linux.intel.com>, Hans Verkuil <hverkuil@xs4all.nl>, "Mauro Carvalho Chehab" <mchehab@kernel.org> CC: <linux-media@vger.kernel.org>, <linux-stm32@st-md-mailman.stormreply.com>, Hugues Fruchet <hugues.fruchet@st.com>, Benjamin Gaignard <benjamin.gaignard@linaro.org>, Maxime Ripard <maxime.ripard@bootlin.com>, Jacopo Mondi <jacopo@jmondi.org> Subject: [PATCH v2] media: ov5640: add support of V4L2_CID_LINK_FREQ Date: Thu, 27 Jun 2019 14:57:04 +0200 Message-ID: <1561640224-24062-1-git-send-email-hugues.fruchet@st.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-media-owner@vger.kernel.org Precedence: bulk
Series	[v2] media: ov5640: add support of V4L2_CID_LINK_FREQ \| expand [v2] media: ov5640: add support of V4L2_CID_LINK_FREQ

Hugues FRUCHET June 27, 2019, 12:57 p.m. UTC

Add support of V4L2_CID_LINK_FREQ, this is needed
by some CSI-2 receivers.

384MHz is exposed for the time being, corresponding
to 96MHz pixel clock with 2 bytes per pixel on 2 data lanes.

This setup has been tested successfully with ST MIPID02
CSI-2 to parallel bridge.

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
version 2:
  - do not set control read only as per Hans' comment:
    See https://www.mail-archive.com/linux-media@vger.kernel.org/msg147910.html

 drivers/media/i2c/ov5640.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Sakari Ailus June 27, 2019, 4:05 p.m. UTC | #1

Hi Hugues,

On Thu, Jun 27, 2019 at 02:57:04PM +0200, Hugues Fruchet wrote:
> Add support of V4L2_CID_LINK_FREQ, this is needed
> by some CSI-2 receivers.
> 
> 384MHz is exposed for the time being, corresponding
> to 96MHz pixel clock with 2 bytes per pixel on 2 data lanes.
> 
> This setup has been tested successfully with ST MIPID02
> CSI-2 to parallel bridge.
> 
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> ---

Thanks for the patch.

The driver calculates the sensor configuration based on its configuration,
and this needs to be reflected in the link frequency: it's not a static
value. See e.g. ov5640_calc_sys_clk().

> version 2:
>   - do not set control read only as per Hans' comment:
>     See https://www.mail-archive.com/linux-media@vger.kernel.org/msg147910.html
> 
>  drivers/media/i2c/ov5640.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 82d4ce9..e6307f3 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -218,6 +218,7 @@ struct ov5640_ctrls {
>  	struct v4l2_ctrl *test_pattern;
>  	struct v4l2_ctrl *hflip;
>  	struct v4l2_ctrl *vflip;
> +	struct v4l2_ctrl *link_freq;
>  };
>  
>  struct ov5640_dev {
> @@ -2198,6 +2199,10 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static const s64 link_freq_menu_items[] = {
> +	384000000,
> +};
> +
>  static int ov5640_set_fmt(struct v4l2_subdev *sd,
>  			  struct v4l2_subdev_pad_config *cfg,
>  			  struct v4l2_subdev_format *format)
> @@ -2636,6 +2641,8 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
>  	case V4L2_CID_VFLIP:
>  		ret = ov5640_set_ctrl_vflip(sensor, ctrl->val);
>  		break;
> +	case V4L2_CID_LINK_FREQ:
> +		return 0;
>  	default:
>  		ret = -EINVAL;
>  		break;
> @@ -2703,6 +2710,9 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
>  				       V4L2_CID_POWER_LINE_FREQUENCY_AUTO, 0,
>  				       V4L2_CID_POWER_LINE_FREQUENCY_50HZ);
>  
> +	ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
> +						  0, 0, link_freq_menu_items);
> +
>  	if (hdl->error) {
>  		ret = hdl->error;
>  		goto free_ctrls;
> -- 
> 2.7.4
>

Hugues FRUCHET July 2, 2019, 4:05 p.m. UTC | #2

Hi Sakari,

On 6/27/19 6:05 PM, Sakari Ailus wrote:
> Hi Hugues,
> 
> On Thu, Jun 27, 2019 at 02:57:04PM +0200, Hugues Fruchet wrote:
>> Add support of V4L2_CID_LINK_FREQ, this is needed
>> by some CSI-2 receivers.
>>
>> 384MHz is exposed for the time being, corresponding
>> to 96MHz pixel clock with 2 bytes per pixel on 2 data lanes.
>>
>> This setup has been tested successfully with ST MIPID02
>> CSI-2 to parallel bridge.
>>
>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>> ---
> 
> Thanks for the patch.
> 
> The driver calculates the sensor configuration based on its configuration,
> and this needs to be reflected in the link frequency: it's not a static
> value. See e.g. ov5640_calc_sys_clk().
> 

I know this code, but for a reason I don't understand yet, this seems
to not have effects on the CSI-2 link frequency.

This has been verified with MIPID02 CSI-2 bridge which only work with
this fixed link frequency as input to program its ui_x4 register 
setting, see 
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2028171.html.
All resolutions and all framerate have been tested succesfully with this 
setting.

Initially I tried to set the link frequency according to the value 
computed in ov5640_calc_sys_clk() but it was only functional when 
resolutions/framerate was close to the 384MHz value...

As OV5640 D3 engineering board has been initially developped for 
dragonboard, I'll dig into this and found this commit:
https://lore.kernel.org/patchwork/patch/886794/
which set a fixed pixel rate value to 96MHz, which match perfectly
with the 384MHz value I found for link frequency...

Perhaps other CSI-2 OV5640 users can comment about it and have the 
explanations of what I experiment...
Maxime, Jacopo, do you have any idea about it ?

>> version 2:
>>    - do not set control read only as per Hans' comment:
>>      See https://www.mail-archive.com/linux-media@vger.kernel.org/msg147910.html
>>
>>   drivers/media/i2c/ov5640.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>> index 82d4ce9..e6307f3 100644
>> --- a/drivers/media/i2c/ov5640.c
>> +++ b/drivers/media/i2c/ov5640.c
>> @@ -218,6 +218,7 @@ struct ov5640_ctrls {
>>   	struct v4l2_ctrl *test_pattern;
>>   	struct v4l2_ctrl *hflip;
>>   	struct v4l2_ctrl *vflip;
>> +	struct v4l2_ctrl *link_freq;
>>   };
>>   
>>   struct ov5640_dev {
>> @@ -2198,6 +2199,10 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd,
>>   	return 0;
>>   }
>>   
>> +static const s64 link_freq_menu_items[] = {
>> +	384000000,
>> +};
>> +
>>   static int ov5640_set_fmt(struct v4l2_subdev *sd,
>>   			  struct v4l2_subdev_pad_config *cfg,
>>   			  struct v4l2_subdev_format *format)
>> @@ -2636,6 +2641,8 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
>>   	case V4L2_CID_VFLIP:
>>   		ret = ov5640_set_ctrl_vflip(sensor, ctrl->val);
>>   		break;
>> +	case V4L2_CID_LINK_FREQ:
>> +		return 0;
>>   	default:
>>   		ret = -EINVAL;
>>   		break;
>> @@ -2703,6 +2710,9 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
>>   				       V4L2_CID_POWER_LINE_FREQUENCY_AUTO, 0,
>>   				       V4L2_CID_POWER_LINE_FREQUENCY_50HZ);
>>   
>> +	ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
>> +						  0, 0, link_freq_menu_items);
>> +
>>   	if (hdl->error) {
>>   		ret = hdl->error;
>>   		goto free_ctrls;
>> -- 
>> 2.7.4
>>
> 

BR,
Hugues.

Sakari Ailus Aug. 20, 2019, 9:13 a.m. UTC | #3

Hi Hugues,

On Tue, Jul 02, 2019 at 04:05:46PM +0000, Hugues FRUCHET wrote:
> Hi Sakari,
> 
> On 6/27/19 6:05 PM, Sakari Ailus wrote:
> > Hi Hugues,
> > 
> > On Thu, Jun 27, 2019 at 02:57:04PM +0200, Hugues Fruchet wrote:
> >> Add support of V4L2_CID_LINK_FREQ, this is needed
> >> by some CSI-2 receivers.
> >>
> >> 384MHz is exposed for the time being, corresponding
> >> to 96MHz pixel clock with 2 bytes per pixel on 2 data lanes.
> >>
> >> This setup has been tested successfully with ST MIPID02
> >> CSI-2 to parallel bridge.
> >>
> >> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> >> ---
> > 
> > Thanks for the patch.
> > 
> > The driver calculates the sensor configuration based on its configuration,
> > and this needs to be reflected in the link frequency: it's not a static
> > value. See e.g. ov5640_calc_sys_clk().
> > 
> 
> I know this code, but for a reason I don't understand yet, this seems
> to not have effects on the CSI-2 link frequency.
> 
> This has been verified with MIPID02 CSI-2 bridge which only work with
> this fixed link frequency as input to program its ui_x4 register 
> setting, see 
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2028171.html.
> All resolutions and all framerate have been tested succesfully with this 
> setting.
> 
> Initially I tried to set the link frequency according to the value 
> computed in ov5640_calc_sys_clk() but it was only functional when 
> resolutions/framerate was close to the 384MHz value...
> 
> As OV5640 D3 engineering board has been initially developped for 
> dragonboard, I'll dig into this and found this commit:
> https://lore.kernel.org/patchwork/patch/886794/
> which set a fixed pixel rate value to 96MHz, which match perfectly
> with the 384MHz value I found for link frequency...
> 
> Perhaps other CSI-2 OV5640 users can comment about it and have the 
> explanations of what I experiment...
> Maxime, Jacopo, do you have any idea about it ?

I would also like to wee a comment from someone who's familiar with the
device. Yet I can tell a static value of 348 MHz is clearly incorrect as it
ignores sensor runtime configuration as well as platform configuration
(external clock frequency for instance).

Generally speaking, configuring a CSI-2 receiver to expect a particular
frequency usually doesn't mean it's going to fail even if the transmitter
uses a different frequency, albeit the likelihood of it not working
increases as the difference grows. Could the problem be at the receiver's
end?

Have you checked what kind of values ov5640_calc_pclk() returns for
different configurations? It would seem like that this is what the
LINK_FREQ (divided by lanes and multiplied by two) should be.

> 
> 
> >> version 2:
> >>    - do not set control read only as per Hans' comment:
> >>      See https://www.mail-archive.com/linux-media@vger.kernel.org/msg147910.html
> >>
> >>   drivers/media/i2c/ov5640.c | 10 ++++++++++
> >>   1 file changed, 10 insertions(+)
> >>
> >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> >> index 82d4ce9..e6307f3 100644
> >> --- a/drivers/media/i2c/ov5640.c
> >> +++ b/drivers/media/i2c/ov5640.c
> >> @@ -218,6 +218,7 @@ struct ov5640_ctrls {
> >>   	struct v4l2_ctrl *test_pattern;
> >>   	struct v4l2_ctrl *hflip;
> >>   	struct v4l2_ctrl *vflip;
> >> +	struct v4l2_ctrl *link_freq;
> >>   };
> >>   
> >>   struct ov5640_dev {
> >> @@ -2198,6 +2199,10 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd,
> >>   	return 0;
> >>   }
> >>   
> >> +static const s64 link_freq_menu_items[] = {
> >> +	384000000,
> >> +};
> >> +
> >>   static int ov5640_set_fmt(struct v4l2_subdev *sd,
> >>   			  struct v4l2_subdev_pad_config *cfg,
> >>   			  struct v4l2_subdev_format *format)
> >> @@ -2636,6 +2641,8 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
> >>   	case V4L2_CID_VFLIP:
> >>   		ret = ov5640_set_ctrl_vflip(sensor, ctrl->val);
> >>   		break;
> >> +	case V4L2_CID_LINK_FREQ:
> >> +		return 0;
> >>   	default:
> >>   		ret = -EINVAL;
> >>   		break;
> >> @@ -2703,6 +2710,9 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
> >>   				       V4L2_CID_POWER_LINE_FREQUENCY_AUTO, 0,
> >>   				       V4L2_CID_POWER_LINE_FREQUENCY_50HZ);
> >>   
> >> +	ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
> >> +						  0, 0, link_freq_menu_items);
> >> +
> >>   	if (hdl->error) {
> >>   		ret = hdl->error;
> >>   		goto free_ctrls;
> >> -- 
> >> 2.7.4
> >>
> > 
> 
> BR,
> Hugues.

Jacopo Mondi Aug. 21, 2019, 2:24 p.m. UTC | #4

Hello,
   +laura who has been working on supporting RAW capture for this
   driver.

On Tue, Aug 20, 2019 at 12:13:12PM +0300, Sakari Ailus wrote:
> Hi Hugues,
>
> On Tue, Jul 02, 2019 at 04:05:46PM +0000, Hugues FRUCHET wrote:
> > Hi Sakari,
> >
> > On 6/27/19 6:05 PM, Sakari Ailus wrote:
> > > Hi Hugues,
> > >
> > > On Thu, Jun 27, 2019 at 02:57:04PM +0200, Hugues Fruchet wrote:
> > >> Add support of V4L2_CID_LINK_FREQ, this is needed
> > >> by some CSI-2 receivers.
> > >>
> > >> 384MHz is exposed for the time being, corresponding
> > >> to 96MHz pixel clock with 2 bytes per pixel on 2 data lanes.
> > >>
> > >> This setup has been tested successfully with ST MIPID02
> > >> CSI-2 to parallel bridge.
> > >>
> > >> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> > >> ---
> > >
> > > Thanks for the patch.
> > >
> > > The driver calculates the sensor configuration based on its configuration,
> > > and this needs to be reflected in the link frequency: it's not a static
> > > value. See e.g. ov5640_calc_sys_clk().
> > >
> >
> > I know this code, but for a reason I don't understand yet, this seems
> > to not have effects on the CSI-2 link frequency.
> >

This seems unlikely to me, as the ov5640_calc_sys_clk() calculates the
system clock, which then generates the MIPI CLK.

I would really be interested to know if you could measure somehow the
actual frequency of the clock lane, to make sure it actually
changes according to to the driver calculations.

> > This has been verified with MIPID02 CSI-2 bridge which only work with
> > this fixed link frequency as input to program its ui_x4 register
> > setting, see
> > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2028171.html.
> > All resolutions and all framerate have been tested succesfully with this
> > setting.
> >
> > Initially I tried to set the link frequency according to the value
> > computed in ov5640_calc_sys_clk() but it was only functional when
> > resolutions/framerate was close to the 384MHz value...
> >
> > As OV5640 D3 engineering board has been initially developped for
> > dragonboard, I'll dig into this and found this commit:
> > https://lore.kernel.org/patchwork/patch/886794/
> > which set a fixed pixel rate value to 96MHz, which match perfectly
> > with the 384MHz value I found for link frequency...
> >
> > Perhaps other CSI-2 OV5640 users can comment about it and have the
> > explanations of what I experiment...
> > Maxime, Jacopo, do you have any idea about it ?
>
> I would also like to wee a comment from someone who's familiar with the
> device. Yet I can tell a static value of 348 MHz is clearly incorrect as it
> ignores sensor runtime configuration as well as platform configuration
> (external clock frequency for instance).
>
> Generally speaking, configuring a CSI-2 receiver to expect a particular
> frequency usually doesn't mean it's going to fail even if the transmitter
> uses a different frequency, albeit the likelihood of it not working
> increases as the difference grows. Could the problem be at the receiver's
> end?
>
> Have you checked what kind of values ov5640_calc_pclk() returns for
> different configurations? It would seem like that this is what the
> LINK_FREQ (divided by lanes and multiplied by two) should be.

Not exactly. ov5640_calc_pclk() is only used when computing the
parallel bus pixel clock not for the CSI-2 bus.

To obtain the link frequency for the MIPI interface I would consider
the system clock frequency calculated by ov5640_calc_sys_clk(), which
represents the bandwidth per lane in bits per second. This should then
be divided by 2 to compensate the MIPI DDR mode [*] (no need to divide
by the number of lanes, as this is already the bandwidth -per lane-)

For 640x480 YUYV8_2X8 (and 1024x768) with 2 data lanes and 24MHz xclk
I have calculated the following values:

SYSCLK = 492MHz
MIPISCLK = 256MHz
PCLK = 61,50MHz

For 1280x720 with the same setup
SYSCLK = 340MHz
MIPI_CLK = 170MHz
PCLK = 42,50 (this seems veeery low according to datasheet)

For 1920x1080
SYSCLK = 680MHz
MIPI_CLK = 340MHz
PCLK = 85MHz (slightly slow according to the datasheet)

All captured images seems fine...

If you could confirm those values by probing the actual clock lane it
would be just great.

However, this only work assuming the here below [*], and we're still
missing something because RAW still doesn't work as expected :(

Anyway, clock tree madness apart, fixing the LINK_FREQ control value
to 96MHz seems like it does not reflect what the driver actual does.

Could you try setting the control value to what the driver actually
computes as here suggested?

Thanks
   j

PS. I'm still confused why the have the link-frequencies property in
DTS bindings. What is its purpose if all drivers compute or hardcode
it?

[*] There is a big assumption here, that the MIPI bus clock gets
generated by two different clock outputs depending if the mode goes
through the the scaler or not. You can see this at line 967 in the
driver code. If mode goes though the scaler, MIPI_DIV = 2 and you get
the bus clock from the MIPISCLK signal. If it does not go through the
scaler MIPI_DIV = 1 and then the MIPI bus clock gets provided by the
MIPI_CLK signal. This is the result of several attempts to decode the
ov5640 clock tree, but it has only been tested with modes with bpp =
16 and 2 data lanes. As it has been recently reported, for RAW mode,
this does not work well, so the assumption might not be correct.

>
> >
> >
> > >> version 2:
> > >>    - do not set control read only as per Hans' comment:
> > >>      See https://www.mail-archive.com/linux-media@vger.kernel.org/msg147910.html
> > >>
> > >>   drivers/media/i2c/ov5640.c | 10 ++++++++++
> > >>   1 file changed, 10 insertions(+)
> > >>
> > >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > >> index 82d4ce9..e6307f3 100644
> > >> --- a/drivers/media/i2c/ov5640.c
> > >> +++ b/drivers/media/i2c/ov5640.c
> > >> @@ -218,6 +218,7 @@ struct ov5640_ctrls {
> > >>   	struct v4l2_ctrl *test_pattern;
> > >>   	struct v4l2_ctrl *hflip;
> > >>   	struct v4l2_ctrl *vflip;
> > >> +	struct v4l2_ctrl *link_freq;
> > >>   };
> > >>
> > >>   struct ov5640_dev {
> > >> @@ -2198,6 +2199,10 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd,
> > >>   	return 0;
> > >>   }
> > >>
> > >> +static const s64 link_freq_menu_items[] = {
> > >> +	384000000,
> > >> +};
> > >> +
> > >>   static int ov5640_set_fmt(struct v4l2_subdev *sd,
> > >>   			  struct v4l2_subdev_pad_config *cfg,
> > >>   			  struct v4l2_subdev_format *format)
> > >> @@ -2636,6 +2641,8 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
> > >>   	case V4L2_CID_VFLIP:
> > >>   		ret = ov5640_set_ctrl_vflip(sensor, ctrl->val);
> > >>   		break;
> > >> +	case V4L2_CID_LINK_FREQ:
> > >> +		return 0;
> > >>   	default:
> > >>   		ret = -EINVAL;
> > >>   		break;
> > >> @@ -2703,6 +2710,9 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
> > >>   				       V4L2_CID_POWER_LINE_FREQUENCY_AUTO, 0,
> > >>   				       V4L2_CID_POWER_LINE_FREQUENCY_50HZ);
> > >>
> > >> +	ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
> > >> +						  0, 0, link_freq_menu_items);
> > >> +
> > >>   	if (hdl->error) {
> > >>   		ret = hdl->error;
> > >>   		goto free_ctrls;
> > >> --
> > >> 2.7.4
> > >>
> > >
> >
> > BR,
> > Hugues.
>
> --
> Sakari Ailus
> sakari.ailus@linux.intel.com

Hugues FRUCHET Sept. 3, 2019, 2:48 p.m. UTC | #5

Hi Jacopo,

On 8/21/19 4:24 PM, Jacopo Mondi wrote:
> Hello,
>     +laura who has been working on supporting RAW capture for this
>     driver.
> 
> On Tue, Aug 20, 2019 at 12:13:12PM +0300, Sakari Ailus wrote:
>> Hi Hugues,
>>
>> On Tue, Jul 02, 2019 at 04:05:46PM +0000, Hugues FRUCHET wrote:
>>> Hi Sakari,
>>>
>>> On 6/27/19 6:05 PM, Sakari Ailus wrote:
>>>> Hi Hugues,
>>>>
>>>> On Thu, Jun 27, 2019 at 02:57:04PM +0200, Hugues Fruchet wrote:
>>>>> Add support of V4L2_CID_LINK_FREQ, this is needed
>>>>> by some CSI-2 receivers.
>>>>>
>>>>> 384MHz is exposed for the time being, corresponding
>>>>> to 96MHz pixel clock with 2 bytes per pixel on 2 data lanes.
>>>>>
>>>>> This setup has been tested successfully with ST MIPID02
>>>>> CSI-2 to parallel bridge.
>>>>>
>>>>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>>>>> ---
>>>>
>>>> Thanks for the patch.
>>>>
>>>> The driver calculates the sensor configuration based on its configuration,
>>>> and this needs to be reflected in the link frequency: it's not a static
>>>> value. See e.g. ov5640_calc_sys_clk().
>>>>
>>>
>>> I know this code, but for a reason I don't understand yet, this seems
>>> to not have effects on the CSI-2 link frequency.
>>>
> 
> This seems unlikely to me, as the ov5640_calc_sys_clk() calculates the
> system clock, which then generates the MIPI CLK.
> 
> I would really be interested to know if you could measure somehow the
> actual frequency of the clock lane, to make sure it actually
> changes according to to the driver calculations.
> 
>>> This has been verified with MIPID02 CSI-2 bridge which only work with
>>> this fixed link frequency as input to program its ui_x4 register
>>> setting, see
>>> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2028171.html.
>>> All resolutions and all framerate have been tested succesfully with this
>>> setting.
>>>
>>> Initially I tried to set the link frequency according to the value
>>> computed in ov5640_calc_sys_clk() but it was only functional when
>>> resolutions/framerate was close to the 384MHz value...
>>>
>>> As OV5640 D3 engineering board has been initially developped for
>>> dragonboard, I'll dig into this and found this commit:
>>> https://lore.kernel.org/patchwork/patch/886794/
>>> which set a fixed pixel rate value to 96MHz, which match perfectly
>>> with the 384MHz value I found for link frequency...
>>>
>>> Perhaps other CSI-2 OV5640 users can comment about it and have the
>>> explanations of what I experiment...
>>> Maxime, Jacopo, do you have any idea about it ?
>>
>> I would also like to wee a comment from someone who's familiar with the
>> device. Yet I can tell a static value of 348 MHz is clearly incorrect as it
>> ignores sensor runtime configuration as well as platform configuration
>> (external clock frequency for instance).
>>
>> Generally speaking, configuring a CSI-2 receiver to expect a particular
>> frequency usually doesn't mean it's going to fail even if the transmitter
>> uses a different frequency, albeit the likelihood of it not working
>> increases as the difference grows. Could the problem be at the receiver's
>> end?
>>
>> Have you checked what kind of values ov5640_calc_pclk() returns for
>> different configurations? It would seem like that this is what the
>> LINK_FREQ (divided by lanes and multiplied by two) should be.
> 
> Not exactly. ov5640_calc_pclk() is only used when computing the
> parallel bus pixel clock not for the CSI-2 bus.
> 
> To obtain the link frequency for the MIPI interface I would consider
> the system clock frequency calculated by ov5640_calc_sys_clk(), which
> represents the bandwidth per lane in bits per second. This should then
> be divided by 2 to compensate the MIPI DDR mode [*] (no need to divide
> by the number of lanes, as this is already the bandwidth -per lane-)
> 
> For 640x480 YUYV8_2X8 (and 1024x768) with 2 data lanes and 24MHz xclk
> I have calculated the following values:
> 
> SYSCLK = 492MHz
> MIPISCLK = 256MHz
> PCLK = 61,50MHz
> 
> For 1280x720 with the same setup
> SYSCLK = 340MHz
> MIPI_CLK = 170MHz
> PCLK = 42,50 (this seems veeery low according to datasheet)
> 
> For 1920x1080
> SYSCLK = 680MHz
> MIPI_CLK = 340MHz
> PCLK = 85MHz (slightly slow according to the datasheet)
> 
> All captured images seems fine...
> 
> If you could confirm those values by probing the actual clock lane it
> would be just great.
> 

I've made some measurements probing clock lane with YUYV 16 bits 
grabbing, here are the results:

* VGA @ 30fps
v4l2-ctl --set-parm=30;v4l2-ctl 
--set-fmt-video=width=640,height=480,pixelformat=YUYV --stream-mmap
[ 1624.854273] ov5640 0-003c: rate=491443200, 640x480 from 1896x1080
[ 1624.859192] ov5640 0-003c: sysclk=492000000, mipi_div=2
Measured clock lane frequency: 123MHz

* 1024x768 @ 30fps
v4l2-ctl --set-parm=30;v4l2-ctl 
--set-fmt-video=width=1024,height=768,pixelformat=YUYV --stream-mmap
[  558.833885] ov5640 0-003c: rate=491443200, 1024x768 from 1896x1080
[  558.840188] ov5640 0-003c: sysclk=492000000, mipi_div=2
Measured clock lane frequency: 124MHz

* 720p @ 30fps
v4l2-ctl --set-parm=30;v4l2-ctl 
--set-fmt-video=width=1280,height=720,pixelformat=YUYV --stream-mmap
[ 2920.785392] ov5640 0-003c: rate=336019200, 1280x720 from 1892x740
[ 2920.794771] ov5640 0-003c: sysclk=340000000, mipi_div=1
Measured clock lane frequency: 171MHz

* 1080p @ 30fps
v4l2-ctl --set-parm=30;v4l2-ctl 
--set-fmt-video=width=1920,height=1080,pixelformat=YUYV --stream-mmap
[ 1650.143701] ov5640 0-003c: rate=672000000, 1920x1080 from 2500x1120
[ 1650.149876] ov5640 0-003c: sysclk=672000000, mipi_div=1
Measured clock lane frequency: 330MHz

So it seems that formula must also take care of mipi_div:
link_frequency = sys_div / 2 / mipi_div


Unfortunately, as said previously, when updating ov5640 codebase so that 
CID_LINK_FREQUENCY returns this computed value instead of 384MHz fixed 
value, the MIPID02 bridge has some troubles and some 
resolutions/framerate are broken:

dmesg -n8
echo "format link_freq +p" > /sys/kernel/debug/dynamic_debug/control

* VGA @ 30fps => broken
v4l2-ctl --set-parm=30;v4l2-ctl 
--set-fmt-video=width=640,height=480,pixelformat=YUYV --stream-mmap
[ 1624.854273] ov5640 0-003c: rate=491443200, 640x480 from 1896x1080
[ 1624.859192] ov5640 0-003c: sysclk=492000000, mipi_div=2
[ 1624.924287] st-mipid02 1-0014: detect link_freq = 123000000 Hz
==> no frames sent to DCMI

* 720p @ 30fps => very slow framerate
v4l2-ctl --set-parm=30;v4l2-ctl 
--set-fmt-video=width=1280,height=720,pixelformat=YUYV --stream-mmap
[ 1900.707822] ov5640 0-003c: rate=336019200, 1280x720 from 1892x740
[ 1900.712717] ov5640 0-003c: sysclk=340000000, mipi_div=1
[ 1900.782402] st-mipid02 1-0014: detect link_freq = 170000000 Hz
<< 0.69 fps
< 0.67 fps
< 0.67 fps

* 1080p @ 30fps => very slow framerate
v4l2-ctl --set-parm=30;v4l2-ctl 
--set-fmt-video=width=1920,height=1080,pixelformat=YUYV --stream-mmap
[ 1366.550832] ov5640 0-003c: rate=336000000, 1920x1080 from 2500x1120
[ 1366.555668] ov5640 0-003c: sysclk=336000000, mipi_div=1
Frame rate set to 30.000 fps
[ 1367.105503] ov5640 0-003c: rate=672000000, 1920x1080 from 2500x1120
[ 1367.110420] ov5640 0-003c: sysclk=672000000, mipi_div=1
[ 1367.214498] st-mipid02 1-0014: detect link_freq = 336000000 Hz
<< 0.11 fps
< 0.11 fps


* 1080p @ 15fps => OK, 15fps as expected
v4l2-ctl --set-parm=15;v4l2-ctl 
--set-fmt-video=width=1920,height=1080,pixelformat=YUYV --stream-mmap
[ 1327.417545] ov5640 0-003c: rate=336000000, 1920x1080 from 2500x1120
[ 1327.422560] ov5640 0-003c: sysclk=336000000, mipi_div=1
[ 1327.527852] st-mipid02 1-0014: detect link_freq = 168000000 Hz
<<<<<<<<<<<<<<<<< 15.09 fps
<<<<<<<<<<<<<<< 15.04 fps


One could argue that this is the mipid02 bridge codebase that is wrong:
drivers/media/i2c/st-mipid02.c:
   mipid02_configure_from_rx_speed()
     [...]
     bridge->r.clk_lane_reg1 |= (2000000000 / link_freq) << 2;

but computation is inline with MIPID02 spec:

"CLOCK LANE 1 REGISTER"
Bit_no   Bit_name
7        ui_x4_clk_lane[5]
6        ui_x4_clk_lane[4]
5        ui_x4_clk_lane[3]
4        ui_x4_clk_lane[2]
3        ui_x4_clk_lane[1]
2        ui_x4_clk_lane[0]
Unit interval time multiplied by four
This signal indicates the bit period in units of
0.25 ns. If the unit interval is 3 ns, twelve (0x0C)
should be programmed. This value is used to
generate delays. Therefore, if the period is not a
multiple of 0.25 ns, the value should be rounded
down. For example, a 600 Mbit/s single lane
linkuses a unit interval of 1.667 ns. Multiplying
by four results in 6.667. In this case, a value of 6
(not 7) should be programmed.


Perhaps that someone which knows well CSI-2 interfaces could
detect what is wrong here, but personally I don't.


> However, this only work assuming the here below [*], and we're still
> missing something because RAW still doesn't work as expected :(
> 
> Anyway, clock tree madness apart, fixing the LINK_FREQ control value
> to 96MHz seems like it does not reflect what the driver actual does.
> 
> Could you try setting the control value to what the driver actually
> computes as here suggested?
> 
> Thanks
>     j
> 
> PS. I'm still confused why the have the link-frequencies property in
> DTS bindings. What is its purpose if all drivers compute or hardcode
> it?

I share this point of view, I don't know how to return a dynamically 
computed link frequency. Current drivers are exposing a single one
or a set of discrete frequencies.
For a matter of test purpose, I've done this hack:

-static const s64 link_freq_menu_items[] = {
+static s64 link_freq_menu_items[] = {
	384000000,
};

+	link_freq_menu_items[0] = _rate / 2 / mipi_div;


> 
> [*] There is a big assumption here, that the MIPI bus clock gets
> generated by two different clock outputs depending if the mode goes
> through the the scaler or not. You can see this at line 967 in the
> driver code. If mode goes though the scaler, MIPI_DIV = 2 and you get
> the bus clock from the MIPISCLK signal. If it does not go through the
> scaler MIPI_DIV = 1 and then the MIPI bus clock gets provided by the
> MIPI_CLK signal. This is the result of several attempts to decode the
> ov5640 clock tree, but it has only been tested with modes with bpp =
> 16 and 2 data lanes. As it has been recently reported, for RAW mode,
> this does not work well, so the assumption might not be correct.
> 
>>
>>>
>>>
>>>>> version 2:
>>>>>     - do not set control read only as per Hans' comment:
>>>>>       See https://www.mail-archive.com/linux-media@vger.kernel.org/msg147910.html
>>>>>
>>>>>    drivers/media/i2c/ov5640.c | 10 ++++++++++
>>>>>    1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>>>>> index 82d4ce9..e6307f3 100644
>>>>> --- a/drivers/media/i2c/ov5640.c
>>>>> +++ b/drivers/media/i2c/ov5640.c
>>>>> @@ -218,6 +218,7 @@ struct ov5640_ctrls {
>>>>>    	struct v4l2_ctrl *test_pattern;
>>>>>    	struct v4l2_ctrl *hflip;
>>>>>    	struct v4l2_ctrl *vflip;
>>>>> +	struct v4l2_ctrl *link_freq;
>>>>>    };
>>>>>
>>>>>    struct ov5640_dev {
>>>>> @@ -2198,6 +2199,10 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd,
>>>>>    	return 0;
>>>>>    }
>>>>>
>>>>> +static const s64 link_freq_menu_items[] = {
>>>>> +	384000000,
>>>>> +};
>>>>> +
>>>>>    static int ov5640_set_fmt(struct v4l2_subdev *sd,
>>>>>    			  struct v4l2_subdev_pad_config *cfg,
>>>>>    			  struct v4l2_subdev_format *format)
>>>>> @@ -2636,6 +2641,8 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
>>>>>    	case V4L2_CID_VFLIP:
>>>>>    		ret = ov5640_set_ctrl_vflip(sensor, ctrl->val);
>>>>>    		break;
>>>>> +	case V4L2_CID_LINK_FREQ:
>>>>> +		return 0;
>>>>>    	default:
>>>>>    		ret = -EINVAL;
>>>>>    		break;
>>>>> @@ -2703,6 +2710,9 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
>>>>>    				       V4L2_CID_POWER_LINE_FREQUENCY_AUTO, 0,
>>>>>    				       V4L2_CID_POWER_LINE_FREQUENCY_50HZ);
>>>>>
>>>>> +	ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
>>>>> +						  0, 0, link_freq_menu_items);
>>>>> +
>>>>>    	if (hdl->error) {
>>>>>    		ret = hdl->error;
>>>>>    		goto free_ctrls;
>>>>> --
>>>>> 2.7.4
>>>>>
>>>>
>>>
>>> BR,
>>> Hugues.
>>
>> --
>> Sakari Ailus
>> sakari.ailus@linux.intel.com

BR,
Hugues.

Hugues FRUCHET Oct. 19, 2020, 1:40 p.m. UTC | #6

Hi Jacopo, all,

Back on this subject recently, I've found interesting things around 
register 0x4837 PCLK PERIOD thanks to Samuel Bobrowicz work on PLL 
settings [1], but 0x4837 register value computation differs from what 
I've understood/measured.

The OV5640 PCLK_PERIOD register value must be in line with the
ST MIPID02 CSI-2 receiver CLOCK_LANE_1 register, otherwise receptions 
issues are encountered has shown in my previous mail.

Configuring those 2 registers according to measured MIPI clock frequency
allows to cover almost all resolutions/formats (RGB565, YUV422, JPEG).
I've also measured with oscilloscope the MIPI clock-lane frequency and 
verified that this is well matching computed one [2].
Please note that due to platform limitation, I could not test link 
frequency above 720p@15fps, so additional tests are needed with platform 
that could handle 5Mp@15fps (OV5640 limit).

In term of code, this mean:

v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
        		  0, 0, link_freq_menu_items);

ov5640_set_mipi_pclk(rate)
{
   mipi_sclk = ov5640_calc_sys_clk(sensor, rate, &prediv, &mult, &sysdiv);
   link_freq = mipi_sclk / 2 / mipi_div; /* the CSI-2 clock-lane 
frequency, measurements are matching */

   _pclk_period = (u8) (2000000000UL / link_freq);/* expressed in ns and 
with 1-bit decimal: 0x1=0.5ns, 0x2=1ns, 0x3=1.5ns and so on... */
   ov5640_write_reg(sensor, OV5640_REG_PCLK_PERIOD, _pclk_period);

   link_freq_menu_items[0] = link_freq;/* Currently a hack to inform 
receiver about the dynamically computed clock frequency, I don't know 
yet how to handle this correctly, please advise... */

   ...
}

As explained in comment, I miss a V4L2 way to set dynamically 
V4L2_CID_LINK_FREQ in order to propose a new patchset.


[1] 
https://patchwork.kernel.org/project/linux-media/patch/1539797508-127629-1-git-send-email-sam@elite-embedded.com/

[2] measure of the CSI-2 clock frequency, tested with JPEG (easier to 
check that capture is fine) and various resolutions:

****************
***  QQVGA  ****
****************
* JPEG QQVGA@15fps
v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl 
--set-fmt-video=width=160,height=120,pixelformat=JPEG --stream-mmap 
--stream-count=-1
[  371.459607] ov5640 1-003c: Bandwidth Per Lane=223879680, 160x120 from 
1896x984
[  371.465516] ov5640 1-003c: mipi_sclk=224000000, mipi_div=2, prediv=3, 
mult=28, sysdiv=1
[  371.473727] ov5640 1-003c: link freq=56000000 Hz
[  371.478529] ov5640 1-003c: PCLK PERIOD 0x4837=0x23
[  371.538193] st-mipid02 2-0014: clk_lane_reg1=0x8d
<<<<<<<<<<<<<<<<< 15.09 fps
<<<<<<<<<<<<<<< 15.04 fps
<<<<<<<<<<<<<<< 15.03 fps
<<<<<<<<<<<<<<< 15.02 fps
<<<<<<<<<<<<<<< 15.01 fps
Clock-lane frequency measured at #18ns (55.5MHz)


* RGB565 QQVGA@15fps
v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl 
--set-fmt-video=width=160,height=120,pixelformat=RGBP --stream-mmap 
--stream-count=-1
[ 1229.422079] st-mipid02 2-0014: clk_lane_reg1=0x8d
[ 1229.431758] ov5640 1-003c: Bandwidth Per Lane=223879680, 160x120 from 
1896x984
[ 1229.437905] ov5640 1-003c: mipi_sclk=224000000, mipi_div=2, prediv=3, 
mult=28, sysdiv=1
[ 1229.445620] ov5640 1-003c: link freq=56000000 Hz
[ 1229.450397] ov5640 1-003c: PCLK PERIOD 0x4837=0x23
<<<<<<<<<<<<<<<<< 15.09 fps
<<<<<<<<<<<<<<< 15.04 fps
<<<<<<<<<<<<<< 14.70 fps
<<<<<<<<<<<<<<< 14.77 fps
Measured #18ns (55.5MHz)


**************
***  VGA  ****
**************
* JPEG VGA@15fps
v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl 
--set-fmt-video=width=640,height=480,pixelformat=JPEG --stream-mmap 
--stream-count=-1
[ 1105.621884] ov5640 1-003c: Bandwidth Per Lane=245721600, 640x480 from 
1896x1080
[ 1105.627927] ov5640 1-003c: mipi_sclk=246000000, mipi_div=2, prediv=3, 
mult=123, sysdiv=4
[ 1105.641273] ov5640 1-003c: link freq=61500000 Hz
[ 1105.646789] ov5640 1-003c: PCLK PERIOD 0x4837=0x40
[ 1105.709626] st-mipid02 2-0014: clk_lane_reg1=0x81
<<<<<<<<<<<<<<<<< 15.09 fps
<<<<<<<<<<<<<<< 15.04 fps
<<<<<<<<<<<<<<< 15.03 fps
<<<<<<<<<<<<<<< 15.02 fps
<<<<<<<<<<<<<<< 15.01 fps

Measured #16ns (62.5MHz)


* JPEG VGA@30fps
v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=30;v4l2-ctl 
--set-fmt-video=width=640,height=480,pixelformat=JPEG --stream-mmap 
--stream-count=-1
[ 1144.539082] ov5640 1-003c: Bandwidth Per Lane=491443200, 640x480 from 
1896x1080
[ 1144.545143] ov5640 1-003c: mipi_sclk=492000000, mipi_div=2, prediv=3, 
mult=123, sysdiv=2
[ 1144.558756] ov5640 1-003c: link freq=123000000 Hz
[ 1144.562012] ov5640 1-003c: PCLK PERIOD 0x4837=0x20
[ 1144.623895] st-mipid02 2-0014: clk_lane_reg1=0x41
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.09 fps
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.04 fps
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.13 fps

Measured #8ns (125MHz)


*******************
***  1024x768  ****
*******************
* JPEG 1024x768@15fps
v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl 
--set-fmt-video=width=1024,height=768,pixelformat=JPEG --stream-mmap 
--stream-count=-1
<<<<<<<<<<<<<<<<< 15.09 fps
<<<<<<<<<<<<<<< 15.04 fps
<<<<<<<<<<<<<<< 15.03 fps
<<<<<<<<<<<<<<< 15.02 fps

Measured #16ns (62.5MHz)


* JPEG 1024x768@30fps
v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=30;v4l2-ctl 
--set-fmt-video=width=1024,height=768,pixelformat=JPEG --stream-mmap 
--stream-count=-1
[ 1218.632061] ov5640 1-003c: Bandwidth Per Lane=491443200, 1024x768 
from 1896x1080
[ 1218.638216] ov5640 1-003c: mipi_sclk=492000000, mipi_div=2, prediv=3, 
mult=123, sysdiv=2
[ 1218.651788] ov5640 1-003c: link freq=123000000 Hz
[ 1218.655345] ov5640 1-003c: PCLK PERIOD 0x4837=0x20
[ 1218.724149] st-mipid02 2-0014: clk_lane_reg1=0x41
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.09 fps
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.04 fps
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.13 fps
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.09 fps
Measured #8ns (125MHz)


***************
***  720p  ****
***************
* JPEG 720p@15fps
v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl 
--set-fmt-video=width=1280,height=720,pixelformat=JPEG --stream-mmap 
--stream-count=-1
[ 7781.232498] ov5640 1-003c: Bandwidth Per Lane=168009600, 1280x720 
from 1892x740
[ 7781.238554] ov5640 1-003c: mipi_sclk=169600000, mipi_div=1, prediv=3, 
mult=106, sysdiv=5
[ 7781.247117] ov5640 1-003c: link freq=84800000 Hz
[ 7781.251116] ov5640 1-003c: PCLK PERIOD 0x4837=0x17
[ 7781.316980] st-mipid02 2-0014: clk_lane_reg1=0x5d

Measured #12ns (83MHz)

* JPEG 720p@30fps
v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=30;v4l2-ctl 
--set-fmt-video=width=1280,height=720,pixelformat=JPEG --stream-mmap 
--stream-count=-1
[  515.548519] ov5640 1-003c: Bandwidth Per Lane=336019200, 1280x720 
from 1892x740
[  515.554646] ov5640 1-003c: mipi_sclk=340000000, mipi_div=1, prediv=3, 
mult=85, sysdiv=2
[  515.562406] ov5640 1-003c: link freq=170000000 Hz
[  515.574376] ov5640 1-003c: PCLK PERIOD 0x4837=0xb
[  515.639862] st-mipid02 2-0014: clk_lane_reg1=0x2d
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.39 fps
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.50 fps
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.36 fps
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.42 fps
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.40 fps
Measured #6ns (166MHz)
==> 30fps but image artefacts (out of MP1 capabilities)


****************
***  1080p  ****
****************
* JPEG HD@15fps
v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl 
--set-fmt-video=width=1920,height=1080,pixelformat=JPEG --stream-mmap 
--stream-count=-1
[ 1998.266915] ov5640 1-003c: Bandwidth Per Lane=336000000, 1920x1080 
from 2500x1120
[ 1998.273040] ov5640 1-003c: mipi_sclk=336000000, mipi_div=1, prediv=3, 
mult=42, sysdiv=1
[ 1998.280987] ov5640 1-003c: link freq=168000000 Hz
[ 1998.286142] ov5640 1-003c: PCLK PERIOD 0x4837=0xb
[ 1998.380206] st-mipid02 2-0014: clk_lane_reg1=0x2d
<<< 1.88 fps
<< 1.87 fps
<< 1.88 fps
<< 1.87 fps
<< 1.87 fps
Measured #6ns (166MHz)

* JPEG HD@30fps
v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=30;v4l2-ctl 
--set-fmt-video=width=1920,height=1080,pixelformat=JPEG --stream-mmap 
--stream-count=-1
[ 2196.981767] ov5640 1-003c: Bandwidth Per Lane=672000000, 1920x1080 
from 2500x1120
[ 2196.987882] ov5640 1-003c: mipi_sclk=672000000, mipi_div=1, prediv=3, 
mult=84, sysdiv=1
[ 2196.996225] ov5640 1-003c: link freq=336000000 Hz
[ 2197.000553] ov5640 1-003c: PCLK PERIOD 0x4837=0x5
[ 2197.097244] st-mipid02 2-0014: clk_lane_reg1=0x15
Measured #3ns (333MHz)

==> no images but out of MP1 capabilities


**************
***  5Mp  ****
**************
* JPEG 5Mp@15fps
v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl 
--set-fmt-video=width=2592,height=1944,pixelformat=JPEG --stream-mmap 
--stream-count=-1

[ 2479.203457] ov5640 1-003c: Bandwidth Per Lane=671639040, 2592x1944 
from 2844x1968
[ 2479.209647] ov5640 1-003c: mipi_sclk=672000000, mipi_div=1, prediv=3, 
mult=84, sysdiv=1
[ 2479.217961] ov5640 1-003c: link freq=336000000 Hz
[ 2479.222240] ov5640 1-003c: PCLK PERIOD 0x4837=0x5
[ 2479.388400] st-mipid02 2-0014: clk_lane_reg1=0x15

Measured #3ns (333MHz)

==> no images but out of MP1 capabilities



On 9/3/19 4:48 PM, Hugues FRUCHET wrote:
> Hi Jacopo,
> 
> On 8/21/19 4:24 PM, Jacopo Mondi wrote:
>> Hello,
>>      +laura who has been working on supporting RAW capture for this
>>      driver.
>>
>> On Tue, Aug 20, 2019 at 12:13:12PM +0300, Sakari Ailus wrote:
>>> Hi Hugues,
>>>
>>> On Tue, Jul 02, 2019 at 04:05:46PM +0000, Hugues FRUCHET wrote:
>>>> Hi Sakari,
>>>>
>>>> On 6/27/19 6:05 PM, Sakari Ailus wrote:
>>>>> Hi Hugues,
>>>>>
>>>>> On Thu, Jun 27, 2019 at 02:57:04PM +0200, Hugues Fruchet wrote:
>>>>>> Add support of V4L2_CID_LINK_FREQ, this is needed
>>>>>> by some CSI-2 receivers.
>>>>>>
>>>>>> 384MHz is exposed for the time being, corresponding
>>>>>> to 96MHz pixel clock with 2 bytes per pixel on 2 data lanes.
>>>>>>
>>>>>> This setup has been tested successfully with ST MIPID02
>>>>>> CSI-2 to parallel bridge.
>>>>>>
>>>>>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>>>>>> ---
>>>>>
>>>>> Thanks for the patch.
>>>>>
>>>>> The driver calculates the sensor configuration based on its configuration,
>>>>> and this needs to be reflected in the link frequency: it's not a static
>>>>> value. See e.g. ov5640_calc_sys_clk().
>>>>>
>>>>
>>>> I know this code, but for a reason I don't understand yet, this seems
>>>> to not have effects on the CSI-2 link frequency.
>>>>
>>
>> This seems unlikely to me, as the ov5640_calc_sys_clk() calculates the
>> system clock, which then generates the MIPI CLK.
>>
>> I would really be interested to know if you could measure somehow the
>> actual frequency of the clock lane, to make sure it actually
>> changes according to to the driver calculations.
>>
>>>> This has been verified with MIPID02 CSI-2 bridge which only work with
>>>> this fixed link frequency as input to program its ui_x4 register
>>>> setting, see
>>>> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2028171.html.
>>>> All resolutions and all framerate have been tested succesfully with this
>>>> setting.
>>>>
>>>> Initially I tried to set the link frequency according to the value
>>>> computed in ov5640_calc_sys_clk() but it was only functional when
>>>> resolutions/framerate was close to the 384MHz value...
>>>>
>>>> As OV5640 D3 engineering board has been initially developped for
>>>> dragonboard, I'll dig into this and found this commit:
>>>> https://lore.kernel.org/patchwork/patch/886794/
>>>> which set a fixed pixel rate value to 96MHz, which match perfectly
>>>> with the 384MHz value I found for link frequency...
>>>>
>>>> Perhaps other CSI-2 OV5640 users can comment about it and have the
>>>> explanations of what I experiment...
>>>> Maxime, Jacopo, do you have any idea about it ?
>>>
>>> I would also like to wee a comment from someone who's familiar with the
>>> device. Yet I can tell a static value of 348 MHz is clearly incorrect as it
>>> ignores sensor runtime configuration as well as platform configuration
>>> (external clock frequency for instance).
>>>
>>> Generally speaking, configuring a CSI-2 receiver to expect a particular
>>> frequency usually doesn't mean it's going to fail even if the transmitter
>>> uses a different frequency, albeit the likelihood of it not working
>>> increases as the difference grows. Could the problem be at the receiver's
>>> end?
>>>
>>> Have you checked what kind of values ov5640_calc_pclk() returns for
>>> different configurations? It would seem like that this is what the
>>> LINK_FREQ (divided by lanes and multiplied by two) should be.
>>
>> Not exactly. ov5640_calc_pclk() is only used when computing the
>> parallel bus pixel clock not for the CSI-2 bus.
>>
>> To obtain the link frequency for the MIPI interface I would consider
>> the system clock frequency calculated by ov5640_calc_sys_clk(), which
>> represents the bandwidth per lane in bits per second. This should then
>> be divided by 2 to compensate the MIPI DDR mode [*] (no need to divide
>> by the number of lanes, as this is already the bandwidth -per lane-)
>>
>> For 640x480 YUYV8_2X8 (and 1024x768) with 2 data lanes and 24MHz xclk
>> I have calculated the following values:
>>
>> SYSCLK = 492MHz
>> MIPISCLK = 256MHz
>> PCLK = 61,50MHz
>>
>> For 1280x720 with the same setup
>> SYSCLK = 340MHz
>> MIPI_CLK = 170MHz
>> PCLK = 42,50 (this seems veeery low according to datasheet)
>>
>> For 1920x1080
>> SYSCLK = 680MHz
>> MIPI_CLK = 340MHz
>> PCLK = 85MHz (slightly slow according to the datasheet)
>>
>> All captured images seems fine...
>>
>> If you could confirm those values by probing the actual clock lane it
>> would be just great.
>>
> 
> I've made some measurements probing clock lane with YUYV 16 bits
> grabbing, here are the results:
> 
> * VGA @ 30fps
> v4l2-ctl --set-parm=30;v4l2-ctl
> --set-fmt-video=width=640,height=480,pixelformat=YUYV --stream-mmap
> [ 1624.854273] ov5640 0-003c: rate=491443200, 640x480 from 1896x1080
> [ 1624.859192] ov5640 0-003c: sysclk=492000000, mipi_div=2
> Measured clock lane frequency: 123MHz
> 
> * 1024x768 @ 30fps
> v4l2-ctl --set-parm=30;v4l2-ctl
> --set-fmt-video=width=1024,height=768,pixelformat=YUYV --stream-mmap
> [  558.833885] ov5640 0-003c: rate=491443200, 1024x768 from 1896x1080
> [  558.840188] ov5640 0-003c: sysclk=492000000, mipi_div=2
> Measured clock lane frequency: 124MHz
> 
> * 720p @ 30fps
> v4l2-ctl --set-parm=30;v4l2-ctl
> --set-fmt-video=width=1280,height=720,pixelformat=YUYV --stream-mmap
> [ 2920.785392] ov5640 0-003c: rate=336019200, 1280x720 from 1892x740
> [ 2920.794771] ov5640 0-003c: sysclk=340000000, mipi_div=1
> Measured clock lane frequency: 171MHz
> 
> * 1080p @ 30fps
> v4l2-ctl --set-parm=30;v4l2-ctl
> --set-fmt-video=width=1920,height=1080,pixelformat=YUYV --stream-mmap
> [ 1650.143701] ov5640 0-003c: rate=672000000, 1920x1080 from 2500x1120
> [ 1650.149876] ov5640 0-003c: sysclk=672000000, mipi_div=1
> Measured clock lane frequency: 330MHz
> 
> So it seems that formula must also take care of mipi_div:
> link_frequency = sys_div / 2 / mipi_div
> 
> 
> Unfortunately, as said previously, when updating ov5640 codebase so that
> CID_LINK_FREQUENCY returns this computed value instead of 384MHz fixed
> value, the MIPID02 bridge has some troubles and some
> resolutions/framerate are broken:
> 
> dmesg -n8
> echo "format link_freq +p" > /sys/kernel/debug/dynamic_debug/control
> 
> * VGA @ 30fps => broken
> v4l2-ctl --set-parm=30;v4l2-ctl
> --set-fmt-video=width=640,height=480,pixelformat=YUYV --stream-mmap
> [ 1624.854273] ov5640 0-003c: rate=491443200, 640x480 from 1896x1080
> [ 1624.859192] ov5640 0-003c: sysclk=492000000, mipi_div=2
> [ 1624.924287] st-mipid02 1-0014: detect link_freq = 123000000 Hz
> ==> no frames sent to DCMI
> 
> * 720p @ 30fps => very slow framerate
> v4l2-ctl --set-parm=30;v4l2-ctl
> --set-fmt-video=width=1280,height=720,pixelformat=YUYV --stream-mmap
> [ 1900.707822] ov5640 0-003c: rate=336019200, 1280x720 from 1892x740
> [ 1900.712717] ov5640 0-003c: sysclk=340000000, mipi_div=1
> [ 1900.782402] st-mipid02 1-0014: detect link_freq = 170000000 Hz
> << 0.69 fps
> < 0.67 fps
> < 0.67 fps
> 
> * 1080p @ 30fps => very slow framerate
> v4l2-ctl --set-parm=30;v4l2-ctl
> --set-fmt-video=width=1920,height=1080,pixelformat=YUYV --stream-mmap
> [ 1366.550832] ov5640 0-003c: rate=336000000, 1920x1080 from 2500x1120
> [ 1366.555668] ov5640 0-003c: sysclk=336000000, mipi_div=1
> Frame rate set to 30.000 fps
> [ 1367.105503] ov5640 0-003c: rate=672000000, 1920x1080 from 2500x1120
> [ 1367.110420] ov5640 0-003c: sysclk=672000000, mipi_div=1
> [ 1367.214498] st-mipid02 1-0014: detect link_freq = 336000000 Hz
> << 0.11 fps
> < 0.11 fps
> 
> 
> * 1080p @ 15fps => OK, 15fps as expected
> v4l2-ctl --set-parm=15;v4l2-ctl
> --set-fmt-video=width=1920,height=1080,pixelformat=YUYV --stream-mmap
> [ 1327.417545] ov5640 0-003c: rate=336000000, 1920x1080 from 2500x1120
> [ 1327.422560] ov5640 0-003c: sysclk=336000000, mipi_div=1
> [ 1327.527852] st-mipid02 1-0014: detect link_freq = 168000000 Hz
> <<<<<<<<<<<<<<<<< 15.09 fps
> <<<<<<<<<<<<<<< 15.04 fps
> 
> 
> One could argue that this is the mipid02 bridge codebase that is wrong:
> drivers/media/i2c/st-mipid02.c:
>     mipid02_configure_from_rx_speed()
>       [...]
>       bridge->r.clk_lane_reg1 |= (2000000000 / link_freq) << 2;
> 
> but computation is inline with MIPID02 spec:
> 
> "CLOCK LANE 1 REGISTER"
> Bit_no   Bit_name
> 7        ui_x4_clk_lane[5]
> 6        ui_x4_clk_lane[4]
> 5        ui_x4_clk_lane[3]
> 4        ui_x4_clk_lane[2]
> 3        ui_x4_clk_lane[1]
> 2        ui_x4_clk_lane[0]
> Unit interval time multiplied by four
> This signal indicates the bit period in units of
> 0.25 ns. If the unit interval is 3 ns, twelve (0x0C)
> should be programmed. This value is used to
> generate delays. Therefore, if the period is not a
> multiple of 0.25 ns, the value should be rounded
> down. For example, a 600 Mbit/s single lane
> linkuses a unit interval of 1.667 ns. Multiplying
> by four results in 6.667. In this case, a value of 6
> (not 7) should be programmed.
> 
> 
> Perhaps that someone which knows well CSI-2 interfaces could
> detect what is wrong here, but personally I don't.
> 
> 
>> However, this only work assuming the here below [*], and we're still
>> missing something because RAW still doesn't work as expected :(
>>
>> Anyway, clock tree madness apart, fixing the LINK_FREQ control value
>> to 96MHz seems like it does not reflect what the driver actual does.
>>
>> Could you try setting the control value to what the driver actually
>> computes as here suggested?
>>
>> Thanks
>>      j
>>
>> PS. I'm still confused why the have the link-frequencies property in
>> DTS bindings. What is its purpose if all drivers compute or hardcode
>> it?
> 
> I share this point of view, I don't know how to return a dynamically
> computed link frequency. Current drivers are exposing a single one
> or a set of discrete frequencies.
> For a matter of test purpose, I've done this hack:
> 
> -static const s64 link_freq_menu_items[] = {
> +static s64 link_freq_menu_items[] = {
> 	384000000,
> };
> 
> +	link_freq_menu_items[0] = _rate / 2 / mipi_div;
> 
> 
>>
>> [*] There is a big assumption here, that the MIPI bus clock gets
>> generated by two different clock outputs depending if the mode goes
>> through the the scaler or not. You can see this at line 967 in the
>> driver code. If mode goes though the scaler, MIPI_DIV = 2 and you get
>> the bus clock from the MIPISCLK signal. If it does not go through the
>> scaler MIPI_DIV = 1 and then the MIPI bus clock gets provided by the
>> MIPI_CLK signal. This is the result of several attempts to decode the
>> ov5640 clock tree, but it has only been tested with modes with bpp =
>> 16 and 2 data lanes. As it has been recently reported, for RAW mode,
>> this does not work well, so the assumption might not be correct.
>>
>>>
>>>>
>>>>
>>>>>> version 2:
>>>>>>      - do not set control read only as per Hans' comment:
>>>>>>        See https://www.mail-archive.com/linux-media@vger.kernel.org/msg147910.html
>>>>>>
>>>>>>     drivers/media/i2c/ov5640.c | 10 ++++++++++
>>>>>>     1 file changed, 10 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>>>>>> index 82d4ce9..e6307f3 100644
>>>>>> --- a/drivers/media/i2c/ov5640.c
>>>>>> +++ b/drivers/media/i2c/ov5640.c
>>>>>> @@ -218,6 +218,7 @@ struct ov5640_ctrls {
>>>>>>     	struct v4l2_ctrl *test_pattern;
>>>>>>     	struct v4l2_ctrl *hflip;
>>>>>>     	struct v4l2_ctrl *vflip;
>>>>>> +	struct v4l2_ctrl *link_freq;
>>>>>>     };
>>>>>>
>>>>>>     struct ov5640_dev {
>>>>>> @@ -2198,6 +2199,10 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd,
>>>>>>     	return 0;
>>>>>>     }
>>>>>>
>>>>>> +static const s64 link_freq_menu_items[] = {
>>>>>> +	384000000,
>>>>>> +};
>>>>>> +
>>>>>>     static int ov5640_set_fmt(struct v4l2_subdev *sd,
>>>>>>     			  struct v4l2_subdev_pad_config *cfg,
>>>>>>     			  struct v4l2_subdev_format *format)
>>>>>> @@ -2636,6 +2641,8 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
>>>>>>     	case V4L2_CID_VFLIP:
>>>>>>     		ret = ov5640_set_ctrl_vflip(sensor, ctrl->val);
>>>>>>     		break;
>>>>>> +	case V4L2_CID_LINK_FREQ:
>>>>>> +		return 0;
>>>>>>     	default:
>>>>>>     		ret = -EINVAL;
>>>>>>     		break;
>>>>>> @@ -2703,6 +2710,9 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
>>>>>>     				       V4L2_CID_POWER_LINE_FREQUENCY_AUTO, 0,
>>>>>>     				       V4L2_CID_POWER_LINE_FREQUENCY_50HZ);
>>>>>>
>>>>>> +	ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
>>>>>> +						  0, 0, link_freq_menu_items);
>>>>>> +
>>>>>>     	if (hdl->error) {
>>>>>>     		ret = hdl->error;
>>>>>>     		goto free_ctrls;
>>>>>> --
>>>>>> 2.7.4
>>>>>>
>>>>>
>>>>
>>>> BR,
>>>> Hugues.
>>>
>>> --
>>> Sakari Ailus
>>> sakari.ailus@linux.intel.com
> 
> BR,
> Hugues.
> 
BR,
Hugues.

Jacopo Mondi Oct. 21, 2020, 4:51 p.m. UTC | #7

Hi Hugues,

On Mon, Oct 19, 2020 at 01:40:26PM +0000, Hugues FRUCHET wrote:
> Hi Jacopo, all,
>
> Back on this subject recently, I've found interesting things around
> register 0x4837 PCLK PERIOD thanks to Samuel Bobrowicz work on PLL
> settings [1], but 0x4837 register value computation differs from what
> I've understood/measured.

Thanks, very useful to know!

>
> The OV5640 PCLK_PERIOD register value must be in line with the
> ST MIPID02 CSI-2 receiver CLOCK_LANE_1 register, otherwise receptions
> issues are encountered has shown in my previous mail.
>
> Configuring those 2 registers according to measured MIPI clock frequency
> allows to cover almost all resolutions/formats (RGB565, YUV422, JPEG).
> I've also measured with oscilloscope the MIPI clock-lane frequency and
> verified that this is well matching computed one [2].

Is this based on the current mainline driver ? I still wonder if
Tomi's patch that adjusts the vtot and htot value to reflect datasheet
needs to be applied.

> Please note that due to platform limitation, I could not test link
> frequency above 720p@15fps, so additional tests are needed with platform
> that could handle 5Mp@15fps (OV5640 limit).
>
> In term of code, this mean:
>
> v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
>         		  0, 0, link_freq_menu_items);
>
> ov5640_set_mipi_pclk(rate)
> {
>    mipi_sclk = ov5640_calc_sys_clk(sensor, rate, &prediv, &mult, &sysdiv);
>    link_freq = mipi_sclk / 2 / mipi_div; /* the CSI-2 clock-lane
> frequency, measurements are matching */
>
>    _pclk_period = (u8) (2000000000UL / link_freq);/* expressed in ns and

Is 2000000000UL to compensate for the / 2 above ?

> with 1-bit decimal: 0x1=0.5ns, 0x2=1ns, 0x3=1.5ns and so on... */
>    ov5640_write_reg(sensor, OV5640_REG_PCLK_PERIOD, _pclk_period);
>
>    link_freq_menu_items[0] = link_freq;/* Currently a hack to inform
> receiver about the dynamically computed clock frequency, I don't know
> yet how to handle this correctly, please advise... */

Can you call __v4l2_ctrl_s_ctrl on a menu control ?

>
>    ...
> }
>
> As explained in comment, I miss a V4L2 way to set dynamically
> V4L2_CID_LINK_FREQ in order to propose a new patchset.
>
>
> [1]
> https://patchwork.kernel.org/project/linux-media/patch/1539797508-127629-1-git-send-email-sam@elite-embedded.com/
>
> [2] measure of the CSI-2 clock frequency, tested with JPEG (easier to
> check that capture is fine) and various resolutions:
>

On my test platform setting 0x4837 with the value calculated as you
reported breaks capture. I'll investigate if I need to setup anything
on the receiver side as you have to (it's an i.MX6, if anyone reading
knows that platform better than what I do :)

Thanks
  j

> ****************
> ***  QQVGA  ****
> ****************
> * JPEG QQVGA@15fps
> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl
> --set-fmt-video=width=160,height=120,pixelformat=JPEG --stream-mmap
> --stream-count=-1
> [  371.459607] ov5640 1-003c: Bandwidth Per Lane=223879680, 160x120 from
> 1896x984
> [  371.465516] ov5640 1-003c: mipi_sclk=224000000, mipi_div=2, prediv=3,
> mult=28, sysdiv=1
> [  371.473727] ov5640 1-003c: link freq=56000000 Hz
> [  371.478529] ov5640 1-003c: PCLK PERIOD 0x4837=0x23
> [  371.538193] st-mipid02 2-0014: clk_lane_reg1=0x8d
> <<<<<<<<<<<<<<<<< 15.09 fps
> <<<<<<<<<<<<<<< 15.04 fps
> <<<<<<<<<<<<<<< 15.03 fps
> <<<<<<<<<<<<<<< 15.02 fps
> <<<<<<<<<<<<<<< 15.01 fps
> Clock-lane frequency measured at #18ns (55.5MHz)
>
>
> * RGB565 QQVGA@15fps
> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl
> --set-fmt-video=width=160,height=120,pixelformat=RGBP --stream-mmap
> --stream-count=-1
> [ 1229.422079] st-mipid02 2-0014: clk_lane_reg1=0x8d
> [ 1229.431758] ov5640 1-003c: Bandwidth Per Lane=223879680, 160x120 from
> 1896x984
> [ 1229.437905] ov5640 1-003c: mipi_sclk=224000000, mipi_div=2, prediv=3,
> mult=28, sysdiv=1
> [ 1229.445620] ov5640 1-003c: link freq=56000000 Hz
> [ 1229.450397] ov5640 1-003c: PCLK PERIOD 0x4837=0x23
> <<<<<<<<<<<<<<<<< 15.09 fps
> <<<<<<<<<<<<<<< 15.04 fps
> <<<<<<<<<<<<<< 14.70 fps
> <<<<<<<<<<<<<<< 14.77 fps
> Measured #18ns (55.5MHz)
>
>
> **************
> ***  VGA  ****
> **************
> * JPEG VGA@15fps
> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl
> --set-fmt-video=width=640,height=480,pixelformat=JPEG --stream-mmap
> --stream-count=-1
> [ 1105.621884] ov5640 1-003c: Bandwidth Per Lane=245721600, 640x480 from
> 1896x1080
> [ 1105.627927] ov5640 1-003c: mipi_sclk=246000000, mipi_div=2, prediv=3,
> mult=123, sysdiv=4
> [ 1105.641273] ov5640 1-003c: link freq=61500000 Hz
> [ 1105.646789] ov5640 1-003c: PCLK PERIOD 0x4837=0x40
> [ 1105.709626] st-mipid02 2-0014: clk_lane_reg1=0x81
> <<<<<<<<<<<<<<<<< 15.09 fps
> <<<<<<<<<<<<<<< 15.04 fps
> <<<<<<<<<<<<<<< 15.03 fps
> <<<<<<<<<<<<<<< 15.02 fps
> <<<<<<<<<<<<<<< 15.01 fps
>
> Measured #16ns (62.5MHz)
>
>
> * JPEG VGA@30fps
> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=30;v4l2-ctl
> --set-fmt-video=width=640,height=480,pixelformat=JPEG --stream-mmap
> --stream-count=-1
> [ 1144.539082] ov5640 1-003c: Bandwidth Per Lane=491443200, 640x480 from
> 1896x1080
> [ 1144.545143] ov5640 1-003c: mipi_sclk=492000000, mipi_div=2, prediv=3,
> mult=123, sysdiv=2
> [ 1144.558756] ov5640 1-003c: link freq=123000000 Hz
> [ 1144.562012] ov5640 1-003c: PCLK PERIOD 0x4837=0x20
> [ 1144.623895] st-mipid02 2-0014: clk_lane_reg1=0x41
> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.09 fps
> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.04 fps
> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.13 fps
>
> Measured #8ns (125MHz)
>
>
> *******************
> ***  1024x768  ****
> *******************
> * JPEG 1024x768@15fps
> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl
> --set-fmt-video=width=1024,height=768,pixelformat=JPEG --stream-mmap
> --stream-count=-1
> <<<<<<<<<<<<<<<<< 15.09 fps
> <<<<<<<<<<<<<<< 15.04 fps
> <<<<<<<<<<<<<<< 15.03 fps
> <<<<<<<<<<<<<<< 15.02 fps
>
> Measured #16ns (62.5MHz)
>
>
> * JPEG 1024x768@30fps
> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=30;v4l2-ctl
> --set-fmt-video=width=1024,height=768,pixelformat=JPEG --stream-mmap
> --stream-count=-1
> [ 1218.632061] ov5640 1-003c: Bandwidth Per Lane=491443200, 1024x768
> from 1896x1080
> [ 1218.638216] ov5640 1-003c: mipi_sclk=492000000, mipi_div=2, prediv=3,
> mult=123, sysdiv=2
> [ 1218.651788] ov5640 1-003c: link freq=123000000 Hz
> [ 1218.655345] ov5640 1-003c: PCLK PERIOD 0x4837=0x20
> [ 1218.724149] st-mipid02 2-0014: clk_lane_reg1=0x41
> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.09 fps
> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.04 fps
> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.13 fps
> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.09 fps
> Measured #8ns (125MHz)
>
>
> ***************
> ***  720p  ****
> ***************
> * JPEG 720p@15fps
> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl
> --set-fmt-video=width=1280,height=720,pixelformat=JPEG --stream-mmap
> --stream-count=-1
> [ 7781.232498] ov5640 1-003c: Bandwidth Per Lane=168009600, 1280x720
> from 1892x740
> [ 7781.238554] ov5640 1-003c: mipi_sclk=169600000, mipi_div=1, prediv=3,
> mult=106, sysdiv=5
> [ 7781.247117] ov5640 1-003c: link freq=84800000 Hz
> [ 7781.251116] ov5640 1-003c: PCLK PERIOD 0x4837=0x17
> [ 7781.316980] st-mipid02 2-0014: clk_lane_reg1=0x5d
>
> Measured #12ns (83MHz)
>
> * JPEG 720p@30fps
> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=30;v4l2-ctl
> --set-fmt-video=width=1280,height=720,pixelformat=JPEG --stream-mmap
> --stream-count=-1
> [  515.548519] ov5640 1-003c: Bandwidth Per Lane=336019200, 1280x720
> from 1892x740
> [  515.554646] ov5640 1-003c: mipi_sclk=340000000, mipi_div=1, prediv=3,
> mult=85, sysdiv=2
> [  515.562406] ov5640 1-003c: link freq=170000000 Hz
> [  515.574376] ov5640 1-003c: PCLK PERIOD 0x4837=0xb
> [  515.639862] st-mipid02 2-0014: clk_lane_reg1=0x2d
> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.39 fps
> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.50 fps
> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.36 fps
> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.42 fps
> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.40 fps
> Measured #6ns (166MHz)
> ==> 30fps but image artefacts (out of MP1 capabilities)
>
>
> ****************
> ***  1080p  ****
> ****************
> * JPEG HD@15fps
> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl
> --set-fmt-video=width=1920,height=1080,pixelformat=JPEG --stream-mmap
> --stream-count=-1
> [ 1998.266915] ov5640 1-003c: Bandwidth Per Lane=336000000, 1920x1080
> from 2500x1120
> [ 1998.273040] ov5640 1-003c: mipi_sclk=336000000, mipi_div=1, prediv=3,
> mult=42, sysdiv=1
> [ 1998.280987] ov5640 1-003c: link freq=168000000 Hz
> [ 1998.286142] ov5640 1-003c: PCLK PERIOD 0x4837=0xb
> [ 1998.380206] st-mipid02 2-0014: clk_lane_reg1=0x2d
> <<< 1.88 fps
> << 1.87 fps
> << 1.88 fps
> << 1.87 fps
> << 1.87 fps
> Measured #6ns (166MHz)
>
> * JPEG HD@30fps
> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=30;v4l2-ctl
> --set-fmt-video=width=1920,height=1080,pixelformat=JPEG --stream-mmap
> --stream-count=-1
> [ 2196.981767] ov5640 1-003c: Bandwidth Per Lane=672000000, 1920x1080
> from 2500x1120
> [ 2196.987882] ov5640 1-003c: mipi_sclk=672000000, mipi_div=1, prediv=3,
> mult=84, sysdiv=1
> [ 2196.996225] ov5640 1-003c: link freq=336000000 Hz
> [ 2197.000553] ov5640 1-003c: PCLK PERIOD 0x4837=0x5
> [ 2197.097244] st-mipid02 2-0014: clk_lane_reg1=0x15
> Measured #3ns (333MHz)
>
> ==> no images but out of MP1 capabilities
>
>
> **************
> ***  5Mp  ****
> **************
> * JPEG 5Mp@15fps
> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl
> --set-fmt-video=width=2592,height=1944,pixelformat=JPEG --stream-mmap
> --stream-count=-1
>
> [ 2479.203457] ov5640 1-003c: Bandwidth Per Lane=671639040, 2592x1944
> from 2844x1968
> [ 2479.209647] ov5640 1-003c: mipi_sclk=672000000, mipi_div=1, prediv=3,
> mult=84, sysdiv=1
> [ 2479.217961] ov5640 1-003c: link freq=336000000 Hz
> [ 2479.222240] ov5640 1-003c: PCLK PERIOD 0x4837=0x5
> [ 2479.388400] st-mipid02 2-0014: clk_lane_reg1=0x15
>
> Measured #3ns (333MHz)
>
> ==> no images but out of MP1 capabilities
>
>
>
> On 9/3/19 4:48 PM, Hugues FRUCHET wrote:
> > Hi Jacopo,
> >
> > On 8/21/19 4:24 PM, Jacopo Mondi wrote:
> >> Hello,
> >>      +laura who has been working on supporting RAW capture for this
> >>      driver.
> >>
> >> On Tue, Aug 20, 2019 at 12:13:12PM +0300, Sakari Ailus wrote:
> >>> Hi Hugues,
> >>>
> >>> On Tue, Jul 02, 2019 at 04:05:46PM +0000, Hugues FRUCHET wrote:
> >>>> Hi Sakari,
> >>>>
> >>>> On 6/27/19 6:05 PM, Sakari Ailus wrote:
> >>>>> Hi Hugues,
> >>>>>
> >>>>> On Thu, Jun 27, 2019 at 02:57:04PM +0200, Hugues Fruchet wrote:
> >>>>>> Add support of V4L2_CID_LINK_FREQ, this is needed
> >>>>>> by some CSI-2 receivers.
> >>>>>>
> >>>>>> 384MHz is exposed for the time being, corresponding
> >>>>>> to 96MHz pixel clock with 2 bytes per pixel on 2 data lanes.
> >>>>>>
> >>>>>> This setup has been tested successfully with ST MIPID02
> >>>>>> CSI-2 to parallel bridge.
> >>>>>>
> >>>>>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> >>>>>> ---
> >>>>>
> >>>>> Thanks for the patch.
> >>>>>
> >>>>> The driver calculates the sensor configuration based on its configuration,
> >>>>> and this needs to be reflected in the link frequency: it's not a static
> >>>>> value. See e.g. ov5640_calc_sys_clk().
> >>>>>
> >>>>
> >>>> I know this code, but for a reason I don't understand yet, this seems
> >>>> to not have effects on the CSI-2 link frequency.
> >>>>
> >>
> >> This seems unlikely to me, as the ov5640_calc_sys_clk() calculates the
> >> system clock, which then generates the MIPI CLK.
> >>
> >> I would really be interested to know if you could measure somehow the
> >> actual frequency of the clock lane, to make sure it actually
> >> changes according to to the driver calculations.
> >>
> >>>> This has been verified with MIPID02 CSI-2 bridge which only work with
> >>>> this fixed link frequency as input to program its ui_x4 register
> >>>> setting, see
> >>>> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2028171.html.
> >>>> All resolutions and all framerate have been tested succesfully with this
> >>>> setting.
> >>>>
> >>>> Initially I tried to set the link frequency according to the value
> >>>> computed in ov5640_calc_sys_clk() but it was only functional when
> >>>> resolutions/framerate was close to the 384MHz value...
> >>>>
> >>>> As OV5640 D3 engineering board has been initially developped for
> >>>> dragonboard, I'll dig into this and found this commit:
> >>>> https://lore.kernel.org/patchwork/patch/886794/
> >>>> which set a fixed pixel rate value to 96MHz, which match perfectly
> >>>> with the 384MHz value I found for link frequency...
> >>>>
> >>>> Perhaps other CSI-2 OV5640 users can comment about it and have the
> >>>> explanations of what I experiment...
> >>>> Maxime, Jacopo, do you have any idea about it ?
> >>>
> >>> I would also like to wee a comment from someone who's familiar with the
> >>> device. Yet I can tell a static value of 348 MHz is clearly incorrect as it
> >>> ignores sensor runtime configuration as well as platform configuration
> >>> (external clock frequency for instance).
> >>>
> >>> Generally speaking, configuring a CSI-2 receiver to expect a particular
> >>> frequency usually doesn't mean it's going to fail even if the transmitter
> >>> uses a different frequency, albeit the likelihood of it not working
> >>> increases as the difference grows. Could the problem be at the receiver's
> >>> end?
> >>>
> >>> Have you checked what kind of values ov5640_calc_pclk() returns for
> >>> different configurations? It would seem like that this is what the
> >>> LINK_FREQ (divided by lanes and multiplied by two) should be.
> >>
> >> Not exactly. ov5640_calc_pclk() is only used when computing the
> >> parallel bus pixel clock not for the CSI-2 bus.
> >>
> >> To obtain the link frequency for the MIPI interface I would consider
> >> the system clock frequency calculated by ov5640_calc_sys_clk(), which
> >> represents the bandwidth per lane in bits per second. This should then
> >> be divided by 2 to compensate the MIPI DDR mode [*] (no need to divide
> >> by the number of lanes, as this is already the bandwidth -per lane-)
> >>
> >> For 640x480 YUYV8_2X8 (and 1024x768) with 2 data lanes and 24MHz xclk
> >> I have calculated the following values:
> >>
> >> SYSCLK = 492MHz
> >> MIPISCLK = 256MHz
> >> PCLK = 61,50MHz
> >>
> >> For 1280x720 with the same setup
> >> SYSCLK = 340MHz
> >> MIPI_CLK = 170MHz
> >> PCLK = 42,50 (this seems veeery low according to datasheet)
> >>
> >> For 1920x1080
> >> SYSCLK = 680MHz
> >> MIPI_CLK = 340MHz
> >> PCLK = 85MHz (slightly slow according to the datasheet)
> >>
> >> All captured images seems fine...
> >>
> >> If you could confirm those values by probing the actual clock lane it
> >> would be just great.
> >>
> >
> > I've made some measurements probing clock lane with YUYV 16 bits
> > grabbing, here are the results:
> >
> > * VGA @ 30fps
> > v4l2-ctl --set-parm=30;v4l2-ctl
> > --set-fmt-video=width=640,height=480,pixelformat=YUYV --stream-mmap
> > [ 1624.854273] ov5640 0-003c: rate=491443200, 640x480 from 1896x1080
> > [ 1624.859192] ov5640 0-003c: sysclk=492000000, mipi_div=2
> > Measured clock lane frequency: 123MHz
> >
> > * 1024x768 @ 30fps
> > v4l2-ctl --set-parm=30;v4l2-ctl
> > --set-fmt-video=width=1024,height=768,pixelformat=YUYV --stream-mmap
> > [  558.833885] ov5640 0-003c: rate=491443200, 1024x768 from 1896x1080
> > [  558.840188] ov5640 0-003c: sysclk=492000000, mipi_div=2
> > Measured clock lane frequency: 124MHz
> >
> > * 720p @ 30fps
> > v4l2-ctl --set-parm=30;v4l2-ctl
> > --set-fmt-video=width=1280,height=720,pixelformat=YUYV --stream-mmap
> > [ 2920.785392] ov5640 0-003c: rate=336019200, 1280x720 from 1892x740
> > [ 2920.794771] ov5640 0-003c: sysclk=340000000, mipi_div=1
> > Measured clock lane frequency: 171MHz
> >
> > * 1080p @ 30fps
> > v4l2-ctl --set-parm=30;v4l2-ctl
> > --set-fmt-video=width=1920,height=1080,pixelformat=YUYV --stream-mmap
> > [ 1650.143701] ov5640 0-003c: rate=672000000, 1920x1080 from 2500x1120
> > [ 1650.149876] ov5640 0-003c: sysclk=672000000, mipi_div=1
> > Measured clock lane frequency: 330MHz
> >
> > So it seems that formula must also take care of mipi_div:
> > link_frequency = sys_div / 2 / mipi_div
> >
> >
> > Unfortunately, as said previously, when updating ov5640 codebase so that
> > CID_LINK_FREQUENCY returns this computed value instead of 384MHz fixed
> > value, the MIPID02 bridge has some troubles and some
> > resolutions/framerate are broken:
> >
> > dmesg -n8
> > echo "format link_freq +p" > /sys/kernel/debug/dynamic_debug/control
> >
> > * VGA @ 30fps => broken
> > v4l2-ctl --set-parm=30;v4l2-ctl
> > --set-fmt-video=width=640,height=480,pixelformat=YUYV --stream-mmap
> > [ 1624.854273] ov5640 0-003c: rate=491443200, 640x480 from 1896x1080
> > [ 1624.859192] ov5640 0-003c: sysclk=492000000, mipi_div=2
> > [ 1624.924287] st-mipid02 1-0014: detect link_freq = 123000000 Hz
> > ==> no frames sent to DCMI
> >
> > * 720p @ 30fps => very slow framerate
> > v4l2-ctl --set-parm=30;v4l2-ctl
> > --set-fmt-video=width=1280,height=720,pixelformat=YUYV --stream-mmap
> > [ 1900.707822] ov5640 0-003c: rate=336019200, 1280x720 from 1892x740
> > [ 1900.712717] ov5640 0-003c: sysclk=340000000, mipi_div=1
> > [ 1900.782402] st-mipid02 1-0014: detect link_freq = 170000000 Hz
> > << 0.69 fps
> > < 0.67 fps
> > < 0.67 fps
> >
> > * 1080p @ 30fps => very slow framerate
> > v4l2-ctl --set-parm=30;v4l2-ctl
> > --set-fmt-video=width=1920,height=1080,pixelformat=YUYV --stream-mmap
> > [ 1366.550832] ov5640 0-003c: rate=336000000, 1920x1080 from 2500x1120
> > [ 1366.555668] ov5640 0-003c: sysclk=336000000, mipi_div=1
> > Frame rate set to 30.000 fps
> > [ 1367.105503] ov5640 0-003c: rate=672000000, 1920x1080 from 2500x1120
> > [ 1367.110420] ov5640 0-003c: sysclk=672000000, mipi_div=1
> > [ 1367.214498] st-mipid02 1-0014: detect link_freq = 336000000 Hz
> > << 0.11 fps
> > < 0.11 fps
> >
> >
> > * 1080p @ 15fps => OK, 15fps as expected
> > v4l2-ctl --set-parm=15;v4l2-ctl
> > --set-fmt-video=width=1920,height=1080,pixelformat=YUYV --stream-mmap
> > [ 1327.417545] ov5640 0-003c: rate=336000000, 1920x1080 from 2500x1120
> > [ 1327.422560] ov5640 0-003c: sysclk=336000000, mipi_div=1
> > [ 1327.527852] st-mipid02 1-0014: detect link_freq = 168000000 Hz
> > <<<<<<<<<<<<<<<<< 15.09 fps
> > <<<<<<<<<<<<<<< 15.04 fps
> >
> >
> > One could argue that this is the mipid02 bridge codebase that is wrong:
> > drivers/media/i2c/st-mipid02.c:
> >     mipid02_configure_from_rx_speed()
> >       [...]
> >       bridge->r.clk_lane_reg1 |= (2000000000 / link_freq) << 2;
> >
> > but computation is inline with MIPID02 spec:
> >
> > "CLOCK LANE 1 REGISTER"
> > Bit_no   Bit_name
> > 7        ui_x4_clk_lane[5]
> > 6        ui_x4_clk_lane[4]
> > 5        ui_x4_clk_lane[3]
> > 4        ui_x4_clk_lane[2]
> > 3        ui_x4_clk_lane[1]
> > 2        ui_x4_clk_lane[0]
> > Unit interval time multiplied by four
> > This signal indicates the bit period in units of
> > 0.25 ns. If the unit interval is 3 ns, twelve (0x0C)
> > should be programmed. This value is used to
> > generate delays. Therefore, if the period is not a
> > multiple of 0.25 ns, the value should be rounded
> > down. For example, a 600 Mbit/s single lane
> > linkuses a unit interval of 1.667 ns. Multiplying
> > by four results in 6.667. In this case, a value of 6
> > (not 7) should be programmed.
> >
> >
> > Perhaps that someone which knows well CSI-2 interfaces could
> > detect what is wrong here, but personally I don't.
> >
> >
> >> However, this only work assuming the here below [*], and we're still
> >> missing something because RAW still doesn't work as expected :(
> >>
> >> Anyway, clock tree madness apart, fixing the LINK_FREQ control value
> >> to 96MHz seems like it does not reflect what the driver actual does.
> >>
> >> Could you try setting the control value to what the driver actually
> >> computes as here suggested?
> >>
> >> Thanks
> >>      j
> >>
> >> PS. I'm still confused why the have the link-frequencies property in
> >> DTS bindings. What is its purpose if all drivers compute or hardcode
> >> it?
> >
> > I share this point of view, I don't know how to return a dynamically
> > computed link frequency. Current drivers are exposing a single one
> > or a set of discrete frequencies.
> > For a matter of test purpose, I've done this hack:
> >
> > -static const s64 link_freq_menu_items[] = {
> > +static s64 link_freq_menu_items[] = {
> > 	384000000,
> > };
> >
> > +	link_freq_menu_items[0] = _rate / 2 / mipi_div;
> >
> >
> >>
> >> [*] There is a big assumption here, that the MIPI bus clock gets
> >> generated by two different clock outputs depending if the mode goes
> >> through the the scaler or not. You can see this at line 967 in the
> >> driver code. If mode goes though the scaler, MIPI_DIV = 2 and you get
> >> the bus clock from the MIPISCLK signal. If it does not go through the
> >> scaler MIPI_DIV = 1 and then the MIPI bus clock gets provided by the
> >> MIPI_CLK signal. This is the result of several attempts to decode the
> >> ov5640 clock tree, but it has only been tested with modes with bpp =
> >> 16 and 2 data lanes. As it has been recently reported, for RAW mode,
> >> this does not work well, so the assumption might not be correct.
> >>
> >>>
> >>>>
> >>>>
> >>>>>> version 2:
> >>>>>>      - do not set control read only as per Hans' comment:
> >>>>>>        See https://www.mail-archive.com/linux-media@vger.kernel.org/msg147910.html
> >>>>>>
> >>>>>>     drivers/media/i2c/ov5640.c | 10 ++++++++++
> >>>>>>     1 file changed, 10 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> >>>>>> index 82d4ce9..e6307f3 100644
> >>>>>> --- a/drivers/media/i2c/ov5640.c
> >>>>>> +++ b/drivers/media/i2c/ov5640.c
> >>>>>> @@ -218,6 +218,7 @@ struct ov5640_ctrls {
> >>>>>>     	struct v4l2_ctrl *test_pattern;
> >>>>>>     	struct v4l2_ctrl *hflip;
> >>>>>>     	struct v4l2_ctrl *vflip;
> >>>>>> +	struct v4l2_ctrl *link_freq;
> >>>>>>     };
> >>>>>>
> >>>>>>     struct ov5640_dev {
> >>>>>> @@ -2198,6 +2199,10 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd,
> >>>>>>     	return 0;
> >>>>>>     }
> >>>>>>
> >>>>>> +static const s64 link_freq_menu_items[] = {
> >>>>>> +	384000000,
> >>>>>> +};
> >>>>>> +
> >>>>>>     static int ov5640_set_fmt(struct v4l2_subdev *sd,
> >>>>>>     			  struct v4l2_subdev_pad_config *cfg,
> >>>>>>     			  struct v4l2_subdev_format *format)
> >>>>>> @@ -2636,6 +2641,8 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
> >>>>>>     	case V4L2_CID_VFLIP:
> >>>>>>     		ret = ov5640_set_ctrl_vflip(sensor, ctrl->val);
> >>>>>>     		break;
> >>>>>> +	case V4L2_CID_LINK_FREQ:
> >>>>>> +		return 0;
> >>>>>>     	default:
> >>>>>>     		ret = -EINVAL;
> >>>>>>     		break;
> >>>>>> @@ -2703,6 +2710,9 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
> >>>>>>     				       V4L2_CID_POWER_LINE_FREQUENCY_AUTO, 0,
> >>>>>>     				       V4L2_CID_POWER_LINE_FREQUENCY_50HZ);
> >>>>>>
> >>>>>> +	ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
> >>>>>> +						  0, 0, link_freq_menu_items);
> >>>>>> +
> >>>>>>     	if (hdl->error) {
> >>>>>>     		ret = hdl->error;
> >>>>>>     		goto free_ctrls;
> >>>>>> --
> >>>>>> 2.7.4
> >>>>>>
> >>>>>
> >>>>
> >>>> BR,
> >>>> Hugues.
> >>>
> >>> --
> >>> Sakari Ailus
> >>> sakari.ailus@linux.intel.com
> >
> > BR,
> > Hugues.
> >
> BR,
> Hugues.

Hugues FRUCHET Oct. 22, 2020, 8:14 a.m. UTC | #8

Hi Jacopo,

On 10/21/20 6:51 PM, Jacopo Mondi wrote:
> Hi Hugues,
> 
> On Mon, Oct 19, 2020 at 01:40:26PM +0000, Hugues FRUCHET wrote:
>> Hi Jacopo, all,
>>
>> Back on this subject recently, I've found interesting things around
>> register 0x4837 PCLK PERIOD thanks to Samuel Bobrowicz work on PLL
>> settings [1], but 0x4837 register value computation differs from what
>> I've understood/measured.
> 
> Thanks, very useful to know!
> 
>>
>> The OV5640 PCLK_PERIOD register value must be in line with the
>> ST MIPID02 CSI-2 receiver CLOCK_LANE_1 register, otherwise receptions
>> issues are encountered has shown in my previous mail.
>>
>> Configuring those 2 registers according to measured MIPI clock frequency
>> allows to cover almost all resolutions/formats (RGB565, YUV422, JPEG).
>> I've also measured with oscilloscope the MIPI clock-lane frequency and
>> verified that this is well matching computed one [2].
> 
> Is this based on the current mainline driver ? I still wonder if
> Tomi's patch that adjusts the vtot and htot value to reflect datasheet
> needs to be applied.

Yes it is based on latest media codebase, so no vtot/htot changes as 
done by Tomi (status of those changes is not clear on my side, but I 
could test on my platform, just let me know).

> 
>> Please note that due to platform limitation, I could not test link
>> frequency above 720p@15fps, so additional tests are needed with platform
>> that could handle 5Mp@15fps (OV5640 limit).
>>
>> In term of code, this mean:
>>
>> v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
>>          		  0, 0, link_freq_menu_items);
>>
>> ov5640_set_mipi_pclk(rate)
>> {
>>     mipi_sclk = ov5640_calc_sys_clk(sensor, rate, &prediv, &mult, &sysdiv);
>>     link_freq = mipi_sclk / 2 / mipi_div; /* the CSI-2 clock-lane
>> frequency, measurements are matching */
>>
>>     _pclk_period = (u8) (2000000000UL / link_freq);/* expressed in ns and
> 
> Is 2000000000UL to compensate for the / 2 above ?

No, it is for value of register expressed in 0.5ns unit as explained in 
comment.

> 
>> with 1-bit decimal: 0x1=0.5ns, 0x2=1ns, 0x3=1.5ns and so on... */
>>     ov5640_write_reg(sensor, OV5640_REG_PCLK_PERIOD, _pclk_period);
>>
>>     link_freq_menu_items[0] = link_freq;/* Currently a hack to inform
>> receiver about the dynamically computed clock frequency, I don't know
>> yet how to handle this correctly, please advise... */
> 
> Can you call __v4l2_ctrl_s_ctrl on a menu control ?
> 

I'll give it a try...

>>
>>     ...
>> }
>>
>> As explained in comment, I miss a V4L2 way to set dynamically
>> V4L2_CID_LINK_FREQ in order to propose a new patchset.
>>
>>
>> [1]
>> https://patchwork.kernel.org/project/linux-media/patch/1539797508-127629-1-git-send-email-sam@elite-embedded.com/
>>
>> [2] measure of the CSI-2 clock frequency, tested with JPEG (easier to
>> check that capture is fine) and various resolutions:
>>
> 
> On my test platform setting 0x4837 with the value calculated as you
> reported breaks capture. I'll investigate if I need to setup anything
> on the receiver side as you have to (it's an i.MX6, if anyone reading
> knows that platform better than what I do :)
> 
> Thanks
>    j
> 

From
https://www.nxp.com/docs/en/application-note/AN5305.pdf

I understand that you have to set MIPI_CSI_TST_CTRL1 register according 
to link frequency as per table 4.

>> ****************
>> ***  QQVGA  ****
>> ****************
>> * JPEG QQVGA@15fps
>> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl
>> --set-fmt-video=width=160,height=120,pixelformat=JPEG --stream-mmap
>> --stream-count=-1
>> [  371.459607] ov5640 1-003c: Bandwidth Per Lane=223879680, 160x120 from
>> 1896x984
>> [  371.465516] ov5640 1-003c: mipi_sclk=224000000, mipi_div=2, prediv=3,
>> mult=28, sysdiv=1
>> [  371.473727] ov5640 1-003c: link freq=56000000 Hz
>> [  371.478529] ov5640 1-003c: PCLK PERIOD 0x4837=0x23
>> [  371.538193] st-mipid02 2-0014: clk_lane_reg1=0x8d
>> <<<<<<<<<<<<<<<<< 15.09 fps
>> <<<<<<<<<<<<<<< 15.04 fps
>> <<<<<<<<<<<<<<< 15.03 fps
>> <<<<<<<<<<<<<<< 15.02 fps
>> <<<<<<<<<<<<<<< 15.01 fps
>> Clock-lane frequency measured at #18ns (55.5MHz)
>>
>>
>> * RGB565 QQVGA@15fps
>> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl
>> --set-fmt-video=width=160,height=120,pixelformat=RGBP --stream-mmap
>> --stream-count=-1
>> [ 1229.422079] st-mipid02 2-0014: clk_lane_reg1=0x8d
>> [ 1229.431758] ov5640 1-003c: Bandwidth Per Lane=223879680, 160x120 from
>> 1896x984
>> [ 1229.437905] ov5640 1-003c: mipi_sclk=224000000, mipi_div=2, prediv=3,
>> mult=28, sysdiv=1
>> [ 1229.445620] ov5640 1-003c: link freq=56000000 Hz
>> [ 1229.450397] ov5640 1-003c: PCLK PERIOD 0x4837=0x23
>> <<<<<<<<<<<<<<<<< 15.09 fps
>> <<<<<<<<<<<<<<< 15.04 fps
>> <<<<<<<<<<<<<< 14.70 fps
>> <<<<<<<<<<<<<<< 14.77 fps
>> Measured #18ns (55.5MHz)
>>
>>
>> **************
>> ***  VGA  ****
>> **************
>> * JPEG VGA@15fps
>> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl
>> --set-fmt-video=width=640,height=480,pixelformat=JPEG --stream-mmap
>> --stream-count=-1
>> [ 1105.621884] ov5640 1-003c: Bandwidth Per Lane=245721600, 640x480 from
>> 1896x1080
>> [ 1105.627927] ov5640 1-003c: mipi_sclk=246000000, mipi_div=2, prediv=3,
>> mult=123, sysdiv=4
>> [ 1105.641273] ov5640 1-003c: link freq=61500000 Hz
>> [ 1105.646789] ov5640 1-003c: PCLK PERIOD 0x4837=0x40
>> [ 1105.709626] st-mipid02 2-0014: clk_lane_reg1=0x81
>> <<<<<<<<<<<<<<<<< 15.09 fps
>> <<<<<<<<<<<<<<< 15.04 fps
>> <<<<<<<<<<<<<<< 15.03 fps
>> <<<<<<<<<<<<<<< 15.02 fps
>> <<<<<<<<<<<<<<< 15.01 fps
>>
>> Measured #16ns (62.5MHz)
>>
>>
>> * JPEG VGA@30fps
>> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=30;v4l2-ctl
>> --set-fmt-video=width=640,height=480,pixelformat=JPEG --stream-mmap
>> --stream-count=-1
>> [ 1144.539082] ov5640 1-003c: Bandwidth Per Lane=491443200, 640x480 from
>> 1896x1080
>> [ 1144.545143] ov5640 1-003c: mipi_sclk=492000000, mipi_div=2, prediv=3,
>> mult=123, sysdiv=2
>> [ 1144.558756] ov5640 1-003c: link freq=123000000 Hz
>> [ 1144.562012] ov5640 1-003c: PCLK PERIOD 0x4837=0x20
>> [ 1144.623895] st-mipid02 2-0014: clk_lane_reg1=0x41
>> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.09 fps
>> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.04 fps
>> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.13 fps
>>
>> Measured #8ns (125MHz)
>>
>>
>> *******************
>> ***  1024x768  ****
>> *******************
>> * JPEG 1024x768@15fps
>> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl
>> --set-fmt-video=width=1024,height=768,pixelformat=JPEG --stream-mmap
>> --stream-count=-1
>> <<<<<<<<<<<<<<<<< 15.09 fps
>> <<<<<<<<<<<<<<< 15.04 fps
>> <<<<<<<<<<<<<<< 15.03 fps
>> <<<<<<<<<<<<<<< 15.02 fps
>>
>> Measured #16ns (62.5MHz)
>>
>>
>> * JPEG 1024x768@30fps
>> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=30;v4l2-ctl
>> --set-fmt-video=width=1024,height=768,pixelformat=JPEG --stream-mmap
>> --stream-count=-1
>> [ 1218.632061] ov5640 1-003c: Bandwidth Per Lane=491443200, 1024x768
>> from 1896x1080
>> [ 1218.638216] ov5640 1-003c: mipi_sclk=492000000, mipi_div=2, prediv=3,
>> mult=123, sysdiv=2
>> [ 1218.651788] ov5640 1-003c: link freq=123000000 Hz
>> [ 1218.655345] ov5640 1-003c: PCLK PERIOD 0x4837=0x20
>> [ 1218.724149] st-mipid02 2-0014: clk_lane_reg1=0x41
>> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.09 fps
>> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.04 fps
>> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.13 fps
>> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.09 fps
>> Measured #8ns (125MHz)
>>
>>
>> ***************
>> ***  720p  ****
>> ***************
>> * JPEG 720p@15fps
>> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl
>> --set-fmt-video=width=1280,height=720,pixelformat=JPEG --stream-mmap
>> --stream-count=-1
>> [ 7781.232498] ov5640 1-003c: Bandwidth Per Lane=168009600, 1280x720
>> from 1892x740
>> [ 7781.238554] ov5640 1-003c: mipi_sclk=169600000, mipi_div=1, prediv=3,
>> mult=106, sysdiv=5
>> [ 7781.247117] ov5640 1-003c: link freq=84800000 Hz
>> [ 7781.251116] ov5640 1-003c: PCLK PERIOD 0x4837=0x17
>> [ 7781.316980] st-mipid02 2-0014: clk_lane_reg1=0x5d
>>
>> Measured #12ns (83MHz)
>>
>> * JPEG 720p@30fps
>> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=30;v4l2-ctl
>> --set-fmt-video=width=1280,height=720,pixelformat=JPEG --stream-mmap
>> --stream-count=-1
>> [  515.548519] ov5640 1-003c: Bandwidth Per Lane=336019200, 1280x720
>> from 1892x740
>> [  515.554646] ov5640 1-003c: mipi_sclk=340000000, mipi_div=1, prediv=3,
>> mult=85, sysdiv=2
>> [  515.562406] ov5640 1-003c: link freq=170000000 Hz
>> [  515.574376] ov5640 1-003c: PCLK PERIOD 0x4837=0xb
>> [  515.639862] st-mipid02 2-0014: clk_lane_reg1=0x2d
>> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.39 fps
>> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.50 fps
>> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.36 fps
>> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.42 fps
>> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.40 fps
>> Measured #6ns (166MHz)
>> ==> 30fps but image artefacts (out of MP1 capabilities)
>>
>>
>> ****************
>> ***  1080p  ****
>> ****************
>> * JPEG HD@15fps
>> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl
>> --set-fmt-video=width=1920,height=1080,pixelformat=JPEG --stream-mmap
>> --stream-count=-1
>> [ 1998.266915] ov5640 1-003c: Bandwidth Per Lane=336000000, 1920x1080
>> from 2500x1120
>> [ 1998.273040] ov5640 1-003c: mipi_sclk=336000000, mipi_div=1, prediv=3,
>> mult=42, sysdiv=1
>> [ 1998.280987] ov5640 1-003c: link freq=168000000 Hz
>> [ 1998.286142] ov5640 1-003c: PCLK PERIOD 0x4837=0xb
>> [ 1998.380206] st-mipid02 2-0014: clk_lane_reg1=0x2d
>> <<< 1.88 fps
>> << 1.87 fps
>> << 1.88 fps
>> << 1.87 fps
>> << 1.87 fps
>> Measured #6ns (166MHz)
>>
>> * JPEG HD@30fps
>> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=30;v4l2-ctl
>> --set-fmt-video=width=1920,height=1080,pixelformat=JPEG --stream-mmap
>> --stream-count=-1
>> [ 2196.981767] ov5640 1-003c: Bandwidth Per Lane=672000000, 1920x1080
>> from 2500x1120
>> [ 2196.987882] ov5640 1-003c: mipi_sclk=672000000, mipi_div=1, prediv=3,
>> mult=84, sysdiv=1
>> [ 2196.996225] ov5640 1-003c: link freq=336000000 Hz
>> [ 2197.000553] ov5640 1-003c: PCLK PERIOD 0x4837=0x5
>> [ 2197.097244] st-mipid02 2-0014: clk_lane_reg1=0x15
>> Measured #3ns (333MHz)
>>
>> ==> no images but out of MP1 capabilities
>>
>>
>> **************
>> ***  5Mp  ****
>> **************
>> * JPEG 5Mp@15fps
>> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl
>> --set-fmt-video=width=2592,height=1944,pixelformat=JPEG --stream-mmap
>> --stream-count=-1
>>
>> [ 2479.203457] ov5640 1-003c: Bandwidth Per Lane=671639040, 2592x1944
>> from 2844x1968
>> [ 2479.209647] ov5640 1-003c: mipi_sclk=672000000, mipi_div=1, prediv=3,
>> mult=84, sysdiv=1
>> [ 2479.217961] ov5640 1-003c: link freq=336000000 Hz
>> [ 2479.222240] ov5640 1-003c: PCLK PERIOD 0x4837=0x5
>> [ 2479.388400] st-mipid02 2-0014: clk_lane_reg1=0x15
>>
>> Measured #3ns (333MHz)
>>
>> ==> no images but out of MP1 capabilities
>>
>>
>>
>> On 9/3/19 4:48 PM, Hugues FRUCHET wrote:
>>> Hi Jacopo,
>>>
>>> On 8/21/19 4:24 PM, Jacopo Mondi wrote:
>>>> Hello,
>>>>       +laura who has been working on supporting RAW capture for this
>>>>       driver.
>>>>
>>>> On Tue, Aug 20, 2019 at 12:13:12PM +0300, Sakari Ailus wrote:
>>>>> Hi Hugues,
>>>>>
>>>>> On Tue, Jul 02, 2019 at 04:05:46PM +0000, Hugues FRUCHET wrote:
>>>>>> Hi Sakari,
>>>>>>
>>>>>> On 6/27/19 6:05 PM, Sakari Ailus wrote:
>>>>>>> Hi Hugues,
>>>>>>>
>>>>>>> On Thu, Jun 27, 2019 at 02:57:04PM +0200, Hugues Fruchet wrote:
>>>>>>>> Add support of V4L2_CID_LINK_FREQ, this is needed
>>>>>>>> by some CSI-2 receivers.
>>>>>>>>
>>>>>>>> 384MHz is exposed for the time being, corresponding
>>>>>>>> to 96MHz pixel clock with 2 bytes per pixel on 2 data lanes.
>>>>>>>>
>>>>>>>> This setup has been tested successfully with ST MIPID02
>>>>>>>> CSI-2 to parallel bridge.
>>>>>>>>
>>>>>>>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>>>>>>>> ---
>>>>>>>
>>>>>>> Thanks for the patch.
>>>>>>>
>>>>>>> The driver calculates the sensor configuration based on its configuration,
>>>>>>> and this needs to be reflected in the link frequency: it's not a static
>>>>>>> value. See e.g. ov5640_calc_sys_clk().
>>>>>>>
>>>>>>
>>>>>> I know this code, but for a reason I don't understand yet, this seems
>>>>>> to not have effects on the CSI-2 link frequency.
>>>>>>
>>>>
>>>> This seems unlikely to me, as the ov5640_calc_sys_clk() calculates the
>>>> system clock, which then generates the MIPI CLK.
>>>>
>>>> I would really be interested to know if you could measure somehow the
>>>> actual frequency of the clock lane, to make sure it actually
>>>> changes according to to the driver calculations.
>>>>
>>>>>> This has been verified with MIPID02 CSI-2 bridge which only work with
>>>>>> this fixed link frequency as input to program its ui_x4 register
>>>>>> setting, see
>>>>>> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2028171.html.
>>>>>> All resolutions and all framerate have been tested succesfully with this
>>>>>> setting.
>>>>>>
>>>>>> Initially I tried to set the link frequency according to the value
>>>>>> computed in ov5640_calc_sys_clk() but it was only functional when
>>>>>> resolutions/framerate was close to the 384MHz value...
>>>>>>
>>>>>> As OV5640 D3 engineering board has been initially developped for
>>>>>> dragonboard, I'll dig into this and found this commit:
>>>>>> https://lore.kernel.org/patchwork/patch/886794/
>>>>>> which set a fixed pixel rate value to 96MHz, which match perfectly
>>>>>> with the 384MHz value I found for link frequency...
>>>>>>
>>>>>> Perhaps other CSI-2 OV5640 users can comment about it and have the
>>>>>> explanations of what I experiment...
>>>>>> Maxime, Jacopo, do you have any idea about it ?
>>>>>
>>>>> I would also like to wee a comment from someone who's familiar with the
>>>>> device. Yet I can tell a static value of 348 MHz is clearly incorrect as it
>>>>> ignores sensor runtime configuration as well as platform configuration
>>>>> (external clock frequency for instance).
>>>>>
>>>>> Generally speaking, configuring a CSI-2 receiver to expect a particular
>>>>> frequency usually doesn't mean it's going to fail even if the transmitter
>>>>> uses a different frequency, albeit the likelihood of it not working
>>>>> increases as the difference grows. Could the problem be at the receiver's
>>>>> end?
>>>>>
>>>>> Have you checked what kind of values ov5640_calc_pclk() returns for
>>>>> different configurations? It would seem like that this is what the
>>>>> LINK_FREQ (divided by lanes and multiplied by two) should be.
>>>>
>>>> Not exactly. ov5640_calc_pclk() is only used when computing the
>>>> parallel bus pixel clock not for the CSI-2 bus.
>>>>
>>>> To obtain the link frequency for the MIPI interface I would consider
>>>> the system clock frequency calculated by ov5640_calc_sys_clk(), which
>>>> represents the bandwidth per lane in bits per second. This should then
>>>> be divided by 2 to compensate the MIPI DDR mode [*] (no need to divide
>>>> by the number of lanes, as this is already the bandwidth -per lane-)
>>>>
>>>> For 640x480 YUYV8_2X8 (and 1024x768) with 2 data lanes and 24MHz xclk
>>>> I have calculated the following values:
>>>>
>>>> SYSCLK = 492MHz
>>>> MIPISCLK = 256MHz
>>>> PCLK = 61,50MHz
>>>>
>>>> For 1280x720 with the same setup
>>>> SYSCLK = 340MHz
>>>> MIPI_CLK = 170MHz
>>>> PCLK = 42,50 (this seems veeery low according to datasheet)
>>>>
>>>> For 1920x1080
>>>> SYSCLK = 680MHz
>>>> MIPI_CLK = 340MHz
>>>> PCLK = 85MHz (slightly slow according to the datasheet)
>>>>
>>>> All captured images seems fine...
>>>>
>>>> If you could confirm those values by probing the actual clock lane it
>>>> would be just great.
>>>>
>>>
>>> I've made some measurements probing clock lane with YUYV 16 bits
>>> grabbing, here are the results:
>>>
>>> * VGA @ 30fps
>>> v4l2-ctl --set-parm=30;v4l2-ctl
>>> --set-fmt-video=width=640,height=480,pixelformat=YUYV --stream-mmap
>>> [ 1624.854273] ov5640 0-003c: rate=491443200, 640x480 from 1896x1080
>>> [ 1624.859192] ov5640 0-003c: sysclk=492000000, mipi_div=2
>>> Measured clock lane frequency: 123MHz
>>>
>>> * 1024x768 @ 30fps
>>> v4l2-ctl --set-parm=30;v4l2-ctl
>>> --set-fmt-video=width=1024,height=768,pixelformat=YUYV --stream-mmap
>>> [  558.833885] ov5640 0-003c: rate=491443200, 1024x768 from 1896x1080
>>> [  558.840188] ov5640 0-003c: sysclk=492000000, mipi_div=2
>>> Measured clock lane frequency: 124MHz
>>>
>>> * 720p @ 30fps
>>> v4l2-ctl --set-parm=30;v4l2-ctl
>>> --set-fmt-video=width=1280,height=720,pixelformat=YUYV --stream-mmap
>>> [ 2920.785392] ov5640 0-003c: rate=336019200, 1280x720 from 1892x740
>>> [ 2920.794771] ov5640 0-003c: sysclk=340000000, mipi_div=1
>>> Measured clock lane frequency: 171MHz
>>>
>>> * 1080p @ 30fps
>>> v4l2-ctl --set-parm=30;v4l2-ctl
>>> --set-fmt-video=width=1920,height=1080,pixelformat=YUYV --stream-mmap
>>> [ 1650.143701] ov5640 0-003c: rate=672000000, 1920x1080 from 2500x1120
>>> [ 1650.149876] ov5640 0-003c: sysclk=672000000, mipi_div=1
>>> Measured clock lane frequency: 330MHz
>>>
>>> So it seems that formula must also take care of mipi_div:
>>> link_frequency = sys_div / 2 / mipi_div
>>>
>>>
>>> Unfortunately, as said previously, when updating ov5640 codebase so that
>>> CID_LINK_FREQUENCY returns this computed value instead of 384MHz fixed
>>> value, the MIPID02 bridge has some troubles and some
>>> resolutions/framerate are broken:
>>>
>>> dmesg -n8
>>> echo "format link_freq +p" > /sys/kernel/debug/dynamic_debug/control
>>>
>>> * VGA @ 30fps => broken
>>> v4l2-ctl --set-parm=30;v4l2-ctl
>>> --set-fmt-video=width=640,height=480,pixelformat=YUYV --stream-mmap
>>> [ 1624.854273] ov5640 0-003c: rate=491443200, 640x480 from 1896x1080
>>> [ 1624.859192] ov5640 0-003c: sysclk=492000000, mipi_div=2
>>> [ 1624.924287] st-mipid02 1-0014: detect link_freq = 123000000 Hz
>>> ==> no frames sent to DCMI
>>>
>>> * 720p @ 30fps => very slow framerate
>>> v4l2-ctl --set-parm=30;v4l2-ctl
>>> --set-fmt-video=width=1280,height=720,pixelformat=YUYV --stream-mmap
>>> [ 1900.707822] ov5640 0-003c: rate=336019200, 1280x720 from 1892x740
>>> [ 1900.712717] ov5640 0-003c: sysclk=340000000, mipi_div=1
>>> [ 1900.782402] st-mipid02 1-0014: detect link_freq = 170000000 Hz
>>> << 0.69 fps
>>> < 0.67 fps
>>> < 0.67 fps
>>>
>>> * 1080p @ 30fps => very slow framerate
>>> v4l2-ctl --set-parm=30;v4l2-ctl
>>> --set-fmt-video=width=1920,height=1080,pixelformat=YUYV --stream-mmap
>>> [ 1366.550832] ov5640 0-003c: rate=336000000, 1920x1080 from 2500x1120
>>> [ 1366.555668] ov5640 0-003c: sysclk=336000000, mipi_div=1
>>> Frame rate set to 30.000 fps
>>> [ 1367.105503] ov5640 0-003c: rate=672000000, 1920x1080 from 2500x1120
>>> [ 1367.110420] ov5640 0-003c: sysclk=672000000, mipi_div=1
>>> [ 1367.214498] st-mipid02 1-0014: detect link_freq = 336000000 Hz
>>> << 0.11 fps
>>> < 0.11 fps
>>>
>>>
>>> * 1080p @ 15fps => OK, 15fps as expected
>>> v4l2-ctl --set-parm=15;v4l2-ctl
>>> --set-fmt-video=width=1920,height=1080,pixelformat=YUYV --stream-mmap
>>> [ 1327.417545] ov5640 0-003c: rate=336000000, 1920x1080 from 2500x1120
>>> [ 1327.422560] ov5640 0-003c: sysclk=336000000, mipi_div=1
>>> [ 1327.527852] st-mipid02 1-0014: detect link_freq = 168000000 Hz
>>> <<<<<<<<<<<<<<<<< 15.09 fps
>>> <<<<<<<<<<<<<<< 15.04 fps
>>>
>>>
>>> One could argue that this is the mipid02 bridge codebase that is wrong:
>>> drivers/media/i2c/st-mipid02.c:
>>>      mipid02_configure_from_rx_speed()
>>>        [...]
>>>        bridge->r.clk_lane_reg1 |= (2000000000 / link_freq) << 2;
>>>
>>> but computation is inline with MIPID02 spec:
>>>
>>> "CLOCK LANE 1 REGISTER"
>>> Bit_no   Bit_name
>>> 7        ui_x4_clk_lane[5]
>>> 6        ui_x4_clk_lane[4]
>>> 5        ui_x4_clk_lane[3]
>>> 4        ui_x4_clk_lane[2]
>>> 3        ui_x4_clk_lane[1]
>>> 2        ui_x4_clk_lane[0]
>>> Unit interval time multiplied by four
>>> This signal indicates the bit period in units of
>>> 0.25 ns. If the unit interval is 3 ns, twelve (0x0C)
>>> should be programmed. This value is used to
>>> generate delays. Therefore, if the period is not a
>>> multiple of 0.25 ns, the value should be rounded
>>> down. For example, a 600 Mbit/s single lane
>>> linkuses a unit interval of 1.667 ns. Multiplying
>>> by four results in 6.667. In this case, a value of 6
>>> (not 7) should be programmed.
>>>
>>>
>>> Perhaps that someone which knows well CSI-2 interfaces could
>>> detect what is wrong here, but personally I don't.
>>>
>>>
>>>> However, this only work assuming the here below [*], and we're still
>>>> missing something because RAW still doesn't work as expected :(
>>>>
>>>> Anyway, clock tree madness apart, fixing the LINK_FREQ control value
>>>> to 96MHz seems like it does not reflect what the driver actual does.
>>>>
>>>> Could you try setting the control value to what the driver actually
>>>> computes as here suggested?
>>>>
>>>> Thanks
>>>>       j
>>>>
>>>> PS. I'm still confused why the have the link-frequencies property in
>>>> DTS bindings. What is its purpose if all drivers compute or hardcode
>>>> it?
>>>
>>> I share this point of view, I don't know how to return a dynamically
>>> computed link frequency. Current drivers are exposing a single one
>>> or a set of discrete frequencies.
>>> For a matter of test purpose, I've done this hack:
>>>
>>> -static const s64 link_freq_menu_items[] = {
>>> +static s64 link_freq_menu_items[] = {
>>> 	384000000,
>>> };
>>>
>>> +	link_freq_menu_items[0] = _rate / 2 / mipi_div;
>>>
>>>
>>>>
>>>> [*] There is a big assumption here, that the MIPI bus clock gets
>>>> generated by two different clock outputs depending if the mode goes
>>>> through the the scaler or not. You can see this at line 967 in the
>>>> driver code. If mode goes though the scaler, MIPI_DIV = 2 and you get
>>>> the bus clock from the MIPISCLK signal. If it does not go through the
>>>> scaler MIPI_DIV = 1 and then the MIPI bus clock gets provided by the
>>>> MIPI_CLK signal. This is the result of several attempts to decode the
>>>> ov5640 clock tree, but it has only been tested with modes with bpp =
>>>> 16 and 2 data lanes. As it has been recently reported, for RAW mode,
>>>> this does not work well, so the assumption might not be correct.
>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>>> version 2:
>>>>>>>>       - do not set control read only as per Hans' comment:
>>>>>>>>         See https://www.mail-archive.com/linux-media@vger.kernel.org/msg147910.html
>>>>>>>>
>>>>>>>>      drivers/media/i2c/ov5640.c | 10 ++++++++++
>>>>>>>>      1 file changed, 10 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>>>>>>>> index 82d4ce9..e6307f3 100644
>>>>>>>> --- a/drivers/media/i2c/ov5640.c
>>>>>>>> +++ b/drivers/media/i2c/ov5640.c
>>>>>>>> @@ -218,6 +218,7 @@ struct ov5640_ctrls {
>>>>>>>>      	struct v4l2_ctrl *test_pattern;
>>>>>>>>      	struct v4l2_ctrl *hflip;
>>>>>>>>      	struct v4l2_ctrl *vflip;
>>>>>>>> +	struct v4l2_ctrl *link_freq;
>>>>>>>>      };
>>>>>>>>
>>>>>>>>      struct ov5640_dev {
>>>>>>>> @@ -2198,6 +2199,10 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd,
>>>>>>>>      	return 0;
>>>>>>>>      }
>>>>>>>>
>>>>>>>> +static const s64 link_freq_menu_items[] = {
>>>>>>>> +	384000000,
>>>>>>>> +};
>>>>>>>> +
>>>>>>>>      static int ov5640_set_fmt(struct v4l2_subdev *sd,
>>>>>>>>      			  struct v4l2_subdev_pad_config *cfg,
>>>>>>>>      			  struct v4l2_subdev_format *format)
>>>>>>>> @@ -2636,6 +2641,8 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
>>>>>>>>      	case V4L2_CID_VFLIP:
>>>>>>>>      		ret = ov5640_set_ctrl_vflip(sensor, ctrl->val);
>>>>>>>>      		break;
>>>>>>>> +	case V4L2_CID_LINK_FREQ:
>>>>>>>> +		return 0;
>>>>>>>>      	default:
>>>>>>>>      		ret = -EINVAL;
>>>>>>>>      		break;
>>>>>>>> @@ -2703,6 +2710,9 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
>>>>>>>>      				       V4L2_CID_POWER_LINE_FREQUENCY_AUTO, 0,
>>>>>>>>      				       V4L2_CID_POWER_LINE_FREQUENCY_50HZ);
>>>>>>>>
>>>>>>>> +	ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
>>>>>>>> +						  0, 0, link_freq_menu_items);
>>>>>>>> +
>>>>>>>>      	if (hdl->error) {
>>>>>>>>      		ret = hdl->error;
>>>>>>>>      		goto free_ctrls;
>>>>>>>> --
>>>>>>>> 2.7.4
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>> BR,
>>>>>> Hugues.
>>>>>
>>>>> --
>>>>> Sakari Ailus
>>>>> sakari.ailus@linux.intel.com
>>>
>>> BR,
>>> Hugues.
>>>
>> BR,
>> Hugues.

BR,
Hugues.

Jacopo Mondi Oct. 22, 2020, 8:32 a.m. UTC | #9

Hi Hugues,

On Thu, Oct 22, 2020 at 08:14:39AM +0000, Hugues FRUCHET wrote:
> Hi Jacopo,
>
> On 10/21/20 6:51 PM, Jacopo Mondi wrote:
> > Hi Hugues,
> >
> > On Mon, Oct 19, 2020 at 01:40:26PM +0000, Hugues FRUCHET wrote:
> >> Hi Jacopo, all,
> >>
> >> Back on this subject recently, I've found interesting things around
> >> register 0x4837 PCLK PERIOD thanks to Samuel Bobrowicz work on PLL
> >> settings [1], but 0x4837 register value computation differs from what
> >> I've understood/measured.
> >
> > Thanks, very useful to know!
> >
> >>
> >> The OV5640 PCLK_PERIOD register value must be in line with the
> >> ST MIPID02 CSI-2 receiver CLOCK_LANE_1 register, otherwise receptions
> >> issues are encountered has shown in my previous mail.
> >>
> >> Configuring those 2 registers according to measured MIPI clock frequency
> >> allows to cover almost all resolutions/formats (RGB565, YUV422, JPEG).
> >> I've also measured with oscilloscope the MIPI clock-lane frequency and
> >> verified that this is well matching computed one [2].
> >
> > Is this based on the current mainline driver ? I still wonder if
> > Tomi's patch that adjusts the vtot and htot value to reflect datasheet
> > needs to be applied.
>
> Yes it is based on latest media codebase, so no vtot/htot changes as
> done by Tomi (status of those changes is not clear on my side, but I
> could test on my platform, just let me know).
>

I've now included that change in my tree again, and see if it could
help or not.

I see where the data Tomi changed come from, but I wonder why vtot and
htot was set in the way they currently are in the original register
blobs :/

> >
> >> Please note that due to platform limitation, I could not test link
> >> frequency above 720p@15fps, so additional tests are needed with platform
> >> that could handle 5Mp@15fps (OV5640 limit).
> >>
> >> In term of code, this mean:
> >>
> >> v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
> >>          		  0, 0, link_freq_menu_items);
> >>
> >> ov5640_set_mipi_pclk(rate)
> >> {
> >>     mipi_sclk = ov5640_calc_sys_clk(sensor, rate, &prediv, &mult, &sysdiv);
> >>     link_freq = mipi_sclk / 2 / mipi_div; /* the CSI-2 clock-lane
> >> frequency, measurements are matching */
> >>
> >>     _pclk_period = (u8) (2000000000UL / link_freq);/* expressed in ns and
> >
> > Is 2000000000UL to compensate for the / 2 above ?
>
> No, it is for value of register expressed in 0.5ns unit as explained in
> comment.

Oh I see. Sorry, confused by the DDR mode compensation.

>
> >
> >> with 1-bit decimal: 0x1=0.5ns, 0x2=1ns, 0x3=1.5ns and so on... */
> >>     ov5640_write_reg(sensor, OV5640_REG_PCLK_PERIOD, _pclk_period);
> >>
> >>     link_freq_menu_items[0] = link_freq;/* Currently a hack to inform
> >> receiver about the dynamically computed clock frequency, I don't know
> >> yet how to handle this correctly, please advise... */
> >
> > Can you call __v4l2_ctrl_s_ctrl on a menu control ?
> >
>
> I'll give it a try...

Looking at ov5645 that's what you're supposed to do (__ or non __
version depending on lock handling)

>
> >>
> >>     ...
> >> }
> >>
> >> As explained in comment, I miss a V4L2 way to set dynamically
> >> V4L2_CID_LINK_FREQ in order to propose a new patchset.
> >>
> >>
> >> [1]
> >> https://patchwork.kernel.org/project/linux-media/patch/1539797508-127629-1-git-send-email-sam@elite-embedded.com/
> >>
> >> [2] measure of the CSI-2 clock frequency, tested with JPEG (easier to
> >> check that capture is fine) and various resolutions:
> >>
> >
> > On my test platform setting 0x4837 with the value calculated as you
> > reported breaks capture. I'll investigate if I need to setup anything
> > on the receiver side as you have to (it's an i.MX6, if anyone reading
> > knows that platform better than what I do :)
> >
> > Thanks
> >    j
> >
>
> From
> https://www.nxp.com/docs/en/application-note/AN5305.pdf
>
> I understand that you have to set MIPI_CSI_TST_CTRL1 register according
> to link frequency as per table 4.

I noticed pretty quickly looking at the CSI-2 receiver driver:

	ctrl = v4l2_ctrl_find(csi2->src_sd->ctrl_handler,
			      V4L2_CID_LINK_FREQ);
	if (!ctrl)
		mbps_per_lane = CSI2_DEFAULT_MAX_MBPS;
	else
		mbps_per_lane = DIV_ROUND_UP_ULL(2 * ctrl->qmenu_int[ctrl->val],
						 USEC_PER_SEC);

So I guess implementing support for LINK_FREQ will make my receiver
happy as well.

Looking forward to receiving or sending a patch to support it, in the
meantime I'll look (again) at those pesky vtot/htot and the clock tree
calculation that follows.

Thanks
  j

>
> >> ****************
> >> ***  QQVGA  ****
> >> ****************
> >> * JPEG QQVGA@15fps
> >> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl
> >> --set-fmt-video=width=160,height=120,pixelformat=JPEG --stream-mmap
> >> --stream-count=-1
> >> [  371.459607] ov5640 1-003c: Bandwidth Per Lane=223879680, 160x120 from
> >> 1896x984
> >> [  371.465516] ov5640 1-003c: mipi_sclk=224000000, mipi_div=2, prediv=3,
> >> mult=28, sysdiv=1
> >> [  371.473727] ov5640 1-003c: link freq=56000000 Hz
> >> [  371.478529] ov5640 1-003c: PCLK PERIOD 0x4837=0x23
> >> [  371.538193] st-mipid02 2-0014: clk_lane_reg1=0x8d
> >> <<<<<<<<<<<<<<<<< 15.09 fps
> >> <<<<<<<<<<<<<<< 15.04 fps
> >> <<<<<<<<<<<<<<< 15.03 fps
> >> <<<<<<<<<<<<<<< 15.02 fps
> >> <<<<<<<<<<<<<<< 15.01 fps
> >> Clock-lane frequency measured at #18ns (55.5MHz)
> >>
> >>
> >> * RGB565 QQVGA@15fps
> >> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl
> >> --set-fmt-video=width=160,height=120,pixelformat=RGBP --stream-mmap
> >> --stream-count=-1
> >> [ 1229.422079] st-mipid02 2-0014: clk_lane_reg1=0x8d
> >> [ 1229.431758] ov5640 1-003c: Bandwidth Per Lane=223879680, 160x120 from
> >> 1896x984
> >> [ 1229.437905] ov5640 1-003c: mipi_sclk=224000000, mipi_div=2, prediv=3,
> >> mult=28, sysdiv=1
> >> [ 1229.445620] ov5640 1-003c: link freq=56000000 Hz
> >> [ 1229.450397] ov5640 1-003c: PCLK PERIOD 0x4837=0x23
> >> <<<<<<<<<<<<<<<<< 15.09 fps
> >> <<<<<<<<<<<<<<< 15.04 fps
> >> <<<<<<<<<<<<<< 14.70 fps
> >> <<<<<<<<<<<<<<< 14.77 fps
> >> Measured #18ns (55.5MHz)
> >>
> >>
> >> **************
> >> ***  VGA  ****
> >> **************
> >> * JPEG VGA@15fps
> >> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl
> >> --set-fmt-video=width=640,height=480,pixelformat=JPEG --stream-mmap
> >> --stream-count=-1
> >> [ 1105.621884] ov5640 1-003c: Bandwidth Per Lane=245721600, 640x480 from
> >> 1896x1080
> >> [ 1105.627927] ov5640 1-003c: mipi_sclk=246000000, mipi_div=2, prediv=3,
> >> mult=123, sysdiv=4
> >> [ 1105.641273] ov5640 1-003c: link freq=61500000 Hz
> >> [ 1105.646789] ov5640 1-003c: PCLK PERIOD 0x4837=0x40
> >> [ 1105.709626] st-mipid02 2-0014: clk_lane_reg1=0x81
> >> <<<<<<<<<<<<<<<<< 15.09 fps
> >> <<<<<<<<<<<<<<< 15.04 fps
> >> <<<<<<<<<<<<<<< 15.03 fps
> >> <<<<<<<<<<<<<<< 15.02 fps
> >> <<<<<<<<<<<<<<< 15.01 fps
> >>
> >> Measured #16ns (62.5MHz)
> >>
> >>
> >> * JPEG VGA@30fps
> >> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=30;v4l2-ctl
> >> --set-fmt-video=width=640,height=480,pixelformat=JPEG --stream-mmap
> >> --stream-count=-1
> >> [ 1144.539082] ov5640 1-003c: Bandwidth Per Lane=491443200, 640x480 from
> >> 1896x1080
> >> [ 1144.545143] ov5640 1-003c: mipi_sclk=492000000, mipi_div=2, prediv=3,
> >> mult=123, sysdiv=2
> >> [ 1144.558756] ov5640 1-003c: link freq=123000000 Hz
> >> [ 1144.562012] ov5640 1-003c: PCLK PERIOD 0x4837=0x20
> >> [ 1144.623895] st-mipid02 2-0014: clk_lane_reg1=0x41
> >> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.09 fps
> >> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.04 fps
> >> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.13 fps
> >>
> >> Measured #8ns (125MHz)
> >>
> >>
> >> *******************
> >> ***  1024x768  ****
> >> *******************
> >> * JPEG 1024x768@15fps
> >> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl
> >> --set-fmt-video=width=1024,height=768,pixelformat=JPEG --stream-mmap
> >> --stream-count=-1
> >> <<<<<<<<<<<<<<<<< 15.09 fps
> >> <<<<<<<<<<<<<<< 15.04 fps
> >> <<<<<<<<<<<<<<< 15.03 fps
> >> <<<<<<<<<<<<<<< 15.02 fps
> >>
> >> Measured #16ns (62.5MHz)
> >>
> >>
> >> * JPEG 1024x768@30fps
> >> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=30;v4l2-ctl
> >> --set-fmt-video=width=1024,height=768,pixelformat=JPEG --stream-mmap
> >> --stream-count=-1
> >> [ 1218.632061] ov5640 1-003c: Bandwidth Per Lane=491443200, 1024x768
> >> from 1896x1080
> >> [ 1218.638216] ov5640 1-003c: mipi_sclk=492000000, mipi_div=2, prediv=3,
> >> mult=123, sysdiv=2
> >> [ 1218.651788] ov5640 1-003c: link freq=123000000 Hz
> >> [ 1218.655345] ov5640 1-003c: PCLK PERIOD 0x4837=0x20
> >> [ 1218.724149] st-mipid02 2-0014: clk_lane_reg1=0x41
> >> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.09 fps
> >> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.04 fps
> >> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.13 fps
> >> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.09 fps
> >> Measured #8ns (125MHz)
> >>
> >>
> >> ***************
> >> ***  720p  ****
> >> ***************
> >> * JPEG 720p@15fps
> >> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl
> >> --set-fmt-video=width=1280,height=720,pixelformat=JPEG --stream-mmap
> >> --stream-count=-1
> >> [ 7781.232498] ov5640 1-003c: Bandwidth Per Lane=168009600, 1280x720
> >> from 1892x740
> >> [ 7781.238554] ov5640 1-003c: mipi_sclk=169600000, mipi_div=1, prediv=3,
> >> mult=106, sysdiv=5
> >> [ 7781.247117] ov5640 1-003c: link freq=84800000 Hz
> >> [ 7781.251116] ov5640 1-003c: PCLK PERIOD 0x4837=0x17
> >> [ 7781.316980] st-mipid02 2-0014: clk_lane_reg1=0x5d
> >>
> >> Measured #12ns (83MHz)
> >>
> >> * JPEG 720p@30fps
> >> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=30;v4l2-ctl
> >> --set-fmt-video=width=1280,height=720,pixelformat=JPEG --stream-mmap
> >> --stream-count=-1
> >> [  515.548519] ov5640 1-003c: Bandwidth Per Lane=336019200, 1280x720
> >> from 1892x740
> >> [  515.554646] ov5640 1-003c: mipi_sclk=340000000, mipi_div=1, prediv=3,
> >> mult=85, sysdiv=2
> >> [  515.562406] ov5640 1-003c: link freq=170000000 Hz
> >> [  515.574376] ov5640 1-003c: PCLK PERIOD 0x4837=0xb
> >> [  515.639862] st-mipid02 2-0014: clk_lane_reg1=0x2d
> >> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.39 fps
> >> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.50 fps
> >> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.36 fps
> >> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.42 fps
> >> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.40 fps
> >> Measured #6ns (166MHz)
> >> ==> 30fps but image artefacts (out of MP1 capabilities)
> >>
> >>
> >> ****************
> >> ***  1080p  ****
> >> ****************
> >> * JPEG HD@15fps
> >> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl
> >> --set-fmt-video=width=1920,height=1080,pixelformat=JPEG --stream-mmap
> >> --stream-count=-1
> >> [ 1998.266915] ov5640 1-003c: Bandwidth Per Lane=336000000, 1920x1080
> >> from 2500x1120
> >> [ 1998.273040] ov5640 1-003c: mipi_sclk=336000000, mipi_div=1, prediv=3,
> >> mult=42, sysdiv=1
> >> [ 1998.280987] ov5640 1-003c: link freq=168000000 Hz
> >> [ 1998.286142] ov5640 1-003c: PCLK PERIOD 0x4837=0xb
> >> [ 1998.380206] st-mipid02 2-0014: clk_lane_reg1=0x2d
> >> <<< 1.88 fps
> >> << 1.87 fps
> >> << 1.88 fps
> >> << 1.87 fps
> >> << 1.87 fps
> >> Measured #6ns (166MHz)
> >>
> >> * JPEG HD@30fps
> >> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=30;v4l2-ctl
> >> --set-fmt-video=width=1920,height=1080,pixelformat=JPEG --stream-mmap
> >> --stream-count=-1
> >> [ 2196.981767] ov5640 1-003c: Bandwidth Per Lane=672000000, 1920x1080
> >> from 2500x1120
> >> [ 2196.987882] ov5640 1-003c: mipi_sclk=672000000, mipi_div=1, prediv=3,
> >> mult=84, sysdiv=1
> >> [ 2196.996225] ov5640 1-003c: link freq=336000000 Hz
> >> [ 2197.000553] ov5640 1-003c: PCLK PERIOD 0x4837=0x5
> >> [ 2197.097244] st-mipid02 2-0014: clk_lane_reg1=0x15
> >> Measured #3ns (333MHz)
> >>
> >> ==> no images but out of MP1 capabilities
> >>
> >>
> >> **************
> >> ***  5Mp  ****
> >> **************
> >> * JPEG 5Mp@15fps
> >> v4l2-ctl --set-ctrl=test_pattern=1;v4l2-ctl --set-parm=15;v4l2-ctl
> >> --set-fmt-video=width=2592,height=1944,pixelformat=JPEG --stream-mmap
> >> --stream-count=-1
> >>
> >> [ 2479.203457] ov5640 1-003c: Bandwidth Per Lane=671639040, 2592x1944
> >> from 2844x1968
> >> [ 2479.209647] ov5640 1-003c: mipi_sclk=672000000, mipi_div=1, prediv=3,
> >> mult=84, sysdiv=1
> >> [ 2479.217961] ov5640 1-003c: link freq=336000000 Hz
> >> [ 2479.222240] ov5640 1-003c: PCLK PERIOD 0x4837=0x5
> >> [ 2479.388400] st-mipid02 2-0014: clk_lane_reg1=0x15
> >>
> >> Measured #3ns (333MHz)
> >>
> >> ==> no images but out of MP1 capabilities
> >>
> >>
> >>
> >> On 9/3/19 4:48 PM, Hugues FRUCHET wrote:
> >>> Hi Jacopo,
> >>>
> >>> On 8/21/19 4:24 PM, Jacopo Mondi wrote:
> >>>> Hello,
> >>>>       +laura who has been working on supporting RAW capture for this
> >>>>       driver.
> >>>>
> >>>> On Tue, Aug 20, 2019 at 12:13:12PM +0300, Sakari Ailus wrote:
> >>>>> Hi Hugues,
> >>>>>
> >>>>> On Tue, Jul 02, 2019 at 04:05:46PM +0000, Hugues FRUCHET wrote:
> >>>>>> Hi Sakari,
> >>>>>>
> >>>>>> On 6/27/19 6:05 PM, Sakari Ailus wrote:
> >>>>>>> Hi Hugues,
> >>>>>>>
> >>>>>>> On Thu, Jun 27, 2019 at 02:57:04PM +0200, Hugues Fruchet wrote:
> >>>>>>>> Add support of V4L2_CID_LINK_FREQ, this is needed
> >>>>>>>> by some CSI-2 receivers.
> >>>>>>>>
> >>>>>>>> 384MHz is exposed for the time being, corresponding
> >>>>>>>> to 96MHz pixel clock with 2 bytes per pixel on 2 data lanes.
> >>>>>>>>
> >>>>>>>> This setup has been tested successfully with ST MIPID02
> >>>>>>>> CSI-2 to parallel bridge.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> >>>>>>>> ---
> >>>>>>>
> >>>>>>> Thanks for the patch.
> >>>>>>>
> >>>>>>> The driver calculates the sensor configuration based on its configuration,
> >>>>>>> and this needs to be reflected in the link frequency: it's not a static
> >>>>>>> value. See e.g. ov5640_calc_sys_clk().
> >>>>>>>
> >>>>>>
> >>>>>> I know this code, but for a reason I don't understand yet, this seems
> >>>>>> to not have effects on the CSI-2 link frequency.
> >>>>>>
> >>>>
> >>>> This seems unlikely to me, as the ov5640_calc_sys_clk() calculates the
> >>>> system clock, which then generates the MIPI CLK.
> >>>>
> >>>> I would really be interested to know if you could measure somehow the
> >>>> actual frequency of the clock lane, to make sure it actually
> >>>> changes according to to the driver calculations.
> >>>>
> >>>>>> This has been verified with MIPID02 CSI-2 bridge which only work with
> >>>>>> this fixed link frequency as input to program its ui_x4 register
> >>>>>> setting, see
> >>>>>> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2028171.html.
> >>>>>> All resolutions and all framerate have been tested succesfully with this
> >>>>>> setting.
> >>>>>>
> >>>>>> Initially I tried to set the link frequency according to the value
> >>>>>> computed in ov5640_calc_sys_clk() but it was only functional when
> >>>>>> resolutions/framerate was close to the 384MHz value...
> >>>>>>
> >>>>>> As OV5640 D3 engineering board has been initially developped for
> >>>>>> dragonboard, I'll dig into this and found this commit:
> >>>>>> https://lore.kernel.org/patchwork/patch/886794/
> >>>>>> which set a fixed pixel rate value to 96MHz, which match perfectly
> >>>>>> with the 384MHz value I found for link frequency...
> >>>>>>
> >>>>>> Perhaps other CSI-2 OV5640 users can comment about it and have the
> >>>>>> explanations of what I experiment...
> >>>>>> Maxime, Jacopo, do you have any idea about it ?
> >>>>>
> >>>>> I would also like to wee a comment from someone who's familiar with the
> >>>>> device. Yet I can tell a static value of 348 MHz is clearly incorrect as it
> >>>>> ignores sensor runtime configuration as well as platform configuration
> >>>>> (external clock frequency for instance).
> >>>>>
> >>>>> Generally speaking, configuring a CSI-2 receiver to expect a particular
> >>>>> frequency usually doesn't mean it's going to fail even if the transmitter
> >>>>> uses a different frequency, albeit the likelihood of it not working
> >>>>> increases as the difference grows. Could the problem be at the receiver's
> >>>>> end?
> >>>>>
> >>>>> Have you checked what kind of values ov5640_calc_pclk() returns for
> >>>>> different configurations? It would seem like that this is what the
> >>>>> LINK_FREQ (divided by lanes and multiplied by two) should be.
> >>>>
> >>>> Not exactly. ov5640_calc_pclk() is only used when computing the
> >>>> parallel bus pixel clock not for the CSI-2 bus.
> >>>>
> >>>> To obtain the link frequency for the MIPI interface I would consider
> >>>> the system clock frequency calculated by ov5640_calc_sys_clk(), which
> >>>> represents the bandwidth per lane in bits per second. This should then
> >>>> be divided by 2 to compensate the MIPI DDR mode [*] (no need to divide
> >>>> by the number of lanes, as this is already the bandwidth -per lane-)
> >>>>
> >>>> For 640x480 YUYV8_2X8 (and 1024x768) with 2 data lanes and 24MHz xclk
> >>>> I have calculated the following values:
> >>>>
> >>>> SYSCLK = 492MHz
> >>>> MIPISCLK = 256MHz
> >>>> PCLK = 61,50MHz
> >>>>
> >>>> For 1280x720 with the same setup
> >>>> SYSCLK = 340MHz
> >>>> MIPI_CLK = 170MHz
> >>>> PCLK = 42,50 (this seems veeery low according to datasheet)
> >>>>
> >>>> For 1920x1080
> >>>> SYSCLK = 680MHz
> >>>> MIPI_CLK = 340MHz
> >>>> PCLK = 85MHz (slightly slow according to the datasheet)
> >>>>
> >>>> All captured images seems fine...
> >>>>
> >>>> If you could confirm those values by probing the actual clock lane it
> >>>> would be just great.
> >>>>
> >>>
> >>> I've made some measurements probing clock lane with YUYV 16 bits
> >>> grabbing, here are the results:
> >>>
> >>> * VGA @ 30fps
> >>> v4l2-ctl --set-parm=30;v4l2-ctl
> >>> --set-fmt-video=width=640,height=480,pixelformat=YUYV --stream-mmap
> >>> [ 1624.854273] ov5640 0-003c: rate=491443200, 640x480 from 1896x1080
> >>> [ 1624.859192] ov5640 0-003c: sysclk=492000000, mipi_div=2
> >>> Measured clock lane frequency: 123MHz
> >>>
> >>> * 1024x768 @ 30fps
> >>> v4l2-ctl --set-parm=30;v4l2-ctl
> >>> --set-fmt-video=width=1024,height=768,pixelformat=YUYV --stream-mmap
> >>> [  558.833885] ov5640 0-003c: rate=491443200, 1024x768 from 1896x1080
> >>> [  558.840188] ov5640 0-003c: sysclk=492000000, mipi_div=2
> >>> Measured clock lane frequency: 124MHz
> >>>
> >>> * 720p @ 30fps
> >>> v4l2-ctl --set-parm=30;v4l2-ctl
> >>> --set-fmt-video=width=1280,height=720,pixelformat=YUYV --stream-mmap
> >>> [ 2920.785392] ov5640 0-003c: rate=336019200, 1280x720 from 1892x740
> >>> [ 2920.794771] ov5640 0-003c: sysclk=340000000, mipi_div=1
> >>> Measured clock lane frequency: 171MHz
> >>>
> >>> * 1080p @ 30fps
> >>> v4l2-ctl --set-parm=30;v4l2-ctl
> >>> --set-fmt-video=width=1920,height=1080,pixelformat=YUYV --stream-mmap
> >>> [ 1650.143701] ov5640 0-003c: rate=672000000, 1920x1080 from 2500x1120
> >>> [ 1650.149876] ov5640 0-003c: sysclk=672000000, mipi_div=1
> >>> Measured clock lane frequency: 330MHz
> >>>
> >>> So it seems that formula must also take care of mipi_div:
> >>> link_frequency = sys_div / 2 / mipi_div
> >>>
> >>>
> >>> Unfortunately, as said previously, when updating ov5640 codebase so that
> >>> CID_LINK_FREQUENCY returns this computed value instead of 384MHz fixed
> >>> value, the MIPID02 bridge has some troubles and some
> >>> resolutions/framerate are broken:
> >>>
> >>> dmesg -n8
> >>> echo "format link_freq +p" > /sys/kernel/debug/dynamic_debug/control
> >>>
> >>> * VGA @ 30fps => broken
> >>> v4l2-ctl --set-parm=30;v4l2-ctl
> >>> --set-fmt-video=width=640,height=480,pixelformat=YUYV --stream-mmap
> >>> [ 1624.854273] ov5640 0-003c: rate=491443200, 640x480 from 1896x1080
> >>> [ 1624.859192] ov5640 0-003c: sysclk=492000000, mipi_div=2
> >>> [ 1624.924287] st-mipid02 1-0014: detect link_freq = 123000000 Hz
> >>> ==> no frames sent to DCMI
> >>>
> >>> * 720p @ 30fps => very slow framerate
> >>> v4l2-ctl --set-parm=30;v4l2-ctl
> >>> --set-fmt-video=width=1280,height=720,pixelformat=YUYV --stream-mmap
> >>> [ 1900.707822] ov5640 0-003c: rate=336019200, 1280x720 from 1892x740
> >>> [ 1900.712717] ov5640 0-003c: sysclk=340000000, mipi_div=1
> >>> [ 1900.782402] st-mipid02 1-0014: detect link_freq = 170000000 Hz
> >>> << 0.69 fps
> >>> < 0.67 fps
> >>> < 0.67 fps
> >>>
> >>> * 1080p @ 30fps => very slow framerate
> >>> v4l2-ctl --set-parm=30;v4l2-ctl
> >>> --set-fmt-video=width=1920,height=1080,pixelformat=YUYV --stream-mmap
> >>> [ 1366.550832] ov5640 0-003c: rate=336000000, 1920x1080 from 2500x1120
> >>> [ 1366.555668] ov5640 0-003c: sysclk=336000000, mipi_div=1
> >>> Frame rate set to 30.000 fps
> >>> [ 1367.105503] ov5640 0-003c: rate=672000000, 1920x1080 from 2500x1120
> >>> [ 1367.110420] ov5640 0-003c: sysclk=672000000, mipi_div=1
> >>> [ 1367.214498] st-mipid02 1-0014: detect link_freq = 336000000 Hz
> >>> << 0.11 fps
> >>> < 0.11 fps
> >>>
> >>>
> >>> * 1080p @ 15fps => OK, 15fps as expected
> >>> v4l2-ctl --set-parm=15;v4l2-ctl
> >>> --set-fmt-video=width=1920,height=1080,pixelformat=YUYV --stream-mmap
> >>> [ 1327.417545] ov5640 0-003c: rate=336000000, 1920x1080 from 2500x1120
> >>> [ 1327.422560] ov5640 0-003c: sysclk=336000000, mipi_div=1
> >>> [ 1327.527852] st-mipid02 1-0014: detect link_freq = 168000000 Hz
> >>> <<<<<<<<<<<<<<<<< 15.09 fps
> >>> <<<<<<<<<<<<<<< 15.04 fps
> >>>
> >>>
> >>> One could argue that this is the mipid02 bridge codebase that is wrong:
> >>> drivers/media/i2c/st-mipid02.c:
> >>>      mipid02_configure_from_rx_speed()
> >>>        [...]
> >>>        bridge->r.clk_lane_reg1 |= (2000000000 / link_freq) << 2;
> >>>
> >>> but computation is inline with MIPID02 spec:
> >>>
> >>> "CLOCK LANE 1 REGISTER"
> >>> Bit_no   Bit_name
> >>> 7        ui_x4_clk_lane[5]
> >>> 6        ui_x4_clk_lane[4]
> >>> 5        ui_x4_clk_lane[3]
> >>> 4        ui_x4_clk_lane[2]
> >>> 3        ui_x4_clk_lane[1]
> >>> 2        ui_x4_clk_lane[0]
> >>> Unit interval time multiplied by four
> >>> This signal indicates the bit period in units of
> >>> 0.25 ns. If the unit interval is 3 ns, twelve (0x0C)
> >>> should be programmed. This value is used to
> >>> generate delays. Therefore, if the period is not a
> >>> multiple of 0.25 ns, the value should be rounded
> >>> down. For example, a 600 Mbit/s single lane
> >>> linkuses a unit interval of 1.667 ns. Multiplying
> >>> by four results in 6.667. In this case, a value of 6
> >>> (not 7) should be programmed.
> >>>
> >>>
> >>> Perhaps that someone which knows well CSI-2 interfaces could
> >>> detect what is wrong here, but personally I don't.
> >>>
> >>>
> >>>> However, this only work assuming the here below [*], and we're still
> >>>> missing something because RAW still doesn't work as expected :(
> >>>>
> >>>> Anyway, clock tree madness apart, fixing the LINK_FREQ control value
> >>>> to 96MHz seems like it does not reflect what the driver actual does.
> >>>>
> >>>> Could you try setting the control value to what the driver actually
> >>>> computes as here suggested?
> >>>>
> >>>> Thanks
> >>>>       j
> >>>>
> >>>> PS. I'm still confused why the have the link-frequencies property in
> >>>> DTS bindings. What is its purpose if all drivers compute or hardcode
> >>>> it?
> >>>
> >>> I share this point of view, I don't know how to return a dynamically
> >>> computed link frequency. Current drivers are exposing a single one
> >>> or a set of discrete frequencies.
> >>> For a matter of test purpose, I've done this hack:
> >>>
> >>> -static const s64 link_freq_menu_items[] = {
> >>> +static s64 link_freq_menu_items[] = {
> >>> 	384000000,
> >>> };
> >>>
> >>> +	link_freq_menu_items[0] = _rate / 2 / mipi_div;
> >>>
> >>>
> >>>>
> >>>> [*] There is a big assumption here, that the MIPI bus clock gets
> >>>> generated by two different clock outputs depending if the mode goes
> >>>> through the the scaler or not. You can see this at line 967 in the
> >>>> driver code. If mode goes though the scaler, MIPI_DIV = 2 and you get
> >>>> the bus clock from the MIPISCLK signal. If it does not go through the
> >>>> scaler MIPI_DIV = 1 and then the MIPI bus clock gets provided by the
> >>>> MIPI_CLK signal. This is the result of several attempts to decode the
> >>>> ov5640 clock tree, but it has only been tested with modes with bpp =
> >>>> 16 and 2 data lanes. As it has been recently reported, for RAW mode,
> >>>> this does not work well, so the assumption might not be correct.
> >>>>
> >>>>>
> >>>>>>
> >>>>>>
> >>>>>>>> version 2:
> >>>>>>>>       - do not set control read only as per Hans' comment:
> >>>>>>>>         See https://www.mail-archive.com/linux-media@vger.kernel.org/msg147910.html
> >>>>>>>>
> >>>>>>>>      drivers/media/i2c/ov5640.c | 10 ++++++++++
> >>>>>>>>      1 file changed, 10 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> >>>>>>>> index 82d4ce9..e6307f3 100644
> >>>>>>>> --- a/drivers/media/i2c/ov5640.c
> >>>>>>>> +++ b/drivers/media/i2c/ov5640.c
> >>>>>>>> @@ -218,6 +218,7 @@ struct ov5640_ctrls {
> >>>>>>>>      	struct v4l2_ctrl *test_pattern;
> >>>>>>>>      	struct v4l2_ctrl *hflip;
> >>>>>>>>      	struct v4l2_ctrl *vflip;
> >>>>>>>> +	struct v4l2_ctrl *link_freq;
> >>>>>>>>      };
> >>>>>>>>
> >>>>>>>>      struct ov5640_dev {
> >>>>>>>> @@ -2198,6 +2199,10 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd,
> >>>>>>>>      	return 0;
> >>>>>>>>      }
> >>>>>>>>
> >>>>>>>> +static const s64 link_freq_menu_items[] = {
> >>>>>>>> +	384000000,
> >>>>>>>> +};
> >>>>>>>> +
> >>>>>>>>      static int ov5640_set_fmt(struct v4l2_subdev *sd,
> >>>>>>>>      			  struct v4l2_subdev_pad_config *cfg,
> >>>>>>>>      			  struct v4l2_subdev_format *format)
> >>>>>>>> @@ -2636,6 +2641,8 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
> >>>>>>>>      	case V4L2_CID_VFLIP:
> >>>>>>>>      		ret = ov5640_set_ctrl_vflip(sensor, ctrl->val);
> >>>>>>>>      		break;
> >>>>>>>> +	case V4L2_CID_LINK_FREQ:
> >>>>>>>> +		return 0;
> >>>>>>>>      	default:
> >>>>>>>>      		ret = -EINVAL;
> >>>>>>>>      		break;
> >>>>>>>> @@ -2703,6 +2710,9 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
> >>>>>>>>      				       V4L2_CID_POWER_LINE_FREQUENCY_AUTO, 0,
> >>>>>>>>      				       V4L2_CID_POWER_LINE_FREQUENCY_50HZ);
> >>>>>>>>
> >>>>>>>> +	ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
> >>>>>>>> +						  0, 0, link_freq_menu_items);
> >>>>>>>> +
> >>>>>>>>      	if (hdl->error) {
> >>>>>>>>      		ret = hdl->error;
> >>>>>>>>      		goto free_ctrls;
> >>>>>>>> --
> >>>>>>>> 2.7.4
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>> BR,
> >>>>>> Hugues.
> >>>>>
> >>>>> --
> >>>>> Sakari Ailus
> >>>>> sakari.ailus@linux.intel.com
> >>>
> >>> BR,
> >>> Hugues.
> >>>
> >> BR,
> >> Hugues.
>
> BR,
> Hugues.

[v2] media: ov5640: add support of V4L2_CID_LINK_FREQ

Commit Message

Comments

Patch