On Fri, Nov 11, 2011 at 2:50 AM, Mauro Carvalho Chehab
<mchehab@xxxxxxxxxx> wrote:
> Em 10-11-2011 13:30, Manu Abraham escreveu:
>> Hi Andreas,
>>
>> On Thu, Nov 10, 2011 at 8:14 PM, Andreas Oberritter <obi@xxxxxxxxxxx> wrote:
>>> Hello Manu,
>>>
>>> please see my comments below:
>>>
>>> On 10.11.2011 15:18, Manu Abraham wrote:
>>>> Hi,
>>>>
>>>> Currently, for a multi standard frontend it is assumed that it just
>>>> has a single standard capability. This is fine in some cases, but
>>>> makes things hard when there are incompatible standards in conjuction.
>>>> Eg: DVB-S can be seen as a subset of DVB-S2, but the same doesn't hold
>>>> the same for DSS. This is not specific to any driver as it is, but a
>>>> generic issue. This was handled correctly in the multiproto tree,
>>>> while such functionality is missing from the v5 API update.
>>>>
>>>> http://www.linuxtv.org/pipermail/vdr/2008-November/018417.html
>>>>
>>>> Later on a FE_CAN_2G_MODULATION was added as a hack to workaround this
>>>> issue in the v5 API, but that hack is incapable of addressing the
>>>> issue, as it can be used to simply distinguish between DVB-S and
>>>> DVB-S2 alone, or another x vs X2 modulation. If there are more
>>>> systems, then you have a potential issue.
>
> For sure something like that is needed.
>
> DVB TURBO is another example where the FE_CAN_2G_MODULATION approach doesn't
> properly address.
>
It is similar to a DVB-S system with a different modulation and FEC.
It was specifically designed to make it proprietary. Nothing wrong
with it as it is now.
>>>>
>>>> In addition to the patch, for illustrative purposes the stb0899 driver
>>>> is depicted providing the said capability information.
>>>>
>>>> An application needs to query the device capabilities before
>>>> requesting an operation from the device.
>>>>
>>>> If people don't have any objections, Probably other drivers can be
>>>> adapted similarly. In fact the change is quite simple.
>>>>
>>>> Comments ?
>>>>
>>>> Regards,
>>>> Manu
>>>>
>>>>
>>>> query_frontend_capabilities.diff
>>>>
>>>>
>>>> diff -r b6eb04718aa9 linux/drivers/media/dvb/dvb-core/dvb_frontend.c
>>>> --- a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c Wed Nov 09 19:52:36 2011 +0530
>>>> +++ b/linux/drivers/media/dvb/dvb-core/dvb_frontend.c Thu Nov 10 13:51:35 2011 +0530
>>>> @@ -973,6 +973,8 @@
>>>> _DTV_CMD(DTV_GUARD_INTERVAL, 0, 0),
>>>> _DTV_CMD(DTV_TRANSMISSION_MODE, 0, 0),
>>>> _DTV_CMD(DTV_HIERARCHY, 0, 0),
>>>> +
>>>> + _DTV_CMD(DTV_DELIVERY_CAPS, 0, 0),
>>>> };
>>>>
>>>> static void dtv_property_dump(struct dtv_property *tvp)
>>>> @@ -1226,7 +1228,18 @@
>>>> c = &cdetected;
>>>> }
>>>>
>>>> + dprintk("%s\n", __func__);
>>>> +
>>>> switch(tvp->cmd) {
>>>> + case DTV_DELIVERY_CAPS:
>>>> + if (fe->ops.delivery_caps) {
>>>> + r = fe->ops.delivery_caps(fe, tvp);
>>>> + if (r < 0)
>>>> + return r;
>>>> + else
>>>> + goto done;
>>>
>>> What's the reason for introducing that label and goto?
>>
>>
>> The idea was to avoid using get_property callback being called.
>
> Nothing warrants that the only property will be DTV_DELIVERY_CAPS.
> calling get_property callback will likely work better.
It is indeed called within a case switch:
+ case DTV_DELIVERY_CAPS:
+ if (fe->ops.delivery_caps) {
+ r = fe->ops.delivery_caps(fe, tvp);
So, It does warrant that delivery caps are filled only with the
DTV_DELIVERY_CAPS, just like any other command, no difference in
there. But as I stated earlier, using get_property is just as fine.
>>
>>
>>
>>>
>>>> + }
>>>> + break;
>>>> case DTV_FREQUENCY:
>>>> tvp->u.data = c->frequency;
>>>> break;
>>>> @@ -1350,7 +1363,7 @@
>>>> if (r < 0)
>>>> return r;
>>>> }
>>>> -
>>>> +done:
>>>> dtv_property_dump(tvp);
>>>>
>>>> return 0;
>>>> diff -r b6eb04718aa9 linux/drivers/media/dvb/dvb-core/dvb_frontend.h
>>>> --- a/linux/drivers/media/dvb/dvb-core/dvb_frontend.h Wed Nov 09 19:52:36 2011 +0530
>>>> +++ b/linux/drivers/media/dvb/dvb-core/dvb_frontend.h Thu Nov 10 13:51:35 2011 +0530
>>>> @@ -305,6 +305,8 @@
>>>>
>>>> int (*set_property)(struct dvb_frontend* fe, struct dtv_property* tvp);
>>>> int (*get_property)(struct dvb_frontend* fe, struct dtv_property* tvp);
>>>> +
>>>> + int (*delivery_caps)(struct dvb_frontend *fe, struct dtv_property *tvp);
>>>> };
>>>
>>> I don't think that another function pointer is required. Drivers can
>>> implement this in their get_property callback. The core could provide a
>>> default implementation, returning values derived from fe->ops.info.type
>>> and the 2G flag.
>>
>>
>> I see your point. While trying to address the issue I had originally
>> get_property being used. One Issue that came across to me was that:
>>
>> - while currently this is just one capability field, things do look
>> fine. As we move ahead we will possibly have more capability related
>> information, given the idea that with shared hardware infrastructure
>> on frontends, this list of properties could grow.
>
> There are two DVBv5 calls that are likely designed to address this
> need:
> DTV_FE_CAPABILITY_COUNT
> DTV_FE_CAPABILITY
>
> Unfortunately, those were never implemented. I _suspect_ that the original
> idea were to allow passing an arbritary count to the driver, and let it fill
> each capability.
This was likely meant to be used for the FE_CAN flags as far as I can
see. This is not what I am trying to address.
>
>> - Once we have a few properties, then we will need to have switches in
>> that callback, given that it is generic and that length will grow.
>> Considering different hardware capabilities, we will have quite a bit
>> of shared code amongst drivers in that large switch, causing code
>> duplication.
>> Eg: with hardware having shared infrastructure - while globally it
>> might have a larger set of capabilities, when a sub part of it is run
>> in some mode, the other sub parts would have reduced set of
>> capabilities, which won't be the same as that exists globally for the
>> device. The driver alone will know the change depending on what's
>> being used.
>>
>>
>> - Additionally, though insignificant a separate callback for each
>> capability will make the drivers look a bit more consistent with
>> regards to code style.
>>
>> The basic issue is a switch that will be duplicated across drivers and
>> that which is expected to grow in length, eventually readability a
>> question. That said, this shouldn't be an issue at all. Given the
>> situation at this exact moment, having a generic callback is just
>> fine, as that will be the only which would be there, right now.
>>
>>
>>>
>>>> #define MAX_EVENT 8
>>>> @@ -362,6 +364,8 @@
>>>>
>>>> /* DVB-T2 specifics */
>>>> u32 dvbt2_plp_id;
>>>> +
>>>> + fe_delivery_system_t delivery_caps[32];
>>>> };
>>>
>>> This array seems to be unused.
>>
>>
>> That just walked in astray while I worked on it. Didn't notice at all. :-)
>> I will send an updated patch, do you still think the generic callback
>> is better ?
>>
>>
>>
>>>
>>> Regards,
>>> Andreas
>>>
>>>> struct dvb_frontend {
>>>> diff -r b6eb04718aa9 linux/drivers/media/dvb/frontends/stb0899_drv.c
>>>> --- a/linux/drivers/media/dvb/frontends/stb0899_drv.c Wed Nov 09 19:52:36 2011 +0530
>>>> +++ b/linux/drivers/media/dvb/frontends/stb0899_drv.c Thu Nov 10 13:51:35 2011 +0530
>>>> @@ -1605,6 +1605,19 @@
>>>> return DVBFE_ALGO_CUSTOM;
>>>> }
>>>>
>>>> +static int stb0899_delivery_caps(struct dvb_frontend *fe, struct dtv_property *caps)
>>>> +{
>>>> + struct stb0899_state *state = fe->demodulator_priv;
>>>> +
>>>> + dprintk(state->verbose, FE_DEBUG, 1, "Get caps");
>>>> + caps->u.buffer.data[0] = SYS_DSS;
>>>> + caps->u.buffer.data[1] = SYS_DVBS;
>>>> + caps->u.buffer.data[2] = SYS_DVBS2;
>>>> + caps->u.buffer.len = 3;
>
> This doesn't sound right to me. If userspace passed space for only one delivery system,
> you can't just adjust the size here and expect it to work, as the array on userspace
> may be smaller than 3 properties.
You misunderstood.
The userspace requested just a single property, not more than that.
The usage at the kernel side is also for a single property.
This would be what would be running at userspace for the specified patch.
struct dtv_property p[] = {
{ .cmd = DTV_DELIVERY_CAPS },
}
struct dtv_properties cmd = {
.num = 1,
.props = p,
}
ioctl(fd, FE_GET_PROPERTY, &cmd);
> Also, the other DVBv5 properties may not be DTV_DELIVERY_CAPS type.
>
> In other words, the driver or core needs to check if the type is DTV_DELIVERY_CAPS
> before filling it.
It is indeed called within a case switch:
+ case DTV_DELIVERY_CAPS:
+ if (fe->ops.delivery_caps) {
+ r = fe->ops.delivery_caps(fe, tvp);
So, It does warrant that delivery caps are filled only with the
DTV_DELIVERY_CAPS, just like any other command, no difference in
there. But as I stated earlier, using get_property is just as fine.
>
> Btw, if the "capabilities" here is just the delivery system, it would be better to
> name it as something like DTV_GET_SUPPORTED_DELIVERY.
Yes that's the intention. But I would like to have a shorter name.
First of all the GET in there is superfluous as it is used in a
FE_GET_PROPERTY. Also you cannot SET from userspace a
SUPPORTED_DELIVERY. Do you feel much of a difference between
DTV_DELIVERY_CAPS and DTV_SUPPORTED_DELIVERY ? It's just a naming
issue. I can live with either of them, but still feel that
DTV_DELIVERY_CAPS is a bit more relevant.
>
> We should also think on a way to enumerate the supported values for each DVB properties,
> the enum fe_caps is not enough anymore to store everything. It currently has 30 bits
> filled (of a total of 32 bits), and we currently have:
> 12 code rates (including AUTO/NONE);
> 13 modulation types;
> 7 transmission modes;
> 7 bandwidths;
> 8 guard intervals (including AUTO);
> 5 hierarchy names;
> 4 rolloff's (probably, we'll need to add 2 more, to distinguish between DVB-C Annex A and Annex C).
These parameters aren't anything related to the current patch.
>
> So, if we would need to add one CAN_foo for each of the above, we would need 56 to 58
> bits, plus 5-6 bits to the other capabilities that currently exists there. So, even
> 64 bits won't be enough for the current needs (even having the delivery system caps
> addressed by something else).
>
> It means that FE_GET_INFO needs to be replaced by something that scales better, e. g.
> that would allow to enumerate the supported ranges for each DVBv5 parameter.
This is not what I am addressing. This is outside the scope of the
proposed patch and requirement. These flags are retrieved correctly as
of now and is used without any issues. The purpose of the patch is to
query DVB delivery system capabilities alone, rather than DVB frontend
info/capability.
Attached is a revised version 2 of the patch, which addresses the
issues that were raised.
Regards,
Manu
diff -r b6eb04718aa9 linux/drivers/media/dvb/dvb-core/dvb_frontend.c
--- a/linux/drivers/media/dvb/dvb-core/dvb_frontend.c Wed Nov 09 19:52:36 2011 +0530
+++ b/linux/drivers/media/dvb/dvb-core/dvb_frontend.c Fri Nov 11 06:05:40 2011 +0530
@@ -973,6 +973,8 @@
_DTV_CMD(DTV_GUARD_INTERVAL, 0, 0),
_DTV_CMD(DTV_TRANSMISSION_MODE, 0, 0),
_DTV_CMD(DTV_HIERARCHY, 0, 0),
+
+ _DTV_CMD(DTV_DELIVERY_CAPS, 0, 0),
};
static void dtv_property_dump(struct dtv_property *tvp)
@@ -1226,7 +1228,11 @@
c = &cdetected;
}
+ dprintk("%s\n", __func__);
+
switch(tvp->cmd) {
+ case DTV_DELIVERY_CAPS:
+ break;
case DTV_FREQUENCY:
tvp->u.data = c->frequency;
break;
@@ -1350,7 +1356,7 @@
if (r < 0)
return r;
}
-
+done:
dtv_property_dump(tvp);
return 0;
diff -r b6eb04718aa9 linux/drivers/media/dvb/frontends/stb0899_drv.c
--- a/linux/drivers/media/dvb/frontends/stb0899_drv.c Wed Nov 09 19:52:36 2011 +0530
+++ b/linux/drivers/media/dvb/frontends/stb0899_drv.c Fri Nov 11 06:05:40 2011 +0530
@@ -1605,6 +1605,22 @@
return DVBFE_ALGO_CUSTOM;
}
+static int stb0899_get_property(struct dvb_frontend *fe, struct dtv_property *p)
+{
+ switch (p->cmd) {
+ case DTV_DELIVERY_CAPS:
+ p->u.buffer.data[0] = SYS_DSS;
+ p->u.buffer.data[1] = SYS_DVBS;
+ p->u.buffer.data[2] = SYS_DVBS2;
+ p->u.buffer.len = 3;
+ break;
+ default:
+ return -EINVAL;
+ }
+ return 0;
+}
+
+
static struct dvb_frontend_ops stb0899_ops = {
.info = {
@@ -1647,6 +1663,8 @@
.diseqc_send_master_cmd = stb0899_send_diseqc_msg,
.diseqc_recv_slave_reply = stb0899_recv_slave_reply,
.diseqc_send_burst = stb0899_send_diseqc_burst,
+
+ .get_property = stb0899_get_property,
};
struct dvb_frontend *stb0899_attach(struct stb0899_config *config, struct i2c_adapter *i2c)
diff -r b6eb04718aa9 linux/include/linux/dvb/frontend.h
--- a/linux/include/linux/dvb/frontend.h Wed Nov 09 19:52:36 2011 +0530
+++ b/linux/include/linux/dvb/frontend.h Fri Nov 11 06:05:40 2011 +0530
@@ -316,7 +316,9 @@
#define DTV_DVBT2_PLP_ID 43
-#define DTV_MAX_COMMAND DTV_DVBT2_PLP_ID
+#define DTV_DELIVERY_CAPS 44
+
+#define DTV_MAX_COMMAND DTV_DELIVERY_CAPS
typedef enum fe_pilot {
PILOT_ON,
