| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <24516b2e-e165-da81-cd2c-600c35b31f69@microchip.com>
Date: Wed, 10 Aug 2016 15:14:43 +0800
From: "Wu, Songjun" <Songjun.Wu@...rochip.com>
To: Hans Verkuil <hverkuil@...all.nl>, <nicolas.ferre@...el.com>,
<robh@...nel.org>
CC: <laurent.pinchart@...asonboard.com>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-media@...r.kernel.org>, "Arnd Bergmann" <arnd@...db.de>,
Niklas Söderlund
<niklas.soderlund+renesas@...natech.se>,
Benoit Parrot <bparrot@...com>, <linux-kernel@...r.kernel.org>,
Andrew-CT Chen <andrew-ct.chen@...iatek.com>,
Sudip Mukherjee <sudipm.mukherjee@...il.com>,
Kamil Debski <kamil@...as.org>,
Tiffany Lin <tiffany.lin@...iatek.com>,
Peter Griffin <peter.griffin@...aro.org>,
Geert Uytterhoeven <geert@...ux-m68k.org>,
Mikhail Ulyanov <mikhail.ulyanov@...entembedded.com>,
Richard Röjfors <richard@...finpack.se>,
Hans Verkuil <hans.verkuil@...co.com>,
Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
Simon Horman <horms+renesas@...ge.net.au>
Subject: Re: [PATCH v8 1/2] [media] atmel-isc: add the Image Sensor Controller
code
On 8/10/2016 15:12, Hans Verkuil wrote:
> On 08/10/2016 07:36 AM, Wu, Songjun wrote:
>>
>>
>> On 8/8/2016 17:56, Hans Verkuil wrote:
>>> On 08/08/2016 11:37 AM, Hans Verkuil wrote:
>>>> On 08/03/2016 10:08 AM, Songjun Wu wrote:
>>>>> Add driver for the Image Sensor Controller. It manages
>>>>> incoming data from a parallel based CMOS/CCD sensor.
>>>>> It has an internal image processor, also integrates a
>>>>> triple channel direct memory access controller master
>>>>> interface.
>>>>>
>>>>> Signed-off-by: Songjun Wu <songjun.wu@...rochip.com>
>>>>> ---
>>>>>
>>>>> Changes in v8:
>>>>> - Power on the sensor on the first open in function
>>>>> 'isc_open'.
>>>>> - Power off the sensor on the last release in function
>>>>> 'isc_release'.
>>>>> - Remove the switch of the pipeline.
>>>>>
>>>>> Changes in v7:
>>>>> - Add enum_framesizes and enum_frameintervals.
>>>>> - Call s_stream(0) when stream start fail.
>>>>> - Fill the device_caps field of struct video_device
>>>>> with V4L2_CAP_STREAMING and V4L2_CAP_VIDEO_CAPTURE.
>>>>> - Initialize the dev of struct vb2_queue.
>>>>> - Set field to FIELD_NONE if the pix field is not supported.
>>>>> - Return the result directly when call g/s_parm of subdev.
>>>>>
>>>>> Changes in v6: None
>>>>> Changes in v5:
>>>>> - Modify the macro definition and the related code.
>>>>>
>>>>> Changes in v4:
>>>>> - Modify the isc clock code since the dt is changed.
>>>>>
>>>>> Changes in v3:
>>>>> - Add pm runtime feature.
>>>>> - Modify the isc clock code since the dt is changed.
>>>>>
>>>>> Changes in v2:
>>>>> - Add "depends on COMMON_CLK" and "VIDEO_V4L2_SUBDEV_API"
>>>>> in Kconfig file.
>>>>> - Correct typos and coding style according to Laurent's remarks
>>>>> - Delete the loop while in 'isc_clk_enable' function.
>>>>> - Replace 'hsync_active', 'vsync_active' and 'pclk_sample'
>>>>> with 'pfe_cfg0' in struct isc_subdev_entity.
>>>>> - Add the code to support VIDIOC_CREATE_BUFS in
>>>>> 'isc_queue_setup' function.
>>>>> - Invoke isc_config to configure register in
>>>>> 'isc_start_streaming' function.
>>>>> - Add the struct completion 'comp' to synchronize with
>>>>> the frame end interrupt in 'isc_stop_streaming' function.
>>>>> - Check the return value of the clk_prepare_enable
>>>>> in 'isc_open' function.
>>>>> - Set the default format in 'isc_open' function.
>>>>> - Add an exit condition in the loop while in 'isc_config'.
>>>>> - Delete the hardware setup operation in 'isc_set_format'.
>>>>> - Refuse format modification during streaming
>>>>> in 'isc_s_fmt_vid_cap' function.
>>>>> - Invoke v4l2_subdev_alloc_pad_config to allocate and
>>>>> initialize the pad config in 'isc_async_complete' function.
>>>>> - Remove the '.owner = THIS_MODULE,' in atmel_isc_driver.
>>>>> - Replace the module_platform_driver_probe() with
>>>>> module_platform_driver().
>>>>>
>>>>> drivers/media/platform/Kconfig | 1 +
>>>>> drivers/media/platform/Makefile | 2 +
>>>>> drivers/media/platform/atmel/Kconfig | 9 +
>>>>> drivers/media/platform/atmel/Makefile | 1 +
>>>>> drivers/media/platform/atmel/atmel-isc-regs.h | 165 +++
>>>>> drivers/media/platform/atmel/atmel-isc.c | 1503 +++++++++++++++++++++++++
>>>>> 6 files changed, 1681 insertions(+)
>>>>> create mode 100644 drivers/media/platform/atmel/Kconfig
>>>>> create mode 100644 drivers/media/platform/atmel/Makefile
>>>>> create mode 100644 drivers/media/platform/atmel/atmel-isc-regs.h
>>>>> create mode 100644 drivers/media/platform/atmel/atmel-isc.c
>>>>>
>>>>
>>>> <snip>
>>>>
>>>>> diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
>>>>> new file mode 100644
>>>>> index 0000000..d99d4a5
>>>>> --- /dev/null
>>>>> +++ b/drivers/media/platform/atmel/atmel-isc.c
>>>>
>>>> <snip>
>>>>
>>>>> +static int isc_set_default_fmt(struct isc_device *isc)
>>>>> +{
>>>>> + u32 index = isc->num_user_formats - 1;
>>>>
>>>> Why pick the last format? Strictly speaking it doesn't matter, but in practice
>>>> the most common formats tend to be at the beginning of the format list.
>>>>
>>>>> + struct v4l2_format f = {
>>>>> + .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
>>>>> + .fmt.pix = {
>>>>> + .width = VGA_WIDTH,
>>>>> + .height = VGA_HEIGHT,
>>>>> + .field = V4L2_FIELD_NONE,
>>>>> + .pixelformat = isc->user_formats[index]->fourcc,
>>>>> + },
>>>>> + };
>>>>> +
>>>>> + return isc_set_fmt(isc, &f);
>>>>> +}
>>>>> +
>>>>> +static int isc_open(struct file *file)
>>>>> +{
>>>>> + struct isc_device *isc = video_drvdata(file);
>>>>> + struct v4l2_subdev *sd = isc->current_subdev->sd;
>>>>> + int ret;
>>>>> +
>>>>> + if (mutex_lock_interruptible(&isc->lock))
>>>>> + return -ERESTARTSYS;
>>>>> +
>>>>> + ret = v4l2_fh_open(file);
>>>>> + if (ret < 0)
>>>>> + goto unlock;
>>>>> +
>>>>> + if (!v4l2_fh_is_singular_file(file))
>>>>> + goto unlock;
>>>>> +
>>>>> + ret = v4l2_subdev_call(sd, core, s_power, 1);
>>>>> + if (ret < 0 && ret != -ENOIOCTLCMD)
>>>>> + goto unlock;
>>>>> +
>>>>> + ret = isc_set_default_fmt(isc);
>>>>
>>>> This doesn't belong here, this needs to be done in isc_async_complete().
>>>>
>>>> Having the code here means that every time you open the device, the format
>>>> changes back to the default. That's not what you want.
>>>
>>> Actually, you do need to set the format here since here is where you turn on
>>> the sensor power, but it should be the current format, not the default format.
>>>
>>> And in isc_set_default_fmt I recommend that you call the try fmt of the subdev
>>> in order to let the subdev adjust the proposed default format. The 'try' doesn't
>>> need to power on the sensor.
>>>
>> I replace the 'set fmt' with 'try fmt', then test with v4l2-compliance,
>> but there are tree fail cases. I think it's reasonable that setting a
>> default fmt on the first open. When app open the device on the first
>> open, it should set a fmt, or the default fmt will be set.
>
> It's not reasonable :-). In V4L2 all format (or any other for that matter) settings
> are persistent.
>
> Anyway, take a look at my atmel-isi driver:
>
> https://git.linuxtv.org/hverkuil/media_tree.git/tree/drivers/media/platform/atmel-isi.c?h=sama5d3
>
> There I use try_fmt as well, and that passes the compliance test just fine.
>
Thank you, I will try again.
> Regards,
>
> Hans
>
Powered by blists - more mailing lists