Re: [PATCH] media: i2c: imx471: return 0 for V4L2_SEL_TGT_CROP

Hi John,

Thank you for working on this.

On Mon, Aug 17, 2026 at 3:43 AM John Cronin <[email protected]> wrote:
>
> libcamera issues VIDIOC_SUBDEV_G_SELECTION with target CROP (rectangle
> 0) during CameraSensorLegacy bring-up. imx471_get_selection filled the
> rectangle then broke out of the switch and returned -EINVAL, so
> libcamera logged:
>
>   Unable to get rectangle 0 on pad 0/0: Invalid argument
>   Failed to retrieve the sensor crop rectangle
>
> Match imx219: return 0 after CROP. Also store the analog crop window
> from X/Y_ADD_STA/END in the subdev state when the mode is selected, so
> the CROP rectangle is not an uninitialized 0x0 box.
>
> Analog crop for the 1928x1088 2x2-binned mode is the programmed analog
> window 8,408 / 4640x2644 (native array coordinates).
>
> Tested on ThinkPad X9-15 Gen 1 (SONY471A / IPU7), Fedora 44
> kernel 7.1.8-200.fc44. After loading this module:
>
>   VIDIOC_SUBDEV_G_SELECTION CROP         → 8,408 4640x2644
>   VIDIOC_SUBDEV_G_SELECTION CROP_BOUNDS  → 8,8 4656x3496
>   VIDIOC_SUBDEV_G_SELECTION NATIVE_SIZE  → 0,0 4672x3512
>
> libcamera no longer logs "Unable to get rectangle 0 on pad 0/0".
>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2454119
> Signed-off-by: John Cronin <[email protected]>
> ---
>  drivers/media/i2c/imx471.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/imx471.c b/drivers/media/i2c/imx471.c
> index ae0e4d4..18b81aa 100644
> --- a/drivers/media/i2c/imx471.c
> +++ b/drivers/media/i2c/imx471.c
> @@ -131,6 +131,9 @@ struct imx471_mode {
>         /* H-timing */
>         u32 llp;
>
> +       /* Analog crop window in native array coordinates */
> +       struct v4l2_rect crop;
The crop area can be dynamically calculated when setting the format so
this can be dropped.
and the reason is described below.

> +
>         const struct cci_reg_sequence *default_mode_regs;
>         unsigned int default_mode_regs_length;
>  };
> @@ -283,6 +286,13 @@ static const struct imx471_mode imx471_modes[] = {
>                 .fll_def = 1308,
>                 .fll_min = 1308,
>                 .llp = 2328,
> +               /* X/Y_ADD_STA/END: 8..4647 x 408..3051 */
> +               .crop = {
> +                       .left = 8,
> +                       .top = 408,
> +                       .width = 4640,
> +                       .height = 2644,
> +               },
If we want to calculate the crop dynamically, this can be dropped.

>                 .default_mode_regs = mode_1928x1088_regs,
>                 .default_mode_regs_length = ARRAY_SIZE(mode_1928x1088_regs),
>         },
> @@ -430,6 +440,7 @@ static int imx471_set_pad_format(struct v4l2_subdev *sd,
>         imx471_update_pad_format(sensor, mode, fmt);
>
>         *v4l2_subdev_state_get_format(sd_state, fmt->pad) = fmt->format;
> +       *v4l2_subdev_state_get_crop(sd_state, fmt->pad) = mode->crop;

We can calculate the crop here similar to how it was handled in the
IMX219. Since the IMX471 supports a binning mode with 2328x1748 (4:3)
and 2328x1304 (16:9) image sizes, the maximum scaling ratio for width
and height is 2, which should work for us.

u8 bin_h, bin_v, binning;
struct v4l2_rect *crop;
int h_blank, ret;

... skip

*v4l2_subdev_state_get_format(sd_state, fmt->pad) = fmt->format;
crop = v4l2_subdev_state_get_crop(sd_state, fmt->pad);

bin_h = min(IMX471_PIXEL_ARRAY_WIDTH / fmt->format.width, 2U);
bin_v = min(IMX471_PIXEL_ARRAY_HEIGHT / fmt->format.height, 2U);
binning = min(bin_h, bin_v);
crop->width = fmt->format.width * binning;
crop->height = fmt->format.height * binning;
crop->left = (IMX471_NATIVE_WIDTH - crop->width) / 2;
crop->top = (IMX471_NATIVE_HEIGHT - crop->height) / 2;

>
>         if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
>                 return 0;
> @@ -461,7 +472,7 @@ static int imx471_get_selection(struct v4l2_subdev *sd,
>         switch (sel->target) {
>         case V4L2_SEL_TGT_CROP:
>                 sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad);
> -               break;
> +               return 0;
>
>         case V4L2_SEL_TGT_NATIVE_SIZE:
>                 sel->r.top = 0;

For the fix to the return value. It can be as follows:

static int imx471_get_selection(struct v4l2_subdev *sd,
                                struct v4l2_subdev_state *sd_state,
                                struct v4l2_subdev_selection *sel)
{
        switch (sel->target) {
        case V4L2_SEL_TGT_CROP:
                sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad);
                break;

        case V4L2_SEL_TGT_NATIVE_SIZE:
                sel->r.top = 0;
                sel->r.left = 0;
                sel->r.width = IMX471_NATIVE_WIDTH;
                sel->r.height = IMX471_NATIVE_HEIGHT;
                break;

        case V4L2_SEL_TGT_CROP_DEFAULT:
        case V4L2_SEL_TGT_CROP_BOUNDS:
                sel->r.top = IMX471_PIXEL_ARRAY_TOP;
                sel->r.left = IMX471_PIXEL_ARRAY_LEFT;
                sel->r.width = IMX471_PIXEL_ARRAY_WIDTH;
                sel->r.height = IMX471_PIXEL_ARRAY_HEIGHT;
                break;

        default:
                return -EINVAL;
        }

        return 0;
}

> --
> 2.55.0
>


-- 
BR,
Kate