On Tue, Apr 02, 2019 at 09:49:00AM +0200, Daniel Vetter wrote:
On Mon, Apr 01, 2019 at 09:49:30AM -0400, Sean Paul wrote:
On Fri, Mar 29, 2019 at 08:21:31PM +0100, Daniel Vetter wrote:
On Fri, Mar 29, 2019 at 7:10 PM Sean Paul sean@poorly.run wrote:
On Fri, Mar 29, 2019 at 04:36:32PM +0100, Daniel Vetter wrote:
On Fri, Mar 29, 2019 at 09:16:59AM -0400, Sean Paul wrote:
On Fri, Mar 29, 2019 at 09:21:10AM +0100, Daniel Vetter wrote: > On Thu, Mar 28, 2019 at 05:03:03PM -0400, Sean Paul wrote: > > On Wed, Mar 27, 2019 at 07:15:00PM +0100, Daniel Vetter wrote: > > > On Tue, Mar 26, 2019 at 04:44:54PM -0400, Sean Paul wrote: > > > > From: Sean Paul seanpaul@chromium.org > > > > > > > > This patch adds a new drm helper library to help drivers implement > > > > self refresh. Drivers choosing to use it will register crtcs and > > > > will receive callbacks when it's time to enter or exit self refresh > > > > mode. > > > > > > > > In its current form, it has a timer which will trigger after a > > > > driver-specified amount of inactivity. When the timer triggers, the > > > > helpers will submit a new atomic commit to shut the refreshing pipe > > > > off. On the next atomic commit, the drm core will revert the self > > > > refresh state and bring everything back up to be actively driven. > > > > > > > > From the driver's perspective, this works like a regular disable/enable > > > > cycle. The driver need only check the 'self_refresh_active' and/or > > > > 'self_refresh_changed' state in crtc_state and connector_state. It > > > > should initiate self refresh mode on the panel and enter an off or > > > > low-power state. > > > > > > > > Changes in v2: > > > > - s/psr/self_refresh/ (Daniel) > > > > - integrated the psr exit into the commit that wakes it up (Jose/Daniel) > > > > - made the psr state per-crtc (Jose/Daniel) > > > > > > > > Link to v1: https://patchwork.freedesktop.org/patch/msgid/20190228210939.83386-2-sean@po... > > > > > > > > Cc: Daniel Vetter daniel@ffwll.ch > > > > Cc: Jose Souza jose.souza@intel.com > > > > Cc: Zain Wang wzz@rock-chips.com > > > > Cc: Tomasz Figa tfiga@chromium.org > > > > Signed-off-by: Sean Paul seanpaul@chromium.org > > > > --- > > > > Documentation/gpu/drm-kms-helpers.rst | 9 + > > > > drivers/gpu/drm/Makefile | 3 +- > > > > drivers/gpu/drm/drm_atomic.c | 4 + > > > > drivers/gpu/drm/drm_atomic_helper.c | 36 +++- > > > > drivers/gpu/drm/drm_atomic_state_helper.c | 8 + > > > > drivers/gpu/drm/drm_atomic_uapi.c | 5 +- > > > > drivers/gpu/drm/drm_self_refresh_helper.c | 212 ++++++++++++++++++++++ > > > > include/drm/drm_atomic.h | 15 ++ > > > > include/drm/drm_connector.h | 31 ++++ > > > > include/drm/drm_crtc.h | 19 ++ > > > > include/drm/drm_self_refresh_helper.h | 23 +++ > > > > 11 files changed, 360 insertions(+), 5 deletions(-) > > > > create mode 100644 drivers/gpu/drm/drm_self_refresh_helper.c > > > > create mode 100644 include/drm/drm_self_refresh_helper.h > > > >
/snip
> > > > index 4985384e51f6..ec90c527deed 100644 > > > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > > > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > > > > @@ -105,6 +105,10 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc, > > > > state->commit = NULL; > > > > state->event = NULL; > > > > state->pageflip_flags = 0; > > > > + > > > > + /* Self refresh should be canceled when a new update is available */ > > > > + state->active = drm_atomic_crtc_effectively_active(state); > > > > + state->self_refresh_active = false; > > > > } > > > > EXPORT_SYMBOL(__drm_atomic_helper_crtc_duplicate_state); > > > > > > > > @@ -370,6 +374,10 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector, > > > > > > > > /* Don't copy over a writeback job, they are used only once */ > > > > state->writeback_job = NULL; > > > > + > > > > + /* Self refresh should be canceled when a new update is available */ > > > > + state->self_refresh_changed = state->self_refresh_active; > > > > + state->self_refresh_active = false; > > > > > > Why the duplication in self-refresh tracking? Connectors never have a > > > different self-refresh state, and you can always look at the right > > > crtc_state. Duplication just gives us the chance to screw up and get out > > > of sync (e.g. if the crtc for a connector changes). > > > > > > > On disable the crtc is cleared from connector_state, so we don't have access to > > it. If I add the appropriate atomic_enable/disable hooks as suggested below, we > > should be able to nuke these. > > Yeah we'd need the old state to look at the crtc and all that. Which is a > lot more trickier. > > Since it's such a special case, should we have a dedicated callback for > the direct self-refresh -> completely off transition? It'll be asymetric, > but that's the nature of this I think.
Right, the asymmetry is really annoying here. If the driver is SR-aware, it makes sense since SR-active to disable is a real transition. However if the driver is not SR-aware (ie: it just gets turned off when SR becomes active), the disable function gets called twice without an enable. So that changes the "for every enable there is a disable and vice versa" assumption.
This is one of the benefits of the v1 design, SR was bolted on and no existing rules (async/no_modeset/enable-disable pairs) were [explicitly] broken. That's not to say it was better, it wasn't, but it was a big consideration.
So, what to do.
I really like the idea that drivers shouldn't have to be SR-aware to be involved in the pipeline. So if we add a hook for this like you suggest, we could avoid calling disable twice on anything not SR-aware. We would need to add the hook on crtc/encoder/bridge to make sure you could mix n' match SR-aware and non-SR-aware devices.
It probably makes sense to just add matching SR hooks at this point. Since if the driver is doing something special in disable, it'll need to do something special in enable. It also reserves enable and disable for what they've traditionally done. If a device is not SR-aware, it'll just fall back to the full enable/disable and we'll make sure to not double up on the disable in the helpers.
So we'll keep symmetry, and avoid having an awful hook name like disable_from_self_refresh.. yuck!
Thoughts?
I like the asymetry actually, it has grown on a bit while working out and pondering this :-)
I'm not quite there with you, I still think it's better to split it all out.
Benefits:
- we keep the 100% symmetry of enable/disable hooks
- self-refresh aware connector code also gets a bit simpler I think: in the normal enable/disable hooks it can just check for connector->state->crtc->state->self_refresh_active for sr state changes while the pipe is logically staying on
- the one asymmetric case due to this design where we disable the pipe harder has an awkward special hook, which gives us a great opportunity to explain why it's needed
- nothing changes for non-sr aware drivers
- also no need to duplicate sr state into connectors, since it's all fairly explit already in all three state transitions.
To be fair, only one of these is exclusive to asymmetry, and it's the one that provides the opportunity to add a comment. If the sr functions are symmetric, the code becomes much more "normal" and less deserving of the explanation.
The reason I would like to split out entry and exit is that it makes the driver code a bit easier to rationalize. Currently we need to check the state at the beginning of enable/disable to determine whether we want the full enable/disable or the psr exit/enter. So the sr_disable function would really just be plain old disable without the special casing at the top. In that case, we don't even need the separate function, we could just limit disable calls only on those objects which are effectively on (active || sr). That starts sounding a lot like what we already have here.
Further, doing SR in enable/disable is really just legacy from v1 which tried to keep as much the same as possible. Now that we're "in it", I think it makes sense to go all in and make SR a first class citizen.
Hm, question is: How many hooks do you need? Just something on the connector, or on the encoder, or everywhere?
bridge/encoder/crtc all do special things during SR transitions, I don't think connector is necessary. This is the same for any .sr_disable function, everyone would need to implement it.
Hm, that's a lot of new callbacks ...
And how do you handle the various state transitions. On the disable side we have:
- active on -> active off, no sr (userspace disables crtc)
- active on, sr off -> active ooff, sr on (sr timer fires and suspends crtc)
- active off, sr on -> active off, sr off (userspace disable crtc
while crtc is in sr) These are all "logical active on" -> "something" transitions where we disable something (crtc, or display or both)
So in a way you'd need 3 hooks here for the full matrix. And they all kinda disable something. On the enable side we have:
- active off, sr off -> active on, sr off (userspace enables crtc)
- active off, sr on -> active on, sr off (userspace does a pageflip, stops sr)
Here we either enable the crtc (display already on) or both. Since we only go into sr with the timer there's no 3rd case of only enabling the display. So still asymetric, even with lots more hooks.
We don't need the (active off, sr on) -> (active off, sr off) (third) case above, it's the same as the first. Just doing a full disable is sufficient, so you would have symmetry in the enable/disable calls and asymmetry in the sr calls. This is similar to enabling a plane, or turning other HW features on while enabled. SR is after all just a feature of the hardware.
Hm yeah I guess we can treat it like plane disabling, which implicitly happens in crtc->disable too. Or the implicit plane enable in crtc->enable (although that case doesn't exist for sr, since we never go directly into sr).
If you want the full matrix, there's going to be a _lot_ of hooks. I think slightly more awkward driver, but less hooks is better. Hence the slightly awkward middle ground of a special disable_from_sr hook. But maybe there's a better option somewhere else ...
There's really no reason to even have the sr_disable function. The .disable function in the driver will already need special casing to detect psr_entry vs full disable, so it'd be better to just call disable twice. The .sr_disable function would always just do a full disable (ie: the .disable implementation without the sr checks at the top).
So the debate should be: add sr_enable/disable pair of hooks, or overload disable with asymmetry (current implementation).
I guess that means we're back to no new hooks, and the driver just dtrt in the existing hooks with the state transition bits we have? I thought the issue with that is that we can't get at all the right bits, hence the sr_disable special case hook.
Or is your plan to roll out a full new set of hooks, equipped with old/new_state for everything? I think we'd only need old/new_state for the object at hand, since with the old_state you can get at drm_atomic_state, which allows you to get anything else really.
I would suggest all new hooks should be specified as do_stuff(struct drm_atomic_state *state, struct drm_foo *foo); That way there is less confusion how to get at other states besides the old/new foo states explicitly passed in.