Hello Hans
On Wed, Feb 12, 2014 at 2:26 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> On 02/12/14 14:13, Ricardo Ribalda Delgado wrote:
>> Hello Hans
>>
>> Thanks for you promptly response
>>
>> On Wed, Feb 12, 2014 at 1:40 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>>> On 02/12/14 13:11, Ricardo Ribalda Delgado wrote:
>>>> Hi Hans
>>>>
>>>> Thanks for your reply
>>>>
>>>> On Wed, Feb 12, 2014 at 12:20 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>>>>> Hi Ricardo,
>>>>>
>>>>> On 02/12/14 11:44, Ricardo Ribalda Delgado wrote:
>>>>>> Hello Hans
>>>>>>
>>>>>> In the case of U8 and U16 data types. Why dont you fill the elem_size
>>>>>> automatically in v4l2_ctrl and request the driver to fill the field?
>>>>>
>>>>> When you create the control the control framework has to know the element
>>>>> size beforehand as it will use that to allocate the memory containing the
>>>>> control's value. The control framework is aware of the 'old' control types
>>>>> and will fill in the elem_size accordingly, but it cannot do that in the
>>>>> general case for these complex types. I guess it could be filled in by the
>>>>> framework for the more common types (U8, U16) but I felt it was more
>>>>> consistent to just require drivers to fill it in manually, rather than have
>>>>> it set for some types but not for others.
>>>>>
>>>>>>
>>>>>> Other option would be not declaring the basic data types (U8, U16,
>>>>>> U32...) and use elem_size. Ie. If type==V4L2_CTRL_COMPLEX_TYPES, then
>>>>>> the type is basic and elem_size is the size of the type. If the type
>>>>>>> V4L2_CTRL_COMPLEX_TYPES the type is not basic.
>>>>>
>>>>> You still need to know the type. Applications have to be able to check for
>>>>> the type, the element size by itself doesn't tell you how to interpret the
>>>>> data, you need the type identifier as well.
>>>>
>>>> I think that the driver is setting twice the same info. I see no gain
>>>> in declaring U8, U16 types etc if we still have to set the element
>>>> size. This is why I believe that we should only declare the "structs".
>>>
>>> Just to make sure I understand you: for simple types like U8/U16 you want
>>> the control framework to fill in elem_size, for more complex types (structs)
>>> you want the driver to fill in elem_size?
>>
>> I dont like that the type contains the size of the element, and then I
>> have to provide the size again. (Hungarian notation)
>>
>> Instead, I think it is better:
>>
>> Defines ONLY this two types for simple types:
>> V4L2_CTRL_COMPLEX_TYPE_SIGNED_INTEGER and
>> V4L2_CTRL_COMPLEX_TYPE_UNSIGNED_INTEGER and use elem_size to determine
>> the size.
>
> It sounds great, but it isn't in practice because this will produce awful
> code like this:
>
> switch (type) {
> case V4L2_CTRL_COMPLEX_TYPE_SIGNED_INTEGER:
> switch (elem_size) {
> case 1: // it's a u8!
> break;
> case 2: // it's a u16!
> break;
> }
> etc.
> }
I slightly disagree here. Your proposal will produce this code:
case V4L2_CTRL_TYPE_U8:
ptr.p_u8[idx] = ctrl->default_value;
break;
case V4L2_CTRL_TYPE_U16:
ptr.p_u16[idx] = ctrl->default_value;
break;
case V4L2_CTRL_TYPE_U32:
ptr.p_s32[idx] = ctrl->default_value;
break;
case V4L2_CTRL_TYPE_U64:
ptr.p_s64[idx] = ctrl->default_value;
break;
case V4L2_CTRL_TYPE_S8:
ptr.p_s8[idx] = ctrl->default_value;
break;
case V4L2_CTRL_TYPE_S16:
ptr.p_s16[idx] = ctrl->default_value;
break;
case V4L2_CTRL_TYPE_S32:
ptr.p_s32[idx] = ctrl->default_value;
break;
case V4L2_CTRL_TYPE_S64:
ptr.p_s64[idx] = ctrl->default_value;
break;
instead of:
case V4L2_CTRL_COMPLEX_TYPE_SIGNED_INTEGER:
case V4L2_CTRL_COMPLEX_TYPE_UNSIGNED_INTEGER:
memcpy(&ptr.p[idx],&ctrl->default_value,ctrl->elem_size)
Anyway, this is just a opinion, technically, both solutions are fine,
and yours is already implemented and reviewed.
Regards!
>
> It makes for very awkward code, both in the kernel and in applications.
>
>> And then one define per "structured types" ie:
>> V4L2_CTRL_COMPLEX_TYPE_POINT V4L2_CTRL_COMPLEX_TYPE_IRRATIONAL.. with
>> elem_size determining the size.
>>
>> But if you dont like that idea, as second preference then I think
>> elem_size should be filled by the subsystem for simple types.
>
> I think having the framework fill in elem_size for the basic types such
> as u8 and u16 does make sense. These are already handled by the standard
> number validators, so we should probably have the elem_size set as well.
>
> Regards,
>
> Hans
>
>>
>>
>> Thanks!
>>>
>>>> what about something like: V4L2_CTRL_COMPLEX_TYPE_SIGNED_INTEGER +
>>>> size, V4L2_CTRL_COMPLEX_TYPES_UNSIGNED_INTEGER + size.... instead of
>>>> V4L2_CTRL_COMPLEX_TYPES_U8, V4L2_CTRL_COMPLEX_TYPES_U16,
>>>> V4L2_CTRL_COMPLEX_TYPES_U32, V4L2_CTRL_COMPLEX_TYPES_S8 ....
>>>>
>>>> Btw, I am trying to implement a dead pixel control on the top of you
>>>> api. Shall I wait until you patchset is merged or shall I send the
>>>> patches right away?
>>>
>>> You're free to experiment, but I am not going to ask Mauro to pull additional
>>> patches as long as this initial patch set isn't merged.
>>>
>>> Regards,
>>>
>>> Hans
>>
>>
>>
>>
>>
>
--
Ricardo Ribalda
--
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
