[libcamera-devel] [PATCH 7/7] libcamera: ipu3: imgu: Change IF_CROP_MAX to 40

[Jacopo Mondi]

Hi Laurent,

On Tue, Apr 13, 2021 at 02:53:38AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Mar 18, 2021 at 11:39:41AM +0100, Jacopo Mondi wrote:
> > From: Dave Olsthoorn <dave at bewaar.me>
> >
> > The value of IF_CROP_MAX seems to be a typo. A resolution of 40x540 seems
> > unlikely and excludes camera's with a 640x480 resolution, like the OV7251
> > in several Microsoft Surface products, from working.
>
> IF_CROP_MAX_H seems to be the maximum cropping that can be applied
> vertically, and is used to limit the search space. I'm not sure if
> that's a device limitation or a software limitation, but in any case,
> it's not a minimum vertical size as this commit message seems to imply,
> and I don't see why it would excluse a 640x480 input.

Indeed, the value does not reprent the min resolution...

>
> Could the problem be instead caused by
>
> 	unsigned int minIFHeight = iif.height - IF_CROP_MAX_H;
>
> in calculateBDSHeight() that will be a very large positive value if
> iif.height < IF_CROP_MAX_H ?
>

I can add a check for that, unfortunately I cannot reproduce the
problem and I cannot tell if that would fix it.

Dave could you test a follow-up patch eventually ?

Thanks
  j

> > This patch corrects the value to 40 since a minimal resolution of 40x40
> > seems more logical.
> >
> > Signed-off-by: Dave Olsthoorn <dave at bewaar.me>
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/imgu.cpp | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> > index b6a1fe34494e..dd7d2e947141 100644
> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> > @@ -44,7 +44,7 @@ static constexpr unsigned int BDS_ALIGN_W = 2;
> >  static constexpr unsigned int BDS_ALIGN_H = 4;
> >
> >  static constexpr unsigned int IF_CROP_MAX_W = 40;
> > -static constexpr unsigned int IF_CROP_MAX_H = 540;
> > +static constexpr unsigned int IF_CROP_MAX_H = 40;
> >
> >  static constexpr float BDS_SF_MAX = 2.5;
> >  static constexpr float BDS_SF_MIN = 1.0;
>
> --
> Regards,
>
> Laurent Pinchart