On Friday, February 18th, 2022 at 11:38, Hans de Goede hdegoede@redhat.com wrote:
What I'm reading in the above is that it is being considered to allow changing the panel-orientation value after the connector has been made available to userspace; and let userspace know about this through a uevent.
I believe that this is a bad idea, it is important to keep in mind here what userspace (e.g. plymouth) uses this prorty for. This property is used to rotate the image being rendered / shown on the framebuffer to adjust for the panel orientation.
So now lets assume we apply the correct upside-down orientation later on a device with an upside-down mounted LCD panel. Then on boot the following could happen:
- amdgpu exports a connector for the LCD panel to userspace without
setting panel-orient=upside-down 2. plymouth sees this and renders its splash normally, but since the panel is upside-down it will now actually show upside-down
At this point amdgpu hasn't probed the connector yet. So the connector will be marked as disconnected, and plymouth shouldn't render anything.
- amdgpu adjusts the panel-orient prop to upside-down, sends out
uevents
That's when amdgpu marks the connector as connected. So everything should be fine I believe, no bad frame.
- Lets assume plymouth handles this well (i) and now adjust its
rendering and renders the next frame of the bootsplash 180° rotated to compensate for the panel being upside down. Then from now on the user will see the splash normally
So this means that the user will briefly see the bootsplash rendered upside down which IMHO is not acceptable behavior. Also see my footnote about how I seriously doubt plymouth will see the panel-orient change at all.
I'm also a bit unsure about:
a) How you can register the panel connector with userspace before reading the edid, don't you need the edid to give the physical size + modeline to userspace, which you cannot just leave out ?
Yup. The KMS EDID property is created before the EDID is read, and is set to zero (NULL blob). The width/height in mm and other info are also zero. You can try inspecting the state printed by drm_info on any disconnected connector to see for yourself.
I guess the initial modeline is inherited from the video-bios, but what about the physical size? Note that you cannot just change the physical size later either, that gets used to determine the hidpi scaling factor in the bootsplash, and changing that after the initial bootsplash dislay will also look ugly
b) Why you need the edid for the panel-orientation property at all, typically the edid prom is part of the panel and the panel does not know that it is mounted e.g. upside down at all, that is a property of the system as a whole not of the panel as a standalone unit so in my experience getting panel-orient info is something which comes from the firmware /video-bios not from edid ?
This is an internal DRM thing. The orientation quirks logic uses the mode size advertised by the EDID. I agree that at least in the Steam Deck case it may not make a lot of sense to use any info from the EDID, but that's needed for the current status quo.
Also note, DisplayID has a bit to indicate the panel orientation IIRC. Would be nice to support parsing this at some point.