[RFC,WIP,v4,07/11] media: vidtv: add MPEG TS common code

  From: "Daniel W. S. Almeida" <dwlsalmeida@gmail.com>

Add code to work with MPEG TS packets, such as TS headers, adaptation
fields, PCR packets and NULL packets.

Signed-off-by: Daniel W. S. Almeida <dwlsalmeida@gmail.com>
---
 drivers/media/test-drivers/vidtv/Makefile   |   2 +-
 drivers/media/test-drivers/vidtv/vidtv_ts.c | 130 ++++++++++++++++++++
 drivers/media/test-drivers/vidtv/vidtv_ts.h | 103 ++++++++++++++++
 3 files changed, 234 insertions(+), 1 deletion(-)
 create mode 100644 drivers/media/test-drivers/vidtv/vidtv_ts.c
 create mode 100644 drivers/media/test-drivers/vidtv/vidtv_ts.h
  

Em Sat,  2 May 2020 00:22:12 -0300
"Daniel W. S. Almeida" <dwlsalmeida@gmail.com> escreveu:

> From: "Daniel W. S. Almeida" <dwlsalmeida@gmail.com>
> 
> Add code to work with MPEG TS packets, such as TS headers, adaptation
> fields, PCR packets and NULL packets.
> 
> Signed-off-by: Daniel W. S. Almeida <dwlsalmeida@gmail.com>
> ---
>  drivers/media/test-drivers/vidtv/Makefile   |   2 +-
>  drivers/media/test-drivers/vidtv/vidtv_ts.c | 130 ++++++++++++++++++++
>  drivers/media/test-drivers/vidtv/vidtv_ts.h | 103 ++++++++++++++++
>  3 files changed, 234 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/media/test-drivers/vidtv/vidtv_ts.c
>  create mode 100644 drivers/media/test-drivers/vidtv/vidtv_ts.h
> 
> diff --git a/drivers/media/test-drivers/vidtv/Makefile b/drivers/media/test-drivers/vidtv/Makefile
> index 9ea9485d42189..92001bc348615 100644
> --- a/drivers/media/test-drivers/vidtv/Makefile
> +++ b/drivers/media/test-drivers/vidtv/Makefile
> @@ -1,6 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
>  vidtv_demod-objs := vidtv_common.o
> -vidtv_bridge-objs := vidtv_common.o
> +vidtv_bridge-objs := vidtv_common.o vidtv_ts.o
>  
>  obj-$(CONFIG_DVB_VIDTV)	+= vidtv_tuner.o vidtv_demod.o vidtv_bridge.o
> diff --git a/drivers/media/test-drivers/vidtv/vidtv_ts.c b/drivers/media/test-drivers/vidtv/vidtv_ts.c
> new file mode 100644
> index 0000000000000..f545c45c0fe7c
> --- /dev/null
> +++ b/drivers/media/test-drivers/vidtv/vidtv_ts.c
> @@ -0,0 +1,130 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * The Virtual DVB test driver serves as a reference DVB driver and helps
> + * validate the existing APIs in the media subsystem. It can also aid
> + * developers working on userspace applications.
> + *
> + * Written by Daniel W. S. Almeida <dwlsalmeida@gmail.com>
> + */
> +
> +#include <linux/types.h>
> +#include <asm/byteorder.h>
> +#include "vidtv_ts.h"
> +#include "vidtv_common.h"
> +
> +static u32 vidtv_ts_write_pcr_bits(u8 *buf, u64 pcr)
> +{
> +	/* Exact same from ffmpeg. PCR is a counter driven by a 27Mhz clock */
> +	u64 pcr_low = pcr % 300, pcr_high = pcr / 300;
> +
> +	*buf++ = pcr_high >> 25;
> +	*buf++ = pcr_high >> 17;
> +	*buf++ = pcr_high >>  9;
> +	*buf++ = pcr_high >>  1;
> +	*buf++ = pcr_high <<  7 | pcr_low >> 8 | 0x7e;
> +	*buf++ = pcr_low;
> +
> +	return 6;
> +}
> +
> +void vidtv_ts_inc_cc(u8 *continuity_counter)
> +{
> +	++*continuity_counter;
> +	if (*continuity_counter > TS_CC_MAX_VAL)
> +		*continuity_counter = 0;
> +}
> +
> +u32 vidtv_ts_null_write_into(struct null_packet_write_args args)
> +{
> +	u32    nbytes                  = 0;
> +	struct vidtv_mpeg_ts ts_header = {0};
> +
> +	ts_header.sync_byte          = TS_SYNC_BYTE;
> +	ts_header.pid                = TS_NULL_PACKET_PID;
> +	ts_header.payload            = 1;
> +	ts_header.continuity_counter = *args.continuity_counter;
> +
> +	cpu_to_be16s(&ts_header.bitfield);
> +
> +	/* copy TS header */
> +	nbytes += vidtv_memcpy(args.dest_buf + args.dest_offset + nbytes,
> +			       &ts_header,
> +			       sizeof(ts_header),
> +			       args.dest_offset + nbytes,
> +			       args.buf_sz);

Hmm... now I see why you're returning 0 to vidtv_memcpy().

Yet, if the buffer is full, you should just drop the entire package,
as otherwise you may end copying things that aren't multiple of 188 bytes,
causing sync issues at the client.

> +
> +	be16_to_cpus(&ts_header.bitfield);
> +
> +	vidtv_ts_inc_cc(args.continuity_counter);
> +
> +	/* fill the rest with empty data */
> +	nbytes += vidtv_memset(args.dest_buf + args.dest_offset + nbytes,
> +			       0xff,
> +			       TS_PACKET_LEN - nbytes,
> +			       args.dest_offset + nbytes,
> +			       args.buf_sz);
> +
> +	/* we should have written exactly _one_ 188byte packet */
> +	WARN_ON(nbytes != TS_PACKET_LEN);

A WARN_ON() seems too severe here. Also, if something bad happens, it
will end causing lots of problems that can make the machine very slow,
ad this will flood dmesg.

So, the best would be to use, instead, dev_warn_ratelimited().

PS.: same notes here apply to the function below (and on next patches).

> +
> +	return nbytes;
> +}
> +
> +u32 vidtv_ts_pcr_write_into(struct pcr_write_args args)
> +{
> +	u32    nbytes                         = 0;
> +	struct vidtv_mpeg_ts ts_header        = {0};
> +	struct vidtv_mpeg_ts_adaption ts_adap = {0};
> +
> +	ts_header.sync_byte     = TS_SYNC_BYTE;
> +	ts_header.tei           = 0;
> +	ts_header.payload_start = 0;
> +	ts_header.pid           = args.pid;
> +	ts_header.priority      = 0;
> +	ts_header.scrambling    = 0;
> +	/* cc is not incremented, see 13818-1 clause 2.4.3.3 */
> +	ts_header.continuity_counter = *args.continuity_counter;
> +	ts_header.payload            = 0;
> +	ts_header.adaptation_field   = 1;
> +
> +	/* 13818-1 clause 2.4.3.5 */
> +	ts_adap.length = 183;
> +	ts_adap.PCR    = 1;
> +
> +	cpu_to_be16s(&ts_header.bitfield);
> +
> +	/* copy TS header */
> +	nbytes += vidtv_memcpy(args.dest_buf + args.dest_offset + nbytes,
> +			       &ts_header,
> +			       sizeof(ts_header),
> +			       args.dest_offset + nbytes,
> +			       args.buf_sz);
> +
> +	be16_to_cpus(&ts_header.bitfield);
> +
> +	/* write the adap after the TS header */
> +	nbytes += vidtv_memcpy(args.dest_buf + args.dest_offset + nbytes,
> +			       &ts_adap,
> +			       sizeof(ts_adap),
> +			       args.dest_offset + nbytes,
> +			       args.buf_sz);
> +
> +	/* write the PCR optional */
> +	cpu_to_be64s(&args.pcr);
> +	nbytes += vidtv_ts_write_pcr_bits(args.dest_buf +
> +					  args.dest_offset +
> +					  nbytes,
> +					  args.pcr);
> +	be64_to_cpus(&args.pcr);
> +
> +	nbytes += vidtv_memset(args.dest_buf + args.dest_offset + nbytes,
> +			       0xff,
> +			       TS_PACKET_LEN - nbytes,
> +			       args.dest_offset + nbytes,
> +			       args.buf_sz);
> +
> +	/* we should have written exactly _one_ 188byte packet */
> +	WARN_ON(nbytes != TS_PACKET_LEN);
> +
> +	return nbytes;
> +}
> diff --git a/drivers/media/test-drivers/vidtv/vidtv_ts.h b/drivers/media/test-drivers/vidtv/vidtv_ts.h
> new file mode 100644
> index 0000000000000..2c07bddc46119
> --- /dev/null
> +++ b/drivers/media/test-drivers/vidtv/vidtv_ts.h
> @@ -0,0 +1,103 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * The Virtual DVB test driver serves as a reference DVB driver and helps
> + * validate the existing APIs in the media subsystem. It can also aid
> + * developers working on userspace applications.
> + *
> + * Written by Daniel W. S. Almeida <dwlsalmeida@gmail.com>
> + */
> +
> +#ifndef VIDTV_TS_H
> +#define VIDTV_TS_H
> +
> +#include <linux/types.h>
> +#include <asm/byteorder.h>
> +
> +#define TS_SYNC_BYTE 0x47
> +#define TS_PACKET_LEN 188
> +#define TS_PAYLOAD_LEN 184
> +#define TS_NULL_PACKET_PID 0x1fff
> +#define TS_CC_MAX_VAL 0x0f /* 4 bits */
> +#define TS_LAST_VALID_PID 8191
> +
> +struct vidtv_mpeg_ts_adaption {
> +	u8 length;
> +	struct {
> +#if defined(__LITTLE_ENDIAN_BITFIELD)
> +		u8 extension:1;
> +		u8 private_data:1;
> +		u8 splicing_point:1;
> +		u8 OPCR:1;
> +		u8 PCR:1;
> +		u8 priority:1;
> +		u8 random_access:1;
> +		u8 discontinued:1;
> +#elif defined(__BIG_ENDIAN_BITFIELD)
> +		u8 discontinued:1;
> +		u8 random_access:1;
> +		u8 priority:1;
> +		u8 PCR:1;
> +		u8 OPCR:1;
> +		u8 splicing_point:1;
> +		u8 private_data:1;
> +		u8 extension:1;
> +#else
> +#error  "Please fix <asm/byteorder.h>"
> +#endif
> +	} __packed;
> +	u8 data[];
> +} __packed;
> +
> +struct vidtv_mpeg_ts {
> +	u8 sync_byte;
> +	union {
> +		u16 bitfield;
> +		struct {
> +			u16 pid:13;
> +			u16 priority:1;
> +			u16 payload_start:1;
> +			u16 tei:1;
> +		} __packed;
> +	} __packed;
> +	struct {
> +#if defined(__LITTLE_ENDIAN_BITFIELD)
> +		u8 continuity_counter:4;
> +		u8 payload:1;
> +		u8 adaptation_field:1;
> +		u8 scrambling:2;
> +#elif defined(__BIG_ENDIAN_BITFIELD)
> +		u8 scrambling:2;
> +		u8 adaptation_field:1;
> +		u8 payload:1;
> +		u8 continuity_counter:4;
> +#else
> +#error  "Please fix <asm/byteorder.h>"
> +#endif
> +	} __packed;
> +	struct vidtv_mpeg_ts_adaption adaption[];
> +} __packed;
> +
> +struct pcr_write_args {
> +	void *dest_buf; /* The buffer to write into */
> +	u32 dest_offset; /* The byte offset into the buffer */
> +	u16 pid; /* the TS PID for the PCR packets */
> +	u32 buf_sz; /* protect against overflow when this field is not zero */
> +	u8 *continuity_counter;
> +	u64 pcr; /* A sample from the system clock */
> +};
> +
> +struct null_packet_write_args {
> +	void *dest_buf;/* The buffer to write into */
> +	u32 dest_offset;/* The byte offset into the buffer */
> +	u32 buf_sz; /* protect against overflow when this field is not zero */
> +	u8 *continuity_counter;
> +};
> +
> +/* Increment the continuity counter */
> +void vidtv_ts_inc_cc(u8 *continuity_counter);
> +
> +u32 vidtv_ts_null_write_into(struct null_packet_write_args args);
> +
> +u32 vidtv_ts_pcr_write_into(struct pcr_write_args args);
> +
> +#endif //VIDTV_TS_H



Thanks,
Mauro
  

Hi Mauro, thanks for reviewing this!

 > Em Sat,  2 May 2020 00:22:12 -0300
 > "Daniel W. S. Almeida" <dwlsalmeida@gmail.com> escreveu:
 >
 >> From: "Daniel W. S. Almeida" <dwlsalmeida@gmail.com>
 >>
 >> Add code to work with MPEG TS packets, such as TS headers, adaptation
 >> fields, PCR packets and NULL packets.
 >>
 >> Signed-off-by: Daniel W. S. Almeida <dwlsalmeida@gmail.com>
 >> ---
 >>   drivers/media/test-drivers/vidtv/Makefile   |   2 +-
 >>   drivers/media/test-drivers/vidtv/vidtv_ts.c | 130 ++++++++++++++++++++
 >>   drivers/media/test-drivers/vidtv/vidtv_ts.h | 103 ++++++++++++++++
 >>   3 files changed, 234 insertions(+), 1 deletion(-)
 >>   create mode 100644 drivers/media/test-drivers/vidtv/vidtv_ts.c
 >>   create mode 100644 drivers/media/test-drivers/vidtv/vidtv_ts.h
 >>
 >> diff --git a/drivers/media/test-drivers/vidtv/Makefile 
b/drivers/media/test-drivers/vidtv/Makefile
 >> index 9ea9485d42189..92001bc348615 100644
 >> --- a/drivers/media/test-drivers/vidtv/Makefile
 >> +++ b/drivers/media/test-drivers/vidtv/Makefile
 >> @@ -1,6 +1,6 @@
 >>   # SPDX-License-Identifier: GPL-2.0
 >>
 >>   vidtv_demod-objs := vidtv_common.o
 >> -vidtv_bridge-objs := vidtv_common.o
 >> +vidtv_bridge-objs := vidtv_common.o vidtv_ts.o
 >>
 >>   obj-$(CONFIG_DVB_VIDTV)	+= vidtv_tuner.o vidtv_demod.o vidtv_bridge.o
 >> diff --git a/drivers/media/test-drivers/vidtv/vidtv_ts.c 
b/drivers/media/test-drivers/vidtv/vidtv_ts.c
 >> new file mode 100644
 >> index 0000000000000..f545c45c0fe7c
 >> --- /dev/null
 >> +++ b/drivers/media/test-drivers/vidtv/vidtv_ts.c
 >> @@ -0,0 +1,130 @@
 >> +// SPDX-License-Identifier: GPL-2.0
 >> +/*
 >> + * The Virtual DVB test driver serves as a reference DVB driver and 
helps
 >> + * validate the existing APIs in the media subsystem. It can also aid
 >> + * developers working on userspace applications.
 >> + *
 >> + * Written by Daniel W. S. Almeida <dwlsalmeida@gmail.com>
 >> + */
 >> +
 >> +#include <linux/types.h>
 >> +#include <asm/byteorder.h>
 >> +#include "vidtv_ts.h"
 >> +#include "vidtv_common.h"
 >> +
 >> +static u32 vidtv_ts_write_pcr_bits(u8 *buf, u64 pcr)
 >> +{
 >> +	/* Exact same from ffmpeg. PCR is a counter driven by a 27Mhz clock */
 >> +	u64 pcr_low = pcr % 300, pcr_high = pcr / 300;
 >> +
 >> +	*buf++ = pcr_high >> 25;
 >> +	*buf++ = pcr_high >> 17;
 >> +	*buf++ = pcr_high >>  9;
 >> +	*buf++ = pcr_high >>  1;
 >> +	*buf++ = pcr_high <<  7 | pcr_low >> 8 | 0x7e;
 >> +	*buf++ = pcr_low;
 >> +
 >> +	return 6;
 >> +}
 >> +
 >> +void vidtv_ts_inc_cc(u8 *continuity_counter)
 >> +{
 >> +	++*continuity_counter;
 >> +	if (*continuity_counter > TS_CC_MAX_VAL)
 >> +		*continuity_counter = 0;
 >> +}
 >> +
 >> +u32 vidtv_ts_null_write_into(struct null_packet_write_args args)
 >> +{
 >> +	u32    nbytes                  = 0;
 >> +	struct vidtv_mpeg_ts ts_header = {0};
 >> +
 >> +	ts_header.sync_byte          = TS_SYNC_BYTE;
 >> +	ts_header.pid                = TS_NULL_PACKET_PID;
 >> +	ts_header.payload            = 1;
 >> +	ts_header.continuity_counter = *args.continuity_counter;
 >> +
 >> +	cpu_to_be16s(&ts_header.bitfield);
 >> +
 >> +	/* copy TS header */
 >> +	nbytes += vidtv_memcpy(args.dest_buf + args.dest_offset + nbytes,
 >> +			       &ts_header,
 >> +			       sizeof(ts_header),
 >> +			       args.dest_offset + nbytes,
 >> +			       args.buf_sz);
 >
 > Hmm... now I see why you're returning 0 to vidtv_memcpy().
 >
 > Yet, if the buffer is full, you should just drop the entire package,
 > as otherwise you may end copying things that aren't multiple of 188 
bytes,
 > causing sync issues at the client.

I'd like to provide a counterargument for this.

The way I am dealing with errors throughout vidtv so far is:
If we hit any of these WARN_ON macros, pr_err and the like, then all 
bets are off. This means that the resulting stream will likely be 
invalid and that something needs to be rewritten in the source code and 
my main concern is then preventing the whole driver from crashing.

This is exactly the behavior that you see in vidtv_memcpy and 
vidtv_memset: nothing gets written so we don't end up with an overflow, 
a diagnostic message is printed and there are no attempts at recovery.

In this particular example, I compromised by allowing the size of the 
buffer to be a module param, i.e.

 >> +static unsigned int ts_buf_sz = 20 * 188;
 >> +module_param(ts_buf_sz, uint, 0644);
 >> +MODULE_PARM_DESC(ts_buf_sz, "Optional size for the TS buffer");

I think that what I am trying to say is better seen in the last patch 
for this series: [RFC, WIP, v4 11/11] media: vidtv: Add a MPEG Transport 
Stream Multiplexer.

The following takes place in vidtv_mux.c:

	1. We wake up from sleep, take note of how long we slept for and store 
it into "elapsed_time". This is usually between 10ms and 20ms.
	2. We encode "elapsed_time" miliseconds worth of data into a buffer
	3. We call dvb_dmx_swfilter(), passing a pointer to the buffer
	4. We clear the buffer, sleep for a bit and then go back to 1.

If the buffer is somehow full then it means that we are trying to 
simulate too many fake channels while allocating too little memory, so 
either we scale down on the amount of fake channels or we choose another 
value for the "ts_buf_sz" module_param.

I feel that this is better than adding more logic in an attempt to 
circumvent the issue, specially since I can add more logic and still 
fail due to my limited experience. The result is more bloat on the 
source code for little gain.

 > A WARN_ON() seems too severe here. Also, if something bad happens, it
 > will end causing lots of problems that can make the machine very slow,
 > ad this will flood dmesg.
 >
 > So, the best would be to use, instead, dev_warn_ratelimited().

You're right, I did not consider this. I will use dev_warn_ratelimited() 
in place of WARN_ON in the next revisions.
  

Em Sat, 2 May 2020 19:22:06 -0300
"Daniel W. S. Almeida" <dwlsalmeida@gmail.com> escreveu:

> Hi Mauro, thanks for reviewing this!
> 
>  > Em Sat,  2 May 2020 00:22:12 -0300
>  > "Daniel W. S. Almeida" <dwlsalmeida@gmail.com> escreveu:
>  >  
>  >> From: "Daniel W. S. Almeida" <dwlsalmeida@gmail.com>
>  >>
>  >> Add code to work with MPEG TS packets, such as TS headers, adaptation
>  >> fields, PCR packets and NULL packets.
>  >>
>  >> Signed-off-by: Daniel W. S. Almeida <dwlsalmeida@gmail.com>
>  >> ---
>  >>   drivers/media/test-drivers/vidtv/Makefile   |   2 +-
>  >>   drivers/media/test-drivers/vidtv/vidtv_ts.c | 130 ++++++++++++++++++++
>  >>   drivers/media/test-drivers/vidtv/vidtv_ts.h | 103 ++++++++++++++++
>  >>   3 files changed, 234 insertions(+), 1 deletion(-)
>  >>   create mode 100644 drivers/media/test-drivers/vidtv/vidtv_ts.c
>  >>   create mode 100644 drivers/media/test-drivers/vidtv/vidtv_ts.h
>  >>
>  >> diff --git a/drivers/media/test-drivers/vidtv/Makefile   
> b/drivers/media/test-drivers/vidtv/Makefile
>  >> index 9ea9485d42189..92001bc348615 100644
>  >> --- a/drivers/media/test-drivers/vidtv/Makefile
>  >> +++ b/drivers/media/test-drivers/vidtv/Makefile
>  >> @@ -1,6 +1,6 @@
>  >>   # SPDX-License-Identifier: GPL-2.0
>  >>
>  >>   vidtv_demod-objs := vidtv_common.o
>  >> -vidtv_bridge-objs := vidtv_common.o
>  >> +vidtv_bridge-objs := vidtv_common.o vidtv_ts.o
>  >>
>  >>   obj-$(CONFIG_DVB_VIDTV)	+= vidtv_tuner.o vidtv_demod.o vidtv_bridge.o
>  >> diff --git a/drivers/media/test-drivers/vidtv/vidtv_ts.c   
> b/drivers/media/test-drivers/vidtv/vidtv_ts.c
>  >> new file mode 100644
>  >> index 0000000000000..f545c45c0fe7c
>  >> --- /dev/null
>  >> +++ b/drivers/media/test-drivers/vidtv/vidtv_ts.c
>  >> @@ -0,0 +1,130 @@
>  >> +// SPDX-License-Identifier: GPL-2.0
>  >> +/*
>  >> + * The Virtual DVB test driver serves as a reference DVB driver and   
> helps
>  >> + * validate the existing APIs in the media subsystem. It can also aid
>  >> + * developers working on userspace applications.
>  >> + *
>  >> + * Written by Daniel W. S. Almeida <dwlsalmeida@gmail.com>
>  >> + */
>  >> +
>  >> +#include <linux/types.h>
>  >> +#include <asm/byteorder.h>
>  >> +#include "vidtv_ts.h"
>  >> +#include "vidtv_common.h"
>  >> +
>  >> +static u32 vidtv_ts_write_pcr_bits(u8 *buf, u64 pcr)
>  >> +{
>  >> +	/* Exact same from ffmpeg. PCR is a counter driven by a 27Mhz clock */
>  >> +	u64 pcr_low = pcr % 300, pcr_high = pcr / 300;
>  >> +
>  >> +	*buf++ = pcr_high >> 25;
>  >> +	*buf++ = pcr_high >> 17;
>  >> +	*buf++ = pcr_high >>  9;
>  >> +	*buf++ = pcr_high >>  1;
>  >> +	*buf++ = pcr_high <<  7 | pcr_low >> 8 | 0x7e;
>  >> +	*buf++ = pcr_low;
>  >> +
>  >> +	return 6;
>  >> +}
>  >> +
>  >> +void vidtv_ts_inc_cc(u8 *continuity_counter)
>  >> +{
>  >> +	++*continuity_counter;
>  >> +	if (*continuity_counter > TS_CC_MAX_VAL)
>  >> +		*continuity_counter = 0;
>  >> +}
>  >> +
>  >> +u32 vidtv_ts_null_write_into(struct null_packet_write_args args)
>  >> +{
>  >> +	u32    nbytes                  = 0;
>  >> +	struct vidtv_mpeg_ts ts_header = {0};
>  >> +
>  >> +	ts_header.sync_byte          = TS_SYNC_BYTE;
>  >> +	ts_header.pid                = TS_NULL_PACKET_PID;
>  >> +	ts_header.payload            = 1;
>  >> +	ts_header.continuity_counter = *args.continuity_counter;
>  >> +
>  >> +	cpu_to_be16s(&ts_header.bitfield);
>  >> +
>  >> +	/* copy TS header */
>  >> +	nbytes += vidtv_memcpy(args.dest_buf + args.dest_offset + nbytes,
>  >> +			       &ts_header,
>  >> +			       sizeof(ts_header),
>  >> +			       args.dest_offset + nbytes,
>  >> +			       args.buf_sz);  
>  >
>  > Hmm... now I see why you're returning 0 to vidtv_memcpy().
>  >
>  > Yet, if the buffer is full, you should just drop the entire package,
>  > as otherwise you may end copying things that aren't multiple of 188   
> bytes,
>  > causing sync issues at the client.  
> 
> I'd like to provide a counterargument for this.
> 
> The way I am dealing with errors throughout vidtv so far is:
> If we hit any of these WARN_ON macros, pr_err and the like, then all 
> bets are off. This means that the resulting stream will likely be 
> invalid

Losing some data is not really a big issue. 

I mean, a stream with some dropped packages is acceptable. That 
actually happens in practice, as sometimes userspace just can't read
everything in time. 

The only thing is that, if buffer overflow conditions are detected
(and allowed - se more below), the code should ensure that the package
counters should be updated, so the next frame will show a 
discontinuity. This way, userspace can detect packet losses.

> and that something needs to be rewritten in the source code and 
> my main concern is then preventing the whole driver from crashing.

A WARN_ON() won't prevent a crash. It will just output (very noisly)
stuff at dmesg. This can actually cause a crash: if someone is using
a serial console, for example, the amount of data at dmesg will
be so much that the machine will become unresponsive.

Btw, when WARN_ON() is detecting something that would cause crashes,
it should use this pattern:

	if (WARN_ON(...))
		return -EINVAL; // or some other error code like -ENOMEM

> This is exactly the behavior that you see in vidtv_memcpy and 
> vidtv_memset: nothing gets written so we don't end up with an overflow, 
> a diagnostic message is printed and there are no attempts at recovery.

Yeah, after looking how you're using it, I'm ok with the way you're
using vidtv_memcpy (but see the comments I posted today about them).

> 
> In this particular example, I compromised by allowing the size of the 
> buffer to be a module param, i.e.
> 
>  >> +static unsigned int ts_buf_sz = 20 * 188;
>  >> +module_param(ts_buf_sz, uint, 0644);
>  >> +MODULE_PARM_DESC(ts_buf_sz, "Optional size for the TS buffer");  

(Unrelated to this discussion)
What happens if userspace sets ts_buf_sz < 188? What happens if
ts_buf_sz is bigger than the available RAM size? For sure you need
a callback there in order validate its result and ensure it would
between a certain range that would make sense.

> 
> I think that what I am trying to say is better seen in the last patch 
> for this series: [RFC, WIP, v4 11/11] media: vidtv: Add a MPEG Transport 
> Stream Multiplexer.
> 
> The following takes place in vidtv_mux.c:
> 
> 	1. We wake up from sleep, take note of how long we slept for and store 
> it into "elapsed_time". This is usually between 10ms and 20ms.
> 	2. We encode "elapsed_time" miliseconds worth of data into a buffer
> 	3. We call dvb_dmx_swfilter(), passing a pointer to the buffer
> 	4. We clear the buffer, sleep for a bit and then go back to 1.
> 
> If the buffer is somehow full then it means that we are trying to 
> simulate too many fake channels while allocating too little memory, so 
> either we scale down on the amount of fake channels or we choose another 
> value for the "ts_buf_sz" module_param.
> 
> I feel that this is better than adding more logic in an attempt to 
> circumvent the issue, specially since I can add more logic and still 
> fail due to my limited experience. The result is more bloat on the 
> source code for little gain.

Ok, there are some different situations buffer overflow conditions:

1) userspace may not be calling read() fast enough;
2) the buffer is too small for a single stream;
3) the buffer will support a limited number of streams.

The code should work fine with condition (1), not causing warnings
or errors.

The minimal buffer size should be big enough to avoid condition (2).

Different strategies could be used to handle condition (3).

-

Let's forget for a while the Kernel driver, focusing on the other
side of the coin:

This driver is meant to allow testing userspace programs, emulating
a real hardware.

With real hardware, the TV board will receive a signal with some
noise. The signal is encoded with several error detection mechanisms.

When a board hardware detects that a package has issues, it will
discard it. That's why some packets have a continuity_counter,
while others are just continuously repeated.

So, from this aspect, having some packages dropped from a stream
is actually not a bad thing.

Back to the implementation part, in order to simulate a package
drop, we could simply have a rand() call that would be randomly
dropping some packages. Or, we could set the driver to produce
more streams that it would fit into the MPEG-TS stream.

For now, I would just do the latter: if the buffer is too small,
just drop an entire package, preserving the alignment.

> 
>  > A WARN_ON() seems too severe here. Also, if something bad happens, it
>  > will end causing lots of problems that can make the machine very slow,
>  > ad this will flood dmesg.
>  >
>  > So, the best would be to use, instead, dev_warn_ratelimited().  
> 
> You're right, I did not consider this. I will use dev_warn_ratelimited() 
> in place of WARN_ON in the next revisions.

yeah, dev_warn_ratelimited() would work. You could also use
WARN_ON_ONCE(), as probably there's not much value on keep
repeating the message every time.

Thanks,
Mauro