From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mta0.migadu.com (out-83.mta0.migadu.com [91.218.175.83]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 430824963A6 for ; Mon, 28 Sep 2026 11:24:34 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=91.218.175.83 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790594678; cv=none; b=hiod4AHU16etE7zh1dZS3HmqK/losqp2f2npzGN7vgVPHbpyNnB5BgYnrcXMySJPex1cvbvPhKmx3PAU4IDJa43JScAxTykLEQpRJl82gqpzrRTomZ5dWdZvoi5DQ3+20EGzu2I2DfwmyHP/slFO0bN2QwK0gd6QordD/b+8TMQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790594678; c=relaxed/simple; bh=WnGWhNvaH7q5bzvu7KMH/yNAVi0tmobq4UUzPbUO7yM=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=bVPEwx34r+cZpoS3r9fUrUBc9RzIlJPEpZAgMLT3CBLBwulbgmtTbkJKzoS7MtrBVs0Q2AJyVMjTvQ54+BonPVoIOOjdRrm1RPZbXenOsvkH7JB8VSoseuukhI+Aw6ldWCT4olnVNIb18OsTlN1WJXfFYzzGpE4oBTN629gS/Rk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=DXG5HE5o; arc=none smtp.client-ip=91.218.175.83 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="DXG5HE5o" X-Envelope-To: linux-kernel@vger.kernel.org DKIM-Signature: a=rsa-sha256; bh=WnGWhNvaH7q5bzvu7KMH/yNAVi0tmobq4UUzPbUO7yM=; c=simple/simple; d=linux.dev; h=from:to:subject:date:message-id:mime-version:content-type; s=key1; t=1790594673; v=1; x=1791199473; b=DXG5HE5oM+IT1wOrce2Q1snF7LIbuokVOv0khbcmrrkvdYUvaUhlw3MYr1jXxgRCHTeGX0G/ N/43ysFgZz64B2wYE8a71x2E7wA5/dP8y/412yrq60BFAO5VvJdIbgf50eP8EjOR+qezbveajLt VgzPMUW4Dw9sf0N79XdUwYEs= X-Envelope-To: linux-kernel@vger.kernel.org Received: by smtp.migadu.com with ESMTPS id 93c82698bd379aea; Mon, 28 Sep 2026 11:24:32 +0000 X-Mizu-Trace-ID: 93c82698bd379aea X-Migadu-Flow: FLOW_OUT Date: Mon, 28 Sep 2026 13:24:24 +0200 From: Richard Leitner To: Dave Stevenson Cc: Sakari Ailus , Mauro Carvalho Chehab , Martina Krasteva , "Paul J. Murphy" , Daniele Alessandrelli , Hans Verkuil , Mauro Carvalho Chehab , Gyula Kelemen , linux-media@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 02/10] media: i2c: ov9282: fix line time and exposure time calculation Message-ID: References: <20260914-ov9282-fixes-v1-0-f520af59df1b@linux.dev> <20260914-ov9282-fixes-v1-2-f520af59df1b@linux.dev> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: On Mon, Sep 28, 2026 at 12:02:18PM +0100, Dave Stevenson wrote: > Hi Richard > > On Mon, 28 Sept 2026 at 09:56, Richard Leitner > wrote: > > > > Hi Dave, > > > > thanks for your feedback! > > > > On Wed, Sep 23, 2026 at 03:16:27PM +0100, Dave Stevenson wrote: > > > Hi Richard > > > > > > On Mon, 14 Sept 2026 at 20:21, Richard Leitner > > > wrote: > > > > > > > > ov9282_exposure_to_us() divided the line length by the pixel rate control, > > > > which is the MIPI rate and not the clock HTS is counted in. > > > > > > It really shouldn't be. PIXEL_RATE is the pixel array and LINK_FREQ is > > > the MIPI rate. Many drivers do conflate the two. > > > > > > However I did notice a couple of weeks back that pixel_rate is > > > incorrect for 8bit readout on ov9282 [1]. (It's correct for 10bit > > > readout). > > > > > > Actually I see the problem. In the hardware the pixel rate for 8 bit > > > is scaled by the change of PLL2 multiplier from 0x50 to 0x60 = x1.2. > > > The pixel rate control is scaled from /10 to /8 or x1.25. 1.2/1.25 = > > > 0.96, so my 96% of speed would be spot on. The use of link frequency > > > in computing pixel rate is totally bogus as they are on independent > > > PLLs > > > > So I guess I should also get rid of the pixel rate pre-processor > > calculation which depends on the link frequency? > > > > Is it OK with you to just hard-code them? > > > > #define OV9282_PIXEL_RATE_10BIT 160000000 > > #define OV9282_PIXEL_RATE_8BIT 192000000 > > > > AFAICT it should be possible to calculate the PLL1 pix clk from the input > > clock (inclk or XVCLK in the datasheet) provided in the dts. But tbh I > > would prefer to not add this to this series. If it's desired and possible > > from your point of view, I can tackle this in a future series. > > I'm happy for them to be hard coded, possibly with a comment that the > pixel rate is derived from the PLL2 clock tree. For 10bit readout, the > whole PLL config is exactly the same as the sample PLL configuration > given in the datasheet (table 2-10 for me with datasheet v1.53). 8bit > readout changes the PLL2 multiplier. > > The registers initially looked pretty close to the CCS standard and so > possible to use the ccs-pll helpers, except the dividers support /1.5 > and /2.5 in places so I don't think it can be used as-is (PLL1 prediv > is set to /1.5 in the current setup). > > Whilst feasible, if no one has had a use case needing an alternate > input clock rate in 5 years since the ov9282 driver was merged (Aug > 2021), then there's little point in jumping through hoops to > accommodate a theoretical future user who may never surface. I'd save > yourself the effort. That's of course fine with me ;-) Thanks for the quick response. Then I'll add a comment and will likely send v2 later today. regards;rl > > > > > > > If you correct OV9282_PIXEL_RATE_8BIT to being 192000000 (instead of > > > the current 200000000), does that solve your problem without > > > recomputing things? > > > > Thanks for the explanation above. I guess you're right, the link freq/mipi > > rate should not be in there. I've just ran a quick test and it should be > > fine with that change. > > Great. If just correcting the pixel rate solves the problem, then it's > better not to complicate the calculations in other places > unnecessarily. > > > As i would like to keep the ov9282_line_time_ns() function, what's your > > take on the following implementation? Would that be OK for a v2? > > > > static u32 ov9282_line_time_ns(struct ov9282 *ov9282) > > { > > u64 hts = ov9282->cur_mode->width + ov9282->hblank_ctrl->val; > > return div_u64(hts * NSEC_PER_SEC, ov9282->pixel_rate->val); > > } > > I have no issue with having a helper for the flash calculations. > > Dave > > > thanks! > > > > regards;rl > > > > > > > > Dave > > > > > > [1] https://lore.kernel.org/linux-media/CAPY8ntBaSHjztuSLTOtv9KYvEbmcbinSMJ78ZDg+0sZg-1fz3Q@mail.gmail.com/ > > > The original reporter did come back to me and acknowledge he was using > > > the downstream Rockchip driver which is doing the wrong thing. > > > > > > > The right > > > > clock is PLL2's system clock. With the PLL2 dividers left at their reset > > > > values the chain > > > > > > > > SYS_CLK = XVCLK / pre_div0 / pre_div * loop_div / sys_pre_div / sys_div > > > > = 24 / 1 / 3 * loop_div / 4 / 2 > > > > > > > > collapses to SYS_CLK = loop_div MHz. > > > > > > > > Fix this by introducing a new static function to calculate the current > > > > line time and use it in ov9282_exposure_to_us(). > > > > > > > > Signed-off-by: Richard Leitner > > > > --- > > > > drivers/media/i2c/ov9282.c | 46 +++++++++++++++++++++++++++++++++++++--------- > > > > 1 file changed, 37 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c > > > > index c10b2e205834e..3f83a6cf338d8 100644 > > > > --- a/drivers/media/i2c/ov9282.c > > > > +++ b/drivers/media/i2c/ov9282.c > > > > @@ -10,10 +10,12 @@ > > > > #include > > > > #include > > > > #include > > > > +#include > > > > #include > > > > #include > > > > #include > > > > #include > > > > +#include > > > > > > > > #include > > > > #include > > > > @@ -472,6 +474,41 @@ static inline struct ov9282 *to_ov9282(struct v4l2_subdev *subdev) > > > > return container_of(subdev, struct ov9282, sd); > > > > } > > > > > > > > +/** > > > > + * ov9282_line_time_ns() - Calculate duration of one sensor line. > > > > + * @ov9282: pointer to ov9282 device > > > > + * > > > > + * The line time and therefore OV9282_REG_TIMING_HTS and the strobe frame span > > > > + * are counted in PLL2's system clock. We assume the PLL2 dividers are at their > > > > + * reset values, so the formula reduces to SYS_CLK = loop_div MHz. > > > > + * > > > > + * Return: line time in nanoseconds. > > > > + */ > > > > +static u32 ov9282_line_time_ns(struct ov9282 *ov9282) > > > > +{ > > > > + u32 hts = ov9282->cur_mode->width + ov9282->hblank_ctrl->val; > > > > + u32 sclk_rate_mhz = ov9282->code == MEDIA_BUS_FMT_Y10_1X10 ? > > > > + OV9282_PLL_CTRL_0D_RAW10 : OV9282_PLL_CTRL_0D_RAW8; > > > > + > > > > + /* > > > > + * OV9282_REG_TIMING_HTS counts 2-pixel units > > > > + */ > > > > + return DIV_ROUND_CLOSEST(hts * (u32)NSEC_PER_USEC, 2 * sclk_rate_mhz); > > > > +} > > > > + > > > > +/** > > > > + * ov9282_exposure_to_us() - Convert an exposure register value to microseconds > > > > + * @ov9282: pointer to ov9282 device > > > > + * @exposure: exposure register value to convert > > > > + * > > > > + * Return: microsecond represenation of the given exposure register value. > > > > + */ > > > > +static u32 ov9282_exposure_to_us(struct ov9282 *ov9282, u32 exposure) > > > > +{ > > > > + return div_u64((u64)exposure * ov9282_line_time_ns(ov9282), > > > > + NSEC_PER_USEC); > > > > +} > > > > + > > > > /** > > > > * ov9282_update_controls() - Update control ranges based on streaming mode > > > > * @ov9282: pointer to ov9282 device > > > > @@ -510,15 +547,6 @@ static int ov9282_update_controls(struct ov9282 *ov9282, > > > > mode->vblank_max, 1, mode->vblank); > > > > } > > > > > > > > -static u32 ov9282_exposure_to_us(struct ov9282 *ov9282, u32 exposure) > > > > -{ > > > > - /* calculate exposure time in µs */ > > > > - u32 frame_width = ov9282->cur_mode->width + ov9282->hblank_ctrl->val; > > > > - u32 trow_us = frame_width / (ov9282->pixel_rate->val / 1000000UL); > > > > - > > > > - return exposure * trow_us; > > > > -} > > > > - > > > > /** > > > > * ov9282_update_exp_gain() - Set updated exposure and gain > > > > * @ov9282: pointer to ov9282 device > > > > > > > > -- > > > > 2.53.0 > > > > > > > >