* [PATCH 0/3] media: atmel: atmel-isc: some fixes
@ 2019-04-12 10:19 Eugen.Hristev
2019-04-12 10:19 ` [PATCH 1/3] media: atmel: atmel-isc: limit incoming pixels per frame Eugen.Hristev
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Eugen.Hristev @ 2019-04-12 10:19 UTC (permalink / raw)
To: linux-media, hverkuil, linux-arm-kernel, linux-kernel
Cc: Nicolas.Ferre, mchehab, ksloat, Eugen.Hristev
From: Eugen Hristev <eugen.hristev@microchip.com>
Hello,
This series includes some fixes for the atmel-isc media driver.
As requested by Hans , created a first series of only fixes,
the patches are the same as in the previous patch series, but removed
the one with safe checks: Hans said it's overkill.
Another series based on this one I will send soon with the new feature set,
reworked based on Hans' review.
Thanks !
Eugen Hristev (3):
media: atmel: atmel-isc: limit incoming pixels per frame
media: atmel: atmel-isc: fix INIT_WORK misplacement
media: atmel: atmel-isc: fix asd memory allocation
drivers/media/platform/atmel/atmel-isc-regs.h | 19 +++++++++++
drivers/media/platform/atmel/atmel-isc.c | 46 ++++++++++++++++++++++++---
2 files changed, 61 insertions(+), 4 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/3] media: atmel: atmel-isc: limit incoming pixels per frame 2019-04-12 10:19 [PATCH 0/3] media: atmel: atmel-isc: some fixes Eugen.Hristev @ 2019-04-12 10:19 ` Eugen.Hristev 2019-04-12 11:50 ` Hans Verkuil 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 2 siblings, 1 reply; 9+ messages in thread From: Eugen.Hristev @ 2019-04-12 10:19 UTC (permalink / raw) To: linux-media, hverkuil, linux-arm-kernel, linux-kernel Cc: Nicolas.Ferre, mchehab, ksloat, Eugen.Hristev 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)) { + 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); -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] media: atmel: atmel-isc: limit incoming pixels per frame 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 0 siblings, 1 reply; 9+ messages in thread From: Hans Verkuil @ 2019-04-12 11:50 UTC (permalink / raw) To: Eugen.Hristev, linux-media, linux-arm-kernel, linux-kernel Cc: Nicolas.Ferre, mchehab, ksloat 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); > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] media: atmel: atmel-isc: limit incoming pixels per frame 2019-04-12 11:50 ` Hans Verkuil @ 2019-04-12 12:02 ` Eugen.Hristev 2019-04-12 12:06 ` Hans Verkuil 0 siblings, 1 reply; 9+ messages in thread From: Eugen.Hristev @ 2019-04-12 12:02 UTC (permalink / raw) To: hverkuil, linux-media, linux-arm-kernel, linux-kernel; +Cc: ksloat, mchehab 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 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] media: atmel: atmel-isc: limit incoming pixels per frame 2019-04-12 12:02 ` Eugen.Hristev @ 2019-04-12 12:06 ` Hans Verkuil 2019-04-12 12:13 ` Eugen.Hristev 0 siblings, 1 reply; 9+ messages in thread From: Hans Verkuil @ 2019-04-12 12:06 UTC (permalink / raw) To: Eugen.Hristev, linux-media, linux-arm-kernel, linux-kernel Cc: ksloat, mchehab 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 >> >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] media: atmel: atmel-isc: limit incoming pixels per frame 2019-04-12 12:06 ` Hans Verkuil @ 2019-04-12 12:13 ` Eugen.Hristev 2019-04-23 13:47 ` Eugen.Hristev 0 siblings, 1 reply; 9+ messages in thread From: Eugen.Hristev @ 2019-04-12 12:13 UTC (permalink / raw) To: hverkuil, linux-media, linux-arm-kernel, linux-kernel; +Cc: ksloat, mchehab 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 >>> >>> > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] media: atmel: atmel-isc: limit incoming pixels per frame 2019-04-12 12:13 ` Eugen.Hristev @ 2019-04-23 13:47 ` Eugen.Hristev 0 siblings, 0 replies; 9+ messages in thread From: Eugen.Hristev @ 2019-04-23 13:47 UTC (permalink / raw) To: hverkuil, linux-media, linux-arm-kernel, linux-kernel; +Cc: ksloat, mchehab 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] ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] media: atmel: atmel-isc: fix INIT_WORK misplacement 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 10:19 ` Eugen.Hristev 2019-04-12 10:19 ` [PATCH 3/3] media: atmel: atmel-isc: fix asd memory allocation Eugen.Hristev 2 siblings, 0 replies; 9+ messages in thread From: Eugen.Hristev @ 2019-04-12 10:19 UTC (permalink / raw) To: linux-media, hverkuil, linux-arm-kernel, linux-kernel Cc: Nicolas.Ferre, mchehab, ksloat, Eugen.Hristev From: Eugen Hristev <eugen.hristev@microchip.com> In case the completion function failes, unbind will be called which will call cancel_work for awb_work. This will trigger a WARN message from the workqueue. To avoid this, move the INIT_WORK call at the start of the completion function. This way the work is always initialized, which corresponds to the 'always canceled' unbind code. Fixes: 93d4a26c3d ("[media] atmel-isc: add the isc pipeline function") Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com> --- drivers/media/platform/atmel/atmel-isc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c index ea7520a..aef90db 100644 --- a/drivers/media/platform/atmel/atmel-isc.c +++ b/drivers/media/platform/atmel/atmel-isc.c @@ -1998,6 +1998,8 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier) struct vb2_queue *q = &isc->vb2_vidq; int ret; + INIT_WORK(&isc->awb_work, isc_awb_work); + ret = v4l2_device_register_subdev_nodes(&isc->v4l2_dev); if (ret < 0) { v4l2_err(&isc->v4l2_dev, "Failed to register subdev nodes\n"); @@ -2051,8 +2053,6 @@ static int isc_async_complete(struct v4l2_async_notifier *notifier) return ret; } - INIT_WORK(&isc->awb_work, isc_awb_work); - /* Register video device */ strscpy(vdev->name, ATMEL_ISC_NAME, sizeof(vdev->name)); vdev->release = video_device_release_empty; -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] media: atmel: atmel-isc: fix asd memory allocation 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 10:19 ` [PATCH 2/3] media: atmel: atmel-isc: fix INIT_WORK misplacement Eugen.Hristev @ 2019-04-12 10:19 ` Eugen.Hristev 2 siblings, 0 replies; 9+ messages in thread From: Eugen.Hristev @ 2019-04-12 10:19 UTC (permalink / raw) To: linux-media, hverkuil, linux-arm-kernel, linux-kernel Cc: Nicolas.Ferre, mchehab, ksloat, Eugen.Hristev From: Eugen Hristev <eugen.hristev@microchip.com> The subsystem will free the asd memory on notifier cleanup, if the asd is added to the notifier. However the memory is freed using kfree. Thus, we cannot allocate the asd using devm_* This can lead to crashes and problems. To test this issue, just return an error at probe, but cleanup the notifier beforehand. Fixes: 106267444f ("[media] atmel-isc: add the Image Sensor Controller code") Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com> --- drivers/media/platform/atmel/atmel-isc.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c index aef90db..8e072a9 100644 --- a/drivers/media/platform/atmel/atmel-isc.c +++ b/drivers/media/platform/atmel/atmel-isc.c @@ -2168,8 +2168,11 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc) break; } - subdev_entity->asd = devm_kzalloc(dev, - sizeof(*subdev_entity->asd), GFP_KERNEL); + /* asd will be freed by the subsystem once it's added to the + * notifier list + */ + subdev_entity->asd = kzalloc(sizeof(*subdev_entity->asd), + GFP_KERNEL); if (!subdev_entity->asd) { of_node_put(rem); ret = -ENOMEM; @@ -2313,6 +2316,7 @@ static int atmel_isc_probe(struct platform_device *pdev) subdev_entity->asd); if (ret) { fwnode_handle_put(subdev_entity->asd->match.fwnode); + kfree(subdev_entity->asd); goto cleanup_subdev; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-04-23 13:47 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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
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).