Re: [PATCH v3] Add tw5864 driver

[Hans Verkuil]

On 07/11/2016 01:48 PM, Andrey Utkin wrote:
> Thanks for review Hans!
> 
> On Mon, Jul 11, 2016 at 07:58:38AM +0200, Hans Verkuil wrote:
>>> +" v4l2-ctl --device $dev --set-ctrl=video_gop_size=1; done\n"
>>
>> Replace $dev by /dev/videoX
>>
>> Wouldn't it make more sense to default to this? And show the warning only if
>> P-frames are enabled?
> 
> I believe it's better to leave P-frames on by default. All-I-frames
> stream has huge bitrate. And the pixels artifacts is not very strong,
> it's 0 - 10 bad pixels on picture at same time in our dev environment,
> and probably up to 50 bad pixels max in other environments I know of.
> 
>>> +   dma_sync_single_for_cpu(>pci->dev, cur_frame->vlc.dma_addr,
>>> +   H264_VLC_BUF_SIZE, DMA_FROM_DEVICE);
>>> +   dma_sync_single_for_cpu(>pci->dev, cur_frame->mv.dma_addr,
>>> +   H264_MV_BUF_SIZE, DMA_FROM_DEVICE);
>>
>> This is almost certainly the wrong place. This should probably happen in the
>> tasklet. The tasklet runs after the isr, so by the time the tasklet runs
>> you've already called dma_sync_single_for_device.
> 
> Thanks, moved to tasklet subroutine tw5864_handle_frame().
> 
> I didn't seem to me like dma_sync_single_for_* can take long time or be
> otherwise bad to be done from interrupt context.
> 
>>> +static int tw5864_querycap(struct file *file, void *priv,
>>> +  struct v4l2_capability *cap)
>>> +{
>>> +   struct tw5864_input *input = video_drvdata(file);
>>> +
>>> +   strcpy(cap->driver, "tw5864");
>>> +   snprintf(cap->card, sizeof(cap->card), "TW5864 Encoder %d",
>>> +input->nr);
>>> +   sprintf(cap->bus_info, "PCI:%s", pci_name(input->root->pci));
>>> +   cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_READWRITE |
>>> +   V4L2_CAP_STREAMING;
>>> +   cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
>>
>> This line can be dropped, the core will fill in the capabilities field for 
>> you.
> 
> No, removing this line causes v4l2-compliance failures and also ffmpeg fails 
> to
> play the device.

My fault. I mixed things up. The struct video_device has a new device_caps 
field. That
should be filled in with the caps and then in the function above you can drop 
both
the device_caps and capabitlities assignments.

Sorry about that.

Regards,

Hans

> 
> Required ioctls:
> fail: v4l2-compliance.cpp(550): dcaps & ~caps
> test VIDIOC_QUERYCAP: FAIL
> 
> Allow for multiple opens:
> test second video open: OK
> fail: v4l2-compliance.cpp(550): dcaps & ~caps
> test VIDIOC_QUERYCAP: FAIL
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] Add tw5864 driver

[Andrey Utkin]

Thanks for review Hans!

On Mon, Jul 11, 2016 at 07:58:38AM +0200, Hans Verkuil wrote:
> > +" v4l2-ctl --device $dev --set-ctrl=video_gop_size=1; done\n"
> 
> Replace $dev by /dev/videoX
> 
> Wouldn't it make more sense to default to this? And show the warning only if
> P-frames are enabled?

I believe it's better to leave P-frames on by default. All-I-frames
stream has huge bitrate. And the pixels artifacts is not very strong,
it's 0 - 10 bad pixels on picture at same time in our dev environment,
and probably up to 50 bad pixels max in other environments I know of.

> > +   dma_sync_single_for_cpu(>pci->dev, cur_frame->vlc.dma_addr,
> > +   H264_VLC_BUF_SIZE, DMA_FROM_DEVICE);
> > +   dma_sync_single_for_cpu(>pci->dev, cur_frame->mv.dma_addr,
> > +   H264_MV_BUF_SIZE, DMA_FROM_DEVICE);
> 
> This is almost certainly the wrong place. This should probably happen in the
> tasklet. The tasklet runs after the isr, so by the time the tasklet runs
> you've already called dma_sync_single_for_device.

Thanks, moved to tasklet subroutine tw5864_handle_frame().

I didn't seem to me like dma_sync_single_for_* can take long time or be
otherwise bad to be done from interrupt context.

> > +static int tw5864_querycap(struct file *file, void *priv,
> > +  struct v4l2_capability *cap)
> > +{
> > +   struct tw5864_input *input = video_drvdata(file);
> > +
> > +   strcpy(cap->driver, "tw5864");
> > +   snprintf(cap->card, sizeof(cap->card), "TW5864 Encoder %d",
> > +input->nr);
> > +   sprintf(cap->bus_info, "PCI:%s", pci_name(input->root->pci));
> > +   cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_READWRITE |
> > +   V4L2_CAP_STREAMING;
> > +   cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
> 
> This line can be dropped, the core will fill in the capabilities field for 
> you.

No, removing this line causes v4l2-compliance failures and also ffmpeg fails to
play the device.

Required ioctls:
fail: v4l2-compliance.cpp(550): dcaps & ~caps
test VIDIOC_QUERYCAP: FAIL

Allow for multiple opens:
test second video open: OK
fail: v4l2-compliance.cpp(550): dcaps & ~caps
test VIDIOC_QUERYCAP: FAIL
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] Add tw5864 driver

[Jiri Slaby]

On 07/11/2016, 07:58 AM, Hans Verkuil wrote:
>> +static char *artifacts_warning = "BEWARE OF KNOWN ISSUES WITH VIDEO 
>> QUALITY\n"
> 
> const char * const

Or even better, drop the extra space for pointer:

static const char artifacts_warning[] =

thanks,
-- 
js
suse labs
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] Add tw5864 driver

[Hans Verkuil]

Hi Andrey,

Thanks for this driver. Some review comments below:

On 07/09/2016 09:46 PM, Andrey Utkin wrote:
> From: Andrey Utkin 
> 
> 
> Changes in v3 since v2:
>  - Kconfig: select VIDEOBUF2_DMA_CONTIG, not SG
>  - drop i2c code as unused
>  - Dropped num_buffers check in queue_setup as suggested by Hans
>  - Drop std autodetect on streaming start as suggested by Hans
>  - Cleanup buf queue on enable_input() failure
>  - Change container_of() to list_entry() where applicable
>  - Changed V4L2_FIELD_NONE to V4L2_FIELD_INTERLACED as suggested
>  - frameinterval rework suggested by Hans
>  - Add enum_framesizes
>  - Report framesize based on std, not input w/h
>  - Add printed warning about known video quality issues
>  - MAINTAINERS: fix path
>  - request_mem_region() -> pci_request_regions()
>  - Rebase onto Hans' "for-v4.8i" branch
>  - Follow changes from patchset "vb2: replace allocation context by device 
> pointer"
> 
> Below log is produced by today's v4l2-compliance from v4l-utils git, and this
> patch based on branch "for-v4.8i" of 
> git://linuxtv.org/hverkuil/media_tree.git .
> Compliance test runs fine.
> 
> checkpatch.pl is happy with this patch except for artifacts_warning which
> produces a bunch of such warnings:
> 
> WARNING: quoted string split across lines
> #155: FILE: drivers/media/pci/tw5864/tw5864-core.c:44:
> +"This driver was developed by Bluecherry LLC by deducing behaviour of 
> original"
> +" manufacturer's driver, from both source code and execution traces.\n"
> 
> I believe I'd better not join all the lines to avoid them looking like
> 
> "Paragraph one.\Paragraph two.\Paragraph three.\n"
> 
>  # v4l2-compliance -d 6 -s
> v4l2-compliance SHA   : 5e74f6a15aa14c01d8319e086d98f33d96a6a04d
> 
> Driver Info:
> Driver name   : tw5864
> Card type : TW5864 Encoder 2
> Bus info  : PCI::06:05.0
> Driver version: 4.7.0
> Capabilities  : 0x8521
> 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/video6 (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
> test VIDIOC_QUERYCTRL: OK
> test VIDIOC_G/S_CTRL: OK
> test VIDIOC_G/S/TRY_EXT_CTRLS: OK
> test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
> test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
> Standard Controls: 11 Private Controls: 0
> 
> Format ioctls:
> 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
> test VIDIOC_S_FMT: OK
> test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
> test Cropping: OK (Not Supported)
> test Composing: OK (Not Supported)
> test Scaling: 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
>