Re: DVB-S2 and S2X API extensions

[Mauro Carvalho Chehab <mchehab@s-opensource.com>]

Em Thu, 30 Nov 2017 23:15:40 +0200
Αθανάσιος Οικονόμου         <athoik@gmail.com> escreveu:

> 2017-11-30 20:26 GMT+02:00 Mauro Carvalho Chehab <mchehab@s-opensource.com>:
> > Em Mon, 20 Nov 2017 02:05:35 +0100
> > Ralph Metzler <rjkm@metzlerbros.de> escreveu:
> >  
> >> Hi,  
> >
> > (C/C Athanasios, as he also rised concerns about S2X).  
> >>
> >> Mauro Carvalho Chehab writes:  
> >>  > Em Thu, 9 Nov 2017 16:28:18 +0100
> >>  > Ralph Metzler <rjkm@metzlerbros.de> escreveu:
> >>  >  
> >>  > > Hi,
> >>  > >
> >>  > > I have a few RFCs regarding new needed enums and
> >>  > > properties for DVB-S2 and DVB-S2X:
> >>  > >
> >>  > > - DVB-S2/X physical layer scrambling
> >>  > >
> >>  > > I currently use the inofficial DTV_PLS for setting the scrambling
> >>  > > sequence index (cf. ETSI EN 300 468 table 41) of
> >>  > > the physical layer scrambling in DVB-S2 and DVB-S2X.
> >>  > > Some drivers already misuse bits 8-27 of the DTV_STREAM_ID
> >>  > > for setting this. They also differentiate between gold, root
> >>  > > and combo codes.
> >>  > > The gold code is the actual scrambling sequence index.
> >>  > > The root code is just an intermediate calculation
> >>  > > accepted by some chips, but derived from the gold code.
> >>  > > It can be easily mapped one-to-one to the gold code.
> >>  > > (see https://github.com/DigitalDevices/dddvb/blob/master/apps/pls.c,
> >>  > > A more optimized version of this could be added to dvb-math.c)
> >>  > > The combo code does not really exist but is a by-product
> >>  > > of the buggy usage of a gold to root code conversion in ST chips.
> >>  > >
> >>  > > So, I would propose to officially introduce a property
> >>  > > for the scrambling sequence index (=gold code).
> >>  > > Should it be called DTV_PLS (which I already used in the dddvb package)
> >>  > > or rather DTV_SSI?
> >>  > > I realized PLS can be confused with physical layer signalling which
> >>  > > uses the acronym PLS in the DVB-S2 specs.  
> >>  >
> >>  > Yes, it makes sense to have a DTV property for the PLS gold code.
> >>  >
> >>  > I would prefer to use a clearer name, like DTV_PLS_GOLD_CODE,
> >>  > to make easier to identify what it means.
> >>  >
> >>  > At documentation, we should point to EN 302 307 section 5.5.4 and
> >>  > to EN 300 468 table 41, with a good enough description to make
> >>  > clear that it is the gold code, and not the root code (or
> >>  > a chipset-specific "combo" code).  
> >>
> >> If we use a long descriptive name, DTV_SCRAMBLING_SEQUENCE_INDEX might
> >> be better because it is the actual name used for it in the documentation
> >> and SI fields.  
> >
> > I'm OK with that.
> >  
> >> And, to make it absolutely clear, the combo code is not just a
> >> chipset-specific code, it is utter BS!
> >> Here is why (sorry for the lengthy explanation):
> >>
> >> The STV090X/0910 chips have 3 8-bit registers for setting the
> >> root code called PLROOT0, PLROOT1 and PLROOT2.
> >> The code has 18 bits. So PLROOT0, PLROOT1 and the lower 2 bits
> >> (bits 0 and 1) of PLROOT2 are the root code.
> >> Bits 2 and 3 of PLROOT2 determine a mode with the following
> >> 3 possible values:
> >> 0 = the 18 bits are the root code
> >> 1 = the 18 bits are the gold code
> >> 2 = if you write the gold code to the 18 bits, they will
> >>     be converted to the root code and you can then
> >>     read them back, mode will be changed to 0 after
> >>     conversion
> >>
> >> This mode 2 is what somebody called "combo code".
> >> But why is it seen as a different code? It should
> >> behave just identical to writing a gold code!
> >> Because some Linux drivers, probably also some
> >> Windows drivers, first write PLROOT2, then PLROOT1 and
> >> PLROOT0, also in the case of mode 2.
> >> Writing the 2 in the mode bits actually triggers the
> >> gold->root conversion and this conversion takes some time!
> >>
> >> So, the conversion is triggered by the write to PLROOT2
> >> even though PLROOT1 and PLROOT0 have not yet been written.
> >> Depending on many factors like I2C write speed, the
> >> computer speed, other tasks running, etc. and especially also
> >> the previous values of PLROOT1 and PLROOT2, you will get
> >> varying results after the conversion.
> >> The length of the conversion also depends on the size of
> >> the gold code.
> >> For small gold codes the conversion is so fast that
> >> it is finished before PLROOT1 and PLROOT2 are written.
> >> The lower 16 bits of the conversion results will actually be overwritten
> >> again! For larger gold codes only the lower 8 bits, etc.
> >>
> >> Think about all the race conditions and wrong initial values in this
> >> process and everybody please forget about "combo code"!  
> >
> > Yeah, it sounds messy ;-)
> >  
> >>  > > DVB-S2X also defines 7 preferred scrambling code sequences
> >>  > > (EN 302 307-2 5.5.4) which should be checked during tuning.
> >>  > > If the demod does not do this, should the DVB kernel layer or
> >>  > > application do this?  
> >>  >
> >>  > IMHO, it should be up to the kernel to check if the received
> >>  > gold code is one of those 7 codes from EM 302 307 part 2 spec,
> >>  > if the delivery system is DVB_S2X (btw, we likely need to add it
> >>  > to the list of delivery systems). Not sure what would be the
> >>  > best way to implement it. Perhaps via some ancillary routine
> >>  > that the demods would be using.
> >>  >  
> >>  > > - slices
> >>  > >
> >>  > > DVB-S2 and DVB-C2, additionally to ISI/PLP, also can have
> >>  > > slicing. For DVB-C2 I currently use bits 8-15 of DTV_STREAM_ID as slice id.  
> >>  >
> >>  > Better to use a separate property for that. On the documentation
> >>  > patches I wrote, I made clear that, for DVB-S2, only 8 bits of
> >>  > DTV_STREAM_ID are valid.
> >>  >
> >>  > We need to add DVB-C2 delivery system and update documentation
> >>  > accordingly. I made an effort to document, per DTV property,
> >>  > what delivery systems accept them, and what are the valid
> >>  > values, per standard.
> >>  >  
> >>  > > For DVB-S2/X the misuse of bits 8-27 by some drivers for selecting
> >>  > > the scrambling sequence index code could cause problems.
> >>  > > Should there be a new property for slice selection?  
> >>  >
> >>  > Yes.
> >>  >  
> >>  > > It is recommended that slice id and ISI/PLP id are identical but they
> >>  > > can be different.  
> >>  >
> >>  > The new property should reserve a value (0 or (unsigned)-1) to mean "AUTO",
> >>  > in the sense that slice ID will be identical to ISI/PLP, being the default.
> >>  >  
> >>  > > - new DVB-S2X features
> >>  > >
> >>  > > DVB-S2X needs some more roll-offs, FECs and modulations. I guess adding those
> >>  > > should be no problem?  
> >>  >
> >>  > Yes, just adding those are OK. We should just document what values are
> >>  > valid for DVB-S2X at the spec.
> >>  >
> >>  > Ok, this is actually already at the specs, but it helps application
> >>  > developers to ensure that their apps will only send valid values to the
> >>  > Kernel, if we keep such information at the uAPI documentation.
> >>  >  
> >>  > > Do we need FE_CAN_SLICES, FE_CAN_3G_MODULATION, etc?  
> >>  >
> >>  > That is a good question. On my opinion, yes, we should add new
> >>  > capabilities there, but we're out of space at the u32 caps that we
> >>  > use for capabilities (there are other missing caps there for other
> >>  > new standards).
> >>  >
> >>  > We could start using a DTV property for capabilities, or define
> >>  > a variant of FE_GET_INFO that would use an u64 value for
> >>  > the caps field.
> >>  >  
> >>  > > Or would a new delivery system type for S2X make sense?  
> >>  >
> >>  > IMHO, it makes sense to have a new delivery system type for S2X.
> >>  > A FE_CAN_3G_MODULATION (and, in the future, CAN_4G, CAN_5G, ...)
> >>  > could work too, but not sure how this would scale in the future,
> >>  > as support for older variants could be removed from some devices,
> >>  > e. g. a given demod could, for instance, support 3G, 4G and 5G
> >>  > but won't be able to work with 1G or 2G.
> >>  >
> >>  > My guess is that multiple delivery systems would scale better.  
> >>
> >>
> >> In the case of DVB-S2X I am not sure anymore if this is really a new
> >> delivery system. S2X only consists of extensions to S2.
> >> E.g., some modcods which were optional for DVB-S2 were made
> >> mandatory for DVB-S2X (so they are rather just a capability like
> >> multi-stream), other modcods are new in S2X.
> >> Features like slicing are even just an optional annex to S2 and not
> >> part of S2X.  
> >
> > Yeah, I won't doubt that some day, we may end by having a DVB-S3, but, for
> > now, it looks more like an extension to DVB-S2. Btw, DVB turbo also somewhat
> > falls on a similar case: it is just an extension to DVB-S.
> >
> > That's by the way why I think that a CAN_GEN3 flag is a bad idea: it is not
> > clear when we should increment the generation number, and we might end by
> > having a DVB-S3 associated with CAN_GEN4.
> >
> > So, IMHO, the best would be to choose between:
> >
> > 1) name it as DVB_S2X;
> > 2) add one FE_CAN capability for every supported feature.
> >
> > I suspect that (2) would be easier for userspace to support, as it can always
> > discard a channel transponder if the modulation doesn't match a capability
> > mask.
> >
> > The issue here is that we won't have enough space at
> > enum fe_caps.
> >
> > Right now, we only have 2 bits available, between those:
> >
> >         FE_HAS_EXTENDED_CAPS            = 0x800000,
> >         FE_CAN_MULTISTREAM              = 0x4000000,
> >
> > Well, I guess FE_HAS_EXTENDED_CAPS were meant to say that the capabilities
> > should be read elsewhere, but this is not backward-compatible. Anyway,
> > even if we reuse it, we have just 3 bits left.
> >
> > That's for sure not enough to add one capability per modulation
> > or per FEC type.
> >  
> 
> Exactly one of the problems we are currently having especially with
> frontends supporting more than one delsys.
> 
> We have and S2/S/T2/C capable frontend, and T2 is capable for
> multistream, so the multistream is enabled on caps.
> 
> Now the userspace is now aware if the multistream refers to specific
> modulation, so in order to be precise you need to make
> assumptions based on frontend name!

I see. Yeah, the information provided by FE_GET_INFO is not
perfect when multiple delivery systems are supported.

> 
> > -
> >
> > Just as a brainstorming, perhaps we should consider using a
> > dvb_frontend_info_v2. Something like:
> >
> > struct dvb_frontend_info_v2 {
> >         char       name[128];
> >
> >         // Frequencies in Hz for all standards
> >
> >         __u64      frequency_min;
> >         __u64      frequency_max;
> >         __u64      frequency_stepsize;
> >         __u64      frequency_tolerance;
> >
> >         __u32      symbol_rate_min;
> >         __u32      symbol_rate_max;
> >         __u32      symbol_rate_tolerance;
> >
> >         __u64      fe_general_flags;            // IS_STUPID, INVERSION_AUTO(?), and all the remaining FOO_AUTO flags
> >         __u64      supported_standards;         // perhaps a bitmask of supported standards would be interesting here?
> >         __u64      fe_modulation_flags;         // only modulation types: 8VSB, 16QAM, ...
> >         __u64      fe_modulation_auto_flags;    // subset of fe_modulation_flags. Indicate what modulations are autodetected
> >         __u64      fe_fec_flags;                // can FEC 1/2, 3/4, ...
> >         __u64      fe_guard_interval_flags;
> >         __u64      fe_transmission_mode_flags;
> >         // should we need flags also for hierarchy and interleaving mode?
> > };

This wouldn't solve the multiple delivery systems you pointed above.

It would solve one other problem we have though: frontends capable of
both satellite and non-satellite delivery systems need two separate
frontend entries, because frequencies for satellite are specified in
kHz.

> >
> > Another alternative would be to have a set of properties that would be
> > querying for valid values for a given parameter, e. g. userspace would
> > send a request like:
> >
> >         struct dtv_get_info {
> >                 __u32 delsys;
> >                 __u32 property;
> >                 __u64 supported_bitmask;
> >                 __u64 auto_bitmask;
> >         } __attribute__ ((packed));
> >
> >         struct dtv_property {
> >                 __u32 cmd;
> >                 __u32 reserved[3];
> >                 union {
> >                         __u32 data;
> >                         struct dtv_fe_stats st;
> >                         struct {
> >                                 __u8 data[32];
> >                                 __u32 len;
> >                                 __u32 reserved1[3];
> >                                 void *reserved2;
> >                         } buffer;
> >                         struct dtv_get_info info;
> >                 }
> >         } u;
> >         int result;
> > } __attribute__ ((packed));
> >
> >
> > Filling it like:
> >
> >         cmd = DTV_FE_CAPABILITY
> >         delsys = SYS_DVBS2
> >         u.info.property = DTV_MODULATION
> >
> > At return, if the property is supported for the delivery system,
> > it would return with supported_bitmask and auto_bitmask filled
> > with a bitmask associated with such property.

One issue with this approach is how will we pass the other
FE_GET_INFO data (name, frequency range, symbol rate range).
Such parameters are unique per frontend.

> > As multiple properties can be filled at the same time, just one
> > ioctl() call should be enough to replace FE_GET_INFO.
> >  
> 
> That sounds promising! Having something that can be easily extendable
> (like DELSYS introduced to cover the need to new delivery systems) is
> the way to go.

Yes, that makes sense, at least for parameters that are dependent on
the delivery system.

> 
> 
> >>  > > -DVB-S2 base band frame support
> >>  > >
> >>  > > There are some older patches which allowed to switch the demod
> >>  > > to a raw BB frame mode (if it and the bridge support this) and
> >>  > > have those parsed in the DVB layer.
> >>  > >
> >>  > > See
> >>  > > https://patchwork.linuxtv.org/patch/10402/
> >>  > > or
> >>  > > https://linuxtv.org/pipermail/linux-dvb/2007-December/022217.html
> >>  > >
> >>  > > Chris Lee seems to have a tree based on those:
> >>  > > https://bitbucket.org/updatelee/v4l-updatelee
> >>  > >
> >>  > >
> >>  > > Another method to support this is to wrap the BB frames
> >>  > > into sections and deliver them as a normal transport stream.
> >>  > > Some demods and/or PCIe bridges support this in hardware.
> >>  > > This has the advantage that it would even work with SAT>IP.
> >>  > >
> >>  > > How should the latter method be supported in the DVB API?
> >>  > > With a special stream id or separate porperty?
> >>  > > Switching to this mode could even be done automatically
> >>  > > in case of non-TS streams.  
> >>  >
> >>  > That's a very good question.
> >>  >
> >>  > I guess we'll need to add support at the demux API to inform/select
> >>  > the output format anyway, in order to support, for example, ATSC
> >>  > version 3, with is based on MMT, instead of MPEG-TS.
> >>  >
> >>  > One thing that it is on my todo list for a while (with very low priority)
> >>  > would be to allow userspace to select between 188 or 204 packet sizes,
> >>  > as recording full mpeg-TS with 204 size makes easy to reproduce ISDB-T
> >>  > data on my Dectec RF modulators :-) The dtplay default command line
> >>  > application doesn't allow specifying layer information (there is a fork
> >>  > of it that does, though). Yet, as this would require to change the
> >>  > ISDB-T demod as well to be useful (and this is just meant to
> >>  > avoid me the need to run the MS application), this is something that I've
> >>  > been systematically postponing, in favor of things that would be more
> >>  > useful for the general audience.
> >>  >
> >>  > Anyway, IMHO, it is time to work at the demux API to add a way to list
> >>  > what kind of output types it supports and to let userspace select the
> >>  > one that it is more adequate for its usecase, if multiple ones are
> >>  > supported.  
> >>
> >>
> >> For the BB frame wrapping in sections I wrote about above I only need
> >> a selection switch and no changes to the demux.
> >> The wrapping is all done in hardware.
> >>
> >> But you are right, more flexibility in the demux API could be useful
> >> for the 204 byte packets you mentioned, other formats which contain
> >> timestamp headers, MMT, etc.  
> >
> >
> > Yes.
> >
> >
> > Thanks,
> > Mauro  
> 
> Thanks for your great suggestions guys. I would really like to see
> patches for above improvements.
> 
> I have never send before a patch to linux-media, so I think can try
> sending a proper patch for the new code rate and modulations, if Ralph
> is not up to it already.

Whatever works best. Most of the rules for patch submission are at linuxtv
wiki pages.

There are a few other rules that apply for new API submissions:

1) at least one real driver should use it. The rationale is that
   it helps others to see how the API should be implemented, and
   to test it;

2) both kernelspace and userspace APIs should be documented under
   Documentation/media.

Thanks,
Mauro