Re: [PATCH v2] Add tw5864 driver
Hi Hans,
Thanks for great help.
I believe the issues highlighted by your are rectified by now.
One chunk of your proposed changes seems to be wrong.
Also I have one non-technical change I want to introduce to this driver, see it
in the bottom of this letter ("Also, I decided to document known video quality
issues in a printed warning...").
On Fri, Jul 01, 2016 at 03:35:40PM +0200, Hans Verkuil wrote:
> On 06/10/2016 12:11 AM, Andrey Utkin wrote:
> > + cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
>
> This line can be dropped: the v4l2 core will do this automatically.
This seems not so: dropping it resulted in new compliance fails:
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
I am running latest v4l-utils from git.
This particular fail happens on kernels built from next-20160707 and
next-20160609.
BTW next-20160707 makes my dev machine to hang after few minutes of uptime,
regardless of my module being loaded, so for now I am testing driver on
next-20160609.
This (running old linux-next) causes such new fail with latest v4l-utils:
fail: v4l2-test-buffers.cpp(293): g_flags() & V4L2_BUF_FLAG_DONE
which is understandable because of recent commit to v4l-utils flipping expected
behaviour in this regard:
commit 7d784c6894b10cdf5ec025c2cd7c320320f5f658
Author: Hans Verkuil
Date: Fri Jul 8 23:10:34 2016 +0200
v4l2-compliance: fix a check for the DONE flag
This was always set by vb2 drivers due to a bug. It is now cleared
again after that bug was fixed, but the test should now be inverted.
Signed-off-by: Hans Verkuil
diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp
b/utils/v4l2-compliance/v4l2-test-buffers.cpp
index fb14170..dc82918 100644
--- a/utils/v4l2-compliance/v4l2-test-buffers.cpp
+++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
@@ -290,7 +290,7 @@ int buffer::check(unsigned type, unsigned memory, unsigned
index,
fail_on_test(g_bytesused(p) > g_length(p));
}
fail_on_test(!g_timestamp().tv_sec && !g_timestamp().tv_usec);
- fail_on_test(!(g_flags() & V4L2_BUF_FLAG_DONE));
+ fail_on_test(g_flags() & V4L2_BUF_FLAG_DONE);
fail_on_test((int)g_sequence() < seq.last_seq + 1);
if (v4l_type_is_video(g_type())) {
fail_on_test(g_field() == V4L2_FIELD_ALTERNATE);
So please expect this fail in v4l2-compliance logs of my new submission.
Also, I decided to document known video quality issues in a printed warning; I
like how it looks now both in code and in dmesg, but checkpatch.pl doesn't like
it. See commit at
https://github.com/bluecherrydvr/linux/commit/83395b6c5e1e5ceb642c9a04a28db5fc22566c87
The message is split in pieces because otherwise it gets truncated.
I'd like some approval or suggestion for rework on this.
It looks like this in dmesg:
[ 5101.182151] tw5864 :06:07.0: BEWARE OF KNOWN ISSUES WITH VIDEO QUALITY
This driver was developed by Bluecherry LLC by deducing
behaviour of original manufacturer's driver, from both source code and
execution traces.
It is known that there are some artifacts on output video with
this driver:
- on all known hardware samples: random pixels of wrong color
(mostly white, red or blue) appearing and disappearing on sequences of P-frames;
- on some hardware samples (known with H.264 core version
e006:2800): total madness on P-frames: blocks of wrong luminance; blocks of
wrong colors "creeping" across the picture.
There is a workaround for both issues: avoid P-frames by setting
GOP size to 1. To do that, run such command on device files created by this
driver:
for dev in /dev/video*; do v4l2-ctl --device $dev
--set-ctrl=video_gop_size=1; done
[ 5101.357312] systemd-journald[219]: Compressed data object 850 -> 636 using XZ
[ 5101.471071] tw5864 :06:07.0: These issues are not decoding errors; all
produced H.264 streams are decoded properly. Streams without P-frames don't
have these artifacts so it's not analog-to-digital conversion issues nor
internal memory errors; we conclude it's internal H.264 encoder issues.
We cannot even check the original driver's behaviour because it
has never worked properly at all in our development environment. So these
issues may be actually related to firmware or hardware. However it may be that
there's just some more register settings missing in the driver which would
please the hardware.
Manufacturer didn't help much on our inquiries, but feel free to
disturb again the support of Intersil (owner of former Techwell).
And checkpatch says this:
$
Re: [PATCH v2] Add tw5864 driver
On 06/10/2016 12:11 AM, Andrey Utkin wrote:
> Fixed most of issues discovered by v1 and RFCv0 reviewers. Refactored a lot.
>
> The only thing from previous review I haven't got my head around yet is
> framerate control - Hans Verkuil has told to support 1001/3 frame
> interval,
> but it's hard for me to fit it into current model. I see that I'm still not
> getting this aspect right so I'd be grateful for some more spoon-feeding on
> this. Otherwise, it would be nice to get some more reviews while I'm figuring
> this out by myself, so I dare to submit the current state for review.
>
> This submission is awkward in a way. There was a code to "initialize" the
> subordinate chips tw286{4,5} which are connected to main chip via I2C, by
> means
> of special r/w procedure on dedicated register. I was told to organize the
> I2C-related code using kernel-provided interfaces (struct i2c_adapter etc.),
> which I have accomplished. It was tested and traced in runtime, and supposedly
> this new code works. But after that, I figured out that this
> initialize-chips-via-i2c code is not necessary at all. So now there are
> "proper" I2C I/O routines, but they are unused. Also, there remains a very
> similar mechanism which is called "indirect space map" which exposes mostly
> the
> same registers which tw286{4,5} chips expose by themselves and which could be
> accessed also by "i2c communication" routines. But address mapping between
> these spaces seems inconsistent: some addresses match, some don't, and both
> current code and reference driver use mostly "indirect space map" mechanism.
> In
> current code there are 33 invocations of "indirect" i/o. They are left intact
> for now, but if maintainers insist on reworking that to "i2c communication",
> I'm ok to do this.
So if I understand this correctly instead of using i2c you can now use the
indirect i/o to program these internal tw286{4,5} devices? I assume these
are integrated on the same die and the i2c bus is internal to the tw5864 chip?
Assuming this is true, then I would just drop the i2c part since it is unused
and since I suspect that the indirect i/o is faster anyway.
>
> checkpatch.pl gives no notices on a patch nor on individual source files.
>
> sparse ("make C=2 M=drivers/media/pci/tw5864") also gives no notices.
>
> root@localhost:~# v4l2-compliance -d 6 -s
> 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
> 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
Re: [PATCH v2] Add tw5864 driver
Update: added local change - to require fewer DMA buffers; - to require fewer vb2_queue buffers; - don't require vb2_queue buffers to be DMA-capable. https://github.com/bluecherrydvr/linux/commit/410a86b08d230ff2a401ac9f5be3b30f8b29f30d -- 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
