On Tue, Apr 2, 2019 at 6:47 PM Sean Paul <s...@poorly.run> wrote: > On Tue, Apr 02, 2019 at 06:05:48PM +0200, Daniel Vetter wrote: > > On Tue, Apr 2, 2019 at 3:24 PM Sean Paul <s...@poorly.run> wrote: > > > > > > 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 <s...@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 <seanp...@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-s...@poorly.run > > > > > > > > > > > > > > > > > > > > > > > > > > Cc: Daniel Vetter <dan...@ffwll.ch> > > > > > > > > > > > > > Cc: Jose Souza <jose.so...@intel.com> > > > > > > > > > > > > > Cc: Zain Wang <w...@rock-chips.com> > > > > > > > > > > > > > Cc: Tomasz Figa <tf...@chromium.org> > > > > > > > > > > > > > Signed-off-by: Sean Paul <seanp...@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 don't think we even need to pass the state to the sr hooks, just add > > > > > > void self_refresh_enter(struct drm_<type> *<name>); > > > void self_refresh_exit(struct drm_<type> *<name>); > > > > > > to the funcs vtable for crtc/encoder/bridge. > > > > > > Of course it's not _quite_ as straightforward as that :) > > > > > > With the current model, the powerdown/powerup order of components is > > > implicitly > > > broken. With this new model, it's much more obvious, this is easiest to > > > illustrate with bridges, but it's true for crtcs and encoders as well. > > > > > > Assume you have the following bridge chain: > > > > > > ENC0 (not SR-aware) > > > -> BR0 (SR-aware) > > > -> BR1 (not SR-aware) > > > -> BR2 (SR-aware) > > > -> CON0 > > > > > > An SR-enter transition would be: > > > BR2->self_refresh_enter > > > BR1->disable > > > BR0->self_refresh_enter > > > ENC0->disable > > > BR1->post_disable > > > > > > SR-exit is: > > > BR1->pre_enable > > > ENC0->enable > > > BR0->self_refresh_exit > > > BR1->enable > > > BR2->self_refresh_exit > > > > > > Disabling from SR becomes: > > > BR2->disable > > > BR0->disable > > > BR2->post_disable > > > BR0->post_disable > > > > > > So I'm starting to question falling back on disable. I think it was a fine > > > choice when we would exit psr before disable (ie: v1), but I think it > > > might be > > > too complicated now. We could make BR2 and BR0 do the right thing on > > > disable-from-SR, but I'm worried that mixing up the order for SR-unaware > > > devices > > > (ENC0/BR1) might cause issues. > > > > > > Perhaps we should scale this back and just treat self_refresh as its own > > > thing > > > and not go through the enable/disable path at all. Devices which are not > > > SR-aware stay on (which has it's own issues if BR1 underflows because it's > > > expecting video from BR0). Maybe we have to ensure the entire pipe is > > > SR-aware > > > before we do an SR-enter. > > > > Imo if the driver tries to enable SR on a pipe where some pieces > > aren't SR aware, that's a driver bug. > > If you assume that most bridges (aside from the "bridges" that represent > shared > silicon IP) can be arbitrarily mixed with most other bridges and drivers, then > yeah, this is unavoidable and not something that's easily fixed since we'd > need > to make all SR-aware or at the very least audit them to make sure they don't > foul up if the things around them go to sleep. > > > And if you really want to > > implement the above sequence (well, need to implement it), then I > > agree that helpers aren't the thing you're looking for and you should > > just roll your own modeset code. > > > > But we started all this assuming that you're hw isn't in the need of > > the full state matrix with hooks for everything, hence that maybe a > > helper would make sense. It feels a bit like the discussion lost > > contact with the (driver) reality ... > > > > > Thoughts? > > > > ... so imo if we can help out drivers by repurposing the existing > > hooks to cover most cases, with a few special cases in callbacks, then > > we can roll these helpers. If that doesn't happen, then probably > > better if each driver just rolls their own sr enter/exit code and > > calls it done. It's not like we don't allow subclassing of states or > > also vtable hooks where you could just add more of your own stuff as > > you see fit. But I thought sr for most devices would amount to a) > > shutting the pipe down b) some special casing to keep the display > > alive and nothing else. But now it sounds like you need hooks for > > everything, which probably doesnt make sense to cover in the helpers. > > For most everything upstream of the connector, you don't _need_ a hook since > shutting them down is fine. However if your crtc takes a while to come back on > (like with rockchip), then you start to want hooks everywhere to optimize > things. > > I'll put this on the shelf and wait for a few more drivers to implement their > own SR. Perhaps a pattern will emerge.
I did scroll through the rockchip implementation patches again. Adding a few conditions seems not too onerous, repurposing the current hooks works. And I think if we pimp the atomic hooks as ville suggested (just the ones where we need it, we can be lazy), then I think we can also get rid of the duplicated tracking in connectors and essentially go with v2. So wondering a bit whether we managed to derail this unecessarily? It seems to work ... And I think some state dependent code flow is unavoidable anyway, whether you roll your own modeset code or use the helpers. Most drivers have such "hacks" for something somewhere. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
