On Mon, Nov 08, 2021 at 10:32:04AM -0500, Kazlauskas, Nicholas wrote: > On 2021-11-08 10:07 a.m., Daniel Vetter wrote: > > On Fri, Nov 05, 2021 at 04:47:29PM -0400, Kazlauskas, Nicholas wrote: > > > Hi Daniel, > > > > > > Just got bitten by this warning when trying to do some refactoring in amdgpu > > > for trying to get rid of the DRM private object we use for our DC state. > > > > > > From a userspace perspective I understand that we want to avoid judder, > > > -EBUSY and other issues affecting the compositor from kernel having to drag > > > these CRTCs (or their planes) into the atomic state. > > > > > > For bandwidth validation we need to understand the state of all CRTCs and > > > planes in use. Existing driver code maintains this as part of a global state > > > object in a DRM private atomic state. We have stalls in atomic check (bad) > > > to avoid freeing this state or modifying it at the wrong times which avoid > > > hitting this warning but essentially cause the same judder issue. > > > > > > While most hardware has independent pipes, I think almost all hardware ends > > > up having the memory interface/bandwidth as a global shared resource that > > > software state can't really abstract around. > > > > > > There are cases where we know that there will be no (or minimal) impact to > > > the overall memory requirements for particular DRM updates. Our validation > > > already "over-allocates" for common display changes - page flips, some > > > format changes, cursor enable/disable. But for most cases outside of that we > > > do want to pull in _all_ the CRTCs and planes. > > > > > > On our HW you won't get a blankout unless you're actually modifying a stream > > > timing itself so I think the ALLOW_MODESET flag is overkill here. > > > > > > Rejecting the commit when the flag isn't set also ends up breaking userspace > > > in the process since it expects commits like pageflips between different > > > tiling modes to succeed with the legacy IOCTLs. > > > > > > Any ideas about this? I missed the IRC discussion regarding this before so > > > I'm not sure if we have any alternatives that were dropped in favor of this. > > > > We have, I while ago I had a lengthy discussion with I think Maxime about > > how this is done properly. I think Maxime volunteered to type up some docs > > even. tldr; > > > > - Have some global state for this stuff (using the private state helper > > stuff ideally, handrolling very much not adviced, might even make sense > > to put some of your dc state/structs in there) > > This is what we do today, but DRM private objects have no synchronization > like CRTCs do. Any time a commit modifies our bandwidth calculations we need > to propagate those changes back up to the global state. > > Read only access to the global state can be concurrent, but as soon as > anything needs to write and update it then we need to force ordering. > > The bug we had before was that competing commits could complete in a > different order than they would be programmed to HW if they were > non-blocking. Yeah it's probably high time we add a drm_crtc_commit to drm private state objects and make this work. Also maybe rename it to drm_hw_commit or so since it's not about crtc only anymore. > > - Both the crtc/plane and the global state hold copies of your limits. The > > global state in addition also holds the current settings for your > > derived values (like clocks or allocations or whatever you have). > There are per pipe limits but there are also global limits on the whole > system. > > For example, you may pass validation for a configuration applied to a single > CRTC and plane alone. But if you were to run 4 CRTCs using that same valid > configuration it would fail. > > The resource in question here isn't a pipe, an encoder, or something of that > nature - it's just the memory bandwidth available on the system. > > On an APU you wouldn't have enough memory bandwidth to drive multiple CRTCs > at "extreme" resolutions or refresh rates. > > It also doesn't seem right to block the user from running any display at > these modes just because you can't enable 4 identical displays at the same > time. Yeah this is just standard stuff. And yes you do _not_ want to force the limit to be global / #crtcs. It must be dynamic as you enable/disable crtcs. So I'm a bit confused what you mean here? > > - Rules are that only grabbing the global state is enough to read all your > > crtc/plane requirements (since you have a read-only copy of that in your > > global state). Only for changing them do you also have to have the > > corresponding crtc/plane state. Furthermore you _only_ grab the global > > objects if the commit reasonably changes your local requirements (i.e. > > more planes or whatever). > > > > - You might need to have a cascade here (plane -> crtc and crct -> global > > state), and/or maybe multiple different global states. Don't try to put > > everything into one, it's better to have a separate private state > > handling for each separate thing, at least generally. > > > > - No more "we have to take all objects all the time, always", so no more > > over-locking and over-sync. > > > > - Integrating this into DC is probably going to be "fun". Could be that > > you first need to convert a pile of the dc_ structs into driver private > > state stuff, similar in spirit to the refactor we've done for plane/crtc > > state before DC was landed. > > ...so while we can do CRTC and plane validation it doesn't seem like there's > a good place for that global system level validation that we need to happen. Nah, you need to do that afterwards in your dc_atomic_check, iff the global state has been added. crtc/plane will pull the global state in if any requirements/limits they have have changed. Doing the global checks in the objects atomic_check callbacks wont work, those only a) check whether limits/requirements they have changed and b) if so, grab the right corresponding global state and update the values in there. > > Pls check with Maxime that this is documented somewhere as implementation > > pattern (it's really common, and your aproach of "grab all crtc/plane" is > > the really common wrong approach). And ofc happy to discuss how to best > > solve this for DC, but maybe irc is better for that. > > > > Cheers, Daniel > > I can try and hop on IRC sometime to chat about this with you and the > others. > > Thanks for the response! Cheers, Daniel > > Regards, > Nicholas Kazlauskas > > > > > > > > > Regards, > > > Nicholas Kazlauskas > > > > > > On 2020-09-25 4:46 a.m., Daniel Vetter wrote: > > > > When doing an atomic modeset with ALLOW_MODESET drivers are allowed to > > > > pull in arbitrary other resources, including CRTCs (e.g. when > > > > reconfiguring global resources). > > > > > > > > But in nonblocking mode userspace has then no idea this happened, > > > > which can lead to spurious EBUSY calls, both: > > > > - when that other CRTC is currently busy doing a page_flip the > > > > ALLOW_MODESET commit can fail with an EBUSY > > > > - on the other CRTC a normal atomic flip can fail with EBUSY because > > > > of the additional commit inserted by the kernel without userspace's > > > > knowledge > > > > > > > > For blocking commits this isn't a problem, because everyone else will > > > > just block until all the CRTC are reconfigured. Only thing userspace > > > > can notice is the dropped frames without any reason for why frames got > > > > dropped. > > > > > > > > Consensus is that we need new uapi to handle this properly, but no one > > > > has any idea what exactly the new uapi should look like. Since this > > > > has been shipping for years already compositors need to deal no matter > > > > what, so as a first step just try to enforce this across drivers > > > > better with some checks. > > > > > > > > v2: Add comments and a WARN_ON to enforce this only when allowed - we > > > > don't want to silently convert page flips into blocking plane updates > > > > just because the driver is buggy. > > > > > > > > v3: Fix inverted WARN_ON (Pekka). > > > > > > > > v4: Drop the uapi changes, only add a WARN_ON for now to enforce some > > > > rules for drivers. > > > > > > > > v5: Make the WARNING more informative (Daniel) > > > > > > > > v6: Add unconditional debug output for compositor hackers to figure > > > > out what's going on when they get an EBUSY (Daniel) > > > > > > > > v7: Fix up old/new_crtc_state confusion for real (Pekka/Ville) > > > > > > > > References: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2018-July%2F182281.html&data=04%7C01%7Cnicholas.kazlauskas%40amd.com%7C1b07173e73194ee6563a08d9a2c97eef%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637719808574816469%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=zmfJd7M5ZnI5if%2FHM%2FYnBQdoLsWtfjwQW%2BaX0R6Hrj0%3D&reserved=0 > > > > Bugzilla: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fwayland%2Fweston%2F-%2Fissues%2F24%23note_9568&data=04%7C01%7Cnicholas.kazlauskas%40amd.com%7C1b07173e73194ee6563a08d9a2c97eef%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637719808574816469%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=ynHonfkaMn%2BC%2BsykK1prK%2BAXi9eg3CbIO1vbG8eNjT0%3D&reserved=0 > > > > Cc: Daniel Stone <daniel@xxxxxxxxxxxxx> > > > > Cc: Pekka Paalanen <pekka.paalanen@xxxxxxxxxxxxxxx> > > > > Cc: Simon Ser <contact@xxxxxxxxxxx> > > > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> > > > > --- > > > > drivers/gpu/drm/drm_atomic.c | 29 +++++++++++++++++++++++++++++ > > > > 1 file changed, 29 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > > > index 58527f151984..aac9122f1da2 100644 > > > > --- a/drivers/gpu/drm/drm_atomic.c > > > > +++ b/drivers/gpu/drm/drm_atomic.c > > > > @@ -281,6 +281,10 @@ EXPORT_SYMBOL(__drm_atomic_state_free); > > > > * needed. It will also grab the relevant CRTC lock to make sure that the state > > > > * is consistent. > > > > * > > > > + * WARNING: Drivers may only add new CRTC states to a @state if > > > > + * drm_atomic_state.allow_modeset is set, or if it's a driver-internal commit > > > > + * not created by userspace through an IOCTL call. > > > > + * > > > > * Returns: > > > > * > > > > * Either the allocated state or the error code encoded into the pointer. When > > > > @@ -1262,10 +1266,15 @@ int drm_atomic_check_only(struct drm_atomic_state *state) > > > > struct drm_crtc_state *new_crtc_state; > > > > struct drm_connector *conn; > > > > struct drm_connector_state *conn_state; > > > > + unsigned requested_crtc = 0; > > > > + unsigned affected_crtc = 0; > > > > int i, ret = 0; > > > > DRM_DEBUG_ATOMIC("checking %p\n", state); > > > > + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) > > > > + requested_crtc |= drm_crtc_mask(crtc); > > > > + > > > > for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { > > > > ret = drm_atomic_plane_check(old_plane_state, new_plane_state); > > > > if (ret) { > > > > @@ -1313,6 +1322,26 @@ int drm_atomic_check_only(struct drm_atomic_state *state) > > > > } > > > > } > > > > + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) > > > > + affected_crtc |= drm_crtc_mask(crtc); > > > > + > > > > + /* > > > > + * For commits that allow modesets drivers can add other CRTCs to the > > > > + * atomic commit, e.g. when they need to reallocate global resources. > > > > + * This can cause spurious EBUSY, which robs compositors of a very > > > > + * effective sanity check for their drawing loop. Therefor only allow > > > > + * drivers to add unrelated CRTC states for modeset commits. > > > > + * > > > > + * FIXME: Should add affected_crtc mask to the ATOMIC IOCTL as an output > > > > + * so compositors know what's going on. > > > > + */ > > > > + if (affected_crtc != requested_crtc) { > > > > + DRM_DEBUG_ATOMIC("driver added CRTC to commit: requested 0x%x, affected 0x%0x\n", > > > > + requested_crtc, affected_crtc); > > > > + WARN(!state->allow_modeset, "adding CRTC not allowed without modesets: requested 0x%x, affected 0x%0x\n", > > > > + requested_crtc, affected_crtc); > > > > + } > > > > + > > > > return 0; > > > > } > > > > EXPORT_SYMBOL(drm_atomic_check_only); > > > > > > > > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
