| Message ID | 20171113133622.8593-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 13-11-17 om 14:36 schreef Ville Syrjala: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > We have plenty of global registers and whatnot programmed without > any further locking by the modeset code. Currently non-bocking > modesets are allowed to execute in parallel which could corrupt > said registers. > > To avoid the problem let's run all non-blocking modesets on an > ordered workqueue. We still put page flips etc. to system_unbound_wq > allowing page flips on one pipe to execute in parallel with page flips > or a modeset on a another pipe (assuming no known state is shared > between them, at which point they would have been added to the same > atomic commit and serialized that way). > > Blocking modesets are already serialized with each other by > connection_mutex, and thus are safe. To serialize them with > non-blocking modesets we just flush the workqueue before executing > blocking modesets. > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Fixes: 94f050246b42 ("drm/i915: nonblocking commit") > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> This patch won't really fix it, you could still have a blocking modeset in parallel to a nonblocking one. What would really be needed to fix those instances here?
On Mon, Nov 13, 2017 at 03:36:58PM +0100, Maarten Lankhorst wrote: > Op 13-11-17 om 14:36 schreef Ville Syrjala: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > We have plenty of global registers and whatnot programmed without > > any further locking by the modeset code. Currently non-bocking > > modesets are allowed to execute in parallel which could corrupt > > said registers. > > > > To avoid the problem let's run all non-blocking modesets on an > > ordered workqueue. We still put page flips etc. to system_unbound_wq > > allowing page flips on one pipe to execute in parallel with page flips > > or a modeset on a another pipe (assuming no known state is shared > > between them, at which point they would have been added to the same > > atomic commit and serialized that way). > > > > Blocking modesets are already serialized with each other by > > connection_mutex, and thus are safe. To serialize them with > > non-blocking modesets we just flush the workqueue before executing > > blocking modesets. > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Fixes: 94f050246b42 ("drm/i915: nonblocking commit") > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > This patch won't really fix it, you could still have a blocking modeset in parallel to a nonblocking one. Hence the flush_workqueue(). > What would really be needed to fix those instances here? Someone needs to review the entire modeset code and come up with a fix for all the cases where we are frobbing some global state.
On Mon, Nov 13, 2017 at 04:44:08PM +0200, Ville Syrjälä wrote: > On Mon, Nov 13, 2017 at 03:36:58PM +0100, Maarten Lankhorst wrote: > > Op 13-11-17 om 14:36 schreef Ville Syrjala: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > We have plenty of global registers and whatnot programmed without > > > any further locking by the modeset code. Currently non-bocking > > > modesets are allowed to execute in parallel which could corrupt > > > said registers. > > > > > > To avoid the problem let's run all non-blocking modesets on an > > > ordered workqueue. We still put page flips etc. to system_unbound_wq > > > allowing page flips on one pipe to execute in parallel with page flips > > > or a modeset on a another pipe (assuming no known state is shared > > > between them, at which point they would have been added to the same > > > atomic commit and serialized that way). > > > > > > Blocking modesets are already serialized with each other by > > > connection_mutex, and thus are safe. To serialize them with > > > non-blocking modesets we just flush the workqueue before executing > > > blocking modesets. > > > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Fixes: 94f050246b42 ("drm/i915: nonblocking commit") > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > This patch won't really fix it, you could still have a blocking modeset in parallel to a nonblocking one. > > Hence the flush_workqueue(). > > > What would really be needed to fix those instances here? > > Someone needs to review the entire modeset code and come up with a fix > for all the cases where we are frobbing some global state. So what do we want to do here? 1) someone volunteering to find all the broken code and fix it? 2) go with my ordered wq? 3) someone want to come up with some kind of modeset mutex? 4) revert nonblocking modesets?
Quoting Ville Syrjala (2017-11-13 13:36:22) > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > We have plenty of global registers and whatnot programmed without > any further locking by the modeset code. Currently non-bocking > modesets are allowed to execute in parallel which could corrupt > said registers. > > To avoid the problem let's run all non-blocking modesets on an > ordered workqueue. We still put page flips etc. to system_unbound_wq > allowing page flips on one pipe to execute in parallel with page flips > or a modeset on a another pipe (assuming no known state is shared > between them, at which point they would have been added to the same > atomic commit and serialized that way). > > Blocking modesets are already serialized with each other by > connection_mutex, and thus are safe. To serialize them with > non-blocking modesets we just flush the workqueue before executing > blocking modesets. I did something very similar in my patches for tracking concurrent modesets using plane granularity. I used a fence to impose a barrier for each level of registers (plane, crtc, global) and tracking the dependencies/ordering via those barrier fences. In effect, it gives the same barrier as switching between an ordered-wq, but I feel using fences should be easier to extend down to finer granularity. (What once was an easy patch is no more :-( -Chris
On Mon, Nov 13, 2017 at 3:36 PM, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: > Op 13-11-17 om 14:36 schreef Ville Syrjala: >> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >> >> We have plenty of global registers and whatnot programmed without >> any further locking by the modeset code. Currently non-bocking >> modesets are allowed to execute in parallel which could corrupt >> said registers. >> >> To avoid the problem let's run all non-blocking modesets on an >> ordered workqueue. We still put page flips etc. to system_unbound_wq >> allowing page flips on one pipe to execute in parallel with page flips >> or a modeset on a another pipe (assuming no known state is shared >> between them, at which point they would have been added to the same >> atomic commit and serialized that way). >> >> Blocking modesets are already serialized with each other by >> connection_mutex, and thus are safe. To serialize them with >> non-blocking modesets we just flush the workqueue before executing >> blocking modesets. >> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Fixes: 94f050246b42 ("drm/i915: nonblocking commit") >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > This patch won't really fix it, you could still have a blocking modeset in parallel to a nonblocking one. What would really be needed to fix those instances here? The idea was that anything touching anything global would grap all crtc state, and then we'd track those using the drm_crtc_commit stuff. Putting everything onto one queue also doesn't work because they're meant to somewhat overlap (plane cleanup is in the same work, but should/can overlap with the next update). Imo the right fix is to make sure we do add all the crtc states everywhere we touch something global. And if that doesn't scale, then modeset objects to track those bits. -Daniel
On Wed, Nov 22, 2017 at 1:25 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > On Mon, Nov 13, 2017 at 3:36 PM, Maarten Lankhorst > <maarten.lankhorst@linux.intel.com> wrote: >> Op 13-11-17 om 14:36 schreef Ville Syrjala: >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> >>> We have plenty of global registers and whatnot programmed without >>> any further locking by the modeset code. Currently non-bocking >>> modesets are allowed to execute in parallel which could corrupt >>> said registers. >>> >>> To avoid the problem let's run all non-blocking modesets on an >>> ordered workqueue. We still put page flips etc. to system_unbound_wq >>> allowing page flips on one pipe to execute in parallel with page flips >>> or a modeset on a another pipe (assuming no known state is shared >>> between them, at which point they would have been added to the same >>> atomic commit and serialized that way). >>> >>> Blocking modesets are already serialized with each other by >>> connection_mutex, and thus are safe. To serialize them with >>> non-blocking modesets we just flush the workqueue before executing >>> blocking modesets. >>> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> Fixes: 94f050246b42 ("drm/i915: nonblocking commit") >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> This patch won't really fix it, you could still have a blocking modeset in parallel to a nonblocking one. What would really be needed to fix those instances here? > > The idea was that anything touching anything global would grap all > crtc state, and then we'd track those using the drm_crtc_commit stuff. > Putting everything onto one queue also doesn't work because they're > meant to somewhat overlap (plane cleanup is in the same work, but > should/can overlap with the next update). > > Imo the right fix is to make sure we do add all the crtc states > everywhere we touch something global. And if that doesn't scale, then > modeset objects to track those bits. Backtracking a lot here: What kind of global registers do you mean here? I have a bit the feeling that in a bunch of cases the problem would then also be that it's not really atomic when it matters how we interleave the updates ... -Daniel
On Wed, Nov 22, 2017 at 01:35:39PM +0100, Daniel Vetter wrote: > On Wed, Nov 22, 2017 at 1:25 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > On Mon, Nov 13, 2017 at 3:36 PM, Maarten Lankhorst > > <maarten.lankhorst@linux.intel.com> wrote: > >> Op 13-11-17 om 14:36 schreef Ville Syrjala: > >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> > >>> > >>> We have plenty of global registers and whatnot programmed without > >>> any further locking by the modeset code. Currently non-bocking > >>> modesets are allowed to execute in parallel which could corrupt > >>> said registers. > >>> > >>> To avoid the problem let's run all non-blocking modesets on an > >>> ordered workqueue. We still put page flips etc. to system_unbound_wq > >>> allowing page flips on one pipe to execute in parallel with page flips > >>> or a modeset on a another pipe (assuming no known state is shared > >>> between them, at which point they would have been added to the same > >>> atomic commit and serialized that way). > >>> > >>> Blocking modesets are already serialized with each other by > >>> connection_mutex, and thus are safe. To serialize them with > >>> non-blocking modesets we just flush the workqueue before executing > >>> blocking modesets. > >>> > >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>> Fixes: 94f050246b42 ("drm/i915: nonblocking commit") > >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> This patch won't really fix it, you could still have a blocking modeset in parallel to a nonblocking one. What would really be needed to fix those instances here? > > > > The idea was that anything touching anything global would grap all > > crtc state, and then we'd track those using the drm_crtc_commit stuff. > > Putting everything onto one queue also doesn't work because they're > > meant to somewhat overlap (plane cleanup is in the same work, but > > should/can overlap with the next update). > > > > Imo the right fix is to make sure we do add all the crtc states > > everywhere we touch something global. And if that doesn't scale, then > > modeset objects to track those bits. > > Backtracking a lot here: What kind of global registers do you mean > here? I have a bit the feeling that in a bunch of cases the problem > would then also be that it's not really atomic when it matters how we > interleave the updates ... There are at least various global clock related registers we're frobbing in DDI and PCH paths. And those are just the ones I've come across by accident. The problem is that no one has even tried to go throgh the code looking for these sorts of problems.
On Wed, Nov 22, 2017 at 02:56:10PM +0200, Ville Syrjälä wrote: > On Wed, Nov 22, 2017 at 01:35:39PM +0100, Daniel Vetter wrote: > > On Wed, Nov 22, 2017 at 1:25 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > On Mon, Nov 13, 2017 at 3:36 PM, Maarten Lankhorst > > > <maarten.lankhorst@linux.intel.com> wrote: > > >> Op 13-11-17 om 14:36 schreef Ville Syrjala: > > >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > >>> > > >>> We have plenty of global registers and whatnot programmed without > > >>> any further locking by the modeset code. Currently non-bocking > > >>> modesets are allowed to execute in parallel which could corrupt > > >>> said registers. > > >>> > > >>> To avoid the problem let's run all non-blocking modesets on an > > >>> ordered workqueue. We still put page flips etc. to system_unbound_wq > > >>> allowing page flips on one pipe to execute in parallel with page flips > > >>> or a modeset on a another pipe (assuming no known state is shared > > >>> between them, at which point they would have been added to the same > > >>> atomic commit and serialized that way). > > >>> > > >>> Blocking modesets are already serialized with each other by > > >>> connection_mutex, and thus are safe. To serialize them with > > >>> non-blocking modesets we just flush the workqueue before executing > > >>> blocking modesets. > > >>> > > >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > >>> Fixes: 94f050246b42 ("drm/i915: nonblocking commit") > > >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > >> This patch won't really fix it, you could still have a blocking modeset in parallel to a nonblocking one. What would really be needed to fix those instances here? > > > > > > The idea was that anything touching anything global would grap all > > > crtc state, and then we'd track those using the drm_crtc_commit stuff. > > > Putting everything onto one queue also doesn't work because they're > > > meant to somewhat overlap (plane cleanup is in the same work, but > > > should/can overlap with the next update). > > > > > > Imo the right fix is to make sure we do add all the crtc states > > > everywhere we touch something global. And if that doesn't scale, then > > > modeset objects to track those bits. > > > > Backtracking a lot here: What kind of global registers do you mean > > here? I have a bit the feeling that in a bunch of cases the problem > > would then also be that it's not really atomic when it matters how we > > interleave the updates ... > > There are at least various global clock related registers we're frobbing > in DDI and PCH paths. And those are just the ones I've come across by > accident. The problem is that no one has even tried to go throgh the > code looking for these sorts of problems. Hm ... The thing I'm worried about is that we do have some global shared state not yet extracted properly. But then even for the shared state we do track we don't sync correctly on updates to it. I guess long term we need to beef up the private_state stuff to automagically take care of that. Either way needs some duct-tape interim, and I think the ordered queue is probably the least invasive, and least likely to be a roadblock for whatever we pick in the future. Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Op 23-11-17 om 09:11 schreef Daniel Vetter: > On Wed, Nov 22, 2017 at 02:56:10PM +0200, Ville Syrjälä wrote: >> On Wed, Nov 22, 2017 at 01:35:39PM +0100, Daniel Vetter wrote: >>> On Wed, Nov 22, 2017 at 1:25 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >>>> On Mon, Nov 13, 2017 at 3:36 PM, Maarten Lankhorst >>>> <maarten.lankhorst@linux.intel.com> wrote: >>>>> Op 13-11-17 om 14:36 schreef Ville Syrjala: >>>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >>>>>> >>>>>> We have plenty of global registers and whatnot programmed without >>>>>> any further locking by the modeset code. Currently non-bocking >>>>>> modesets are allowed to execute in parallel which could corrupt >>>>>> said registers. >>>>>> >>>>>> To avoid the problem let's run all non-blocking modesets on an >>>>>> ordered workqueue. We still put page flips etc. to system_unbound_wq >>>>>> allowing page flips on one pipe to execute in parallel with page flips >>>>>> or a modeset on a another pipe (assuming no known state is shared >>>>>> between them, at which point they would have been added to the same >>>>>> atomic commit and serialized that way). >>>>>> >>>>>> Blocking modesets are already serialized with each other by >>>>>> connection_mutex, and thus are safe. To serialize them with >>>>>> non-blocking modesets we just flush the workqueue before executing >>>>>> blocking modesets. >>>>>> >>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>>>> Fixes: 94f050246b42 ("drm/i915: nonblocking commit") >>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >>>>> This patch won't really fix it, you could still have a blocking modeset in parallel to a nonblocking one. What would really be needed to fix those instances here? >>>> The idea was that anything touching anything global would grap all >>>> crtc state, and then we'd track those using the drm_crtc_commit stuff. >>>> Putting everything onto one queue also doesn't work because they're >>>> meant to somewhat overlap (plane cleanup is in the same work, but >>>> should/can overlap with the next update). >>>> >>>> Imo the right fix is to make sure we do add all the crtc states >>>> everywhere we touch something global. And if that doesn't scale, then >>>> modeset objects to track those bits. >>> Backtracking a lot here: What kind of global registers do you mean >>> here? I have a bit the feeling that in a bunch of cases the problem >>> would then also be that it's not really atomic when it matters how we >>> interleave the updates ... >> There are at least various global clock related registers we're frobbing >> in DDI and PCH paths. And those are just the ones I've come across by >> accident. The problem is that no one has even tried to go throgh the >> code looking for these sorts of problems. > Hm ... > > The thing I'm worried about is that we do have some global shared state > not yet extracted properly. But then even for the shared state we do track > we don't sync correctly on updates to it. I guess long term we need to > beef up the private_state stuff to automagically take care of that. > > Either way needs some duct-tape interim, and I think the ordered queue is > probably the least invasive, and least likely to be a roadblock for > whatever we pick in the future. > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> I'm also worried about the opposite, we worry deeply about tracked shared resources currently because they may be used concurrently, for new features we should assume it's used in parallel and we might not care as much with this workaround. That said, on newer platforms watermark allocations always lock out all concurrent CRTC updates anyway, so this will mostly affect older platforms.. Meh, Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 40012b6daea2..0ff528e56e88 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2378,6 +2378,9 @@ struct drm_i915_private { */ struct workqueue_struct *wq; + /* ordered wq for modesets */ + struct workqueue_struct *modeset_wq; + /* Display functions */ struct drm_i915_display_funcs display; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 5c7540f3f5dc..f9b79a145db1 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12540,11 +12540,15 @@ static int intel_atomic_commit(struct drm_device *dev, INIT_WORK(&state->commit_work, intel_atomic_commit_work); i915_sw_fence_commit(&intel_state->commit_ready); - if (nonblock) + if (nonblock && intel_state->modeset) { + queue_work(dev_priv->modeset_wq, &state->commit_work); + } else if (nonblock) { queue_work(system_unbound_wq, &state->commit_work); - else + } else { + if (intel_state->modeset) + flush_workqueue(dev_priv->modeset_wq); intel_atomic_commit_tail(state); - + } return 0; } @@ -14475,6 +14479,8 @@ int intel_modeset_init(struct drm_device *dev) enum pipe pipe; struct intel_crtc *crtc; + dev_priv->modeset_wq = alloc_ordered_workqueue("i915_modeset", 0); + drm_mode_config_init(dev); dev->mode_config.min_width = 0; @@ -15273,6 +15279,8 @@ void intel_modeset_cleanup(struct drm_device *dev) intel_cleanup_gt_powersave(dev_priv); intel_teardown_gmbus(dev_priv); + + destroy_workqueue(dev_priv->modeset_wq); } void intel_connector_attach_encoder(struct intel_connector *connector,