Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 6 May 2020 03:28:17 -0300
From:   "Daniel W. S. Almeida" <dwlsalmeida@gmail.com>
To:     Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Cc:     "=?utf-8?Q?sean=40mess.org?=" <sean@mess.org>,
        "=?utf-8?Q?kstewart=40linuxfoundation.org?=" 
        <kstewart@linuxfoundation.org>,
        "=?utf-8?Q?allison=40lohutok.net?=" <allison@lohutok.net>,
        "=?utf-8?Q?tglx=40linutronix.de?=" <tglx@linutronix.de>,
        "=?utf-8?Q?linux-media=40vger.kernel.org?=" 
        <linux-media@vger.kernel.org>,
        "=?utf-8?Q?skhan=40linuxfoundation.org?=" <skhan@linuxfoundation.org>,
        "=?utf-8?Q?linux-kernel-mentees=40lists.linuxfoundation.org?=" 
        <linux-kernel-mentees@lists.linuxfoundation.org>,
        "=?utf-8?Q?linux-kernel=40vger.kernel.org?=" 
        <linux-kernel@vger.kernel.org>
Message-ID: <81B965F9-3A09-40D0-87DF-611A153E744C@getmailspring.com>
In-Reply-To: <20200503095105.6b8f0d3f@coco.lan>
References: <20200503095105.6b8f0d3f@coco.lan>
Subject: Re: [RFC, WIP, v4 08/11] media: vidtv: implement a PSI
 generator
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Mauro,


>> +static u32
>> +vidtv=5Fpsi=5Fts=5Fpsi=5Fwrite=5Finto(struct psi=5Fwrite=5Fargs args)=

>> +=7B
>> +	/*
>> +	 * Packetize PSI sections into TS packets:
>> +	 * push a TS header (4bytes) every 184 bytes
>> +	 * manage the continuity=5Fcounter
>> +	 * add stuffing after the CRC
>> +	 */
>> +
>> +	u32  nbytes=5Fpast=5Fboundary =3D (args.dest=5Foffset % TS=5FPACKET=5F=
LEN);
>> +	bool aligned              =3D nbytes=5Fpast=5Fboundary =3D=3D 0;
>> +
>> +	/*
>> +	 * whether we need to fragment the data into multiple ts packets
>> +	 * if we are aligned we need to spare one byte for the pointer=5Ffie=
ld
>> +	 */
>> +	bool split =3D (aligned) =3F
>> +		     args.len > TS=5FPAYLOAD=5FLEN - 1 :
>> +		     nbytes=5Fpast=5Fboundary + args.len > TS=5FPACKET=5FLEN;
>> +
>> +	/* how much we can write in this packet */
>> +	u32 payload=5Fwrite=5Flen =3D (split) =3F
>> +				(aligned)     =3F TS=5FPAYLOAD=5FLEN       :
>> +				TS=5FPACKET=5FLEN - nbytes=5Fpast=5Fboundary :
>> +				args.len;
>> +
>> +	struct psi=5Fwrite=5Fargs new=5Fargs =3D =7B0=7D;
>> +	struct vidtv=5Fmpeg=5Fts ts=5Fheader =3D =7B0=7D;
>> +
>> +	u32 nbytes =3D 0;  /* number of bytes written by this function */
>> +	u32 temp   =3D 0;
>> +
>> +	/* Just a sanity check, should not really happen because we stuff
>> +	 * the packet when we finish a section, i.e. when we write the crc a=
t
>> +	 * the end. But if this happens then we have messed up the logic
>> +	 * somewhere.
>> +	 */
>> +	WARN=5FON(args.new=5Fpsi=5Fsection && =21aligned);
> =20
> Please use a ratelimited printk instead (here and on all similar cases
> at the TS generator).
> =20
> Also, I think that, on such case, the driver should be filling the
> remaining frame with pad bytes. E. g.:
> =20
> 	/* =20
> 	 * Assuming that vidtv=5Fmemset() args from patch 06/11 were changed =20
> 	 * according with this prototype: =20
> 	 */
> 	size=5Ft vidtv=5Fmemset(void *to, size=5Ft to=5Foffset, size=5Ft to=5F=
size,
> 			    u8 c, size=5Ft len);
> =20
> =20
> 	=23define TS=5F=46ILL=5FBYTE 0xff
> =20
> 	if (args.new=5Fpsi=5Fsection && =21aligned) =7B
> 		pr=5Fwarn=5Fratelimit(=22Warning: PSI not aligned. Re-aligning it=5Cn=
=22);
> =20
> 		vidtv=5Fmemset(args.dest=5Fbuf,
> 			     args.dest=5Foffset + nbytes=5Fpast=5Fboundary,
> 			     args.dest=5Fbuf=5Fsz,
> 			     TS=5F=46ILL=5FBYTE,	=09
> 			     TS=5FPACKET=5FLEN - nbytes=5Fpast=5Fboundary);
> 		args.dest=5Foffset +=3D TS=5FPACKET=5FLEN - nbytes=5Fpast=5Fboundary;=

> 		aligned =3D 1;
> 		nbytes=5Fpast=5Fboundary =3D 0;
> 	=7D
> =20

Sure, that's fine then=21 Just to be clear this should not happen at all,=

because the writes should go through one of these four functions (IIRC=21=
):

u32 vidtv=5Fts=5Fnull=5Fwrite=5Finto(struct null=5Fpacket=5Fwrite=5Fargs =
args)
u32 vidtv=5Fts=5Fpcr=5Fwrite=5Finto(struct pcr=5Fwrite=5Fargs args)

...which will write a single packet at a time, thus leaving the buffer
aligned if it was already aligned to begin with,

and

u32 vidtv=5Fpes=5Fwrite=5Finto(struct pes=5Fwrite=5Fargs args)
static u32
vidtv=5Fpsi=5Fts=5Fpsi=5Fwrite=5Finto(struct psi=5Fwrite=5Fargs args)

...which will pad when they finish writing a PES packet or a table
section, respectively.

I left these warnings behind as a way to warn me if the padding logic
itself is broken.


> Please use a ratelimited printk instead (here and on all similar cases
> at the TS generator).

OK.


>> +static void vidtv=5Fpsi=5Fdesc=5Fto=5Fbe(struct vidtv=5Fpsi=5Fdesc *d=
esc)
>> +=7B
>> +	/*
>> +	 * Convert descriptor endianness to big-endian on a field-by-field
>> basis
>> +	 * where applicable
>> +	 */
>> +
>> +	switch (desc->type) =7B
>> +	/* nothing do do */
>> +	case SERVICE=5FDESCRIPTOR:
>> +		break;
>> +	case REGISTRATION=5FDESCRIPTOR:
>> +		cpu=5Fto=5Fbe32s(&((struct vidtv=5Fpsi=5Fdesc=5Fregistration *)
>> +			     desc)->format=5Fidentifier);
>> +		pr=5Falert(=22%s: descriptor type %d found=5Cn=22,
>> +			 =5F=5Ffunc=5F=5F,
>> +			 desc->type);
>> +		pr=5Falert(=22%s: change 'additional=5Finfo' endianness before
>> calling=5Cn=22,
>> +			 =5F=5Ffunc=5F=5F);
> =20
> The above pr=5Falert() calls sound weird. Why are you unconditionally
> calling it (and still doing the BE conversion) for all
> REGISTRATION=5FDESCRIPTOR types=3F

To be honest, I did not know what to do. Here's what 13818-1 has to say
about registration descriptors:

>2.6.9
> Semantic definition of fields in registration descriptor
>format=5Fidentifier =E2=80=93 The format=5Fidentifier is a 32-bit value =
obtained
>from a Registration Authority as designated by
>ISO/IEC JTC 1/SC 29.
>additional=5Fidentification=5Finfo =E2=80=93 The meaning of
>additional=5Fidentification=5Finfo bytes, if any, are defined by the
>assignee of that format=5Fidentifier, and once defined they shall not ch=
ange.

So I took the cue from libdvbv5 and defined the following struct for it,
with a flexible array member at the end:

struct vidtv=5Fpsi=5Fdesc=5Fregistration =7B
	u8 type;
	u8 length;
	struct vidtv=5Fpsi=5Fdesc *next;

	/*
	 * The format=5Fidentifier is a 32-bit value obtained from a Registratio=
n
	 * Authority as designated by ISO/IEC JTC 1/SC 29.
	 */
	u32 format=5Fidentifier;
	/*
	 * The meaning of additional=5Fidentification=5Finfo bytes, if any, are
	 * defined by the assignee of that format=5Fidentifier, and once defined=

	 * they shall not change.
	 */
	u8 additional=5Fidentification=5Finfo=5B=5D;
=7D =5F=5Fpacked


As you know, I was changing the endianness from host to BE before
serializing and then changing back from BE to host. Given the struct
definition above, there was no way to do this to the
'additional=5Fidentification=5Finfo' member, since we do not know its lay=
out.

If we go with your approach instead (i.e. using =5F=5Fbe16, =5F=5Fbe32...=
etc)
then I think we can remove these two functions (and more)


thanks,
- Daniel