From 173f6eacc8a89ae4b08b166735930e6c46951a81 Mon Sep 17 00:00:00 2001 From: Ezequiel Garcia Date: Sat, 20 Jul 2019 07:47:07 -0400 Subject: [PATCH] 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. Signed-off-by: Ezequiel Garcia Signed-off-by: Hans Verkuil Signed-off-by: Mauro Carvalho Chehab --- Documentation/media/kapi/v4l2-dev.rst | 1 + drivers/media/platform/omap3isp/ispvideo.c | 4 +- drivers/media/v4l2-core/v4l2-ctrls.c | 126 ++++++++++++++++----- drivers/media/v4l2-core/v4l2-ioctl.c | 18 ++- drivers/media/v4l2-core/v4l2-subdev.c | 6 +- include/media/v4l2-ctrls.h | 9 +- include/media/v4l2-ioctl.h | 2 + 7 files changed, 127 insertions(+), 39 deletions(-) diff --git a/Documentation/media/kapi/v4l2-dev.rst b/Documentation/media/kapi/v4l2-dev.rst index b359f1804bbe..4c5a15c53dbf 100644 --- a/Documentation/media/kapi/v4l2-dev.rst +++ b/Documentation/media/kapi/v4l2-dev.rst @@ -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 diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c index b52c6fe825ac..ee183c35ff3b 100644 --- a/drivers/media/platform/omap3isp/ispvideo.c +++ b/drivers/media/platform/omap3isp/ispvideo.c @@ -1020,8 +1020,8 @@ 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); diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c index 13236c191796..76fa2db0e8fb 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls.c +++ b/drivers/media/v4l2-core/v4l2-ctrls.c @@ -6,6 +6,8 @@ */ +#define pr_fmt(fmt) "v4l2-ctrls: " fmt + #include #include #include @@ -16,6 +18,12 @@ #include #include +#define dprintk(vdev, fmt, arg...) do { \ + if (!WARN_ON(!(vdev)) && ((vdev)->dev_debug & V4L2_DEV_DEBUG_CTRL)) \ + 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) \ @@ -3260,6 +3268,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; @@ -3277,20 +3286,31 @@ 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\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; @@ -3300,10 +3320,17 @@ 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; @@ -3364,7 +3391,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; @@ -3390,7 +3418,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++) @@ -3483,8 +3511,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; @@ -3520,7 +3548,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); @@ -3663,7 +3691,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; @@ -3675,16 +3705,24 @@ 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. @@ -3719,7 +3757,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; @@ -3729,13 +3768,19 @@ 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); @@ -3746,9 +3791,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++) { @@ -3833,7 +3878,9 @@ 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; @@ -3841,21 +3888,39 @@ 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); @@ -3864,7 +3929,11 @@ 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); @@ -3875,17 +3944,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); diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index e36629ae2203..38765af9ad2d 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -2186,9 +2186,11 @@ 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) : @@ -2205,9 +2207,11 @@ 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) : @@ -2224,9 +2228,11 @@ 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) : diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 25c73c13cc7e..f725cd9b66b9 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -372,19 +372,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)) diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h index 6e9dc9c44bb1..570ff4b0205a 100644 --- a/include/media/v4l2-ctrls.h +++ b/include/media/v4l2-ctrls.h @@ -1268,25 +1268,28 @@ int v4l2_s_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl, * :ref:`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 ` 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); @@ -1296,12 +1299,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); diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h index 400f2e46c108..4bba65a59d46 100644 --- a/include/media/v4l2-ioctl.h +++ b/include/media/v4l2-ioctl.h @@ -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 */