Re: [PATCH v5] [media] pci: Add tw5864 driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Andrey,

Just a few small comments, I'd say the next version should be ready for merging
in 4.9 (it's too late for 4.8).

On 07/13/2016 04:12 PM, Andrey Utkin wrote:
> Changes since v4:
>  - Decrease motion data buffer to bare minimum
>  - Fix all "checkpatch.pl --strict" notices (whitespace)
> 
> Changes since v3:
>  - updated copyright year
>  - Add VB2_DMABUF flag
>  - fix whitespace as suggested
>  - Move dma_sync_single_for_*() calls to tasklet function
>  - Reflow warning messages (add newlines)
>  - amend caps declaration as suggested
> 
> 
> News here!
> 
> Briefly: there's a new hope for fixing artifacts, but please consider accepting
> current submission.
> 
> One nice guy, a user of tw5864-based board, got in touch with me and
> shared different vendor-provided driver. It was obfuscated, but after trivial
> de-obfuscation it seems much clearer than what I had to tinker with during
> these long 1.5 years of development of this driver.
> https://github.com/bluecherrydvr/linux/blob/new_tw5864/drivers/media/pci/Isil5864/tw5864.c
> 
> I don't swear that I'll fix video artifacts soon, though. I will have time to
> tinker with this not sooner than August (and my free time is going to get very
> limited by then). And what our driver does is still very similar to what this
> new driver does, so the difference must be subtle, so please don't hold your
> breath.
> 
> 
> 
> 16:55:49j@zver /j/Sync/src/linux
>  $ ./scripts/checkpatch.pl --strict 0001-media-pci-Add-tw5864-driver.patch 
> total: 0 errors, 0 warnings, 0 checks, 4555 lines checked
> 
> 0001-media-pci-Add-tw5864-driver.patch has no obvious style problems and is ready for submission.
> [OK]
> 16:56:00j@zver /j/Sync/src/linux
>  $ make M=drivers/media/pci/tw5864 C=2
>   CHECK   drivers/media/pci/tw5864/tw5864-core.c
>   CC [M]  drivers/media/pci/tw5864/tw5864-core.o
>   CHECK   drivers/media/pci/tw5864/tw5864-video.c
>   CC [M]  drivers/media/pci/tw5864/tw5864-video.o
>   CHECK   drivers/media/pci/tw5864/tw5864-h264.c
>   CC [M]  drivers/media/pci/tw5864/tw5864-h264.o
>   CHECK   drivers/media/pci/tw5864/tw5864-util.c
>   CC [M]  drivers/media/pci/tw5864/tw5864-util.o
>   LD [M]  drivers/media/pci/tw5864/tw5864.o
>   Building modules, stage 2.
>   MODPOST 1 modules
>   LD [M]  drivers/media/pci/tw5864/tw5864.ko
> [OK]
> 
> 
> root@localhost:/src/linux# v4l2-compliance -d 6 -s
> v4l2-compliance SHA   : 5e74f6a15aa14c01d8319e086d98f33d96a6a04d
> 
> Driver Info:
>         Driver name   : tw5864
>         Card type     : TW5864 Encoder 2
>         Bus info      : PCI:0000:06:05.0
>         Driver version: 4.7.0
>         Capabilities  : 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/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
>                 test VIDIOC_EXPBUF: OK
> 
> Test input 0:
> 
> Streaming ioctls:
>         test read/write: OK
>         test MMAP: OK                                     
>         test USERPTR: OK (Not Supported)
>         test DMABUF: Cannot test, specify --expbuf-device
> 
> 
> Total: 45, Succeeded: 45, Failed: 0, Warnings: 0
> 
> ---8<---
> 
> Support for boards based on Techwell TW5864 chip which provides
> multichannel video & audio grabbing and encoding (H.264, MJPEG,
> ADPCM G.726).
> 
> This submission implements only H.264 encoding of all channels at D1
> resolution.
> 
> Thanks to Mark Thompson <sw@xxxxxxxxx> for help, and for contribution of
> H.264 startcode emulation prevention code.
> 
> Signed-off-by: Andrey Utkin <andrey.utkin@xxxxxxxxxxxxxxxxxxx>
> Tested-by: Andrey Utkin <andrey.utkin@xxxxxxxxxxxxxxxxxxx>
> ---
>  MAINTAINERS                             |    8 +
>  drivers/media/pci/Kconfig               |    1 +
>  drivers/media/pci/Makefile              |    1 +
>  drivers/media/pci/tw5864/Kconfig        |   11 +
>  drivers/media/pci/tw5864/Makefile       |    3 +
>  drivers/media/pci/tw5864/tw5864-core.c  |  361 ++++++
>  drivers/media/pci/tw5864/tw5864-h264.c  |  259 ++++
>  drivers/media/pci/tw5864/tw5864-reg.h   | 2133 +++++++++++++++++++++++++++++++
>  drivers/media/pci/tw5864/tw5864-util.c  |   37 +
>  drivers/media/pci/tw5864/tw5864-video.c | 1521 ++++++++++++++++++++++
>  drivers/media/pci/tw5864/tw5864.h       |  205 +++
>  11 files changed, 4540 insertions(+)
>  create mode 100644 drivers/media/pci/tw5864/Kconfig
>  create mode 100644 drivers/media/pci/tw5864/Makefile
>  create mode 100644 drivers/media/pci/tw5864/tw5864-core.c
>  create mode 100644 drivers/media/pci/tw5864/tw5864-h264.c
>  create mode 100644 drivers/media/pci/tw5864/tw5864-reg.h
>  create mode 100644 drivers/media/pci/tw5864/tw5864-util.c
>  create mode 100644 drivers/media/pci/tw5864/tw5864-video.c
>  create mode 100644 drivers/media/pci/tw5864/tw5864.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a975b8e..46daa99 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11598,6 +11598,14 @@ T:	git git://linuxtv.org/media_tree.git
>  S:	Odd fixes
>  F:	drivers/media/usb/tm6000/
>  
> +TW5864 VIDEO4LINUX DRIVER
> +M:	Bluecherry Maintainers <maintainers@xxxxxxxxxxxxxxxxx>
> +M:	Andrey Utkin <andrey.utkin@xxxxxxxxxxxxxxxxxxx>
> +M:	Andrey Utkin <andrey_utkin@xxxxxxxxxxxx>
> +L:	linux-media@xxxxxxxxxxxxxxx
> +S:	Supported
> +F:	drivers/media/pci/tw5864/
> +
>  TW68 VIDEO4LINUX DRIVER
>  M:	Hans Verkuil <hverkuil@xxxxxxxxx>
>  L:	linux-media@xxxxxxxxxxxxxxx
> diff --git a/drivers/media/pci/Kconfig b/drivers/media/pci/Kconfig
> index 4f6467f..da28e68 100644
> --- a/drivers/media/pci/Kconfig
> +++ b/drivers/media/pci/Kconfig
> @@ -13,6 +13,7 @@ if MEDIA_CAMERA_SUPPORT
>  source "drivers/media/pci/meye/Kconfig"
>  source "drivers/media/pci/solo6x10/Kconfig"
>  source "drivers/media/pci/sta2x11/Kconfig"
> +source "drivers/media/pci/tw5864/Kconfig"
>  source "drivers/media/pci/tw68/Kconfig"
>  source "drivers/media/pci/tw686x/Kconfig"
>  source "drivers/media/pci/zoran/Kconfig"
> diff --git a/drivers/media/pci/Makefile b/drivers/media/pci/Makefile
> index 2e54c36..a7e8af0 100644
> --- a/drivers/media/pci/Makefile
> +++ b/drivers/media/pci/Makefile
> @@ -31,3 +31,4 @@ obj-$(CONFIG_VIDEO_MEYE) += meye/
>  obj-$(CONFIG_STA2X11_VIP) += sta2x11/
>  obj-$(CONFIG_VIDEO_SOLO6X10) += solo6x10/
>  obj-$(CONFIG_VIDEO_COBALT) += cobalt/
> +obj-$(CONFIG_VIDEO_TW5864) += tw5864/
> diff --git a/drivers/media/pci/tw5864/Kconfig b/drivers/media/pci/tw5864/Kconfig
> new file mode 100644
> index 0000000..760fb11
> --- /dev/null
> +++ b/drivers/media/pci/tw5864/Kconfig
> @@ -0,0 +1,11 @@
> +config VIDEO_TW5864
> +	tristate "Techwell TW5864 video/audio grabber and encoder"
> +	depends on VIDEO_DEV && PCI && VIDEO_V4L2
> +	select VIDEOBUF2_DMA_CONTIG
> +	---help---
> +	  Support for boards based on Techwell TW5864 chip which provides
> +	  multichannel video & audio grabbing and encoding (H.264, MJPEG,
> +	  ADPCM G.726).
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called tw5864.
> diff --git a/drivers/media/pci/tw5864/Makefile b/drivers/media/pci/tw5864/Makefile
> new file mode 100644
> index 0000000..4fc8b3b
> --- /dev/null
> +++ b/drivers/media/pci/tw5864/Makefile
> @@ -0,0 +1,3 @@
> +tw5864-objs := tw5864-core.o tw5864-video.o tw5864-h264.o tw5864-util.o
> +
> +obj-$(CONFIG_VIDEO_TW5864) += tw5864.o
> diff --git a/drivers/media/pci/tw5864/tw5864-core.c b/drivers/media/pci/tw5864/tw5864-core.c
> new file mode 100644
> index 0000000..d405dc5
> --- /dev/null
> +++ b/drivers/media/pci/tw5864/tw5864-core.c
> @@ -0,0 +1,361 @@
> +/*
> + *  TW5864 driver - core functions
> + *
> + *  Copyright (C) 2016 Bluecherry, LLC <maintainers@xxxxxxxxxxxxxxxxx>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/kmod.h>
> +#include <linux/sound.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/pm.h>
> +#include <linux/pci_ids.h>
> +#include <linux/jiffies.h>
> +#include <asm/dma.h>
> +#include <media/v4l2-dev.h>
> +
> +#include "tw5864.h"
> +#include "tw5864-reg.h"
> +
> +MODULE_DESCRIPTION("V4L2 driver module for tw5864-based multimedia capture & encoding devices");
> +MODULE_AUTHOR("Bluecherry Maintainers <maintainers@xxxxxxxxxxxxxxxxx>");
> +MODULE_AUTHOR("Andrey Utkin <andrey.utkin@xxxxxxxxxxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> +
> +static const char * const artifacts_warning =
> +"BEWARE OF KNOWN ISSUES WITH VIDEO QUALITY\n"
> +"\n"
> +"This driver was developed by Bluecherry LLC by deducing behaviour of\n"
> +"original manufacturer's driver, from both source code and execution traces.\n"
> +"It is known that there are some artifacts on output video with this driver:\n"
> +" - on all known hardware samples: random pixels of wrong color (mostly\n"
> +"   white, red or blue) appearing and disappearing on sequences of P-frames;\n"
> +" - on some hardware samples (known with H.264 core version e006:2800):\n"
> +"   total madness on P-frames: blocks of wrong luminance; blocks of wrong\n"
> +"   colors \"creeping\" across the picture.\n"
> +"There is a workaround for both issues: avoid P-frames by setting GOP size\n"
> +"to 1. To do that, run this command on device files created by this driver:\n"
> +"\n"
> +"v4l2-ctl --device /dev/videoX --set-ctrl=video_gop_size=1\n"
> +"\n";
> +
> +static char *artifacts_warning_continued =
> +"These issues are not decoding errors; all produced H.264 streams are decoded\n"
> +"properly. Streams without P-frames don't have these artifacts so it's not\n"
> +"analog-to-digital conversion issues nor internal memory errors; we conclude\n"
> +"it's internal H.264 encoder issues.\n"
> +"We cannot even check the original driver's behaviour because it has never\n"
> +"worked properly at all in our development environment. So these issues may\n"
> +"be actually related to firmware or hardware. However it may be that there's\n"
> +"just some more register settings missing in the driver which would please\n"
> +"the hardware.\n"
> +"Manufacturer didn't help much on our inquiries, but feel free to disturb\n"
> +"again the support of Intersil (owner of former Techwell).\n"
> +"\n";

<snip>

> +static int tw5864_initdev(struct pci_dev *pci_dev,
> +			  const struct pci_device_id *pci_id)
> +{
> +	struct tw5864_dev *dev;
> +	int err;
> +
> +	dev = devm_kzalloc(&pci_dev->dev, sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;
> +
> +	snprintf(dev->name, sizeof(dev->name), "tw5864:%s", pci_name(pci_dev));
> +
> +	err = v4l2_device_register(&pci_dev->dev, &dev->v4l2_dev);
> +	if (err)
> +		return err;
> +
> +	/* pci init */
> +	dev->pci = pci_dev;
> +	err = pci_enable_device(pci_dev);
> +	if (err) {
> +		dev_err(&dev->pci->dev, "pci_enable_device() failed\n");
> +		goto unreg_v4l2;
> +	}
> +
> +	pci_set_master(pci_dev);
> +
> +	err = pci_set_dma_mask(pci_dev, DMA_BIT_MASK(32));
> +	if (err) {
> +		dev_err(&dev->pci->dev, "32 bit PCI DMA is not supported\n");
> +		goto disable_pci;
> +	}
> +
> +	/* get mmio */
> +	err = pci_request_regions(pci_dev, dev->name);
> +	if (err) {
> +		dev_err(&dev->pci->dev, "Cannot request regions for MMIO\n");
> +		goto disable_pci;
> +	}
> +	dev->mmio = pci_ioremap_bar(pci_dev, 0);
> +	if (!dev->mmio) {
> +		err = -EIO;
> +		dev_err(&dev->pci->dev, "can't ioremap() MMIO memory\n");
> +		goto release_mmio;
> +	}
> +
> +	spin_lock_init(&dev->slock);
> +
> +	dev_info(&pci_dev->dev, "TW5864 hardware version: %04x\n",
> +		 tw_readl(TW5864_HW_VERSION));
> +	dev_info(&pci_dev->dev, "TW5864 H.264 core version: %04x:%04x\n",
> +		 tw_readl(TW5864_H264REV),
> +		 tw_readl(TW5864_UNDECLARED_H264REV_PART2));
> +
> +	err = tw5864_video_init(dev, video_nr);
> +	if (err)
> +		goto unmap_mmio;
> +
> +	/* get irq */
> +	err = devm_request_irq(&pci_dev->dev, pci_dev->irq, tw5864_isr,
> +			       IRQF_SHARED, "tw5864", dev);
> +	if (err < 0) {
> +		dev_err(&dev->pci->dev, "can't get IRQ %d\n", pci_dev->irq);
> +		goto fini_video;
> +	}
> +
> +	dev_warn(&pci_dev->dev, "%s", artifacts_warning);
> +	dev_warn(&pci_dev->dev, "%s", artifacts_warning_continued);

That's too much kernel log spamming.

I propose that you turn those strings into a comment block at the start of the source
and here you just print:

	dev_info(&pci_dev->dev, "Note: there are known video quality issues. For details\n");
	dev_info(&pci_dev->dev, "see the comment in drivers/media/pci/tw5864/tw5864-core.c.\n");

That looks much better to me.

> +
> +	return 0;
> +
> +fini_video:
> +	tw5864_video_fini(dev);
> +unmap_mmio:
> +	iounmap(dev->mmio);
> +release_mmio:
> +	pci_release_regions(pci_dev);
> +disable_pci:
> +	pci_disable_device(pci_dev);
> +unreg_v4l2:
> +	v4l2_device_unregister(&dev->v4l2_dev);
> +	return err;
> +}
> +

<snip>

> diff --git a/drivers/media/pci/tw5864/tw5864-video.c b/drivers/media/pci/tw5864/tw5864-video.c
> new file mode 100644
> index 0000000..dce0256
> --- /dev/null
> +++ b/drivers/media/pci/tw5864/tw5864-video.c

<snip>

> +static int tw5864_input_std_get(struct tw5864_input *input,
> +				enum tw5864_vid_std *std_arg)
> +{
> +	struct tw5864_dev *dev = input->root;
> +	enum tw5864_vid_std std;
> +	u8 std_reg = tw_indir_readb(TW5864_INDIR_VIN_E(input->nr));
> +
> +	std = (std_reg & 0x70) >> 4;
> +
> +	if (std_reg & 0x80) {
> +		dev_err(&dev->pci->dev,
> +			"Video format detection is in progress, please wait\n");
> +		return -EAGAIN;
> +	}
> +
> +	if (std == STD_INVALID) {
> +		dev_err(&dev->pci->dev, "No valid video format detected\n");

In this case set *std_arg to V4L2_STD_UNKNOWN and just return 0. As per the QUERYSTD spec.

> +		return -EPERM;
> +	}
> +
> +	*std_arg = std;
> +	return 0;
> +}

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux