- 29 Jan, 2016 23 commits
-
-
Paulo Zanoni authored
Our dmesg messages started being misleading after we converted to the enable+activate model: we always print "Disabling FBC", even when we're just deactivating it. So, for example, when I boot my machine and do "dmesg | grep -i fbc", I see: [drm:intel_fbc_enable] Enabling FBC on pipe A [drm:set_no_fbc_reason] Disabling FBC: framebuffer not tiled or fenced but then, if I read the debugfs file, I will see: $ sudo cat i915_fbc_status FBC enabled Compressing: yes so we can conclude that dmesg is misleading, since FBC is actually enabled. What happened is that we deactivated FBC due to fbcon not being tiled, but when we silently reactivated it when the display manager started. We don't print activation messages since there may be way too many of these operations per second during normal desktop usage. One possible solution would be to change set_no_fbc_reason to correctly differentiate between disable and deactivation, but we removed support from printing activation/deactivation messages in the past because they were too frequent. So instead of doing this, let's just not print anything on dmesg, and leave the debugfs file if the user needs to investigate something. We already print when we enable and disable FBC anyway on a given pipe, so this should already help triaging bugs. Reviewed-by:
Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Signed-off-by:
Paulo Zanoni <paulo.r.zanoni@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1453210558-7875-22-git-send-email-paulo.r.zanoni@intel.com
-
Paulo Zanoni authored
During FBC invalidation, don't call intel_fbc_deactivate if it's not enabled. This doesn't fix any bug, but helps making the interface saner. Reviewed-by:
-
Paulo Zanoni authored
Move intel_fbc_enable to a place where it is called regardless of the "modeset" variable, and make sure intel_fbc_enable can be called multiple times without intel_fbc_disable being called. Reviewed-by:
-
Paulo Zanoni authored
Instead of duplicating the calls for every platform, let's just put them in the correct places inside intel_atomic_commit. This will also make it easier for us to move the enable call in order to support fasbtoot. Reviewed-by:
-
Paulo Zanoni authored
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:
-
Paulo Zanoni authored
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:
-
Paulo Zanoni authored
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:
-
Paulo Zanoni authored
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:
Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch> Reviewed-by:
-
Paulo Zanoni authored
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:
-
Paulo Zanoni authored
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:
-
Paulo Zanoni authored
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:
-
Paulo Zanoni authored
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:
-
Paulo Zanoni authored
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:
-
Paulo Zanoni authored
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:
-
Paulo Zanoni authored
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:
-
Paulo Zanoni authored
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:
-
Paulo Zanoni authored
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:
-
Paulo Zanoni authored
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:
-
Paulo Zanoni authored
Make our enable/activate checking model more explicit, especially since we now have intel_fbc_can_activate(). Reviewed-by:
-
Paulo Zanoni authored
Extract all the code that checks if the FBC configuration is valid to its own function, making __intel_fbc_update() much simpler. Reviewed-by:
-
Paulo Zanoni authored
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:Rodrigo Vivi <rodrigo.vivi@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1453406585-10233-1-git-send-email-paulo.r.zanoni@intel.com
-
Mat Martineau authored
No functional change Signed-off-by:
Mat 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:
Jani Nikula <jani.nikula@intel.com>
-
Gerd Hoffmann authored
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:
Bjørn Mork <bjorn@mork.no> Signed-off-by:
Gerd Hoffmann <kraxel@redhat.com> Tested-by:
Bruno Wolff III <bruno@wolff.to> Cc: drm-intel-fixes@lists.freedesktop.org Signed-off-by:
-
- 28 Jan, 2016 12 commits
-
-
Ville Syrjälä authored
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:
Ville 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:
-
Ville Syrjälä authored
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:
-
Ville Syrjälä authored
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:
-
Ville Syrjälä authored
intel_rotate_fb_obj_pages() doens't need the entire gtt view, just the rotation info suffices. Signed-off-by:
-
Ville Syrjälä authored
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:
-
Ville Syrjälä authored
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:
-
Dave Gordon authored
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:
Dave Gordon <david.s.gordon@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Reviewed-by:
Tvrtko Ursulin <tvrtko.ursulin@intel.com> Signed-off-by:
-
Tvrtko Ursulin authored
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:
Chris 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
-
Tvrtko Ursulin authored
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:
-
Tvrtko Ursulin authored
Will simplify the following fix and sounds logical. v2: Add some whitespace to separate logic better. (Chris Wilson) Signed-off-by:
-
Tvrtko Ursulin authored
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:
-
Tvrtko Ursulin authored
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:
-
- 27 Jan, 2016 5 commits
-
-
Imre Deak authored
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:
Imre Deak <imre.deak@intel.com> Reviewed-by:
David Weinehall <david.weinehall@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1453209992-25995-8-git-send-email-imre.deak@intel.com
-
Imre Deak authored
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:
-
Imre Deak authored
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:
-
Imre Deak authored
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:
-
Imre Deak authored
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:
-
