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@xxxxxxxxxx> 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);
