Re: [PATCH 1/3] media: atmel: atmel-isc: limit incoming pixels per frame

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 12.04.2019 15:06, Hans Verkuil wrote:
> External E-Mail
> 
> 
> On 4/12/19 2:02 PM, Eugen.Hristev@xxxxxxxxxxxxx wrote:
>>
>>
>> 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...
> 
> 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@xxxxxxxxxxxxxxxxxxx
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
>>>
> 




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux