On Fri, Feb 18, 2022 at 11:57 PM Harry Wentland harry.wentland@amd.com wrote:
On 2022-02-18 07:12, Simon Ser wrote:
On Friday, February 18th, 2022 at 12:54, Hans de Goede hdegoede@redhat.com wrote:
On 2/18/22 12:39, Simon Ser wrote:
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.
If before the initial probe of the connector there is a /dev/dri/card0 which plymouth can access, then plymouth may at this point decide to disable any seemingly unused crtcs, which will make the screen go black...
I'm not sure if plymouth will actually do this, but AFAICT this would not be invalid behavior for a userspace kms consumer to do and I believe it is likely that mutter will disable unused crtcs.
IMHO it is just a bad idea to register /dev/dri/card0 with userspace before the initial connector probe is done. Nothing good can come of that.
If all the exposed connectors initially are going to show up as disconnected anyways what is the value in registering /dev/dri/card0 with userspace early ?
OK. I'm still unsure how I feel about this, but I think I agree with you. That said, the amdgpu architecture is quite involved with multiple abstraction levels, so I don't think I'm equipped to write a patch to fix this...
amdgpu_dm's connector registration already triggers a detection. See the calls to dc_link_detect and amdgpu_dm_update_connector_after_detect in amdgpu_dm_initialize_drm_device.
dc_link_detect is supposed to read the edid via dm_helpers_read_local_edid and amdgpu_dm_update_connector_after_detect will update the EDID on the connector via a drm_connector_update_edid_property call.
This all happens at driver load.
I don't know why you're seeing the embedded connector as disconnected unless the DP-MIPI bridge for some reason doesn't indicate that the panel is connected at driver load.
Harry
cc Daniel Vetter: can you confirm probing all connectors is a good thing to do on driver module load?
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.
The DMI based quirking does, yes. But e.g. the quirk code directly reading this from the Intel VBT does not rely on the mode.
But if you are planning on using a DMI based quirk for the steamdeck then yes that needs the mode.
Thee mode check is there for 2 reasons:
- To avoid also applying the quirk to external displays, but
I think that that is also solved in most drivers by only checking for a quirk at all on the eDP connector
- Some laptop models ship with different panels in different badges
some of these are portrait (so need a panel-orient) setting and others are landscape.
That makes sense. So yeah the EDID mode based matching logic needs to stay to accomodate for these cases.
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.
We could extend the DMI quirk mechanism to allow quirks which don't do the mode check, for use on devices where we can guarantee neither 1 nor 2 happens, then amdgpu could call the quirk code early simply passing 0x0 as resolution.
Yeah. But per the above amdgpu should maybe probe connectors on module load. If/when amdgpu is fixed to do this, then we don't need to disable the mode matching logic in panel-orientation quirks anymore.
Hi all,
Thanks for all of the discussion. I'm not sure about how amd drm works, but for some SoC, the panel orientation is set in panel[1]. The goal of this patch is to separate the property creation, so some drm can optionally create it earlier before drm_dev_register().
I've sent the v9 to address some issues in v8, but the basic idea is still the same. It has no effect to drm_connector_set_panel_orientation_with_quirk() used in amdgpu and i915, they work the same as before. Do you think this is reasonable?
[1] https://elixir.bootlin.com/linux/v5.17-rc7/source/drivers/gpu/drm/panel/pane...