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);
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.
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.
>> 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;
...
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().
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.
Anyway, first things first. Let's apply Manu's patches (after the proposed changes),
and then work on it.
>
> 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
--
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
