| Message ID | 1555064098-19310-2-git-send-email-eugen.hristev@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: atmel: atmel-isc: some fixes | expand |
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); >
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... 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 > >
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 >> >>
On 12.04.2019 15:06, Hans Verkuil wrote: > External E-Mail > > > 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. Thank you. I mostly sent this series to get your early review. It would be important to get the new feature set integrated, so, thank you for that, and I will send the v2 on the new feature set, so I can work on it until the first patches are merged. Eugen > > 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 >>> >>> >
On 12.04.2019 15:09, Eugen Hristev wrote: > > > On 12.04.2019 15:06, Hans Verkuil wrote: >> External E-Mail >> >> >> 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. Hello Hans, This series should apply OK now on latest media_tree.git Eugen > > Thank you. > > I mostly sent this series to get your early review. It would be > important to get the new feature set integrated, so, thank you for that, > and I will send the v2 on the new feature set, so I can work on it until > the first patches are merged. > > Eugen > >> >> Regards, >> >> Hans >> >>> >>> Eugen >>> >>> >>>> [snip]
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)) { + 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);