Re: [PATCH 0/3] drm/i915: Handle hanging during nonblocking modeset correctly.

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 30-01-17 om 09:17 schreef Daniel Vetter:
> On Fri, Jan 27, 2017 at 03:08:45PM +0000, Chris Wilson wrote:
>> On Fri, Jan 27, 2017 at 03:58:08PM +0100, Daniel Vetter wrote:
>>> On Fri, Jan 27, 2017 at 02:31:55PM +0000, Chris Wilson wrote:
>>>> On Fri, Jan 27, 2017 at 03:21:29PM +0100, Daniel Vetter wrote:
>>>>> On Fri, Jan 27, 2017 at 09:30:50AM +0000, Chris Wilson wrote:
>>>>>> On Thu, Jan 26, 2017 at 04:59:21PM +0100, Maarten Lankhorst wrote:
>>>>>>> When writing some testcases for nonblocking modesets. I found out that the
>>>>>>> infinite wait on the old fb was causing issues.
>>>>>> The crux of the issue here is the locked wait for old dependencies and
>>>>>> the inability to inject the intel_prepare_reset disabling of all planes.
>>>>>> There are a couple of locked waits on struct_mutex within the modeset
>>>>>> locks for intel_overlay and if we happen to be using the display plane
>>>>>> for the first time.
>>>>>>
>>>>>> The first I suggested solving using fences to track dependencies and
>>>>>> keep the order between atomic states. Cancelling the outstanding
>>>>>> modesets, replacing with a disable and then on restore jumping to the
>>>>>> final state look doable. It also requires avoiding the struct_mutex for
>>>>>> disabling, which is quite easy. To avoid the wait under struct_mutex,
>>>>>> we've talked about switching to mmio, but for starters we could move the
>>>>>> wait from inside intel_overlay into the fence for the atomic operation.
>>>>>> (But's that a little more surgery than we would like for intel_overlay I
>>>>>> guess - dig out Ville's patches for overlay planes?) And to prevent the
>>>>>> wait under struct_mutex for pin_to_display_plane, my plane is to move
>>>>>> that to an async fenced operation that is then naturally waited upon by
>>>>>> the atomic modeset.
>>>>> A bit more a hack, but a different idea, and I think hack for gen234.0 is
>>>>> ok:
>>>>>
>>>>> We complete all the requests before we start the hw reset with fence.error
>>>>> = -EIO. But we do this only when we need to get at the display locks. A
>>>>> slightly more elegant solution would be to trylock modeset locks, and if
>>>>> one of them fails (and only then) complete all requests with -EIO to get
>>>>> the concurrent modeset to proceed before we reset the hardware. That's
>>>>> essentially the logic we had before all the reworks, and it worked. But I
>>>>> didn't look at how scary that all would be to make it work again ...
>>>> The modeset lock may not just be waiting on our requests (even on pnv we
>>>> can expect that there are already users celebrating that pnv+nouveau
>>>> finally works ;) and that the display is not the only user/observer of
>>>> those requests. Using the requests to break the modeset lock just feels
>>>> like the wrong approach.
>>> It's a cycle, and we need to break it somewhere. Another option might be
>>> to break the cycle the same way we do it for gem locks: Wake up everyone
>>> and restart the modeset ioctl. Since the trouble only happens for
>>> synchronous modesets where we hold the locks while waiting for fences, we
>>> can also break out of that and restart. And I also don't think that would
>>> leak to other drivers, after all our gem locking restart dances also don't
>>> leak to other drivers - it's just our own driver's lock which are affected
>>> by these special wakupe semantics.
>> It's a queue of nonblocking modesets that we need to worry about, afaik.
>> Moving the wait for blocking modeset outside of modeset lock is easily
>> achievable (and avoiding the other waits under both the modeset + 
>> struct_mutex I have at least an idea for). So the challenge is how to
>> inject all-planes-off for gen3 and then allow the queue to continue again
>> afterwards.
> Hm right, I missed the nonblocking updates which don't take locks. But
> assuming we do the display reset for gpu resets as a full modeset (i.e.
> going through ->atomic_commit) it should still work out correctly:
>
> Starting state: gpu is hung, nonblocking modeset waiting for some requests
> to complete.
Missing one evil detail here, else things would have moved forward..

A unrelated thread performs a blocking commit, and holds all locks until the nonblocking modeset completes.
> 1. hangcheck kicks in, fires off reset work.
>
> 2. We complete all requests with fence.error = -EIO and wake up any
> waiters. That means no re-queueing for older platforms, but oh well.
>
> 3. We grab all the display locks. Nothing happens yet.
>
> 4. We reset the chip, display dies.
>
> 5. We run ->atomic_commit to restore things. This will also force the
> nonblocking commit worker to complete before this display restore touches
> anything.
>
> The only trouble I see is that the nonblocking worker can still touch the
> display block while we kill it, which isn't awesome. But we can fix that
> by waiting for all pending nonblocking commits in step 3 manually (without
> calling into atomic_commit), as long as we do step 2.
>
> So completing everything with EIO unconditionally still seems like the
> simplest option that actually works for pre-g4x ...
> -Daniel


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