From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lj1-f181.google.com (mail-lj1-f181.google.com [209.85.208.181]) (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 926DC3D16E4 for ; Fri, 12 Jun 2026 13:16:07 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.208.181 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781270169; cv=none; b=NG48oPpxBrcrP7gVAKIg+H9/EVmL651GUOLcMkmu6SSOJ/r2N5snZkm8M/E6qP77DE8F+1bM0n9pVfis1kbcuc8mF374kIbsUy/CLo/KoJCqwnP44Mo9ro82sM3mOK6G/d1ab1wGq5EaiJGEpOIa+zcMgVh7lLQAmigMWTfMNco= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781270169; c=relaxed/simple; bh=2RbO04fUi3IJaYFtf7jGrbpwZ5R1BgTfIRB2dy5lXsw=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=cVSKb7gB2O0EAhXZh1XPKZJQ8l5259JjY3d7hj8BN+0ci7cdwmjOH0s+a38HOiI3zgC+7A2G6O2HH+4hIQwwQl67c8JckAlzl5UX+otq7THqISli26g8WRs1D0T5JH001LW59/zASS1sN6wU6vvQ2oL/VdxDDIWtNX6t0lRcRoA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=A9mai+OX; arc=none smtp.client-ip=209.85.208.181 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="A9mai+OX" Received: by mail-lj1-f181.google.com with SMTP id 38308e7fff4ca-3991fff17f6so397001fa.3 for ; Fri, 12 Jun 2026 06:16:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1781270166; x=1781874966; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:references:cc:to:subject :user-agent:mime-version:date:message-id:from:to:cc:subject:date :message-id:reply-to; bh=yYJLgdM+gn9R331SmxgguPUu0Ms/YAMbM6ICKsm3dW0=; b=A9mai+OXv3+Wd2fGIt5P1xKq07BegcQr/L/ylU0SPdc5MA/ze/X4TpVjVP3L2khfr6 wSmHGhGU9gp57t4HPdIt1FrzuT/wNEXl4fcvoJptzxUJ9qsBgSM3m5pDqZWfX2ou92Xj ymRB9iuuwWxahRHknDf61aCCbCFBHf72WlKmsU842dPy/0Htj2ikNUIXK9CvTsDKY22U IjKUYG2FDPtekcsx4Aq5ozLRAZeg8x5ncKR+UOU0H22CQy10Eo4rKmDQl8RYsriJ0tfa /GQauicj0Ahlh5rvpYcUul+3RehVLdePAeU8nfsGzjMS8XBxcpvSo75rXR9V5mnCvbNk 2Swg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1781270166; x=1781874966; h=content-transfer-encoding:in-reply-to:from:references:cc:to:subject :user-agent:mime-version:date:message-id:x-gm-gg:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=yYJLgdM+gn9R331SmxgguPUu0Ms/YAMbM6ICKsm3dW0=; b=A1MjK5zuaanJKsDqJjMKarTpnm1ngwo2jxvDpvk4SFZJ6R2/OZBXIuqEd41RlWNUWg Q8d12yuRlBzgrEpA3kRwDsRM4X5IF9SV4dIL7Atb+wocSs0L9M9l7ju1E9VRzFmVU1qG 1vuJQAkP0trTDGSE6bZrhnoaclElOno97IVvZtCmGJxSoj8BIFVK3x7rc3B0hRNofuED h/xaTqvJpEPsuhgY973tFyS4G8/b31WA7pN/p6Ckxwfreyrs21MpJKfQS7S0ECDmIf36 CS1nGP2EFBmhtPiOP+xSiz2w+UbiIsD41Fkgf590apyqlVnBIAFdJxhY2J44ub1WVK0P PxxQ== X-Forwarded-Encrypted: i=1; AFNElJ977fpzB8QxI9530TOXA4EQ8YcE9LOrmMPowU40UGq6nXIKGzQcY2b+ihWJvvjkH6fcMwiv6r/V+ox+11o=@vger.kernel.org X-Gm-Message-State: AOJu0Yxk++pGJyJ+KHla+X2tBrCIYHJW3uJkUYeyUApM89wH5qdz4vGL RcboQRlrHfYPyMSMsZZyNPYXCaUNqzh5otfmCov4qs3P1XRXgNhX9IOhGLJacC9yKj8= X-Gm-Gg: Acq92OGgEA7C9CVbidKrcWrq3XcOQScZgTsL7XwzMTZTfZ3OOVWlk+MX4At5uyh/dEP AwXblFMQ4RJ3uKfTuxAtpieRtmuBEkDLCSdO0SVT+y7nYCaIQLENNCsK880Quo6lihmsak9fu32 CPQmcZ2LXnpJsJAtjBCjrv44qudfXkFUHQI+3ayaS5A5hFXADM3e2Lwq1jY3COcg0oXuTg/rlcu UAJS9wt2DmWmpCGPeKMD622BWUR+Bv/GRREGZiNFGVvfAHZA1Uc31EQj7dU7zGadp4TLSAMe6Kg 42qrQNXu1ITwXghk7ABShY+9BtidpWM3BJVdbEcZ3ZH1PpZo7INt64eRn0gYf2iFb3kuMm7dKAX 5kTR2/42WOUItYfEXDm+x4MbFIsS0ORWW66L6ss77VVy06NWIytnNkPs6WSQPC0Cz8ZdgKiCFyA jOBzm3Ob6bFBDRtvD2mIgG5PM+h+l8m/Yp06QWH7u5X3SzvPVDs/mN+sv2oGlNDUuI5tlq7lXL1 H22Cg== X-Received: by 2002:a2e:b88b:0:b0:394:152d:f9c6 with SMTP id 38308e7fff4ca-3992b28f883mr3404631fa.8.1781270165550; Fri, 12 Jun 2026 06:16:05 -0700 (PDT) Received: from [192.168.1.100] (91-159-24-186.elisa-laajakaista.fi. [91.159.24.186]) by smtp.gmail.com with ESMTPSA id 38308e7fff4ca-39929c6c9f7sm5791011fa.18.2026.06.12.06.16.02 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 12 Jun 2026 06:16:03 -0700 (PDT) Message-ID: <6f664514-34bf-44db-8df1-9b1a774e6367@linaro.org> Date: Fri, 12 Jun 2026 16:16:02 +0300 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 3/3] media: i2c: os02g10: implement crop handling with set_selection To: Tarang Raval Cc: Elgin Perumbilly , "laurent.pinchart@ideasonboard.com" , "sakari.ailus@linux.intel.com" , Mauro Carvalho Chehab , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Hans Verkuil , Hans de Goede , Mehdi Djait , Sylvain Petinot , Benjamin Mugnier , Bryan O'Donoghue , Heimir Thor Sverrisson , Hardevsinh Palaniya , "linux-media@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" References: <20260424092554.26130-1-elgin.perumbilly@siliconsignals.io> <20260424092554.26130-4-elgin.perumbilly@siliconsignals.io> <0f4bfe08-0504-48ff-83ce-c84600e6f38d@linaro.org> From: Vladimir Zapolskiy In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 6/12/26 14:41, Tarang Raval wrote: > Hi Vladimir, > > Thank you for the review. > > Since I authored this patch, I will try to address the comments below. > >> On 4/24/26 12:25, Elgin Perumbilly wrote: >>> From: Tarang Raval >>> >>> Add crop support to os02g10 by implementing .set_selection() and >>> storing the crop rectangle in subdev state. >>> >>> Initialize the default crop to the active area, make set_fmt() use the >>> current crop, and update the output format when the crop size changes. >>> Also program the sensor window from the active crop/format state instead >>> of using the fixed supported_modes entry. >>> >>> This allows userspace to configure the sensor crop window explicitly. >>> >>> Signed-off-by: Tarang Raval >>> Signed-off-by: Elgin Perumbilly >>> --- >>> drivers/media/i2c/os02g10.c | 166 ++++++++++++++++++++++-------------- >>> 1 file changed, 103 insertions(+), 63 deletions(-) >>> >>> diff --git a/drivers/media/i2c/os02g10.c b/drivers/media/i2c/os02g10.c >>> index fad2dd0ad7aa..9bf8f5d1caea 100644 >>> --- a/drivers/media/i2c/os02g10.c >>> +++ b/drivers/media/i2c/os02g10.c >>> @@ -112,6 +112,11 @@ >>> #define OS02G10_ORIENTATION_BAYER_FIX 0x32 >>> >>> #define OS02G10_LINK_FREQ_720MHZ (720 * HZ_PER_MHZ) >>> +#define OS02G10_WINDOW_WIDTH_MIN 2 >>> +#define OS02G10_WINDOW_HEIGHT_MIN 2 >> >> Add a blank line before the new group of macro. >> >>> +#define OS02G10_VBLANK_DEF 166 >> >> This one is computable, and it can be dropped. > > > Can you explain how this value can be computed ? It is (supported_modes[0].vts_def - supported_modes[0].height) on the base of 2/3 change. > >>> +#define OS02G10_VBLANK_MIN 25 >> >> This macro shall be added to the group of OS02G10_REG_FRAME_LENGTH >> register, and it should be included into the previous change. >> >>> +#define OS02G10_EXPOSURE_DEF 1100 >> >> This macro shall be added to the group of OS02G10_REG_LONG_EXPOSURE >> register, and it should be included into the previous change. > > > I would prefer to introduce these macros here only, as they are related to > this patch. There is no use of these macros in the previous patch, so moving > them there would not provide any benefit. Unavoidably there shall be a user of "default exposure" in the change 2/3, and therefore the value is present, please reference to supported_modes[0].exp_def. So, this macro goes to the 2/3 patch. > > I will group them in the appropriate place within this patch as per your > suggestion. > > >>> >>> /* OS02G10 native and active pixel array size */ >>> static const struct v4l2_rect os02g10_native_area = { >>> @@ -152,15 +157,6 @@ struct os02g10 { >>> struct v4l2_ctrl *hflip; >>> }; >>> >>> -struct os02g10_mode { >>> - u32 width; >>> - u32 height; >>> - u32 vts_def; >>> - u32 exp_def; >>> - u32 x_start; >>> - u32 y_start; >>> -}; >>> - >>> static const struct cci_reg_sequence os02g10_common_regs[] = { >>> { OS02G10_REG_PLL_DIV_CTRL, 0x0a}, >>> { OS02G10_REG_PLL_DCTL_BIAS_CTRL, 0x04}, > > ... > >>> static const struct v4l2_subdev_video_ops os02g10_video_ops = { >>> @@ -645,6 +684,7 @@ static const struct v4l2_subdev_pad_ops os02g10_pad_ops = { >>> .get_fmt = v4l2_subdev_get_fmt, >>> .set_fmt = os02g10_set_pad_format, >>> .get_selection = os02g10_get_selection, >>> + .set_selection = os02g10_set_selection, >>> .enum_frame_size = os02g10_enum_frame_size, >>> .enable_streams = os02g10_enable_streams, >>> .disable_streams = os02g10_disable_streams, >> >> I understand that this change is written by another person, and likely >> it is not squashed with the previous one to preserve authorship, however >> it significantly rewrites the change already found in the series. > > I don't think this patch significantly rewrites the previous changes. Its > main purpose is to introduce a crop rectangle and implement set_selection(), > allowing userspace to stream arbitrary resolutions within the sensor limits > instead of being restricted to 1920x1080. What are these "sensor limits" number? Are they defined in this 3/3 change? For whatever reason I can't find it, but I may be blind. > >> I don't see information about the maximum supported frame height/width >> or default VTS setting etc. anymore, for me it's hard to say, if >> this kind of information can be dropped with no consequences in runtime. > > The maximum supported width and height are still defined and enforced in > os02g10_set_selection(). The default VTS handling is implemented in > os02g10_set_pad_format(), where VBLANK is adjusted to maintain 30 fps for > the selected resolution. So I don't believe any information has been dropped. > >> Probably this 3/3 change will break a quick inclusion of the sensor >> driver, you may consider to exlcude it from the series now, and publish >> it afterwards. > > Regarding whether patch 3/3 should be included in the current series, I am > happy to leave that decision to Sakari. If needed, this patch can be merged > separately once the new raw sensor model is finalized. > > This patch was created based on Laurent's suggestion. > > Link: https://lore.kernel.org/linux-media/20260414084952.217215-1-elgin.perumbilly@siliconsignals.io/T/#t > That's a helpful reference to the previous patch review, thank you. Likely I don't have the whole picture of the new advances in linux-media. -- Best wishes, Vladimir