On 2022-02-18 07:12, Simon Ser wrote: > 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... > 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: >> >> 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.
