Hi Mauro, > Em Thu, 6 Aug 2009 22:04:13 +0200 > Hans Verkuil <hverkuil@xxxxxxxxx> escreveu: > >> Hi Mauro, >> >> Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb for the >> following: >> >> - v4l: introduce string control support. >> - v4l2-spec: document the new string control type. >> - v4l2-ctl: modulator bug fixes >> - v4l2-ctl: add support for string controls >> >> It's the same as the previous version, except for reverting the variable >> renaming and for improving the comments above the ctrl_is_value64 and >> ctrl_is_pointer functions. I've decided to keep the ctrl_is_value64() >> function because I want to keep the correct value64 conversion code in >> compat32 rather than having to do that work again at some point in the >> future. > > I agree with Trent, this really sucks: > > static inline int ctrl_is_value64(u32 id) > { > - return 0; > + switch (id) { > + case V4L2_CID_RDS_TX_PS_NAME: > + case V4L2_CID_RDS_TX_RADIO_TEXT: > + return 1; > + default: > + return 0; > + } > } > > Yet, by looking on how the struct is defined: > > struct v4l2_ext_control { > __u32 id; > __u32 reserved2[2]; > union { > __s32 value; > __s64 value64; > void *reserved; > }; > } __attribute__ ((packed)); > > Except on the strings case, I don't see why we need a compat stuff for > this > routine at the first place, copying field by field. Correct me if I'm > wrong, > but, on both 32 and 64 bit cases, the fields will be at the same position > on > both 32 and 64 bits. So, the code could be just: > > copy_in_user(&kcontrols, &ucontrols, sizeof(ucontrols)); > if (ctrl_is_pointer(id)) { > /* get_user/compat_ptr magic */ > } OK, makes sense. > > I also agree with Trent that, instead of having to maintain some sort of > list > or code to identify string controls, the better would be to reserve one > bit to > indicate pointer controls (like bit 31). People tend to forget about > v4l2-compat32 layer, and I bet that, if we do some sort of list, people > will > forget to update it when adding a new control, and compat32 layer will > break. > So, ctrl_is_pointer() can be just: > > #define ctrl_is_pointer(id) (id & (1 << 31)) Now THAT is ugly. Anyway, you can't do it like that. Control IDs are not random but are grouped in control classes so that similar controls are together when you enumerate them. Adding flags to mark it as a specific type is 1) an ugly hack to work around an infrastructure problem, and 2) will make it impossible to group string controls together with controls that have a similar purpose. Anyway, I've started working on a proper control handling framework that will make it possible for the compat module to query the type. That will solve this. But that probably won't be ready for the upcoming 2.6.32, while the si4713 is ready right now. What should have happened is that we could test whether the size field is non-zero. Unfortunately, some apps do not set that field (currently a reserved field) to 0 and we never checked that in the kernel. So sadly we have to do it another way. > >> Also note that it is very likely that these two functions will disappear >> again in a few months: once the v4l framework has proper control support >> these functions should no longer be needed since this information can >> then be obtained directly from the framework. > > This don't change the fact that you'll need a sort of list or code that > associates a control id with a type, called by the compat layer. Testing > for a > bit costs less than any table seek method. O(log N) where N is the number of registered controls. It's implemented as a binary search. It is generic and will work with whatever types we might create in the future. I will prepare a new pull request where I use copy_in_user. Regards, Hans -- 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
