Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1440753 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4] [media] pci: Add tw5864 driver Joe Perches <joe@perches.com> - 2016-07-11 18:50 +0200
    Re: [PATCH v4] [media] pci: Add tw5864 driver Andrey Utkin <andrey_utkin@fastmail.com> - 2016-07-13 04:00 +0200

#1440753 — Re: [PATCH v4] [media] pci: Add tw5864 driver

On Mon, 2016-07-11 at 18:17 +0300, Andrey Utkin wrote:
[]
> diff --git a/drivers/media/pci/tw5864/tw5864-core.c b/drivers/media/pci/tw5864/tw5864-core.c
[]
> +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";
[]
> +static int tw5864_initdev(struct pci_dev *pci_dev,
> +			  const struct pci_device_id *pci_id)
> +{
[]
> +	dev_warn(&pci_dev->dev, "%s", artifacts_warning);
> +	dev_warn(&pci_dev->dev, "%s", artifacts_warning_continued);

Is all that verbosity useful?

And trivially:

Each of these blocks will start with the dev_<level> prefix
and the subsequent lines will not have the same prefix

Perhaps it'd be better to write this something like:

static const char * const artifacts_warning[] = {
	"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 this command on device files created by this driver:",
	"",
	"v4l2-ctl --device /dev/videoX --set-ctrl=video_gop_size=1",
	"",
	"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).\n"
};

and use

	for (i = 0; i < ARRAY_SIZE(artifacts_warning), i++)
		dev_warn(&pci_dev->dev, %s\n", artifacts_warning[i]);

so that each line is prefixed.

It also might be better to issue something like a single
line dev_warn referring to the driver code and just leave
this comment in the driver sources.

Something like:

	dev_warn(&pci_dev->dev,
		"This driver has known defects in video quality\n");

[toc] | [next] | [standalone]

#1441955

On Mon, Jul 11, 2016 at 09:40:53AM -0700, Joe Perches wrote:
> Each of these blocks will start with the dev_<level> prefix
> and the subsequent lines will not have the same prefix

Yes. I have checked how it looks before submitting, but I didn't see
this as a problem. I don't mind changing that (anyway I have found few
micro-issues with checkpatch --strict and would like to resubmit), but
would like to hear some second opinion.

> It also might be better to issue something like a single
> line dev_warn referring to the driver code and just leave
> this comment in the driver sources.
> 
> Something like:
> 
> 	dev_warn(&pci_dev->dev,
> 		"This driver has known defects in video quality\n");

Things get complicated if you consider mainstream distros and their
years-behind kernels. The simplest way to preserve correspondence
between state of driver and such notice is to contain the notice in the
compiled driver. I hope the state of affairs will change to better
someday :)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web