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 <hdego...@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;
