Hi Laurent,
Laurent Pinchart wrote:
> Hi Sakari,
>
> On Friday 06 January 2012 12:27:03 Sakari Ailus wrote:
>> Laurent Pinchart wrote:
>>> On Tuesday 20 December 2011 21:27:56 Sakari Ailus wrote:
>>>> From: Sakari Ailus <sakari.ailus@xxxxxx>
>>>>
>>>> Add support for VIDIOC_SUBDEV_S_SELECTION and VIDIOC_SUBDEV_G_SELECTION
>>>> IOCTLs. They replace functionality provided by VIDIOC_SUBDEV_S_CROP and
>>>> VIDIOC_SUBDEV_G_CROP IOCTLs and also add new functionality (composing).
>>>>
>>>> VIDIOC_SUBDEV_G_CROP and VIDIOC_SUBDEV_S_CROP continue to be supported.
>>>
>>> As those ioctls are experimental, should we deprecate them ?
>>
>> I'm also in favour of doing that. But I'll make it a separate patch.
>>
>>>> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxx>
>>>> ---
>>>>
>>>> drivers/media/video/v4l2-subdev.c | 26 ++++++++++++++++++++-
>>>> include/linux/v4l2-subdev.h | 45
>>>> ++++++++++++++++++++++++++++++++++ include/media/v4l2-subdev.h |
>>>> 5 ++++
>>>> 3 files changed, 75 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/drivers/media/video/v4l2-subdev.c
>>>> b/drivers/media/video/v4l2-subdev.c index 65ade5f..e8ae098 100644
>>>> --- a/drivers/media/video/v4l2-subdev.c
>>>> +++ b/drivers/media/video/v4l2-subdev.c
>>>> @@ -36,13 +36,17 @@ static int subdev_fh_init(struct v4l2_subdev_fh *fh,
>>>> struct v4l2_subdev *sd) {
>>>>
>>>> #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
>>>>
>>>> /* Allocate try format and crop in the same memory block */
>>>>
>>>> - fh->try_fmt = kzalloc((sizeof(*fh->try_fmt) + sizeof(*fh->try_crop))
>>>> + fh->try_fmt = kzalloc((sizeof(*fh->try_fmt) + sizeof(*fh->try_crop)
>>>> + + sizeof(*fh->try_compose))
>>>>
>>>> * sd->entity.num_pads, GFP_KERNEL);
>>>
>>> Could you check how the 3 structures are aligned on 64-bit platforms ?
>>> I'm a bit worried about the compiler expecting a 64-bit alignment for
>>> one of them, and getting only a 32-bit alignment in the end.
>>>
>>> What about using kcalloc ?
>>
>> kcalloc won't make a difference --- see the implementation. Do you think
>> this is really an issue in practice?
>
> It won't make a difference for the alignment, it's just that we allocate an
> array, so kcalloc seemed right.
>
>> If we want to ensure alignment I'd just allocate them separately. Or
>> create a struct out of them locally, and get the pointers from that
>> struct --- then the alignment would be the same as if those were part of
>> a single struct. That achieves the desired result and also keeps error
>> handling trivial.
>>
>> I wouldn't want to start relying on the alignment based on the sizes of
>> these structures.
>
> Sounds good to me. Allocating them as part of a bigger structure internally
> could be more efficient than separate allocations, but I'm fine with both.
On second thought, I think I'll combine them into a new anonymous struct
the field name of which I call "pad", unless that requires too intrusive
changes in other drivers. How about that?
--
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxxxxxxxxxxxxx
--
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
