From: Hans Verkuil <hverkuil@xs4all.nl>
To: Eugen.Hristev@microchip.com, linux-media@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Cc: ksloat@aampglobal.com, mchehab@kernel.org
Subject: Re: [PATCH 1/3] media: atmel: atmel-isc: limit incoming pixels per frame
Date: Fri, 12 Apr 2019 14:06:28 +0200 [thread overview]
Message-ID: <48dc0a0e-85ba-0a77-effe-cb4fc17c74c5@xs4all.nl> (raw)
In-Reply-To: <7461deba-eca0-24cb-f232-3dbade95c297@microchip.com>
On 4/12/19 2:02 PM, Eugen.Hristev@microchip.com wrote:
>
>
> On 12.04.2019 14:50, Hans Verkuil wrote:
>
>>
>> On 4/12/19 12:19 PM, Eugen.Hristev@microchip.com wrote:
>>> From: Eugen Hristev <eugen.hristev@microchip.com>
>>>
>>> This will limit the incoming pixels per frame from the sensor.
>>> Currently, the ISC will stop sampling the frame only when the vsync/hsync
>>> are detected.
>>> If we misconfigure the resolution in the sensor w.r.t. resolution in the ISC,
>>> the buffer used for DMA in the ISC will be smaller than the number of pixels
>>> that the ISC DMA engine will copy.
>>> In this case it happens that the DMA will overwrite parts of the memory which
>>> should not be written, leading to memory corruption.
>>> To avoid this situation, use the PFE CFG1 and PFE CFG2 registers, which crop
>>> the incoming frame to the resolution that we configure.
>>> This way the DMA engine will never write more data than we expect it to.
>>>
>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>>> ---
>>> drivers/media/platform/atmel/atmel-isc-regs.h | 19 +++++++++++++++
>>> drivers/media/platform/atmel/atmel-isc.c | 34 +++++++++++++++++++++++++++
>>> 2 files changed, 53 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/atmel/atmel-isc-regs.h b/drivers/media/platform/atmel/atmel-isc-regs.h
>>> index 2aadc19..768a5ad 100644
>>> --- a/drivers/media/platform/atmel/atmel-isc-regs.h
>>> +++ b/drivers/media/platform/atmel/atmel-isc-regs.h
>>> @@ -35,6 +35,25 @@
>>> #define ISC_PFG_CFG0_BPS_TWELVE (0x0 << 28)
>>> #define ISC_PFE_CFG0_BPS_MASK GENMASK(30, 28)
>>>
>>> +#define ISC_PFE_CFG0_COLEN BIT(12)
>>> +#define ISC_PFE_CFG0_ROWEN BIT(13)
>>> +
>>> +/* ISC Parallel Front End Configuration 1 Register */
>>> +#define ISC_PFE_CFG1 0x00000010
>>> +
>>> +#define ISC_PFE_CFG1_COLMIN(v) ((v))
>>> +#define ISC_PFE_CFG1_COLMIN_MASK GENMASK(15, 0)
>>> +#define ISC_PFE_CFG1_COLMAX(v) ((v) << 16)
>>> +#define ISC_PFE_CFG1_COLMAX_MASK GENMASK(31, 16)
>>> +
>>> +/* ISC Parallel Front End Configuration 2 Register */
>>> +#define ISC_PFE_CFG2 0x00000014
>>> +
>>> +#define ISC_PFE_CFG2_ROWMIN(v) ((v))
>>> +#define ISC_PFE_CFG2_ROWMIN_MASK GENMASK(15, 0)
>>> +#define ISC_PFE_CFG2_ROWMAX(v) ((v) << 16)
>>> +#define ISC_PFE_CFG2_ROWMAX_MASK GENMASK(31, 16)
>>> +
>>> /* ISC Clock Enable Register */
>>> #define ISC_CLKEN 0x00000018
>>>
>>> diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
>>> index a10db16..ea7520a 100644
>>> --- a/drivers/media/platform/atmel/atmel-isc.c
>>> +++ b/drivers/media/platform/atmel/atmel-isc.c
>>> @@ -721,6 +721,40 @@ static void isc_start_dma(struct isc_device *isc)
>>> u32 sizeimage = isc->fmt.fmt.pix.sizeimage;
>>> u32 dctrl_dview;
>>> dma_addr_t addr0;
>>> + u32 h, w;
>>> +
>>> + h = isc->fmt.fmt.pix.height;
>>> + w = isc->fmt.fmt.pix.width;
>>> +
>>> + /*
>>> + * In case the sensor is not RAW, it will output a pixel (12-16 bits)
>>> + * with two samples on the ISC Data bus (which is 8-12)
>>> + * ISC will count each sample, so, we need to multiply these values
>>> + * by two, to get the real number of samples for the required pixels.
>>> + */
>>> + if (!ISC_IS_FORMAT_RAW(isc->config.sd_format->mbus_code)) {
>>
>> The ISC_IS_FORMAT_RAW define doesn't exist?!
>>
>> Something clearly went wrong...
>>
>> Regards,
>>
>> Hans
>
> Hello Hans,
>
> Sorry , I forgot to copy this from the previous series
> (https://www.spinics.net/lists/linux-media/msg149501.html for reference):
>
> It applies only on top of my previous patchset:
> media: atmel: atmel-isc: removed ARGB32 added ABGR32 and XBGR32
> media: atmel: atmel-isc: reworked driver and formats
> available at:
> https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=for-v5.2b&id=03ef1b56cba6ad17f6ead13c85a81e0e80fbc9d1
>
> So it should work on top of those patches...
Ah, now I see. I'll park this patch series until the pull request containing
those two patches is merged. Feel free to remind me of this series once Mauro
merged that pull request.
Regards,
Hans
>
> Eugen
>
>
>>
>>
>>> + h <<= 1;
>>> + w <<= 1;
>>> + }
>>> +
>>> + /*
>>> + * We limit the column/row count that the ISC will output according
>>> + * to the configured resolution that we want.
>>> + * This will avoid the situation where the sensor is misconfigured,
>>> + * sending more data, and the ISC will just take it and DMA to memory,
>>> + * causing corruption.
>>> + */
>>> + regmap_write(regmap, ISC_PFE_CFG1,
>>> + (ISC_PFE_CFG1_COLMIN(0) & ISC_PFE_CFG1_COLMIN_MASK) |
>>> + (ISC_PFE_CFG1_COLMAX(w - 1) & ISC_PFE_CFG1_COLMAX_MASK));
>>> +
>>> + regmap_write(regmap, ISC_PFE_CFG2,
>>> + (ISC_PFE_CFG2_ROWMIN(0) & ISC_PFE_CFG2_ROWMIN_MASK) |
>>> + (ISC_PFE_CFG2_ROWMAX(h - 1) & ISC_PFE_CFG2_ROWMAX_MASK));
>>> +
>>> + regmap_update_bits(regmap, ISC_PFE_CFG0,
>>> + ISC_PFE_CFG0_COLEN | ISC_PFE_CFG0_ROWEN,
>>> + ISC_PFE_CFG0_COLEN | ISC_PFE_CFG0_ROWEN);
>>>
>>> addr0 = vb2_dma_contig_plane_dma_addr(&isc->cur_frm->vb.vb2_buf, 0);
>>> regmap_write(regmap, ISC_DAD0, addr0);
>>>
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>>
next prev parent reply other threads:[~2019-04-12 12:06 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-12 10:19 [PATCH 0/3] media: atmel: atmel-isc: some fixes Eugen.Hristev
2019-04-12 10:19 ` [PATCH 1/3] media: atmel: atmel-isc: limit incoming pixels per frame Eugen.Hristev
2019-04-12 11:50 ` Hans Verkuil
2019-04-12 12:02 ` Eugen.Hristev
2019-04-12 12:06 ` Hans Verkuil [this message]
2019-04-12 12:13 ` Eugen.Hristev
2019-04-23 13:47 ` Eugen.Hristev
2019-04-12 10:19 ` [PATCH 2/3] media: atmel: atmel-isc: fix INIT_WORK misplacement Eugen.Hristev
2019-04-12 10:19 ` [PATCH 3/3] media: atmel: atmel-isc: fix asd memory allocation Eugen.Hristev
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=48dc0a0e-85ba-0a77-effe-cb4fc17c74c5@xs4all.nl \
--to=hverkuil@xs4all.nl \
--cc=Eugen.Hristev@microchip.com \
--cc=ksloat@aampglobal.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).