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;
MIME-Version: 1.0
References: <20210208051749.1785246-1-sergey.senozhatsky@gmail.com>
 <20210208051749.1785246-2-sergey.senozhatsky@gmail.com> <CAPybu_19hztQQEi0H40sWZQMb-X7g7dDuW4Mz8_gRv-nG2tghw@mail.gmail.com>
 <YFFb2ePwiW+8ti4D@google.com>
In-Reply-To: <YFFb2ePwiW+8ti4D@google.com>
From:   Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
Date:   Wed, 17 Mar 2021 09:04:02 +0100
Message-ID: <CAPybu_1ng4GBVx64FQRR+rm2FcqLHkpW9c78AXg_P_6aR=2BKg@mail.gmail.com>
Subject: Re: [PATCHv2 1/3] media: v4l UAPI docs: document ROI selection targets
To:     Sergey Senozhatsky <sergey.senozhatsky.work@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>,
        Ricardo Ribalda <ribalda@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi

On Wed, Mar 17, 2021 at 2:31 AM Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> 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_BOUNDS    0x0102
> > >  /* Current composing area plus all padding pixels */
> > >  #define V4L2_SEL_TGT_COMPOSE_PADDED    0x0103
> > > +/* 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_BOUNDS        0x0202
> > > +/* 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