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

[<Eugen.Hristev@microchip.com>]

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]