Re: [PATCH v2] drm/amd/display: add panel_power_savings sysfs entry to eDP connectors

[Pekka Paalanen <pekka.paalanen@haloniitty.fi>]

[-- Attachment #1: Type: text/plain, Size: 5249 bytes --]

On Fri, 16 Feb 2024 10:32:10 -0600
Mario Limonciello <mario.limonciello@amd.com> wrote:

> On 2/16/2024 10:13, Harry Wentland wrote:
> > 
> > 
> > On 2024-02-16 11:11, Harry Wentland wrote:  
> >>
> >>
> >> On 2024-02-16 10:42, Pekka Paalanen wrote:  
> >>> On Fri, 16 Feb 2024 09:33:47 -0500
> >>> Harry Wentland <harry.wentland@amd.com> wrote:
> >>>  
> >>>> On 2024-02-16 03:19, Pekka Paalanen wrote:  
> >>>>> On Fri, 2 Feb 2024 10:28:35 -0500
> >>>>> Hamza Mahfooz <hamza.mahfooz@amd.com> wrote:
> >>>>>      
> >>>>>> We want programs besides the compositor to be able to enable or disable
> >>>>>> panel power saving features.  
> >>>>>
> >>>>> Could you also explain why, in the commit message, please?
> >>>>>
> >>>>> It is unexpected for arbitrary programs to be able to override the KMS
> >>>>> client, and certainly new ways to do so should not be added without an
> >>>>> excellent justification.
> >>>>>
> >>>>> Maybe debugfs would be more appropriate if the purpose is only testing
> >>>>> rather than production environments?
> >>>>>      
> >>>>>> However, since they are currently only
> >>>>>> configurable through DRM properties, that isn't possible. So, to remedy
> >>>>>> that issue introduce a new "panel_power_savings" sysfs attribute.  
> >>>>>
> >>>>> When the DRM property was added, what was used as the userspace to
> >>>>> prove its workings?
> >>>>>      
> >>>>
> >>>> I don't think there ever was a userspace implementation and doubt any
> >>>> exists today. Part of that is on me. In hindsight, the KMS prop should
> >>>> have never gone upstream.
> >>>>
> >>>> I suggest we drop the KMS prop entirely.  
> >>>
> >>> Sounds good. What about the sysfs thing? Should it be a debugfs thing
> >>> instead, assuming the below question will be resolved?
> >>>  
> >>
> >>
> >> It's intended to be used by the power profiles daemon (PPD). I don't think
> >> debugfs is the right choice. See
> >> https://gitlab.freedesktop.org/upower/power-profiles-daemon/-/commit/41ed5d33a82b0ceb7b6d473551eb2aa62cade6bc
> >>  
> >>>> As for the color accuracy topic, I think it is important that compositors
> >>>> can have full control over that if needed, while it's also important
> >>>> for HW vendors to optimize for power when absolute color accuracy is not
> >>>> needed. An average end-user writing code or working on their slides
> >>>> would rather have a longer battery life than a perfectly color-accurate
> >>>> display. We should probably think of a solution that can support both
> >>>> use-cases.  
> >>>
> >>> I agree. Maybe this pondering should start from "how would it work from
> >>> end user perspective"?
> >>>
> >>> "Automatically" is probably be most desirable answer. Some kind of  
> >>
> >> I agree
> >>  
> >>> desktop settings with options like "save power at the expense of image
> >>> quality":
> >>> - always
> >>> - not if watching movies/gaming
> >>> - on battery
> >>> - on battery, unless I'm watching movies/gaming
> >>> - never
> >>>  
> >>
> >> It's interesting that you split out movies/gaming, specifically. AMD's
> >> ABM algorithm seems to have considered movies in particular when
> >> evaluating the power/fidelity trade-off.
> >>
> >> I wouldn't think consumer media is very particular about a specific
> >> color fidelity (despite what HDR specs try to make you believe). Where
> >> color fidelity would matter to me is when I'd want to edit pictures or
> >> video.
> >>
> >> The "abm_level" property that we expose is really just that, a setting
> >> for the strength of the power-savings effect, with 0 being off and 4 being
> >> maximum strength and power saving, at the expense of fidelity.
> >>
> >> Mario's work is to let the PPD control it and set the ABM levels based on
> >> the selected power profile:
> >> 0 - Performance
> >> 1 - Balance
> >> 3 - Power
> >>
> >> And I believe we've looked at disabling ABM (setting it to 0) automatically
> >> if we know we're on AC power.
> >>  
> >>> Or maybe there already is something like that, and it only needs to be
> >>> plumbed through?
> >>>
> >>> Which would point towards KMS clients needing to control it, which
> >>> means a generic KMS prop rather than vendor specific?
> >>>
> >>> Or should the desktop compositor be talking to some daemon instead of
> >>> KMS for this? Maybe they already are?
> >>>  
> >>
> >> I think the intention is for the PPD to be that daemon. Mario can elaborate.
> >>  
> > 
> > Some more details and screenshots on how the PPD is expected to work and look:
> > https://linuxconfig.org/how-to-manage-power-profiles-over-d-bus-with-power-profiles-daemon-on-linux  
> 
> Right, thanks!
> 
> The most important point is that the user indicates intent from the GUI.
> The daemon orchestrates the various knobs to get that intent.
> 
> It's intentionally very coarse - 3 power states.  The policy of what to 
> do for those states is managed by the daemon.
> 
> In the case of ABM it will only apply the policy if the daemon detects 
> the system is on battery.
> 

Sounds like sysfs is the best option, and it should never have been a
KMS property, indeed.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]