On Fri, Jul 08, 2011 at 08:43:47PM +0100, Chris Wilson wrote:
> Oh dear, it was all looking too good. Works fine with just one output.
> Runs into a nasty race if you add a second and start unplugging things...
>
> The issue is that when unplugging an output, userspace sees this and
> appropriately reconfigures from an extended desktop to just using, for the
> sake of argument, the LVDS. As the desktop is changing size this requires
> a new framebuffer and so we begin to teardown the two crtcs and replace
> with just the one with the new fb. The first thing we do is disable the
> crtc connected to the unplugged display. This triggers an
> intel_update_fbc() which spots that it can now enable FBC on the current
> single crtc + old fb and promptly dispatches a delayed task to do so.
>
> The mode reconfiguration continues apace and we disable the other crtc and
> start to replace the plane's FB. This involves a couple of
> wait-for-vblanks, and so is quite slow. During this time we avoid taking
> the struct_mutex. Meanwhile, the delayed task kicks off on a second CPU
> grabs the struct_mutex and reconfigures the FBC registers. Apparently
> enabling FBC on a dead plane is an undefined operation. Hilarity ensues,
> followed by a swift reboot.
>
> Bumping to 250ms sufficiently delays the task to miss the race, but we
> can not foretell just how long any given crtc modeset will take. So what
> we need is to take the mode_config.lock in order to prevent concurrent
> access to the FBC registers and also to prevent the fb from disappearing
> beneath us:
I've just noticed that we seem to miss this plug to fix fbc issues. Care
to resend a proper patch in case we still need this?
Also, I we could try to also disable fbc when we detect frontbuffer
rendering (and generally abolish setting up the fence to detect such
writes), maybe that helps with our fbc troubles (if it's actually worth
it). Eugeni?
-Daniel
>
> ---
> drivers/gpu/drm/i915/intel_display.c | 8 +++++++-
> 1 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e375500..bec9e1d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1603,6 +1603,11 @@ static void intel_fbc_work_fn(struct work_struct *__work)
> struct drm_device *dev = work->crtc->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> + if (!mutex_trylock(&dev->mode_config.mutex)) {
> + schedule_delayed_work(&work->work, msecs_to_jiffies(100));
> + return;
> + }
> +
> mutex_lock(&dev->struct_mutex);
> if (work == dev_priv->fbc_work) {
> /* Double check that we haven't switched fb without cancelling
> @@ -1620,6 +1625,7 @@ static void intel_fbc_work_fn(struct work_struct *__work)
> dev_priv->fbc_work = NULL;
> }
> mutex_unlock(&dev->struct_mutex);
> + mutex_unlock(&dev->mode_config.mutex);
>
> kfree(work);
> }
> @@ -1684,7 +1690,7 @@ static void intel_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
> * and indeed performing the enable as a co-routine and not
> * waiting synchronously upon the vblank.
> */
> - schedule_delayed_work(&work->work, msecs_to_jiffies(50));
> + schedule_delayed_work(&work->work, msecs_to_jiffies(250));
> }
>
> void intel_disable_fbc(struct drm_device *dev)
> --
> 1.7.6
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48
