subdev sensor driver and V4L2_FRMIVAL_TYPE_CONTINUOUS/V4L2_FRMIVAL_TYPE_STEPWISE

subdev sensor driver and V4L2_FRMIVAL_TYPE_CONTINUOUS/V4L2_FRMIVAL_TYPE_STEPWISE

[Philippe De Muyter]

Hi,

Sorry if you read the following twice, but the subject of my previous post
was not precise enough :(

I am in the process of converting a sensor driver compatible with the imx6
freescale linux kernel, to a subdev driver compatible with a current kernel
and Steve Longerbeam's work.

My sensor can work at any fps (even fractional) up to 60 fps with its default
frame size or even higher when using crop or "binning'.  That fact is reflected
in my previous implemetation of VIDIOC_ENUM_FRAMEINTERVALS, which answered
with a V4L2_FRMIVAL_TYPE_CONTINUOUS-type reply.

This seem not possible anymore because of the lack of the needed fields
in the 'struct v4l2_subdev_frame_interval_enum' used to delegate the question
to the subdev driver.  V4L2_FRMIVAL_TYPE_STEPWISE does not seem possible
anymore either.  Has that been replaced by something else or is that
functionality not considered relevant anymorea ?

Best regards

Philippe

-- 
Philippe De Muyter +32 2 6101532 Macq SA rue de l'Aeronef 2 B-1140 Bruxelles

Re: subdev sensor driver and V4L2_FRMIVAL_TYPE_CONTINUOUS/V4L2_FRMIVAL_TYPE_STEPWISE

[Sakari Ailus]

Hi Philippe,

On Tue, Mar 15, 2016 at 11:14:17AM +0100, Philippe De Muyter wrote:
> Hi,
> 
> Sorry if you read the following twice, but the subject of my previous post
> was not precise enough :(
> 
> I am in the process of converting a sensor driver compatible with the imx6
> freescale linux kernel, to a subdev driver compatible with a current kernel
> and Steve Longerbeam's work.
> 
> My sensor can work at any fps (even fractional) up to 60 fps with its default
> frame size or even higher when using crop or "binning'.  That fact is reflected
> in my previous implemetation of VIDIOC_ENUM_FRAMEINTERVALS, which answered
> with a V4L2_FRMIVAL_TYPE_CONTINUOUS-type reply.
> 
> This seem not possible anymore because of the lack of the needed fields
> in the 'struct v4l2_subdev_frame_interval_enum' used to delegate the question
> to the subdev driver.  V4L2_FRMIVAL_TYPE_STEPWISE does not seem possible
> anymore either.  Has that been replaced by something else or is that
> functionality not considered relevant anymorea ?

I think the issue was that the CONTINUOUS and STEPWISE were considered too
clumsy for applications and practically no application was using them, or at
least the need for these was not seen to be there. They were not added to
the V4L2 sub-device implementation of the interface as a result.

Cc Hans.

The smiapp driver uses horizontal and vertical blanking controls for
changing the frame rate. That's a bit lower level interface than most
drivers use, but then you have to provide enough other information to the
user space as well, including the pixel rate.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: subdev sensor driver and V4L2_FRMIVAL_TYPE_CONTINUOUS/V4L2_FRMIVAL_TYPE_STEPWISE

[Hans Verkuil]

On 03/15/16 12:06, Sakari Ailus wrote:
> Hi Philippe,
> 
> On Tue, Mar 15, 2016 at 11:14:17AM +0100, Philippe De Muyter wrote:
>> Hi,
>>
>> Sorry if you read the following twice, but the subject of my previous post
>> was not precise enough :(
>>
>> I am in the process of converting a sensor driver compatible with the imx6
>> freescale linux kernel, to a subdev driver compatible with a current kernel
>> and Steve Longerbeam's work.
>>
>> My sensor can work at any fps (even fractional) up to 60 fps with its default
>> frame size or even higher when using crop or "binning'.  That fact is reflected
>> in my previous implemetation of VIDIOC_ENUM_FRAMEINTERVALS, which answered
>> with a V4L2_FRMIVAL_TYPE_CONTINUOUS-type reply.
>>
>> This seem not possible anymore because of the lack of the needed fields
>> in the 'struct v4l2_subdev_frame_interval_enum' used to delegate the question
>> to the subdev driver.  V4L2_FRMIVAL_TYPE_STEPWISE does not seem possible
>> anymore either.  Has that been replaced by something else or is that
>> functionality not considered relevant anymorea ?
> 
> I think the issue was that the CONTINUOUS and STEPWISE were considered too
> clumsy for applications and practically no application was using them, or at
> least the need for these was not seen to be there. They were not added to
> the V4L2 sub-device implementation of the interface as a result.

It never made sense to me why the two APIs weren't kept the same.

My suggestion would be to add step_width and step_height fields to
struct v4l2_subdev_frame_size_enum, that way you have the same functionality
back.

I.e. for discrete formats the min values equal the max values, for continuous
formats the step values are both 1 (or 0 for compability's sake) and the
remainder maps to a stepwise range.

Regards,

	Hans

> 
> Cc Hans.
> 
> The smiapp driver uses horizontal and vertical blanking controls for
> changing the frame rate. That's a bit lower level interface than most
> drivers use, but then you have to provide enough other information to the
> user space as well, including the pixel rate.
> 

Re: subdev sensor driver and V4L2_FRMIVAL_TYPE_CONTINUOUS/V4L2_FRMIVAL_TYPE_STEPWISE

[Philippe De Muyter]

My post is about framerate, not framesize :)  I agree the macro's
names are misleading.

On Tue, Mar 15, 2016 at 12:54:22PM +0100, Hans Verkuil wrote:
> On 03/15/16 12:06, Sakari Ailus wrote:
> > Hi Philippe,
> > 
> > On Tue, Mar 15, 2016 at 11:14:17AM +0100, Philippe De Muyter wrote:
> >> Hi,
> >>
> >> Sorry if you read the following twice, but the subject of my previous post
> >> was not precise enough :(
> >>
> >> I am in the process of converting a sensor driver compatible with the imx6
> >> freescale linux kernel, to a subdev driver compatible with a current kernel
> >> and Steve Longerbeam's work.
> >>
> >> My sensor can work at any fps (even fractional) up to 60 fps with its default
> >> frame size or even higher when using crop or "binning'.  That fact is reflected
> >> in my previous implemetation of VIDIOC_ENUM_FRAMEINTERVALS, which answered
> >> with a V4L2_FRMIVAL_TYPE_CONTINUOUS-type reply.
> >>
> >> This seem not possible anymore because of the lack of the needed fields
> >> in the 'struct v4l2_subdev_frame_interval_enum' used to delegate the question
> >> to the subdev driver.  V4L2_FRMIVAL_TYPE_STEPWISE does not seem possible
> >> anymore either.  Has that been replaced by something else or is that
> >> functionality not considered relevant anymorea ?
> > 
> > I think the issue was that the CONTINUOUS and STEPWISE were considered too
> > clumsy for applications and practically no application was using them, or at
> > least the need for these was not seen to be there. They were not added to
> > the V4L2 sub-device implementation of the interface as a result.
> 
> It never made sense to me why the two APIs weren't kept the same.

I agree completely with that.

> 
> My suggestion would be to add step_width and step_height fields to
> struct v4l2_subdev_frame_size_enum, that way you have the same functionality
> back.
> 
> I.e. for discrete formats the min values equal the max values, for continuous
> formats the step values are both 1 (or 0 for compability's sake) and the
> remainder maps to a stepwise range.
> 
> Regards,
> 
> 	Hans
> 
> > 
> > Cc Hans.
> > 
> > The smiapp driver uses horizontal and vertical blanking controls for
> > changing the frame rate. That's a bit lower level interface than most
> > drivers use, but then you have to provide enough other information to the
> > user space as well, including the pixel rate.
> > 

Philippe

Re: subdev sensor driver and V4L2_FRMIVAL_TYPE_CONTINUOUS/V4L2_FRMIVAL_TYPE_STEPWISE

[Hans Verkuil]

On 03/15/16 12:58, Philippe De Muyter wrote:
> My post is about framerate, not framesize :)  I agree the macro's
> names are misleading.

Oops. Well, the solution would be similar (i.e. adding max_interval and step_interval
fields to struct v4l2_subdev_frame_interval_enum. It takes away 4 fields from the
reserved array, but this should have been done right from the start :-(

Regards,

	Hans

> 
> On Tue, Mar 15, 2016 at 12:54:22PM +0100, Hans Verkuil wrote:
>> On 03/15/16 12:06, Sakari Ailus wrote:
>>> Hi Philippe,
>>>
>>> On Tue, Mar 15, 2016 at 11:14:17AM +0100, Philippe De Muyter wrote:
>>>> Hi,
>>>>
>>>> Sorry if you read the following twice, but the subject of my previous post
>>>> was not precise enough :(
>>>>
>>>> I am in the process of converting a sensor driver compatible with the imx6
>>>> freescale linux kernel, to a subdev driver compatible with a current kernel
>>>> and Steve Longerbeam's work.
>>>>
>>>> My sensor can work at any fps (even fractional) up to 60 fps with its default
>>>> frame size or even higher when using crop or "binning'.  That fact is reflected
>>>> in my previous implemetation of VIDIOC_ENUM_FRAMEINTERVALS, which answered
>>>> with a V4L2_FRMIVAL_TYPE_CONTINUOUS-type reply.
>>>>
>>>> This seem not possible anymore because of the lack of the needed fields
>>>> in the 'struct v4l2_subdev_frame_interval_enum' used to delegate the question
>>>> to the subdev driver.  V4L2_FRMIVAL_TYPE_STEPWISE does not seem possible
>>>> anymore either.  Has that been replaced by something else or is that
>>>> functionality not considered relevant anymorea ?
>>>
>>> I think the issue was that the CONTINUOUS and STEPWISE were considered too
>>> clumsy for applications and practically no application was using them, or at
>>> least the need for these was not seen to be there. They were not added to
>>> the V4L2 sub-device implementation of the interface as a result.
>>
>> It never made sense to me why the two APIs weren't kept the same.
> 
> I agree completely with that.
> 
>>
>> My suggestion would be to add step_width and step_height fields to
>> struct v4l2_subdev_frame_size_enum, that way you have the same functionality
>> back.
>>
>> I.e. for discrete formats the min values equal the max values, for continuous
>> formats the step values are both 1 (or 0 for compability's sake) and the
>> remainder maps to a stepwise range.
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>> Cc Hans.
>>>
>>> The smiapp driver uses horizontal and vertical blanking controls for
>>> changing the frame rate. That's a bit lower level interface than most
>>> drivers use, but then you have to provide enough other information to the
>>> user space as well, including the pixel rate.
>>>
> 
> Philippe
> 

Re: subdev sensor driver and V4L2_FRMIVAL_TYPE_CONTINUOUS/V4L2_FRMIVAL_TYPE_STEPWISE

[Sakari Ailus]

Hi Hans,

On Tue, Mar 15, 2016 at 12:54:22PM +0100, Hans Verkuil wrote:
> On 03/15/16 12:06, Sakari Ailus wrote:
> > Hi Philippe,
> > 
> > On Tue, Mar 15, 2016 at 11:14:17AM +0100, Philippe De Muyter wrote:
> >> Hi,
> >>
> >> Sorry if you read the following twice, but the subject of my previous post
> >> was not precise enough :(
> >>
> >> I am in the process of converting a sensor driver compatible with the imx6
> >> freescale linux kernel, to a subdev driver compatible with a current kernel
> >> and Steve Longerbeam's work.
> >>
> >> My sensor can work at any fps (even fractional) up to 60 fps with its default
> >> frame size or even higher when using crop or "binning'.  That fact is reflected
> >> in my previous implemetation of VIDIOC_ENUM_FRAMEINTERVALS, which answered
> >> with a V4L2_FRMIVAL_TYPE_CONTINUOUS-type reply.
> >>
> >> This seem not possible anymore because of the lack of the needed fields
> >> in the 'struct v4l2_subdev_frame_interval_enum' used to delegate the question
> >> to the subdev driver.  V4L2_FRMIVAL_TYPE_STEPWISE does not seem possible
> >> anymore either.  Has that been replaced by something else or is that
> >> functionality not considered relevant anymorea ?
> > 
> > I think the issue was that the CONTINUOUS and STEPWISE were considered too
> > clumsy for applications and practically no application was using them, or at
> > least the need for these was not seen to be there. They were not added to
> > the V4L2 sub-device implementation of the interface as a result.
> 
> It never made sense to me why the two APIs weren't kept the same.
> 
> My suggestion would be to add step_width and step_height fields to
> struct v4l2_subdev_frame_size_enum, that way you have the same functionality
> back.
> 
> I.e. for discrete formats the min values equal the max values, for continuous
> formats the step values are both 1 (or 0 for compability's sake) and the
> remainder maps to a stepwise range.

On frame intervals... I'm not against changing it if there's a need to.

Cc'ing Laurent who I believe wrote the original API. I probably acked it.

We just needed to add the type field (snatch one from the reserved array),
DISCRETE translates to zero so there are on compatibility issues either.
struct v4l2_frmival_stepwise will consume all remaining reserved entries
with the exception of one, but there is still room for it.

Documentation is needed as well.

I don't think it'd be very pretty but I guess there's no way around that at
this point. We should have had the which field before the interval field.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[PATCH 1/2] [media] v4l2-subdev.h: allow V4L2_FRMIVAL_TYPE_CONTINUOUS & _STEPWISE

[Philippe De Muyter]

add max_interval and step_interval to struct
v4l2_subdev_frame_interval_enum.

When filled correctly by the sensor driver, those fields must be
used as follows by the intermediate level :

	struct v4l2_frmivalenum *fival;
	struct v4l2_subdev_frame_interval_enum fie;

	if (fie.max_interval.numerator == 0) {
		fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
		fival->discrete = fie.interval;
	} else if (fie.step_interval.numerator == 0) {
		fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
		fival->stepwise.min = fie.interval;
		fival->stepwise.max = fie.max_interval;
	} else {
		fival->type = V4L2_FRMIVAL_TYPE_STEPWISE;
		fival->stepwise.min = fie.interval;
		fival->stepwise.max = fie.max_interval;
		fival->stepwise.step = fie.step_interval;
	}

Signed-off-by: Philippe De Muyter <phdm@macqel.be>
---
 include/uapi/linux/v4l2-subdev.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
index dbce2b554..846dd36 100644
--- a/include/uapi/linux/v4l2-subdev.h
+++ b/include/uapi/linux/v4l2-subdev.h
@@ -127,7 +127,9 @@ struct v4l2_subdev_frame_interval_enum {
 	__u32 height;
 	struct v4l2_fract interval;
 	__u32 which;
-	__u32 reserved[8];
+	struct v4l2_fract max_interval;
+	struct v4l2_fract step_interval;
+	__u32 reserved[4];
 };
 
 /**
-- 
1.8.1.4

[PATCH 2/2] media: mx6-camif: add V4L2_FRMIVAL_TYPE_STEPWISE & _CONTINUOUS

[Philippe De Muyter]

Signed-off-by: Philippe De Muyter <phdm@macqel.be>
---
 drivers/staging/media/imx6/capture/mx6-camif.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/imx6/capture/mx6-camif.c b/drivers/staging/media/imx6/capture/mx6-camif.c
index b36f9d1..137733e 100644
--- a/drivers/staging/media/imx6/capture/mx6-camif.c
+++ b/drivers/staging/media/imx6/capture/mx6-camif.c
@@ -1344,8 +1344,19 @@ static int vidioc_enum_frameintervals(struct file *file, void *priv,
 	if (ret)
 		return ret;
 
-	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
-	fival->discrete = fie.interval;
+	if (fie.max_interval.numerator == 0) {
+		fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
+		fival->discrete = fie.interval;
+	} else if (fie.step_interval.numerator == 0) {
+		fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
+		fival->stepwise.min = fie.interval;
+		fival->stepwise.max = fie.max_interval;
+	} else {
+		fival->type = V4L2_FRMIVAL_TYPE_STEPWISE;
+		fival->stepwise.min = fie.interval;
+		fival->stepwise.max = fie.max_interval;
+		fival->stepwise.step = fie.step_interval;
+	}
 	return 0;
 }
 
-- 
1.8.1.4

Re: [PATCH 1/2] [media] v4l2-subdev.h: allow V4L2_FRMIVAL_TYPE_CONTINUOUS & _STEPWISE

[Mauro Carvalho Chehab]

Em Sun, 12 Jun 2016 00:20:59 +0200
Philippe De Muyter <phdm@macqel.be> escreveu:

> add max_interval and step_interval to struct
> v4l2_subdev_frame_interval_enum.
> 
> When filled correctly by the sensor driver, those fields must be
> used as follows by the intermediate level :
> 
> 	struct v4l2_frmivalenum *fival;
> 	struct v4l2_subdev_frame_interval_enum fie;
> 
> 	if (fie.max_interval.numerator == 0) {
> 		fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
> 		fival->discrete = fie.interval;
> 	} else if (fie.step_interval.numerator == 0) {
> 		fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
> 		fival->stepwise.min = fie.interval;
> 		fival->stepwise.max = fie.max_interval;
> 	} else {
> 		fival->type = V4L2_FRMIVAL_TYPE_STEPWISE;
> 		fival->stepwise.min = fie.interval;
> 		fival->stepwise.max = fie.max_interval;
> 		fival->stepwise.step = fie.step_interval;
> 	}
> 
> Signed-off-by: Philippe De Muyter <phdm@macqel.be>
> ---
>  include/uapi/linux/v4l2-subdev.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> index dbce2b554..846dd36 100644
> --- a/include/uapi/linux/v4l2-subdev.h
> +++ b/include/uapi/linux/v4l2-subdev.h
> @@ -127,7 +127,9 @@ struct v4l2_subdev_frame_interval_enum {
>  	__u32 height;
>  	struct v4l2_fract interval;
>  	__u32 which;
> -	__u32 reserved[8];
> +	struct v4l2_fract max_interval;
> +	struct v4l2_fract step_interval;
> +	__u32 reserved[4];

As this changes the userspace API, you need to also patch the
Documentation/media to reflect such change and explain the meaning
for the new fields.

Please notice that the rst documentation is under the "docs-next"
branch at the media_tree.git. You should either patch against that
or wait for the end for 4.8-rc1, where the documentation will be
merged back on the master branch.

>  };
>  
>  /**


-- 
Thanks,
Mauro