On 12.04.2019 14:50, Hans Verkuil wrote: > > On 4/12/19 12:19 PM, Eugen.Hristev@xxxxxxxxxxxxx wrote: >> From: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx> >> >> 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@xxxxxxxxxxxxx> >> --- >> 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... 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@xxxxxxxxxxxxxxxxxxx > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > >