Re: [PATCH v4 1/2] media: davinci: vpif capture: upgrade the driver with v4l offerings

[Prabhakar Lad]

Hi Hans,

Thanks for the review.

On Mon, May 12, 2014 at 3:20 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> Hi Prabhakar,
>
> Thanks for the patch, but I have a few comments...
>
> On 05/12/2014 10:58 AM, Lad, Prabhakar wrote:
>> From: "Lad, Prabhakar" <prabhakar.csengg@xxxxxxxxx>
>>
>> This patch upgrades the vpif display driver with
>> v4l helpers, this patch does the following,
>>
>> 1: initialize the vb2 queue and context at the time of probe
>> and removes context at remove() callback.
>> 2: uses vb2_ioctl_*() helpers.
>> 3: uses vb2_fop_*() helpers.
>> 4: uses SIMPLE_DEV_PM_OPS.
>> 5: uses vb2_ioctl_*() helpers.
>> 6: vidioc_g/s_priority is now handled by v4l core.
>> 7: removed driver specific fh and now using one provided by v4l.
>> 8: fixes checkpatch warnings.
>
> This really packs too much in one patch. At the very least 1, 4, 6 and
> 8 can be done in separate patches. 2 and 5 are duplicates, BTW. The way
> it is now makes this hard to review. So it would be much appreciated if
> you can split it up.
>
Ok, I have started working on splitting them up.

>>
>> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx>
>> ---
>>
>> root@da850-omapl138-evm:/usr# ./v4l2-compliance -d /dev/video0 -i -s -v
>> Driver Info:
>>         Driver name   : vpif_capture
>> vpif_capture vpif_capture: =================  START STATUS  =================
>>
>>         Bus info      : platform:vpif_capture
>>         Drivervpif_capture vpif_capture: ==================  END STATUS  ==================
>>  version: 3.15.0
>>         Capabilities  : 0x84000001
>>                 Video Capture
>>                 Streaming
>>                 Device Capabilities
>>         Device Caps   : 0x04000001
>>                 Video Capture
>>                 Streaming
>>
>> 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: 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_QUERYCTRL/MENU: 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 1 formats for buftype 1
>>                 test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
>>                 fail: v4l2-test-formats.cpp(1003): cap->readbuffers
>
> Just set readbuffers to the minimum number of buffers that queue_setup uses.
>

OK.

>>                 test VIDIOC_G/S_PARM: FAIL
>>                 test VIDIOC_G_FBUF: OK (Not Supported)
>>                 fail: v4l2-test-formats.cpp(405): !pix.sizeimage
>
> Highly dubious. You need to investigate this!
>
Fixed it.

>>                 test VIDIOC_G_FMT: FAIL
>>                 test VIDIOC_TRY_FMT: OK (Not Supported)
>>                 test VIDIOC_S_FMT: OK (Not Supported)
>>                 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:
>>         test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
>>                 fail: v4l2-test-buffers.cpp(506): q.has_expbuf()
>
> This is weird. I'm not sure why this happens since you seem to have VB2_DMABUF set
> and also vb2_ioctl_expbuf.
>
>>         test VIDIOC_EXPBUF: FAIL
>>
>> Total: 38, Succeeded: 35, Failed: 3, Warnings: 0
>
> Also test with 'v4l2-compliance -s' (streaming). The '-i' option is available to
> test streaming from a specific input.
>
BTW the output is with -s option set.

Thanks,
--Prabhakar Lad
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/