| Message ID | 20240606020404.210989-3-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for 'power saving policy' property | expand |
On 2024-06-05 22:04, Mario Limonciello wrote: > When the `power_saving_policy` property is set to bit mask > "Require color accuracy" ABM should be disabled immediately and > any requests by sysfs to update will return an -EBUSY error. > > When the `power_saving_policy` property is set to bit mask > "Require low latency" PSR should be disabled. > > When the property is restored to an empty bit mask the previous > value of ABM and pSR will be restored. > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> Reviewed-by: Leo Li <sunpeng.li@amd.com> Thanks! > --- > v2->v3: > * Use `disallow_edp_enter_psr` instead > * Drop case in dm_update_crtc_state() > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 4 ++ > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 50 +++++++++++++++++-- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 2 + > 3 files changed, 51 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > index 3ecc7ef95172..cfb5220cf182 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > @@ -1350,6 +1350,10 @@ int amdgpu_display_modeset_create_props(struct amdgpu_device *adev) > "dither", > amdgpu_dither_enum_list, sz); > > + if (adev->dc_enabled) > + drm_mode_create_power_saving_policy_property(adev_to_drm(adev), > + DRM_MODE_POWER_SAVING_POLICY_ALL); > + > return 0; > } > > 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 f1d67c6f4b98..5fd7210b2479 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -6436,6 +6436,13 @@ 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 == dev->mode_config.power_saving_policy) { > + dm_new_state->abm_forbidden = val & DRM_MODE_REQUIRE_COLOR_ACCURACY; > + dm_new_state->abm_level = (dm_new_state->abm_forbidden || !amdgpu_dm_abm_level) ? > + ABM_LEVEL_IMMEDIATE_DISABLE : > + amdgpu_dm_abm_level; > + dm_new_state->psr_forbidden = val & DRM_MODE_REQUIRE_LOW_LATENCY; > + ret = 0; > } > > return ret; > @@ -6478,6 +6485,13 @@ 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 == dev->mode_config.power_saving_policy) { > + *val = 0; > + if (dm_state->psr_forbidden) > + *val |= DRM_MODE_REQUIRE_LOW_LATENCY; > + if (dm_state->abm_forbidden) > + *val |= DRM_MODE_REQUIRE_COLOR_ACCURACY; > + ret = 0; > } > > return ret; > @@ -6504,9 +6518,12 @@ static ssize_t panel_power_savings_show(struct device *device, > u8 val; > > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > - val = to_dm_connector_state(connector->state)->abm_level == > - ABM_LEVEL_IMMEDIATE_DISABLE ? 0 : > - to_dm_connector_state(connector->state)->abm_level; > + if (to_dm_connector_state(connector->state)->abm_forbidden) > + val = 0; > + else > + val = to_dm_connector_state(connector->state)->abm_level == > + ABM_LEVEL_IMMEDIATE_DISABLE ? 0 : > + to_dm_connector_state(connector->state)->abm_level; > drm_modeset_unlock(&dev->mode_config.connection_mutex); > > return sysfs_emit(buf, "%u\n", val); > @@ -6530,10 +6547,16 @@ static ssize_t panel_power_savings_store(struct device *device, > return -EINVAL; > > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > - to_dm_connector_state(connector->state)->abm_level = val ?: > - ABM_LEVEL_IMMEDIATE_DISABLE; > + if (to_dm_connector_state(connector->state)->abm_forbidden) > + ret = -EBUSY; > + else > + to_dm_connector_state(connector->state)->abm_level = val ?: > + ABM_LEVEL_IMMEDIATE_DISABLE; > drm_modeset_unlock(&dev->mode_config.connection_mutex); > > + if (ret) > + return ret; > + > drm_kms_helper_hotplug_event(dev); > > return count; > @@ -7704,6 +7727,13 @@ 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)) { > + drm_object_attach_property(&aconnector->base.base, > + dm->ddev->mode_config.power_saving_policy, 0); > + } > + > if (connector_type == DRM_MODE_CONNECTOR_HDMIA) { > /* Content Type is currently only implemented for HDMI. */ > drm_connector_attach_content_type_property(&aconnector->base); > @@ -9307,6 +9337,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state) > for_each_oldnew_connector_in_state(state, connector, old_con_state, new_con_state, i) { > struct dm_connector_state *dm_new_con_state = to_dm_connector_state(new_con_state); > struct dm_connector_state *dm_old_con_state = to_dm_connector_state(old_con_state); > + struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector); > struct amdgpu_crtc *acrtc = to_amdgpu_crtc(dm_new_con_state->base.crtc); > struct dc_surface_update *dummy_updates; > struct dc_stream_update stream_update; > @@ -9360,6 +9391,15 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state) > stream_update.hdr_static_metadata = &hdr_packet; > } > > + aconnector->disallow_edp_enter_psr = dm_new_con_state->psr_forbidden; > + > + /* immediately disable PSR if disallowed */ > + if (aconnector->disallow_edp_enter_psr) { > + mutex_lock(&dm->dc_lock); > + amdgpu_dm_psr_disable(dm_new_crtc_state->stream); > + mutex_unlock(&dm->dc_lock); > + } > + > status = dc_stream_get_status(dm_new_crtc_state->stream); > > if (WARN_ON(!status)) > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h > index 09519b7abf67..b246e82f5b0d 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h > @@ -873,6 +873,8 @@ struct dm_connector_state { > bool underscan_enable; > bool freesync_capable; > bool update_hdcp; > + bool abm_forbidden; > + bool psr_forbidden; > uint8_t abm_level; > int vcpi_slots; > uint64_t pbn;
On 6/18/2024 17:36, Leo Li wrote: > > > On 2024-06-05 22:04, Mario Limonciello wrote: >> When the `power_saving_policy` property is set to bit mask >> "Require color accuracy" ABM should be disabled immediately and >> any requests by sysfs to update will return an -EBUSY error. >> >> When the `power_saving_policy` property is set to bit mask >> "Require low latency" PSR should be disabled. >> >> When the property is restored to an empty bit mask the previous >> value of ABM and pSR will be restored. >> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > Reviewed-by: Leo Li <sunpeng.li@amd.com> Thanks! I don't have permissions, so can you (or someone else) please apply to drm-misc-next for me? After it's merged I'll rebase and work on the feedback for the new IGT tests. > > Thanks! > >> --- >> v2->v3: >> * Use `disallow_edp_enter_psr` instead >> * Drop case in dm_update_crtc_state() >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 4 ++ >> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 50 +++++++++++++++++-- >> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 2 + >> 3 files changed, 51 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c >> index 3ecc7ef95172..cfb5220cf182 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c >> @@ -1350,6 +1350,10 @@ int amdgpu_display_modeset_create_props(struct >> amdgpu_device *adev) >> "dither", >> amdgpu_dither_enum_list, sz); >> + if (adev->dc_enabled) >> + drm_mode_create_power_saving_policy_property(adev_to_drm(adev), >> + DRM_MODE_POWER_SAVING_POLICY_ALL); >> + >> return 0; >> } >> 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 f1d67c6f4b98..5fd7210b2479 100644 >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> @@ -6436,6 +6436,13 @@ 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 == dev->mode_config.power_saving_policy) { >> + dm_new_state->abm_forbidden = val & >> DRM_MODE_REQUIRE_COLOR_ACCURACY; >> + dm_new_state->abm_level = (dm_new_state->abm_forbidden || >> !amdgpu_dm_abm_level) ? >> + ABM_LEVEL_IMMEDIATE_DISABLE : >> + amdgpu_dm_abm_level; >> + dm_new_state->psr_forbidden = val & >> DRM_MODE_REQUIRE_LOW_LATENCY; >> + ret = 0; >> } >> return ret; >> @@ -6478,6 +6485,13 @@ 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 == dev->mode_config.power_saving_policy) { >> + *val = 0; >> + if (dm_state->psr_forbidden) >> + *val |= DRM_MODE_REQUIRE_LOW_LATENCY; >> + if (dm_state->abm_forbidden) >> + *val |= DRM_MODE_REQUIRE_COLOR_ACCURACY; >> + ret = 0; >> } >> return ret; >> @@ -6504,9 +6518,12 @@ static ssize_t panel_power_savings_show(struct >> device *device, >> u8 val; >> drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); >> - val = to_dm_connector_state(connector->state)->abm_level == >> - ABM_LEVEL_IMMEDIATE_DISABLE ? 0 : >> - to_dm_connector_state(connector->state)->abm_level; >> + if (to_dm_connector_state(connector->state)->abm_forbidden) >> + val = 0; >> + else >> + val = to_dm_connector_state(connector->state)->abm_level == >> + ABM_LEVEL_IMMEDIATE_DISABLE ? 0 : >> + to_dm_connector_state(connector->state)->abm_level; >> drm_modeset_unlock(&dev->mode_config.connection_mutex); >> return sysfs_emit(buf, "%u\n", val); >> @@ -6530,10 +6547,16 @@ static ssize_t >> panel_power_savings_store(struct device *device, >> return -EINVAL; >> drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); >> - to_dm_connector_state(connector->state)->abm_level = val ?: >> - ABM_LEVEL_IMMEDIATE_DISABLE; >> + if (to_dm_connector_state(connector->state)->abm_forbidden) >> + ret = -EBUSY; >> + else >> + to_dm_connector_state(connector->state)->abm_level = val ?: >> + ABM_LEVEL_IMMEDIATE_DISABLE; >> drm_modeset_unlock(&dev->mode_config.connection_mutex); >> + if (ret) >> + return ret; >> + >> drm_kms_helper_hotplug_event(dev); >> return count; >> @@ -7704,6 +7727,13 @@ 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)) { >> + drm_object_attach_property(&aconnector->base.base, >> + dm->ddev->mode_config.power_saving_policy, 0); >> + } >> + >> if (connector_type == DRM_MODE_CONNECTOR_HDMIA) { >> /* Content Type is currently only implemented for HDMI. */ >> drm_connector_attach_content_type_property(&aconnector->base); >> @@ -9307,6 +9337,7 @@ static void amdgpu_dm_atomic_commit_tail(struct >> drm_atomic_state *state) >> for_each_oldnew_connector_in_state(state, connector, >> old_con_state, new_con_state, i) { >> struct dm_connector_state *dm_new_con_state = >> to_dm_connector_state(new_con_state); >> struct dm_connector_state *dm_old_con_state = >> to_dm_connector_state(old_con_state); >> + struct amdgpu_dm_connector *aconnector = >> to_amdgpu_dm_connector(connector); >> struct amdgpu_crtc *acrtc = >> to_amdgpu_crtc(dm_new_con_state->base.crtc); >> struct dc_surface_update *dummy_updates; >> struct dc_stream_update stream_update; >> @@ -9360,6 +9391,15 @@ static void amdgpu_dm_atomic_commit_tail(struct >> drm_atomic_state *state) >> stream_update.hdr_static_metadata = &hdr_packet; >> } >> + aconnector->disallow_edp_enter_psr = >> dm_new_con_state->psr_forbidden; >> + >> + /* immediately disable PSR if disallowed */ >> + if (aconnector->disallow_edp_enter_psr) { >> + mutex_lock(&dm->dc_lock); >> + amdgpu_dm_psr_disable(dm_new_crtc_state->stream); >> + mutex_unlock(&dm->dc_lock); >> + } >> + >> status = dc_stream_get_status(dm_new_crtc_state->stream); >> if (WARN_ON(!status)) >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h >> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h >> index 09519b7abf67..b246e82f5b0d 100644 >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h >> @@ -873,6 +873,8 @@ struct dm_connector_state { >> bool underscan_enable; >> bool freesync_capable; >> bool update_hdcp; >> + bool abm_forbidden; >> + bool psr_forbidden; >> uint8_t abm_level; >> int vcpi_slots; >> uint64_t pbn;
Am Mi., 19. Juni 2024 um 06:08 Uhr schrieb Mario Limonciello <mario.limonciello@amd.com> > Thanks! I don't have permissions, so can you (or someone else) please > apply to drm-misc-next for me? > > After it's merged I'll rebase and work on the feedback for the new IGT > tests. Merging can only happen once a real world userspace application has implemented support for it. I'll try to do that sometime next week in KWin
Am Do., 20. Juni 2024 um 22:22 Uhr schrieb Xaver Hugl <xaver.hugl@gmail.com>: > Merging can only happen once a real world userspace application has > implemented support for it. I'll try to do that sometime next week in > KWin Here's the promised implementation: https://invent.kde.org/plasma/kwin/-/merge_requests/6028 In testing with the patches on top of kernel 6.9.6, setting the property to `Require color accuracy` makes the sysfs file correctly report "Device or resource busy" when trying to change the power saving level, but setting the property to zero doesn't really work. Once KWin sets the property to zero, changing the power saving level "works" but the screen blanks for a moment (might just be a single frame) and reading from the file returns zero again, with the visuals and backlight level unchanged as well.
On 7/1/2024 13:47, Xaver Hugl wrote: > Am Do., 20. Juni 2024 um 22:22 Uhr schrieb Xaver Hugl <xaver.hugl@gmail.com>: >> Merging can only happen once a real world userspace application has >> implemented support for it. I'll try to do that sometime next week in >> KWin > > Here's the promised implementation: > https://invent.kde.org/plasma/kwin/-/merge_requests/6028 Thanks! > > In testing with the patches on top of kernel 6.9.6, setting the > property to `Require color accuracy` makes the sysfs file correctly > report "Device or resource busy" when trying to change the power > saving level, but setting the property to zero doesn't really work. > Once KWin sets the property to zero, changing the power saving level > "works" but the screen blanks for a moment (might just be a single > frame) and reading from the file returns zero again, with the visuals > and backlight level unchanged as well. Hmm I'm a bit surprised the IGT tests I did didn't catch this. Are you working on a system with two GPUs by chance (like a Framework 16)? If so; can you try the "other GPU"? As it seems your PR to span 3 projects and I've never built KDE before can you spit out some artifacts somewhere that I can have a play with to reproduce your result and find the kernel issue? Arch pkgs would be preferable for me, but some RPMs or DEBs are fine too.
Am Mo., 1. Juli 2024 um 21:02 Uhr schrieb Mario Limonciello <mario.limonciello@amd.com>: > Hmm I'm a bit surprised the IGT tests I did didn't catch this. > > Are you working on a system with two GPUs by chance (like a Framework > 16)? If so; can you try the "other GPU"? No, I tested on a Framework 13. > As it seems your PR to span 3 projects and I've never built KDE before > can you spit out some artifacts somewhere that I can have a play with to > reproduce your result and find the kernel issue? Arch pkgs would be > preferable for me, but some RPMs or DEBs are fine too. Here you go: https://nx44777.your-storageshare.de/s/2j4Jy5anDwwzCtF and https://nx44777.your-storageshare.de/s/2rxJ4Tp2L8gdc8Y You can set the drm property to "Require color accuracy" with "kscreen-doctor output.1.allowColorPowerSaving.disallow" and to zero again with "kscreen-doctor output.1.allowColorPowerSaving.allow"
On 7/1/2024 17:34, Xaver Hugl wrote: > Am Mo., 1. Juli 2024 um 21:02 Uhr schrieb Mario Limonciello > <mario.limonciello@amd.com>: >> Hmm I'm a bit surprised the IGT tests I did didn't catch this. >> >> Are you working on a system with two GPUs by chance (like a Framework >> 16)? If so; can you try the "other GPU"? > > No, I tested on a Framework 13. > >> As it seems your PR to span 3 projects and I've never built KDE before >> can you spit out some artifacts somewhere that I can have a play with to >> reproduce your result and find the kernel issue? Arch pkgs would be >> preferable for me, but some RPMs or DEBs are fine too. > > Here you go: https://nx44777.your-storageshare.de/s/2j4Jy5anDwwzCtF > and https://nx44777.your-storageshare.de/s/2rxJ4Tp2L8gdc8Y > You can set the drm property to "Require color accuracy" with > "kscreen-doctor output.1.allowColorPowerSaving.disallow" and to zero > again with "kscreen-doctor output.1.allowColorPowerSaving.allow" Thanks, I snagged the artifacts. Of course when I tried to reproduce I hit a new issue where my series is causing problems with DSC on my panel. X crashes and consequently SDDM doesn't come up so I don't even get to the point I can toggle the knob. Mostly posting trace below in case root cause jumps out at anyone what actually went wrong. It reproduces with amdgpu.abmlevel=0 on both 6.9.7 and 6.10-rc6, but drop my patches and it's gone. [drm] DSC precompute is not needed. ------------[ cut here ]------------ WARNING: CPU: 7 PID: 321 at drivers/gpu/drm/amd/amdgpu/../display/dc/dsc/dcn20/dcn20_dsc.c:272 dsc2_disable+0xec/0x170 [amdgpu] Modules linked in: amdgpu(+) amdxcp i2c_algo_bit drm_ttm_helper ttm drm_exec gpu_sched drm_suballoc_helper drm_buddy drm_display_helper drm_kms_helper drm drm_panel_orientation_quirks nvme serio_raw nvme_core video CPU: 7 PID: 321 Comm: (udev-worker) Not tainted 6.9.7-00002-gde664ea69218 #241 Hardware name: Framework Laptop 13 (AMD Ryzen 7040Series)/FRANMDCP05, BIOS 03.05 03/29/2024 RIP: 0010:dsc2_disable+0xec/0x170 [amdgpu] Code: 4c 24 0c 44 8b 43 10 48 8b 40 10 48 8b 30 48 85 f6 74 04 48 8b 76 08 48 c7 c1 98 71 49 c1 ba 08 00 00 00 31 ff e8 84 53 7d ff <0f> 0b 48 8b 53 20 48 8b 43 28 45 31 c9 48 8b 7b 08 0f b6 8a b4 00 RSP: 0018:ffffb96f80a46e80 EFLAGS: 00010246 RAX: 0000000000000000 RBX: ffff95a144c19000 RCX: ffffffffc1497198 RDX: 0000000000000008 RSI: ffff95a141d8b0c8 RDI: 0000000000000000 RBP: ffff95a1552002d8 R08: 0000000000000000 R09: 0000000000000000 R10: 000000000000014b R11: 0000000000000000 R12: ffff95a14f41d680 R13: 0000000000000001 R14: ffff95a151338000 R15: ffff95a151338000 FS: 00007f3e17557880(0000) GS:ffff95a4ae780000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 000055ab019b1288 CR3: 000000010bb2c000 CR4: 0000000000f50ef0 PKRU: 55555554 Call Trace: <TASK> ? dsc2_disable+0xec/0x170 [amdgpu] ? __warn.cold+0x8e/0xe8 ? dsc2_disable+0xec/0x170 [amdgpu] ? report_bug+0xef/0x1d0 ? handle_bug+0x3c/0x80 ? exc_invalid_op+0x13/0x60 ? asm_exc_invalid_op+0x16/0x20 ? dsc2_disable+0xec/0x170 [amdgpu] link_set_dsc_on_stream+0x3f2/0x470 [amdgpu] ? srso_alias_return_thunk+0x5/0xfbef5 ? dm_helpers_dp_write_dsc_enable+0x286/0x720 [amdgpu] ? srso_alias_return_thunk+0x5/0xfbef5 ? disable_link+0x1e1/0x210 [amdgpu] link_set_dsc_enable+0x7f/0x90 [amdgpu] link_set_dpms_off+0x1ad/0x990 [amdgpu] ? srso_alias_return_thunk+0x5/0xfbef5 ? srso_alias_return_thunk+0x5/0xfbef5 ? dmub_srv_get_fw_boot_status+0x43/0x60 [amdgpu] ? srso_alias_return_thunk+0x5/0xfbef5 ? srso_alias_return_thunk+0x5/0xfbef5 ? dm_read_reg_func+0x57/0xc0 [amdgpu] ? srso_alias_return_thunk+0x5/0xfbef5 dc_commit_state_no_check+0x878/0x1990 [amdgpu] dc_commit_streams+0x29d/0x580 [amdgpu] ? srso_alias_return_thunk+0x5/0xfbef5 amdgpu_dm_atomic_commit_tail+0x686/0x44c0 [amdgpu] ? srso_alias_return_thunk+0x5/0xfbef5 ? dcn314_validate_bandwidth+0x181/0x3f0 [amdgpu] ? srso_alias_return_thunk+0x5/0xfbef5 ? srso_alias_return_thunk+0x5/0xfbef5 ? dma_resv_get_fences+0xb7/0x310 ? srso_alias_return_thunk+0x5/0xfbef5 ? srso_alias_return_thunk+0x5/0xfbef5 ? amdgpu_dm_plane_helper_prepare_fb+0x1a0/0x2b0 [amdgpu] commit_tail+0xbf/0x170 [drm_kms_helper] drm_atomic_helper_commit+0x116/0x140 [drm_kms_helper] drm_atomic_commit+0x99/0xd0 [drm] ? __pfx___drm_printfn_info+0x10/0x10 [drm] drm_client_modeset_commit_atomic+0x1e2/0x220 [drm] drm_client_modeset_commit_locked+0x56/0x160 [drm] ? srso_alias_return_thunk+0x5/0xfbef5 drm_client_modeset_commit+0x21/0x40 [drm] __drm_fb_helper_restore_fbdev_mode_unlocked+0xae/0xf0 [drm_kms_helper] drm_fb_helper_set_par+0x2c/0x40 [drm_kms_helper] fbcon_init+0x2d6/0x670 visual_init+0xea/0x180 do_bind_con_driver.isra.0+0x241/0x390 ? fbcon_startup+0x1fe/0x2e0 do_take_over_console+0x177/0x1f0 ? kmalloc_trace+0x246/0x2f0 do_fbcon_takeover+0x72/0x130 fbcon_fb_registered+0x33/0x80 register_framebuffer+0x192/0x2b0 __drm_fb_helper_initial_config_and_unlock+0x389/0x450 [drm_kms_helper] ? srso_alias_return_thunk+0x5/0xfbef5 ? amdgpu_driver_open_kms+0xcd/0x260 [amdgpu] drm_fbdev_generic_client_hotplug+0x67/0xa0 [drm_kms_helper] ? srso_alias_return_thunk+0x5/0xfbef5 drm_client_register+0x72/0xb0 [drm] amdgpu_pci_probe+0x441/0x4c0 [amdgpu] ? srso_alias_return_thunk+0x5/0xfbef5 local_pci_probe+0x3e/0x80 pci_device_probe+0xbd/0x2a0 really_probe+0xeb/0x350 ? pm_runtime_barrier+0x50/0x90 __driver_probe_device+0x87/0x130 driver_probe_device+0x1f/0xc0 __driver_attach+0xcb/0x1f0 ? __pfx___driver_attach+0x10/0x10 bus_for_each_dev+0x77/0xd0 bus_add_driver+0x10e/0x200 driver_register+0x6e/0xc0 ? __pfx_amdgpu_init+0x10/0x10 [amdgpu] do_one_initcall+0x3f/0x2f0 ? do_init_module+0x22/0x230 do_init_module+0x60/0x230 init_module_from_file+0x86/0xc0 idempotent_init_module+0x10b/0x2a0 __x64_sys_finit_module+0x5a/0xb0 do_syscall_64+0x80/0x180 ? srso_alias_return_thunk+0x5/0xfbef5 ? vfs_fstatat+0x90/0xb0 ? srso_alias_return_thunk+0x5/0xfbef5 ? __do_sys_newfstatat+0x25/0x60 ? do_syscall_64+0x8d/0x180 ? srso_alias_return_thunk+0x5/0xfbef5 ? vfs_read+0x1f9/0x330 ? srso_alias_return_thunk+0x5/0xfbef5 ? syscall_exit_to_user_mode+0x71/0x210 ? srso_alias_return_thunk+0x5/0xfbef5 ? do_syscall_64+0x8d/0x180 ? srso_alias_return_thunk+0x5/0xfbef5 ? syscall_exit_to_user_mode+0x71/0x210 ? srso_alias_return_thunk+0x5/0xfbef5 ? do_syscall_64+0x8d/0x180 ? srso_alias_return_thunk+0x5/0xfbef5 ? do_sys_openat2+0x82/0xc0 ? do_syscall_64+0x8d/0x180 ? srso_alias_return_thunk+0x5/0xfbef5 ? syscall_exit_to_user_mode+0x71/0x210 ? srso_alias_return_thunk+0x5/0xfbef5 ? do_syscall_64+0x8d/0x180 ? srso_alias_return_thunk+0x5/0xfbef5 entry_SYSCALL_64_after_hwframe+0x76/0x7e RIP: 0033:0x7f3e17db6f9d Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 5b 6d 0c 00 f7 d8 64 89 01 48 RSP: 002b:00007ffd4f93b6c8 EFLAGS: 00000246 ORIG_RAX: 0000000000000139 RAX: ffffffffffffffda RBX: 000055ab019a43d0 RCX: 00007f3e17db6f9d RDX: 0000000000000000 RSI: 000055ab019a85b0 RDI: 0000000000000021 RBP: 000055ab019a85b0 R08: 0000000000000040 R09: 000055ab0199158f R10: fffffffffffffec0 R11: 0000000000000246 R12: 0000000000020000 R13: 000055ab019a6300 R14: 000055ab019aae70 R15: 000055ab019aaa90 </TASK> ---[ end trace 0000000000000000 ]---
On Mon, 01 Jul 2024, Xaver Hugl <xaver.hugl@gmail.com> wrote: > Am Do., 20. Juni 2024 um 22:22 Uhr schrieb Xaver Hugl <xaver.hugl@gmail.com>: >> Merging can only happen once a real world userspace application has >> implemented support for it. I'll try to do that sometime next week in >> KWin > > Here's the promised implementation: > https://invent.kde.org/plasma/kwin/-/merge_requests/6028 The requirement is that the userspace patches must be reviewed and ready for merging into a suitable and canonical upstream project. Are they? BR, Jani. > > In testing with the patches on top of kernel 6.9.6, setting the > property to `Require color accuracy` makes the sysfs file correctly > report "Device or resource busy" when trying to change the power > saving level, but setting the property to zero doesn't really work. > Once KWin sets the property to zero, changing the power saving level > "works" but the screen blanks for a moment (might just be a single > frame) and reading from the file returns zero again, with the visuals > and backlight level unchanged as well.
Hi Am 01.08.24 um 14:33 schrieb Jani Nikula: > On Mon, 01 Jul 2024, Xaver Hugl <xaver.hugl@gmail.com> wrote: >> Am Do., 20. Juni 2024 um 22:22 Uhr schrieb Xaver Hugl <xaver.hugl@gmail.com>: >>> Merging can only happen once a real world userspace application has >>> implemented support for it. I'll try to do that sometime next week in >>> KWin >> Here's the promised implementation: >> https://invent.kde.org/plasma/kwin/-/merge_requests/6028 > The requirement is that the userspace patches must be reviewed and ready > for merging into a suitable and canonical upstream project. > > Are they? I already saw this series in today's PR for drm-misc-next. :/ Best regards Thomas > > > BR, > Jani. > > >> In testing with the patches on top of kernel 6.9.6, setting the >> property to `Require color accuracy` makes the sysfs file correctly >> report "Device or resource busy" when trying to change the power >> saving level, but setting the property to zero doesn't really work. >> Once KWin sets the property to zero, changing the power saving level >> "works" but the screen blanks for a moment (might just be a single >> frame) and reading from the file returns zero again, with the visuals >> and backlight level unchanged as well.
On Thu, 01 Aug 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: > Hi > > Am 01.08.24 um 14:33 schrieb Jani Nikula: >> On Mon, 01 Jul 2024, Xaver Hugl <xaver.hugl@gmail.com> wrote: >>> Am Do., 20. Juni 2024 um 22:22 Uhr schrieb Xaver Hugl <xaver.hugl@gmail.com>: >>>> Merging can only happen once a real world userspace application has >>>> implemented support for it. I'll try to do that sometime next week in >>>> KWin >>> Here's the promised implementation: >>> https://invent.kde.org/plasma/kwin/-/merge_requests/6028 >> The requirement is that the userspace patches must be reviewed and ready >> for merging into a suitable and canonical upstream project. >> >> Are they? > > I already saw this series in today's PR for drm-misc-next. :/ Exactly what triggered the question! BR, Jani. > > Best regards > Thomas > >> >> >> BR, >> Jani. >> >> >>> In testing with the patches on top of kernel 6.9.6, setting the >>> property to `Require color accuracy` makes the sysfs file correctly >>> report "Device or resource busy" when trying to change the power >>> saving level, but setting the property to zero doesn't really work. >>> Once KWin sets the property to zero, changing the power saving level >>> "works" but the screen blanks for a moment (might just be a single >>> frame) and reading from the file returns zero again, with the visuals >>> and backlight level unchanged as well.
Am Do., 1. Aug. 2024 um 14:34 Uhr schrieb Jani Nikula <jani.nikula@linux.intel.com>: > > On Mon, 01 Jul 2024, Xaver Hugl <xaver.hugl@gmail.com> wrote: > > Am Do., 20. Juni 2024 um 22:22 Uhr schrieb Xaver Hugl <xaver.hugl@gmail.com>: > >> Merging can only happen once a real world userspace application has > >> implemented support for it. I'll try to do that sometime next week in > >> KWin > > > > Here's the promised implementation: > > https://invent.kde.org/plasma/kwin/-/merge_requests/6028 > > The requirement is that the userspace patches must be reviewed and ready > for merging into a suitable and canonical upstream project. > > Are they? I've talked about the property with other KWin developers before, but there's indeed no official review for the MR yet. As some new discussions about alternative approaches have started as well, maybe it should be reverted until we're more certain about how to proceed? > BR, > Jani. > > > > > > In testing with the patches on top of kernel 6.9.6, setting the > > property to `Require color accuracy` makes the sysfs file correctly > > report "Device or resource busy" when trying to change the power > > saving level, but setting the property to zero doesn't really work. > > Once KWin sets the property to zero, changing the power saving level > > "works" but the screen blanks for a moment (might just be a single > > frame) and reading from the file returns zero again, with the visuals > > and backlight level unchanged as well. > > -- > Jani Nikula, Intel
I'm very unhappy about how this has played out. We have a new sysfs property that controls a feature of the display path that has been set to a default(!) which changes the color behavior! This broke color management for everyone who is on a device which supports this feature. What should have been done is the following: * add the KMS property and have it default to "sysfs cannot override this" * add the sysfs property after the KMS property has been introduced so it stays disabled by default * add support for the new property in compositors and let them enable this feature only when they allow colors to randomly get broken Every other path results in broken colors at least temporarily and is breaking user space! The sysfs property *must* be reverted because it breaks user space. The KMS property *must* be reverted because it didn't meet the merging criteria. Another note: The only way to make sure that this isn't breaking user space is if user space tells the kernel that this is okay. This means that the sysfs property can only be used if the compositor grows support for the new KMS property and at that point, why do we have the sysfs property? On Fri, Aug 2, 2024 at 2:49 PM Xaver Hugl <xaver.hugl@gmail.com> wrote: > > Am Do., 1. Aug. 2024 um 14:34 Uhr schrieb Jani Nikula > <jani.nikula@linux.intel.com>: > > > > On Mon, 01 Jul 2024, Xaver Hugl <xaver.hugl@gmail.com> wrote: > > > Am Do., 20. Juni 2024 um 22:22 Uhr schrieb Xaver Hugl <xaver.hugl@gmail.com>: > > >> Merging can only happen once a real world userspace application has > > >> implemented support for it. I'll try to do that sometime next week in > > >> KWin > > > > > > Here's the promised implementation: > > > https://invent.kde.org/plasma/kwin/-/merge_requests/6028 > > > > The requirement is that the userspace patches must be reviewed and ready > > for merging into a suitable and canonical upstream project. > > > > Are they? > > I've talked about the property with other KWin developers before, but > there's indeed no official review for the MR yet. > As some new discussions about alternative approaches have started as > well, maybe it should be reverted until we're more certain about how > to proceed? > > > BR, > > Jani. > > > > > > > > > > In testing with the patches on top of kernel 6.9.6, setting the > > > property to `Require color accuracy` makes the sysfs file correctly > > > report "Device or resource busy" when trying to change the power > > > saving level, but setting the property to zero doesn't really work. > > > Once KWin sets the property to zero, changing the power saving level > > > "works" but the screen blanks for a moment (might just be a single > > > frame) and reading from the file returns zero again, with the visuals > > > and backlight level unchanged as well. > > > > -- > > Jani Nikula, Intel >
On 2024-08-02 09:28, Sebastian Wick wrote: > I'm very unhappy about how this has played out. > > We have a new sysfs property that controls a feature of the display > path that has been set to a default(!) which changes the color > behavior! This broke color management for everyone who is on a device > which supports this feature. > Has this been a problem that people have noticed or complained about? Are there bug reports? AFAIK the default is "off" and PPD will enable ABM if in power or balanced mode and on battery. > What should have been done is the following: > > * add the KMS property and have it default to "sysfs cannot override this" > * add the sysfs property after the KMS property has been introduced so > it stays disabled by default > * add support for the new property in compositors and let them enable > this feature only when they allow colors to randomly get broken > > Every other path results in broken colors at least temporarily and is > breaking user space! The sysfs property *must* be reverted because it > breaks user space. The KMS property *must* be reverted because it > didn't meet the merging criteria. > I agree we should revert the KMS property until we have a userspace implementation that is reviewed and ready to be merged. It was merged based on a misunderstanding and shouldn't have been merged. I don't think we should revert the sysfs property. The power savings to end-users can be significant. I would like to see compositors take control if it via the KMS property. > Another note: The only way to make sure that this isn't breaking user > space is if user space tells the kernel that this is okay. This means > that the sysfs property can only be used if the compositor grows > support for the new KMS property and at that point, why do we have the > sysfs property? > We have a good handful of widely used compositors. We have one PPD with a replacement for it in the works. A sysfs allows all users to get the power benefits even if compositors don't explicitly enable support for power saving features in KMS. The goal of PPD and co. is power savings while that is not always a primary goal for all compositors (even though compositors play a large role in a system's power usage). Harry > On Fri, Aug 2, 2024 at 2:49 PM Xaver Hugl <xaver.hugl@gmail.com> wrote: >> >> Am Do., 1. Aug. 2024 um 14:34 Uhr schrieb Jani Nikula >> <jani.nikula@linux.intel.com>: >>> >>> On Mon, 01 Jul 2024, Xaver Hugl <xaver.hugl@gmail.com> wrote: >>>> Am Do., 20. Juni 2024 um 22:22 Uhr schrieb Xaver Hugl <xaver.hugl@gmail.com>: >>>>> Merging can only happen once a real world userspace application has >>>>> implemented support for it. I'll try to do that sometime next week in >>>>> KWin >>>> >>>> Here's the promised implementation: >>>> https://invent.kde.org/plasma/kwin/-/merge_requests/6028 >>> >>> The requirement is that the userspace patches must be reviewed and ready >>> for merging into a suitable and canonical upstream project. >>> >>> Are they? >> >> I've talked about the property with other KWin developers before, but >> there's indeed no official review for the MR yet. >> As some new discussions about alternative approaches have started as >> well, maybe it should be reverted until we're more certain about how >> to proceed? >> >>> BR, >>> Jani. >>> >>> >>>> >>>> In testing with the patches on top of kernel 6.9.6, setting the >>>> property to `Require color accuracy` makes the sysfs file correctly >>>> report "Device or resource busy" when trying to change the power >>>> saving level, but setting the property to zero doesn't really work. >>>> Once KWin sets the property to zero, changing the power saving level >>>> "works" but the screen blanks for a moment (might just be a single >>>> frame) and reading from the file returns zero again, with the visuals >>>> and backlight level unchanged as well. >>> >>> -- >>> Jani Nikula, Intel >> >
On Fri, Aug 2, 2024 at 4:37 PM Harry Wentland <harry.wentland@amd.com> wrote: > > On 2024-08-02 09:28, Sebastian Wick wrote: > > I'm very unhappy about how this has played out. > > > > We have a new sysfs property that controls a feature of the display > > path that has been set to a default(!) which changes the color > > behavior! This broke color management for everyone who is on a device > > which supports this feature. > > > > Has this been a problem that people have noticed or complained about? > Are there bug reports? Yes. Even worse, it's not obvious to people that something is broken, where it is broken and why it is broken. https://gitlab.gnome.org/GNOME/mutter/-/issues/3589 > > AFAIK the default is "off" and PPD will enable ABM if in power or > balanced mode and on battery. > > > What should have been done is the following: > > > > * add the KMS property and have it default to "sysfs cannot override this" > > * add the sysfs property after the KMS property has been introduced so > > it stays disabled by default > > * add support for the new property in compositors and let them enable > > this feature only when they allow colors to randomly get broken > > > > Every other path results in broken colors at least temporarily and is > > breaking user space! The sysfs property *must* be reverted because it > > breaks user space. The KMS property *must* be reverted because it > > didn't meet the merging criteria. > > > > I agree we should revert the KMS property until we have a userspace > implementation that is reviewed and ready to be merged. It was merged > based on a misunderstanding and shouldn't have been merged. > > I don't think we should revert the sysfs property. The power savings to > end-users can be significant. I would like to see compositors take > control if it via the KMS property. If this was just a KMS property then I wouldn't have anything to complain about. The problem here is the sysfs / PPD thing. > > Another note: The only way to make sure that this isn't breaking user > > space is if user space tells the kernel that this is okay. This means > > that the sysfs property can only be used if the compositor grows > > support for the new KMS property and at that point, why do we have the > > sysfs property? > > > > We have a good handful of widely used compositors. We have one PPD > with a replacement for it in the works. A sysfs allows all users > to get the power benefits even if compositors don't explicitly > enable support for power saving features in KMS. The goal of PPD > and co. is power savings while that is not always a primary goal > for all compositors (even though compositors play a large role in > a system's power usage). The problem is that if the compositor didn't explicitly opt-in, then it can break something (and it actually did). I appreciate the intent (enabling it as broadly as possible) but the only way I can see how you can enable feature at all is by somehow doing it per-compositor. Given all of that, there shouldn't be a sysfs property (you should revert this!) but it should be yet another KMS property. > Harry > > > On Fri, Aug 2, 2024 at 2:49 PM Xaver Hugl <xaver.hugl@gmail.com> wrote: > >> > >> Am Do., 1. Aug. 2024 um 14:34 Uhr schrieb Jani Nikula > >> <jani.nikula@linux.intel.com>: > >>> > >>> On Mon, 01 Jul 2024, Xaver Hugl <xaver.hugl@gmail.com> wrote: > >>>> Am Do., 20. Juni 2024 um 22:22 Uhr schrieb Xaver Hugl <xaver.hugl@gmail.com>: > >>>>> Merging can only happen once a real world userspace application has > >>>>> implemented support for it. I'll try to do that sometime next week in > >>>>> KWin > >>>> > >>>> Here's the promised implementation: > >>>> https://invent.kde.org/plasma/kwin/-/merge_requests/6028 > >>> > >>> The requirement is that the userspace patches must be reviewed and ready > >>> for merging into a suitable and canonical upstream project. > >>> > >>> Are they? > >> > >> I've talked about the property with other KWin developers before, but > >> there's indeed no official review for the MR yet. > >> As some new discussions about alternative approaches have started as > >> well, maybe it should be reverted until we're more certain about how > >> to proceed? > >> > >>> BR, > >>> Jani. > >>> > >>> > >>>> > >>>> In testing with the patches on top of kernel 6.9.6, setting the > >>>> property to `Require color accuracy` makes the sysfs file correctly > >>>> report "Device or resource busy" when trying to change the power > >>>> saving level, but setting the property to zero doesn't really work. > >>>> Once KWin sets the property to zero, changing the power saving level > >>>> "works" but the screen blanks for a moment (might just be a single > >>>> frame) and reading from the file returns zero again, with the visuals > >>>> and backlight level unchanged as well. > >>> > >>> -- > >>> Jani Nikula, Intel > >> > > >
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c index 3ecc7ef95172..cfb5220cf182 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c @@ -1350,6 +1350,10 @@ int amdgpu_display_modeset_create_props(struct amdgpu_device *adev) "dither", amdgpu_dither_enum_list, sz); + if (adev->dc_enabled) + drm_mode_create_power_saving_policy_property(adev_to_drm(adev), + DRM_MODE_POWER_SAVING_POLICY_ALL); + return 0; } 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 f1d67c6f4b98..5fd7210b2479 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -6436,6 +6436,13 @@ 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 == dev->mode_config.power_saving_policy) { + dm_new_state->abm_forbidden = val & DRM_MODE_REQUIRE_COLOR_ACCURACY; + dm_new_state->abm_level = (dm_new_state->abm_forbidden || !amdgpu_dm_abm_level) ? + ABM_LEVEL_IMMEDIATE_DISABLE : + amdgpu_dm_abm_level; + dm_new_state->psr_forbidden = val & DRM_MODE_REQUIRE_LOW_LATENCY; + ret = 0; } return ret; @@ -6478,6 +6485,13 @@ 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 == dev->mode_config.power_saving_policy) { + *val = 0; + if (dm_state->psr_forbidden) + *val |= DRM_MODE_REQUIRE_LOW_LATENCY; + if (dm_state->abm_forbidden) + *val |= DRM_MODE_REQUIRE_COLOR_ACCURACY; + ret = 0; } return ret; @@ -6504,9 +6518,12 @@ static ssize_t panel_power_savings_show(struct device *device, u8 val; drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); - val = to_dm_connector_state(connector->state)->abm_level == - ABM_LEVEL_IMMEDIATE_DISABLE ? 0 : - to_dm_connector_state(connector->state)->abm_level; + if (to_dm_connector_state(connector->state)->abm_forbidden) + val = 0; + else + val = to_dm_connector_state(connector->state)->abm_level == + ABM_LEVEL_IMMEDIATE_DISABLE ? 0 : + to_dm_connector_state(connector->state)->abm_level; drm_modeset_unlock(&dev->mode_config.connection_mutex); return sysfs_emit(buf, "%u\n", val); @@ -6530,10 +6547,16 @@ static ssize_t panel_power_savings_store(struct device *device, return -EINVAL; drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); - to_dm_connector_state(connector->state)->abm_level = val ?: - ABM_LEVEL_IMMEDIATE_DISABLE; + if (to_dm_connector_state(connector->state)->abm_forbidden) + ret = -EBUSY; + else + to_dm_connector_state(connector->state)->abm_level = val ?: + ABM_LEVEL_IMMEDIATE_DISABLE; drm_modeset_unlock(&dev->mode_config.connection_mutex); + if (ret) + return ret; + drm_kms_helper_hotplug_event(dev); return count; @@ -7704,6 +7727,13 @@ 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)) { + drm_object_attach_property(&aconnector->base.base, + dm->ddev->mode_config.power_saving_policy, 0); + } + if (connector_type == DRM_MODE_CONNECTOR_HDMIA) { /* Content Type is currently only implemented for HDMI. */ drm_connector_attach_content_type_property(&aconnector->base); @@ -9307,6 +9337,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state) for_each_oldnew_connector_in_state(state, connector, old_con_state, new_con_state, i) { struct dm_connector_state *dm_new_con_state = to_dm_connector_state(new_con_state); struct dm_connector_state *dm_old_con_state = to_dm_connector_state(old_con_state); + struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector); struct amdgpu_crtc *acrtc = to_amdgpu_crtc(dm_new_con_state->base.crtc); struct dc_surface_update *dummy_updates; struct dc_stream_update stream_update; @@ -9360,6 +9391,15 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state) stream_update.hdr_static_metadata = &hdr_packet; } + aconnector->disallow_edp_enter_psr = dm_new_con_state->psr_forbidden; + + /* immediately disable PSR if disallowed */ + if (aconnector->disallow_edp_enter_psr) { + mutex_lock(&dm->dc_lock); + amdgpu_dm_psr_disable(dm_new_crtc_state->stream); + mutex_unlock(&dm->dc_lock); + } + status = dc_stream_get_status(dm_new_crtc_state->stream); if (WARN_ON(!status)) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h index 09519b7abf67..b246e82f5b0d 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h @@ -873,6 +873,8 @@ struct dm_connector_state { bool underscan_enable; bool freesync_capable; bool update_hdcp; + bool abm_forbidden; + bool psr_forbidden; uint8_t abm_level; int vcpi_slots; uint64_t pbn;
When the `power_saving_policy` property is set to bit mask "Require color accuracy" ABM should be disabled immediately and any requests by sysfs to update will return an -EBUSY error. When the `power_saving_policy` property is set to bit mask "Require low latency" PSR should be disabled. When the property is restored to an empty bit mask the previous value of ABM and pSR will be restored. Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- v2->v3: * Use `disallow_edp_enter_psr` instead * Drop case in dm_update_crtc_state() --- drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 4 ++ .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 50 +++++++++++++++++-- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 2 + 3 files changed, 51 insertions(+), 5 deletions(-)