This opens the possibility of implementing nicer schemes to choose the
CRTC, such as checking the amount of stolen memory available, or
choosing the best pipe on platforms that don't die FBC to pipe or
plane A.

This code was written for another refactor that I ended up discarding,
so I don't actually need it, but I figured this patch would be an
improvement on its own so I kept it on the series.
Reviewed-by: NMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: NPaulo Zanoni <paulo.r.zanoni@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453210558-7875-18-git-send-email-paulo.r.zanoni@intel.com

We already have a dev variable, there's no need to access state->dev.
Also, I plan to add another dev_priv user here, so declare one for the
current user.
Reviewed-by: NMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: NPaulo Zanoni <paulo.r.zanoni@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453210558-7875-17-git-send-email-paulo.r.zanoni@intel.com

Older FBC platforms have this restriction where FBC can't be enabled
if multiple pipes are enabled. In the current code, we disable FBC
before the second pipe becomes visible.

One of the problems with this code is that the current
multiple_pipes_ok() implementation just iterates through all CRTCs
looking at their states, but it doesn't make sure that the state
locks are grabbed. It also can't just grab the locks for every CRTC
since this would kill one of the biggest advantages of atomic
modesetting.

After the recent FBC changes, we now have the appropriate locks for
the given CRTC, so we can just try to maintain the state of each CRTC
and update it once intel_fbc_pre_update is called.

As a last note, I don't have gen 2/3 machines to test this code. My
current plan is to enable FBC on just the newer platforms, so this
patch is just an attempt to get the gen 2/3 code at least looking
sane, so if one day someone decide to fix FBC on these platforms, they
may have less work to do.

Not-tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com> (only on HSW+)
Reviewed-by: NMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: NPaulo Zanoni <paulo.r.zanoni@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453210558-7875-16-git-send-email-paulo.r.zanoni@intel.com

Just to be sure nothing will survive a module unload. We need to do
this after the unlock in order to make sure the function won't get
stuck trying to grab the lock we already own while we wait for it to
finish.
Reported-by: NReported-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: NMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: NPaulo Zanoni <paulo.r.zanoni@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453210558-7875-15-git-send-email-paulo.r.zanoni@intel.com

Instead of:
 - intel_fbc_disable_crtc(crtc)
 - intel_fbc_disable(dev_priv)
we now have:
 - intel_fbc_disable(crtc)
 - intel_fbc_global_disable(dev_priv)

This is because all the other functions that take a CRTC are called
 - intel_fbc_something(crtc)
Instead of:
 - intel_fbc_something_crtc(crtc)

And I also hope that the word "global" is going to help make it more
explicit that "global" is the unusual case, not the opposite.
Reported-by: NDaniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: NMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: NPaulo Zanoni <paulo.r.zanoni@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453210558-7875-14-git-send-email-paulo.r.zanoni@intel.com

With the addition and usage of intel_fbc_pre_update,
intel_fbc_deactivate is not used anymore outside intel_fbc.c, so kill
the exported function and rename __intel_fbc_deactivate.
Reviewed-by: NMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: NPaulo Zanoni <paulo.r.zanoni@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453210558-7875-13-git-send-email-paulo.r.zanoni@intel.com

We'll now call intel_fbc_pre_update instead of intel_fbc_deactivate
during atomic commits. This will continue to guarantee that we
deactivate FBC and it will also update the state checking structures
at the correct time. Then, later, at the point where we were calling
intel_fbc_update, we'll only need to call intel_fbc_post_update.

Also add the proper warnings in case we don't have the appropriate
locks. Daniel mentioned the warnings will have to be removed for async
commits, but let's keep them here while we can.
Reviewed-by: NMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: NPaulo Zanoni <paulo.r.zanoni@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453210558-7875-12-git-send-email-paulo.r.zanoni@intel.com

So now pre_update will be responsible for unconditionally deactivating
FBC and updating the state cache, while post_update will be
responsible for checking if it can be enabled, then enabling it.

This is one more step into proper locking.

Notice that intel_fbc_flush now calls post_update directly. The FBC
flush can only happen for drawing operations - since we explicitly
ignore the flips -, so the FBC state is not expected to have changed
at this point. With this we can just run post_update, which will make
sure we won't deactivate+reactivate FBC as would be the case now if we
called pre_update + post_update.
Reviewed-by: NMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: NPaulo Zanoni <paulo.r.zanoni@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453210558-7875-11-git-send-email-paulo.r.zanoni@intel.com

Per the new atomic locking rules, we need to cache the CRTC, plane and
FB state structures we use so we can access them later without needing
more locks. So do this.

Notice that there are some pieces of the FBC code that look at things
that are only computed during the modeset, so we can't just can't
precompute whether FBC can be activated during the update_state_cache
stage. We may be able to do this later.
Reviewed-by: NMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: NPaulo Zanoni <paulo.r.zanoni@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453210558-7875-10-git-send-email-paulo.r.zanoni@intel.com

We unconditionally disable/update FBC even during the page flip
IOCTLs, and an unconditional disable/update at every atomic commit
touching the primary plane shouldn't impact PC state residency
noticeably. Besides, the code that checks for rotation is a good hint
that we may be forgetting something else, so let's leave all the
decisions to intel_fbc.c, making the code much safer.

Once we have the code to properly make FBC enable/update decisions
based on atomic states, with proper locking, then we'll be able to
evaluate whether it will be worth trying to optimize the cases where a
disable isn't needed.

v2: Upstream moved and now our patch needs to remove dev_priv.
Reviewed-by: NMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: NPaulo Zanoni <paulo.r.zanoni@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453406837-10511-1-git-send-email-paulo.r.zanoni@intel.com

If frontbuffer_bits doesn't match the current frontbuffer, there's no
reason to recompress or update FBC.

There was a plan to make the FBC test suite catch this type of
problem, but it never got implemented due to being low priority.

While at it, also implement Ville's suggestion and use
plane->frontbuffer_bit instead of INTEL_FRONTBUFFER_PRIMARY.
Reviewed-by: NMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: NPaulo Zanoni <paulo.r.zanoni@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453210558-7875-8-git-send-email-paulo.r.zanoni@intel.com

Before this patch, page flips would call intel_frontbuffer_flip() and
intel_frontbuffer_flip_complete(), which would call intel_fbc_flush(),
which would call intel_fbc_update(). The problem is that drawing
operations also trigger intel_fbc_flush() calls, so it's not
guaranteed that we have the CRTC and FB locks grabbed when
intel_fbc_flush() happens, since the call trace may come from the
rendering path.

We're trying to make the FBC code grab the appropriate CRTC/FB locks,
so split the drawing and the flipping logic in order to achieve that
in later patches. So now the frontbuffer tracking code is just going
to be used for frontbuffer drawing, and intel_fbc_update() is going to
be used directly for actual page flips.

As a note, we don't need to call intel_fbc_flip() during the two
places where we call intel_frontbuffer_flip() since in one of them we
already have an intel_fbc_update() call, and in the other we have the
planes disabled.
Reviewed-by: NMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: NPaulo Zanoni <paulo.r.zanoni@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453210558-7875-7-git-send-email-paulo.r.zanoni@intel.com

We say "dev_priv->fbc.something" way too many times in our code while
we could be saying just "fbc->something" with a previous declaration
of fbc. This has been bothering me for a while but I didn't want to
patch it since I wanted to fix the real problems first. But as I add
more code I keep thinking about it, especially since it makes the code
easier to read and it can make us fit 80 columns easier, so let's just
do the change now.

While at it, also rename from i915_fbc to intel_fbc because the whole
FBC code uses intel_fbc.

v2: Rebase after the work_fn changes.
Reviewed-by: NMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: NPaulo Zanoni <paulo.r.zanoni@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453406763-10400-1-git-send-email-paulo.r.zanoni@intel.com

The early return inside __intel_fbc_update does not completely check
all the parameters that affect the FBC register values. For example,
we currently lack looking at crtc->adjusted_y (for the fence Y offset)
and all the parameters that affect the CFB size (for i8xx).

Instead of just adding the missing parameters to the check and hoping
that any changes to the fbc_activate functions also come with a
matching change to the __intel_fbc_update check, introduce a new
structure where we store these parameters and use the structure at the
fbc_activate function. Of course, it's still possible to access
everything from dev_priv in those functions, but IMHO the new code
will be harder to break.

v2: Rebase.
Reviewed-by: NMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: NPaulo Zanoni <paulo.r.zanoni@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453210558-7875-5-git-send-email-paulo.r.zanoni@intel.com

Make our enable/activate checking model more explicit, especially
since we now have intel_fbc_can_activate().
Reviewed-by: NMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: NPaulo Zanoni <paulo.r.zanoni@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453210558-7875-4-git-send-email-paulo.r.zanoni@intel.com

Extract all the code that checks if the FBC configuration is valid to
its own function, making __intel_fbc_update() much simpler.
Reviewed-by: NMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: NPaulo Zanoni <paulo.r.zanoni@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453210558-7875-3-git-send-email-paulo.r.zanoni@intel.com

Instead of waiting for 50ms, just wait until the next vblank, since
it's the minimum requirement. The whole infrastructure of FBC is based
on vblanks, so waiting for X vblanks instead of X milliseconds sounds
like the correct way to go. Besides, 50ms may be less than a vblank on
super slow modes that may or may not exist.

There are some small improvements in PC state residency (due to the
fact that we're now using 16ms for the common modes instead of 50ms),
but the biggest advantage is still the correctness of being
vblank-based instead of time-based.

v2:
  - Rebase after changing the patch order.
  - Update the commit message.
v3:
  - Fix bogus vblank_get() instead of vblank_count() (Ville).
  - Don't forget to call drm_crtc_vblank_{get,put} (Chris, Ville)
  - Adjust the performance details on the commit message.
v4:
  - Don't grab the FBC mutex just to grab the vblank (Maarten)
Signed-off-by: NPaulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: NRodrigo Vivi <rodrigo.vivi@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453406585-10233-1-git-send-email-paulo.r.zanoni@intel.com

No functional change
Signed-off-by: NMat Martineau <mathew.j.martineau@linux.intel.com>
Fixes: f8d03ea0 ("drm/i915: increase the tries for HDMI hotplug live status checking")
Link: http://patchwork.freedesktop.org/patch/msgid/1454023325-26265-1-git-send-email-mathew.j.martineau@linux.intel.comSigned-off-by: NJani Nikula <jani.nikula@intel.com>

The test for the qemu q35 south bridge added by commit
"39bfcd52 drm/i915: more virtual south bridge detection"
also matches on real hardware.  Having the check for
virtual systems last in the list is not enough to avoid
that ...

Refine the check by additionally verifying the pci
subsystem id to see whenever it *really* is qemu.

[ v2: fix subvendor tyops ]
Reported-and-tested-by: NBjørn Mork <bjorn@mork.no>
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
Tested-by: NBruno Wolff III <bruno@wolff.to>
Cc: drm-intel-fixes@lists.freedesktop.org
Signed-off-by: NJani Nikula <jani.nikula@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453719748-10944-1-git-send-email-kraxel@redhat.com

The fb_modifiers and cpp arguments passed to intel_tile_width() in
intel_fill_fb_ggtt_view() got accidentally swapped around. I'm pretty
sure I fixed this already, but could be I lost the fix accidentally
during some rebases or something. Anyway, fix it up for real.

Fixes: d9b3288e ("drm/i915: change intel_fill_fb_ggtt_view() to use the real tile size")
Signed-off-by: NVille Syrjälä <ville.syrjala@linux.intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453316739-13296-8-git-send-email-ville.syrjala@linux.intel.com
Testcase: igt/kms_rotation_crc/primary-rotation-90
Reviewed-by: NDaniel Vetter <daniel.vetter@ffwll.ch>
Cc: drm-intel-fixes@lists.freedesktop.org

We more or less randomly call the "bytes per pixel" value
'cpp', 'bytes_per_pixel', 'pixel_size', or even 'bpp'. Let's just pick
one and stick to it. I've chosen 'cpp'.
Signed-off-by: NVille Syrjälä <ville.syrjala@linux.intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453316739-13296-6-git-send-email-ville.syrjala@linux.intel.comReviewed-by: NDaniel Vetter <daniel.vetter@ffwll.ch>

Using 'unsigned long' for ggtt offsets doesn't make much sense. Use
'u32' instead since we've not yet seen a >4GiB ggtt.
Signed-off-by: NVille Syrjälä <ville.syrjala@linux.intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453316739-13296-5-git-send-email-ville.syrjala@linux.intel.comReviewed-by: NDaniel Vetter <daniel.vetter@ffwll.ch>

intel_rotate_fb_obj_pages() doens't need the entire gtt view, just the
rotation info suffices.
Signed-off-by: NVille Syrjälä <ville.syrjala@linux.intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453316739-13296-4-git-send-email-ville.syrjala@linux.intel.comReviewed-by: NDaniel Vetter <daniel.vetter@ffwll.ch>

Pass stride in addition to width and height to rotate_pages(). For now
width and stride are the same, but once framebuffer offsets enter the
scene that may no longer be the case.
Signed-off-by: NVille Syrjälä <ville.syrjala@linux.intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453316739-13296-3-git-send-email-ville.syrjala@linux.intel.comReviewed-by: NDaniel Vetter <daniel.vetter@ffwll.ch>

Also rename 'rotation_info' to 'rotated' to match the view type exactly,
this should avoid confusion which union members is valid for each view
type.
Signed-off-by: NVille Syrjälä <ville.syrjala@linux.intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453316739-13296-2-git-send-email-ville.syrjala@linux.intel.comReviewed-by: NDaniel Vetter <daniel.vetter@ffwll.ch>

In the error-handling paths of i915_gem_do_execbuffer() and
intel_crtc_page_flip(), the local pointer-to-request variables
were expected to be either valid pointers or NULL. Since

  26827088 drm/i915: simplify allocation of driver-internal requests

they could also be ERR_PTR() values, so the tests need to be
updated to accommodate this case.
Signed-off-by: NDave Gordon <david.s.gordon@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: NTvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: NTvrtko Ursulin <tvrtko.ursulin@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453978089-29127-1-git-send-email-tvrtko.ursulin@linux.intel.com

In GuC mode LRC pinning lifetime depends exclusively on the
request liftime. Since that is terminated by the seqno update
that opens up a race condition between GPU finishing writing
out the context image and the driver unpinning the LRC.

To extend the LRC lifetime we will employ a similar approach
to what legacy ringbuffer submission does.

We will start tracking the last submitted context per engine
and keep it pinned until it is replaced by another one.

Note that the driver unload path is a bit fragile and could
benefit greatly from efforts to unify the legacy and exec
list submission code paths.

At the moment i915_gem_context_fini has special casing for the
two which are potentialy not needed, and also depends on
i915_gem_cleanup_ringbuffer running before itself.

v2:
 * Move pinning into engine->emit_request and actually fix
   the reference/unreference logic. (Chris Wilson)

 * ring->dev can be NULL on driver unload so use a different
   route towards it.

v3:
 * Rebase.
 * Handle the reset path. (Chris Wilson)
 * Exclude default context from the pinning - it is impossible
   to get it right before default context special casing in
   general is eliminated.

v4:
 * Rebased & moved context tracking to
   intel_logical_ring_advance_and_submit.
Signed-off-by: NTvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: NChris Wilson <chris@chris-wilson.co.uk>
Issue: VIZ-4277
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Nick Hoath <nicholas.hoath@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453976997-25424-1-git-send-email-tvrtko.ursulin@linux.intel.com

Will enable cleaner implementation of a following fix and
easier code unification in the future.

Idea and code by Chris Wilson.

v2: Do not return before last_contexts on engines are unpinned.
Signed-off-by: NTvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: NChris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>

Will simplify the following fix and sounds logical.

v2: Add some whitespace to separate logic better. (Chris Wilson)
Signed-off-by: NTvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: NChris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Nick Hoath <nicholas.hoath@intel.com>

Previously intel_lr_context_(un)pin were operating on requests
which is in conflict with their names.

If we make them take a context and an engine, it makes the names
make more sense and it also makes future fixes possible.

v2: Rebase for default_context/kernel_context change.
Signed-off-by: NTvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: NChris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Nick Hoath <nicholas.hoath@intel.com>

This got broken in:

   commit de1add36
   Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
   Date:   Fri Jan 15 15:12:50 2016 +0000

       drm/i915: Decouple execbuf uAPI from internal implementation

BSD ring flags need to be shifted before they can be considered
indices into the ring array.

Reported by Zhipeng Gong.

v2: Simplify the code. (Chris Wilson)
Signed-off-by: NTvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Zhipeng Gong <zhipeng.gong@intel.com>
Reviewed-by: NChris Wilson <chris@chris-wilson.co.uk>
Link: http://patchwork.freedesktop.org/patch/msgid/1453902069-31353-1-git-send-email-tvrtko.ursulin@linux.intel.com
Testcase: igt/gem_exec_basic # bdw-gt3

The only device specific dependency of the stolen memory setup is the
MMIO mapping and the stolen memory size. Both are already available in
i915_gtt_init(), so move the stolen initialization to there. The
clean-up code for i915_gtt_init() is in i915_global_gtt_cleanup(), so
move the stolen memory clean-up code there too.

This will be needed by an upcoming patch that needs the details of the
memory we reserve, but the change is also part of our generic goal to
move the initialization of resources with no or little dependencies on
other device specific resources towards the beginning of the init
sequence.
Suggested-by: NChris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: NImre Deak <imre.deak@intel.com>
Reviewed-by: NDavid Weinehall <david.weinehall@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453209992-25995-8-git-send-email-imre.deak@intel.com

Move the MCHBAR setup right after the MMIO setup, since the two things
are logically related and the MCHBAR setup code doesn't depend on any
other device specific resource. We'll also need MCHBAR to be ready
earlier in an upcoming patch, so this is also a preparation for that.

Factor out the init/clean-up code to separate functions to make things
clearer in the i915_driver_load()/unload() functions.
Suggested-by: NChris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: NImre Deak <imre.deak@intel.com>
Reviewed-by: NDavid Weinehall <david.weinehall@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453209992-25995-7-git-send-email-imre.deak@intel.com

Workqueue initalization doesn't depend on any other device specific
resource, so move it close to the beginning, so we don't need to
consider them when thinking about dependencies for other resources.

Also factor out things to separate init/cleanup functions to make
i915_driver_load()/unload() clearer, atm it's somewhat difficult to
follow there in what order resources are inited/cleaned-up.
Suggested-by: NChris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: NImre Deak <imre.deak@intel.com>
Reviewed-by: NDavid Weinehall <david.weinehall@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453209992-25995-6-git-send-email-imre.deak@intel.com

Factor out common clean-up code for the GEM load time init function.
Also rename i915_gem_load() to i915_gem_load_init() to have a better
match with its new clean-up function.

No functional change.
Signed-off-by: NImre Deak <imre.deak@intel.com>
Reviewed-by: NDavid Weinehall <david.weinehall@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453209992-25995-5-git-send-email-imre.deak@intel.com

Factor out the common GEM shrinker clean-up code and call the shrinker
init function from the same function from where the corresponding
shrinker clean-up function is called. Also add sanity checking to the
shrinker and OOM registration calls.
Signed-off-by: NImre Deak <imre.deak@intel.com>
Reviewed-by: NDavid Weinehall <david.weinehall@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453209992-25995-4-git-send-email-imre.deak@intel.com

Clarify the name of the label on the error path, making it clear what's
being cleaned up. The kmem_cache_destroy() calls are NOPs on the
corresponding error path.
Signed-off-by: NImre Deak <imre.deak@intel.com>
Reviewed-by: NDavid Weinehall <david.weinehall@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453209992-25995-3-git-send-email-imre.deak@intel.com

commit ebae38d0
Author: Animesh Manna <animesh.manna@intel.com>
Date:   Wed Oct 28 23:58:55 2015 +0200

    drm/i915/gen9: csr_init after runtime pm enable

moved the DMC/CSR initialization later during driver loading, but didn't
move the cleanup earlier correspondingly during unloading. Fix this up.
Signed-off-by: NImre Deak <imre.deak@intel.com>
Reviewed-by: NDavid Weinehall <david.weinehall@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1453209992-25995-2-git-send-email-imre.deak@intel.com

This reverts commit 1803c035.

It seems to blow up on module unload due to a use-after free hitting a
BUG_ON with CONFIG_DEBUG_SG. Quoting from Tvrtko's mail:

"I've decoded the instructions and it pointed to SG_MAGIC checking:

488b8098010000  mov 0x198(%rax),%rax
ba21436587      mov $0x87654321,%edx
488b00          mov (%rax),%rax       *** CRASH

"Grep showed 0x87654321 is SG_MAGIC, so likely candidate for this code
pattern is:

static inline struct page *sg_page(struct scatterlist *sg)
{
    BUG_ON(sg->sg_magic != SG_MAGIC);
    BUG_ON(sg_is_chain(sg));
    return (struct page *)((sg)->page_link & ~0x3);
}

"Which would mean the offender is in intel_logical_ring_cleanup is most
likely:

...
    if (ring->status_page.obj) {
        kunmap(sg_page(ring->status_page.obj->pages->sgl));
        ring->status_page.obj = NULL;
    }
...

"I think that the i915_gem_context_fini will do a final unref on
dev_priv->kernel_context and then the ring buff has a copy which is
left dangling because:

    lrc_setup_hardware_status_page(ring,
        dev_priv->kernel_context->engine[ring->id].state);

and:

ring->status_page.obj = default_ctx_obj;

"Where default_ctx_obj == dev_priv->kernel_context->engine[ring->id].state
So indeed looks like the unload ordering is the trigger.  In fact it
is almost the same fragility wrt/ kernel_context hidden dependency I
expressed my worry about in an e-mail yesterday or so. It only shows
if CONFIG_DEBUG_SG is set, otherwise it accesses freed memory and
probably just survives."

This causes serious trouble in our CI system since it took out all
gen8+ machines. Not yet clear why this wasn't caught in pre-merge
testing.

Backtrace from CI, for posterity:

[  163.737836] general protection fault: 0000 [#1] PREEMPT SMP
[  163.737849] Modules linked in: ax88179_178a usbnet mii snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915(-) x86_pkg_temp_thermal intel_powerclamp coretemp crct10dif_pclmul crc32_pclmul ghash_clmulni_intel snd_hda_codec snd_hwdep snd_hda_core snd_pcm mei_me mei i2c_hid e1000e ptp pps_core [last unloaded: snd_hda_intel]
[  163.737902] CPU: 0 PID: 5812 Comm: rmmod Tainted: G     U  W       4.5.0-rc1-gfxbench+ #1
[  163.737911] Hardware name: System manufacturer System Product Name/Z170M-PLUS, BIOS 0505 11/16/2015
[  163.737920] task: ffff8800bb99cf80 ti: ffff88022ff2c000 task.ti: ffff88022ff2c000
[  163.737928] RIP: 0010:[<ffffffffa018f723>]  [<ffffffffa018f723>] intel_logical_ring_cleanup+0x83/0x100 [i915]
[  163.737969] RSP: 0018:ffff88022ff2fd30  EFLAGS: 00010282
[  163.737975] RAX: 6b6b6b6b6b6b6b6b RBX: ffff8800bb2f31b8 RCX: 0000000000000002
[  163.737982] RDX: 0000000087654321 RSI: 000000000000000d RDI: ffff8800bb2f31f0
[  163.737989] RBP: ffff88022ff2fd40 R08: 0000000000000000 R09: 0000000000000001
[  163.737996] R10: 0000000000000000 R11: 0000000000000000 R12: ffff8800bb2f0000
[  163.738003] R13: ffff8800bb2f8fc8 R14: ffff8800bb285668 R15: 000055af1ae55210
[  163.738010] FS:  00007f187014b700(0000) GS:ffff88023bc00000(0000) knlGS:0000000000000000
[  163.738021] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  163.738030] CR2: 0000558f84e4cbc8 CR3: 000000022cd55000 CR4: 00000000003406f0
[  163.738039] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  163.738048] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  163.738057] Stack:
[  163.738062]  ffff8800bb2f31b8 ffff8800bb2f0000 ffff88022ff2fd70 ffffffffa0180414
[  163.738079]  ffff8800bb2f0000 ffff8800bb285668 ffff8800bb2856c8 ffffffffa0242460
[  163.738094]  ffff88022ff2fd98 ffffffffa0202d30 ffff8800bb285668 ffff8800bb285668
[  163.738109] Call Trace:
[  163.738140]  [<ffffffffa0180414>] i915_gem_cleanup_engines+0x34/0x60 [i915]
[  163.738185]  [<ffffffffa0202d30>] i915_driver_unload+0x150/0x270 [i915]
[  163.738198]  [<ffffffff815100f4>] drm_dev_unregister+0x24/0xa0
[  163.738208]  [<ffffffff815106ce>] drm_put_dev+0x1e/0x60
[  163.738225]  [<ffffffffa01412a0>] i915_pci_remove+0x10/0x20 [i915]
[  163.738237]  [<ffffffff8143d9b4>] pci_device_remove+0x34/0xb0
[  163.738249]  [<ffffffff81533d15>] __device_release_driver+0x95/0x140
[  163.738259]  [<ffffffff81533eb6>] driver_detach+0xb6/0xc0
[  163.738268]  [<ffffffff81532de3>] bus_remove_driver+0x53/0xd0
[  163.738278]  [<ffffffff815348d7>] driver_unregister+0x27/0x50
[  163.738289]  [<ffffffff8143ca15>] pci_unregister_driver+0x25/0x70
[  163.738299]  [<ffffffff81511de4>] drm_pci_exit+0x74/0x90
[  163.738337]  [<ffffffffa02034a9>] i915_exit+0x20/0x1a5 [i915]
[  163.738349]  [<ffffffff8110400f>] SyS_delete_module+0x18f/0x1f0
[  163.738361]  [<ffffffff817b8a9b>] entry_SYSCALL_64_fastpath+0x16/0x73
[  163.738370] Code: ff d0 48 89 df e8 de a1 fd ff 48 8d 7b 38 e8 25 ab fd ff 48 8b 83 90 00 00 00 48 85 c0 74 25 48 8b 80 98 01 00 00 ba 21 43 65 87 <48> 8b 00 48 39 10 75 3c f6 40 08 01 75 38 48 c7 83 90 00 00 00
[  163.738459] RIP  [<ffffffffa018f723>] intel_logical_ring_cleanup+0x83/0x100 [i915]
[  163.738498]  RSP <ffff88022ff2fd30>
[  163.738507] ---[ end trace 68f69ce4740fa44f ]---

Cc: Nick Hoath <nicholas.hoath@intel.com>
Cc: Dave Gordon <david.s.gordon@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Reviewed-by: NMika Kuoppala <mika.kuoppala@linux.intel.com>
Tested-by: NMika Kuoppala <mika.kuoppala@linux.intel.com>
Signed-off-by: NDaniel Vetter <daniel.vetter@intel.com>

Swap the order of context & engine cleanup, so that contexts are cleaned
up first, and *then* engines. This is a more sensible order anyway, but
in particular has become necessary since the 'intel_ring_initialized()
must be simple and inline' patch, which now uses ring->dev as an
'initialised' flag, so it can now be NULL after engine teardown. This
in turn can cause a problem in the context code, which (used to) check
the ring->dev->struct_mutex -- causing a fault if ring->dev was NULL.

Also rename the cleanup function to reflect what it actually does
(cleanup engines, not a ringbuffer), and fix an annoying whitespace issue.

v2: Also make the fix in i915_load_modeset_init, not just in
    i915_driver_unload (Chris Wilson)
v3: Had extra stuff in it.
v4: Reverted extra stuff (so we're back to v2).
    Rebased and updated commentary above (Dave Gordon).
Signed-off-by: NNick Hoath <nicholas.hoath@intel.com>
Signed-off-by: NDave Gordon <david.s.gordon@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v2)
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Link: http://patchwork.freedesktop.org/patch/msgid/1453405067-32890-3-git-send-email-david.s.gordon@intel.comSigned-off-by: NDaniel Vetter <daniel.vetter@ffwll.ch>