On Friday, February 18th, 2022 at 12:54, Hans de Goede <[email protected]> wrote:
> On 2/18/22 12:39, Simon Ser wrote:
> > On Friday, February 18th, 2022 at 11:38, Hans de Goede <[email protected]> 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.
On 2022-02-18 07:12, Simon Ser wrote:
> On Friday, February 18th, 2022 at 12:54, Hans de Goede <[email protected]> wrote:
>
>> On 2/18/22 12:39, Simon Ser wrote:
>>> On Friday, February 18th, 2022 at 11:38, Hans de Goede <[email protected]> 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.
On Fri, Feb 18, 2022 at 7:13 AM Simon Ser <[email protected]> wrote:
>
> On Friday, February 18th, 2022 at 12:54, Hans de Goede <[email protected]> wrote:
>
> > On 2/18/22 12:39, Simon Ser wrote:
> > > On Friday, February 18th, 2022 at 11:38, Hans de Goede <[email protected]> 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 don't think it's a big deal to change, but at least my
understanding, albeit this was back in the early KMS days, was that
probing was driven by things outside of the driver. I.e., there is no
need to probe displays if nothing is going to use them. If you want
to use the displays, you'd call probe first before trying to use them
so you know what is available.
Alex
>
> > >> 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.
On Fri, Feb 18, 2022 at 11:57 PM Harry Wentland <[email protected]> wrote:
>
> On 2022-02-18 07:12, Simon Ser wrote:
> > On Friday, February 18th, 2022 at 12:54, Hans de Goede <[email protected]> wrote:
> >
> >> On 2/18/22 12:39, Simon Ser wrote:
> >>> On Friday, February 18th, 2022 at 11:38, Hans de Goede <[email protected]> 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