[PATCH] drm/i915: Put all non-blocking modesets onto an ordered wq

[PATCH] drm/i915: Put all non-blocking modesets onto an ordered wq

[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>
---
 drivers/gpu/drm/i915/i915_drv.h      |  3 +++
 drivers/gpu/drm/i915/intel_display.c | 14 +++++++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

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,
-- 
2.13.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Put all non-blocking modesets onto an ordered wq

[Patchwork]

== Series Details ==

Series: drm/i915: Put all non-blocking modesets onto an ordered wq
URL   : https://patchwork.freedesktop.org/series/33712/
State : success

== Summary ==

Series 33712v1 drm/i915: Put all non-blocking modesets onto an ordered wq
https://patchwork.freedesktop.org/api/1.0/series/33712/revisions/1/mbox/

Test chamelium:
        Subgroup dp-crc-fast:
                fail       -> PASS       (fi-kbl-7500u) fdo#102514
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                pass       -> INCOMPLETE (fi-kbl-7560u) fdo#102846

fdo#102514 https://bugs.freedesktop.org/show_bug.cgi?id=102514
fdo#102846 https://bugs.freedesktop.org/show_bug.cgi?id=102846

fi-bdw-5557u     total:285  pass:263  dwarn:0   dfail:1   fail:0   skip:20 
fi-bdw-gvtdvm    total:285  pass:260  dwarn:0   dfail:1   fail:0   skip:23 
fi-blb-e6850     total:285  pass:218  dwarn:1   dfail:1   fail:0   skip:64 
fi-bsw-n3050     total:285  pass:238  dwarn:0   dfail:1   fail:0   skip:45 
fi-bwr-2160      total:285  pass:178  dwarn:0   dfail:1   fail:0   skip:105
fi-bxt-dsi       total:285  pass:254  dwarn:0   dfail:1   fail:0   skip:29 
fi-bxt-j4205     total:285  pass:255  dwarn:0   dfail:1   fail:0   skip:28 
fi-byt-j1900     total:285  pass:249  dwarn:0   dfail:1   fail:0   skip:34 
fi-byt-n2820     total:285  pass:245  dwarn:0   dfail:1   fail:0   skip:38 
fi-elk-e7500     total:285  pass:224  dwarn:0   dfail:1   fail:0   skip:59 
fi-gdg-551       total:285  pass:174  dwarn:0   dfail:1   fail:1   skip:108
fi-glk-1         total:285  pass:256  dwarn:0   dfail:1   fail:0   skip:27 
fi-hsw-4770      total:285  pass:257  dwarn:0   dfail:1   fail:0   skip:26 
fi-hsw-4770r     total:285  pass:257  dwarn:0   dfail:1   fail:0   skip:26 
fi-ilk-650       total:285  pass:223  dwarn:0   dfail:1   fail:0   skip:60 
fi-ivb-3520m     total:285  pass:255  dwarn:0   dfail:1   fail:0   skip:28 
fi-ivb-3770      total:285  pass:255  dwarn:0   dfail:1   fail:0   skip:28 
fi-kbl-7500u     total:285  pass:259  dwarn:1   dfail:1   fail:0   skip:23 
fi-kbl-7560u     total:245  pass:228  dwarn:0   dfail:0   fail:0   skip:16 
fi-kbl-7567u     total:285  pass:264  dwarn:0   dfail:1   fail:0   skip:19 
fi-kbl-r         total:285  pass:257  dwarn:0   dfail:1   fail:0   skip:26 
fi-pnv-d510      total:285  pass:217  dwarn:1   dfail:1   fail:0   skip:65 
fi-skl-6260u     total:285  pass:264  dwarn:0   dfail:1   fail:0   skip:19 
fi-skl-6600u     total:285  pass:257  dwarn:0   dfail:1   fail:0   skip:26 
fi-skl-6700hq    total:285  pass:258  dwarn:0   dfail:1   fail:0   skip:25 
fi-skl-6700k     total:285  pass:260  dwarn:0   dfail:1   fail:0   skip:23 
fi-skl-6770hq    total:285  pass:264  dwarn:0   dfail:1   fail:0   skip:19 
fi-skl-gvtdvm    total:285  pass:261  dwarn:0   dfail:1   fail:0   skip:22 
fi-snb-2520m     total:246  pass:212  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:285  pass:244  dwarn:0   dfail:1   fail:0   skip:39 
Blacklisted hosts:
fi-cfl-s         total:285  pass:252  dwarn:0   dfail:1   fail:0   skip:31 
fi-cnl-y failed to connect after reboot
fi-glk-dsi failed to connect after reboot

9a81c142768eb620a24b02a136716a44c304563f drm-tip: 2017y-11m-13d-11h-44m-54s UTC integration manifest
0375ef4c926b drm/i915: Put all non-blocking modesets onto an ordered wq

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7091/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Put all non-blocking modesets onto an ordered wq

[Maarten Lankhorst]

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?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Put all non-blocking modesets onto an ordered wq

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: warning for drm/i915: Put all non-blocking modesets onto an ordered wq

[Patchwork]

== Series Details ==

Series: drm/i915: Put all non-blocking modesets onto an ordered wq
URL   : https://patchwork.freedesktop.org/series/33712/
State : warning

== Summary ==

Test drv_module_reload:
        Subgroup basic-no-display:
                dmesg-warn -> PASS       (shard-hsw) fdo#102707 +1
Test kms_busy:
        Subgroup extended-pageflip-modeset-hang-oldfb-render-b:
                pass       -> SKIP       (shard-hsw)
Test pm_rpm:
        Subgroup legacy-planes-dpms:
                pass       -> SKIP       (shard-hsw)
Test kms_draw_crc:
        Subgroup draw-method-xrgb8888-pwrite-xtiled:
                pass       -> SKIP       (shard-hsw)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> FAIL       (shard-hsw) fdo#100368
        Subgroup flip-vs-expired-vblank-interruptible:
                pass       -> FAIL       (shard-hsw) fdo#102887

fdo#102707 https://bugs.freedesktop.org/show_bug.cgi?id=102707
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887

shard-hsw        total:2584 pass:1466 dwarn:3   dfail:2   fail:11  skip:1102 time:9523s
Blacklisted hosts:
shard-apl        total:2544 pass:1599 dwarn:3   dfail:2   fail:21  skip:918 time:13032s
shard-kbl        total:2509 pass:1656 dwarn:9   dfail:5   fail:26  skip:811 time:10280s
shard-snb        total:2551 pass:1191 dwarn:2   dfail:2   fail:12  skip:1343 time:7615s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7091/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Put all non-blocking modesets onto an ordered wq

[Ville Syrjälä]

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?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Put all non-blocking modesets onto an ordered wq

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Put all non-blocking modesets onto an ordered wq

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Put all non-blocking modesets onto an ordered wq

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Put all non-blocking modesets onto an ordered wq

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Put all non-blocking modesets onto an ordered wq

[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>
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Put all non-blocking modesets onto an ordered wq

[Maarten Lankhorst]

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>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Put all non-blocking modesets onto an ordered wq

[Patchwork]

== Series Details ==

Series: drm/i915: Put all non-blocking modesets onto an ordered wq
URL   : https://patchwork.freedesktop.org/series/33712/
State : success

== Summary ==

Series 33712v1 drm/i915: Put all non-blocking modesets onto an ordered wq
https://patchwork.freedesktop.org/api/1.0/series/33712/revisions/1/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                dmesg-warn -> PASS       (fi-elk-e7500) fdo#103989 +1
                dmesg-warn -> PASS       (fi-bdw-gvtdvm) fdo#103938 +1

fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
fdo#103938 https://bugs.freedesktop.org/show_bug.cgi?id=103938

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:445s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:445s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:385s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:523s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:282s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:505s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:504s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:484s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:476s
fi-elk-e7500     total:224  pass:163  dwarn:15  dfail:0   fail:0   skip:45 
fi-gdg-551       total:288  pass:178  dwarn:1   dfail:0   fail:1   skip:108 time:269s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:542s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:367s
fi-hsw-4770r     total:288  pass:224  dwarn:0   dfail:0   fail:0   skip:64  time:258s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:392s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:487s
fi-ivb-3770      total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:450s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:489s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:527s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:478s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:534s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:589s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:448s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:539s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:569s
fi-skl-6700k     total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:517s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:494s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:446s
fi-snb-2520m     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:549s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:416s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:605s
fi-cnl-y         total:281  pass:255  dwarn:0   dfail:0   fail:0   skip:25 
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:488s

0d0fe916f52ad8f05dddab384ae7c90bb62ebac4 drm-tip: 2017y-12m-05d-14h-52m-17s UTC integration manifest
8d1afc151847 drm/i915: Put all non-blocking modesets onto an ordered wq

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7411/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx