On Tuesday, February 15th, 2022 at 15:38, Emil Velikov <[email protected]> wrote:
> On Tue, 15 Feb 2022 at 13:55, Simon Ser <[email protected]> wrote:
> >
> > On Tuesday, February 15th, 2022 at 13:04, Emil Velikov <[email protected]> wrote:
> >
> > > Greetings everyone,
> > >
> > > Padron for joining in so late o/
> > >
> > > On Tue, 8 Feb 2022 at 08:42, Hsin-Yi Wang <[email protected]> wrote:
> > > >
> > > > drm_dev_register() sets connector->registration_state to
> > > > DRM_CONNECTOR_REGISTERED and dev->registered to true. If
> > > > drm_connector_set_panel_orientation() is first called after
> > > > drm_dev_register(), it will fail several checks and results in following
> > > > warning.
> > > >
> > > > Add a function to create panel orientation property and set default value
> > > > to UNKNOWN, so drivers can call this function to init the property earlier
> > > > , and let the panel set the real value later.
> > > >
> > >
> > > The warning illustrates a genuine race condition, where userspace will
> > > read the old/invalid property value/state. So this patch masks away
> > > the WARNING without addressing the actual issue.
> > > Instead can we fix the respective drivers, so that no properties are
> > > created after drm_dev_register()?
> > >
> > > Longer version:
> > > As we look into drm_dev_register() it's in charge of creating the
> > > dev/sysfs nodes (et al). Note that connectors cannot disappear at
> > > runtime.
> > > For panel orientation, we are creating an immutable connector
> > > properly, meaning that as soon as drm_dev_register() is called we must
> > > ensure that the property is available (if applicable) and set to the
> > > correct value.
> >
> > Unfortunately we can't quite do this. To apply the panel orientation quirks we
> > need to grab the EDID of the eDP connector, and this happened too late in my
> > testing.
> >
> > What we can do is create the prop early during module load, and update it when
> > we read the EDID (at the place where we create it right now). User-space will
> > receive a hotplug event after the EDID is read, so will be able to pick up the
> > new value if any.
>
> Didn't quite get that, are you saying that a GETPROPERTY for the EDID,
> the ioctl blocks or that we get an empty EDID?
I'm not referring to GETPROPERTY, I'm referring to the driver getting the EDID
from the sink (here, the eDP panel). In my experimentations with amdgpu I
noticed that the driver module load finished before the EDID was available to
the driver. Maybe other drivers behave differently and probe connectors when
loaded, not sure.
> The EDID hotplug even thing is neat - sounds like it also signals on
> panel orientation, correct?
> On such an event, which properties userspace should be re-fetching -
> everything or guess randomly?
>
> Looking through the documentation, I cannot see a clear answer :-\
User-space should re-fetch *all* properties. In practice some user-space may
only be fetching some properties, but that should get fixed in user-space.
Also the kernel can indicate that only a single connector changed via the
"CONNECTOR" uevent prop, or even a single connector property via "PROPERTY".
See [1] for a user-space implementation. But all of this is purely an optional
optimization. Re-fetching all properties is a bit slower (especially if some
drmModeGetConnector calls force-probe connectors) but works perfectly fine.
It would be nice to document, if you have the time feel free to send a patch
and CC danvet, pq and me.
[1]: https://gitlab.freedesktop.org/wlroots/wlroots/-/blob/252b2348bd62170d97c4e81fb2050f757b56d67e/backend/session/session.c#L144
On Tue, 15 Feb 2022 at 16:37, Simon Ser <[email protected]> wrote:
>
> On Tuesday, February 15th, 2022 at 15:38, Emil Velikov <[email protected]> wrote:
>
> > On Tue, 15 Feb 2022 at 13:55, Simon Ser <[email protected]> wrote:
> > >
> > > On Tuesday, February 15th, 2022 at 13:04, Emil Velikov <[email protected]> wrote:
> > >
> > > > Greetings everyone,
> > > >
> > > > Padron for joining in so late o/
> > > >
> > > > On Tue, 8 Feb 2022 at 08:42, Hsin-Yi Wang <[email protected]> wrote:
> > > > >
> > > > > drm_dev_register() sets connector->registration_state to
> > > > > DRM_CONNECTOR_REGISTERED and dev->registered to true. If
> > > > > drm_connector_set_panel_orientation() is first called after
> > > > > drm_dev_register(), it will fail several checks and results in following
> > > > > warning.
> > > > >
> > > > > Add a function to create panel orientation property and set default value
> > > > > to UNKNOWN, so drivers can call this function to init the property earlier
> > > > > , and let the panel set the real value later.
> > > > >
> > > >
> > > > The warning illustrates a genuine race condition, where userspace will
> > > > read the old/invalid property value/state. So this patch masks away
> > > > the WARNING without addressing the actual issue.
> > > > Instead can we fix the respective drivers, so that no properties are
> > > > created after drm_dev_register()?
> > > >
> > > > Longer version:
> > > > As we look into drm_dev_register() it's in charge of creating the
> > > > dev/sysfs nodes (et al). Note that connectors cannot disappear at
> > > > runtime.
> > > > For panel orientation, we are creating an immutable connector
> > > > properly, meaning that as soon as drm_dev_register() is called we must
> > > > ensure that the property is available (if applicable) and set to the
> > > > correct value.
> > >
> > > Unfortunately we can't quite do this. To apply the panel orientation quirks we
> > > need to grab the EDID of the eDP connector, and this happened too late in my
> > > testing.
> > >
> > > What we can do is create the prop early during module load, and update it when
> > > we read the EDID (at the place where we create it right now). User-space will
> > > receive a hotplug event after the EDID is read, so will be able to pick up the
> > > new value if any.
> >
> > Didn't quite get that, are you saying that a GETPROPERTY for the EDID,
> > the ioctl blocks or that we get an empty EDID?
>
> I'm not referring to GETPROPERTY, I'm referring to the driver getting the EDID
> from the sink (here, the eDP panel). In my experimentations with amdgpu I
> noticed that the driver module load finished before the EDID was available to
> the driver. Maybe other drivers behave differently and probe connectors when
> loaded, not sure.
>
I see thanks.
> > The EDID hotplug even thing is neat - sounds like it also signals on
> > panel orientation, correct?
> > On such an event, which properties userspace should be re-fetching -
> > everything or guess randomly?
> >
> > Looking through the documentation, I cannot see a clear answer :-\
>
> User-space should re-fetch *all* properties. In practice some user-space may
> only be fetching some properties, but that should get fixed in user-space.
>
> Also the kernel can indicate that only a single connector changed via the
> "CONNECTOR" uevent prop, or even a single connector property via "PROPERTY".
> See [1] for a user-space implementation. But all of this is purely an optional
> optimization. Re-fetching all properties is a bit slower (especially if some
> drmModeGetConnector calls force-probe connectors) but works perfectly fine.
>
Looking at KDE/kwin (the one I'm running) - doesn't seem like it
handles any of the three "HOTPLUG", "PROPERTY" or "CONNECTOR" uevents
:-\
Skimming through GNOME/mutter - it handles "HOTPLUG", but not the optional ones.
Guess we're in the clear wrt potential races, even though the
documentation on the topic is lacklustre.
> It would be nice to document, if you have the time feel free to send a patch
> and CC danvet, pq and me.
>
Doubt I will have the time in the upcoming weeks, but I'll add it to
my ever-growing TODO list :-P
Thanks
Emil
Hi,
Sorry for jumping in in the middle of the thread I did
not notice this thread before.
On 2/16/22 13:00, Emil Velikov wrote:
> On Tue, 15 Feb 2022 at 16:37, Simon Ser <[email protected]> wrote:
>>
>> On Tuesday, February 15th, 2022 at 15:38, Emil Velikov <[email protected]> wrote:
>>
>>> On Tue, 15 Feb 2022 at 13:55, Simon Ser <[email protected]> wrote:
>>>>
>>>> On Tuesday, February 15th, 2022 at 13:04, Emil Velikov <[email protected]> wrote:
>>>>
>>>>> Greetings everyone,
>>>>>
>>>>> Padron for joining in so late o/
>>>>>
>>>>> On Tue, 8 Feb 2022 at 08:42, Hsin-Yi Wang <[email protected]> wrote:
>>>>>>
>>>>>> drm_dev_register() sets connector->registration_state to
>>>>>> DRM_CONNECTOR_REGISTERED and dev->registered to true. If
>>>>>> drm_connector_set_panel_orientation() is first called after
>>>>>> drm_dev_register(), it will fail several checks and results in following
>>>>>> warning.
>>>>>>
>>>>>> Add a function to create panel orientation property and set default value
>>>>>> to UNKNOWN, so drivers can call this function to init the property earlier
>>>>>> , and let the panel set the real value later.
>>>>>>
>>>>>
>>>>> The warning illustrates a genuine race condition, where userspace will
>>>>> read the old/invalid property value/state. So this patch masks away
>>>>> the WARNING without addressing the actual issue.
>>>>> Instead can we fix the respective drivers, so that no properties are
>>>>> created after drm_dev_register()?
>>>>>
>>>>> Longer version:
>>>>> As we look into drm_dev_register() it's in charge of creating the
>>>>> dev/sysfs nodes (et al). Note that connectors cannot disappear at
>>>>> runtime.
>>>>> For panel orientation, we are creating an immutable connector
>>>>> properly, meaning that as soon as drm_dev_register() is called we must
>>>>> ensure that the property is available (if applicable) and set to the
>>>>> correct value.
>>>>
>>>> Unfortunately we can't quite do this. To apply the panel orientation quirks we
>>>> need to grab the EDID of the eDP connector, and this happened too late in my
>>>> testing.
>>>>
>>>> What we can do is create the prop early during module load, and update it when
>>>> we read the EDID (at the place where we create it right now). User-space will
>>>> receive a hotplug event after the EDID is read, so will be able to pick up the
>>>> new value if any.
>>>
>>> Didn't quite get that, are you saying that a GETPROPERTY for the EDID,
>>> the ioctl blocks or that we get an empty EDID?
>>
>> I'm not referring to GETPROPERTY, I'm referring to the driver getting the EDID
>> from the sink (here, the eDP panel). In my experimentations with amdgpu I
>> noticed that the driver module load finished before the EDID was available to
>> the driver. Maybe other drivers behave differently and probe connectors when
>> loaded, not sure.
>>
> I see thanks.
>
>>> The EDID hotplug even thing is neat - sounds like it also signals on
>>> panel orientation, correct?
>>> On such an event, which properties userspace should be re-fetching -
>>> everything or guess randomly?
>>>
>>> Looking through the documentation, I cannot see a clear answer :-\
>>
>> User-space should re-fetch *all* properties. In practice some user-space may
>> only be fetching some properties, but that should get fixed in user-space.
>>
>> Also the kernel can indicate that only a single connector changed via the
>> "CONNECTOR" uevent prop, or even a single connector property via "PROPERTY".
>> See [1] for a user-space implementation. But all of this is purely an optional
>> optimization. Re-fetching all properties is a bit slower (especially if some
>> drmModeGetConnector calls force-probe connectors) but works perfectly fine
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
3. amdgpu adjusts the panel-orient prop to upside-down, sends out
uevents
4. Lets assume plymouth handles this well (i) and now adjust its
rendering and renders the next frame of the bootsplash 180° rotated
to compensate for the panel being upside down. Then from now on
the user will see the splash normally
So this means that the user will briefly see the bootsplash rendered
upside down which IMHO is not acceptable behavior. Also see my footnote
about how I seriously doubt plymouth will see the panel-orient change
at all.
I'm also a bit unsure about:
a) How you can register the panel connector with userspace before
reading the edid, don't you need the edid to give the physical size +
modeline to userspace, which you cannot just leave out ?
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 ?
Regards,
Hans
i) I don't think plymouth will handle this well though, since it tries to
skip unchanged connectors and I believe it only checks the crtc routing +
preferred modeline to determine "unchanged".
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.
> 3. amdgpu adjusts the panel-orient prop to upside-down, sends out
> uevents
That's when amdgpu marks the connector as connected. So everything
should be fine I believe, no bad frame.
> 4. Lets assume plymouth handles this well (i) and now adjust its
> rendering and renders the next frame of the bootsplash 180° rotated
> to compensate for the panel being upside down. Then from now on
> the user will see the splash normally
>
> So this means that the user will briefly see the bootsplash rendered
> upside down which IMHO is not acceptable behavior. Also see my footnote
> about how I seriously doubt plymouth will see the panel-orient change
> at all.
>
> I'm also a bit unsure about:
>
> a) How you can register the panel connector with userspace before
> reading the edid, don't you need the edid to give the physical size +
> modeline to userspace, which you cannot just leave out ?
Yup. The KMS EDID property is created before the EDID is read, and is set
to zero (NULL blob). The width/height in mm and other info are also zero.
You can try inspecting the state printed by drm_info on any disconnected
connector to see for yourself.
> 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. 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.
Also note, DisplayID has a bit to indicate the panel orientation IIRC.
Would be nice to support parsing this at some point.