| Message ID | 20240216153334.83372-1-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amd: Drop abm_level property | expand |
On 2024-02-16 10:33, Mario Limonciello wrote: > This vendor specific property has never been used by userspace > software and conflicts with the panel_power_savings sysfs file. > That is a compositor and user could fight over the same data. > > Fixes: f97e4303da16 ("drm/amd/display: add panel_power_savings sysfs entry to eDP connectors") > Suggested-by: Harry Wentland <Harry.Wentland@amd.com> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > -- > Cc: Hamza Mahfooz <Hamza.Mahfooz@amd.com> > Cc: Sun peng (Leo) Li <Sunpeng.Li@amd.com> Reviewed-by: Harry Wentland <harry.wentland@amd.com> Harry > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 8 -------- > drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 2 -- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 14 -------------- > 3 files changed, 24 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > index b8fbe97efe1d..3ecc7ef95172 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > @@ -1350,14 +1350,6 @@ int amdgpu_display_modeset_create_props(struct amdgpu_device *adev) > "dither", > amdgpu_dither_enum_list, sz); > > - if (adev->dc_enabled) { > - adev->mode_info.abm_level_property = > - drm_property_create_range(adev_to_drm(adev), 0, > - "abm level", 0, 4); > - if (!adev->mode_info.abm_level_property) > - return -ENOMEM; > - } > - > return 0; > } > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h > index 2e4911050cc5..1fe21a70ddd0 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h > @@ -324,8 +324,6 @@ struct amdgpu_mode_info { > struct drm_property *audio_property; > /* FMT dithering */ > struct drm_property *dither_property; > - /* Adaptive Backlight Modulation (power feature) */ > - struct drm_property *abm_level_property; > /* hardcoded DFP edid from BIOS */ > struct edid *bios_hardcoded_edid; > int bios_hardcoded_edid_size; > 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 b9ac3d2f8029..e3b32ffba85a 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -6388,9 +6388,6 @@ int amdgpu_dm_connector_atomic_set_property(struct drm_connector *connector, > } else if (property == adev->mode_info.underscan_property) { > dm_new_state->underscan_enable = val; > ret = 0; > - } else if (property == adev->mode_info.abm_level_property) { > - dm_new_state->abm_level = val ?: ABM_LEVEL_IMMEDIATE_DISABLE; > - ret = 0; > } > > return ret; > @@ -6433,10 +6430,6 @@ int amdgpu_dm_connector_atomic_get_property(struct drm_connector *connector, > } else if (property == adev->mode_info.underscan_property) { > *val = dm_state->underscan_enable; > ret = 0; > - } else if (property == adev->mode_info.abm_level_property) { > - *val = (dm_state->abm_level != ABM_LEVEL_IMMEDIATE_DISABLE) ? > - dm_state->abm_level : 0; > - ret = 0; > } > > return ret; > @@ -7652,13 +7645,6 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm, > aconnector->base.state->max_bpc = 16; > aconnector->base.state->max_requested_bpc = aconnector->base.state->max_bpc; > > - if (connector_type == DRM_MODE_CONNECTOR_eDP && > - (dc_is_dmcu_initialized(adev->dm.dc) || > - adev->dm.dc->ctx->dmub_srv) && amdgpu_dm_abm_level < 0) { > - drm_object_attach_property(&aconnector->base.base, > - adev->mode_info.abm_level_property, 0); > - } > - > if (connector_type == DRM_MODE_CONNECTOR_HDMIA) { > /* Content Type is currently only implemented for HDMI. */ > drm_connector_attach_content_type_property(&aconnector->base);
Am 16.02.24 um 16:33 schrieb Mario Limonciello: > This vendor specific property has never been used by userspace > software and conflicts with the panel_power_savings sysfs file. > That is a compositor and user could fight over the same data. > > Fixes: f97e4303da16 ("drm/amd/display: add panel_power_savings sysfs entry to eDP connectors") > Suggested-by: Harry Wentland <Harry.Wentland@amd.com> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> Acked-by: Christian König <christian.koenig@amd.com> > -- > Cc: Hamza Mahfooz <Hamza.Mahfooz@amd.com> > Cc: Sun peng (Leo) Li <Sunpeng.Li@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 8 -------- > drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 2 -- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 14 -------------- > 3 files changed, 24 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > index b8fbe97efe1d..3ecc7ef95172 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > @@ -1350,14 +1350,6 @@ int amdgpu_display_modeset_create_props(struct amdgpu_device *adev) > "dither", > amdgpu_dither_enum_list, sz); > > - if (adev->dc_enabled) { > - adev->mode_info.abm_level_property = > - drm_property_create_range(adev_to_drm(adev), 0, > - "abm level", 0, 4); > - if (!adev->mode_info.abm_level_property) > - return -ENOMEM; > - } > - > return 0; > } > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h > index 2e4911050cc5..1fe21a70ddd0 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h > @@ -324,8 +324,6 @@ struct amdgpu_mode_info { > struct drm_property *audio_property; > /* FMT dithering */ > struct drm_property *dither_property; > - /* Adaptive Backlight Modulation (power feature) */ > - struct drm_property *abm_level_property; > /* hardcoded DFP edid from BIOS */ > struct edid *bios_hardcoded_edid; > int bios_hardcoded_edid_size; > 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 b9ac3d2f8029..e3b32ffba85a 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -6388,9 +6388,6 @@ int amdgpu_dm_connector_atomic_set_property(struct drm_connector *connector, > } else if (property == adev->mode_info.underscan_property) { > dm_new_state->underscan_enable = val; > ret = 0; > - } else if (property == adev->mode_info.abm_level_property) { > - dm_new_state->abm_level = val ?: ABM_LEVEL_IMMEDIATE_DISABLE; > - ret = 0; > } > > return ret; > @@ -6433,10 +6430,6 @@ int amdgpu_dm_connector_atomic_get_property(struct drm_connector *connector, > } else if (property == adev->mode_info.underscan_property) { > *val = dm_state->underscan_enable; > ret = 0; > - } else if (property == adev->mode_info.abm_level_property) { > - *val = (dm_state->abm_level != ABM_LEVEL_IMMEDIATE_DISABLE) ? > - dm_state->abm_level : 0; > - ret = 0; > } > > return ret; > @@ -7652,13 +7645,6 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm, > aconnector->base.state->max_bpc = 16; > aconnector->base.state->max_requested_bpc = aconnector->base.state->max_bpc; > > - if (connector_type == DRM_MODE_CONNECTOR_eDP && > - (dc_is_dmcu_initialized(adev->dm.dc) || > - adev->dm.dc->ctx->dmub_srv) && amdgpu_dm_abm_level < 0) { > - drm_object_attach_property(&aconnector->base.base, > - adev->mode_info.abm_level_property, 0); > - } > - > if (connector_type == DRM_MODE_CONNECTOR_HDMIA) { > /* Content Type is currently only implemented for HDMI. */ > drm_connector_attach_content_type_property(&aconnector->base);
Like already mentioned in the power profiles daemon repository, I don't think this makes sense. This is a display setting, which compositors have interest in controlling, for example to: - disable it in a bright environment, because afaiu it reduces the maximum screen brightness - disable it when it shows color critical content - disable it while profiling the display - enable it when it shows content that's definitely not color critical (based on the content-type property of Wayland surfaces) - enable it as a first step before properly dimming the screen on idle If the primary concern here is that this hasn't been used by compositors and potential power savings aren't being realized, that could be solved by providing documentation about what the feature does in the kernel, and by sending a mail to wayland-devel describing why it should be used. If the goal is to implement it in power profiles daemon and not get conflicts, I think disabling the property by default and instead enable it + disable the sysfs file when a CAP for it is set would make more sense than making the listed features impossible.
On 3/6/2024 11:08, Xaver Hugl wrote: > Like already mentioned in the power profiles daemon repository, I don't think > this makes sense. This is a display setting, which compositors have interest > in controlling, for example to: > - disable it in a bright environment, because afaiu it reduces the maximum > screen brightness > - disable it when it shows color critical content > - disable it while profiling the display > - enable it when it shows content that's definitely not color critical (based > on the content-type property of Wayland surfaces) > - enable it as a first step before properly dimming the screen on idle > This specific topic is on the agenda to discuss at 2024 Display Next Hackfest. > If the primary concern here is that this hasn't been used by compositors and > potential power savings aren't being realized, that could be solved by > providing documentation about what the feature does in the kernel, and by > sending a mail to wayland-devel describing why it should be used. > > If the goal is to implement it in power profiles daemon and not get conflicts, I > think disabling the property by default and instead enable it + disable the > sysfs file when a CAP for it is set would make more sense than making the > listed features impossible. > > So the idea being if the compositor isn't using it we let power-profiles-daemon (or any other software) take control via sysfs and if the compositor does want to control it then it then it writes a DRM cap and we destroy the sysfs file?
Am Mi., 6. März 2024 um 18:19 Uhr schrieb Mario Limonciello <mario.limonciello@amd.com>: > So the idea being if the compositor isn't using it we let > power-profiles-daemon (or any other software) take control via sysfs and > if the compositor does want to control it then it then it writes a DRM > cap and we destroy the sysfs file? Yes. That way still only one party controls it at a given time, and we can get both good default behavior for display servers that don't care (like Xorg or compositors without color management support), and compositors that want to put in the effort can do more specific things with it.
On 3/6/2024 12:00, Xaver Hugl wrote: > Am Mi., 6. März 2024 um 18:19 Uhr schrieb Mario Limonciello > <mario.limonciello@amd.com>: >> So the idea being if the compositor isn't using it we let >> power-profiles-daemon (or any other software) take control via sysfs and >> if the compositor does want to control it then it then it writes a DRM >> cap and we destroy the sysfs file? > > Yes. That way still only one party controls it at a given time, and we > can get both good default behavior for display servers that don't care > (like Xorg or compositors without color management support), and > compositors that want to put in the effort can do more specific things > with it. I think that's a very good solution. Harry, Hamza, what do you guys think?
On 2024-03-06 13:02, Mario Limonciello wrote: > On 3/6/2024 12:00, Xaver Hugl wrote: >> Am Mi., 6. März 2024 um 18:19 Uhr schrieb Mario Limonciello >> <mario.limonciello@amd.com>: >>> So the idea being if the compositor isn't using it we let >>> power-profiles-daemon (or any other software) take control via sysfs and >>> if the compositor does want to control it then it then it writes a DRM >>> cap and we destroy the sysfs file? >> >> Yes. That way still only one party controls it at a given time, and we >> can get both good default behavior for display servers that don't care >> (like Xorg or compositors without color management support), and >> compositors that want to put in the effort can do more specific things >> with it. > > I think that's a very good solution. > > Harry, Hamza, what do you guys think? In theory I like it. But how will this look in practice? Is PPD or compositor on the scene first? Would it be possible to yank the sysfs away from PPD? DRM client caps are set by the client when the client interacts with DRM. At driver creation there is no client. How will the driver set things up? A user might switch between DRM clients (login manager, to desktop compositor, maybe to another VT with a different compositor). I know everything but the login manager to desktop compositor hand-off is today considered exotic, but what if someone starts building a use-case for it? I've done a bunch of gamescope or IGT work in a different VT while I've had Plasma running on its default VT. If someone can sketch this out, with answers to all the questions above and any other questions you can come up (be creative), I'd be happy to review. Alternatively we can discuss this at the hackfest and maybe arrive at a solution. Harry
On 3/6/2024 12:59, Harry Wentland wrote: > > > On 2024-03-06 13:02, Mario Limonciello wrote: >> On 3/6/2024 12:00, Xaver Hugl wrote: >>> Am Mi., 6. März 2024 um 18:19 Uhr schrieb Mario Limonciello >>> <mario.limonciello@amd.com>: >>>> So the idea being if the compositor isn't using it we let >>>> power-profiles-daemon (or any other software) take control via sysfs and >>>> if the compositor does want to control it then it then it writes a DRM >>>> cap and we destroy the sysfs file? >>> >>> Yes. That way still only one party controls it at a given time, and we >>> can get both good default behavior for display servers that don't care >>> (like Xorg or compositors without color management support), and >>> compositors that want to put in the effort can do more specific things >>> with it. >> >> I think that's a very good solution. >> >> Harry, Hamza, what do you guys think? > > In theory I like it. But how will this look in practice? Is PPD or compositor > on the scene first? Would it be possible to yank the sysfs away from PPD? > I double checked the existing PPD code to see how well this case maps out. * If the compositor shows up first then PPD just wouldn't find amdgpu support when it probed. * If PPD goes first and then the compositor I expect right now that PPD wouldn't explode. All the cases that would update it check that the file exists before trying to write it. I think the only risk is TOCTOU for that check relative to when it would write the file, but the worst case is you have an error in the journal log that it tried to write the file but it's not present (there is already error handling everywhere for this). What we can do to smooth it out even more is export a CHANGE uevent from amdgpu when this happens. PPD can react to the change event and totally stop the amdgpu plugin. > DRM client caps are set by the client when the client interacts with DRM. > At driver creation there is no client. How will the driver set things up? > You jog my mind for another idea. 1) We could reintroduce abm_level property. 2) If client connects with the cap set, we make any reads or writes to the sysfs return -EBUSY and we make amdgpu issue a CHANGE uevent. * Unchanged PPD will likely log a message to the journal that writes fail, but won't explode. * A changed PPD could react to the uevent and look for -EBUSY to not bother even logging a message to the journal. > A user might switch between DRM clients (login manager, to desktop compositor, > maybe to another VT with a different compositor). I know everything but the > login manager to desktop compositor hand-off is today considered exotic, but > what if someone starts building a use-case for it? I've done a bunch of gamescope > or IGT work in a different VT while I've had Plasma running on its default > VT. > If going off the cap of the client to decide whether or not to report -EBUSY to sysfs I think it handles the handoff cleanly. > If someone can sketch this out, with answers to all the questions above and > any other questions you can come up (be creative), I'd be happy to review. > Alternatively we can discuss this at the hackfest and maybe arrive at a > solution. > Yes; I think we should use this thread as a basis for that discussion.
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c index b8fbe97efe1d..3ecc7ef95172 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c @@ -1350,14 +1350,6 @@ int amdgpu_display_modeset_create_props(struct amdgpu_device *adev) "dither", amdgpu_dither_enum_list, sz); - if (adev->dc_enabled) { - adev->mode_info.abm_level_property = - drm_property_create_range(adev_to_drm(adev), 0, - "abm level", 0, 4); - if (!adev->mode_info.abm_level_property) - return -ENOMEM; - } - return 0; } diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h index 2e4911050cc5..1fe21a70ddd0 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h @@ -324,8 +324,6 @@ struct amdgpu_mode_info { struct drm_property *audio_property; /* FMT dithering */ struct drm_property *dither_property; - /* Adaptive Backlight Modulation (power feature) */ - struct drm_property *abm_level_property; /* hardcoded DFP edid from BIOS */ struct edid *bios_hardcoded_edid; int bios_hardcoded_edid_size; 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 b9ac3d2f8029..e3b32ffba85a 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -6388,9 +6388,6 @@ int amdgpu_dm_connector_atomic_set_property(struct drm_connector *connector, } else if (property == adev->mode_info.underscan_property) { dm_new_state->underscan_enable = val; ret = 0; - } else if (property == adev->mode_info.abm_level_property) { - dm_new_state->abm_level = val ?: ABM_LEVEL_IMMEDIATE_DISABLE; - ret = 0; } return ret; @@ -6433,10 +6430,6 @@ int amdgpu_dm_connector_atomic_get_property(struct drm_connector *connector, } else if (property == adev->mode_info.underscan_property) { *val = dm_state->underscan_enable; ret = 0; - } else if (property == adev->mode_info.abm_level_property) { - *val = (dm_state->abm_level != ABM_LEVEL_IMMEDIATE_DISABLE) ? - dm_state->abm_level : 0; - ret = 0; } return ret; @@ -7652,13 +7645,6 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm, aconnector->base.state->max_bpc = 16; aconnector->base.state->max_requested_bpc = aconnector->base.state->max_bpc; - if (connector_type == DRM_MODE_CONNECTOR_eDP && - (dc_is_dmcu_initialized(adev->dm.dc) || - adev->dm.dc->ctx->dmub_srv) && amdgpu_dm_abm_level < 0) { - drm_object_attach_property(&aconnector->base.base, - adev->mode_info.abm_level_property, 0); - } - if (connector_type == DRM_MODE_CONNECTOR_HDMIA) { /* Content Type is currently only implemented for HDMI. */ drm_connector_attach_content_type_property(&aconnector->base);
This vendor specific property has never been used by userspace software and conflicts with the panel_power_savings sysfs file. That is a compositor and user could fight over the same data. Fixes: f97e4303da16 ("drm/amd/display: add panel_power_savings sysfs entry to eDP connectors") Suggested-by: Harry Wentland <Harry.Wentland@amd.com> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> -- Cc: Hamza Mahfooz <Hamza.Mahfooz@amd.com> Cc: Sun peng (Leo) Li <Sunpeng.Li@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 8 -------- drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 2 -- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 14 -------------- 3 files changed, 24 deletions(-)