[RFC v2 6/6] drm/amd/display: Pass proper parent for DM backlight device registration v2

[Hans de Goede]

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 at 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;