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 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. -- Ville Syrjälä Intel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
