* [PATCHv2 1/9] v4l2-common: create v4l2_g/s_parm_cap helpers
2018-01-22 12:31 [PATCHv2 0/9] media: replace g/s_parm by g/s_frame_interval Hans Verkuil
@ 2018-01-22 12:31 ` Hans Verkuil
2018-02-14 15:50 ` Mauro Carvalho Chehab
2018-01-22 12:31 ` [PATCHv2 2/9] media: convert g/s_parm to g/s_frame_interval in subdevs Hans Verkuil
` (8 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Hans Verkuil @ 2018-01-22 12:31 UTC (permalink / raw)
To: linux-media; +Cc: Sakari Ailus, Jacopo Mondi, Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
Create helpers to handle VIDIOC_G/S_PARM by querying the
g/s_frame_interval v4l2_subdev ops.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/v4l2-core/v4l2-common.c | 48 +++++++++++++++++++++++++++++++++++
include/media/v4l2-common.h | 26 +++++++++++++++++++
2 files changed, 74 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 8650ad92b64d..96c1b31de9e3 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -392,3 +392,51 @@ void v4l2_get_timestamp(struct timeval *tv)
tv->tv_usec = ts.tv_nsec / NSEC_PER_USEC;
}
EXPORT_SYMBOL_GPL(v4l2_get_timestamp);
+
+int v4l2_g_parm_cap(struct video_device *vdev,
+ struct v4l2_subdev *sd, struct v4l2_streamparm *a)
+{
+ struct v4l2_subdev_frame_interval ival = { 0 };
+ int ret;
+
+ if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
+ a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
+ return -EINVAL;
+
+ if (vdev->device_caps & V4L2_CAP_READWRITE)
+ a->parm.capture.readbuffers = 2;
+ if (v4l2_subdev_has_op(sd, video, g_frame_interval))
+ a->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
+ ret = v4l2_subdev_call(sd, video, g_frame_interval, &ival);
+ if (!ret)
+ a->parm.capture.timeperframe = ival.interval;
+ return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_g_parm_cap);
+
+int v4l2_s_parm_cap(struct video_device *vdev,
+ struct v4l2_subdev *sd, struct v4l2_streamparm *a)
+{
+ struct v4l2_subdev_frame_interval ival = {
+ .interval = a->parm.capture.timeperframe
+ };
+ int ret;
+
+ if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
+ a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
+ return -EINVAL;
+
+ memset(&a->parm, 0, sizeof(a->parm));
+ if (vdev->device_caps & V4L2_CAP_READWRITE)
+ a->parm.capture.readbuffers = 2;
+ else
+ a->parm.capture.readbuffers = 0;
+
+ if (v4l2_subdev_has_op(sd, video, g_frame_interval))
+ a->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
+ ret = v4l2_subdev_call(sd, video, s_frame_interval, &ival);
+ if (!ret)
+ a->parm.capture.timeperframe = ival.interval;
+ return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_s_parm_cap);
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index e0d95a7c5d48..f3aa1d728c0b 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -341,4 +341,30 @@ v4l2_find_nearest_format(const struct v4l2_frmsize_discrete *sizes,
*/
void v4l2_get_timestamp(struct timeval *tv);
+/**
+ * v4l2_g_parm_cap - helper routine for vidioc_g_parm to fill this in by
+ * calling the g_frame_interval op of the given subdev. It only works
+ * for V4L2_BUF_TYPE_VIDEO_CAPTURE(_MPLANE), hence the _cap in the
+ * function name.
+ *
+ * @vdev: the struct video_device pointer. Used to determine the device caps.
+ * @sd: the sub-device pointer.
+ * @a: the VIDIOC_G_PARM argument.
+ */
+int v4l2_g_parm_cap(struct video_device *vdev,
+ struct v4l2_subdev *sd, struct v4l2_streamparm *a);
+
+/**
+ * v4l2_s_parm_cap - helper routine for vidioc_s_parm to fill this in by
+ * calling the s_frame_interval op of the given subdev. It only works
+ * for V4L2_BUF_TYPE_VIDEO_CAPTURE(_MPLANE), hence the _cap in the
+ * function name.
+ *
+ * @vdev: the struct video_device pointer. Used to determine the device caps.
+ * @sd: the sub-device pointer.
+ * @a: the VIDIOC_S_PARM argument.
+ */
+int v4l2_s_parm_cap(struct video_device *vdev,
+ struct v4l2_subdev *sd, struct v4l2_streamparm *a);
+
#endif /* V4L2_COMMON_H_ */
--
2.15.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCHv2 1/9] v4l2-common: create v4l2_g/s_parm_cap helpers
2018-01-22 12:31 ` [PATCHv2 1/9] v4l2-common: create v4l2_g/s_parm_cap helpers Hans Verkuil
@ 2018-02-14 15:50 ` Mauro Carvalho Chehab
2018-02-14 16:23 ` Hans Verkuil
0 siblings, 1 reply; 35+ messages in thread
From: Mauro Carvalho Chehab @ 2018-02-14 15:50 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Sakari Ailus, Jacopo Mondi, Hans Verkuil
Em Mon, 22 Jan 2018 13:31:17 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> Create helpers to handle VIDIOC_G/S_PARM by querying the
> g/s_frame_interval v4l2_subdev ops.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/v4l2-core/v4l2-common.c | 48 +++++++++++++++++++++++++++++++++++
> include/media/v4l2-common.h | 26 +++++++++++++++++++
> 2 files changed, 74 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index 8650ad92b64d..96c1b31de9e3 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -392,3 +392,51 @@ void v4l2_get_timestamp(struct timeval *tv)
> tv->tv_usec = ts.tv_nsec / NSEC_PER_USEC;
> }
> EXPORT_SYMBOL_GPL(v4l2_get_timestamp);
> +
> +int v4l2_g_parm_cap(struct video_device *vdev,
> + struct v4l2_subdev *sd, struct v4l2_streamparm *a)
> +{
> + struct v4l2_subdev_frame_interval ival = { 0 };
> + int ret;
> +
> + if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
> + a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> + return -EINVAL;
> +
> + if (vdev->device_caps & V4L2_CAP_READWRITE)
> + a->parm.capture.readbuffers = 2;
Hmm... why don't you also initialize readbuffers otherwise?
> + if (v4l2_subdev_has_op(sd, video, g_frame_interval))
> + a->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> + ret = v4l2_subdev_call(sd, video, g_frame_interval, &ival);
> + if (!ret)
> + a->parm.capture.timeperframe = ival.interval;
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_g_parm_cap);
> +
> +int v4l2_s_parm_cap(struct video_device *vdev,
> + struct v4l2_subdev *sd, struct v4l2_streamparm *a)
> +{
> + struct v4l2_subdev_frame_interval ival = {
> + .interval = a->parm.capture.timeperframe
> + };
> + int ret;
> +
> + if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
> + a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> + return -EINVAL;
> +
> + memset(&a->parm, 0, sizeof(a->parm));
> + if (vdev->device_caps & V4L2_CAP_READWRITE)
> + a->parm.capture.readbuffers = 2;
> + else
> + a->parm.capture.readbuffers = 0;
> +
> + if (v4l2_subdev_has_op(sd, video, g_frame_interval))
> + a->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> + ret = v4l2_subdev_call(sd, video, s_frame_interval, &ival);
> + if (!ret)
> + a->parm.capture.timeperframe = ival.interval;
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_s_parm_cap);
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index e0d95a7c5d48..f3aa1d728c0b 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -341,4 +341,30 @@ v4l2_find_nearest_format(const struct v4l2_frmsize_discrete *sizes,
> */
> void v4l2_get_timestamp(struct timeval *tv);
>
> +/**
> + * v4l2_g_parm_cap - helper routine for vidioc_g_parm to fill this in by
> + * calling the g_frame_interval op of the given subdev. It only works
> + * for V4L2_BUF_TYPE_VIDEO_CAPTURE(_MPLANE), hence the _cap in the
> + * function name.
> + *
> + * @vdev: the struct video_device pointer. Used to determine the device caps.
> + * @sd: the sub-device pointer.
> + * @a: the VIDIOC_G_PARM argument.
> + */
> +int v4l2_g_parm_cap(struct video_device *vdev,
> + struct v4l2_subdev *sd, struct v4l2_streamparm *a);
> +
> +/**
> + * v4l2_s_parm_cap - helper routine for vidioc_s_parm to fill this in by
> + * calling the s_frame_interval op of the given subdev. It only works
> + * for V4L2_BUF_TYPE_VIDEO_CAPTURE(_MPLANE), hence the _cap in the
> + * function name.
> + *
> + * @vdev: the struct video_device pointer. Used to determine the device caps.
> + * @sd: the sub-device pointer.
> + * @a: the VIDIOC_S_PARM argument.
> + */
> +int v4l2_s_parm_cap(struct video_device *vdev,
> + struct v4l2_subdev *sd, struct v4l2_streamparm *a);
> +
> #endif /* V4L2_COMMON_H_ */
Thanks,
Mauro
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCHv2 1/9] v4l2-common: create v4l2_g/s_parm_cap helpers
2018-02-14 15:50 ` Mauro Carvalho Chehab
@ 2018-02-14 16:23 ` Hans Verkuil
2018-02-14 16:35 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 35+ messages in thread
From: Hans Verkuil @ 2018-02-14 16:23 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: linux-media, Sakari Ailus, Jacopo Mondi, Hans Verkuil
On 14/02/18 16:50, Mauro Carvalho Chehab wrote:
> Em Mon, 22 Jan 2018 13:31:17 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Create helpers to handle VIDIOC_G/S_PARM by querying the
>> g/s_frame_interval v4l2_subdev ops.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>> drivers/media/v4l2-core/v4l2-common.c | 48 +++++++++++++++++++++++++++++++++++
>> include/media/v4l2-common.h | 26 +++++++++++++++++++
>> 2 files changed, 74 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
>> index 8650ad92b64d..96c1b31de9e3 100644
>> --- a/drivers/media/v4l2-core/v4l2-common.c
>> +++ b/drivers/media/v4l2-core/v4l2-common.c
>> @@ -392,3 +392,51 @@ void v4l2_get_timestamp(struct timeval *tv)
>> tv->tv_usec = ts.tv_nsec / NSEC_PER_USEC;
>> }
>> EXPORT_SYMBOL_GPL(v4l2_get_timestamp);
>> +
>> +int v4l2_g_parm_cap(struct video_device *vdev,
>> + struct v4l2_subdev *sd, struct v4l2_streamparm *a)
>> +{
>> + struct v4l2_subdev_frame_interval ival = { 0 };
>> + int ret;
>> +
>> + if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
>> + a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
>> + return -EINVAL;
>> +
>> + if (vdev->device_caps & V4L2_CAP_READWRITE)
>> + a->parm.capture.readbuffers = 2;
>
> Hmm... why don't you also initialize readbuffers otherwise?
It's specifically for read(). If read() is not supported, then this
is meaningless and should just stay 0. v4l2-compliance checks for this.
The 'readbuffers' field is completely outdated and once this is in
the next step is to see if we can come up with something better. I hate
G/S_PARM.
Regards,
Hans
>
>> + if (v4l2_subdev_has_op(sd, video, g_frame_interval))
>> + a->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
>> + ret = v4l2_subdev_call(sd, video, g_frame_interval, &ival);
>> + if (!ret)
>> + a->parm.capture.timeperframe = ival.interval;
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_g_parm_cap);
>> +
>> +int v4l2_s_parm_cap(struct video_device *vdev,
>> + struct v4l2_subdev *sd, struct v4l2_streamparm *a)
>> +{
>> + struct v4l2_subdev_frame_interval ival = {
>> + .interval = a->parm.capture.timeperframe
>> + };
>> + int ret;
>> +
>> + if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
>> + a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
>> + return -EINVAL;
>> +
>> + memset(&a->parm, 0, sizeof(a->parm));
>> + if (vdev->device_caps & V4L2_CAP_READWRITE)
>> + a->parm.capture.readbuffers = 2;
>> + else
>> + a->parm.capture.readbuffers = 0;
>> +
>> + if (v4l2_subdev_has_op(sd, video, g_frame_interval))
>> + a->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
>> + ret = v4l2_subdev_call(sd, video, s_frame_interval, &ival);
>> + if (!ret)
>> + a->parm.capture.timeperframe = ival.interval;
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_s_parm_cap);
>> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
>> index e0d95a7c5d48..f3aa1d728c0b 100644
>> --- a/include/media/v4l2-common.h
>> +++ b/include/media/v4l2-common.h
>> @@ -341,4 +341,30 @@ v4l2_find_nearest_format(const struct v4l2_frmsize_discrete *sizes,
>> */
>> void v4l2_get_timestamp(struct timeval *tv);
>>
>> +/**
>> + * v4l2_g_parm_cap - helper routine for vidioc_g_parm to fill this in by
>> + * calling the g_frame_interval op of the given subdev. It only works
>> + * for V4L2_BUF_TYPE_VIDEO_CAPTURE(_MPLANE), hence the _cap in the
>> + * function name.
>> + *
>> + * @vdev: the struct video_device pointer. Used to determine the device caps.
>> + * @sd: the sub-device pointer.
>> + * @a: the VIDIOC_G_PARM argument.
>> + */
>> +int v4l2_g_parm_cap(struct video_device *vdev,
>> + struct v4l2_subdev *sd, struct v4l2_streamparm *a);
>> +
>> +/**
>> + * v4l2_s_parm_cap - helper routine for vidioc_s_parm to fill this in by
>> + * calling the s_frame_interval op of the given subdev. It only works
>> + * for V4L2_BUF_TYPE_VIDEO_CAPTURE(_MPLANE), hence the _cap in the
>> + * function name.
>> + *
>> + * @vdev: the struct video_device pointer. Used to determine the device caps.
>> + * @sd: the sub-device pointer.
>> + * @a: the VIDIOC_S_PARM argument.
>> + */
>> +int v4l2_s_parm_cap(struct video_device *vdev,
>> + struct v4l2_subdev *sd, struct v4l2_streamparm *a);
>> +
>> #endif /* V4L2_COMMON_H_ */
>
>
>
> Thanks,
> Mauro
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCHv2 1/9] v4l2-common: create v4l2_g/s_parm_cap helpers
2018-02-14 16:23 ` Hans Verkuil
@ 2018-02-14 16:35 ` Mauro Carvalho Chehab
2018-02-14 16:47 ` Hans Verkuil
0 siblings, 1 reply; 35+ messages in thread
From: Mauro Carvalho Chehab @ 2018-02-14 16:35 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Sakari Ailus, Jacopo Mondi, Hans Verkuil
Em Wed, 14 Feb 2018 17:23:51 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> On 14/02/18 16:50, Mauro Carvalho Chehab wrote:
> > Em Mon, 22 Jan 2018 13:31:17 +0100
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >
> >> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>
> >> Create helpers to handle VIDIOC_G/S_PARM by querying the
> >> g/s_frame_interval v4l2_subdev ops.
> >>
> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> ---
> >> drivers/media/v4l2-core/v4l2-common.c | 48 +++++++++++++++++++++++++++++++++++
> >> include/media/v4l2-common.h | 26 +++++++++++++++++++
> >> 2 files changed, 74 insertions(+)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> >> index 8650ad92b64d..96c1b31de9e3 100644
> >> --- a/drivers/media/v4l2-core/v4l2-common.c
> >> +++ b/drivers/media/v4l2-core/v4l2-common.c
> >> @@ -392,3 +392,51 @@ void v4l2_get_timestamp(struct timeval *tv)
> >> tv->tv_usec = ts.tv_nsec / NSEC_PER_USEC;
> >> }
> >> EXPORT_SYMBOL_GPL(v4l2_get_timestamp);
> >> +
> >> +int v4l2_g_parm_cap(struct video_device *vdev,
> >> + struct v4l2_subdev *sd, struct v4l2_streamparm *a)
> >> +{
> >> + struct v4l2_subdev_frame_interval ival = { 0 };
> >> + int ret;
> >> +
> >> + if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
> >> + a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> >> + return -EINVAL;
> >> +
> >> + if (vdev->device_caps & V4L2_CAP_READWRITE)
> >> + a->parm.capture.readbuffers = 2;
> >
> > Hmm... why don't you also initialize readbuffers otherwise?
>
> It's specifically for read(). If read() is not supported, then this
> is meaningless and should just stay 0. v4l2-compliance checks for this.
Well, API states that:
"When an application requests zero buffers, drivers should just return the current setting rather than the minimum or an error code."
So, something should zero it, if not used and type is capture or
capture_mplane.
> The 'readbuffers' field is completely outdated and once this is in
> the next step is to see if we can come up with something better. I hate
> G/S_PARM.
Yes, it is a weird ioctl, but I'm not yet convinced that we should
increase API complexity by adding newer ioctls due to that.
Instead, I would just get rid of .g_parm/.s_parm callbacks, implementing
a better kAPI, without bothering adding more complexity to uAPI.
>
> Regards,
>
> Hans
>
> >
> >> + if (v4l2_subdev_has_op(sd, video, g_frame_interval))
> >> + a->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> >> + ret = v4l2_subdev_call(sd, video, g_frame_interval, &ival);
> >> + if (!ret)
> >> + a->parm.capture.timeperframe = ival.interval;
> >> + return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(v4l2_g_parm_cap);
> >> +
> >> +int v4l2_s_parm_cap(struct video_device *vdev,
> >> + struct v4l2_subdev *sd, struct v4l2_streamparm *a)
> >> +{
> >> + struct v4l2_subdev_frame_interval ival = {
> >> + .interval = a->parm.capture.timeperframe
> >> + };
> >> + int ret;
> >> +
> >> + if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
> >> + a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> >> + return -EINVAL;
> >> +
> >> + memset(&a->parm, 0, sizeof(a->parm));
> >> + if (vdev->device_caps & V4L2_CAP_READWRITE)
> >> + a->parm.capture.readbuffers = 2;
> >> + else
> >> + a->parm.capture.readbuffers = 0;
> >> +
> >> + if (v4l2_subdev_has_op(sd, video, g_frame_interval))
> >> + a->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> >> + ret = v4l2_subdev_call(sd, video, s_frame_interval, &ival);
> >> + if (!ret)
> >> + a->parm.capture.timeperframe = ival.interval;
> >> + return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(v4l2_s_parm_cap);
> >> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> >> index e0d95a7c5d48..f3aa1d728c0b 100644
> >> --- a/include/media/v4l2-common.h
> >> +++ b/include/media/v4l2-common.h
> >> @@ -341,4 +341,30 @@ v4l2_find_nearest_format(const struct v4l2_frmsize_discrete *sizes,
> >> */
> >> void v4l2_get_timestamp(struct timeval *tv);
> >>
> >> +/**
> >> + * v4l2_g_parm_cap - helper routine for vidioc_g_parm to fill this in by
> >> + * calling the g_frame_interval op of the given subdev. It only works
> >> + * for V4L2_BUF_TYPE_VIDEO_CAPTURE(_MPLANE), hence the _cap in the
> >> + * function name.
> >> + *
> >> + * @vdev: the struct video_device pointer. Used to determine the device caps.
> >> + * @sd: the sub-device pointer.
> >> + * @a: the VIDIOC_G_PARM argument.
> >> + */
> >> +int v4l2_g_parm_cap(struct video_device *vdev,
> >> + struct v4l2_subdev *sd, struct v4l2_streamparm *a);
> >> +
> >> +/**
> >> + * v4l2_s_parm_cap - helper routine for vidioc_s_parm to fill this in by
> >> + * calling the s_frame_interval op of the given subdev. It only works
> >> + * for V4L2_BUF_TYPE_VIDEO_CAPTURE(_MPLANE), hence the _cap in the
> >> + * function name.
> >> + *
> >> + * @vdev: the struct video_device pointer. Used to determine the device caps.
> >> + * @sd: the sub-device pointer.
> >> + * @a: the VIDIOC_S_PARM argument.
> >> + */
> >> +int v4l2_s_parm_cap(struct video_device *vdev,
> >> + struct v4l2_subdev *sd, struct v4l2_streamparm *a);
> >> +
> >> #endif /* V4L2_COMMON_H_ */
> >
> >
> >
> > Thanks,
> > Mauro
> >
>
Thanks,
Mauro
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCHv2 1/9] v4l2-common: create v4l2_g/s_parm_cap helpers
2018-02-14 16:35 ` Mauro Carvalho Chehab
@ 2018-02-14 16:47 ` Hans Verkuil
0 siblings, 0 replies; 35+ messages in thread
From: Hans Verkuil @ 2018-02-14 16:47 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: linux-media, Sakari Ailus, Jacopo Mondi, Hans Verkuil
On 14/02/18 17:35, Mauro Carvalho Chehab wrote:
> Em Wed, 14 Feb 2018 17:23:51 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>
>> On 14/02/18 16:50, Mauro Carvalho Chehab wrote:
>>> Em Mon, 22 Jan 2018 13:31:17 +0100
>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>
>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>
>>>> Create helpers to handle VIDIOC_G/S_PARM by querying the
>>>> g/s_frame_interval v4l2_subdev ops.
>>>>
>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>> ---
>>>> drivers/media/v4l2-core/v4l2-common.c | 48 +++++++++++++++++++++++++++++++++++
>>>> include/media/v4l2-common.h | 26 +++++++++++++++++++
>>>> 2 files changed, 74 insertions(+)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
>>>> index 8650ad92b64d..96c1b31de9e3 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-common.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-common.c
>>>> @@ -392,3 +392,51 @@ void v4l2_get_timestamp(struct timeval *tv)
>>>> tv->tv_usec = ts.tv_nsec / NSEC_PER_USEC;
>>>> }
>>>> EXPORT_SYMBOL_GPL(v4l2_get_timestamp);
>>>> +
>>>> +int v4l2_g_parm_cap(struct video_device *vdev,
>>>> + struct v4l2_subdev *sd, struct v4l2_streamparm *a)
>>>> +{
>>>> + struct v4l2_subdev_frame_interval ival = { 0 };
>>>> + int ret;
>>>> +
>>>> + if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
>>>> + a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
>>>> + return -EINVAL;
>>>> +
>>>> + if (vdev->device_caps & V4L2_CAP_READWRITE)
>>>> + a->parm.capture.readbuffers = 2;
>>>
>>> Hmm... why don't you also initialize readbuffers otherwise?
>>
>> It's specifically for read(). If read() is not supported, then this
>> is meaningless and should just stay 0. v4l2-compliance checks for this.
>
> Well, API states that:
>
> "When an application requests zero buffers, drivers should just return the current setting rather than the minimum or an error code."
>
> So, something should zero it, if not used and type is capture or
> capture_mplane.
All fields after the type field are zeroed by the core in v4l2-ioctl.c:
IOCTL_INFO_FNC(VIDIOC_G_PARM, v4l_g_parm, v4l_print_streamparm, INFO_FL_CLEAR(v4l2_streamparm, type)),
>
>> The 'readbuffers' field is completely outdated and once this is in
>> the next step is to see if we can come up with something better. I hate
>> G/S_PARM.
>
> Yes, it is a weird ioctl, but I'm not yet convinced that we should
> increase API complexity by adding newer ioctls due to that.
>
> Instead, I would just get rid of .g_parm/.s_parm callbacks, implementing
> a better kAPI, without bothering adding more complexity to uAPI.
I will probably do that as a second step anyway. We can discuss the pros and
cons of adding a new ioctl after that. I rather like the VIDIOC_SUBDEV_G/S_FRAME_INTERVAL
ioctls. Simple and to the point. It's really what you would expect as an
end-user.
Regards,
Hans
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCHv2 2/9] media: convert g/s_parm to g/s_frame_interval in subdevs
2018-01-22 12:31 [PATCHv2 0/9] media: replace g/s_parm by g/s_frame_interval Hans Verkuil
2018-01-22 12:31 ` [PATCHv2 1/9] v4l2-common: create v4l2_g/s_parm_cap helpers Hans Verkuil
@ 2018-01-22 12:31 ` Hans Verkuil
2018-01-23 14:47 ` jacopo mondi
2018-02-14 16:03 ` Mauro Carvalho Chehab
2018-01-22 12:31 ` [PATCHv2 3/9] staging: atomisp: Kill subdev s_parm abuse Hans Verkuil
` (7 subsequent siblings)
9 siblings, 2 replies; 35+ messages in thread
From: Hans Verkuil @ 2018-01-22 12:31 UTC (permalink / raw)
To: linux-media; +Cc: Sakari Ailus, Jacopo Mondi, Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
Convert all g/s_parm calls to g/s_frame_interval. This allows us
to remove the g/s_parm ops since those are a duplicate of
g/s_frame_interval.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/i2c/mt9v011.c | 31 +++++++-------------
drivers/media/i2c/ov6650.c | 35 +++++++++-------------
drivers/media/i2c/ov7670.c | 24 +++++++--------
drivers/media/i2c/ov7740.c | 31 +++++++-------------
drivers/media/i2c/tvp514x.c | 39 +++++++++----------------
drivers/media/i2c/vs6624.c | 29 +++++++-----------
drivers/media/platform/atmel/atmel-isc.c | 10 ++-----
drivers/media/platform/atmel/atmel-isi.c | 12 ++------
drivers/media/platform/blackfin/bfin_capture.c | 14 +++------
drivers/media/platform/marvell-ccic/mcam-core.c | 12 ++++----
drivers/media/platform/soc_camera/soc_camera.c | 10 ++++---
drivers/media/platform/via-camera.c | 4 +--
drivers/media/usb/em28xx/em28xx-video.c | 36 +++++++++++++++++++----
13 files changed, 122 insertions(+), 165 deletions(-)
diff --git a/drivers/media/i2c/mt9v011.c b/drivers/media/i2c/mt9v011.c
index 5e29064fae91..3e23c5b0de1f 100644
--- a/drivers/media/i2c/mt9v011.c
+++ b/drivers/media/i2c/mt9v011.c
@@ -364,33 +364,24 @@ static int mt9v011_set_fmt(struct v4l2_subdev *sd,
return 0;
}
-static int mt9v011_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
+static int mt9v011_g_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *ival)
{
- struct v4l2_captureparm *cp = &parms->parm.capture;
-
- if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
- return -EINVAL;
-
- memset(cp, 0, sizeof(struct v4l2_captureparm));
- cp->capability = V4L2_CAP_TIMEPERFRAME;
+ memset(ival->reserved, 0, sizeof(ival->reserved));
calc_fps(sd,
- &cp->timeperframe.numerator,
- &cp->timeperframe.denominator);
+ &ival->interval.numerator,
+ &ival->interval.denominator);
return 0;
}
-static int mt9v011_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
+static int mt9v011_s_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *ival)
{
- struct v4l2_captureparm *cp = &parms->parm.capture;
- struct v4l2_fract *tpf = &cp->timeperframe;
+ struct v4l2_fract *tpf = &ival->interval;
u16 speed;
- if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
- return -EINVAL;
- if (cp->extendedmode != 0)
- return -EINVAL;
-
+ memset(ival->reserved, 0, sizeof(ival->reserved));
speed = calc_speed(sd, tpf->numerator, tpf->denominator);
mt9v011_write(sd, R0A_MT9V011_CLK_SPEED, speed);
@@ -469,8 +460,8 @@ static const struct v4l2_subdev_core_ops mt9v011_core_ops = {
};
static const struct v4l2_subdev_video_ops mt9v011_video_ops = {
- .g_parm = mt9v011_g_parm,
- .s_parm = mt9v011_s_parm,
+ .g_frame_interval = mt9v011_g_frame_interval,
+ .s_frame_interval = mt9v011_s_frame_interval,
};
static const struct v4l2_subdev_pad_ops mt9v011_pad_ops = {
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 8975d16b2b24..3f962dae7534 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -201,7 +201,7 @@ struct ov6650 {
struct v4l2_rect rect; /* sensor cropping window */
unsigned long pclk_limit; /* from host */
unsigned long pclk_max; /* from resolution and format */
- struct v4l2_fract tpf; /* as requested with s_parm */
+ struct v4l2_fract tpf; /* as requested with s_frame_interval */
u32 code;
enum v4l2_colorspace colorspace;
};
@@ -723,42 +723,33 @@ static int ov6650_enum_mbus_code(struct v4l2_subdev *sd,
return 0;
}
-static int ov6650_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
+static int ov6650_g_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *ival)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct ov6650 *priv = to_ov6650(client);
- struct v4l2_captureparm *cp = &parms->parm.capture;
- if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
- return -EINVAL;
-
- memset(cp, 0, sizeof(*cp));
- cp->capability = V4L2_CAP_TIMEPERFRAME;
- cp->timeperframe.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf,
+ memset(ival->reserved, 0, sizeof(ival->reserved));
+ ival->interval.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf,
priv->pclk_limit, priv->pclk_max));
- cp->timeperframe.denominator = FRAME_RATE_MAX;
+ ival->interval.denominator = FRAME_RATE_MAX;
dev_dbg(&client->dev, "Frame interval: %u/%u s\n",
- cp->timeperframe.numerator, cp->timeperframe.denominator);
+ ival->interval.numerator, ival->interval.denominator);
return 0;
}
-static int ov6650_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
+static int ov6650_s_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *ival)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct ov6650 *priv = to_ov6650(client);
- struct v4l2_captureparm *cp = &parms->parm.capture;
- struct v4l2_fract *tpf = &cp->timeperframe;
+ struct v4l2_fract *tpf = &ival->interval;
int div, ret;
u8 clkrc;
- if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
- return -EINVAL;
-
- if (cp->extendedmode != 0)
- return -EINVAL;
-
+ memset(ival->reserved, 0, sizeof(ival->reserved));
if (tpf->numerator == 0 || tpf->denominator == 0)
div = 1; /* Reset to full rate */
else
@@ -921,8 +912,8 @@ static int ov6650_s_mbus_config(struct v4l2_subdev *sd,
static const struct v4l2_subdev_video_ops ov6650_video_ops = {
.s_stream = ov6650_s_stream,
- .g_parm = ov6650_g_parm,
- .s_parm = ov6650_s_parm,
+ .g_frame_interval = ov6650_g_frame_interval,
+ .s_frame_interval = ov6650_s_frame_interval,
.g_mbus_config = ov6650_g_mbus_config,
.s_mbus_config = ov6650_s_mbus_config,
};
diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
index fd229bc8a0e5..25b26d465aec 100644
--- a/drivers/media/i2c/ov7670.c
+++ b/drivers/media/i2c/ov7670.c
@@ -1100,30 +1100,26 @@ static int ov7670_get_fmt(struct v4l2_subdev *sd,
* Implement G/S_PARM. There is a "high quality" mode we could try
* to do someday; for now, we just do the frame rate tweak.
*/
-static int ov7670_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
+static int ov7670_g_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *ival)
{
- struct v4l2_captureparm *cp = &parms->parm.capture;
struct ov7670_info *info = to_state(sd);
- if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
- return -EINVAL;
+ memset(ival->reserved, 0, sizeof(ival->reserved));
- cp->capability = V4L2_CAP_TIMEPERFRAME;
- info->devtype->get_framerate(sd, &cp->timeperframe);
+ info->devtype->get_framerate(sd, &ival->interval);
return 0;
}
-static int ov7670_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
+static int ov7670_s_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *ival)
{
- struct v4l2_captureparm *cp = &parms->parm.capture;
- struct v4l2_fract *tpf = &cp->timeperframe;
+ struct v4l2_fract *tpf = &ival->interval;
struct ov7670_info *info = to_state(sd);
- if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
- return -EINVAL;
+ memset(ival->reserved, 0, sizeof(ival->reserved));
- cp->capability = V4L2_CAP_TIMEPERFRAME;
return info->devtype->set_framerate(sd, tpf);
}
@@ -1636,8 +1632,8 @@ static const struct v4l2_subdev_core_ops ov7670_core_ops = {
};
static const struct v4l2_subdev_video_ops ov7670_video_ops = {
- .s_parm = ov7670_s_parm,
- .g_parm = ov7670_g_parm,
+ .s_frame_interval = ov7670_s_frame_interval,
+ .g_frame_interval = ov7670_g_frame_interval,
};
static const struct v4l2_subdev_pad_ops ov7670_pad_ops = {
diff --git a/drivers/media/i2c/ov7740.c b/drivers/media/i2c/ov7740.c
index 0308ba437bbb..6ae7f3795374 100644
--- a/drivers/media/i2c/ov7740.c
+++ b/drivers/media/i2c/ov7740.c
@@ -624,17 +624,12 @@ static int ov7740_set_stream(struct v4l2_subdev *sd, int enable)
return ret;
}
-static int ov7740_get_parm(struct v4l2_subdev *sd,
- struct v4l2_streamparm *parms)
+static int ov7740_g_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *ival)
{
- struct v4l2_captureparm *cp = &parms->parm.capture;
- struct v4l2_fract *tpf = &cp->timeperframe;
+ struct v4l2_fract *tpf = &ival->interval;
- if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
- return -EINVAL;
-
- memset(cp, 0, sizeof(struct v4l2_captureparm));
- cp->capability = V4L2_CAP_TIMEPERFRAME;
+ memset(ival->reserved, 0, sizeof(ival->reserved));
tpf->numerator = 1;
tpf->denominator = 60;
@@ -642,18 +637,12 @@ static int ov7740_get_parm(struct v4l2_subdev *sd,
return 0;
}
-static int ov7740_set_parm(struct v4l2_subdev *sd,
- struct v4l2_streamparm *parms)
+static int ov7740_s_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *ival)
{
- struct v4l2_captureparm *cp = &parms->parm.capture;
- struct v4l2_fract *tpf = &cp->timeperframe;
-
- if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
- return -EINVAL;
- if (cp->extendedmode != 0)
- return -EINVAL;
+ struct v4l2_fract *tpf = &ival->interval;
- cp->capability = V4L2_CAP_TIMEPERFRAME;
+ memset(ival->reserved, 0, sizeof(ival->reserved));
tpf->numerator = 1;
tpf->denominator = 60;
@@ -663,8 +652,8 @@ static int ov7740_set_parm(struct v4l2_subdev *sd,
static struct v4l2_subdev_video_ops ov7740_subdev_video_ops = {
.s_stream = ov7740_set_stream,
- .s_parm = ov7740_set_parm,
- .g_parm = ov7740_get_parm,
+ .s_frame_interval = ov7740_s_frame_interval,
+ .g_frame_interval = ov7740_g_frame_interval,
};
static const struct reg_sequence ov7740_format_yuyv[] = {
diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
index d575b3e7e835..1ea2cfc83342 100644
--- a/drivers/media/i2c/tvp514x.c
+++ b/drivers/media/i2c/tvp514x.c
@@ -747,60 +747,49 @@ static int tvp514x_s_ctrl(struct v4l2_ctrl *ctrl)
}
/**
- * tvp514x_g_parm() - V4L2 decoder interface handler for g_parm
+ * tvp514x_g_frame_interval() - V4L2 decoder interface handler
* @sd: pointer to standard V4L2 sub-device structure
- * @a: pointer to standard V4L2 VIDIOC_G_PARM ioctl structure
+ * @a: pointer to a v4l2_subdev_frame_interval structure
*
* Returns the decoder's video CAPTURE parameters.
*/
static int
-tvp514x_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *a)
+tvp514x_g_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *ival)
{
struct tvp514x_decoder *decoder = to_decoder(sd);
- struct v4l2_captureparm *cparm;
enum tvp514x_std current_std;
- if (a == NULL)
- return -EINVAL;
-
- if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
- /* only capture is supported */
- return -EINVAL;
+ memset(ival->reserved, 0, sizeof(ival->reserved));
/* get the current standard */
current_std = decoder->current_std;
- cparm = &a->parm.capture;
- cparm->capability = V4L2_CAP_TIMEPERFRAME;
- cparm->timeperframe =
+ ival->interval =
decoder->std_list[current_std].standard.frameperiod;
return 0;
}
/**
- * tvp514x_s_parm() - V4L2 decoder interface handler for s_parm
+ * tvp514x_s_frame_interval() - V4L2 decoder interface handler
* @sd: pointer to standard V4L2 sub-device structure
- * @a: pointer to standard V4L2 VIDIOC_S_PARM ioctl structure
+ * @a: pointer to a v4l2_subdev_frame_interval structure
*
* Configures the decoder to use the input parameters, if possible. If
* not possible, returns the appropriate error code.
*/
static int
-tvp514x_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *a)
+tvp514x_s_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *ival)
{
struct tvp514x_decoder *decoder = to_decoder(sd);
struct v4l2_fract *timeperframe;
enum tvp514x_std current_std;
- if (a == NULL)
- return -EINVAL;
-
- if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
- /* only capture is supported */
- return -EINVAL;
+ memset(ival->reserved, 0, sizeof(ival->reserved));
- timeperframe = &a->parm.capture.timeperframe;
+ timeperframe = &ival->interval;
/* get the current standard */
current_std = decoder->current_std;
@@ -961,8 +950,8 @@ static const struct v4l2_subdev_video_ops tvp514x_video_ops = {
.s_std = tvp514x_s_std,
.s_routing = tvp514x_s_routing,
.querystd = tvp514x_querystd,
- .g_parm = tvp514x_g_parm,
- .s_parm = tvp514x_s_parm,
+ .g_frame_interval = tvp514x_g_frame_interval,
+ .s_frame_interval = tvp514x_s_frame_interval,
.s_stream = tvp514x_s_stream,
};
diff --git a/drivers/media/i2c/vs6624.c b/drivers/media/i2c/vs6624.c
index 560738213c00..50eb39a90126 100644
--- a/drivers/media/i2c/vs6624.c
+++ b/drivers/media/i2c/vs6624.c
@@ -657,31 +657,24 @@ static int vs6624_get_fmt(struct v4l2_subdev *sd,
return 0;
}
-static int vs6624_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
+static int vs6624_g_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *ival)
{
struct vs6624 *sensor = to_vs6624(sd);
- struct v4l2_captureparm *cp = &parms->parm.capture;
- if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
- return -EINVAL;
-
- memset(cp, 0, sizeof(*cp));
- cp->capability = V4L2_CAP_TIMEPERFRAME;
- cp->timeperframe.numerator = sensor->frame_rate.denominator;
- cp->timeperframe.denominator = sensor->frame_rate.numerator;
+ memset(ival->reserved, 0, sizeof(ival->reserved));
+ ival->interval.numerator = sensor->frame_rate.denominator;
+ ival->interval.denominator = sensor->frame_rate.numerator;
return 0;
}
-static int vs6624_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
+static int vs6624_s_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *ival)
{
struct vs6624 *sensor = to_vs6624(sd);
- struct v4l2_captureparm *cp = &parms->parm.capture;
- struct v4l2_fract *tpf = &cp->timeperframe;
+ struct v4l2_fract *tpf = &ival->interval;
- if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
- return -EINVAL;
- if (cp->extendedmode != 0)
- return -EINVAL;
+ memset(ival->reserved, 0, sizeof(ival->reserved));
if (tpf->numerator == 0 || tpf->denominator == 0
|| (tpf->denominator > tpf->numerator * MAX_FRAME_RATE)) {
@@ -738,8 +731,8 @@ static const struct v4l2_subdev_core_ops vs6624_core_ops = {
};
static const struct v4l2_subdev_video_ops vs6624_video_ops = {
- .s_parm = vs6624_s_parm,
- .g_parm = vs6624_g_parm,
+ .s_frame_interval = vs6624_s_frame_interval,
+ .g_frame_interval = vs6624_g_frame_interval,
.s_stream = vs6624_s_stream,
};
diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
index 34676409ca08..92d695b29fa9 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1417,20 +1417,14 @@ static int isc_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
{
struct isc_device *isc = video_drvdata(file);
- if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
- return -EINVAL;
-
- return v4l2_subdev_call(isc->current_subdev->sd, video, g_parm, a);
+ return v4l2_g_parm_cap(video_devdata(file), isc->current_subdev->sd, a);
}
static int isc_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
{
struct isc_device *isc = video_drvdata(file);
- if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
- return -EINVAL;
-
- return v4l2_subdev_call(isc->current_subdev->sd, video, s_parm, a);
+ return v4l2_s_parm_cap(video_devdata(file), isc->current_subdev->sd, a);
}
static int isc_enum_framesizes(struct file *file, void *fh,
diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
index 9958918e2449..e5be21a31640 100644
--- a/drivers/media/platform/atmel/atmel-isi.c
+++ b/drivers/media/platform/atmel/atmel-isi.c
@@ -689,22 +689,14 @@ static int isi_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
{
struct atmel_isi *isi = video_drvdata(file);
- if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
- return -EINVAL;
-
- a->parm.capture.readbuffers = 2;
- return v4l2_subdev_call(isi->entity.subdev, video, g_parm, a);
+ return v4l2_g_parm_cap(video_devdata(file), isi->entity.subdev, a);
}
static int isi_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
{
struct atmel_isi *isi = video_drvdata(file);
- if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
- return -EINVAL;
-
- a->parm.capture.readbuffers = 2;
- return v4l2_subdev_call(isi->entity.subdev, video, s_parm, a);
+ return v4l2_s_parm_cap(video_devdata(file), isi->entity.subdev, a);
}
static int isi_enum_framesizes(struct file *file, void *fh,
diff --git a/drivers/media/platform/blackfin/bfin_capture.c b/drivers/media/platform/blackfin/bfin_capture.c
index 41f179117fb0..b7660b1000fd 100644
--- a/drivers/media/platform/blackfin/bfin_capture.c
+++ b/drivers/media/platform/blackfin/bfin_capture.c
@@ -712,24 +712,18 @@ static int bcap_querycap(struct file *file, void *priv,
return 0;
}
-static int bcap_g_parm(struct file *file, void *fh,
- struct v4l2_streamparm *a)
+static int bcap_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
{
struct bcap_device *bcap_dev = video_drvdata(file);
- if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
- return -EINVAL;
- return v4l2_subdev_call(bcap_dev->sd, video, g_parm, a);
+ return v4l2_g_parm_cap(video_devdata(file), bcap_dev->sd, a);
}
-static int bcap_s_parm(struct file *file, void *fh,
- struct v4l2_streamparm *a)
+static int bcap_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
{
struct bcap_device *bcap_dev = video_drvdata(file);
- if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
- return -EINVAL;
- return v4l2_subdev_call(bcap_dev->sd, video, s_parm, a);
+ return v4l2_s_parm_cap(video_devdata(file), bcap_dev->sd, a);
}
static int bcap_log_status(struct file *file, void *priv)
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
index 7b7250b1cff8..80670eeee142 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -1443,24 +1443,24 @@ static int mcam_vidioc_s_input(struct file *filp, void *priv, unsigned int i)
* the level which controls the number of read buffers.
*/
static int mcam_vidioc_g_parm(struct file *filp, void *priv,
- struct v4l2_streamparm *parms)
+ struct v4l2_streamparm *a)
{
struct mcam_camera *cam = video_drvdata(filp);
int ret;
- ret = sensor_call(cam, video, g_parm, parms);
- parms->parm.capture.readbuffers = n_dma_bufs;
+ ret = v4l2_g_parm_cap(video_devdata(filp), cam->sensor, a);
+ a->parm.capture.readbuffers = n_dma_bufs;
return ret;
}
static int mcam_vidioc_s_parm(struct file *filp, void *priv,
- struct v4l2_streamparm *parms)
+ struct v4l2_streamparm *a)
{
struct mcam_camera *cam = video_drvdata(filp);
int ret;
- ret = sensor_call(cam, video, s_parm, parms);
- parms->parm.capture.readbuffers = n_dma_bufs;
+ ret = v4l2_s_parm_cap(video_devdata(filp), cam->sensor, a);
+ a->parm.capture.readbuffers = n_dma_bufs;
return ret;
}
diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index d13e2c5fb06f..1a9d4610045f 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -1788,17 +1788,19 @@ static int default_s_selection(struct soc_camera_device *icd,
}
static int default_g_parm(struct soc_camera_device *icd,
- struct v4l2_streamparm *parm)
+ struct v4l2_streamparm *a)
{
struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
- return v4l2_subdev_call(sd, video, g_parm, parm);
+
+ return v4l2_g_parm_cap(icd->vdev, sd, a);
}
static int default_s_parm(struct soc_camera_device *icd,
- struct v4l2_streamparm *parm)
+ struct v4l2_streamparm *a)
{
struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
- return v4l2_subdev_call(sd, video, s_parm, parm);
+
+ return v4l2_s_parm_cap(icd->vdev, sd, a);
}
static int default_enum_framesizes(struct soc_camera_device *icd,
diff --git a/drivers/media/platform/via-camera.c b/drivers/media/platform/via-camera.c
index 805d4a8fc17e..c632279a4209 100644
--- a/drivers/media/platform/via-camera.c
+++ b/drivers/media/platform/via-camera.c
@@ -1112,7 +1112,7 @@ static int viacam_g_parm(struct file *filp, void *priv,
int ret;
mutex_lock(&cam->lock);
- ret = sensor_call(cam, video, g_parm, parm);
+ ret = v4l2_g_parm_cap(video_devdata(filp), cam->sensor, parm);
mutex_unlock(&cam->lock);
parm->parm.capture.readbuffers = cam->n_cap_bufs;
return ret;
@@ -1125,7 +1125,7 @@ static int viacam_s_parm(struct file *filp, void *priv,
int ret;
mutex_lock(&cam->lock);
- ret = sensor_call(cam, video, s_parm, parm);
+ ret = v4l2_s_parm_cap(video_devdata(filp), cam->sensor, parm);
mutex_unlock(&cam->lock);
parm->parm.capture.readbuffers = cam->n_cap_bufs;
return ret;
diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index a2ba2d905952..2724e3b99af2 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -1582,17 +1582,26 @@ static int vidioc_s_std(struct file *file, void *priv, v4l2_std_id norm)
static int vidioc_g_parm(struct file *file, void *priv,
struct v4l2_streamparm *p)
{
+ struct v4l2_subdev_frame_interval ival = { 0 };
struct em28xx *dev = video_drvdata(file);
struct em28xx_v4l2 *v4l2 = dev->v4l2;
int rc = 0;
+ if (p->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
+ p->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
+ return -EINVAL;
+
p->parm.capture.readbuffers = EM28XX_MIN_BUF;
- if (dev->board.is_webcam)
+ p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
+ if (dev->board.is_webcam) {
rc = v4l2_device_call_until_err(&v4l2->v4l2_dev, 0,
- video, g_parm, p);
- else
+ video, g_frame_interval, &ival);
+ if (!rc)
+ p->parm.capture.timeperframe = ival.interval;
+ } else {
v4l2_video_std_frame_period(v4l2->norm,
&p->parm.capture.timeperframe);
+ }
return rc;
}
@@ -1601,10 +1610,27 @@ static int vidioc_s_parm(struct file *file, void *priv,
struct v4l2_streamparm *p)
{
struct em28xx *dev = video_drvdata(file);
+ struct v4l2_subdev_frame_interval ival = {
+ 0,
+ p->parm.capture.timeperframe
+ };
+ int rc = 0;
+
+ if (!dev->board.is_webcam)
+ return -ENOTTY;
+ if (p->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
+ p->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
+ return -EINVAL;
+
+ memset(&p->parm, 0, sizeof(p->parm));
p->parm.capture.readbuffers = EM28XX_MIN_BUF;
- return v4l2_device_call_until_err(&dev->v4l2->v4l2_dev,
- 0, video, s_parm, p);
+ p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
+ rc = v4l2_device_call_until_err(&dev->v4l2->v4l2_dev, 0,
+ video, s_frame_interval, &ival);
+ if (!rc)
+ p->parm.capture.timeperframe = ival.interval;
+ return rc;
}
static int vidioc_enum_input(struct file *file, void *priv,
--
2.15.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCHv2 2/9] media: convert g/s_parm to g/s_frame_interval in subdevs
2018-01-22 12:31 ` [PATCHv2 2/9] media: convert g/s_parm to g/s_frame_interval in subdevs Hans Verkuil
@ 2018-01-23 14:47 ` jacopo mondi
2018-01-23 14:52 ` Hans Verkuil
2018-02-14 16:03 ` Mauro Carvalho Chehab
1 sibling, 1 reply; 35+ messages in thread
From: jacopo mondi @ 2018-01-23 14:47 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Sakari Ailus, Jacopo Mondi, Hans Verkuil
Hi Hans,
On Mon, Jan 22, 2018 at 01:31:18PM +0100, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> --- a/drivers/media/platform/atmel/atmel-isi.c
> +++ b/drivers/media/platform/atmel/atmel-isi.c
> @@ -689,22 +689,14 @@ static int isi_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
> {
> struct atmel_isi *isi = video_drvdata(file);
>
> - if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> - return -EINVAL;
> -
> - a->parm.capture.readbuffers = 2;
> - return v4l2_subdev_call(isi->entity.subdev, video, g_parm, a);
> + return v4l2_g_parm_cap(video_devdata(file), isi->entity.subdev, a);
> }
>
> static int isi_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
> {
> struct atmel_isi *isi = video_drvdata(file);
>
> - if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> - return -EINVAL;
I've now tested the right version with CEU, and v4l2-compliance
reports:
fail: v4l2-test-formats.cpp(1218): Video Capture cap not set, but
G/S_PARM worked
To get rid of this error I had to refuse V4L2_BUF_TYPE_VIDEO_CAPTURE
type in g/s_parm as my driver only supports VIDEO_CAPTURE_MPLANE type.
static int ceu_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
{
struct ceu_device *ceudev = video_drvdata(file);
if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
return -EINVAL;
return v4l2_s_parm_cap(video_devdata(file), ceudev->sd->v4l2_sd, a);
}
To make sure the same error does not happen for atmel-isi/isc and other
platform drivers this patch modifies, I would keep the checks on
a->type you have removed here.
I now wonder if the following test in the newly added v4l2_g/s_parm_cap()
still makes sense if platform drivers have to check for the correct
buffer type anyhow (the same for the _cap name suffix)
int v4l2_g_parm_cap(struct video_device *vdev,
struct v4l2_subdev *sd, struct v4l2_streamparm *a)
{
struct v4l2_subdev_frame_interval ival = { 0 };
int ret;
if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
return -EINVAL;
Thanks
j
> -
> - a->parm.capture.readbuffers = 2;
> - return v4l2_subdev_call(isi->entity.subdev, video, s_parm, a);
> + return v4l2_s_parm_cap(video_devdata(file), isi->entity.subdev, a);
> }
>
> static int isi_enum_framesizes(struct file *file, void *fh,
> diff --git a/drivers/media/platform/blackfin/bfin_capture.c b/drivers/media/platform/blackfin/bfin_capture.c
> index 41f179117fb0..b7660b1000fd 100644
> --- a/drivers/media/platform/blackfin/bfin_capture.c
> +++ b/drivers/media/platform/blackfin/bfin_capture.c
> @@ -712,24 +712,18 @@ static int bcap_querycap(struct file *file, void *priv,
> return 0;
> }
>
> -static int bcap_g_parm(struct file *file, void *fh,
> - struct v4l2_streamparm *a)
> +static int bcap_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
> {
> struct bcap_device *bcap_dev = video_drvdata(file);
>
> - if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> - return -EINVAL;
> - return v4l2_subdev_call(bcap_dev->sd, video, g_parm, a);
> + return v4l2_g_parm_cap(video_devdata(file), bcap_dev->sd, a);
> }
>
> -static int bcap_s_parm(struct file *file, void *fh,
> - struct v4l2_streamparm *a)
> +static int bcap_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
> {
> struct bcap_device *bcap_dev = video_drvdata(file);
>
> - if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> - return -EINVAL;
> - return v4l2_subdev_call(bcap_dev->sd, video, s_parm, a);
> + return v4l2_s_parm_cap(video_devdata(file), bcap_dev->sd, a);
> }
>
> static int bcap_log_status(struct file *file, void *priv)
> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
> index 7b7250b1cff8..80670eeee142 100644
> --- a/drivers/media/platform/marvell-ccic/mcam-core.c
> +++ b/drivers/media/platform/marvell-ccic/mcam-core.c
> @@ -1443,24 +1443,24 @@ static int mcam_vidioc_s_input(struct file *filp, void *priv, unsigned int i)
> * the level which controls the number of read buffers.
> */
> static int mcam_vidioc_g_parm(struct file *filp, void *priv,
> - struct v4l2_streamparm *parms)
> + struct v4l2_streamparm *a)
> {
> struct mcam_camera *cam = video_drvdata(filp);
> int ret;
>
> - ret = sensor_call(cam, video, g_parm, parms);
> - parms->parm.capture.readbuffers = n_dma_bufs;
> + ret = v4l2_g_parm_cap(video_devdata(filp), cam->sensor, a);
> + a->parm.capture.readbuffers = n_dma_bufs;
> return ret;
> }
>
> static int mcam_vidioc_s_parm(struct file *filp, void *priv,
> - struct v4l2_streamparm *parms)
> + struct v4l2_streamparm *a)
> {
> struct mcam_camera *cam = video_drvdata(filp);
> int ret;
>
> - ret = sensor_call(cam, video, s_parm, parms);
> - parms->parm.capture.readbuffers = n_dma_bufs;
> + ret = v4l2_s_parm_cap(video_devdata(filp), cam->sensor, a);
> + a->parm.capture.readbuffers = n_dma_bufs;
> return ret;
> }
>
> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
> index d13e2c5fb06f..1a9d4610045f 100644
> --- a/drivers/media/platform/soc_camera/soc_camera.c
> +++ b/drivers/media/platform/soc_camera/soc_camera.c
> @@ -1788,17 +1788,19 @@ static int default_s_selection(struct soc_camera_device *icd,
> }
>
> static int default_g_parm(struct soc_camera_device *icd,
> - struct v4l2_streamparm *parm)
> + struct v4l2_streamparm *a)
> {
> struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> - return v4l2_subdev_call(sd, video, g_parm, parm);
> +
> + return v4l2_g_parm_cap(icd->vdev, sd, a);
> }
>
> static int default_s_parm(struct soc_camera_device *icd,
> - struct v4l2_streamparm *parm)
> + struct v4l2_streamparm *a)
> {
> struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> - return v4l2_subdev_call(sd, video, s_parm, parm);
> +
> + return v4l2_s_parm_cap(icd->vdev, sd, a);
> }
>
> static int default_enum_framesizes(struct soc_camera_device *icd,
> diff --git a/drivers/media/platform/via-camera.c b/drivers/media/platform/via-camera.c
> index 805d4a8fc17e..c632279a4209 100644
> --- a/drivers/media/platform/via-camera.c
> +++ b/drivers/media/platform/via-camera.c
> @@ -1112,7 +1112,7 @@ static int viacam_g_parm(struct file *filp, void *priv,
> int ret;
>
> mutex_lock(&cam->lock);
> - ret = sensor_call(cam, video, g_parm, parm);
> + ret = v4l2_g_parm_cap(video_devdata(filp), cam->sensor, parm);
> mutex_unlock(&cam->lock);
> parm->parm.capture.readbuffers = cam->n_cap_bufs;
> return ret;
> @@ -1125,7 +1125,7 @@ static int viacam_s_parm(struct file *filp, void *priv,
> int ret;
>
> mutex_lock(&cam->lock);
> - ret = sensor_call(cam, video, s_parm, parm);
> + ret = v4l2_s_parm_cap(video_devdata(filp), cam->sensor, parm);
> mutex_unlock(&cam->lock);
> parm->parm.capture.readbuffers = cam->n_cap_bufs;
> return ret;
> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> index a2ba2d905952..2724e3b99af2 100644
> --- a/drivers/media/usb/em28xx/em28xx-video.c
> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> @@ -1582,17 +1582,26 @@ static int vidioc_s_std(struct file *file, void *priv, v4l2_std_id norm)
> static int vidioc_g_parm(struct file *file, void *priv,
> struct v4l2_streamparm *p)
> {
> + struct v4l2_subdev_frame_interval ival = { 0 };
> struct em28xx *dev = video_drvdata(file);
> struct em28xx_v4l2 *v4l2 = dev->v4l2;
> int rc = 0;
>
> + if (p->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
> + p->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> + return -EINVAL;
> +
> p->parm.capture.readbuffers = EM28XX_MIN_BUF;
> - if (dev->board.is_webcam)
> + p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> + if (dev->board.is_webcam) {
> rc = v4l2_device_call_until_err(&v4l2->v4l2_dev, 0,
> - video, g_parm, p);
> - else
> + video, g_frame_interval, &ival);
> + if (!rc)
> + p->parm.capture.timeperframe = ival.interval;
> + } else {
> v4l2_video_std_frame_period(v4l2->norm,
> &p->parm.capture.timeperframe);
> + }
>
> return rc;
> }
> @@ -1601,10 +1610,27 @@ static int vidioc_s_parm(struct file *file, void *priv,
> struct v4l2_streamparm *p)
> {
> struct em28xx *dev = video_drvdata(file);
> + struct v4l2_subdev_frame_interval ival = {
> + 0,
> + p->parm.capture.timeperframe
> + };
> + int rc = 0;
> +
> + if (!dev->board.is_webcam)
> + return -ENOTTY;
>
> + if (p->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
> + p->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> + return -EINVAL;
> +
> + memset(&p->parm, 0, sizeof(p->parm));
> p->parm.capture.readbuffers = EM28XX_MIN_BUF;
> - return v4l2_device_call_until_err(&dev->v4l2->v4l2_dev,
> - 0, video, s_parm, p);
> + p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> + rc = v4l2_device_call_until_err(&dev->v4l2->v4l2_dev, 0,
> + video, s_frame_interval, &ival);
> + if (!rc)
> + p->parm.capture.timeperframe = ival.interval;
> + return rc;
> }
>
> static int vidioc_enum_input(struct file *file, void *priv,
> --
> 2.15.1
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCHv2 2/9] media: convert g/s_parm to g/s_frame_interval in subdevs
2018-01-23 14:47 ` jacopo mondi
@ 2018-01-23 14:52 ` Hans Verkuil
0 siblings, 0 replies; 35+ messages in thread
From: Hans Verkuil @ 2018-01-23 14:52 UTC (permalink / raw)
To: jacopo mondi; +Cc: linux-media, Sakari Ailus, Jacopo Mondi, Hans Verkuil
On 23/01/18 15:47, jacopo mondi wrote:
> Hi Hans,
>
> On Mon, Jan 22, 2018 at 01:31:18PM +0100, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> --- a/drivers/media/platform/atmel/atmel-isi.c
>> +++ b/drivers/media/platform/atmel/atmel-isi.c
>> @@ -689,22 +689,14 @@ static int isi_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>> {
>> struct atmel_isi *isi = video_drvdata(file);
>>
>> - if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> - return -EINVAL;
>> -
>> - a->parm.capture.readbuffers = 2;
>> - return v4l2_subdev_call(isi->entity.subdev, video, g_parm, a);
>> + return v4l2_g_parm_cap(video_devdata(file), isi->entity.subdev, a);
>> }
>>
>> static int isi_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>> {
>> struct atmel_isi *isi = video_drvdata(file);
>>
>> - if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> - return -EINVAL;
>
> I've now tested the right version with CEU, and v4l2-compliance
> reports:
>
> fail: v4l2-test-formats.cpp(1218): Video Capture cap not set, but
> G/S_PARM worked
Oops, that's a bug in v4l2-compliance. The public API for g/s_parm has
been relaxed a bit by this patch in that we now allow both CAPTURE and
CAPTURE_MPLANE. But v4l2-compliance hasn't been updated for that change
yet.
Regards,
Hans
>
> To get rid of this error I had to refuse V4L2_BUF_TYPE_VIDEO_CAPTURE
> type in g/s_parm as my driver only supports VIDEO_CAPTURE_MPLANE type.
>
> static int ceu_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
> {
> struct ceu_device *ceudev = video_drvdata(file);
>
> if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> return -EINVAL;
>
> return v4l2_s_parm_cap(video_devdata(file), ceudev->sd->v4l2_sd, a);
> }
>
> To make sure the same error does not happen for atmel-isi/isc and other
> platform drivers this patch modifies, I would keep the checks on
> a->type you have removed here.
>
> I now wonder if the following test in the newly added v4l2_g/s_parm_cap()
> still makes sense if platform drivers have to check for the correct
> buffer type anyhow (the same for the _cap name suffix)
>
> int v4l2_g_parm_cap(struct video_device *vdev,
> struct v4l2_subdev *sd, struct v4l2_streamparm *a)
> {
> struct v4l2_subdev_frame_interval ival = { 0 };
> int ret;
>
> if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
> a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> return -EINVAL;
>
> Thanks
> j
>
>> -
>> - a->parm.capture.readbuffers = 2;
>> - return v4l2_subdev_call(isi->entity.subdev, video, s_parm, a);
>> + return v4l2_s_parm_cap(video_devdata(file), isi->entity.subdev, a);
>> }
>>
>> static int isi_enum_framesizes(struct file *file, void *fh,
>> diff --git a/drivers/media/platform/blackfin/bfin_capture.c b/drivers/media/platform/blackfin/bfin_capture.c
>> index 41f179117fb0..b7660b1000fd 100644
>> --- a/drivers/media/platform/blackfin/bfin_capture.c
>> +++ b/drivers/media/platform/blackfin/bfin_capture.c
>> @@ -712,24 +712,18 @@ static int bcap_querycap(struct file *file, void *priv,
>> return 0;
>> }
>>
>> -static int bcap_g_parm(struct file *file, void *fh,
>> - struct v4l2_streamparm *a)
>> +static int bcap_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>> {
>> struct bcap_device *bcap_dev = video_drvdata(file);
>>
>> - if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> - return -EINVAL;
>> - return v4l2_subdev_call(bcap_dev->sd, video, g_parm, a);
>> + return v4l2_g_parm_cap(video_devdata(file), bcap_dev->sd, a);
>> }
>>
>> -static int bcap_s_parm(struct file *file, void *fh,
>> - struct v4l2_streamparm *a)
>> +static int bcap_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>> {
>> struct bcap_device *bcap_dev = video_drvdata(file);
>>
>> - if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> - return -EINVAL;
>> - return v4l2_subdev_call(bcap_dev->sd, video, s_parm, a);
>> + return v4l2_s_parm_cap(video_devdata(file), bcap_dev->sd, a);
>> }
>>
>> static int bcap_log_status(struct file *file, void *priv)
>> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
>> index 7b7250b1cff8..80670eeee142 100644
>> --- a/drivers/media/platform/marvell-ccic/mcam-core.c
>> +++ b/drivers/media/platform/marvell-ccic/mcam-core.c
>> @@ -1443,24 +1443,24 @@ static int mcam_vidioc_s_input(struct file *filp, void *priv, unsigned int i)
>> * the level which controls the number of read buffers.
>> */
>> static int mcam_vidioc_g_parm(struct file *filp, void *priv,
>> - struct v4l2_streamparm *parms)
>> + struct v4l2_streamparm *a)
>> {
>> struct mcam_camera *cam = video_drvdata(filp);
>> int ret;
>>
>> - ret = sensor_call(cam, video, g_parm, parms);
>> - parms->parm.capture.readbuffers = n_dma_bufs;
>> + ret = v4l2_g_parm_cap(video_devdata(filp), cam->sensor, a);
>> + a->parm.capture.readbuffers = n_dma_bufs;
>> return ret;
>> }
>>
>> static int mcam_vidioc_s_parm(struct file *filp, void *priv,
>> - struct v4l2_streamparm *parms)
>> + struct v4l2_streamparm *a)
>> {
>> struct mcam_camera *cam = video_drvdata(filp);
>> int ret;
>>
>> - ret = sensor_call(cam, video, s_parm, parms);
>> - parms->parm.capture.readbuffers = n_dma_bufs;
>> + ret = v4l2_s_parm_cap(video_devdata(filp), cam->sensor, a);
>> + a->parm.capture.readbuffers = n_dma_bufs;
>> return ret;
>> }
>>
>> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
>> index d13e2c5fb06f..1a9d4610045f 100644
>> --- a/drivers/media/platform/soc_camera/soc_camera.c
>> +++ b/drivers/media/platform/soc_camera/soc_camera.c
>> @@ -1788,17 +1788,19 @@ static int default_s_selection(struct soc_camera_device *icd,
>> }
>>
>> static int default_g_parm(struct soc_camera_device *icd,
>> - struct v4l2_streamparm *parm)
>> + struct v4l2_streamparm *a)
>> {
>> struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>> - return v4l2_subdev_call(sd, video, g_parm, parm);
>> +
>> + return v4l2_g_parm_cap(icd->vdev, sd, a);
>> }
>>
>> static int default_s_parm(struct soc_camera_device *icd,
>> - struct v4l2_streamparm *parm)
>> + struct v4l2_streamparm *a)
>> {
>> struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>> - return v4l2_subdev_call(sd, video, s_parm, parm);
>> +
>> + return v4l2_s_parm_cap(icd->vdev, sd, a);
>> }
>>
>> static int default_enum_framesizes(struct soc_camera_device *icd,
>> diff --git a/drivers/media/platform/via-camera.c b/drivers/media/platform/via-camera.c
>> index 805d4a8fc17e..c632279a4209 100644
>> --- a/drivers/media/platform/via-camera.c
>> +++ b/drivers/media/platform/via-camera.c
>> @@ -1112,7 +1112,7 @@ static int viacam_g_parm(struct file *filp, void *priv,
>> int ret;
>>
>> mutex_lock(&cam->lock);
>> - ret = sensor_call(cam, video, g_parm, parm);
>> + ret = v4l2_g_parm_cap(video_devdata(filp), cam->sensor, parm);
>> mutex_unlock(&cam->lock);
>> parm->parm.capture.readbuffers = cam->n_cap_bufs;
>> return ret;
>> @@ -1125,7 +1125,7 @@ static int viacam_s_parm(struct file *filp, void *priv,
>> int ret;
>>
>> mutex_lock(&cam->lock);
>> - ret = sensor_call(cam, video, s_parm, parm);
>> + ret = v4l2_s_parm_cap(video_devdata(filp), cam->sensor, parm);
>> mutex_unlock(&cam->lock);
>> parm->parm.capture.readbuffers = cam->n_cap_bufs;
>> return ret;
>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
>> index a2ba2d905952..2724e3b99af2 100644
>> --- a/drivers/media/usb/em28xx/em28xx-video.c
>> +++ b/drivers/media/usb/em28xx/em28xx-video.c
>> @@ -1582,17 +1582,26 @@ static int vidioc_s_std(struct file *file, void *priv, v4l2_std_id norm)
>> static int vidioc_g_parm(struct file *file, void *priv,
>> struct v4l2_streamparm *p)
>> {
>> + struct v4l2_subdev_frame_interval ival = { 0 };
>> struct em28xx *dev = video_drvdata(file);
>> struct em28xx_v4l2 *v4l2 = dev->v4l2;
>> int rc = 0;
>>
>> + if (p->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
>> + p->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
>> + return -EINVAL;
>> +
>> p->parm.capture.readbuffers = EM28XX_MIN_BUF;
>> - if (dev->board.is_webcam)
>> + p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
>> + if (dev->board.is_webcam) {
>> rc = v4l2_device_call_until_err(&v4l2->v4l2_dev, 0,
>> - video, g_parm, p);
>> - else
>> + video, g_frame_interval, &ival);
>> + if (!rc)
>> + p->parm.capture.timeperframe = ival.interval;
>> + } else {
>> v4l2_video_std_frame_period(v4l2->norm,
>> &p->parm.capture.timeperframe);
>> + }
>>
>> return rc;
>> }
>> @@ -1601,10 +1610,27 @@ static int vidioc_s_parm(struct file *file, void *priv,
>> struct v4l2_streamparm *p)
>> {
>> struct em28xx *dev = video_drvdata(file);
>> + struct v4l2_subdev_frame_interval ival = {
>> + 0,
>> + p->parm.capture.timeperframe
>> + };
>> + int rc = 0;
>> +
>> + if (!dev->board.is_webcam)
>> + return -ENOTTY;
>>
>> + if (p->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
>> + p->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
>> + return -EINVAL;
>> +
>> + memset(&p->parm, 0, sizeof(p->parm));
>> p->parm.capture.readbuffers = EM28XX_MIN_BUF;
>> - return v4l2_device_call_until_err(&dev->v4l2->v4l2_dev,
>> - 0, video, s_parm, p);
>> + p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
>> + rc = v4l2_device_call_until_err(&dev->v4l2->v4l2_dev, 0,
>> + video, s_frame_interval, &ival);
>> + if (!rc)
>> + p->parm.capture.timeperframe = ival.interval;
>> + return rc;
>> }
>>
>> static int vidioc_enum_input(struct file *file, void *priv,
>> --
>> 2.15.1
>>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCHv2 2/9] media: convert g/s_parm to g/s_frame_interval in subdevs
2018-01-22 12:31 ` [PATCHv2 2/9] media: convert g/s_parm to g/s_frame_interval in subdevs Hans Verkuil
2018-01-23 14:47 ` jacopo mondi
@ 2018-02-14 16:03 ` Mauro Carvalho Chehab
2018-02-14 16:34 ` Hans Verkuil
1 sibling, 1 reply; 35+ messages in thread
From: Mauro Carvalho Chehab @ 2018-02-14 16:03 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Sakari Ailus, Jacopo Mondi, Hans Verkuil
Em Mon, 22 Jan 2018 13:31:18 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> Convert all g/s_parm calls to g/s_frame_interval. This allows us
> to remove the g/s_parm ops since those are a duplicate of
> g/s_frame_interval.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/i2c/mt9v011.c | 31 +++++++-------------
> drivers/media/i2c/ov6650.c | 35 +++++++++-------------
> drivers/media/i2c/ov7670.c | 24 +++++++--------
> drivers/media/i2c/ov7740.c | 31 +++++++-------------
> drivers/media/i2c/tvp514x.c | 39 +++++++++----------------
> drivers/media/i2c/vs6624.c | 29 +++++++-----------
> drivers/media/platform/atmel/atmel-isc.c | 10 ++-----
> drivers/media/platform/atmel/atmel-isi.c | 12 ++------
> drivers/media/platform/blackfin/bfin_capture.c | 14 +++------
> drivers/media/platform/marvell-ccic/mcam-core.c | 12 ++++----
> drivers/media/platform/soc_camera/soc_camera.c | 10 ++++---
> drivers/media/platform/via-camera.c | 4 +--
> drivers/media/usb/em28xx/em28xx-video.c | 36 +++++++++++++++++++----
> 13 files changed, 122 insertions(+), 165 deletions(-)
>
> diff --git a/drivers/media/i2c/mt9v011.c b/drivers/media/i2c/mt9v011.c
> index 5e29064fae91..3e23c5b0de1f 100644
> --- a/drivers/media/i2c/mt9v011.c
> +++ b/drivers/media/i2c/mt9v011.c
> @@ -364,33 +364,24 @@ static int mt9v011_set_fmt(struct v4l2_subdev *sd,
> return 0;
> }
>
> -static int mt9v011_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
> +static int mt9v011_g_frame_interval(struct v4l2_subdev *sd,
> + struct v4l2_subdev_frame_interval *ival)
> {
> - struct v4l2_captureparm *cp = &parms->parm.capture;
> -
> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> - return -EINVAL;
> -
> - memset(cp, 0, sizeof(struct v4l2_captureparm));
> - cp->capability = V4L2_CAP_TIMEPERFRAME;
> + memset(ival->reserved, 0, sizeof(ival->reserved));
Hmm.. why to repeat memset everywhere? If the hole idea is to stop abusing,
the best would be to do, instead:
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index c5639817db34..b18b418c080f 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -350,6 +350,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
if (fi->pad >= sd->entity.num_pads)
return -EINVAL;
+ memset(fi->reserved, 0, sizeof(ival->reserved));
return v4l2_subdev_call(sd, video, g_frame_interval, arg);
}
(same applies to s_frame_interval).
> calc_fps(sd,
> - &cp->timeperframe.numerator,
> - &cp->timeperframe.denominator);
> + &ival->interval.numerator,
> + &ival->interval.denominator);
>
> return 0;
> }
>
> -static int mt9v011_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
> +static int mt9v011_s_frame_interval(struct v4l2_subdev *sd,
> + struct v4l2_subdev_frame_interval *ival)
> {
> - struct v4l2_captureparm *cp = &parms->parm.capture;
> - struct v4l2_fract *tpf = &cp->timeperframe;
> + struct v4l2_fract *tpf = &ival->interval;
> u16 speed;
>
> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> - return -EINVAL;
> - if (cp->extendedmode != 0)
> - return -EINVAL;
> -
Hmm... why are you removing those sanity checks everywhere?
The core doesn't do it.
All the above comments also apply to the other files modified by
this patch.
> + memset(ival->reserved, 0, sizeof(ival->reserved));
> speed = calc_speed(sd, tpf->numerator, tpf->denominator);
>
> mt9v011_write(sd, R0A_MT9V011_CLK_SPEED, speed);
> @@ -469,8 +460,8 @@ static const struct v4l2_subdev_core_ops mt9v011_core_ops = {
> };
>
> static const struct v4l2_subdev_video_ops mt9v011_video_ops = {
> - .g_parm = mt9v011_g_parm,
> - .s_parm = mt9v011_s_parm,
> + .g_frame_interval = mt9v011_g_frame_interval,
> + .s_frame_interval = mt9v011_s_frame_interval,
> };
>
> static const struct v4l2_subdev_pad_ops mt9v011_pad_ops = {
> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> index 8975d16b2b24..3f962dae7534 100644
> --- a/drivers/media/i2c/ov6650.c
> +++ b/drivers/media/i2c/ov6650.c
> @@ -201,7 +201,7 @@ struct ov6650 {
> struct v4l2_rect rect; /* sensor cropping window */
> unsigned long pclk_limit; /* from host */
> unsigned long pclk_max; /* from resolution and format */
> - struct v4l2_fract tpf; /* as requested with s_parm */
> + struct v4l2_fract tpf; /* as requested with s_frame_interval */
> u32 code;
> enum v4l2_colorspace colorspace;
> };
> @@ -723,42 +723,33 @@ static int ov6650_enum_mbus_code(struct v4l2_subdev *sd,
> return 0;
> }
>
> -static int ov6650_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
> +static int ov6650_g_frame_interval(struct v4l2_subdev *sd,
> + struct v4l2_subdev_frame_interval *ival)
> {
> struct i2c_client *client = v4l2_get_subdevdata(sd);
> struct ov6650 *priv = to_ov6650(client);
> - struct v4l2_captureparm *cp = &parms->parm.capture;
>
> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> - return -EINVAL;
> -
> - memset(cp, 0, sizeof(*cp));
> - cp->capability = V4L2_CAP_TIMEPERFRAME;
> - cp->timeperframe.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf,
> + memset(ival->reserved, 0, sizeof(ival->reserved));
> + ival->interval.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf,
> priv->pclk_limit, priv->pclk_max));
> - cp->timeperframe.denominator = FRAME_RATE_MAX;
> + ival->interval.denominator = FRAME_RATE_MAX;
>
> dev_dbg(&client->dev, "Frame interval: %u/%u s\n",
> - cp->timeperframe.numerator, cp->timeperframe.denominator);
> + ival->interval.numerator, ival->interval.denominator);
Hmm... not sure if a debug is needed here. Yet, if this is needed,
IMHO, it would make mroe sense to move it to the core.
Thanks,
Mauro
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCHv2 2/9] media: convert g/s_parm to g/s_frame_interval in subdevs
2018-02-14 16:03 ` Mauro Carvalho Chehab
@ 2018-02-14 16:34 ` Hans Verkuil
2018-02-14 17:02 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 35+ messages in thread
From: Hans Verkuil @ 2018-02-14 16:34 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: linux-media, Sakari Ailus, Jacopo Mondi, Hans Verkuil
On 14/02/18 17:03, Mauro Carvalho Chehab wrote:
> Em Mon, 22 Jan 2018 13:31:18 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Convert all g/s_parm calls to g/s_frame_interval. This allows us
>> to remove the g/s_parm ops since those are a duplicate of
>> g/s_frame_interval.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>> drivers/media/i2c/mt9v011.c | 31 +++++++-------------
>> drivers/media/i2c/ov6650.c | 35 +++++++++-------------
>> drivers/media/i2c/ov7670.c | 24 +++++++--------
>> drivers/media/i2c/ov7740.c | 31 +++++++-------------
>> drivers/media/i2c/tvp514x.c | 39 +++++++++----------------
>> drivers/media/i2c/vs6624.c | 29 +++++++-----------
>> drivers/media/platform/atmel/atmel-isc.c | 10 ++-----
>> drivers/media/platform/atmel/atmel-isi.c | 12 ++------
>> drivers/media/platform/blackfin/bfin_capture.c | 14 +++------
>> drivers/media/platform/marvell-ccic/mcam-core.c | 12 ++++----
>> drivers/media/platform/soc_camera/soc_camera.c | 10 ++++---
>> drivers/media/platform/via-camera.c | 4 +--
>> drivers/media/usb/em28xx/em28xx-video.c | 36 +++++++++++++++++++----
>> 13 files changed, 122 insertions(+), 165 deletions(-)
>>
>> diff --git a/drivers/media/i2c/mt9v011.c b/drivers/media/i2c/mt9v011.c
>> index 5e29064fae91..3e23c5b0de1f 100644
>> --- a/drivers/media/i2c/mt9v011.c
>> +++ b/drivers/media/i2c/mt9v011.c
>> @@ -364,33 +364,24 @@ static int mt9v011_set_fmt(struct v4l2_subdev *sd,
>> return 0;
>> }
>>
>> -static int mt9v011_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
>> +static int mt9v011_g_frame_interval(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_frame_interval *ival)
>> {
>> - struct v4l2_captureparm *cp = &parms->parm.capture;
>> -
>> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> - return -EINVAL;
>> -
>> - memset(cp, 0, sizeof(struct v4l2_captureparm));
>> - cp->capability = V4L2_CAP_TIMEPERFRAME;
>> + memset(ival->reserved, 0, sizeof(ival->reserved));
>
> Hmm.. why to repeat memset everywhere? If the hole idea is to stop abusing,
> the best would be to do, instead:
g_frame_interval is called by bridge drivers through the subdev ops. So that
path doesn't go through subdev_do_ioctl(). So it doesn't help putting it in
v4l2-subdev.c.
That doesn't mean it shouldn't be there as well. I believe my MC patch series
actually adds the memset in subdev_do_ioctl.
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index c5639817db34..b18b418c080f 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -350,6 +350,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> if (fi->pad >= sd->entity.num_pads)
> return -EINVAL;
>
> + memset(fi->reserved, 0, sizeof(ival->reserved));
> return v4l2_subdev_call(sd, video, g_frame_interval, arg);
> }
>
> (same applies to s_frame_interval).
>
>
>> calc_fps(sd,
>> - &cp->timeperframe.numerator,
>> - &cp->timeperframe.denominator);
>> + &ival->interval.numerator,
>> + &ival->interval.denominator);
>>
>> return 0;
>> }
>>
>> -static int mt9v011_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
>> +static int mt9v011_s_frame_interval(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_frame_interval *ival)
>> {
>> - struct v4l2_captureparm *cp = &parms->parm.capture;
>> - struct v4l2_fract *tpf = &cp->timeperframe;
>> + struct v4l2_fract *tpf = &ival->interval;
>> u16 speed;
>>
>> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> - return -EINVAL;
>> - if (cp->extendedmode != 0)
>> - return -EINVAL;
>> -
>
> Hmm... why are you removing those sanity checks everywhere?
> The core doesn't do it.
>
> All the above comments also apply to the other files modified by
> this patch.
struct v4l2_subdev_frame_interval has neither type nor extendedmode.
The check for type is done in the v4l2_g/s_parm_cap helpers instead.
And extendedmode is always set to 0.
>
>> + memset(ival->reserved, 0, sizeof(ival->reserved));
>> speed = calc_speed(sd, tpf->numerator, tpf->denominator);
>>
>> mt9v011_write(sd, R0A_MT9V011_CLK_SPEED, speed);
>> @@ -469,8 +460,8 @@ static const struct v4l2_subdev_core_ops mt9v011_core_ops = {
>> };
>>
>> static const struct v4l2_subdev_video_ops mt9v011_video_ops = {
>> - .g_parm = mt9v011_g_parm,
>> - .s_parm = mt9v011_s_parm,
>> + .g_frame_interval = mt9v011_g_frame_interval,
>> + .s_frame_interval = mt9v011_s_frame_interval,
>> };
>>
>> static const struct v4l2_subdev_pad_ops mt9v011_pad_ops = {
>> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
>> index 8975d16b2b24..3f962dae7534 100644
>> --- a/drivers/media/i2c/ov6650.c
>> +++ b/drivers/media/i2c/ov6650.c
>> @@ -201,7 +201,7 @@ struct ov6650 {
>> struct v4l2_rect rect; /* sensor cropping window */
>> unsigned long pclk_limit; /* from host */
>> unsigned long pclk_max; /* from resolution and format */
>> - struct v4l2_fract tpf; /* as requested with s_parm */
>> + struct v4l2_fract tpf; /* as requested with s_frame_interval */
>> u32 code;
>> enum v4l2_colorspace colorspace;
>> };
>> @@ -723,42 +723,33 @@ static int ov6650_enum_mbus_code(struct v4l2_subdev *sd,
>> return 0;
>> }
>>
>> -static int ov6650_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
>> +static int ov6650_g_frame_interval(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_frame_interval *ival)
>> {
>> struct i2c_client *client = v4l2_get_subdevdata(sd);
>> struct ov6650 *priv = to_ov6650(client);
>> - struct v4l2_captureparm *cp = &parms->parm.capture;
>>
>> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> - return -EINVAL;
>> -
>> - memset(cp, 0, sizeof(*cp));
>> - cp->capability = V4L2_CAP_TIMEPERFRAME;
>> - cp->timeperframe.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf,
>> + memset(ival->reserved, 0, sizeof(ival->reserved));
>> + ival->interval.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf,
>> priv->pclk_limit, priv->pclk_max));
>> - cp->timeperframe.denominator = FRAME_RATE_MAX;
>> + ival->interval.denominator = FRAME_RATE_MAX;
>>
>> dev_dbg(&client->dev, "Frame interval: %u/%u s\n",
>> - cp->timeperframe.numerator, cp->timeperframe.denominator);
>> + ival->interval.numerator, ival->interval.denominator);
>
> Hmm... not sure if a debug is needed here. Yet, if this is needed,
> IMHO, it would make mroe sense to move it to the core.
The core doesn't see this if this subdev op is called from a bridge driver.
Regards,
Hans
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCHv2 2/9] media: convert g/s_parm to g/s_frame_interval in subdevs
2018-02-14 16:34 ` Hans Verkuil
@ 2018-02-14 17:02 ` Mauro Carvalho Chehab
2018-02-14 17:16 ` Hans Verkuil
0 siblings, 1 reply; 35+ messages in thread
From: Mauro Carvalho Chehab @ 2018-02-14 17:02 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Sakari Ailus, Jacopo Mondi, Hans Verkuil
Em Wed, 14 Feb 2018 17:34:17 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> On 14/02/18 17:03, Mauro Carvalho Chehab wrote:
> > Em Mon, 22 Jan 2018 13:31:18 +0100
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >
> >> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>
> >> Convert all g/s_parm calls to g/s_frame_interval. This allows us
> >> to remove the g/s_parm ops since those are a duplicate of
> >> g/s_frame_interval.
> >>
> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> ---
> >> drivers/media/i2c/mt9v011.c | 31 +++++++-------------
> >> drivers/media/i2c/ov6650.c | 35 +++++++++-------------
> >> drivers/media/i2c/ov7670.c | 24 +++++++--------
> >> drivers/media/i2c/ov7740.c | 31 +++++++-------------
> >> drivers/media/i2c/tvp514x.c | 39 +++++++++----------------
> >> drivers/media/i2c/vs6624.c | 29 +++++++-----------
> >> drivers/media/platform/atmel/atmel-isc.c | 10 ++-----
> >> drivers/media/platform/atmel/atmel-isi.c | 12 ++------
> >> drivers/media/platform/blackfin/bfin_capture.c | 14 +++------
> >> drivers/media/platform/marvell-ccic/mcam-core.c | 12 ++++----
> >> drivers/media/platform/soc_camera/soc_camera.c | 10 ++++---
> >> drivers/media/platform/via-camera.c | 4 +--
> >> drivers/media/usb/em28xx/em28xx-video.c | 36 +++++++++++++++++++----
> >> 13 files changed, 122 insertions(+), 165 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/mt9v011.c b/drivers/media/i2c/mt9v011.c
> >> index 5e29064fae91..3e23c5b0de1f 100644
> >> --- a/drivers/media/i2c/mt9v011.c
> >> +++ b/drivers/media/i2c/mt9v011.c
> >> @@ -364,33 +364,24 @@ static int mt9v011_set_fmt(struct v4l2_subdev *sd,
> >> return 0;
> >> }
> >>
> >> -static int mt9v011_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
> >> +static int mt9v011_g_frame_interval(struct v4l2_subdev *sd,
> >> + struct v4l2_subdev_frame_interval *ival)
> >> {
> >> - struct v4l2_captureparm *cp = &parms->parm.capture;
> >> -
> >> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> >> - return -EINVAL;
> >> -
> >> - memset(cp, 0, sizeof(struct v4l2_captureparm));
> >> - cp->capability = V4L2_CAP_TIMEPERFRAME;
> >> + memset(ival->reserved, 0, sizeof(ival->reserved));
> >
> > Hmm.. why to repeat memset everywhere? If the hole idea is to stop abusing,
> > the best would be to do, instead:
>
> g_frame_interval is called by bridge drivers through the subdev ops. So that
> path doesn't go through subdev_do_ioctl(). So it doesn't help putting it in
> v4l2-subdev.c.
True, but you could also do the same for v4l2 ioctl() handling logic.
That would mean just two places with memset() instead of repeating the same
pattern everywhere.
> That doesn't mean it shouldn't be there as well. I believe my MC patch series
> actually adds the memset in subdev_do_ioctl.
>
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index c5639817db34..b18b418c080f 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -350,6 +350,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> > if (fi->pad >= sd->entity.num_pads)
> > return -EINVAL;
> >
> > + memset(fi->reserved, 0, sizeof(ival->reserved));
> > return v4l2_subdev_call(sd, video, g_frame_interval, arg);
> > }
> >
> > (same applies to s_frame_interval).
> >
> >
> >> calc_fps(sd,
> >> - &cp->timeperframe.numerator,
> >> - &cp->timeperframe.denominator);
> >> + &ival->interval.numerator,
> >> + &ival->interval.denominator);
> >>
> >> return 0;
> >> }
> >>
> >> -static int mt9v011_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
> >> +static int mt9v011_s_frame_interval(struct v4l2_subdev *sd,
> >> + struct v4l2_subdev_frame_interval *ival)
> >> {
> >> - struct v4l2_captureparm *cp = &parms->parm.capture;
> >> - struct v4l2_fract *tpf = &cp->timeperframe;
> >> + struct v4l2_fract *tpf = &ival->interval;
> >> u16 speed;
> >>
> >> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> >> - return -EINVAL;
> >> - if (cp->extendedmode != 0)
> >> - return -EINVAL;
> >> -
> >
> > Hmm... why are you removing those sanity checks everywhere?
> > The core doesn't do it.
> >
> > All the above comments also apply to the other files modified by
> > this patch.
>
> struct v4l2_subdev_frame_interval has neither type nor extendedmode.
>
> The check for type is done in the v4l2_g/s_parm_cap helpers instead.
Well, the subdev handler at v4l2-subdev.c doesn't seem to be checking it.
> And extendedmode is always set to 0.
>
> >
> >> + memset(ival->reserved, 0, sizeof(ival->reserved));
> >> speed = calc_speed(sd, tpf->numerator, tpf->denominator);
> >>
> >> mt9v011_write(sd, R0A_MT9V011_CLK_SPEED, speed);
> >> @@ -469,8 +460,8 @@ static const struct v4l2_subdev_core_ops mt9v011_core_ops = {
> >> };
> >>
> >> static const struct v4l2_subdev_video_ops mt9v011_video_ops = {
> >> - .g_parm = mt9v011_g_parm,
> >> - .s_parm = mt9v011_s_parm,
> >> + .g_frame_interval = mt9v011_g_frame_interval,
> >> + .s_frame_interval = mt9v011_s_frame_interval,
> >> };
> >>
> >> static const struct v4l2_subdev_pad_ops mt9v011_pad_ops = {
> >> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> >> index 8975d16b2b24..3f962dae7534 100644
> >> --- a/drivers/media/i2c/ov6650.c
> >> +++ b/drivers/media/i2c/ov6650.c
> >> @@ -201,7 +201,7 @@ struct ov6650 {
> >> struct v4l2_rect rect; /* sensor cropping window */
> >> unsigned long pclk_limit; /* from host */
> >> unsigned long pclk_max; /* from resolution and format */
> >> - struct v4l2_fract tpf; /* as requested with s_parm */
> >> + struct v4l2_fract tpf; /* as requested with s_frame_interval */
> >> u32 code;
> >> enum v4l2_colorspace colorspace;
> >> };
> >> @@ -723,42 +723,33 @@ static int ov6650_enum_mbus_code(struct v4l2_subdev *sd,
> >> return 0;
> >> }
> >>
> >> -static int ov6650_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
> >> +static int ov6650_g_frame_interval(struct v4l2_subdev *sd,
> >> + struct v4l2_subdev_frame_interval *ival)
> >> {
> >> struct i2c_client *client = v4l2_get_subdevdata(sd);
> >> struct ov6650 *priv = to_ov6650(client);
> >> - struct v4l2_captureparm *cp = &parms->parm.capture;
> >>
> >> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> >> - return -EINVAL;
> >> -
> >> - memset(cp, 0, sizeof(*cp));
> >> - cp->capability = V4L2_CAP_TIMEPERFRAME;
> >> - cp->timeperframe.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf,
> >> + memset(ival->reserved, 0, sizeof(ival->reserved));
> >> + ival->interval.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf,
> >> priv->pclk_limit, priv->pclk_max));
> >> - cp->timeperframe.denominator = FRAME_RATE_MAX;
> >> + ival->interval.denominator = FRAME_RATE_MAX;
> >>
> >> dev_dbg(&client->dev, "Frame interval: %u/%u s\n",
> >> - cp->timeperframe.numerator, cp->timeperframe.denominator);
> >> + ival->interval.numerator, ival->interval.denominator);
> >
> > Hmm... not sure if a debug is needed here. Yet, if this is needed,
> > IMHO, it would make mroe sense to move it to the core.
>
> The core doesn't see this if this subdev op is called from a bridge driver.
True, but, when calling via a bridge driver, there's already a way to
enable such kind debug.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCHv2 2/9] media: convert g/s_parm to g/s_frame_interval in subdevs
2018-02-14 17:02 ` Mauro Carvalho Chehab
@ 2018-02-14 17:16 ` Hans Verkuil
2018-02-14 17:30 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 35+ messages in thread
From: Hans Verkuil @ 2018-02-14 17:16 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: linux-media, Sakari Ailus, Jacopo Mondi, Hans Verkuil
On 14/02/18 18:02, Mauro Carvalho Chehab wrote:
> Em Wed, 14 Feb 2018 17:34:17 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>
>> On 14/02/18 17:03, Mauro Carvalho Chehab wrote:
>>> Em Mon, 22 Jan 2018 13:31:18 +0100
>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>
>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>
>>>> Convert all g/s_parm calls to g/s_frame_interval. This allows us
>>>> to remove the g/s_parm ops since those are a duplicate of
>>>> g/s_frame_interval.
>>>>
>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>> ---
>>>> drivers/media/i2c/mt9v011.c | 31 +++++++-------------
>>>> drivers/media/i2c/ov6650.c | 35 +++++++++-------------
>>>> drivers/media/i2c/ov7670.c | 24 +++++++--------
>>>> drivers/media/i2c/ov7740.c | 31 +++++++-------------
>>>> drivers/media/i2c/tvp514x.c | 39 +++++++++----------------
>>>> drivers/media/i2c/vs6624.c | 29 +++++++-----------
>>>> drivers/media/platform/atmel/atmel-isc.c | 10 ++-----
>>>> drivers/media/platform/atmel/atmel-isi.c | 12 ++------
>>>> drivers/media/platform/blackfin/bfin_capture.c | 14 +++------
>>>> drivers/media/platform/marvell-ccic/mcam-core.c | 12 ++++----
>>>> drivers/media/platform/soc_camera/soc_camera.c | 10 ++++---
>>>> drivers/media/platform/via-camera.c | 4 +--
>>>> drivers/media/usb/em28xx/em28xx-video.c | 36 +++++++++++++++++++----
>>>> 13 files changed, 122 insertions(+), 165 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/mt9v011.c b/drivers/media/i2c/mt9v011.c
>>>> index 5e29064fae91..3e23c5b0de1f 100644
>>>> --- a/drivers/media/i2c/mt9v011.c
>>>> +++ b/drivers/media/i2c/mt9v011.c
>>>> @@ -364,33 +364,24 @@ static int mt9v011_set_fmt(struct v4l2_subdev *sd,
>>>> return 0;
>>>> }
>>>>
>>>> -static int mt9v011_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
>>>> +static int mt9v011_g_frame_interval(struct v4l2_subdev *sd,
>>>> + struct v4l2_subdev_frame_interval *ival)
>>>> {
>>>> - struct v4l2_captureparm *cp = &parms->parm.capture;
>>>> -
>>>> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>>>> - return -EINVAL;
>>>> -
>>>> - memset(cp, 0, sizeof(struct v4l2_captureparm));
>>>> - cp->capability = V4L2_CAP_TIMEPERFRAME;
>>>> + memset(ival->reserved, 0, sizeof(ival->reserved));
>>>
>>> Hmm.. why to repeat memset everywhere? If the hole idea is to stop abusing,
>>> the best would be to do, instead:
>>
>> g_frame_interval is called by bridge drivers through the subdev ops. So that
>> path doesn't go through subdev_do_ioctl(). So it doesn't help putting it in
>> v4l2-subdev.c.
>
> True, but you could also do the same for v4l2 ioctl() handling logic.
>
> That would mean just two places with memset() instead of repeating the same
> pattern everywhere.
>
>> That doesn't mean it shouldn't be there as well. I believe my MC patch series
>> actually adds the memset in subdev_do_ioctl.
What could be done is that this patch https://patchwork.linuxtv.org/patch/46955/
is applied first. After that these memsets can be removed since internally we
don't need to touch them.
>>
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>>> index c5639817db34..b18b418c080f 100644
>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>> @@ -350,6 +350,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>> if (fi->pad >= sd->entity.num_pads)
>>> return -EINVAL;
>>>
>>> + memset(fi->reserved, 0, sizeof(ival->reserved));
>>> return v4l2_subdev_call(sd, video, g_frame_interval, arg);
>>> }
>>>
>>> (same applies to s_frame_interval).
>>>
>>>
>>>> calc_fps(sd,
>>>> - &cp->timeperframe.numerator,
>>>> - &cp->timeperframe.denominator);
>>>> + &ival->interval.numerator,
>>>> + &ival->interval.denominator);
>>>>
>>>> return 0;
>>>> }
>>>>
>>>> -static int mt9v011_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
>>>> +static int mt9v011_s_frame_interval(struct v4l2_subdev *sd,
>>>> + struct v4l2_subdev_frame_interval *ival)
>>>> {
>>>> - struct v4l2_captureparm *cp = &parms->parm.capture;
>>>> - struct v4l2_fract *tpf = &cp->timeperframe;
>>>> + struct v4l2_fract *tpf = &ival->interval;
>>>> u16 speed;
>>>>
>>>> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>>>> - return -EINVAL;
>>>> - if (cp->extendedmode != 0)
>>>> - return -EINVAL;
>>>> -
>>>
>>> Hmm... why are you removing those sanity checks everywhere?
>>> The core doesn't do it.
>>>
>>> All the above comments also apply to the other files modified by
>>> this patch.
>>
>> struct v4l2_subdev_frame_interval has neither type nor extendedmode.
>>
>> The check for type is done in the v4l2_g/s_parm_cap helpers instead.
>
> Well, the subdev handler at v4l2-subdev.c doesn't seem to be checking it.
????
Are you confusing struct v4l2_streamparm with struct v4l2_subdev_frame_interval?
v4l2_subdev.c deals with the latter, and struct v4l2_subdev_frame_interval has
no type field. There is nothing to check.
'type' makes no sense in subdev drivers anyway since it refers to a buffer type
and subdevs do not deal with buffers.
>
>
>> And extendedmode is always set to 0.
>>
>>>
>>>> + memset(ival->reserved, 0, sizeof(ival->reserved));
>>>> speed = calc_speed(sd, tpf->numerator, tpf->denominator);
>>>>
>>>> mt9v011_write(sd, R0A_MT9V011_CLK_SPEED, speed);
>>>> @@ -469,8 +460,8 @@ static const struct v4l2_subdev_core_ops mt9v011_core_ops = {
>>>> };
>>>>
>>>> static const struct v4l2_subdev_video_ops mt9v011_video_ops = {
>>>> - .g_parm = mt9v011_g_parm,
>>>> - .s_parm = mt9v011_s_parm,
>>>> + .g_frame_interval = mt9v011_g_frame_interval,
>>>> + .s_frame_interval = mt9v011_s_frame_interval,
>>>> };
>>>>
>>>> static const struct v4l2_subdev_pad_ops mt9v011_pad_ops = {
>>>> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
>>>> index 8975d16b2b24..3f962dae7534 100644
>>>> --- a/drivers/media/i2c/ov6650.c
>>>> +++ b/drivers/media/i2c/ov6650.c
>>>> @@ -201,7 +201,7 @@ struct ov6650 {
>>>> struct v4l2_rect rect; /* sensor cropping window */
>>>> unsigned long pclk_limit; /* from host */
>>>> unsigned long pclk_max; /* from resolution and format */
>>>> - struct v4l2_fract tpf; /* as requested with s_parm */
>>>> + struct v4l2_fract tpf; /* as requested with s_frame_interval */
>>>> u32 code;
>>>> enum v4l2_colorspace colorspace;
>>>> };
>>>> @@ -723,42 +723,33 @@ static int ov6650_enum_mbus_code(struct v4l2_subdev *sd,
>>>> return 0;
>>>> }
>>>>
>>>> -static int ov6650_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
>>>> +static int ov6650_g_frame_interval(struct v4l2_subdev *sd,
>>>> + struct v4l2_subdev_frame_interval *ival)
>>>> {
>>>> struct i2c_client *client = v4l2_get_subdevdata(sd);
>>>> struct ov6650 *priv = to_ov6650(client);
>>>> - struct v4l2_captureparm *cp = &parms->parm.capture;
>>>>
>>>> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>>>> - return -EINVAL;
>>>> -
>>>> - memset(cp, 0, sizeof(*cp));
>>>> - cp->capability = V4L2_CAP_TIMEPERFRAME;
>>>> - cp->timeperframe.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf,
>>>> + memset(ival->reserved, 0, sizeof(ival->reserved));
>>>> + ival->interval.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf,
>>>> priv->pclk_limit, priv->pclk_max));
>>>> - cp->timeperframe.denominator = FRAME_RATE_MAX;
>>>> + ival->interval.denominator = FRAME_RATE_MAX;
>>>>
>>>> dev_dbg(&client->dev, "Frame interval: %u/%u s\n",
>>>> - cp->timeperframe.numerator, cp->timeperframe.denominator);
>>>> + ival->interval.numerator, ival->interval.denominator);
>>>
>>> Hmm... not sure if a debug is needed here. Yet, if this is needed,
>>> IMHO, it would make mroe sense to move it to the core.
>>
>> The core doesn't see this if this subdev op is called from a bridge driver.
>
> True, but, when calling via a bridge driver, there's already a way to
> enable such kind debug.
It can debug VIDIOC_G/S_PARM, not the g_frame_interval op. Also, when called
via a v4l-subdev device node there is currently NO core logging.
For the record, I don't really care about this debug statement myself one
way or another, but changing this one way or another doesn't belong in this
patch series.
Regards,
Hans
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCHv2 2/9] media: convert g/s_parm to g/s_frame_interval in subdevs
2018-02-14 17:16 ` Hans Verkuil
@ 2018-02-14 17:30 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 35+ messages in thread
From: Mauro Carvalho Chehab @ 2018-02-14 17:30 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Sakari Ailus, Jacopo Mondi, Hans Verkuil
Em Wed, 14 Feb 2018 18:16:55 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> On 14/02/18 18:02, Mauro Carvalho Chehab wrote:
> > Em Wed, 14 Feb 2018 17:34:17 +0100
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >
> >> On 14/02/18 17:03, Mauro Carvalho Chehab wrote:
> >>> Em Mon, 22 Jan 2018 13:31:18 +0100
> >>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >>>
> >>>> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>>>
> >>>> Convert all g/s_parm calls to g/s_frame_interval. This allows us
> >>>> to remove the g/s_parm ops since those are a duplicate of
> >>>> g/s_frame_interval.
> >>>>
> >>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>>> ---
> >>>> drivers/media/i2c/mt9v011.c | 31 +++++++-------------
> >>>> drivers/media/i2c/ov6650.c | 35 +++++++++-------------
> >>>> drivers/media/i2c/ov7670.c | 24 +++++++--------
> >>>> drivers/media/i2c/ov7740.c | 31 +++++++-------------
> >>>> drivers/media/i2c/tvp514x.c | 39 +++++++++----------------
> >>>> drivers/media/i2c/vs6624.c | 29 +++++++-----------
> >>>> drivers/media/platform/atmel/atmel-isc.c | 10 ++-----
> >>>> drivers/media/platform/atmel/atmel-isi.c | 12 ++------
> >>>> drivers/media/platform/blackfin/bfin_capture.c | 14 +++------
> >>>> drivers/media/platform/marvell-ccic/mcam-core.c | 12 ++++----
> >>>> drivers/media/platform/soc_camera/soc_camera.c | 10 ++++---
> >>>> drivers/media/platform/via-camera.c | 4 +--
> >>>> drivers/media/usb/em28xx/em28xx-video.c | 36 +++++++++++++++++++----
> >>>> 13 files changed, 122 insertions(+), 165 deletions(-)
> >>>>
> >>>> diff --git a/drivers/media/i2c/mt9v011.c b/drivers/media/i2c/mt9v011.c
> >>>> index 5e29064fae91..3e23c5b0de1f 100644
> >>>> --- a/drivers/media/i2c/mt9v011.c
> >>>> +++ b/drivers/media/i2c/mt9v011.c
> >>>> @@ -364,33 +364,24 @@ static int mt9v011_set_fmt(struct v4l2_subdev *sd,
> >>>> return 0;
> >>>> }
> >>>>
> >>>> -static int mt9v011_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
> >>>> +static int mt9v011_g_frame_interval(struct v4l2_subdev *sd,
> >>>> + struct v4l2_subdev_frame_interval *ival)
> >>>> {
> >>>> - struct v4l2_captureparm *cp = &parms->parm.capture;
> >>>> -
> >>>> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> >>>> - return -EINVAL;
> >>>> -
> >>>> - memset(cp, 0, sizeof(struct v4l2_captureparm));
> >>>> - cp->capability = V4L2_CAP_TIMEPERFRAME;
> >>>> + memset(ival->reserved, 0, sizeof(ival->reserved));
> >>>
> >>> Hmm.. why to repeat memset everywhere? If the hole idea is to stop abusing,
> >>> the best would be to do, instead:
> >>
> >> g_frame_interval is called by bridge drivers through the subdev ops. So that
> >> path doesn't go through subdev_do_ioctl(). So it doesn't help putting it in
> >> v4l2-subdev.c.
> >
> > True, but you could also do the same for v4l2 ioctl() handling logic.
> >
> > That would mean just two places with memset() instead of repeating the same
> > pattern everywhere.
> >
> >> That doesn't mean it shouldn't be there as well. I believe my MC patch series
> >> actually adds the memset in subdev_do_ioctl.
>
> What could be done is that this patch https://patchwork.linuxtv.org/patch/46955/
> is applied first. After that these memsets can be removed since internally we
> don't need to touch them.
Works for me.
> >>
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >>> index c5639817db34..b18b418c080f 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >>> @@ -350,6 +350,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> >>> if (fi->pad >= sd->entity.num_pads)
> >>> return -EINVAL;
> >>>
> >>> + memset(fi->reserved, 0, sizeof(ival->reserved));
> >>> return v4l2_subdev_call(sd, video, g_frame_interval, arg);
> >>> }
> >>>
> >>> (same applies to s_frame_interval).
> >>>
> >>>
> >>>> calc_fps(sd,
> >>>> - &cp->timeperframe.numerator,
> >>>> - &cp->timeperframe.denominator);
> >>>> + &ival->interval.numerator,
> >>>> + &ival->interval.denominator);
> >>>>
> >>>> return 0;
> >>>> }
> >>>>
> >>>> -static int mt9v011_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
> >>>> +static int mt9v011_s_frame_interval(struct v4l2_subdev *sd,
> >>>> + struct v4l2_subdev_frame_interval *ival)
> >>>> {
> >>>> - struct v4l2_captureparm *cp = &parms->parm.capture;
> >>>> - struct v4l2_fract *tpf = &cp->timeperframe;
> >>>> + struct v4l2_fract *tpf = &ival->interval;
> >>>> u16 speed;
> >>>>
> >>>> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> >>>> - return -EINVAL;
> >>>> - if (cp->extendedmode != 0)
> >>>> - return -EINVAL;
> >>>> -
> >>>
> >>> Hmm... why are you removing those sanity checks everywhere?
> >>> The core doesn't do it.
> >>>
> >>> All the above comments also apply to the other files modified by
> >>> this patch.
> >>
> >> struct v4l2_subdev_frame_interval has neither type nor extendedmode.
> >>
> >> The check for type is done in the v4l2_g/s_parm_cap helpers instead.
> >
> > Well, the subdev handler at v4l2-subdev.c doesn't seem to be checking it.
>
> ????
>
> Are you confusing struct v4l2_streamparm with struct v4l2_subdev_frame_interval?
>
> v4l2_subdev.c deals with the latter, and struct v4l2_subdev_frame_interval has
> no type field. There is nothing to check.
>
> 'type' makes no sense in subdev drivers anyway since it refers to a buffer type
> and subdevs do not deal with buffers.
Yeah, you're right: those checks can be removed.
> >> And extendedmode is always set to 0.
> >>
> >>>
> >>>> + memset(ival->reserved, 0, sizeof(ival->reserved));
> >>>> speed = calc_speed(sd, tpf->numerator, tpf->denominator);
> >>>>
> >>>> mt9v011_write(sd, R0A_MT9V011_CLK_SPEED, speed);
> >>>> @@ -469,8 +460,8 @@ static const struct v4l2_subdev_core_ops mt9v011_core_ops = {
> >>>> };
> >>>>
> >>>> static const struct v4l2_subdev_video_ops mt9v011_video_ops = {
> >>>> - .g_parm = mt9v011_g_parm,
> >>>> - .s_parm = mt9v011_s_parm,
> >>>> + .g_frame_interval = mt9v011_g_frame_interval,
> >>>> + .s_frame_interval = mt9v011_s_frame_interval,
> >>>> };
> >>>>
> >>>> static const struct v4l2_subdev_pad_ops mt9v011_pad_ops = {
> >>>> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> >>>> index 8975d16b2b24..3f962dae7534 100644
> >>>> --- a/drivers/media/i2c/ov6650.c
> >>>> +++ b/drivers/media/i2c/ov6650.c
> >>>> @@ -201,7 +201,7 @@ struct ov6650 {
> >>>> struct v4l2_rect rect; /* sensor cropping window */
> >>>> unsigned long pclk_limit; /* from host */
> >>>> unsigned long pclk_max; /* from resolution and format */
> >>>> - struct v4l2_fract tpf; /* as requested with s_parm */
> >>>> + struct v4l2_fract tpf; /* as requested with s_frame_interval */
> >>>> u32 code;
> >>>> enum v4l2_colorspace colorspace;
> >>>> };
> >>>> @@ -723,42 +723,33 @@ static int ov6650_enum_mbus_code(struct v4l2_subdev *sd,
> >>>> return 0;
> >>>> }
> >>>>
> >>>> -static int ov6650_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
> >>>> +static int ov6650_g_frame_interval(struct v4l2_subdev *sd,
> >>>> + struct v4l2_subdev_frame_interval *ival)
> >>>> {
> >>>> struct i2c_client *client = v4l2_get_subdevdata(sd);
> >>>> struct ov6650 *priv = to_ov6650(client);
> >>>> - struct v4l2_captureparm *cp = &parms->parm.capture;
> >>>>
> >>>> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> >>>> - return -EINVAL;
> >>>> -
> >>>> - memset(cp, 0, sizeof(*cp));
> >>>> - cp->capability = V4L2_CAP_TIMEPERFRAME;
> >>>> - cp->timeperframe.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf,
> >>>> + memset(ival->reserved, 0, sizeof(ival->reserved));
> >>>> + ival->interval.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf,
> >>>> priv->pclk_limit, priv->pclk_max));
> >>>> - cp->timeperframe.denominator = FRAME_RATE_MAX;
> >>>> + ival->interval.denominator = FRAME_RATE_MAX;
> >>>>
> >>>> dev_dbg(&client->dev, "Frame interval: %u/%u s\n",
> >>>> - cp->timeperframe.numerator, cp->timeperframe.denominator);
> >>>> + ival->interval.numerator, ival->interval.denominator);
> >>>
> >>> Hmm... not sure if a debug is needed here. Yet, if this is needed,
> >>> IMHO, it would make mroe sense to move it to the core.
> >>
> >> The core doesn't see this if this subdev op is called from a bridge driver.
> >
> > True, but, when calling via a bridge driver, there's already a way to
> > enable such kind debug.
>
> It can debug VIDIOC_G/S_PARM, not the g_frame_interval op. Also, when called
> via a v4l-subdev device node there is currently NO core logging.
>
> For the record, I don't really care about this debug statement myself one
> way or another, but changing this one way or another doesn't belong in this
> patch series.
Ok. Yet, IMHO, it is probably safe to just remove the debug statements
on the above driver, as I fail to see that just ov6650 driver would
need this.
>
> Regards,
>
> Hans
Thanks,
Mauro
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCHv2 3/9] staging: atomisp: Kill subdev s_parm abuse
2018-01-22 12:31 [PATCHv2 0/9] media: replace g/s_parm by g/s_frame_interval Hans Verkuil
2018-01-22 12:31 ` [PATCHv2 1/9] v4l2-common: create v4l2_g/s_parm_cap helpers Hans Verkuil
2018-01-22 12:31 ` [PATCHv2 2/9] media: convert g/s_parm to g/s_frame_interval in subdevs Hans Verkuil
@ 2018-01-22 12:31 ` Hans Verkuil
2018-02-14 16:14 ` Mauro Carvalho Chehab
2018-01-22 12:31 ` [PATCHv2 4/9] staging: atomisp: i2c: Disable non-preview configurations Hans Verkuil
` (6 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Hans Verkuil @ 2018-01-22 12:31 UTC (permalink / raw)
To: linux-media; +Cc: Sakari Ailus, Jacopo Mondi, Hans Verkuil
From: Sakari Ailus <sakari.ailus@linux.intel.com>
Remove sensor driver's interface that made use of use case specific
knowledge of platform capabilities.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 26 ---------
drivers/staging/media/atomisp/i2c/atomisp-gc2235.c | 26 ---------
drivers/staging/media/atomisp/i2c/atomisp-ov2680.c | 29 ---------
drivers/staging/media/atomisp/i2c/atomisp-ov2722.c | 26 ---------
drivers/staging/media/atomisp/i2c/gc0310.h | 43 --------------
drivers/staging/media/atomisp/i2c/gc2235.h | 1 -
drivers/staging/media/atomisp/i2c/ov2680.h | 68 ----------------------
.../media/atomisp/i2c/ov5693/atomisp-ov5693.c | 27 ---------
.../media/atomisp/pci/atomisp2/atomisp_cmd.c | 9 +--
.../media/atomisp/pci/atomisp2/atomisp_subdev.c | 12 +---
10 files changed, 3 insertions(+), 264 deletions(-)
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index 61b7598469eb..572c9127c24d 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -1224,37 +1224,12 @@ static int gc0310_g_parm(struct v4l2_subdev *sd,
if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
param->parm.capture.timeperframe.numerator = 1;
- param->parm.capture.capturemode = dev->run_mode;
param->parm.capture.timeperframe.denominator =
gc0310_res[dev->fmt_idx].fps;
}
return 0;
}
-static int gc0310_s_parm(struct v4l2_subdev *sd,
- struct v4l2_streamparm *param)
-{
- struct gc0310_device *dev = to_gc0310_sensor(sd);
- dev->run_mode = param->parm.capture.capturemode;
-
- mutex_lock(&dev->input_lock);
- switch (dev->run_mode) {
- case CI_MODE_VIDEO:
- gc0310_res = gc0310_res_video;
- N_RES = N_RES_VIDEO;
- break;
- case CI_MODE_STILL_CAPTURE:
- gc0310_res = gc0310_res_still;
- N_RES = N_RES_STILL;
- break;
- default:
- gc0310_res = gc0310_res_preview;
- N_RES = N_RES_PREVIEW;
- }
- mutex_unlock(&dev->input_lock);
- return 0;
-}
-
static int gc0310_g_frame_interval(struct v4l2_subdev *sd,
struct v4l2_subdev_frame_interval *interval)
{
@@ -1314,7 +1289,6 @@ static const struct v4l2_subdev_sensor_ops gc0310_sensor_ops = {
static const struct v4l2_subdev_video_ops gc0310_video_ops = {
.s_stream = gc0310_s_stream,
.g_parm = gc0310_g_parm,
- .s_parm = gc0310_s_parm,
.g_frame_interval = gc0310_g_frame_interval,
};
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
index d8de46da64ae..2bc179f3afe5 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
@@ -964,37 +964,12 @@ static int gc2235_g_parm(struct v4l2_subdev *sd,
if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
param->parm.capture.timeperframe.numerator = 1;
- param->parm.capture.capturemode = dev->run_mode;
param->parm.capture.timeperframe.denominator =
gc2235_res[dev->fmt_idx].fps;
}
return 0;
}
-static int gc2235_s_parm(struct v4l2_subdev *sd,
- struct v4l2_streamparm *param)
-{
- struct gc2235_device *dev = to_gc2235_sensor(sd);
- dev->run_mode = param->parm.capture.capturemode;
-
- mutex_lock(&dev->input_lock);
- switch (dev->run_mode) {
- case CI_MODE_VIDEO:
- gc2235_res = gc2235_res_video;
- N_RES = N_RES_VIDEO;
- break;
- case CI_MODE_STILL_CAPTURE:
- gc2235_res = gc2235_res_still;
- N_RES = N_RES_STILL;
- break;
- default:
- gc2235_res = gc2235_res_preview;
- N_RES = N_RES_PREVIEW;
- }
- mutex_unlock(&dev->input_lock);
- return 0;
-}
-
static int gc2235_g_frame_interval(struct v4l2_subdev *sd,
struct v4l2_subdev_frame_interval *interval)
{
@@ -1053,7 +1028,6 @@ static const struct v4l2_subdev_sensor_ops gc2235_sensor_ops = {
static const struct v4l2_subdev_video_ops gc2235_video_ops = {
.s_stream = gc2235_s_stream,
.g_parm = gc2235_g_parm,
- .s_parm = gc2235_s_parm,
.g_frame_interval = gc2235_g_frame_interval,
};
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index 84f8d33ce2d1..e3e0fdd0c816 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -1300,40 +1300,12 @@ static int ov2680_g_parm(struct v4l2_subdev *sd,
if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
param->parm.capture.timeperframe.numerator = 1;
- param->parm.capture.capturemode = dev->run_mode;
param->parm.capture.timeperframe.denominator =
ov2680_res[dev->fmt_idx].fps;
}
return 0;
}
-static int ov2680_s_parm(struct v4l2_subdev *sd,
- struct v4l2_streamparm *param)
-{
- struct ov2680_device *dev = to_ov2680_sensor(sd);
- struct i2c_client *client = v4l2_get_subdevdata(sd);
- dev->run_mode = param->parm.capture.capturemode;
-
- v4l2_info(client, "\n%s:run_mode :%x\n", __func__, dev->run_mode);
-
- mutex_lock(&dev->input_lock);
- switch (dev->run_mode) {
- case CI_MODE_VIDEO:
- ov2680_res = ov2680_res_video;
- N_RES = N_RES_VIDEO;
- break;
- case CI_MODE_STILL_CAPTURE:
- ov2680_res = ov2680_res_still;
- N_RES = N_RES_STILL;
- break;
- default:
- ov2680_res = ov2680_res_preview;
- N_RES = N_RES_PREVIEW;
- }
- mutex_unlock(&dev->input_lock);
- return 0;
-}
-
static int ov2680_g_frame_interval(struct v4l2_subdev *sd,
struct v4l2_subdev_frame_interval *interval)
{
@@ -1388,7 +1360,6 @@ static int ov2680_g_skip_frames(struct v4l2_subdev *sd, u32 *frames)
static const struct v4l2_subdev_video_ops ov2680_video_ops = {
.s_stream = ov2680_s_stream,
.g_parm = ov2680_g_parm,
- .s_parm = ov2680_s_parm,
.g_frame_interval = ov2680_g_frame_interval,
};
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
index 2b6ae0faf972..cd9f6433cd42 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
@@ -1103,37 +1103,12 @@ static int ov2722_g_parm(struct v4l2_subdev *sd,
if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
param->parm.capture.timeperframe.numerator = 1;
- param->parm.capture.capturemode = dev->run_mode;
param->parm.capture.timeperframe.denominator =
ov2722_res[dev->fmt_idx].fps;
}
return 0;
}
-static int ov2722_s_parm(struct v4l2_subdev *sd,
- struct v4l2_streamparm *param)
-{
- struct ov2722_device *dev = to_ov2722_sensor(sd);
- dev->run_mode = param->parm.capture.capturemode;
-
- mutex_lock(&dev->input_lock);
- switch (dev->run_mode) {
- case CI_MODE_VIDEO:
- ov2722_res = ov2722_res_video;
- N_RES = N_RES_VIDEO;
- break;
- case CI_MODE_STILL_CAPTURE:
- ov2722_res = ov2722_res_still;
- N_RES = N_RES_STILL;
- break;
- default:
- ov2722_res = ov2722_res_preview;
- N_RES = N_RES_PREVIEW;
- }
- mutex_unlock(&dev->input_lock);
- return 0;
-}
-
static int ov2722_g_frame_interval(struct v4l2_subdev *sd,
struct v4l2_subdev_frame_interval *interval)
{
@@ -1193,7 +1168,6 @@ static const struct v4l2_subdev_sensor_ops ov2722_sensor_ops = {
static const struct v4l2_subdev_video_ops ov2722_video_ops = {
.s_stream = ov2722_s_stream,
.g_parm = ov2722_g_parm,
- .s_parm = ov2722_s_parm,
.g_frame_interval = ov2722_g_frame_interval,
};
diff --git a/drivers/staging/media/atomisp/i2c/gc0310.h b/drivers/staging/media/atomisp/i2c/gc0310.h
index c422d0398fc7..af6b11f6e5e7 100644
--- a/drivers/staging/media/atomisp/i2c/gc0310.h
+++ b/drivers/staging/media/atomisp/i2c/gc0310.h
@@ -150,7 +150,6 @@ struct gc0310_device {
struct camera_sensor_platform_data *platform_data;
int vt_pix_clk_freq_mhz;
int fmt_idx;
- int run_mode;
u8 res;
u8 type;
};
@@ -400,48 +399,6 @@ struct gc0310_resolution gc0310_res_preview[] = {
};
#define N_RES_PREVIEW (ARRAY_SIZE(gc0310_res_preview))
-struct gc0310_resolution gc0310_res_still[] = {
- {
- .desc = "gc0310_VGA_30fps",
- .width = 656, // 648,
- .height = 496, // 488,
- .fps = 30,
- //.pix_clk_freq = 73,
- .used = 0,
-#if 0
- .pixels_per_line = 0x0314,
- .lines_per_frame = 0x0213,
-#endif
- .bin_factor_x = 1,
- .bin_factor_y = 1,
- .bin_mode = 0,
- .skip_frames = 2,
- .regs = gc0310_VGA_30fps,
- },
-};
-#define N_RES_STILL (ARRAY_SIZE(gc0310_res_still))
-
-struct gc0310_resolution gc0310_res_video[] = {
- {
- .desc = "gc0310_VGA_30fps",
- .width = 656, // 648,
- .height = 496, // 488,
- .fps = 30,
- //.pix_clk_freq = 73,
- .used = 0,
-#if 0
- .pixels_per_line = 0x0314,
- .lines_per_frame = 0x0213,
-#endif
- .bin_factor_x = 1,
- .bin_factor_y = 1,
- .bin_mode = 0,
- .skip_frames = 2,
- .regs = gc0310_VGA_30fps,
- },
-};
-#define N_RES_VIDEO (ARRAY_SIZE(gc0310_res_video))
-
static struct gc0310_resolution *gc0310_res = gc0310_res_preview;
static unsigned long N_RES = N_RES_PREVIEW;
#endif
diff --git a/drivers/staging/media/atomisp/i2c/gc2235.h b/drivers/staging/media/atomisp/i2c/gc2235.h
index 3c30a05c3991..45a54fea5466 100644
--- a/drivers/staging/media/atomisp/i2c/gc2235.h
+++ b/drivers/staging/media/atomisp/i2c/gc2235.h
@@ -156,7 +156,6 @@ struct gc2235_device {
struct camera_sensor_platform_data *platform_data;
int vt_pix_clk_freq_mhz;
int fmt_idx;
- int run_mode;
u8 res;
u8 type;
};
diff --git a/drivers/staging/media/atomisp/i2c/ov2680.h b/drivers/staging/media/atomisp/i2c/ov2680.h
index 03f75dd80f87..cb38e6e79409 100644
--- a/drivers/staging/media/atomisp/i2c/ov2680.h
+++ b/drivers/staging/media/atomisp/i2c/ov2680.h
@@ -850,74 +850,6 @@ struct ov2680_format {
};
#define N_RES_PREVIEW (ARRAY_SIZE(ov2680_res_preview))
-static struct ov2680_resolution ov2680_res_still[] = {
- {
- .desc = "ov2680_1616x1216_30fps",
- .width = 1616,
- .height = 1216,
- .pix_clk_freq = 66,
- .fps = 30,
- .used = 0,
- .pixels_per_line = 1698,//1704,
- .lines_per_frame = 1294,
- .bin_factor_x = 0,
- .bin_factor_y = 0,
- .bin_mode = 0,
- .skip_frames = 3,
- .regs = ov2680_1616x1216_30fps,
- },
- {
- .desc = "ov2680_1616x916_30fps",
- .width = 1616,
- .height = 916,
- .fps = 30,
- .pix_clk_freq = 66,
- .used = 0,
- .pixels_per_line = 1698,//1704,
- .lines_per_frame = 1294,
- .bin_factor_x = 0,
- .bin_factor_y = 0,
- .bin_mode = 0,
- .skip_frames = 3,
- .regs = ov2680_1616x916_30fps,
- },
-};
-#define N_RES_STILL (ARRAY_SIZE(ov2680_res_still))
-
-static struct ov2680_resolution ov2680_res_video[] = {
- {
- .desc = "ov2680_1616x1216_30fps",
- .width = 1616,
- .height = 1216,
- .pix_clk_freq = 66,
- .fps = 30,
- .used = 0,
- .pixels_per_line = 1698,//1704,
- .lines_per_frame = 1294,
- .bin_factor_x = 0,
- .bin_factor_y = 0,
- .bin_mode = 0,
- .skip_frames = 3,
- .regs = ov2680_1616x1216_30fps,
- },
- {
- .desc = "ov2680_720p_30fps",
- .width = 1616,
- .height = 916,
- .fps = 30,
- .pix_clk_freq = 66,
- .used = 0,
- .pixels_per_line = 1698,//1704,
- .lines_per_frame = 1294,
- .bin_factor_x = 0,
- .bin_factor_y = 0,
- .bin_mode = 0,
- .skip_frames = 3,
- .regs = ov2680_1616x916_30fps,
- },
-};
-#define N_RES_VIDEO (ARRAY_SIZE(ov2680_res_video))
-
static struct ov2680_resolution *ov2680_res = ov2680_res_preview;
static unsigned long N_RES = N_RES_PREVIEW;
diff --git a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
index 40d01bf4bf28..7f594c7de76e 100644
--- a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
+++ b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
@@ -1825,38 +1825,12 @@ static int ov5693_g_parm(struct v4l2_subdev *sd,
if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
param->parm.capture.timeperframe.numerator = 1;
- param->parm.capture.capturemode = dev->run_mode;
param->parm.capture.timeperframe.denominator =
ov5693_res[dev->fmt_idx].fps;
}
return 0;
}
-static int ov5693_s_parm(struct v4l2_subdev *sd,
- struct v4l2_streamparm *param)
-{
- struct ov5693_device *dev = to_ov5693_sensor(sd);
-
- dev->run_mode = param->parm.capture.capturemode;
-
- mutex_lock(&dev->input_lock);
- switch (dev->run_mode) {
- case CI_MODE_VIDEO:
- ov5693_res = ov5693_res_video;
- N_RES = N_RES_VIDEO;
- break;
- case CI_MODE_STILL_CAPTURE:
- ov5693_res = ov5693_res_still;
- N_RES = N_RES_STILL;
- break;
- default:
- ov5693_res = ov5693_res_preview;
- N_RES = N_RES_PREVIEW;
- }
- mutex_unlock(&dev->input_lock);
- return 0;
-}
-
static int ov5693_g_frame_interval(struct v4l2_subdev *sd,
struct v4l2_subdev_frame_interval *interval)
{
@@ -1900,7 +1874,6 @@ static int ov5693_enum_frame_size(struct v4l2_subdev *sd,
static const struct v4l2_subdev_video_ops ov5693_video_ops = {
.s_stream = ov5693_s_stream,
.g_parm = ov5693_g_parm,
- .s_parm = ov5693_s_parm,
.g_frame_interval = ov5693_g_frame_interval,
};
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index debf0e3853ff..3410a7fb1fcf 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -2091,7 +2091,7 @@ int atomisp_set_sensor_runmode(struct atomisp_sub_device *asd,
struct atomisp_device *isp = asd->isp;
struct v4l2_ctrl *c;
struct v4l2_streamparm p = {0};
- int ret;
+ int ret = 0;
int modes[] = { CI_MODE_NONE,
CI_MODE_VIDEO,
CI_MODE_STILL_CAPTURE,
@@ -2105,13 +2105,8 @@ int atomisp_set_sensor_runmode(struct atomisp_sub_device *asd,
c = v4l2_ctrl_find(isp->inputs[asd->input_curr].camera->ctrl_handler,
V4L2_CID_RUN_MODE);
- if (c) {
+ if (c)
ret = v4l2_ctrl_s_ctrl(c, runmode->mode);
- } else {
- p.parm.capture.capturemode = modes[runmode->mode];
- ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
- video, s_parm, &p);
- }
mutex_unlock(asd->ctrl_handler.lock);
return ret;
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
index f3e18d627b0a..b78276ac22da 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
@@ -819,12 +819,6 @@ static int __atomisp_update_run_mode(struct atomisp_sub_device *asd)
struct atomisp_device *isp = asd->isp;
struct v4l2_ctrl *ctrl = asd->run_mode;
struct v4l2_ctrl *c;
- struct v4l2_streamparm p = {0};
- int modes[] = { CI_MODE_NONE,
- CI_MODE_VIDEO,
- CI_MODE_STILL_CAPTURE,
- CI_MODE_CONTINUOUS,
- CI_MODE_PREVIEW };
s32 mode;
if (ctrl->val != ATOMISP_RUN_MODE_VIDEO &&
@@ -840,11 +834,7 @@ static int __atomisp_update_run_mode(struct atomisp_sub_device *asd)
if (c)
return v4l2_ctrl_s_ctrl(c, mode);
- /* Fall back to obsolete s_parm */
- p.parm.capture.capturemode = modes[mode];
-
- return v4l2_subdev_call(
- isp->inputs[asd->input_curr].camera, video, s_parm, &p);
+ return 0;
}
int atomisp_update_run_mode(struct atomisp_sub_device *asd)
--
2.15.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCHv2 3/9] staging: atomisp: Kill subdev s_parm abuse
2018-01-22 12:31 ` [PATCHv2 3/9] staging: atomisp: Kill subdev s_parm abuse Hans Verkuil
@ 2018-02-14 16:14 ` Mauro Carvalho Chehab
2018-02-16 9:04 ` Sakari Ailus
0 siblings, 1 reply; 35+ messages in thread
From: Mauro Carvalho Chehab @ 2018-02-14 16:14 UTC (permalink / raw)
To: Hans Verkuil, Sakari Ailus, Jacopo Mondi; +Cc: linux-media, Hans Verkuil
Sakari,
Em Mon, 22 Jan 2018 13:31:19 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> From: Sakari Ailus <sakari.ailus@linux.intel.com>
>
> Remove sensor driver's interface that made use of use case specific
> knowledge of platform capabilities.
Could you better describe it? What s_param abuse?
What happens after this patch? It seems that atomISP relies on
gc0310_res. So, I would be expecting that a patch removing
s_param would be also adding/changing other parts of the code
accordingly, in order to get rid of that as a hole (or initialize
it somewhere else).
Regards,
Mauro
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 26 ---------
> drivers/staging/media/atomisp/i2c/atomisp-gc2235.c | 26 ---------
> drivers/staging/media/atomisp/i2c/atomisp-ov2680.c | 29 ---------
> drivers/staging/media/atomisp/i2c/atomisp-ov2722.c | 26 ---------
> drivers/staging/media/atomisp/i2c/gc0310.h | 43 --------------
> drivers/staging/media/atomisp/i2c/gc2235.h | 1 -
> drivers/staging/media/atomisp/i2c/ov2680.h | 68 ----------------------
> .../media/atomisp/i2c/ov5693/atomisp-ov5693.c | 27 ---------
> .../media/atomisp/pci/atomisp2/atomisp_cmd.c | 9 +--
> .../media/atomisp/pci/atomisp2/atomisp_subdev.c | 12 +---
> 10 files changed, 3 insertions(+), 264 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> index 61b7598469eb..572c9127c24d 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> @@ -1224,37 +1224,12 @@ static int gc0310_g_parm(struct v4l2_subdev *sd,
> if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
> param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> param->parm.capture.timeperframe.numerator = 1;
> - param->parm.capture.capturemode = dev->run_mode;
> param->parm.capture.timeperframe.denominator =
> gc0310_res[dev->fmt_idx].fps;
> }
> return 0;
> }
>
> -static int gc0310_s_parm(struct v4l2_subdev *sd,
> - struct v4l2_streamparm *param)
> -{
> - struct gc0310_device *dev = to_gc0310_sensor(sd);
> - dev->run_mode = param->parm.capture.capturemode;
> -
> - mutex_lock(&dev->input_lock);
> - switch (dev->run_mode) {
> - case CI_MODE_VIDEO:
> - gc0310_res = gc0310_res_video;
> - N_RES = N_RES_VIDEO;
> - break;
> - case CI_MODE_STILL_CAPTURE:
> - gc0310_res = gc0310_res_still;
> - N_RES = N_RES_STILL;
> - break;
> - default:
> - gc0310_res = gc0310_res_preview;
> - N_RES = N_RES_PREVIEW;
> - }
> - mutex_unlock(&dev->input_lock);
> - return 0;
> -}
> -
> static int gc0310_g_frame_interval(struct v4l2_subdev *sd,
> struct v4l2_subdev_frame_interval *interval)
> {
> @@ -1314,7 +1289,6 @@ static const struct v4l2_subdev_sensor_ops gc0310_sensor_ops = {
> static const struct v4l2_subdev_video_ops gc0310_video_ops = {
> .s_stream = gc0310_s_stream,
> .g_parm = gc0310_g_parm,
> - .s_parm = gc0310_s_parm,
> .g_frame_interval = gc0310_g_frame_interval,
> };
>
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
> index d8de46da64ae..2bc179f3afe5 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
> @@ -964,37 +964,12 @@ static int gc2235_g_parm(struct v4l2_subdev *sd,
> if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
> param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> param->parm.capture.timeperframe.numerator = 1;
> - param->parm.capture.capturemode = dev->run_mode;
> param->parm.capture.timeperframe.denominator =
> gc2235_res[dev->fmt_idx].fps;
> }
> return 0;
> }
>
> -static int gc2235_s_parm(struct v4l2_subdev *sd,
> - struct v4l2_streamparm *param)
> -{
> - struct gc2235_device *dev = to_gc2235_sensor(sd);
> - dev->run_mode = param->parm.capture.capturemode;
> -
> - mutex_lock(&dev->input_lock);
> - switch (dev->run_mode) {
> - case CI_MODE_VIDEO:
> - gc2235_res = gc2235_res_video;
> - N_RES = N_RES_VIDEO;
> - break;
> - case CI_MODE_STILL_CAPTURE:
> - gc2235_res = gc2235_res_still;
> - N_RES = N_RES_STILL;
> - break;
> - default:
> - gc2235_res = gc2235_res_preview;
> - N_RES = N_RES_PREVIEW;
> - }
> - mutex_unlock(&dev->input_lock);
> - return 0;
> -}
> -
> static int gc2235_g_frame_interval(struct v4l2_subdev *sd,
> struct v4l2_subdev_frame_interval *interval)
> {
> @@ -1053,7 +1028,6 @@ static const struct v4l2_subdev_sensor_ops gc2235_sensor_ops = {
> static const struct v4l2_subdev_video_ops gc2235_video_ops = {
> .s_stream = gc2235_s_stream,
> .g_parm = gc2235_g_parm,
> - .s_parm = gc2235_s_parm,
> .g_frame_interval = gc2235_g_frame_interval,
> };
>
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> index 84f8d33ce2d1..e3e0fdd0c816 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> @@ -1300,40 +1300,12 @@ static int ov2680_g_parm(struct v4l2_subdev *sd,
> if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
> param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> param->parm.capture.timeperframe.numerator = 1;
> - param->parm.capture.capturemode = dev->run_mode;
> param->parm.capture.timeperframe.denominator =
> ov2680_res[dev->fmt_idx].fps;
> }
> return 0;
> }
>
> -static int ov2680_s_parm(struct v4l2_subdev *sd,
> - struct v4l2_streamparm *param)
> -{
> - struct ov2680_device *dev = to_ov2680_sensor(sd);
> - struct i2c_client *client = v4l2_get_subdevdata(sd);
> - dev->run_mode = param->parm.capture.capturemode;
> -
> - v4l2_info(client, "\n%s:run_mode :%x\n", __func__, dev->run_mode);
> -
> - mutex_lock(&dev->input_lock);
> - switch (dev->run_mode) {
> - case CI_MODE_VIDEO:
> - ov2680_res = ov2680_res_video;
> - N_RES = N_RES_VIDEO;
> - break;
> - case CI_MODE_STILL_CAPTURE:
> - ov2680_res = ov2680_res_still;
> - N_RES = N_RES_STILL;
> - break;
> - default:
> - ov2680_res = ov2680_res_preview;
> - N_RES = N_RES_PREVIEW;
> - }
> - mutex_unlock(&dev->input_lock);
> - return 0;
> -}
> -
> static int ov2680_g_frame_interval(struct v4l2_subdev *sd,
> struct v4l2_subdev_frame_interval *interval)
> {
> @@ -1388,7 +1360,6 @@ static int ov2680_g_skip_frames(struct v4l2_subdev *sd, u32 *frames)
> static const struct v4l2_subdev_video_ops ov2680_video_ops = {
> .s_stream = ov2680_s_stream,
> .g_parm = ov2680_g_parm,
> - .s_parm = ov2680_s_parm,
> .g_frame_interval = ov2680_g_frame_interval,
> };
>
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> index 2b6ae0faf972..cd9f6433cd42 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> @@ -1103,37 +1103,12 @@ static int ov2722_g_parm(struct v4l2_subdev *sd,
> if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
> param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> param->parm.capture.timeperframe.numerator = 1;
> - param->parm.capture.capturemode = dev->run_mode;
> param->parm.capture.timeperframe.denominator =
> ov2722_res[dev->fmt_idx].fps;
> }
> return 0;
> }
>
> -static int ov2722_s_parm(struct v4l2_subdev *sd,
> - struct v4l2_streamparm *param)
> -{
> - struct ov2722_device *dev = to_ov2722_sensor(sd);
> - dev->run_mode = param->parm.capture.capturemode;
> -
> - mutex_lock(&dev->input_lock);
> - switch (dev->run_mode) {
> - case CI_MODE_VIDEO:
> - ov2722_res = ov2722_res_video;
> - N_RES = N_RES_VIDEO;
> - break;
> - case CI_MODE_STILL_CAPTURE:
> - ov2722_res = ov2722_res_still;
> - N_RES = N_RES_STILL;
> - break;
> - default:
> - ov2722_res = ov2722_res_preview;
> - N_RES = N_RES_PREVIEW;
> - }
> - mutex_unlock(&dev->input_lock);
> - return 0;
> -}
> -
> static int ov2722_g_frame_interval(struct v4l2_subdev *sd,
> struct v4l2_subdev_frame_interval *interval)
> {
> @@ -1193,7 +1168,6 @@ static const struct v4l2_subdev_sensor_ops ov2722_sensor_ops = {
> static const struct v4l2_subdev_video_ops ov2722_video_ops = {
> .s_stream = ov2722_s_stream,
> .g_parm = ov2722_g_parm,
> - .s_parm = ov2722_s_parm,
> .g_frame_interval = ov2722_g_frame_interval,
> };
>
> diff --git a/drivers/staging/media/atomisp/i2c/gc0310.h b/drivers/staging/media/atomisp/i2c/gc0310.h
> index c422d0398fc7..af6b11f6e5e7 100644
> --- a/drivers/staging/media/atomisp/i2c/gc0310.h
> +++ b/drivers/staging/media/atomisp/i2c/gc0310.h
> @@ -150,7 +150,6 @@ struct gc0310_device {
> struct camera_sensor_platform_data *platform_data;
> int vt_pix_clk_freq_mhz;
> int fmt_idx;
> - int run_mode;
> u8 res;
> u8 type;
> };
> @@ -400,48 +399,6 @@ struct gc0310_resolution gc0310_res_preview[] = {
> };
> #define N_RES_PREVIEW (ARRAY_SIZE(gc0310_res_preview))
>
> -struct gc0310_resolution gc0310_res_still[] = {
> - {
> - .desc = "gc0310_VGA_30fps",
> - .width = 656, // 648,
> - .height = 496, // 488,
> - .fps = 30,
> - //.pix_clk_freq = 73,
> - .used = 0,
> -#if 0
> - .pixels_per_line = 0x0314,
> - .lines_per_frame = 0x0213,
> -#endif
> - .bin_factor_x = 1,
> - .bin_factor_y = 1,
> - .bin_mode = 0,
> - .skip_frames = 2,
> - .regs = gc0310_VGA_30fps,
> - },
> -};
> -#define N_RES_STILL (ARRAY_SIZE(gc0310_res_still))
> -
> -struct gc0310_resolution gc0310_res_video[] = {
> - {
> - .desc = "gc0310_VGA_30fps",
> - .width = 656, // 648,
> - .height = 496, // 488,
> - .fps = 30,
> - //.pix_clk_freq = 73,
> - .used = 0,
> -#if 0
> - .pixels_per_line = 0x0314,
> - .lines_per_frame = 0x0213,
> -#endif
> - .bin_factor_x = 1,
> - .bin_factor_y = 1,
> - .bin_mode = 0,
> - .skip_frames = 2,
> - .regs = gc0310_VGA_30fps,
> - },
> -};
> -#define N_RES_VIDEO (ARRAY_SIZE(gc0310_res_video))
> -
> static struct gc0310_resolution *gc0310_res = gc0310_res_preview;
> static unsigned long N_RES = N_RES_PREVIEW;
> #endif
> diff --git a/drivers/staging/media/atomisp/i2c/gc2235.h b/drivers/staging/media/atomisp/i2c/gc2235.h
> index 3c30a05c3991..45a54fea5466 100644
> --- a/drivers/staging/media/atomisp/i2c/gc2235.h
> +++ b/drivers/staging/media/atomisp/i2c/gc2235.h
> @@ -156,7 +156,6 @@ struct gc2235_device {
> struct camera_sensor_platform_data *platform_data;
> int vt_pix_clk_freq_mhz;
> int fmt_idx;
> - int run_mode;
> u8 res;
> u8 type;
> };
> diff --git a/drivers/staging/media/atomisp/i2c/ov2680.h b/drivers/staging/media/atomisp/i2c/ov2680.h
> index 03f75dd80f87..cb38e6e79409 100644
> --- a/drivers/staging/media/atomisp/i2c/ov2680.h
> +++ b/drivers/staging/media/atomisp/i2c/ov2680.h
> @@ -850,74 +850,6 @@ struct ov2680_format {
> };
> #define N_RES_PREVIEW (ARRAY_SIZE(ov2680_res_preview))
>
> -static struct ov2680_resolution ov2680_res_still[] = {
> - {
> - .desc = "ov2680_1616x1216_30fps",
> - .width = 1616,
> - .height = 1216,
> - .pix_clk_freq = 66,
> - .fps = 30,
> - .used = 0,
> - .pixels_per_line = 1698,//1704,
> - .lines_per_frame = 1294,
> - .bin_factor_x = 0,
> - .bin_factor_y = 0,
> - .bin_mode = 0,
> - .skip_frames = 3,
> - .regs = ov2680_1616x1216_30fps,
> - },
> - {
> - .desc = "ov2680_1616x916_30fps",
> - .width = 1616,
> - .height = 916,
> - .fps = 30,
> - .pix_clk_freq = 66,
> - .used = 0,
> - .pixels_per_line = 1698,//1704,
> - .lines_per_frame = 1294,
> - .bin_factor_x = 0,
> - .bin_factor_y = 0,
> - .bin_mode = 0,
> - .skip_frames = 3,
> - .regs = ov2680_1616x916_30fps,
> - },
> -};
> -#define N_RES_STILL (ARRAY_SIZE(ov2680_res_still))
> -
> -static struct ov2680_resolution ov2680_res_video[] = {
> - {
> - .desc = "ov2680_1616x1216_30fps",
> - .width = 1616,
> - .height = 1216,
> - .pix_clk_freq = 66,
> - .fps = 30,
> - .used = 0,
> - .pixels_per_line = 1698,//1704,
> - .lines_per_frame = 1294,
> - .bin_factor_x = 0,
> - .bin_factor_y = 0,
> - .bin_mode = 0,
> - .skip_frames = 3,
> - .regs = ov2680_1616x1216_30fps,
> - },
> - {
> - .desc = "ov2680_720p_30fps",
> - .width = 1616,
> - .height = 916,
> - .fps = 30,
> - .pix_clk_freq = 66,
> - .used = 0,
> - .pixels_per_line = 1698,//1704,
> - .lines_per_frame = 1294,
> - .bin_factor_x = 0,
> - .bin_factor_y = 0,
> - .bin_mode = 0,
> - .skip_frames = 3,
> - .regs = ov2680_1616x916_30fps,
> - },
> -};
> -#define N_RES_VIDEO (ARRAY_SIZE(ov2680_res_video))
> -
> static struct ov2680_resolution *ov2680_res = ov2680_res_preview;
> static unsigned long N_RES = N_RES_PREVIEW;
>
> diff --git a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
> index 40d01bf4bf28..7f594c7de76e 100644
> --- a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
> +++ b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
> @@ -1825,38 +1825,12 @@ static int ov5693_g_parm(struct v4l2_subdev *sd,
> if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
> param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> param->parm.capture.timeperframe.numerator = 1;
> - param->parm.capture.capturemode = dev->run_mode;
> param->parm.capture.timeperframe.denominator =
> ov5693_res[dev->fmt_idx].fps;
> }
> return 0;
> }
>
> -static int ov5693_s_parm(struct v4l2_subdev *sd,
> - struct v4l2_streamparm *param)
> -{
> - struct ov5693_device *dev = to_ov5693_sensor(sd);
> -
> - dev->run_mode = param->parm.capture.capturemode;
> -
> - mutex_lock(&dev->input_lock);
> - switch (dev->run_mode) {
> - case CI_MODE_VIDEO:
> - ov5693_res = ov5693_res_video;
> - N_RES = N_RES_VIDEO;
> - break;
> - case CI_MODE_STILL_CAPTURE:
> - ov5693_res = ov5693_res_still;
> - N_RES = N_RES_STILL;
> - break;
> - default:
> - ov5693_res = ov5693_res_preview;
> - N_RES = N_RES_PREVIEW;
> - }
> - mutex_unlock(&dev->input_lock);
> - return 0;
> -}
> -
> static int ov5693_g_frame_interval(struct v4l2_subdev *sd,
> struct v4l2_subdev_frame_interval *interval)
> {
> @@ -1900,7 +1874,6 @@ static int ov5693_enum_frame_size(struct v4l2_subdev *sd,
> static const struct v4l2_subdev_video_ops ov5693_video_ops = {
> .s_stream = ov5693_s_stream,
> .g_parm = ov5693_g_parm,
> - .s_parm = ov5693_s_parm,
> .g_frame_interval = ov5693_g_frame_interval,
> };
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> index debf0e3853ff..3410a7fb1fcf 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> @@ -2091,7 +2091,7 @@ int atomisp_set_sensor_runmode(struct atomisp_sub_device *asd,
> struct atomisp_device *isp = asd->isp;
> struct v4l2_ctrl *c;
> struct v4l2_streamparm p = {0};
> - int ret;
> + int ret = 0;
> int modes[] = { CI_MODE_NONE,
> CI_MODE_VIDEO,
> CI_MODE_STILL_CAPTURE,
> @@ -2105,13 +2105,8 @@ int atomisp_set_sensor_runmode(struct atomisp_sub_device *asd,
> c = v4l2_ctrl_find(isp->inputs[asd->input_curr].camera->ctrl_handler,
> V4L2_CID_RUN_MODE);
>
> - if (c) {
> + if (c)
> ret = v4l2_ctrl_s_ctrl(c, runmode->mode);
> - } else {
> - p.parm.capture.capturemode = modes[runmode->mode];
> - ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
> - video, s_parm, &p);
> - }
>
> mutex_unlock(asd->ctrl_handler.lock);
> return ret;
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
> index f3e18d627b0a..b78276ac22da 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
> @@ -819,12 +819,6 @@ static int __atomisp_update_run_mode(struct atomisp_sub_device *asd)
> struct atomisp_device *isp = asd->isp;
> struct v4l2_ctrl *ctrl = asd->run_mode;
> struct v4l2_ctrl *c;
> - struct v4l2_streamparm p = {0};
> - int modes[] = { CI_MODE_NONE,
> - CI_MODE_VIDEO,
> - CI_MODE_STILL_CAPTURE,
> - CI_MODE_CONTINUOUS,
> - CI_MODE_PREVIEW };
> s32 mode;
>
> if (ctrl->val != ATOMISP_RUN_MODE_VIDEO &&
> @@ -840,11 +834,7 @@ static int __atomisp_update_run_mode(struct atomisp_sub_device *asd)
> if (c)
> return v4l2_ctrl_s_ctrl(c, mode);
>
> - /* Fall back to obsolete s_parm */
> - p.parm.capture.capturemode = modes[mode];
> -
> - return v4l2_subdev_call(
> - isp->inputs[asd->input_curr].camera, video, s_parm, &p);
> + return 0;
> }
>
> int atomisp_update_run_mode(struct atomisp_sub_device *asd)
Thanks,
Mauro
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCHv2 3/9] staging: atomisp: Kill subdev s_parm abuse
2018-02-14 16:14 ` Mauro Carvalho Chehab
@ 2018-02-16 9:04 ` Sakari Ailus
2018-02-16 11:58 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 35+ messages in thread
From: Sakari Ailus @ 2018-02-16 9:04 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Hans Verkuil, Sakari Ailus, Jacopo Mondi, linux-media, Hans Verkuil
On Wed, Feb 14, 2018 at 02:14:30PM -0200, Mauro Carvalho Chehab wrote:
> Sakari,
>
> Em Mon, 22 Jan 2018 13:31:19 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>
> > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> >
> > Remove sensor driver's interface that made use of use case specific
> > knowledge of platform capabilities.
>
> Could you better describe it? What s_param abuse?
> What happens after this patch? It seems that atomISP relies on
I'd like to remind you this is a staging driver that got where it is
without any review. If you insist on improving the commit message, then
this is what I propose:
Remove sensor driver's interface for setting the use case specific mode
list as well as the mode lists that are related to other than
CI_MODE_PREVIEW. This removes s_parm abuse in using driver specific values
in v4l2_streamparm.capture.capturemode. The drivers already support
[gs]_frame_interval so removing support for [gs]_parm is enough.
> gc0310_res. So, I would be expecting that a patch removing
> s_param would be also adding/changing other parts of the code
> accordingly, in order to get rid of that as a hole (or initialize
> it somewhere else).
>
> Regards,
> Mauro
>
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> > drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 26 ---------
> > drivers/staging/media/atomisp/i2c/atomisp-gc2235.c | 26 ---------
> > drivers/staging/media/atomisp/i2c/atomisp-ov2680.c | 29 ---------
> > drivers/staging/media/atomisp/i2c/atomisp-ov2722.c | 26 ---------
> > drivers/staging/media/atomisp/i2c/gc0310.h | 43 --------------
> > drivers/staging/media/atomisp/i2c/gc2235.h | 1 -
> > drivers/staging/media/atomisp/i2c/ov2680.h | 68 ----------------------
> > .../media/atomisp/i2c/ov5693/atomisp-ov5693.c | 27 ---------
> > .../media/atomisp/pci/atomisp2/atomisp_cmd.c | 9 +--
> > .../media/atomisp/pci/atomisp2/atomisp_subdev.c | 12 +---
> > 10 files changed, 3 insertions(+), 264 deletions(-)
> >
> > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> > index 61b7598469eb..572c9127c24d 100644
> > --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> > +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> > @@ -1224,37 +1224,12 @@ static int gc0310_g_parm(struct v4l2_subdev *sd,
> > if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
> > param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> > param->parm.capture.timeperframe.numerator = 1;
> > - param->parm.capture.capturemode = dev->run_mode;
> > param->parm.capture.timeperframe.denominator =
> > gc0310_res[dev->fmt_idx].fps;
> > }
> > return 0;
> > }
> >
> > -static int gc0310_s_parm(struct v4l2_subdev *sd,
> > - struct v4l2_streamparm *param)
> > -{
> > - struct gc0310_device *dev = to_gc0310_sensor(sd);
> > - dev->run_mode = param->parm.capture.capturemode;
> > -
> > - mutex_lock(&dev->input_lock);
> > - switch (dev->run_mode) {
> > - case CI_MODE_VIDEO:
> > - gc0310_res = gc0310_res_video;
> > - N_RES = N_RES_VIDEO;
> > - break;
> > - case CI_MODE_STILL_CAPTURE:
> > - gc0310_res = gc0310_res_still;
> > - N_RES = N_RES_STILL;
> > - break;
> > - default:
> > - gc0310_res = gc0310_res_preview;
> > - N_RES = N_RES_PREVIEW;
> > - }
> > - mutex_unlock(&dev->input_lock);
> > - return 0;
> > -}
> > -
> > static int gc0310_g_frame_interval(struct v4l2_subdev *sd,
> > struct v4l2_subdev_frame_interval *interval)
> > {
> > @@ -1314,7 +1289,6 @@ static const struct v4l2_subdev_sensor_ops gc0310_sensor_ops = {
> > static const struct v4l2_subdev_video_ops gc0310_video_ops = {
> > .s_stream = gc0310_s_stream,
> > .g_parm = gc0310_g_parm,
> > - .s_parm = gc0310_s_parm,
> > .g_frame_interval = gc0310_g_frame_interval,
> > };
> >
> > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
> > index d8de46da64ae..2bc179f3afe5 100644
> > --- a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
> > +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
> > @@ -964,37 +964,12 @@ static int gc2235_g_parm(struct v4l2_subdev *sd,
> > if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
> > param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> > param->parm.capture.timeperframe.numerator = 1;
> > - param->parm.capture.capturemode = dev->run_mode;
> > param->parm.capture.timeperframe.denominator =
> > gc2235_res[dev->fmt_idx].fps;
> > }
> > return 0;
> > }
> >
> > -static int gc2235_s_parm(struct v4l2_subdev *sd,
> > - struct v4l2_streamparm *param)
> > -{
> > - struct gc2235_device *dev = to_gc2235_sensor(sd);
> > - dev->run_mode = param->parm.capture.capturemode;
> > -
> > - mutex_lock(&dev->input_lock);
> > - switch (dev->run_mode) {
> > - case CI_MODE_VIDEO:
> > - gc2235_res = gc2235_res_video;
> > - N_RES = N_RES_VIDEO;
> > - break;
> > - case CI_MODE_STILL_CAPTURE:
> > - gc2235_res = gc2235_res_still;
> > - N_RES = N_RES_STILL;
> > - break;
> > - default:
> > - gc2235_res = gc2235_res_preview;
> > - N_RES = N_RES_PREVIEW;
> > - }
> > - mutex_unlock(&dev->input_lock);
> > - return 0;
> > -}
> > -
> > static int gc2235_g_frame_interval(struct v4l2_subdev *sd,
> > struct v4l2_subdev_frame_interval *interval)
> > {
> > @@ -1053,7 +1028,6 @@ static const struct v4l2_subdev_sensor_ops gc2235_sensor_ops = {
> > static const struct v4l2_subdev_video_ops gc2235_video_ops = {
> > .s_stream = gc2235_s_stream,
> > .g_parm = gc2235_g_parm,
> > - .s_parm = gc2235_s_parm,
> > .g_frame_interval = gc2235_g_frame_interval,
> > };
> >
> > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> > index 84f8d33ce2d1..e3e0fdd0c816 100644
> > --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> > +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> > @@ -1300,40 +1300,12 @@ static int ov2680_g_parm(struct v4l2_subdev *sd,
> > if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
> > param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> > param->parm.capture.timeperframe.numerator = 1;
> > - param->parm.capture.capturemode = dev->run_mode;
> > param->parm.capture.timeperframe.denominator =
> > ov2680_res[dev->fmt_idx].fps;
> > }
> > return 0;
> > }
> >
> > -static int ov2680_s_parm(struct v4l2_subdev *sd,
> > - struct v4l2_streamparm *param)
> > -{
> > - struct ov2680_device *dev = to_ov2680_sensor(sd);
> > - struct i2c_client *client = v4l2_get_subdevdata(sd);
> > - dev->run_mode = param->parm.capture.capturemode;
> > -
> > - v4l2_info(client, "\n%s:run_mode :%x\n", __func__, dev->run_mode);
> > -
> > - mutex_lock(&dev->input_lock);
> > - switch (dev->run_mode) {
> > - case CI_MODE_VIDEO:
> > - ov2680_res = ov2680_res_video;
> > - N_RES = N_RES_VIDEO;
> > - break;
> > - case CI_MODE_STILL_CAPTURE:
> > - ov2680_res = ov2680_res_still;
> > - N_RES = N_RES_STILL;
> > - break;
> > - default:
> > - ov2680_res = ov2680_res_preview;
> > - N_RES = N_RES_PREVIEW;
> > - }
> > - mutex_unlock(&dev->input_lock);
> > - return 0;
> > -}
> > -
> > static int ov2680_g_frame_interval(struct v4l2_subdev *sd,
> > struct v4l2_subdev_frame_interval *interval)
> > {
> > @@ -1388,7 +1360,6 @@ static int ov2680_g_skip_frames(struct v4l2_subdev *sd, u32 *frames)
> > static const struct v4l2_subdev_video_ops ov2680_video_ops = {
> > .s_stream = ov2680_s_stream,
> > .g_parm = ov2680_g_parm,
> > - .s_parm = ov2680_s_parm,
> > .g_frame_interval = ov2680_g_frame_interval,
> > };
> >
> > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> > index 2b6ae0faf972..cd9f6433cd42 100644
> > --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> > +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> > @@ -1103,37 +1103,12 @@ static int ov2722_g_parm(struct v4l2_subdev *sd,
> > if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
> > param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> > param->parm.capture.timeperframe.numerator = 1;
> > - param->parm.capture.capturemode = dev->run_mode;
> > param->parm.capture.timeperframe.denominator =
> > ov2722_res[dev->fmt_idx].fps;
> > }
> > return 0;
> > }
> >
> > -static int ov2722_s_parm(struct v4l2_subdev *sd,
> > - struct v4l2_streamparm *param)
> > -{
> > - struct ov2722_device *dev = to_ov2722_sensor(sd);
> > - dev->run_mode = param->parm.capture.capturemode;
> > -
> > - mutex_lock(&dev->input_lock);
> > - switch (dev->run_mode) {
> > - case CI_MODE_VIDEO:
> > - ov2722_res = ov2722_res_video;
> > - N_RES = N_RES_VIDEO;
> > - break;
> > - case CI_MODE_STILL_CAPTURE:
> > - ov2722_res = ov2722_res_still;
> > - N_RES = N_RES_STILL;
> > - break;
> > - default:
> > - ov2722_res = ov2722_res_preview;
> > - N_RES = N_RES_PREVIEW;
> > - }
> > - mutex_unlock(&dev->input_lock);
> > - return 0;
> > -}
> > -
> > static int ov2722_g_frame_interval(struct v4l2_subdev *sd,
> > struct v4l2_subdev_frame_interval *interval)
> > {
> > @@ -1193,7 +1168,6 @@ static const struct v4l2_subdev_sensor_ops ov2722_sensor_ops = {
> > static const struct v4l2_subdev_video_ops ov2722_video_ops = {
> > .s_stream = ov2722_s_stream,
> > .g_parm = ov2722_g_parm,
> > - .s_parm = ov2722_s_parm,
> > .g_frame_interval = ov2722_g_frame_interval,
> > };
> >
> > diff --git a/drivers/staging/media/atomisp/i2c/gc0310.h b/drivers/staging/media/atomisp/i2c/gc0310.h
> > index c422d0398fc7..af6b11f6e5e7 100644
> > --- a/drivers/staging/media/atomisp/i2c/gc0310.h
> > +++ b/drivers/staging/media/atomisp/i2c/gc0310.h
> > @@ -150,7 +150,6 @@ struct gc0310_device {
> > struct camera_sensor_platform_data *platform_data;
> > int vt_pix_clk_freq_mhz;
> > int fmt_idx;
> > - int run_mode;
> > u8 res;
> > u8 type;
> > };
> > @@ -400,48 +399,6 @@ struct gc0310_resolution gc0310_res_preview[] = {
> > };
> > #define N_RES_PREVIEW (ARRAY_SIZE(gc0310_res_preview))
> >
> > -struct gc0310_resolution gc0310_res_still[] = {
> > - {
> > - .desc = "gc0310_VGA_30fps",
> > - .width = 656, // 648,
> > - .height = 496, // 488,
> > - .fps = 30,
> > - //.pix_clk_freq = 73,
> > - .used = 0,
> > -#if 0
> > - .pixels_per_line = 0x0314,
> > - .lines_per_frame = 0x0213,
> > -#endif
> > - .bin_factor_x = 1,
> > - .bin_factor_y = 1,
> > - .bin_mode = 0,
> > - .skip_frames = 2,
> > - .regs = gc0310_VGA_30fps,
> > - },
> > -};
> > -#define N_RES_STILL (ARRAY_SIZE(gc0310_res_still))
> > -
> > -struct gc0310_resolution gc0310_res_video[] = {
> > - {
> > - .desc = "gc0310_VGA_30fps",
> > - .width = 656, // 648,
> > - .height = 496, // 488,
> > - .fps = 30,
> > - //.pix_clk_freq = 73,
> > - .used = 0,
> > -#if 0
> > - .pixels_per_line = 0x0314,
> > - .lines_per_frame = 0x0213,
> > -#endif
> > - .bin_factor_x = 1,
> > - .bin_factor_y = 1,
> > - .bin_mode = 0,
> > - .skip_frames = 2,
> > - .regs = gc0310_VGA_30fps,
> > - },
> > -};
> > -#define N_RES_VIDEO (ARRAY_SIZE(gc0310_res_video))
> > -
> > static struct gc0310_resolution *gc0310_res = gc0310_res_preview;
> > static unsigned long N_RES = N_RES_PREVIEW;
> > #endif
> > diff --git a/drivers/staging/media/atomisp/i2c/gc2235.h b/drivers/staging/media/atomisp/i2c/gc2235.h
> > index 3c30a05c3991..45a54fea5466 100644
> > --- a/drivers/staging/media/atomisp/i2c/gc2235.h
> > +++ b/drivers/staging/media/atomisp/i2c/gc2235.h
> > @@ -156,7 +156,6 @@ struct gc2235_device {
> > struct camera_sensor_platform_data *platform_data;
> > int vt_pix_clk_freq_mhz;
> > int fmt_idx;
> > - int run_mode;
> > u8 res;
> > u8 type;
> > };
> > diff --git a/drivers/staging/media/atomisp/i2c/ov2680.h b/drivers/staging/media/atomisp/i2c/ov2680.h
> > index 03f75dd80f87..cb38e6e79409 100644
> > --- a/drivers/staging/media/atomisp/i2c/ov2680.h
> > +++ b/drivers/staging/media/atomisp/i2c/ov2680.h
> > @@ -850,74 +850,6 @@ struct ov2680_format {
> > };
> > #define N_RES_PREVIEW (ARRAY_SIZE(ov2680_res_preview))
> >
> > -static struct ov2680_resolution ov2680_res_still[] = {
> > - {
> > - .desc = "ov2680_1616x1216_30fps",
> > - .width = 1616,
> > - .height = 1216,
> > - .pix_clk_freq = 66,
> > - .fps = 30,
> > - .used = 0,
> > - .pixels_per_line = 1698,//1704,
> > - .lines_per_frame = 1294,
> > - .bin_factor_x = 0,
> > - .bin_factor_y = 0,
> > - .bin_mode = 0,
> > - .skip_frames = 3,
> > - .regs = ov2680_1616x1216_30fps,
> > - },
> > - {
> > - .desc = "ov2680_1616x916_30fps",
> > - .width = 1616,
> > - .height = 916,
> > - .fps = 30,
> > - .pix_clk_freq = 66,
> > - .used = 0,
> > - .pixels_per_line = 1698,//1704,
> > - .lines_per_frame = 1294,
> > - .bin_factor_x = 0,
> > - .bin_factor_y = 0,
> > - .bin_mode = 0,
> > - .skip_frames = 3,
> > - .regs = ov2680_1616x916_30fps,
> > - },
> > -};
> > -#define N_RES_STILL (ARRAY_SIZE(ov2680_res_still))
> > -
> > -static struct ov2680_resolution ov2680_res_video[] = {
> > - {
> > - .desc = "ov2680_1616x1216_30fps",
> > - .width = 1616,
> > - .height = 1216,
> > - .pix_clk_freq = 66,
> > - .fps = 30,
> > - .used = 0,
> > - .pixels_per_line = 1698,//1704,
> > - .lines_per_frame = 1294,
> > - .bin_factor_x = 0,
> > - .bin_factor_y = 0,
> > - .bin_mode = 0,
> > - .skip_frames = 3,
> > - .regs = ov2680_1616x1216_30fps,
> > - },
> > - {
> > - .desc = "ov2680_720p_30fps",
> > - .width = 1616,
> > - .height = 916,
> > - .fps = 30,
> > - .pix_clk_freq = 66,
> > - .used = 0,
> > - .pixels_per_line = 1698,//1704,
> > - .lines_per_frame = 1294,
> > - .bin_factor_x = 0,
> > - .bin_factor_y = 0,
> > - .bin_mode = 0,
> > - .skip_frames = 3,
> > - .regs = ov2680_1616x916_30fps,
> > - },
> > -};
> > -#define N_RES_VIDEO (ARRAY_SIZE(ov2680_res_video))
> > -
> > static struct ov2680_resolution *ov2680_res = ov2680_res_preview;
> > static unsigned long N_RES = N_RES_PREVIEW;
> >
> > diff --git a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
> > index 40d01bf4bf28..7f594c7de76e 100644
> > --- a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
> > +++ b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
> > @@ -1825,38 +1825,12 @@ static int ov5693_g_parm(struct v4l2_subdev *sd,
> > if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
> > param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> > param->parm.capture.timeperframe.numerator = 1;
> > - param->parm.capture.capturemode = dev->run_mode;
> > param->parm.capture.timeperframe.denominator =
> > ov5693_res[dev->fmt_idx].fps;
> > }
> > return 0;
> > }
> >
> > -static int ov5693_s_parm(struct v4l2_subdev *sd,
> > - struct v4l2_streamparm *param)
> > -{
> > - struct ov5693_device *dev = to_ov5693_sensor(sd);
> > -
> > - dev->run_mode = param->parm.capture.capturemode;
> > -
> > - mutex_lock(&dev->input_lock);
> > - switch (dev->run_mode) {
> > - case CI_MODE_VIDEO:
> > - ov5693_res = ov5693_res_video;
> > - N_RES = N_RES_VIDEO;
> > - break;
> > - case CI_MODE_STILL_CAPTURE:
> > - ov5693_res = ov5693_res_still;
> > - N_RES = N_RES_STILL;
> > - break;
> > - default:
> > - ov5693_res = ov5693_res_preview;
> > - N_RES = N_RES_PREVIEW;
> > - }
> > - mutex_unlock(&dev->input_lock);
> > - return 0;
> > -}
> > -
> > static int ov5693_g_frame_interval(struct v4l2_subdev *sd,
> > struct v4l2_subdev_frame_interval *interval)
> > {
> > @@ -1900,7 +1874,6 @@ static int ov5693_enum_frame_size(struct v4l2_subdev *sd,
> > static const struct v4l2_subdev_video_ops ov5693_video_ops = {
> > .s_stream = ov5693_s_stream,
> > .g_parm = ov5693_g_parm,
> > - .s_parm = ov5693_s_parm,
> > .g_frame_interval = ov5693_g_frame_interval,
> > };
> >
> > diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> > index debf0e3853ff..3410a7fb1fcf 100644
> > --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> > +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> > @@ -2091,7 +2091,7 @@ int atomisp_set_sensor_runmode(struct atomisp_sub_device *asd,
> > struct atomisp_device *isp = asd->isp;
> > struct v4l2_ctrl *c;
> > struct v4l2_streamparm p = {0};
> > - int ret;
> > + int ret = 0;
> > int modes[] = { CI_MODE_NONE,
> > CI_MODE_VIDEO,
> > CI_MODE_STILL_CAPTURE,
> > @@ -2105,13 +2105,8 @@ int atomisp_set_sensor_runmode(struct atomisp_sub_device *asd,
> > c = v4l2_ctrl_find(isp->inputs[asd->input_curr].camera->ctrl_handler,
> > V4L2_CID_RUN_MODE);
> >
> > - if (c) {
> > + if (c)
> > ret = v4l2_ctrl_s_ctrl(c, runmode->mode);
> > - } else {
> > - p.parm.capture.capturemode = modes[runmode->mode];
> > - ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
> > - video, s_parm, &p);
> > - }
> >
> > mutex_unlock(asd->ctrl_handler.lock);
> > return ret;
> > diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
> > index f3e18d627b0a..b78276ac22da 100644
> > --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
> > +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
> > @@ -819,12 +819,6 @@ static int __atomisp_update_run_mode(struct atomisp_sub_device *asd)
> > struct atomisp_device *isp = asd->isp;
> > struct v4l2_ctrl *ctrl = asd->run_mode;
> > struct v4l2_ctrl *c;
> > - struct v4l2_streamparm p = {0};
> > - int modes[] = { CI_MODE_NONE,
> > - CI_MODE_VIDEO,
> > - CI_MODE_STILL_CAPTURE,
> > - CI_MODE_CONTINUOUS,
> > - CI_MODE_PREVIEW };
> > s32 mode;
> >
> > if (ctrl->val != ATOMISP_RUN_MODE_VIDEO &&
> > @@ -840,11 +834,7 @@ static int __atomisp_update_run_mode(struct atomisp_sub_device *asd)
> > if (c)
> > return v4l2_ctrl_s_ctrl(c, mode);
> >
> > - /* Fall back to obsolete s_parm */
> > - p.parm.capture.capturemode = modes[mode];
> > -
> > - return v4l2_subdev_call(
> > - isp->inputs[asd->input_curr].camera, video, s_parm, &p);
> > + return 0;
> > }
> >
> > int atomisp_update_run_mode(struct atomisp_sub_device *asd)
>
>
>
> Thanks,
> Mauro
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCHv2 3/9] staging: atomisp: Kill subdev s_parm abuse
2018-02-16 9:04 ` Sakari Ailus
@ 2018-02-16 11:58 ` Mauro Carvalho Chehab
2018-02-19 11:01 ` Hans Verkuil
0 siblings, 1 reply; 35+ messages in thread
From: Mauro Carvalho Chehab @ 2018-02-16 11:58 UTC (permalink / raw)
To: Sakari Ailus
Cc: Hans Verkuil, Sakari Ailus, Jacopo Mondi, linux-media, Hans Verkuil
Em Fri, 16 Feb 2018 11:04:36 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> On Wed, Feb 14, 2018 at 02:14:30PM -0200, Mauro Carvalho Chehab wrote:
> > Sakari,
> >
> > Em Mon, 22 Jan 2018 13:31:19 +0100
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >
> > > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > >
> > > Remove sensor driver's interface that made use of use case specific
> > > knowledge of platform capabilities.
> >
> > Could you better describe it? What s_param abuse?
> > What happens after this patch? It seems that atomISP relies on
>
> I'd like to remind you this is a staging driver that got where it is
> without any review.
Ok, but this is not an excuse to not properly document any further
patches to it.
> If you insist on improving the commit message, then
> this is what I propose:
>
> Remove sensor driver's interface for setting the use case specific mode
> list as well as the mode lists that are related to other than
> CI_MODE_PREVIEW. This removes s_parm abuse in using driver specific values
> in v4l2_streamparm.capture.capturemode. The drivers already support
> [gs]_frame_interval so removing support for [gs]_parm is enough.
Works for me.
>
> > gc0310_res. So, I would be expecting that a patch removing
> > s_param would be also adding/changing other parts of the code
> > accordingly, in order to get rid of that as a hole (or initialize
> > it somewhere else).
> >
> > Regards,
> > Mauro
> >
> > >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > ---
> > > drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 26 ---------
> > > drivers/staging/media/atomisp/i2c/atomisp-gc2235.c | 26 ---------
> > > drivers/staging/media/atomisp/i2c/atomisp-ov2680.c | 29 ---------
> > > drivers/staging/media/atomisp/i2c/atomisp-ov2722.c | 26 ---------
> > > drivers/staging/media/atomisp/i2c/gc0310.h | 43 --------------
> > > drivers/staging/media/atomisp/i2c/gc2235.h | 1 -
> > > drivers/staging/media/atomisp/i2c/ov2680.h | 68 ----------------------
> > > .../media/atomisp/i2c/ov5693/atomisp-ov5693.c | 27 ---------
> > > .../media/atomisp/pci/atomisp2/atomisp_cmd.c | 9 +--
> > > .../media/atomisp/pci/atomisp2/atomisp_subdev.c | 12 +---
> > > 10 files changed, 3 insertions(+), 264 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> > > index 61b7598469eb..572c9127c24d 100644
> > > --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> > > +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> > > @@ -1224,37 +1224,12 @@ static int gc0310_g_parm(struct v4l2_subdev *sd,
> > > if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
> > > param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> > > param->parm.capture.timeperframe.numerator = 1;
> > > - param->parm.capture.capturemode = dev->run_mode;
> > > param->parm.capture.timeperframe.denominator =
> > > gc0310_res[dev->fmt_idx].fps;
> > > }
> > > return 0;
> > > }
> > >
> > > -static int gc0310_s_parm(struct v4l2_subdev *sd,
> > > - struct v4l2_streamparm *param)
> > > -{
> > > - struct gc0310_device *dev = to_gc0310_sensor(sd);
> > > - dev->run_mode = param->parm.capture.capturemode;
> > > -
> > > - mutex_lock(&dev->input_lock);
> > > - switch (dev->run_mode) {
> > > - case CI_MODE_VIDEO:
> > > - gc0310_res = gc0310_res_video;
> > > - N_RES = N_RES_VIDEO;
> > > - break;
> > > - case CI_MODE_STILL_CAPTURE:
> > > - gc0310_res = gc0310_res_still;
> > > - N_RES = N_RES_STILL;
> > > - break;
> > > - default:
> > > - gc0310_res = gc0310_res_preview;
> > > - N_RES = N_RES_PREVIEW;
> > > - }
> > > - mutex_unlock(&dev->input_lock);
> > > - return 0;
> > > -}
> > > -
> > > static int gc0310_g_frame_interval(struct v4l2_subdev *sd,
> > > struct v4l2_subdev_frame_interval *interval)
> > > {
> > > @@ -1314,7 +1289,6 @@ static const struct v4l2_subdev_sensor_ops gc0310_sensor_ops = {
> > > static const struct v4l2_subdev_video_ops gc0310_video_ops = {
> > > .s_stream = gc0310_s_stream,
> > > .g_parm = gc0310_g_parm,
> > > - .s_parm = gc0310_s_parm,
> > > .g_frame_interval = gc0310_g_frame_interval,
> > > };
> > >
> > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
> > > index d8de46da64ae..2bc179f3afe5 100644
> > > --- a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
> > > +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
> > > @@ -964,37 +964,12 @@ static int gc2235_g_parm(struct v4l2_subdev *sd,
> > > if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
> > > param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> > > param->parm.capture.timeperframe.numerator = 1;
> > > - param->parm.capture.capturemode = dev->run_mode;
> > > param->parm.capture.timeperframe.denominator =
> > > gc2235_res[dev->fmt_idx].fps;
> > > }
> > > return 0;
> > > }
> > >
> > > -static int gc2235_s_parm(struct v4l2_subdev *sd,
> > > - struct v4l2_streamparm *param)
> > > -{
> > > - struct gc2235_device *dev = to_gc2235_sensor(sd);
> > > - dev->run_mode = param->parm.capture.capturemode;
> > > -
> > > - mutex_lock(&dev->input_lock);
> > > - switch (dev->run_mode) {
> > > - case CI_MODE_VIDEO:
> > > - gc2235_res = gc2235_res_video;
> > > - N_RES = N_RES_VIDEO;
> > > - break;
> > > - case CI_MODE_STILL_CAPTURE:
> > > - gc2235_res = gc2235_res_still;
> > > - N_RES = N_RES_STILL;
> > > - break;
> > > - default:
> > > - gc2235_res = gc2235_res_preview;
> > > - N_RES = N_RES_PREVIEW;
> > > - }
> > > - mutex_unlock(&dev->input_lock);
> > > - return 0;
> > > -}
> > > -
> > > static int gc2235_g_frame_interval(struct v4l2_subdev *sd,
> > > struct v4l2_subdev_frame_interval *interval)
> > > {
> > > @@ -1053,7 +1028,6 @@ static const struct v4l2_subdev_sensor_ops gc2235_sensor_ops = {
> > > static const struct v4l2_subdev_video_ops gc2235_video_ops = {
> > > .s_stream = gc2235_s_stream,
> > > .g_parm = gc2235_g_parm,
> > > - .s_parm = gc2235_s_parm,
> > > .g_frame_interval = gc2235_g_frame_interval,
> > > };
> > >
> > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> > > index 84f8d33ce2d1..e3e0fdd0c816 100644
> > > --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> > > +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> > > @@ -1300,40 +1300,12 @@ static int ov2680_g_parm(struct v4l2_subdev *sd,
> > > if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
> > > param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> > > param->parm.capture.timeperframe.numerator = 1;
> > > - param->parm.capture.capturemode = dev->run_mode;
> > > param->parm.capture.timeperframe.denominator =
> > > ov2680_res[dev->fmt_idx].fps;
> > > }
> > > return 0;
> > > }
> > >
> > > -static int ov2680_s_parm(struct v4l2_subdev *sd,
> > > - struct v4l2_streamparm *param)
> > > -{
> > > - struct ov2680_device *dev = to_ov2680_sensor(sd);
> > > - struct i2c_client *client = v4l2_get_subdevdata(sd);
> > > - dev->run_mode = param->parm.capture.capturemode;
> > > -
> > > - v4l2_info(client, "\n%s:run_mode :%x\n", __func__, dev->run_mode);
> > > -
> > > - mutex_lock(&dev->input_lock);
> > > - switch (dev->run_mode) {
> > > - case CI_MODE_VIDEO:
> > > - ov2680_res = ov2680_res_video;
> > > - N_RES = N_RES_VIDEO;
> > > - break;
> > > - case CI_MODE_STILL_CAPTURE:
> > > - ov2680_res = ov2680_res_still;
> > > - N_RES = N_RES_STILL;
> > > - break;
> > > - default:
> > > - ov2680_res = ov2680_res_preview;
> > > - N_RES = N_RES_PREVIEW;
> > > - }
> > > - mutex_unlock(&dev->input_lock);
> > > - return 0;
> > > -}
> > > -
> > > static int ov2680_g_frame_interval(struct v4l2_subdev *sd,
> > > struct v4l2_subdev_frame_interval *interval)
> > > {
> > > @@ -1388,7 +1360,6 @@ static int ov2680_g_skip_frames(struct v4l2_subdev *sd, u32 *frames)
> > > static const struct v4l2_subdev_video_ops ov2680_video_ops = {
> > > .s_stream = ov2680_s_stream,
> > > .g_parm = ov2680_g_parm,
> > > - .s_parm = ov2680_s_parm,
> > > .g_frame_interval = ov2680_g_frame_interval,
> > > };
> > >
> > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> > > index 2b6ae0faf972..cd9f6433cd42 100644
> > > --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> > > +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> > > @@ -1103,37 +1103,12 @@ static int ov2722_g_parm(struct v4l2_subdev *sd,
> > > if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
> > > param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> > > param->parm.capture.timeperframe.numerator = 1;
> > > - param->parm.capture.capturemode = dev->run_mode;
> > > param->parm.capture.timeperframe.denominator =
> > > ov2722_res[dev->fmt_idx].fps;
> > > }
> > > return 0;
> > > }
> > >
> > > -static int ov2722_s_parm(struct v4l2_subdev *sd,
> > > - struct v4l2_streamparm *param)
> > > -{
> > > - struct ov2722_device *dev = to_ov2722_sensor(sd);
> > > - dev->run_mode = param->parm.capture.capturemode;
> > > -
> > > - mutex_lock(&dev->input_lock);
> > > - switch (dev->run_mode) {
> > > - case CI_MODE_VIDEO:
> > > - ov2722_res = ov2722_res_video;
> > > - N_RES = N_RES_VIDEO;
> > > - break;
> > > - case CI_MODE_STILL_CAPTURE:
> > > - ov2722_res = ov2722_res_still;
> > > - N_RES = N_RES_STILL;
> > > - break;
> > > - default:
> > > - ov2722_res = ov2722_res_preview;
> > > - N_RES = N_RES_PREVIEW;
> > > - }
> > > - mutex_unlock(&dev->input_lock);
> > > - return 0;
> > > -}
> > > -
> > > static int ov2722_g_frame_interval(struct v4l2_subdev *sd,
> > > struct v4l2_subdev_frame_interval *interval)
> > > {
> > > @@ -1193,7 +1168,6 @@ static const struct v4l2_subdev_sensor_ops ov2722_sensor_ops = {
> > > static const struct v4l2_subdev_video_ops ov2722_video_ops = {
> > > .s_stream = ov2722_s_stream,
> > > .g_parm = ov2722_g_parm,
> > > - .s_parm = ov2722_s_parm,
> > > .g_frame_interval = ov2722_g_frame_interval,
> > > };
> > >
> > > diff --git a/drivers/staging/media/atomisp/i2c/gc0310.h b/drivers/staging/media/atomisp/i2c/gc0310.h
> > > index c422d0398fc7..af6b11f6e5e7 100644
> > > --- a/drivers/staging/media/atomisp/i2c/gc0310.h
> > > +++ b/drivers/staging/media/atomisp/i2c/gc0310.h
> > > @@ -150,7 +150,6 @@ struct gc0310_device {
> > > struct camera_sensor_platform_data *platform_data;
> > > int vt_pix_clk_freq_mhz;
> > > int fmt_idx;
> > > - int run_mode;
> > > u8 res;
> > > u8 type;
> > > };
> > > @@ -400,48 +399,6 @@ struct gc0310_resolution gc0310_res_preview[] = {
> > > };
> > > #define N_RES_PREVIEW (ARRAY_SIZE(gc0310_res_preview))
> > >
> > > -struct gc0310_resolution gc0310_res_still[] = {
> > > - {
> > > - .desc = "gc0310_VGA_30fps",
> > > - .width = 656, // 648,
> > > - .height = 496, // 488,
> > > - .fps = 30,
> > > - //.pix_clk_freq = 73,
> > > - .used = 0,
> > > -#if 0
> > > - .pixels_per_line = 0x0314,
> > > - .lines_per_frame = 0x0213,
> > > -#endif
> > > - .bin_factor_x = 1,
> > > - .bin_factor_y = 1,
> > > - .bin_mode = 0,
> > > - .skip_frames = 2,
> > > - .regs = gc0310_VGA_30fps,
> > > - },
> > > -};
> > > -#define N_RES_STILL (ARRAY_SIZE(gc0310_res_still))
> > > -
> > > -struct gc0310_resolution gc0310_res_video[] = {
> > > - {
> > > - .desc = "gc0310_VGA_30fps",
> > > - .width = 656, // 648,
> > > - .height = 496, // 488,
> > > - .fps = 30,
> > > - //.pix_clk_freq = 73,
> > > - .used = 0,
> > > -#if 0
> > > - .pixels_per_line = 0x0314,
> > > - .lines_per_frame = 0x0213,
> > > -#endif
> > > - .bin_factor_x = 1,
> > > - .bin_factor_y = 1,
> > > - .bin_mode = 0,
> > > - .skip_frames = 2,
> > > - .regs = gc0310_VGA_30fps,
> > > - },
> > > -};
> > > -#define N_RES_VIDEO (ARRAY_SIZE(gc0310_res_video))
> > > -
> > > static struct gc0310_resolution *gc0310_res = gc0310_res_preview;
> > > static unsigned long N_RES = N_RES_PREVIEW;
> > > #endif
> > > diff --git a/drivers/staging/media/atomisp/i2c/gc2235.h b/drivers/staging/media/atomisp/i2c/gc2235.h
> > > index 3c30a05c3991..45a54fea5466 100644
> > > --- a/drivers/staging/media/atomisp/i2c/gc2235.h
> > > +++ b/drivers/staging/media/atomisp/i2c/gc2235.h
> > > @@ -156,7 +156,6 @@ struct gc2235_device {
> > > struct camera_sensor_platform_data *platform_data;
> > > int vt_pix_clk_freq_mhz;
> > > int fmt_idx;
> > > - int run_mode;
> > > u8 res;
> > > u8 type;
> > > };
> > > diff --git a/drivers/staging/media/atomisp/i2c/ov2680.h b/drivers/staging/media/atomisp/i2c/ov2680.h
> > > index 03f75dd80f87..cb38e6e79409 100644
> > > --- a/drivers/staging/media/atomisp/i2c/ov2680.h
> > > +++ b/drivers/staging/media/atomisp/i2c/ov2680.h
> > > @@ -850,74 +850,6 @@ struct ov2680_format {
> > > };
> > > #define N_RES_PREVIEW (ARRAY_SIZE(ov2680_res_preview))
> > >
> > > -static struct ov2680_resolution ov2680_res_still[] = {
> > > - {
> > > - .desc = "ov2680_1616x1216_30fps",
> > > - .width = 1616,
> > > - .height = 1216,
> > > - .pix_clk_freq = 66,
> > > - .fps = 30,
> > > - .used = 0,
> > > - .pixels_per_line = 1698,//1704,
> > > - .lines_per_frame = 1294,
> > > - .bin_factor_x = 0,
> > > - .bin_factor_y = 0,
> > > - .bin_mode = 0,
> > > - .skip_frames = 3,
> > > - .regs = ov2680_1616x1216_30fps,
> > > - },
> > > - {
> > > - .desc = "ov2680_1616x916_30fps",
> > > - .width = 1616,
> > > - .height = 916,
> > > - .fps = 30,
> > > - .pix_clk_freq = 66,
> > > - .used = 0,
> > > - .pixels_per_line = 1698,//1704,
> > > - .lines_per_frame = 1294,
> > > - .bin_factor_x = 0,
> > > - .bin_factor_y = 0,
> > > - .bin_mode = 0,
> > > - .skip_frames = 3,
> > > - .regs = ov2680_1616x916_30fps,
> > > - },
> > > -};
> > > -#define N_RES_STILL (ARRAY_SIZE(ov2680_res_still))
> > > -
> > > -static struct ov2680_resolution ov2680_res_video[] = {
> > > - {
> > > - .desc = "ov2680_1616x1216_30fps",
> > > - .width = 1616,
> > > - .height = 1216,
> > > - .pix_clk_freq = 66,
> > > - .fps = 30,
> > > - .used = 0,
> > > - .pixels_per_line = 1698,//1704,
> > > - .lines_per_frame = 1294,
> > > - .bin_factor_x = 0,
> > > - .bin_factor_y = 0,
> > > - .bin_mode = 0,
> > > - .skip_frames = 3,
> > > - .regs = ov2680_1616x1216_30fps,
> > > - },
> > > - {
> > > - .desc = "ov2680_720p_30fps",
> > > - .width = 1616,
> > > - .height = 916,
> > > - .fps = 30,
> > > - .pix_clk_freq = 66,
> > > - .used = 0,
> > > - .pixels_per_line = 1698,//1704,
> > > - .lines_per_frame = 1294,
> > > - .bin_factor_x = 0,
> > > - .bin_factor_y = 0,
> > > - .bin_mode = 0,
> > > - .skip_frames = 3,
> > > - .regs = ov2680_1616x916_30fps,
> > > - },
> > > -};
> > > -#define N_RES_VIDEO (ARRAY_SIZE(ov2680_res_video))
> > > -
> > > static struct ov2680_resolution *ov2680_res = ov2680_res_preview;
> > > static unsigned long N_RES = N_RES_PREVIEW;
> > >
> > > diff --git a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
> > > index 40d01bf4bf28..7f594c7de76e 100644
> > > --- a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
> > > +++ b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
> > > @@ -1825,38 +1825,12 @@ static int ov5693_g_parm(struct v4l2_subdev *sd,
> > > if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
> > > param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> > > param->parm.capture.timeperframe.numerator = 1;
> > > - param->parm.capture.capturemode = dev->run_mode;
> > > param->parm.capture.timeperframe.denominator =
> > > ov5693_res[dev->fmt_idx].fps;
> > > }
> > > return 0;
> > > }
> > >
> > > -static int ov5693_s_parm(struct v4l2_subdev *sd,
> > > - struct v4l2_streamparm *param)
> > > -{
> > > - struct ov5693_device *dev = to_ov5693_sensor(sd);
> > > -
> > > - dev->run_mode = param->parm.capture.capturemode;
> > > -
> > > - mutex_lock(&dev->input_lock);
> > > - switch (dev->run_mode) {
> > > - case CI_MODE_VIDEO:
> > > - ov5693_res = ov5693_res_video;
> > > - N_RES = N_RES_VIDEO;
> > > - break;
> > > - case CI_MODE_STILL_CAPTURE:
> > > - ov5693_res = ov5693_res_still;
> > > - N_RES = N_RES_STILL;
> > > - break;
> > > - default:
> > > - ov5693_res = ov5693_res_preview;
> > > - N_RES = N_RES_PREVIEW;
> > > - }
> > > - mutex_unlock(&dev->input_lock);
> > > - return 0;
> > > -}
> > > -
> > > static int ov5693_g_frame_interval(struct v4l2_subdev *sd,
> > > struct v4l2_subdev_frame_interval *interval)
> > > {
> > > @@ -1900,7 +1874,6 @@ static int ov5693_enum_frame_size(struct v4l2_subdev *sd,
> > > static const struct v4l2_subdev_video_ops ov5693_video_ops = {
> > > .s_stream = ov5693_s_stream,
> > > .g_parm = ov5693_g_parm,
> > > - .s_parm = ov5693_s_parm,
> > > .g_frame_interval = ov5693_g_frame_interval,
> > > };
> > >
> > > diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> > > index debf0e3853ff..3410a7fb1fcf 100644
> > > --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> > > +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> > > @@ -2091,7 +2091,7 @@ int atomisp_set_sensor_runmode(struct atomisp_sub_device *asd,
> > > struct atomisp_device *isp = asd->isp;
> > > struct v4l2_ctrl *c;
> > > struct v4l2_streamparm p = {0};
> > > - int ret;
> > > + int ret = 0;
> > > int modes[] = { CI_MODE_NONE,
> > > CI_MODE_VIDEO,
> > > CI_MODE_STILL_CAPTURE,
> > > @@ -2105,13 +2105,8 @@ int atomisp_set_sensor_runmode(struct atomisp_sub_device *asd,
> > > c = v4l2_ctrl_find(isp->inputs[asd->input_curr].camera->ctrl_handler,
> > > V4L2_CID_RUN_MODE);
> > >
> > > - if (c) {
> > > + if (c)
> > > ret = v4l2_ctrl_s_ctrl(c, runmode->mode);
> > > - } else {
> > > - p.parm.capture.capturemode = modes[runmode->mode];
> > > - ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
> > > - video, s_parm, &p);
> > > - }
> > >
> > > mutex_unlock(asd->ctrl_handler.lock);
> > > return ret;
> > > diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
> > > index f3e18d627b0a..b78276ac22da 100644
> > > --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
> > > +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
> > > @@ -819,12 +819,6 @@ static int __atomisp_update_run_mode(struct atomisp_sub_device *asd)
> > > struct atomisp_device *isp = asd->isp;
> > > struct v4l2_ctrl *ctrl = asd->run_mode;
> > > struct v4l2_ctrl *c;
> > > - struct v4l2_streamparm p = {0};
> > > - int modes[] = { CI_MODE_NONE,
> > > - CI_MODE_VIDEO,
> > > - CI_MODE_STILL_CAPTURE,
> > > - CI_MODE_CONTINUOUS,
> > > - CI_MODE_PREVIEW };
> > > s32 mode;
> > >
> > > if (ctrl->val != ATOMISP_RUN_MODE_VIDEO &&
> > > @@ -840,11 +834,7 @@ static int __atomisp_update_run_mode(struct atomisp_sub_device *asd)
> > > if (c)
> > > return v4l2_ctrl_s_ctrl(c, mode);
> > >
> > > - /* Fall back to obsolete s_parm */
> > > - p.parm.capture.capturemode = modes[mode];
> > > -
> > > - return v4l2_subdev_call(
> > > - isp->inputs[asd->input_curr].camera, video, s_parm, &p);
> > > + return 0;
> > > }
> > >
> > > int atomisp_update_run_mode(struct atomisp_sub_device *asd)
> >
> >
> >
> > Thanks,
> > Mauro
>
Thanks,
Mauro
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCHv2 3/9] staging: atomisp: Kill subdev s_parm abuse
2018-02-16 11:58 ` Mauro Carvalho Chehab
@ 2018-02-19 11:01 ` Hans Verkuil
0 siblings, 0 replies; 35+ messages in thread
From: Hans Verkuil @ 2018-02-19 11:01 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus
Cc: Sakari Ailus, Jacopo Mondi, linux-media, Hans Verkuil
On 02/16/2018 12:58 PM, Mauro Carvalho Chehab wrote:
> Em Fri, 16 Feb 2018 11:04:36 +0200
> Sakari Ailus <sakari.ailus@iki.fi> escreveu:
>
>> On Wed, Feb 14, 2018 at 02:14:30PM -0200, Mauro Carvalho Chehab wrote:
>>> Sakari,
>>>
>>> Em Mon, 22 Jan 2018 13:31:19 +0100
>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>
>>>> From: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>>
>>>> Remove sensor driver's interface that made use of use case specific
>>>> knowledge of platform capabilities.
>>>
>>> Could you better describe it? What s_param abuse?
>>> What happens after this patch? It seems that atomISP relies on
>>
>> I'd like to remind you this is a staging driver that got where it is
>> without any review.
>
> Ok, but this is not an excuse to not properly document any further
> patches to it.
>
>> If you insist on improving the commit message, then
>> this is what I propose:
>>
>> Remove sensor driver's interface for setting the use case specific mode
>> list as well as the mode lists that are related to other than
>> CI_MODE_PREVIEW. This removes s_parm abuse in using driver specific values
>> in v4l2_streamparm.capture.capturemode. The drivers already support
>> [gs]_frame_interval so removing support for [gs]_parm is enough.
>
> Works for me.
I'll update the commit message for this patch.
Regards,
Hans
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCHv2 4/9] staging: atomisp: i2c: Disable non-preview configurations
2018-01-22 12:31 [PATCHv2 0/9] media: replace g/s_parm by g/s_frame_interval Hans Verkuil
` (2 preceding siblings ...)
2018-01-22 12:31 ` [PATCHv2 3/9] staging: atomisp: Kill subdev s_parm abuse Hans Verkuil
@ 2018-01-22 12:31 ` Hans Verkuil
2018-02-14 16:20 ` Mauro Carvalho Chehab
2018-02-19 13:27 ` [PATCH v2.1 " Sakari Ailus
2018-01-22 12:31 ` [PATCHv2 5/9] staging: atomisp: i2c: Drop g_parm support in sensor drivers Hans Verkuil
` (5 subsequent siblings)
9 siblings, 2 replies; 35+ messages in thread
From: Hans Verkuil @ 2018-01-22 12:31 UTC (permalink / raw)
To: linux-media; +Cc: Sakari Ailus, Jacopo Mondi, Hans Verkuil
From: Sakari Ailus <sakari.ailus@linux.intel.com>
Disable configurations for non-preview modes until configuration selection
is improved.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/staging/media/atomisp/i2c/gc2235.h | 2 ++
drivers/staging/media/atomisp/i2c/ov2722.h | 2 ++
drivers/staging/media/atomisp/i2c/ov5693/ov5693.h | 2 ++
3 files changed, 6 insertions(+)
diff --git a/drivers/staging/media/atomisp/i2c/gc2235.h b/drivers/staging/media/atomisp/i2c/gc2235.h
index 45a54fea5466..817c0068c1d3 100644
--- a/drivers/staging/media/atomisp/i2c/gc2235.h
+++ b/drivers/staging/media/atomisp/i2c/gc2235.h
@@ -574,6 +574,7 @@ static struct gc2235_resolution gc2235_res_preview[] = {
};
#define N_RES_PREVIEW (ARRAY_SIZE(gc2235_res_preview))
+#if 0 /* Disable non-previes configurations for now */
static struct gc2235_resolution gc2235_res_still[] = {
{
.desc = "gc2235_1600_900_30fps",
@@ -658,6 +659,7 @@ static struct gc2235_resolution gc2235_res_video[] = {
};
#define N_RES_VIDEO (ARRAY_SIZE(gc2235_res_video))
+#endif
static struct gc2235_resolution *gc2235_res = gc2235_res_preview;
static unsigned long N_RES = N_RES_PREVIEW;
diff --git a/drivers/staging/media/atomisp/i2c/ov2722.h b/drivers/staging/media/atomisp/i2c/ov2722.h
index d8a973d71699..f133439adfd5 100644
--- a/drivers/staging/media/atomisp/i2c/ov2722.h
+++ b/drivers/staging/media/atomisp/i2c/ov2722.h
@@ -1148,6 +1148,7 @@ struct ov2722_resolution ov2722_res_preview[] = {
};
#define N_RES_PREVIEW (ARRAY_SIZE(ov2722_res_preview))
+#if 0 /* Disable non-previes configurations for now */
struct ov2722_resolution ov2722_res_still[] = {
{
.desc = "ov2722_480P_30fps",
@@ -1250,6 +1251,7 @@ struct ov2722_resolution ov2722_res_video[] = {
},
};
#define N_RES_VIDEO (ARRAY_SIZE(ov2722_res_video))
+#endif
static struct ov2722_resolution *ov2722_res = ov2722_res_preview;
static unsigned long N_RES = N_RES_PREVIEW;
diff --git a/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h b/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h
index 68cfcb4a6c3c..15a33dcd2d59 100644
--- a/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h
+++ b/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h
@@ -1147,6 +1147,7 @@ struct ov5693_resolution ov5693_res_preview[] = {
};
#define N_RES_PREVIEW (ARRAY_SIZE(ov5693_res_preview))
+#if 0 /* Disable non-previes configurations for now */
struct ov5693_resolution ov5693_res_still[] = {
{
.desc = "ov5693_736x496_30fps",
@@ -1364,6 +1365,7 @@ struct ov5693_resolution ov5693_res_video[] = {
},
};
#define N_RES_VIDEO (ARRAY_SIZE(ov5693_res_video))
+#endif
static struct ov5693_resolution *ov5693_res = ov5693_res_preview;
static unsigned long N_RES = N_RES_PREVIEW;
--
2.15.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCHv2 4/9] staging: atomisp: i2c: Disable non-preview configurations
2018-01-22 12:31 ` [PATCHv2 4/9] staging: atomisp: i2c: Disable non-preview configurations Hans Verkuil
@ 2018-02-14 16:20 ` Mauro Carvalho Chehab
2018-02-16 10:12 ` Sakari Ailus
2018-02-19 13:27 ` [PATCH v2.1 " Sakari Ailus
1 sibling, 1 reply; 35+ messages in thread
From: Mauro Carvalho Chehab @ 2018-02-14 16:20 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Sakari Ailus, Jacopo Mondi, Hans Verkuil
Em Mon, 22 Jan 2018 13:31:20 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> From: Sakari Ailus <sakari.ailus@linux.intel.com>
>
> Disable configurations for non-preview modes until configuration selection
> is improved.
Again, a poor description. It just repeats the subject.
A good subject/description should answer 3 questions:
what?
why?
how?
Anyway, looking at this patch's contents, it partially answers my
questions:
the previous patch do cause regressions at the code.
Ok, this is staging. So, we don't have very strict rules here,
but still causing regressions without providing a very good
reason why sucks.
I would also merge this with the previous one, in order to place all
regressions on a single patch.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/staging/media/atomisp/i2c/gc2235.h | 2 ++
> drivers/staging/media/atomisp/i2c/ov2722.h | 2 ++
> drivers/staging/media/atomisp/i2c/ov5693/ov5693.h | 2 ++
> 3 files changed, 6 insertions(+)
>
> diff --git a/drivers/staging/media/atomisp/i2c/gc2235.h b/drivers/staging/media/atomisp/i2c/gc2235.h
> index 45a54fea5466..817c0068c1d3 100644
> --- a/drivers/staging/media/atomisp/i2c/gc2235.h
> +++ b/drivers/staging/media/atomisp/i2c/gc2235.h
> @@ -574,6 +574,7 @@ static struct gc2235_resolution gc2235_res_preview[] = {
> };
> #define N_RES_PREVIEW (ARRAY_SIZE(gc2235_res_preview))
>
> +#if 0 /* Disable non-previes configurations for now */
typo (here and other cut-and-paste paces)
non-previes -> non-previews
also, please add a FIXME: or HACK: and describe the need for a fix
on atomisp TODO file.
> static struct gc2235_resolution gc2235_res_still[] = {
> {
> .desc = "gc2235_1600_900_30fps",
> @@ -658,6 +659,7 @@ static struct gc2235_resolution gc2235_res_video[] = {
>
> };
> #define N_RES_VIDEO (ARRAY_SIZE(gc2235_res_video))
> +#endif
>
> static struct gc2235_resolution *gc2235_res = gc2235_res_preview;
> static unsigned long N_RES = N_RES_PREVIEW;
> diff --git a/drivers/staging/media/atomisp/i2c/ov2722.h b/drivers/staging/media/atomisp/i2c/ov2722.h
> index d8a973d71699..f133439adfd5 100644
> --- a/drivers/staging/media/atomisp/i2c/ov2722.h
> +++ b/drivers/staging/media/atomisp/i2c/ov2722.h
> @@ -1148,6 +1148,7 @@ struct ov2722_resolution ov2722_res_preview[] = {
> };
> #define N_RES_PREVIEW (ARRAY_SIZE(ov2722_res_preview))
>
> +#if 0 /* Disable non-previes configurations for now */
> struct ov2722_resolution ov2722_res_still[] = {
> {
> .desc = "ov2722_480P_30fps",
> @@ -1250,6 +1251,7 @@ struct ov2722_resolution ov2722_res_video[] = {
> },
> };
> #define N_RES_VIDEO (ARRAY_SIZE(ov2722_res_video))
> +#endif
>
> static struct ov2722_resolution *ov2722_res = ov2722_res_preview;
> static unsigned long N_RES = N_RES_PREVIEW;
> diff --git a/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h b/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h
> index 68cfcb4a6c3c..15a33dcd2d59 100644
> --- a/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h
> +++ b/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h
> @@ -1147,6 +1147,7 @@ struct ov5693_resolution ov5693_res_preview[] = {
> };
> #define N_RES_PREVIEW (ARRAY_SIZE(ov5693_res_preview))
>
> +#if 0 /* Disable non-previes configurations for now */
> struct ov5693_resolution ov5693_res_still[] = {
> {
> .desc = "ov5693_736x496_30fps",
> @@ -1364,6 +1365,7 @@ struct ov5693_resolution ov5693_res_video[] = {
> },
> };
> #define N_RES_VIDEO (ARRAY_SIZE(ov5693_res_video))
> +#endif
>
> static struct ov5693_resolution *ov5693_res = ov5693_res_preview;
> static unsigned long N_RES = N_RES_PREVIEW;
Thanks,
Mauro
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCHv2 4/9] staging: atomisp: i2c: Disable non-preview configurations
2018-02-14 16:20 ` Mauro Carvalho Chehab
@ 2018-02-16 10:12 ` Sakari Ailus
2018-02-16 12:04 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 35+ messages in thread
From: Sakari Ailus @ 2018-02-16 10:12 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Hans Verkuil, linux-media, Sakari Ailus, Jacopo Mondi, Hans Verkuil
Hi Mauro,
On Wed, Feb 14, 2018 at 02:20:50PM -0200, Mauro Carvalho Chehab wrote:
> Em Mon, 22 Jan 2018 13:31:20 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>
> > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> >
> > Disable configurations for non-preview modes until configuration selection
> > is improved.
>
> Again, a poor description. It just repeats the subject.
> A good subject/description should answer 3 questions:
>
> what?
> why?
> how?
>
> Anyway, looking at this patch's contents, it partially answers my
> questions:
>
> the previous patch do cause regressions at the code.
>
> Ok, this is staging. So, we don't have very strict rules here,
> but still causing regressions without providing a very good
> reason why sucks.
>
> I would also merge this with the previous one, in order to place all
> regressions on a single patch.
It's trivial to bring back the configurations disabled here by just
reverting this patch. The other patch does not disable any. That's why
they're separate.
>
>
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> > drivers/staging/media/atomisp/i2c/gc2235.h | 2 ++
> > drivers/staging/media/atomisp/i2c/ov2722.h | 2 ++
> > drivers/staging/media/atomisp/i2c/ov5693/ov5693.h | 2 ++
> > 3 files changed, 6 insertions(+)
> >
> > diff --git a/drivers/staging/media/atomisp/i2c/gc2235.h b/drivers/staging/media/atomisp/i2c/gc2235.h
> > index 45a54fea5466..817c0068c1d3 100644
> > --- a/drivers/staging/media/atomisp/i2c/gc2235.h
> > +++ b/drivers/staging/media/atomisp/i2c/gc2235.h
> > @@ -574,6 +574,7 @@ static struct gc2235_resolution gc2235_res_preview[] = {
> > };
> > #define N_RES_PREVIEW (ARRAY_SIZE(gc2235_res_preview))
> >
> > +#if 0 /* Disable non-previes configurations for now */
>
> typo (here and other cut-and-paste paces)
> non-previes -> non-previews
>
> also, please add a FIXME: or HACK: and describe the need for a fix
> on atomisp TODO file.
>
> > static struct gc2235_resolution gc2235_res_still[] = {
> > {
> > .desc = "gc2235_1600_900_30fps",
> > @@ -658,6 +659,7 @@ static struct gc2235_resolution gc2235_res_video[] = {
> >
> > };
> > #define N_RES_VIDEO (ARRAY_SIZE(gc2235_res_video))
> > +#endif
> >
> > static struct gc2235_resolution *gc2235_res = gc2235_res_preview;
> > static unsigned long N_RES = N_RES_PREVIEW;
> > diff --git a/drivers/staging/media/atomisp/i2c/ov2722.h b/drivers/staging/media/atomisp/i2c/ov2722.h
> > index d8a973d71699..f133439adfd5 100644
> > --- a/drivers/staging/media/atomisp/i2c/ov2722.h
> > +++ b/drivers/staging/media/atomisp/i2c/ov2722.h
> > @@ -1148,6 +1148,7 @@ struct ov2722_resolution ov2722_res_preview[] = {
> > };
> > #define N_RES_PREVIEW (ARRAY_SIZE(ov2722_res_preview))
> >
> > +#if 0 /* Disable non-previes configurations for now */
> > struct ov2722_resolution ov2722_res_still[] = {
> > {
> > .desc = "ov2722_480P_30fps",
> > @@ -1250,6 +1251,7 @@ struct ov2722_resolution ov2722_res_video[] = {
> > },
> > };
> > #define N_RES_VIDEO (ARRAY_SIZE(ov2722_res_video))
> > +#endif
> >
> > static struct ov2722_resolution *ov2722_res = ov2722_res_preview;
> > static unsigned long N_RES = N_RES_PREVIEW;
> > diff --git a/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h b/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h
> > index 68cfcb4a6c3c..15a33dcd2d59 100644
> > --- a/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h
> > +++ b/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h
> > @@ -1147,6 +1147,7 @@ struct ov5693_resolution ov5693_res_preview[] = {
> > };
> > #define N_RES_PREVIEW (ARRAY_SIZE(ov5693_res_preview))
> >
> > +#if 0 /* Disable non-previes configurations for now */
> > struct ov5693_resolution ov5693_res_still[] = {
> > {
> > .desc = "ov5693_736x496_30fps",
> > @@ -1364,6 +1365,7 @@ struct ov5693_resolution ov5693_res_video[] = {
> > },
> > };
> > #define N_RES_VIDEO (ARRAY_SIZE(ov5693_res_video))
> > +#endif
> >
> > static struct ov5693_resolution *ov5693_res = ov5693_res_preview;
> > static unsigned long N_RES = N_RES_PREVIEW;
>
>
>
> Thanks,
> Mauro
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCHv2 4/9] staging: atomisp: i2c: Disable non-preview configurations
2018-02-16 10:12 ` Sakari Ailus
@ 2018-02-16 12:04 ` Mauro Carvalho Chehab
2018-02-19 12:24 ` Sakari Ailus
0 siblings, 1 reply; 35+ messages in thread
From: Mauro Carvalho Chehab @ 2018-02-16 12:04 UTC (permalink / raw)
To: Sakari Ailus
Cc: Hans Verkuil, linux-media, Sakari Ailus, Jacopo Mondi, Hans Verkuil
Em Fri, 16 Feb 2018 12:12:20 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> Hi Mauro,
>
> On Wed, Feb 14, 2018 at 02:20:50PM -0200, Mauro Carvalho Chehab wrote:
> > Em Mon, 22 Jan 2018 13:31:20 +0100
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >
> > > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > >
> > > Disable configurations for non-preview modes until configuration selection
> > > is improved.
> >
> > Again, a poor description. It just repeats the subject.
> > A good subject/description should answer 3 questions:
> >
> > what?
> > why?
> > how?
> >
> > Anyway, looking at this patch's contents, it partially answers my
> > questions:
> >
> > the previous patch do cause regressions at the code.
> >
> > Ok, this is staging. So, we don't have very strict rules here,
> > but still causing regressions without providing a very good
> > reason why sucks.
> >
> > I would also merge this with the previous one, in order to place all
> > regressions on a single patch.
>
> It's trivial to bring back the configurations disabled here by just
> reverting this patch. The other patch does not disable any. That's why
> they're separate.
Yes, and I'm not saying otherwise.
The main issue here is that it lacks a description (as what's
there is just a copy of the subject).
Why is it needed to "disable non-preview configurations"?
Also, as you're actually commenting the code with #if 0, I'm assuming
that you're thinking on re-enable the code (or re-implement with a
different logic) in the future.
So, please add a note before the #if 0, as otherwise I'm pretty
sure someone will end by sending us patches just stripping it.
Regards,
Mauro
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCHv2 4/9] staging: atomisp: i2c: Disable non-preview configurations
2018-02-16 12:04 ` Mauro Carvalho Chehab
@ 2018-02-19 12:24 ` Sakari Ailus
0 siblings, 0 replies; 35+ messages in thread
From: Sakari Ailus @ 2018-02-19 12:24 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Sakari Ailus, Hans Verkuil, linux-media, Jacopo Mondi, Hans Verkuil
Hi Mauro,
On Fri, Feb 16, 2018 at 10:04:13AM -0200, Mauro Carvalho Chehab wrote:
> Em Fri, 16 Feb 2018 12:12:20 +0200
> Sakari Ailus <sakari.ailus@iki.fi> escreveu:
>
> > Hi Mauro,
> >
> > On Wed, Feb 14, 2018 at 02:20:50PM -0200, Mauro Carvalho Chehab wrote:
> > > Em Mon, 22 Jan 2018 13:31:20 +0100
> > > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > >
> > > > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > >
> > > > Disable configurations for non-preview modes until configuration selection
> > > > is improved.
> > >
> > > Again, a poor description. It just repeats the subject.
> > > A good subject/description should answer 3 questions:
> > >
> > > what?
> > > why?
> > > how?
> > >
> > > Anyway, looking at this patch's contents, it partially answers my
> > > questions:
> > >
> > > the previous patch do cause regressions at the code.
> > >
> > > Ok, this is staging. So, we don't have very strict rules here,
> > > but still causing regressions without providing a very good
> > > reason why sucks.
> > >
> > > I would also merge this with the previous one, in order to place all
> > > regressions on a single patch.
> >
> > It's trivial to bring back the configurations disabled here by just
> > reverting this patch. The other patch does not disable any. That's why
> > they're separate.
>
> Yes, and I'm not saying otherwise.
>
> The main issue here is that it lacks a description (as what's
> there is just a copy of the subject).
>
> Why is it needed to "disable non-preview configurations"?
>
> Also, as you're actually commenting the code with #if 0, I'm assuming
> that you're thinking on re-enable the code (or re-implement with a
> different logic) in the future.
Yes, that's the intention. This being atomisp, I don't want to invest a lot
of time on this until we know the driver has a future in the kernel.
>
> So, please add a note before the #if 0, as otherwise I'm pretty
> sure someone will end by sending us patches just stripping it.
Good point. I'll respin the patch, with updated commit description and a
comment there.
--
Regards,
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2.1 4/9] staging: atomisp: i2c: Disable non-preview configurations
2018-01-22 12:31 ` [PATCHv2 4/9] staging: atomisp: i2c: Disable non-preview configurations Hans Verkuil
2018-02-14 16:20 ` Mauro Carvalho Chehab
@ 2018-02-19 13:27 ` Sakari Ailus
2018-02-19 13:30 ` Hans Verkuil
1 sibling, 1 reply; 35+ messages in thread
From: Sakari Ailus @ 2018-02-19 13:27 UTC (permalink / raw)
To: linux-media; +Cc: hverkuil, mchehab
These sensor drivers have use case specific mode lists. There's no need
for this nor there is a standard API for selecting the mode list. Disable
configurations for non-preview modes until configuration selection is
improved so that all the configurations are always usable.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
Hi Mauro and Hans,
Would you like this one better?
drivers/staging/media/atomisp/i2c/gc2235.h | 6 ++++++
drivers/staging/media/atomisp/i2c/ov2722.h | 6 ++++++
drivers/staging/media/atomisp/i2c/ov5693/ov5693.h | 6 ++++++
3 files changed, 18 insertions(+)
diff --git a/drivers/staging/media/atomisp/i2c/gc2235.h b/drivers/staging/media/atomisp/i2c/gc2235.h
index 45a54fea5466..0e805bcfa4d8 100644
--- a/drivers/staging/media/atomisp/i2c/gc2235.h
+++ b/drivers/staging/media/atomisp/i2c/gc2235.h
@@ -574,6 +574,11 @@ static struct gc2235_resolution gc2235_res_preview[] = {
};
#define N_RES_PREVIEW (ARRAY_SIZE(gc2235_res_preview))
+/*
+ * Disable non-preview configurations until the configuration selection is
+ * improved.
+ */
+#if 0
static struct gc2235_resolution gc2235_res_still[] = {
{
.desc = "gc2235_1600_900_30fps",
@@ -658,6 +663,7 @@ static struct gc2235_resolution gc2235_res_video[] = {
};
#define N_RES_VIDEO (ARRAY_SIZE(gc2235_res_video))
+#endif
static struct gc2235_resolution *gc2235_res = gc2235_res_preview;
static unsigned long N_RES = N_RES_PREVIEW;
diff --git a/drivers/staging/media/atomisp/i2c/ov2722.h b/drivers/staging/media/atomisp/i2c/ov2722.h
index d8a973d71699..028b04aaaa8f 100644
--- a/drivers/staging/media/atomisp/i2c/ov2722.h
+++ b/drivers/staging/media/atomisp/i2c/ov2722.h
@@ -1148,6 +1148,11 @@ struct ov2722_resolution ov2722_res_preview[] = {
};
#define N_RES_PREVIEW (ARRAY_SIZE(ov2722_res_preview))
+/*
+ * Disable non-preview configurations until the configuration selection is
+ * improved.
+ */
+#if 0
struct ov2722_resolution ov2722_res_still[] = {
{
.desc = "ov2722_480P_30fps",
@@ -1250,6 +1255,7 @@ struct ov2722_resolution ov2722_res_video[] = {
},
};
#define N_RES_VIDEO (ARRAY_SIZE(ov2722_res_video))
+#endif
static struct ov2722_resolution *ov2722_res = ov2722_res_preview;
static unsigned long N_RES = N_RES_PREVIEW;
diff --git a/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h b/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h
index 68cfcb4a6c3c..6d27dd849a62 100644
--- a/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h
+++ b/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h
@@ -1147,6 +1147,11 @@ struct ov5693_resolution ov5693_res_preview[] = {
};
#define N_RES_PREVIEW (ARRAY_SIZE(ov5693_res_preview))
+/*
+ * Disable non-preview configurations until the configuration selection is
+ * improved.
+ */
+#if 0
struct ov5693_resolution ov5693_res_still[] = {
{
.desc = "ov5693_736x496_30fps",
@@ -1364,6 +1369,7 @@ struct ov5693_resolution ov5693_res_video[] = {
},
};
#define N_RES_VIDEO (ARRAY_SIZE(ov5693_res_video))
+#endif
static struct ov5693_resolution *ov5693_res = ov5693_res_preview;
static unsigned long N_RES = N_RES_PREVIEW;
--
2.11.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v2.1 4/9] staging: atomisp: i2c: Disable non-preview configurations
2018-02-19 13:27 ` [PATCH v2.1 " Sakari Ailus
@ 2018-02-19 13:30 ` Hans Verkuil
2018-02-19 13:48 ` Sakari Ailus
0 siblings, 1 reply; 35+ messages in thread
From: Hans Verkuil @ 2018-02-19 13:30 UTC (permalink / raw)
To: Sakari Ailus, linux-media; +Cc: mchehab
On 02/19/2018 02:27 PM, Sakari Ailus wrote:
> These sensor drivers have use case specific mode lists. There's no need
> for this nor there is a standard API for selecting the mode list. Disable
> configurations for non-preview modes until configuration selection is
> improved so that all the configurations are always usable.
It's a bit confusing. This seems contradictory: if there is no need for it,
then it can be removed. Or there is a need for it, but we don't have a
standard API to do it. You can't have both, can you?
Or did you mean that this functionality is currently unused, and that if
we wanted to use it, we first need a new API? That would actually make sense.
Regards,
Hans
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Hi Mauro and Hans,
>
> Would you like this one better?
>
> drivers/staging/media/atomisp/i2c/gc2235.h | 6 ++++++
> drivers/staging/media/atomisp/i2c/ov2722.h | 6 ++++++
> drivers/staging/media/atomisp/i2c/ov5693/ov5693.h | 6 ++++++
> 3 files changed, 18 insertions(+)
>
> diff --git a/drivers/staging/media/atomisp/i2c/gc2235.h b/drivers/staging/media/atomisp/i2c/gc2235.h
> index 45a54fea5466..0e805bcfa4d8 100644
> --- a/drivers/staging/media/atomisp/i2c/gc2235.h
> +++ b/drivers/staging/media/atomisp/i2c/gc2235.h
> @@ -574,6 +574,11 @@ static struct gc2235_resolution gc2235_res_preview[] = {
> };
> #define N_RES_PREVIEW (ARRAY_SIZE(gc2235_res_preview))
>
> +/*
> + * Disable non-preview configurations until the configuration selection is
> + * improved.
> + */
> +#if 0
> static struct gc2235_resolution gc2235_res_still[] = {
> {
> .desc = "gc2235_1600_900_30fps",
> @@ -658,6 +663,7 @@ static struct gc2235_resolution gc2235_res_video[] = {
>
> };
> #define N_RES_VIDEO (ARRAY_SIZE(gc2235_res_video))
> +#endif
>
> static struct gc2235_resolution *gc2235_res = gc2235_res_preview;
> static unsigned long N_RES = N_RES_PREVIEW;
> diff --git a/drivers/staging/media/atomisp/i2c/ov2722.h b/drivers/staging/media/atomisp/i2c/ov2722.h
> index d8a973d71699..028b04aaaa8f 100644
> --- a/drivers/staging/media/atomisp/i2c/ov2722.h
> +++ b/drivers/staging/media/atomisp/i2c/ov2722.h
> @@ -1148,6 +1148,11 @@ struct ov2722_resolution ov2722_res_preview[] = {
> };
> #define N_RES_PREVIEW (ARRAY_SIZE(ov2722_res_preview))
>
> +/*
> + * Disable non-preview configurations until the configuration selection is
> + * improved.
> + */
> +#if 0
> struct ov2722_resolution ov2722_res_still[] = {
> {
> .desc = "ov2722_480P_30fps",
> @@ -1250,6 +1255,7 @@ struct ov2722_resolution ov2722_res_video[] = {
> },
> };
> #define N_RES_VIDEO (ARRAY_SIZE(ov2722_res_video))
> +#endif
>
> static struct ov2722_resolution *ov2722_res = ov2722_res_preview;
> static unsigned long N_RES = N_RES_PREVIEW;
> diff --git a/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h b/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h
> index 68cfcb4a6c3c..6d27dd849a62 100644
> --- a/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h
> +++ b/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h
> @@ -1147,6 +1147,11 @@ struct ov5693_resolution ov5693_res_preview[] = {
> };
> #define N_RES_PREVIEW (ARRAY_SIZE(ov5693_res_preview))
>
> +/*
> + * Disable non-preview configurations until the configuration selection is
> + * improved.
> + */
> +#if 0
> struct ov5693_resolution ov5693_res_still[] = {
> {
> .desc = "ov5693_736x496_30fps",
> @@ -1364,6 +1369,7 @@ struct ov5693_resolution ov5693_res_video[] = {
> },
> };
> #define N_RES_VIDEO (ARRAY_SIZE(ov5693_res_video))
> +#endif
>
> static struct ov5693_resolution *ov5693_res = ov5693_res_preview;
> static unsigned long N_RES = N_RES_PREVIEW;
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2.1 4/9] staging: atomisp: i2c: Disable non-preview configurations
2018-02-19 13:30 ` Hans Verkuil
@ 2018-02-19 13:48 ` Sakari Ailus
2018-02-19 13:58 ` Hans Verkuil
0 siblings, 1 reply; 35+ messages in thread
From: Sakari Ailus @ 2018-02-19 13:48 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, mchehab
Hi Hans,
On Mon, Feb 19, 2018 at 02:30:18PM +0100, Hans Verkuil wrote:
> On 02/19/2018 02:27 PM, Sakari Ailus wrote:
> > These sensor drivers have use case specific mode lists. There's no need
> > for this nor there is a standard API for selecting the mode list. Disable
> > configurations for non-preview modes until configuration selection is
> > improved so that all the configurations are always usable.
>
> It's a bit confusing. This seems contradictory: if there is no need for it,
> then it can be removed. Or there is a need for it, but we don't have a
> standard API to do it. You can't have both, can you?
>
> Or did you mean that this functionality is currently unused, and that if
> we wanted to use it, we first need a new API? That would actually make sense.
Correct. All it takes might be just to put these modes to the common list.
But I can't test that as I have no hardware. It's easy to implement that
later on though, that's the idea.
--
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2.1 4/9] staging: atomisp: i2c: Disable non-preview configurations
2018-02-19 13:48 ` Sakari Ailus
@ 2018-02-19 13:58 ` Hans Verkuil
2018-02-19 14:05 ` Sakari Ailus
0 siblings, 1 reply; 35+ messages in thread
From: Hans Verkuil @ 2018-02-19 13:58 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, mchehab
On 02/19/2018 02:48 PM, Sakari Ailus wrote:
> Hi Hans,
>
> On Mon, Feb 19, 2018 at 02:30:18PM +0100, Hans Verkuil wrote:
>> On 02/19/2018 02:27 PM, Sakari Ailus wrote:
>>> These sensor drivers have use case specific mode lists. There's no need
>>> for this nor there is a standard API for selecting the mode list. Disable
>>> configurations for non-preview modes until configuration selection is
>>> improved so that all the configurations are always usable.
>>
>> It's a bit confusing. This seems contradictory: if there is no need for it,
>> then it can be removed. Or there is a need for it, but we don't have a
>> standard API to do it. You can't have both, can you?
>>
>> Or did you mean that this functionality is currently unused, and that if
>> we wanted to use it, we first need a new API? That would actually make sense.
>
> Correct. All it takes might be just to put these modes to the common list.
> But I can't test that as I have no hardware. It's easy to implement that
> later on though, that's the idea.
>
I've reworded the second sentence slightly:
"These sensor drivers have use case specific mode lists. This is currently
not used nor there is a standard API for selecting the mode list. Disable
configurations for non-preview modes until configuration selection is
improved so that all the configurations are always usable."
Is that OK?
Regards,
Hans
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2.1 4/9] staging: atomisp: i2c: Disable non-preview configurations
2018-02-19 13:58 ` Hans Verkuil
@ 2018-02-19 14:05 ` Sakari Ailus
0 siblings, 0 replies; 35+ messages in thread
From: Sakari Ailus @ 2018-02-19 14:05 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, mchehab
On Mon, Feb 19, 2018 at 02:58:45PM +0100, Hans Verkuil wrote:
> On 02/19/2018 02:48 PM, Sakari Ailus wrote:
> > Hi Hans,
> >
> > On Mon, Feb 19, 2018 at 02:30:18PM +0100, Hans Verkuil wrote:
> >> On 02/19/2018 02:27 PM, Sakari Ailus wrote:
> >>> These sensor drivers have use case specific mode lists. There's no need
> >>> for this nor there is a standard API for selecting the mode list. Disable
> >>> configurations for non-preview modes until configuration selection is
> >>> improved so that all the configurations are always usable.
> >>
> >> It's a bit confusing. This seems contradictory: if there is no need for it,
> >> then it can be removed. Or there is a need for it, but we don't have a
> >> standard API to do it. You can't have both, can you?
> >>
> >> Or did you mean that this functionality is currently unused, and that if
> >> we wanted to use it, we first need a new API? That would actually make sense.
> >
> > Correct. All it takes might be just to put these modes to the common list.
> > But I can't test that as I have no hardware. It's easy to implement that
> > later on though, that's the idea.
> >
>
> I've reworded the second sentence slightly:
>
> "These sensor drivers have use case specific mode lists. This is currently
> not used nor there is a standard API for selecting the mode list. Disable
> configurations for non-preview modes until configuration selection is
> improved so that all the configurations are always usable."
>
> Is that OK?
Works for me. Thanks!
--
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCHv2 5/9] staging: atomisp: i2c: Drop g_parm support in sensor drivers
2018-01-22 12:31 [PATCHv2 0/9] media: replace g/s_parm by g/s_frame_interval Hans Verkuil
` (3 preceding siblings ...)
2018-01-22 12:31 ` [PATCHv2 4/9] staging: atomisp: i2c: Disable non-preview configurations Hans Verkuil
@ 2018-01-22 12:31 ` Hans Verkuil
2018-01-22 12:31 ` [PATCHv2 6/9] staging: atomisp: mt9m114: Drop empty s_parm callback Hans Verkuil
` (4 subsequent siblings)
9 siblings, 0 replies; 35+ messages in thread
From: Hans Verkuil @ 2018-01-22 12:31 UTC (permalink / raw)
To: linux-media; +Cc: Sakari Ailus, Jacopo Mondi, Hans Verkuil
From: Sakari Ailus <sakari.ailus@linux.intel.com>
These drivers already support g_frame_interval. Therefore just dropping
g_parm is enough.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 27 ----------------------
drivers/staging/media/atomisp/i2c/atomisp-gc2235.c | 27 ----------------------
drivers/staging/media/atomisp/i2c/atomisp-ov2680.c | 27 ----------------------
drivers/staging/media/atomisp/i2c/atomisp-ov2722.c | 27 ----------------------
.../media/atomisp/i2c/ov5693/atomisp-ov5693.c | 27 ----------------------
5 files changed, 135 deletions(-)
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index 572c9127c24d..93753cb96180 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -1204,32 +1204,6 @@ static int gc0310_s_config(struct v4l2_subdev *sd,
return ret;
}
-static int gc0310_g_parm(struct v4l2_subdev *sd,
- struct v4l2_streamparm *param)
-{
- struct gc0310_device *dev = to_gc0310_sensor(sd);
- struct i2c_client *client = v4l2_get_subdevdata(sd);
-
- if (!param)
- return -EINVAL;
-
- if (param->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) {
- dev_err(&client->dev, "unsupported buffer type.\n");
- return -EINVAL;
- }
-
- memset(param, 0, sizeof(*param));
- param->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
-
- if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
- param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
- param->parm.capture.timeperframe.numerator = 1;
- param->parm.capture.timeperframe.denominator =
- gc0310_res[dev->fmt_idx].fps;
- }
- return 0;
-}
-
static int gc0310_g_frame_interval(struct v4l2_subdev *sd,
struct v4l2_subdev_frame_interval *interval)
{
@@ -1288,7 +1262,6 @@ static const struct v4l2_subdev_sensor_ops gc0310_sensor_ops = {
static const struct v4l2_subdev_video_ops gc0310_video_ops = {
.s_stream = gc0310_s_stream,
- .g_parm = gc0310_g_parm,
.g_frame_interval = gc0310_g_frame_interval,
};
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
index 2bc179f3afe5..93f9c618f3d8 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
@@ -944,32 +944,6 @@ static int gc2235_s_config(struct v4l2_subdev *sd,
return ret;
}
-static int gc2235_g_parm(struct v4l2_subdev *sd,
- struct v4l2_streamparm *param)
-{
- struct gc2235_device *dev = to_gc2235_sensor(sd);
- struct i2c_client *client = v4l2_get_subdevdata(sd);
-
- if (!param)
- return -EINVAL;
-
- if (param->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) {
- dev_err(&client->dev, "unsupported buffer type.\n");
- return -EINVAL;
- }
-
- memset(param, 0, sizeof(*param));
- param->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
-
- if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
- param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
- param->parm.capture.timeperframe.numerator = 1;
- param->parm.capture.timeperframe.denominator =
- gc2235_res[dev->fmt_idx].fps;
- }
- return 0;
-}
-
static int gc2235_g_frame_interval(struct v4l2_subdev *sd,
struct v4l2_subdev_frame_interval *interval)
{
@@ -1027,7 +1001,6 @@ static const struct v4l2_subdev_sensor_ops gc2235_sensor_ops = {
static const struct v4l2_subdev_video_ops gc2235_video_ops = {
.s_stream = gc2235_s_stream,
- .g_parm = gc2235_g_parm,
.g_frame_interval = gc2235_g_frame_interval,
};
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index e3e0fdd0c816..11412061c40e 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -1280,32 +1280,6 @@ static int ov2680_s_config(struct v4l2_subdev *sd,
return ret;
}
-static int ov2680_g_parm(struct v4l2_subdev *sd,
- struct v4l2_streamparm *param)
-{
- struct ov2680_device *dev = to_ov2680_sensor(sd);
- struct i2c_client *client = v4l2_get_subdevdata(sd);
-
- if (!param)
- return -EINVAL;
-
- if (param->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) {
- dev_err(&client->dev, "unsupported buffer type.\n");
- return -EINVAL;
- }
-
- memset(param, 0, sizeof(*param));
- param->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
-
- if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
- param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
- param->parm.capture.timeperframe.numerator = 1;
- param->parm.capture.timeperframe.denominator =
- ov2680_res[dev->fmt_idx].fps;
- }
- return 0;
-}
-
static int ov2680_g_frame_interval(struct v4l2_subdev *sd,
struct v4l2_subdev_frame_interval *interval)
{
@@ -1359,7 +1333,6 @@ static int ov2680_g_skip_frames(struct v4l2_subdev *sd, u32 *frames)
static const struct v4l2_subdev_video_ops ov2680_video_ops = {
.s_stream = ov2680_s_stream,
- .g_parm = ov2680_g_parm,
.g_frame_interval = ov2680_g_frame_interval,
};
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
index cd9f6433cd42..e59358ac89ce 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
@@ -1083,32 +1083,6 @@ static int ov2722_s_config(struct v4l2_subdev *sd,
return ret;
}
-static int ov2722_g_parm(struct v4l2_subdev *sd,
- struct v4l2_streamparm *param)
-{
- struct ov2722_device *dev = to_ov2722_sensor(sd);
- struct i2c_client *client = v4l2_get_subdevdata(sd);
-
- if (!param)
- return -EINVAL;
-
- if (param->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) {
- dev_err(&client->dev, "unsupported buffer type.\n");
- return -EINVAL;
- }
-
- memset(param, 0, sizeof(*param));
- param->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
-
- if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
- param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
- param->parm.capture.timeperframe.numerator = 1;
- param->parm.capture.timeperframe.denominator =
- ov2722_res[dev->fmt_idx].fps;
- }
- return 0;
-}
-
static int ov2722_g_frame_interval(struct v4l2_subdev *sd,
struct v4l2_subdev_frame_interval *interval)
{
@@ -1167,7 +1141,6 @@ static const struct v4l2_subdev_sensor_ops ov2722_sensor_ops = {
static const struct v4l2_subdev_video_ops ov2722_video_ops = {
.s_stream = ov2722_s_stream,
- .g_parm = ov2722_g_parm,
.g_frame_interval = ov2722_g_frame_interval,
};
diff --git a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
index 7f594c7de76e..56f3cd0d8c23 100644
--- a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
+++ b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
@@ -1805,32 +1805,6 @@ static int ov5693_s_config(struct v4l2_subdev *sd,
return ret;
}
-static int ov5693_g_parm(struct v4l2_subdev *sd,
- struct v4l2_streamparm *param)
-{
- struct ov5693_device *dev = to_ov5693_sensor(sd);
- struct i2c_client *client = v4l2_get_subdevdata(sd);
-
- if (!param)
- return -EINVAL;
-
- if (param->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) {
- dev_err(&client->dev, "unsupported buffer type.\n");
- return -EINVAL;
- }
-
- memset(param, 0, sizeof(*param));
- param->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
-
- if (dev->fmt_idx >= 0 && dev->fmt_idx < N_RES) {
- param->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
- param->parm.capture.timeperframe.numerator = 1;
- param->parm.capture.timeperframe.denominator =
- ov5693_res[dev->fmt_idx].fps;
- }
- return 0;
-}
-
static int ov5693_g_frame_interval(struct v4l2_subdev *sd,
struct v4l2_subdev_frame_interval *interval)
{
@@ -1873,7 +1847,6 @@ static int ov5693_enum_frame_size(struct v4l2_subdev *sd,
static const struct v4l2_subdev_video_ops ov5693_video_ops = {
.s_stream = ov5693_s_stream,
- .g_parm = ov5693_g_parm,
.g_frame_interval = ov5693_g_frame_interval,
};
--
2.15.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCHv2 6/9] staging: atomisp: mt9m114: Drop empty s_parm callback
2018-01-22 12:31 [PATCHv2 0/9] media: replace g/s_parm by g/s_frame_interval Hans Verkuil
` (4 preceding siblings ...)
2018-01-22 12:31 ` [PATCHv2 5/9] staging: atomisp: i2c: Drop g_parm support in sensor drivers Hans Verkuil
@ 2018-01-22 12:31 ` Hans Verkuil
2018-01-22 12:31 ` [PATCHv2 7/9] staging: atomisp: Drop g_parm and s_parm subdev ops use Hans Verkuil
` (3 subsequent siblings)
9 siblings, 0 replies; 35+ messages in thread
From: Hans Verkuil @ 2018-01-22 12:31 UTC (permalink / raw)
To: linux-media; +Cc: Sakari Ailus, Jacopo Mondi, Hans Verkuil
From: Sakari Ailus <sakari.ailus@linux.intel.com>
The s_parm callback in mt9m114 driver did nothing, remove it.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c b/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c
index df253a557c76..834fba8c4fa0 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c
@@ -1684,11 +1684,6 @@ static int mt9m114_t_vflip(struct v4l2_subdev *sd, int value)
return !!err;
}
-static int mt9m114_s_parm(struct v4l2_subdev *sd,
- struct v4l2_streamparm *param)
-{
- return 0;
-}
static int mt9m114_g_frame_interval(struct v4l2_subdev *sd,
struct v4l2_subdev_frame_interval *interval)
@@ -1781,7 +1776,6 @@ static int mt9m114_g_skip_frames(struct v4l2_subdev *sd, u32 *frames)
}
static const struct v4l2_subdev_video_ops mt9m114_video_ops = {
- .s_parm = mt9m114_s_parm,
.s_stream = mt9m114_s_stream,
.g_frame_interval = mt9m114_g_frame_interval,
};
--
2.15.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCHv2 7/9] staging: atomisp: Drop g_parm and s_parm subdev ops use
2018-01-22 12:31 [PATCHv2 0/9] media: replace g/s_parm by g/s_frame_interval Hans Verkuil
` (5 preceding siblings ...)
2018-01-22 12:31 ` [PATCHv2 6/9] staging: atomisp: mt9m114: Drop empty s_parm callback Hans Verkuil
@ 2018-01-22 12:31 ` Hans Verkuil
2018-01-22 12:31 ` [PATCHv2 8/9] v4l2-subdev.h: remove obsolete g/s_parm Hans Verkuil
` (2 subsequent siblings)
9 siblings, 0 replies; 35+ messages in thread
From: Hans Verkuil @ 2018-01-22 12:31 UTC (permalink / raw)
To: linux-media; +Cc: Sakari Ailus, Jacopo Mondi, Hans Verkuil
From: Sakari Ailus <sakari.ailus@linux.intel.com>
The s_parm and g_parm did nothing. Remove them.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
.../staging/media/atomisp/pci/atomisp2/atomisp_file.c | 16 ----------------
drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c | 14 --------------
2 files changed, 30 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c
index 377ec2a9fa6d..c6d96987561d 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c
@@ -77,20 +77,6 @@ static int file_input_s_stream(struct v4l2_subdev *sd, int enable)
return 0;
}
-static int file_input_g_parm(struct v4l2_subdev *sd,
- struct v4l2_streamparm *param)
-{
- /*to fake*/
- return 0;
-}
-
-static int file_input_s_parm(struct v4l2_subdev *sd,
- struct v4l2_streamparm *param)
-{
- /*to fake*/
- return 0;
-}
-
static int file_input_get_fmt(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *format)
@@ -166,8 +152,6 @@ static int file_input_enum_frame_ival(struct v4l2_subdev *sd,
static const struct v4l2_subdev_video_ops file_input_video_ops = {
.s_stream = file_input_s_stream,
- .g_parm = file_input_g_parm,
- .s_parm = file_input_s_parm,
};
static const struct v4l2_subdev_core_ops file_input_core_ops = {
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c
index b71cc7bcdbab..adc900272f6f 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c
@@ -27,18 +27,6 @@ static int tpg_s_stream(struct v4l2_subdev *sd, int enable)
return 0;
}
-static int tpg_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *param)
-{
- /*to fake*/
- return 0;
-}
-
-static int tpg_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *param)
-{
- /*to fake*/
- return 0;
-}
-
static int tpg_get_fmt(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *format)
@@ -101,8 +89,6 @@ static int tpg_enum_frame_ival(struct v4l2_subdev *sd,
static const struct v4l2_subdev_video_ops tpg_video_ops = {
.s_stream = tpg_s_stream,
- .g_parm = tpg_g_parm,
- .s_parm = tpg_s_parm,
};
static const struct v4l2_subdev_core_ops tpg_core_ops = {
--
2.15.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCHv2 8/9] v4l2-subdev.h: remove obsolete g/s_parm
2018-01-22 12:31 [PATCHv2 0/9] media: replace g/s_parm by g/s_frame_interval Hans Verkuil
` (6 preceding siblings ...)
2018-01-22 12:31 ` [PATCHv2 7/9] staging: atomisp: Drop g_parm and s_parm subdev ops use Hans Verkuil
@ 2018-01-22 12:31 ` Hans Verkuil
2018-01-22 12:31 ` [PATCHv2 9/9] vidioc-g-parm.rst: also allow _MPLANE buffer types Hans Verkuil
2018-01-22 12:38 ` [PATCHv2 0/9] media: replace g/s_parm by g/s_frame_interval Sakari Ailus
9 siblings, 0 replies; 35+ messages in thread
From: Hans Verkuil @ 2018-01-22 12:31 UTC (permalink / raw)
To: linux-media; +Cc: Sakari Ailus, Jacopo Mondi, Hans Verkuil
From: Hans Verkuil <hansverk@cisco.com>
Signed-off-by: Hans Verkuil <hansverk@cisco.com>
---
include/media/v4l2-subdev.h | 6 ------
1 file changed, 6 deletions(-)
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 980a86c08fce..457917e9237f 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -393,10 +393,6 @@ struct v4l2_mbus_frame_desc {
*
* @g_pixelaspect: callback to return the pixelaspect ratio.
*
- * @g_parm: callback for VIDIOC_G_PARM() ioctl handler code.
- *
- * @s_parm: callback for VIDIOC_S_PARM() ioctl handler code.
- *
* @g_frame_interval: callback for VIDIOC_SUBDEV_G_FRAME_INTERVAL()
* ioctl handler code.
*
@@ -434,8 +430,6 @@ struct v4l2_subdev_video_ops {
int (*g_input_status)(struct v4l2_subdev *sd, u32 *status);
int (*s_stream)(struct v4l2_subdev *sd, int enable);
int (*g_pixelaspect)(struct v4l2_subdev *sd, struct v4l2_fract *aspect);
- int (*g_parm)(struct v4l2_subdev *sd, struct v4l2_streamparm *param);
- int (*s_parm)(struct v4l2_subdev *sd, struct v4l2_streamparm *param);
int (*g_frame_interval)(struct v4l2_subdev *sd,
struct v4l2_subdev_frame_interval *interval);
int (*s_frame_interval)(struct v4l2_subdev *sd,
--
2.15.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCHv2 9/9] vidioc-g-parm.rst: also allow _MPLANE buffer types
2018-01-22 12:31 [PATCHv2 0/9] media: replace g/s_parm by g/s_frame_interval Hans Verkuil
` (7 preceding siblings ...)
2018-01-22 12:31 ` [PATCHv2 8/9] v4l2-subdev.h: remove obsolete g/s_parm Hans Verkuil
@ 2018-01-22 12:31 ` Hans Verkuil
2018-01-22 12:38 ` [PATCHv2 0/9] media: replace g/s_parm by g/s_frame_interval Sakari Ailus
9 siblings, 0 replies; 35+ messages in thread
From: Hans Verkuil @ 2018-01-22 12:31 UTC (permalink / raw)
To: linux-media; +Cc: Sakari Ailus, Jacopo Mondi, Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
The specification mentions that type can be V4L2_BUF_TYPE_VIDEO_CAPTURE,
but the v4l2 core implementation also allows the _MPLANE variant.
Document this.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
Documentation/media/uapi/v4l/vidioc-g-parm.rst | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/Documentation/media/uapi/v4l/vidioc-g-parm.rst b/Documentation/media/uapi/v4l/vidioc-g-parm.rst
index 616a5ea3f8fa..e831fa5512f0 100644
--- a/Documentation/media/uapi/v4l/vidioc-g-parm.rst
+++ b/Documentation/media/uapi/v4l/vidioc-g-parm.rst
@@ -66,7 +66,7 @@ union holding separate parameters for input and output devices.
-
- The buffer (stream) type, same as struct
:c:type:`v4l2_format` ``type``, set by the
- application. See :c:type:`v4l2_buf_type`
+ application. See :c:type:`v4l2_buf_type`.
* - union
- ``parm``
-
@@ -75,12 +75,13 @@ union holding separate parameters for input and output devices.
- struct :c:type:`v4l2_captureparm`
- ``capture``
- Parameters for capture devices, used when ``type`` is
- ``V4L2_BUF_TYPE_VIDEO_CAPTURE``.
+ ``V4L2_BUF_TYPE_VIDEO_CAPTURE`` or
+ ``V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE``.
* -
- struct :c:type:`v4l2_outputparm`
- ``output``
- Parameters for output devices, used when ``type`` is
- ``V4L2_BUF_TYPE_VIDEO_OUTPUT``.
+ ``V4L2_BUF_TYPE_VIDEO_OUTPUT`` or ``V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE``.
* -
- __u8
- ``raw_data``\ [200]
--
2.15.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCHv2 0/9] media: replace g/s_parm by g/s_frame_interval
2018-01-22 12:31 [PATCHv2 0/9] media: replace g/s_parm by g/s_frame_interval Hans Verkuil
` (8 preceding siblings ...)
2018-01-22 12:31 ` [PATCHv2 9/9] vidioc-g-parm.rst: also allow _MPLANE buffer types Hans Verkuil
@ 2018-01-22 12:38 ` Sakari Ailus
9 siblings, 0 replies; 35+ messages in thread
From: Sakari Ailus @ 2018-01-22 12:38 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Jacopo Mondi
Hi Hans,
On Mon, Jan 22, 2018 at 01:31:16PM +0100, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> There are currently two subdev ops variants to get/set the frame interval:
> g/s_parm and g/s_frame_interval.
>
> This patch series replaces all g/s_parm calls by g/s_frame_interval.
>
> The first patch adds helper functions that can be used by bridge drivers.
> Only em28xx can't use it and it needs custom code (it uses v4l2_device_call()
> to try all subdevs instead of calling a specific subdev).
>
> The next patch converts all non-staging drivers, then come Sakari's
> atomisp staging fixes.
>
> The v4l2-subdev.h patch removes the now obsolete g/s_parm ops and the
> final patch clarifies the documentation a bit (the core allows for
> _MPLANE to be used as well).
>
> I would really like to take the next step and introduce two new ioctls
> VIDIOC_G/S_FRAME_INTERVAL (just like the SUBDEV variants that already
> exist) and convert all bridge drivers to use that and just have helper
> functions in the core for VIDIOC_G/S_PARM.
>
> I hate that ioctl and it always confuses driver developers. It would
> also prevent the type of abuse that was present in the atomisp driver.
>
> But that's for later, let's simplify the subdev drivers first.
Apart from my patches,
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
--
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 35+ messages in thread