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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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