Surface Pro 9: Set correct front camera rotation #160
Conversation
|
If I understand correctly, this would rotate all OV5693 senors, regardless of the actual device / mounting. Is there some way to get the mounting orientation from ACPI? @djrscally maybe you know more? |
|
I could remove the auto-rotation and leave the rotation control implemented if that's better? |
|
Switched default rotation to 0: ctrls->rotation = v4l2_ctrl_new_std(&ctrls->handler, ops,
V4L2_CID_CAMERA_SENSOR_ROTATION, 0, 270, 90, 0); |
qzed
left a comment
qzed
left a comment
There was a problem hiding this comment.
Okay, looks good to me now.
I'm still a bit concerned about the rotation being applied for all OVTI5693 matches. I have the feeling that that somehow should be specified in some ACPI method, but that might need some reverse engineering to figure out. At least from the ACPI dumps it looks like the older devices all use the INT33BE match, so for now this should not create problems changing the orientation on those. I still want to give it a test first before I merge though.
|
I've set this up to use a parameter instead to set the default rotation, as it's the only way I managed to get it to work. And I can see: v4l2-ctl --device /dev/v4l-subdev4 --list-ctrls
User Controls
exposure 0x00980911 (int) : min=1 max=2070 step=1 default=1030 value=1030 flags=has-min-max
horizontal_flip 0x00980914 (bool) : default=0 value=0 flags=has-min-max
vertical_flip 0x00980915 (bool) : default=0 value=0 flags=has-min-max
Camera Controls
camera_orientation 0x009a0922 (menu) : min=0 max=2 default=0 value=0 (Front) flags=read-only, has-min-max
camera_sensor_rotation 0x009a0923 (int) : min=0 max=270 step=90 default=180 value=180 flags=read-only, has-min-max
Image Source Controls
vertical_blanking 0x009e0901 (int) : min=4 max=63591 step=1 default=134 value=134 flags=has-min-max
horizontal_blanking 0x009e0902 (int) : min=96 max=96 step=1 default=96 value=96 flags=read-only, has-min-max
analogue_gain 0x009e0903 (int) : min=1 max=127 step=1 default=8 value=8 flags=has-min-max
Image Processing Controls
link_frequency 0x009f0901 (intmenu): min=0 max=0 default=0 value=0 (419200000 0x18fc7c00) flags=read-only, has-min-max
pixel_rate 0x009f0902 (int64) : min=0 max=167680000 step=1 default=167680000 value=167680000 flags=read-only, has-min-max
test_pattern 0x009f0903 (menu) : min=0 max=3 default=0 value=0 (Disabled) flags=has-min-max
digital_gain 0x009f0905 (int) : min=1 max=4095 step=1 default=1024 value=1024 flags=has-min-maxThis seems more of a workaround than a long-term solution but at least it's a way to get the correct rotation. Feel free to close this if it's not up to standard but I thought I'd share in case anyone else wants to look into this. |
gabrielstedman
force-pushed
the
v6.18-surface-devel
branch
from
7324d6d to
988a2cd
Compare
|
Okay, for some reason this flips the front camera on my SB2 (same sensor) by default. With |
Good catch so my patch causes regression in your SB2 I'm guessing because the firmware does return the correct rotation but my patch is overriding that. /* Rotation */
ctrls->rotation = v4l2_ctrl_new_std(&ctrls->handler, ops,
V4L2_CID_CAMERA_SENSOR_ROTATION, 0, 270, 90, sensor_default_rotation);If the parameter is explicitly set - should we try this? |
|
Yeah, sounds like that should work. |
|
Hello - sorry for the delay replying. This shouldn't be necessary. The driver calls
My guess would be the first is more likely...I would look for a message reading "Unknown rotation %d. Assume 0 degree rotation" in the dmesg output to confirm that...but @gabrielstedman I think what you say above implies that the rotation control is not exposed at all by the ov5693 driver without your patches...is that right? |
What do you think should be the next steps? |
|
I would start by trying to see why the control isn't exposed as it should be...the function that registers them won't do that if the property values are initialised to By way of explainer; these devices have firmware nodes created by the kernel's ACPI subsystem, which are the primaries. These have historically been totally useless and so the ipu-bridge creates software nodes that contain all the right data, and we rely on those to make this work - these are the secondary nodes, and when the kernel says "get the orientation property for this device" it should look at the secondary / software node instead of the primary one. Could you possibly put a debug printout right here and see if it's hit or not, and what the value of the |
|
I just wanted to point out here that on the Surface Pro 7+ the camera does not need to be flipped (ref linux-surface/linux-surface#1867 (comment)) |
You are right the control is actually set without this patch. v4l2-ctl --device /dev/v4l-subdev4 --list-ctrls
...
Camera Controls
camera_orientation 0x009a0922 (menu) : min=0 max=2 default=0 value=0 (Front) flags=read-only, has-min-max
camera_sensor_rotation 0x009a0923 (int) : min=0 max=0 step=1 default=0 value=0 flags=read-only, has-min-max
...I will look into |
|
Ah ok; if the control is being created, but set to zero, then the problem is probably either that the SSDB buffer says it's zero or it says something that's total garbage and so the ipu-bridge just assumes 0 to be safe. Do you see a message reading "Unknown rotation %d. Assume 0 degree rotation" in the output from dmesg? |
I don't see this message. So this means /* Data representation as it is in ACPI SSDB buffer */
struct ipu_sensor_ssdb {
...
u8 degree;
...
} __packed;If I interpret this correctly for SB2 degree is set to 1, whilst SP9 degree is set to 0 (u8 value at 0x54 offset) SP9: |
Yep that tracks, so the firmware says that the camera is not inverted. How fun. |
I just introduced a change so that only if the parameter is explicitly set will it define default rotation. Not sure how we would handle this in a more elegant way. |
gabrielstedman
force-pushed
the
v6.18-surface-devel
branch
from
d1d228b to
0c8a7b5
Compare
4 participants
media: i2c: ov5693: add rotation control and apply 180° quirk for OVTI5693
The OVTI5693 module used in the Surface Pro 9 front camera is mounted upside‑down. libcamera ignores HFLIP/VFLIP and relies on V4L2_CID_CAMERA_SENSOR_ROTATION to determine sensor orientation, but the OV5693 driver did not expose this control correctly (V4L2_FWNODE_PROPERTY_UNSET)
This patch adds a rotation control to the driver and
applies a 180° default for the OVTI5693 ACPI‑matched variant.