linux-kernel - Re: [PATCH v5 2/2] [media] imx214: Add imx214 camera sensor driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAPybu_2CPQFwuGF9Pbbnqe6KHCZzm_70+Zs2xs3iPsUgQqjECQ@mail.gmail.com>
Date:   Thu, 4 Oct 2018 12:06:56 +0200
From:   Ricardo Ribalda Delgado <ricardo.ribalda@...il.com>
To:     Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc:     Hans Verkuil <hans.verkuil@...co.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-media <linux-media@...r.kernel.org>, jacopo@...ndi.org
Subject: Re: [PATCH v5 2/2] [media] imx214: Add imx214 camera sensor driver

Hi Sakari
On Thu, Oct 4, 2018 at 12:14 AM Sakari Ailus
<sakari.ailus@...ux.intel.com> wrote:
>
> On Wed, Oct 03, 2018 at 10:24:17PM +0200, Ricardo Ribalda Delgado wrote:
> ...
> > > > +static int imx214_enum_frame_size(struct v4l2_subdev *subdev,
> > > > +                               struct v4l2_subdev_pad_config *cfg,
> > > > +                               struct v4l2_subdev_frame_size_enum *fse)
> > > > +{
> > > > +     if (fse->code != IMX214_MBUS_CODE)
> > > > +             return -EINVAL;
> > > > +
> > > > +     if (fse->index >= ARRAY_SIZE(imx214_modes))
> > >
> > > array_index_nospec() ?? I find it scary that you'd need that in drivers.
> > > :-o
>
> Uh... not needed.
>
> The value is just sent back to the user as such so this is fine AFAICT.
>
> > >
> > > > +             return -EINVAL;
> > > > +
> > > > +     fse->min_width = fse->max_width = imx214_modes[fse->index].width;
> > > > +     fse->min_height = fse->max_height = imx214_modes[fse->index].height;
> > > > +
> > > > +     return 0;
> > > > +}
>
> ...
>
> > > > +     /*
> > > > +      * WARNING!
> > > > +      * Values obtained reverse engineering blobs and/or devices.
> > > > +      * Ranges and functionality might be wrong.
> > > > +      *
> > > > +      * Sony, please release some register set documentation for the
> > > > +      * device.
> > > > +      *
> > > > +      * Yours sincerely, Ricardo.
> > > > +      */
> > > > +     imx214->exposure = v4l2_ctrl_new_std(&imx214->ctrls, &imx214_ctrl_ops,
> > > > +                                          V4L2_CID_EXPOSURE,
> > > > +                                          0, 0xffff, 1, 0x0c70);
> > >
> > > The exposure is in lines so it can't exceed frame height + blanking.
> > > There's a marginal, too. I don't know what it might be for this sensor
> > > though. Usually it's small, such as 8 or 16. The image will almost
> > > certainly be garbled if you exceed the allowed value.
> > >Seems that this sensor
> >
> > On this sensor what I am experiencing instead of garbage is that the
> > fps gets reduced. So I believe it is fine to
> > set it up this way.
>
> That's rather confusing as well. The user should explicitly need to change
> fps first, rather than it happening as a side effect of a seemingly
> unrelated control.
>

I'd rather maintain this behaviour until we get access to better documentation.

Currently I cannot change the fps on demand, just changing the
exposure time, so I have no proper way of set/get fps



> --
> Regards,
>
> Sakari Ailus
> sakari.ailus@...ux.intel.com



--
Ricardo Ribalda