media: i2c: ov13858: report the right bayer order The driver advertises MEDIA_BUS_FMT_SGRBG10_1X10 unconditionally, but that is not what the sensor delivers in any flip state on the Surface Pro 12in. The horizontal mirror moves the Bayer phase by one column, and the native order is GBRG rather than GRBG, so the two correct codes are GBRG unmirrored and BGGR mirrored. The rear ov13858 is mounted upside down (rotation = <180> in DT), so userspace enables both flips to compensate and every frame is then debayered as RGGB while the sensor emits BGGR. Red and blue are exchanged: an orange object comes out cyan, warm wood comes out blue, while neutrals are unaffected, which is what makes it easy to mistake for a white balance problem. Two measurements on a Surface Pro 12in, from raw frames captured off the CAMSS RDI video node. The first locates the green pair by comparing the mean absolute difference of the diagonal against the antidiagonal samples of each 2x2 quad, the two greens correlating far better than red against blue: hflip=0 vflip=0 diagonal 8.65 antidiagonal 56.82 -> greens diagonal hflip=1 vflip=0 diagonal 56.25 antidiagonal 8.34 -> greens antidiagonal hflip=0 vflip=1 diagonal 13.21 antidiagonal 55.94 -> greens diagonal hflip=1 vflip=1 diagonal 56.04 antidiagonal 13.16 -> greens antidiagonal So only the horizontal mirror moves the phase; the ISP Y window offset written for VFLIP compensates the vertical one. That test cannot tell red from blue, because GBRG and GRBG share a green diagonal just as BGGR and RGGB share a green antidiagonal. Resolving the remaining ambiguity needs colour: a scene of objects with known colour was captured in all four states, and the red channel identified as the one whose ratio to blue matches the scene. The rejected assignment is the reciprocal, so the margin is large: hflip=0 vflip=0 R/B 1.651 (reciprocal 0.606) -> GBRG hflip=0 vflip=1 R/B 1.622 (reciprocal 0.616) -> GBRG hflip=1 vflip=0 R/B 1.651 (reciprocal 0.606) -> BGGR hflip=1 vflip=1 R/B 1.625 (reciprocal 0.615) -> BGGR Derive the media bus code from HFLIP and tag the control with V4L2_CTRL_FLAG_MODIFY_LAYOUT so userspace knows to renegotiate the format after toggling it. The native order disagreeing with upstream is suspicious in itself. The removal of the per-mode {0x3811, 0x04} and {0x3813, 0x05} ISP window offsets in "media: i2c: ov13858: add rotate control support", which now writes 1 or 2 into those registers from the flip handlers, changes the X parity, and that patch also sets BIT(3) of ROTATE_CONTROL when HFLIP is clear while VFLIP sets BIT(4) when set. Whether the phase should instead be corrected there is left alone here: this patch only reports what the sensor is measured to emit. --- a/drivers/media/i2c/ov13858.c +++ b/drivers/media/i2c/ov13858.c @@ -1032,6 +1032,7 @@ struct v4l2_ctrl *vblank; struct v4l2_ctrl *hblank; struct v4l2_ctrl *exposure; + struct v4l2_ctrl *hflip; struct clk *img_clk; struct gpio_desc *reset_gpio; @@ -1222,8 +1223,10 @@ OV13858_DGTL_GAIN_MIN, OV13858_DGTL_GAIN_MAX, OV13858_DGTL_GAIN_STEP, OV13858_DGTL_GAIN_DEFAULT); - v4l2_ctrl_new_std(ctrl_hdlr, &ov13858_ctrl_ops, V4L2_CID_HFLIP, - 0, 1, 1, 0); + ov13858->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &ov13858_ctrl_ops, + V4L2_CID_HFLIP, 0, 1, 1, 0); + if (ov13858->hflip) + ov13858->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT; v4l2_ctrl_new_std(ctrl_hdlr, &ov13858_ctrl_ops, V4L2_CID_VFLIP, 0, 1, 1, 0); @@ -1258,12 +1261,24 @@ return ret; } -static void ov13858_update_pad_format(const struct ov13858_mode *mode, +/* + * Mirroring shifts the Bayer phase by one column, so the native GBRG order + * becomes BGGR. The vertical flip is compensated by the ISP Y window offset + * and leaves the order alone. + */ +static u32 ov13858_get_format_code(struct ov13858 *ov13858) +{ + return ov13858->hflip->val ? MEDIA_BUS_FMT_SBGGR10_1X10 + : MEDIA_BUS_FMT_SGBRG10_1X10; +} + +static void ov13858_update_pad_format(struct ov13858 *ov13858, + const struct ov13858_mode *mode, struct v4l2_mbus_framefmt *fmt) { fmt->width = mode->width; fmt->height = mode->height; - fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10; + fmt->code = ov13858_get_format_code(ov13858); fmt->field = V4L2_FIELD_NONE; } @@ -1418,7 +1433,7 @@ ARRAY_SIZE(supported_modes), width, height, fmt->format.width, fmt->format.height); - ov13858_update_pad_format(mode, &fmt->format); + ov13858_update_pad_format(ov13858, mode, &fmt->format); *v4l2_subdev_state_get_format(sd_state, fmt->pad) = fmt->format; if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) @@ -1453,11 +1468,11 @@ struct v4l2_subdev_state *sd_state, struct v4l2_subdev_mbus_code_enum *code) { - /* Only one bayer order(GRBG) is supported */ + /* Only one bayer order is supported, which one depends on the mirror */ if (code->index > 0) return -EINVAL; - code->code = MEDIA_BUS_FMT_SGRBG10_1X10; + code->code = ov13858_get_format_code(to_ov13858(sd)); return 0; } @@ -1469,7 +1484,7 @@ if (fse->index >= ARRAY_SIZE(supported_modes)) return -EINVAL; - if (fse->code != MEDIA_BUS_FMT_SGRBG10_1X10) + if (fse->code != ov13858_get_format_code(to_ov13858(sd))) return -EINVAL; fse->min_width = supported_modes[fse->index].width; @@ -1520,7 +1535,7 @@ { struct ov13858 *ov13858 = to_ov13858(sd); - ov13858_update_pad_format(ov13858->cur_mode, + ov13858_update_pad_format(ov13858, ov13858->cur_mode, v4l2_subdev_state_get_format(sd_state, 0)); return 0;