Em Thu, 30 Nov 2017 23:15:40 +0200 Αθανάσιος Οικονόμου <athoik@xxxxxxxxx> escreveu: > 2017-11-30 20:26 GMT+02:00 Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxxx>: > > Em Mon, 20 Nov 2017 02:05:35 +0100 > > Ralph Metzler <rjkm@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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
