From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Jacopo Mondi <jacopo@jmondi.org>
Cc: Steve Longerbeam <slongerbeam@gmail.com>,
sakari.ailus@iki.fi, hverkuil-cisco@xs4all.nl,
mirela.rabulea@nxp.com, xavier.roumegue@oss.nxp.com,
tomi.valkeinen@ideasonboard.com, hugues.fruchet@st.com,
prabhakar.mahadev-lad.rj@bp.renesas.com, aford173@gmail.com,
festevam@gmail.com, Eugen.Hristev@microchip.com,
jbrunet@baylibre.com, Mauro Carvalho Chehab <mchehab@kernel.org>,
linux-media@vger.kernel.org
Subject: Re: [v2.1] media: ov5640: Rework analog crop rectangles
Date: Sun, 20 Feb 2022 14:56:33 +0200 [thread overview]
Message-ID: <YhI6gZUiIbDkiHlf@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20220211093432.41977-1-jacopo@jmondi.org>
Hi Jacopo,
Thank you for the patch.
On Fri, Feb 11, 2022 at 10:34:32AM +0100, Jacopo Mondi wrote:
> The OV5640 pixel array is composed as:
> - vertically: 16 dummy columns, 2592 valid ones and 16 dummy columns for
> a total of 2624 columns
> - horizontally: 8 optical black lines, 6 dummy ones, 1944 valid and 6
> dummies for a total of 1964 lines
>
> Adjust the analog crop rectangle in all modes to:
> - Skip the first 16 dummy columns
> - Skip the first 14 black/dummy lines
> - Pass the whole valid pixel array size to the ISP for all modes except
> 1024x768, 720p and 1080p which are obtained by cropping the valid pixel array.
>
> Adjust how timings is programmed to comply with the new definitions.
>
> Tested in RGB565, UYVY, RGB565 and RGB888 modes.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> drivers/media/i2c/ov5640.c | 120 +++++++++++++++++++++----------------
> 1 file changed, 68 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index f817f865ad16..232afd4d906a 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -626,14 +626,14 @@ static const struct ov5640_mode_info ov5640_mode_init_data = {
> .dn_mode = SUBSAMPLING,
> .pixel_rate = OV5640_PIXEL_RATE_96M,
> .analog_crop = {
> - .left = 0,
> - .top = 4,
> - .width = 2623,
> - .height = 1947,
> + .left = OV5640_PIXEL_ARRAY_LEFT,
> + .top = OV5640_PIXEL_ARRAY_TOP,
> + .width = OV5640_PIXEL_ARRAY_WIDTH,
> + .height = OV5640_PIXEL_ARRAY_HEIGHT,
These macros are added in 17/23.
> },
> .crop = {
> - .left = 16,
> - .top = 6,
> + .left = 2,
> + .top = 4,
> .width = 640,
> .height = 480,
> },
> @@ -644,22 +644,23 @@ static const struct ov5640_mode_info ov5640_mode_init_data = {
> .max_fps = OV5640_30_FPS
> };
>
> -static const struct ov5640_mode_info
> -ov5640_mode_data[OV5640_NUM_MODES] = {
> +static const struct ov5640_mode_info ov5640_mode_data[OV5640_NUM_MODES] = {
> {
> /* 160x120 */
> .id = OV5640_MODE_QQVGA_160_120,
> .dn_mode = SUBSAMPLING,
> .pixel_rate = OV5640_PIXEL_RATE_48M,
> .analog_crop = {
> - .left = 0,
> - .top = 4,
> - .width = 2623,
> - .height = 1947,
> + /* Feed the full valid pixel array to the ISP. */
> + .left = OV5640_PIXEL_ARRAY_LEFT,
> + .top = OV5640_PIXEL_ARRAY_TOP,
> + .width = OV5640_PIXEL_ARRAY_WIDTH,
> + .height = OV5640_PIXEL_ARRAY_HEIGHT,
> },
> .crop = {
> - .left = 16,
> - .top = 6,
> + /* Maintain a minimum digital crop processing margins. */
> + .left = 2,
> + .top = 4,
> .width = 160,
> .height = 120,
> },
> @@ -674,14 +675,16 @@ ov5640_mode_data[OV5640_NUM_MODES] = {
> .dn_mode = SUBSAMPLING,
> .pixel_rate = OV5640_PIXEL_RATE_48M,
> .analog_crop = {
> - .left = 0,
> - .top = 4,
> - .width = 2623,
> - .height = 1947,
> + /* Feed the full valid pixel array to the ISP. */
> + .left = OV5640_PIXEL_ARRAY_LEFT,
> + .top = OV5640_PIXEL_ARRAY_TOP,
> + .width = OV5640_PIXEL_ARRAY_WIDTH,
> + .height = OV5640_PIXEL_ARRAY_HEIGHT,
> },
> .crop = {
> - .left = 16,
> - .top = 6,
> + /* Maintain a minimum digital crop processing margins. */
> + .left = 2,
> + .top = 4,
> .width = 176,
> .height = 144,
> },
> @@ -696,14 +699,16 @@ ov5640_mode_data[OV5640_NUM_MODES] = {
> .dn_mode = SUBSAMPLING,
> .pixel_rate = OV5640_PIXEL_RATE_48M,
> .analog_crop = {
> - .left = 0,
> - .top = 4,
> - .width = 2623,
> - .height = 1947,
> + /* Feed the full valid pixel array to the ISP. */
> + .left = OV5640_PIXEL_ARRAY_LEFT,
> + .top = OV5640_PIXEL_ARRAY_TOP,
> + .width = OV5640_PIXEL_ARRAY_WIDTH,
> + .height = OV5640_PIXEL_ARRAY_HEIGHT,
> },
> .crop = {
> - .left = 16,
> - .top = 6,
> + /* Maintain a minimum digital crop processing margins. */
> + .left = 2,
> + .top = 4,
> .width = 320,
> .height = 240,
> },
> @@ -718,14 +723,16 @@ ov5640_mode_data[OV5640_NUM_MODES] = {
> .dn_mode = SUBSAMPLING,
> .pixel_rate = OV5640_PIXEL_RATE_48M,
> .analog_crop = {
> - .left = 0,
> - .top = 4,
> - .width = 2623,
> - .height = 1947,
> + /* Feed the full valid pixel array to the ISP. */
> + .left = OV5640_PIXEL_ARRAY_LEFT,
> + .top = OV5640_PIXEL_ARRAY_TOP,
> + .width = OV5640_PIXEL_ARRAY_WIDTH,
> + .height = OV5640_PIXEL_ARRAY_HEIGHT,
> },
> .crop = {
> - .left = 16,
> - .top = 6,
> + /* Maintain a minimum digital crop processing margins. */
> + .left = 2,
> + .top = 4,
> .width = 640,
> .height = 480,
> },
> @@ -740,12 +747,14 @@ ov5640_mode_data[OV5640_NUM_MODES] = {
> .dn_mode = SUBSAMPLING,
> .pixel_rate = OV5640_PIXEL_RATE_96M,
> .analog_crop = {
> - .left = 0,
> - .top = 4,
> - .width = 2623,
> - .height = 1947,
> + /* Feed the full valid pixel array to the ISP. */
> + .left = OV5640_PIXEL_ARRAY_LEFT,
> + .top = OV5640_PIXEL_ARRAY_TOP,
> + .width = OV5640_PIXEL_ARRAY_WIDTH,
> + .height = OV5640_PIXEL_ARRAY_HEIGHT,
> },
> .crop = {
> + /* Maintain a minimum digital crop processing margins. */
> .left = 56,
> .top = 60,
> .width = 720,
> @@ -762,12 +771,14 @@ ov5640_mode_data[OV5640_NUM_MODES] = {
> .dn_mode = SUBSAMPLING,
> .pixel_rate = OV5640_PIXEL_RATE_96M,
> .analog_crop = {
> - .left = 0,
> - .top = 4,
> - .width = 2623,
> - .height = 1947,
> + /* Feed the full valid pixel array to the ISP. */
> + .left = OV5640_PIXEL_ARRAY_LEFT,
> + .top = OV5640_PIXEL_ARRAY_TOP,
> + .width = OV5640_PIXEL_ARRAY_WIDTH,
> + .height = OV5640_PIXEL_ARRAY_HEIGHT,
> },
> .crop = {
> + /* Maintain a minimum digital crop processing margins. */
> .left = 56,
> .top = 6,
> .width = 720,
> @@ -786,8 +797,8 @@ ov5640_mode_data[OV5640_NUM_MODES] = {
> .analog_crop = {
> .left = 0,
> .top = 4,
> - .width = 2623,
> - .height = 1947,
> + .width = OV5640_NATIVE_WIDTH,
> + .height = OV5640_PIXEL_ARRAY_HEIGHT,
> },
> .crop = {
> .left = 16,
> @@ -808,8 +819,8 @@ ov5640_mode_data[OV5640_NUM_MODES] = {
> .analog_crop = {
> .left = 0,
> .top = 250,
> - .width = 2623,
> - .height = 1705,
> + .width = 2624,
> + .height = 1456,
> },
> .crop = {
> .left = 16,
> @@ -828,12 +839,14 @@ ov5640_mode_data[OV5640_NUM_MODES] = {
> .dn_mode = SCALING,
> .pixel_rate = OV5640_PIXEL_RATE_148M,
> .analog_crop = {
> + /* Crop the full valid pixel array in the center. */
> .left = 336,
> .top = 434,
> - .width = 2287,
> - .height = 1521,
> + .width = 1952,
> + .height = 1088,
> },
> .crop = {
> + /* Maintain a larger digital crop processing margins. */
> .left = 16,
> .top = 4,
> .width = 1920,
> @@ -850,16 +863,17 @@ ov5640_mode_data[OV5640_NUM_MODES] = {
> .dn_mode = SCALING,
> .pixel_rate = OV5640_PIXEL_RATE_168M,
> .analog_crop = {
> + /* Give more processing margin to full resolution. */
> .left = 0,
> .top = 0,
> - .width = 2623,
> - .height = 1951,
> + .width = OV5640_NATIVE_WIDTH,
> + .height = 1952,
> },
> .crop = {
> .left = 16,
> .top = 4,
> - .width = 2592,
> - .height = 1944,
> + .width = OV5640_PIXEL_ARRAY_WIDTH,
> + .height = OV5640_PIXEL_ARRAY_HEIGHT,
> },
> .htot = 2844,
> .vblank_def = 24,
> @@ -1384,11 +1398,13 @@ static int ov5640_set_timings(struct ov5640_dev *sensor,
> if (ret < 0)
> return ret;
>
> - ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_HW, analog_crop->width);
> + ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_HW,
> + analog_crop->width + analog_crop->left - 1);
> if (ret < 0)
> return ret;
>
> - ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_VH, analog_crop->height);
> + ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_VH,
> + analog_crop->height + analog_crop->top - 1);
I'd move this to 08/23 as mentioned in the review of that patch.
> if (ret < 0)
> return ret;
>
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2022-02-20 12:56 UTC|newest]
Thread overview: 64+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-10 11:04 [PATCH v2 00/23] media: ov5640: Rework the clock tree programming for MIPI Jacopo Mondi
2022-02-10 11:04 ` [PATCH v2 01/23] media: ov5640: Add pixel rate to modes Jacopo Mondi
2022-02-20 11:53 ` Laurent Pinchart
2022-02-21 20:14 ` Adam Ford
2022-02-22 7:48 ` Jacopo Mondi
2022-02-22 19:08 ` Adam Ford
2022-02-10 11:04 ` [PATCH v2 02/23] media: ov5604: Re-arrange modes definition Jacopo Mondi
2022-02-10 11:04 ` [PATCH v2 03/23] media: ov5640: Add ov5640_is_csi2() function Jacopo Mondi
2022-02-10 11:04 ` [PATCH v2 04/23] media: ov5640: Associate bpp with formats Jacopo Mondi
2022-02-10 11:04 ` [PATCH v2 05/23] media: ov5640: Add LINK_FREQ control Jacopo Mondi
2022-02-20 11:55 ` Laurent Pinchart
2022-02-10 11:04 ` [PATCH v2 06/23] media: ov5640: Update pixel_rate and link_freq Jacopo Mondi
2022-02-10 11:04 ` [PATCH v2 07/23] media: ov5640: Rework CSI-2 clock tree Jacopo Mondi
2022-02-20 12:17 ` Laurent Pinchart
2022-02-21 11:39 ` Jacopo Mondi
2022-02-21 12:12 ` Laurent Pinchart
2022-02-10 11:04 ` [PATCH v2 08/23] media: ov5640: Rework timings programming Jacopo Mondi
2022-02-20 12:44 ` Laurent Pinchart
2022-02-10 11:04 ` [PATCH v2 09/23] media: ov5640: Fix 720x480 in RGB888 mode Jacopo Mondi
2022-02-20 12:50 ` Laurent Pinchart
2022-02-10 11:04 ` [PATCH v2 10/23] media: ov5640: Rework analog crop rectangles Jacopo Mondi
2022-02-11 9:34 ` [v2.1] " Jacopo Mondi
2022-02-20 12:56 ` Laurent Pinchart [this message]
2022-02-10 11:04 ` [PATCH v2 11/23] media: ov5640: Re-sort per-mode register tables Jacopo Mondi
2022-02-20 12:52 ` Laurent Pinchart
2022-02-20 12:59 ` Laurent Pinchart
2022-02-10 11:04 ` [PATCH v2 12/23] media: ov5640: Remove ov5640_mode_init_data Jacopo Mondi
2022-02-20 12:58 ` Laurent Pinchart
2022-02-10 11:09 ` [PATCH v2 13/23] media: ov5640: Add HBLANK control Jacopo Mondi
2022-02-10 11:09 ` [PATCH v2 14/23] media: ov5640: Add VBLANK control Jacopo Mondi
2022-02-20 13:01 ` Laurent Pinchart
2022-02-10 11:10 ` [PATCH v2 15/23] media: ov5640: Fix durations to comply with FPS Jacopo Mondi
2022-02-20 13:03 ` Laurent Pinchart
2022-02-10 11:10 ` [PATCH v2 16/23] media: ov5640: Implement init_cfg Jacopo Mondi
2022-02-10 11:10 ` [PATCH v2 17/23] media: ov5640: Implement get_selection Jacopo Mondi
2022-02-20 13:06 ` Laurent Pinchart
2022-02-10 11:10 ` [PATCH v2 18/23] media: ov5640: Limit frame_interval to DVP mode only Jacopo Mondi
2022-02-10 11:10 ` [PATCH v2 19/23] media: ov5640: Register device properties Jacopo Mondi
2022-02-10 11:10 ` [PATCH v2 20/23] media: ov5640: Add RGB565_1X16 format Jacopo Mondi
2022-02-20 13:07 ` Laurent Pinchart
2022-02-10 11:10 ` [PATCH v2 21/23] media: ov5640: Add RGB888/BGR888 formats Jacopo Mondi
2022-02-20 13:13 ` Laurent Pinchart
2022-02-10 11:10 ` [PATCH v2 22/23] media: ov5640: Restrict sizes to mbus code Jacopo Mondi
2022-02-20 13:16 ` Laurent Pinchart
2022-02-21 12:42 ` Jacopo Mondi
2022-02-10 11:10 ` [PATCH v2 23/23] media: ov5640: Adjust format to bpp in s_fmt Jacopo Mondi
2022-02-10 12:03 ` [PATCH v2 00/23] media: ov5640: Rework the clock tree programming for MIPI Tomi Valkeinen
2022-02-10 12:10 ` Tomi Valkeinen
2022-02-10 13:00 ` Tomi Valkeinen
2022-02-10 17:11 ` Jacopo Mondi
2022-02-11 7:55 ` Tomi Valkeinen
2022-02-11 8:01 ` Tomi Valkeinen
2022-02-11 9:36 ` Jacopo Mondi
2022-02-11 10:09 ` Eugen.Hristev
2022-02-11 11:25 ` Jacopo Mondi
2022-02-14 14:06 ` Eugen.Hristev
2022-02-14 14:38 ` Jacopo Mondi
2022-02-14 15:08 ` Eugen.Hristev
2022-02-14 18:56 ` Jacopo Mondi
2022-02-17 14:25 ` Eugen.Hristev
2022-02-21 8:51 ` Jacopo Mondi
2022-02-21 9:04 ` Eugen.Hristev
2022-02-21 11:18 ` Jacopo Mondi
2022-02-21 13:31 ` Jacopo Mondi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YhI6gZUiIbDkiHlf@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=Eugen.Hristev@microchip.com \
--cc=aford173@gmail.com \
--cc=festevam@gmail.com \
--cc=hugues.fruchet@st.com \
--cc=hverkuil-cisco@xs4all.nl \
--cc=jacopo@jmondi.org \
--cc=jbrunet@baylibre.com \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=mirela.rabulea@nxp.com \
--cc=prabhakar.mahadev-lad.rj@bp.renesas.com \
--cc=sakari.ailus@iki.fi \
--cc=slongerbeam@gmail.com \
--cc=tomi.valkeinen@ideasonboard.com \
--cc=xavier.roumegue@oss.nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.