Re: [PATCH v3] media: platform: add VPFE capture driver support for AM437X

2014-12-05 Thread Hans Verkuil 
Hi Prabhakar,

Sorry, there are still a few items that need to be fixed.
If you can make a v4 with these issues addressed, then I can still make a
pull request, although it depends on Mauro whether it is still accepted for
3.19.

On 12/04/2014 12:12 AM, Lad, Prabhakar wrote:
 From: Benoit Parrot bpar...@ti.com
 
 This patch adds Video Processing Front End (VPFE) driver for
 AM437X family of devices
 Driver supports the following:
 - V4L2 API using MMAP buffer access based on videobuf2 api
 - Asynchronous sensor/decoder sub device registration
 - DT support

Just to confirm: this driver only supports SDTV formats? No HDTV?
I didn't see any VIDIOC_*_DV_TIMINGS support, so I assume it really
isn't supported.

 
 Signed-off-by: Benoit Parrot bpar...@ti.com
 Signed-off-by: Darren Etheridge detheri...@ti.com
 Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com
 ---

snip

 diff --git a/drivers/media/platform/am437x/am437x-vpfe.c 
 b/drivers/media/platform/am437x/am437x-vpfe.c
 new file mode 100644
 index 000..25863e8
 --- /dev/null
 +++ b/drivers/media/platform/am437x/am437x-vpfe.c

snip

 +
 +static int
 +cmp_v4l2_format(const struct v4l2_format *lhs, const struct v4l2_format *rhs)
 +{
 + return lhs-type == rhs-type 
 + lhs-fmt.pix.width == rhs-fmt.pix.width 
 + lhs-fmt.pix.height == rhs-fmt.pix.height 
 + lhs-fmt.pix.pixelformat == rhs-fmt.pix.pixelformat 
 + lhs-fmt.pix.field == rhs-fmt.pix.field 
 + lhs-fmt.pix.colorspace == rhs-fmt.pix.colorspace;

Add a check for pix.ycbcr_enc and pix.quantization.

snip

 +/*
 + * vpfe_release : This function is based on the vb2_fop_release
 + * helper function.
 + * It has been augmented to handle module power management,
 + * by disabling/enabling h/w module fcntl clock when necessary.
 + */
 +static int vpfe_release(struct file *file)
 +{
 + struct vpfe_device *vpfe = video_drvdata(file);
 + int ret;
 +
 + vpfe_dbg(2, vpfe, vpfe_release\n);
 +
 + ret = _vb2_fop_release(file, NULL);

This isn't going to work. _vb2_fop_release calls v4l2_fh_release(), so
the v4l2_fh_is_singular_file(file) will be wrong and you release the fh
once too many.

I would do this:

if (!v4l2_fh_is_singular_file(file))
return vb2_fop_release(file);
mutex_lock(vpfe-lock);
ret = _vb2_fop_release(file, NULL);
vpfe_ccdc_close(vpfe-ccdc, vpfe-pdev);
mutex_unlock(vpfe-lock);
return ret;

 +
 + if (v4l2_fh_is_singular_file(file)) {
 + mutex_lock(vpfe-lock);
 + vpfe_ccdc_close(vpfe-ccdc, vpfe-pdev);
 + v4l2_fh_release(file);
 + mutex_unlock(vpfe-lock);
 + }
 +
 + return ret;
 +}

snip

 +static int vpfe_enum_size(struct file *file, void  *priv,
 +   struct v4l2_frmsizeenum *fsize)
 +{
 + struct vpfe_device *vpfe = video_drvdata(file);
 + struct v4l2_subdev_frame_size_enum fse;
 + struct vpfe_subdev_info *sdinfo;
 + struct v4l2_mbus_framefmt mbus;
 + struct v4l2_pix_format pix;
 + struct vpfe_fmt *fmt;
 + int ret;
 +
 + vpfe_dbg(2, vpfe, vpfe_enum_size\n);
 +
 + /* check for valid format */
 + fmt = find_format_by_pix(fsize-pixel_format);
 + if (!fmt) {
 + vpfe_dbg(3, vpfe, Invalid pixel code: %x, default used 
 instead\n,
 + fsize-pixel_format);
 + return -EINVAL;
 + }
 +
 + memset(fsize-reserved, 0x0, sizeof(fsize-reserved));
 +
 + sdinfo = vpfe-current_subdev;
 + if (!sdinfo-sd)
 + return -EINVAL;
 +
 + memset(pix, 0x0, sizeof(pix));
 + /* Construct pix from parameter and use default for the rest */
 + pix.pixelformat = fsize-pixel_format;
 + pix.width = 640;
 + pix.height = 480;
 + pix.colorspace = V4L2_COLORSPACE_SRGB;
 + pix.field = V4L2_FIELD_NONE;
 + pix_to_mbus(vpfe, pix, mbus);
 +
 + memset(fse, 0x0, sizeof(fse));
 + fse.index = fsize-index;
 + fse.pad = 0;
 + fse.code = mbus.code;
 + ret = v4l2_subdev_call(sdinfo-sd, pad, enum_frame_size, NULL, fse);

FYI: strictly speaking this is wrong since this op theoretically expects a
v4l2_subdev_fh pointer instead of a NULL argument. However, you do not have
an alternative right now. As you know, I've been working on fixing this, so
if that gets accepted, then you need to update this code as well in a later
patch.

 + if (ret)
 + return -EINVAL;
 +
 + vpfe_dbg(1, vpfe, vpfe_enum_size: index: %d code: %x W:[%d,%d] 
 H:[%d,%d]\n,
 + fse.index, fse.code, fse.min_width, fse.max_width,
 + fse.min_height, fse.max_height);
 +
 + fsize-type = V4L2_FRMSIZE_TYPE_DISCRETE;
 + fsize-discrete.width = fse.max_width;
 + fsize-discrete.height = fse.max_height;
 +
 + vpfe_dbg(1, vpfe, vpfe_enum_size: index: %d pixformat: %s size: 
 %dx%d\n,
 + fsize-index, print_fourcc(fsize-pixel_format),
 +

Re: [PATCH v3] media: platform: add VPFE capture driver support for AM437X

2014-12-05 Thread Prabhakar Lad 
Hi Hans,

On Fri, Dec 5, 2014 at 12:24 PM, Hans Verkuil hverk...@xs4all.nl wrote:
 Hi Prabhakar,

 Sorry, there are still a few items that need to be fixed.
 If you can make a v4 with these issues addressed, then I can still make a
 pull request, although it depends on Mauro whether it is still accepted for
 3.19.

OK will post a v4 tonight fixing all the below issues.

FYI: Looking at the response of Mauro on 'soc-camera: 1st set for 3.19'
he wont accept it!

Thanks,
--Prabhakar Lad

 On 12/04/2014 12:12 AM, Lad, Prabhakar wrote:
 From: Benoit Parrot bpar...@ti.com

 This patch adds Video Processing Front End (VPFE) driver for
 AM437X family of devices
 Driver supports the following:
 - V4L2 API using MMAP buffer access based on videobuf2 api
 - Asynchronous sensor/decoder sub device registration
 - DT support

 Just to confirm: this driver only supports SDTV formats? No HDTV?
 I didn't see any VIDIOC_*_DV_TIMINGS support, so I assume it really
 isn't supported.


 Signed-off-by: Benoit Parrot bpar...@ti.com
 Signed-off-by: Darren Etheridge detheri...@ti.com
 Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com
 ---

 snip

 diff --git a/drivers/media/platform/am437x/am437x-vpfe.c 
 b/drivers/media/platform/am437x/am437x-vpfe.c
 new file mode 100644
 index 000..25863e8
 --- /dev/null
 +++ b/drivers/media/platform/am437x/am437x-vpfe.c

 snip

 +
 +static int
 +cmp_v4l2_format(const struct v4l2_format *lhs, const struct v4l2_format 
 *rhs)
 +{
 + return lhs-type == rhs-type 
 + lhs-fmt.pix.width == rhs-fmt.pix.width 
 + lhs-fmt.pix.height == rhs-fmt.pix.height 
 + lhs-fmt.pix.pixelformat == rhs-fmt.pix.pixelformat 
 + lhs-fmt.pix.field == rhs-fmt.pix.field 
 + lhs-fmt.pix.colorspace == rhs-fmt.pix.colorspace;

 Add a check for pix.ycbcr_enc and pix.quantization.

OK

 snip

 +/*
 + * vpfe_release : This function is based on the vb2_fop_release
 + * helper function.
 + * It has been augmented to handle module power management,
 + * by disabling/enabling h/w module fcntl clock when necessary.
 + */
 +static int vpfe_release(struct file *file)
 +{
 + struct vpfe_device *vpfe = video_drvdata(file);
 + int ret;
 +
 + vpfe_dbg(2, vpfe, vpfe_release\n);
 +
 + ret = _vb2_fop_release(file, NULL);

 This isn't going to work. _vb2_fop_release calls v4l2_fh_release(), so
 the v4l2_fh_is_singular_file(file) will be wrong and you release the fh
 once too many.

 I would do this:

 if (!v4l2_fh_is_singular_file(file))
 return vb2_fop_release(file);
 mutex_lock(vpfe-lock);
 ret = _vb2_fop_release(file, NULL);
 vpfe_ccdc_close(vpfe-ccdc, vpfe-pdev);
 mutex_unlock(vpfe-lock);
 return ret;

 +
 + if (v4l2_fh_is_singular_file(file)) {
 + mutex_lock(vpfe-lock);
 + vpfe_ccdc_close(vpfe-ccdc, vpfe-pdev);
 + v4l2_fh_release(file);
 + mutex_unlock(vpfe-lock);
 + }
 +
 + return ret;
 +}

 snip

 +static int vpfe_enum_size(struct file *file, void  *priv,
 +   struct v4l2_frmsizeenum *fsize)
 +{
 + struct vpfe_device *vpfe = video_drvdata(file);
 + struct v4l2_subdev_frame_size_enum fse;
 + struct vpfe_subdev_info *sdinfo;
 + struct v4l2_mbus_framefmt mbus;
 + struct v4l2_pix_format pix;
 + struct vpfe_fmt *fmt;
 + int ret;
 +
 + vpfe_dbg(2, vpfe, vpfe_enum_size\n);
 +
 + /* check for valid format */
 + fmt = find_format_by_pix(fsize-pixel_format);
 + if (!fmt) {
 + vpfe_dbg(3, vpfe, Invalid pixel code: %x, default used 
 instead\n,
 + fsize-pixel_format);
 + return -EINVAL;
 + }
 +
 + memset(fsize-reserved, 0x0, sizeof(fsize-reserved));
 +
 + sdinfo = vpfe-current_subdev;
 + if (!sdinfo-sd)
 + return -EINVAL;
 +
 + memset(pix, 0x0, sizeof(pix));
 + /* Construct pix from parameter and use default for the rest */
 + pix.pixelformat = fsize-pixel_format;
 + pix.width = 640;
 + pix.height = 480;
 + pix.colorspace = V4L2_COLORSPACE_SRGB;
 + pix.field = V4L2_FIELD_NONE;
 + pix_to_mbus(vpfe, pix, mbus);
 +
 + memset(fse, 0x0, sizeof(fse));
 + fse.index = fsize-index;
 + fse.pad = 0;
 + fse.code = mbus.code;
 + ret = v4l2_subdev_call(sdinfo-sd, pad, enum_frame_size, NULL, fse);

 FYI: strictly speaking this is wrong since this op theoretically expects a
 v4l2_subdev_fh pointer instead of a NULL argument. However, you do not have
 an alternative right now. As you know, I've been working on fixing this, so
 if that gets accepted, then you need to update this code as well in a later
 patch.

 + if (ret)
 + return -EINVAL;
 +
 + vpfe_dbg(1, vpfe, vpfe_enum_size: index: %d code: %x W:[%d,%d] 
 H:[%d,%d]\n,
 + fse.index, fse.code, fse.min_width, fse.max_width,
 + fse.min_height, fse.max_height);
 +
 +

[PATCH v3] media: platform: add VPFE capture driver support for AM437X

2014-12-03 Thread Lad, Prabhakar 
From: Benoit Parrot bpar...@ti.com

This patch adds Video Processing Front End (VPFE) driver for
AM437X family of devices
Driver supports the following:
- V4L2 API using MMAP buffer access based on videobuf2 api
- Asynchronous sensor/decoder sub device registration
- DT support

Signed-off-by: Benoit Parrot bpar...@ti.com
Signed-off-by: Darren Etheridge detheri...@ti.com
Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com
---
 Changes for v3:
 a Fixed review comments pointed by Hans.
 
 Following is the v4l2-compliance output:-
 
 
 root@am437x-evm:~# ./v4l2-compliance -s -i 0 -vv
Driver Info:
Driver name   : vpfe
[   99.723462] vpfe 48328000.vpfe: =  START STATUS  
=

Bus info  : platform:vpfe 48328000.vpfe
[   99.735701] vpfe 48328000.vpfe: ==  END STATUS  
==
Driver version: 3.18.0
Capabilities  : 0x85200[   99.748824] vpfe 48328000.vpfe: invalid input 
index: 1
001
Video Capture
Read/Write
Streaming
Extended Pix Format
Device Capabilities
Device Caps   : 0x0521
Video Capture
Read/Write
Streaming
Extended Pix Format

Compliance test for device /dev/video0 (not using libv4l2):

Required ioctls:
test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
test second video open: OK
test VIDIOC_QUERYCAP: OK
test VIDIOC_G/S_PRIORITY: OK

Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
test VIDIOC_LOG_STATUS: OK

Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
test VIDIOC_ENUMAUDIO: OK (Not Supported)
test VIDIOC_G/S/ENUMINPUT: OK
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_ENUMAUDOUT: OK (Not Supported)
test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK
test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
test VIDIOC_G/S_EDID: OK (Not Supported)

Test input 0:

Control ioctls:
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported)
test VIDIOC_QUERYCTRL: OK (Not Supported)
test VIDIOC_G/S_CTRL: OK (Not Supported)
test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
Standard Controls: 0 Private Controls: 0

Format ioctls:
info: found 7 framesizes for pixel format 56595559
info: found 7 framesizes for pixel format 59565955
info: found 7 framesizes for pixel format 52424752
info: found 7 framesizes for pixel format 31384142
info: found 4 formats for buftype 1
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
test VIDIOC_G/S_PARM: OK
test VIDIOC_G_FBUF: OK (Not Supported)
test VIDIOC_G_FMT: OK
test VIDIOC_TRY_FMT: OK
info: Could not perform global format test
test VIDIOC_S_FMT: OK
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)

Codec ioctls:
test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
test VIDIOC_G_ENC_INDEX: OK (Not Supported)
test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
info: test buftype Video Capture
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
test VIDIOC_EXPBUF: OK

Streaming ioctls:
test read/write: OK
Video Capture:
Buffer: 0 Sequence: 0 Field: None Timestamp: 104.346919s
Buffer: 1 Sequence: 1 Field: None Timestamp: 104.369798s
Buffer: 2 Sequence: 2 Field: None Timestamp: 104.392679s
Buffer: 3 Sequence: 3 Field: None Timestamp: 104.598605s
Buffer: 0 Sequence: 4 Field: None Timestamp: 104.621486s
Buffer: 1 Sequence: 5 Field: None Timestamp: 104.644364s
Buffer: 2 Sequence: 6 Field: None Timestamp: 104.667245s
Buffer: 3 Sequence: 7 Field: None Timestamp: 104.690126s
Buffer: 0 Sequence: 8 Field: None Timestamp: 104.713005s