Re: subdev sensor driver and V4L2_FRMIVAL_TYPE_CONTINUOUS/V4L2_FRMIVAL_TYPE_STEPWISE

[Sakari Ailus <sakari.ailus@iki.fi>]

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