On 11.11.2011 23:02, Mauro Carvalho Chehab wrote:
> Em 11-11-2011 18:09, Andreas Oberritter escreveu:
>> On 11.11.2011 18:14, Mauro Carvalho Chehab wrote:
>>> Em 11-11-2011 13:06, Andreas Oberritter escreveu:
>>>> On 11.11.2011 15:43, Mauro Carvalho Chehab wrote:
>>>>> IMHO, the better is to set all parameters via stb0899_get_property(). We should
>>>>> work on deprecate the old way, as, by having all frontends implementing the
>>>>> get/set property ops, we can remove part of the legacy code inside the DVB core.
>>>>
>>>> I'm not sure what "legacy code" you're referring to. If you're referring
>>>> to the big switch in dtv_property_process_get(), which presets output
>>>> values based on previously set tuning parameters, then no, please don't
>>>> deprecate it. It doesn't improve driver code if you move this switch
>>>> down into every driver.
>>>
>>> What I mean is that drivers should get rid of implementing get_frontend() and
>>> set_frontend(), restricting the usage of struct dvb_frontend_parameters for DVBv3
>>> calls from userspace.
>>
>> This would generate quite some work without much benefit.
>
> Some effort is indeed needed. There are some benefits, though. Tests I made
> when writing a v5 library showed some hard to fix bugs with the current way:
>
> 1) DVB-C Annex C is currently broken, as there's no way to select it
> with the current API. A fix for it is simple, but requires adding one more
> parameter for example, to represent the roll-off (Annex C uses 0.13 for roll-off,
> instead of 0.15 for Annex A);
An alternative would be to rename SYS_DVBC_ANNEX_AC to SYS_DVBC_ANNEX_A,
add SYS_DVBC_ANNEX_C, and add SYS_DVBC_ANNEX_AC as a define for
backwards compatibility.
>
> 2) The *legacy*() calls at the code don't handle well ATSC x ANNEX B, e. g.
> a get after a set returns the wrong delivery system.
Do you know what exactly is wrong with it?
> Ok, we may be able to find some workarounds, but that means adding some hacks at
> the core.
>
>>> In other words, it should be part of the core logic to get all the parameters
>>> passed from userspace and passing them via one single call to something similar
>>> to set_property.
>>
>> That's exactly what we have now with the set_frontend, tune, search and
>> track callbacks.
>
> Yes, except that the same information is passed twice at the driver for DVB-C,
> DVB-S, DVB-T and ATSC/J.83 Annex B.
>
> The core needs to duplicate everything into the legacy structure, as it assumes
> that the drivers could use the legacy stuff.
Yes.
>>> In other words, ideally, the implementation for DTV set should be
>>> like:
>>>
>>> static int dtv_property_process_set(struct dvb_frontend *fe,
>>> struct dtv_property *tvp,
>>> struct file *file)
>>> {
>>> struct dtv_frontend_properties *c = &fe->dtv_property_cache;
>>>
>>> switch(tvp->cmd) {
>>> case DTV_CLEAR:
>>> dvb_frontend_clear_cache(fe);
>>> break;
>>> case DTV_FREQUENCY:
>>> c->frequency = tvp->u.data;
>>> break;
>>> case DTV_MODULATION:
>>> c->modulation = tvp->u.data;
>>> break;
>>> case DTV_BANDWIDTH_HZ:
>>> c->bandwidth_hz = tvp->u.data;
>>> break;
>>> ...
>>> case DTV_TUNE:
>>> /* interpret the cache of data */
>>> if (fe->ops.new_set_frontend) {
>>> r = fe->ops.new_set_frontend(fe);
>>> if (r < 0)
>>> return r;
>>> }
>>
>> set_frontend is called by the frontend thread, multiple times with
>> alternating parameters if necessary. Depending on the tuning algorithm,
>> drivers may implement tune or search and track instead. You cannot just
>> call a "new_set_frontend" driver function from here and expect it to
>> work as before.
>
> I know. The solution is not as simple as the above.
>
>>> break;
>>>
>>> E. g. instead of using the struct dvb_frontend_parameters, the drivers would
>>> use struct dtv_frontend_properties (already stored at the frontend
>>> struct as fe->dtv_property_cache).
>>
>> Drivers are already free to ignore dvb_frontend_parameters and use the
>> properties stored in dtv_property_cache today.
>
> True, and they do that already, but this still data needs to be copied
> twice. There is also a problem with this approach on get_properties:
> as some drivers may be storing returned data into dvb_frontend_parameters, while
> others may be storing at dtv_frontend_properties, the code will need to know
> what data is reliable and what data should be discarded.
>
> The current code assumes that "legacy" delivery systems will always return data
> via dtv_frontend_properties.
>
> Btw, the emulation code is currently broken for ISDB-T and DVB-T2: both emulate
> a DVB-T delivery system, so, at the DVB structure info.type = FE_OFDM.
>
> If you look at the code, you'll see things like:
>
> ...
> switch (fe->ops.info.type) {
> ...
> case FE_OFDM:
> c->delivery_system = SYS_DVBT;
> break;
> ...
This code path only gets executed when calling FE_SET_FRONTEND from
userspace. There's no DVB-T2 support in v3, so this should be ok.
> static void dtv_property_cache_sync(struct dvb_frontend *fe,
> ...
> case FE_OFDM:
> if (p->u.ofdm.bandwidth == BANDWIDTH_6_MHZ)
> c->bandwidth_hz = 6000000;
> else if (p->u.ofdm.bandwidth == BANDWIDTH_7_MHZ)
> c->bandwidth_hz = 7000000;
> else if (p->u.ofdm.bandwidth == BANDWIDTH_8_MHZ)
> c->bandwidth_hz = 8000000;
> else
> /* Including BANDWIDTH_AUTO */
> c->bandwidth_hz = 0;
> c->code_rate_HP = p->u.ofdm.code_rate_HP;
> c->code_rate_LP = p->u.ofdm.code_rate_LP;
> c->modulation = p->u.ofdm.constellation;
> c->transmission_mode = p->u.ofdm.transmission_mode;
> c->guard_interval = p->u.ofdm.guard_interval;
> c->hierarchy = p->u.ofdm.hierarchy_information;
> break;
>
> So, even a pure ISDB-T driver will need to change DVB-T u.ofdm.*, as touching
> at c->* will be discarded by dtv_property_cache_sync().
Why do ISDB-T drivers pretend to be DVB-T drivers by specifying FE_OFDM?
Even though it's called FE_OFDM, it really means "DVB-T" and not "any
delivery system using OFDM".
ISDB-T doesn't have a v3 interface, so ISDB-T drivers shouldn't
implement the legacy get_frontend callback, in which case
dtv_property_cache_sync won't be called. Furthermore, a driver may set
all properties in its get_property callback, whether
dtv_property_cache_sync was called or not.
> The same thing will also occur with all 2 GEN drivers that fill info.type.
>
> Such behavior is ugly, and not expected, and may lead into hard to detect
> bugs, as the driver code will look right, but won't behave as expected.
>
> The thing is that those emulation stuff are broken, and fixing it will probably
> be more complex than a simple scriptable change applied at the drivers that would
> replace the union by a direct access to the cache info. On most drivers, the change
> is as simple as:
> s/p->u.ofdm./c->/
> s/p->u.qpsk./c->/
> s/p->u.vsm./c->/
> s/p->u.qam./c->/
>
>>> Btw, with such change, it would actually make sense the original proposal
>>> from Manu of having a separate callback for supported delivery systems.
>>
>> Why? How does setting parameters relate to querying capabilies?
>
> Because, after such change, set_property() could likely be removed.
But how does this relate to get_property? set_property isn't used to
query capabilities.
Regards,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
