Hi, On 3/8/23 22:58, 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. > > Changes in v2: > Together with changing the parent, also move the registration to > drm_connector_funcs.late_register() this is necessary because the parent > device (which now is the drm_connector) must be registered before > the backlight class device is, otherwise the backlight class device ends > up without any parent set at all. > > This brings the backlight class device registration timing inline with > nouveau and i915 which also use drm_connector_funcs.late_register() > for this. > > Note this slightly changes backlight_device_register() error handling, > instead of not increasing dm->num_of_edps and re-using the current > bl_idx for a potential other backlight device, dm->backlight_dev[bl_idx] > is now simply left NULL on failure. This is ok because all code > looking at dm->backlight_dev[i] also checks it is not NULL. > > Link: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/issues/730 > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> Self nack, the amdgpu_dm_register_backlight_device() call in amdgpu_dm_connector_late_register() leads to the driver now trying to register a backlight class device for each connector (I hit this on my AMD APU based desktop machine). I'll prepare a non RFC version with this fixed. Regards, Hans > --- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 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 038bf897cc28..051074d5812f 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -4162,7 +4162,7 @@ amdgpu_dm_register_backlight_device(struct amdgpu_dm_connector *aconnector) > drm->primary->index + aconnector->bl_idx); > > dm->backlight_dev[aconnector->bl_idx] = > - backlight_device_register(bl_name, drm->dev, dm, > + backlight_device_register(bl_name, aconnector->base.kdev, dm, > &amdgpu_dm_backlight_ops, &props); > > if (IS_ERR(dm->backlight_dev[aconnector->bl_idx])) { > @@ -4232,13 +4232,6 @@ static void setup_backlight_device(struct amdgpu_display_manager *dm, > > amdgpu_dm_update_backlight_caps(dm, bl_idx); > dm->brightness[bl_idx] = AMDGPU_MAX_BL_LEVEL; > - > - amdgpu_dm_register_backlight_device(aconnector); > - if (!dm->backlight_dev[bl_idx]) { > - aconnector->bl_idx = -1; > - return; > - } > - > dm->backlight_link[bl_idx] = link; > dm->num_of_edps++; > > @@ -6297,6 +6290,8 @@ amdgpu_dm_connector_late_register(struct drm_connector *connector) > to_amdgpu_dm_connector(connector); > int r; > > + amdgpu_dm_register_backlight_device(amdgpu_dm_connector); > + > if ((connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) || > (connector->connector_type == DRM_MODE_CONNECTOR_eDP)) { > amdgpu_dm_connector->dm_dp_aux.aux.dev = connector->kdev;
