Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 17 Mar 2021 10:34:53 +0900
From:   Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
To:     Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
Cc:     Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Tomasz Figa <tomasz.figa@gmail.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        linux-media <linux-media@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Sergey Senozhatsky <senozhatsky@chromium.org>
Subject: Re: [PATCHv2 2/3] media: uvcvideo: add ROI auto controls
Message-ID: <YFFcvbXRlCCB+pv/@google.com>
References: <20210208051749.1785246-1-sergey.senozhatsky@gmail.com>
 <20210208051749.1785246-3-sergey.senozhatsky@gmail.com>
 <CAPybu_2ZRNUiZbFHfuW6i119xhs21-zTigoaO8sZc-Ye3D18xA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAPybu_2ZRNUiZbFHfuW6i119xhs21-zTigoaO8sZc-Ye3D18xA@mail.gmail.com>
Precedence: bulk

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   Field            Size
> >   0        wROI_Top         2
> >   2        wROI_Left        2
> >   4        wROI_Bottom      2
> >   6        wROI_Right       2
> >   8        bmAutoControls   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?

> > +    * - ``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?

> >  #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?