Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 1/3] media: atmel: atmel-isc: limit incoming pixels per
 frame
To:     Eugen.Hristev@microchip.com, linux-media@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Cc:     Nicolas.Ferre@microchip.com, mchehab@kernel.org,
        ksloat@aampglobal.com
References: <1555064098-19310-1-git-send-email-eugen.hristev@microchip.com>
 <1555064098-19310-2-git-send-email-eugen.hristev@microchip.com>
From:   Hans Verkuil <hverkuil@xs4all.nl>
Message-ID: <7978caee-9ae1-a428-af14-34bfa12ad223@xs4all.nl>
Date:   Fri, 12 Apr 2019 13:50:48 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.6.1
MIME-Version: 1.0
In-Reply-To: <1555064098-19310-2-git-send-email-eugen.hristev@microchip.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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


> +		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);
>