| Message ID | 20230215113833.477999-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] drm/amd/display: Pass proper parent for DM backlight device registration | expand |
On Wed, Feb 15, 2023 at 6:38 AM Hans de Goede <hdegoede@redhat.com> wrote: > > The parent for the backlight device should be the drm-connector object, > not the PCI device. > > Userspace relies on this to be able to detect which backlight class device > to use on hybrid gfx devices where there may be multiple native (raw) > backlight devices registered. > > Specifically gnome-settings-daemon expects the parent device to have > an "enabled" sysfs attribute (as drm_connector devices do) and tests > that this returns "enabled" when read. > > This aligns the parent of the backlight device with i915, nouveau, radeon. > Note that drivers/gpu/drm/amd/amdgpu/atombios_encoders.c also already > uses the drm_connector as parent, only amdgpu_dm.c used the PCI device > as parent before this change. > > Note this is marked as a RFC because I don't have hw to test, so this > has only been compile tested! If someone can test this on actual > hw which hits the changed code path that would be great. > > Link: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/issues/730 > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Patch looks fine to me. I'll apply it and we can run it through our CI system. Alex > --- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index 31bce529f685..33b0e1de2770 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -4065,7 +4065,8 @@ static const struct backlight_ops amdgpu_dm_backlight_ops = { > }; > > static void > -amdgpu_dm_register_backlight_device(struct amdgpu_display_manager *dm) > +amdgpu_dm_register_backlight_device(struct amdgpu_display_manager *dm, > + struct amdgpu_dm_connector *aconnector) > { > char bl_name[16]; > struct backlight_properties props = { 0 }; > @@ -4088,7 +4089,7 @@ amdgpu_dm_register_backlight_device(struct amdgpu_display_manager *dm) > adev_to_drm(dm->adev)->primary->index + dm->num_of_edps); > > dm->backlight_dev[dm->num_of_edps] = backlight_device_register(bl_name, > - adev_to_drm(dm->adev)->dev, > + aconnector->base.kdev, > dm, > &amdgpu_dm_backlight_ops, > &props); > @@ -4141,6 +4142,7 @@ static int initialize_plane(struct amdgpu_display_manager *dm, > > > static void register_backlight_device(struct amdgpu_display_manager *dm, > + struct amdgpu_dm_connector *aconnector, > struct dc_link *link) > { > if ((link->connector_signal & (SIGNAL_TYPE_EDP | SIGNAL_TYPE_LVDS)) && > @@ -4151,7 +4153,7 @@ static void register_backlight_device(struct amdgpu_display_manager *dm, > * is better then a black screen. > */ > if (!dm->backlight_dev[dm->num_of_edps]) > - amdgpu_dm_register_backlight_device(dm); > + amdgpu_dm_register_backlight_device(dm, aconnector); > > if (dm->backlight_dev[dm->num_of_edps]) { > dm->backlight_link[dm->num_of_edps] = link; > @@ -4337,7 +4339,7 @@ static int amdgpu_dm_initialize_drm_device(struct amdgpu_device *adev) > > if (ret) { > amdgpu_dm_update_connector_after_detect(aconnector); > - register_backlight_device(dm, link); > + register_backlight_device(dm, aconnector, link); > > if (dm->num_of_edps) > update_connector_ext_caps(aconnector); > -- > 2.39.1 >
Hi, On 2/15/23 12:38, Hans de Goede wrote: > The parent for the backlight device should be the drm-connector object, > not the PCI device. > > Userspace relies on this to be able to detect which backlight class device > to use on hybrid gfx devices where there may be multiple native (raw) > backlight devices registered. > > Specifically gnome-settings-daemon expects the parent device to have > an "enabled" sysfs attribute (as drm_connector devices do) and tests > that this returns "enabled" when read. > > This aligns the parent of the backlight device with i915, nouveau, radeon. > Note that drivers/gpu/drm/amd/amdgpu/atombios_encoders.c also already > uses the drm_connector as parent, only amdgpu_dm.c used the PCI device > as parent before this change. > > Note this is marked as a RFC because I don't have hw to test, so this > has only been compile tested! If someone can test this on actual > hw which hits the changed code path that would be great. > > Link: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/issues/730 > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Self NACK. This has been tested by 2 reporters of: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/issues/730 Now and it does not work. Instead of setting the parent device pointer correctly, this makes the backlight device not have a parent device any more at all. I already was afraid this might happen, since the drm_connector object is not yet registered at the time when the amdgpu code calls backlight_device_register(). Other drivers like e.g. nouveau register the backlight later from a drm_connector_funcs.late_register callback. I was hoping doing it the simple way as this patch did would work, but it looks like some bigger changes to the amdgpu code (using a drm_connector_funcs.late_register callback) are necessary. I'll try to make some time to prepare a new patch. Regards, Hans > --- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index 31bce529f685..33b0e1de2770 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -4065,7 +4065,8 @@ static const struct backlight_ops amdgpu_dm_backlight_ops = { > }; > > static void > -amdgpu_dm_register_backlight_device(struct amdgpu_display_manager *dm) > +amdgpu_dm_register_backlight_device(struct amdgpu_display_manager *dm, > + struct amdgpu_dm_connector *aconnector) > { > char bl_name[16]; > struct backlight_properties props = { 0 }; > @@ -4088,7 +4089,7 @@ amdgpu_dm_register_backlight_device(struct amdgpu_display_manager *dm) > adev_to_drm(dm->adev)->primary->index + dm->num_of_edps); > > dm->backlight_dev[dm->num_of_edps] = backlight_device_register(bl_name, > - adev_to_drm(dm->adev)->dev, > + aconnector->base.kdev, > dm, > &amdgpu_dm_backlight_ops, > &props); > @@ -4141,6 +4142,7 @@ static int initialize_plane(struct amdgpu_display_manager *dm, > > > static void register_backlight_device(struct amdgpu_display_manager *dm, > + struct amdgpu_dm_connector *aconnector, > struct dc_link *link) > { > if ((link->connector_signal & (SIGNAL_TYPE_EDP | SIGNAL_TYPE_LVDS)) && > @@ -4151,7 +4153,7 @@ static void register_backlight_device(struct amdgpu_display_manager *dm, > * is better then a black screen. > */ > if (!dm->backlight_dev[dm->num_of_edps]) > - amdgpu_dm_register_backlight_device(dm); > + amdgpu_dm_register_backlight_device(dm, aconnector); > > if (dm->backlight_dev[dm->num_of_edps]) { > dm->backlight_link[dm->num_of_edps] = link; > @@ -4337,7 +4339,7 @@ static int amdgpu_dm_initialize_drm_device(struct amdgpu_device *adev) > > if (ret) { > amdgpu_dm_update_connector_after_detect(aconnector); > - register_backlight_device(dm, link); > + register_backlight_device(dm, aconnector, link); > > if (dm->num_of_edps) > update_connector_ext_caps(aconnector);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 31bce529f685..33b0e1de2770 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -4065,7 +4065,8 @@ static const struct backlight_ops amdgpu_dm_backlight_ops = { }; static void -amdgpu_dm_register_backlight_device(struct amdgpu_display_manager *dm) +amdgpu_dm_register_backlight_device(struct amdgpu_display_manager *dm, + struct amdgpu_dm_connector *aconnector) { char bl_name[16]; struct backlight_properties props = { 0 }; @@ -4088,7 +4089,7 @@ amdgpu_dm_register_backlight_device(struct amdgpu_display_manager *dm) adev_to_drm(dm->adev)->primary->index + dm->num_of_edps); dm->backlight_dev[dm->num_of_edps] = backlight_device_register(bl_name, - adev_to_drm(dm->adev)->dev, + aconnector->base.kdev, dm, &amdgpu_dm_backlight_ops, &props); @@ -4141,6 +4142,7 @@ static int initialize_plane(struct amdgpu_display_manager *dm, static void register_backlight_device(struct amdgpu_display_manager *dm, + struct amdgpu_dm_connector *aconnector, struct dc_link *link) { if ((link->connector_signal & (SIGNAL_TYPE_EDP | SIGNAL_TYPE_LVDS)) && @@ -4151,7 +4153,7 @@ static void register_backlight_device(struct amdgpu_display_manager *dm, * is better then a black screen. */ if (!dm->backlight_dev[dm->num_of_edps]) - amdgpu_dm_register_backlight_device(dm); + amdgpu_dm_register_backlight_device(dm, aconnector); if (dm->backlight_dev[dm->num_of_edps]) { dm->backlight_link[dm->num_of_edps] = link; @@ -4337,7 +4339,7 @@ static int amdgpu_dm_initialize_drm_device(struct amdgpu_device *adev) if (ret) { amdgpu_dm_update_connector_after_detect(aconnector); - register_backlight_device(dm, link); + register_backlight_device(dm, aconnector, link); if (dm->num_of_edps) update_connector_ext_caps(aconnector);
The parent for the backlight device should be the drm-connector object, not the PCI device. Userspace relies on this to be able to detect which backlight class device to use on hybrid gfx devices where there may be multiple native (raw) backlight devices registered. Specifically gnome-settings-daemon expects the parent device to have an "enabled" sysfs attribute (as drm_connector devices do) and tests that this returns "enabled" when read. This aligns the parent of the backlight device with i915, nouveau, radeon. Note that drivers/gpu/drm/amd/amdgpu/atombios_encoders.c also already uses the drm_connector as parent, only amdgpu_dm.c used the PCI device as parent before this change. Note this is marked as a RFC because I don't have hw to test, so this has only been compile tested! If someone can test this on actual hw which hits the changed code path that would be great. Link: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/issues/730 Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)