media: v4l: ctrls: Add debug messages
Subsystems (2)
PATCH v2 1/1 media: v4l: ctrls: Add debug messages
Currently, the v4l2 control code is a bit silent on errors. Add debug messages on (hopefully) most of the error paths.
---
Changes from v1:
* Drop changes in the debug parameter semantics.
* Drop new module debug parameter.
* Add documentation.
* Add a debug error in all places where control can fail.
* Reorder the vdev parameter, to make the patch less invasive.
--- Documentation/media/kapi/v4l2-dev.rst | 1 + drivers/media/platform/omap3isp/ispvideo.c | 2 +- drivers/media/v4l2-core/v4l2-ctrls.c | 106 ++++++++++++++++----- drivers/media/v4l2-core/v4l2-ioctl.c | 12 +-- drivers/media/v4l2-core/v4l2-subdev.c | 6 +- include/media/v4l2-ctrls.h | 9 +- include/media/v4l2-ioctl.h | 2 + 7 files changed, 100 insertions(+), 38 deletions(-)
Documentation/media/kapi/v4l2-dev.rst +1 -0
@@ -288,6 +288,7 @@ Mask Description 0x08 Log the read and write file operations and the VIDIOC_QBUF and VIDIOC_DQBUF ioctls. 0x10 Log the poll file operation. +0x20 Log error and messages in the control operations. ===== ================================================================ Video device cleanup
drivers/media/platform/omap3isp/ispvideo.c +1 -1
@@ -1028,7 +1028,7 @@ static int isp_video_check_external_subdevs(struct isp_video *video, ctrls.count =1 ; ctrls.controls = &ctrl; - ret =v4l2_g_ext_ctrls (pipe->external->ctrl_handler,NULL , &ctrls); + ret =v4l2_g_ext_ctrls (pipe->external->ctrl_handler, &video->video,NULL , &ctrls);if (ret <0 ) {dev_warn (isp->dev,"no pixel rate control in subdev %s\n" , pipe->external->name);
drivers/media/v4l2-core/v4l2-ctrls.c +80 -26
261 lines not on this page · raw message
drivers/media/v4l2-core/v4l2-ioctl.c +6 -6
@@ -2165,9 +2165,9 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops, p->error_idx = p->count;if (vfh && vfh->ctrl_handler) -return v4l2_g_ext_ctrls (vfh->ctrl_handler, vfd->v4l2_dev->mdev, p); +return v4l2_g_ext_ctrls (vfh->ctrl_handler, vfd, vfd->v4l2_dev->mdev, p);if (vfd->ctrl_handler) -return v4l2_g_ext_ctrls (vfd->ctrl_handler, vfd->v4l2_dev->mdev, p); +return v4l2_g_ext_ctrls (vfd->ctrl_handler, vfd, vfd->v4l2_dev->mdev, p);if (ops->vidioc_g_ext_ctrls ==NULL )return -ENOTTY ; return check_ext_ctrls(p, 0) ? ops->vidioc_g_ext_ctrls(file, fh, p) :
@@ -2184,9 +2184,9 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops, p->error_idx = p->count;if (vfh && vfh->ctrl_handler) -return v4l2_s_ext_ctrls (vfh, vfh->ctrl_handler, vfd->v4l2_dev->mdev, p); +return v4l2_s_ext_ctrls (vfh, vfh->ctrl_handler, vfd, vfd->v4l2_dev->mdev, p);if (vfd->ctrl_handler) -return v4l2_s_ext_ctrls (NULL , vfd->ctrl_handler, vfd->v4l2_dev->mdev, p); +return v4l2_s_ext_ctrls (NULL , vfd->ctrl_handler, vfd, vfd->v4l2_dev->mdev, p);if (ops->vidioc_s_ext_ctrls ==NULL )return -ENOTTY ; return check_ext_ctrls(p, 0) ? ops->vidioc_s_ext_ctrls(file, fh, p) :
@@ -2203,9 +2203,9 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops, p->error_idx = p->count;if (vfh && vfh->ctrl_handler) -return v4l2_try_ext_ctrls (vfh->ctrl_handler, vfd->v4l2_dev->mdev, p); +return v4l2_try_ext_ctrls (vfh->ctrl_handler, vfd, vfd->v4l2_dev->mdev, p);if (vfd->ctrl_handler) -return v4l2_try_ext_ctrls (vfd->ctrl_handler, vfd->v4l2_dev->mdev, p); +return v4l2_try_ext_ctrls (vfd->ctrl_handler, vfd, vfd->v4l2_dev->mdev, p);if (ops->vidioc_try_ext_ctrls ==NULL )return -ENOTTY ; return check_ext_ctrls(p, 0) ? ops->vidioc_try_ext_ctrls(file, fh, p) :
drivers/media/v4l2-core/v4l2-subdev.c +3 -3
@@ -211,19 +211,19 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)if (!vfh->ctrl_handler)return -ENOTTY ;return v4l2_g_ext_ctrls (vfh->ctrl_handler, - sd->v4l2_dev->mdev, arg); + vdev, sd->v4l2_dev->mdev, arg);case VIDIOC_S_EXT_CTRLS :if (!vfh->ctrl_handler)return -ENOTTY ;return v4l2_s_ext_ctrls (vfh, vfh->ctrl_handler, - sd->v4l2_dev->mdev, arg); + vdev, sd->v4l2_dev->mdev, arg);case VIDIOC_TRY_EXT_CTRLS :if (!vfh->ctrl_handler)return -ENOTTY ;return v4l2_try_ext_ctrls (vfh->ctrl_handler, - sd->v4l2_dev->mdev, arg); + vdev, sd->v4l2_dev->mdev, arg);case VIDIOC_DQEVENT :if (!(sd->flags &V4L2_SUBDEV_FL_HAS_EVENTS ))
include/media/v4l2-ctrls.h +7 -2
@@ -1265,25 +1265,28 @@ int v4l2_s_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,* :ref:`VIDIOC_G_EXT_CTRLS <vidioc_g_ext_ctrls>` ioctl * * @hdl: pointer to &struct v4l2_ctrl_handler +* @vdev: pointer to &struct video_device * @mdev: pointer to &struct media_device * @c: pointer to &struct v4l2_ext_controls * * If hdl == NULL then they will all return -EINVAL. */ -int v4l2_g_ext_ctrls (struct v4l2_ctrl_handler *hdl,struct media_device *mdev, -struct v4l2_ext_controls *c); +int v4l2_g_ext_ctrls (struct v4l2_ctrl_handler *hdl,struct video_device *vdev, +struct media_device *mdev,struct v4l2_ext_controls *c);/** * v4l2_try_ext_ctrls - Helper function to implement * :ref:`VIDIOC_TRY_EXT_CTRLS <vidioc_g_ext_ctrls>` ioctl * * @hdl: pointer to &struct v4l2_ctrl_handler +* @vdev: pointer to &struct video_device * @mdev: pointer to &struct media_device * @c: pointer to &struct v4l2_ext_controls * * If hdl == NULL then they will all return -EINVAL. */ int v4l2_try_ext_ctrls (struct v4l2_ctrl_handler *hdl, +struct video_device *vdev,struct media_device *mdev,struct v4l2_ext_controls *c);
@@ -1293,12 +1296,14 @@ int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl,* * @fh: pointer to &struct v4l2_fh * @hdl: pointer to &struct v4l2_ctrl_handler +* @vdev: pointer to &struct video_device * @mdev: pointer to &struct media_device * @c: pointer to &struct v4l2_ext_controls * * If hdl == NULL then they will all return -EINVAL. */ int v4l2_s_ext_ctrls (struct v4l2_fh *fh,struct v4l2_ctrl_handler *hdl, +struct video_device *vdev,struct media_device *mdev,struct v4l2_ext_controls *c);
include/media/v4l2-ioctl.h +2 -0
@@ -602,6 +602,8 @@ struct v4l2_ioctl_ops {#define V4L2_DEV_DEBUG_STREAMING 0x08/* Log poll() */ #define V4L2_DEV_DEBUG_POLL 0x10 +/* Log controls */ +#define V4L2_DEV_DEBUG_CTRL 0x20/* Video standard functions */
Signature · 1 line
2.20.1
Quoted 60 lines · Ezequiel Garcia
Currently, the v4l2 control code is a bit silent on errors. Add debug messages on (hopefully) most of the error paths.
Signed-off-by Ezequiel Garcia <[email protected]>---
Changes from v1:
* Drop changes in the debug parameter semantics.
* Drop new module debug parameter.
* Add documentation.
* Add a debug error in all places where control can fail.
* Reorder the vdev parameter, to make the patch less invasive.--- Documentation/media/kapi/v4l2-dev.rst | 1 + drivers/media/platform/omap3isp/ispvideo.c | 2 +- drivers/media/v4l2-core/v4l2-ctrls.c | 106 ++++++++++++++++----- drivers/media/v4l2-core/v4l2-ioctl.c | 12 +-- drivers/media/v4l2-core/v4l2-subdev.c | 6 +- include/media/v4l2-ctrls.h | 9 +- include/media/v4l2-ioctl.h | 2 + 7 files changed, 100 insertions(+), 38 deletions(-)Documentation/media/kapi/v4l2-dev.rst +1
-0@@ -288,6 +288,7 @@ Mask Description 0x08 Log the read and write file operations and the VIDIOC_QBUF and VIDIOC_DQBUF ioctls. 0x10 Log the poll file operation. +0x20 Log error and messages in the control operations. ===== ================================================================ Video device cleanupdrivers/media/platform/omap3isp/ispvideo.c +1
-1@@ -1028,7 +1028,7 @@ static int isp_video_check_external_subdevs(struct isp_video *video, ctrls.count =1 ; ctrls.controls = &ctrl; - ret =v4l2_g_ext_ctrls (pipe->external->ctrl_handler,NULL , &ctrls); + ret =v4l2_g_ext_ctrls (pipe->external->ctrl_handler, &video->video,NULL , &ctrls);if (ret <0 ) {dev_warn (isp->dev,"no pixel rate control in subdev %s\n" , pipe->external->name);drivers/media/v4l2-core/v4l2-ctrls.c +2
-0@@ -6,6 +6,8 @@*/ +#define pr_fmt (fmt) "v4l2-ctrls: " fmt +#include <linux/ctype.h> #include <linux/mm.h> #include <linux/slab.h>
drivers/media/v4l2-core/v4l2-ctrls.c line 22
Quoted 5 lines
drivers/media/v4l2-core/v4l2-ctrls.c +2
-0@@ -16,6 +18,12 @@#include <media/v4l2-event.h> #include <media/v4l2-dev.h> +#define dprintk(vdev, fmt, arg...) do { \ + if (vdev->dev_debug & V4L2_DEV_DEBUG_CTRL) \
This isn't very robust if vdev is NULL. I know it shouldn't, but it's better to check for this.
I'd use this instead:
if (!WARN_ON(!vdev) && ((vdev)->dev_debug & V4L2_DEV_DEBUG_CTRL)) \
That way nothing crashes if vdev is NULL, and you get a warning as well in that case.
Regards,
Hans
Quoted 395 lines
drivers/media/v4l2-core/v4l2-ctrls.c +76
-26+ printk(KERN_DEBUG pr_fmt("%s: %s: " fmt), \ + __func__, video_device_node_name(vdev), ##arg); \ +} while (0) +#define has_op (master, op) \ (master->ops && master->ops->op)#define call_op (master, op) \@@ -3211,6 +3219,7 @@ static int v4l2_ctrl_request_bind(struct media_request *req,static int prepare_ext_ctrls (struct v4l2_ctrl_handler *hdl,struct v4l2_ext_controls *cs,struct v4l2_ctrl_helper *helpers, +struct video_device *vdev,bool get) {struct v4l2_ctrl_helper *h;@@ -3228,20 +3237,28 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,if (cs->which && cs->which !=V4L2_CTRL_WHICH_DEF_VAL && cs->which !=V4L2_CTRL_WHICH_REQUEST_VAL && -V4L2_CTRL_ID2WHICH (id) != cs->which) +V4L2_CTRL_ID2WHICH (id) != cs->which) { +dprintk (vdev,"invalid which 0x%x or control id 0x%x\n" , cs->which, id);return -EINVAL ; + }/* Old-style private controls are not allowed for extended controls */ -if (id >=V4L2_CID_PRIVATE_BASE ) +if (id >=V4L2_CID_PRIVATE_BASE ) { +dprintk (vdev,"old-style private controls not allowed for extended controls\n" );return -EINVAL ; + } ref =find_ref_lock (hdl, id); -if (ref ==NULL ) +if (ref ==NULL ) { +dprintk (vdev,"cannot find control id 0x%x\n" , id);return -EINVAL ; + } h->ref = ref; ctrl = ref->ctrl; -if (ctrl->flags &V4L2_CTRL_FLAG_DISABLED ) +if (ctrl->flags &V4L2_CTRL_FLAG_DISABLED ) { +dprintk (vdev,"control id 0x%x is disabled\n" , id);return -EINVAL ; + }if (ctrl->cluster[0 ]->ncontrols >1 ) have_clusters = true;@@ -3251,10 +3268,16 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,unsigned tot_size = ctrl->elems * ctrl->elem_size;if (c->size < tot_size) { +/* +* In the get case the application first queries +* to obtain the size of the control. +*/ if (get) { c->size = tot_size;return -ENOSPC ; } +dprintk (vdev,"pointer control id 0x%x size too small, %d bytes but %d bytes needed\n" , + id, c->size, tot_size);return -EFAULT ; } c->size = tot_size;@@ -3315,7 +3338,8 @@ static int class_check(struct v4l2_ctrl_handler *hdl, u32 which)/* Get extended controls. Allocates the helpers array if needed. */ static int v4l2_g_ext_ctrls_common (struct v4l2_ctrl_handler *hdl, -struct v4l2_ext_controls *cs) +struct v4l2_ext_controls *cs, +struct video_device *vdev) {struct v4l2_ctrl_helper helper[4 ];struct v4l2_ctrl_helper *helpers = helper;@@ -3341,7 +3365,7 @@ static int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl,return -ENOMEM ; } - ret =prepare_ext_ctrls (hdl, cs, helpers, true); + ret =prepare_ext_ctrls (hdl, cs, helpers, vdev, true); cs->error_idx = cs->count;for (i =0 ; !ret && i < cs->count; i++)@@ -3434,8 +3458,8 @@ v4l2_ctrls_find_req_obj(struct v4l2_ctrl_handler *hdl,return obj; } -int v4l2_g_ext_ctrls (struct v4l2_ctrl_handler *hdl,struct media_device *mdev, -struct v4l2_ext_controls *cs) +int v4l2_g_ext_ctrls (struct v4l2_ctrl_handler *hdl,struct video_device *vdev, +struct media_device *mdev,struct v4l2_ext_controls *cs) {struct media_request_object *obj =NULL ;struct media_request *req =NULL ;@@ -3471,7 +3495,7 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev, req_obj); } - ret =v4l2_g_ext_ctrls_common (hdl, cs); + ret =v4l2_g_ext_ctrls_common (hdl, cs, vdev);if (obj) {media_request_unlock_for_access (req);@@ -3614,7 +3638,9 @@ static int try_or_set_cluster(struct v4l2_fh *fh, struct v4l2_ctrl *master,/* Validate controls. */ static int validate_ctrls (struct v4l2_ext_controls *cs, -struct v4l2_ctrl_helper *helpers,bool set) +struct v4l2_ctrl_helper *helpers, +struct video_device *vdev, +bool set) {unsigned i;int ret =0 ;@@ -3626,16 +3652,20 @@ static int validate_ctrls(struct v4l2_ext_controls *cs, cs->error_idx = i; -if (ctrl->flags &V4L2_CTRL_FLAG_READ_ONLY ) +if (ctrl->flags &V4L2_CTRL_FLAG_READ_ONLY ) { +dprintk (vdev,"control id 0x%x is read-only\n" , ctrl->id);return -EACCES ; + }/* This test is also done in try_set_control_cluster() which is called in atomic context, so that has the final say, but it makes sense to do an up-front check as well. Once an error occurs in try_set_control_cluster() some other controls may have been set already and we want to do a best-effort to avoid that. */ -if (set && (ctrl->flags &V4L2_CTRL_FLAG_GRABBED )) +if (set && (ctrl->flags &V4L2_CTRL_FLAG_GRABBED )) { +dprintk (vdev,"control id 0x%x is grabbed, cannot set\n" , ctrl->id);return -EBUSY ; + }/* * Skip validation for now if the payload needs to be copied * from userspace into kernelspace. We'll validate those later. @@ -3670,7 +3700,8 @@ static void update_from_auto_cluster(struct v4l2_ctrl *master)/* Try or try-and-set controls */ static int try_set_ext_ctrls_common (struct v4l2_fh *fh,struct v4l2_ctrl_handler *hdl, -struct v4l2_ext_controls *cs,bool set) +struct v4l2_ext_controls *cs, +struct video_device *vdev,bool set) {struct v4l2_ctrl_helper helper[4 ];struct v4l2_ctrl_helper *helpers = helper;@@ -3680,13 +3711,17 @@ static int try_set_ext_ctrls_common(struct v4l2_fh *fh, cs->error_idx = cs->count;/* Default value cannot be changed */ -if (cs->which ==V4L2_CTRL_WHICH_DEF_VAL ) +if (cs->which ==V4L2_CTRL_WHICH_DEF_VAL ) { +dprintk (vdev,"%s: cannot change default value\n" ,video_device_node_name (vdev));return -EINVAL ; + } cs->which =V4L2_CTRL_ID2WHICH (cs->which); -if (hdl ==NULL ) +if (hdl ==NULL ) { +dprintk (vdev,"%s: invalid null control handler\n" ,video_device_node_name (vdev));return -EINVAL ; + }if (cs->count ==0 )return class_check (hdl, cs->which);@@ -3697,9 +3732,9 @@ static int try_set_ext_ctrls_common(struct v4l2_fh *fh,if (!helpers)return -ENOMEM ; } - ret =prepare_ext_ctrls (hdl, cs, helpers, false); + ret =prepare_ext_ctrls (hdl, cs, helpers, vdev, false);if (!ret) - ret =validate_ctrls (cs, helpers, set); + ret =validate_ctrls (cs, helpers, vdev, set);if (ret && set) cs->error_idx = cs->count;for (i =0 ; !ret && i < cs->count; i++) {@@ -3784,7 +3819,8 @@ static int try_set_ext_ctrls_common(struct v4l2_fh *fh, }static int try_set_ext_ctrls (struct v4l2_fh *fh, -struct v4l2_ctrl_handler *hdl,struct media_device *mdev, +struct v4l2_ctrl_handler *hdl, +struct video_device *vdev,struct media_device *mdev,struct v4l2_ext_controls *cs,bool set) {struct media_request_object *obj =NULL ;@@ -3792,21 +3828,32 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,int ret;if (cs->which ==V4L2_CTRL_WHICH_REQUEST_VAL ) { -if (!mdev || cs->request_fd <0 ) +if (!mdev) { +dprintk (vdev,"%s: missing media device\n" ,video_device_node_name (vdev)); +return -EINVAL ; + } + +if (cs->request_fd <0 ) { +dprintk (vdev,"%s: invalid request fd %d\n" ,video_device_node_name (vdev), cs->request_fd);return -EINVAL ; + } req =media_request_get_by_fd (mdev, cs->request_fd); -if (IS_ERR (req)) +if (IS_ERR (req)) { +dprintk (vdev,"%s: cannot find request fd %d\n" ,video_device_node_name (vdev), cs->request_fd);return PTR_ERR (req); + } ret =media_request_lock_for_update (req);if (ret) { +dprintk (vdev,"%s: cannot lock request fd %d\n" ,video_device_node_name (vdev), cs->request_fd);media_request_put (req);return ret; } obj =v4l2_ctrls_find_req_obj (hdl, req, set);if (IS_ERR (obj)) { +dprintk (vdev,"%s: cannot find request object for request fd %d\n" ,video_device_node_name (vdev), cs->request_fd);media_request_unlock_for_update (req);media_request_put (req);return PTR_ERR (obj);@@ -3815,7 +3862,9 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, req_obj); } - ret =try_set_ext_ctrls_common (fh, hdl, cs, set); + ret =try_set_ext_ctrls_common (fh, hdl, cs, vdev, set); +if (ret) +dprintk (vdev,"%s: try_set_ext_ctrls_common failed (%d)\n" ,video_device_node_name (vdev), ret);if (obj) {media_request_unlock_for_update (req);@@ -3826,17 +3875,22 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,return ret; } -int v4l2_try_ext_ctrls (struct v4l2_ctrl_handler *hdl,struct media_device *mdev, +int v4l2_try_ext_ctrls (struct v4l2_ctrl_handler *hdl, +struct video_device *vdev, +struct media_device *mdev,struct v4l2_ext_controls *cs) { -return try_set_ext_ctrls (NULL , hdl, mdev, cs, false); +return try_set_ext_ctrls (NULL , hdl, vdev, mdev, cs, false); }EXPORT_SYMBOL (v4l2_try_ext_ctrls); -int v4l2_s_ext_ctrls (struct v4l2_fh *fh,struct v4l2_ctrl_handler *hdl, -struct media_device *mdev,struct v4l2_ext_controls *cs) +int v4l2_s_ext_ctrls (struct v4l2_fh *fh, +struct v4l2_ctrl_handler *hdl, +struct video_device *vdev, +struct media_device *mdev, +struct v4l2_ext_controls *cs) { -return try_set_ext_ctrls (fh, hdl, mdev, cs, true); +return try_set_ext_ctrls (fh, hdl, vdev, mdev, cs, true); }EXPORT_SYMBOL (v4l2_s_ext_ctrls);drivers/media/v4l2-core/v4l2-ioctl.c +6
-6@@ -2165,9 +2165,9 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops, p->error_idx = p->count;if (vfh && vfh->ctrl_handler) -return v4l2_g_ext_ctrls (vfh->ctrl_handler, vfd->v4l2_dev->mdev, p); +return v4l2_g_ext_ctrls (vfh->ctrl_handler, vfd, vfd->v4l2_dev->mdev, p);if (vfd->ctrl_handler) -return v4l2_g_ext_ctrls (vfd->ctrl_handler, vfd->v4l2_dev->mdev, p); +return v4l2_g_ext_ctrls (vfd->ctrl_handler, vfd, vfd->v4l2_dev->mdev, p);if (ops->vidioc_g_ext_ctrls ==NULL )return -ENOTTY ; return check_ext_ctrls(p, 0) ? ops->vidioc_g_ext_ctrls(file, fh, p) :@@ -2184,9 +2184,9 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops, p->error_idx = p->count;if (vfh && vfh->ctrl_handler) -return v4l2_s_ext_ctrls (vfh, vfh->ctrl_handler, vfd->v4l2_dev->mdev, p); +return v4l2_s_ext_ctrls (vfh, vfh->ctrl_handler, vfd, vfd->v4l2_dev->mdev, p);if (vfd->ctrl_handler) -return v4l2_s_ext_ctrls (NULL , vfd->ctrl_handler, vfd->v4l2_dev->mdev, p); +return v4l2_s_ext_ctrls (NULL , vfd->ctrl_handler, vfd, vfd->v4l2_dev->mdev, p);if (ops->vidioc_s_ext_ctrls ==NULL )return -ENOTTY ; return check_ext_ctrls(p, 0) ? ops->vidioc_s_ext_ctrls(file, fh, p) :@@ -2203,9 +2203,9 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops, p->error_idx = p->count;if (vfh && vfh->ctrl_handler) -return v4l2_try_ext_ctrls (vfh->ctrl_handler, vfd->v4l2_dev->mdev, p); +return v4l2_try_ext_ctrls (vfh->ctrl_handler, vfd, vfd->v4l2_dev->mdev, p);if (vfd->ctrl_handler) -return v4l2_try_ext_ctrls (vfd->ctrl_handler, vfd->v4l2_dev->mdev, p); +return v4l2_try_ext_ctrls (vfd->ctrl_handler, vfd, vfd->v4l2_dev->mdev, p);if (ops->vidioc_try_ext_ctrls ==NULL )return -ENOTTY ; return check_ext_ctrls(p, 0) ? ops->vidioc_try_ext_ctrls(file, fh, p) :drivers/media/v4l2-core/v4l2-subdev.c +3
-3@@ -211,19 +211,19 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)if (!vfh->ctrl_handler)return -ENOTTY ;return v4l2_g_ext_ctrls (vfh->ctrl_handler, - sd->v4l2_dev->mdev, arg); + vdev, sd->v4l2_dev->mdev, arg);case VIDIOC_S_EXT_CTRLS :if (!vfh->ctrl_handler)return -ENOTTY ;return v4l2_s_ext_ctrls (vfh, vfh->ctrl_handler, - sd->v4l2_dev->mdev, arg); + vdev, sd->v4l2_dev->mdev, arg);case VIDIOC_TRY_EXT_CTRLS :if (!vfh->ctrl_handler)return -ENOTTY ;return v4l2_try_ext_ctrls (vfh->ctrl_handler, - sd->v4l2_dev->mdev, arg); + vdev, sd->v4l2_dev->mdev, arg);case VIDIOC_DQEVENT :if (!(sd->flags &V4L2_SUBDEV_FL_HAS_EVENTS ))include/media/v4l2-ctrls.h +7
-2@@ -1265,25 +1265,28 @@ int v4l2_s_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,* :ref:`VIDIOC_G_EXT_CTRLS <vidioc_g_ext_ctrls>` ioctl * * @hdl: pointer to &struct v4l2_ctrl_handler +* @vdev: pointer to &struct video_device * @mdev: pointer to &struct media_device * @c: pointer to &struct v4l2_ext_controls * * If hdl == NULL then they will all return -EINVAL. */ -int v4l2_g_ext_ctrls (struct v4l2_ctrl_handler *hdl,struct media_device *mdev, -struct v4l2_ext_controls *c); +int v4l2_g_ext_ctrls (struct v4l2_ctrl_handler *hdl,struct video_device *vdev, +struct media_device *mdev,struct v4l2_ext_controls *c);/** * v4l2_try_ext_ctrls - Helper function to implement * :ref:`VIDIOC_TRY_EXT_CTRLS <vidioc_g_ext_ctrls>` ioctl * * @hdl: pointer to &struct v4l2_ctrl_handler +* @vdev: pointer to &struct video_device * @mdev: pointer to &struct media_device * @c: pointer to &struct v4l2_ext_controls * * If hdl == NULL then they will all return -EINVAL. */ int v4l2_try_ext_ctrls (struct v4l2_ctrl_handler *hdl, +struct video_device *vdev,struct media_device *mdev,struct v4l2_ext_controls *c);@@ -1293,12 +1296,14 @@ int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl,* * @fh: pointer to &struct v4l2_fh * @hdl: pointer to &struct v4l2_ctrl_handler +* @vdev: pointer to &struct video_device * @mdev: pointer to &struct media_device * @c: pointer to &struct v4l2_ext_controls * * If hdl == NULL then they will all return -EINVAL. */ int v4l2_s_ext_ctrls (struct v4l2_fh *fh,struct v4l2_ctrl_handler *hdl, +struct video_device *vdev,struct media_device *mdev,struct v4l2_ext_controls *c);include/media/v4l2-ioctl.h +2
-0@@ -602,6 +602,8 @@ struct v4l2_ioctl_ops {#define V4L2_DEV_DEBUG_STREAMING 0x08/* Log poll() */ #define V4L2_DEV_DEBUG_POLL 0x10 +/* Log controls */ +#define V4L2_DEV_DEBUG_CTRL 0x20/* Video standard functions */
Quoted 70 lines · Hans Verkuil
Quoted 60 lines · Ezequiel Garcia
Currently, the v4l2 control code is a bit silent on errors. Add debug messages on (hopefully) most of the error paths.
Signed-off-by Ezequiel Garcia <[email protected]>---
Changes from v1:
* Drop changes in the debug parameter semantics.
* Drop new module debug parameter.
* Add documentation.
* Add a debug error in all places where control can fail.
* Reorder the vdev parameter, to make the patch less invasive.--- Documentation/media/kapi/v4l2-dev.rst | 1 + drivers/media/platform/omap3isp/ispvideo.c | 2 +- drivers/media/v4l2-core/v4l2-ctrls.c | 106 ++++++++++++++++----- drivers/media/v4l2-core/v4l2-ioctl.c | 12 +-- drivers/media/v4l2-core/v4l2-subdev.c | 6 +- include/media/v4l2-ctrls.h | 9 +- include/media/v4l2-ioctl.h | 2 + 7 files changed, 100 insertions(+), 38 deletions(-)Documentation/media/kapi/v4l2-dev.rst +1
-0@@ -288,6 +288,7 @@ Mask Description 0x08 Log the read and write file operations and the VIDIOC_QBUF and VIDIOC_DQBUF ioctls. 0x10 Log the poll file operation. +0x20 Log error and messages in the control operations. ===== ================================================================ Video device cleanupdrivers/media/platform/omap3isp/ispvideo.c +1
-1@@ -1028,7 +1028,7 @@ static int isp_video_check_external_subdevs(struct isp_video *video, ctrls.count =1 ; ctrls.controls = &ctrl; - ret =v4l2_g_ext_ctrls (pipe->external->ctrl_handler,NULL , &ctrls); + ret =v4l2_g_ext_ctrls (pipe->external->ctrl_handler, &video->video,NULL , &ctrls);if (ret <0 ) {dev_warn (isp->dev,"no pixel rate control in subdev %s\n" , pipe->external->name);drivers/media/v4l2-core/v4l2-ctrls.c +2
-0@@ -6,6 +6,8 @@*/ +#define pr_fmt (fmt) "v4l2-ctrls: " fmt +#include <linux/ctype.h> #include <linux/mm.h> #include <linux/slab.h> drivers/media/v4l2-core/v4l2-ctrls.c line 22
Quoted 5 lines
drivers/media/v4l2-core/v4l2-ctrls.c +2
-0@@ -16,6 +18,12 @@#include <media/v4l2-event.h> #include <media/v4l2-dev.h> +#define dprintk(vdev, fmt, arg...) do { \ + if (vdev->dev_debug & V4L2_DEV_DEBUG_CTRL) \This isn't very robust if vdev is NULL. I know it shouldn't, but it's better to check for this.
Quoted 6 lines
I'd use this instead:
if (!WARN_ON(!vdev) && ((vdev)->dev_debug & V4L2_DEV_DEBUG_CTRL)) \That way nothing crashes if vdev is NULL, and you get a warning as well in that case.
OK, no problem.
Thanks,
Eze