| Message ID | 20230308215831.782266-7-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amd/display: Pass proper parent for DM backlight device v2 | expand |
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@redhat.com> 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;
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;
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@redhat.com> --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)