On Friday, February 18th, 2022 at 12:54, Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > On 2/18/22 12:39, Simon Ser wrote: > > On Friday, February 18th, 2022 at 11:38, Hans de Goede <hdegoede@xxxxxxxxxx> 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: > >> > >> 1. 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... 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: > > 1. 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 > > 2. 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.
