Re: [PATCH 0/2] Add support for Panel Power Savings property
On 2024-05-21 13:32, Mario Limonciello wrote: On 5/21/2024 12:27, Leo Li wrote: On 2024-05-21 12:21, Mario Limonciello wrote: On 5/21/2024 11:14, Xaver Hugl wrote: Am Di., 21. Mai 2024 um 16:00 Uhr schrieb Mario Limonciello : On 5/21/2024 08:43, Simon Ser wrote: This makes sense to me in general. I like the fact that it's simple and vendor-neutral. Do we want to hardcode "panel" in the name? Are we sure that this will ever only apply to panels? Do we want to use a name which reflects the intent, rather than the mechanism? In other words, something like "color fidelity" = "preferred" maybe? (I don't know, just throwing ideas around.) In that vein, how about: "power saving policy" --> "power saving" --> "color fidelity" It's not just about colors though, is it? The compositor might want to disable it to increase the backlight brightness in bright environments, so "color fidelity" doesn't really sound right Either of these better? "power saving policy" --> "power saving" --> "accuracy" "power saving policy" --> "allowed" --> "forbidden" Or any other idea? Another consideration in addition to accuracy is latency. I suppose a compositor may want to disable features such as PSR for use-cases requiring low latency. Touch and stylus input are some examples. I wonder if flags would work better than enums? A compositor can set something like `REQUIRE_ACCURACY & REQUIRE_LOW_LATENCY`, for example. I thought we had said the PSR latency issue discussed at the hackfest was likely just a bug and that nominally it won't matter? Ah, either my memory failed, or I failed to clarify. Let me fix that below. If it really will matter enough then yeah I think you're right that flags would be better. More drivers would probably want to opt into the property for the purpose of turning off PSR/PSR2/panel replay as well then. Latency technically shouldn't be a problem for Panel Replay, but is a problem for PSR/PSR2 due to their design. At least it is a problem for amdgpu, not sure about other drivers. FWIU, in short, when a panel (DPRX) goes into self-refresh, it's clock can drift from the gpu (DPTX). Since there's no resync mechanism in PSR/PSR2, it is possible for the panel to lag behind by 1-frame-time(ms) for a noticeable period of time. Panel replay has a resync mechanism, so it theoretically can resync within 1 frame. Since, fwict, replay panels are still rare, I think it'll be nice for compositors that care about latency to have the option to temporarily disable PSR/PSR2. - Leo - Leo Would be nice to add documentation for the property in the "standard connector properties" section. Ack.
Re: [PATCH 0/2] Add support for Panel Power Savings property
On Tuesday, May 21st, 2024 at 19:27, Leo Li wrote: > I wonder if flags would work better than enums? A compositor can set something > like `REQUIRE_ACCURACY & REQUIRE_LOW_LATENCY`, for example. (FWIW, the KMS uAPI has first-class support for bitfields.)
Re: [PATCH 0/2] Add support for Panel Power Savings property
Am Di., 21. Mai 2024 um 19:28 Uhr schrieb Leo Li : > > > > On 2024-05-21 12:21, Mario Limonciello wrote: > > On 5/21/2024 11:14, Xaver Hugl wrote: > >> Am Di., 21. Mai 2024 um 16:00 Uhr schrieb Mario Limonciello > >> : > >>> > >>> On 5/21/2024 08:43, Simon Ser wrote: > This makes sense to me in general. I like the fact that it's simple and > vendor-neutral. > > Do we want to hardcode "panel" in the name? Are we sure that this will > ever only apply to panels? > > Do we want to use a name which reflects the intent, rather than the > mechanism? In other words, something like "color fidelity" = "preferred" > maybe? (I don't know, just throwing ideas around.) > >>> > >>> In that vein, how about: > >>> > >>> "power saving policy" > >>> --> "power saving" > >>> --> "color fidelity" > >> > >> It's not just about colors though, is it? The compositor might want to > >> disable it to increase the backlight brightness in bright > >> environments, so "color fidelity" doesn't really sound right > > > > Either of these better? > > > > "power saving policy" > > --> "power saving" > > --> "accuracy" > > > > "power saving policy" > > --> "allowed" > > --> "forbidden" > > > > Or any other idea? > > Another consideration in addition to accuracy is latency. > > I suppose a compositor may want to disable features such as PSR for use-cases > requiring low latency. Touch and stylus input are some examples. > > I wonder if flags would work better than enums? A compositor can set something > like `REQUIRE_ACCURACY & REQUIRE_LOW_LATENCY`, for example. I think that's a good idea. With a flag for color accuracy and one for brightness, the compositor's intent would be communicated well. > - Leo > > > > >> > > Would be nice to add documentation for the property in the "standard > connector properties" section. > >>> > >>> Ack. > >>> > >
Re: [PATCH 0/2] Add support for Panel Power Savings property
On 5/21/2024 12:27, Leo Li wrote: On 2024-05-21 12:21, Mario Limonciello wrote: On 5/21/2024 11:14, Xaver Hugl wrote: Am Di., 21. Mai 2024 um 16:00 Uhr schrieb Mario Limonciello : On 5/21/2024 08:43, Simon Ser wrote: This makes sense to me in general. I like the fact that it's simple and vendor-neutral. Do we want to hardcode "panel" in the name? Are we sure that this will ever only apply to panels? Do we want to use a name which reflects the intent, rather than the mechanism? In other words, something like "color fidelity" = "preferred" maybe? (I don't know, just throwing ideas around.) In that vein, how about: "power saving policy" --> "power saving" --> "color fidelity" It's not just about colors though, is it? The compositor might want to disable it to increase the backlight brightness in bright environments, so "color fidelity" doesn't really sound right Either of these better? "power saving policy" --> "power saving" --> "accuracy" "power saving policy" --> "allowed" --> "forbidden" Or any other idea? Another consideration in addition to accuracy is latency. I suppose a compositor may want to disable features such as PSR for use-cases requiring low latency. Touch and stylus input are some examples. I wonder if flags would work better than enums? A compositor can set something like `REQUIRE_ACCURACY & REQUIRE_LOW_LATENCY`, for example. I thought we had said the PSR latency issue discussed at the hackfest was likely just a bug and that nominally it won't matter? If it really will matter enough then yeah I think you're right that flags would be better. More drivers would probably want to opt into the property for the purpose of turning off PSR/PSR2/panel replay as well then. - Leo Would be nice to add documentation for the property in the "standard connector properties" section. Ack.
Re: [PATCH 0/2] Add support for Panel Power Savings property
On 2024-05-21 12:21, Mario Limonciello wrote: On 5/21/2024 11:14, Xaver Hugl wrote: Am Di., 21. Mai 2024 um 16:00 Uhr schrieb Mario Limonciello : On 5/21/2024 08:43, Simon Ser wrote: This makes sense to me in general. I like the fact that it's simple and vendor-neutral. Do we want to hardcode "panel" in the name? Are we sure that this will ever only apply to panels? Do we want to use a name which reflects the intent, rather than the mechanism? In other words, something like "color fidelity" = "preferred" maybe? (I don't know, just throwing ideas around.) In that vein, how about: "power saving policy" --> "power saving" --> "color fidelity" It's not just about colors though, is it? The compositor might want to disable it to increase the backlight brightness in bright environments, so "color fidelity" doesn't really sound right Either of these better? "power saving policy" --> "power saving" --> "accuracy" "power saving policy" --> "allowed" --> "forbidden" Or any other idea? Another consideration in addition to accuracy is latency. I suppose a compositor may want to disable features such as PSR for use-cases requiring low latency. Touch and stylus input are some examples. I wonder if flags would work better than enums? A compositor can set something like `REQUIRE_ACCURACY & REQUIRE_LOW_LATENCY`, for example. - Leo Would be nice to add documentation for the property in the "standard connector properties" section. Ack.
Re: [PATCH 0/2] Add support for Panel Power Savings property
On 5/21/2024 11:14, Xaver Hugl wrote: Am Di., 21. Mai 2024 um 16:00 Uhr schrieb Mario Limonciello : On 5/21/2024 08:43, Simon Ser wrote: This makes sense to me in general. I like the fact that it's simple and vendor-neutral. Do we want to hardcode "panel" in the name? Are we sure that this will ever only apply to panels? Do we want to use a name which reflects the intent, rather than the mechanism? In other words, something like "color fidelity" = "preferred" maybe? (I don't know, just throwing ideas around.) In that vein, how about: "power saving policy" --> "power saving" --> "color fidelity" It's not just about colors though, is it? The compositor might want to disable it to increase the backlight brightness in bright environments, so "color fidelity" doesn't really sound right Either of these better? "power saving policy" --> "power saving" --> "accuracy" "power saving policy" --> "allowed" --> "forbidden" Or any other idea? Would be nice to add documentation for the property in the "standard connector properties" section. Ack.
Re: [PATCH 0/2] Add support for Panel Power Savings property
Am Di., 21. Mai 2024 um 16:00 Uhr schrieb Mario Limonciello : > > On 5/21/2024 08:43, Simon Ser wrote: > > This makes sense to me in general. I like the fact that it's simple and > > vendor-neutral. > > > > Do we want to hardcode "panel" in the name? Are we sure that this will > > ever only apply to panels? > > > > Do we want to use a name which reflects the intent, rather than the > > mechanism? In other words, something like "color fidelity" = "preferred" > > maybe? (I don't know, just throwing ideas around.) > > In that vein, how about: > > "power saving policy" > --> "power saving" > --> "color fidelity" It's not just about colors though, is it? The compositor might want to disable it to increase the backlight brightness in bright environments, so "color fidelity" doesn't really sound right > > > > Would be nice to add documentation for the property in the "standard > > connector properties" section. > > Ack. >
Re: [PATCH 0/2] Add support for Panel Power Savings property
On 5/21/2024 08:43, Simon Ser wrote: This makes sense to me in general. I like the fact that it's simple and vendor-neutral. Do we want to hardcode "panel" in the name? Are we sure that this will ever only apply to panels? Do we want to use a name which reflects the intent, rather than the mechanism? In other words, something like "color fidelity" = "preferred" maybe? (I don't know, just throwing ideas around.) In that vein, how about: "power saving policy" --> "power saving" --> "color fidelity" Would be nice to add documentation for the property in the "standard connector properties" section. Ack.
Re: [PATCH 0/2] Add support for Panel Power Savings property
This makes sense to me in general. I like the fact that it's simple and vendor-neutral. Do we want to hardcode "panel" in the name? Are we sure that this will ever only apply to panels? Do we want to use a name which reflects the intent, rather than the mechanism? In other words, something like "color fidelity" = "preferred" maybe? (I don't know, just throwing ideas around.) Would be nice to add documentation for the property in the "standard connector properties" section.
[PATCH 0/2] Add support for Panel Power Savings property
During the Display Next hackfest 2024 one of the topics discussed was the need for compositor to be able to relay intention to drivers that color fidelity is preferred over power savings. To accomplish this a new optional DRM property is being introduced called "panel power saving". This property is an enum that can be configured by the compositor to "Allowed" or "Forbidden". When a driver advertises support for this property and the compositor sets it to "Forbidden" then the driver will disable any power saving features that can compromise color fidelity. In practice the main feature this currently applies to is the "Adaptive Backlight Modulation" feature within AMD DCN on eDP panels. When the compositor has marked the property "Forbidden" then this feature will be disabled and any userspace that tries to turn it on will get an -EBUSY return code. When the compositor has restored the value back to "Allowed" then the previous value that would have been programmed will be restored. Mario Limonciello (2): drm: Introduce panel_power_saving drm property drm/amd: Add panel_power_saving drm property to eDP connectors drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 3 ++ .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 34 ++--- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 + drivers/gpu/drm/drm_connector.c | 37 +++ include/drm/drm_connector.h | 1 + include/drm/drm_mode_config.h | 6 +++ include/uapi/drm/drm_mode.h | 4 ++ 7 files changed, 81 insertions(+), 5 deletions(-) -- 2.45.0
