On Fri, Feb 18, 2022 at 11:57 PM Harry Wentland <harry.wentland@xxxxxxx> wrote: > > 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. > 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/panel-edp.c#L556
