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

[Hans de Goede]

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 at redhat.com>

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