Re: [PATCH v9 19/22] uvcvideo: uvc_ctrl_is_accessible: check for INACTIVE
Hello Hans
On Fri, Mar 26, 2021 at 11:01 AM Ricardo Ribalda wrote:
>
> From: Hans Verkuil
>
> Check for inactive controls in uvc_ctrl_is_accessible().
> Use the new value for the master_id controls if present,
> otherwise use the existing value to determine if it is OK
> to set the control. Doing this here avoids attempting to
> set an inactive control, which will return an error from the
> USB device.
>
> Signed-off-by: Hans Verkuil
> ---
> drivers/media/usb/uvc/uvc_ctrl.c | 28 +++-
> drivers/media/usb/uvc/uvc_v4l2.c | 4 ++--
> drivers/media/usb/uvc/uvcvideo.h | 3 ++-
> 3 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> b/drivers/media/usb/uvc/uvc_ctrl.c
> index d9d4add1e813..6e7b904bc33d 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1047,10 +1047,18 @@ static int uvc_query_v4l2_class(struct
> uvc_video_chain *chain, u32 req_id,
> }
>
> int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
> - bool read)
> + const struct v4l2_ext_controls *ctrls,
> + unsigned long ioctl)
> {
> + struct uvc_control_mapping *master_map = NULL;
> + struct uvc_control *master_ctrl = NULL;
> struct uvc_control_mapping *mapping;
> struct uvc_control *ctrl;
> + bool read = ioctl == VIDIOC_G_EXT_CTRLS;
> + bool try = ioctl == VIDIOC_TRY_EXT_CTRLS;
> + s32 val;
> + int ret;
> + int i;
>
> if (__uvc_query_v4l2_class(chain, v4l2_id, 0) >= 0)
> return -EACCES;
> @@ -1065,6 +1073,24 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain
> *chain, u32 v4l2_id,
> if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR) && !read)
> return -EACCES;
>
> + if (read || try || !mapping->master_id)
> + return 0;
> +
> + for (i = ctrls->count - 1; i >= 0; i--)
> + if (ctrls->controls[i].id == mapping->master_id)
> + return ctrls->controls[i].value ==
> + mapping->master_manual ? 0 : -EACCES;
> +
> + __uvc_find_control(ctrl->entity, mapping->master_id, _map,
> + _ctrl, 0);
> +
> + if (!master_ctrl || !(master_ctrl->info.flags &
> UVC_CTRL_FLAG_GET_CUR))
> + return 0;
> +
> + ret = __uvc_ctrl_get(chain, master_ctrl, master_map, );
> + if (ret >= 0 && val != mapping->master_manual)
> + return -EACCES;
Maybe we could use uvc_ctrl_is_inactive().
Please checkout:
https://git.kernel.org/pub/scm/linux/kernel/git/ribalda/linux.git/commit/?h=uvc-compliance-v10=f49f7c84ece18df72094a18011f2fbeb20dc1b15
and if you like it better that will be what I resend for v10 ;)
Thanks!
> +
> return 0;
> }
>
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> b/drivers/media/usb/uvc/uvc_v4l2.c
> index 8d8b12a4db34..0f4d893eff46 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1000,8 +1000,8 @@ static int uvc_ctrl_check_access(struct uvc_video_chain
> *chain,
> int ret = 0;
>
> for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> - ret = uvc_ctrl_is_accessible(chain, ctrl->id,
> - ioctl == VIDIOC_G_EXT_CTRLS);
> + ret = uvc_ctrl_is_accessible(chain, ctrl->id, ctrls,
> + ioctl);
> if (ret)
> break;
> }
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> b/drivers/media/usb/uvc/uvcvideo.h
> index 0313b30f0cea..20bc681315fb 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -901,7 +901,8 @@ static inline int uvc_ctrl_rollback(struct uvc_fh *handle)
> int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control
> *xctrl);
> int uvc_ctrl_set(struct uvc_fh *handle, struct v4l2_ext_control *xctrl);
> int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
> - bool read);
> + const struct v4l2_ext_controls *ctrls,
> + unsigned long ioctl);
>
> int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
> struct uvc_xu_control_query *xqry);
> --
> 2.31.0.291.g576ba9dcdaf-goog
>
--
Ricardo Ribalda
Re: [PATCH v7 15/17] media: uvcvideo: Refactor __uvc_ctrl_commit
Hello Hans
On Fri, Mar 19, 2021 at 9:35 AM Hans Verkuil wrote:
>
> On 18/03/2021 21:29, Ricardo Ribalda wrote:
> > Take a v4l2_ext_controls instead of an array of controls, this way we
> > can access the error_idx in future changes.
> >
> > Signed-off-by: Ricardo Ribalda
> > ---
> > drivers/media/usb/uvc/uvc_ctrl.c | 5 ++---
> > drivers/media/usb/uvc/uvc_v4l2.c | 8 ++--
> > drivers/media/usb/uvc/uvcvideo.h | 10 --
> > 3 files changed, 12 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 1ec8333811bc..fb8155ca0c0d 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1664,8 +1664,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device
> > *dev,
> > }
> >
> > int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
> > - const struct v4l2_ext_control *xctrls,
> > - unsigned int xctrls_count)
> > + struct v4l2_ext_controls *ctrls)
> > {
> > struct uvc_video_chain *chain = handle->chain;
> > struct uvc_entity *entity;
> > @@ -1679,7 +1678,7 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int
> > rollback,
> > }
> >
> > if (!rollback)
> > - uvc_ctrl_send_events(handle, xctrls, xctrls_count);
> > + uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count);
> > done:
> > mutex_unlock(>ctrl_mutex);
> > return ret;
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> > b/drivers/media/usb/uvc/uvc_v4l2.c
> > index ddebdeb5a81b..ea2c41b04664 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -1025,6 +1025,10 @@ static int uvc_ioctl_s_ctrl(struct file *file, void
> > *fh,
> > struct uvc_fh *handle = fh;
> > struct uvc_video_chain *chain = handle->chain;
> > struct v4l2_ext_control xctrl;
> > + struct v4l2_ext_controls ctrls = {
> > + .count = 1,
> > + .controls = ,
> > + };
>
> Rather than creating this extra ctrls struct, I think this can be simplified
> by just
> removing uvc_ioctl_s_ctrl and uvc_ioctl_g_ctrl altogether. The v4l2-ioctl.c
> source
> will call vidioc_g/s_ext_ctrls if the driver doesn't provide vidioc_g/s_ctrl
> ops.
>
> Let's just simplify this by adding another patch before this one that just
> removes
> uvc_ioctl_s_ctrl and uvc_ioctl_g_ctrl.
>
> Otherwise this patch looks good.
I think I have found a 13 year old bug.
https://git.kernel.org/pub/scm/linux/kernel/git/ribalda/linux.git/commit/?h=uvc-compliance-v8=7034e9ed5a8203c73cef9ab972ece48ade70b22f
>
> Regards,
>
> Hans
>
> > int ret;
> >
> > ret = uvc_ctrl_is_accessible(chain, ctrl->id, VIDIOC_S_CTRL);
> > @@ -1045,7 +1049,7 @@ static int uvc_ioctl_s_ctrl(struct file *file, void
> > *fh,
> > return ret;
> > }
> >
> > - ret = uvc_ctrl_commit(handle, , 1);
> > + ret = uvc_ctrl_commit(handle, );
> > if (ret < 0)
> > return ret;
> >
> > @@ -1149,7 +1153,7 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh
> > *handle,
> > ctrls->error_idx = 0;
> >
> > if (ioctl == VIDIOC_S_EXT_CTRLS)
> > - return uvc_ctrl_commit(handle, ctrls->controls, ctrls->count);
> > + return uvc_ctrl_commit(handle, ctrls);
> > else
> > return uvc_ctrl_rollback(handle);
> > }
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h
> > b/drivers/media/usb/uvc/uvcvideo.h
> > index a93aeedb5499..4e7b6da3c6d2 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -887,17 +887,15 @@ void uvc_ctrl_status_event(struct uvc_video_chain
> > *chain,
> >
> > int uvc_ctrl_begin(struct uvc_video_chain *chain);
> > int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
> > - const struct v4l2_ext_control *xctrls,
> > - unsigned int xctrls_count);
> > + struct v4l2_ext_controls *ctrls);
> > static inline int uvc_ctrl_commit(struct uvc_fh *handle,
> > - const struct v4l2_ext_control *xctrls,
> > - unsigned int xctrls_count)
> > + struct v4l2_ext_controls *ctrls)
> > {
> > - return __uvc_ctrl_commit(handle, 0, xctrls, xctrls_count);
> > + return __uvc_ctrl_commit(handle, 0, ctrls);
> > }
> > static inline int uvc_ctrl_rollback(struct uvc_fh *handle)
> > {
> > - return __uvc_ctrl_commit(handle, 1, NULL, 0);
> > + return __uvc_ctrl_commit(handle, 1, NULL);
> > }
> >
> > int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control
> > *xctrl);
> >
>
--
Ricardo Ribalda
Re: [PATCHv2 2/3] media: uvcvideo: add ROI auto controls
On Wed, Mar 17, 2021 at 2:34 AM Sergey Senozhatsky
wrote:
>
> On (21/03/16 19:29), Ricardo Ribalda Delgado wrote:
> > > ROI control is a compound data type:
> > > Control Selector CT_REGION_OF_INTEREST_CONTROL
> > > Mandatory Requests SET_CUR, GET_CUR, GET_MIN, GET_MAX, GET_DEF
> > > wLength 10
> > > Offset FieldSize
> > > 0wROI_Top 2
> > > 2wROI_Left2
> > > 4wROI_Bottom 2
> > > 6wROI_Right 2
> > > 8bmAutoControls 2 (Bitmap)
> > >
> > > uvc_control_mapping, however, can handle only s32 data type at the
> > > moment: ->get() returns s32 value, ->set() accepts s32 value; while
> > > v4l2_ctrl maximum/minimum/default_value can hold only s64 values.
> > >
> > > Hence ROI control handling is split into two patches:
> > > a) bmAutoControls is handled via uvc_control_mapping as
> > > V4L2_CTRL_TYPE_MENU
> > > b) ROI rectangle (SET_CUR, GET_CUR, GET_DEF) handling is implemented
> > >separately, by the means of selection API.
> >
> > Maybe a reference to the uvc doc would be a good thing to add here.
>
> OK. What should be referenced there?
Nevermind, I thought there was a non-pdf version of the standard :(
https://www.usb.org/document-library/video-class-v15-document-set
>
> > > +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_EXPOSURE``
> > > + - Auto Exposure.
> > > +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_IRIS``
> > > + - Auto Iris.
> > > +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_WHITE_BALANCE``
> > > + - Auto White Balance.
> > > +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_FOCUS``
> > > + - Auto Focus.
> > > +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_FACE_DETECT``
> > > + - Auto Face Detect.
> > > +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK``
> > > + - Auto Detect and Track.
> > > +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_IMAGE_STABILIXATION``
> > > + - Image Stabilization.
> > > +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY``
> > > + - Higher Quality.
> > > +
> > >
> > Nit: Same as before splitting doc and code.
>
> OK, I guess I can split.
>
> > > static const struct uvc_menu_info power_line_frequency_controls[] = {
> > > @@ -753,6 +762,16 @@ static const struct uvc_control_mapping
> > > uvc_ctrl_mappings[] = {
> > > .v4l2_type = V4L2_CTRL_TYPE_BOOLEAN,
> > > .data_type = UVC_CTRL_DATA_TYPE_BOOLEAN,
> > > },
> > > + {
> > > + .id = V4L2_CID_REGION_OF_INTEREST_AUTO,
> > > + .name = "Region of Interest (auto)",
> > > + .entity = UVC_GUID_UVC_CAMERA,
> > > + .selector = UVC_CT_REGION_OF_INTEREST_CONTROL,
> > > + .size = 16,
> > > + .offset = 64,
> > > + .v4l2_type = V4L2_CTRL_TYPE_BITMASK,
> > Are
>
> Are?
Aye!
You are not a good kernel reviewer if you do not talk pirate :P.
I wanted to make sure that V4L2_CTRL_TYPE_BITMASK was handled by the
uvc driver, and I think that it will work fine.
Sorry for the noise.
>
> > > #define V4L2_CID_PAN_SPEED
> > > (V4L2_CID_CAMERA_CLASS_BASE+32)
> > > #define V4L2_CID_TILT_SPEED
> > > (V4L2_CID_CAMERA_CLASS_BASE+33)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO
> > > (V4L2_CID_CAMERA_CLASS_BASE+34)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_EXPOSURE (1 << 0)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_IRIS (1 << 1)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_WHITE_BALANCE (1 << 2)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_FOCUS (1 << 3)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_FACE_DETECT (1 << 4)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK (1 << 5)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_IMAGE_STABILIXATION (1 << 6)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY(1 << 7)
> > >
> > > #define V4L2_CID_CAMERA_ORIENTATION
> > > (V4L2_CID_CAMERA_CLASS_BASE+34)
> > > #define V4L2_CAMERA_ORIENTATION_FRONT 0
> > > --
> > > 2.30.0
> > >
> >
> > I think we have to add the CID to v4l2_ctrl_get_name()
>
> Sounds good. Only this one - V4L2_CID_REGION_OF_INTEREST_AUTO, right?
I think so :)
Thanks!
--
Ricardo Ribalda
Re: [PATCHv2 1/3] media: v4l UAPI docs: document ROI selection targets
Hi On Wed, Mar 17, 2021 at 2:31 AM Sergey Senozhatsky wrote: > > On (21/03/16 19:19), Ricardo Ribalda Delgado wrote: > > > +Configuration of Region of Interest (ROI) > > > += > > > + > > > +The range of coordinates of the top left corner, width and height of > > > +areas that can be ROI is given by the ``V4L2_SEL_TGT_ROI_BOUNDS`` target. > > > +It is recommended for the driver developers to put the top/left corner > > > +at position ``(0,0)``. The rectangle's coordinates are in global sensor > > > +coordinates. The units are in pixels and independent of the field of > > > view. > > > +They are not impacted by any cropping or scaling that is currently being > > > +used. > > > > Can we also mention binning here? > > What's binning? Is it in the UVC spec? Binning is when you reduce an image by adding up surrounding pixels. So you have a 100x100 image that you convert to a 50x50 but showing the same area of interest. > > > > +The top left corner, width and height of the Region of Interest area > > > +currently being employed by the device is given by the > > > +``V4L2_SEL_TGT_ROI_CURRENT`` target. It uses the same coordinate system > > > +as ``V4L2_SEL_TGT_ROI_BOUNDS``. > > > > Why do we need current? Cant we just read back V4L2_SEL_TGT_ROI ? > > We don't. Will remove it. > > > > +* - ``V4L2_SEL_TGT_ROI_CURRENT`` > > > + - 0x0200 > > > + - Current Region of Interest rectangle. > > > + - Yes > > > + - No > > > +* - ``V4L2_SEL_TGT_ROI_DEFAULT`` > > > + - 0x0201 > > > + - Suggested Region of Interest rectangle. > > > + - Yes > > > + - No > > > +* - ``V4L2_SEL_TGT_ROI_BOUNDS`` > > > + - 0x0202 > > > + - Bounds of the Region of Interest rectangle. All valid ROI > > > rectangles fit > > > + inside the ROI bounds rectangle. > > > + - Yes > > > + - No > > > +* - ``V4L2_SEL_TGT_ROI`` > > > + - 0x0203 > > > + - Sets the new Region of Interest rectangle. > > > + - Yes > > > + - No > > As mentioned before I think we should not have TGT_ROI_CURRENT and TGT_ROI > > Agreed. > > > > diff --git a/include/uapi/linux/v4l2-common.h > > > b/include/uapi/linux/v4l2-common.h > > > index 7d21c1634b4d..d0c108fba638 100644 > > > --- a/include/uapi/linux/v4l2-common.h > > > +++ b/include/uapi/linux/v4l2-common.h > > > @@ -78,6 +78,14 @@ > > > #define V4L2_SEL_TGT_COMPOSE_BOUNDS0x0102 > > > /* Current composing area plus all padding pixels */ > > > #define V4L2_SEL_TGT_COMPOSE_PADDED0x0103 > > > +/* Current Region of Interest area */ > > > +#define V4L2_SEL_TGT_ROI_CURRENT 0x0200 > > > +/* Default Region of Interest area */ > > > +#define V4L2_SEL_TGT_ROI_DEFAULT 0x0201 > > > +/* Region of Interest bounds */ > > > +#define V4L2_SEL_TGT_ROI_BOUNDS0x0202 > > > +/* Set Region of Interest area */ > > > +#define V4L2_SEL_TGT_ROI 0x0203 > > > > Nit: Maybe it could be a good idea to split doc and code. This way the > > backports/fixes are easier. > > I'm quite sure this is the first time I'm being asked to split code > and documentation :) I'm usually asked to do the opposite - merge code > and documentation. I got answered in both directions. I prefer to split it because the doc can go to different audience than the code, and then it makes my life easier when backporting. But if you or Laurent prefer otherwise I am of course happy with any option ;) -- Ricardo Ribalda
Re: [PATCHv2 3/3] media: uvcvideo: add UVC 1.5 ROI control
Hi
On Wed, Mar 17, 2021 at 2:59 AM Sergey Senozhatsky
wrote:
>
> On (21/03/16 19:46), Ricardo Ribalda Delgado wrote:
> > > -static int uvc_ioctl_g_selection(struct file *file, void *fh,
> > > -struct v4l2_selection *sel)
> > > +/* UVC 1.5 ROI rectangle is half the size of v4l2_rect */
> > > +struct uvc_roi_rect {
> > > + __u16 top;
> > > + __u16 left;
> > > + __u16 bottom;
> > > + __u16 right;
> > > +};
> >
> > Perhaps __packed; ?
>
> Perhaps.
>
> > > +static int uvc_ioctl_g_selection(struct file *file, void *fh,
> > > +struct v4l2_selection *sel)
> > > +{
> > > + struct uvc_fh *handle = fh;
> > > + struct uvc_streaming *stream = handle->stream;
> > > +
> > > + if (sel->type != stream->type)
> > > + return -EINVAL;
> > > +
> > > + switch (sel->target) {
> > > + case V4L2_SEL_TGT_CROP_DEFAULT:
> > > + case V4L2_SEL_TGT_CROP_BOUNDS:
> > > + case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> > > + case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> > > + return uvc_ioctl_g_sel_target(file, fh, sel);
> > > + case V4L2_SEL_TGT_ROI_CURRENT:
> > > + case V4L2_SEL_TGT_ROI_DEFAULT:
> > > + case V4L2_SEL_TGT_ROI_BOUNDS:
> > > + return uvc_ioctl_g_roi_target(file, fh, sel);
> > > + }
> > > +
> > > + return -EINVAL;
> > > +}
> >
> > Are you sure that there is no lock needed between the control and the
> > selection API?
>
> Between V4L2_CID_REGION_OF_INTEREST_AUTO and this path?
> Hmm. They write to different offsets and don't seem to be overlapping.
>
> > > +static bool validate_roi_bounds(struct uvc_streaming *stream,
> > > + struct v4l2_selection *sel)
> > > +{
> > > + bool ok = true;
> > > +
> > > + if (sel->r.left > USHRT_MAX || sel->r.top > USHRT_MAX ||
> > > + sel->r.width > USHRT_MAX || sel->r.height > USHRT_MAX)
> > > + return false;
> > > +
> > > + /* perhaps also can test against ROI GET_MAX */
> > > +
> > > + mutex_lock(>mutex);
> [...]
> > > + if ((u16)sel->r.width > stream->cur_frame->wWidth)
> > > + ok = false;
> > > + if ((u16)sel->r.height > stream->cur_frame->wHeight)
> > > + ok = false;
>
> > Maybe you should not release this mutex until query_ctrl is done?
>
> Maybe... These two tests are somewhat made up. Not sure if we need them.
> On one hand it's reasonable to keep ROI within the frames. On the other
> hand - such relation is not spelled out in the spec. If the stream change
> its frame dimensions ROI is not getting invalidated or re-calculated by
> the firmware. UVC spec says that ROI should be bounded only by the
> CT_DIGITAL_WINDOW_CONTROL (GET_MIN / GET_MAX), ut we don't implement
> WINDOW_CONTROL.
I would remove this check completely then, and rely on set_cur,
get_cur. Leave only the < 0x1 check
>
> So maybe I can remove stream->cuf_frame boundaries check instead.
>
> > > + mutex_unlock(>mutex);
> > > +
> > > + return ok;
> > > +}
> > > +
> > > +static int uvc_ioctl_s_roi(struct file *file, void *fh,
> > > + struct v4l2_selection *sel)
> > > +{
> > > + struct uvc_fh *handle = fh;
> > > + struct uvc_streaming *stream = handle->stream;
> > > + struct uvc_roi_rect *roi;
> > > + int ret;
> > > +
> > > + if (!validate_roi_bounds(stream, sel))
> > > + return -E2BIG;
> > > +
> > > + roi = kzalloc(sizeof(struct uvc_roi_rect), GFP_KERNEL);
> > > + if (!roi)
> > > + return -ENOMEM;
> > > +
> > > + roi->left = sel->r.left;
> > > + roi->top= sel->r.top;
> > > + roi->right = sel->r.width;
> > > + roi->bottom = sel->r.height;
> > > +
> > > + ret = uvc_query_ctrl(stream->dev, UVC_SET_CUR, 1,
> > > stream->dev->intfnum,
> > > +UVC_CT_REGION_OF_INTEREST_CONTROL, roi,
Re: [PATCHv2 3/3] media: uvcvideo: add UVC 1.5 ROI control
Hi Sergey
Thanks for the patch
On Mon, Feb 8, 2021 at 6:23 AM Sergey Senozhatsky
wrote:
>
> From: Sergey Senozhatsky
>
> This patch implements parts of UVC 1.5 Region of Interest (ROI)
> control, using the uvcvideo selection API.
>
> There are several things to mention here.
>
> First, UVC 1.5 defines CT_DIGITAL_WINDOW_CONTROL; and ROI rectangle
> coordinates "must be within the current Digital Window as specified
> by the CT_WINDOW control." (4.2.2.1.20 Digital Region of Interest
> (ROI) Control.) This is not entirely clear if we need to implement
> CT_DIGITAL_WINDOW_CONTROL. ROI is naturally limited by: ROI GET_MIN
> and GET_MAX rectangles. Besides, the H/W that I'm playing with
> implements ROI, but doesn't implement CT_DIGITAL_WINDOW_CONTROL,
> so WINDOW_CONTROL is probably optional.
>
> Second, the patch doesn't implement all of the ROI requests.
> Namely, SEL_TGT_BOUNDS for ROI implements GET_MAX (that is maximal
> ROI rectangle area). GET_MIN is not implemented (as of now) since
> it's not very clear if user space would need such information.
>
> Signed-off-by: Sergey Senozhatsky
> ---
> drivers/media/usb/uvc/uvc_v4l2.c | 143 ++-
> 1 file changed, 140 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> b/drivers/media/usb/uvc/uvc_v4l2.c
> index 252136cc885c..71b4577196e5 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1139,14 +1139,60 @@ static int uvc_ioctl_querymenu(struct file *file,
> void *fh,
> return uvc_query_v4l2_menu(chain, qm);
> }
>
> -static int uvc_ioctl_g_selection(struct file *file, void *fh,
> -struct v4l2_selection *sel)
> +/* UVC 1.5 ROI rectangle is half the size of v4l2_rect */
> +struct uvc_roi_rect {
> + __u16 top;
> + __u16 left;
> + __u16 bottom;
> + __u16 right;
> +};
Perhaps __packed; ?
> +
> +static int uvc_ioctl_g_roi_target(struct file *file, void *fh,
> + struct v4l2_selection *sel)
> {
> struct uvc_fh *handle = fh;
> struct uvc_streaming *stream = handle->stream;
> + struct uvc_roi_rect *roi;
> + u8 query;
> + int ret;
>
> - if (sel->type != stream->type)
> + switch (sel->target) {
> + case V4L2_SEL_TGT_ROI_DEFAULT:
> + query = UVC_GET_DEF;
> + break;
> + case V4L2_SEL_TGT_ROI_CURRENT:
> + query = UVC_GET_CUR;
> + break;
> + case V4L2_SEL_TGT_ROI_BOUNDS:
> + query = UVC_GET_MAX;
> + break;
> + default:
> return -EINVAL;
> + }
> +
> + roi = kzalloc(sizeof(struct uvc_roi_rect), GFP_KERNEL);
> + if (!roi)
> + return -ENOMEM;
> +
> + ret = uvc_query_ctrl(stream->dev, query, 1, stream->dev->intfnum,
> +UVC_CT_REGION_OF_INTEREST_CONTROL, roi,
> +sizeof(struct uvc_roi_rect));
It is a pity that we have to alloc memory for this :(.
@Laurent, do you know a better way?
> + if (!ret) {
> + sel->r.left = roi->left;
> + sel->r.top = roi->top;
> + sel->r.width= roi->right;
> + sel->r.height = roi->bottom;
> + }
> +
> + kfree(roi);
> + return ret;
> +}
> +
> +static int uvc_ioctl_g_sel_target(struct file *file, void *fh,
> + struct v4l2_selection *sel)
> +{
> + struct uvc_fh *handle = fh;
> + struct uvc_streaming *stream = handle->stream;
>
> switch (sel->target) {
> case V4L2_SEL_TGT_CROP_DEFAULT:
> @@ -1173,6 +1219,96 @@ static int uvc_ioctl_g_selection(struct file *file,
> void *fh,
> return 0;
> }
>
> +static int uvc_ioctl_g_selection(struct file *file, void *fh,
> +struct v4l2_selection *sel)
> +{
> + struct uvc_fh *handle = fh;
> + struct uvc_streaming *stream = handle->stream;
> +
> + if (sel->type != stream->type)
> + return -EINVAL;
> +
> + switch (sel->target) {
> + case V4L2_SEL_TGT_CROP_DEFAULT:
> + case V4L2_SEL_TGT_CROP_BOUNDS:
> + case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> + case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> + return uvc_ioctl_g_sel_target(file, fh, sel);
> + case V4L2_SEL_TGT_ROI_CURRENT:
> + case V4L2_SEL_TGT_ROI_DEFAULT:
> + case V4L2_SEL_TGT_ROI_BOUNDS:
> + return uvc_ioctl_g_roi_target(file, fh, sel);
> + }
> +
> + return -EINVAL;
> +}
Are you sure that there is no lock needed between the control and the
selection API?
> +
> +static bool validate_roi_bounds(struct uvc_streaming *stream,
> + struct v4l2_selection *sel)
> +{
> + bool ok = true;
> +
> + if (sel->r.left >
Re: [PATCHv2 2/3] media: uvcvideo: add ROI auto controls
Hi Sergey
Thanks for the patch
On Mon, Feb 8, 2021 at 6:24 AM Sergey Senozhatsky
wrote:
>
> From: Sergey Senozhatsky
>
> This patch adds support for Region of Interest bmAutoControls.
>
> ROI control is a compound data type:
> Control Selector CT_REGION_OF_INTEREST_CONTROL
> Mandatory Requests SET_CUR, GET_CUR, GET_MIN, GET_MAX, GET_DEF
> wLength 10
> Offset FieldSize
> 0wROI_Top 2
> 2wROI_Left2
> 4wROI_Bottom 2
> 6wROI_Right 2
> 8bmAutoControls 2 (Bitmap)
>
> uvc_control_mapping, however, can handle only s32 data type at the
> moment: ->get() returns s32 value, ->set() accepts s32 value; while
> v4l2_ctrl maximum/minimum/default_value can hold only s64 values.
>
> Hence ROI control handling is split into two patches:
> a) bmAutoControls is handled via uvc_control_mapping as V4L2_CTRL_TYPE_MENU
> b) ROI rectangle (SET_CUR, GET_CUR, GET_DEF) handling is implemented
>separately, by the means of selection API.
Maybe a reference to the uvc doc would be a good thing to add here.
>
> Signed-off-by: Sergey Senozhatsky
> ---
> .../media/v4l/ext-ctrls-camera.rst| 25 +++
> drivers/media/usb/uvc/uvc_ctrl.c | 19 ++
> include/uapi/linux/usb/video.h| 1 +
> include/uapi/linux/v4l2-controls.h| 9 +++
> 4 files changed, 54 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> index c05a2d2c675d..1593c999c8e2 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> @@ -653,6 +653,31 @@ enum v4l2_scene_mode -
>||
>++
>
> +``V4L2_CID_REGION_OF_INTEREST_AUTO (bitmask)``
> +This determines which, if any, on board features should track to the
> +Region of Interest.
> +
> +.. flat-table::
> +:header-rows: 0
> +:stub-columns: 0
> +
> +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_EXPOSURE``
> + - Auto Exposure.
> +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_IRIS``
> + - Auto Iris.
> +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_WHITE_BALANCE``
> + - Auto White Balance.
> +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_FOCUS``
> + - Auto Focus.
> +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_FACE_DETECT``
> + - Auto Face Detect.
> +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK``
> + - Auto Detect and Track.
> +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_IMAGE_STABILIXATION``
> + - Image Stabilization.
> +* - ``V4L2_CID_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY``
> + - Higher Quality.
> +
>
Nit: Same as before splitting doc and code.
> .. [#f1]
> This control may be changed to a menu control in the future, if more
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> b/drivers/media/usb/uvc/uvc_ctrl.c
> index b3dde98499f4..5502fe540519 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -355,6 +355,15 @@ static const struct uvc_control_info uvc_ctrls[] = {
> .flags = UVC_CTRL_FLAG_GET_CUR
> | UVC_CTRL_FLAG_AUTO_UPDATE,
> },
> + {
> + .entity = UVC_GUID_UVC_CAMERA,
> + .selector = UVC_CT_REGION_OF_INTEREST_CONTROL,
> + .index = 21,
> + .size = 10,
> + .flags = UVC_CTRL_FLAG_SET_CUR |
> UVC_CTRL_FLAG_GET_CUR
> + | UVC_CTRL_FLAG_GET_MIN |
> UVC_CTRL_FLAG_GET_MAX
> + | UVC_CTRL_FLAG_GET_DEF
> + },
> };
>
> static const struct uvc_menu_info power_line_frequency_controls[] = {
> @@ -753,6 +762,16 @@ static const struct uvc_control_mapping
> uvc_ctrl_mappings[] = {
> .v4l2_type = V4L2_CTRL_TYPE_BOOLEAN,
> .data_type = UVC_CTRL_DATA_TYPE_BOOLEAN,
> },
> + {
> + .id = V4L2_CID_REGION_OF_INTEREST_AUTO,
> + .name = "Region of Interest (auto)",
> + .entity = UVC_GUID_UVC_CAMERA,
> + .selector = UVC_CT_REGION_OF_INTEREST_CONTROL,
> + .size = 16,
> + .offset = 64,
> + .v4l2_type = V4L2_CTRL_TYPE_BITMASK,
Are
> + .data_type = UVC_CTRL_DATA_TYPE_BITMASK,
> + },
> };
>
> /*
> diff --git a/include/uapi/linux/usb/video.h b/include/uapi/linux/usb/video.h
> index d854cb19c42c..c87624962896 100644
> --- a/include/uapi/linux/usb/video.h
> +++ b/include/uapi/linux/usb/video.h
> @@ -104,6 +104,7 @@
> #define
Re: [PATCHv2 1/3] media: v4l UAPI docs: document ROI selection targets
Hi Sergey Thanks for the patch! On Mon, Feb 8, 2021 at 6:21 AM Sergey Senozhatsky wrote: > > From: Sergey Senozhatsky > > Document new v4l2-selection target which will be used for the > Region of Interest v4l2 control. > > Signed-off-by: Sergey Senozhatsky > --- > .../media/v4l/selection-api-configuration.rst | 23 +++ > .../media/v4l/v4l2-selection-targets.rst | 21 + > include/uapi/linux/v4l2-common.h | 8 +++ > 3 files changed, 52 insertions(+) > > diff --git > a/Documentation/userspace-api/media/v4l/selection-api-configuration.rst > b/Documentation/userspace-api/media/v4l/selection-api-configuration.rst > index fee49bf1a1c0..9f69d71803f6 100644 > --- a/Documentation/userspace-api/media/v4l/selection-api-configuration.rst > +++ b/Documentation/userspace-api/media/v4l/selection-api-configuration.rst > @@ -135,3 +135,26 @@ and the height of rectangles obtained using > ``V4L2_SEL_TGT_CROP`` and > ``V4L2_SEL_TGT_COMPOSE`` targets. If these are not equal then the > scaling is applied. The application can compute the scaling ratios using > these values. > + > +Configuration of Region of Interest (ROI) > += > + > +The range of coordinates of the top left corner, width and height of > +areas that can be ROI is given by the ``V4L2_SEL_TGT_ROI_BOUNDS`` target. > +It is recommended for the driver developers to put the top/left corner > +at position ``(0,0)``. The rectangle's coordinates are in global sensor > +coordinates. The units are in pixels and independent of the field of view. > +They are not impacted by any cropping or scaling that is currently being > +used. Can we also mention binning here? > + > +The top left corner, width and height of the Region of Interest area > +currently being employed by the device is given by the > +``V4L2_SEL_TGT_ROI_CURRENT`` target. It uses the same coordinate system > +as ``V4L2_SEL_TGT_ROI_BOUNDS``. Why do we need current? Cant we just read back V4L2_SEL_TGT_ROI ? > + > +In order to change active ROI top left, width and height coordinates > +use ``V4L2_SEL_TGT_ROI`` target. > + > +Each capture device has a default ROI rectangle, given by the > +``V4L2_SEL_TGT_ROI_DEFAULT`` target. Drivers shall set the ROI rectangle > +to the default when the driver is first loaded, but not later. > diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > index e877ebbdb32e..cb3809418fa6 100644 > --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > @@ -69,3 +69,24 @@ of the two interfaces they are used. > modified by hardware. >- Yes >- No > +* - ``V4L2_SEL_TGT_ROI_CURRENT`` > + - 0x0200 > + - Current Region of Interest rectangle. > + - Yes > + - No > +* - ``V4L2_SEL_TGT_ROI_DEFAULT`` > + - 0x0201 > + - Suggested Region of Interest rectangle. > + - Yes > + - No > +* - ``V4L2_SEL_TGT_ROI_BOUNDS`` > + - 0x0202 > + - Bounds of the Region of Interest rectangle. All valid ROI rectangles > fit > + inside the ROI bounds rectangle. > + - Yes > + - No > +* - ``V4L2_SEL_TGT_ROI`` > + - 0x0203 > + - Sets the new Region of Interest rectangle. > + - Yes > + - No As mentioned before I think we should not have TGT_ROI_CURRENT and TGT_ROI > diff --git a/include/uapi/linux/v4l2-common.h > b/include/uapi/linux/v4l2-common.h > index 7d21c1634b4d..d0c108fba638 100644 > --- a/include/uapi/linux/v4l2-common.h > +++ b/include/uapi/linux/v4l2-common.h > @@ -78,6 +78,14 @@ > #define V4L2_SEL_TGT_COMPOSE_BOUNDS0x0102 > /* Current composing area plus all padding pixels */ > #define V4L2_SEL_TGT_COMPOSE_PADDED0x0103 > +/* Current Region of Interest area */ > +#define V4L2_SEL_TGT_ROI_CURRENT 0x0200 > +/* Default Region of Interest area */ > +#define V4L2_SEL_TGT_ROI_DEFAULT 0x0201 > +/* Region of Interest bounds */ > +#define V4L2_SEL_TGT_ROI_BOUNDS0x0202 > +/* Set Region of Interest area */ > +#define V4L2_SEL_TGT_ROI 0x0203 Nit: Maybe it could be a good idea to split doc and code. This way the backports/fixes are easier. > > /* Selection flags */ > #define V4L2_SEL_FLAG_GE (1 << 0) > -- > 2.30.0 > -- Ricardo Ribalda
Re: [PATCH v3 7/8] media: uvcvideo: Set a different name for the metadata entity
Hi Laurent
Thanks!
On Sat, Mar 13, 2021 at 12:29 AM Laurent Pinchart
wrote:
>
> Hi Ricardo,
>
> On Sat, Mar 13, 2021 at 12:17:50AM +0100, Ricardo Ribalda Delgado wrote:
> > On Fri, Mar 12, 2021 at 11:30 PM Laurent Pinchart wrote:
> > > On Fri, Mar 12, 2021 at 01:48:29PM +0100, Ricardo Ribalda wrote:
> > > > All the entities must have a unique name. And now that we are at it, we
> > > > append the entity->id to the name to avoid collisions on multi-chain
> > > > devices.
> > > >
> > > > Fixes v4l2-compliance:
> > > > Media Controller ioctls:
> > > > fail: v4l2-test-media.cpp(205):
> > > > v2_entity_names_set.find(key) != v2_entity_names_set.end()
> > > > test MEDIA_IOC_G_TOPOLOGY: FAIL
> > > > fail: v4l2-test-media.cpp(394): num_data_links !=
> > > > num_links
> > > > test MEDIA_IOC_ENUM_ENTITIES/LINKS: FAIL
> > > >
> > > > Signed-off-by: Ricardo Ribalda
> > > > ---
> > > > drivers/media/usb/uvc/uvc_driver.c | 21 -
> > > > 1 file changed, 20 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c
> > > > b/drivers/media/usb/uvc/uvc_driver.c
> > > > index 35873cf2773d..6c928e708615 100644
> > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > @@ -2154,6 +2154,18 @@ static void uvc_unregister_video(struct
> > > > uvc_device *dev)
> > > > #endif
> > > > }
> > > >
> > > > +static int uvc_oterm_id(struct uvc_video_chain *chain)
> > > > +{
> > > > + struct uvc_entity *entity;
> > > > +
> > > > + list_for_each_entry(entity, >entities, chain) {
> > > > + if (UVC_ENTITY_IS_OTERM(entity))
> > > > + return entity->id;
> > >
> > > It can also be an ITERM for output devices. You can drop this function
> > > and use stream>header.bTerminalLink below (see uvc_stream_by_id() and
> > > its usage in uvc_register_terms()).
> > >
> > > > + }
> > > > +
> > > > + return -1;
> > > > +}
> > > > +
> > > > int uvc_register_video_device(struct uvc_device *dev,
> > > > struct uvc_streaming *stream,
> > > > struct video_device *vdev,
> > > > @@ -2162,6 +2174,8 @@ int uvc_register_video_device(struct uvc_device
> > > > *dev,
> > > > const struct v4l2_file_operations *fops,
> > > > const struct v4l2_ioctl_ops *ioctl_ops)
> > > > {
> > > > + char prefix[sizeof(vdev->name) - 9];
> > > > + const char *suffix;
> > > > int ret;
> > > >
> > > > /* Initialize the video buffers queue. */
> > > > @@ -2190,16 +2204,21 @@ int uvc_register_video_device(struct uvc_device
> > > > *dev,
> > > > case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> > > > default:
> > > > vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE |
> > > > V4L2_CAP_STREAMING;
> > > > + suffix = "video";
> > > > break;
> > > > case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> > > > vdev->device_caps = V4L2_CAP_VIDEO_OUTPUT |
> > > > V4L2_CAP_STREAMING;
> > > > + suffix = "out";
> > >
> > > I wonder if these two should be video-cap and video-out (or vid-cap and
> > > vid-out if you want to shorten them) ?
> > >
> > > > break;
> > > > case V4L2_BUF_TYPE_META_CAPTURE:
> > > > vdev->device_caps = V4L2_CAP_META_CAPTURE |
> > > > V4L2_CAP_STREAMING;
> > > > + suffix = "meta";
> > > > break;
> > > > }
> > > >
> > > > - strscpy(vdev->name, dev->name, sizeof(vdev->name));
> > > > + strscpy(prefix, dev->name, sizeof(prefix));
> > > > + snprintf(vdev->name, sizeof(vdev->name), "%s-%d %s", prefix,
> > >
> > > The unit ID is never negative, so %u ?
> > >
> > > > + uvc_oterm_id(stream->chain)
Re: [PATCH v3 7/8] media: uvcvideo: Set a different name for the metadata entity
HI Laurent
Thanks for the review
On Fri, Mar 12, 2021 at 11:30 PM Laurent Pinchart
wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Fri, Mar 12, 2021 at 01:48:29PM +0100, Ricardo Ribalda wrote:
> > All the entities must have a unique name. And now that we are at it, we
> > append the entity->id to the name to avoid collisions on multi-chain
> > devices.
> >
> > Fixes v4l2-compliance:
> > Media Controller ioctls:
> > fail: v4l2-test-media.cpp(205):
> > v2_entity_names_set.find(key) != v2_entity_names_set.end()
> > test MEDIA_IOC_G_TOPOLOGY: FAIL
> > fail: v4l2-test-media.cpp(394): num_data_links != num_links
> > test MEDIA_IOC_ENUM_ENTITIES/LINKS: FAIL
> >
> > Signed-off-by: Ricardo Ribalda
> > ---
> > drivers/media/usb/uvc/uvc_driver.c | 21 -
> > 1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c
> > b/drivers/media/usb/uvc/uvc_driver.c
> > index 35873cf2773d..6c928e708615 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -2154,6 +2154,18 @@ static void uvc_unregister_video(struct uvc_device
> > *dev)
> > #endif
> > }
> >
> > +static int uvc_oterm_id(struct uvc_video_chain *chain)
> > +{
> > + struct uvc_entity *entity;
> > +
> > + list_for_each_entry(entity, >entities, chain) {
> > + if (UVC_ENTITY_IS_OTERM(entity))
> > + return entity->id;
>
> It can also be an ITERM for output devices. You can drop this function
> and use stream>header.bTerminalLink below (see uvc_stream_by_id() and
> its usage in uvc_register_terms()).
>
> > + }
> > +
> > + return -1;
> > +}
> > +
> > int uvc_register_video_device(struct uvc_device *dev,
> > struct uvc_streaming *stream,
> > struct video_device *vdev,
> > @@ -2162,6 +2174,8 @@ int uvc_register_video_device(struct uvc_device *dev,
> > const struct v4l2_file_operations *fops,
> > const struct v4l2_ioctl_ops *ioctl_ops)
> > {
> > + char prefix[sizeof(vdev->name) - 9];
> > + const char *suffix;
> > int ret;
> >
> > /* Initialize the video buffers queue. */
> > @@ -2190,16 +2204,21 @@ int uvc_register_video_device(struct uvc_device
> > *dev,
> > case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> > default:
> > vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE |
> > V4L2_CAP_STREAMING;
> > + suffix = "video";
> > break;
> > case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> > vdev->device_caps = V4L2_CAP_VIDEO_OUTPUT |
> > V4L2_CAP_STREAMING;
> > + suffix = "out";
>
> I wonder if these two should be video-cap and video-out (or vid-cap and
> vid-out if you want to shorten them) ?
>
> > break;
> > case V4L2_BUF_TYPE_META_CAPTURE:
> > vdev->device_caps = V4L2_CAP_META_CAPTURE |
> > V4L2_CAP_STREAMING;
> > + suffix = "meta";
> > break;
> > }
> >
> > - strscpy(vdev->name, dev->name, sizeof(vdev->name));
> > + strscpy(prefix, dev->name, sizeof(prefix));
> > + snprintf(vdev->name, sizeof(vdev->name), "%s-%d %s", prefix,
>
> The unit ID is never negative, so %u ?
>
> > + uvc_oterm_id(stream->chain), suffix);
>
> Truncating the device name at the beginning of the video node name isn't
> very nice :-S How about the following ?
>
> snprintf(vdev->name, sizeof(vdev->name), "%s-%u (%s)", type_name,
> uvc_oterm_id(stream->chain), dev->name);
>
> with the suffix variable renamed to type_name ?
>
> Thinking some more about it, vdev->name serves two purposes in the
> driver: creating the entity name, and reporting the card name in
> querycap. The former is done in the V4L2 core, which uses vdev->name
> as-is. In this context, we con't need to add dev->name, it would be
> redundant as the media controller device already reports it. The latter
> is done in uvc_ioctl_querycap(). How about dropping dev->name from
> vdev->name, and modifying uvc_ioctl_querycap() to use dev->name instead
> of cap->card ?
>
Something like ?
https://git.kernel.org/pub/scm/linux/kernel/git/ribalda/linux.git/commit/?h=uvc-compliance-v4=d4f7363455837116268152c96bf4b78d9761ad1e
https://git.kernel.org/pub/scm/linux/kernel/git/ribalda/linux.git/commit/?h=uvc-compliance-v4=ee3916f12b30f56c03d5622ba8a599b9c610a055
I need to work on the V4L2_CTRL_FLAG_GRABBED issue and then I will
send the whole v4 series that can pass all the v4l2-compliance test :)
Thanks!
> >
> > /*
> >* Set the driver data before calling video_register_device, otherwise
>
> --
> Regards,
>
> Laurent Pinchart
--
Ricardo Ribalda
Re: [PATCH v2 6/6] media: uvcvideo: Use dma_alloc_noncontiguous API
Hi Laurent
Thanks a lot for the review
On Fri, Mar 12, 2021 at 10:19 PM Laurent Pinchart
wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Fri, Mar 12, 2021 at 01:57:09PM +0100, Ricardo Ribalda wrote:
> > On architectures where there is no coherent caching such as ARM use the
> > dma_alloc_noncontiguous API and handle manually the cache flushing using
> > dma_sync_sgtable().
>
> You're actually switching to dma_alloc_noncontiguous() unconditionally,
> not only on those architectures :-) Do I assume correctly that
> dma_alloc_noncontiguous() will do the right thing on x86 too ?
Yes dma_alloc_noncontiguous does all the magic. It falls back to
alloc_dma_pages. Christoph has done a great job.
I tried it in my notebook and although the images are nothing but
pretty it is not the APIs fault. It is because the barbers are closed
due to the pandemic ;).
>
> > With this patch on the affected architectures we can measure up to 20x
> > performance improvement in uvc_video_copy_data_work().
>
> Have you measured performances on x86 to ensure there's no regression ?
Yes in the early stages. I am adding the latest x86 measurements in
the commit message in v3 to make it more clear.
>
> > Eg: aarch64 with an external usb camera
> >
> > NON_CONTIGUOUS
> > frames: 999
> > packets: 999
> > empty: 0 (0 %)
> > errors: 0
> > invalid: 0
> > pts: 0 early, 0 initial, 999 ok
> > scr: 0 count ok, 0 diff ok
> > sof: 2048 <= sof <= 0, freq 0.000 kHz
> > bytes 67034480 : duration 33303
> > FPS: 29.99
> > URB: 523446/4993 uS/qty: 104.836 avg 132.532 std 13.230 min 831.094 max (uS)
> > header: 76564/4993 uS/qty: 15.334 avg 15.229 std 3.438 min 186.875 max (uS)
> > latency: 468945/4992 uS/qty: 93.939 avg 132.577 std 9.531 min 824.010 max
> > (uS)
> > decode: 54161/4993 uS/qty: 10.847 avg 6.313 std 1.614 min 111.458 max (uS)
> > raw decode speed: 9.931 Gbits/s
> > raw URB handling speed: 1.025 Gbits/s
> > throughput: 16.102 Mbits/s
> > URB decode CPU usage 0.162600 %
> >
> > COHERENT
> > frames: 999
> > packets: 999
> > empty: 0 (0 %)
> > errors: 0
> > invalid: 0
> > pts: 0 early, 0 initial, 999 ok
> > scr: 0 count ok, 0 diff ok
> > sof: 2048 <= sof <= 0, freq 0.000 kHz
> > bytes 54683536 : duration 33302
> > FPS: 29.99
> > URB: 1478135/4000 uS/qty: 369.533 avg 390.357 std 22.968 min 3337.865 max
> > (uS)
> > header: 79761/4000 uS/qty: 19.940 avg 18.495 std 1.875 min 336.719 max (uS)
> > latency: 281077/4000 uS/qty: 70.269 avg 83.102 std 5.104 min 735.000 max
> > (uS)
> > decode: 1197057/4000 uS/qty: 299.264 avg 318.080 std 1.615 min 2806.667 max
> > (uS)
> > raw decode speed: 365.470 Mbits/s
> > raw URB handling speed: 295.986 Mbits/s
> > throughput: 13.136 Mbits/s
> > URB decode CPU usage 3.594500 %
> >
> > Signed-off-by: Ricardo Ribalda
> > Reviewed-by: Tomasz Figa
> > Signed-off-by: Christoph Hellwig
> > ---
> >
> > Changelog from v2: (Thanks Laurent)
> >
> > - Fix typos
> > - Use the right dma direction if not capturing
> > - Clear sgt during free
> >
> > drivers/media/usb/uvc/uvc_video.c | 92 +++
> > drivers/media/usb/uvc/uvcvideo.h | 5 +-
> > 2 files changed, 74 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c
> > b/drivers/media/usb/uvc/uvc_video.c
> > index f2f565281e63..8e60f81e2257 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -6,11 +6,14 @@
> > * Laurent Pinchart (laurent.pinch...@ideasonboard.com)
> > */
> >
> > +#include
> > +#include
> > #include
> > #include
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -1096,6 +1099,34 @@ static int uvc_video_decode_start(struct
> > uvc_streaming *stream,
> > return data[0];
> > }
> >
> > +static inline enum dma_data_direction stream_dir(struct uvc_streaming
> > *stream)
> > +{
> > + if (stream->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > + return DMA_FROM_DEVICE;
> > + else
> > + return DMA_TO_DEVICE;
> > +}
> > +
> > +static inline struct device *stream_to_dmadev(struct uvc_streaming *stream)
> > +{
> > + return bus_to_hcd(stream->dev->udev->bus)->self.sysdev;
> > +}
> > +
> > +static void uvc_urb_dma_sync(struct uvc_urb *uvc_urb, bool for_device)
>
> Maybe nitpicking a little bit, but wouldn't the code be clearer if you
> created uvc_urb_dma_sync_for_cpu() and uvc_urb_dma_sync_for_device() ?
> When reading
>
> uvc_urb_dma_sync(uvc_urb, true);
>
> I have to constantly look up the definition of the function to figure
> out what boolean value corresponds to what direction.
>
> Given that uvc_urb_dma_sync(..., true) is always called right before
> submitting the URB, we could even create a uvc_submit_urb() function
> that groups the dma_sync_sgtable_for_device() and usb_submit_urb()
> calls, and do without uvc_urb_dma_sync_for_device(). Up to you on this
> one.
I Like it! thanks for the idea
>
>
Re: [PATCH v2 5/6] media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS
HI Laurent
Thanks for the prompt reply :)
On Fri, Mar 12, 2021 at 2:25 AM Laurent Pinchart
wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Thu, Mar 11, 2021 at 11:19:45PM +0100, Ricardo Ribalda wrote:
> > Create all the class controls for the device defined controls.
> >
> > Fixes v4l2-compliance:
> > Control ioctls (Input 0):
> > fail: v4l2-test-controls.cpp(216): missing control class for
> > class 0098
> > fail: v4l2-test-controls.cpp(216): missing control tclass for
> > class 009a
> > test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: FAIL
> >
> > Signed-off-by: Ricardo Ribalda
> > ---
> > drivers/media/usb/uvc/uvc_ctrl.c | 90
> > drivers/media/usb/uvc/uvcvideo.h | 7 +++
> > 2 files changed, 97 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > b/drivers/media/usb/uvc/uvc_ctrl.c
> > index b3dde98499f4..4e0ed2595ae9 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -357,6 +357,17 @@ static const struct uvc_control_info uvc_ctrls[] = {
> > },
> > };
> >
> > +static const struct uvc_control_class uvc_control_class[] = {
> > + {
> > + .id = V4L2_CID_CAMERA_CLASS,
> > + .name = "Camera Controls",
> > + },
> > + {
> > + .id = V4L2_CID_USER_CLASS,
> > + .name = "User Controls",
> > + },
> > +};
> > +
> > static const struct uvc_menu_info power_line_frequency_controls[] = {
> > { 0, "Disabled" },
> > { 1, "50 Hz" },
> > @@ -1024,6 +1035,49 @@ static int __uvc_ctrl_get(struct uvc_video_chain
> > *chain,
> > return 0;
> > }
> >
> > +static int __uvc_query_v4l2_class(struct uvc_device *dev, u32 req_id,
> > + u32 found_id)
> > +{
> > + bool find_next = req_id & V4L2_CTRL_FLAG_NEXT_CTRL;
> > + int i;
>
> unsigned int as i will never be negative.
Sometimes you are a bit negative with my patches... :)
(sorry, it is Friday)
>
> > +
> > + req_id &= V4L2_CTRL_ID_MASK;
> > +
> > + for (i = 0; i < ARRAY_SIZE(uvc_control_class); i++) {
> > + if (!(dev->ctrl_class_bitmap & BIT(i)))
> > + continue;
> > + if (!find_next) {
> > + if (uvc_control_class[i].id == req_id)
> > + return i;
> > + continue;
> > + }
> > + if ((uvc_control_class[i].id > req_id) &&
> > + (uvc_control_class[i].id < found_id))
>
> No need for the inner parentheses.
>
> > + return i;
> > + }
> > +
> > + return -ENODEV;
> > +}
> > +
> > +static int uvc_query_v4l2_class(struct uvc_device *dev, u32 req_id,
> > + u32 found_id, struct v4l2_queryctrl
> > *v4l2_ctrl)
> > +{
> > + int idx;
> > +
> > + idx = __uvc_query_v4l2_class(dev, req_id, found_id);
> > + if (idx < 0)
> > + return -ENODEV;
> > +
> > + memset(v4l2_ctrl, 0, sizeof(*v4l2_ctrl));
> > + v4l2_ctrl->id = uvc_control_class[idx].id;
> > + strscpy(v4l2_ctrl->name, uvc_control_class[idx].name,
> > + sizeof(v4l2_ctrl->name));
> > + v4l2_ctrl->type = V4L2_CTRL_TYPE_CTRL_CLASS;
> > + v4l2_ctrl->flags = V4L2_CTRL_FLAG_WRITE_ONLY |
> > + V4L2_CTRL_FLAG_READ_ONLY;
>
> v4l2_ctrl->flags = V4L2_CTRL_FLAG_WRITE_ONLY
> | V4L2_CTRL_FLAG_READ_ONLY;
>
> > + return 0;
> > +}
>
> If you agree with the comments below, you could inline
> __uvc_query_v4l2_class() in uvc_query_v4l2_class() as it won't be called
> separately.
>
> > +
> > static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> > struct uvc_control *ctrl,
> > struct uvc_control_mapping *mapping,
> > @@ -1123,6 +1177,14 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain
> > *chain,
> > struct uvc_control_mapping *mapping;
> > int ret;
> >
> > + /* Check if the ctrl is a know class */
> > + if (!(v4l2_ctrl->id & V4L2_CTRL_FLAG_NEXT_CTRL)) {
> > + ret = uvc_query_v4l2_class(chain->dev, v4l2_ctrl->id,
> > +v4l2_ctrl->id, v4l2_ctrl);
>
> You could pass 0 for found_id here.
>
> > + if (!ret)
> > + return 0;
> > + }
> > +
>
> Should this be done with the chain->ctrl_mutex locked, as
> __uvc_query_v4l2_class() accesses dev->ctrl_class_bitmap that could be
> modified concurrently ?
>
> > ret = mutex_lock_interruptible(>ctrl_mutex);
> > if (ret < 0)
> > return -ERESTARTSYS;
> > @@ -1133,6 +1195,13 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain
> > *chain,
> > goto done;
> > }
> >
>
> A comment here along the lines of
>
> /*
> * If we're enumerating control with V4L2_CTRL_FLAG_NEXT_CTRL, check
> if
> *
Re: [PATCH v2 3/6] media: uvcvideo: Return -EIO for control errors
Hi Laurent On Fri, Mar 12, 2021 at 12:30 AM Laurent Pinchart wrote: > > Hi Ricardo, > > On Thu, Mar 11, 2021 at 11:59:27PM +0100, Ricardo Ribalda Delgado wrote: > > On Thu, Mar 11, 2021 at 11:53 PM Laurent Pinchart wrote: > > > On Thu, Mar 11, 2021 at 11:19:43PM +0100, Ricardo Ribalda wrote: > > > > The device is doing something unspected with the control. Either because > > > > the protocol is not properly implemented or there has been a HW error. > > > > > > > > Fixes v4l2-compliance: > > > > > > > > Control ioctls (Input 0): > > > > fail: v4l2-test-controls.cpp(448): s_ctrl returned an > > > > error (22) > > > > test VIDIOC_G/S_CTRL: FAIL > > > > fail: v4l2-test-controls.cpp(698): s_ext_ctrls returned > > > > an error (22) > > > > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > > > > > > > > Signed-off-by: Ricardo Ribalda > > > > > > The change looks good to me. > > > > > > Reviewed-by: Laurent Pinchart > > > > > > Which of the error codes below do you get with your camera, and for > > > which control ? > > > > Bus 001 Device 003: ID 05c8:03a2 Cheng Uei Precision Industry Co., Ltd > > (Foxlink) XiaoMi USB 2.0 Webcam > > > > info: checking extended control 'White Balance Temperature' (0x0098091a) > > Control error 7 > > info: checking extended control 'Exposure (Absolute)' (0x009a0902) > > Control error 6 > > :-S And what's the request (GET_CUR, GET_INFO, ...) ? Failed to query (SET_CUR) UVC control 10 on unit 2: -32 (exp. 2). Failed to query (SET_CUR) UVC control 4 on unit 1: -32 (exp. 4) > > > > > --- > > > > drivers/media/usb/uvc/uvc_video.c | 5 + > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c > > > > b/drivers/media/usb/uvc/uvc_video.c > > > > index f2f565281e63..25fd8aa23529 100644 > > > > --- a/drivers/media/usb/uvc/uvc_video.c > > > > +++ b/drivers/media/usb/uvc/uvc_video.c > > > > @@ -112,6 +112,11 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 > > > > query, u8 unit, > > > > case 5: /* Invalid unit */ > > > > case 6: /* Invalid control */ > > > > case 7: /* Invalid Request */ > > > > + /* > > > > + * The firmware has not properly implemented > > > > + * the control or there has been a HW error. > > > > + */ > > > > + return -EIO; > > > > case 8: /* Invalid value within range */ > > > > return -EINVAL; > > > > default: /* reserved or unknown */ > > -- > Regards, > > Laurent Pinchart -- Ricardo Ribalda
Re: [PATCH v2 3/6] media: uvcvideo: Return -EIO for control errors
Hi Laurent On Thu, Mar 11, 2021 at 11:53 PM Laurent Pinchart wrote: > > Hi Ricardo, > > Thank you for the patch. Thank you :) > > On Thu, Mar 11, 2021 at 11:19:43PM +0100, Ricardo Ribalda wrote: > > The device is doing something unspected with the control. Either because > > the protocol is not properly implemented or there has been a HW error. > > > > Fixes v4l2-compliance: > > > > Control ioctls (Input 0): > > fail: v4l2-test-controls.cpp(448): s_ctrl returned an error > > (22) > > test VIDIOC_G/S_CTRL: FAIL > > fail: v4l2-test-controls.cpp(698): s_ext_ctrls returned an > > error (22) > > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > > > > Signed-off-by: Ricardo Ribalda > > The change looks good to me. > > Reviewed-by: Laurent Pinchart > > Which of the error codes below do you get with your camera, and for > which control ? Bus 001 Device 003: ID 05c8:03a2 Cheng Uei Precision Industry Co., Ltd (Foxlink) XiaoMi USB 2.0 Webcam info: checking extended control 'White Balance Temperature' (0x0098091a) Control error 7 info: checking extended control 'Exposure (Absolute)' (0x009a0902) Control error 6 > > > --- > > drivers/media/usb/uvc/uvc_video.c | 5 + > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c > > b/drivers/media/usb/uvc/uvc_video.c > > index f2f565281e63..25fd8aa23529 100644 > > --- a/drivers/media/usb/uvc/uvc_video.c > > +++ b/drivers/media/usb/uvc/uvc_video.c > > @@ -112,6 +112,11 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, > > u8 unit, > > case 5: /* Invalid unit */ > > case 6: /* Invalid control */ > > case 7: /* Invalid Request */ > > + /* > > + * The firmware has not properly implemented > > + * the control or there has been a HW error. > > + */ > > + return -EIO; > > case 8: /* Invalid value within range */ > > return -EINVAL; > > default: /* reserved or unknown */ > > -- > Regards, > > Laurent Pinchart -- Ricardo Ribalda
Re: [PATCH 03/10] media: uvcvideo: Return -EIO for control errors
Hi Laurent On Thu, Mar 11, 2021 at 4:59 PM Laurent Pinchart wrote: > > Hi Ricardo, > > Thank you for the patch. > > On Thu, Mar 11, 2021 at 03:08:22PM +0100, Ricardo Ribalda wrote: > > As discussed in the IRC with Hans > > > > We need to specify in the commit message that this is most likely due > > to hw error. > > > > On Thu, Mar 11, 2021 at 1:20 PM Ricardo Ribalda > > wrote: > > > > > > Fixes v4l2-compliance: > > > > > > Control ioctls (Input 0): > > > fail: v4l2-test-controls.cpp(448): s_ctrl returned an > > > error (22) > > > test VIDIOC_G/S_CTRL: FAIL > > > fail: v4l2-test-controls.cpp(698): s_ext_ctrls returned > > > an error (22) > > > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > > As this isn't supposed to happen, how do you reproduce this ? Just run v4l2-compliance in my notebook camera. > > > > Signed-off-by: Ricardo Ribalda > > > --- > > > drivers/media/usb/uvc/uvc_video.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c > > > b/drivers/media/usb/uvc/uvc_video.c > > > index f2f565281e63..5442e9be1c55 100644 > > > --- a/drivers/media/usb/uvc/uvc_video.c > > > +++ b/drivers/media/usb/uvc/uvc_video.c > > > @@ -113,7 +113,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, > > > u8 unit, > > > case 6: /* Invalid control */ > > > case 7: /* Invalid Request */ > > For cases 5-7 I think -EIO is fine, as the driver should really not call > this function with an invalid unit, control or request. If it does, it's > a bug in the driver (we can check the units and controls the device > claims to support, and the requests are defined by the UVC > specification), if it doesn't and the device still returns this error, > it's a bug on the device side. > > > > case 8: /* Invalid value within range */ > > For this case, however, isn't it valid for a device to return an error > if the control value isn't valid ? There's one particular code path I'm > concerned about, uvc_ioctl_default(UVCIOC_CTRL_QUERY) -> > uvc_xu_ctrl_query() -> uvc_query_ctrl(), where it could be useful for > userspace to know that the value it sets isn't valid. > Will fix in v2 Thanks! > > > - return -EINVAL; > > > + return -EIO; > > > default: /* reserved or unknown */ > > > break; > > > } > > -- > Regards, > > Laurent Pinchart -- Ricardo Ribalda
Re: [PATCH 07/10] media: uvcvideo: set error_idx to count on EACCESS
Hi Laurent
On Thu, Mar 11, 2021 at 5:20 PM Laurent Pinchart
wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
Thank you for the review :)
>
> On Thu, Mar 11, 2021 at 01:20:37PM +0100, Ricardo Ribalda wrote:
> > According to the doc:
>
> The previous paragraph states:
>
> This check is done to avoid leaving the hardware in an inconsistent
> state due to easy-to-avoid problems. But it leads to another problem:
> the application needs to know whether an error came from the validation
> step (meaning that the hardware was not touched) or from an error during
> the actual reading from/writing to hardware.
I think we wrote his standard when we were young and naive ;)
>
> > The, in hindsight quite poor, solution for that is to set error_idx to
> > count if the validation failed.
> >
> > Fixes v4l2-compliance:
> > Control ioctls (Input 0):
> > fail: v4l2-test-controls.cpp(645): invalid error index
> > write only control
> > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL
> >
> > Signed-off-by: Ricardo Ribalda
> > ---
> > drivers/media/usb/uvc/uvc_v4l2.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> > b/drivers/media/usb/uvc/uvc_v4l2.c
> > index 625c216c46b5..9b6454bb2f28 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -1076,7 +1076,8 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file,
> > void *fh,
> > ret = uvc_ctrl_get(chain, ctrl);
> > if (ret < 0) {
> > uvc_ctrl_rollback(handle);
> > - ctrls->error_idx = i;
> > + ctrls->error_idx = (ret == -EACCES) ?
> > + ctrls->count : i;
>
> No need for parentheses.
I really like my parenthesis before the ? :. Can I leave it? :)
If it bothers you a lot I remove it, but I like to delimit where the
ternary operators end.
>
> I'm not sure this is correct though. -EACCES is returned by
> __uvc_ctrl_get() when the control is found and is a write-only control.
> The uvc_ctrl_get() calls for the previous controls will have potentially
> touched the device to read the current control value if it wasn't cached
> already, to this contradicts the rationale from the specification.
>
> I understand the need for this when setting controls, but when reading
> them, it's more puzzling, as the interactions with the hardware to read
> the controls are not supposed to affect the hardware state in a way that
> applications should care about. It may be an issue in the V4L2
> specification.
I have no strong opinions in both directions. If v4l2-compliance is
the de-facto standard I believe we should keep this patch or change
the compliance test.
Hans, what do think?
>
> > return ret;
> > }
> > }
>
> --
> Regards,
>
> Laurent Pinchart
--
Ricardo Ribalda
Re: [PATCH 10/10] media: uvcvideo: Populate only active control classes
Hi Hans
Thanks for your review!
On Thu, Mar 11, 2021 at 3:32 PM Hans Verkuil wrote:
>
> On 11/03/2021 13:20, Ricardo Ribalda wrote:
> > Do not create Control Classes for empty classes.
>
> Shouldn't this be squashed with patch 06/10?
Most of the cameras I have used have the two classes, So I am not
sure if squash with 6/10, or remove it. I separated it to feel what
Laurent has to say :)
>
> Regards,
>
> Hans
>
> >
> > Fixes v4l2-compliance:
> >
> > Control ioctls (Input 0):
> > fail: v4l2-test-controls.cpp(255): no controls in
> > class 009d
> > test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: FAIL
> >
> > Signed-off-by: Ricardo Ribalda
> > ---
> > drivers/media/usb/uvc/uvc_ctrl.c | 11 +++
> > drivers/media/usb/uvc/uvc_driver.c | 1 -
> > drivers/media/usb/uvc/uvcvideo.h | 1 -
> > 3 files changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 433342efc63f..5efbb3b5aa5b 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -2128,6 +2128,17 @@ static int __uvc_ctrl_add_mapping(struct uvc_device
> > *dev,
> > if (map->set == NULL)
> > map->set = uvc_set_le_value;
> >
> > + switch (V4L2_CTRL_ID2WHICH(map->id)) {
> > + case V4L2_CTRL_ID2WHICH(V4L2_CID_CAMERA_CLASS):
> > + dev->ctrl_class_unit->ctrl_class.bmControls[0] |=
> > + BIT(UVC_CC_CAMERA_CLASS);
> > + break;
> > + case V4L2_CTRL_ID2WHICH(V4L2_CID_USER_CLASS):
> > + dev->ctrl_class_unit->ctrl_class.bmControls[0] |=
> > + BIT(UVC_CC_USER_CLASS);
> > + break;
> > + }
> > +
> > list_add_tail(>list, >info.mappings);
> > uvc_dbg(dev, CONTROL, "Adding mapping '%s' to control %pUl/%u\n",
> > map->name, ctrl->info.entity, ctrl->info.selector);
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c
> > b/drivers/media/usb/uvc/uvc_driver.c
> > index 996e8bd06ac5..4f368ab3a1f1 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1501,7 +1501,6 @@ static int uvc_ctrl_class_parse(struct uvc_device
> > *dev)
> >
> > unit->ctrl_class.bControlSize = 1;
> > unit->ctrl_class.bmControls = (u8 *)unit + sizeof(*unit);
> > - unit->ctrl_class.bmControls[0] = (1 << (UVC_CC_LAST_CLASS + 1)) - 1;
> > unit->get_info = uvc_ctrl_class_get_info;
> > strncpy(unit->name, "Control Class", sizeof(unit->name) - 1);
> >
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h
> > b/drivers/media/usb/uvc/uvcvideo.h
> > index 1d59ac10c2eb..cc573d63e459 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -186,7 +186,6 @@
> > */
> > #define UVC_CC_CAMERA_CLASS 0
> > #define UVC_CC_USER_CLASS1
> > -#define UVC_CC_LAST_CLASSUVC_CC_USER_CLASS
> >
> > /*
> > * Driver specific constants.
> >
>
--
Ricardo Ribalda
Re: [PATCH] regmap: Add support for 12/20 register formatting
HI Mark On Thu, Sep 17, 2020 at 1:22 PM Mark Brown wrote: > > On Thu, Sep 17, 2020 at 08:31:54AM +0200, Ricardo Ribalda Delgado wrote: > > On Wed, Sep 16, 2020 at 6:29 PM Mark Brown wrote: > > > > What exactly is the format you're trying to describe here? It sounds > > > like there's two blocks of padding in here (I'm assuing that's what > > > dummy means) but what's the exact arrangement here and what are the > > > commands? It sounds like this might not work ideally with things like > > > the cache code (if it makes things seems sparser than they are) and > > > might not be obvious to someone looking at the datsheet. > > > The format is > > > > > > Where X is dont care, C is command, A is address and D is data bits. I > > > Shall I add this to the commit message? I want to send a V2 anyway, > > because I screwed up the identity (ribalda.com instead of kernel.org) > > Yes, please. I was fairly sure it worked, it was just a question of if > it was ideal for the format described. The only issue I can see with > the above is that the users will need to left shift their data - on the > face of it it would seem better to add a facility for padding the LSBs > of the data field to the core so that users can just use the data field > as documented. I was thinking also about that, the problem is that there are many devices on that family that are "software" compatible and it only changes the width of the data. Eg: So if we need to make a driver, we could use the same driver for all the chips on that family, saying to the user that the data size is always 20 bits I will send v2 ASAP with the updated doc. Thanks
Re: [PATCH] regmap: Add support for 12/20 register formatting
Hi Mark
On Wed, Sep 16, 2020 at 6:29 PM Mark Brown wrote:
>
> On Wed, Sep 16, 2020 at 06:05:52PM +0200, Ricardo Ribalda wrote:
> > From: Ricardo Ribalda
> >
> > Devices such as the AD5628 require 32 bits of data divided in 12 bits
> > for dummy, command and address, and 20 for data and dummy.
>
> What exactly is the format you're trying to describe here? It sounds
> like there's two blocks of padding in here (I'm assuing that's what
> dummy means) but what's the exact arrangement here and what are the
> commands? It sounds like this might not work ideally with things like
> the cache code (if it makes things seems sparser than they are) and
> might not be obvious to someone looking at the datsheet.
The format is
Where X is dont care, C is command, A is address and D is data bits. I
am using the following config successfully:
static const struct regmap_config config_dac = {
.reg_bits = 12,
.val_bits = 20,
.max_register = 0xff,
};
Shall I add this to the commit message? I want to send a V2 anyway,
because I screwed up the identity (ribalda.com instead of kernel.org)
Thanks
Re: [PATCH] i2c: designware: platdrv: Set class based on dmi
Hi Andy
On Wed, Jun 24, 2020 at 12:46 PM Andy Shevchenko
wrote:
>
> On Wed, Jun 24, 2020 at 11:12:39AM +0200, Ricardo Ribalda wrote:
> > Current AMD's zen-based APUs use this core for some of its i2c-buses.
> >
> > With this patch we re-enable autodetection of hwmon-alike devices, so
> > lm-sensors will be able to work automatically.
> >
> > It does not affect the boot-time of embedded devices, as the class is
> > set based on the dmi information.
>
> Hmm... Do we really need to have DMI? I mean wouldn't be safe just always
> provide this to be compatible with HWMON class?
>
I do not care :), I was just trying to follow the logic of
70fba8302adecfa08a087c6f1dd39537a55f5bd3
If it is decided otherwise I can change it, no problem ;)
> ...
>
> > +static bool dw_i2c_hwmon_bus(void)
> > +{
> > + if (strstr(dmi_get_system_info(DMI_PRODUCT_NAME), "QT5222"))
> > + return true;
> > + return false;
> > +}
>
> I don't like this. Perhaps for now you may simple use dmi_get_system_info()
> directly below.
I just realised that if there is no DMI, then we get a NULL, so we
need to add that check.
With the two ifs, I still prefer to put it on other function like now
(check v2).
If you still dont like it I will move it into the main function.
>
> ...
>
> > + adap->class = dw_i2c_hwmon_bus() ? I2C_CLASS_HWMON
> > + : I2C_CLASS_DEPRECATED;
>
> It's one line.
Fixed thanks
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Re: [PATCH] mtd: Fix mtd not registered due to nvmem name collision
Hi
This is just a friendly ping after two weeks ;)
On Mon, May 4, 2020 at 10:44 AM Miquel Raynal wrote:
>
> Hi Richard,
>
> Ricardo Ribalda Delgado wrote on Thu, 30 Apr 2020
> 15:17:21 +0200:
>
> > From: Ricardo Ribalda Delgado
> >
> > When the nvmem framework is enabled, a nvmem device is created per mtd
> > device/partition.
> >
> > It is not uncommon that a device can have multiple mtd devices with
> > partitions that have the same name. Eg, when there DT overlay is allowed
> > and the same device with mtd is attached twice.
> >
> > Under that circumstances, the mtd fails to register due to a name
> > duplication on the nvmem framework.
> >
> > With this patch we use the mtdX name instead of the partition name,
> > which is unique.
> >
> > [8.948991] sysfs: cannot create duplicate filename
> > '/bus/nvmem/devices/Production Data'
> > [8.948992] CPU: 7 PID: 246 Comm: systemd-udevd Not tainted
> > 5.5.0-qtec-standard #13
> > [8.948993] Hardware name: AMD Dibbler/Dibbler, BIOS 05.22.04.0019
> > 10/26/2019
> > [8.948994] Call Trace:
> > [8.948996] dump_stack+0x50/0x70
> > [8.948998] sysfs_warn_dup.cold+0x17/0x2d
> > [8.949000] sysfs_do_create_link_sd.isra.0+0xc2/0xd0
> > [8.949002] bus_add_device+0x74/0x140
> > [8.949004] device_add+0x34b/0x850
> > [8.949006] nvmem_register.part.0+0x1bf/0x640
> > ...
> > [8.948926] mtd mtd8: Failed to register NVMEM device
> >
> > Fixes: c4dfa25ab307 ("mtd: add support for reading MTD devices via the
> > nvmem API")
> > Signed-off-by: Ricardo Ribalda Delgado
> > ---
> > drivers/mtd/mtdcore.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> > index 2916674208b3..29d41003d6e0 100644
> > --- a/drivers/mtd/mtdcore.c
> > +++ b/drivers/mtd/mtdcore.c
> > @@ -555,7 +555,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
> >
> > config.id = -1;
> > config.dev = >dev;
> > - config.name = mtd->name;
> > + config.name = dev_name(>dev);
> > config.owner = THIS_MODULE;
> > config.reg_read = mtd_nvmem_reg_read;
> > config.size = mtd->size;
>
> We hope this will definitely fix the NVMEM duplicate name issue. If it
> does not reliably, we might want to revert this patch and create an MTD
> unique ID field which, for each MTD device, concatenates the name of
> its parent and its own mtd->name.
>
> Acked-by: Miquel Raynal
>
> Thanks,
> Miquèl
--
Ricardo Ribalda
[PATCH] mtd: Fix mtd not registered due to nvmem name collision
From: Ricardo Ribalda Delgado
When the nvmem framework is enabled, a nvmem device is created per mtd
device/partition.
It is not uncommon that a device can have multiple mtd devices with
partitions that have the same name. Eg, when there DT overlay is allowed
and the same device with mtd is attached twice.
Under that circumstances, the mtd fails to register due to a name
duplication on the nvmem framework.
With this patch we use the mtdX name instead of the partition name,
which is unique.
[8.948991] sysfs: cannot create duplicate filename
'/bus/nvmem/devices/Production Data'
[8.948992] CPU: 7 PID: 246 Comm: systemd-udevd Not tainted
5.5.0-qtec-standard #13
[8.948993] Hardware name: AMD Dibbler/Dibbler, BIOS 05.22.04.0019 10/26/2019
[8.948994] Call Trace:
[8.948996] dump_stack+0x50/0x70
[8.948998] sysfs_warn_dup.cold+0x17/0x2d
[8.949000] sysfs_do_create_link_sd.isra.0+0xc2/0xd0
[8.949002] bus_add_device+0x74/0x140
[8.949004] device_add+0x34b/0x850
[8.949006] nvmem_register.part.0+0x1bf/0x640
...
[8.948926] mtd mtd8: Failed to register NVMEM device
Fixes: c4dfa25ab307 ("mtd: add support for reading MTD devices via the nvmem
API")
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/mtd/mtdcore.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 2916674208b3..29d41003d6e0 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -555,7 +555,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
config.id = -1;
config.dev = >dev;
- config.name = mtd->name;
+ config.name = dev_name(>dev);
config.owner = THIS_MODULE;
config.reg_read = mtd_nvmem_reg_read;
config.size = mtd->size;
--
2.26.2
Re: [PATCH v2] mtd: Fix mtd not the same name not registered if nvmem
Hi On Mon, Apr 27, 2020 at 9:10 PM Boris Brezillon wrote: > > On Mon, 27 Apr 2020 16:49:22 +0200 > Miquel Raynal wrote: > > > Hi Boris, > > > > Boris Brezillon wrote on Mon, 27 Apr > > 2020 16:37:11 +0200: > > > > > On Mon, 27 Apr 2020 16:22:22 +0200 > > > Miquel Raynal wrote: > > > > > > > Hi Ricardo, > > > > > > > > Ricardo Ribalda Delgado wrote on Tue, 14 Apr 2020 > > > > 15:47:23 +0200: > > > > > > > > > Ping? > > > > > > > > > > On Thu, Apr 2, 2020 at 8:59 AM Ricardo Ribalda Delgado > > > > > wrote: > > > > > > > > > > > > When the nvmem framework is enabled, a nvmem device is created per > > > > > > mtd > > > > > > device/partition. > > > > > > > > > > > > It is not uncommon that a device can have multiple mtd devices with > > > > > > partitions that have the same name. Eg, when there DT overlay is > > > > > > allowed > > > > > > and the same device with mtd is attached twice. > > > > > > > > > > > > Under that circumstances, the mtd fails to register due to a name > > > > > > duplication on the nvmem framework. > > > > > > > > > > > > With this patch we add a _1, _2, _X to the subsequent names if > > > > > > there is > > > > > > a collition, and throw a warning, instead of not starting the mtd > > > > > > device. > > > > > > > > > > > > [8.948991] sysfs: cannot create duplicate filename > > > > > > '/bus/nvmem/devices/Production Data' > > > > > > [8.948992] CPU: 7 PID: 246 Comm: systemd-udevd Not tainted > > > > > > 5.5.0-qtec-standard #13 > > > > > > [ 8.948993] Hardware name: AMD Dibbler/Dibbler, BIOS > > > > > > 05.22.04.0019 10/26/2019 > > > > > > [8.948994] Call Trace: > > > > > > [8.948996] dump_stack+0x50/0x70 > > > > > > [8.948998] sysfs_warn_dup.cold+0x17/0x2d > > > > > > [8.949000] sysfs_do_create_link_sd.isra.0+0xc2/0xd0 > > > > > > [8.949002] bus_add_device+0x74/0x140 > > > > > > [8.949004] device_add+0x34b/0x850 > > > > > > [8.949006] nvmem_register.part.0+0x1bf/0x640 > > > > > > ... > > > > > > [8.948926] mtd mtd8: Failed to register NVMEM device > > > > > > > > > > > > Signed-off-by: Ricardo Ribalda Delgado > > > > > > > > Thanks for proposing this change. Indeed we are aware of the problem > > > > and the best solution that we could come up with was to create an > > > > additional "unique_name" field to the mtd_info structure. This new > > > > field would have the form: > > > > > > > > [] > > > > > > > > The separator might be '~' (but I am completely open on that), and that > > > > would give for instance: > > > > > > > > my-controller~my-device~my-part~mysub-part > > > > > > I'd prefer something slightly more standard for the separator, like '/', > > > which is what we usually use when we want to represent a path in a tree. > > > I do agree on the general approach though. > > > > I am not sure / is a valid separator here we would use this > > name to create a sysfs entry. Would it work? > > Hm, you're probably right, I didn't check how the name was used by > nvmem. I also see that partition names can contain spaces, which is > not that great. So, if we want to use mtd->unique_name we should > probably sanitize it before passing it to nvmem. All this makes me > reconsider your initial proposal: use 'mtdX' as the nvmem name. It's > unique and it's simple enough to not require this extra sanitization > pass, on the other hand, guessing the nvmem partition based on such an > opaque name is not simple, not to mention that the mtd device name can > change depending on the probe order. > > I also wonder if creating nvmem devs unconditionally for all mtd > devices is a good idea. Sounds like we should only do that if there's an > explicit request to have one partition exposed as an nvmem. > > Note that, no matter what we decide about nvmem, I think having unique > names at the MTD level is a good thing. I have no preference one way or another. The issue is that our current master leads to mtds not working fine and making the system unusable. Whatever we decide it must be fast and the patch backported. My original patch follows the same logic as led does where there is a duplicated name. I can send a separate patch that uses mtdX and then you decide what to pick. Or we can go with a third way, but it needs to be soon ;) Best regards!
[PATCH v2] media: v4l2-ctrl: Add p_def to v4l2_ctrl_config
This allows setting the default value on compound controls created via
v4l2_ctrl_new_custom.
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/v4l2-core/v4l2-ctrls.c | 3 ++-
include/media/v4l2-ctrls.h | 2 ++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index bf50d37ef6c1..939aa110daa0 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -2583,7 +2583,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct
v4l2_ctrl_handler *hdl,
type, min, max,
is_menu ? cfg->menu_skip_mask : step, def,
cfg->dims, cfg->elem_size,
- flags, qmenu, qmenu_int, ptr_null, priv);
+ flags, qmenu, qmenu_int,
+ v4l2_ctrl_ptr_create((void *)cfg->p_def), priv);
if (ctrl)
ctrl->is_private = cfg->is_private;
return ctrl;
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 26205ba3a0a0..d08d19a4ae34 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -375,6 +375,7 @@ struct v4l2_ctrl_handler {
* @max: The control's maximum value.
* @step: The control's step value for non-menu controls.
* @def: The control's default value.
+ * @p_def: The control's default value for compound controls.
* @dims: The size of each dimension.
* @elem_size: The size in bytes of the control.
* @flags: The control's flags.
@@ -403,6 +404,7 @@ struct v4l2_ctrl_config {
s64 max;
u64 step;
s64 def;
+ const void *p_def;
u32 dims[V4L2_CTRL_MAX_DIMS];
u32 elem_size;
u32 flags;
--
2.23.0
Re: [PATCH] media: v4l2-ctrl: Add p_def to v4l2_ctrl_config
Hi Hans
On Wed, Oct 16, 2019 at 2:57 PM Hans Verkuil wrote:
>
> On 10/16/19 2:43 PM, Hans Verkuil wrote:
> > On 10/16/19 2:39 PM, Ricardo Ribalda Delgado wrote:
> >> Hi Hans:
> >>
> >> On Wed, Oct 16, 2019 at 2:32 PM Hans Verkuil
> >> wrote:
> >>>
> >>> On 10/16/19 2:20 PM, Ricardo Ribalda Delgado wrote:
> >>>> Hi Hans
> >>>>
> >>>> Not that awkward, the user has to use the brand new
> >>>> v4l2_ctrl_ptr_create() ;). But if you prefer void * I can make the
> >>>> change.
> >>>
> >>> Well, a struct v4l2_ctrl_config is typically a static const, so you can't
> >>> use
> >>> v4l2_ctrl_ptr_create().
> >>>
> >>> Hmm, perhaps it is as easy as:
> >>>
> >>> static const struct v4l2_area def_area = {
> >>> ...
> >>> };
> >>>
> >>> static const struct v4l2_ctrl_config ctrl = {
> >>> ...
> >>>
> >>> .p_def.p_area = _area,
> >>> ...
> >>> };
> >>>
> >>> Can you do a quick compile check that I am not overlooking anything?
> >>>
> >>> If this works, then I'll take this patch.
> >>
> >> Testing with gcc 9.2.1
> >>
> >> This works fine, no warning/error:
> >>
> >> static struct v4l2_area unit_size = {
> >> .width = UNIT_SIZE,
> >> .height = UNIT_SIZE,
> >> };
> >> static struct v4l2_ctrl_config area_ctrl = {
> >> .type = V4L2_CTRL_TYPE_AREA,
> >> .flags = V4L2_CTRL_FLAG_READ_ONLY,
> >> .p_def.p_area = _size,
> >> };
> >>
> >> but if unit_size is set as CONST:
> >> static const struct v4l2_area
> >>
> >> Then:
> >> drivers/qtec/qtec_sony.c: In function ‘qtec_sony_probe’:
> >> drivers/qtec/qtec_sony.c:3151:19: warning: initialization discards
> >> ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> >> 3151 | .p_def.p_area = _size,
> >> |
> >
> > Hmm. So we need a const void *p_def instead.
>
> Ah, v4l2_ctrl_ptr_create() expects a non-const pointer.
>
> What happens if union v4l2_ctrl_ptr p_def; in struct v4l2_ctrl changes
> to const union v4l2_ctrl_ptr p_def; ?
>
> You'll need to add const elsewhere as well, but since the default value
> is const, this might work.
>
> I'm not entirely sure this is correct code, though, but it's worth trying
> it.
>
This might be tricky. p_def needs to be allocated "in run time", and
we need to set p_def afterwards.
I am afraid we will get a " assignment of read-only member" error
> Regards,
>
> Hans
>
> >
> > Regards,
> >
> > Hans
> >
> >>
> >>>
> >>> Regards,
> >>>
> >>> Hans
> >>>
> >>>>
> >>>> Regards
> >>>>
> >>>> On Wed, Oct 16, 2019 at 2:17 PM Hans Verkuil
> >>>> wrote:
> >>>>>
> >>>>> On 10/14/19 4:14 PM, Ricardo Ribalda Delgado wrote:
> >>>>>> This allows setting the default value on compound controls created via
> >>>>>> v4l2_ctrl_new_custom.
> >>>>>>
> >>>>>> Signed-off-by: Ricardo Ribalda Delgado
> >>>>>> ---
> >>>>>> drivers/media/v4l2-core/v4l2-ctrls.c | 2 +-
> >>>>>> include/media/v4l2-ctrls.h | 2 ++
> >>>>>> 2 files changed, 3 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> >>>>>> b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>>>>> index bf50d37ef6c1..12cf38f73f7b 100644
> >>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> >>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>>>>> @@ -2583,7 +2583,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct
> >>>>>> v4l2_ctrl_handler *hdl,
> >>>>>> type, min, max,
> >>>>>> is_menu ? cfg->menu_skip_mask : step, def,
> >>>>>> cfg->dims, cfg->elem_size,
> >>>>>> - flags, qmenu, qmenu_int, ptr_null, priv);
> >>>>>> + flags, qmenu, qmenu_int, cfg->p_def, priv);
> >>>>>> if (ctrl)
> >>>>>> ctrl->is_private = cfg->is_private;
> >>>>>> return ctrl;
> >>>>>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> >>>>>> index 26205ba3a0a0..2fca5b823961 100644
> >>>>>> --- a/include/media/v4l2-ctrls.h
> >>>>>> +++ b/include/media/v4l2-ctrls.h
> >>>>>> @@ -375,6 +375,7 @@ struct v4l2_ctrl_handler {
> >>>>>> * @max: The control's maximum value.
> >>>>>> * @step:The control's step value for non-menu controls.
> >>>>>> * @def: The control's default value.
> >>>>>> + * @p_def: The control's default value for compound controls.
> >>>>>> * @dims:The size of each dimension.
> >>>>>> * @elem_size: The size in bytes of the control.
> >>>>>> * @flags: The control's flags.
> >>>>>> @@ -403,6 +404,7 @@ struct v4l2_ctrl_config {
> >>>>>> s64 max;
> >>>>>> u64 step;
> >>>>>> s64 def;
> >>>>>> + union v4l2_ctrl_ptr p_def;
> >>>>>> u32 dims[V4L2_CTRL_MAX_DIMS];
> >>>>>> u32 elem_size;
> >>>>>> u32 flags;
> >>>>>>
> >>>>>
> >>>>> I'm not sure about this. It might be a bit awkward to initialize p_def
> >>>>> given that it is a union.
> >>>>>
> >>>>> Perhaps a simple void pointer would be easier?
> >>>>>
> >>>>> Regards,
> >>>>>
> >>>>> Hans
> >>>
> >
>
Re: [PATCH] media: v4l2-ctrl: Add p_def to v4l2_ctrl_config
Hi Hans
On Wed, Oct 16, 2019 at 2:43 PM Hans Verkuil wrote:
>
> On 10/16/19 2:39 PM, Ricardo Ribalda Delgado wrote:
> > Hi Hans:
> >
> > On Wed, Oct 16, 2019 at 2:32 PM Hans Verkuil
> > wrote:
> >>
> >> On 10/16/19 2:20 PM, Ricardo Ribalda Delgado wrote:
> >>> Hi Hans
> >>>
> >>> Not that awkward, the user has to use the brand new
> >>> v4l2_ctrl_ptr_create() ;). But if you prefer void * I can make the
> >>> change.
> >>
> >> Well, a struct v4l2_ctrl_config is typically a static const, so you can't
> >> use
> >> v4l2_ctrl_ptr_create().
> >>
> >> Hmm, perhaps it is as easy as:
> >>
> >> static const struct v4l2_area def_area = {
> >> ...
> >> };
> >>
> >> static const struct v4l2_ctrl_config ctrl = {
> >> ...
> >>
> >> .p_def.p_area = _area,
> >> ...
> >> };
> >>
> >> Can you do a quick compile check that I am not overlooking anything?
> >>
> >> If this works, then I'll take this patch.
> >
> > Testing with gcc 9.2.1
> >
> > This works fine, no warning/error:
> >
> > static struct v4l2_area unit_size = {
> > .width = UNIT_SIZE,
> > .height = UNIT_SIZE,
> > };
> > static struct v4l2_ctrl_config area_ctrl = {
> > .type = V4L2_CTRL_TYPE_AREA,
> > .flags = V4L2_CTRL_FLAG_READ_ONLY,
> > .p_def.p_area = _size,
> > };
> >
> > but if unit_size is set as CONST:
> > static const struct v4l2_area
> >
> > Then:
> > drivers/qtec/qtec_sony.c: In function ‘qtec_sony_probe’:
> > drivers/qtec/qtec_sony.c:3151:19: warning: initialization discards
> > ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> > 3151 | .p_def.p_area = _size,
> > |
>
> Hmm. So we need a const void *p_def instead.
>
If we make p_def in ctrl_config const then this will fail:
drivers/qtec/qtec_sony.c:3273:18: error: assignment of read-only member ‘p_def’
3273 | area_ctrl.p_def = v4l2_ctrl_ptr_create((void *)_size);
I think we need to leave it as in the proposed patch.
> Regards,
>
> Hans
>
> >
> >>
> >> Regards,
> >>
> >> Hans
> >>
> >>>
> >>> Regards
> >>>
> >>> On Wed, Oct 16, 2019 at 2:17 PM Hans Verkuil
> >>> wrote:
> >>>>
> >>>> On 10/14/19 4:14 PM, Ricardo Ribalda Delgado wrote:
> >>>>> This allows setting the default value on compound controls created via
> >>>>> v4l2_ctrl_new_custom.
> >>>>>
> >>>>> Signed-off-by: Ricardo Ribalda Delgado
> >>>>> ---
> >>>>> drivers/media/v4l2-core/v4l2-ctrls.c | 2 +-
> >>>>> include/media/v4l2-ctrls.h | 2 ++
> >>>>> 2 files changed, 3 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> >>>>> b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>>>> index bf50d37ef6c1..12cf38f73f7b 100644
> >>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> >>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>>>> @@ -2583,7 +2583,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct
> >>>>> v4l2_ctrl_handler *hdl,
> >>>>> type, min, max,
> >>>>> is_menu ? cfg->menu_skip_mask : step, def,
> >>>>> cfg->dims, cfg->elem_size,
> >>>>> - flags, qmenu, qmenu_int, ptr_null, priv);
> >>>>> + flags, qmenu, qmenu_int, cfg->p_def, priv);
> >>>>> if (ctrl)
> >>>>> ctrl->is_private = cfg->is_private;
> >>>>> return ctrl;
> >>>>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> >>>>> index 26205ba3a0a0..2fca5b823961 100644
> >>>>> --- a/include/media/v4l2-ctrls.h
> >>>>> +++ b/include/media/v4l2-ctrls.h
> >>>>> @@ -375,6 +375,7 @@ struct v4l2_ctrl_handler {
> >>>>> * @max: The control's maximum value.
> >>>>> * @step:The control's step value for non-menu controls.
> >>>>> * @def: The control's default value.
> >>>>> + * @p_def: The control's default value for compound controls.
> >>>>> * @dims:The size of each dimension.
> >>>>> * @elem_size: The size in bytes of the control.
> >>>>> * @flags: The control's flags.
> >>>>> @@ -403,6 +404,7 @@ struct v4l2_ctrl_config {
> >>>>> s64 max;
> >>>>> u64 step;
> >>>>> s64 def;
> >>>>> + union v4l2_ctrl_ptr p_def;
> >>>>> u32 dims[V4L2_CTRL_MAX_DIMS];
> >>>>> u32 elem_size;
> >>>>> u32 flags;
> >>>>>
> >>>>
> >>>> I'm not sure about this. It might be a bit awkward to initialize p_def
> >>>> given that it is a union.
> >>>>
> >>>> Perhaps a simple void pointer would be easier?
> >>>>
> >>>> Regards,
> >>>>
> >>>> Hans
> >>
>
Re: [PATCH] media: v4l2-ctrl: Add p_def to v4l2_ctrl_config
Hi Hans:
On Wed, Oct 16, 2019 at 2:32 PM Hans Verkuil wrote:
>
> On 10/16/19 2:20 PM, Ricardo Ribalda Delgado wrote:
> > Hi Hans
> >
> > Not that awkward, the user has to use the brand new
> > v4l2_ctrl_ptr_create() ;). But if you prefer void * I can make the
> > change.
>
> Well, a struct v4l2_ctrl_config is typically a static const, so you can't use
> v4l2_ctrl_ptr_create().
>
> Hmm, perhaps it is as easy as:
>
> static const struct v4l2_area def_area = {
> ...
> };
>
> static const struct v4l2_ctrl_config ctrl = {
> ...
>
> .p_def.p_area = _area,
> ...
> };
>
> Can you do a quick compile check that I am not overlooking anything?
>
> If this works, then I'll take this patch.
Testing with gcc 9.2.1
This works fine, no warning/error:
static struct v4l2_area unit_size = {
.width = UNIT_SIZE,
.height = UNIT_SIZE,
};
static struct v4l2_ctrl_config area_ctrl = {
.type = V4L2_CTRL_TYPE_AREA,
.flags = V4L2_CTRL_FLAG_READ_ONLY,
.p_def.p_area = _size,
};
but if unit_size is set as CONST:
static const struct v4l2_area
Then:
drivers/qtec/qtec_sony.c: In function ‘qtec_sony_probe’:
drivers/qtec/qtec_sony.c:3151:19: warning: initialization discards
‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
3151 | .p_def.p_area = _size,
|
>
> Regards,
>
> Hans
>
> >
> > Regards
> >
> > On Wed, Oct 16, 2019 at 2:17 PM Hans Verkuil
> > wrote:
> >>
> >> On 10/14/19 4:14 PM, Ricardo Ribalda Delgado wrote:
> >>> This allows setting the default value on compound controls created via
> >>> v4l2_ctrl_new_custom.
> >>>
> >>> Signed-off-by: Ricardo Ribalda Delgado
> >>> ---
> >>> drivers/media/v4l2-core/v4l2-ctrls.c | 2 +-
> >>> include/media/v4l2-ctrls.h | 2 ++
> >>> 2 files changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> index bf50d37ef6c1..12cf38f73f7b 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> @@ -2583,7 +2583,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct
> >>> v4l2_ctrl_handler *hdl,
> >>> type, min, max,
> >>> is_menu ? cfg->menu_skip_mask : step, def,
> >>> cfg->dims, cfg->elem_size,
> >>> - flags, qmenu, qmenu_int, ptr_null, priv);
> >>> + flags, qmenu, qmenu_int, cfg->p_def, priv);
> >>> if (ctrl)
> >>> ctrl->is_private = cfg->is_private;
> >>> return ctrl;
> >>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> >>> index 26205ba3a0a0..2fca5b823961 100644
> >>> --- a/include/media/v4l2-ctrls.h
> >>> +++ b/include/media/v4l2-ctrls.h
> >>> @@ -375,6 +375,7 @@ struct v4l2_ctrl_handler {
> >>> * @max: The control's maximum value.
> >>> * @step:The control's step value for non-menu controls.
> >>> * @def: The control's default value.
> >>> + * @p_def: The control's default value for compound controls.
> >>> * @dims:The size of each dimension.
> >>> * @elem_size: The size in bytes of the control.
> >>> * @flags: The control's flags.
> >>> @@ -403,6 +404,7 @@ struct v4l2_ctrl_config {
> >>> s64 max;
> >>> u64 step;
> >>> s64 def;
> >>> + union v4l2_ctrl_ptr p_def;
> >>> u32 dims[V4L2_CTRL_MAX_DIMS];
> >>> u32 elem_size;
> >>> u32 flags;
> >>>
> >>
> >> I'm not sure about this. It might be a bit awkward to initialize p_def
> >> given that it is a union.
> >>
> >> Perhaps a simple void pointer would be easier?
> >>
> >> Regards,
> >>
> >> Hans
>
Re: [PATCH] media: v4l2-ctrl: Add p_def to v4l2_ctrl_config
Hi Hans
Not that awkward, the user has to use the brand new
v4l2_ctrl_ptr_create() ;). But if you prefer void * I can make the
change.
Regards
On Wed, Oct 16, 2019 at 2:17 PM Hans Verkuil wrote:
>
> On 10/14/19 4:14 PM, Ricardo Ribalda Delgado wrote:
> > This allows setting the default value on compound controls created via
> > v4l2_ctrl_new_custom.
> >
> > Signed-off-by: Ricardo Ribalda Delgado
> > ---
> > drivers/media/v4l2-core/v4l2-ctrls.c | 2 +-
> > include/media/v4l2-ctrls.h | 2 ++
> > 2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> > b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index bf50d37ef6c1..12cf38f73f7b 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -2583,7 +2583,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct
> > v4l2_ctrl_handler *hdl,
> > type, min, max,
> > is_menu ? cfg->menu_skip_mask : step, def,
> > cfg->dims, cfg->elem_size,
> > - flags, qmenu, qmenu_int, ptr_null, priv);
> > + flags, qmenu, qmenu_int, cfg->p_def, priv);
> > if (ctrl)
> > ctrl->is_private = cfg->is_private;
> > return ctrl;
> > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> > index 26205ba3a0a0..2fca5b823961 100644
> > --- a/include/media/v4l2-ctrls.h
> > +++ b/include/media/v4l2-ctrls.h
> > @@ -375,6 +375,7 @@ struct v4l2_ctrl_handler {
> > * @max: The control's maximum value.
> > * @step:The control's step value for non-menu controls.
> > * @def: The control's default value.
> > + * @p_def: The control's default value for compound controls.
> > * @dims:The size of each dimension.
> > * @elem_size: The size in bytes of the control.
> > * @flags: The control's flags.
> > @@ -403,6 +404,7 @@ struct v4l2_ctrl_config {
> > s64 max;
> > u64 step;
> > s64 def;
> > + union v4l2_ctrl_ptr p_def;
> > u32 dims[V4L2_CTRL_MAX_DIMS];
> > u32 elem_size;
> > u32 flags;
> >
>
> I'm not sure about this. It might be a bit awkward to initialize p_def given
> that it is a union.
>
> Perhaps a simple void pointer would be easier?
>
> Regards,
>
> Hans
[PATCH] media: v4l2-ctrl: Add p_def to v4l2_ctrl_config
This allows setting the default value on compound controls created via
v4l2_ctrl_new_custom.
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/v4l2-core/v4l2-ctrls.c | 2 +-
include/media/v4l2-ctrls.h | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index bf50d37ef6c1..12cf38f73f7b 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -2583,7 +2583,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct
v4l2_ctrl_handler *hdl,
type, min, max,
is_menu ? cfg->menu_skip_mask : step, def,
cfg->dims, cfg->elem_size,
- flags, qmenu, qmenu_int, ptr_null, priv);
+ flags, qmenu, qmenu_int, cfg->p_def, priv);
if (ctrl)
ctrl->is_private = cfg->is_private;
return ctrl;
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 26205ba3a0a0..2fca5b823961 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -375,6 +375,7 @@ struct v4l2_ctrl_handler {
* @max: The control's maximum value.
* @step: The control's step value for non-menu controls.
* @def: The control's default value.
+ * @p_def: The control's default value for compound controls.
* @dims: The size of each dimension.
* @elem_size: The size in bytes of the control.
* @flags: The control's flags.
@@ -403,6 +404,7 @@ struct v4l2_ctrl_config {
s64 max;
u64 step;
s64 def;
+ union v4l2_ctrl_ptr p_def;
u32 dims[V4L2_CTRL_MAX_DIMS];
u32 elem_size;
u32 flags;
--
2.23.0
[PATCH v12 3/8] media: add V4L2_CTRL_TYPE_AREA control type
From: Ricardo Ribalda Delgado
This type contains the width and the height of a rectangular area.
Reviewed-by: Jacopo Mondi
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/v4l2-core/v4l2-ctrls.c | 21 ++
include/media/v4l2-ctrls.h | 42
include/uapi/linux/videodev2.h | 6
3 files changed, 69 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index 219d8aeefa20..96cab2e173d3 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1677,6 +1677,7 @@ static int std_validate_compound(const struct v4l2_ctrl
*ctrl, u32 idx,
{
struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
+ struct v4l2_area *area;
void *p = ptr.p + idx * ctrl->elem_size;
switch ((u32)ctrl->type) {
@@ -1753,6 +1754,11 @@ static int std_validate_compound(const struct v4l2_ctrl
*ctrl, u32 idx,
zero_padding(p_vp8_frame_header->entropy_header);
zero_padding(p_vp8_frame_header->coder_state);
break;
+ case V4L2_CTRL_TYPE_AREA:
+ area = p;
+ if (!area->width || !area->height)
+ return -EINVAL;
+ break;
default:
return -EINVAL;
}
@@ -2427,6 +2433,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
v4l2_ctrl_handler *hdl,
case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header);
break;
+ case V4L2_CTRL_TYPE_AREA:
+ elem_size = sizeof(struct v4l2_area);
+ break;
default:
if (type < V4L2_CTRL_COMPOUND_TYPES)
elem_size = sizeof(s32);
@@ -4116,6 +4125,18 @@ int __v4l2_ctrl_s_ctrl_string(struct v4l2_ctrl *ctrl,
const char *s)
}
EXPORT_SYMBOL(__v4l2_ctrl_s_ctrl_string);
+int __v4l2_ctrl_s_ctrl_area(struct v4l2_ctrl *ctrl,
+ const struct v4l2_area *area)
+{
+ lockdep_assert_held(ctrl->handler->lock);
+
+ /* It's a driver bug if this happens. */
+ WARN_ON(ctrl->type != V4L2_CTRL_TYPE_AREA);
+ *ctrl->p_new.p_area = *area;
+ return set_ctrl(NULL, ctrl, 0);
+}
+EXPORT_SYMBOL(__v4l2_ctrl_s_ctrl_area);
+
void v4l2_ctrl_request_complete(struct media_request *req,
struct v4l2_ctrl_handler *main_hdl)
{
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 90a8ee48c2f3..5331cf6c8517 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -50,6 +50,7 @@ struct poll_table_struct;
* @p_h264_slice_params: Pointer to a struct v4l2_ctrl_h264_slice_params.
* @p_h264_decode_params: Pointer to a struct
v4l2_ctrl_h264_decode_params.
* @p_vp8_frame_header:Pointer to a VP8 frame header structure.
+ * @p_area:Pointer to an area.
* @p: Pointer to a compound value.
*/
union v4l2_ctrl_ptr {
@@ -68,6 +69,7 @@ union v4l2_ctrl_ptr {
struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
struct v4l2_ctrl_h264_decode_params *p_h264_decode_params;
struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
+ struct v4l2_area *p_area;
void *p;
};
@@ -1086,6 +1088,46 @@ static inline int v4l2_ctrl_s_ctrl_string(struct
v4l2_ctrl *ctrl, const char *s)
return rval;
}
+/**
+ * __v4l2_ctrl_s_ctrl_area() - Unlocked variant of v4l2_ctrl_s_ctrl_area().
+ *
+ * @ctrl: The control.
+ * @area: The new area.
+ *
+ * This sets the control's new area safely by going through the control
+ * framework. This function assumes the control's handler is already locked,
+ * allowing it to be used from within the _ctrl_ops functions.
+ *
+ * This function is for area type controls only.
+ */
+int __v4l2_ctrl_s_ctrl_area(struct v4l2_ctrl *ctrl,
+ const struct v4l2_area *area);
+
+/**
+ * v4l2_ctrl_s_ctrl_area() - Helper function to set a control's area value
+ * from within a driver.
+ *
+ * @ctrl: The control.
+ * @area: The new area.
+ *
+ * This sets the control's new area safely by going through the control
+ * framework. This function will lock the control's handler, so it cannot be
+ * used from within the _ctrl_ops functions.
+ *
+ * This function is for area type controls only.
+ */
+static inline int v4l2_ctrl_s_ctrl_area(struct v4l2_ctrl *ctrl,
+ const struct v4l2_area *area)
+{
+ int rval;
+
+ v4l2_ctrl_lock(ctrl);
+ rval = __v4l2_ctrl_s_ctrl_area(ctrl, area);
+ v4l2_ctrl_unlock(ctrl);
+
+ return rval;
+}
+
/* Internal helper functions that deal with control events. */
extern const struct v4l2_subscribed_event_ops v
[PATCH v12 6/8] Documentation: media: Describe V4L2_CID_UNIT_CELL_SIZE
New control to pass to userspace the width/height of a pixel. Which is needed for calibration and lens selection. Reviewed-by: Philipp Zabel Signed-off-by: Ricardo Ribalda Delgado --- .../media/uapi/v4l/ext-ctrls-image-source.rst | 10 ++ 1 file changed, 10 insertions(+) diff --git a/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst b/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst index 2c3ab5796d76..2d3e2b83d6dd 100644 --- a/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst +++ b/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst @@ -55,3 +55,13 @@ Image Source Control IDs ``V4L2_CID_TEST_PATTERN_GREENB (integer)`` Test pattern green (next to blue) colour component. + +``V4L2_CID_UNIT_CELL_SIZE (struct)`` +This control returns the unit cell size in nanometers. The struct +:c:type:`v4l2_area` provides the width and the height in separate +fields to take into consideration asymmetric pixels. +This control does not take into consideration any possible hardware +binning. +The unit cell consists of the whole area of the pixel, sensitive and +non-sensitive. +This control is required for automatic calibration of sensors/cameras. -- 2.23.0
[PATCH v12 8/8] media: imx214: Add new control with V4L2_CID_UNIT_CELL_SIZE
According to the product brief, the unit cell size is 1120 nanometers^2.
https://www.sony-semicon.co.jp/products_en/IS/sensor1/img/products/ProductBrief_IMX214_20150428.pdf
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/i2c/imx214.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 159a3a604f0e..adcaaa8c86d1 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -47,6 +47,7 @@ struct imx214 {
struct v4l2_ctrl *pixel_rate;
struct v4l2_ctrl *link_freq;
struct v4l2_ctrl *exposure;
+ struct v4l2_ctrl *unit_size;
struct regulator_bulk_data supplies[IMX214_NUM_SUPPLIES];
@@ -948,6 +949,10 @@ static int imx214_probe(struct i2c_client *client)
static const s64 link_freq[] = {
IMX214_DEFAULT_LINK_FREQ,
};
+ static const struct v4l2_area unit_size = {
+ .width = 1120,
+ .height = 1120,
+ };
int ret;
ret = imx214_parse_fwnode(dev);
@@ -1029,6 +1034,10 @@ static int imx214_probe(struct i2c_client *client)
V4L2_CID_EXPOSURE,
0, 3184, 1, 0x0c70);
+ imx214->unit_size = v4l2_ctrl_new_std_compound(>ctrls,
+ NULL,
+ V4L2_CID_UNIT_CELL_SIZE,
+ v4l2_ctrl_ptr_create((void *)_size));
ret = imx214->ctrls.error;
if (ret) {
dev_err(>dev, "%s control init failed (%d)\n",
--
2.23.0
[PATCH v12 7/8] media: v4l2-ctrl: Add new helper v4l2_ctrl_ptr_create
This helper function simplifies the code by not needing a union
v4l2_ctrl_ptr and an assignment every time we need to use
a ctrl_ptr.
Suggested-by: Hans Verkuil
Signed-off-by: Ricardo Ribalda Delgado
---
include/media/v4l2-ctrls.h | 12
1 file changed, 12 insertions(+)
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 5331cf6c8517..d4e1b1902044 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -73,6 +73,18 @@ union v4l2_ctrl_ptr {
void *p;
};
+/**
+ * v4l2_ctrl_ptr_create() - Helper function to return a v4l2_ctrl_ptr from a
+ * void pointer
+ * @ptr: The void pointer
+ */
+static inline union v4l2_ctrl_ptr v4l2_ctrl_ptr_create(void *ptr)
+{
+ union v4l2_ctrl_ptr p = { .p = ptr };
+
+ return p;
+}
+
/**
* struct v4l2_ctrl_ops - The control operations that the driver has to
provide.
*
--
2.23.0
[PATCH v12 4/8] Documentation: media: Document V4L2_CTRL_TYPE_AREA
A struct v4l2_area containing the width and the height of a rectangular area. Reviewed-by: Jacopo Mondi Reviewed-by: Philipp Zabel Signed-off-by: Ricardo Ribalda Delgado --- Documentation/media/uapi/v4l/vidioc-queryctrl.rst | 6 ++ Documentation/media/videodev2.h.rst.exceptions| 1 + 2 files changed, 7 insertions(+) diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst index a3d56ffbf4cc..33aff21b7d11 100644 --- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst +++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst @@ -443,6 +443,12 @@ See also the examples in :ref:`control`. - n/a - A struct :c:type:`v4l2_ctrl_mpeg2_quantization`, containing MPEG-2 quantization matrices for stateless video decoders. +* - ``V4L2_CTRL_TYPE_AREA`` + - n/a + - n/a + - n/a + - A struct :c:type:`v4l2_area`, containing the width and the height +of a rectangular area. Units depend on the use case. * - ``V4L2_CTRL_TYPE_H264_SPS`` - n/a - n/a diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions index adeb6b7a15cb..b58e381bdf7b 100644 --- a/Documentation/media/videodev2.h.rst.exceptions +++ b/Documentation/media/videodev2.h.rst.exceptions @@ -141,6 +141,7 @@ replace symbol V4L2_CTRL_TYPE_H264_PPS :c:type:`v4l2_ctrl_type` replace symbol V4L2_CTRL_TYPE_H264_SCALING_MATRIX :c:type:`v4l2_ctrl_type` replace symbol V4L2_CTRL_TYPE_H264_SLICE_PARAMS :c:type:`v4l2_ctrl_type` replace symbol V4L2_CTRL_TYPE_H264_DECODE_PARAMS :c:type:`v4l2_ctrl_type` +replace symbol V4L2_CTRL_TYPE_AREA :c:type:`v4l2_ctrl_type` # V4L2 capability defines replace define V4L2_CAP_VIDEO_CAPTURE device-capabilities -- 2.23.0
[PATCH v12 5/8] media: add V4L2_CID_UNIT_CELL_SIZE control
This control returns the unit cell size in nanometres. The struct provides
the width and the height in separated fields to take into consideration
asymmetric pixels and/or hardware binning.
This control is required for automatic calibration of sensors/cameras.
Reviewed-by: Philipp Zabel
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/v4l2-core/v4l2-ctrls.c | 5 +
include/uapi/linux/v4l2-controls.h | 1 +
2 files changed, 6 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index 96cab2e173d3..bf50d37ef6c1 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -996,6 +996,7 @@ const char *v4l2_ctrl_get_name(u32 id)
case V4L2_CID_AUTO_FOCUS_RANGE: return "Auto Focus, Range";
case V4L2_CID_PAN_SPEED:return "Pan, Speed";
case V4L2_CID_TILT_SPEED: return "Tilt, Speed";
+ case V4L2_CID_UNIT_CELL_SIZE: return "Unit Cell Size";
/* FM Radio Modulator controls */
/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1377,6 +1378,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
v4l2_ctrl_type *type,
case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER:
*type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER;
break;
+ case V4L2_CID_UNIT_CELL_SIZE:
+ *type = V4L2_CTRL_TYPE_AREA;
+ *flags |= V4L2_CTRL_FLAG_READ_ONLY;
+ break;
default:
*type = V4L2_CTRL_TYPE_INTEGER;
break;
diff --git a/include/uapi/linux/v4l2-controls.h
b/include/uapi/linux/v4l2-controls.h
index a2669b79b294..5a7bedee2b0e 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1034,6 +1034,7 @@ enum v4l2_jpeg_chroma_subsampling {
#define V4L2_CID_TEST_PATTERN_GREENR
(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 5)
#define V4L2_CID_TEST_PATTERN_BLUE
(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 6)
#define V4L2_CID_TEST_PATTERN_GREENB
(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 7)
+#define V4L2_CID_UNIT_CELL_SIZE
(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 8)
/* Image processing controls */
--
2.23.0
[PATCH v12 2/8] Documentation: v4l2_ctrl_new_std_compound
From: Ricardo Ribalda Delgado Function for initializing compound controls with a default value. Suggested-by: Hans Verkuil Reviewed-by: Jacopo Mondi Signed-off-by: Ricardo Ribalda Delgado --- Documentation/media/kapi/v4l2-controls.rst | 9 + 1 file changed, 9 insertions(+) diff --git a/Documentation/media/kapi/v4l2-controls.rst b/Documentation/media/kapi/v4l2-controls.rst index ebe2a55908be..b20800cae3f2 100644 --- a/Documentation/media/kapi/v4l2-controls.rst +++ b/Documentation/media/kapi/v4l2-controls.rst @@ -140,6 +140,15 @@ Menu controls with a driver specific menu are added by calling const struct v4l2_ctrl_ops *ops, u32 id, s32 max, s32 skip_mask, s32 def, const char * const *qmenu); +Standard compound controls can be added by calling +:c:func:`v4l2_ctrl_new_std_compound`: + +.. code-block:: c + + struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, + const struct v4l2_ctrl_ops *ops, u32 id, + const union v4l2_ctrl_ptr p_def); + Integer menu controls with a driver specific menu can be added by calling :c:func:`v4l2_ctrl_new_int_menu`: -- 2.23.0
[PATCH v12 1/8] media: v4l2-core: Implement v4l2_ctrl_new_std_compound
From: Ricardo Ribalda Delgado
Currently compound controls do not have a simple way of initializing its
values. This results in ofuscated code with type_ops init.
This patch introduces a new field on the control with the default value
for the compound control that can be set with the brand new
v4l2_ctrl_new_std_compound function
Suggested-by: Hans Verkuil
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/v4l2-core/v4l2-ctrls.c | 50
include/media/v4l2-ctrls.h | 21
2 files changed, 64 insertions(+), 7 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index 1d8f38824631..219d8aeefa20 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -29,6 +29,8 @@
#define call_op(master, op) \
(has_op(master, op) ? master->ops->op(master) : 0)
+static const union v4l2_ctrl_ptr ptr_null;
+
/* Internal temporary helper struct, one for each v4l2_ext_control */
struct v4l2_ctrl_helper {
/* Pointer to the control reference of the master control */
@@ -1530,7 +1532,10 @@ static void std_init_compound(const struct v4l2_ctrl
*ctrl, u32 idx,
struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
void *p = ptr.p + idx * ctrl->elem_size;
- memset(p, 0, ctrl->elem_size);
+ if (ctrl->p_def.p)
+ memcpy(p, ctrl->p_def.p, ctrl->elem_size);
+ else
+ memset(p, 0, ctrl->elem_size);
/*
* The cast is needed to get rid of a gcc warning complaining that
@@ -2354,7 +2359,8 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
v4l2_ctrl_handler *hdl,
s64 min, s64 max, u64 step, s64 def,
const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size,
u32 flags, const char * const *qmenu,
- const s64 *qmenu_int, void *priv)
+ const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def,
+ void *priv)
{
struct v4l2_ctrl *ctrl;
unsigned sz_extra;
@@ -2460,6 +2466,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
v4l2_ctrl_handler *hdl,
is_array)
sz_extra += 2 * tot_ctrl_size;
+ if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p)
+ sz_extra += elem_size;
+
ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL);
if (ctrl == NULL) {
handler_set_err(hdl, -ENOMEM);
@@ -2503,6 +2512,12 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
v4l2_ctrl_handler *hdl,
ctrl->p_new.p = >val;
ctrl->p_cur.p = >cur.val;
}
+
+ if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) {
+ ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size;
+ memcpy(ctrl->p_def.p, p_def.p, elem_size);
+ }
+
for (idx = 0; idx < elems; idx++) {
ctrl->type_ops->init(ctrl, idx, ctrl->p_cur);
ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
@@ -2554,7 +2569,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct
v4l2_ctrl_handler *hdl,
type, min, max,
is_menu ? cfg->menu_skip_mask : step, def,
cfg->dims, cfg->elem_size,
- flags, qmenu, qmenu_int, priv);
+ flags, qmenu, qmenu_int, ptr_null, priv);
if (ctrl)
ctrl->is_private = cfg->is_private;
return ctrl;
@@ -2579,7 +2594,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct
v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
min, max, step, def, NULL, 0,
-flags, NULL, NULL, NULL);
+flags, NULL, NULL, ptr_null, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_std);
@@ -2612,7 +2627,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct
v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
0, max, mask, def, NULL, 0,
-flags, qmenu, qmenu_int, NULL);
+flags, qmenu, qmenu_int, ptr_null, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_std_menu);
@@ -2644,11 +2659,32 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct
v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
0, max, mask, def, NULL, 0,
-flags, qmenu, NULL, NULL);
+flags, qmenu, NULL, ptr_null, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
+/* Helper function for standard compound controls */
+struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
+
[PATCH v12 0/8] Implement UNIT_CELL_SIZE control
UNIT_CELL_SIZE is a control that represents the size of a cell (pixel). We required a bit of boilerplate to add this control :) - New way to init compount controls - New control type Thanks to Hans, Jacopo and Philipp for your help. You might want to see the series at my github repository if needed. https://github.com/ribalda/linux/tree/unit-size-v10 v12: Fix htmldocs warnings (Thanks Hans!) v11: Fix documentation related to binning v10: Typos in documentation and minor color style v9: Rename helper to v4l2_ctrl_ptr_create v8: Fix my email on some patches (sorry for the mess) v7: Add new helper v4l2_ctrl_ptr_from_void v4, v5 of this patchset never reached the mailing list. Ricardo Ribalda Delgado (8): media: v4l2-core: Implement v4l2_ctrl_new_std_compound Documentation: v4l2_ctrl_new_std_compound media: add V4L2_CTRL_TYPE_AREA control type Documentation: media: Document V4L2_CTRL_TYPE_AREA media: add V4L2_CID_UNIT_CELL_SIZE control Documentation: media: Describe V4L2_CID_UNIT_CELL_SIZE media: v4l2-ctrl: Add new helper v4l2_ctrl_ptr_create media: imx214: Add new control with V4L2_CID_UNIT_CELL_SIZE Documentation/media/kapi/v4l2-controls.rst| 9 +++ .../media/uapi/v4l/ext-ctrls-image-source.rst | 10 +++ .../media/uapi/v4l/vidioc-queryctrl.rst | 6 ++ .../media/videodev2.h.rst.exceptions | 1 + drivers/media/i2c/imx214.c| 9 +++ drivers/media/v4l2-core/v4l2-ctrls.c | 76 +-- include/media/v4l2-ctrls.h| 75 ++ include/uapi/linux/v4l2-controls.h| 1 + include/uapi/linux/videodev2.h| 6 ++ 9 files changed, 186 insertions(+), 7 deletions(-) -- 2.23.0
[PATCH v8 6/6] media: ad5820: Add support for ad5821 and ad5823
According to the datasheet, both AD5821 and AD5820 share a compatible
register-set:
http://www.analog.com/media/en/technical-documentation/data-sheets/AD5821.pdf
Some camera modules also refer that AD5823 is a replacement of AD5820:
https://download.kamami.com/p564094-OV8865_DS.pdf
Suggested-by: Pavel Machek
Signed-off-by: Ricardo Ribalda Delgado
Acked-by: Pavel Machek
---
drivers/media/i2c/ad5820.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
index 5651609e5095..19c74db0649f 100644
--- a/drivers/media/i2c/ad5820.c
+++ b/drivers/media/i2c/ad5820.c
@@ -25,8 +25,6 @@
#include
#include
-#define AD5820_NAME"ad5820"
-
/* Register definitions */
#define AD5820_POWER_DOWN (1 << 15)
#define AD5820_DAC_SHIFT 4
@@ -359,13 +357,17 @@ static int ad5820_remove(struct i2c_client *client)
}
static const struct i2c_device_id ad5820_id_table[] = {
- { AD5820_NAME, 0 },
+ { "ad5820", 0 },
+ { "ad5821", 0 },
+ { "ad5823", 0 },
{ }
};
MODULE_DEVICE_TABLE(i2c, ad5820_id_table);
static const struct of_device_id ad5820_of_table[] = {
{ .compatible = "adi,ad5820" },
+ { .compatible = "adi,ad5821" },
+ { .compatible = "adi,ad5823" },
{ }
};
MODULE_DEVICE_TABLE(of, ad5820_of_table);
@@ -374,7 +376,7 @@ static SIMPLE_DEV_PM_OPS(ad5820_pm, ad5820_suspend,
ad5820_resume);
static struct i2c_driver ad5820_i2c_driver = {
.driver = {
- .name = AD5820_NAME,
+ .name = "ad5820",
.pm = _pm,
.of_match_table = ad5820_of_table,
},
--
2.23.0
[PATCH v8 4/6] media: ad5820: Add support for of-autoload
Since kernel 4.16, i2c devices with DT compatible tag are modprobed
using their DT modalias.
Without this patch, if this driver is build as module it would never
be autoprobed.
There is no need to mask it with CONFIG_OF to allow ACPI loading, this
also builds find with CONFIG_OF=n.
Signed-off-by: Ricardo Ribalda Delgado
Acked-by: Pavel Machek
Cc: Sakari Ailus
---
drivers/media/i2c/ad5820.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
index 76aab651f217..5651609e5095 100644
--- a/drivers/media/i2c/ad5820.c
+++ b/drivers/media/i2c/ad5820.c
@@ -364,12 +364,19 @@ static const struct i2c_device_id ad5820_id_table[] = {
};
MODULE_DEVICE_TABLE(i2c, ad5820_id_table);
+static const struct of_device_id ad5820_of_table[] = {
+ { .compatible = "adi,ad5820" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, ad5820_of_table);
+
static SIMPLE_DEV_PM_OPS(ad5820_pm, ad5820_suspend, ad5820_resume);
static struct i2c_driver ad5820_i2c_driver = {
.driver = {
.name = AD5820_NAME,
.pm = _pm,
+ .of_match_table = ad5820_of_table,
},
.probe = ad5820_probe,
.remove = ad5820_remove,
--
2.23.0
[PATCH v8 2/6] media: ad5820: DT new optional field enable-gpios
Document new enable-gpio field. It can be used to disable the part
without turning down its regulator.
Cc: devicet...@vger.kernel.org
Signed-off-by: Ricardo Ribalda Delgado
Acked-by: Pavel Machek
Reviewed-by: Laurent Pinchart
Reviewed-by: Rob Herring
---
Documentation/devicetree/bindings/media/i2c/ad5820.txt | 6 ++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
b/Documentation/devicetree/bindings/media/i2c/ad5820.txt
index 5940ca11c021..db596e8eb0ba 100644
--- a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
+++ b/Documentation/devicetree/bindings/media/i2c/ad5820.txt
@@ -8,6 +8,11 @@ Required Properties:
- VANA-supply: supply of voltage for VANA pin
+Optional properties:
+
+ - enable-gpios : GPIO spec for the XSHUTDOWN pin. The XSHUTDOWN signal is
+active low, a high level on the pin enables the device.
+
Example:
ad5820: coil@c {
@@ -15,5 +20,6 @@ Example:
reg = <0x0c>;
VANA-supply = <>;
+ enable-gpios = < 26 GPIO_ACTIVE_HIGH>;
};
--
2.23.0
[PATCH v8 0/6] ad5820: Multiple fixes
-Support for enable-pin, of-autoload, enable-gpios and ad5821 and ad5823 For some reason these patchset was lost in translation for a year ;) v8: I screwed up sending v7, I sent it from a dirty directory and clicked on send-all without checking what was under v7*. Sorry :( This made patchwork very unhappy. I send v8 to make patchwork happy and hopefuly also the maintainer. Sorry again v7: Rebase on current media/master Ricardo Ribalda Delgado (6): media: ad5820: Define entity function media: ad5820: DT new optional field enable-gpios media: ad5820: Add support for enable pin media: ad5820: Add support for of-autoload media: ad5820: DT new compatible devices media: ad5820: Add support for ad5821 and ad5823 .../devicetree/bindings/media/i2c/ad5820.txt | 11 +- drivers/media/i2c/Kconfig | 2 +- drivers/media/i2c/ad5820.c| 35 --- 3 files changed, 42 insertions(+), 6 deletions(-) -- 2.23.0
[PATCH v8 5/6] media: ad5820: DT new compatible devices
Document new compatible devices. Cc: devicet...@vger.kernel.org Signed-off-by: Ricardo Ribalda Delgado Reviewed-by: Rob Herring Acked-by: Pavel Machek --- Documentation/devicetree/bindings/media/i2c/ad5820.txt | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/media/i2c/ad5820.txt b/Documentation/devicetree/bindings/media/i2c/ad5820.txt index db596e8eb0ba..5764cbedf9b7 100644 --- a/Documentation/devicetree/bindings/media/i2c/ad5820.txt +++ b/Documentation/devicetree/bindings/media/i2c/ad5820.txt @@ -2,7 +2,10 @@ Required Properties: - - compatible: Must contain "adi,ad5820" + - compatible: Must contain one of: + - "adi,ad5820" + - "adi,ad5821" + - "adi,ad5823" - reg: I2C slave address -- 2.23.0
[PATCH v8 3/6] media: ad5820: Add support for enable pin
This patch adds support for a programmable enable pin. It can be used in
situations where the ANA-vcc is not configurable (dummy-regulator), or
just to have a more fine control of the power saving.
The use of the enable pin is optional.
Signed-off-by: Ricardo Ribalda Delgado
Acked-by: Pavel Machek
---
drivers/media/i2c/Kconfig | 2 +-
drivers/media/i2c/ad5820.c | 17 +
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 78dc64d7b0e7..30d460844dff 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -975,7 +975,7 @@ if MEDIA_CAMERA_SUPPORT
config VIDEO_AD5820
tristate "AD5820 lens voice coil support"
- depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
+ depends on GPIOLIB && I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
help
This is a driver for the AD5820 camera lens voice coil.
It is used for example in Nokia N900 (RX-51).
diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
index 7a49651f4d1f..76aab651f217 100644
--- a/drivers/media/i2c/ad5820.c
+++ b/drivers/media/i2c/ad5820.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include
#include
@@ -47,6 +48,8 @@ struct ad5820_device {
u32 focus_ramp_time;
u32 focus_ramp_mode;
+ struct gpio_desc *enable_gpio;
+
struct mutex power_lock;
int power_count;
@@ -114,6 +117,8 @@ static int ad5820_power_off(struct ad5820_device *coil,
bool standby)
ret = ad5820_update_hw(coil);
}
+ gpiod_set_value_cansleep(coil->enable_gpio, 0);
+
ret2 = regulator_disable(coil->vana);
if (ret)
return ret;
@@ -128,6 +133,8 @@ static int ad5820_power_on(struct ad5820_device *coil, bool
restore)
if (ret < 0)
return ret;
+ gpiod_set_value_cansleep(coil->enable_gpio, 1);
+
if (restore) {
/* Restore the hardware settings. */
coil->standby = false;
@@ -138,6 +145,7 @@ static int ad5820_power_on(struct ad5820_device *coil, bool
restore)
return 0;
fail:
+ gpiod_set_value_cansleep(coil->enable_gpio, 0);
coil->standby = true;
regulator_disable(coil->vana);
@@ -304,6 +312,15 @@ static int ad5820_probe(struct i2c_client *client,
return ret;
}
+ coil->enable_gpio = devm_gpiod_get_optional(>dev, "enable",
+ GPIOD_OUT_LOW);
+ if (IS_ERR(coil->enable_gpio)) {
+ ret = PTR_ERR(coil->enable_gpio);
+ if (ret != -EPROBE_DEFER)
+ dev_err(>dev, "could not get enable gpio\n");
+ return ret;
+ }
+
mutex_init(>power_lock);
v4l2_i2c_subdev_init(>subdev, client, _ops);
--
2.23.0
[PATCH v8 1/6] media: ad5820: Define entity function
Without this patch, media_device_register_entity throws a warning: dev_warn(mdev->dev, "Entity type for entity %s was not initialized!\n", entity->name); Signed-off-by: Ricardo Ribalda Delgado Acked-by: Pavel Machek Reviewed-by: Laurent Pinchart --- drivers/media/i2c/ad5820.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c index 925c171e7797..7a49651f4d1f 100644 --- a/drivers/media/i2c/ad5820.c +++ b/drivers/media/i2c/ad5820.c @@ -309,6 +309,7 @@ static int ad5820_probe(struct i2c_client *client, v4l2_i2c_subdev_init(>subdev, client, _ops); coil->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; coil->subdev.internal_ops = _internal_ops; + coil->subdev.entity.function = MEDIA_ENT_F_LENS; strscpy(coil->subdev.name, "ad5820 focus", sizeof(coil->subdev.name)); ret = media_entity_pads_init(>subdev.entity, 0, NULL); -- 2.23.0
[PATCH v7 5/6] media: ad5820: DT new compatible devices
Document new compatible devices. Cc: devicet...@vger.kernel.org Signed-off-by: Ricardo Ribalda Delgado Reviewed-by: Rob Herring Acked-by: Pavel Machek --- Documentation/devicetree/bindings/media/i2c/ad5820.txt | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/media/i2c/ad5820.txt b/Documentation/devicetree/bindings/media/i2c/ad5820.txt index db596e8eb0ba..5764cbedf9b7 100644 --- a/Documentation/devicetree/bindings/media/i2c/ad5820.txt +++ b/Documentation/devicetree/bindings/media/i2c/ad5820.txt @@ -2,7 +2,10 @@ Required Properties: - - compatible: Must contain "adi,ad5820" + - compatible: Must contain one of: + - "adi,ad5820" + - "adi,ad5821" + - "adi,ad5823" - reg: I2C slave address -- 2.23.0
[PATCH v7 3/6] ad5820: Add support for enable pin
This patch adds support for a programmable enable pin. It can be used in
situations where the ANA-vcc is not configurable (dummy-regulator), or
just to have a more fine control of the power saving.
The use of the enable pin is optional.
Signed-off-by: Ricardo Ribalda Delgado
Acked-by: Pavel Machek
---
drivers/media/i2c/Kconfig | 2 +-
drivers/media/i2c/ad5820.c | 17 +
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 78dc64d7b0e7..30d460844dff 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -975,7 +975,7 @@ if MEDIA_CAMERA_SUPPORT
config VIDEO_AD5820
tristate "AD5820 lens voice coil support"
- depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
+ depends on GPIOLIB && I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
help
This is a driver for the AD5820 camera lens voice coil.
It is used for example in Nokia N900 (RX-51).
diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
index 7a49651f4d1f..76aab651f217 100644
--- a/drivers/media/i2c/ad5820.c
+++ b/drivers/media/i2c/ad5820.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include
#include
@@ -47,6 +48,8 @@ struct ad5820_device {
u32 focus_ramp_time;
u32 focus_ramp_mode;
+ struct gpio_desc *enable_gpio;
+
struct mutex power_lock;
int power_count;
@@ -114,6 +117,8 @@ static int ad5820_power_off(struct ad5820_device *coil,
bool standby)
ret = ad5820_update_hw(coil);
}
+ gpiod_set_value_cansleep(coil->enable_gpio, 0);
+
ret2 = regulator_disable(coil->vana);
if (ret)
return ret;
@@ -128,6 +133,8 @@ static int ad5820_power_on(struct ad5820_device *coil, bool
restore)
if (ret < 0)
return ret;
+ gpiod_set_value_cansleep(coil->enable_gpio, 1);
+
if (restore) {
/* Restore the hardware settings. */
coil->standby = false;
@@ -138,6 +145,7 @@ static int ad5820_power_on(struct ad5820_device *coil, bool
restore)
return 0;
fail:
+ gpiod_set_value_cansleep(coil->enable_gpio, 0);
coil->standby = true;
regulator_disable(coil->vana);
@@ -304,6 +312,15 @@ static int ad5820_probe(struct i2c_client *client,
return ret;
}
+ coil->enable_gpio = devm_gpiod_get_optional(>dev, "enable",
+ GPIOD_OUT_LOW);
+ if (IS_ERR(coil->enable_gpio)) {
+ ret = PTR_ERR(coil->enable_gpio);
+ if (ret != -EPROBE_DEFER)
+ dev_err(>dev, "could not get enable gpio\n");
+ return ret;
+ }
+
mutex_init(>power_lock);
v4l2_i2c_subdev_init(>subdev, client, _ops);
--
2.23.0
[PATCH v7 6/6] ad5820: Add support for ad5821 and ad5823
According to the datasheet, both AD5821 and AD5820 share a compatible
register-set:
http://www.analog.com/media/en/technical-documentation/data-sheets/AD5821.pdf
Some camera modules also refer that AD5823 is a replacement of AD5820:
https://download.kamami.com/p564094-OV8865_DS.pdf
Suggested-by: Pavel Machek
Signed-off-by: Ricardo Ribalda Delgado
Acked-by: Pavel Machek
---
drivers/media/i2c/ad5820.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
index 5651609e5095..19c74db0649f 100644
--- a/drivers/media/i2c/ad5820.c
+++ b/drivers/media/i2c/ad5820.c
@@ -25,8 +25,6 @@
#include
#include
-#define AD5820_NAME"ad5820"
-
/* Register definitions */
#define AD5820_POWER_DOWN (1 << 15)
#define AD5820_DAC_SHIFT 4
@@ -359,13 +357,17 @@ static int ad5820_remove(struct i2c_client *client)
}
static const struct i2c_device_id ad5820_id_table[] = {
- { AD5820_NAME, 0 },
+ { "ad5820", 0 },
+ { "ad5821", 0 },
+ { "ad5823", 0 },
{ }
};
MODULE_DEVICE_TABLE(i2c, ad5820_id_table);
static const struct of_device_id ad5820_of_table[] = {
{ .compatible = "adi,ad5820" },
+ { .compatible = "adi,ad5821" },
+ { .compatible = "adi,ad5823" },
{ }
};
MODULE_DEVICE_TABLE(of, ad5820_of_table);
@@ -374,7 +376,7 @@ static SIMPLE_DEV_PM_OPS(ad5820_pm, ad5820_suspend,
ad5820_resume);
static struct i2c_driver ad5820_i2c_driver = {
.driver = {
- .name = AD5820_NAME,
+ .name = "ad5820",
.pm = _pm,
.of_match_table = ad5820_of_table,
},
--
2.23.0
[PATCH v7 4/6] ad5820: Add support for of-autoload
Since kernel 4.16, i2c devices with DT compatible tag are modprobed
using their DT modalias.
Without this patch, if this driver is build as module it would never
be autoprobed.
There is no need to mask it with CONFIG_OF to allow ACPI loading, this
also builds find with CONFIG_OF=n.
Signed-off-by: Ricardo Ribalda Delgado
Acked-by: Pavel Machek
Cc: Sakari Ailus
---
drivers/media/i2c/ad5820.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
index 76aab651f217..5651609e5095 100644
--- a/drivers/media/i2c/ad5820.c
+++ b/drivers/media/i2c/ad5820.c
@@ -364,12 +364,19 @@ static const struct i2c_device_id ad5820_id_table[] = {
};
MODULE_DEVICE_TABLE(i2c, ad5820_id_table);
+static const struct of_device_id ad5820_of_table[] = {
+ { .compatible = "adi,ad5820" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, ad5820_of_table);
+
static SIMPLE_DEV_PM_OPS(ad5820_pm, ad5820_suspend, ad5820_resume);
static struct i2c_driver ad5820_i2c_driver = {
.driver = {
.name = AD5820_NAME,
.pm = _pm,
+ .of_match_table = ad5820_of_table,
},
.probe = ad5820_probe,
.remove = ad5820_remove,
--
2.23.0
[PATCH v7 6/6] media: ad5820: Add support for ad5821 and ad5823
According to the datasheet, both AD5821 and AD5820 share a compatible
register-set:
http://www.analog.com/media/en/technical-documentation/data-sheets/AD5821.pdf
Some camera modules also refer that AD5823 is a replacement of AD5820:
https://download.kamami.com/p564094-OV8865_DS.pdf
Suggested-by: Pavel Machek
Signed-off-by: Ricardo Ribalda Delgado
Acked-by: Pavel Machek
---
drivers/media/i2c/ad5820.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
index 5651609e5095..19c74db0649f 100644
--- a/drivers/media/i2c/ad5820.c
+++ b/drivers/media/i2c/ad5820.c
@@ -25,8 +25,6 @@
#include
#include
-#define AD5820_NAME"ad5820"
-
/* Register definitions */
#define AD5820_POWER_DOWN (1 << 15)
#define AD5820_DAC_SHIFT 4
@@ -359,13 +357,17 @@ static int ad5820_remove(struct i2c_client *client)
}
static const struct i2c_device_id ad5820_id_table[] = {
- { AD5820_NAME, 0 },
+ { "ad5820", 0 },
+ { "ad5821", 0 },
+ { "ad5823", 0 },
{ }
};
MODULE_DEVICE_TABLE(i2c, ad5820_id_table);
static const struct of_device_id ad5820_of_table[] = {
{ .compatible = "adi,ad5820" },
+ { .compatible = "adi,ad5821" },
+ { .compatible = "adi,ad5823" },
{ }
};
MODULE_DEVICE_TABLE(of, ad5820_of_table);
@@ -374,7 +376,7 @@ static SIMPLE_DEV_PM_OPS(ad5820_pm, ad5820_suspend,
ad5820_resume);
static struct i2c_driver ad5820_i2c_driver = {
.driver = {
- .name = AD5820_NAME,
+ .name = "ad5820",
.pm = _pm,
.of_match_table = ad5820_of_table,
},
--
2.23.0
[PATCH v7 4/6] media: ad5820: Add support for of-autoload
Since kernel 4.16, i2c devices with DT compatible tag are modprobed
using their DT modalias.
Without this patch, if this driver is build as module it would never
be autoprobed.
There is no need to mask it with CONFIG_OF to allow ACPI loading, this
also builds find with CONFIG_OF=n.
Signed-off-by: Ricardo Ribalda Delgado
Acked-by: Pavel Machek
Cc: Sakari Ailus
---
drivers/media/i2c/ad5820.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
index 76aab651f217..5651609e5095 100644
--- a/drivers/media/i2c/ad5820.c
+++ b/drivers/media/i2c/ad5820.c
@@ -364,12 +364,19 @@ static const struct i2c_device_id ad5820_id_table[] = {
};
MODULE_DEVICE_TABLE(i2c, ad5820_id_table);
+static const struct of_device_id ad5820_of_table[] = {
+ { .compatible = "adi,ad5820" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, ad5820_of_table);
+
static SIMPLE_DEV_PM_OPS(ad5820_pm, ad5820_suspend, ad5820_resume);
static struct i2c_driver ad5820_i2c_driver = {
.driver = {
.name = AD5820_NAME,
.pm = _pm,
+ .of_match_table = ad5820_of_table,
},
.probe = ad5820_probe,
.remove = ad5820_remove,
--
2.23.0
[PATCH v7 2/6] ad5820: DT new optional field enable-gpios
Document new enable-gpio field. It can be used to disable the part
without turning down its regulator.
Cc: devicet...@vger.kernel.org
Signed-off-by: Ricardo Ribalda Delgado
Acked-by: Pavel Machek
Reviewed-by: Laurent Pinchart
Reviewed-by: Rob Herring
---
Documentation/devicetree/bindings/media/i2c/ad5820.txt | 6 ++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
b/Documentation/devicetree/bindings/media/i2c/ad5820.txt
index 5940ca11c021..db596e8eb0ba 100644
--- a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
+++ b/Documentation/devicetree/bindings/media/i2c/ad5820.txt
@@ -8,6 +8,11 @@ Required Properties:
- VANA-supply: supply of voltage for VANA pin
+Optional properties:
+
+ - enable-gpios : GPIO spec for the XSHUTDOWN pin. The XSHUTDOWN signal is
+active low, a high level on the pin enables the device.
+
Example:
ad5820: coil@c {
@@ -15,5 +20,6 @@ Example:
reg = <0x0c>;
VANA-supply = <>;
+ enable-gpios = < 26 GPIO_ACTIVE_HIGH>;
};
--
2.23.0
[PATCH v7 2/6] media: ad5820: DT new optional field enable-gpios
Document new enable-gpio field. It can be used to disable the part
without turning down its regulator.
Cc: devicet...@vger.kernel.org
Signed-off-by: Ricardo Ribalda Delgado
Acked-by: Pavel Machek
Reviewed-by: Laurent Pinchart
Reviewed-by: Rob Herring
---
Documentation/devicetree/bindings/media/i2c/ad5820.txt | 6 ++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
b/Documentation/devicetree/bindings/media/i2c/ad5820.txt
index 5940ca11c021..db596e8eb0ba 100644
--- a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
+++ b/Documentation/devicetree/bindings/media/i2c/ad5820.txt
@@ -8,6 +8,11 @@ Required Properties:
- VANA-supply: supply of voltage for VANA pin
+Optional properties:
+
+ - enable-gpios : GPIO spec for the XSHUTDOWN pin. The XSHUTDOWN signal is
+active low, a high level on the pin enables the device.
+
Example:
ad5820: coil@c {
@@ -15,5 +20,6 @@ Example:
reg = <0x0c>;
VANA-supply = <>;
+ enable-gpios = < 26 GPIO_ACTIVE_HIGH>;
};
--
2.23.0
[PATCH v7 1/6] media: ad5820: Define entity function
Without this patch, media_device_register_entity throws a warning: dev_warn(mdev->dev, "Entity type for entity %s was not initialized!\n", entity->name); Signed-off-by: Ricardo Ribalda Delgado Acked-by: Pavel Machek Reviewed-by: Laurent Pinchart --- drivers/media/i2c/ad5820.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c index 925c171e7797..7a49651f4d1f 100644 --- a/drivers/media/i2c/ad5820.c +++ b/drivers/media/i2c/ad5820.c @@ -309,6 +309,7 @@ static int ad5820_probe(struct i2c_client *client, v4l2_i2c_subdev_init(>subdev, client, _ops); coil->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; coil->subdev.internal_ops = _internal_ops; + coil->subdev.entity.function = MEDIA_ENT_F_LENS; strscpy(coil->subdev.name, "ad5820 focus", sizeof(coil->subdev.name)); ret = media_entity_pads_init(>subdev.entity, 0, NULL); -- 2.23.0
[PATCH v7 3/6] media: ad5820: Add support for enable pin
This patch adds support for a programmable enable pin. It can be used in
situations where the ANA-vcc is not configurable (dummy-regulator), or
just to have a more fine control of the power saving.
The use of the enable pin is optional.
Signed-off-by: Ricardo Ribalda Delgado
Acked-by: Pavel Machek
---
drivers/media/i2c/Kconfig | 2 +-
drivers/media/i2c/ad5820.c | 17 +
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 78dc64d7b0e7..30d460844dff 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -975,7 +975,7 @@ if MEDIA_CAMERA_SUPPORT
config VIDEO_AD5820
tristate "AD5820 lens voice coil support"
- depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
+ depends on GPIOLIB && I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
help
This is a driver for the AD5820 camera lens voice coil.
It is used for example in Nokia N900 (RX-51).
diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
index 7a49651f4d1f..76aab651f217 100644
--- a/drivers/media/i2c/ad5820.c
+++ b/drivers/media/i2c/ad5820.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include
#include
@@ -47,6 +48,8 @@ struct ad5820_device {
u32 focus_ramp_time;
u32 focus_ramp_mode;
+ struct gpio_desc *enable_gpio;
+
struct mutex power_lock;
int power_count;
@@ -114,6 +117,8 @@ static int ad5820_power_off(struct ad5820_device *coil,
bool standby)
ret = ad5820_update_hw(coil);
}
+ gpiod_set_value_cansleep(coil->enable_gpio, 0);
+
ret2 = regulator_disable(coil->vana);
if (ret)
return ret;
@@ -128,6 +133,8 @@ static int ad5820_power_on(struct ad5820_device *coil, bool
restore)
if (ret < 0)
return ret;
+ gpiod_set_value_cansleep(coil->enable_gpio, 1);
+
if (restore) {
/* Restore the hardware settings. */
coil->standby = false;
@@ -138,6 +145,7 @@ static int ad5820_power_on(struct ad5820_device *coil, bool
restore)
return 0;
fail:
+ gpiod_set_value_cansleep(coil->enable_gpio, 0);
coil->standby = true;
regulator_disable(coil->vana);
@@ -304,6 +312,15 @@ static int ad5820_probe(struct i2c_client *client,
return ret;
}
+ coil->enable_gpio = devm_gpiod_get_optional(>dev, "enable",
+ GPIOD_OUT_LOW);
+ if (IS_ERR(coil->enable_gpio)) {
+ ret = PTR_ERR(coil->enable_gpio);
+ if (ret != -EPROBE_DEFER)
+ dev_err(>dev, "could not get enable gpio\n");
+ return ret;
+ }
+
mutex_init(>power_lock);
v4l2_i2c_subdev_init(>subdev, client, _ops);
--
2.23.0
[PATCH v7 5/6] ad5820: DT new compatible devices
Document new compatible devices. Cc: devicet...@vger.kernel.org Signed-off-by: Ricardo Ribalda Delgado Reviewed-by: Rob Herring Acked-by: Pavel Machek --- Documentation/devicetree/bindings/media/i2c/ad5820.txt | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/media/i2c/ad5820.txt b/Documentation/devicetree/bindings/media/i2c/ad5820.txt index db596e8eb0ba..5764cbedf9b7 100644 --- a/Documentation/devicetree/bindings/media/i2c/ad5820.txt +++ b/Documentation/devicetree/bindings/media/i2c/ad5820.txt @@ -2,7 +2,10 @@ Required Properties: - - compatible: Must contain "adi,ad5820" + - compatible: Must contain one of: + - "adi,ad5820" + - "adi,ad5821" + - "adi,ad5823" - reg: I2C slave address -- 2.23.0
[PATCH v7 0/6] ad5820: Multiple fixes
-Support for enable-pin, of-autoload, enable-gpios and ad5821 and ad5823 For some reason these patchset was lost in translation for a year ;) v7: Rebase on current media/master Ricardo Ribalda Delgado (6): media: ad5820: Define entity function media: ad5820: DT new optional field enable-gpios media: ad5820: Add support for enable pin media: ad5820: Add support for of-autoload media: ad5820: DT new compatible devices media: ad5820: Add support for ad5821 and ad5823 .../devicetree/bindings/media/i2c/ad5820.txt | 11 +- drivers/media/i2c/Kconfig | 2 +- drivers/media/i2c/ad5820.c| 35 --- 3 files changed, 42 insertions(+), 6 deletions(-) -- 2.23.0
[PATCH v7 1/6] ad5820: Define entity function
Without this patch, media_device_register_entity throws a warning: dev_warn(mdev->dev, "Entity type for entity %s was not initialized!\n", entity->name); Signed-off-by: Ricardo Ribalda Delgado Acked-by: Pavel Machek Reviewed-by: Laurent Pinchart --- drivers/media/i2c/ad5820.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c index 925c171e7797..7a49651f4d1f 100644 --- a/drivers/media/i2c/ad5820.c +++ b/drivers/media/i2c/ad5820.c @@ -309,6 +309,7 @@ static int ad5820_probe(struct i2c_client *client, v4l2_i2c_subdev_init(>subdev, client, _ops); coil->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; coil->subdev.internal_ops = _internal_ops; + coil->subdev.entity.function = MEDIA_ENT_F_LENS; strscpy(coil->subdev.name, "ad5820 focus", sizeof(coil->subdev.name)); ret = media_entity_pads_init(>subdev.entity, 0, NULL); -- 2.23.0
[PATCH v11 6/8] Documentation: media: Describe V4L2_CID_UNIT_CELL_SIZE
New control to pass to userspace the width/height of a pixel. Which is needed for calibration and lens selection. Reviewed-by: Philipp Zabel Signed-off-by: Ricardo Ribalda Delgado --- .../media/uapi/v4l/ext-ctrls-image-source.rst | 10 ++ 1 file changed, 10 insertions(+) diff --git a/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst b/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst index 2c3ab5796d76..2d3e2b83d6dd 100644 --- a/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst +++ b/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst @@ -55,3 +55,13 @@ Image Source Control IDs ``V4L2_CID_TEST_PATTERN_GREENB (integer)`` Test pattern green (next to blue) colour component. + +``V4L2_CID_UNIT_CELL_SIZE (struct)`` +This control returns the unit cell size in nanometers. The struct +:c:type:`v4l2_area` provides the width and the height in separate +fields to take into consideration asymmetric pixels. +This control does not take into consideration any possible hardware +binning. +The unit cell consists of the whole area of the pixel, sensitive and +non-sensitive. +This control is required for automatic calibration of sensors/cameras. -- 2.23.0
Re: [PATCH v10 6/8] Documentation: media: Describe V4L2_CID_UNIT_CELL_SIZE
Hi Hans On Mon, Oct 7, 2019 at 2:01 PM Hans Verkuil wrote: > > On 10/7/19 1:35 PM, Ricardo Ribalda Delgado wrote: > > New control to pass to userspace the width/height of a pixel. Which is > > needed for calibration and lens selection. > > > > Reviewed-by: Philipp Zabel > > Signed-off-by: Ricardo Ribalda Delgado > > --- > > Documentation/media/uapi/v4l/ext-ctrls-image-source.rst | 9 + > > 1 file changed, 9 insertions(+) > > > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst > > b/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst > > index 2c3ab5796d76..6388668855d0 100644 > > --- a/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst > > @@ -55,3 +55,12 @@ Image Source Control IDs > > > > ``V4L2_CID_TEST_PATTERN_GREENB (integer)`` > > Test pattern green (next to blue) colour component. > > + > > +``V4L2_CID_UNIT_CELL_SIZE (struct)`` > > +This control returns the unit cell size in nanometers. The struct > > +:c:type:`v4l2_area` provides the width and the height in separate > > +fields to take into consideration asymmetric pixels and/or hardware > > +binning. > > This still states that this control takes binning into account. I understood > that > we decided not to do that? > Good catch, seems that at some point I changed my mind :). Will fix this doc. Can I resend only this patch to avoid spamming the list? > Regards, > > Hans > > > +The unit cell consists of the whole area of the pixel, sensitive and > > +non-sensitive. > > +This control is required for automatic calibration of sensors/cameras. > > >
[PATCH v10 2/8] Documentation: v4l2_ctrl_new_std_compound
From: Ricardo Ribalda Delgado Function for initializing compound controls with a default value. Suggested-by: Hans Verkuil Reviewed-by: Jacopo Mondi Signed-off-by: Ricardo Ribalda Delgado --- Documentation/media/kapi/v4l2-controls.rst | 9 + 1 file changed, 9 insertions(+) diff --git a/Documentation/media/kapi/v4l2-controls.rst b/Documentation/media/kapi/v4l2-controls.rst index ebe2a55908be..b20800cae3f2 100644 --- a/Documentation/media/kapi/v4l2-controls.rst +++ b/Documentation/media/kapi/v4l2-controls.rst @@ -140,6 +140,15 @@ Menu controls with a driver specific menu are added by calling const struct v4l2_ctrl_ops *ops, u32 id, s32 max, s32 skip_mask, s32 def, const char * const *qmenu); +Standard compound controls can be added by calling +:c:func:`v4l2_ctrl_new_std_compound`: + +.. code-block:: c + + struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, + const struct v4l2_ctrl_ops *ops, u32 id, + const union v4l2_ctrl_ptr p_def); + Integer menu controls with a driver specific menu can be added by calling :c:func:`v4l2_ctrl_new_int_menu`: -- 2.23.0
[PATCH v10 1/8] media: v4l2-core: Implement v4l2_ctrl_new_std_compound
From: Ricardo Ribalda Delgado
Currently compound controls do not have a simple way of initializing its
values. This results in ofuscated code with type_ops init.
This patch introduces a new field on the control with the default value
for the compound control that can be set with the brand new
v4l2_ctrl_new_std_compound function
Suggested-by: Hans Verkuil
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/v4l2-core/v4l2-ctrls.c | 50
include/media/v4l2-ctrls.h | 21
2 files changed, 64 insertions(+), 7 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index 1d8f38824631..219d8aeefa20 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -29,6 +29,8 @@
#define call_op(master, op) \
(has_op(master, op) ? master->ops->op(master) : 0)
+static const union v4l2_ctrl_ptr ptr_null;
+
/* Internal temporary helper struct, one for each v4l2_ext_control */
struct v4l2_ctrl_helper {
/* Pointer to the control reference of the master control */
@@ -1530,7 +1532,10 @@ static void std_init_compound(const struct v4l2_ctrl
*ctrl, u32 idx,
struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
void *p = ptr.p + idx * ctrl->elem_size;
- memset(p, 0, ctrl->elem_size);
+ if (ctrl->p_def.p)
+ memcpy(p, ctrl->p_def.p, ctrl->elem_size);
+ else
+ memset(p, 0, ctrl->elem_size);
/*
* The cast is needed to get rid of a gcc warning complaining that
@@ -2354,7 +2359,8 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
v4l2_ctrl_handler *hdl,
s64 min, s64 max, u64 step, s64 def,
const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size,
u32 flags, const char * const *qmenu,
- const s64 *qmenu_int, void *priv)
+ const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def,
+ void *priv)
{
struct v4l2_ctrl *ctrl;
unsigned sz_extra;
@@ -2460,6 +2466,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
v4l2_ctrl_handler *hdl,
is_array)
sz_extra += 2 * tot_ctrl_size;
+ if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p)
+ sz_extra += elem_size;
+
ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL);
if (ctrl == NULL) {
handler_set_err(hdl, -ENOMEM);
@@ -2503,6 +2512,12 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
v4l2_ctrl_handler *hdl,
ctrl->p_new.p = >val;
ctrl->p_cur.p = >cur.val;
}
+
+ if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) {
+ ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size;
+ memcpy(ctrl->p_def.p, p_def.p, elem_size);
+ }
+
for (idx = 0; idx < elems; idx++) {
ctrl->type_ops->init(ctrl, idx, ctrl->p_cur);
ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
@@ -2554,7 +2569,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct
v4l2_ctrl_handler *hdl,
type, min, max,
is_menu ? cfg->menu_skip_mask : step, def,
cfg->dims, cfg->elem_size,
- flags, qmenu, qmenu_int, priv);
+ flags, qmenu, qmenu_int, ptr_null, priv);
if (ctrl)
ctrl->is_private = cfg->is_private;
return ctrl;
@@ -2579,7 +2594,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct
v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
min, max, step, def, NULL, 0,
-flags, NULL, NULL, NULL);
+flags, NULL, NULL, ptr_null, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_std);
@@ -2612,7 +2627,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct
v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
0, max, mask, def, NULL, 0,
-flags, qmenu, qmenu_int, NULL);
+flags, qmenu, qmenu_int, ptr_null, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_std_menu);
@@ -2644,11 +2659,32 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct
v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
0, max, mask, def, NULL, 0,
-flags, qmenu, NULL, NULL);
+flags, qmenu, NULL, ptr_null, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
+/* Helper function for standard compound controls */
+struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
+
[PATCH v10 7/8] media: v4l2-ctrl: Add new helper v4l2_ctrl_ptr_create
This helper function simplifies the code by not needing a union
v4l2_ctrl_ptr and an assignment every time we need to use
a ctrl_ptr.
Suggested-by: Hans Verkuil
Signed-off-by: Ricardo Ribalda Delgado
---
include/media/v4l2-ctrls.h | 12
1 file changed, 12 insertions(+)
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index c42f164e2c9e..0bfedd3d70a6 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -73,6 +73,18 @@ union v4l2_ctrl_ptr {
void *p;
};
+/**
+ * v4l2_ctrl_ptr_create() - Helper function to return a v4l2_ctrl_ptr from a
+ * void pointer
+ * @ptr: The void pointer
+ */
+static inline union v4l2_ctrl_ptr v4l2_ctrl_ptr_create(void *ptr)
+{
+ union v4l2_ctrl_ptr p = { .p = ptr };
+
+ return p;
+}
+
/**
* struct v4l2_ctrl_ops - The control operations that the driver has to
provide.
*
--
2.23.0
[PATCH v10 5/8] media: add V4L2_CID_UNIT_CELL_SIZE control
This control returns the unit cell size in nanometres. The struct provides
the width and the height in separated fields to take into consideration
asymmetric pixels and/or hardware binning.
This control is required for automatic calibration of sensors/cameras.
Reviewed-by: Philipp Zabel
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/v4l2-core/v4l2-ctrls.c | 5 +
include/uapi/linux/v4l2-controls.h | 1 +
2 files changed, 6 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index 96cab2e173d3..bf50d37ef6c1 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -996,6 +996,7 @@ const char *v4l2_ctrl_get_name(u32 id)
case V4L2_CID_AUTO_FOCUS_RANGE: return "Auto Focus, Range";
case V4L2_CID_PAN_SPEED:return "Pan, Speed";
case V4L2_CID_TILT_SPEED: return "Tilt, Speed";
+ case V4L2_CID_UNIT_CELL_SIZE: return "Unit Cell Size";
/* FM Radio Modulator controls */
/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1377,6 +1378,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
v4l2_ctrl_type *type,
case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER:
*type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER;
break;
+ case V4L2_CID_UNIT_CELL_SIZE:
+ *type = V4L2_CTRL_TYPE_AREA;
+ *flags |= V4L2_CTRL_FLAG_READ_ONLY;
+ break;
default:
*type = V4L2_CTRL_TYPE_INTEGER;
break;
diff --git a/include/uapi/linux/v4l2-controls.h
b/include/uapi/linux/v4l2-controls.h
index a2669b79b294..5a7bedee2b0e 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1034,6 +1034,7 @@ enum v4l2_jpeg_chroma_subsampling {
#define V4L2_CID_TEST_PATTERN_GREENR
(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 5)
#define V4L2_CID_TEST_PATTERN_BLUE
(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 6)
#define V4L2_CID_TEST_PATTERN_GREENB
(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 7)
+#define V4L2_CID_UNIT_CELL_SIZE
(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 8)
/* Image processing controls */
--
2.23.0
[PATCH v10 8/8] media: imx214: Add new control with V4L2_CID_UNIT_CELL_SIZE
According to the product brief, the unit cell size is 1120 nanometers^2.
https://www.sony-semicon.co.jp/products_en/IS/sensor1/img/products/ProductBrief_IMX214_20150428.pdf
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/i2c/imx214.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 159a3a604f0e..adcaaa8c86d1 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -47,6 +47,7 @@ struct imx214 {
struct v4l2_ctrl *pixel_rate;
struct v4l2_ctrl *link_freq;
struct v4l2_ctrl *exposure;
+ struct v4l2_ctrl *unit_size;
struct regulator_bulk_data supplies[IMX214_NUM_SUPPLIES];
@@ -948,6 +949,10 @@ static int imx214_probe(struct i2c_client *client)
static const s64 link_freq[] = {
IMX214_DEFAULT_LINK_FREQ,
};
+ static const struct v4l2_area unit_size = {
+ .width = 1120,
+ .height = 1120,
+ };
int ret;
ret = imx214_parse_fwnode(dev);
@@ -1029,6 +1034,10 @@ static int imx214_probe(struct i2c_client *client)
V4L2_CID_EXPOSURE,
0, 3184, 1, 0x0c70);
+ imx214->unit_size = v4l2_ctrl_new_std_compound(>ctrls,
+ NULL,
+ V4L2_CID_UNIT_CELL_SIZE,
+ v4l2_ctrl_ptr_create((void *)_size));
ret = imx214->ctrls.error;
if (ret) {
dev_err(>dev, "%s control init failed (%d)\n",
--
2.23.0
[PATCH v10 4/8] Documentation: media: Document V4L2_CTRL_TYPE_AREA
A struct v4l2_area containing the width and the height of a rectangular area. Reviewed-by: Jacopo Mondi Reviewed-by: Philipp Zabel Signed-off-by: Ricardo Ribalda Delgado --- Documentation/media/uapi/v4l/vidioc-queryctrl.rst | 6 ++ 1 file changed, 6 insertions(+) diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst index a3d56ffbf4cc..33aff21b7d11 100644 --- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst +++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst @@ -443,6 +443,12 @@ See also the examples in :ref:`control`. - n/a - A struct :c:type:`v4l2_ctrl_mpeg2_quantization`, containing MPEG-2 quantization matrices for stateless video decoders. +* - ``V4L2_CTRL_TYPE_AREA`` + - n/a + - n/a + - n/a + - A struct :c:type:`v4l2_area`, containing the width and the height +of a rectangular area. Units depend on the use case. * - ``V4L2_CTRL_TYPE_H264_SPS`` - n/a - n/a -- 2.23.0
[PATCH v10 3/8] media: add V4L2_CTRL_TYPE_AREA control type
From: Ricardo Ribalda Delgado
This type contains the width and the height of a rectangular area.
Reviewed-by: Jacopo Mondi
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/v4l2-core/v4l2-ctrls.c | 21 ++
include/media/v4l2-ctrls.h | 42
include/uapi/linux/videodev2.h | 6
3 files changed, 69 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index 219d8aeefa20..96cab2e173d3 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1677,6 +1677,7 @@ static int std_validate_compound(const struct v4l2_ctrl
*ctrl, u32 idx,
{
struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
+ struct v4l2_area *area;
void *p = ptr.p + idx * ctrl->elem_size;
switch ((u32)ctrl->type) {
@@ -1753,6 +1754,11 @@ static int std_validate_compound(const struct v4l2_ctrl
*ctrl, u32 idx,
zero_padding(p_vp8_frame_header->entropy_header);
zero_padding(p_vp8_frame_header->coder_state);
break;
+ case V4L2_CTRL_TYPE_AREA:
+ area = p;
+ if (!area->width || !area->height)
+ return -EINVAL;
+ break;
default:
return -EINVAL;
}
@@ -2427,6 +2433,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
v4l2_ctrl_handler *hdl,
case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header);
break;
+ case V4L2_CTRL_TYPE_AREA:
+ elem_size = sizeof(struct v4l2_area);
+ break;
default:
if (type < V4L2_CTRL_COMPOUND_TYPES)
elem_size = sizeof(s32);
@@ -4116,6 +4125,18 @@ int __v4l2_ctrl_s_ctrl_string(struct v4l2_ctrl *ctrl,
const char *s)
}
EXPORT_SYMBOL(__v4l2_ctrl_s_ctrl_string);
+int __v4l2_ctrl_s_ctrl_area(struct v4l2_ctrl *ctrl,
+ const struct v4l2_area *area)
+{
+ lockdep_assert_held(ctrl->handler->lock);
+
+ /* It's a driver bug if this happens. */
+ WARN_ON(ctrl->type != V4L2_CTRL_TYPE_AREA);
+ *ctrl->p_new.p_area = *area;
+ return set_ctrl(NULL, ctrl, 0);
+}
+EXPORT_SYMBOL(__v4l2_ctrl_s_ctrl_area);
+
void v4l2_ctrl_request_complete(struct media_request *req,
struct v4l2_ctrl_handler *main_hdl)
{
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 90a8ee48c2f3..c42f164e2c9e 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -50,6 +50,7 @@ struct poll_table_struct;
* @p_h264_slice_params: Pointer to a struct v4l2_ctrl_h264_slice_params.
* @p_h264_decode_params: Pointer to a struct
v4l2_ctrl_h264_decode_params.
* @p_vp8_frame_header:Pointer to a VP8 frame header structure.
+ * @p_area:Pointer to an area.
* @p: Pointer to a compound value.
*/
union v4l2_ctrl_ptr {
@@ -68,6 +69,7 @@ union v4l2_ctrl_ptr {
struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
struct v4l2_ctrl_h264_decode_params *p_h264_decode_params;
struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
+ struct v4l2_area *p_area;
void *p;
};
@@ -1086,6 +1088,46 @@ static inline int v4l2_ctrl_s_ctrl_string(struct
v4l2_ctrl *ctrl, const char *s)
return rval;
}
+/**
+ * __v4l2_ctrl_s_ctrl_area() - Unlocked variant of v4l2_ctrl_s_ctrl_area().
+ *
+ * @ctrl: The control.
+ * @area: The new area.
+ *
+ * This sets the control's new area safely by going through the control
+ * framework. This function assumes the control's handler is already locked,
+ * allowing it to be used from within the _ctrl_ops functions.
+ *
+ * This function is for area type controls only.
+ */
+int __v4l2_ctrl_s_ctrl_area(struct v4l2_ctrl *ctrl,
+ const struct v4l2_area *area);
+
+/**
+ * v4l2_ctrl_s_ctrl_area() - Helper function to set a control's area value
+ * from within a driver.
+ *
+ * @ctrl: The control.
+ * @s: The new area.
+ *
+ * This sets the control's new area safely by going through the control
+ * framework. This function will lock the control's handler, so it cannot be
+ * used from within the _ctrl_ops functions.
+ *
+ * This function is for area type controls only.
+ */
+static inline int v4l2_ctrl_s_ctrl_area(struct v4l2_ctrl *ctrl,
+ const struct v4l2_area *area)
+{
+ int rval;
+
+ v4l2_ctrl_lock(ctrl);
+ rval = __v4l2_ctrl_s_ctrl_area(ctrl, area);
+ v4l2_ctrl_unlock(ctrl);
+
+ return rval;
+}
+
/* Internal helper functions that deal with control events. */
extern const struct v4l2_subscribed_event_ops v
[PATCH v10 0/8] Implement UNIT_CELL_SIZE control
UNIT_CELL_SIZE is a control that represents the size of a cell (pixel). We required a bit of boilerplate to add this control :) - New way to init compount controls - New control type Thanks to Hans, Jacopo and Philipp for your help. You might want to see the series at my github repository if needed. https://github.com/ribalda/linux/tree/unit-size-v10 v10: Typos in documentation and minor color style v9: Rename helper to v4l2_ctrl_ptr_create v8: Fix my email on some patches (sorry for the mess) v7: Add new helper v4l2_ctrl_ptr_from_void v4, v5 of this patchset never reached the mailing list. Ricardo Ribalda Delgado (8): media: v4l2-core: Implement v4l2_ctrl_new_std_compound Documentation: v4l2_ctrl_new_std_compound media: add V4L2_CTRL_TYPE_AREA control type Documentation: media: Document V4L2_CTRL_TYPE_AREA media: add V4L2_CID_UNIT_CELL_SIZE control Documentation: media: Describe V4L2_CID_UNIT_CELL_SIZE media: v4l2-ctrl: Add new helper v4l2_ctrl_ptr_create media: imx214: Add new control with V4L2_CID_UNIT_CELL_SIZE Documentation/media/kapi/v4l2-controls.rst| 9 +++ .../media/uapi/v4l/ext-ctrls-image-source.rst | 9 +++ .../media/uapi/v4l/vidioc-queryctrl.rst | 6 ++ drivers/media/i2c/imx214.c| 9 +++ drivers/media/v4l2-core/v4l2-ctrls.c | 76 +-- include/media/v4l2-ctrls.h| 75 ++ include/uapi/linux/v4l2-controls.h| 1 + include/uapi/linux/videodev2.h| 6 ++ 8 files changed, 184 insertions(+), 7 deletions(-) -- 2.23.0
[PATCH v10 6/8] Documentation: media: Describe V4L2_CID_UNIT_CELL_SIZE
New control to pass to userspace the width/height of a pixel. Which is needed for calibration and lens selection. Reviewed-by: Philipp Zabel Signed-off-by: Ricardo Ribalda Delgado --- Documentation/media/uapi/v4l/ext-ctrls-image-source.rst | 9 + 1 file changed, 9 insertions(+) diff --git a/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst b/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst index 2c3ab5796d76..6388668855d0 100644 --- a/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst +++ b/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst @@ -55,3 +55,12 @@ Image Source Control IDs ``V4L2_CID_TEST_PATTERN_GREENB (integer)`` Test pattern green (next to blue) colour component. + +``V4L2_CID_UNIT_CELL_SIZE (struct)`` +This control returns the unit cell size in nanometers. The struct +:c:type:`v4l2_area` provides the width and the height in separate +fields to take into consideration asymmetric pixels and/or hardware +binning. +The unit cell consists of the whole area of the pixel, sensitive and +non-sensitive. +This control is required for automatic calibration of sensors/cameras. -- 2.23.0
Re: [PATCH v9 8/8] media: imx214: Add new control with V4L2_CID_UNIT_CELL_SIZE
Hi Hans
On Fri, Oct 4, 2019 at 12:31 PM Hans Verkuil wrote:
>
> On 10/1/19 1:24 PM, Ricardo Ribalda Delgado wrote:
> > According to the product brief, the unit cell size is 1120 nanometers^2.
> >
> > https://www.sony-semicon.co.jp/products_en/IS/sensor1/img/products/ProductBrief_IMX214_20150428.pdf
> >
> > Signed-off-by: Ricardo Ribalda Delgado
> > ---
> > drivers/media/i2c/imx214.c | 9 +
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > index 159a3a604f0e..adcaaa8c86d1 100644
> > --- a/drivers/media/i2c/imx214.c
> > +++ b/drivers/media/i2c/imx214.c
> > @@ -47,6 +47,7 @@ struct imx214 {
> > struct v4l2_ctrl *pixel_rate;
> > struct v4l2_ctrl *link_freq;
> > struct v4l2_ctrl *exposure;
> > + struct v4l2_ctrl *unit_size;
> >
> > struct regulator_bulk_data supplies[IMX214_NUM_SUPPLIES];
> >
> > @@ -948,6 +949,10 @@ static int imx214_probe(struct i2c_client *client)
> > static const s64 link_freq[] = {
> > IMX214_DEFAULT_LINK_FREQ,
> > };
> > + static const struct v4l2_area unit_size = {
> > + .width = 1120,
> > + .height = 1120,
> > + };
> > int ret;
> >
> > ret = imx214_parse_fwnode(dev);
> > @@ -1029,6 +1034,10 @@ static int imx214_probe(struct i2c_client *client)
> >V4L2_CID_EXPOSURE,
> >0, 3184, 1, 0x0c70);
> >
> > + imx214->unit_size = v4l2_ctrl_new_std_compound(>ctrls,
> > + NULL,
> > + V4L2_CID_UNIT_CELL_SIZE,
> > + v4l2_ctrl_ptr_create((void *)_size));
>
> The imx214 supports two modes: 4096x2304 and 1920x1080. I assume that the
> latter is using binning? So shouldn't the unit cell size be different in that
> case?
To be honest I do not know if it is cropping or binning. I can try to
turn on the sensor and figure out if the field of view changes.
>
> I'm not so sure the unit cell size should depend on binning. I'd rather have
> the binning information exposed somehow and let userspace do the calculation.
I think there are good arguments on both directions ;).
The issue with considering the binning is that if there a later step
on the pipeline that does the bining then it has to change also the
unit_cell_size control. I would say, for now lets keep them
independent.
>
> Regards,
>
> Hans
>
> > ret = imx214->ctrls.error;
> > if (ret) {
> > dev_err(>dev, "%s control init failed (%d)\n",
> >
>
Re: [PATCH] media: imx214: Fix stop streaming
Ups sorry about that. Hopefully it works fine also without the
patch, but it needs to be fixed.
On Thu, Oct 3, 2019 at 4:46 PM Daniel Gomez wrote:
>
> Stop video streaming when requested.
>
> When s_stream is called to stop the video streaming, if/else condition calls
> start_streaming function instead of the one for stopping it.
>
> Fixes: 436190596241 ("media: imx214: Add imx214 camera sensor driver")
> Signed-off-by: Daniel Gomez
Signed-off-by: Ricardo Ribalda
> ---
>
> You can find some logs before/after running in the hardware. Notice 0x100
> register is for starting/stopping the video streaming from the imx214 sensor.
>
> * Before patch:
>
> # media-ctl -d /dev/media0 -l
> '"msm_csiphy0":1->"msm_csid0":0[1],"msm_csid0":1->"msm_ispif0":0[1],"msm_ispif0":1->"msm_vfe0_rdi0":0[1]'
> # media-ctl -d /dev/media0 -V '"imx214
> 3-001a":0[fmt:SRGGB10/1920x1080],"msm_csiphy0":0[fmt:SRGGB10/1920x1080],"msm_csid0":0[fmt:SRGGB10/1920x1080],"msm_ispif0":0[fmt:SRGGB10/1920x1080],"msm_vfe0_rdi0":0[fmt:SRGGB10/1920x1080]'
> # yavta -f SRGGB10P -s 1920x1080 -n 1 --capture=5
> /dev/v4l/by-path/platform-a34000.camss-video-index0
> Device /dev/v4l/by-path/platform-a34000.camss-video-index0 opened.
> Device `Qualcomm Camera Subsystem' on `platform:a34000.camss' (driver
> 'qcom-camss') supports video, capture, with mplanes.
> Video format set: SRGGB10P (41415270) 1920x1080 field none, 1 planes:
> * Stride 2400, buffer size 2592000
> Video format: SRGGB10P (41415270) 1920x1080 field none, 1 planes:
> * Stride 2400, buffer size 2592000
> 1 buffers requested.
> length: 1 offset: 4093609832 timestamp type/source: mono/EoF
> Buffer 0/0 mapped at address 0x84b6b000.
> 0 (0) [-] none 0 2592000 B 30.682759 30.705111 4.697 fps ts mono/EoF
> 1 (0) [-] none 1 2592000 B 30.749391 30.771609 15.008 fps ts mono/EoF
> 2 (0) [-] none 2 2592000 B 30.816042 30.838225 15.004 fps ts mono/EoF
> 3 (0) [-] none 3 2592000 B 30.882690 30.904992 15.004 fps ts mono/EoF
> 4 (0) [-] none 4 2592000 B 30.949333 30.971543 15.005 fps ts mono/EoF
> Captured 5 frames in 0.501681 seconds (9.966480 fps, 0.00 B/s).
> 1 buffers released.
> # v4l2-dbg -d /dev/v4l-subdev19 -g 0x100
> ioctl: VIDIOC_DBG_G_REGISTER
> Register 0x0100 = 1h (1d 0001b)
>
> * After patch:
>
> # media-ctl -d /dev/media0 -l
> '"msm_csiphy0":1->"msm_csid0":0[1],"msm_csid0":1->"msm_ispif0":0[1],"msm_ispif0":1->"msm_vfe0_rdi0":0[1]'
> # media-ctl -d /dev/media0 -V '"imx214
> 3-001a":0[fmt:SRGGB10/1920x1080],"msm_csiphy0":0[fmt:SRGGB10/1920x1080],"msm_csid0":0[fmt:SRGGB10/1920x1080],"msm_ispif0":0[fmt:SRGGB10/1920x1080],"msm_vfe0_rdi0":0[fmt:SRGGB10/1920x1080]'
> # yavta -f SRGGB10P -s 1920x1080 -n 1 --capture=5
> /dev/v4l/by-path/platform-a34000.camss-video-index0
> Device /dev/v4l/by-path/platform-a34000.camss-video-index0 opened.
> Device `Qualcomm Camera Subsystem' on `platform:a34000.camss' (driver
> 'qcom-camss') supports video, capture, with mplanes.
> Video format set: SRGGB10P (41415270) 1920x1080 field none, 1 planes:
> * Stride 2400, buffer size 2592000
> Video format: SRGGB10P (41415270) 1920x1080 field none, 1 planes:
> * Stride 2400, buffer size 2592000
> 1 buffers requested.
> length: 1 offset: 3764913896 timestamp type/source: mono/EoF
> Buffer 0/0 mapped at address 0xb62f7000.
> 0 (0) [-] none 0 2592000 B 31.283473 31.306390 4.697 fps ts mono/EoF
> 1 (0) [-] none 1 2592000 B 31.350115 31.372475 15.006 fps ts mono/EoF
> 2 (0) [-] none 2 2592000 B 31.416765 31.439728 15.004 fps ts mono/EoF
> 3 (0) [-] none 3 2592000 B 31.483410 31.505791 15.005 fps ts mono/EoF
> 4 (0) [-] none 4 2592000 B 31.550058 31.573025 15.004 fps ts mono/EoF
> Captured 5 frames in 0.502440 seconds (9.951430 fps, 0.00 B/s).
> 1 buffers released.
> # v4l2-dbg -d /dev/v4l-subdev19 -g 0x100
> ioctl: VIDIOC_DBG_G_REGISTER
> Register 0x0100 = 0h (0d b)
>
> drivers/media/i2c/imx214.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 159a3a604f0e..24659cb0d083 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -785,7 +785,7 @@ static int imx214_s_stream(struct v4l2_subdev *subdev,
> int enable)
> if (ret < 0)
> goto err_rpm_put;
> } else {
> - ret = imx214_start_streaming(imx214);
> + ret = imx214_stop_streaming(imx214);
> if (ret < 0)
> goto err_rpm_put;
> pm_runtime_put(imx214->dev);
> --
> 2.20.1
>
--
Ricardo Ribalda
[PATCH v9 7/8] media: v4l2-ctrl: Add new helper v4l2_ctrl_ptr_create
This helper function simplifies the code by not needing a union
v4l2_ctrl_ptr and an assignment every time we need to use
a ctrl_ptr.
Suggested-by: Hans Verkuil
Signed-off-by: Ricardo Ribalda Delgado
---
include/media/v4l2-ctrls.h | 12
1 file changed, 12 insertions(+)
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index c42f164e2c9e..51d74fa7c7e2 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -73,6 +73,18 @@ union v4l2_ctrl_ptr {
void *p;
};
+/**
+ * v4l2_ctrl_ptr_create() - Helper function to return a v4l2_ctrl_ptr from a
+ * void pointer
+ * @ptr: The void pointer
+ */
+static inline union v4l2_ctrl_ptr v4l2_ctrl_ptr_create(void *ptr)
+{
+ union v4l2_ctrl_ptr p = {.p = ptr};
+
+ return p;
+}
+
/**
* struct v4l2_ctrl_ops - The control operations that the driver has to
provide.
*
--
2.23.0
[PATCH v9 0/8] Implement UNIT_CELL_SIZE control
UNIT_CELL_SIZE is a control that represents the size of a cell (pixel). We required a bit of boilerplate to add this control :) - New way to init compount controls - New control type Thanks to Hans, Jacopo and Philipp for your help. You might want to see the series at my github repository if needed. https://github.com/ribalda/linux/tree/unit-size-v9 v9: Rename helper to v4l2_ctrl_ptr_create v8: Fix my email on some patches (sorry for the mess) v7: Add new helper v4l2_ctrl_ptr_from_void v4, v5 of this patchset never reached the mailing list. Ricardo Ribalda Delgado (8): media: v4l2-core: Implement v4l2_ctrl_new_std_compound Documentation: v4l2_ctrl_new_std_compound media: add V4L2_CTRL_TYPE_AREA control type Documentation: media: Document V4L2_CTRL_TYPE_AREA media: add V4L2_CID_UNIT_CELL_SIZE control Documentation: media: Describe V4L2_CID_UNIT_CELL_SIZE media: v4l2-ctrl: Add new helper v4l2_ctrl_ptr_create media: imx214: Add new control with V4L2_CID_UNIT_CELL_SIZE Documentation/media/kapi/v4l2-controls.rst| 9 +++ .../media/uapi/v4l/ext-ctrls-image-source.rst | 9 +++ .../media/uapi/v4l/vidioc-queryctrl.rst | 6 ++ drivers/media/i2c/imx214.c| 9 +++ drivers/media/v4l2-core/v4l2-ctrls.c | 76 +-- include/media/v4l2-ctrls.h| 75 ++ include/uapi/linux/v4l2-controls.h| 1 + include/uapi/linux/videodev2.h| 6 ++ 8 files changed, 184 insertions(+), 7 deletions(-) -- 2.23.0
[PATCH v9 4/8] Documentation: media: Document V4L2_CTRL_TYPE_AREA
A struct v4l2_area containing the width and the height of a rectangular area. Reviewed-by: Jacopo Mondi Reviewed-by: Philipp Zabel Signed-off-by: Ricardo Ribalda Delgado --- Documentation/media/uapi/v4l/vidioc-queryctrl.rst | 6 ++ 1 file changed, 6 insertions(+) diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst index a3d56ffbf4cc..33aff21b7d11 100644 --- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst +++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst @@ -443,6 +443,12 @@ See also the examples in :ref:`control`. - n/a - A struct :c:type:`v4l2_ctrl_mpeg2_quantization`, containing MPEG-2 quantization matrices for stateless video decoders. +* - ``V4L2_CTRL_TYPE_AREA`` + - n/a + - n/a + - n/a + - A struct :c:type:`v4l2_area`, containing the width and the height +of a rectangular area. Units depend on the use case. * - ``V4L2_CTRL_TYPE_H264_SPS`` - n/a - n/a -- 2.23.0
[PATCH v9 3/8] media: add V4L2_CTRL_TYPE_AREA control type
From: Ricardo Ribalda Delgado
This type contains the width and the height of a rectangular area.
Reviewed-by: Jacopo Mondi
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/v4l2-core/v4l2-ctrls.c | 21 ++
include/media/v4l2-ctrls.h | 42
include/uapi/linux/videodev2.h | 6
3 files changed, 69 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index 219d8aeefa20..b9a46f536406 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1678,6 +1678,7 @@ static int std_validate_compound(const struct v4l2_ctrl
*ctrl, u32 idx,
struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
void *p = ptr.p + idx * ctrl->elem_size;
+ struct v4l2_area *area;
switch ((u32)ctrl->type) {
case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
@@ -1753,6 +1754,11 @@ static int std_validate_compound(const struct v4l2_ctrl
*ctrl, u32 idx,
zero_padding(p_vp8_frame_header->entropy_header);
zero_padding(p_vp8_frame_header->coder_state);
break;
+ case V4L2_CTRL_TYPE_AREA:
+ area = p;
+ if (!area->width || !area->height)
+ return -EINVAL;
+ break;
default:
return -EINVAL;
}
@@ -2427,6 +2433,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
v4l2_ctrl_handler *hdl,
case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header);
break;
+ case V4L2_CTRL_TYPE_AREA:
+ elem_size = sizeof(struct v4l2_area);
+ break;
default:
if (type < V4L2_CTRL_COMPOUND_TYPES)
elem_size = sizeof(s32);
@@ -4116,6 +4125,18 @@ int __v4l2_ctrl_s_ctrl_string(struct v4l2_ctrl *ctrl,
const char *s)
}
EXPORT_SYMBOL(__v4l2_ctrl_s_ctrl_string);
+int __v4l2_ctrl_s_ctrl_area(struct v4l2_ctrl *ctrl,
+ const struct v4l2_area *area)
+{
+ lockdep_assert_held(ctrl->handler->lock);
+
+ /* It's a driver bug if this happens. */
+ WARN_ON(ctrl->type != V4L2_CTRL_TYPE_AREA);
+ memcpy(ctrl->p_new.p_area, area, sizeof(*area));
+ return set_ctrl(NULL, ctrl, 0);
+}
+EXPORT_SYMBOL(__v4l2_ctrl_s_ctrl_area);
+
void v4l2_ctrl_request_complete(struct media_request *req,
struct v4l2_ctrl_handler *main_hdl)
{
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 90a8ee48c2f3..c42f164e2c9e 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -50,6 +50,7 @@ struct poll_table_struct;
* @p_h264_slice_params: Pointer to a struct v4l2_ctrl_h264_slice_params.
* @p_h264_decode_params: Pointer to a struct
v4l2_ctrl_h264_decode_params.
* @p_vp8_frame_header:Pointer to a VP8 frame header structure.
+ * @p_area:Pointer to an area.
* @p: Pointer to a compound value.
*/
union v4l2_ctrl_ptr {
@@ -68,6 +69,7 @@ union v4l2_ctrl_ptr {
struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
struct v4l2_ctrl_h264_decode_params *p_h264_decode_params;
struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
+ struct v4l2_area *p_area;
void *p;
};
@@ -1086,6 +1088,46 @@ static inline int v4l2_ctrl_s_ctrl_string(struct
v4l2_ctrl *ctrl, const char *s)
return rval;
}
+/**
+ * __v4l2_ctrl_s_ctrl_area() - Unlocked variant of v4l2_ctrl_s_ctrl_area().
+ *
+ * @ctrl: The control.
+ * @area: The new area.
+ *
+ * This sets the control's new area safely by going through the control
+ * framework. This function assumes the control's handler is already locked,
+ * allowing it to be used from within the _ctrl_ops functions.
+ *
+ * This function is for area type controls only.
+ */
+int __v4l2_ctrl_s_ctrl_area(struct v4l2_ctrl *ctrl,
+ const struct v4l2_area *area);
+
+/**
+ * v4l2_ctrl_s_ctrl_area() - Helper function to set a control's area value
+ * from within a driver.
+ *
+ * @ctrl: The control.
+ * @s: The new area.
+ *
+ * This sets the control's new area safely by going through the control
+ * framework. This function will lock the control's handler, so it cannot be
+ * used from within the _ctrl_ops functions.
+ *
+ * This function is for area type controls only.
+ */
+static inline int v4l2_ctrl_s_ctrl_area(struct v4l2_ctrl *ctrl,
+ const struct v4l2_area *area)
+{
+ int rval;
+
+ v4l2_ctrl_lock(ctrl);
+ rval = __v4l2_ctrl_s_ctrl_area(ctrl, area);
+ v4l2_ctrl_unlock(ctrl);
+
+ return rval;
+}
+
/* Internal helper functions that deal with
[PATCH v9 6/8] Documentation: media: Describe V4L2_CID_UNIT_CELL_SIZE
New control to pass to userspace the width/height of a pixel. Which is needed for calibration and lens selection. Reviewed-by: Philipp Zabel Signed-off-by: Ricardo Ribalda Delgado --- Documentation/media/uapi/v4l/ext-ctrls-image-source.rst | 9 + 1 file changed, 9 insertions(+) diff --git a/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst b/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst index 2c3ab5796d76..033672dcb43d 100644 --- a/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst +++ b/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst @@ -55,3 +55,12 @@ Image Source Control IDs ``V4L2_CID_TEST_PATTERN_GREENB (integer)`` Test pattern green (next to blue) colour component. + +``V4L2_CID_UNIT_CELL_SIZE (struct)`` +This control returns the unit cell size in nanometres. The struct +:c:type:`v4l2_area` provides the width and the height in separated +fields to take into consideration asymmetric pixels and/or hardware +binning. +The unit cell consists of the whole area of the pixel, sensitive and +non-sensitive. +This control is required for automatic calibration sensors/cameras. -- 2.23.0
[PATCH v9 2/8] Documentation: v4l2_ctrl_new_std_compound
From: Ricardo Ribalda Delgado Function for initializing compound controls with a default value. Suggested-by: Hans Verkuil Reviewed-by: Jacopo Mondi Signed-off-by: Ricardo Ribalda Delgado --- Documentation/media/kapi/v4l2-controls.rst | 9 + 1 file changed, 9 insertions(+) diff --git a/Documentation/media/kapi/v4l2-controls.rst b/Documentation/media/kapi/v4l2-controls.rst index ebe2a55908be..b20800cae3f2 100644 --- a/Documentation/media/kapi/v4l2-controls.rst +++ b/Documentation/media/kapi/v4l2-controls.rst @@ -140,6 +140,15 @@ Menu controls with a driver specific menu are added by calling const struct v4l2_ctrl_ops *ops, u32 id, s32 max, s32 skip_mask, s32 def, const char * const *qmenu); +Standard compound controls can be added by calling +:c:func:`v4l2_ctrl_new_std_compound`: + +.. code-block:: c + + struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, + const struct v4l2_ctrl_ops *ops, u32 id, + const union v4l2_ctrl_ptr p_def); + Integer menu controls with a driver specific menu can be added by calling :c:func:`v4l2_ctrl_new_int_menu`: -- 2.23.0
[PATCH v9 5/8] media: add V4L2_CID_UNIT_CELL_SIZE control
This control returns the unit cell size in nanometres. The struct provides
the width and the height in separated fields to take into consideration
asymmetric pixels and/or hardware binning.
This control is required for automatic calibration of sensors/cameras.
Reviewed-by: Philipp Zabel
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/v4l2-core/v4l2-ctrls.c | 5 +
include/uapi/linux/v4l2-controls.h | 1 +
2 files changed, 6 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index b9a46f536406..f626f9983408 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -996,6 +996,7 @@ const char *v4l2_ctrl_get_name(u32 id)
case V4L2_CID_AUTO_FOCUS_RANGE: return "Auto Focus, Range";
case V4L2_CID_PAN_SPEED:return "Pan, Speed";
case V4L2_CID_TILT_SPEED: return "Tilt, Speed";
+ case V4L2_CID_UNIT_CELL_SIZE: return "Unit Cell Size";
/* FM Radio Modulator controls */
/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1377,6 +1378,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
v4l2_ctrl_type *type,
case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER:
*type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER;
break;
+ case V4L2_CID_UNIT_CELL_SIZE:
+ *type = V4L2_CTRL_TYPE_AREA;
+ *flags |= V4L2_CTRL_FLAG_READ_ONLY;
+ break;
default:
*type = V4L2_CTRL_TYPE_INTEGER;
break;
diff --git a/include/uapi/linux/v4l2-controls.h
b/include/uapi/linux/v4l2-controls.h
index a2669b79b294..5a7bedee2b0e 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1034,6 +1034,7 @@ enum v4l2_jpeg_chroma_subsampling {
#define V4L2_CID_TEST_PATTERN_GREENR
(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 5)
#define V4L2_CID_TEST_PATTERN_BLUE
(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 6)
#define V4L2_CID_TEST_PATTERN_GREENB
(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 7)
+#define V4L2_CID_UNIT_CELL_SIZE
(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 8)
/* Image processing controls */
--
2.23.0
[PATCH v9 8/8] media: imx214: Add new control with V4L2_CID_UNIT_CELL_SIZE
According to the product brief, the unit cell size is 1120 nanometers^2.
https://www.sony-semicon.co.jp/products_en/IS/sensor1/img/products/ProductBrief_IMX214_20150428.pdf
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/i2c/imx214.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 159a3a604f0e..adcaaa8c86d1 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -47,6 +47,7 @@ struct imx214 {
struct v4l2_ctrl *pixel_rate;
struct v4l2_ctrl *link_freq;
struct v4l2_ctrl *exposure;
+ struct v4l2_ctrl *unit_size;
struct regulator_bulk_data supplies[IMX214_NUM_SUPPLIES];
@@ -948,6 +949,10 @@ static int imx214_probe(struct i2c_client *client)
static const s64 link_freq[] = {
IMX214_DEFAULT_LINK_FREQ,
};
+ static const struct v4l2_area unit_size = {
+ .width = 1120,
+ .height = 1120,
+ };
int ret;
ret = imx214_parse_fwnode(dev);
@@ -1029,6 +1034,10 @@ static int imx214_probe(struct i2c_client *client)
V4L2_CID_EXPOSURE,
0, 3184, 1, 0x0c70);
+ imx214->unit_size = v4l2_ctrl_new_std_compound(>ctrls,
+ NULL,
+ V4L2_CID_UNIT_CELL_SIZE,
+ v4l2_ctrl_ptr_create((void *)_size));
ret = imx214->ctrls.error;
if (ret) {
dev_err(>dev, "%s control init failed (%d)\n",
--
2.23.0
[PATCH v9 1/8] media: v4l2-core: Implement v4l2_ctrl_new_std_compound
From: Ricardo Ribalda Delgado
Currently compound controls do not have a simple way of initializing its
values. This results in ofuscated code with type_ops init.
This patch introduces a new field on the control with the default value
for the compound control that can be set with the brand new
v4l2_ctrl_new_std_compound function
Suggested-by: Hans Verkuil
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/v4l2-core/v4l2-ctrls.c | 50
include/media/v4l2-ctrls.h | 21
2 files changed, 64 insertions(+), 7 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index 1d8f38824631..219d8aeefa20 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -29,6 +29,8 @@
#define call_op(master, op) \
(has_op(master, op) ? master->ops->op(master) : 0)
+static const union v4l2_ctrl_ptr ptr_null;
+
/* Internal temporary helper struct, one for each v4l2_ext_control */
struct v4l2_ctrl_helper {
/* Pointer to the control reference of the master control */
@@ -1530,7 +1532,10 @@ static void std_init_compound(const struct v4l2_ctrl
*ctrl, u32 idx,
struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
void *p = ptr.p + idx * ctrl->elem_size;
- memset(p, 0, ctrl->elem_size);
+ if (ctrl->p_def.p)
+ memcpy(p, ctrl->p_def.p, ctrl->elem_size);
+ else
+ memset(p, 0, ctrl->elem_size);
/*
* The cast is needed to get rid of a gcc warning complaining that
@@ -2354,7 +2359,8 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
v4l2_ctrl_handler *hdl,
s64 min, s64 max, u64 step, s64 def,
const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size,
u32 flags, const char * const *qmenu,
- const s64 *qmenu_int, void *priv)
+ const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def,
+ void *priv)
{
struct v4l2_ctrl *ctrl;
unsigned sz_extra;
@@ -2460,6 +2466,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
v4l2_ctrl_handler *hdl,
is_array)
sz_extra += 2 * tot_ctrl_size;
+ if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p)
+ sz_extra += elem_size;
+
ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL);
if (ctrl == NULL) {
handler_set_err(hdl, -ENOMEM);
@@ -2503,6 +2512,12 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
v4l2_ctrl_handler *hdl,
ctrl->p_new.p = >val;
ctrl->p_cur.p = >cur.val;
}
+
+ if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) {
+ ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size;
+ memcpy(ctrl->p_def.p, p_def.p, elem_size);
+ }
+
for (idx = 0; idx < elems; idx++) {
ctrl->type_ops->init(ctrl, idx, ctrl->p_cur);
ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
@@ -2554,7 +2569,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct
v4l2_ctrl_handler *hdl,
type, min, max,
is_menu ? cfg->menu_skip_mask : step, def,
cfg->dims, cfg->elem_size,
- flags, qmenu, qmenu_int, priv);
+ flags, qmenu, qmenu_int, ptr_null, priv);
if (ctrl)
ctrl->is_private = cfg->is_private;
return ctrl;
@@ -2579,7 +2594,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct
v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
min, max, step, def, NULL, 0,
-flags, NULL, NULL, NULL);
+flags, NULL, NULL, ptr_null, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_std);
@@ -2612,7 +2627,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct
v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
0, max, mask, def, NULL, 0,
-flags, qmenu, qmenu_int, NULL);
+flags, qmenu, qmenu_int, ptr_null, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_std_menu);
@@ -2644,11 +2659,32 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct
v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
0, max, mask, def, NULL, 0,
-flags, qmenu, NULL, NULL);
+flags, qmenu, NULL, ptr_null, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
+/* Helper function for standard compound controls */
+struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
+
Re: [PATCH v8 7/8] media: v4l2-ctrl: Add new helper v4l2_ctrl_ptr_from_void
Hi Hans
Thanks for the review
On Mon, Sep 30, 2019 at 3:07 PM Hans Verkuil wrote:
>
> On 9/30/19 12:18 PM, Ricardo Ribalda Delgado wrote:
> > This helper function simplifies the code by not needing a union
> > v4l2_ctrl_ptr and an assignment every time we need to use
> > a ctrl_ptr.
> >
> > Suggested-by: Hans Verkuil
> > Signed-off-by: Ricardo Ribalda Delgado
> > ---
> > include/media/v4l2-ctrls.h | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> > index c42f164e2c9e..d69cfdffd41d 100644
> > --- a/include/media/v4l2-ctrls.h
> > +++ b/include/media/v4l2-ctrls.h
> > @@ -73,6 +73,17 @@ union v4l2_ctrl_ptr {
> > void *p;
> > };
> >
> > +/**
> > + * v4l2_ctrl_ptr() - Helper function to return a v4l2_ctrl_ptr from a
> > + * void pointer
> > + * @ptr: The void pointer
> > + */
> > +static inline union v4l2_ctrl_ptr v4l2_ctrl_ptr_from_void(void *ptr)
>
> Mismatch between function name and the comment above.
>
> But _from_void is not a good name, since it's from a void pointer.
>
> How about: v4l2_ctrl_ptr_create(void *ptr)
>
> since you create a v4l2_ctrl_ptr.
>
> Regards,
>
> Hans
After talking on IRC:
Will fix both changes and resend v9 after v5.4-rc1 is merged back into
the media tree.
Thanks again!
>
> > +{
> > + BUILD_BUG_ON(sizeof(union v4l2_ctrl_ptr) != sizeof(void *));
> > + return (union v4l2_ctrl_ptr) ptr;
> > +}
> > +
> > /**
> > * struct v4l2_ctrl_ops - The control operations that the driver has to
> > provide.
> > *
> >
>
[PATCH v8 8/8] media: imx214: Add new control with V4L2_CID_UNIT_CELL_SIZE
According to the product brief, the unit cell size is 1120 nanometers^2.
https://www.sony-semicon.co.jp/products_en/IS/sensor1/img/products/ProductBrief_IMX214_20150428.pdf
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/i2c/imx214.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 159a3a604f0e..4d9879f60dfb 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -47,6 +47,7 @@ struct imx214 {
struct v4l2_ctrl *pixel_rate;
struct v4l2_ctrl *link_freq;
struct v4l2_ctrl *exposure;
+ struct v4l2_ctrl *unit_size;
struct regulator_bulk_data supplies[IMX214_NUM_SUPPLIES];
@@ -948,6 +949,10 @@ static int imx214_probe(struct i2c_client *client)
static const s64 link_freq[] = {
IMX214_DEFAULT_LINK_FREQ,
};
+ static const struct v4l2_area unit_size = {
+ .width = 1120,
+ .height = 1120,
+ };
int ret;
ret = imx214_parse_fwnode(dev);
@@ -1029,6 +1034,10 @@ static int imx214_probe(struct i2c_client *client)
V4L2_CID_EXPOSURE,
0, 3184, 1, 0x0c70);
+ imx214->unit_size = v4l2_ctrl_new_std_compound(>ctrls,
+ NULL,
+ V4L2_CID_UNIT_CELL_SIZE,
+ v4l2_ctrl_ptr_from_void((void *)_size));
ret = imx214->ctrls.error;
if (ret) {
dev_err(>dev, "%s control init failed (%d)\n",
--
2.23.0
[PATCH v8 5/8] media: add V4L2_CID_UNIT_CELL_SIZE control
This control returns the unit cell size in nanometres. The struct provides
the width and the height in separated fields to take into consideration
asymmetric pixels and/or hardware binning.
This control is required for automatic calibration of sensors/cameras.
Reviewed-by: Philipp Zabel
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/v4l2-core/v4l2-ctrls.c | 5 +
include/uapi/linux/v4l2-controls.h | 1 +
2 files changed, 6 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index b9a46f536406..f626f9983408 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -996,6 +996,7 @@ const char *v4l2_ctrl_get_name(u32 id)
case V4L2_CID_AUTO_FOCUS_RANGE: return "Auto Focus, Range";
case V4L2_CID_PAN_SPEED:return "Pan, Speed";
case V4L2_CID_TILT_SPEED: return "Tilt, Speed";
+ case V4L2_CID_UNIT_CELL_SIZE: return "Unit Cell Size";
/* FM Radio Modulator controls */
/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1377,6 +1378,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
v4l2_ctrl_type *type,
case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER:
*type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER;
break;
+ case V4L2_CID_UNIT_CELL_SIZE:
+ *type = V4L2_CTRL_TYPE_AREA;
+ *flags |= V4L2_CTRL_FLAG_READ_ONLY;
+ break;
default:
*type = V4L2_CTRL_TYPE_INTEGER;
break;
diff --git a/include/uapi/linux/v4l2-controls.h
b/include/uapi/linux/v4l2-controls.h
index a2669b79b294..5a7bedee2b0e 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1034,6 +1034,7 @@ enum v4l2_jpeg_chroma_subsampling {
#define V4L2_CID_TEST_PATTERN_GREENR
(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 5)
#define V4L2_CID_TEST_PATTERN_BLUE
(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 6)
#define V4L2_CID_TEST_PATTERN_GREENB
(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 7)
+#define V4L2_CID_UNIT_CELL_SIZE
(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 8)
/* Image processing controls */
--
2.23.0
[PATCH v8 6/8] Documentation: media: Describe V4L2_CID_UNIT_CELL_SIZE
New control to pass to userspace the width/height of a pixel. Which is needed for calibration and lens selection. Reviewed-by: Philipp Zabel Signed-off-by: Ricardo Ribalda Delgado --- Documentation/media/uapi/v4l/ext-ctrls-image-source.rst | 9 + 1 file changed, 9 insertions(+) diff --git a/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst b/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst index 2c3ab5796d76..033672dcb43d 100644 --- a/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst +++ b/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst @@ -55,3 +55,12 @@ Image Source Control IDs ``V4L2_CID_TEST_PATTERN_GREENB (integer)`` Test pattern green (next to blue) colour component. + +``V4L2_CID_UNIT_CELL_SIZE (struct)`` +This control returns the unit cell size in nanometres. The struct +:c:type:`v4l2_area` provides the width and the height in separated +fields to take into consideration asymmetric pixels and/or hardware +binning. +The unit cell consists of the whole area of the pixel, sensitive and +non-sensitive. +This control is required for automatic calibration sensors/cameras. -- 2.23.0
[PATCH v8 4/8] Documentation: media: Document V4L2_CTRL_TYPE_AREA
A struct v4l2_area containing the width and the height of a rectangular area. Reviewed-by: Jacopo Mondi Reviewed-by: Philipp Zabel Signed-off-by: Ricardo Ribalda Delgado --- Documentation/media/uapi/v4l/vidioc-queryctrl.rst | 6 ++ 1 file changed, 6 insertions(+) diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst index a3d56ffbf4cc..33aff21b7d11 100644 --- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst +++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst @@ -443,6 +443,12 @@ See also the examples in :ref:`control`. - n/a - A struct :c:type:`v4l2_ctrl_mpeg2_quantization`, containing MPEG-2 quantization matrices for stateless video decoders. +* - ``V4L2_CTRL_TYPE_AREA`` + - n/a + - n/a + - n/a + - A struct :c:type:`v4l2_area`, containing the width and the height +of a rectangular area. Units depend on the use case. * - ``V4L2_CTRL_TYPE_H264_SPS`` - n/a - n/a -- 2.23.0
[PATCH v8 7/8] media: v4l2-ctrl: Add new helper v4l2_ctrl_ptr_from_void
This helper function simplifies the code by not needing a union
v4l2_ctrl_ptr and an assignment every time we need to use
a ctrl_ptr.
Suggested-by: Hans Verkuil
Signed-off-by: Ricardo Ribalda Delgado
---
include/media/v4l2-ctrls.h | 11 +++
1 file changed, 11 insertions(+)
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index c42f164e2c9e..d69cfdffd41d 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -73,6 +73,17 @@ union v4l2_ctrl_ptr {
void *p;
};
+/**
+ * v4l2_ctrl_ptr() - Helper function to return a v4l2_ctrl_ptr from a
+ * void pointer
+ * @ptr: The void pointer
+ */
+static inline union v4l2_ctrl_ptr v4l2_ctrl_ptr_from_void(void *ptr)
+{
+ BUILD_BUG_ON(sizeof(union v4l2_ctrl_ptr) != sizeof(void *));
+ return (union v4l2_ctrl_ptr) ptr;
+}
+
/**
* struct v4l2_ctrl_ops - The control operations that the driver has to
provide.
*
--
2.23.0
[PATCH v8 3/8] media: add V4L2_CTRL_TYPE_AREA control type
From: Ricardo Ribalda Delgado
This type contains the width and the height of a rectangular area.
Reviewed-by: Jacopo Mondi
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/v4l2-core/v4l2-ctrls.c | 21 ++
include/media/v4l2-ctrls.h | 42
include/uapi/linux/videodev2.h | 6
3 files changed, 69 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index 219d8aeefa20..b9a46f536406 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1678,6 +1678,7 @@ static int std_validate_compound(const struct v4l2_ctrl
*ctrl, u32 idx,
struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
void *p = ptr.p + idx * ctrl->elem_size;
+ struct v4l2_area *area;
switch ((u32)ctrl->type) {
case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
@@ -1753,6 +1754,11 @@ static int std_validate_compound(const struct v4l2_ctrl
*ctrl, u32 idx,
zero_padding(p_vp8_frame_header->entropy_header);
zero_padding(p_vp8_frame_header->coder_state);
break;
+ case V4L2_CTRL_TYPE_AREA:
+ area = p;
+ if (!area->width || !area->height)
+ return -EINVAL;
+ break;
default:
return -EINVAL;
}
@@ -2427,6 +2433,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
v4l2_ctrl_handler *hdl,
case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header);
break;
+ case V4L2_CTRL_TYPE_AREA:
+ elem_size = sizeof(struct v4l2_area);
+ break;
default:
if (type < V4L2_CTRL_COMPOUND_TYPES)
elem_size = sizeof(s32);
@@ -4116,6 +4125,18 @@ int __v4l2_ctrl_s_ctrl_string(struct v4l2_ctrl *ctrl,
const char *s)
}
EXPORT_SYMBOL(__v4l2_ctrl_s_ctrl_string);
+int __v4l2_ctrl_s_ctrl_area(struct v4l2_ctrl *ctrl,
+ const struct v4l2_area *area)
+{
+ lockdep_assert_held(ctrl->handler->lock);
+
+ /* It's a driver bug if this happens. */
+ WARN_ON(ctrl->type != V4L2_CTRL_TYPE_AREA);
+ memcpy(ctrl->p_new.p_area, area, sizeof(*area));
+ return set_ctrl(NULL, ctrl, 0);
+}
+EXPORT_SYMBOL(__v4l2_ctrl_s_ctrl_area);
+
void v4l2_ctrl_request_complete(struct media_request *req,
struct v4l2_ctrl_handler *main_hdl)
{
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 90a8ee48c2f3..c42f164e2c9e 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -50,6 +50,7 @@ struct poll_table_struct;
* @p_h264_slice_params: Pointer to a struct v4l2_ctrl_h264_slice_params.
* @p_h264_decode_params: Pointer to a struct
v4l2_ctrl_h264_decode_params.
* @p_vp8_frame_header:Pointer to a VP8 frame header structure.
+ * @p_area:Pointer to an area.
* @p: Pointer to a compound value.
*/
union v4l2_ctrl_ptr {
@@ -68,6 +69,7 @@ union v4l2_ctrl_ptr {
struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
struct v4l2_ctrl_h264_decode_params *p_h264_decode_params;
struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
+ struct v4l2_area *p_area;
void *p;
};
@@ -1086,6 +1088,46 @@ static inline int v4l2_ctrl_s_ctrl_string(struct
v4l2_ctrl *ctrl, const char *s)
return rval;
}
+/**
+ * __v4l2_ctrl_s_ctrl_area() - Unlocked variant of v4l2_ctrl_s_ctrl_area().
+ *
+ * @ctrl: The control.
+ * @area: The new area.
+ *
+ * This sets the control's new area safely by going through the control
+ * framework. This function assumes the control's handler is already locked,
+ * allowing it to be used from within the _ctrl_ops functions.
+ *
+ * This function is for area type controls only.
+ */
+int __v4l2_ctrl_s_ctrl_area(struct v4l2_ctrl *ctrl,
+ const struct v4l2_area *area);
+
+/**
+ * v4l2_ctrl_s_ctrl_area() - Helper function to set a control's area value
+ * from within a driver.
+ *
+ * @ctrl: The control.
+ * @s: The new area.
+ *
+ * This sets the control's new area safely by going through the control
+ * framework. This function will lock the control's handler, so it cannot be
+ * used from within the _ctrl_ops functions.
+ *
+ * This function is for area type controls only.
+ */
+static inline int v4l2_ctrl_s_ctrl_area(struct v4l2_ctrl *ctrl,
+ const struct v4l2_area *area)
+{
+ int rval;
+
+ v4l2_ctrl_lock(ctrl);
+ rval = __v4l2_ctrl_s_ctrl_area(ctrl, area);
+ v4l2_ctrl_unlock(ctrl);
+
+ return rval;
+}
+
/* Internal helper functions that deal with
[PATCH v8 1/8] media: v4l2-core: Implement v4l2_ctrl_new_std_compound
From: Ricardo Ribalda Delgado
Currently compound controls do not have a simple way of initializing its
values. This results in ofuscated code with type_ops init.
This patch introduces a new field on the control with the default value
for the compound control that can be set with the brand new
v4l2_ctrl_new_std_compound function
Suggested-by: Hans Verkuil
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/v4l2-core/v4l2-ctrls.c | 50
include/media/v4l2-ctrls.h | 21
2 files changed, 64 insertions(+), 7 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index 1d8f38824631..219d8aeefa20 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -29,6 +29,8 @@
#define call_op(master, op) \
(has_op(master, op) ? master->ops->op(master) : 0)
+static const union v4l2_ctrl_ptr ptr_null;
+
/* Internal temporary helper struct, one for each v4l2_ext_control */
struct v4l2_ctrl_helper {
/* Pointer to the control reference of the master control */
@@ -1530,7 +1532,10 @@ static void std_init_compound(const struct v4l2_ctrl
*ctrl, u32 idx,
struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
void *p = ptr.p + idx * ctrl->elem_size;
- memset(p, 0, ctrl->elem_size);
+ if (ctrl->p_def.p)
+ memcpy(p, ctrl->p_def.p, ctrl->elem_size);
+ else
+ memset(p, 0, ctrl->elem_size);
/*
* The cast is needed to get rid of a gcc warning complaining that
@@ -2354,7 +2359,8 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
v4l2_ctrl_handler *hdl,
s64 min, s64 max, u64 step, s64 def,
const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size,
u32 flags, const char * const *qmenu,
- const s64 *qmenu_int, void *priv)
+ const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def,
+ void *priv)
{
struct v4l2_ctrl *ctrl;
unsigned sz_extra;
@@ -2460,6 +2466,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
v4l2_ctrl_handler *hdl,
is_array)
sz_extra += 2 * tot_ctrl_size;
+ if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p)
+ sz_extra += elem_size;
+
ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL);
if (ctrl == NULL) {
handler_set_err(hdl, -ENOMEM);
@@ -2503,6 +2512,12 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
v4l2_ctrl_handler *hdl,
ctrl->p_new.p = >val;
ctrl->p_cur.p = >cur.val;
}
+
+ if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) {
+ ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size;
+ memcpy(ctrl->p_def.p, p_def.p, elem_size);
+ }
+
for (idx = 0; idx < elems; idx++) {
ctrl->type_ops->init(ctrl, idx, ctrl->p_cur);
ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
@@ -2554,7 +2569,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct
v4l2_ctrl_handler *hdl,
type, min, max,
is_menu ? cfg->menu_skip_mask : step, def,
cfg->dims, cfg->elem_size,
- flags, qmenu, qmenu_int, priv);
+ flags, qmenu, qmenu_int, ptr_null, priv);
if (ctrl)
ctrl->is_private = cfg->is_private;
return ctrl;
@@ -2579,7 +2594,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct
v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
min, max, step, def, NULL, 0,
-flags, NULL, NULL, NULL);
+flags, NULL, NULL, ptr_null, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_std);
@@ -2612,7 +2627,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct
v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
0, max, mask, def, NULL, 0,
-flags, qmenu, qmenu_int, NULL);
+flags, qmenu, qmenu_int, ptr_null, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_std_menu);
@@ -2644,11 +2659,32 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct
v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
0, max, mask, def, NULL, 0,
-flags, qmenu, NULL, NULL);
+flags, qmenu, NULL, ptr_null, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
+/* Helper function for standard compound controls */
+struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
+
[PATCH v8 2/8] Documentation: v4l2_ctrl_new_std_compound
From: Ricardo Ribalda Delgado Function for initializing compound controls with a default value. Suggested-by: Hans Verkuil Reviewed-by: Jacopo Mondi Signed-off-by: Ricardo Ribalda Delgado --- Documentation/media/kapi/v4l2-controls.rst | 9 + 1 file changed, 9 insertions(+) diff --git a/Documentation/media/kapi/v4l2-controls.rst b/Documentation/media/kapi/v4l2-controls.rst index ebe2a55908be..b20800cae3f2 100644 --- a/Documentation/media/kapi/v4l2-controls.rst +++ b/Documentation/media/kapi/v4l2-controls.rst @@ -140,6 +140,15 @@ Menu controls with a driver specific menu are added by calling const struct v4l2_ctrl_ops *ops, u32 id, s32 max, s32 skip_mask, s32 def, const char * const *qmenu); +Standard compound controls can be added by calling +:c:func:`v4l2_ctrl_new_std_compound`: + +.. code-block:: c + + struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, + const struct v4l2_ctrl_ops *ops, u32 id, + const union v4l2_ctrl_ptr p_def); + Integer menu controls with a driver specific menu can be added by calling :c:func:`v4l2_ctrl_new_int_menu`: -- 2.23.0
[PATCH v8 0/8] Implement UNIT_CELL_SIZE control
UNIT_CELL_SIZE is a control that represents the size of a cell (pixel). We required a bit of boilerplate to add this control :) - New way to init compount controls - New control type Thanks to Hans, Jacopo and Philipp for your help. You might want to see the series at my github repository if needed. https://github.com/ribalda/linux/tree/unit-size-v6 v8: Fix my email on some patches (sorry for the mess) v7: Add new helper v4l2_ctrl_ptr_from_void v4, v5 of this patchset never reached the mailing list. Ricardo Ribalda Delgado (8): media: v4l2-core: Implement v4l2_ctrl_new_std_compound Documentation: v4l2_ctrl_new_std_compound media: add V4L2_CTRL_TYPE_AREA control type Documentation: media: Document V4L2_CTRL_TYPE_AREA media: add V4L2_CID_UNIT_CELL_SIZE control Documentation: media: Describe V4L2_CID_UNIT_CELL_SIZE media: v4l2-ctrl: Add new helper v4l2_ctrl_ptr_from_void media: imx214: Add new control with V4L2_CID_UNIT_CELL_SIZE Documentation/media/kapi/v4l2-controls.rst| 9 +++ .../media/uapi/v4l/ext-ctrls-image-source.rst | 9 +++ .../media/uapi/v4l/vidioc-queryctrl.rst | 6 ++ drivers/media/i2c/imx214.c| 9 +++ drivers/media/v4l2-core/v4l2-ctrls.c | 76 +-- include/media/v4l2-ctrls.h| 74 ++ include/uapi/linux/v4l2-controls.h| 1 + include/uapi/linux/videodev2.h| 6 ++ 8 files changed, 183 insertions(+), 7 deletions(-) -- 2.23.0
[PATCH v7 3/8] media: add V4L2_CTRL_TYPE_AREA control type
From: Ricardo Ribalda Delgado
This type contains the width and the height of a rectangular area.
Reviewed-by: Jacopo Mondi
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/v4l2-core/v4l2-ctrls.c | 21 ++
include/media/v4l2-ctrls.h | 42
include/uapi/linux/videodev2.h | 6
3 files changed, 69 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index 219d8aeefa20..b9a46f536406 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1678,6 +1678,7 @@ static int std_validate_compound(const struct v4l2_ctrl
*ctrl, u32 idx,
struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
void *p = ptr.p + idx * ctrl->elem_size;
+ struct v4l2_area *area;
switch ((u32)ctrl->type) {
case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
@@ -1753,6 +1754,11 @@ static int std_validate_compound(const struct v4l2_ctrl
*ctrl, u32 idx,
zero_padding(p_vp8_frame_header->entropy_header);
zero_padding(p_vp8_frame_header->coder_state);
break;
+ case V4L2_CTRL_TYPE_AREA:
+ area = p;
+ if (!area->width || !area->height)
+ return -EINVAL;
+ break;
default:
return -EINVAL;
}
@@ -2427,6 +2433,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
v4l2_ctrl_handler *hdl,
case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header);
break;
+ case V4L2_CTRL_TYPE_AREA:
+ elem_size = sizeof(struct v4l2_area);
+ break;
default:
if (type < V4L2_CTRL_COMPOUND_TYPES)
elem_size = sizeof(s32);
@@ -4116,6 +4125,18 @@ int __v4l2_ctrl_s_ctrl_string(struct v4l2_ctrl *ctrl,
const char *s)
}
EXPORT_SYMBOL(__v4l2_ctrl_s_ctrl_string);
+int __v4l2_ctrl_s_ctrl_area(struct v4l2_ctrl *ctrl,
+ const struct v4l2_area *area)
+{
+ lockdep_assert_held(ctrl->handler->lock);
+
+ /* It's a driver bug if this happens. */
+ WARN_ON(ctrl->type != V4L2_CTRL_TYPE_AREA);
+ memcpy(ctrl->p_new.p_area, area, sizeof(*area));
+ return set_ctrl(NULL, ctrl, 0);
+}
+EXPORT_SYMBOL(__v4l2_ctrl_s_ctrl_area);
+
void v4l2_ctrl_request_complete(struct media_request *req,
struct v4l2_ctrl_handler *main_hdl)
{
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 90a8ee48c2f3..c42f164e2c9e 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -50,6 +50,7 @@ struct poll_table_struct;
* @p_h264_slice_params: Pointer to a struct v4l2_ctrl_h264_slice_params.
* @p_h264_decode_params: Pointer to a struct
v4l2_ctrl_h264_decode_params.
* @p_vp8_frame_header:Pointer to a VP8 frame header structure.
+ * @p_area:Pointer to an area.
* @p: Pointer to a compound value.
*/
union v4l2_ctrl_ptr {
@@ -68,6 +69,7 @@ union v4l2_ctrl_ptr {
struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
struct v4l2_ctrl_h264_decode_params *p_h264_decode_params;
struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
+ struct v4l2_area *p_area;
void *p;
};
@@ -1086,6 +1088,46 @@ static inline int v4l2_ctrl_s_ctrl_string(struct
v4l2_ctrl *ctrl, const char *s)
return rval;
}
+/**
+ * __v4l2_ctrl_s_ctrl_area() - Unlocked variant of v4l2_ctrl_s_ctrl_area().
+ *
+ * @ctrl: The control.
+ * @area: The new area.
+ *
+ * This sets the control's new area safely by going through the control
+ * framework. This function assumes the control's handler is already locked,
+ * allowing it to be used from within the _ctrl_ops functions.
+ *
+ * This function is for area type controls only.
+ */
+int __v4l2_ctrl_s_ctrl_area(struct v4l2_ctrl *ctrl,
+ const struct v4l2_area *area);
+
+/**
+ * v4l2_ctrl_s_ctrl_area() - Helper function to set a control's area value
+ * from within a driver.
+ *
+ * @ctrl: The control.
+ * @s: The new area.
+ *
+ * This sets the control's new area safely by going through the control
+ * framework. This function will lock the control's handler, so it cannot be
+ * used from within the _ctrl_ops functions.
+ *
+ * This function is for area type controls only.
+ */
+static inline int v4l2_ctrl_s_ctrl_area(struct v4l2_ctrl *ctrl,
+ const struct v4l2_area *area)
+{
+ int rval;
+
+ v4l2_ctrl_lock(ctrl);
+ rval = __v4l2_ctrl_s_ctrl_area(ctrl, area);
+ v4l2_ctrl_unlock(ctrl);
+
+ return rval;
+}
+
/* Internal helper functions that deal with
[PATCH v7 2/8] Documentation: v4l2_ctrl_new_std_compound
From: Ricardo Ribalda Delgado Function for initializing compound controls with a default value. Suggested-by: Hans Verkuil Reviewed-by: Jacopo Mondi Signed-off-by: Ricardo Ribalda Delgado --- Documentation/media/kapi/v4l2-controls.rst | 9 + 1 file changed, 9 insertions(+) diff --git a/Documentation/media/kapi/v4l2-controls.rst b/Documentation/media/kapi/v4l2-controls.rst index ebe2a55908be..b20800cae3f2 100644 --- a/Documentation/media/kapi/v4l2-controls.rst +++ b/Documentation/media/kapi/v4l2-controls.rst @@ -140,6 +140,15 @@ Menu controls with a driver specific menu are added by calling const struct v4l2_ctrl_ops *ops, u32 id, s32 max, s32 skip_mask, s32 def, const char * const *qmenu); +Standard compound controls can be added by calling +:c:func:`v4l2_ctrl_new_std_compound`: + +.. code-block:: c + + struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, + const struct v4l2_ctrl_ops *ops, u32 id, + const union v4l2_ctrl_ptr p_def); + Integer menu controls with a driver specific menu can be added by calling :c:func:`v4l2_ctrl_new_int_menu`: -- 2.23.0
[PATCH v7 1/8] media: v4l2-core: Implement v4l2_ctrl_new_std_compound
From: Ricardo Ribalda Delgado
Currently compound controls do not have a simple way of initializing its
values. This results in ofuscated code with type_ops init.
This patch introduces a new field on the control with the default value
for the compound control that can be set with the brand new
v4l2_ctrl_new_std_compound function
Suggested-by: Hans Verkuil
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/v4l2-core/v4l2-ctrls.c | 50
include/media/v4l2-ctrls.h | 21
2 files changed, 64 insertions(+), 7 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index 1d8f38824631..219d8aeefa20 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -29,6 +29,8 @@
#define call_op(master, op) \
(has_op(master, op) ? master->ops->op(master) : 0)
+static const union v4l2_ctrl_ptr ptr_null;
+
/* Internal temporary helper struct, one for each v4l2_ext_control */
struct v4l2_ctrl_helper {
/* Pointer to the control reference of the master control */
@@ -1530,7 +1532,10 @@ static void std_init_compound(const struct v4l2_ctrl
*ctrl, u32 idx,
struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
void *p = ptr.p + idx * ctrl->elem_size;
- memset(p, 0, ctrl->elem_size);
+ if (ctrl->p_def.p)
+ memcpy(p, ctrl->p_def.p, ctrl->elem_size);
+ else
+ memset(p, 0, ctrl->elem_size);
/*
* The cast is needed to get rid of a gcc warning complaining that
@@ -2354,7 +2359,8 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
v4l2_ctrl_handler *hdl,
s64 min, s64 max, u64 step, s64 def,
const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size,
u32 flags, const char * const *qmenu,
- const s64 *qmenu_int, void *priv)
+ const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def,
+ void *priv)
{
struct v4l2_ctrl *ctrl;
unsigned sz_extra;
@@ -2460,6 +2466,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
v4l2_ctrl_handler *hdl,
is_array)
sz_extra += 2 * tot_ctrl_size;
+ if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p)
+ sz_extra += elem_size;
+
ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL);
if (ctrl == NULL) {
handler_set_err(hdl, -ENOMEM);
@@ -2503,6 +2512,12 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
v4l2_ctrl_handler *hdl,
ctrl->p_new.p = >val;
ctrl->p_cur.p = >cur.val;
}
+
+ if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) {
+ ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size;
+ memcpy(ctrl->p_def.p, p_def.p, elem_size);
+ }
+
for (idx = 0; idx < elems; idx++) {
ctrl->type_ops->init(ctrl, idx, ctrl->p_cur);
ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
@@ -2554,7 +2569,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct
v4l2_ctrl_handler *hdl,
type, min, max,
is_menu ? cfg->menu_skip_mask : step, def,
cfg->dims, cfg->elem_size,
- flags, qmenu, qmenu_int, priv);
+ flags, qmenu, qmenu_int, ptr_null, priv);
if (ctrl)
ctrl->is_private = cfg->is_private;
return ctrl;
@@ -2579,7 +2594,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct
v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
min, max, step, def, NULL, 0,
-flags, NULL, NULL, NULL);
+flags, NULL, NULL, ptr_null, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_std);
@@ -2612,7 +2627,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct
v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
0, max, mask, def, NULL, 0,
-flags, qmenu, qmenu_int, NULL);
+flags, qmenu, qmenu_int, ptr_null, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_std_menu);
@@ -2644,11 +2659,32 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct
v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
0, max, mask, def, NULL, 0,
-flags, qmenu, NULL, NULL);
+flags, qmenu, NULL, ptr_null, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
+/* Helper function for standard compound controls */
+struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
+
[PATCH v7 8/8] media: imx214: Add new control with V4L2_CID_UNIT_CELL_SIZE
According to the product brief, the unit cell size is 1120 nanometers^2.
https://www.sony-semicon.co.jp/products_en/IS/sensor1/img/products/ProductBrief_IMX214_20150428.pdf
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/i2c/imx214.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 159a3a604f0e..4d9879f60dfb 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -47,6 +47,7 @@ struct imx214 {
struct v4l2_ctrl *pixel_rate;
struct v4l2_ctrl *link_freq;
struct v4l2_ctrl *exposure;
+ struct v4l2_ctrl *unit_size;
struct regulator_bulk_data supplies[IMX214_NUM_SUPPLIES];
@@ -948,6 +949,10 @@ static int imx214_probe(struct i2c_client *client)
static const s64 link_freq[] = {
IMX214_DEFAULT_LINK_FREQ,
};
+ static const struct v4l2_area unit_size = {
+ .width = 1120,
+ .height = 1120,
+ };
int ret;
ret = imx214_parse_fwnode(dev);
@@ -1029,6 +1034,10 @@ static int imx214_probe(struct i2c_client *client)
V4L2_CID_EXPOSURE,
0, 3184, 1, 0x0c70);
+ imx214->unit_size = v4l2_ctrl_new_std_compound(>ctrls,
+ NULL,
+ V4L2_CID_UNIT_CELL_SIZE,
+ v4l2_ctrl_ptr_from_void((void *)_size));
ret = imx214->ctrls.error;
if (ret) {
dev_err(>dev, "%s control init failed (%d)\n",
--
2.23.0
[PATCH v7 4/8] Documentation: media: Document V4L2_CTRL_TYPE_AREA
A struct v4l2_area containing the width and the height of a rectangular area. Reviewed-by: Jacopo Mondi Reviewed-by: Philipp Zabel Signed-off-by: Ricardo Ribalda Delgado --- Documentation/media/uapi/v4l/vidioc-queryctrl.rst | 6 ++ 1 file changed, 6 insertions(+) diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst index a3d56ffbf4cc..33aff21b7d11 100644 --- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst +++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst @@ -443,6 +443,12 @@ See also the examples in :ref:`control`. - n/a - A struct :c:type:`v4l2_ctrl_mpeg2_quantization`, containing MPEG-2 quantization matrices for stateless video decoders. +* - ``V4L2_CTRL_TYPE_AREA`` + - n/a + - n/a + - n/a + - A struct :c:type:`v4l2_area`, containing the width and the height +of a rectangular area. Units depend on the use case. * - ``V4L2_CTRL_TYPE_H264_SPS`` - n/a - n/a -- 2.23.0
[PATCH v7 7/8] media: v4l2-ctrl: Add new helper v4l2_ctrl_ptr_from_void
This helper function simplifies the code by not needing a union
v4l2_ctrl_ptr and an assignment every time we need to use
a ctrl_ptr.
Suggested-by: Hans Verkuil
Signed-off-by: Ricardo Ribalda Delgado
---
include/media/v4l2-ctrls.h | 11 +++
1 file changed, 11 insertions(+)
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index c42f164e2c9e..d69cfdffd41d 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -73,6 +73,17 @@ union v4l2_ctrl_ptr {
void *p;
};
+/**
+ * v4l2_ctrl_ptr() - Helper function to return a v4l2_ctrl_ptr from a
+ * void pointer
+ * @ptr: The void pointer
+ */
+static inline union v4l2_ctrl_ptr v4l2_ctrl_ptr_from_void(void *ptr)
+{
+ BUILD_BUG_ON(sizeof(union v4l2_ctrl_ptr) != sizeof(void *));
+ return (union v4l2_ctrl_ptr) ptr;
+}
+
/**
* struct v4l2_ctrl_ops - The control operations that the driver has to
provide.
*
--
2.23.0
[PATCH v7 0/8] Implement UNIT_CELL_SIZE control
UNIT_CELL_SIZE is a control that represents the size of a cell (pixel). We required a bit of boilerplate to add this control :) - New way to init compount controls - New control type Thanks to Hans, Jacopo and Philipp for your help. You might want to see the series at my github repository if needed. https://github.com/ribalda/linux/tree/unit-size-v6 v7: Add new helper v4l2_ctrl_ptr_from_void v4, v5 of this patchset never reached the mailing list. Ricardo Ribalda Delgado (8): media: v4l2-core: Implement v4l2_ctrl_new_std_compound Documentation: v4l2_ctrl_new_std_compound media: add V4L2_CTRL_TYPE_AREA control type Documentation: media: Document V4L2_CTRL_TYPE_AREA media: add V4L2_CID_UNIT_CELL_SIZE control Documentation: media: Describe V4L2_CID_UNIT_CELL_SIZE media: v4l2-ctrl: Add new helper v4l2_ctrl_ptr_from_void media: imx214: Add new control with V4L2_CID_UNIT_CELL_SIZE Documentation/media/kapi/v4l2-controls.rst| 9 +++ .../media/uapi/v4l/ext-ctrls-image-source.rst | 9 +++ .../media/uapi/v4l/vidioc-queryctrl.rst | 6 ++ drivers/media/i2c/imx214.c| 9 +++ drivers/media/v4l2-core/v4l2-ctrls.c | 76 +-- include/media/v4l2-ctrls.h| 74 ++ include/uapi/linux/v4l2-controls.h| 1 + include/uapi/linux/videodev2.h| 6 ++ 8 files changed, 183 insertions(+), 7 deletions(-) -- 2.23.0
[PATCH v7 6/8] Documentation: media: Describe V4L2_CID_UNIT_CELL_SIZE
New control to pass to userspace the width/height of a pixel. Which is needed for calibration and lens selection. Reviewed-by: Philipp Zabel Signed-off-by: Ricardo Ribalda Delgado --- Documentation/media/uapi/v4l/ext-ctrls-image-source.rst | 9 + 1 file changed, 9 insertions(+) diff --git a/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst b/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst index 2c3ab5796d76..033672dcb43d 100644 --- a/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst +++ b/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst @@ -55,3 +55,12 @@ Image Source Control IDs ``V4L2_CID_TEST_PATTERN_GREENB (integer)`` Test pattern green (next to blue) colour component. + +``V4L2_CID_UNIT_CELL_SIZE (struct)`` +This control returns the unit cell size in nanometres. The struct +:c:type:`v4l2_area` provides the width and the height in separated +fields to take into consideration asymmetric pixels and/or hardware +binning. +The unit cell consists of the whole area of the pixel, sensitive and +non-sensitive. +This control is required for automatic calibration sensors/cameras. -- 2.23.0
[PATCH v7 5/8] media: add V4L2_CID_UNIT_CELL_SIZE control
This control returns the unit cell size in nanometres. The struct provides
the width and the height in separated fields to take into consideration
asymmetric pixels and/or hardware binning.
This control is required for automatic calibration of sensors/cameras.
Reviewed-by: Philipp Zabel
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/v4l2-core/v4l2-ctrls.c | 5 +
include/uapi/linux/v4l2-controls.h | 1 +
2 files changed, 6 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index b9a46f536406..f626f9983408 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -996,6 +996,7 @@ const char *v4l2_ctrl_get_name(u32 id)
case V4L2_CID_AUTO_FOCUS_RANGE: return "Auto Focus, Range";
case V4L2_CID_PAN_SPEED:return "Pan, Speed";
case V4L2_CID_TILT_SPEED: return "Tilt, Speed";
+ case V4L2_CID_UNIT_CELL_SIZE: return "Unit Cell Size";
/* FM Radio Modulator controls */
/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1377,6 +1378,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
v4l2_ctrl_type *type,
case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER:
*type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER;
break;
+ case V4L2_CID_UNIT_CELL_SIZE:
+ *type = V4L2_CTRL_TYPE_AREA;
+ *flags |= V4L2_CTRL_FLAG_READ_ONLY;
+ break;
default:
*type = V4L2_CTRL_TYPE_INTEGER;
break;
diff --git a/include/uapi/linux/v4l2-controls.h
b/include/uapi/linux/v4l2-controls.h
index a2669b79b294..5a7bedee2b0e 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1034,6 +1034,7 @@ enum v4l2_jpeg_chroma_subsampling {
#define V4L2_CID_TEST_PATTERN_GREENR
(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 5)
#define V4L2_CID_TEST_PATTERN_BLUE
(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 6)
#define V4L2_CID_TEST_PATTERN_GREENB
(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 7)
+#define V4L2_CID_UNIT_CELL_SIZE
(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 8)
/* Image processing controls */
--
2.23.0
Re: [PATCH v6 7/7] media: imx214: Add new control with V4L2_CID_UNIT_CELL_SIZE
Hi Hans
On Fri, 27 Sep 2019, 09:14 Hans Verkuil, wrote:
>
> On 9/20/19 3:51 PM, Ricardo Ribalda Delgado wrote:
> > From: Ricardo Ribalda Delgado
> >
> > According to the product brief, the unit cell size is 1120 nanometers^2.
> >
> > https://www.sony-semicon.co.jp/products_en/IS/sensor1/img/products/ProductBrief_IMX214_20150428.pdf
> >
> > Signed-off-by: Ricardo Ribalda Delgado
> > ---
> > drivers/media/i2c/imx214.c | 12
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > index 159a3a604f0e..57562e20c4ca 100644
> > --- a/drivers/media/i2c/imx214.c
> > +++ b/drivers/media/i2c/imx214.c
> > @@ -47,6 +47,7 @@ struct imx214 {
> > struct v4l2_ctrl *pixel_rate;
> > struct v4l2_ctrl *link_freq;
> > struct v4l2_ctrl *exposure;
> > + struct v4l2_ctrl *unit_size;
> >
> > struct regulator_bulk_data supplies[IMX214_NUM_SUPPLIES];
> >
> > @@ -948,6 +949,13 @@ static int imx214_probe(struct i2c_client *client)
> > static const s64 link_freq[] = {
> > IMX214_DEFAULT_LINK_FREQ,
> > };
> > + struct v4l2_area unit_size = {
> > + .width = 1120,
> > + .height = 1120,
> > + };
> > + union v4l2_ctrl_ptr p_def = {
> > + .p_area = _size,
> > + };
>
> Use static const for both.
>
> I think you should add a small static inline helper function to v4l2-ctrls.h
> that
> takes a void pointer and returns a union v4l2_ctrl_ptr.
>
> Then you don't need to make a union v4l2_ctrl_ptr just to pass the unit_size
> pointer.
>
That sounds useful, but can we warantee for all the arches that
sizeof(v4l2_ctrl_ptr) <= sizeof (void *)
Of course, it sounds logic, that a union of pointers is the same size
than a pointer... but you never know.
No matter what I will make the helper and resend. with all the changes
from Jacopo
Thanks!
> Regards,
>
> Hans
>
> > int ret;
> >
> > ret = imx214_parse_fwnode(dev);
> > @@ -1029,6 +1037,10 @@ static int imx214_probe(struct i2c_client *client)
> >V4L2_CID_EXPOSURE,
> >0, 3184, 1, 0x0c70);
> >
> > + imx214->unit_size = v4l2_ctrl_new_std_compound(>ctrls,
> > +NULL,
> > +
> > V4L2_CID_UNIT_CELL_SIZE,
> > +p_def);
> > ret = imx214->ctrls.error;
> > if (ret) {
> > dev_err(>dev, "%s control init failed (%d)\n",
> >
>
