[Intel-gfx] [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 at ffwll.ch> wrote:
>>>> On Mon, Nov 13, 2017 at 3:36 PM, Maarten Lankhorst
>>>> <maarten.lankhorst at linux.intel.com> wrote:
>>>>> Op 13-11-17 om 14:36 schreef Ville Syrjala:
>>>>>> From: Ville Syrjälä <ville.syrjala at 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 at ffwll.ch>
>>>>>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>>>>> Fixes: 94f050246b42 ("drm/i915: nonblocking commit")
>>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala at 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 at 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 at linux.intel.com>