lkml.org 
[lkml]   [2022]   [Jun]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v6 4/4] media: i2c: ov5675: add .get_selection support
    Hi

    On Wed, Jun 08, 2022 at 12:04:05AM +0200, Tommaso Merciai wrote:
    > Hi Quentin/Jacopo,
    >
    > On Tue, Jun 07, 2022 at 06:51:36PM +0200, Jacopo Mondi wrote:
    > > Hi Quentin,
    > >
    > > On Tue, Jun 07, 2022 at 05:33:35PM +0200, Quentin Schulz wrote:
    > > > From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
    > > >
    > > > The sensor has 2592*1944 active pixels, surrounded by 16 active dummy
    > > > pixels and there are an additional 24 black rows "at the bottom".
    > > >
    > > > [2624]
    > > > +-----+------------------+-----+
    > > > | | 16 dummy | |
    > > > +-----+------------------+-----+
    > > > | | | |
    > > > | | [2592] | |
    > > > | | | |
    > > > |16 | valid | 16 |[2000]
    > > > |dummy| |dummy|
    > > > | | [1944]| |
    > > > | | | |
    > > > +-----+------------------+-----+
    > > > | | 16 dummy | |
    > > > +-----+------------------+-----+
    > > > | | 24 black lines | |
    > > > +-----+------------------+-----+
    > > >
    > > > The top-left coordinate is gotten from the registers specified in the
    > > > modes which are identical for both currently supported modes.
    > > >
    > > > There are currently two modes supported by this driver: 2592*1944 and
    > > > 1296*972. The second mode is obtained thanks to subsampling while
    > > > keeping the same field of view (FoV). No cropping involved, hence the
    > > > harcoded values.
    > > >
    > > > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
    > > > ---
    > > >
    > > > v6:
    > > > - explicit a bit more the commit log around subsampling for lower
    > > > resolution modes,
    > > > - (again) fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help,
    > > >
    > > > v4:
    > > > - explicit a bit more the commit log,
    > > > - added drawing in the commit log,
    > > > - fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help,
    > > >
    > > > added in v3
    > > >
    > > > drivers/media/i2c/ov5675.c | 21 +++++++++++++++++++++
    > > > 1 file changed, 21 insertions(+)
    > > >
    > > > diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
    > > > index 80840ad7bbb0..2230ff47ef49 100644
    > > > --- a/drivers/media/i2c/ov5675.c
    > > > +++ b/drivers/media/i2c/ov5675.c
    > > > @@ -1121,6 +1121,26 @@ static int ov5675_get_format(struct v4l2_subdev *sd,
    > > > return 0;
    > > > }
    > > >
    > > > +static int ov5675_get_selection(struct v4l2_subdev *sd,
    > > > + struct v4l2_subdev_state *state,
    > > > + struct v4l2_subdev_selection *sel)
    > > > +{
    > > > + if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
    > > > + return -EINVAL;
    > > > +
    > > > + switch (sel->target) {
    > > > + case V4L2_SEL_TGT_CROP:
    > > > + case V4L2_SEL_TGT_CROP_BOUNDS:
    > >
    > > Seem like we have trouble understanding each other, or better, I have
    > > troubles explaining myself most probably :)
    > >
    > > If the dummy/black area is readable, this should just be (0, 0, 2624,
    > > 2000) like it was in your previous version. What has changed that I
    > > have missed ?
    >
    > Taking as reference drivers/media/i2c/ov5693.c and others,
    > seems ok what Quentin have done from my side.
    >
    > Just one thing: maybe is better to avoid magic numbers with more
    > explicit defines like:
    >
    > + case V4L2_SEL_TGT_CROP_DEFAULT:
    > + sel->r.top = OV5675_ACTIVE_START_TOP;
    > + sel->r.left = OV5693_ACTIVE_START_LEFT;
    > + sel->r.width = OV5693_ACTIVE_WIDTH;
    > + sel->r.height = OV5693_ACTIVE_HEIGHT;
    >
    > What do you think about?
    >
    > Thanks,
    > Tommaso
    >
    >

    We have extensively discussed this:
    https://patchwork.linuxtv.org/project/linux-media/patch/20220509143226.531117-4-foss+kernel@0leil.net/
    https://patchwork.linuxtv.org/project/linux-media/patch/20220525145833.1165437-4-foss+kernel@0leil.net/

    From the CROP_BOUNDS definition:
    Bounds of the crop rectangle. All valid crop rectangles fit inside the
    crop bounds rectangle.

    From CROP_DEFAULT:
    Suggested cropping rectangle that covers the “whole picture”. This
    includes only active pixels and excludes other non-active pixels such
    as black pixels.

    If (and only if) dummy/inactive pixels can be read out, the BOUNDS
    rectangle should contain them. In this case, as the analog crop
    rectangle is defined with a 16x16 top-left corner (and with a visible
    size of 2592x1944) from a larger rectangle, I presume it means
    dummy/invalid pixls can be read (IOW you can give to the ISP a rectangle
    that includes them).

    Anyway, we're discussing details really. I think v5 was almost there

    + switch (sel->target) {
    + case V4L2_SEL_TGT_CROP_BOUNDS:
    + sel->r.top = 0;
    + sel->r.left = 0;
    + sel->r.width = 2624;
    + sel->r.height = 2000;
    + return 0;
    + case V4L2_SEL_TGT_CROP:
    + sel->r.top = 16;
    + sel->r.left = 16;
    + sel->r.width = ov5675->cur_mode->width;
    + sel->r.height = ov5675->cur_mode->height;
    + return 0;
    + case V4L2_SEL_TGT_CROP_DEFAULT:
    + sel->r.top = 16;
    + sel->r.left = 16;
    + sel->r.width = supported_modes[0].width;
    + sel->r.height = supported_modes[0].height;
    + return 0;
    + }

    Apart from the fact that TGT_CROP should not report the final image
    size (after binning/digital crop) but the size of the pixel array
    portion processed to obtain the final image.


    + switch (sel->target) {
    + case V4L2_SEL_TGT_CROP_BOUNDS:
    + sel->r.top = 0;
    + sel->r.left = 0;
    + sel->r.width = 2624;
    + sel->r.height = 2000;
    + return 0;
    + case V4L2_SEL_TGT_CROP:
    + case V4L2_SEL_TGT_CROP_DEFAULT:
    + sel->r.top = 16;
    + sel->r.left = 16;
    + sel->r.width = 2592;
    + sel->r.height = 1944;
    + return 0;
    + }

    Let me remind that (in the context of pleasing libcamera requirements
    as Quentin is doing) all targets apart TGT_CROP are only useful to
    report static properties of the camera, which are of no real use
    unless you start actually looking into reading out black pixels etc
    etc.


    > >
    > > Thanks
    > > j
    > >
    > >
    > > > + case V4L2_SEL_TGT_CROP_DEFAULT:
    > > > + sel->r.top = 16;
    > > > + sel->r.left = 16;
    > > > + sel->r.width = 2592;
    > > > + sel->r.height = 1944;
    > > > + return 0;
    > > > + }
    > > > + return -EINVAL;
    > > > +}
    > > > +
    > > > static int ov5675_enum_mbus_code(struct v4l2_subdev *sd,
    > > > struct v4l2_subdev_state *sd_state,
    > > > struct v4l2_subdev_mbus_code_enum *code)
    > > > @@ -1170,6 +1190,7 @@ static const struct v4l2_subdev_video_ops ov5675_video_ops = {
    > > > static const struct v4l2_subdev_pad_ops ov5675_pad_ops = {
    > > > .set_fmt = ov5675_set_format,
    > > > .get_fmt = ov5675_get_format,
    > > > + .get_selection = ov5675_get_selection,
    > > > .enum_mbus_code = ov5675_enum_mbus_code,
    > > > .enum_frame_size = ov5675_enum_frame_size,
    > > > };
    > > > --
    > > > 2.36.1
    > > >
    >
    > --
    > Tommaso Merciai
    > Embedded Linux Engineer
    > tommaso.merciai@amarulasolutions.com
    > __________________________________
    >
    > Amarula Solutions SRL
    > Via Le Canevare 30, 31100 Treviso, Veneto, IT
    > T. +39 042 243 5310
    > info@amarulasolutions.com
    > www.amarulasolutions.com

    \
     
     \ /
      Last update: 2022-06-08 09:24    [W:3.912 / U:0.228 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site