Re: [PATCH v12 56/86] media: imx219: Add internal pads, routes for common raw sensor model

[Jai Luthra <jai.luthra@ideasonboard.com>]

Hi Sakari,

Quoting Sakari Ailus (2026-04-10 01:44:31)
> Support internal pads, routing, formats and selections for the newly added
> common raw sensor model, with compatibility interface provided for the
> existing users.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Co-developed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/i2c/imx219.c | 230 +++++++++++++++++++++++++++----------
>  1 file changed, 171 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 67b02fa338d0..b4aa62cbb826 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -139,13 +139,19 @@
>  #define IMX219_DEFAULT_LINK_FREQ_4LANE_UNSUPPORTED     363000000
>  #define IMX219_DEFAULT_LINK_FREQ_4LANE 364000000
>  
> +/* For compatibility with the old UAPI. */
> +#define IMX219_COMPAT_VISIBLE_TOP      8U
>  /* IMX219 native and active pixel array size. */
> -#define IMX219_NATIVE_WIDTH            3296U
> -#define IMX219_NATIVE_HEIGHT           2480U
> +#define IMX219_NON_VISIBLE_HEIGHT      42U
>  #define IMX219_VISIBLE_LEFT            8U
> -#define IMX219_VISIBLE_TOP             8U
> +#define IMX219_VISIBLE_TOP             (IMX219_NON_VISIBLE_HEIGHT + 8U)
>  #define IMX219_VISIBLE_WIDTH           3280U
>  #define IMX219_VISIBLE_HEIGHT          2464U
> +#define IMX219_PIXEL_ARRAY_LEFT                0U
> +#define IMX219_PIXEL_ARRAY_TOP         IMX219_NON_VISIBLE_HEIGHT
> +#define IMX219_PIXEL_ARRAY_WIDTH       3296U
> +#define IMX219_PIXEL_ARRAY_HEIGHT      2480U
> +#define IMX219_NATIVE_FORMAT           MEDIA_BUS_FMT_SRGGB10_1X10
>  
>  /* Mode : resolution and related config&values */
>  struct imx219_mode {

[...]

>  static int imx219_get_selection(struct v4l2_subdev *sd,
>                                 const struct v4l2_subdev_client_info *ci,
>                                 struct v4l2_subdev_state *state,
>                                 struct v4l2_subdev_selection *sel)
>  {
> -       switch (sel->target) {
> -       case V4L2_SEL_TGT_CROP:
> -               sel->r = *v4l2_subdev_state_get_crop(state, 0);
> -               return 0;
> +       if (!(ci &&
> +             ci->client_caps & V4L2_SUBDEV_CLIENT_CAP_COMMON_RAW_SENSOR)) {
> +               if (sel->pad != IMX219_PAD_SOURCE)
> +                       return -EINVAL;
> +
> +               switch (sel->target) {
> +               case V4L2_SEL_TGT_CROP:
> +                       sel->r = *v4l2_subdev_state_get_crop(state,
> +                                                            IMX219_PAD_IMAGE);
> +                       sel->r.top -= IMX219_VISIBLE_TOP;

This leads to sel->r.top = 0, when before this patch it reported 8 to
userspace.

It caused a segfault in the VC4 IPA's lens shading algorithm when running
libcamera without CLIENT_CAP_COMMON_RAW_SENSOR because it was trying to set
cropY = -8 and underflowing (not a very fun debug :)


> +                       return 0;
> +
> +               case V4L2_SEL_TGT_NATIVE_SIZE:
> +                       sel->r.top = 0;
> +                       sel->r.left = 0;
> +                       sel->r.width = IMX219_VISIBLE_WIDTH;
> +                       sel->r.height = IMX219_VISIBLE_HEIGHT;
> +

Same here, shouldn't TGT_NATIVE_SIZE width/height remain 3296x2480 like it
was before? The current version makes NATIVE_SIZE smaller than CROP_BOUNDS
below.

> +                       return 0;
> +
> +               case V4L2_SEL_TGT_CROP_DEFAULT:
> +               case V4L2_SEL_TGT_CROP_BOUNDS:
> +                       sel->r.top = IMX219_COMPAT_VISIBLE_TOP;
> +                       sel->r.left = IMX219_VISIBLE_LEFT;
> +                       sel->r.width = IMX219_VISIBLE_WIDTH;
> +                       sel->r.height = IMX219_VISIBLE_HEIGHT;
> +
> +                       return 0;
> +               }

I have applied the following changes in my local tree:

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 9d5479e00886..a30bd4247c8d 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -1287,14 +1287,14 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
                case V4L2_SEL_TGT_CROP:
                        sel->r = *v4l2_subdev_state_get_crop(state,
                                                             IMX219_PAD_IMAGE);
-                       sel->r.top -= IMX219_ACTIVE_AREA_TOP;
+                       sel->r.top -= IMX219_INACTIVE_AREA_HEIGHT;
                        return 0;

                case V4L2_SEL_TGT_NATIVE_SIZE:
                        sel->r.top = 0;
                        sel->r.left = 0;
-                       sel->r.width = IMX219_ACTIVE_AREA_WIDTH;
-                       sel->r.height = IMX219_ACTIVE_AREA_HEIGHT;
+                       sel->r.width = IMX219_PIXEL_ARRAY_WIDTH;
+                       sel->r.height = IMX219_PIXEL_ARRAY_HEIGHT;

                        return 0;

Thanks,
Jai

>  
> -       case V4L2_SEL_TGT_NATIVE_SIZE:
> -               sel->r.top = 0;
> -               sel->r.left = 0;
> -               sel->r.width = IMX219_NATIVE_WIDTH;
> -               sel->r.height = IMX219_NATIVE_HEIGHT;
> +               return -EINVAL;
> +       }
>  
> -               return 0;
> +       if (sel->pad != IMX219_PAD_IMAGE)
> +               return -EINVAL;
>  
> -       case V4L2_SEL_TGT_CROP_DEFAULT:
> +       switch (sel->target) {
>         case V4L2_SEL_TGT_CROP_BOUNDS:
>                 sel->r.top = IMX219_VISIBLE_TOP;
>                 sel->r.left = IMX219_VISIBLE_LEFT;
>                 sel->r.width = IMX219_VISIBLE_WIDTH;
>                 sel->r.height = IMX219_VISIBLE_HEIGHT;
> -
>                 return 0;
> +       case V4L2_SEL_TGT_CROP_DEFAULT:
> +               sel->r.top = IMX219_VISIBLE_TOP;
> +               sel->r.left = IMX219_VISIBLE_LEFT;
> +               sel->r.width = IMX219_VISIBLE_WIDTH;
> +               sel->r.height = IMX219_VISIBLE_HEIGHT;
> +               return 0;
> +       case V4L2_SEL_TGT_CROP:
> +               sel->r = *v4l2_subdev_state_get_crop(state, sel->pad);
> +               return 0;
> +       case V4L2_SEL_TGT_COMPOSE:
> +               sel->r = *v4l2_subdev_state_get_compose(state, sel->pad);
> +               return 0;
> +       default:
> +               return -EINVAL;
>         }
> -
> -       return -EINVAL;
>  }
>  

[...]