[RFC,04/17] v4l: VIDIOC_SUBDEV_S_SELECTION and VIDIOC_SUBDEV_G_SELECTION IOCTLs
Commit Message
From: Sakari Ailus <sakari.ailus@iki.fi>
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.
Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
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(-)
Comments
Hi Sakari,
Thanks for the patch.
On Tuesday 20 December 2011 21:27:56 Sakari Ailus wrote:
> From: Sakari Ailus <sakari.ailus@iki.fi>
>
> 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 ?
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
> 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 ?
> if (fh->try_fmt == NULL)
> return -ENOMEM;
>
> fh->try_crop = (struct v4l2_rect *)
> (fh->try_fmt + sd->entity.num_pads);
> +
> + fh->try_compose = (struct v4l2_rect *)
> + (fh->try_crop + sd->entity.num_pads);
> #endif
> return 0;
> }
> @@ -281,6 +285,26 @@ static long subdev_do_ioctl(struct file *file,
> unsigned int cmd, void *arg) return v4l2_subdev_call(sd, pad,
> enum_frame_interval, subdev_fh, fie);
> }
> +
> + case VIDIOC_SUBDEV_G_SELECTION: {
> + struct v4l2_subdev_selection *sel = arg;
Shouldn't you check sel->which ?
> + if (sel->pad >= sd->entity.num_pads)
> + return -EINVAL;
> +
> + return v4l2_subdev_call(
> + sd, pad, get_selection, subdev_fh, sel);
> + }
> +
> + case VIDIOC_SUBDEV_S_SELECTION: {
> + struct v4l2_subdev_selection *sel = arg;
And here too ?
> + if (sel->pad >= sd->entity.num_pads)
> + return -EINVAL;
> +
> + return v4l2_subdev_call(
> + sd, pad, set_selection, subdev_fh, sel);
> + }
> #endif
> default:
> return v4l2_subdev_call(sd, core, ioctl, cmd, arg);
> diff --git a/include/linux/v4l2-subdev.h b/include/linux/v4l2-subdev.h
> index ed29cbb..d53d775 100644
> --- a/include/linux/v4l2-subdev.h
> +++ b/include/linux/v4l2-subdev.h
> @@ -123,6 +123,47 @@ struct v4l2_subdev_frame_interval_enum {
> __u32 reserved[9];
> };
>
> +#define V4L2_SUBDEV_SEL_FLAG_SIZE_GE (1 << 0)
> +#define V4L2_SUBDEV_SEL_FLAG_SIZE_LE (1 << 1)
> +#define V4L2_SUBDEV_SEL_FLAG_KEEP_CONFIG (1 << 2)
> +
> +/* active cropping area */
> +#define V4L2_SUBDEV_SEL_TGT_CROP_ACTIVE 0
> +/* default cropping area */
> +#define V4L2_SUBDEV_SEL_TGT_CROP_DEFAULT 1
> +/* cropping bounds */
> +#define V4L2_SUBDEV_SEL_TGT_CROP_BOUNDS 2
> +/* current composing area */
> +#define V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTIVE 256
> +/* default composing area */
> +#define V4L2_SUBDEV_SEL_TGT_COMPOSE_DEFAULT 257
> +/* composing bounds */
> +#define V4L2_SUBDEV_SEL_TGT_COMPOSE_BOUNDS 258
> +
> +
> +/**
> + * struct v4l2_subdev_selection - selection info
> + *
> + * @which: either V4L2_SUBDEV_FORMAT_ACTIVE or V4L2_SUBDEV_FORMAT_TRY
> + * @pad: pad number, as reported by the media API
> + * @target: selection target, used to choose one of possible rectangles
> + * @flags: constraints flags
> + * @r: coordinates of selection window
> + * @reserved: for future use, rounds structure size to 64 bytes, set to
> zero + *
> + * Hardware may use multiple helper window to process a video stream.
> + * The structure is used to exchange this selection areas between
> + * an application and a driver.
> + */
> +struct v4l2_subdev_selection {
> + __u32 which;
> + __u32 pad;
> + __u32 target;
> + __u32 flags;
> + struct v4l2_rect r;
> + __u32 reserved[8];
> +};
> +
> #define VIDIOC_SUBDEV_G_FMT _IOWR('V', 4, struct v4l2_subdev_format)
> #define VIDIOC_SUBDEV_S_FMT _IOWR('V', 5, struct v4l2_subdev_format)
> #define VIDIOC_SUBDEV_G_FRAME_INTERVAL \
> @@ -137,5 +178,9 @@ struct v4l2_subdev_frame_interval_enum {
> _IOWR('V', 75, struct v4l2_subdev_frame_interval_enum)
> #define VIDIOC_SUBDEV_G_CROP _IOWR('V', 59, struct v4l2_subdev_crop)
> #define VIDIOC_SUBDEV_S_CROP _IOWR('V', 60, struct v4l2_subdev_crop)
> +#define VIDIOC_SUBDEV_G_SELECTION \
> + _IOWR('V', 61, struct v4l2_subdev_selection)
> +#define VIDIOC_SUBDEV_S_SELECTION \
> + _IOWR('V', 62, struct v4l2_subdev_selection)
>
> #endif
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index f0f3358..26eeaa4 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -466,6 +466,10 @@ struct v4l2_subdev_pad_ops {
> struct v4l2_subdev_crop *crop);
> int (*get_crop)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
> struct v4l2_subdev_crop *crop);
> + int (*get_selection)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
> + struct v4l2_subdev_selection *sel);
> + int (*set_selection)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
> + struct v4l2_subdev_selection *sel);
> };
>
> struct v4l2_subdev_ops {
> @@ -551,6 +555,7 @@ struct v4l2_subdev_fh {
> #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> struct v4l2_mbus_framefmt *try_fmt;
> struct v4l2_rect *try_crop;
> + struct v4l2_rect *try_compose;
> #endif
> };
Hi Laurent,
Thanks for the comments!
Laurent Pinchart wrote:
> On Tuesday 20 December 2011 21:27:56 Sakari Ailus wrote:
>> From: Sakari Ailus <sakari.ailus@iki.fi>
>>
>> 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@iki.fi>
>> ---
>> 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?
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.
>> if (fh->try_fmt == NULL)
>> return -ENOMEM;
>>
>> fh->try_crop = (struct v4l2_rect *)
>> (fh->try_fmt + sd->entity.num_pads);
>> +
>> + fh->try_compose = (struct v4l2_rect *)
>> + (fh->try_crop + sd->entity.num_pads);
>> #endif
>> return 0;
>> }
>> @@ -281,6 +285,26 @@ static long subdev_do_ioctl(struct file *file,
>> unsigned int cmd, void *arg) return v4l2_subdev_call(sd, pad,
>> enum_frame_interval, subdev_fh, fie);
>> }
>> +
>> + case VIDIOC_SUBDEV_G_SELECTION: {
>> + struct v4l2_subdev_selection *sel = arg;
>
> Shouldn't you check sel->which ?
Yes.
>> + if (sel->pad >= sd->entity.num_pads)
>> + return -EINVAL;
>> +
>> + return v4l2_subdev_call(
>> + sd, pad, get_selection, subdev_fh, sel);
>> + }
>> +
>> + case VIDIOC_SUBDEV_S_SELECTION: {
>> + struct v4l2_subdev_selection *sel = arg;
>
> And here too ?
Yes.
>> + if (sel->pad >= sd->entity.num_pads)
>> + return -EINVAL;
>> +
>> + return v4l2_subdev_call(
>> + sd, pad, set_selection, subdev_fh, sel);
>> + }
>> #endif
>> default:
>> return v4l2_subdev_call(sd, core, ioctl, cmd, arg);
>> diff --git a/include/linux/v4l2-subdev.h b/include/linux/v4l2-subdev.h
>> index ed29cbb..d53d775 100644
>> --- a/include/linux/v4l2-subdev.h
>> +++ b/include/linux/v4l2-subdev.h
>> @@ -123,6 +123,47 @@ struct v4l2_subdev_frame_interval_enum {
>> __u32 reserved[9];
>> };
>>
>> +#define V4L2_SUBDEV_SEL_FLAG_SIZE_GE (1 << 0)
>> +#define V4L2_SUBDEV_SEL_FLAG_SIZE_LE (1 << 1)
>> +#define V4L2_SUBDEV_SEL_FLAG_KEEP_CONFIG (1 << 2)
>> +
>> +/* active cropping area */
>> +#define V4L2_SUBDEV_SEL_TGT_CROP_ACTIVE 0
>> +/* default cropping area */
>> +#define V4L2_SUBDEV_SEL_TGT_CROP_DEFAULT 1
>> +/* cropping bounds */
>> +#define V4L2_SUBDEV_SEL_TGT_CROP_BOUNDS 2
>> +/* current composing area */
>> +#define V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTIVE 256
>> +/* default composing area */
>> +#define V4L2_SUBDEV_SEL_TGT_COMPOSE_DEFAULT 257
>> +/* composing bounds */
>> +#define V4L2_SUBDEV_SEL_TGT_COMPOSE_BOUNDS 258
>> +
>> +
>> +/**
>> + * struct v4l2_subdev_selection - selection info
>> + *
>> + * @which: either V4L2_SUBDEV_FORMAT_ACTIVE or V4L2_SUBDEV_FORMAT_TRY
>> + * @pad: pad number, as reported by the media API
>> + * @target: selection target, used to choose one of possible rectangles
>> + * @flags: constraints flags
>> + * @r: coordinates of selection window
>> + * @reserved: for future use, rounds structure size to 64 bytes, set to
>> zero + *
>> + * Hardware may use multiple helper window to process a video stream.
>> + * The structure is used to exchange this selection areas between
>> + * an application and a driver.
>> + */
>> +struct v4l2_subdev_selection {
>> + __u32 which;
>> + __u32 pad;
>> + __u32 target;
>> + __u32 flags;
>> + struct v4l2_rect r;
>> + __u32 reserved[8];
>> +};
>> +
>> #define VIDIOC_SUBDEV_G_FMT _IOWR('V', 4, struct v4l2_subdev_format)
>> #define VIDIOC_SUBDEV_S_FMT _IOWR('V', 5, struct v4l2_subdev_format)
>> #define VIDIOC_SUBDEV_G_FRAME_INTERVAL \
>> @@ -137,5 +178,9 @@ struct v4l2_subdev_frame_interval_enum {
>> _IOWR('V', 75, struct v4l2_subdev_frame_interval_enum)
>> #define VIDIOC_SUBDEV_G_CROP _IOWR('V', 59, struct v4l2_subdev_crop)
>> #define VIDIOC_SUBDEV_S_CROP _IOWR('V', 60, struct v4l2_subdev_crop)
>> +#define VIDIOC_SUBDEV_G_SELECTION \
>> + _IOWR('V', 61, struct v4l2_subdev_selection)
>> +#define VIDIOC_SUBDEV_S_SELECTION \
>> + _IOWR('V', 62, struct v4l2_subdev_selection)
>>
>> #endif
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index f0f3358..26eeaa4 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -466,6 +466,10 @@ struct v4l2_subdev_pad_ops {
>> struct v4l2_subdev_crop *crop);
>> int (*get_crop)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
>> struct v4l2_subdev_crop *crop);
>> + int (*get_selection)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
>> + struct v4l2_subdev_selection *sel);
>> + int (*set_selection)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
>> + struct v4l2_subdev_selection *sel);
>> };
>>
>> struct v4l2_subdev_ops {
>> @@ -551,6 +555,7 @@ struct v4l2_subdev_fh {
>> #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
>> struct v4l2_mbus_framefmt *try_fmt;
>> struct v4l2_rect *try_crop;
>> + struct v4l2_rect *try_compose;
>> #endif
>> };
>
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@iki.fi>
> >>
> >> 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@iki.fi>
> >> ---
> >>
> >> 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.
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@iki.fi>
>>>>
>>>> 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@iki.fi>
>>>> ---
>>>>
>>>> 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 wrote:
...
> 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?
And the answer to that is "no". The smia++ driver does store the format,
crop and compose values in arrays indexed by pad numbers which I think
is a natural thing for the driver to do. In many functiona the driver
uses internally it's trivial to choose the array either from driver's
internal data structure (V4L2_SUBDEV_FORMAT_ACTIVE) or the file handle
(V4L2_SUBDEV_FORMAT_TRY).
Alternatively a named struct could be created for the same, but the
drivers might not need all the fields at all, or choose to store them in
a different form.
Hi Sakari,
On Saturday 07 January 2012 12:09:29 Sakari Ailus wrote:
> Sakari Ailus wrote:
> ...
>
> > 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?
>
> And the answer to that is "no". The smia++ driver does store the format,
> crop and compose values in arrays indexed by pad numbers which I think
> is a natural thing for the driver to do. In many functiona the driver
> uses internally it's trivial to choose the array either from driver's
> internal data structure (V4L2_SUBDEV_FORMAT_ACTIVE) or the file handle
> (V4L2_SUBDEV_FORMAT_TRY).
>
> Alternatively a named struct could be created for the same, but the
> drivers might not need all the fields at all, or choose to store them in
> a different form.
Drivers should use the v4l2_subdev_get_try_format(),
v4l2_subdev_get_try_crop() and v4l2_subdev_get_try_compose() functions to
access TRY formats and selection rectangles on file handles, so they shouldn't
care about the allocation details.
Laurent Pinchart wrote:
> Hi Sakari,
>
> On Saturday 07 January 2012 12:09:29 Sakari Ailus wrote:
>> Sakari Ailus wrote:
>> ...
>>
>>> 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?
>>
>> And the answer to that is "no". The smia++ driver does store the format,
>> crop and compose values in arrays indexed by pad numbers which I think
>> is a natural thing for the driver to do. In many functiona the driver
>> uses internally it's trivial to choose the array either from driver's
>> internal data structure (V4L2_SUBDEV_FORMAT_ACTIVE) or the file handle
>> (V4L2_SUBDEV_FORMAT_TRY).
>>
>> Alternatively a named struct could be created for the same, but the
>> drivers might not need all the fields at all, or choose to store them in
>> a different form.
>
> Drivers should use the v4l2_subdev_get_try_format(),
> v4l2_subdev_get_try_crop() and v4l2_subdev_get_try_compose() functions to
> access TRY formats and selection rectangles on file handles, so they shouldn't
> care about the allocation details.
Good point... That requires some changes as well, like introduction of
v4l2_subdev_get_try_compose(). :-)
@@ -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);
if (fh->try_fmt == NULL)
return -ENOMEM;
fh->try_crop = (struct v4l2_rect *)
(fh->try_fmt + sd->entity.num_pads);
+
+ fh->try_compose = (struct v4l2_rect *)
+ (fh->try_crop + sd->entity.num_pads);
#endif
return 0;
}
@@ -281,6 +285,26 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh,
fie);
}
+
+ case VIDIOC_SUBDEV_G_SELECTION: {
+ struct v4l2_subdev_selection *sel = arg;
+
+ if (sel->pad >= sd->entity.num_pads)
+ return -EINVAL;
+
+ return v4l2_subdev_call(
+ sd, pad, get_selection, subdev_fh, sel);
+ }
+
+ case VIDIOC_SUBDEV_S_SELECTION: {
+ struct v4l2_subdev_selection *sel = arg;
+
+ if (sel->pad >= sd->entity.num_pads)
+ return -EINVAL;
+
+ return v4l2_subdev_call(
+ sd, pad, set_selection, subdev_fh, sel);
+ }
#endif
default:
return v4l2_subdev_call(sd, core, ioctl, cmd, arg);
@@ -123,6 +123,47 @@ struct v4l2_subdev_frame_interval_enum {
__u32 reserved[9];
};
+#define V4L2_SUBDEV_SEL_FLAG_SIZE_GE (1 << 0)
+#define V4L2_SUBDEV_SEL_FLAG_SIZE_LE (1 << 1)
+#define V4L2_SUBDEV_SEL_FLAG_KEEP_CONFIG (1 << 2)
+
+/* active cropping area */
+#define V4L2_SUBDEV_SEL_TGT_CROP_ACTIVE 0
+/* default cropping area */
+#define V4L2_SUBDEV_SEL_TGT_CROP_DEFAULT 1
+/* cropping bounds */
+#define V4L2_SUBDEV_SEL_TGT_CROP_BOUNDS 2
+/* current composing area */
+#define V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTIVE 256
+/* default composing area */
+#define V4L2_SUBDEV_SEL_TGT_COMPOSE_DEFAULT 257
+/* composing bounds */
+#define V4L2_SUBDEV_SEL_TGT_COMPOSE_BOUNDS 258
+
+
+/**
+ * struct v4l2_subdev_selection - selection info
+ *
+ * @which: either V4L2_SUBDEV_FORMAT_ACTIVE or V4L2_SUBDEV_FORMAT_TRY
+ * @pad: pad number, as reported by the media API
+ * @target: selection target, used to choose one of possible rectangles
+ * @flags: constraints flags
+ * @r: coordinates of selection window
+ * @reserved: for future use, rounds structure size to 64 bytes, set to zero
+ *
+ * Hardware may use multiple helper window to process a video stream.
+ * The structure is used to exchange this selection areas between
+ * an application and a driver.
+ */
+struct v4l2_subdev_selection {
+ __u32 which;
+ __u32 pad;
+ __u32 target;
+ __u32 flags;
+ struct v4l2_rect r;
+ __u32 reserved[8];
+};
+
#define VIDIOC_SUBDEV_G_FMT _IOWR('V', 4, struct v4l2_subdev_format)
#define VIDIOC_SUBDEV_S_FMT _IOWR('V', 5, struct v4l2_subdev_format)
#define VIDIOC_SUBDEV_G_FRAME_INTERVAL \
@@ -137,5 +178,9 @@ struct v4l2_subdev_frame_interval_enum {
_IOWR('V', 75, struct v4l2_subdev_frame_interval_enum)
#define VIDIOC_SUBDEV_G_CROP _IOWR('V', 59, struct v4l2_subdev_crop)
#define VIDIOC_SUBDEV_S_CROP _IOWR('V', 60, struct v4l2_subdev_crop)
+#define VIDIOC_SUBDEV_G_SELECTION \
+ _IOWR('V', 61, struct v4l2_subdev_selection)
+#define VIDIOC_SUBDEV_S_SELECTION \
+ _IOWR('V', 62, struct v4l2_subdev_selection)
#endif
@@ -466,6 +466,10 @@ struct v4l2_subdev_pad_ops {
struct v4l2_subdev_crop *crop);
int (*get_crop)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
struct v4l2_subdev_crop *crop);
+ int (*get_selection)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
+ struct v4l2_subdev_selection *sel);
+ int (*set_selection)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
+ struct v4l2_subdev_selection *sel);
};
struct v4l2_subdev_ops {
@@ -551,6 +555,7 @@ struct v4l2_subdev_fh {
#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
struct v4l2_mbus_framefmt *try_fmt;
struct v4l2_rect *try_crop;
+ struct v4l2_rect *try_compose;
#endif
};