Hi Hans,
On Tue, Dec 2, 2014 at 7:26 AM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> On 12/01/2014 11:17 PM, Prabhakar Lad wrote:
>> Hi Hans,
>>
>> Thanks for the review.
>>
>> On Mon, Dec 1, 2014 at 10:11 AM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>>> Hi all,
>>>
>>> Thanks for the patch, review comments are below.
>>>
>>> For the next version I would appreciate if someone can test this driver with
>>> the latest v4l2-compliance from the v4l-utils git repo. If possible test streaming
>>> as well (v4l2-compliance -s), ideally with both a sensor and a STD receiver input.
>>> But that depends on the available hardware of course.
>>>
>>> I'd like to see the v4l2-compliance output. It's always good to have that on
>>> record.
>>>
>> Following is the compliance output, missed it post it along with patch:
>>
>> root@am437x-evm:~# ./v4l2-compliance -s -i 0 -v
>> Driver Info:
>> Driver name : vpfe
>> Card type :[ 70.363908] vpfe 48326000.vpfe:
>> ================= START STATUS =================
>> TI AM437x VPFE
>> Bus info : platform:vpfe [ 70.375576] vpfe
>> 48326000.vpfe: ================== END STATUS ==================
>> 48326000.vpfe
>> Driver version: 3.18.0
>> Capabil[ 70.388485] vpfe 48326000.vpfe: invalid input index: 1
>> ities : 0x85200001
>> Video Capture
>> Read/Write
>> Streaming
>> Extended Pix Format
>> Device Capabilities
>> Device Caps : 0x05200001
>> 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: Global format check succeeded for type 1
>> 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: 74.956437s
>> Buffer: 1 Sequence: 0 Field: None Timestamp: 74.979310s
>> Buffer: 2 Sequence: 0 Field: None Timestamp: 75.002191s
>> Buffer: 3 Sequence: 0 Field: None Timestamp: 75.208114s
>> Buffer: 0 Sequence: 0 Field: None Timestamp: 75.230998s
>
> Hmm, sequence is always 0. Is the sequence field set? And why doesn't
> v4l2-compliance fail on this?
>
>> Buffer: 1 Sequence: 0 Field: None Timestamp: 75.253877s
>
> <snip>
>
>> test MMAP: OK
>> test USERPTR: OK (Not Supported)
>> test DMABUF: Cannot test, specify --expbuf-device
>>
>> Total: 42, Succeeded: 42, Failed: 0, Warnings: 0
>>>
>> [Snip]
>
>>>> +static int vpfe_enum_fmt(struct file *file, void *priv,
>>>> + struct v4l2_fmtdesc *f)
>>>> +{
>>>> + struct vpfe_device *vpfe = video_drvdata(file);
>>>> + struct v4l2_subdev_mbus_code_enum mbus_code;
>>>> + struct vpfe_subdev_info *sdinfo;
>>>> + struct vpfe_fmt *fmt;
>>>> + int ret;
>>>> +
>>>> + vpfe_dbg(2, vpfe, "vpfe_enum_format index:%d\n",
>>>> + f->index);
>>>> +
>>>> + sdinfo = vpfe->current_subdev;
>>>> + if (!sdinfo->sd)
>>>> + return -EINVAL;
>>>> +
>>>> + mbus_code.index = f->index;
>>>> + ret = v4l2_subdev_call(sdinfo->sd, pad, enum_mbus_code,
>>>> + NULL, &mbus_code);
>>>> + if (ret)
>>>> + return -EINVAL;
>>>> +
>>>> + /* convert mbus_format to v4l2_format */
>>>> + fmt = find_format_by_code(mbus_code.code);
>>>> + if (!fmt) {
>>>> + vpfe_err(vpfe, "mbus code 0x%08x not found\n",
>>>> + mbus_code.code);
>>>> + return -EINVAL;
>>>> + }
>>>
>>> Hmm. If a subdev supports more media bus codes then are supported by this
>>> driver, then the enumeration will stop as soon as such an unsupported code
>>> is found.
>>>
>>> What you want to do here is enumerate over the pixelformats that are supported
>>> by both this driver and the subdev. It is probably something you want to
>>> determine at the time the subdev is loaded and just mark unsupported formats
>>> at that time so that they can be skipped here.
>>>
>> So probably populate the supported pixelformats in s_input call ,
>> by calling enm_mbus_code ?
>
> I would do this during driver initialization, it's a one time thing that can
> be done there.
>
OK will do it when subdev is registered.
Thanks,
--Prabhakar Lad
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
