From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 5F8A53DF000 for ; Thu, 16 Apr 2026 14:22:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=213.167.242.64 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1776349343; cv=none; b=SIls6coIwJjW1w/A0xLL2lJ5UNRKzmFswvg7BWh8pQNRykcZX9SM178VyR4htxBalaypRGH8ZhZVkxyeiTHl/nHK9bL3cBhuk/NCL8gNhfZxMNqdmufUuDaGVz7Taavto/0y/U0tAE2o46dx2tO9lVhG/m670tzvSp8ITv8680g= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1776349343; c=relaxed/simple; bh=OHTujtMYuv1Q5fTFjfBaCoLi/MEwRqst6ee3dREnrQA=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=thq1tg9+nO7jFz2QPqQO6SNQyqNF4wHtNhx4cIS/9f0RlOmZgmffQ2w46qMo4Whf3NtHJvFwnT+LyQT3qKzp2EZIMAd9k58TerC7nwMlRFDDO1RfP8AR1ukCWRiZIfKHEsq0/Y8vfwdQBUEy8g1xtF7ZQ5UEdyr9KHQG1ZAZSO0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=ideasonboard.com; spf=pass smtp.mailfrom=ideasonboard.com; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b=plw1pfwb; arc=none smtp.client-ip=213.167.242.64 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="plw1pfwb" Received: from killaraus.ideasonboard.com (2001-14ba-703d-e500--2a1.rev.dnainternet.fi [IPv6:2001:14ba:703d:e500::2a1]) by perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 368BE161; Thu, 16 Apr 2026 16:20:43 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1776349243; bh=OHTujtMYuv1Q5fTFjfBaCoLi/MEwRqst6ee3dREnrQA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=plw1pfwbqjIEbvS6rUO11m4jvHP2hfs8uzR065wrRzfh1YD0LXXsKlJGDiwsjx8jP 7Ex2NqC3oiRXfojjXEiBbIMriIqL/l+c3geOocV3zmCi6qduWq8FkZH4WqfhkhqA6F ZVa0AsHc/DjDDuGz3Zt1s/LztdmAe8iabl+zBnBk= Date: Thu, 16 Apr 2026 17:22:15 +0300 From: Laurent Pinchart To: Jacopo Mondi Cc: Sakari Ailus , linux-media@vger.kernel.org, hans@jjverkuil.nl, Prabhakar , Kate Hsuan , Dave Stevenson , Tommaso Merciai , Benjamin Mugnier , Sylvain Petinot , Christophe JAILLET , Julien Massot , Naushir Patuck , "Yan, Dongcheng" , "Cao, Bingbu" , "Qiu, Tian Shu" , Stefan Klug , Mirela Rabulea , =?utf-8?B?QW5kcsOp?= Apitzsch , Heimir Thor Sverrisson , Kieran Bingham , Mehdi Djait , Ricardo Ribalda Delgado , Hans de Goede , Tomi Valkeinen , David Plowman , "Yu, Ong Hock" , "Ng, Khai Wen" , Jai Luthra , Rishikesh Donadkar Subject: Re: [PATCH v4 03/29] media: imx219: Set horizontal blanking on mode change Message-ID: <20260416142215.GD1775831@killaraus.ideasonboard.com> References: <20260408153939.969381-1-sakari.ailus@linux.intel.com> <20260408153939.969381-4-sakari.ailus@linux.intel.com> Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: On Fri, Apr 10, 2026 at 09:27:34AM +0200, Jacopo Mondi wrote: > On Wed, Apr 08, 2026 at 06:39:12PM +0300, Sakari Ailus wrote: > > The driver UAPI is mode-based, allowing the user to choose a mode from a > > small list based on the output size. The vertical blanking is set based on > > the mode, do the same for horizontal blanking so the frame rate obtained > > is constant. > > > > Additionally, it's best to use a known-good horizontal blanking value as > > choosing the value freely may affect image quality. While the minimum > > value may not be the best value for horizontal blanking, at least it is > > constant rather than a minimum value of a different configuration. > > As Dave suggested, we should probably better define the desired behaviour. > > As far as I can see the driver doesn't specify a line lenght in the > supported_modes array, and I guess we're always running with the min > valid blanking. From a libcamera perspective only RPi changes the > HBLANK control value, all other pipelines use the default, so if > Dave's fine with this, I'm fine as well. Interactions between formats and controls are notoriously badly specified, so I'm all for improving that. That being said, this patch simplifies the behaviour of the driver and leads to more predictable results, so, until we have a formal spec, Reviewed-by: Laurent Pinchart > > Signed-off-by: Sakari Ailus > > Reviewed-by: Dave Stevenson > > Reviewed-by: Jacopo Mondi > > > --- > > drivers/media/i2c/imx219.c | 15 +++------------ > > 1 file changed, 3 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > > index 89061dc1842d..62a23541b1dc 100644 > > --- a/drivers/media/i2c/imx219.c > > +++ b/drivers/media/i2c/imx219.c > > @@ -837,11 +837,9 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > > struct v4l2_mbus_framefmt *format; > > struct v4l2_rect *crop; > > u8 bin_h, bin_v, binning; > > - u32 prev_line_len; > > int ret; > > > > format = v4l2_subdev_state_get_format(state, 0); > > - prev_line_len = format->width + imx219->hblank->val; > > > > /* > > * Adjust the requested format to match the closest mode. The Bayer > > @@ -882,7 +880,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > > if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) { > > int exposure_max; > > int exposure_def; > > - int hblank, llp_min; > > + int llp_min; > > int pixel_rate; > > > > /* Update limits and set FPS to default */ > > @@ -924,15 +922,8 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > > llp_min - mode->width); > > if (ret) > > return ret; > > - /* > > - * Retain PPL setting from previous mode so that the > > - * line time does not change on a mode change. > > - * Limits have to be recomputed as the controls define > > - * the blanking only, so PPL values need to have the > > - * mode width subtracted. > > - */ > > - hblank = prev_line_len - mode->width; > > - ret = __v4l2_ctrl_s_ctrl(imx219->hblank, hblank); > > + > > + ret = __v4l2_ctrl_s_ctrl(imx219->hblank, llp_min - mode->width); > > if (ret) > > return ret; > > -- Regards, Laurent Pinchart