Hi Quentin,
On Tue, Jun 07, 2022 at 05:33:35PM +0200, Quentin Schulz wrote:
> From: Quentin Schulz <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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 ?
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
>
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 <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> >
> > 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
>
> 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
[email protected]
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
[email protected]
http://www.amarulasolutions.com
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 <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > >
> > > 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/[email protected]/
https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/
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
> [email protected]
> __________________________________
>
> Amarula Solutions SRL
> Via Le Canevare 30, 31100 Treviso, Veneto, IT
> T. +39 042 243 5310
> [email protected]
> http://www.amarulasolutions.com
Hi,
On Wed, Jun 08, 2022 at 08:42:09AM +0200, Jacopo Mondi wrote:
> 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 <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > ---
> > > >
> > > > 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/[email protected]/
> https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/
>
> 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).
Thanks for sharing this.
>
> 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.
Got it.
Thanks for clarifications.
Regards,
Tommaso
>
>
> > >
> > > 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
> > [email protected]
> > __________________________________
> >
> > Amarula Solutions SRL
> > Via Le Canevare 30, 31100 Treviso, Veneto, IT
> > T. +39 042 243 5310
> > [email protected]
> > http://www.amarulasolutions.com
--
Tommaso Merciai
Embedded Linux Engineer
[email protected]
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
[email protected]
http://www.amarulasolutions.com
Jacopo, Tommaso,
On 6/8/22 08:42, Jacopo Mondi wrote:
> 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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>>>
>>>> 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 ?
>>
I wouldn't say there's some misunderstanding, it's just super hard to
figure out how to match what the datasheet says to what the kernel
wants. Yay to obscure/confusing datasheets \o/
I just did things too quickly, nothing changed. Sorry, will send a v7.
>> 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;
>>
They are hardcoded today but actually depend on what;s set in the
registers too, which might differ if we add more modes in the future?
It's anyway auto-magic and it's the only place it's used, so not sure it
brings much especially since the variable names on the left hand side of
the operator are pretty self-explanatory (not talking about
V4L2_SEL_TGT_CROP_* :p)? Not that I'm against it.
Cheers,
Quentin
Hi,
On Wed, Jun 08, 2022 at 03:05:29PM +0200, Quentin Schulz wrote:
> Jacopo, Tommaso,
>
> On 6/8/22 08:42, Jacopo Mondi wrote:
> > 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 <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> > > > > ---
> > > > >
> > > > > 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 ?
> > >
>
> I wouldn't say there's some misunderstanding, it's just super hard to figure
> out how to match what the datasheet says to what the kernel wants. Yay to
> obscure/confusing datasheets \o/
I know your feels :)
>
> I just did things too quickly, nothing changed. Sorry, will send a v7.
>
> > > 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;
> > >
>
> They are hardcoded today but actually depend on what;s set in the registers
> too, which might differ if we add more modes in the future? It's anyway
> auto-magic and it's the only place it's used, so not sure it brings much
> especially since the variable names on the left hand side of the operator
> are pretty self-explanatory (not talking about V4L2_SEL_TGT_CROP_* :p)? Not
> that I'm against it.
Thanks for the explaination.
You are right.
Regards,
Tommaso
>
> Cheers,
> Quentin
--
Tommaso Merciai
Embedded Linux Engineer
[email protected]
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
[email protected]
http://www.amarulasolutions.com