Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751868AbdFZJyE (ORCPT ); Mon, 26 Jun 2017 05:54:04 -0400 Received: from mx07-00178001.pphosted.com ([62.209.51.94]:59287 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751372AbdFZJxy (ORCPT ); Mon, 26 Jun 2017 05:53:54 -0400 From: Hugues FRUCHET To: Hans Verkuil , Maxime Coquelin , Alexandre TORGUE , Mauro Carvalho Chehab CC: "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "linux-media@vger.kernel.org" , Benjamin Gaignard , Yannick FERTRE Subject: Re: [PATCH v1 3/5] [media] stm32-dcmi: crop sensor image to match user resolution Thread-Topic: [PATCH v1 3/5] [media] stm32-dcmi: crop sensor image to match user resolution Thread-Index: AQHS62oSD4V9v3cU+EiItW8QhmYH5aIw3aWAgAXuEgA= Date: Mon, 26 Jun 2017 09:53:18 +0000 Message-ID: <8ae4c160-3137-0fa8-3d78-e4e1284521a4@st.com> References: <1498144371-13310-1-git-send-email-hugues.fruchet@st.com> <1498144371-13310-4-git-send-email-hugues.fruchet@st.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: user-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 x-ms-exchange-messagesentrepresentingtype: 1 x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.75.127.50] Content-Type: text/plain; charset="utf-8" Content-ID: <50B199C8E011B048ABAA5CE0E98D5542@st.com> MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-06-26_08:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id v5Q9sBgi019874 Content-Length: 5382 Lines: 156 Hi Hans, thanks for review. Reply below. BR Hugues. On 06/22/2017 05:19 PM, Hans Verkuil wrote: > On 06/22/2017 05:12 PM, Hugues Fruchet wrote: >> Add flexibility on supported resolutions by cropping sensor >> image to fit user resolution format request. >> >> Signed-off-by: Hugues Fruchet >> --- >> drivers/media/platform/stm32/stm32-dcmi.c | 54 ++++++++++++++++++++++++++++++- >> 1 file changed, 53 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c >> index 75d53aa..bc5e052 100644 >> --- a/drivers/media/platform/stm32/stm32-dcmi.c >> +++ b/drivers/media/platform/stm32/stm32-dcmi.c >> @@ -131,6 +131,8 @@ struct stm32_dcmi { >> struct v4l2_async_notifier notifier; >> struct dcmi_graph_entity entity; >> struct v4l2_format fmt; >> + struct v4l2_rect crop; >> + bool do_crop; >> >> const struct dcmi_format **user_formats; >> unsigned int num_user_formats; >> @@ -538,6 +540,27 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count) >> if (dcmi->bus.flags & V4L2_MBUS_PCLK_SAMPLE_RISING) >> val |= CR_PCKPOL; >> >> + if (dcmi->do_crop) { >> + u32 size, start; >> + >> + /* Crop resolution */ >> + size = ((dcmi->crop.height - 1) << 16) | >> + ((dcmi->crop.width << 1) - 1); >> + reg_write(dcmi->regs, DCMI_CWSIZE, size); >> + >> + /* Crop start point */ >> + start = ((dcmi->crop.top) << 16) | >> + ((dcmi->crop.left << 1)); >> + reg_write(dcmi->regs, DCMI_CWSTRT, start); >> + >> + dev_dbg(dcmi->dev, "Cropping to %ux%u@%u:%u\n", >> + dcmi->crop.width, dcmi->crop.height, >> + dcmi->crop.left, dcmi->crop.top); >> + >> + /* Enable crop */ >> + val |= CR_CROP; >> + }; >> + >> reg_write(dcmi->regs, DCMI_CR, val); >> >> /* Enable dcmi */ >> @@ -707,6 +730,8 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f, >> .which = V4L2_SUBDEV_FORMAT_TRY, >> }; >> int ret; >> + __u32 width, height; >> + struct v4l2_mbus_framefmt *mf = &format.format; >> >> dcmi_fmt = find_format_by_fourcc(dcmi, pixfmt->pixelformat); >> if (!dcmi_fmt) { >> @@ -724,8 +749,18 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f, >> if (ret < 0) >> return ret; >> >> + /* Align format on what sensor can do */ >> + width = pixfmt->width; >> + height = pixfmt->height; >> v4l2_fill_pix_format(pixfmt, &format.format); >> >> + /* We can do any resolution thanks to crop */ >> + if ((mf->width > width) || (mf->height > height)) { >> + /* Restore width/height */ >> + pixfmt->width = width; >> + pixfmt->height = height; >> + }; >> + >> pixfmt->field = V4L2_FIELD_NONE; >> pixfmt->bytesperline = pixfmt->width * dcmi_fmt->bpp; >> pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height; >> @@ -741,6 +776,8 @@ static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f) >> struct v4l2_subdev_format format = { >> .which = V4L2_SUBDEV_FORMAT_ACTIVE, >> }; >> + struct v4l2_mbus_framefmt *mf = &format.format; >> + struct v4l2_pix_format *pixfmt = &f->fmt.pix; >> const struct dcmi_format *current_fmt; >> int ret; >> >> @@ -748,13 +785,28 @@ static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f) >> if (ret) >> return ret; >> >> - v4l2_fill_mbus_format(&format.format, &f->fmt.pix, >> + v4l2_fill_mbus_format(&format.format, pixfmt, >> current_fmt->mbus_code); >> ret = v4l2_subdev_call(dcmi->entity.subdev, pad, >> set_fmt, NULL, &format); >> if (ret < 0) >> return ret; >> >> + /* Enable crop if sensor resolution is larger than request */ >> + dcmi->do_crop = false; >> + if ((mf->width > pixfmt->width) || (mf->height > pixfmt->height)) { >> + dcmi->crop.width = pixfmt->width; >> + dcmi->crop.height = pixfmt->height; >> + dcmi->crop.left = (mf->width - pixfmt->width) / 2; >> + dcmi->crop.top = (mf->height - pixfmt->height) / 2; >> + dcmi->do_crop = true; > > Why not implement the selection API instead? I assume that you can crop from any > region of the sensor, not just the center part. The point here was to add some flexibility for user in term of resolution and also less memory consumption. For example here I want to make a 480x272 preview: - without this change: S_FMT(480x272) returns VGA (the OV9655 larger discrete resolution), then app has to capture VGA frames then crop to fit 480x272 frame buffer. - with this change: S_FMT(480x272) returns 480x272 (crop done by hardware), app can directly capture 480x272 then copy to framebuffer without any conversion. Implementation of V4L2 crop using SELECTION API could also be used, but I need to change app. More generally, with a given couple ISP+sensor, will S_FMT() return the sensor only supported resolutions ? or the supported resolutions of the couple ISP+sensor (ISP will downscale/upscale/crop the sensor discrete resolution to fit user request) ? Hans, what are your recommendations ? > > Regards, > > Hans > >> + >> + dev_dbg(dcmi->dev, "%ux%u cropped to %ux%u@(%u,%u)\n", >> + mf->width, mf->height, >> + dcmi->crop.width, dcmi->crop.height, >> + dcmi->crop.left, dcmi->crop.top); >> + }; >> + >> dcmi->fmt = *f; >> dcmi->current_fmt = current_fmt; >> >> >