Hello, Jacopo, Sakari and I ended up having a long discussion today about the interactions between sensor rotation (as described in the device tree) and the V4L2 flip controls. The conversation started from the imx258 series that Jacopo recently posted ([1]) and ended up as an in-depth analysis of the problem. The notes we have taken are copied below. Feedback would be appreciated, I will then translate that into patches for the kernel documentation. [1] https://lore.kernel.org/linux-media/20230117100603.51631-1-jacopo.mondi@xxxxxxxxxxxxxxxx/ ## Problem description V4L2 has five different elements that related to flipping and rotation: - Device tree "rotation" property - V4L2_CID_CAMERA_SENSOR_ROTATION control - V4L2_CID_ROTATE control - V4L2_CID_HFLIP and V4L2_CID_VFLIP controls - Bayer pattern exposed through media bus codes While all those components are (more or less) well-defined in the API, their interactions have never been defined. This has led to different drivers implementing different behaviours. ### Full-featured drivers List of drivers that parse the DT rotation property and expose V4L2_CID_CAMERA_SENSOR_ROTATION, V4L2_CID_HFLIP and V4L2_CID_VFLIP: $ git grep -l FLIP $(git grep -l v4l2_ctrl_new_fwnode_properties -- drivers/media/i2c/) - drivers/media/i2c/imx219.c - drivers/media/i2c/ov08x40.c - drivers/media/i2c/ov13b10.c - drivers/media/i2c/ov5640.c - drivers/media/i2c/ov5675.c (to be upstreamed) - drivers/media/i2c/ov5693.c - drivers/media/i2c/ov8865.c - drivers/media/i2c/ov9282.c All those drivers by ov5640 program the sensor with the HFLIP and VFLIP values as-is, without taking the rotation property into account. ov5640 inverts the flipping controls transparently when the rotation is 180, but does still expose the rotation value to userspace unmodified (commit 1066fc1c2afdbe5977eae37314f0c21462e82b9a, merged in v6.0). ### Flip-enabled drivers List of drivers that expose the V4L2_CID_HFLIP and V4L2_CID_VFLIP but not V4L2_CID_CAMERA_SENSOR_ROTATION: $ git grep -vl v4l2_ctrl_new_fwnode_properties $(git grep -l V4L2_CID_HFLIP -- drivers/media/i2c/) - drivers/media/i2c/ccs/ccs-core.c - drivers/media/i2c/hi847.c - drivers/media/i2c/imx208.c - drivers/media/i2c/imx219.c - drivers/media/i2c/imx319.c - drivers/media/i2c/imx355.c - drivers/media/i2c/mt9m032.c - drivers/media/i2c/mt9m111.c - drivers/media/i2c/mt9p031.c - drivers/media/i2c/mt9v011.c - drivers/media/i2c/ov08d10.c - drivers/media/i2c/ov08x40.c - drivers/media/i2c/ov13b10.c - drivers/media/i2c/ov2640.c - drivers/media/i2c/ov2680.c - drivers/media/i2c/ov5640.c - drivers/media/i2c/ov5645.c - drivers/media/i2c/ov5648.c - drivers/media/i2c/ov5675.c - drivers/media/i2c/ov5693.c - drivers/media/i2c/ov6650.c - drivers/media/i2c/ov7251.c - drivers/media/i2c/ov7670.c - drivers/media/i2c/ov772x.c - drivers/media/i2c/ov7740.c - drivers/media/i2c/ov8856.c - drivers/media/i2c/ov8865.c - drivers/media/i2c/ov9282.c - drivers/media/i2c/ov9640.c - drivers/media/i2c/ov9650.c - drivers/media/i2c/rj54n1cb0c.c - drivers/media/i2c/s5k5baf.c - drivers/media/i2c/s5k6aa.c - drivers/media/i2c/st-vgxy61.c - drivers/media/i2c/vs6624.c Among those, the ccs driver parses the DT rotation property manually and compensates for it transparently by inverting the flip values. The ov772x and s5k6aa use a similar mechanism, but based on platform data instead of DT. ### Rotation-aware drivers List of drivers that parse the DT rotation property manually: $ git grep -l '"rotation"' -- drivers/media/i2c/ - drivers/media/i2c/ccs/ccs-core.c - drivers/media/i2c/imx258.c - drivers/media/i2c/ov02a10.c All those drivers parse the DT rotation property and compensates for it transparently. The ccs driver inverts the HFLIP and VFLIP controls exposed to userspace, while the imx258 and ov02a10 flip the image internally but do not expose the HFLIP and VFLIP controls. ## API standardization There is a consensus that a standardized API is required. There is also a consensus that the V4L2_CID_ROTATE control must *not* be used by any sensor driver. No sensor driver expose that control at the moment, so this shouldn't be a problem. ### API for new drivers - Expose the rotation property through V4L2_CID_CAMERA_SENSOR_ROTATION as-is. - Expose the V4L2_CID_HFLIP and V4L2_CID_VFLIP controls as-is. - A sensor driver that enables horizontal or vertical flipping *must* expose the HFLIP and VFLIP controls. It *should* expose them writable, but *may* expose them read-only if not enough information is available to implement them as writable in the driver. ### Backward-compatibility For drivers: - We don't care about existing drivers that use platform data (ov772x and s5k6aa). The s5k6aa driver requires platform data, so it could be dropped as nobody is supplying platform data in mainline. - The full-featured drivers comply with the API for new drivers except for ov5640. Those are thus fine. - The ov5640 gained V4L2_CID_CAMERA_SENSOR_ROTATION support in v6.0, we should fix it, even if it changes the userspace-visible behaviour. - Dropping the internal flip has a higher risk of breaking applications. - Overriding the V4L2_CID_CAMERA_SENSOR_ROTATION value and setting it to 0 when it is 180 is less risky. - ccs should expose the V4L2_CID_CAMERA_SENSOR_ROTATION control, and modify it internally to account the transparent 180° compensation. - For imx258 and ov02a10, two options are possible: - Expose the V4L2_CID_CAMERA_SENSOR_ROTATION control as-is, and expose the HFLIP and VFLIP controls read-only and hardcoded to enabled (for imx258) or set based on the rotation (for ov02a10). The controls could later be made writable. This only risk of userspace breakage would be with applications that consider the V4L2_CID_CAMERA_SENSOR_ROTATION control but not the flip controls. This is considered to be low-risk. - Do as ccs (overriding the V4L2_CID_CAMERA_SENSOR_ROTATION value). This is the option preferred by Sakari as it would unify the behaviour of the ccs, imx258 and ov02a10 drivers. For userspace: - If the V4L2_CID_CAMERA_SENSOR_ROTATION control is not exposed, userspace *must* assume that the rotation is 0. - If the HFLIP and VFLIP controls are not exposed, userspace *must* assume that no flipping occurs. - The captured video is upright if rotation == 0 and both flipping controls are disabled or rotation == 180 and both flipping controls are enabled. - Userspace *must* support read-only HFLIP and VFLIP controls. -- Regards, Laurent Pinchart
