Steven Toth wrote:
>
> Hello!
>
> Here's the feedback collected from various people. Thank you! I've
> indented their comments and replied directly inline but more suggestions
> and potential changes.
>
> * Johannes:
> *
> * OK can't help myself to give a few techical remarks:
> *
> * - I'm not sure because I don't have a 64bit platform but I
> * think tv_property_t has different size on 64bit due to
> * alignment? Please check, to avoid compat_ioctl
> *
> * - typedefs? hm, I guess to match the style of the existing code...
>
> Looking at the CodingStyle section on typedefs, it tries to discourage
> their use.
> It probably makes sense to switch to result structs.
Done.
>
> tv_property_t type becomes struct dtv_property.
>
> *
> * - why both FE_SET_PROPERTY/FE_GET_PROPERTY and
> * TV_SET_FREQUENCY/TV_GET_FREQUENCY etc?
> *
>
> If I understand correctly then I think the suggestion is
> FE_SET_PROPERTY(TV_FREQUENCY, ...)
> makes more sense. Han's also touched on this. I think I agree, removing
> the notion of SET/GET
> from the command name is cleaner, less confusing.
>
> * - instead of "typedef tv_property_t tv_properties_t[16];" and the
> * TV_SEQ_* things why not
> * typedef struct {
> * __u32 num;
> * tv_property_t *props;
> * } tv_properties_t;
> * and then copy_from_user() an arbitrary number of properties?
> * But its all a matter of taste and if it works is find by me.
>
> Perfect sense, I'll change this, it will become
>
> struct dtv_properties {
> __u32 num;
> dtv_property *props;
> };
I varied this slightly to accomodate some reserved fields, but it's
done. The 16 command restriction is removed.
I've placed #define in frontend.h making a 64 command restriction, else
the handing in dvb_frontend.c could become unsafe. This matches similar
limits placed on the i2c-dev.c and video-ioctl.c similar variable length
handling schemes.
>
>
> * Patrick:
> *
> * I think it would be better to change TV_ to FE_ (because TV is by
> * far not
> * the only application for linux-dvb) , but this is a very unimportant
> * detail.
>
> A few people prefer dtv_ and DTV_, rather than tv_ and TV_. is fe_
> and FE_ still important to you?
Done.
>
>
> * Hans:
> *
> * I noticed that the properties work very similar as to how extended
> * controls work in v4l:
> * http://www.linuxtv.org/downloads/video4linux/API/V4L2_API/spec-si
> * ngle/v4l2.html#VIDIOC-G-EXT-CTRLS
> *
> * It might be useful to compare the two.
>
> Yes - similar concepts, I'll read further.
>
> Last year, Mauro has suggested we also consider the VIDIOC_QUERYCTRL,
> VIDIOC_QUERYMENU ioctls,
> giving the applications to ability to query supported functions on a
> given demodulator.
>
> *
> * I would recommend adding a few reserved fields, since that has
> * proven to be very useful in v4l to make the API more future proof.
Done.
>
> Good point, __u32 and __u64's could be added.
>
> struct dtv_property {
> __u32 cmd;
> __u64 reserved1;
> __u64 reserved2;
> __u64 reserved3;
> __u64 reserved4;
> union {
> __u32 data;
> struct {
> __u8 data[32];
> __u32 len;
> } buffer;
> } u;
> };
>
> Question: Is their significant penalties for switching completely from
> __u32 to __u64 now?
> Is this something that should be done completely in this struct, or are
> __u32 types less
> problematic?
This remains unanswered.
>
> This feels perhaps a little too heavy handed, I'd welcome comments on
> the types and
> number of entries.
>
> *
> * Also: is setting multiple properties an atomic action? E.g. if one
> * fails, are all other changes rolled back as well? And how do you give
> * the caller feedback on which property in the list failed? Will there
> * also be a TRY_PROPERTIES variant which just checks for correctness
> * without actually setting it? How do you query/test whether a
> * device has certain properties?
>
> Atomic vs non-atomic.
>
> That's a level of complexity that leads easily to confusion, and we
> should avoid. The code we have working today assumes that that
> set_frontend() on the demodulators is called when the TV_SET_FREQUENCY
> or TV_SEQ_COMPLETE are passed. I think removing the notion of begin and
> end sequence, in favour of short structures is less confusing and less
> code to maintain in the dvb-core.
>
> What's does this translate into for application developers? Well, a tune
> request may look something like:
>
> properties {
> { MODULATION, QAM64)}
> { INVERSION, ON)}
> { SET_FREQUENCY, 474000000)}
> { TUNE }
> };
>
> All of the commands prior to TUNE are cache in the dvb-core, TUNE
> explicitly results in a call to set_frontend();
>
> The odd item to note is satellite tuning, where certain commands need to
> be done immediatly. SET_VOLTAGE or SET_TONE, or DiSEqC related commands.
>
> So, passing SET_VOLTAGE commands would likely execute immediately. I
> don't know whether this is a good or bad thing, without spending a lot
> of time reviewing our existing dvb-core code. In some respects, the
> current in-kernel API expects multiple ioctls to occurs for satellite
> tuning to take effect. This (hopefully) will turn that into a single ioctl.
>
> Summary: This would allow the applications to call set/get properties on
> cache values, without them being committed to hardware, or directly
> committed depending on the type of tuning system and delivery system
> being used. An Important fact for any documentation.
Partially done, the notion of SEQ has been removed. The switch to remove
GET/SET is remaining.
>
> *
> * Do you need separate get and set commands? Why not either set or get a
> * list of properties, and setting them implies an automatic SEQ_COMPLETE
> * at the end. By having a variable length array of properties you do not
> * need to set the properties in multiple blocks of 16, so that
> * simplifies the API as well.
>
> Correct, this is going to change to a variable length array.
Done, as mentioned above.
>
> *
> * As I said, extended controls in v4l do something very similar.
> * I thought about the extended controls a great deal at the time, so
> * it might provide a handy comparison for you.
>
> Thanks.
>
> * Christophe:
> *
> * P.S.
> * 1) imho, DTV_ prefix would make more sense.
>
> Understood.
>
> * Andreas:
> *
> * Regarding the code:
> * 1) What's TV_SEQ_CONTINUE good for? It seems to be unused.
>
> It was going to allow unforseen long command structure to be passed to
> the kernel. It's since been superseeded and will be removed.
>
> *
> * 2) Like Christophe I'd prefer to use DTV_ and dtv_ prefixes.
>
> Understood.
>
> *
> * 3) Did you mean p.u.qam.modulation below? Also, p.u.qam.fec_inner is
> * missing.
> *
> * + printk("%s() Preparing QAM req\n", __FUNCTION__);
> * + /* TODO: Insert sanity code to validate a little. */
> * + p.frequency = c->frequency;
> * + p.inversion = c->inversion;
> * + p.u.qam.symbol_rate = c->symbol_rate;
> * + p.u.vsb.modulation = c->modulation;
>
>
> Yes, typo, thanks for mentioning this. I'll fix this.
Not done yet.
>
>
> *
> * 4) About enum tv_cmd_types:
> *
> * SYMBOLRATE -> SYMBOL_RATE?
> * INNERFEC -> INNER_FEC (or FEC)?
Done.
>
> OK, this was inconsistent. This will change to SYMBOL_RATE and INNER_FEC.
>
> *
> * The Tone Burst command got lost (FE_DISEQC_SEND_BURST). How about
> * TV_SET_TONE_BURST?
> *
> * FE_ENABLE_HIGH_LNB_VOLTAGE got lost, too.
> *
> * Which old ioctls should be considered as obsolete? Do you plan to
> * add a tv_cmd for every old ioctl?
>
> SET_TONE and SET_VOLTAGE was added late last week, and appear to be
> working correctly. If I've miss-understood the implementation then we
> can review/refine again until it makes sense.
>
>
> * Janne:
> *
> * I have also a slightly preference for DTV/dtv as prefix but it's not
> * really important.
>
> Understood.
>
> *
> *
> * >> 16 properties per ioctl are probably enough but a variable-length
> * >> > > property array would be safe. I'm unsure if this justifies a more
> * >> > > complicate copy from/to userspace in the ioctls.
> * > >
> * > > Johannes suggested we lose the fixed length approach and instead pass
> * > > in struct containing the number of elements... I happen to like this,
> * > > and it removed an unnecessary restriction.
> *
> * That's the same V4L2 handles the extended controls. For reference look
> * at VIDIOC_[G|S|TRY]_EXT_CTRLS in v4l2-ioctl.c.
>
>
> I think makes a lot of sense (when used along with _QUERYMENU). It
> allows application developers to build tools like v4l2ctl which can
> tweak frontend behaviour.
>
> I don't know whether change dtv_property to match v4l2_ext_control makes
> much sense, but the spirit of the API is strong, and should probably be
> carried forward into any new digital TV API.
>
> Let me look into this.
No investigation done yet.
>
> Summary, what do we do next?
>
> I'm going to make some patches for the structure changes, reserved
> fields, typedefs, nomenclature and generate a tree (and tune.c) for
> review/test.
That's done.
>
> That will leave the EXT_CTRLS discussion for further debate, and any
> other items that are raised during this review and round of discussions.
> We will address these in round 2.
Todo, along with bandwidth suggestions form Patrick.
>
> I welcome your feedback.
>
> - Steve
>
_______________________________________________
linux-dvb mailing list
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
