| Message ID | 20201028225706.110078-1-jacopo+renesas@jmondi.org |
|---|---|
| Headers | show |
| Series | media: ov5640: Adjust htot, rework clock tree, add LINK_FREQ | expand |
Hi Jacopo, On 29/10/2020 00:57, Jacopo Mondi wrote: > Hi Hugues Tomi and Sam > > this small series collects Tomi's patch on adjusting htot which has been > floating around for some time with a rework of the clock tree based on > Hugues' and Sam's work on setting pclk_period. It also address the need to > suppport LINK_FREQUENCY control as pointed out by Hugues. > > I'm sort of happy with the result as I've removed quite some chrun and the clock > tree calculation is more linear. All modes work except full-resolution which a > bit annoys me, as I can't select it through s_fmt (to be honest I have not > investigated that in detail, that's why an RFC). > > Framerate is better than before, but still off for some combinations: > 640x480@30 gives me ~40 FPS, 1920x1080@15 gives me ~7. > The other combinations I've tested looks good. > > Can I have your opinion on these changes and if they help you with your > platforms? > > I've only been able to test YUYV, support for formats with != bpp will need > some work most probably, but that was like this before (although iirc Hugues > has captured JPEG, right ?) > > There's a bit more cleanup on top to be done (I've left TODOs around) and > probably the HBLANK calculation should be checked to see if it works with the > new htot values. Unfortunately the second patch seems to break capture on AM6 EVM + OV5640. The effect is pretty odd. The picture is very dark, with odd vertical lines, but it's still capturing something as I can see a correctly shaped shadow of my hand if I wave my hand over the sensor. Tomi
Hi Tomi,
thanks for testing
On Tue, Nov 03, 2020 at 09:19:17AM +0200, Tomi Valkeinen wrote:
> Hi Jacopo,
>
> On 29/10/2020 00:57, Jacopo Mondi wrote:
> > Hi Hugues Tomi and Sam
> >
> > this small series collects Tomi's patch on adjusting htot which has been
> > floating around for some time with a rework of the clock tree based on
> > Hugues' and Sam's work on setting pclk_period. It also address the need to
> > suppport LINK_FREQUENCY control as pointed out by Hugues.
> >
> > I'm sort of happy with the result as I've removed quite some chrun and the clock
> > tree calculation is more linear. All modes work except full-resolution which a
> > bit annoys me, as I can't select it through s_fmt (to be honest I have not
> > investigated that in detail, that's why an RFC).
> >
> > Framerate is better than before, but still off for some combinations:
> > 640x480@30 gives me ~40 FPS, 1920x1080@15 gives me ~7.
> > The other combinations I've tested looks good.
> >
> > Can I have your opinion on these changes and if they help you with your
> > platforms?
> >
> > I've only been able to test YUYV, support for formats with != bpp will need
> > some work most probably, but that was like this before (although iirc Hugues
> > has captured JPEG, right ?)
> >
> > There's a bit more cleanup on top to be done (I've left TODOs around) and
> > probably the HBLANK calculation should be checked to see if it works with the
> > new htot values.
>
> Unfortunately the second patch seems to break capture on AM6 EVM + OV5640. The effect is pretty odd.
> The picture is very dark, with odd vertical lines, but it's still capturing something as I can see a
> correctly shaped shadow of my hand if I wave my hand over the sensor.
This saddens me quite a lot :( The current clock tree programming
procedure is horrid and it has been bugging me for 2 years now :(
Is capture broken in all modes, or have you tested a single one ?
>
> Tomi
>
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 03/11/2020 10:19, Jacopo Mondi wrote: > Hi Tomi, > thanks for testing > > On Tue, Nov 03, 2020 at 09:19:17AM +0200, Tomi Valkeinen wrote: >> Hi Jacopo, >> >> On 29/10/2020 00:57, Jacopo Mondi wrote: >>> Hi Hugues Tomi and Sam >>> >>> this small series collects Tomi's patch on adjusting htot which has been >>> floating around for some time with a rework of the clock tree based on >>> Hugues' and Sam's work on setting pclk_period. It also address the need to >>> suppport LINK_FREQUENCY control as pointed out by Hugues. >>> >>> I'm sort of happy with the result as I've removed quite some chrun and the clock >>> tree calculation is more linear. All modes work except full-resolution which a >>> bit annoys me, as I can't select it through s_fmt (to be honest I have not >>> investigated that in detail, that's why an RFC). >>> >>> Framerate is better than before, but still off for some combinations: >>> 640x480@30 gives me ~40 FPS, 1920x1080@15 gives me ~7. >>> The other combinations I've tested looks good. >>> >>> Can I have your opinion on these changes and if they help you with your >>> platforms? >>> >>> I've only been able to test YUYV, support for formats with != bpp will need >>> some work most probably, but that was like this before (although iirc Hugues >>> has captured JPEG, right ?) >>> >>> There's a bit more cleanup on top to be done (I've left TODOs around) and >>> probably the HBLANK calculation should be checked to see if it works with the >>> new htot values. >> >> Unfortunately the second patch seems to break capture on AM6 EVM + OV5640. The effect is pretty odd. >> The picture is very dark, with odd vertical lines, but it's still capturing something as I can see a >> correctly shaped shadow of my hand if I wave my hand over the sensor. > > This saddens me quite a lot :( The current clock tree programming > procedure is horrid and it has been bugging me for 2 years now :( > > Is capture broken in all modes, or have you tested a single one ? I tested 640x480, 720x480, 720x576. I have only this sensor to test the CSI RX on AM6 EVM, so I would not be surprised if there are issues in the CSI RX driver (too). But this is super frustrating to debug, as the sensor is a badly documented black box, and I don't have means to probe the CSI lines... Tomi
Hi again, On Tue, Nov 03, 2020 at 10:24:38AM +0200, Tomi Valkeinen wrote: > On 03/11/2020 10:19, Jacopo Mondi wrote: > > Hi Tomi, > > thanks for testing > > > > On Tue, Nov 03, 2020 at 09:19:17AM +0200, Tomi Valkeinen wrote: > >> Hi Jacopo, > >> > >> On 29/10/2020 00:57, Jacopo Mondi wrote: > >>> Hi Hugues Tomi and Sam > >>> > >>> this small series collects Tomi's patch on adjusting htot which has been > >>> floating around for some time with a rework of the clock tree based on > >>> Hugues' and Sam's work on setting pclk_period. It also address the need to > >>> suppport LINK_FREQUENCY control as pointed out by Hugues. > >>> > >>> I'm sort of happy with the result as I've removed quite some chrun and the clock > >>> tree calculation is more linear. All modes work except full-resolution which a > >>> bit annoys me, as I can't select it through s_fmt (to be honest I have not > >>> investigated that in detail, that's why an RFC). > >>> > >>> Framerate is better than before, but still off for some combinations: > >>> 640x480@30 gives me ~40 FPS, 1920x1080@15 gives me ~7. > >>> The other combinations I've tested looks good. > >>> > >>> Can I have your opinion on these changes and if they help you with your > >>> platforms? > >>> > >>> I've only been able to test YUYV, support for formats with != bpp will need > >>> some work most probably, but that was like this before (although iirc Hugues > >>> has captured JPEG, right ?) > >>> > >>> There's a bit more cleanup on top to be done (I've left TODOs around) and > >>> probably the HBLANK calculation should be checked to see if it works with the > >>> new htot values. > >> > >> Unfortunately the second patch seems to break capture on AM6 EVM + OV5640. The effect is pretty odd. > >> The picture is very dark, with odd vertical lines, but it's still capturing something as I can see a > >> correctly shaped shadow of my hand if I wave my hand over the sensor. > > > > This saddens me quite a lot :( The current clock tree programming > > procedure is horrid and it has been bugging me for 2 years now :( > > > > Is capture broken in all modes, or have you tested a single one ? > > I tested 640x480, 720x480, 720x576. > > I have only this sensor to test the CSI RX on AM6 EVM, so I would not be surprised if there are > issues in the CSI RX driver (too). But this is super frustrating to debug, as the sensor is a badly > documented black box, and I don't have means to probe the CSI lines... I see.. I'm sure you noticed, but as you mentioned the 'second patch' I'll point it out anyway: the series has to be applied in full, as the last patch adds support for reporting the link frequency, that has been re-calculated by patch 2/3. On imx6 and on Hugues' platforms adjusting the receiver's link frequency based on what's reported makes a difference. Maybe Hugues can give this series a spin to provide an additional data point ? Thanks j > > Tomi > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Jacopo, On 11/3/20 9:45 AM, Jacopo Mondi wrote: > Hi again, > > On Tue, Nov 03, 2020 at 10:24:38AM +0200, Tomi Valkeinen wrote: >> On 03/11/2020 10:19, Jacopo Mondi wrote: >>> Hi Tomi, >>> thanks for testing >>> >>> On Tue, Nov 03, 2020 at 09:19:17AM +0200, Tomi Valkeinen wrote: >>>> Hi Jacopo, >>>> >>>> On 29/10/2020 00:57, Jacopo Mondi wrote: >>>>> Hi Hugues Tomi and Sam >>>>> >>>>> this small series collects Tomi's patch on adjusting htot which has been >>>>> floating around for some time with a rework of the clock tree based on >>>>> Hugues' and Sam's work on setting pclk_period. It also address the need to >>>>> suppport LINK_FREQUENCY control as pointed out by Hugues. >>>>> >>>>> I'm sort of happy with the result as I've removed quite some chrun and the clock >>>>> tree calculation is more linear. All modes work except full-resolution which a >>>>> bit annoys me, as I can't select it through s_fmt (to be honest I have not >>>>> investigated that in detail, that's why an RFC). >>>>> >>>>> Framerate is better than before, but still off for some combinations: >>>>> 640x480@30 gives me ~40 FPS, 1920x1080@15 gives me ~7. >>>>> The other combinations I've tested looks good. >>>>> >>>>> Can I have your opinion on these changes and if they help you with your >>>>> platforms? >>>>> >>>>> I've only been able to test YUYV, support for formats with != bpp will need >>>>> some work most probably, but that was like this before (although iirc Hugues >>>>> has captured JPEG, right ?) >>>>> >>>>> There's a bit more cleanup on top to be done (I've left TODOs around) and >>>>> probably the HBLANK calculation should be checked to see if it works with the >>>>> new htot values. >>>> >>>> Unfortunately the second patch seems to break capture on AM6 EVM + OV5640. The effect is pretty odd. >>>> The picture is very dark, with odd vertical lines, but it's still capturing something as I can see a >>>> correctly shaped shadow of my hand if I wave my hand over the sensor. >>> >>> This saddens me quite a lot :( The current clock tree programming >>> procedure is horrid and it has been bugging me for 2 years now :( >>> >>> Is capture broken in all modes, or have you tested a single one ? >> >> I tested 640x480, 720x480, 720x576. >> >> I have only this sensor to test the CSI RX on AM6 EVM, so I would not be surprised if there are >> issues in the CSI RX driver (too). But this is super frustrating to debug, as the sensor is a badly >> documented black box, and I don't have means to probe the CSI lines... > > I see.. I'm sure you noticed, but as you mentioned the 'second patch' > I'll point it out anyway: the series has to be applied in full, as the > last patch adds support for reporting the link frequency, that has > been re-calculated by patch 2/3. On imx6 and on Hugues' platforms > adjusting the receiver's link frequency based on what's reported makes a > difference. > > Maybe Hugues can give this series a spin to provide an additional data > point ? For sure, I'll try today. > > Thanks > j > >> >> Tomi >> >> -- >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. >> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 03/11/2020 10:45, Jacopo Mondi wrote: >> I tested 640x480, 720x480, 720x576. >> >> I have only this sensor to test the CSI RX on AM6 EVM, so I would not be surprised if there are >> issues in the CSI RX driver (too). But this is super frustrating to debug, as the sensor is a badly >> documented black box, and I don't have means to probe the CSI lines... > > I see.. I'm sure you noticed, but as you mentioned the 'second patch' > I'll point it out anyway: the series has to be applied in full, as the > last patch adds support for reporting the link frequency, that has > been re-calculated by patch 2/3. On imx6 and on Hugues' platforms > adjusting the receiver's link frequency based on what's reported makes a > difference. Yes, I first tried with all three, then tested one by one, and the second one started failing. drivers/media/platform/ti-vpe/cal-camerarx.c doesn't use V4L2_CID_LINK_FREQ (it uses V4L2_CID_PIXEL_RATE), though, so why would the third patch matter? Or do you mean that V4L2_CID_LINK_FREQ must be used to get ov5640 work? Aren't pixel rate and link freq directly linked? Tomi
Hi Tomi, On Tue, Nov 03, 2020 at 11:31:06AM +0200, Tomi Valkeinen wrote: > On 03/11/2020 10:45, Jacopo Mondi wrote: > > >> I tested 640x480, 720x480, 720x576. > >> > >> I have only this sensor to test the CSI RX on AM6 EVM, so I would not be surprised if there are > >> issues in the CSI RX driver (too). But this is super frustrating to debug, as the sensor is a badly > >> documented black box, and I don't have means to probe the CSI lines... > > > > I see.. I'm sure you noticed, but as you mentioned the 'second patch' > > I'll point it out anyway: the series has to be applied in full, as the > > last patch adds support for reporting the link frequency, that has > > been re-calculated by patch 2/3. On imx6 and on Hugues' platforms > > adjusting the receiver's link frequency based on what's reported makes a > > difference. > > Yes, I first tried with all three, then tested one by one, and the second one started failing. > Ok, thanks for the clarification. > drivers/media/platform/ti-vpe/cal-camerarx.c doesn't use V4L2_CID_LINK_FREQ (it uses > V4L2_CID_PIXEL_RATE), though, so why would the third patch matter? Or do you mean that > V4L2_CID_LINK_FREQ must be used to get ov5640 work? Aren't pixel rate and link freq directly linked? Oh I see. As I read in the driver the PIXEL_RATE control gets only updated when the frame interval is changed. It should be probably updated when the mode changes as well. Although, it would be fairly easy to deduct the pixel rate from the link frequency in the receiver. > > Tomi > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Jacopo,
Here is the results of tests with 0V5640 CSI-2 on Avenger96 board.
1) First of all, the framerate is broken, it is almost 2 times greater
that expected. Checking code it seems that mipi_div is missing when
computing link_freq:
+ /*
* The 'rate' parameter is the bitrate = VTOT * HTOT * FPS * BPP
*
* Adjust it to represent the CSI-2 link frequency and use it to
* update the associated control.
*/
- link_freq = rate / sensor->ep.bus.mipi_csi2.num_data_lanes / 2;
+ link_freq = rate / sensor->ep.bus.mipi_csi2.num_data_lanes / 2 / mipi_div;
To test the setup I have patched the link frequency control to report
dynamically the frequency instead of hardcoded value:
+#if 0
freq_index = OV5640_LINK_FREQS_NUM - 1;
for (i = 0; i < OV5640_LINK_FREQS_NUM; ++i) {
if (ov5640_link_freqs[i] == link_freq) {
@@ -966,18 +979,12 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev
*sensor,
ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, freq_index);
if (ret < 0)
return ret;
+#else
+ ov5640_link_freqs[0] = link_freq;
+ ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, 0);
+#endif
2) Second problem comes from "media: i2c: ov5640: Adjust htot", this is
breaking 1024x768@30fps & VGA@30fps which are slowdown to 15fps
3) I have some instabilities when switching between framerate, I have to
investigate the point. In few words this is a race problem between the
OV5640 which set the frequency control and the MIPID02 which read the
frequency control. I'll dig into the issue to see how to fix that.
To summarize:
-------------
1) "media: i2c: ov5640: Rework CSI-2 clock tree"
Almost OK but mipi_div is missing
2) "media: i2c: ov5640: Adjust htot"
Is breaking some resolutions/fps, so better to drop.
Tomi, perhaps could you recheck with the fixed Jacopo serie if you still
encounter your DPHY error issues ?
With 1) fixed and 2) reverted, I'm back on track and have a successfull
non-regression on my side + some better figures on some resolutions:
- 1024x768@30fps which was not at the right framerate previously
- 720p@30fps which was not at the right framerate previously
- HD@15fps which was not at the right framerate previously
Please note that I cannot go above HD@15fps on this platform.
* QCIF 176x144 RGB565 15fps => OK, got 15
* QCIF 176x144 YUYV 15fps => OK, got 15
* QCIF 176x144 JPEG 15fps => OK, got 15
* QCIF 176x144 RGB565 30fps => OK, got 30
* QCIF 176x144 YUYV 30fps => OK, got 30
* QCIF 176x144 JPEG 30fps => OK, got 30
* QVGA 320x240 RGB565 15fps => OK, got 15
* QVGA 320x240 YUYV 15fps => OK, got 15
* QVGA 320x240 JPEG 15fps => OK, got 15
* QVGA 320x240 RGB565 30fps => OK, got 29
* QVGA 320x240 YUYV 30fps => OK, got 30
* QVGA 320x240 JPEG 30fps => OK, got 29
* VGA 640x480 RGB565 15fps => OK, got 15
* VGA 640x480 YUYV 15fps => OK, got 15
* VGA 640x480 JPEG 15fps => OK, got 15
* VGA 640x480 RGB565 30fps => OK, got 30
* VGA 640x480 YUYV 30fps => OK, got 30
* VGA 640x480 JPEG 30fps => OK, got 30
* 480p 720x480 RGB565 15fps => OK, got 15
* 480p 720x480 YUYV 15fps => OK, got 15
* 480p 720x480 JPEG 15fps => OK, got 15
* 480p 720x480 RGB565 30fps => OK, got 30
* 480p 720x480 YUYV 30fps => OK, got 30
* 480p 720x480 JPEG 30fps => OK, got 30
* XGA 1024x768 RGB565 15fps => OK, got 15
* XGA 1024x768 YUYV 15fps => OK, got 15
* XGA 1024x768 JPEG 15fps => OK, got 15
* XGA 1024x768 RGB565 30fps => OK, got 30
* XGA 1024x768 YUYV 30fps => OK, got 30
* XGA 1024x768 JPEG 30fps => OK, got 30
* 720p 1280x720 RGB565 15fps => OK, got 15
* 720p 1280x720 YUYV 15fps => OK, got 15
* 720p 1280x720 JPEG 15fps => OK, got 15
* 720p 1280x720 RGB565 30fps => OK, got 30
* 720p 1280x720 YUYV 30fps => OK, got 30
* 720p 1280x720 JPEG 30fps => OK, got 30
* HD 1920x1080 RGB565 15fps => OK, got 15
* HD 1920x1080 YUYV 15fps => OK, got 15
* HD 1920x1080 JPEG 15fps => OK, got 15
So in few words, it sounds good, thanks Jacopo !
On 10/28/20 11:57 PM, Jacopo Mondi wrote:
> Hi Hugues Tomi and Sam
>
> this small series collects Tomi's patch on adjusting htot which has been
> floating around for some time with a rework of the clock tree based on
> Hugues' and Sam's work on setting pclk_period. It also address the need to
> suppport LINK_FREQUENCY control as pointed out by Hugues.
>
> I'm sort of happy with the result as I've removed quite some chrun and the clock
> tree calculation is more linear. All modes work except full-resolution which a
> bit annoys me, as I can't select it through s_fmt (to be honest I have not
> investigated that in detail, that's why an RFC).
>
> Framerate is better than before, but still off for some combinations:
> 640x480@30 gives me ~40 FPS, 1920x1080@15 gives me ~7.
> The other combinations I've tested looks good.
>
> Can I have your opinion on these changes and if they help you with your
> platforms?
>
> I've only been able to test YUYV, support for formats with != bpp will need
> some work most probably, but that was like this before (although iirc Hugues
> has captured JPEG, right ?)
>
> There's a bit more cleanup on top to be done (I've left TODOs around) and
> probably the HBLANK calculation should be checked to see if it works with the
> new htot values.
>
> Thanks
> j
>
> Jacopo Mondi (2):
> media: i2c: ov5640: Rework CSI-2 clock tree
> media: i2c: ov5640: Add V4L2_CID_LINK_FREQ support
>
> Tomi Valkeinen (1):
> media: i2c: ov5640: Adjust htot
>
> drivers/media/i2c/ov5640.c | 176 +++++++++++++++++++++++++------------
> 1 file changed, 118 insertions(+), 58 deletions(-)
>
> --
> 2.28.0
>
Best regards,
Hugues.
Hi Jacopo, Tomi, This patch is breaking 1024x768@30fps & VGA@30fps on my side which are slowdown to 15fps. Tomi, perhaps could you recheck with the fixed Jacopo serie if you still encounter your DPHY error issues ? On 10/28/20 11:57 PM, Jacopo Mondi wrote: > From: Tomi Valkeinen <tomi.valkeinen@ti.com> > > Adjust htot for most of the modes. The numbers are from the OV5640 > datasheet, and with these the driver works more reliably on DRA76 EVM + > OV5640, using 2 datalanes. > > Without the patch, I see often ComplexIO (i.e. PHY) errors when > starting the streaming, and 1280x720 does not work at all without this > change. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > drivers/media/i2c/ov5640.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index 8d0254d0e5ea..117ac22683ad 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -553,42 +553,42 @@ static const struct ov5640_mode_info ov5640_mode_init_data = { > static const struct ov5640_mode_info > ov5640_mode_data[OV5640_NUM_MODES] = { > {OV5640_MODE_QCIF_176_144, SUBSAMPLING, > - 176, 1896, 144, 984, > + 176, 2844, 144, 984, > ov5640_setting_QCIF_176_144, > ARRAY_SIZE(ov5640_setting_QCIF_176_144), > OV5640_30_FPS}, > {OV5640_MODE_QVGA_320_240, SUBSAMPLING, > - 320, 1896, 240, 984, > + 320, 2844, 240, 984, > ov5640_setting_QVGA_320_240, > ARRAY_SIZE(ov5640_setting_QVGA_320_240), > OV5640_30_FPS}, > {OV5640_MODE_VGA_640_480, SUBSAMPLING, > - 640, 1896, 480, 1080, > + 640, 2844, 480, 1080, > ov5640_setting_VGA_640_480, > ARRAY_SIZE(ov5640_setting_VGA_640_480), > OV5640_60_FPS}, > {OV5640_MODE_NTSC_720_480, SUBSAMPLING, > - 720, 1896, 480, 984, > + 720, 2844, 480, 984, > ov5640_setting_NTSC_720_480, > ARRAY_SIZE(ov5640_setting_NTSC_720_480), > OV5640_30_FPS}, > {OV5640_MODE_PAL_720_576, SUBSAMPLING, > - 720, 1896, 576, 984, > + 720, 2844, 576, 984, > ov5640_setting_PAL_720_576, > ARRAY_SIZE(ov5640_setting_PAL_720_576), > OV5640_30_FPS}, > {OV5640_MODE_XGA_1024_768, SUBSAMPLING, > - 1024, 1896, 768, 1080, > + 1024, 2844, 768, 1080, > ov5640_setting_XGA_1024_768, > ARRAY_SIZE(ov5640_setting_XGA_1024_768), > OV5640_30_FPS}, > {OV5640_MODE_720P_1280_720, SUBSAMPLING, > - 1280, 1892, 720, 740, > + 1280, 2844, 720, 740, > ov5640_setting_720P_1280_720, > ARRAY_SIZE(ov5640_setting_720P_1280_720), > OV5640_30_FPS}, > {OV5640_MODE_1080P_1920_1080, SCALING, > - 1920, 2500, 1080, 1120, > + 1920, 2844, 1080, 1120, > ov5640_setting_1080P_1920_1080, > ARRAY_SIZE(ov5640_setting_1080P_1920_1080), > OV5640_30_FPS}, > BR, Hugues.
Hello Hugues,
thanks so much for testing
On Tue, Nov 03, 2020 at 04:53:21PM +0000, Hugues FRUCHET wrote:
> Hi Jacopo,
>
> Here is the results of tests with 0V5640 CSI-2 on Avenger96 board.
>
> 1) First of all, the framerate is broken, it is almost 2 times greater
> that expected. Checking code it seems that mipi_div is missing when
> computing link_freq:
>
> + /*
> * The 'rate' parameter is the bitrate = VTOT * HTOT * FPS * BPP
> *
> * Adjust it to represent the CSI-2 link frequency and use it to
> * update the associated control.
> */
> - link_freq = rate / sensor->ep.bus.mipi_csi2.num_data_lanes / 2;
> + link_freq = rate / sensor->ep.bus.mipi_csi2.num_data_lanes / 2 / mipi_div;
I don't think this is correct I'm sorry.
In my platform this fixes (in example) 640x480@30FPS but breaks
640x480@15FPS which now runs at 7.5FPS (with Tomi's patch reverted)..
What a weird behaviour
The reasoning behing link_frequency calculation is that
pixel_rate = vtot * htot * fps
bit_rate = pixel_rate * bpp
link_freq = bit_rate / num_lanes / 2 (CSI-2 DDR)
MIPI_DIV is not yet into play, as we're calculating the CSI-2 clock
lane freqeuency without applying it to the clock tree
In my clock diagram link_freq is what is the MIPI_CLK output
To transform it in SYSCLK you walk the clock tree backward and
sysclk = link_freq * 2 * mipi_div
>
> To test the setup I have patched the link frequency control to report
> dynamically the frequency instead of hardcoded value:
> +#if 0
> freq_index = OV5640_LINK_FREQS_NUM - 1;
> for (i = 0; i < OV5640_LINK_FREQS_NUM; ++i) {
> if (ov5640_link_freqs[i] == link_freq) {
> @@ -966,18 +979,12 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev
> *sensor,
> ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, freq_index);
> if (ret < 0)
> return ret;
> +#else
> + ov5640_link_freqs[0] = link_freq;
> + ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, 0);
> +#endif
I wonder if this is acceptable for mainline. Pre-calculating the link
frequency is really a pain. I wonder why LINK_FREQ is a menu control
in first place :/
>
> 2) Second problem comes from "media: i2c: ov5640: Adjust htot", this is
> breaking 1024x768@30fps & VGA@30fps which are slowdown to 15fps
Weird, as 'Adjust htot' -increases- the htot values resulting in a
-faster- clock output, right ? Are you sure this is not due to the
above "/ mipi_div;" you've added ?
>
> 3) I have some instabilities when switching between framerate, I have to
> investigate the point. In few words this is a race problem between the
> OV5640 which set the frequency control and the MIPID02 which read the
> frequency control. I'll dig into the issue to see how to fix that.
>
>
> To summarize:
> -------------
> 1) "media: i2c: ov5640: Rework CSI-2 clock tree"
> Almost OK but mipi_div is missing
>
> 2) "media: i2c: ov5640: Adjust htot"
> Is breaking some resolutions/fps, so better to drop.
> Tomi, perhaps could you recheck with the fixed Jacopo serie if you still
> encounter your DPHY error issues ?
>
> With 1) fixed and 2) reverted, I'm back on track and have a successfull
> non-regression on my side + some better figures on some resolutions:
> - 1024x768@30fps which was not at the right framerate previously
> - 720p@30fps which was not at the right framerate previously
> - HD@15fps which was not at the right framerate previously
>
> Please note that I cannot go above HD@15fps on this platform.
>
> * QCIF 176x144 RGB565 15fps => OK, got 15
> * QCIF 176x144 YUYV 15fps => OK, got 15
> * QCIF 176x144 JPEG 15fps => OK, got 15
> * QCIF 176x144 RGB565 30fps => OK, got 30
> * QCIF 176x144 YUYV 30fps => OK, got 30
> * QCIF 176x144 JPEG 30fps => OK, got 30
> * QVGA 320x240 RGB565 15fps => OK, got 15
> * QVGA 320x240 YUYV 15fps => OK, got 15
> * QVGA 320x240 JPEG 15fps => OK, got 15
> * QVGA 320x240 RGB565 30fps => OK, got 29
> * QVGA 320x240 YUYV 30fps => OK, got 30
> * QVGA 320x240 JPEG 30fps => OK, got 29
> * VGA 640x480 RGB565 15fps => OK, got 15
> * VGA 640x480 YUYV 15fps => OK, got 15
> * VGA 640x480 JPEG 15fps => OK, got 15
> * VGA 640x480 RGB565 30fps => OK, got 30
> * VGA 640x480 YUYV 30fps => OK, got 30
> * VGA 640x480 JPEG 30fps => OK, got 30
> * 480p 720x480 RGB565 15fps => OK, got 15
> * 480p 720x480 YUYV 15fps => OK, got 15
> * 480p 720x480 JPEG 15fps => OK, got 15
> * 480p 720x480 RGB565 30fps => OK, got 30
> * 480p 720x480 YUYV 30fps => OK, got 30
> * 480p 720x480 JPEG 30fps => OK, got 30
> * XGA 1024x768 RGB565 15fps => OK, got 15
> * XGA 1024x768 YUYV 15fps => OK, got 15
> * XGA 1024x768 JPEG 15fps => OK, got 15
> * XGA 1024x768 RGB565 30fps => OK, got 30
> * XGA 1024x768 YUYV 30fps => OK, got 30
> * XGA 1024x768 JPEG 30fps => OK, got 30
> * 720p 1280x720 RGB565 15fps => OK, got 15
> * 720p 1280x720 YUYV 15fps => OK, got 15
> * 720p 1280x720 JPEG 15fps => OK, got 15
> * 720p 1280x720 RGB565 30fps => OK, got 30
> * 720p 1280x720 YUYV 30fps => OK, got 30
> * 720p 1280x720 JPEG 30fps => OK, got 30
> * HD 1920x1080 RGB565 15fps => OK, got 15
> * HD 1920x1080 YUYV 15fps => OK, got 15
> * HD 1920x1080 JPEG 15fps => OK, got 15
>
>
> So in few words, it sounds good, thanks Jacopo !
That's sweet, but doesn't match what I see on iMX.6 /o\
>
>
> On 10/28/20 11:57 PM, Jacopo Mondi wrote:
> > Hi Hugues Tomi and Sam
> >
> > this small series collects Tomi's patch on adjusting htot which has been
> > floating around for some time with a rework of the clock tree based on
> > Hugues' and Sam's work on setting pclk_period. It also address the need to
> > suppport LINK_FREQUENCY control as pointed out by Hugues.
> >
> > I'm sort of happy with the result as I've removed quite some chrun and the clock
> > tree calculation is more linear. All modes work except full-resolution which a
> > bit annoys me, as I can't select it through s_fmt (to be honest I have not
> > investigated that in detail, that's why an RFC).
> >
> > Framerate is better than before, but still off for some combinations:
> > 640x480@30 gives me ~40 FPS, 1920x1080@15 gives me ~7.
> > The other combinations I've tested looks good.
> >
> > Can I have your opinion on these changes and if they help you with your
> > platforms?
> >
> > I've only been able to test YUYV, support for formats with != bpp will need
> > some work most probably, but that was like this before (although iirc Hugues
> > has captured JPEG, right ?)
> >
> > There's a bit more cleanup on top to be done (I've left TODOs around) and
> > probably the HBLANK calculation should be checked to see if it works with the
> > new htot values.
> >
> > Thanks
> > j
> >
> > Jacopo Mondi (2):
> > media: i2c: ov5640: Rework CSI-2 clock tree
> > media: i2c: ov5640: Add V4L2_CID_LINK_FREQ support
> >
> > Tomi Valkeinen (1):
> > media: i2c: ov5640: Adjust htot
> >
> > drivers/media/i2c/ov5640.c | 176 +++++++++++++++++++++++++------------
> > 1 file changed, 118 insertions(+), 58 deletions(-)
> >
> > --
> > 2.28.0
> >
>
> Best regards,
> Hugues.
Hi Jacopo, On 11/5/20 11:14 AM, Jacopo Mondi wrote: > Hello Hugues, > > thanks so much for testing > > On Tue, Nov 03, 2020 at 04:53:21PM +0000, Hugues FRUCHET wrote: >> Hi Jacopo, >> >> Here is the results of tests with 0V5640 CSI-2 on Avenger96 board. >> >> 1) First of all, the framerate is broken, it is almost 2 times greater >> that expected. Checking code it seems that mipi_div is missing when >> computing link_freq: >> >> + /* >> * The 'rate' parameter is the bitrate = VTOT * HTOT * FPS * BPP >> * >> * Adjust it to represent the CSI-2 link frequency and use it to >> * update the associated control. >> */ >> - link_freq = rate / sensor->ep.bus.mipi_csi2.num_data_lanes / 2; >> + link_freq = rate / sensor->ep.bus.mipi_csi2.num_data_lanes / 2 / mipi_div; > > I don't think this is correct I'm sorry. > But this is what is observed with oscilloscope: 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 Frame rate set to 30.000 fps [ 3501.482829] ov5640 1-003c: Bandwidth Per Lane=491443200, 640x480 from 1896x1080 [ 3501.488822] ov5640 1-003c: ov5640_set_mipi_pclk: __v4l2_ctrl_s_ctrl 0 122860800 Hz [ 3501.496415] ov5640 1-003c: sysclk=491443200, _rate=492000000, mipi_div=2, prediv=3, mult=123, sysdiv=2 [ 3501.511064] ov5640 1-003c: PCLK PERIOD 0x4837=0x20 [ 3501.569487] st-mipid02 2-0014: clk_lane_reg1=0x41 <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.00 fps Measured #8ns (125MHz) ==> in line with 122860800 Hz 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 Frame rate set to 15.000 fps [ 5019.240550] ov5640 1-003c: Bandwidth Per Lane=245721600, 640x480 from 1896x1080 [ 5019.246542] ov5640 1-003c: ov5640_set_mipi_pclk: __v4l2_ctrl_s_ctrl 0 61430400 Hz [ 5019.257485] ov5640 1-003c: sysclk=245721600, _rate=246000000, mipi_div=2, prediv=3, mult=123, sysdiv=4 [ 5019.271894] ov5640 1-003c: PCLK PERIOD 0x4837=0x41 [ 5019.329693] st-mipid02 2-0014: clk_lane_reg1=0x81 <<<<<<<<<<<<<<<<< 15.09 fps Measured #16ns (62.5MHz) => in line with 61430400 Hz > In my platform this fixes (in example) 640x480@30FPS but breaks > 640x480@15FPS which now runs at 7.5FPS (with Tomi's patch reverted).. > What a weird behaviour > > The reasoning behing link_frequency calculation is that > > pixel_rate = vtot * htot * fps > bit_rate = pixel_rate * bpp > link_freq = bit_rate / num_lanes / 2 (CSI-2 DDR) > > MIPI_DIV is not yet into play, as we're calculating the CSI-2 clock > lane freqeuency without applying it to the clock tree > > In my clock diagram link_freq is what is the MIPI_CLK output > To transform it in SYSCLK you walk the clock tree backward and > > sysclk = link_freq * 2 * mipi_div > Could you add in your codebase the debug patches below and measure the clock lane frequency with oscilloscope so that we have a chance to understand what happens ? @@ -1842,6 +1842,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor) bool auto_exp = sensor->ctrls.auto_exp->val == V4L2_EXPOSURE_AUTO; unsigned long rate; int ret; + struct i2c_client *client = sensor->i2c_client; if (!orig_mode) orig_mode = mode; @@ -1867,6 +1868,10 @@ static int ov5640_set_mode(struct ov5640_dev *sensor) * the same rate than YUV, so we can just use 16 bpp all the time. */ rate = ov5640_calc_pixel_rate(sensor) * 16; + + dev_info(&client->dev, "Bandwidth Per Lane=%lu, %dx%d from %dx%d\n", + rate / sensor->ep.bus.mipi_csi2.num_data_lanes, mode->hact, mode->vact, mode->htot, mode->vtot); + if (sensor->ep.bus_type == V4L2_MBUS_CSI2) { @@ -944,6 +944,8 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev *sensor, unsigned long sysclk; u8 pclk_period; int ret; + struct i2c_client *client = sensor->i2c_client; + unsigned long _rate; sysclk = link_freq * 2 * mipi_div; - ov5640_calc_sys_clk(sensor, sysclk, &prediv, &mult, &sysdiv); + _rate = ov5640_calc_sys_clk(sensor, sysclk, &prediv, &mult, &sysdiv); + + dev_info(&client->dev, "sysclk=%lu, _rate=%lu, mipi_div=%d, prediv=%d, mult=%d, sysdiv=%d\n", + sysclk, _rate, mipi_div, prediv, mult, sysdiv); >> >> To test the setup I have patched the link frequency control to report >> dynamically the frequency instead of hardcoded value: >> +#if 0 >> freq_index = OV5640_LINK_FREQS_NUM - 1; >> for (i = 0; i < OV5640_LINK_FREQS_NUM; ++i) { >> if (ov5640_link_freqs[i] == link_freq) { >> @@ -966,18 +979,12 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev >> *sensor, >> ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, freq_index); >> if (ret < 0) >> return ret; >> +#else >> + ov5640_link_freqs[0] = link_freq; >> + ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, 0); >> +#endif > > I wonder if this is acceptable for mainline. Pre-calculating the link > frequency is really a pain. I wonder why LINK_FREQ is a menu control > in first place :/ > Yes would be nice to get rid of that. >> >> 2) Second problem comes from "media: i2c: ov5640: Adjust htot", this is >> breaking 1024x768@30fps & VGA@30fps which are slowdown to 15fps > > Weird, as 'Adjust htot' -increases- the htot values resulting in a > -faster- clock output, right ? Are you sure this is not due to the > above "/ mipi_div;" you've added ? > Another explanation is that there are errors so that 1/2 frame is dropped. >> >> 3) I have some instabilities when switching between framerate, I have to >> investigate the point. In few words this is a race problem between the >> OV5640 which set the frequency control and the MIPID02 which read the >> frequency control. I'll dig into the issue to see how to fix that. >> >> >> To summarize: >> ------------- >> 1) "media: i2c: ov5640: Rework CSI-2 clock tree" >> Almost OK but mipi_div is missing >> >> 2) "media: i2c: ov5640: Adjust htot" >> Is breaking some resolutions/fps, so better to drop. >> Tomi, perhaps could you recheck with the fixed Jacopo serie if you still >> encounter your DPHY error issues ? >> >> With 1) fixed and 2) reverted, I'm back on track and have a successfull >> non-regression on my side + some better figures on some resolutions: >> - 1024x768@30fps which was not at the right framerate previously >> - 720p@30fps which was not at the right framerate previously >> - HD@15fps which was not at the right framerate previously >> >> Please note that I cannot go above HD@15fps on this platform. >> >> * QCIF 176x144 RGB565 15fps => OK, got 15 >> * QCIF 176x144 YUYV 15fps => OK, got 15 >> * QCIF 176x144 JPEG 15fps => OK, got 15 >> * QCIF 176x144 RGB565 30fps => OK, got 30 >> * QCIF 176x144 YUYV 30fps => OK, got 30 >> * QCIF 176x144 JPEG 30fps => OK, got 30 >> * QVGA 320x240 RGB565 15fps => OK, got 15 >> * QVGA 320x240 YUYV 15fps => OK, got 15 >> * QVGA 320x240 JPEG 15fps => OK, got 15 >> * QVGA 320x240 RGB565 30fps => OK, got 29 >> * QVGA 320x240 YUYV 30fps => OK, got 30 >> * QVGA 320x240 JPEG 30fps => OK, got 29 >> * VGA 640x480 RGB565 15fps => OK, got 15 >> * VGA 640x480 YUYV 15fps => OK, got 15 >> * VGA 640x480 JPEG 15fps => OK, got 15 >> * VGA 640x480 RGB565 30fps => OK, got 30 >> * VGA 640x480 YUYV 30fps => OK, got 30 >> * VGA 640x480 JPEG 30fps => OK, got 30 >> * 480p 720x480 RGB565 15fps => OK, got 15 >> * 480p 720x480 YUYV 15fps => OK, got 15 >> * 480p 720x480 JPEG 15fps => OK, got 15 >> * 480p 720x480 RGB565 30fps => OK, got 30 >> * 480p 720x480 YUYV 30fps => OK, got 30 >> * 480p 720x480 JPEG 30fps => OK, got 30 >> * XGA 1024x768 RGB565 15fps => OK, got 15 >> * XGA 1024x768 YUYV 15fps => OK, got 15 >> * XGA 1024x768 JPEG 15fps => OK, got 15 >> * XGA 1024x768 RGB565 30fps => OK, got 30 >> * XGA 1024x768 YUYV 30fps => OK, got 30 >> * XGA 1024x768 JPEG 30fps => OK, got 30 >> * 720p 1280x720 RGB565 15fps => OK, got 15 >> * 720p 1280x720 YUYV 15fps => OK, got 15 >> * 720p 1280x720 JPEG 15fps => OK, got 15 >> * 720p 1280x720 RGB565 30fps => OK, got 30 >> * 720p 1280x720 YUYV 30fps => OK, got 30 >> * 720p 1280x720 JPEG 30fps => OK, got 30 >> * HD 1920x1080 RGB565 15fps => OK, got 15 >> * HD 1920x1080 YUYV 15fps => OK, got 15 >> * HD 1920x1080 JPEG 15fps => OK, got 15 >> >> >> So in few words, it sounds good, thanks Jacopo ! > > That's sweet, but doesn't match what I see on iMX.6 /o\ Yes, I feel that debug traces and oscilloscope will help to understand what happens. > > >> >> >> On 10/28/20 11:57 PM, Jacopo Mondi wrote: >>> Hi Hugues Tomi and Sam >>> >>> this small series collects Tomi's patch on adjusting htot which has been >>> floating around for some time with a rework of the clock tree based on >>> Hugues' and Sam's work on setting pclk_period. It also address the need to >>> suppport LINK_FREQUENCY control as pointed out by Hugues. >>> >>> I'm sort of happy with the result as I've removed quite some chrun and the clock >>> tree calculation is more linear. All modes work except full-resolution which a >>> bit annoys me, as I can't select it through s_fmt (to be honest I have not >>> investigated that in detail, that's why an RFC). >>> >>> Framerate is better than before, but still off for some combinations: >>> 640x480@30 gives me ~40 FPS, 1920x1080@15 gives me ~7. >>> The other combinations I've tested looks good. >>> >>> Can I have your opinion on these changes and if they help you with your >>> platforms? >>> >>> I've only been able to test YUYV, support for formats with != bpp will need >>> some work most probably, but that was like this before (although iirc Hugues >>> has captured JPEG, right ?) >>> >>> There's a bit more cleanup on top to be done (I've left TODOs around) and >>> probably the HBLANK calculation should be checked to see if it works with the >>> new htot values. >>> >>> Thanks >>> j >>> >>> Jacopo Mondi (2): >>> media: i2c: ov5640: Rework CSI-2 clock tree >>> media: i2c: ov5640: Add V4L2_CID_LINK_FREQ support >>> >>> Tomi Valkeinen (1): >>> media: i2c: ov5640: Adjust htot >>> >>> drivers/media/i2c/ov5640.c | 176 +++++++++++++++++++++++++------------ >>> 1 file changed, 118 insertions(+), 58 deletions(-) >>> >>> -- >>> 2.28.0 >>> >> >> Best regards, >> Hugues. BR, Hugues.
Hi Hugues, thanks for the detail, as soon as I have a bit of time I'll re-look into this. But in the meantime, I wonder, are you testing with JPEG only ? What is the bpp of a JPEG image ? So far, I only tested with YUYV as that's what I can capture on my platform... On Thu, Nov 05, 2020 at 03:33:18PM +0000, Hugues FRUCHET wrote: > Hi Jacopo, > > On 11/5/20 11:14 AM, Jacopo Mondi wrote: > > Hello Hugues, > > > > thanks so much for testing > > > > On Tue, Nov 03, 2020 at 04:53:21PM +0000, Hugues FRUCHET wrote: > >> Hi Jacopo, > >> > >> Here is the results of tests with 0V5640 CSI-2 on Avenger96 board. > >> > >> 1) First of all, the framerate is broken, it is almost 2 times greater > >> that expected. Checking code it seems that mipi_div is missing when > >> computing link_freq: > >> > >> + /* > >> * The 'rate' parameter is the bitrate = VTOT * HTOT * FPS * BPP > >> * > >> * Adjust it to represent the CSI-2 link frequency and use it to > >> * update the associated control. > >> */ > >> - link_freq = rate / sensor->ep.bus.mipi_csi2.num_data_lanes / 2; > >> + link_freq = rate / sensor->ep.bus.mipi_csi2.num_data_lanes / 2 / mipi_div; > > > > I don't think this is correct I'm sorry. > > > > But this is what is observed with oscilloscope: > > 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 > Frame rate set to 30.000 fps > [ 3501.482829] ov5640 1-003c: Bandwidth Per Lane=491443200, 640x480 from > 1896x1080 > [ 3501.488822] ov5640 1-003c: ov5640_set_mipi_pclk: __v4l2_ctrl_s_ctrl 0 > 122860800 Hz > [ 3501.496415] ov5640 1-003c: sysclk=491443200, _rate=492000000, > mipi_div=2, prediv=3, mult=123, sysdiv=2 > [ 3501.511064] ov5640 1-003c: PCLK PERIOD 0x4837=0x20 > [ 3501.569487] st-mipid02 2-0014: clk_lane_reg1=0x41 > <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 30.00 fps > Measured #8ns (125MHz) ==> in line with 122860800 Hz > > 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 > Frame rate set to 15.000 fps > [ 5019.240550] ov5640 1-003c: Bandwidth Per Lane=245721600, 640x480 from > 1896x1080 > [ 5019.246542] ov5640 1-003c: ov5640_set_mipi_pclk: __v4l2_ctrl_s_ctrl 0 > 61430400 Hz > [ 5019.257485] ov5640 1-003c: sysclk=245721600, _rate=246000000, > mipi_div=2, prediv=3, mult=123, sysdiv=4 > [ 5019.271894] ov5640 1-003c: PCLK PERIOD 0x4837=0x41 > [ 5019.329693] st-mipid02 2-0014: clk_lane_reg1=0x81 > <<<<<<<<<<<<<<<<< 15.09 fps > Measured #16ns (62.5MHz) => in line with 61430400 Hz > > > > In my platform this fixes (in example) 640x480@30FPS but breaks > > 640x480@15FPS which now runs at 7.5FPS (with Tomi's patch reverted).. > > What a weird behaviour > > > > The reasoning behing link_frequency calculation is that > > > > pixel_rate = vtot * htot * fps > > bit_rate = pixel_rate * bpp > > link_freq = bit_rate / num_lanes / 2 (CSI-2 DDR) > > > > MIPI_DIV is not yet into play, as we're calculating the CSI-2 clock > > lane freqeuency without applying it to the clock tree > > > > In my clock diagram link_freq is what is the MIPI_CLK output > > To transform it in SYSCLK you walk the clock tree backward and > > > > sysclk = link_freq * 2 * mipi_div > > > > Could you add in your codebase the debug patches below and measure the > clock lane frequency with oscilloscope so that we have a chance to > understand what happens ? > > > @@ -1842,6 +1842,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor) > bool auto_exp = sensor->ctrls.auto_exp->val == V4L2_EXPOSURE_AUTO; > unsigned long rate; > int ret; > + struct i2c_client *client = sensor->i2c_client; > > if (!orig_mode) > orig_mode = mode; > @@ -1867,6 +1868,10 @@ static int ov5640_set_mode(struct ov5640_dev *sensor) > * the same rate than YUV, so we can just use 16 bpp all the time. > */ > rate = ov5640_calc_pixel_rate(sensor) * 16; > + > + dev_info(&client->dev, "Bandwidth Per Lane=%lu, %dx%d from %dx%d\n", > + rate / sensor->ep.bus.mipi_csi2.num_data_lanes, mode->hact, > mode->vact, mode->htot, mode->vtot); > + > if (sensor->ep.bus_type == V4L2_MBUS_CSI2) { > > > > @@ -944,6 +944,8 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev > *sensor, > unsigned long sysclk; > u8 pclk_period; > int ret; > + struct i2c_client *client = sensor->i2c_client; > + unsigned long _rate; > > > sysclk = link_freq * 2 * mipi_div; > - ov5640_calc_sys_clk(sensor, sysclk, &prediv, &mult, &sysdiv); > + _rate = ov5640_calc_sys_clk(sensor, sysclk, &prediv, &mult, &sysdiv); > + > + dev_info(&client->dev, "sysclk=%lu, _rate=%lu, mipi_div=%d, prediv=%d, > mult=%d, sysdiv=%d\n", > + sysclk, _rate, mipi_div, prediv, mult, sysdiv); > > > > >> > >> To test the setup I have patched the link frequency control to report > >> dynamically the frequency instead of hardcoded value: > >> +#if 0 > >> freq_index = OV5640_LINK_FREQS_NUM - 1; > >> for (i = 0; i < OV5640_LINK_FREQS_NUM; ++i) { > >> if (ov5640_link_freqs[i] == link_freq) { > >> @@ -966,18 +979,12 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev > >> *sensor, > >> ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, freq_index); > >> if (ret < 0) > >> return ret; > >> +#else > >> + ov5640_link_freqs[0] = link_freq; > >> + ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, 0); > >> +#endif > > > > I wonder if this is acceptable for mainline. Pre-calculating the link > > frequency is really a pain. I wonder why LINK_FREQ is a menu control > > in first place :/ > > > > Yes would be nice to get rid of that. > > >> > >> 2) Second problem comes from "media: i2c: ov5640: Adjust htot", this is > >> breaking 1024x768@30fps & VGA@30fps which are slowdown to 15fps > > > > Weird, as 'Adjust htot' -increases- the htot values resulting in a > > -faster- clock output, right ? Are you sure this is not due to the > > above "/ mipi_div;" you've added ? > > > > Another explanation is that there are errors so that 1/2 frame is dropped. > > >> > >> 3) I have some instabilities when switching between framerate, I have to > >> investigate the point. In few words this is a race problem between the > >> OV5640 which set the frequency control and the MIPID02 which read the > >> frequency control. I'll dig into the issue to see how to fix that. > >> > >> > >> To summarize: > >> ------------- > >> 1) "media: i2c: ov5640: Rework CSI-2 clock tree" > >> Almost OK but mipi_div is missing > >> > >> 2) "media: i2c: ov5640: Adjust htot" > >> Is breaking some resolutions/fps, so better to drop. > >> Tomi, perhaps could you recheck with the fixed Jacopo serie if you still > >> encounter your DPHY error issues ? > >> > >> With 1) fixed and 2) reverted, I'm back on track and have a successfull > >> non-regression on my side + some better figures on some resolutions: > >> - 1024x768@30fps which was not at the right framerate previously > >> - 720p@30fps which was not at the right framerate previously > >> - HD@15fps which was not at the right framerate previously > >> > >> Please note that I cannot go above HD@15fps on this platform. > >> > >> * QCIF 176x144 RGB565 15fps => OK, got 15 > >> * QCIF 176x144 YUYV 15fps => OK, got 15 > >> * QCIF 176x144 JPEG 15fps => OK, got 15 > >> * QCIF 176x144 RGB565 30fps => OK, got 30 > >> * QCIF 176x144 YUYV 30fps => OK, got 30 > >> * QCIF 176x144 JPEG 30fps => OK, got 30 > >> * QVGA 320x240 RGB565 15fps => OK, got 15 > >> * QVGA 320x240 YUYV 15fps => OK, got 15 > >> * QVGA 320x240 JPEG 15fps => OK, got 15 > >> * QVGA 320x240 RGB565 30fps => OK, got 29 > >> * QVGA 320x240 YUYV 30fps => OK, got 30 > >> * QVGA 320x240 JPEG 30fps => OK, got 29 > >> * VGA 640x480 RGB565 15fps => OK, got 15 > >> * VGA 640x480 YUYV 15fps => OK, got 15 > >> * VGA 640x480 JPEG 15fps => OK, got 15 > >> * VGA 640x480 RGB565 30fps => OK, got 30 > >> * VGA 640x480 YUYV 30fps => OK, got 30 > >> * VGA 640x480 JPEG 30fps => OK, got 30 > >> * 480p 720x480 RGB565 15fps => OK, got 15 > >> * 480p 720x480 YUYV 15fps => OK, got 15 > >> * 480p 720x480 JPEG 15fps => OK, got 15 > >> * 480p 720x480 RGB565 30fps => OK, got 30 > >> * 480p 720x480 YUYV 30fps => OK, got 30 > >> * 480p 720x480 JPEG 30fps => OK, got 30 > >> * XGA 1024x768 RGB565 15fps => OK, got 15 > >> * XGA 1024x768 YUYV 15fps => OK, got 15 > >> * XGA 1024x768 JPEG 15fps => OK, got 15 > >> * XGA 1024x768 RGB565 30fps => OK, got 30 > >> * XGA 1024x768 YUYV 30fps => OK, got 30 > >> * XGA 1024x768 JPEG 30fps => OK, got 30 > >> * 720p 1280x720 RGB565 15fps => OK, got 15 > >> * 720p 1280x720 YUYV 15fps => OK, got 15 > >> * 720p 1280x720 JPEG 15fps => OK, got 15 > >> * 720p 1280x720 RGB565 30fps => OK, got 30 > >> * 720p 1280x720 YUYV 30fps => OK, got 30 > >> * 720p 1280x720 JPEG 30fps => OK, got 30 > >> * HD 1920x1080 RGB565 15fps => OK, got 15 > >> * HD 1920x1080 YUYV 15fps => OK, got 15 > >> * HD 1920x1080 JPEG 15fps => OK, got 15 > >> > >> > >> So in few words, it sounds good, thanks Jacopo ! > > > > That's sweet, but doesn't match what I see on iMX.6 /o\ > > Yes, I feel that debug traces and oscilloscope will help to understand > what happens. > > > > > > >> > >> > >> On 10/28/20 11:57 PM, Jacopo Mondi wrote: > >>> Hi Hugues Tomi and Sam > >>> > >>> this small series collects Tomi's patch on adjusting htot which has been > >>> floating around for some time with a rework of the clock tree based on > >>> Hugues' and Sam's work on setting pclk_period. It also address the need to > >>> suppport LINK_FREQUENCY control as pointed out by Hugues. > >>> > >>> I'm sort of happy with the result as I've removed quite some chrun and the clock > >>> tree calculation is more linear. All modes work except full-resolution which a > >>> bit annoys me, as I can't select it through s_fmt (to be honest I have not > >>> investigated that in detail, that's why an RFC). > >>> > >>> Framerate is better than before, but still off for some combinations: > >>> 640x480@30 gives me ~40 FPS, 1920x1080@15 gives me ~7. > >>> The other combinations I've tested looks good. > >>> > >>> Can I have your opinion on these changes and if they help you with your > >>> platforms? > >>> > >>> I've only been able to test YUYV, support for formats with != bpp will need > >>> some work most probably, but that was like this before (although iirc Hugues > >>> has captured JPEG, right ?) > >>> > >>> There's a bit more cleanup on top to be done (I've left TODOs around) and > >>> probably the HBLANK calculation should be checked to see if it works with the > >>> new htot values. > >>> > >>> Thanks > >>> j > >>> > >>> Jacopo Mondi (2): > >>> media: i2c: ov5640: Rework CSI-2 clock tree > >>> media: i2c: ov5640: Add V4L2_CID_LINK_FREQ support > >>> > >>> Tomi Valkeinen (1): > >>> media: i2c: ov5640: Adjust htot > >>> > >>> drivers/media/i2c/ov5640.c | 176 +++++++++++++++++++++++++------------ > >>> 1 file changed, 118 insertions(+), 58 deletions(-) > >>> > >>> -- > >>> 2.28.0 > >>> > >> > >> Best regards, > >> Hugues. > > BR, > Hugues.