| Message ID | 20181011092107.30715-6-maxime.ripard@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ov5640: Misc cleanup and improvements | expand |
Hi Maxime, A couple of questions, Maxime Ripard <maxime.ripard@bootlin.com> wrote on Thu [2018-Oct-11 04:21:00 -0500]: > The clock rate, while hardcoded until now, is actually a function of the > resolution, framerate and bytes per pixel. Now that we have an algorithm to > adjust our clock rate, we can select it dynamically when we change the > mode. > > This changes a bit the clock rate being used, with the following effect: > > +------+------+------+------+-----+-----------------+----------------+-----------+ > | Hact | Vact | Htot | Vtot | FPS | Hardcoded clock | Computed clock | Deviation | > +------+------+------+------+-----+-----------------+----------------+-----------+ > | 640 | 480 | 1896 | 1080 | 15 | 56000000 | 61430400 | 8.84 % | > | 640 | 480 | 1896 | 1080 | 30 | 112000000 | 122860800 | 8.84 % | > | 1024 | 768 | 1896 | 1080 | 15 | 56000000 | 61430400 | 8.84 % | > | 1024 | 768 | 1896 | 1080 | 30 | 112000000 | 122860800 | 8.84 % | > | 320 | 240 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > | 320 | 240 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > | 176 | 144 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > | 176 | 144 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > | 720 | 480 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > | 720 | 480 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > | 720 | 576 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > | 720 | 576 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > | 1280 | 720 | 1892 | 740 | 15 | 42000000 | 42002400 | 0.01 % | > | 1280 | 720 | 1892 | 740 | 30 | 84000000 | 84004800 | 0.01 % | > | 1920 | 1080 | 2500 | 1120 | 15 | 84000000 | 84000000 | 0.00 % | > | 1920 | 1080 | 2500 | 1120 | 30 | 168000000 | 168000000 | 0.00 % | > | 2592 | 1944 | 2844 | 1944 | 15 | 84000000 | 165862080 | 49.36 % | > +------+------+------+------+-----+-----------------+----------------+-----------+ Is the computed clock above the same for both parallel and CSI2? I want to add controls for PIXEL_RATE and LINK_FREQ, would you have any quick pointer on taking the computed clock and translating that into the PIXEL_RATE and LINK_FREQ values? I am trying to use this sensor with TI CAL driver which at the moment uses the PIXEL_RATE values in order to compute ths_settle and ths_term values needed to program the DPHY properly. This is similar in behavior as the way omap3isp relies on this info as well. Regards, Benoit > > Only the 640x480, 1024x768 and 2592x1944 modes are significantly affected > by the new formula. > > In this case, 640x480 and 1024x768 are actually fixed by this change. > Indeed, the sensor was sending data at, for example, 27.33fps instead of > 30fps. This is -9%, which is roughly what we're seeing in the array. > Testing these modes with the new clock setup actually fix that error, and > data are now sent at around 30fps. > > 2592x1944, on the other hand, is probably due to the fact that this mode > can only be used using MIPI-CSI2, in a two lane mode, and never really > tested with a DVP bus. > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > --- > drivers/media/i2c/ov5640.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index 5114d401b8eb..34eaa9dd5237 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -1915,7 +1915,8 @@ static int ov5640_set_mode(struct ov5640_dev *sensor) > * which is 8 bits per pixel. > */ > bpp = sensor->fmt.code == MEDIA_BUS_FMT_JPEG_1X8 ? 8 : 16; > - rate = mode->pixel_clock * bpp; > + rate = mode->vtot * mode->htot * bpp; > + rate *= ov5640_framerates[sensor->current_fr]; > if (sensor->ep.bus_type == V4L2_MBUS_CSI2) { > rate = rate / sensor->ep.bus.mipi_csi2.num_data_lanes; > ret = ov5640_set_mipi_pclk(sensor, rate); > -- > 2.19.1 >
On Thu, Feb 21, 2019 at 10:20:20AM -0600, Benoit Parrot wrote: > Hi Maxime, > > A couple of questions, > > Maxime Ripard <maxime.ripard@bootlin.com> wrote on Thu [2018-Oct-11 04:21:00 -0500]: > > The clock rate, while hardcoded until now, is actually a function of the > > resolution, framerate and bytes per pixel. Now that we have an algorithm to > > adjust our clock rate, we can select it dynamically when we change the > > mode. > > > > This changes a bit the clock rate being used, with the following effect: > > > > +------+------+------+------+-----+-----------------+----------------+-----------+ > > | Hact | Vact | Htot | Vtot | FPS | Hardcoded clock | Computed clock | Deviation | > > +------+------+------+------+-----+-----------------+----------------+-----------+ > > | 640 | 480 | 1896 | 1080 | 15 | 56000000 | 61430400 | 8.84 % | > > | 640 | 480 | 1896 | 1080 | 30 | 112000000 | 122860800 | 8.84 % | > > | 1024 | 768 | 1896 | 1080 | 15 | 56000000 | 61430400 | 8.84 % | > > | 1024 | 768 | 1896 | 1080 | 30 | 112000000 | 122860800 | 8.84 % | > > | 320 | 240 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > | 320 | 240 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > | 176 | 144 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > | 176 | 144 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > | 720 | 480 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > | 720 | 480 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > | 720 | 576 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > | 720 | 576 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > | 1280 | 720 | 1892 | 740 | 15 | 42000000 | 42002400 | 0.01 % | > > | 1280 | 720 | 1892 | 740 | 30 | 84000000 | 84004800 | 0.01 % | > > | 1920 | 1080 | 2500 | 1120 | 15 | 84000000 | 84000000 | 0.00 % | > > | 1920 | 1080 | 2500 | 1120 | 30 | 168000000 | 168000000 | 0.00 % | > > | 2592 | 1944 | 2844 | 1944 | 15 | 84000000 | 165862080 | 49.36 % | > > +------+------+------+------+-----+-----------------+----------------+-----------+ > > Is the computed clock above the same for both parallel and CSI2? > > I want to add controls for PIXEL_RATE and LINK_FREQ, would you have any > quick pointer on taking the computed clock and translating that into the > PIXEL_RATE and LINK_FREQ values? > > I am trying to use this sensor with TI CAL driver which at the moment uses > the PIXEL_RATE values in order to compute ths_settle and ths_term values > needed to program the DPHY properly. This is similar in behavior as the way > omap3isp relies on this info as well. I haven't looked that much into the csi-2 case, but the pixel rate should be the same at least. Maxime
Maxime Ripard <maxime.ripard@bootlin.com> wrote on Fri [2019-Feb-22 15:39:59 +0100]: > On Thu, Feb 21, 2019 at 10:20:20AM -0600, Benoit Parrot wrote: > > Hi Maxime, > > > > A couple of questions, > > > > Maxime Ripard <maxime.ripard@bootlin.com> wrote on Thu [2018-Oct-11 04:21:00 -0500]: > > > The clock rate, while hardcoded until now, is actually a function of the > > > resolution, framerate and bytes per pixel. Now that we have an algorithm to > > > adjust our clock rate, we can select it dynamically when we change the > > > mode. > > > > > > This changes a bit the clock rate being used, with the following effect: > > > > > > +------+------+------+------+-----+-----------------+----------------+-----------+ > > > | Hact | Vact | Htot | Vtot | FPS | Hardcoded clock | Computed clock | Deviation | > > > +------+------+------+------+-----+-----------------+----------------+-----------+ > > > | 640 | 480 | 1896 | 1080 | 15 | 56000000 | 61430400 | 8.84 % | > > > | 640 | 480 | 1896 | 1080 | 30 | 112000000 | 122860800 | 8.84 % | > > > | 1024 | 768 | 1896 | 1080 | 15 | 56000000 | 61430400 | 8.84 % | > > > | 1024 | 768 | 1896 | 1080 | 30 | 112000000 | 122860800 | 8.84 % | > > > | 320 | 240 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > > | 320 | 240 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > > | 176 | 144 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > > | 176 | 144 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > > | 720 | 480 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > > | 720 | 480 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > > | 720 | 576 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > > | 720 | 576 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > > | 1280 | 720 | 1892 | 740 | 15 | 42000000 | 42002400 | 0.01 % | > > > | 1280 | 720 | 1892 | 740 | 30 | 84000000 | 84004800 | 0.01 % | > > > | 1920 | 1080 | 2500 | 1120 | 15 | 84000000 | 84000000 | 0.00 % | > > > | 1920 | 1080 | 2500 | 1120 | 30 | 168000000 | 168000000 | 0.00 % | > > > | 2592 | 1944 | 2844 | 1944 | 15 | 84000000 | 165862080 | 49.36 % | > > > +------+------+------+------+-----+-----------------+----------------+-----------+ > > > > Is the computed clock above the same for both parallel and CSI2? > > > > I want to add controls for PIXEL_RATE and LINK_FREQ, would you have any > > quick pointer on taking the computed clock and translating that into the > > PIXEL_RATE and LINK_FREQ values? > > > > I am trying to use this sensor with TI CAL driver which at the moment uses > > the PIXEL_RATE values in order to compute ths_settle and ths_term values > > needed to program the DPHY properly. This is similar in behavior as the way > > omap3isp relies on this info as well. > > I haven't looked that much into the csi-2 case, but the pixel rate > should be the same at least. I'll have to study the way the computed clock is actually calculated for either case, but if they yield the same number then I would be surprised that the pixel rate would be the same as in parallel mode you get 8 data bits per clock whereas in CSI2 using 2 data lanes you get 4 data bits per clock. So just to be certain here the "Computed clock" column above would be the pixel clock frequency? Benoit > > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On Fri, Feb 22, 2019 at 08:54:56AM -0600, Benoit Parrot wrote: > Maxime Ripard <maxime.ripard@bootlin.com> wrote on Fri [2019-Feb-22 15:39:59 +0100]: > > On Thu, Feb 21, 2019 at 10:20:20AM -0600, Benoit Parrot wrote: > > > Hi Maxime, > > > > > > A couple of questions, > > > > > > Maxime Ripard <maxime.ripard@bootlin.com> wrote on Thu [2018-Oct-11 04:21:00 -0500]: > > > > The clock rate, while hardcoded until now, is actually a function of the > > > > resolution, framerate and bytes per pixel. Now that we have an algorithm to > > > > adjust our clock rate, we can select it dynamically when we change the > > > > mode. > > > > > > > > This changes a bit the clock rate being used, with the following effect: > > > > > > > > +------+------+------+------+-----+-----------------+----------------+-----------+ > > > > | Hact | Vact | Htot | Vtot | FPS | Hardcoded clock | Computed clock | Deviation | > > > > +------+------+------+------+-----+-----------------+----------------+-----------+ > > > > | 640 | 480 | 1896 | 1080 | 15 | 56000000 | 61430400 | 8.84 % | > > > > | 640 | 480 | 1896 | 1080 | 30 | 112000000 | 122860800 | 8.84 % | > > > > | 1024 | 768 | 1896 | 1080 | 15 | 56000000 | 61430400 | 8.84 % | > > > > | 1024 | 768 | 1896 | 1080 | 30 | 112000000 | 122860800 | 8.84 % | > > > > | 320 | 240 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > > > | 320 | 240 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > > > | 176 | 144 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > > > | 176 | 144 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > > > | 720 | 480 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > > > | 720 | 480 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > > > | 720 | 576 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > > > | 720 | 576 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > > > | 1280 | 720 | 1892 | 740 | 15 | 42000000 | 42002400 | 0.01 % | > > > > | 1280 | 720 | 1892 | 740 | 30 | 84000000 | 84004800 | 0.01 % | > > > > | 1920 | 1080 | 2500 | 1120 | 15 | 84000000 | 84000000 | 0.00 % | > > > > | 1920 | 1080 | 2500 | 1120 | 30 | 168000000 | 168000000 | 0.00 % | > > > > | 2592 | 1944 | 2844 | 1944 | 15 | 84000000 | 165862080 | 49.36 % | > > > > +------+------+------+------+-----+-----------------+----------------+-----------+ > > > > > > Is the computed clock above the same for both parallel and CSI2? > > > > > > I want to add controls for PIXEL_RATE and LINK_FREQ, would you have any > > > quick pointer on taking the computed clock and translating that into the > > > PIXEL_RATE and LINK_FREQ values? > > > > > > I am trying to use this sensor with TI CAL driver which at the moment uses > > > the PIXEL_RATE values in order to compute ths_settle and ths_term values > > > needed to program the DPHY properly. This is similar in behavior as the way > > > omap3isp relies on this info as well. > > > > I haven't looked that much into the csi-2 case, but the pixel rate > > should be the same at least. > > I'll have to study the way the computed clock is actually calculated for > either case, but if they yield the same number then I would be surprised > that the pixel rate would be the same as in parallel mode you get 8 data > bits per clock whereas in CSI2 using 2 data lanes you get 4 data bits per > clock. The bus rate will be different, but the pixel rate is the same: you have as many pixels per frames and as many frames per seconds in the parallel and CSI cases. > So just to be certain here the "Computed clock" column above would be the > pixel clock frequency? it is Maxime
Hello Maxime, Benoit, sorry for chiming in, but I'm a bit confused... On Fri, Feb 22, 2019 at 04:04:21PM +0100, Maxime Ripard wrote: > On Fri, Feb 22, 2019 at 08:54:56AM -0600, Benoit Parrot wrote: > > Maxime Ripard <maxime.ripard@bootlin.com> wrote on Fri [2019-Feb-22 15:39:59 +0100]: > > > On Thu, Feb 21, 2019 at 10:20:20AM -0600, Benoit Parrot wrote: > > > > Hi Maxime, > > > > > > > > A couple of questions, > > > > > > > > Maxime Ripard <maxime.ripard@bootlin.com> wrote on Thu [2018-Oct-11 04:21:00 -0500]: > > > > > The clock rate, while hardcoded until now, is actually a function of the > > > > > resolution, framerate and bytes per pixel. Now that we have an algorithm to > > > > > adjust our clock rate, we can select it dynamically when we change the > > > > > mode. > > > > > > > > > > This changes a bit the clock rate being used, with the following effect: > > > > > > > > > > +------+------+------+------+-----+-----------------+----------------+-----------+ > > > > > | Hact | Vact | Htot | Vtot | FPS | Hardcoded clock | Computed clock | Deviation | > > > > > +------+------+------+------+-----+-----------------+----------------+-----------+ > > > > > | 640 | 480 | 1896 | 1080 | 15 | 56000000 | 61430400 | 8.84 % | > > > > > | 640 | 480 | 1896 | 1080 | 30 | 112000000 | 122860800 | 8.84 % | > > > > > | 1024 | 768 | 1896 | 1080 | 15 | 56000000 | 61430400 | 8.84 % | > > > > > | 1024 | 768 | 1896 | 1080 | 30 | 112000000 | 122860800 | 8.84 % | > > > > > | 320 | 240 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > > > > | 320 | 240 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > > > > | 176 | 144 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > > > > | 176 | 144 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > > > > | 720 | 480 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > > > > | 720 | 480 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > > > > | 720 | 576 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > > > > | 720 | 576 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > > > > | 1280 | 720 | 1892 | 740 | 15 | 42000000 | 42002400 | 0.01 % | > > > > > | 1280 | 720 | 1892 | 740 | 30 | 84000000 | 84004800 | 0.01 % | > > > > > | 1920 | 1080 | 2500 | 1120 | 15 | 84000000 | 84000000 | 0.00 % | > > > > > | 1920 | 1080 | 2500 | 1120 | 30 | 168000000 | 168000000 | 0.00 % | > > > > > | 2592 | 1944 | 2844 | 1944 | 15 | 84000000 | 165862080 | 49.36 % | > > > > > +------+------+------+------+-----+-----------------+----------------+-----------+ > > > > > > > > Is the computed clock above the same for both parallel and CSI2? > > > > > > > > I want to add controls for PIXEL_RATE and LINK_FREQ, would you have any > > > > quick pointer on taking the computed clock and translating that into the > > > > PIXEL_RATE and LINK_FREQ values? > > > > > > > > I am trying to use this sensor with TI CAL driver which at the moment uses > > > > the PIXEL_RATE values in order to compute ths_settle and ths_term values > > > > needed to program the DPHY properly. This is similar in behavior as the way > > > > omap3isp relies on this info as well. > > > > > > I haven't looked that much into the csi-2 case, but the pixel rate > > > should be the same at least. > > > > I'll have to study the way the computed clock is actually calculated for > > either case, but if they yield the same number then I would be surprised > > that the pixel rate would be the same as in parallel mode you get 8 data > > bits per clock whereas in CSI2 using 2 data lanes you get 4 data bits per > > clock. > > The bus rate will be different, but the pixel rate is the same: you > have as many pixels per frames and as many frames per seconds in the > parallel and CSI cases. > I agree with that, but.. > > So just to be certain here the "Computed clock" column above would be the > > pixel clock frequency? > > it is ...it seems to me the Computed clock column is actually the "byte clock". From a simple calculation for the 640x480@15FPS case: "Computed clock" = 1896 * 1080 * 15 * 2 = 61430400 While, in my understanding, the pixel clock would just be pixel_clock = HTOT * VTOT * FPS = 1896 * 1080 * 15 = 30715200 So I suspect the "* 2" there is the number of bytes per pixel. That would match what's also reported here file:///home/jmondi/project/renesas/linux/linux-build/Documentation/output/media/kapi/csi2.html?highlight=link_freq Where: link_freq = (pixel_rate * bpp) / (2 * nr_lanes) So if I were to calculate PIXEL_RATE and LINK_FREQ in this driver, that would be: PIXEL_RATE = mode->vtot * mode->htot * ov5640_framerates[sensor->current_fr]; LINK_FREQ = PIXEL_RATE * 16 / ( 2 * sensor->ep.bus.mipi_csi2.num_data_lanes); (assuming, as the driver does now, all formats have 16bpp) Does this match your understanding as well? Thanks j > > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
Sorry again, On Mon, Feb 25, 2019 at 10:21:51AM +0100, Jacopo Mondi wrote: > Hello Maxime, Benoit, > sorry for chiming in, but I'm a bit confused... > > On Fri, Feb 22, 2019 at 04:04:21PM +0100, Maxime Ripard wrote: > > On Fri, Feb 22, 2019 at 08:54:56AM -0600, Benoit Parrot wrote: > > > Maxime Ripard <maxime.ripard@bootlin.com> wrote on Fri [2019-Feb-22 15:39:59 +0100]: > > > > On Thu, Feb 21, 2019 at 10:20:20AM -0600, Benoit Parrot wrote: > > > > > Hi Maxime, > > > > > > > > > > A couple of questions, > > > > > > > > > > Maxime Ripard <maxime.ripard@bootlin.com> wrote on Thu [2018-Oct-11 04:21:00 -0500]: > > > > > > The clock rate, while hardcoded until now, is actually a function of the > > > > > > resolution, framerate and bytes per pixel. Now that we have an algorithm to > > > > > > adjust our clock rate, we can select it dynamically when we change the > > > > > > mode. > > > > > > > > > > > > This changes a bit the clock rate being used, with the following effect: > > > > > > > > > > > > +------+------+------+------+-----+-----------------+----------------+-----------+ > > > > > > | Hact | Vact | Htot | Vtot | FPS | Hardcoded clock | Computed clock | Deviation | > > > > > > +------+------+------+------+-----+-----------------+----------------+-----------+ > > > > > > | 640 | 480 | 1896 | 1080 | 15 | 56000000 | 61430400 | 8.84 % | > > > > > > | 640 | 480 | 1896 | 1080 | 30 | 112000000 | 122860800 | 8.84 % | > > > > > > | 1024 | 768 | 1896 | 1080 | 15 | 56000000 | 61430400 | 8.84 % | > > > > > > | 1024 | 768 | 1896 | 1080 | 30 | 112000000 | 122860800 | 8.84 % | > > > > > > | 320 | 240 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > > > > > | 320 | 240 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > > > > > | 176 | 144 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > > > > > | 176 | 144 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > > > > > | 720 | 480 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > > > > > | 720 | 480 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > > > > > | 720 | 576 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > > > > > | 720 | 576 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > > > > > | 1280 | 720 | 1892 | 740 | 15 | 42000000 | 42002400 | 0.01 % | > > > > > > | 1280 | 720 | 1892 | 740 | 30 | 84000000 | 84004800 | 0.01 % | > > > > > > | 1920 | 1080 | 2500 | 1120 | 15 | 84000000 | 84000000 | 0.00 % | > > > > > > | 1920 | 1080 | 2500 | 1120 | 30 | 168000000 | 168000000 | 0.00 % | > > > > > > | 2592 | 1944 | 2844 | 1944 | 15 | 84000000 | 165862080 | 49.36 % | > > > > > > +------+------+------+------+-----+-----------------+----------------+-----------+ > > > > > > > > > > Is the computed clock above the same for both parallel and CSI2? > > > > > > > > > > I want to add controls for PIXEL_RATE and LINK_FREQ, would you have any > > > > > quick pointer on taking the computed clock and translating that into the > > > > > PIXEL_RATE and LINK_FREQ values? > > > > > > > > > > I am trying to use this sensor with TI CAL driver which at the moment uses > > > > > the PIXEL_RATE values in order to compute ths_settle and ths_term values > > > > > needed to program the DPHY properly. This is similar in behavior as the way > > > > > omap3isp relies on this info as well. > > > > > > > > I haven't looked that much into the csi-2 case, but the pixel rate > > > > should be the same at least. > > > > > > I'll have to study the way the computed clock is actually calculated for > > > either case, but if they yield the same number then I would be surprised > > > that the pixel rate would be the same as in parallel mode you get 8 data > > > bits per clock whereas in CSI2 using 2 data lanes you get 4 data bits per > > > clock. > > > > The bus rate will be different, but the pixel rate is the same: you > > have as many pixels per frames and as many frames per seconds in the > > parallel and CSI cases. > > > > I agree with that, but.. > > > > So just to be certain here the "Computed clock" column above would be the > > > pixel clock frequency? > > > > it is > > ...it seems to me the Computed clock column is actually the "byte clock". > > From a simple calculation for the 640x480@15FPS case: > "Computed clock" = 1896 * 1080 * 15 * 2 = 61430400 > > While, in my understanding, the pixel clock would just be > pixel_clock = HTOT * VTOT * FPS = 1896 * 1080 * 15 = 30715200 > > So I suspect the "* 2" there is the number of bytes per pixel. > > That would match what's also reported here > file:///home/jmondi/project/renesas/linux/linux-build/Documentation/output/media/kapi/csi2.html?highlight=link_freq > Of course that is a link to the local copy of the documentation, what I actually meant to link was: https://www.kernel.org/doc/html/latest/media/kapi/csi2.html?highlight=link_freq Sorry! j > Where: > link_freq = (pixel_rate * bpp) / (2 * nr_lanes) > > So if I were to calculate PIXEL_RATE and LINK_FREQ in this driver, > that would be: > PIXEL_RATE = mode->vtot * mode->htot * ov5640_framerates[sensor->current_fr]; > LINK_FREQ = PIXEL_RATE * 16 / ( 2 * sensor->ep.bus.mipi_csi2.num_data_lanes); > (assuming, as the driver does now, all formats have 16bpp) > > Does this match your understanding as well? > > Thanks > j > > > > > Maxime > > > > -- > > Maxime Ripard, Bootlin > > Embedded Linux and Kernel engineering > > https://bootlin.com > >
On Mon, Feb 25, 2019 at 10:21:51AM +0100, Jacopo Mondi wrote: > Hello Maxime, Benoit, > sorry for chiming in, but I'm a bit confused... > > On Fri, Feb 22, 2019 at 04:04:21PM +0100, Maxime Ripard wrote: > > On Fri, Feb 22, 2019 at 08:54:56AM -0600, Benoit Parrot wrote: > > > Maxime Ripard <maxime.ripard@bootlin.com> wrote on Fri [2019-Feb-22 15:39:59 +0100]: > > > > On Thu, Feb 21, 2019 at 10:20:20AM -0600, Benoit Parrot wrote: > > > > > Hi Maxime, > > > > > > > > > > A couple of questions, > > > > > > > > > > Maxime Ripard <maxime.ripard@bootlin.com> wrote on Thu [2018-Oct-11 04:21:00 -0500]: > > > > > > The clock rate, while hardcoded until now, is actually a function of the > > > > > > resolution, framerate and bytes per pixel. Now that we have an algorithm to > > > > > > adjust our clock rate, we can select it dynamically when we change the > > > > > > mode. > > > > > > > > > > > > This changes a bit the clock rate being used, with the following effect: > > > > > > > > > > > > +------+------+------+------+-----+-----------------+----------------+-----------+ > > > > > > | Hact | Vact | Htot | Vtot | FPS | Hardcoded clock | Computed clock | Deviation | > > > > > > +------+------+------+------+-----+-----------------+----------------+-----------+ > > > > > > | 640 | 480 | 1896 | 1080 | 15 | 56000000 | 61430400 | 8.84 % | > > > > > > | 640 | 480 | 1896 | 1080 | 30 | 112000000 | 122860800 | 8.84 % | > > > > > > | 1024 | 768 | 1896 | 1080 | 15 | 56000000 | 61430400 | 8.84 % | > > > > > > | 1024 | 768 | 1896 | 1080 | 30 | 112000000 | 122860800 | 8.84 % | > > > > > > | 320 | 240 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > > > > > | 320 | 240 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > > > > > | 176 | 144 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > > > > > | 176 | 144 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > > > > > | 720 | 480 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > > > > > | 720 | 480 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > > > > > | 720 | 576 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | > > > > > > | 720 | 576 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | > > > > > > | 1280 | 720 | 1892 | 740 | 15 | 42000000 | 42002400 | 0.01 % | > > > > > > | 1280 | 720 | 1892 | 740 | 30 | 84000000 | 84004800 | 0.01 % | > > > > > > | 1920 | 1080 | 2500 | 1120 | 15 | 84000000 | 84000000 | 0.00 % | > > > > > > | 1920 | 1080 | 2500 | 1120 | 30 | 168000000 | 168000000 | 0.00 % | > > > > > > | 2592 | 1944 | 2844 | 1944 | 15 | 84000000 | 165862080 | 49.36 % | > > > > > > +------+------+------+------+-----+-----------------+----------------+-----------+ > > > > > > > > > > Is the computed clock above the same for both parallel and CSI2? > > > > > > > > > > I want to add controls for PIXEL_RATE and LINK_FREQ, would you have any > > > > > quick pointer on taking the computed clock and translating that into the > > > > > PIXEL_RATE and LINK_FREQ values? > > > > > > > > > > I am trying to use this sensor with TI CAL driver which at the moment uses > > > > > the PIXEL_RATE values in order to compute ths_settle and ths_term values > > > > > needed to program the DPHY properly. This is similar in behavior as the way > > > > > omap3isp relies on this info as well. > > > > > > > > I haven't looked that much into the csi-2 case, but the pixel rate > > > > should be the same at least. > > > > > > I'll have to study the way the computed clock is actually calculated for > > > either case, but if they yield the same number then I would be surprised > > > that the pixel rate would be the same as in parallel mode you get 8 data > > > bits per clock whereas in CSI2 using 2 data lanes you get 4 data bits per > > > clock. > > > > The bus rate will be different, but the pixel rate is the same: you > > have as many pixels per frames and as many frames per seconds in the > > parallel and CSI cases. > > > > I agree with that, but.. > > > > So just to be certain here the "Computed clock" column above would be the > > > pixel clock frequency? > > > > it is > > ...it seems to me the Computed clock column is actually the "byte clock". > > From a simple calculation for the 640x480@15FPS case: > "Computed clock" = 1896 * 1080 * 15 * 2 = 61430400 > > While, in my understanding, the pixel clock would just be > pixel_clock = HTOT * VTOT * FPS = 1896 * 1080 * 15 = 30715200 > > So I suspect the "* 2" there is the number of bytes per pixel. > > That would match what's also reported here > file:///home/jmondi/project/renesas/linux/linux-build/Documentation/output/media/kapi/csi2.html?highlight=link_freq > > Where: > link_freq = (pixel_rate * bpp) / (2 * nr_lanes) > > So if I were to calculate PIXEL_RATE and LINK_FREQ in this driver, > that would be: > PIXEL_RATE = mode->vtot * mode->htot * ov5640_framerates[sensor->current_fr]; > LINK_FREQ = PIXEL_RATE * 16 / ( 2 * sensor->ep.bus.mipi_csi2.num_data_lanes); > (assuming, as the driver does now, all formats have 16bpp) > > Does this match your understanding as well? You're totally right, sorry about that :) Maxime
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index 5114d401b8eb..34eaa9dd5237 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -1915,7 +1915,8 @@ static int ov5640_set_mode(struct ov5640_dev *sensor) * which is 8 bits per pixel. */ bpp = sensor->fmt.code == MEDIA_BUS_FMT_JPEG_1X8 ? 8 : 16; - rate = mode->pixel_clock * bpp; + rate = mode->vtot * mode->htot * bpp; + rate *= ov5640_framerates[sensor->current_fr]; if (sensor->ep.bus_type == V4L2_MBUS_CSI2) { rate = rate / sensor->ep.bus.mipi_csi2.num_data_lanes; ret = ov5640_set_mipi_pclk(sensor, rate);
The clock rate, while hardcoded until now, is actually a function of the resolution, framerate and bytes per pixel. Now that we have an algorithm to adjust our clock rate, we can select it dynamically when we change the mode. This changes a bit the clock rate being used, with the following effect: +------+------+------+------+-----+-----------------+----------------+-----------+ | Hact | Vact | Htot | Vtot | FPS | Hardcoded clock | Computed clock | Deviation | +------+------+------+------+-----+-----------------+----------------+-----------+ | 640 | 480 | 1896 | 1080 | 15 | 56000000 | 61430400 | 8.84 % | | 640 | 480 | 1896 | 1080 | 30 | 112000000 | 122860800 | 8.84 % | | 1024 | 768 | 1896 | 1080 | 15 | 56000000 | 61430400 | 8.84 % | | 1024 | 768 | 1896 | 1080 | 30 | 112000000 | 122860800 | 8.84 % | | 320 | 240 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | | 320 | 240 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | | 176 | 144 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | | 176 | 144 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | | 720 | 480 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | | 720 | 480 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | | 720 | 576 | 1896 | 984 | 15 | 56000000 | 55969920 | 0.05 % | | 720 | 576 | 1896 | 984 | 30 | 112000000 | 111939840 | 0.05 % | | 1280 | 720 | 1892 | 740 | 15 | 42000000 | 42002400 | 0.01 % | | 1280 | 720 | 1892 | 740 | 30 | 84000000 | 84004800 | 0.01 % | | 1920 | 1080 | 2500 | 1120 | 15 | 84000000 | 84000000 | 0.00 % | | 1920 | 1080 | 2500 | 1120 | 30 | 168000000 | 168000000 | 0.00 % | | 2592 | 1944 | 2844 | 1944 | 15 | 84000000 | 165862080 | 49.36 % | +------+------+------+------+-----+-----------------+----------------+-----------+ Only the 640x480, 1024x768 and 2592x1944 modes are significantly affected by the new formula. In this case, 640x480 and 1024x768 are actually fixed by this change. Indeed, the sensor was sending data at, for example, 27.33fps instead of 30fps. This is -9%, which is roughly what we're seeing in the array. Testing these modes with the new clock setup actually fix that error, and data are now sent at around 30fps. 2592x1944, on the other hand, is probably due to the fact that this mode can only be used using MIPI-CSI2, in a two lane mode, and never really tested with a DVP bus. Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> --- drivers/media/i2c/ov5640.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)