| Message ID | 20210210001401.463-1-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert "drm/atomic: document and enforce rules around "spurious" EBUSY" | expand |
On Tue, Feb 09, 2021 at 04:14:01PM -0800, Manasi Navare wrote: > This reverts commit fb6473a48b635c55d04eb94e579eede52ef39550. > > These additional checks added to avoid EBUSY give unnecessary WARN_ON > in case of big joiner used in i915 in which case even if the modeset > is requested on a single pipe, internally another consecutive > pipe is stolen and used to drive half of the transcoder timings. > So in this case it is expected that requested crtc and affected crtcs > do not match. Hence the added WARN ON becomes irrelevant. > But there is no easy solution to get the bigjoiner information > here at drm level. So for now revert this until we work out > a better solution. > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> Nope. We can maybe rework this so that i915 can do stuff under the hood, but wrt uapi this was the thing we discussed with compositors. Without such a guarantee atomic is defacto broken from a compositor pov. This WARN_ON is not unecessary, compositor people really do not want the kernel to throw around spurious EBUSY they have no visibility into. Please also cc all the compositor people from my original patch if you change anything in this area. -Daniel > --- > drivers/gpu/drm/drm_atomic.c | 29 ----------------------------- > 1 file changed, 29 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index b1efa9322be2..48b2262d69f6 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -320,10 +320,6 @@ 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 > @@ -1306,15 +1302,10 @@ 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) { > @@ -1362,26 +1353,6 @@ 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); > -- > 2.19.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wednesday, February 10th, 2021 at 2:16 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Feb 09, 2021 at 04:14:01PM -0800, Manasi Navare wrote: > > > These additional checks added to avoid EBUSY give unnecessary WARN_ON > > in case of big joiner used in i915 in which case even if the modeset > > is requested on a single pipe, internally another consecutive > > pipe is stolen and used to drive half of the transcoder timings. > > So in this case it is expected that requested crtc and affected crtcs > > do not match. Hence the added WARN ON becomes irrelevant. The WARN_ON only happens if allow_modeset == false. If allow_modeset == true, then the driver is allowed to steal an unrelated pipe. Maybe i915 is stealing a pipe without allow_modeset? > Nope. We can maybe rework this so that i915 can do stuff under the hood, > but wrt uapi this was the thing we discussed with compositors. Without > such a guarantee atomic is defacto broken from a compositor pov. Agreed.
On Wed, Feb 10, 2021 at 01:38:45PM +0000, Simon Ser wrote: > On Wednesday, February 10th, 2021 at 2:16 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Tue, Feb 09, 2021 at 04:14:01PM -0800, Manasi Navare wrote: > > > > > These additional checks added to avoid EBUSY give unnecessary WARN_ON > > > in case of big joiner used in i915 in which case even if the modeset > > > is requested on a single pipe, internally another consecutive > > > pipe is stolen and used to drive half of the transcoder timings. > > > So in this case it is expected that requested crtc and affected crtcs > > > do not match. Hence the added WARN ON becomes irrelevant. > > The WARN_ON only happens if allow_modeset == false. If allow_modeset == true, > then the driver is allowed to steal an unrelated pipe. > > Maybe i915 is stealing a pipe without allow_modeset? No. All page flips etc. will have to get split up internally between multiple crtcs. So I think there's basically three options: a) massive rewrite of i915 to bypass even more of drm_atomic stuff b) allow i915 to silence that warning, which opens up the question whether the warn is doing any good if it can just be bypassed c) nuke the warning entirely a) is not going to happen, and it would any way allow i915 to do things any which way it wants without tripping the warn, rendering the warn entirely toothless. Hmm. Maybe there is a d) which would be to ignore all crtcs that are not logically enabled in the warn? Not sure if that could allow something to slit through that people want it to catch?
On Wed, Feb 10, 2021 at 05:07:03PM +0200, Ville Syrjälä wrote: > On Wed, Feb 10, 2021 at 01:38:45PM +0000, Simon Ser wrote: > > On Wednesday, February 10th, 2021 at 2:16 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > On Tue, Feb 09, 2021 at 04:14:01PM -0800, Manasi Navare wrote: > > > > > > > These additional checks added to avoid EBUSY give unnecessary WARN_ON > > > > in case of big joiner used in i915 in which case even if the modeset > > > > is requested on a single pipe, internally another consecutive > > > > pipe is stolen and used to drive half of the transcoder timings. > > > > So in this case it is expected that requested crtc and affected crtcs > > > > do not match. Hence the added WARN ON becomes irrelevant. > > > > The WARN_ON only happens if allow_modeset == false. If allow_modeset == true, > > then the driver is allowed to steal an unrelated pipe. > > > > Maybe i915 is stealing a pipe without allow_modeset? > > No. All page flips etc. will have to get split up internally > between multiple crtcs. > > So I think there's basically three options: > a) massive rewrite of i915 to bypass even more of drm_atomic stuff > b) allow i915 to silence that warning, which opens up the question > whether the warn is doing any good if it can just be bypassed > c) nuke the warning entirely > > a) is not going to happen, and it would any way allow i915 to > do things any which way it wants without tripping the warn, > rendering the warn entirely toothless. > > Hmm. Maybe there is a d) which would be to ignore all crtcs > that are not logically enabled in the warn? Not sure if that > could allow something to slit through that people want it to > catch? So as per the offline IRC discussions, - We can check for crtc_state->enable and only use the enabled crtcs in the affected crtc calculation. And this enable would only be set when modeset is done. So in case of bigjoiner no modeset on Pipe A, even if Pipe B is stolen, since no modeset and because that pipe doesnt get enabled the affected crtcs would still be 0x1. This should solve the problem. Ville, Danvet - I will make this change? Manasi > > -- > Ville Syrjälä > Intel > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Feb 10, 2021 at 03:26:00PM -0800, Navare, Manasi wrote: > On Wed, Feb 10, 2021 at 05:07:03PM +0200, Ville Syrjälä wrote: > > On Wed, Feb 10, 2021 at 01:38:45PM +0000, Simon Ser wrote: > > > On Wednesday, February 10th, 2021 at 2:16 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > > On Tue, Feb 09, 2021 at 04:14:01PM -0800, Manasi Navare wrote: > > > > > > > > > These additional checks added to avoid EBUSY give unnecessary WARN_ON > > > > > in case of big joiner used in i915 in which case even if the modeset > > > > > is requested on a single pipe, internally another consecutive > > > > > pipe is stolen and used to drive half of the transcoder timings. > > > > > So in this case it is expected that requested crtc and affected crtcs > > > > > do not match. Hence the added WARN ON becomes irrelevant. > > > > > > The WARN_ON only happens if allow_modeset == false. If allow_modeset == true, > > > then the driver is allowed to steal an unrelated pipe. > > > > > > Maybe i915 is stealing a pipe without allow_modeset? > > > > No. All page flips etc. will have to get split up internally > > between multiple crtcs. > > > > So I think there's basically three options: > > a) massive rewrite of i915 to bypass even more of drm_atomic stuff > > b) allow i915 to silence that warning, which opens up the question > > whether the warn is doing any good if it can just be bypassed > > c) nuke the warning entirely > > > > a) is not going to happen, and it would any way allow i915 to > > do things any which way it wants without tripping the warn, > > rendering the warn entirely toothless. > > > > Hmm. Maybe there is a d) which would be to ignore all crtcs > > that are not logically enabled in the warn? Not sure if that > > could allow something to slit through that people want it to > > catch? Yeah it's option d), because the warning is meant to catch funny uapi that compositors don't want to deal with. So if this bigjoiner stuff in i915 is _really_ fully transparent, then it's ok. And excluding completely disabled CRTC from this check makes imo sense, since userspace - is not allowed to issue an atomic flip on these (it's off) - is required to set allow_modeset to enable (at which point i915 can internally move CRTC assignments around however it feels like and it's all fine). Once that fully modeset is done we'd again be in sync with userspace's understanding of what's going on. - hence there cannot be a spurious EBUSY to userspace I think what this needs is a big comment in the code explaining why we can afford to not check this. > So as per the offline IRC discussions, > - We can check for crtc_state->enable and only use the enabled crtcs > in the affected crtc calculation. And this enable would only > be set when modeset is done. So in case of bigjoiner no modeset on Pipe A, > even if Pipe B is stolen, since no modeset and because that pipe doesnt > get enabled the affected crtcs would still be 0x1. > > This should solve the problem. > Ville, Danvet - I will make this change? I think you volunteered :-) Pls Cc: all the people involved in the original patch discussion, so that they can ack the change too. Cheers, Daniel
On Wed, 10 Feb 2021 at 15:07, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Wed, Feb 10, 2021 at 01:38:45PM +0000, Simon Ser wrote: > > The WARN_ON only happens if allow_modeset == false. If allow_modeset == true, > > then the driver is allowed to steal an unrelated pipe. > > > > Maybe i915 is stealing a pipe without allow_modeset? > > No. All page flips etc. will have to get split up internally > between multiple crtcs. I think this is the salient point. > So I think there's basically three options: > a) massive rewrite of i915 to bypass even more of drm_atomic stuff > b) allow i915 to silence that warning, which opens up the question > whether the warn is doing any good if it can just be bypassed > c) nuke the warning entirely > > a) is not going to happen, and it would any way allow i915 to > do things any which way it wants without tripping the warn, > rendering the warn entirely toothless. > > Hmm. Maybe there is a d) which would be to ignore all crtcs > that are not logically enabled in the warn? Not sure if that > could allow something to slit through that people want it to > catch? So from what I understand, if I enable CRTC 44 and the driver magically decides to split it up as a 'big-joiner' output, it will also steal CRTC 50 to work as the other half of the output. Then if I attach plane 47 to CRTC 44, posting a FB to plane 47 will result in me getting atomic completion events for both CRTC 44 and CRTC 50? That's not OK from a userspace perspective. If you want to do magic to create the illusion of a single CRTC, then that magic needs to be consistent. At the moment it's the worst kind of magic: it does implicit things under the hood for you, but then leaks all of the behind-the-scenes detail into userspace. Continuing with that would force us all to just ignore whatever events we see, because we can't reason about what they may be or why they're generated. Which doesn't allow for any kind of best practice in userspace. Cheers, Daniel
On Thu, Feb 11, 2021 at 04:14:02PM +0000, Daniel Stone wrote: > On Wed, 10 Feb 2021 at 15:07, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > On Wed, Feb 10, 2021 at 01:38:45PM +0000, Simon Ser wrote: > > > The WARN_ON only happens if allow_modeset == false. If allow_modeset == true, > > > then the driver is allowed to steal an unrelated pipe. > > > > > > Maybe i915 is stealing a pipe without allow_modeset? > > > > No. All page flips etc. will have to get split up internally > > between multiple crtcs. > > I think this is the salient point. > > > So I think there's basically three options: > > a) massive rewrite of i915 to bypass even more of drm_atomic stuff > > b) allow i915 to silence that warning, which opens up the question > > whether the warn is doing any good if it can just be bypassed > > c) nuke the warning entirely > > > > a) is not going to happen, and it would any way allow i915 to > > do things any which way it wants without tripping the warn, > > rendering the warn entirely toothless. > > > > Hmm. Maybe there is a d) which would be to ignore all crtcs > > that are not logically enabled in the warn? Not sure if that > > could allow something to slit through that people want it to > > catch? > > So from what I understand, if I enable CRTC 44 and the driver > magically decides to split it up as a 'big-joiner' output, it will > also steal CRTC 50 to work as the other half of the output. Then if I > attach plane 47 to CRTC 44, posting a FB to plane 47 will result in me > getting atomic completion events for both CRTC 44 and CRTC 50? > > That's not OK from a userspace perspective. If you want to do magic to > create the illusion of a single CRTC, then that magic needs to be > consistent. At the moment it's the worst kind of magic: it does > implicit things under the hood for you, but then leaks all of the > behind-the-scenes detail into userspace. > > Continuing with that would force us all to just ignore whatever events > we see, because we can't reason about what they may be or why they're > generated. Which doesn't allow for any kind of best practice in > userspace. You should only get externally visibile events for stuff in your request. Or at least if that's not the case then the atomic code is already bork regardless of bigjoiner.
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index b1efa9322be2..48b2262d69f6 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -320,10 +320,6 @@ 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 @@ -1306,15 +1302,10 @@ 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) { @@ -1362,26 +1353,6 @@ 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);
This reverts commit fb6473a48b635c55d04eb94e579eede52ef39550. These additional checks added to avoid EBUSY give unnecessary WARN_ON in case of big joiner used in i915 in which case even if the modeset is requested on a single pipe, internally another consecutive pipe is stolen and used to drive half of the transcoder timings. So in this case it is expected that requested crtc and affected crtcs do not match. Hence the added WARN ON becomes irrelevant. But there is no easy solution to get the bigjoiner information here at drm level. So for now revert this until we work out a better solution. Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: Daniel Vetter <daniel.vetter@intel.com> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> --- drivers/gpu/drm/drm_atomic.c | 29 ----------------------------- 1 file changed, 29 deletions(-)