Re: [Intel-gfx] [PATCH v2 9/9] drm/i915: Fix gen2 and hsw+ scanline counter
On Fri, May 16, 2014 at 11:03 AM, akash goel akash.go...@gmail.com wrote:
Sorry not aware of this specific difference in the starting value of
scanline counter for HSW+ ( gen 2), but implementation wise, patch looks
fine.
Reviewed-by: Akash Goel akash.go...@gmail.com
Don't have enough info about the initial scanline counter values for
HSW+ and gen2. Otherwise, you can add my r-b tag
Reviewed-by: Sourab Gupta sourabgu...@gmail.com
On Thu, May 15, 2014 at 10:53 PM, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
On gen2 the scanline counter behaves a bit differently from the
later generations. Instead of adding one to the raw scanline
counter value, we must subtract one.
On HSW/BDW the scanline counter requires a +2 adjustment on HDMI
outputs. DP outputs on the on the other require the typical +1
adjustment.
As the fixup we must apply to the hardware scanline counter
depends on several factors, compute the desired offset at modeset
time and tuck it away for when it's needed.
v2: Clarify HSW+ situation
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_irq.c | 14 ---
drivers/gpu/drm/i915/intel_display.c | 45
+++-
drivers/gpu/drm/i915/intel_drv.h | 2 ++
3 files changed, 51 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c
b/drivers/gpu/drm/i915/i915_irq.c
index bb9b061..80003b5 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -818,9 +818,9 @@ static int __intel_get_crtc_scanline(struct
intel_crtc *crtc)
struct drm_i915_private *dev_priv = dev-dev_private;
const struct drm_display_mode *mode = crtc-config.adjusted_mode;
enum pipe pipe = crtc-pipe;
- int vtotal = mode-crtc_vtotal;
- int position;
+ int position, vtotal;
+ vtotal = mode-crtc_vtotal;
if (mode-flags DRM_MODE_FLAG_INTERLACE)
vtotal /= 2;
@@ -830,14 +830,10 @@ static int __intel_get_crtc_scanline(struct
intel_crtc *crtc)
position = __raw_i915_read32(dev_priv, PIPEDSL(pipe))
DSL_LINEMASK_GEN3;
/*
-* Scanline counter increments at leading edge of hsync, and
-* it starts counting from vtotal-1 on the first active line.
-* That means the scanline counter value is always one less
-* than what we would expect. Ie. just after start of vblank,
-* which also occurs at start of hsync (on the last active line),
-* the scanline counter will read vblank_start-1.
+* See update_scanline_offset() for the details on the
+* scanline_offset adjustment.
*/
- return (position + 1) % vtotal;
+ return (position + crtc-scanline_offset) % vtotal;
}
static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 0f8f9bc..f7222d7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10164,6 +10164,44 @@ void ironlake_check_encoder_dotclock(const
struct intel_crtc_config *pipe_config
pipe_config-adjusted_mode.crtc_clock, dotclock);
}
+static void update_scanline_offset(struct intel_crtc *crtc)
+{
+ struct drm_device *dev = crtc-base.dev;
+
+ /*
+* The scanline counter increments at the leading edge of hsync.
+*
+* On most platforms it starts counting from vtotal-1 on the
+* first active line. That means the scanline counter value is
+* always one less than what we would expect. Ie. just after
+* start of vblank, which also occurs at start of hsync (on the
+* last active line), the scanline counter will read
vblank_start-1.
+*
+* On gen2 the scanline counter starts counting from 1 instead
+* of vtotal-1, so we have to subtract one (or rather add vtotal-1
+* to keep the value positive), instead of adding one.
+*
+* On HSW+ the behaviour of the scanline counter depends on the
output
+* type. For DP ports it behaves like most other platforms, but
on HDMI
+* there's an extra 1 line difference. So we need to add two
instead of
+* one to the value.
+*/
+ if (IS_GEN2(dev)) {
+ const struct drm_display_mode *mode =
crtc-config.adjusted_mode;
+ int vtotal;
+
+ vtotal = mode-crtc_vtotal;
+ if (mode-flags DRM_MODE_FLAG_INTERLACE)
+ vtotal /= 2;
+
+ crtc-scanline_offset = vtotal - 1;
+ } else if (HAS_DDI(dev)
+ intel_pipe_has_type(crtc-base, INTEL_OUTPUT_HDMI)) {
+ crtc-scanline_offset = 2;
+ } else
+ crtc-scanline_offset = 1;
+}
+
[Intel-gfx] [I-G-T][PATCH] Add panning test for primary plane.
Get CRCs of a full red and a full blue surface as reference.
Create a big framebuffer that is twice width and twice height as the
current display mode.
Fill the top left quarter with red, bottom right quarter with blue
Check the scanned out image with the CRTC at position (0, 0) of the
framebuffer and it should be the same CRC as the full red fb
Check the scanned out image with the CRTC at position (hdisplay,
vdisplay) and it should be the same CRC as the full blue fb
Signed-off-by: Lei Liu lei.a@intel.com
Signed-off-by: Yi Sun yi@intel.com
diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index fffad9f..5cdc48b 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -532,6 +532,13 @@ void igt_display_init(igt_display_t *display, int drm_fd)
display-outputs = calloc(display-n_outputs, sizeof(igt_output_t));
igt_assert(display-outputs);
+ /*
+* Be default display can scan out from the original position of the
frame buffer.
+* But we can change this position making display scan out with a
offset.
+*/
+ display-buffer_x = 0;
+ display-buffer_y = 0;
+
for (i = 0; i display-n_outputs; i++) {
igt_output_t *output = display-outputs[i];
@@ -830,13 +837,13 @@ static int igt_output_commit(igt_output_t *output)
igt_output_name(output),
pipe_name(output-config.pipe),
fb_id,
- 0, 0,
+ display-buffer_x, display-buffer_y,
mode-hdisplay, mode-vdisplay);
ret = drmModeSetCrtc(display-drm_fd,
crtc_id,
fb_id,
-0, 0, /* x, y */
+display-buffer_x,
display-buffer_y, /* x, y */
output-id,
1,
mode);
@@ -849,7 +856,7 @@ static int igt_output_commit(igt_output_t *output)
ret = drmModeSetCrtc(display-drm_fd,
crtc_id,
fb_id,
-0, 0, /* x, y */
+display-buffer_x,
display-buffer_y, /* x, y */
NULL, /* connectors */
0,/* n_connectors */
NULL /* mode */);
@@ -987,6 +994,26 @@ void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb
*fb)
plane-fb_changed = true;
}
+void igt_plane_set_fb_offset(igt_plane_t *plane, struct igt_fb *fb, int x, int
y)
+{
+ igt_pipe_t *pipe = plane-pipe;
+ igt_display_t *display = pipe-display;
+
+ (*display).buffer_x = x;
+ (*display).buffer_y = y;
+
+ LOG(display, %c.%d: plane_set_fb(%d)\n, pipe_name(pipe-pipe),
+ plane-index, fb ? fb-fb_id : 0);
+
+ plane-fb = fb;
+
+ if (plane-is_primary)
+ pipe-need_set_crtc = true;
+ else if (plane-is_cursor)
+ pipe-need_set_cursor = true;
+
+ plane-fb_changed = true;
+}
void igt_plane_set_position(igt_plane_t *plane, int x, int y)
{
igt_pipe_t *pipe = plane-pipe;
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 439a634..cb9370e 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -139,6 +139,8 @@ struct igt_display {
int log_shift;
int n_pipes;
int n_outputs;
+ int buffer_x;
+ int buffer_y;
unsigned long pipes_in_use;
igt_output_t *outputs;
igt_pipe_t pipes[I915_MAX_PIPES];
diff --git a/tests/kms_plane.c b/tests/kms_plane.c
index 6bdbca3..6fcdadb 100644
--- a/tests/kms_plane.c
+++ b/tests/kms_plane.c
@@ -34,6 +34,12 @@
#include igt_kms.h
typedef struct {
+ float red;
+ float green;
+ float blue;
+} colour_t;
+
+typedef struct {
int drm_fd;
igt_display_t display;
} data_t;
@@ -55,12 +61,19 @@ typedef struct {
igt_crc_t reference_crc;
} test_position_t;
+//colour_t colour;
+
+enum {
+ TEST_POSITION_FULLY_COVERED = 1 0,
+ TEST_POSITION_BUFFER_ORIGINAL,
+};
+
/*
* create a green fb with a black rectangle at (rect_x,rect_y) and of size
* (rect_w,rect_h)
*/
static void
-create_fb_for_mode__position(data_t *data, drmModeModeInfo *mode,
+create_fb_for_mode_position(data_t *data, drmModeModeInfo *mode,
double rect_x, double rect_y,
double rect_w, double rect_h,
struct igt_fb *fb /* out */)
@@ -84,13 +97,44 @@ create_fb_for_mode__position(data_t *data, drmModeModeInfo
*mode,
}
static void
-test_position_init(test_position_t *test,
Re: [Intel-gfx] [PATCH 2/4] drm/plane-helper: Add drm_primary_helper_check_update() (v2)
On Fri, May 16, 2014 at 7:25 AM, Lee, Chon Ming chon.ming@intel.com wrote: Cherryview display allow the primary plane to be position at any location similiar to sprite plane for certain port. So, this shouldn't need to check here. And the width/height doesn't need to cover the whole screen. In that case, wouldn't it make sense (at least when you want to expose those features) to *not* use primary plane helpers for that hw? IMHO, the primary plane helpers should be for traditional crtcs which do not have these features.. I don't have the usage model for that windowing the primary plane now. Let say a primary plane at the beginning is using in traditional way, but if it want to switch to window mode, it might not make sense to create another plane to handle this right? I think what Rob means is that for now we should just use the primary plane helpers everywhere for easier conversion. Then later on we can enable the primary plane position feature on chv. So doing things step-by-step. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PULL] drm-intel-fixes
Hi Dave - Intel fixes for regressions, black screens and hangs, for 3.15. BR, Jani. The following changes since commit 2a1235e53bed8fa111e1c1ee2e7d8d91efa71ebc: Merge branch 'drm-nouveau-next' of git://anongit.freedesktop.org/git/nouveau/linux-2.6 into drm-fixes (2014-05-07 09:06:21 +1000) are available in the git repository at: git://anongit.freedesktop.org/drm-intel tags/drm-intel-fixes-2014-05-16 for you to fetch changes up to e95a2f7509f5219177d6821a0a8754f93892ca56: drm/i915: Increase WM memory latency values on SNB (2014-05-15 14:10:11 +0300) Aaron Lu (1): drm/i915: restore backlight precision when converting from ACPI Chris Wilson (1): drm/i915: Use the first mode if there is no preferred mode in the EDID Egbert Eich (1): drm/i915/SDVO: For sysfs link put directory and target in correct order Jani Nikula (4): drm/i915: clean up VBT eDP link param decoding drm/i915: use lane count and link rate from VBT as minimums for eDP drm/i915/vlv: reset VLV media force wake request register drm/i915/dp: force eDP lane count to max available lanes on BDW Paulo Zanoni (1): drm/i915: consider the source max DP lane count too Ville Syrjälä (1): drm/i915: Increase WM memory latency values on SNB drivers/gpu/drm/i915/intel_bios.c | 52 +-- drivers/gpu/drm/i915/intel_dp.c | 55 +++-- drivers/gpu/drm/i915/intel_fbdev.c | 9 ++ drivers/gpu/drm/i915/intel_panel.c | 8 +++--- drivers/gpu/drm/i915/intel_pm.c | 40 +++ drivers/gpu/drm/i915/intel_sdvo.c | 4 +-- drivers/gpu/drm/i915/intel_uncore.c | 2 ++ 7 files changed, 141 insertions(+), 29 deletions(-) -- Jani Nikula, Intel Open Source Technology Center pgpWvcnhjqtkp.pgp Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/66] runtime pm for DPMS
On 04/30/2014 10:08 PM, Imre Deak wrote: On Fri, 2014-04-25 at 10:45 +0200, Daniel Vetter wrote: Ok, review assignements. Please complain if you don't have the bandwidth so that I can find someone else. On Thu, Apr 24, 2014 at 11:54 PM, Daniel Vetter daniel.vet...@ffwll.ch wrote: Daniel Vetter (66): drm/i915: Make encoder-mode_set callbacks optional drm/i915/dvo: Remove -mode_set callback drm/i915/tv: extract set_tv_mode_timings drm/i915/tv: extract set_color_conversion drm/i915/tv: De-magic device check drm/i915/tv: Rip out pipe-disabling nonsense from -mode_set drm/i915/tv: Remove -mode_set callback drm/i915/crt: Remove -mode_set callback drm/i915/sdvo: Remove -mode_set callback Removal of encoder-mode_set callbacks, part 1 Reviewer: Imre 1-9 look good to me: Reviewed-by: Imre Deak imre.d...@intel.com drm/i915/hdmi: Enable hdmi mode on g4x, too drm/i915: Track hdmi mode in the pipe config drm/i915/sdvo: Use pipe_config-limited_color_range consistently drm/i915: state readout and cross checking for limited_color_range drm/i915/sdvo: use config-has_hdmi_sink drm/i915: Simplify audio handling on DDI ports drm/i915: Track has_audio in the pipe config drm/i915/dp: Move port A pll setup to g4x_pre_enable_dp drm/i915/dp: Remove -mode_set callback drm/i915/hdmi: Remove redundant IS_VLV checks drm/i915/hdmi: Remove -mode_set callback Removal of the encoder-mode_set callbacks for hdmi/sdvo/dp with small interludes to move a bit of the hdmi/audio state into the pipe config. Reviewer: Naresh Kumar reviewed 10-20, looks good. Reviewed-by: Naresh Kumar Kachhi naresh.kumar.kac...@intel.com drm/i915/lvds: Remove -mode_set callback drm/i915/ddi: Remove -mode_set callback drm/i915/dsi: Remove -mode_set callback drm/i915: Stop calling encoder-mode_set Final removals of encoder-mode_set callbacks Reviewer: Imre drm/i915: Make -update_primary_plane infallible drm/i915: More cargo-culted locking for intel_update_fbc drm/i915: Sprinkle intel_edp_psr_update over crtc_enable/disable drm/i915: Inline set_base into crtc_mode_set drm/i915: Move fb pinning into __intel_set_mode Some shuffling to get the primary-fb handling out of crtc mode_set callbacks Reviewer: Akash Goel drm/i915: Shovel hw setup code out of i9xx_crtc_mode_set drm/i915: Move lowfreq_avail around a bit in ilk/hsw_crtc_mode_set drm/i915: Shovel hw setup code out of ilk_crtc_mode_set drm/i915: Shovel hw setup code out of hsw_crtc_mode_set drm/i915: Extract i9xx_set_pll_dividers drm/i915: Extract vlv_prepare_pll gmch pll moved out of crtc mode_set callbacks into -enable hooks Reviewer: Shobhit Kumar drm/i915: Only update shared dpll state when needed drm/i915: Extract intel_prepare_shared_dpll drm/i915: s/ironlake_/intel_ for the enable_share_dpll function Prep polish on the existing shared_dpll code Reviewer: Damien (same comment as below) drm/i915: Check hw state in assert_can_disable_lcpll drm/i915: Remove spll_refcount for hsw drm/i915: Clean up WRPLL/SPLL #defines drm/i915: Make intel_wait_for_pipe_off static drm/i915: Disable pipe before ports on ilk drm/i915: Pass port explicitly to intel_ddi_get_hw_state drm/i915: Unexport intel_ddi_connector_get_hw_state drm/i915: Move hsw_fdi_link_train into intel_crt.c drm/i915: Move pch fifo underrun report enabling to hsw_crt_pre_enable drm/i915: Move the SPLL enabling into hsw_crt_pre_enable drm/i915: Move lpt_pch_enable int hsw_crt_enable drm/i915: Move the pch fifo underrun handling into hsw_crt_disable drm/i915: Move lpt_disable_pch_transcoder into the hsw crt encoder drm/i915: Move pch fifo underrun report re-enabling into hsw_crt_post_disable drm/i915: Move the hsw fdi disabling into hsw_crt_post_disable drm/i915: Move SPLL disabling into hsw_crt_post_disable Create a new hsw-specific crt encoder which subsumes the entire fdi/pch handling on haswell. This has the nice upshot to make SPLL logically a port-private clock and so removes it from further considerations. Reviewer: Paulo drm/i915: Add a debugfs file for the shared dpll state drm/i915: Move ddi_pll_sel into the pipe config drm/i915: State readout and cross-checking for ddi_pll_sel drm/i915: Precompute static ddi_pll_sel values in encoders drm/i915: Basic shared dpll support for WRPLLs drm/i915: Document that the pll-mode_set hook is optional drm/i915: State readout support for WRPLLs drm/i915: -disable hook for WRPLLs drm/i915: -enable hook for WRPLLs drm/i915: Switch to common shared dpll framework for WRPLLs drm/i915: Only touch WRPLL hw state in enable/disable hooks Convert wrpll handling to the common shared_dpll framework. We need this since runtime pm for dpms requires us to separately track pll refernces from crtcs and active usage by crtcs Reviewer: Damien (maybe find someone from the vpg guys who do the pll stuff
Re: [Intel-gfx] [PATCH] i915: Add module option to support VGA arbiter on HD devices
On Thu, May 15, 2014 at 10:46:50PM -0600, Alex Williamson wrote: On Fri, 2014-05-16 at 00:50 +0200, Daniel Vetter wrote: On Thu, May 15, 2014 at 11:43 PM, Alex Williamson alex.william...@redhat.com wrote: I don't know what to do with this. It seems like a lot of wishful thinking that in the best case would drag on for years. Even if we get VGA arbitration out of Xorg, the bit about making the userspace VGA arbiter interface lie depending on current-comm sounds tricky and horrible. If we can lie to Xorg there, why don't we do that now? If we can't lie to Xorg now, then what deprecation event or detection of the caller is going to allow us to do so in the future? Well we wouldn't necessarily need to lie to X, but could instead look whether all the vga devices in a system are claimed by kms drivers. If that's the case the userspace doesn't have an awful lot of business touching the VGA registers and we could simply not obey a vga arb request from userspace. More advanced would be if we still obey it for those devices not claimed by kms drivers. So not really a need to key on current-comm. This is a requirement for me, I don't really care about KMS or Xorg, the use case I want to enable is binding a VGA device to vfio-pci so that it can be assigned to a guest virtual machine. This works on an unmodified kernel today so long as you don't have an Intel IGD in your system. If you do, we try to switch the VGA device, but it doesn't actually get switched because i915 opts-out of arbitration yet still claims VGA accesses. I get that its a requirement for you. I've also just detailed the solution for you above, but I'm not going to write the patch itself (since I can't test it really). We have two users of the vga-arb crap relevant here: - pci/pci-sysfs.c, used by X through libpciaccess - vfio/pci/vfio_pci_rdwr.c, for vfio-pci vga forwarding Make the former lie if all vga devices have kms drivers and the latter still work correctly. That will prevent X from going nasty if there are kms drivers, while still keeping vfio going. Then we re-re-revert the i915 to have proper vga-arb. Afaics no need for hacks, module options, special casing or breaking old userspace. And really, if there is a kms driver userspace has zero business touching the vga registers, so imo we don't lose an real functionality. You can always opt to not load kms drivers if you want userspace access. What am I missing? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/5] drm/i915/bdw: Implement a basic PM interrupt handler
On Fri, May 16, 2014 at 01:38:18AM +, O'Rourke, Tom wrote:
+static void gen8_disable_rps_interrupts(struct drm_device *dev) {
+struct drm_i915_private *dev_priv = dev-dev_private;
+
+I915_WRITE(GEN6_PMINTRMSK, 0x);
[TOR:] Please note that for Broadwell, bit 31 in GEN6_PMINTRMSK is not an
interrupt disable bit.
In drm/i915: Enable PM Interrupts target via Display Interface. this bit is
defined as:
+#define GEN8_PMINTR_REDIRECT_TO_NON_DISP (131)
Writing this bit here could have unintended consequences.
Hm, we seem to unconditionally set this bit on gen8 anyway. Still harmful?
Mika, Ville?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] tests/kms_sink_crc_basic: Basic test to verify Sink CRC debugfs.
On Thu, May 15, 2014 at 08:13:57PM -0400, Rodrigo Vivi wrote:
v2: rebase after a long time.
Signed-off-by: Rodrigo Vivi rodrigo.v...@gmail.com
Oh dear, the basic test was completely lost when we've merged the sink_crc
debugfs support :( Applied now, thanks for the patch.
One comment below.
---
tests/Android.mk | 1 +
tests/Makefile.sources | 1 +
tests/kms_sink_crc_basic.c | 201
+
3 files changed, 203 insertions(+)
create mode 100644 tests/kms_sink_crc_basic.c
diff --git a/tests/Android.mk b/tests/Android.mk
index 1cda9a5..b7bf51e 100644
--- a/tests/Android.mk
+++ b/tests/Android.mk
@@ -66,6 +66,7 @@ else
kms_pipe_crc_basic \
kms_fbc_crc \
kms_setmode \
+kms_sink_crc_basic \
gem_render_copy \
pm_lpsp \
kms_fence_pin_leak
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 4bdef36..c3d8720 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -68,6 +68,7 @@ TESTS_progs_M = \
kms_plane \
kms_render \
kms_setmode \
+ kms_sink_crc_basic \
pm_lpsp \
pm_pc8 \
pm_rps \
diff --git a/tests/kms_sink_crc_basic.c b/tests/kms_sink_crc_basic.c
new file mode 100644
index 000..924aada
--- /dev/null
+++ b/tests/kms_sink_crc_basic.c
@@ -0,0 +1,201 @@
+/*
+ * Copyright © 2013 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the Software),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#include errno.h
+#include limits.h
+#include stdbool.h
+#include stdio.h
+#include string.h
+
+#include drm_fourcc.h
+
+#include drmtest.h
+#include igt_debugfs.h
+#include igt_kms.h
+
+enum color {
+ WHITE,
+ BLACK,
+ NUM_COLORS,
+};
+
+typedef struct {
+ struct kmstest_connector_config config;
+ struct igt_fb fb;
+} connector_t;
+
+typedef struct {
+ int drm_fd;
+ drmModeRes *resources;
+} data_t;
+
+static void get_crc(char *crc) {
+ int ret;
+ FILE *file = fopen(/sys/kernel/debug/dri/0/i915_sink_crc_eDP1, r);
+ igt_require(file);
+
+ ret = fscanf(file, %s\n, crc);
+ igt_require(ret 0);
+
+ fclose(file);
+}
+
+static uint32_t create_fb(data_t *data,
+ int w, int h,
+ double r, double g, double b,
+ struct igt_fb *fb)
+{
+ cairo_t *cr;
+ uint32_t fb_id;
+
+ fb_id = igt_create_fb(data-drm_fd, w, h,
+ DRM_FORMAT_XRGB, false, fb);
+ igt_assert(fb_id);
+
+ cr = igt_get_cairo_ctx(data-drm_fd, fb);
+ igt_paint_color(cr, 0, 0, w, h, r, g, b);
+ igt_assert(cairo_status(cr) == 0);
+
+ return fb_id;
+}
+
+static bool
+connector_set_mode(data_t *data, connector_t *connector, drmModeModeInfo
*mode,
+enum color crtc_color)
+{
+ struct kmstest_connector_config *config = connector-config;
+ unsigned int fb_id;
+ int ret;
+
+ if (crtc_color == WHITE)
+ fb_id = create_fb(data, mode-hdisplay, mode-vdisplay,
+ 1.0, 1.0, 1.0, connector-fb);
+ else
+ fb_id = create_fb(data, mode-hdisplay, mode-vdisplay,
+ 0.0, 0.0, 0.0, connector-fb);
+ igt_assert(fb_id);
+
+ ret = drmModeSetCrtc(data-drm_fd,
+ config-crtc-crtc_id,
+ connector-fb.fb_id,
+ 0, 0, /* x, y */
+ config-connector-connector_id,
+ 1,
+ mode);
+ igt_assert(ret == 0);
+
+ return 0;
+}
+
+static void basic_sink_crc_check(data_t *data, uint32_t connector_id)
+{
+ connector_t connector;
+ int ret;
+ char ref_crc_white[12];
+ char ref_crc_black[12];
+ char crc_check[12];
+
Re: [Intel-gfx] [RFC 0/4] Cursor support with universal planes
On Thu, May 15, 2014 at 06:17:25PM -0700, Matt Roper wrote: Cursor planes are a bit trickier to support via the universal plane interface than primary planes were. Legacy cursor ioctls take handles to driver buffers directly whereas the universal plane API takes drm_framebuffer's that represent a buffer; due to this mismatch it isn't possible to implement a set of cursor helpers than provide free generic universal cursor support without driver changes as we did with primary planes. Drivers will need to be updated to receive cursor support via the universal plane API. If a driver tries to implement two interfaces for cursor support (legacy ioctl and universal planes), the reference counting can get very tricky/messy since we need to take into account userspace that may mix and match calls to both interfaces. To address that, patch #1 in this set causes legacy cursor ioctls to be implemented using a driver's universal plane support if the driver registers a primary plane. Calls to the legacy set_cursor ioctl will wrap the ^cursor plane I guess? -Daniel provided driver buffer in a drm_framebuffer and then pass that along to the universal plane interface. From a driver's point of view, this causes all cursor operations to arrive on the universal plane interface, regardless of which userspace API was used, which simplifies things greatly for the driver. Patch #2 makes some minor changes to ensure drivers can successfully register a cursor plane with the DRM core. Patch #3 does some minor i915 refactoring in preparation for the move to universal planes. Patch #4 transitions the i915 driver to universal planes. The changes here are intentionally minimal for ease of review. It's likely that we could perform further cleanup in the future to eliminate some of the cursor state tracked in intel_crtc (e.g., cursor_width/cursor_height) since that information can be also be derived from crtc-cursor-fb. Matt Roper (4): drm: Support legacy cursor ioctls via universal planes when possible drm: Allow drivers to register cursor planes with crtc drm/i915: Add intel_crtc_cursor_set_obj() to set cursor buffer drm/i915: Switch to unified plane cursor handling drivers/gpu/drm/drm_crtc.c | 113 - drivers/gpu/drm/i915/intel_display.c | 188 --- drivers/gpu/drm/i915/intel_drv.h | 1 - include/drm/drm_crtc.h | 6 +- 4 files changed, 267 insertions(+), 41 deletions(-) -- 1.8.5.1 ___ dri-devel mailing list dri-de...@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: Add support for Generic MIPI panel driver
Thanks Damien for your review On Thursday 15 May 2014 10:18 PM, Damien Lespiau wrote: On Mon, Apr 14, 2014 at 11:18:27AM +0530, Shobhit Kumar wrote: This driver makes use of the generic panel information from the VBT. Panel information is classified into two - panel configuration and panel power sequence which is unique to each panel. The generic driver uses the panel configuration and sequence parsed from VBT block #52 and #53 v2: Address review comments by Jani - Move all of the things in driver c file from header - Make all functions static - Make use of video/mipi_display.c instead of redefining - Null checks during sequence execution Signed-off-by: Shobhit Kumarshobhit.ku...@intel.com I've done a first past on this. Overall looks reasonable. I'm missing some documentation to double check the various LP-HS, HS-LP count and other magic around the clocks (send you a mail about it) before I can add my r-b tag. I've added a few tiny comments as well along the road. All look okay to me and Will push updated patch asap. There is one issue which I am struggling for now. If we have all these patches in, then disable sequence pipe off does not work and wait_for_pipe_off gives a warn dump but everything works. Its not this patch issue but DSI patches that are already merged. I know the fix is to actually disable port after disabling pipe and plane but doing that does not succeed enable in first attempt. Subsequent disable/enable works. Looking into that and should have a fix by next week on that. Regards Shobhit ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/5] drm/i915/bdw: Implement a basic PM interrupt handler
On Fri, May 16, 2014 at 11:09:35AM +0200, Daniel Vetter wrote:
On Fri, May 16, 2014 at 01:38:18AM +, O'Rourke, Tom wrote:
+static void gen8_disable_rps_interrupts(struct drm_device *dev) {
+ struct drm_i915_private *dev_priv = dev-dev_private;
+
+ I915_WRITE(GEN6_PMINTRMSK, 0x);
[TOR:] Please note that for Broadwell, bit 31 in GEN6_PMINTRMSK is not an
interrupt disable bit.
In drm/i915: Enable PM Interrupts target via Display Interface.
this bit is defined as:
+#define GEN8_PMINTR_REDIRECT_TO_NON_DISP (131)
Writing this bit here could have unintended consequences.
Hm, we seem to unconditionally set this bit on gen8 anyway. Still harmful?
Mika, Ville?
I was thinking that since we mask all the interrupts anyway it
doesn't really matter which way we set the bit here. But I'm
not actually sure what happens if there's an already pending
interrupt when we flip the bit. That is I have no idea if it
could raise an interrupt on the non-disp side or not.
So leaving the bit as zero here might be the safer option, and
at least it would be more consistent with the rest of the gen8
pm irq code. So if someone wants to make that change, my r-b
will still stand.
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 24/66] drm/i915: Stop calling encoder-mode_set
On Thu, Apr 24, 2014 at 11:55:00PM +0200, Daniel Vetter wrote:
All the callbacks are gone now.
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
Merged up to this patch here, thanks everyone for the reviews.
-Daniel
---
drivers/gpu/drm/i915/intel_display.c | 33 ++---
1 file changed, 2 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index f8ebe9b59746..dec4127a4738 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7202,35 +7202,6 @@ static bool haswell_get_pipe_config(struct intel_crtc
*crtc,
return true;
}
-static int intel_crtc_mode_set(struct drm_crtc *crtc,
-int x, int y,
-struct drm_framebuffer *fb)
-{
- struct drm_device *dev = crtc-dev;
- struct drm_i915_private *dev_priv = dev-dev_private;
- struct intel_encoder *encoder;
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- struct drm_display_mode *mode = intel_crtc-config.requested_mode;
- int ret;
-
- ret = dev_priv-display.crtc_mode_set(crtc, x, y, fb);
-
- if (ret != 0)
- return ret;
-
- for_each_encoder_on_crtc(dev, crtc, encoder) {
- DRM_DEBUG_KMS([ENCODER:%d:%s] set [MODE:%d:%s]\n,
- encoder-base.base.id,
- drm_get_encoder_name(encoder-base),
- mode-base.id, mode-name);
-
- if (encoder-mode_set)
- encoder-mode_set(encoder);
- }
-
- return 0;
-}
-
static struct {
int clock;
u32 config;
@@ -9994,8 +9965,8 @@ static int __intel_set_mode(struct drm_crtc *crtc,
* on the DPLL.
*/
for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
- ret = intel_crtc_mode_set(intel_crtc-base,
- x, y, fb);
+ ret = dev_priv-display.crtc_mode_set(intel_crtc-base,
+ x, y, fb);
if (ret)
goto done;
}
--
1.8.1.4
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Introduce mapping of user pages into video memory (userptr) ioctl
On Thu, Apr 17, 2014 at 03:18:48PM -0700, Volkin, Bradley D wrote:
+ union {
+ struct i915_gem_userptr {
Out of curiosity, what's the point of using both a union and a
struct here, given that everything is within the struct?
Because I stuff other things into this union in other patches, and
compacting our object using a union has been on the cards since
introducing subclasses of objects.
+ struct drm_i915_gem_object *obj = to_intel_bo(gem_obj);
+
+ if (obj-userptr.mm obj-userptr.mn == NULL)
+ return ERR_PTR(-EINVAL);
+
So this is basically saying we won't export an unsynchronized
userptr? I don't think we've documented this or the other
restrictions on unsynchronized userptrs (e.g. root only).
Yes. We cannot control the endpoint of a dmabuf and so we do not know if
the client would be able to control access to the bo accordingly (it
should after all appear to be a regular bo, except for the caveats about
snooped access and lack of CPU mmap support atm etc).
In fact, later I changed to this to obj-ops-export_dmabuf() callback
so that we can demote unsync'ed userptr for exporting.
+static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier
*_mn,
+ struct mm_struct *mm,
+ unsigned long start,
+ unsigned long end)
+{
+ struct i915_mmu_notifier *mn = container_of(_mn, struct
i915_mmu_notifier, mn);
+ struct interval_tree_node *it = NULL;
+ unsigned long serial = 0;
+
+ end--; /* interval ranges are inclusive, but invalidate range is
exclusive */
+ while (start end) {
+ struct drm_i915_gem_object *obj;
+
+ obj = NULL;
+ spin_lock(mn-lock);
+ if (serial == mn-serial)
+ it = interval_tree_iter_next(it, start, end);
+ else
+ it = interval_tree_iter_first(mn-objects, start, end);
Is the issue here just that items being removed from the tree could make 'it'
invalid on the next call to interval_tree_iter_next()? Or are we also worried
about items being added to the tree? If items can be added, we've been
advancing
'start', so I think there would be the potential for adding an item to the
portion of the invalidate range that we've already processed. Whether that
could happen given the locking or if the invalidation should apply to such an
object, I'm not sure.
Both. We have to guard against modifications to the interval-tree, both
by ourselves (through freeing a no longer active bo) and other threads.
Only if the interval-tree was untouched whilst unlocked can we jump
ahead. I presume that the mm will handle serialisation of invalidate
against new users, so that we don't have to continuously walk over the
same range dropping user pages. (i.e. someone won't be able to mmap the
same arena until the earlier munmap is complete)
+static int
+i915_mmu_notifier_add(struct i915_mmu_notifier *mmu,
+ struct i915_mmu_object *mn)
+{
+ struct interval_tree_node *it;
+ int ret;
+
+ /* Make sure we drop the final active reference (and thereby
+* remove the objects from the interval tree) before we do
+* the check for overlapping objects.
+*/
I assume this comment refers to the retire_requests call, in which
case I would move it down.
Once upon a time it was the comment for the function.
+ ret = i915_mutex_lock_interruptible(mmu-dev);
+ if (ret)
+ return ret;
+
+ i915_gem_retire_requests(mmu-dev);
+
+ /* Disallow overlapping userptr objects */
+ spin_lock(mmu-lock);
+ it = interval_tree_iter_first(mmu-objects,
+ mn-it.start, mn-it.last);
+ if (it) {
+ struct drm_i915_gem_object *obj;
+
+ /* We only need to check the first object as it either
+* is idle (and in use elsewhere) or we try again in order
+* to give time for the gup-worker to run and flush its
+* object references. Afterwards if we find another
+* object that is idle (and so referenced elsewhere)
+* we know that the overlap with an pinned object is
+* genuine.
+*/
I found this a bit confusing because it refers to the object being idle when
it's really a question of the worker executing or not. In my mind, the object
has the opposite idle/active state as the worker e.g. if the worker is active,
the object is idle w.r.t. being used by the GPU. Hence the earlier suggestion
r.e. renaming userptr.active.
/* We only need to check the first object in the range as it either
* has cancelled gup work queued and we need to return back to the user
* to give time for the gup-workers to flush their object references
* upon which the object will be
Re: [Intel-gfx] [PATCH 2/5] drm/i915/bdw: Implement a basic PM interrupt handler
On Fri, May 16, 2014 at 12:46:00PM +0300, Ville Syrjälä wrote:
On Fri, May 16, 2014 at 11:09:35AM +0200, Daniel Vetter wrote:
On Fri, May 16, 2014 at 01:38:18AM +, O'Rourke, Tom wrote:
+static void gen8_disable_rps_interrupts(struct drm_device *dev) {
+struct drm_i915_private *dev_priv = dev-dev_private;
+
+I915_WRITE(GEN6_PMINTRMSK, 0x);
[TOR:] Please note that for Broadwell, bit 31 in GEN6_PMINTRMSK is not an
interrupt disable bit.
In drm/i915: Enable PM Interrupts target via Display Interface.
this bit is defined as:
+#define GEN8_PMINTR_REDIRECT_TO_NON_DISP (131)
Writing this bit here could have unintended consequences.
Hm, we seem to unconditionally set this bit on gen8 anyway. Still harmful?
Mika, Ville?
I was thinking that since we mask all the interrupts anyway it
doesn't really matter which way we set the bit here. But I'm
not actually sure what happens if there's an already pending
interrupt when we flip the bit. That is I have no idea if it
could raise an interrupt on the non-disp side or not.
So leaving the bit as zero here might be the safer option, and
at least it would be more consistent with the rest of the gen8
pm irq code. So if someone wants to make that change, my r-b
will still stand.
Imo better as a follow-up patch with the Bspec quote above in the commit
message.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 05/11] drm/i915: Do not try to enable PSR when Panel doesn't suport it.
On Thu, May 15, 2014 at 08:13:04PM -0400, Rodrigo Vivi wrote: Also do not cache aux info. That info could be related to another panel. You should kill the bool sink_support then. There are other places that it used that could be invalid. I'm not sure though that we can cope with eDP panels being swapped at runtime. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/11] drm/i915: Force PSR exit by inactivating it.
On Thu, May 15, 2014 at 08:13:05PM -0400, Rodrigo Vivi wrote: The perfect solution for psr_exit is the hardware tracking the changes and doing the psr exit by itself. This scenario works for HSW and BDW with some environments like Gnome and Wayland. However there are many other scenarios that this isn't true. Mainly one right now is KDE users on HSW and BDW with PSR on. User would miss many screen updates. For instances any key typed could be seen only when mouse cursor is moved. So this patch introduces the ability of trigger PSR exit on kernel side on some common cases that. You know that userspace has been waiting for a PSR flag for over a year now so that it can use the more efficient rendering paths when it makes sense. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/11] drm/i915: Force PSR exit by inactivating it.
On Thu, May 15, 2014 at 08:13:05PM -0400, Rodrigo Vivi wrote: The perfect solution for psr_exit is the hardware tracking the changes and doing the psr exit by itself. This scenario works for HSW and BDW with some environments like Gnome and Wayland. However there are many other scenarios that this isn't true. Mainly one right now is KDE users on HSW and BDW with PSR on. User would miss many screen updates. For instances any key typed could be seen only when mouse cursor is moved. So this patch introduces the ability of trigger PSR exit on kernel side on some common cases that. Most of the cases are coverred by psr_exit at set_domain. The remaining cases are coverred by triggering it at set_domain, busy_ioctl, sw_finish and mark_busy. The downside here might be reducing the residency time on the cases this already work very wall like Gnome environment. But so far let's get focused on fixinge issues sio PSR couild be used for everybody and we could even get it enabled by default. Later we can add some alternatives to choose the level of PSR efficiency over boot flag of even over crtc property. What happened to the front buffer tracking? -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 4/4] drm/i915: Switch to unified plane cursor handling
On Thu, May 15, 2014 at 06:17:29PM -0700, Matt Roper wrote:
The DRM core will translate calls to legacy cursor ioctls into universal
cursor calls automatically, so there's no need to maintain the legacy
cursor support. This greatly simplifies the transition since we don't
have to handle reference counting differently depending on which cursor
interface was called.
The aim here is to transition to the universal plane interface with
minimal code change. There's a lot of cleanup that can be done (e.g.,
using state stored in crtc-cursor-fb rather than intel_crtc) that is
left to future patches.
Signed-off-by: Matt Roper matthew.d.ro...@intel.com
---
+static int
+intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
+ struct drm_framebuffer *fb, int crtc_x, int crtc_y,
+ unsigned int crtc_w, unsigned int crtc_h,
+ uint32_t src_x, uint32_t src_y,
+ uint32_t src_w, uint32_t src_h)
+{
+ struct drm_device *dev = crtc-dev;
+ struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+ struct drm_i915_gem_object *obj = intel_fb-obj;
+ struct drm_rect dest = {
+ /* integer pixels */
+ .x1 = crtc_x,
+ .y1 = crtc_y,
+ .x2 = crtc_x + crtc_w,
+ .y2 = crtc_y + crtc_h,
+ };
+ struct drm_rect src = {
+ /* 16.16 fixed point */
+ .x1 = src_x,
+ .y1 = src_y,
+ .x2 = src_x + src_w,
+ .y2 = src_y + src_h,
+ };
+ const struct drm_rect clip = {
+ /* integer pixels */
+ .x2 = intel_crtc-config.pipe_src_w,
+ .y2 = intel_crtc-config.pipe_src_h,
+ };
+ int hscale, vscale;
+ bool visible;
+
+ /* Check scaling */
+ hscale = drm_rect_calc_hscale(src, dest,
+ DRM_PLANE_HELPER_NO_SCALING,
+ DRM_PLANE_HELPER_NO_SCALING);
+ vscale = drm_rect_calc_vscale(src, dest,
+ DRM_PLANE_HELPER_NO_SCALING,
+ DRM_PLANE_HELPER_NO_SCALING);
+ if (hscale 0 || vscale 0) {
+ DRM_DEBUG_KMS(Invalid scaling of cursor plane\n);
+ return -ERANGE;
+ }
+
+ /* Check dimensions */
+ if (!((crtc_w == 64 crtc_h == 64) ||
+ (crtc_w == 128 crtc_h == 128 !IS_GEN2(dev)) ||
+ (crtc_w == 256 crtc_h == 256 !IS_GEN2(dev {
+ DRM_DEBUG_KMS(Cursor dimension not supported: %dx%d\n,
+ crtc_w, crtc_h);
+ return -EINVAL;
+ }
+
+ /* Clip to display; if no longer visible after clipping, disable */
+ visible = drm_rect_clip_scaled(src, dest, clip, hscale, vscale);
+
+ crtc-cursor_x = crtc_x;
+ crtc-cursor_y = crtc_y;
+ if (fb != crtc-cursor-fb) {
+ return intel_crtc_cursor_set_obj(crtc, visible ? obj : NULL,
+ crtc_w, crtc_h);
+ } else {
+ intel_crtc_update_cursor(crtc, true);
+ return 0;
+ }
Does this do the right thing for a cursor that is no longer visible, and
vice versa? It is not immediately clear how clipping now works with
intel_crtc_update_cursor().
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Be careful with non-disp bit in PMINTRMSK
Bit 31 in GEN6_PMINTRMSK is not an interrupt disable bit with gen8.
Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com
Signed-off-by: Mika Kuoppala mika.kuopp...@intel.com
---
drivers/gpu/drm/i915/intel_pm.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 34b0766..b59e8ab 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3293,7 +3293,7 @@ static void gen8_disable_rps_interrupts(struct drm_device
*dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
- I915_WRITE(GEN6_PMINTRMSK, 0x);
+ I915_WRITE(GEN6_PMINTRMSK, ~GEN8_PMINTR_REDIRECT_TO_NON_DISP);
I915_WRITE(GEN8_GT_IER(2), I915_READ(GEN8_GT_IER(2))
~dev_priv-pm_rps_events);
/* Complete PM interrupt masking here doesn't race with the rps work
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Gracefully handle obj not bound to GGTT in is_pin_display
-Original Message- From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com] Sent: Thursday, May 15, 2014 2:34 PM To: Mateo Lozano, Oscar Cc: Daniel Vetter; intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH] drm/i915: Gracefully handle obj not bound to GGTT in is_pin_display On Thu, May 15, 2014 at 01:14:54PM +, Mateo Lozano, Oscar wrote: But looking at the code a better way should be: 1. Create new bo, wrap it in a kms fb. 2. Slap busy load onto that bo, e.g. reapeatedly fill it with the blitter. 3. Enable evil interruptor (igt_fork_signal_helper). 4. Submit pageflip - Boom since the set_cache_level will block, get interrupted and - exit early with -EINTR. Given sufficient overkill in 2. this should be 100% reliable to reproduce. As soon as I execbuffer to the bo, it gets a vma for the GGTT vm: vm = ctx-vm; if (!USES_FULL_PPGTT(dev)) vm = dev_priv-gtt.base; ... /* Look up object handles */ ret = eb_lookup_vmas(eb, exec, args, vm, file); if (ret) goto err; And then it becomes impossible to reproduce the problem :( Is there any other trick to make set_cache_level fail? What if you make the pin_to_ggtt fail instead? Can you create an object that's too big for the ggtt? Thanks Ville: that worked like a charm ;) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/9] drm/i915: Add a small adjustment to the pixel counter on interlaced modes
On Thu, May 15, 2014 at 08:23:47PM +0530, akash goel wrote: Reviewed the patch it looks good. Just to confirm, this patch tries to address the case of a tiny window of transition, i.e. from the 1st field (last half line) to 2nd field (first half line). The hardware counts things so that one field ends up being one line taller than the other, ie. both half lines get accounted for the same field (as far as the pixel counter is concerned at least). So the total number of pixels in the fields is like this: field A = htotal * floor(vtotal/2) field B = htotal * ceil(vtotal/2) But when we compute the scanout position we assume that the total number of pixels is always 'htotal * floor(vtotal/2)'. So if we start with a number that is greater than that, the value wraps back to zero already before we reach the real 0. And then when we do reach the real zero, the scanout position would appear to jump backwards. So this patch eliminates that problem by making it appear that the scanout position stops moving when we're on that last line of the taller field. Occasionally non-moving (but still non-decreasing) scanout position seems like a lesser evil than one that jumps back and forth at times. And as stated, this more or less matches the scanline counter based scanout position since the scanline counter doesn't include the half lines. Reviewed-by: Akash Goel akash.go...@gmail.com On Tue, Apr 29, 2014 at 4:05 PM, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä ville.syrj...@linux.intel.com In interlaced modes, the pixel counter counts all pixels, so one field will have htotal more pixels. In order to avoid the reported position from jumping backwards when the pixel counter is beyond the length of the shorter field, just clamp the position the length of the shorter field. This matches how the scanline counter based position works since the scanline counter doesn't count the two half lines. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/i915_irq.c | 12 1 file changed, 12 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 7e0d577..64cd888 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -844,6 +844,18 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, vtotal *= htotal; /* +* In interlaced modes, the pixel counter counts all pixels, +* so one field will have htotal more pixels. In order to avoid +* the reported position from jumping backwards when the pixel +* counter is beyond the length of the shorter field, just +* clamp the position the length of the shorter field. This +* matches how the scanline counter based position works since +* the scanline counter doesn't count the two half lines. +*/ + if (position = vtotal) + position = vtotal - 1; + + /* * Start of vblank interrupt is triggered at start of hsync, * just prior to the first active line of vblank. However we * consider lines to start at the leading edge of horizontal -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/50] drm/i915: Plumb the context everywhere in the execbuffer path
On Fri, May 09, 2014 at 01:08:39PM +0100, oscar.ma...@intel.com wrote: From: Oscar Mateo oscar.ma...@intel.com The context are going to become very important pretty soon, and we need to be able to access them in a number of places inside the command submission path. The idea is that, when we need to place commands inside a ringbuffer or update the tail register, we know which context we are working with. We left intel_ring_begin() as a function macro to quickly adapt legacy code, an introduce intel_ringbuffer_begin() as the first of a set of new functions for ringbuffer manipulation (the rest will come in subsequent patches). No functional changes. v2: Do not set the context to NULL. In legacy code, set it to the default ring context (even if it doesn't get used later on). Won't rings be stored within the context? So the context should be derivable from which ring the operation is being issued on. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/50] drm/i915: Plumb the context everywhere in the execbuffer path
-Original Message- From: Chris Wilson [mailto:ch...@chris-wilson.co.uk] Sent: Friday, May 16, 2014 12:05 PM To: Mateo Lozano, Oscar Cc: intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 09/50] drm/i915: Plumb the context everywhere in the execbuffer path On Fri, May 09, 2014 at 01:08:39PM +0100, oscar.ma...@intel.com wrote: From: Oscar Mateo oscar.ma...@intel.com The context are going to become very important pretty soon, and we need to be able to access them in a number of places inside the command submission path. The idea is that, when we need to place commands inside a ringbuffer or update the tail register, we know which context we are working with. We left intel_ring_begin() as a function macro to quickly adapt legacy code, an introduce intel_ringbuffer_begin() as the first of a set of new functions for ringbuffer manipulation (the rest will come in subsequent patches). No functional changes. v2: Do not set the context to NULL. In legacy code, set it to the default ring context (even if it doesn't get used later on). Won't rings be stored within the context? So the context should be derivable from which ring the operation is being issued on. -Chris Rings (as in engine command streamer) still remain in dev_priv and there are only four/five of them. What we store in the context is the new ringbuffer structure (which stores the head, tail, etc...) and the ringbuffer backing object. Knowing only the ring is not enough to derive the context. - Oscar ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2] drm/i915: Gracefully handle obj not bound to GGTT in is_pin_display
From: Oscar Mateo oscar.ma...@intel.com
Otherwise, we do a NULL pointer dereference.
I've seen this happen while handling an error in
i915_gem_object_pin_to_display_plane():
If i915_gem_object_set_cache_level() fails, we call is_pin_display()
to handle the error. At this point, the object is still not pinned
to GGTT and maybe not even bound, so we have to check before we
dereference its GGTT vma.
v2: Chris Wilson says restoring the old value is easier, but that
is_pin_display is useful as a theory of operation. Take the solomonic
decision: at least this way is_pin_display is a little more robust
(until Chris can kill it off).
Issue: VIZ-3772
Signed-off-by: Oscar Mateo oscar.ma...@intel.com
---
drivers/gpu/drm/i915/i915_gem.c | 16 ++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 034ba2c..211b778 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3641,6 +3641,15 @@ unlock:
static bool is_pin_display(struct drm_i915_gem_object *obj)
{
+ struct i915_vma *vma;
+
+ if (list_empty(obj-vma_list))
+ return false;
+
+ vma = i915_gem_obj_to_ggtt(obj);
+ if (!vma)
+ return false;
+
/* There are 3 sources that pin objects:
* 1. The display engine (scanouts, sprites, cursors);
* 2. Reservations for execbuffer;
@@ -3652,7 +3661,7 @@ static bool is_pin_display(struct drm_i915_gem_object
*obj)
* subtracting the potential reference by the user, any pin_count
* remains, it must be due to another use by the display engine.
*/
- return i915_gem_obj_to_ggtt(obj)-pin_count - !!obj-user_pin_count;
+ return vma-pin_count - !!obj-user_pin_count;
}
/*
@@ -3666,6 +3675,7 @@ i915_gem_object_pin_to_display_plane(struct
drm_i915_gem_object *obj,
struct intel_ring_buffer *pipelined)
{
u32 old_read_domains, old_write_domain;
+ bool was_pin_display;
int ret;
if (pipelined != obj-ring) {
@@ -3677,6 +3687,7 @@ i915_gem_object_pin_to_display_plane(struct
drm_i915_gem_object *obj,
/* Mark the pin_display early so that we account for the
* display coherency whilst setting up the cache domains.
*/
+ was_pin_display = obj-pin_display;
obj-pin_display = true;
/* The display engine is not coherent with the LLC cache on gen6. As
@@ -3719,7 +3730,8 @@ i915_gem_object_pin_to_display_plane(struct
drm_i915_gem_object *obj,
return 0;
err_unpin_display:
- obj-pin_display = is_pin_display(obj);
+ WARN_ON(was_pin_display != is_pin_display(obj));
+ obj-pin_display = was_pin_display;
return ret;
}
--
1.9.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] tests/kms_flip: test a fb backed by a bo too big for its own good
From: Oscar Mateo oscar.ma...@intel.com
This is a review by igt test for a bug located in
i915_gem_object_pin_to_display_plane and fixed by:
commit 392013bdd4b6128795e33c84bd6d6d3fd66ff0a3
Author: Oscar Mateo oscar.ma...@intel.com
Date: Fri May 16 11:23:12 2014 +0100
drm/i915: Gracefully handle obj not bound to GGTT in is_pin_display
Otherwise, we do a NULL pointer dereference.
I've seen this happen while handling an error in
i915_gem_object_pin_to_display_plane():
If i915_gem_object_set_cache_level() fails, we call is_pin_display()
to handle the error. At this point, the object is still not pinned
to GGTT and maybe not even bound, so we have to check before we
dereference its GGTT vma.
v2: Chris Wilson says restoring the old value is easier, but that
is_pin_display is useful as a theory of operation. Take the solomonic
decision: at least this way is_pin_display is a little more robust
(until Chris can kill it off).
Signed-off-by: Oscar Mateo oscar.ma...@intel.com
---
tests/kms_flip.c | 102 ---
1 file changed, 97 insertions(+), 5 deletions(-)
diff --git a/tests/kms_flip.c b/tests/kms_flip.c
index bb4f71d..7296122 100644
--- a/tests/kms_flip.c
+++ b/tests/kms_flip.c
@@ -74,6 +74,7 @@
#define TEST_RPM (1 25)
#define TEST_SUSPEND (1 26)
#define TEST_TS_CONT (1 27)
+#define TEST_BO_TOOBIG (1 28)
#define EVENT_FLIP (1 0)
#define EVENT_VBLANK (1 1)
@@ -1208,6 +1209,84 @@ static void free_test_output(struct test_output *o)
}
}
+static int create_bigbo_for_fb(int fd, int width, int height, int bpp,
+ bool tiled, uint32_t *gem_handle_ret,
+ unsigned *size_ret, unsigned *stride_ret)
+{
+ uint32_t gem_handle;
+ int size, ret = 0;
+ unsigned stride;
+
+ if (tiled) {
+ int v;
+
+ v = width * bpp / 8;
+ for (stride = 512; stride v; stride *= 2)
+ ;
+
+ v = stride * height;
+ for (size = 1024*1024; size v; size *= 2)
+ ;
+ } else {
+ /* Scan-out has a 64 byte alignment restriction */
+ stride = (width * (bpp / 8) + 63) ~63;
+ size = stride * height;
+ }
+
+ /* 256 MB is usually the maximum mappable aperture,
+* (make it 4x times that to ensure failure) */
+ gem_handle = gem_create(fd, 4*256*1024*1024);
+
+ if (tiled)
+ ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X, stride);
+
+ *stride_ret = stride;
+ *size_ret = size;
+ *gem_handle_ret = gem_handle;
+
+ return ret;
+}
+
+static unsigned int igt_create_fb_with_bigbo(int fd, int width, int height,
+uint32_t format, bool tiled,
+struct igt_fb *fb)
+{
+ uint32_t handles[4];
+ uint32_t pitches[4];
+ uint32_t offsets[4];
+ uint32_t fb_id;
+ int bpp;
+ int ret;
+
+ memset(fb, 0, sizeof(*fb));
+
+ bpp = igt_drm_format_to_bpp(format);
+ ret = create_bigbo_for_fb(fd, width, height, bpp, tiled,
+ fb-gem_handle, fb-size, fb-stride);
+ if (ret 0)
+ return ret;
+
+ memset(handles, 0, sizeof(handles));
+ handles[0] = fb-gem_handle;
+ memset(pitches, 0, sizeof(pitches));
+ pitches[0] = fb-stride;
+ memset(offsets, 0, sizeof(offsets));
+ if (drmModeAddFB2(fd, width, height, format, handles, pitches,
+ offsets, fb_id, 0) 0) {
+ gem_close(fd, fb-gem_handle);
+
+ return 0;
+ }
+
+ fb-width = width;
+ fb-height = height;
+ fb-tiling = tiled;
+ fb-drm_format = format;
+ fb-fb_id = fb_id;
+
+ return fb_id;
+}
+
static void run_test_on_crtc_set(struct test_output *o, int *crtc_idxs,
int crtc_count, int duration_ms)
{
@@ -1252,9 +1331,15 @@ static void run_test_on_crtc_set(struct test_output *o,
int *crtc_idxs,
o-fb_ids[0] = igt_create_fb(drm_fd, o-fb_width, o-fb_height,
igt_bpp_depth_to_drm_format(o-bpp,
o-depth),
tiled, o-fb_info[0]);
- o-fb_ids[1] = igt_create_fb(drm_fd, o-fb_width, o-fb_height,
-igt_bpp_depth_to_drm_format(o-bpp,
o-depth),
-tiled, o-fb_info[1]);
+ if (o-flags TEST_BO_TOOBIG) {
+ o-fb_ids[1] = igt_create_fb_with_bigbo(drm_fd, o-fb_width,
o-fb_height,
+
igt_bpp_depth_to_drm_format(o-bpp, o-depth),
+tiled,
Re: [Intel-gfx] [PATCH 4/4] drm/i915: Add support for Generic MIPI panel driver
On Thu, May 15, 2014 at 05:48:57PM +0100, Damien Lespiau wrote:
+static struct gpio_table gtable[] = {
const
Please, disregard this comment. It's being written to to track GPIO
initialization.
--
Damien
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Gracefully handle obj not bound to GGTT in is_pin_display
On Fri, May 16, 2014 at 12:08:22PM +0100, oscar.ma...@intel.com wrote:
From: Oscar Mateo oscar.ma...@intel.com
Otherwise, we do a NULL pointer dereference.
I've seen this happen while handling an error in
i915_gem_object_pin_to_display_plane():
If i915_gem_object_set_cache_level() fails, we call is_pin_display()
to handle the error. At this point, the object is still not pinned
to GGTT and maybe not even bound, so we have to check before we
dereference its GGTT vma.
v2: Chris Wilson says restoring the old value is easier, but that
is_pin_display is useful as a theory of operation. Take the solomonic
decision: at least this way is_pin_display is a little more robust
(until Chris can kill it off).
Issue: VIZ-3772
I heard you wrote a testcase?
Signed-off-by: Oscar Mateo oscar.ma...@intel.com
---
drivers/gpu/drm/i915/i915_gem.c | 16 ++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 034ba2c..211b778 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3641,6 +3641,15 @@ unlock:
static bool is_pin_display(struct drm_i915_gem_object *obj)
{
+ struct i915_vma *vma;
+
+ if (list_empty(obj-vma_list))
+ return false;
Hmm, this is so that we don't trigger the WARN from inside
i915_gem_obj_to_ggtt(). I would say that means the WARN in the callee
has outlived its usefulness. Other callers WARN if they fail to find the
ggtt_vma they expect, so I think we can just drop the WARN and save the
duplication here.
+
+ vma = i915_gem_obj_to_ggtt(obj);
+ if (!vma)
+ return false;
+
/* There are 3 sources that pin objects:
* 1. The display engine (scanouts, sprites, cursors);
* 2. Reservations for execbuffer;
@@ -3652,7 +3661,7 @@ static bool is_pin_display(struct drm_i915_gem_object
*obj)
* subtracting the potential reference by the user, any pin_count
* remains, it must be due to another use by the display engine.
*/
- return i915_gem_obj_to_ggtt(obj)-pin_count - !!obj-user_pin_count;
+ return vma-pin_count - !!obj-user_pin_count;
}
/*
@@ -3666,6 +3675,7 @@ i915_gem_object_pin_to_display_plane(struct
drm_i915_gem_object *obj,
struct intel_ring_buffer *pipelined)
{
u32 old_read_domains, old_write_domain;
+ bool was_pin_display;
int ret;
if (pipelined != obj-ring) {
@@ -3677,6 +3687,7 @@ i915_gem_object_pin_to_display_plane(struct
drm_i915_gem_object *obj,
/* Mark the pin_display early so that we account for the
* display coherency whilst setting up the cache domains.
*/
+ was_pin_display = obj-pin_display;
obj-pin_display = true;
/* The display engine is not coherent with the LLC cache on gen6. As
@@ -3719,7 +3730,8 @@ i915_gem_object_pin_to_display_plane(struct
drm_i915_gem_object *obj,
return 0;
err_unpin_display:
- obj-pin_display = is_pin_display(obj);
+ WARN_ON(was_pin_display != is_pin_display(obj));
+ obj-pin_display = was_pin_display;
return ret;
}
Ok, this looks like a useful check.
Other than the debate over the placement of the WARN() in
i915_gem_obj_to_ggtt() (maybe leave a comment here to remind us to drop
the WARN and the check later?),
Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/50] drm/i915: Plumb the context everywhere in the execbuffer path
On Fri, May 16, 2014 at 11:11:38AM +, Mateo Lozano, Oscar wrote: -Original Message- From: Chris Wilson [mailto:ch...@chris-wilson.co.uk] Sent: Friday, May 16, 2014 12:05 PM To: Mateo Lozano, Oscar Cc: intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 09/50] drm/i915: Plumb the context everywhere in the execbuffer path On Fri, May 09, 2014 at 01:08:39PM +0100, oscar.ma...@intel.com wrote: From: Oscar Mateo oscar.ma...@intel.com The context are going to become very important pretty soon, and we need to be able to access them in a number of places inside the command submission path. The idea is that, when we need to place commands inside a ringbuffer or update the tail register, we know which context we are working with. We left intel_ring_begin() as a function macro to quickly adapt legacy code, an introduce intel_ringbuffer_begin() as the first of a set of new functions for ringbuffer manipulation (the rest will come in subsequent patches). No functional changes. v2: Do not set the context to NULL. In legacy code, set it to the default ring context (even if it doesn't get used later on). Won't rings be stored within the context? So the context should be derivable from which ring the operation is being issued on. -Chris Rings (as in engine command streamer) still remain in dev_priv and there are only four/five of them. What we store in the context is the new ringbuffer structure (which stores the head, tail, etc...) and the ringbuffer backing object. Knowing only the ring is not enough to derive the context. Ugh, I thought an earlier restructuring request was that the logical ring interface was context specific. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] rendercopy/bdw: Enable hw-generated binding tables
On Mon, May 12, 2014 at 06:40:43PM +0200, Daniel Vetter wrote: I think as soon as we have the golden context stuff from Mika we could drop our usage of restore_inhibit. We only need that to avoid the hw getting angry if the context state is illegal afaik. Apart from the contexts being over 100x larger than the state required to switch between clients, and that the current code regressed to always update the context between every batch. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [3.14.0-rc4] regression: drm FIFO underruns
On Tue, May 13, 2014 at 06:38:32PM +0200, Daniel Vetter wrote: On Tue, May 13, 2014 at 05:21:49PM +0200, Jörg Otte wrote: 2014-05-13 15:22 GMT+02:00 Daniel Vetter dan...@ffwll.ch: On Tue, May 13, 2014 at 12:38:41PM +0200, Daniel Vetter wrote: On Tue, May 13, 2014 at 12:29 PM, Jörg Otte jrg.o...@gmail.com wrote: Branch drm-intel-nightly as of ed60c27 drm-intel-nightly: 2014y-05m-09d-21h-51m-45s integration manifest looks badly: - KDE splash screen on boot-up is not visible - x-windows don't have title and menu bars - KDE system menu is not visible - moving windows around destroys its content Ugh, that's ugly. Nothing else change like e.g. the version of xfree-video-intel? (II) Loading /usr/lib/xorg/modules/drivers/intel_drv.so (II) Module intel: vendor=X.Org Foundation compiled for 1.11.3, module version = 2.17.0 Module class: X.Org Video Driver ABI class: X.Org Video Driver, version 11.0 Chris, any ideas? It's an ivybridge apparently. For the fifo underruns I think we've fully confirmed that they only happen on boot-up. I'll try to come up with some ideas what could have gone wrong there. Please test the below patch. -Daniel diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index b10fbde1d5ee..63ced2dee027 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -427,9 +427,6 @@ bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev, ret = !intel_crtc-cpu_fifo_underrun_disabled; - if (enable == ret) - goto done; - intel_crtc-cpu_fifo_underrun_disabled = !enable; if (enable (INTEL_INFO(dev)-gen 5 || IS_VALLEYVIEW(dev))) @@ -441,7 +438,6 @@ bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev, else if (IS_GEN8(dev)) broadwell_set_fifo_underrun_reporting(dev, pipe, enable); -done: return ret; } -- Doesn't work for me, I still have an underrun at boot-up. I'm at a loss tbh with ideas. We successfully disable both pipes, then enable pipe A and it all works. Then we enable pipe B and _both_ pipes underrun immediately afterwards. Really strange. Can you please reproduce the issue again on drm-intel-nightly (latest -nightly should also have the display corruptions fixed, so good to retest anyway) and attach a new dmesg with drm.debug=0xe. I see a few underrun on my IVB as well. But it seems to be limited to cases that involve the VGA connector, which doesn't actually exist on this machine so I can't be sure if it's really properly set up on the PCH. But so far with just two HDMI connectors I was unable to reproduce it. Meanwhile I'll try to come up with new theories and ideas. I was thinking that we might frob with the PCH refclk during driver init and that might cause the PCH underrun for Jörg, but it looks like the underruns really happen at the modeset time which is much later than the PCH refclock init. For the 1-n pipe transition, I don't think we handle it correctly at the moment. I have a fix as part of my remaining watermark patches. I rebased those and I'll repost them soon. In the meantime I pushed them to [1]. Jörg, can you give that branch a go? [1] git://gitorious.org/vsyrjala/linux.git watermarks_intm_31_notrace -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] tests/kms_flip: test a fb backed by a bo too big for its own good
On Fri, May 16, 2014 at 12:08:51PM +0100, oscar.ma...@intel.com wrote:
From: Oscar Mateo oscar.ma...@intel.com
This is a review by igt test for a bug located in
i915_gem_object_pin_to_display_plane and fixed by:
commit 392013bdd4b6128795e33c84bd6d6d3fd66ff0a3
Author: Oscar Mateo oscar.ma...@intel.com
Date: Fri May 16 11:23:12 2014 +0100
drm/i915: Gracefully handle obj not bound to GGTT in is_pin_display
Otherwise, we do a NULL pointer dereference.
I've seen this happen while handling an error in
i915_gem_object_pin_to_display_plane():
If i915_gem_object_set_cache_level() fails, we call is_pin_display()
to handle the error. At this point, the object is still not pinned
to GGTT and maybe not even bound, so we have to check before we
dereference its GGTT vma.
v2: Chris Wilson says restoring the old value is easier, but that
is_pin_display is useful as a theory of operation. Take the solomonic
decision: at least this way is_pin_display is a little more robust
(until Chris can kill it off).
Signed-off-by: Oscar Mateo oscar.ma...@intel.com
---
tests/kms_flip.c | 102
---
1 file changed, 97 insertions(+), 5 deletions(-)
diff --git a/tests/kms_flip.c b/tests/kms_flip.c
index bb4f71d..7296122 100644
--- a/tests/kms_flip.c
+++ b/tests/kms_flip.c
@@ -74,6 +74,7 @@
#define TEST_RPM (1 25)
#define TEST_SUSPEND (1 26)
#define TEST_TS_CONT (1 27)
+#define TEST_BO_TOOBIG (1 28)
#define EVENT_FLIP (1 0)
#define EVENT_VBLANK (1 1)
@@ -1208,6 +1209,84 @@ static void free_test_output(struct test_output *o)
}
}
+static int create_bigbo_for_fb(int fd, int width, int height, int bpp,
+bool tiled, uint32_t *gem_handle_ret,
+unsigned *size_ret, unsigned *stride_ret)
+{
+ uint32_t gem_handle;
+ int size, ret = 0;
+ unsigned stride;
+
+ if (tiled) {
+ int v;
+
+ v = width * bpp / 8;
+ for (stride = 512; stride v; stride *= 2)
+ ;
+
+ v = stride * height;
+ for (size = 1024*1024; size v; size *= 2)
+ ;
+ } else {
+ /* Scan-out has a 64 byte alignment restriction */
+ stride = (width * (bpp / 8) + 63) ~63;
+ size = stride * height;
+ }
+
+ /* 256 MB is usually the maximum mappable aperture,
+ * (make it 4x times that to ensure failure) */
+ gem_handle = gem_create(fd, 4*256*1024*1024);
+
+ if (tiled)
+ ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X, stride);
+
+ *stride_ret = stride;
+ *size_ret = size;
+ *gem_handle_ret = gem_handle;
+
+ return ret;
+}
+
+static unsigned int igt_create_fb_with_bigbo(int fd, int width, int height,
+ uint32_t format, bool tiled,
+ struct igt_fb *fb)
+{
+ uint32_t handles[4];
+ uint32_t pitches[4];
+ uint32_t offsets[4];
+ uint32_t fb_id;
+ int bpp;
+ int ret;
+
+ memset(fb, 0, sizeof(*fb));
+
+ bpp = igt_drm_format_to_bpp(format);
+ ret = create_bigbo_for_fb(fd, width, height, bpp, tiled,
+ fb-gem_handle, fb-size, fb-stride);
+ if (ret 0)
+ return ret;
+
+ memset(handles, 0, sizeof(handles));
+ handles[0] = fb-gem_handle;
+ memset(pitches, 0, sizeof(pitches));
+ pitches[0] = fb-stride;
+ memset(offsets, 0, sizeof(offsets));
+ if (drmModeAddFB2(fd, width, height, format, handles, pitches,
+ offsets, fb_id, 0) 0) {
+ gem_close(fd, fb-gem_handle);
+
+ return 0;
+ }
+
+ fb-width = width;
+ fb-height = height;
+ fb-tiling = tiled;
+ fb-drm_format = format;
+ fb-fb_id = fb_id;
+
+ return fb_id;
+}
Could we avoid a bit of code duplication with something like this?
create_bo_for_fb(..., bo_size)
{
...
if (bo_size == 0)
bo_size = size;
gem_handle = gem_create(fd, bo_size);
...
}
igt_create_fb_with_bo_size(xxx, bo_size)
{
...
create_bo_for_fb(..., bo_size);
...
}
igt_create_fb(xxx)
{
return igt_create_fb_with_bo_size(xxx, 0);
}
Otherwise looks pretty good.
+
static void run_test_on_crtc_set(struct test_output *o, int *crtc_idxs,
int crtc_count, int duration_ms)
{
@@ -1252,9 +1331,15 @@ static void run_test_on_crtc_set(struct test_output
*o, int *crtc_idxs,
o-fb_ids[0] = igt_create_fb(drm_fd, o-fb_width, o-fb_height,
igt_bpp_depth_to_drm_format(o-bpp,
o-depth),
tiled,
Re: [Intel-gfx] [PATCH] tests/kms_flip: test a fb backed by a bo too big for its own good
On Fri, May 16, 2014 at 03:04:21PM +0300, Ville Syrjälä wrote:
On Fri, May 16, 2014 at 12:08:51PM +0100, oscar.ma...@intel.com wrote:
From: Oscar Mateo oscar.ma...@intel.com
This is a review by igt test for a bug located in
i915_gem_object_pin_to_display_plane and fixed by:
commit 392013bdd4b6128795e33c84bd6d6d3fd66ff0a3
Author: Oscar Mateo oscar.ma...@intel.com
Date: Fri May 16 11:23:12 2014 +0100
drm/i915: Gracefully handle obj not bound to GGTT in is_pin_display
Otherwise, we do a NULL pointer dereference.
I've seen this happen while handling an error in
i915_gem_object_pin_to_display_plane():
If i915_gem_object_set_cache_level() fails, we call is_pin_display()
to handle the error. At this point, the object is still not pinned
to GGTT and maybe not even bound, so we have to check before we
dereference its GGTT vma.
v2: Chris Wilson says restoring the old value is easier, but that
is_pin_display is useful as a theory of operation. Take the solomonic
decision: at least this way is_pin_display is a little more robust
(until Chris can kill it off).
Signed-off-by: Oscar Mateo oscar.ma...@intel.com
---
tests/kms_flip.c | 102
---
1 file changed, 97 insertions(+), 5 deletions(-)
diff --git a/tests/kms_flip.c b/tests/kms_flip.c
index bb4f71d..7296122 100644
--- a/tests/kms_flip.c
+++ b/tests/kms_flip.c
@@ -74,6 +74,7 @@
#define TEST_RPM (1 25)
#define TEST_SUSPEND (1 26)
#define TEST_TS_CONT (1 27)
+#define TEST_BO_TOOBIG (1 28)
#define EVENT_FLIP (1 0)
#define EVENT_VBLANK (1 1)
@@ -1208,6 +1209,84 @@ static void free_test_output(struct test_output *o)
}
}
+static int create_bigbo_for_fb(int fd, int width, int height, int bpp,
+ bool tiled, uint32_t *gem_handle_ret,
+ unsigned *size_ret, unsigned *stride_ret)
+{
+ uint32_t gem_handle;
+ int size, ret = 0;
+ unsigned stride;
+
+ if (tiled) {
+ int v;
+
+ v = width * bpp / 8;
+ for (stride = 512; stride v; stride *= 2)
+ ;
+
+ v = stride * height;
+ for (size = 1024*1024; size v; size *= 2)
+ ;
+ } else {
+ /* Scan-out has a 64 byte alignment restriction */
+ stride = (width * (bpp / 8) + 63) ~63;
+ size = stride * height;
+ }
+
+ /* 256 MB is usually the maximum mappable aperture,
+* (make it 4x times that to ensure failure) */
+ gem_handle = gem_create(fd, 4*256*1024*1024);
+
+ if (tiled)
+ ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X, stride);
+
+ *stride_ret = stride;
+ *size_ret = size;
+ *gem_handle_ret = gem_handle;
+
+ return ret;
+}
+
+static unsigned int igt_create_fb_with_bigbo(int fd, int width, int height,
+uint32_t format, bool tiled,
+struct igt_fb *fb)
+{
+ uint32_t handles[4];
+ uint32_t pitches[4];
+ uint32_t offsets[4];
+ uint32_t fb_id;
+ int bpp;
+ int ret;
+
+ memset(fb, 0, sizeof(*fb));
+
+ bpp = igt_drm_format_to_bpp(format);
+ ret = create_bigbo_for_fb(fd, width, height, bpp, tiled,
+ fb-gem_handle, fb-size, fb-stride);
+ if (ret 0)
+ return ret;
+
+ memset(handles, 0, sizeof(handles));
+ handles[0] = fb-gem_handle;
+ memset(pitches, 0, sizeof(pitches));
+ pitches[0] = fb-stride;
+ memset(offsets, 0, sizeof(offsets));
+ if (drmModeAddFB2(fd, width, height, format, handles, pitches,
+ offsets, fb_id, 0) 0) {
+ gem_close(fd, fb-gem_handle);
+
+ return 0;
+ }
+
+ fb-width = width;
+ fb-height = height;
+ fb-tiling = tiled;
+ fb-drm_format = format;
+ fb-fb_id = fb_id;
+
+ return fb_id;
+}
Could we avoid a bit of code duplication with something like this?
create_bo_for_fb(..., bo_size)
{
...
if (bo_size == 0)
bo_size = size;
gem_handle = gem_create(fd, bo_size);
...
}
igt_create_fb_with_bo_size(xxx, bo_size)
{
...
create_bo_for_fb(..., bo_size);
...
}
igt_create_fb(xxx)
{
return igt_create_fb_with_bo_size(xxx, 0);
}
Oh and that gives me an idea for another test: try to create fb with
bo_size size and check that we get back some kind of error. Assuming
we don't have such a test already.
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4] drm/i915: Replaced Blitter ring based flips with MMIO flips for VLV
On Thu, 2014-05-15 at 12:27 +, Ville Syrjälä wrote:
On Thu, May 15, 2014 at 11:47:37AM +0530, sourab.gu...@intel.com wrote:
From: Sourab Gupta sourab.gu...@intel.com
Using MMIO based flips on Gen5+ for Media power well residency optimization.
The blitter ring is currently being used just for command streamer based
flip calls. For pure 3D workloads, with MMIO flips, there will be no use
of blitter ring and this will ensure the 100% residency for Media well.
In a subsequent patch, we can make the selection between CS vs MMIO flip
based on a module parameter to give more testing coverage.
v2: The MMIO flips now use the interrupt driven mechanism for issuing the
flips when target seqno is reached. (Incorporating Ville's idea)
v3: Rebasing on latest code. Code restructuring after incorporating
Damien's comments
v4: Addressing Ville's review comments
-general cleanup
-updating only base addr instead of calling update_primary_plane
-extending patch for gen5+ platforms
Signed-off-by: Sourab Gupta sourab.gu...@intel.com
Signed-off-by: Akash Goel akash.g...@intel.com
---
drivers/gpu/drm/i915/i915_dma.c | 1 +
drivers/gpu/drm/i915/i915_drv.h | 6 ++
drivers/gpu/drm/i915/i915_gem.c | 2 +-
drivers/gpu/drm/i915/i915_irq.c | 2 +
drivers/gpu/drm/i915/intel_display.c | 115
+++
drivers/gpu/drm/i915/intel_drv.h | 6 ++
6 files changed, 131 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c
b/drivers/gpu/drm/i915/i915_dma.c
index 46f1dec..672c28f 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1570,6 +1570,7 @@ int i915_driver_load(struct drm_device *dev, unsigned
long flags)
spin_lock_init(dev_priv-backlight_lock);
spin_lock_init(dev_priv-uncore.lock);
spin_lock_init(dev_priv-mm.object_stat_lock);
+ spin_lock_init(dev_priv-mmio_flip_lock);
dev_priv-ring_index = 0;
mutex_init(dev_priv-dpio_lock);
mutex_init(dev_priv-modeset_restore_lock);
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index 4006dfe..38c0820 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1543,6 +1543,8 @@ struct drm_i915_private {
struct i915_ums_state ums;
/* the indicator for dispatch video commands on two BSD rings */
int ring_index;
+ /* protects the mmio flip data */
+ spinlock_t mmio_flip_lock;
};
static inline struct drm_i915_private *to_i915(const struct drm_device
*dev)
@@ -2209,6 +2211,7 @@ bool i915_gem_retire_requests(struct drm_device *dev);
void i915_gem_retire_requests_ring(struct intel_ring_buffer *ring);
int __must_check i915_gem_check_wedge(struct i915_gpu_error *error,
bool interruptible);
+int i915_gem_check_olr(struct intel_ring_buffer *ring, u32 seqno);
static inline bool i915_reset_in_progress(struct i915_gpu_error *error)
{
return unlikely(atomic_read(error-reset_counter)
@@ -2580,6 +2583,9 @@ int i915_reg_read_ioctl(struct drm_device *dev, void
*data,
int i915_get_reset_stats_ioctl(struct drm_device *dev, void *data,
struct drm_file *file);
+void intel_notify_mmio_flip(struct drm_device *dev,
+ struct intel_ring_buffer *ring);
+
/* overlay */
extern struct intel_overlay_error_state
*intel_overlay_capture_error_state(struct drm_device *dev);
extern void intel_overlay_print_error_state(struct
drm_i915_error_state_buf *e,
diff --git a/drivers/gpu/drm/i915/i915_gem.c
b/drivers/gpu/drm/i915/i915_gem.c
index fa5b5ab..5b4e953 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -975,7 +975,7 @@ i915_gem_check_wedge(struct i915_gpu_error *error,
* Compare seqno against outstanding lazy request. Emit a request if they
are
* equal.
*/
-static int
+int
i915_gem_check_olr(struct intel_ring_buffer *ring, u32 seqno)
{
int ret;
diff --git a/drivers/gpu/drm/i915/i915_irq.c
b/drivers/gpu/drm/i915/i915_irq.c
index b10fbde..a353693 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1084,6 +1084,8 @@ static void notify_ring(struct drm_device *dev,
trace_i915_gem_request_complete(ring);
+ intel_notify_mmio_flip(dev, ring);
+
Hmm. How badly is this going to explode with UMS?
Hi Ville,
It seems there would be a small race between the page filp done intr and
the flip done interrupt from previous set base. But it seems to be the
case for CS flips also. In both cases, once we do the
mark_page_flip_active, there may be a window in which page flip intr
from previous set base may arrive.
Have we interpreted the race correctly? Or are we missing something
here?
Also, notify_mmio_flip
Re: [Intel-gfx] [PATCH v4] drm/i915: Replaced Blitter ring based flips with MMIO flips for VLV
On Fri, May 16, 2014 at 12:34:08PM +, Gupta, Sourab wrote:
On Thu, 2014-05-15 at 12:27 +, Ville Syrjälä wrote:
On Thu, May 15, 2014 at 11:47:37AM +0530, sourab.gu...@intel.com wrote:
From: Sourab Gupta sourab.gu...@intel.com
Using MMIO based flips on Gen5+ for Media power well residency
optimization.
The blitter ring is currently being used just for command streamer based
flip calls. For pure 3D workloads, with MMIO flips, there will be no use
of blitter ring and this will ensure the 100% residency for Media well.
In a subsequent patch, we can make the selection between CS vs MMIO flip
based on a module parameter to give more testing coverage.
v2: The MMIO flips now use the interrupt driven mechanism for issuing the
flips when target seqno is reached. (Incorporating Ville's idea)
v3: Rebasing on latest code. Code restructuring after incorporating
Damien's comments
v4: Addressing Ville's review comments
-general cleanup
-updating only base addr instead of calling update_primary_plane
-extending patch for gen5+ platforms
Signed-off-by: Sourab Gupta sourab.gu...@intel.com
Signed-off-by: Akash Goel akash.g...@intel.com
---
drivers/gpu/drm/i915/i915_dma.c | 1 +
drivers/gpu/drm/i915/i915_drv.h | 6 ++
drivers/gpu/drm/i915/i915_gem.c | 2 +-
drivers/gpu/drm/i915/i915_irq.c | 2 +
drivers/gpu/drm/i915/intel_display.c | 115
+++
drivers/gpu/drm/i915/intel_drv.h | 6 ++
6 files changed, 131 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c
b/drivers/gpu/drm/i915/i915_dma.c
index 46f1dec..672c28f 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1570,6 +1570,7 @@ int i915_driver_load(struct drm_device *dev,
unsigned long flags)
spin_lock_init(dev_priv-backlight_lock);
spin_lock_init(dev_priv-uncore.lock);
spin_lock_init(dev_priv-mm.object_stat_lock);
+ spin_lock_init(dev_priv-mmio_flip_lock);
dev_priv-ring_index = 0;
mutex_init(dev_priv-dpio_lock);
mutex_init(dev_priv-modeset_restore_lock);
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index 4006dfe..38c0820 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1543,6 +1543,8 @@ struct drm_i915_private {
struct i915_ums_state ums;
/* the indicator for dispatch video commands on two BSD rings */
int ring_index;
+ /* protects the mmio flip data */
+ spinlock_t mmio_flip_lock;
};
static inline struct drm_i915_private *to_i915(const struct drm_device
*dev)
@@ -2209,6 +2211,7 @@ bool i915_gem_retire_requests(struct drm_device
*dev);
void i915_gem_retire_requests_ring(struct intel_ring_buffer *ring);
int __must_check i915_gem_check_wedge(struct i915_gpu_error *error,
bool interruptible);
+int i915_gem_check_olr(struct intel_ring_buffer *ring, u32 seqno);
static inline bool i915_reset_in_progress(struct i915_gpu_error *error)
{
return unlikely(atomic_read(error-reset_counter)
@@ -2580,6 +2583,9 @@ int i915_reg_read_ioctl(struct drm_device *dev,
void *data,
int i915_get_reset_stats_ioctl(struct drm_device *dev, void *data,
struct drm_file *file);
+void intel_notify_mmio_flip(struct drm_device *dev,
+ struct intel_ring_buffer *ring);
+
/* overlay */
extern struct intel_overlay_error_state
*intel_overlay_capture_error_state(struct drm_device *dev);
extern void intel_overlay_print_error_state(struct
drm_i915_error_state_buf *e,
diff --git a/drivers/gpu/drm/i915/i915_gem.c
b/drivers/gpu/drm/i915/i915_gem.c
index fa5b5ab..5b4e953 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -975,7 +975,7 @@ i915_gem_check_wedge(struct i915_gpu_error *error,
* Compare seqno against outstanding lazy request. Emit a request if
they are
* equal.
*/
-static int
+int
i915_gem_check_olr(struct intel_ring_buffer *ring, u32 seqno)
{
int ret;
diff --git a/drivers/gpu/drm/i915/i915_irq.c
b/drivers/gpu/drm/i915/i915_irq.c
index b10fbde..a353693 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1084,6 +1084,8 @@ static void notify_ring(struct drm_device *dev,
trace_i915_gem_request_complete(ring);
+ intel_notify_mmio_flip(dev, ring);
+
Hmm. How badly is this going to explode with UMS?
Hi Ville,
It seems there would be a small race between the page filp done intr and
the flip done interrupt from previous set base. But it seems to be the
case for CS flips also. In both cases, once we do the
mark_page_flip_active, there may be a window in
Re: [Intel-gfx] [PATCH] tests/kms_flip: test a fb backed by a bo too big for its own good
-Original Message-
From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
Sent: Friday, May 16, 2014 1:14 PM
To: Mateo Lozano, Oscar
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] tests/kms_flip: test a fb backed by a bo too
big
for its own good
On Fri, May 16, 2014 at 03:04:21PM +0300, Ville Syrjälä wrote:
On Fri, May 16, 2014 at 12:08:51PM +0100, oscar.ma...@intel.com wrote:
From: Oscar Mateo oscar.ma...@intel.com
This is a review by igt test for a bug located in
i915_gem_object_pin_to_display_plane and fixed by:
commit 392013bdd4b6128795e33c84bd6d6d3fd66ff0a3
Author: Oscar Mateo oscar.ma...@intel.com
Date: Fri May 16 11:23:12 2014 +0100
drm/i915: Gracefully handle obj not bound to GGTT in
is_pin_display
Otherwise, we do a NULL pointer dereference.
I've seen this happen while handling an error in
i915_gem_object_pin_to_display_plane():
If i915_gem_object_set_cache_level() fails, we call is_pin_display()
to handle the error. At this point, the object is still not pinned
to GGTT and maybe not even bound, so we have to check before we
dereference its GGTT vma.
v2: Chris Wilson says restoring the old value is easier, but that
is_pin_display is useful as a theory of operation. Take the solomonic
decision: at least this way is_pin_display is a little more robust
(until Chris can kill it off).
Signed-off-by: Oscar Mateo oscar.ma...@intel.com
---
tests/kms_flip.c | 102
---
1 file changed, 97 insertions(+), 5 deletions(-)
diff --git a/tests/kms_flip.c b/tests/kms_flip.c index
bb4f71d..7296122 100644
--- a/tests/kms_flip.c
+++ b/tests/kms_flip.c
@@ -74,6 +74,7 @@
#define TEST_RPM (1 25)
#define TEST_SUSPEND (1 26)
#define TEST_TS_CONT (1 27)
+#define TEST_BO_TOOBIG (1 28)
#define EVENT_FLIP (1 0)
#define EVENT_VBLANK (1 1)
@@ -1208,6 +1209,84 @@ static void free_test_output(struct test_output
*o)
}
}
+static int create_bigbo_for_fb(int fd, int width, int height, int bpp,
+bool tiled, uint32_t *gem_handle_ret,
+unsigned *size_ret, unsigned *stride_ret) {
+ uint32_t gem_handle;
+ int size, ret = 0;
+ unsigned stride;
+
+ if (tiled) {
+ int v;
+
+ v = width * bpp / 8;
+ for (stride = 512; stride v; stride *= 2)
+ ;
+
+ v = stride * height;
+ for (size = 1024*1024; size v; size *= 2)
+ ;
+ } else {
+ /* Scan-out has a 64 byte alignment restriction */
+ stride = (width * (bpp / 8) + 63) ~63;
+ size = stride * height;
+ }
+
+ /* 256 MB is usually the maximum mappable aperture,
+ * (make it 4x times that to ensure failure) */
+ gem_handle = gem_create(fd, 4*256*1024*1024);
+
+ if (tiled)
+ ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X, stride);
+
+ *stride_ret = stride;
+ *size_ret = size;
+ *gem_handle_ret = gem_handle;
+
+ return ret;
+}
+
+static unsigned int igt_create_fb_with_bigbo(int fd, int width, int
height,
+ uint32_t format, bool tiled,
+ struct igt_fb *fb)
+{
+ uint32_t handles[4];
+ uint32_t pitches[4];
+ uint32_t offsets[4];
+ uint32_t fb_id;
+ int bpp;
+ int ret;
+
+ memset(fb, 0, sizeof(*fb));
+
+ bpp = igt_drm_format_to_bpp(format);
+ ret = create_bigbo_for_fb(fd, width, height, bpp, tiled,
+ fb-gem_handle, fb-size, fb-stride);
+ if (ret 0)
+ return ret;
+
+ memset(handles, 0, sizeof(handles));
+ handles[0] = fb-gem_handle;
+ memset(pitches, 0, sizeof(pitches));
+ pitches[0] = fb-stride;
+ memset(offsets, 0, sizeof(offsets));
+ if (drmModeAddFB2(fd, width, height, format, handles, pitches,
+ offsets, fb_id, 0) 0) {
+ gem_close(fd, fb-gem_handle);
+
+ return 0;
+ }
+
+ fb-width = width;
+ fb-height = height;
+ fb-tiling = tiled;
+ fb-drm_format = format;
+ fb-fb_id = fb_id;
+
+ return fb_id;
+}
Could we avoid a bit of code duplication with something like this?
create_bo_for_fb(..., bo_size)
{
...
if (bo_size == 0)
bo_size = size;
gem_handle = gem_create(fd, bo_size);
...
}
igt_create_fb_with_bo_size(xxx, bo_size) {
...
create_bo_for_fb(..., bo_size);
...
}
igt_create_fb(xxx)
{
return igt_create_fb_with_bo_size(xxx, 0); }
Sure, I´ll send a new version.
Oh and that gives me an idea for another test: try to create fb with
[Intel-gfx] [PATCH 1/2] lib/igt_fb: igt_create_fb_with_bo_size
From: Oscar Mateo oscar.ma...@intel.com
Useful for testing bigger/smaller fb-wrapped buffer objects.
Cc: Ville Syrjälä ville.syrj...@linux.intel.com
Signed-off-by: Oscar Mateo oscar.ma...@intel.com
---
lib/igt_fb.c | 45 ++---
lib/igt_fb.h | 3 +++
2 files changed, 41 insertions(+), 7 deletions(-)
diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index 2149fcd..f43af93 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -76,7 +76,8 @@ static struct format_desc_struct {
/* helpers to create nice-looking framebuffers */
static int create_bo_for_fb(int fd, int width, int height, int bpp,
bool tiled, uint32_t *gem_handle_ret,
- unsigned *size_ret, unsigned *stride_ret)
+ unsigned *size_ret, unsigned *stride_ret,
+ unsigned bo_size)
{
uint32_t gem_handle;
int size, ret = 0;
@@ -106,7 +107,9 @@ static int create_bo_for_fb(int fd, int width, int height,
int bpp,
size = stride * height;
}
- gem_handle = gem_create(fd, size);
+ if (bo_size == 0)
+ bo_size = size;
+ gem_handle = gem_create(fd, bo_size);
if (tiled)
ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X, stride);
@@ -376,17 +379,18 @@ void igt_paint_image(cairo_t *cr, const char *filename,
}
/**
- * igt_create_fb:
+ * igt_create_fb_with_bo_size:
* @fd: open i915 drm file descriptor
* @width: width of the framebuffer in pixel
* @height: height of the framebuffer in pixel
* @format: drm fourcc pixel format code
* @tiled: X-tiled or linear framebuffer
* @fb: pointer to an #igt_fb structure
+ * @bo_size: size of the backing bo (0 for minimum needed size)
*
* This function allocates a gem buffer object suitable to back a framebuffer
* with the requested properties and then wraps it up in a drm framebuffer
- * object. All metadata is stored in @fb.
+ * object of the requested size. All metadata is stored in @fb.
*
* The backing storage of the framebuffer is filled with all zeros, i.e. black
* for rgb pixel formats.
@@ -395,8 +399,9 @@ void igt_paint_image(cairo_t *cr, const char *filename,
* The kms id of the created framebuffer on success or a negative error code on
* failure.
*/
-unsigned int igt_create_fb(int fd, int width, int height, uint32_t format,
- bool tiled, struct igt_fb *fb)
+unsigned int igt_create_fb_with_bo_size(int fd, int width, int height,
+ uint32_t format, bool tiled,
+ struct igt_fb *fb, unsigned bo_size)
{
uint32_t handles[4];
uint32_t pitches[4];
@@ -409,7 +414,7 @@ unsigned int igt_create_fb(int fd, int width, int height,
uint32_t format,
bpp = igt_drm_format_to_bpp(format);
ret = create_bo_for_fb(fd, width, height, bpp, tiled, fb-gem_handle,
- fb-size, fb-stride);
+ fb-size, fb-stride, bo_size);
if (ret 0)
return ret;
@@ -435,6 +440,32 @@ unsigned int igt_create_fb(int fd, int width, int height,
uint32_t format,
}
/**
+ * igt_create_fb:
+ * @fd: open i915 drm file descriptor
+ * @width: width of the framebuffer in pixel
+ * @height: height of the framebuffer in pixel
+ * @format: drm fourcc pixel format code
+ * @tiled: X-tiled or linear framebuffer
+ * @fb: pointer to an #igt_fb structure
+ *
+ * This function allocates a gem buffer object suitable to back a framebuffer
+ * with the requested properties and then wraps it up in a drm framebuffer
+ * object. All metadata is stored in @fb.
+ *
+ * The backing storage of the framebuffer is filled with all zeros, i.e. black
+ * for rgb pixel formats.
+ *
+ * Returns:
+ * The kms id of the created framebuffer on success or a negative error code on
+ * failure.
+ */
+unsigned int igt_create_fb(int fd, int width, int height, uint32_t format,
+ bool tiled, struct igt_fb *fb)
+{
+ return igt_create_fb_with_bo_size(fd, width, height, format, tiled, fb,
0);
+}
+
+/**
* igt_create_color_fb:
* @fd: open i915 drm file descriptor
* @width: width of the framebuffer in pixel
diff --git a/lib/igt_fb.h b/lib/igt_fb.h
index e8bb2a8..c6558bf 100644
--- a/lib/igt_fb.h
+++ b/lib/igt_fb.h
@@ -62,6 +62,9 @@ enum igt_text_align {
align_hcenter = 0x08,
};
+unsigned int igt_create_fb_with_bo_size(int fd, int width, int height,
+ uint32_t format, bool tiled,
+ struct igt_fb *fb, unsigned bo_size);
unsigned int igt_create_fb(int fd, int width, int height, uint32_t format,
bool tiled, struct igt_fb *fb);
unsigned int igt_create_color_fb(int fd, int width, int height,
--
1.9.0
___
Intel-gfx mailing list
[Intel-gfx] [PATCH v2 2/2] tests/kms_flip: test a fb backed by a bo too big/small for its own good
From: Oscar Mateo oscar.ma...@intel.com
This is a review by igt test for a bug located in
i915_gem_object_pin_to_display_plane and fixed by:
commit 392013bdd4b6128795e33c84bd6d6d3fd66ff0a3
Author: Oscar Mateo oscar.ma...@intel.com
Date: Fri May 16 11:23:12 2014 +0100
drm/i915: Gracefully handle obj not bound to GGTT in is_pin_display
Otherwise, we do a NULL pointer dereference.
I've seen this happen while handling an error in
i915_gem_object_pin_to_display_plane():
If i915_gem_object_set_cache_level() fails, we call is_pin_display()
to handle the error. At this point, the object is still not pinned
to GGTT and maybe not even bound, so we have to check before we
dereference its GGTT vma.
v2: Chris Wilson says restoring the old value is easier, but that
is_pin_display is useful as a theory of operation. Take the solomonic
decision: at least this way is_pin_display is a little more robust
(until Chris can kill it off).
v2: Avoid code duplication by using igt_create_fb_with_bo_size() as
requested by Ville Syrjälä (original author of the too big test idea).
Signed-off-by: Oscar Mateo oscar.ma...@intel.com
---
tests/kms_flip.c | 22 ++
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/tests/kms_flip.c b/tests/kms_flip.c
index bb4f71d..f2ec9ef 100644
--- a/tests/kms_flip.c
+++ b/tests/kms_flip.c
@@ -74,6 +74,7 @@
#define TEST_RPM (1 25)
#define TEST_SUSPEND (1 26)
#define TEST_TS_CONT (1 27)
+#define TEST_BO_TOOBIG (1 28)
#define EVENT_FLIP (1 0)
#define EVENT_VBLANK (1 1)
@@ -1213,6 +1214,7 @@ static void run_test_on_crtc_set(struct test_output *o,
int *crtc_idxs,
{
char test_name[128];
unsigned elapsed;
+ unsigned bo_size = 0;
bool tiled;
int i;
@@ -1249,12 +1251,17 @@ static void run_test_on_crtc_set(struct test_output *o,
int *crtc_idxs,
if (o-flags TEST_FENCE_STRESS)
tiled = true;
+ /* 256 MB is usually the maximum mappable aperture,
+* (make it 4x times that to ensure failure) */
+ if (o-flags TEST_BO_TOOBIG)
+ bo_size = 4*256*1024*1024;
+
o-fb_ids[0] = igt_create_fb(drm_fd, o-fb_width, o-fb_height,
igt_bpp_depth_to_drm_format(o-bpp,
o-depth),
tiled, o-fb_info[0]);
- o-fb_ids[1] = igt_create_fb(drm_fd, o-fb_width, o-fb_height,
+ o-fb_ids[1] = igt_create_fb_with_bo_size(drm_fd, o-fb_width,
o-fb_height,
igt_bpp_depth_to_drm_format(o-bpp,
o-depth),
-tiled, o-fb_info[1]);
+tiled, o-fb_info[1], bo_size);
o-fb_ids[2] = igt_create_fb(drm_fd, o-fb_width, o-fb_height,
igt_bpp_depth_to_drm_format(o-bpp,
o-depth),
true, o-fb_info[2]);
@@ -1264,7 +1271,8 @@ static void run_test_on_crtc_set(struct test_output *o,
int *crtc_idxs,
igt_require(o-fb_ids[2]);
paint_flip_mode(o-fb_info[0], false);
- paint_flip_mode(o-fb_info[1], true);
+ if (!(o-flags TEST_BO_TOOBIG))
+ paint_flip_mode(o-fb_info[1], true);
if (o-fb_ids[2])
paint_flip_mode(o-fb_info[2], true);
@@ -1288,10 +1296,15 @@ static void run_test_on_crtc_set(struct test_output *o,
int *crtc_idxs,
if (o-flags TEST_CHECK_TS)
sleep(1);
- igt_assert(do_page_flip(o, o-fb_ids[1], true) == 0);
+ if (o-flags TEST_BO_TOOBIG) {
+ igt_assert(do_page_flip(o, o-fb_ids[1], true) == -E2BIG);
+ goto out;
+ } else
+ igt_assert(do_page_flip(o, o-fb_ids[1], true) == 0);
wait_for_events(o);
o-current_fb_id = 1;
+
if (o-flags TEST_FLIP)
o-flip_state.seq_step = 1;
else
@@ -1543,6 +1556,7 @@ int main(int argc, char **argv)
{ 10, TEST_VBLANK | TEST_DPMS | TEST_SUSPEND | TEST_TS_CONT,
dpms-vs-suspend },
{ 0, TEST_VBLANK | TEST_MODESET | TEST_RPM | TEST_TS_CONT,
modeset-vs-rpm },
{ 0, TEST_VBLANK | TEST_MODESET | TEST_SUSPEND | TEST_TS_CONT,
modeset-vs-suspend },
+ { 0, TEST_BO_TOOBIG | TEST_NO_2X_OUTPUT, bo-too-big },
};
int i;
--
1.9.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Introduce mapping of user pages into video memory (userptr) ioctl
By exporting the ability to map user address and inserting PTEs representing their backing pages into the GTT, we can exploit UMA in order to utilize normal application data as a texture source or even as a render target (depending upon the capabilities of the chipset). This has a number of uses, with zero-copy downloads to the GPU and efficient readback making the intermixed streaming of CPU and GPU operations fairly efficient. This ability has many widespread implications from faster rendering of client-side software rasterisers (chromium), mitigation of stalls due to read back (firefox) and to faster pipelining of texture data (such as pixel buffer objects in GL or data blobs in CL). v2: Compile with CONFIG_MMU_NOTIFIER v3: We can sleep while performing invalidate-range, which we can utilise to drop our page references prior to the kernel manipulating the vma (for either discard or cloning) and so protect normal users. v4: Only run the invalidate notifier if the range intercepts the bo. v5: Prevent userspace from attempting to GTT mmap non-page aligned buffers v6: Recheck after reacquire mutex for lost mmu. v7: Fix implicit padding of ioctl struct by rounding to next 64bit boundary. v8: Fix rebasing error after forwarding porting the back port. v9: Limit the userptr to page aligned entries. We now expect userspace to handle all the offset-in-page adjustments itself. v10: Prevent vma from being copied across fork to avoid issues with cow. v11: Drop vma behaviour changes -- locking is nigh on impossible. Use a worker to load user pages to avoid lock inversions. v12: Use get_task_mm()/mmput() for correct refcounting of mm. v13: Use a worker to release the mmu_notifier to avoid lock inversion v14: Decouple mmu_notifier from struct_mutex using a custom mmu_notifer with its own locking and tree of objects for each mm/mmu_notifier. v15: Prevent overlapping userptr objects, and invalidate all objects within the mmu_notifier range v16: Fix a typo for iterating over multiple objects in the range and rearrange error path to destroy the mmu_notifier locklessly. Also close a race between invalidate_range and the get_pages_worker. v17: Close a race between get_pages_worker/invalidate_range and fresh allocations of the same userptr range - and notice that struct_mutex was presumed to be held when during creation it wasn't. v18: Sigh. Fix the refactor of st_set_pages() to allocate enough memory for the struct sg_table and to clear it before reporting an error. v19: Always error out on read-only userptr requests as we don't have the hardware infrastructure to support them at the moment. v20: Refuse to implement read-only support until we have the required infrastructure - but reserve the bit in flags for future use. v21: use_mm() is not required for get_user_pages(). It is only meant to be used to fix up the kernel thread's current-mm for use with copy_user(). v22: Use sg_alloc_table_from_pages for that chunky feeling v23: Export a function for sanity checking dma-buf rather than encode userptr details elsewhere, and clean up comments based on suggestions by Bradley. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Tvrtko Ursulin tvrtko.ursu...@linux.intel.com Cc: Gong, Zhipeng zhipeng.g...@intel.com Cc: Akash Goel akash.g...@intel.com Cc: Volkin, Bradley D bradley.d.vol...@intel.com Reviewed-by: Tvrtko Ursulin tvrtko.ursu...@linux.intel.com --- drivers/gpu/drm/i915/Kconfig| 1 + drivers/gpu/drm/i915/Makefile | 1 + drivers/gpu/drm/i915/i915_dma.c | 1 + drivers/gpu/drm/i915/i915_drv.h | 25 +- drivers/gpu/drm/i915/i915_gem.c | 4 + drivers/gpu/drm/i915/i915_gem_dmabuf.c | 8 + drivers/gpu/drm/i915/i915_gem_userptr.c | 711 drivers/gpu/drm/i915/i915_gpu_error.c | 2 + include/uapi/drm/i915_drm.h | 16 + 9 files changed, 768 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/i915/i915_gem_userptr.c diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig index e4e3c01b8cbc..437e1824d0bf 100644 --- a/drivers/gpu/drm/i915/Kconfig +++ b/drivers/gpu/drm/i915/Kconfig @@ -5,6 +5,7 @@ config DRM_I915 depends on (AGP || AGP=n) select INTEL_GTT select AGP_INTEL if AGP + select INTERVAL_TREE # we need shmfs for the swappable backing store, and in particular # the shmem_readpage() which depends upon tmpfs select SHMEM diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index b6ce5640d592..fa9e806259ba 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -28,6 +28,7 @@ i915-y += i915_cmd_parser.o \ i915_gem.o \ i915_gem_stolen.o \ i915_gem_tiling.o \ + i915_gem_userptr.o \ i915_gpu_error.o \ i915_irq.o \ i915_trace_points.o \ diff --git
[Intel-gfx] [PATCH v3] drm/i915: Gracefully handle obj not bound to GGTT in is_pin_display
From: Oscar Mateo oscar.ma...@intel.com
Otherwise, we do a NULL pointer dereference.
I've seen this happen while handling an error in
i915_gem_object_pin_to_display_plane():
If i915_gem_object_set_cache_level() fails, we call is_pin_display()
to handle the error. At this point, the object is still not pinned
to GGTT and maybe not even bound, so we have to check before we
dereference its GGTT vma.
The IGT kms_flip/bo-too-big tests for this bug.
v2: Chris Wilson says restoring the old value is easier, but that
is_pin_display is useful as a theory of operation. Take the solomonic
decision: at least this way is_pin_display is a little more robust
(until Chris can kill it off).
v3: Chris suggests the WARN in i915_gem_obj_to_ggtt has outlived its
usefulness: add a reminder to remove it.
Issue: VIZ-3772
Signed-off-by: Oscar Mateo oscar.ma...@intel.com
---
drivers/gpu/drm/i915/i915_gem.c | 19 +--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 034ba2c..f6c0351 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3641,6 +3641,15 @@ unlock:
static bool is_pin_display(struct drm_i915_gem_object *obj)
{
+ struct i915_vma *vma;
+
+ if (list_empty(obj-vma_list))
+ return false;
+
+ vma = i915_gem_obj_to_ggtt(obj);
+ if (!vma)
+ return false;
+
/* There are 3 sources that pin objects:
* 1. The display engine (scanouts, sprites, cursors);
* 2. Reservations for execbuffer;
@@ -3652,7 +3661,7 @@ static bool is_pin_display(struct drm_i915_gem_object
*obj)
* subtracting the potential reference by the user, any pin_count
* remains, it must be due to another use by the display engine.
*/
- return i915_gem_obj_to_ggtt(obj)-pin_count - !!obj-user_pin_count;
+ return vma-pin_count - !!obj-user_pin_count;
}
/*
@@ -3666,6 +3675,7 @@ i915_gem_object_pin_to_display_plane(struct
drm_i915_gem_object *obj,
struct intel_ring_buffer *pipelined)
{
u32 old_read_domains, old_write_domain;
+ bool was_pin_display;
int ret;
if (pipelined != obj-ring) {
@@ -3677,6 +3687,7 @@ i915_gem_object_pin_to_display_plane(struct
drm_i915_gem_object *obj,
/* Mark the pin_display early so that we account for the
* display coherency whilst setting up the cache domains.
*/
+ was_pin_display = obj-pin_display;
obj-pin_display = true;
/* The display engine is not coherent with the LLC cache on gen6. As
@@ -3719,7 +3730,8 @@ i915_gem_object_pin_to_display_plane(struct
drm_i915_gem_object *obj,
return 0;
err_unpin_display:
- obj-pin_display = is_pin_display(obj);
+ WARN_ON(was_pin_display != is_pin_display(obj));
+ obj-pin_display = was_pin_display;
return ret;
}
@@ -5115,6 +5127,9 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct
drm_i915_gem_object *obj)
{
struct i915_vma *vma;
+ /* This WARN has probably outlived its usefulness (callers already
+* WARN if they don't find the GGTT vma they expect). When removing,
+* remember to remove the pre-check in is_pin_display() as well */
if (WARN_ON(list_empty(obj-vma_list)))
return NULL;
--
1.9.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] 830GM still woes
Hi Daniel, dear list, just tried the latest nightly build of 3.15.0+, and I'm sorry to say that the watermark configuration of the 830GM is still off. This is what I get from the kernel: (not to be taken too serious, but humor is the only thing that keeps me from getting seriously anoyed after so much time of the same bug...) May 16 15:15:42 pike kernel: [6.705950] [drm:drm_calc_vbltimestamp_from_scanoutpos] crtc 1: Noop due to uninitialized mode. May 16 15:15:42 pike kernel: [6.710930] [drm:i830_get_fifo_size] FIFO size - (0x00017e5f) A: 47 May 16 15:15:42 pike kernel: [6.710936] [drm:i830_get_fifo_size] FIFO size - (0x00017e5f) B: 48 May 16 15:15:42 pike kernel: [6.710942] [drm:i9xx_update_wm] FIFO watermarks - A: 45, B: 46 May 16 15:15:42 pike kernel: [6.710947] [drm:i9xx_update_wm] Setting FIFO watermarks - A: 45, B: 46, C: 2, SR 1 May 16 15:15:42 pike kernel: [6.710953] [drm:intel_sanitize_crtc] [CRTC:7] hw state adjusted, was enabled, now disabled Watermarks of 45 and 46 upon initialization. Weird, but it gets weirder... May 16 15:15:42 pike kernel: [6.842042] [drm:i830_get_fifo_size] FIFO size - (0x00017e5f) A: 47 May 16 15:15:42 pike kernel: [6.842046] [drm:i830_get_fifo_size] FIFO size - (0x00017e5f) B: 48 May 16 15:15:42 pike kernel: [6.842050] [drm:i9xx_update_wm] FIFO watermarks - A: 45, B: 46 May 16 15:15:42 pike kernel: [6.842053] [drm:i9xx_update_wm] Setting FIFO watermarks - A: 45, B: 46, C: 2, SR 1 May 16 15:15:42 pike kernel: [6.842061] [drm:drm_calc_timestamping_constants] crtc 5: hwmode: htotal 1688, vtotal 1066, vdisplay 1024 May 16 15:15:42 pike kernel: [6.842065] [drm:drm_calc_timestamping_constants] crtc 5: clock 108000 kHz framedur 16661185 linedur 15629, pixeldur 9 May 16 15:15:42 pike kernel: [6.847625] [drm:i9xx_update_primary_plane] Writing base 0006 0 0 5120 May 16 15:15:42 pike kernel: [6.847633] [drm:intel_crtc_mode_set] [ENCODER:9:DAC-9] set [MODE:0:1280x1024] May 16 15:15:42 pike kernel: [6.848287] [drm:i830_get_fifo_size] FIFO size - (0x00017e5f) A: 47 May 16 15:15:42 pike kernel: [6.848290] [drm:intel_calculate_wm] FIFO entries required for mode: 68 May 16 15:15:42 pike kernel: [6.848292] [drm:intel_calculate_wm] FIFO watermark level: -23 May 16 15:15:42 pike kernel: [6.848295] [drm:i830_get_fifo_size] FIFO size - (0x00017e5f) B: 48 May 16 15:15:42 pike kernel: [6.848297] [drm:i9xx_update_wm] FIFO watermarks - A: 1, B: 46 May 16 15:15:42 pike kernel: [6.848300] [drm:i9xx_update_wm] Setting FIFO watermarks - A: 1, B: 46, C: 2, SR 1 May 16 15:15:42 pike kernel: [6.856893] [drm:intel_connector_check_state] [CONNECTOR:8:VGA-1] May 16 15:15:42 pike kernel: [6.856897] [drm:check_encoder_state] [ENCODER:9:DAC-9] May 16 15:15:42 pike kernel: [6.856901] [drm:check_encoder_state] [ENCODER:10:None-10] Ok, so that computes a watermark of -23 for pipe A (WTF?) then rounded up to +1 (but still too small, needs to be +8), B remains at +46 even though an external monitor is connected. May 16 15:15:42 pike kernel: [6.857023] [drm:intel_dump_pipe_config] adjusted mode: May 16 15:15:42 pike kernel: [6.857029] [drm:drm_mode_debug_printmodeline] Modeline 0:1024x768 60 65000 1024 1048 1184 1344 768 771 777 806 0x8 0x5 May 16 15:15:42 pike kernel: [6.857034] [drm:intel_dump_crtc_timings] crtc timings: 65000 1024 1048 1184 1344 768 771 777 806, type: 0x8 flags: 0x5 May 16 15:15:42 pike kernel: [6.857036] [drm:intel_dump_pipe_config] port clock: 65000 May 16 15:15:42 pike kernel: [6.857038] [drm:intel_dump_pipe_config] pipe src size: 1024x768 May 16 15:15:42 pike kernel: [6.857040] [drm:intel_dump_pipe_config] gmch pfit: control: 0x, ratios: 0x, lvds border: 0x May 16 15:15:42 pike kernel: [6.857043] [drm:intel_dump_pipe_config] pch pfit: pos: 0x, size: 0x, disabled May 16 15:15:42 pike kernel: [6.857045] [drm:intel_dump_pipe_config] ips: 0 May 16 15:15:42 pike kernel: [6.857047] [drm:intel_dump_pipe_config] double wide: 0 May 16 15:15:42 pike kernel: [6.857052] [drm:drm_calc_timestamping_constants] crtc 7: hwmode: htotal 1344, vtotal 806, vdisplay 768 May 16 15:15:42 pike kernel: [6.857055] [drm:drm_calc_timestamping_constants] crtc 7: clock 65000 kHz framedur 16665600 linedur 20676, pixeldur 15 May 16 15:15:42 pike kernel: [6.868773] [drm:i9xx_update_primary_plane] Writing base 0006 0 0 5120 May 16 15:15:42 pike kernel: [6.868784] [drm:intel_crtc_mode_set] [ENCODER:10:None-10] set [MODE:0:1024x768] May 16 15:15:42 pike kernel: [6.869450] [drm:i830_get_fifo_size] FIFO size - (0x00017e5f) A: 47 May 16 15:15:42 pike kernel: [6.869454] [drm:intel_calculate_wm] FIFO entries required for mode: 68 May 16 15:15:42 pike kernel: [6.869456] [drm:intel_calculate_wm] FIFO watermark level: -23 May 16 15:15:42 pike kernel: [
Re: [Intel-gfx] [PATCH v3] drm/i915: Gracefully handle obj not bound to GGTT in is_pin_display
On Fri, May 16, 2014 at 02:20:43PM +0100, oscar.ma...@intel.com wrote:
From: Oscar Mateo oscar.ma...@intel.com
Otherwise, we do a NULL pointer dereference.
I've seen this happen while handling an error in
i915_gem_object_pin_to_display_plane():
If i915_gem_object_set_cache_level() fails, we call is_pin_display()
to handle the error. At this point, the object is still not pinned
to GGTT and maybe not even bound, so we have to check before we
dereference its GGTT vma.
The IGT kms_flip/bo-too-big tests for this bug.
v2: Chris Wilson says restoring the old value is easier, but that
is_pin_display is useful as a theory of operation. Take the solomonic
decision: at least this way is_pin_display is a little more robust
(until Chris can kill it off).
v3: Chris suggests the WARN in i915_gem_obj_to_ggtt has outlived its
usefulness: add a reminder to remove it.
Issue: VIZ-3772
Signed-off-by: Oscar Mateo oscar.ma...@intel.com
Queued for -next, thanks for the patch.
-Daniel
---
drivers/gpu/drm/i915/i915_gem.c | 19 +--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 034ba2c..f6c0351 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3641,6 +3641,15 @@ unlock:
static bool is_pin_display(struct drm_i915_gem_object *obj)
{
+ struct i915_vma *vma;
+
+ if (list_empty(obj-vma_list))
+ return false;
+
+ vma = i915_gem_obj_to_ggtt(obj);
+ if (!vma)
+ return false;
+
/* There are 3 sources that pin objects:
* 1. The display engine (scanouts, sprites, cursors);
* 2. Reservations for execbuffer;
@@ -3652,7 +3661,7 @@ static bool is_pin_display(struct drm_i915_gem_object
*obj)
* subtracting the potential reference by the user, any pin_count
* remains, it must be due to another use by the display engine.
*/
- return i915_gem_obj_to_ggtt(obj)-pin_count - !!obj-user_pin_count;
+ return vma-pin_count - !!obj-user_pin_count;
}
/*
@@ -3666,6 +3675,7 @@ i915_gem_object_pin_to_display_plane(struct
drm_i915_gem_object *obj,
struct intel_ring_buffer *pipelined)
{
u32 old_read_domains, old_write_domain;
+ bool was_pin_display;
int ret;
if (pipelined != obj-ring) {
@@ -3677,6 +3687,7 @@ i915_gem_object_pin_to_display_plane(struct
drm_i915_gem_object *obj,
/* Mark the pin_display early so that we account for the
* display coherency whilst setting up the cache domains.
*/
+ was_pin_display = obj-pin_display;
obj-pin_display = true;
/* The display engine is not coherent with the LLC cache on gen6. As
@@ -3719,7 +3730,8 @@ i915_gem_object_pin_to_display_plane(struct
drm_i915_gem_object *obj,
return 0;
err_unpin_display:
- obj-pin_display = is_pin_display(obj);
+ WARN_ON(was_pin_display != is_pin_display(obj));
+ obj-pin_display = was_pin_display;
return ret;
}
@@ -5115,6 +5127,9 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct
drm_i915_gem_object *obj)
{
struct i915_vma *vma;
+ /* This WARN has probably outlived its usefulness (callers already
+ * WARN if they don't find the GGTT vma they expect). When removing,
+ * remember to remove the pre-check in is_pin_display() as well */
if (WARN_ON(list_empty(obj-vma_list)))
return NULL;
--
1.9.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] rendercopy/bdw: Enable hw-generated binding tables
On Fri, May 16, 2014 at 12:36:09PM +0100, Chris Wilson wrote: On Mon, May 12, 2014 at 06:40:43PM +0200, Daniel Vetter wrote: I think as soon as we have the golden context stuff from Mika we could drop our usage of restore_inhibit. We only need that to avoid the hw getting angry if the context state is illegal afaik. Apart from the contexts being over 100x larger than the state required to switch between clients, and that the current code regressed to always update the context between every batch. We still have the from == to early return in do_switch. That doesn't do what it should any more? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Be careful with non-disp bit in PMINTRMSK
On Fri, May 16, 2014 at 01:44:12PM +0300, Mika Kuoppala wrote: Bit 31 in GEN6_PMINTRMSK is not an interrupt disable bit with gen8. Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com Signed-off-by: Mika Kuoppala mika.kuopp...@intel.com Queued for -next, thanks for the patch. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] i915: Add module option to support VGA arbiter on HD devices
On Fri, 2014-05-16 at 11:06 +0200, Daniel Vetter wrote: On Thu, May 15, 2014 at 10:46:50PM -0600, Alex Williamson wrote: On Fri, 2014-05-16 at 00:50 +0200, Daniel Vetter wrote: On Thu, May 15, 2014 at 11:43 PM, Alex Williamson alex.william...@redhat.com wrote: I don't know what to do with this. It seems like a lot of wishful thinking that in the best case would drag on for years. Even if we get VGA arbitration out of Xorg, the bit about making the userspace VGA arbiter interface lie depending on current-comm sounds tricky and horrible. If we can lie to Xorg there, why don't we do that now? If we can't lie to Xorg now, then what deprecation event or detection of the caller is going to allow us to do so in the future? Well we wouldn't necessarily need to lie to X, but could instead look whether all the vga devices in a system are claimed by kms drivers. If that's the case the userspace doesn't have an awful lot of business touching the VGA registers and we could simply not obey a vga arb request from userspace. More advanced would be if we still obey it for those devices not claimed by kms drivers. So not really a need to key on current-comm. This is a requirement for me, I don't really care about KMS or Xorg, the use case I want to enable is binding a VGA device to vfio-pci so that it can be assigned to a guest virtual machine. This works on an unmodified kernel today so long as you don't have an Intel IGD in your system. If you do, we try to switch the VGA device, but it doesn't actually get switched because i915 opts-out of arbitration yet still claims VGA accesses. I get that its a requirement for you. I've also just detailed the solution for you above, but I'm not going to write the patch itself (since I can't test it really). I repeat it because it seems incompatible with any plan that involves modes of operation where the VGA arbiter says one thing if all VGA devices are bound to KMS drivers and another if they're not. That means Xorg would have a different feature set depending on the driver state of devices we might be using for entirely different purposes. Users want to be able to run X on the host i915 at the same time that they have other VGA devices assigned to guests. We have two users of the vga-arb crap relevant here: - pci/pci-sysfs.c, used by X through libpciaccess - vfio/pci/vfio_pci_rdwr.c, for vfio-pci vga forwarding Do we really know that we only have these two users? Make the former lie if all vga devices have kms drivers and the latter There's that if all vga devices again. Is the user expected to start Xorg with all devices attached to KMS drivers, then unbind the extra VGA devices to be used for other purposes? What happens if the user wants to restart X, do they get a different feature set? Does the output of the VGA arbiter change while X is running when the other devices are unbound from their KMS driver? (unbinding a graphics card from it's driver isn't a foolproof operation btw, so the driver may be blacklisted or avoided through other means) still work correctly. That will prevent X from going nasty if there are kms drivers, while still keeping vfio going. But by lying to Xorg, I think we're saying that it's ok to go mmap VGA MMIO space through /dev/mem and poke VGA I/O through inb/outb, it's not going to change... then at the same time we provide this other interface that allows it to change. Are we just masking i915's bug by breaking the entire interface? Then we re-re-revert the i915 to have proper vga-arb. Afaics no need for hacks, module options, special casing or breaking old userspace. But there is special casing isn't there? Aren't we only able to make the assumption that it's ok to lie through the VGA arbiter userspace interface if only KMS drivers are in use? And really, if there is a kms driver userspace has zero business touching the vga registers, so imo we don't lose an real functionality. You can always opt to not load kms drivers if you want userspace access. What am I missing? I must be the one missing something because this still all sounds terribly convoluted and fragile. Thanks, Alex ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] 830GM still woes
On Fri, May 16, 2014 at 04:02:48PM +0200, Thomas Richter wrote:
It's not that I haven't had a patch for it. Really trivial. I wonder
what keeps you from adding this to the kernel and just make things
working?
You mean this patch?
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f671aca..3981898 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -944,7 +944,7 @@ static const struct intel_watermark_params i915_wm_info = {
static const struct intel_watermark_params i830_wm_info = {
I855GM_FIFO_SIZE,
I915_MAX_WM,
- 1,
+ 8,
2,
I830_FIFO_LINE_SIZE
};
@@ -1001,7 +1001,7 @@ static unsigned long intel_calculate_wm(unsigned long
clock_in_khz,
/* Don't promote wm_size to unsigned... */
if (wm_size (long)wm-max_wm)
wm_size = wm-max_wm;
- if (wm_size = 0)
+ if (wm_size (long)wm-default_wm)
wm_size = wm-default_wm;
return wm_size;
}
I haven't spotted any explanation as to why that is, but a rough guess
would be that we program it to read in blocks of 8 superwords and that
it tries and fails to read from memory when the fifo only has room for 1
superword.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] rendercopy/bdw: Enable hw-generated binding tables
On Fri, May 16, 2014 at 04:27:34PM +0200, Daniel Vetter wrote: On Fri, May 16, 2014 at 12:36:09PM +0100, Chris Wilson wrote: On Mon, May 12, 2014 at 06:40:43PM +0200, Daniel Vetter wrote: I think as soon as we have the golden context stuff from Mika we could drop our usage of restore_inhibit. We only need that to avoid the hw getting angry if the context state is illegal afaik. Apart from the contexts being over 100x larger than the state required to switch between clients, and that the current code regressed to always update the context between every batch. We still have the from == to early return in do_switch. That doesn't do what it should any more? No. commit 0009e46cd54324c4af20b0b52b89973b1b914167 Author: Ben Widawsky b...@bwidawsk.net Date: Fri Dec 6 14:11:02 2013 -0800 drm/i915: Track which ring a context ran on Previously we dropped the association of a context to a ring. It is however very important to know which ring a context ran on (we could have reused the other member, but I was nitpicky). This is very important when we switch address spaces, which unlike context objects, do change per ring. As an example, if we have: RCS BCS ctxA ctx A ctx B ctxB Without tracking the last ring B ran on, we wouldn't know to switch the address space on BCS in the last row. As a result, we no longer need to track which ring a context belongs to, as it never really made much sense anyway. Signed-off-by: Ben Widawsky b...@bwidawsk.net Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch was just baloney. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/10] drm/i915/chv: Added CHV specific register read and write
deepa...@linux.intel.com writes:
From: Deepak S deepa...@linux.intel.com
Support to individually control Media/Render well based on the register
access.
Add CHV specific write function to habdle difference between registers
that are sadowed vs those that need forcewake even for writes.
v2: Drop write FIFO for CHV and add comman well forcewake (Ville)
v3: Fix for decrementing fw count in chv read/write. (Deepak)
Signed-off-by: Deepak S deepa...@linux.intel.com
[vsyrjala: Move the register range macros into intel_uncore.c]
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/intel_uncore.c | 133
+---
1 file changed, 125 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c
b/drivers/gpu/drm/i915/intel_uncore.c
index 76dc185..4f1f199 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -495,6 +495,31 @@ void assert_force_wake_inactive(struct drm_i915_private
*dev_priv)
((reg) = 0x22000 (reg) 0x24000) ||\
((reg) = 0x3 (reg) 0x4))
+#define FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg) \
+ (((reg) = 0x2000 (reg) 0x4000) ||\
+ ((reg) = 0x5000 (reg) 0x8000) ||\
+ ((reg) = 0x8300 (reg) 0x8500) ||\
+ ((reg) = 0xB000 (reg) 0xC000) ||\
+ ((reg) = 0xE000 (reg) 0xE800))
+
+#define FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)\
+ (((reg) = 0x8800 (reg) 0x8900) ||\
+ ((reg) = 0xD000 (reg) 0xD800) ||\
+ ((reg) = 0x12000 (reg) 0x14000) ||\
+ ((reg) = 0x1A000 (reg) 0x1C000) ||\
+ ((reg) = 0x1E800 (reg) 0x1EA00) ||\
+ ((reg) = 0x3 (reg) 0x4))
+
+#define FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)\
+ (((reg) = 0x4000 (reg) 0x5000) ||\
+ ((reg) = 0x8000 (reg) 0x8300) ||\
+ ((reg) = 0x8500 (reg) 0x8600) ||\
+ ((reg) = 0x9000 (reg) 0xB000) ||\
+ ((reg) = 0xC000 (reg) 0xc800) ||\
+ ((reg) = 0xF000 (reg) 0x1) ||\
+ ((reg) = 0x14000 (reg) 0x14400) ||\
+ ((reg) = 0x22000 (reg) 0x24000))
+
For these two last register ranges I couldnt find an indication
if we really need to take fw. I guess it doesnt hurt but
please re-check if these are truely needed.
static void
ilk_dummy_write(struct drm_i915_private *dev_priv)
{
@@ -588,7 +613,45 @@ vlv_read##x(struct drm_i915_private *dev_priv, off_t
reg, bool trace) { \
REG_READ_FOOTER; \
}
+#define __chv_read(x) \
+static u##x \
+chv_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
+ unsigned fwengine = 0; \
+ REG_READ_HEADER(x); \
+ if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) \
+ fwengine = FORCEWAKE_RENDER; \
+ else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) \
+ fwengine = FORCEWAKE_MEDIA; \
+ else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) \
+ fwengine = FORCEWAKE_ALL; \
+ if (FORCEWAKE_RENDER fwengine) { \
+ if (dev_priv-uncore.fw_rendercount++ == 0) \
+ (dev_priv)-uncore.funcs.force_wake_get(dev_priv, \
+ fwengine); \
+ } \
+ if (FORCEWAKE_MEDIA fwengine) { \
+ if (dev_priv-uncore.fw_mediacount++ == 0) \
+ (dev_priv)-uncore.funcs.force_wake_get(dev_priv, \
+ fwengine); \
+ } \
This patch introduces a bug in here because we end up waking up
the same well twice in a row, by delivering fwengine into the wake func.
The following patch in the series fixes this bug. You should
squash these two into a single patch.
-Mika
+ val = __raw_i915_read##x(dev_priv, reg); \
+ if (FORCEWAKE_RENDER fwengine) { \
+ if (--dev_priv-uncore.fw_rendercount == 0) \
+ (dev_priv)-uncore.funcs.force_wake_put(dev_priv, \
+ fwengine); \
+ } \
+ if (FORCEWAKE_MEDIA fwengine) { \
+ if (--dev_priv-uncore.fw_mediacount == 0) \
+ (dev_priv)-uncore.funcs.force_wake_put(dev_priv, \
+ fwengine); \
+ } \
+ REG_READ_FOOTER; \
+}
+__chv_read(8)
+__chv_read(16)
+__chv_read(32)
+__chv_read(64)
__vlv_read(8)
__vlv_read(16)
__vlv_read(32)
@@ -606,6 +669,7 @@ __gen4_read(16)
__gen4_read(32)
__gen4_read(64)
+#undef __chv_read
#undef __vlv_read
#undef __gen6_read
#undef __gen5_read
@@ -710,6 +774,46 @@ gen8_write##x(struct drm_i915_private *dev_priv, off_t
reg, u##x val, bool trace
REG_WRITE_FOOTER; \
}
+#define __chv_write(x) \
+static void \
+chv_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool
trace) { \
+ unsigned fwengine = 0; \
+ bool __needs_put = !is_gen8_shadowed(dev_priv, reg); \
+
Re: [Intel-gfx] [PATCH v2 2/2] tests/kms_flip: test a fb backed by a bo too big/small for its own good
On Fri, May 16, 2014 at 02:07:12PM +0100, oscar.ma...@intel.com wrote: From: Oscar Mateo oscar.ma...@intel.com This is a review by igt test for a bug located in i915_gem_object_pin_to_display_plane and fixed by: Thanks. Both patches pushed. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] rendercopy/bdw: Enable hw-generated binding tables
On Fri, May 16, 2014 at 03:45:24PM +0100, Chris Wilson wrote: On Fri, May 16, 2014 at 04:27:34PM +0200, Daniel Vetter wrote: On Fri, May 16, 2014 at 12:36:09PM +0100, Chris Wilson wrote: On Mon, May 12, 2014 at 06:40:43PM +0200, Daniel Vetter wrote: I think as soon as we have the golden context stuff from Mika we could drop our usage of restore_inhibit. We only need that to avoid the hw getting angry if the context state is illegal afaik. Apart from the contexts being over 100x larger than the state required to switch between clients, and that the current code regressed to always update the context between every batch. We still have the from == to early return in do_switch. That doesn't do what it should any more? No. commit 0009e46cd54324c4af20b0b52b89973b1b914167 Author: Ben Widawsky b...@bwidawsk.net Date: Fri Dec 6 14:11:02 2013 -0800 drm/i915: Track which ring a context ran on Previously we dropped the association of a context to a ring. It is however very important to know which ring a context ran on (we could have reused the other member, but I was nitpicky). This is very important when we switch address spaces, which unlike context objects, do change per ring. As an example, if we have: RCS BCS ctxA ctx A ctx B ctxB Without tracking the last ring B ran on, we wouldn't know to switch the address space on BCS in the last row. As a result, we no longer need to track which ring a context belongs to, as it never really made much sense anyway. Signed-off-by: Ben Widawsky b...@bwidawsk.net Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch was just baloney. Oh, totally forgotten about that fun. I'll add it as a new subtask to the big full ppgtt jira :( Thanks digging this out again. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] 830GM still woes
On Fri, May 16, 2014 at 03:41:05PM +0100, Chris Wilson wrote:
On Fri, May 16, 2014 at 04:02:48PM +0200, Thomas Richter wrote:
It's not that I haven't had a patch for it. Really trivial. I wonder
what keeps you from adding this to the kernel and just make things
working?
You mean this patch?
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f671aca..3981898 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -944,7 +944,7 @@ static const struct intel_watermark_params i915_wm_info =
{
static const struct intel_watermark_params i830_wm_info = {
I855GM_FIFO_SIZE,
I915_MAX_WM,
- 1,
+ 8,
2,
I830_FIFO_LINE_SIZE
};
@@ -1001,7 +1001,7 @@ static unsigned long intel_calculate_wm(unsigned long
clock_in_khz,
/* Don't promote wm_size to unsigned... */
if (wm_size (long)wm-max_wm)
wm_size = wm-max_wm;
- if (wm_size = 0)
+ if (wm_size (long)wm-default_wm)
wm_size = wm-default_wm;
return wm_size;
}
I haven't spotted any explanation as to why that is, but a rough guess
would be that we program it to read in blocks of 8 superwords and that
it tries and fails to read from memory when the fifo only has room for 1
superword.
I have it - we need to proper align watermark limits and fifo sizes and
round them apparently. Bspec at least strongly suggests that, and it would
perfectly fit Thomas' symptoms.
Unfortunately that branch is still sitting ducks somewhere on a machine :(
The problem is that we need to frob the watermark functions a bit to make
this work correctly and in all cases, and my first attempt looked horribly
and disappeared in a tangential death trap ...
Still trying to grab some time from somewhere to resurrect this.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Introduce mapping of user pages into video memory (userptr) ioctl
Reviewed-by: Brad Volkin bradley.d.vol...@intel.com On Fri, May 16, 2014 at 02:22:37PM +0100, Chris Wilson wrote: By exporting the ability to map user address and inserting PTEs representing their backing pages into the GTT, we can exploit UMA in order to utilize normal application data as a texture source or even as a render target (depending upon the capabilities of the chipset). This has a number of uses, with zero-copy downloads to the GPU and efficient readback making the intermixed streaming of CPU and GPU operations fairly efficient. This ability has many widespread implications from faster rendering of client-side software rasterisers (chromium), mitigation of stalls due to read back (firefox) and to faster pipelining of texture data (such as pixel buffer objects in GL or data blobs in CL). v2: Compile with CONFIG_MMU_NOTIFIER v3: We can sleep while performing invalidate-range, which we can utilise to drop our page references prior to the kernel manipulating the vma (for either discard or cloning) and so protect normal users. v4: Only run the invalidate notifier if the range intercepts the bo. v5: Prevent userspace from attempting to GTT mmap non-page aligned buffers v6: Recheck after reacquire mutex for lost mmu. v7: Fix implicit padding of ioctl struct by rounding to next 64bit boundary. v8: Fix rebasing error after forwarding porting the back port. v9: Limit the userptr to page aligned entries. We now expect userspace to handle all the offset-in-page adjustments itself. v10: Prevent vma from being copied across fork to avoid issues with cow. v11: Drop vma behaviour changes -- locking is nigh on impossible. Use a worker to load user pages to avoid lock inversions. v12: Use get_task_mm()/mmput() for correct refcounting of mm. v13: Use a worker to release the mmu_notifier to avoid lock inversion v14: Decouple mmu_notifier from struct_mutex using a custom mmu_notifer with its own locking and tree of objects for each mm/mmu_notifier. v15: Prevent overlapping userptr objects, and invalidate all objects within the mmu_notifier range v16: Fix a typo for iterating over multiple objects in the range and rearrange error path to destroy the mmu_notifier locklessly. Also close a race between invalidate_range and the get_pages_worker. v17: Close a race between get_pages_worker/invalidate_range and fresh allocations of the same userptr range - and notice that struct_mutex was presumed to be held when during creation it wasn't. v18: Sigh. Fix the refactor of st_set_pages() to allocate enough memory for the struct sg_table and to clear it before reporting an error. v19: Always error out on read-only userptr requests as we don't have the hardware infrastructure to support them at the moment. v20: Refuse to implement read-only support until we have the required infrastructure - but reserve the bit in flags for future use. v21: use_mm() is not required for get_user_pages(). It is only meant to be used to fix up the kernel thread's current-mm for use with copy_user(). v22: Use sg_alloc_table_from_pages for that chunky feeling v23: Export a function for sanity checking dma-buf rather than encode userptr details elsewhere, and clean up comments based on suggestions by Bradley. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Tvrtko Ursulin tvrtko.ursu...@linux.intel.com Cc: Gong, Zhipeng zhipeng.g...@intel.com Cc: Akash Goel akash.g...@intel.com Cc: Volkin, Bradley D bradley.d.vol...@intel.com Reviewed-by: Tvrtko Ursulin tvrtko.ursu...@linux.intel.com --- drivers/gpu/drm/i915/Kconfig| 1 + drivers/gpu/drm/i915/Makefile | 1 + drivers/gpu/drm/i915/i915_dma.c | 1 + drivers/gpu/drm/i915/i915_drv.h | 25 +- drivers/gpu/drm/i915/i915_gem.c | 4 + drivers/gpu/drm/i915/i915_gem_dmabuf.c | 8 + drivers/gpu/drm/i915/i915_gem_userptr.c | 711 drivers/gpu/drm/i915/i915_gpu_error.c | 2 + include/uapi/drm/i915_drm.h | 16 + 9 files changed, 768 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/i915/i915_gem_userptr.c diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig index e4e3c01b8cbc..437e1824d0bf 100644 --- a/drivers/gpu/drm/i915/Kconfig +++ b/drivers/gpu/drm/i915/Kconfig @@ -5,6 +5,7 @@ config DRM_I915 depends on (AGP || AGP=n) select INTEL_GTT select AGP_INTEL if AGP + select INTERVAL_TREE # we need shmfs for the swappable backing store, and in particular # the shmem_readpage() which depends upon tmpfs select SHMEM diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index b6ce5640d592..fa9e806259ba 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -28,6 +28,7 @@ i915-y += i915_cmd_parser.o \ i915_gem.o \
Re: [Intel-gfx] [PATCH 2/4] drm/plane-helper: Add drm_primary_helper_check_update() (v2)
On Fri, May 16, 2014 at 10:51:44AM +0800, Lee, Chon Ming wrote:
...
+int drm_primary_helper_check_update(struct drm_plane *plane,
+ struct drm_crtc *crtc,
+ struct drm_framebuffer *fb,
+ struct drm_rect *src,
+ struct drm_rect *dest,
+ const struct drm_rect *clip,
+ int min_scale,
+ int max_scale,
+ bool can_position,
+ bool can_update_disabled,
+ bool *visible)
+{
+ int hscale, vscale;
+
+ if (!crtc-enabled !can_update_disabled) {
+ DRM_DEBUG_KMS(Cannot update primary plane of a disabled
CRTC.\n);
+ return -EINVAL;
+ }
+
+ /* Check scaling */
+ hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
+ vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
+ if (hscale 0 || vscale 0) {
+ DRM_DEBUG_KMS(Invalid scaling of primary plane\n);
+ return -ERANGE;
+ }
+
+ *visible = drm_rect_clip_scaled(src, dest, clip, hscale, vscale);
+ if (!visible)
+ /*
+* Primary plane isn't visible; some drivers can handle this
+* so we just return success here. Drivers that can't
+* (including those that use the primary plane helper's
+* update function) will return an error from their
+* update_plane handler.
+*/
+ return 0;
+
+ if (!can_position !drm_rect_equals(dest, clip)) {
+ DRM_DEBUG_KMS(Primary plane must cover entire CRTC\n);
+ return -EINVAL;
+ }
Cherryview display allow the primary plane to be position at any location
similiar to sprite plane for certain port. So, this shouldn't need to check
here.
And the width/height doesn't need to cover the whole screen.
Right, but this is a general helper function in the DRM core that is
meant to be usable on all hardware and on all vendors' drivers
(including the simple primary planes that are automatically created by
helper functions for drivers that don't provide their own primary plane
support). The goal here is to centralize the common parameter checking
in one place so that all drivers don't have to duplicate the same set of
checks, but give a little bit of flexibility so that drivers for more
feature-rich hardware can relax some of the restrictions that their
hardware can actually handle (such as Cherryview being able to do
primary plane windowing as you pointed out).
It's true that the i915-specific implementation could be further
extended to pass true for the 'can_position' parameter when running on
Cherrytrail and then program the hardware accordingly, but that's really
an extra feature beyond what I'm adding here; we'd want to add that as a
follow-on patch later and come up with a whole extra set of tests to
exercise it. I'd rather focus on getting this general i915 support here
merged first, then go back and start enabling new hardware
functionalities like that on the newer platforms that can handle it.
I'll add Cherryview primary plane windowing to my TODO list for future
work if nobody beats me to it (I think some of the guys in VPG may
already be looking into this).
Matt
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling Development
Intel Corporation
(916) 356-2795
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 1/4] drm: Support legacy cursor ioctls via universal planes when possible
On Thu, May 15, 2014 at 06:17:26PM -0700, Matt Roper wrote:
If drivers support universal planes and have registered a cursor plane
with the DRM core, we should use that universal plane support when
handling legacy cursor ioctls. Drivers that transition to universal
planes won't have to maintain separate legacy ioctl handling; drivers
that don't transition to universal planes will continue to operate
without any change to behavior.
Note that there's a bit of a mismatch between the legacy cursor ioctls
and the universal plane API's --- legacy ioctl's use driver buffer
handles directly whereas the universal plane API takes drm_framebuffers.
Since there's no way to recover the driver handle from a
drm_framebuffer, we can implement legacy ioctl's in terms of universal
plane interfaces, but cannot implement universal plane interfaces in
terms of legacy ioctls. Specifically, there's no way to create a
general cursor helper in the way we previously created a primary plane
helper.
It's important to land this patch before any patches that add universal
cursor support to individual drivers so that drivers don't have to worry
about juggling two different styles of reference counting for cursor
buffers when userspace mixes and matches legacy and universal cursor
calls. With this patch, a driver that switches to universal cursor
support may assume that all cursor buffers are wrapped in a
drm_framebuffer and can rely on framebuffer reference counting for all
cursor operations.
Signed-off-by: Matt Roper matthew.d.ro...@intel.com
Some comments for polish and race avoidance below, but looks sane overall.
-Daniel
---
drivers/gpu/drm/drm_crtc.c | 108
+
include/drm/drm_crtc.h | 4 ++
2 files changed, 112 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index b6d6c04..3afdc45 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2476,6 +2476,107 @@ out:
return ret;
}
+/**
+ * drm_mode_cursor_universal - translate legacy cursor ioctl call into a
+ * universal plane handler call
+ * @crtc: crtc to update cursor for
+ * @req: data pointer for the ioctl
+ * @file_priv: drm file for the ioctl call
+ *
+ * Legacy cursor ioctl's work directly with driver buffer handles. To
+ * translate legacy ioctl calls into universal plane handler calls, we need
to
+ * wrap the native buffer handle in a drm_framebuffer.
+ *
+ * Note that we assume any handle passed to the legacy ioctls was a 32-bit
ARGB
+ * buffer with a pitch of 4*width; the universal plane interface should be
used
+ * directly in cases where the hardware can support other buffer settings and
+ * userspace wants to make use of these capabilities.
+ *
+ * Returns:
+ * Zero on success, errno on failure.
+ */
+static int drm_mode_cursor_universal(struct drm_crtc *crtc,
+ struct drm_mode_cursor2 *req,
+ struct drm_file *file_priv)
+{
+ struct drm_device *dev = crtc-dev;
+ struct drm_framebuffer *fb = NULL;
+ struct drm_mode_fb_cmd2 fbreq = {
+ .width = req-width,
+ .height = req-height,
+ .pixel_format = DRM_FORMAT_ARGB,
+ .pitches = { req-width * 4 },
+ .handles = { req-handle },
+ };
+ struct drm_mode_set_plane planereq = { 0 };
+ int ret = 0;
+
+ BUG_ON(!crtc-cursor);
+
+ if (req-flags DRM_MODE_CURSOR_BO) {
+ if (req-handle) {
+ ret = drm_mode_addfb2(dev, fbreq, file_priv);
+ if (ret) {
+ DRM_DEBUG_KMS(failed to wrap cursor buffer in
drm framebuffer\n);
+ return ret;
+ }
+
+ /*
+ * Get framebuffer we just created (but use
+ * __drm_framebuffer_lookup() so that we don't take an
+ * extra reference to it
+ */
+ mutex_lock(dev-mode_config.fb_lock);
+ fb = __drm_framebuffer_lookup(dev, fbreq.fb_id);
+ mutex_unlock(dev-mode_config.fb_lock);
Imo use the normal function here instead of open-coding and also grab a
reference for the else case. Then drop the acquired reference again later
on.
Actually I think we should extract just the parts we need from addfb2 and
directly return the drm_framebuffer *, reference included - jumping
through this lookup indirection looks fraught with races since userspace
could sneak in and rip it out from underneath you.
So the sequence would be:
1. Call -fb_create directly.
2. Grab additional reference so the fb doesn't disappear.
3. Register fb in the idr (could be extracted from addfb2), this will take
one of the references.
1.-3. alternate for !BO: Just grab reference to
Re: [Intel-gfx] [RFC 3/4] drm/i915: Add intel_crtc_cursor_set_obj() to set cursor buffer
On Thu, May 15, 2014 at 06:17:28PM -0700, Matt Roper wrote:
Refactor cursor buffer setting such that the code to actually update the
cursor lives in a new function, intel_crtc_cursor_set_obj(), and takes
a GEM object as a parameter. The existing legacy cursor ioctl handler,
intel_crtc_cursor_set() will now perform the userspace handle lookup and
then call this new function.
This refactoring is in preparation for the universal plane cursor
support where we'll want to update the cursor with an actual GEM buffer
object (obtained via drm_framebuffer) rather than a userspace handle.
Signed-off-by: Matt Roper matthew.d.ro...@intel.com
---
drivers/gpu/drm/i915/intel_display.c | 56
+---
1 file changed, 45 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index e9f196e..7e75b13 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8015,21 +8015,30 @@ static void intel_crtc_update_cursor(struct drm_crtc
*crtc,
}
}
-static int intel_crtc_cursor_set(struct drm_crtc *crtc,
- struct drm_file *file,
- uint32_t handle,
- uint32_t width, uint32_t height)
+/**
+ * intel_crtc_cursor_set_obj - Set cursor to specified GEM object
+ * @crtc: crtc to set cursor for
+ * @obj: GEM object to set cursor to
+ * @width: cursor width
+ * @height: cursor height
+ *
+ * Programs a crtc's cursor plane with the specified GEM object.
+ * intel_crtc_cursor_set() should be used instead if we have a userspace
buffer
+ * handle rather than the actual GEM object.
+ */
Imo doing kerneldoc for simple static functions doesn't add too much
value. The interesting stuff tends to be functions used all over the place
(and hence exported to files) and used to implement core driver
infrastructure.
Comments always have a cost attached since we need to keep them
up-to-date, and my impression here is that these are just stating the
obvious. Most of the kerneldoc for shared functions is also pretty much
stating the obvious, but it helps to clarify the exact semantics of a
function. Which is much more important for something shared than something
used in just one 1 file at 2 places.
If there's something tricky it's much better to have a comment right next
to the tricky code in-line, with no other fluff to distract people.
Here imo the important bit is that cursor_set will eat the reference for
the passed-in obj, for both the success and failure case.
With that addressed the patch is Reviewed-by: Daniel Vetter
daniel.vet...@ffwll.ch
+static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
+ struct drm_i915_gem_object *obj,
+ uint32_t width, uint32_t height)
{
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- struct drm_i915_gem_object *obj;
unsigned old_width;
uint32_t addr;
int ret;
/* if we want to turn off the cursor ignore width and height */
- if (!handle) {
+ if (!obj) {
DRM_DEBUG_KMS(cursor off\n);
addr = 0;
obj = NULL;
@@ -8045,12 +8054,8 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
return -EINVAL;
}
- obj = to_intel_bo(drm_gem_object_lookup(dev, file, handle));
- if (obj-base == NULL)
- return -ENOENT;
-
if (obj-base.size width * height * 4) {
- DRM_DEBUG_KMS(buffer is to small\n);
+ DRM_DEBUG_KMS(buffer is too small\n);
ret = -ENOMEM;
goto fail;
}
@@ -8138,6 +8143,35 @@ fail:
return ret;
}
+/**
+ * intel_crtc_cursor_set - Update cursor buffer
+ * @crtc: crtc to set cursor for
+ * @handle: userspace handle of buffer to set cursor to
+ * @width: cursor width
+ * @height: cursor height
+ *
+ * Programs a crtc's cursor plane with the buffer referred to by userspace
+ * handle @handle.
+ */
+static int intel_crtc_cursor_set(struct drm_crtc *crtc,
+ struct drm_file *file,
+ uint32_t handle,
+ uint32_t width, uint32_t height)
+{
+ struct drm_device *dev = crtc-dev;
+ struct drm_i915_gem_object *obj;
+
+ if (handle) {
+ obj = to_intel_bo(drm_gem_object_lookup(dev, file, handle));
+ if (obj-base == NULL)
+ return -ENOENT;
+ } else {
+ obj = NULL;
+ }
+
+ return intel_crtc_cursor_set_obj(crtc, obj, width, height);
+}
+
static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
{
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
Re: [Intel-gfx] [RFC 2/4] drm: Allow drivers to register cursor planes with crtc
On Thu, May 15, 2014 at 06:17:27PM -0700, Matt Roper wrote:
Universal plane support had placeholders for cursor planes, but didn't
actually do anything with them. Save the cursor plane reference inside
the crtc and update the cursor plane parameter from void* to drm_plane.
Signed-off-by: Matt Roper matthew.d.ro...@intel.com
Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/drm_crtc.c | 5 -
include/drm/drm_crtc.h | 2 +-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 3afdc45..036acb2 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -707,7 +707,7 @@ EXPORT_SYMBOL(drm_framebuffer_remove);
*/
int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
struct drm_plane *primary,
- void *cursor,
+ struct drm_plane *cursor,
const struct drm_crtc_funcs *funcs)
{
int ret;
@@ -730,8 +730,11 @@ int drm_crtc_init_with_planes(struct drm_device *dev,
struct drm_crtc *crtc,
dev-mode_config.num_crtc++;
crtc-primary = primary;
+ crtc-cursor = cursor;
if (primary)
primary-possible_crtcs = 1 drm_crtc_index(crtc);
+ if (cursor)
+ cursor-possible_crtcs = 1 drm_crtc_index(crtc);
out:
drm_modeset_unlock_all(dev);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index e5d22ff..ede384a 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -834,7 +834,7 @@ extern void drm_warn_on_modeset_not_all_locked(struct
drm_device *dev);
extern int drm_crtc_init_with_planes(struct drm_device *dev,
struct drm_crtc *crtc,
struct drm_plane *primary,
- void *cursor,
+ struct drm_plane *cursor,
const struct drm_crtc_funcs *funcs);
extern int drm_crtc_init(struct drm_device *dev,
struct drm_crtc *crtc,
--
1.8.5.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] 830GM still woes
On Fri, May 16, 2014 at 05:09:53PM +0200, Daniel Vetter wrote:
On Fri, May 16, 2014 at 03:41:05PM +0100, Chris Wilson wrote:
On Fri, May 16, 2014 at 04:02:48PM +0200, Thomas Richter wrote:
It's not that I haven't had a patch for it. Really trivial. I wonder
what keeps you from adding this to the kernel and just make things
working?
You mean this patch?
diff --git a/drivers/gpu/drm/i915/intel_pm.c
b/drivers/gpu/drm/i915/intel_pm.c
index f671aca..3981898 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -944,7 +944,7 @@ static const struct intel_watermark_params i915_wm_info
= {
static const struct intel_watermark_params i830_wm_info = {
I855GM_FIFO_SIZE,
I915_MAX_WM,
- 1,
+ 8,
2,
I830_FIFO_LINE_SIZE
};
@@ -1001,7 +1001,7 @@ static unsigned long intel_calculate_wm(unsigned long
clock_in_khz,
/* Don't promote wm_size to unsigned... */
if (wm_size (long)wm-max_wm)
wm_size = wm-max_wm;
- if (wm_size = 0)
+ if (wm_size (long)wm-default_wm)
wm_size = wm-default_wm;
return wm_size;
}
I haven't spotted any explanation as to why that is, but a rough guess
would be that we program it to read in blocks of 8 superwords and that
it tries and fails to read from memory when the fifo only has room for 1
superword.
I have it - we need to proper align watermark limits and fifo sizes and
round them apparently. Bspec at least strongly suggests that, and it would
perfectly fit Thomas' symptoms.
Where have you seen that? And how should they be aligned? I've never
seen anything like that in the spec. Also based on tests on my 830
it doesn't need special alignment, it just needs some kind of minumum
value that's always somewhere around 6-8 (IIRC).
I do see this note Up to FIFO Size minus burst length + 32 bytes
in one of the tables in the display doc. I can't tell if that means
'fifo_size - (burst_size + 32B)' or 'fifo_size - burst_size + 32B'.
But in any case would actually make the minimum allowed value 7 or 9
since we always configure the burst size to 8.
On Gen3 the units change to 64B but it still has the same note with
the +32B, so I'm not sure what should be done there. I guess it's
just a copy paste fumble and maybe the same minimum value should
still apply.
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 4/4] drm/i915: Switch to unified plane cursor handling
On Thu, May 15, 2014 at 06:17:29PM -0700, Matt Roper wrote:
The DRM core will translate calls to legacy cursor ioctls into universal
cursor calls automatically, so there's no need to maintain the legacy
cursor support. This greatly simplifies the transition since we don't
have to handle reference counting differently depending on which cursor
interface was called.
The aim here is to transition to the universal plane interface with
minimal code change. There's a lot of cleanup that can be done (e.g.,
using state stored in crtc-cursor-fb rather than intel_crtc) that is
left to future patches.
Signed-off-by: Matt Roper matthew.d.ro...@intel.com
Bunch of comments below. I think for testing we have good coverage of
functional cursor tests already, and with the backwards-compat code in the
drm core we'll excerise the cursor plane code sufficiently imo.
But there's a bit of corner-case checking:
- Interactions between legacy cursor ioctl and the new stuff to check the
refcounting and correctness wrt set_bo and visible and fun like that.
- Maybe some checks for the no-upscaling and no-clipping stuff. Hopefully
you can easily copypaste this from the primary plane tests.
But in the end you're worked on this and are the expert, so if you
think a different focus gives a better payoff then I'm all ears - we want
to write tests that have a good chance of catching real bugs after all ;-)
Cheers, Daniel
---
drivers/gpu/drm/i915/intel_display.c | 194
---
drivers/gpu/drm/i915/intel_drv.h | 1 -
2 files changed, 136 insertions(+), 59 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 7e75b13..d145130 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -68,6 +68,11 @@ static const uint32_t intel_primary_formats_gen4[] = {
DRM_FORMAT_ABGR2101010,
};
+/* Cursor formats */
+static const uint32_t intel_cursor_formats[] = {
+ DRM_FORMAT_ARGB,
+};
+
#define DIV_ROUND_CLOSEST_ULL(ll, d) \
({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
@@ -7967,8 +7972,8 @@ static void intel_crtc_update_cursor(struct drm_crtc
*crtc,
struct drm_i915_private *dev_priv = dev-dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
int pipe = intel_crtc-pipe;
- int x = intel_crtc-cursor_x;
- int y = intel_crtc-cursor_y;
+ int x = crtc-cursor_x;
+ int y = crtc-cursor_y;
u32 base = 0, pos = 0;
bool visible;
@@ -8023,8 +8028,6 @@ static void intel_crtc_update_cursor(struct drm_crtc
*crtc,
* @height: cursor height
*
* Programs a crtc's cursor plane with the specified GEM object.
- * intel_crtc_cursor_set() should be used instead if we have a userspace
buffer
- * handle rather than the actual GEM object.
*/
static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
struct drm_i915_gem_object *obj,
@@ -8143,48 +8146,6 @@ fail:
return ret;
}
-/**
- * intel_crtc_cursor_set - Update cursor buffer
- * @crtc: crtc to set cursor for
- * @handle: userspace handle of buffer to set cursor to
- * @width: cursor width
- * @height: cursor height
- *
- * Programs a crtc's cursor plane with the buffer referred to by userspace
- * handle @handle.
- */
-static int intel_crtc_cursor_set(struct drm_crtc *crtc,
- struct drm_file *file,
- uint32_t handle,
- uint32_t width, uint32_t height)
-{
- struct drm_device *dev = crtc-dev;
- struct drm_i915_gem_object *obj;
-
- if (handle) {
- obj = to_intel_bo(drm_gem_object_lookup(dev, file, handle));
- if (obj-base == NULL)
- return -ENOENT;
- } else {
- obj = NULL;
- }
-
- return intel_crtc_cursor_set_obj(crtc, obj, width, height);
-}
-
-static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
-{
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-
- intel_crtc-cursor_x = clamp_t(int, x, SHRT_MIN, SHRT_MAX);
- intel_crtc-cursor_y = clamp_t(int, y, SHRT_MIN, SHRT_MAX);
-
- if (intel_crtc-active)
- intel_crtc_update_cursor(crtc, intel_crtc-cursor_bo != NULL);
-
- return 0;
-}
-
static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
u16 *blue, uint32_t start, uint32_t size)
{
@@ -8806,7 +8767,7 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
kfree(work);
}
- intel_crtc_cursor_set(crtc, NULL, 0, 0, 0);
+ intel_crtc_cursor_set_obj(crtc, NULL, 0, 0);
This shouldn't be needed any more with the plane cleanup code - all the
refcounting is now done with fbs.
drm_crtc_cleanup(crtc);
@@ -10784,8 +10745,6 @@
[Intel-gfx] [PATCH] drm/i915: Sanitize cursors on driver load
Extremely unlikely to ever be required, but BIOSes do like to attack in
unexpected ways.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
---
drivers/gpu/drm/i915/intel_display.c |2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index a943ea7..5583e9b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11817,6 +11817,8 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
/* Adjust the state of the output pipe according to whether we
* have active connectors/encoders. */
intel_crtc_update_dpms(crtc-base);
+ intel_crtc_update_cursor(crtc,
+intel_crtc-active intel_crtc-cursor_bo);
if (crtc-active != crtc-base.enabled) {
struct intel_encoder *encoder;
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Introduce mapping of user pages into video memory (userptr) ioctl
On Fri, May 16, 2014 at 08:34:52AM -0700, Volkin, Bradley D wrote: Reviewed-by: Brad Volkin bradley.d.vol...@intel.com On Fri, May 16, 2014 at 02:22:37PM +0100, Chris Wilson wrote: By exporting the ability to map user address and inserting PTEs representing their backing pages into the GTT, we can exploit UMA in order to utilize normal application data as a texture source or even as a render target (depending upon the capabilities of the chipset). This has a number of uses, with zero-copy downloads to the GPU and efficient readback making the intermixed streaming of CPU and GPU operations fairly efficient. This ability has many widespread implications from faster rendering of client-side software rasterisers (chromium), mitigation of stalls due to read back (firefox) and to faster pipelining of texture data (such as pixel buffer objects in GL or data blobs in CL). v2: Compile with CONFIG_MMU_NOTIFIER v3: We can sleep while performing invalidate-range, which we can utilise to drop our page references prior to the kernel manipulating the vma (for either discard or cloning) and so protect normal users. v4: Only run the invalidate notifier if the range intercepts the bo. v5: Prevent userspace from attempting to GTT mmap non-page aligned buffers v6: Recheck after reacquire mutex for lost mmu. v7: Fix implicit padding of ioctl struct by rounding to next 64bit boundary. v8: Fix rebasing error after forwarding porting the back port. v9: Limit the userptr to page aligned entries. We now expect userspace to handle all the offset-in-page adjustments itself. v10: Prevent vma from being copied across fork to avoid issues with cow. v11: Drop vma behaviour changes -- locking is nigh on impossible. Use a worker to load user pages to avoid lock inversions. v12: Use get_task_mm()/mmput() for correct refcounting of mm. v13: Use a worker to release the mmu_notifier to avoid lock inversion v14: Decouple mmu_notifier from struct_mutex using a custom mmu_notifer with its own locking and tree of objects for each mm/mmu_notifier. v15: Prevent overlapping userptr objects, and invalidate all objects within the mmu_notifier range v16: Fix a typo for iterating over multiple objects in the range and rearrange error path to destroy the mmu_notifier locklessly. Also close a race between invalidate_range and the get_pages_worker. v17: Close a race between get_pages_worker/invalidate_range and fresh allocations of the same userptr range - and notice that struct_mutex was presumed to be held when during creation it wasn't. v18: Sigh. Fix the refactor of st_set_pages() to allocate enough memory for the struct sg_table and to clear it before reporting an error. v19: Always error out on read-only userptr requests as we don't have the hardware infrastructure to support them at the moment. v20: Refuse to implement read-only support until we have the required infrastructure - but reserve the bit in flags for future use. v21: use_mm() is not required for get_user_pages(). It is only meant to be used to fix up the kernel thread's current-mm for use with copy_user(). v22: Use sg_alloc_table_from_pages for that chunky feeling v23: Export a function for sanity checking dma-buf rather than encode userptr details elsewhere, and clean up comments based on suggestions by Bradley. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Tvrtko Ursulin tvrtko.ursu...@linux.intel.com Cc: Gong, Zhipeng zhipeng.g...@intel.com Cc: Akash Goel akash.g...@intel.com Cc: Volkin, Bradley D bradley.d.vol...@intel.com Reviewed-by: Tvrtko Ursulin tvrtko.ursu...@linux.intel.com Bring on the champagne! Queued for -next, thanks for the patch. -Daniel --- drivers/gpu/drm/i915/Kconfig| 1 + drivers/gpu/drm/i915/Makefile | 1 + drivers/gpu/drm/i915/i915_dma.c | 1 + drivers/gpu/drm/i915/i915_drv.h | 25 +- drivers/gpu/drm/i915/i915_gem.c | 4 + drivers/gpu/drm/i915/i915_gem_dmabuf.c | 8 + drivers/gpu/drm/i915/i915_gem_userptr.c | 711 drivers/gpu/drm/i915/i915_gpu_error.c | 2 + include/uapi/drm/i915_drm.h | 16 + 9 files changed, 768 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/i915/i915_gem_userptr.c diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig index e4e3c01b8cbc..437e1824d0bf 100644 --- a/drivers/gpu/drm/i915/Kconfig +++ b/drivers/gpu/drm/i915/Kconfig @@ -5,6 +5,7 @@ config DRM_I915 depends on (AGP || AGP=n) select INTEL_GTT select AGP_INTEL if AGP + select INTERVAL_TREE # we need shmfs for the swappable backing store, and in particular # the shmem_readpage() which depends upon tmpfs select SHMEM diff --git a/drivers/gpu/drm/i915/Makefile
Re: [Intel-gfx] [PATCH 05/11] drm/i915: Do not try to enable PSR when Panel doesn't suport it.
On Fri, May 16, 2014 at 3:21 AM, Chris Wilson ch...@chris-wilson.co.ukwrote: On Thu, May 15, 2014 at 08:13:04PM -0400, Rodrigo Vivi wrote: Also do not cache aux info. That info could be related to another panel. You should kill the bool sink_support then. There are other places that it used that could be invalid. the point here was to keep the info for debugfs in a simple way. I'm not sure though that we can cope with eDP panels being swapped at runtime. the point wan't swap, but the possibility of having more than one eDP in the future, but anyway other parts of the could should be changed to support this. so maybe just get back to the origial version keeping it on sink_support... -Chris -- Chris Wilson, Intel Open Source Technology Centre -- Rodrigo Vivi Blog: http://blog.vivi.eng.br ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/5] drm/i915: Some FIFO underrun detection improvements
From: Ville Syrjälä ville.syrj...@linux.intel.com I was looking at FIFO underruns a bit and came up with a few improvements to the FIFO underrun reporting code. Mainly this should lead to detecting FIFO a bit more more reliably on gmch platforms. I also promoted the IVB/CPT uncleared FIFO underrun messages to errors since that's what we do everywhere else as well. Ville Syrjälä (5): drm/i915: Check for FIFO underuns when disabling reporting on gmch platforms drm/i915: Check for FIFO underruns at the end of modeset on gmch drm/i915: Convert uncleared FIFO underrun message to errors drm/i915: Simplify the uncleared FIFO underrun detection drm/i915: Shuffle fifo underrun disable/enable points for gmch platforms drivers/gpu/drm/i915/i915_irq.c | 67 +--- drivers/gpu/drm/i915/intel_display.c | 36 +-- drivers/gpu/drm/i915/intel_drv.h | 1 + 3 files changed, 80 insertions(+), 24 deletions(-) -- 1.8.5.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/5] drm/i915: Check for FIFO underuns when disabling reporting on gmch platforms
From: Ville Syrjälä ville.syrj...@linux.intel.com
FIFO underruns don't generate an interrupt on gmch platforms, so we
should check whether there were any that we failed to notice when
we're disabling FIFO underrun reporting.
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_irq.c | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b10fbde..8bb564b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -266,16 +266,22 @@ static bool cpt_can_enable_serr_int(struct drm_device
*dev)
return true;
}
-static void i9xx_clear_fifo_underrun(struct drm_device *dev, enum pipe pipe)
+static void i9xx_set_fifo_underrun_reporting(struct drm_device *dev,
+enum pipe pipe, bool enable)
{
struct drm_i915_private *dev_priv = dev-dev_private;
u32 reg = PIPESTAT(pipe);
- u32 pipestat = I915_READ(reg) 0x7fff;
+ u32 pipestat = I915_READ(reg) 0x;
assert_spin_locked(dev_priv-irq_lock);
- I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
- POSTING_READ(reg);
+ if (enable) {
+ I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
+ POSTING_READ(reg);
+ } else {
+ if (pipestat PIPE_FIFO_UNDERRUN_STATUS)
+ DRM_ERROR(pipe %c underrun\n, pipe_name(pipe));
+ }
}
static void ironlake_set_fifo_underrun_reporting(struct drm_device *dev,
@@ -432,8 +438,8 @@ bool __intel_set_cpu_fifo_underrun_reporting(struct
drm_device *dev,
intel_crtc-cpu_fifo_underrun_disabled = !enable;
- if (enable (INTEL_INFO(dev)-gen 5 || IS_VALLEYVIEW(dev)))
- i9xx_clear_fifo_underrun(dev, pipe);
+ if (INTEL_INFO(dev)-gen 5 || IS_VALLEYVIEW(dev))
+ i9xx_set_fifo_underrun_reporting(dev, pipe, enable);
else if (IS_GEN5(dev) || IS_GEN6(dev))
ironlake_set_fifo_underrun_reporting(dev, pipe, enable);
else if (IS_GEN7(dev))
--
1.8.5.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/5] drm/i915: Simplify the uncleared FIFO underrun detection
From: Ville Syrjälä ville.syrj...@linux.intel.com
Checking whether the error interrupt was enabled or not isn't really
necessary when we check for uncleared FIFO underruns. If it was enabled
we'll race with the interrupt handler a bit, but that seems OK as we
still claim the interrupt.
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_irq.c | 13 ++---
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 862964f..dd6e359 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -337,13 +337,9 @@ static void ivybridge_set_fifo_underrun_reporting(struct
drm_device *dev,
ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
} else {
- bool was_enabled = !(I915_READ(DEIMR) DE_ERR_INT_IVB);
-
- /* Change the state _after_ we've read out the current one. */
ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
- if (!was_enabled
- (I915_READ(GEN7_ERR_INT) ERR_INT_FIFO_UNDERRUN(pipe))) {
+ if (I915_READ(GEN7_ERR_INT) ERR_INT_FIFO_UNDERRUN(pipe)) {
DRM_ERROR(uncleared fifo underrun on pipe %c\n,
pipe_name(pipe));
}
@@ -421,14 +417,9 @@ static void cpt_set_fifo_underrun_reporting(struct
drm_device *dev,
ibx_enable_display_interrupt(dev_priv, SDE_ERROR_CPT);
} else {
- uint32_t tmp = I915_READ(SERR_INT);
- bool was_enabled = !(I915_READ(SDEIMR) SDE_ERROR_CPT);
-
- /* Change the state _after_ we've read out the current one. */
ibx_disable_display_interrupt(dev_priv, SDE_ERROR_CPT);
- if (!was_enabled
- (tmp SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder))) {
+ if (I915_READ(SERR_INT)
SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder)) {
DRM_ERROR(uncleared pch fifo underrun on pch
transcoder %c\n,
transcoder_name(pch_transcoder));
}
--
1.8.5.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 5/5] drm/i915: Shuffle fifo underrun disable/enable points for gmch platforms
From: Ville Syrjälä ville.syrj...@linux.intel.com Gen2 reports FIFO underruns whenever no planes are enabled on the pipe. So in order to avoid false positives we must enable the FIFO underrun reporting only when at least one plane is enabled on the pipe. For now just move the underrun reporting enable/disable points to the other side of the plane enable/disable point. That doesn't cover cases when we turn off all the planes for the pipe but leave the pipe running on purpose, but it's better than the current situation. On gen4+ we can actually move the underrun reporting enable/disable to the opposite ends of the crtc enable/disable hooks. I suppose in theory we could leave the underrun reporting enabled all the time, except on VLV where PIPESTAT stops working when the display power well is down. If we ever get around to unifying the PIPESTAT irq handling for all gmch platforms, we should still follow the VLV route for other platforms. It would also micro-optimize the irq handler a bit since we could then skip the PIPESTAT reads for all disabled pipes. Gen3 is still a mystery, but for now I'm going to assume it behaves like gen4+. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/intel_display.c | 30 +++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1b5164c..7f61047 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4514,6 +4514,8 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc) intel_crtc-active = true; + intel_set_cpu_fifo_underrun_reporting(dev, pipe, true); + for_each_encoder_on_crtc(dev, crtc, encoder) if (encoder-pre_pll_enable) encoder-pre_pll_enable(encoder); @@ -4537,7 +4539,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc) intel_update_watermarks(crtc); intel_enable_pipe(intel_crtc); - intel_set_cpu_fifo_underrun_reporting(dev, pipe, true); for_each_encoder_on_crtc(dev, crtc, encoder) encoder-enable(encoder); @@ -4564,6 +4565,9 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) intel_crtc-active = true; + if (!IS_GEN2(dev)) + intel_set_cpu_fifo_underrun_reporting(dev, pipe, true); + for_each_encoder_on_crtc(dev, crtc, encoder) if (encoder-pre_enable) encoder-pre_enable(encoder); @@ -4576,13 +4580,22 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) intel_update_watermarks(crtc); intel_enable_pipe(intel_crtc); - intel_set_cpu_fifo_underrun_reporting(dev, pipe, true); for_each_encoder_on_crtc(dev, crtc, encoder) encoder-enable(encoder); intel_crtc_enable_planes(crtc); + /* +* Gen2 reports pipe underruns whenever all planes are disabled. +* So don't enable underrun reporting before at least some planes +* are enabled. +* FIXME: Need to fix the logic to work when we turn off all planes +* but leave the pipe running. +*/ + if (IS_GEN2(dev)) + intel_set_cpu_fifo_underrun_reporting(dev, pipe, true); + drm_vblank_on(dev, pipe); /* Underruns don't raise interrupts, so check manually. */ @@ -4615,12 +4628,20 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) if (!intel_crtc-active) return; + /* +* Gen2 reports pipe underruns whenever all planes are disabled. +* So diasble underrun reporting before all the planes get disabled. +* FIXME: Need to fix the logic to work when we turn off all planes +* but leave the pipe running. +*/ + if (IS_GEN2(dev)) + intel_set_cpu_fifo_underrun_reporting(dev, pipe, false); + intel_crtc_disable_planes(crtc); for_each_encoder_on_crtc(dev, crtc, encoder) encoder-disable(encoder); - intel_set_cpu_fifo_underrun_reporting(dev, pipe, false); intel_disable_pipe(dev_priv, pipe); i9xx_pfit_disable(intel_crtc); @@ -4638,6 +4659,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) i9xx_disable_pll(dev_priv, pipe); } + if (!IS_GEN2(dev)) + intel_set_cpu_fifo_underrun_reporting(dev, pipe, false); + intel_crtc-active = false; intel_update_watermarks(crtc); -- 1.8.5.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/5] drm/i915: Check for FIFO underruns at the end of modeset on gmch
From: Ville Syrjälä ville.syrj...@linux.intel.com
FIFO underruns don't generate interrupts on gmch platforms, so
if we want to know whether a modeset triggered FIFO underruns we
need to explicitly check for them.
As a modeset on one pipe could cause underruns on other pipes,
check for underruns on all pipes.
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_irq.c | 28
drivers/gpu/drm/i915/intel_display.c | 6 ++
drivers/gpu/drm/i915/intel_drv.h | 1 +
3 files changed, 35 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 8bb564b..fdce260 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -266,6 +266,34 @@ static bool cpt_can_enable_serr_int(struct drm_device *dev)
return true;
}
+void i9xx_check_fifo_underruns(struct drm_device *dev)
+{
+ struct drm_i915_private *dev_priv = dev-dev_private;
+ struct intel_crtc *crtc;
+ unsigned long flags;
+
+ spin_lock_irqsave(dev_priv-irq_lock, flags);
+
+ for_each_intel_crtc(dev, crtc) {
+ u32 reg = PIPESTAT(crtc-pipe);
+ u32 pipestat;
+
+ if (crtc-cpu_fifo_underrun_disabled)
+ continue;
+
+ pipestat = I915_READ(reg) 0x;
+ if ((pipestat PIPE_FIFO_UNDERRUN_STATUS) == 0)
+ continue;
+
+ I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
+ POSTING_READ(reg);
+
+ DRM_ERROR(pipe %c underrun\n, pipe_name(crtc-pipe));
+ }
+
+ spin_unlock_irqrestore(dev_priv-irq_lock, flags);
+}
+
static void i9xx_set_fifo_underrun_reporting(struct drm_device *dev,
enum pipe pipe, bool enable)
{
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 0f8f9bc..1b5164c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4545,6 +4545,9 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
intel_crtc_enable_planes(crtc);
drm_vblank_on(dev, pipe);
+
+ /* Underruns don't raise interrupts, so check manually. */
+ i9xx_check_fifo_underruns(dev);
}
static void i9xx_crtc_enable(struct drm_crtc *crtc)
@@ -4581,6 +4584,9 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
intel_crtc_enable_planes(crtc);
drm_vblank_on(dev, pipe);
+
+ /* Underruns don't raise interrupts, so check manually. */
+ i9xx_check_fifo_underruns(dev);
}
static void i9xx_pfit_disable(struct intel_crtc *crtc)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 32a74e1..db0a74d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -671,6 +671,7 @@ void snb_disable_pm_irq(struct drm_i915_private *dev_priv,
uint32_t mask);
void intel_runtime_pm_disable_interrupts(struct drm_device *dev);
void intel_runtime_pm_restore_interrupts(struct drm_device *dev);
int intel_get_crtc_scanline(struct intel_crtc *crtc);
+void i9xx_check_fifo_underruns(struct drm_device *dev);
/* intel_crt.c */
--
1.8.5.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/5] drm/i915: Convert uncleared FIFO underrun message to errors
From: Ville Syrjälä ville.syrj...@linux.intel.com
Some platforms have a shared error interrupt, so if FIFO underrun
reporting gets disabled for one pipe/transcoder it gets disabled
for all pipes/transcoders.
When we disable FIFO underrun reporting we check whether the
interrupt was enabled or not. If it wasn't we might have missed
an underrun and we perform one last check right there. Currently
we print a debug message when an underrun is detect using this
mechanism. Promote the message to DRM_ERROR() to match the other
underrun error messages.
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_irq.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index fdce260..862964f 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -344,8 +344,8 @@ static void ivybridge_set_fifo_underrun_reporting(struct
drm_device *dev,
if (!was_enabled
(I915_READ(GEN7_ERR_INT) ERR_INT_FIFO_UNDERRUN(pipe))) {
- DRM_DEBUG_KMS(uncleared fifo underrun on pipe %c\n,
- pipe_name(pipe));
+ DRM_ERROR(uncleared fifo underrun on pipe %c\n,
+ pipe_name(pipe));
}
}
}
@@ -429,8 +429,8 @@ static void cpt_set_fifo_underrun_reporting(struct
drm_device *dev,
if (!was_enabled
(tmp SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder))) {
- DRM_DEBUG_KMS(uncleared pch fifo underrun on pch
transcoder %c\n,
- transcoder_name(pch_transcoder));
+ DRM_ERROR(uncleared pch fifo underrun on pch
transcoder %c\n,
+ transcoder_name(pch_transcoder));
}
}
}
--
1.8.5.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/11] drm/i915: Force PSR exit by inactivating it.
On Fri, May 16, 2014 at 3:23 AM, Chris Wilson ch...@chris-wilson.co.ukwrote: On Thu, May 15, 2014 at 08:13:05PM -0400, Rodrigo Vivi wrote: The perfect solution for psr_exit is the hardware tracking the changes and doing the psr exit by itself. This scenario works for HSW and BDW with some environments like Gnome and Wayland. However there are many other scenarios that this isn't true. Mainly one right now is KDE users on HSW and BDW with PSR on. User would miss many screen updates. For instances any key typed could be seen only when mouse cursor is moved. So this patch introduces the ability of trigger PSR exit on kernel side on some common cases that. You know that userspace has been waiting for a PSR flag for over a year now so that it can use the more efficient rendering paths when it makes sense. yeah... this item is lingering on my to do list... but reaching a point where I won't be able to continue postponing it ;) What happened to the front buffer tracking? What front buffer tracking? hehe I'm wondering about this since I started looking to fbc and psr and could never find a reliable way. -Chris -- Chris Wilson, Intel Open Source Technology Centre -- Rodrigo Vivi Blog: http://blog.vivi.eng.br ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PULL] drm-intel-next
Hi Dave, drm-intel-next-2014-05-06: - ring init improvements (Chris) - vebox2 support (Zhao Yakui) - more prep work for runtime pm on Baytrail (Imre) - eDram support for BDW (Ben) - prep work for userptr support (Chris) - first parts of the encoder-mode_set callback removal (Daniel) - 64b reloc fixes (Ben) - first part of atomic plane updates (Ville) Also another ping about topic/core-stuff and for pushing out drm-next with the properties doc patch applied. Cheers, Daniel The following changes since commit 444c9a08bf787e8236e132fab7eceeb2f065aa4c: Merge branch 'drm-init-cleanup' of git://people.freedesktop.org/~danvet/drm into drm-next (2014-05-01 09:32:21 +1000) are available in the git repository at: git://anongit.freedesktop.org/drm-intel tags/drm-intel-next-2014-05-06 for you to fetch changes up to 10efa9321efe5f62637b189587539e4086726a2b: drm/i915: Remove useless checks from primary enable/disable (2014-05-06 10:18:04 +0200) - ring init improvements (Chris) - vebox2 support (Zhao Yakui) - more prep work for runtime pm on Baytrail (Imre) - eDram support for BDW (Ben) - prep work for userptr support (Chris) - first parts of the encoder-mode_set callback removal (Daniel) - 64b reloc fixes (Ben) - first part of atomic plane updates (Ville) Ben Widawsky (8): drm/i915/bdw: Add WT caching ability drm/i915/bdw: enable eDRAM. drm/i915/bdw: Disable idle DOP clock gating drm/i915: Move semaphore specific ring members to struct drm/i915: Virtualize the ringbuffer signal func drm/i915: Move ring_begin to signal() drm/i915: Support 64b execbuf drm/i915: Support 64b relocations Chris Wilson (10): drm/i915: Replace hardcoded cacheline size with macro drm/i915: Preserve ring buffers objects across resume drm/i915: Allow the module to load even if we fail to setup rings drm/i915: Mark device as wedged if we fail to resume drm/i915: Include a little more information about why ring init fails drm/i915: Validate BDB section before reading drm/i915: Validate VBT header before trusting it lib: Export interval_tree drm/i915: Do not call retire_requests from wait_for_rendering drm/i915: Avoid NULL ctx-obj dereference in debugfs/i915_context_info Daniel Vetter (13): drm/i915: Catch abuse of I915_EXEC_GEN7_SOL_RESET drm/i915: Catch abuse of I915_EXEC_CONSTANTS_* drm/i915: Catch dirt in unused execbuffer fields drm/i915: Integrate cmd parser kerneldoc drm/i915: Make encoder-mode_set callbacks optional drm/i915/dvo: Remove -mode_set callback drm/i915/tv: extract set_tv_mode_timings drm/i915/tv: extract set_color_conversion drm/i915/tv: De-magic device check drm/i915/tv: Rip out pipe-disabling nonsense from -mode_set drm/i915/tv: Remove -mode_set callback drm/i915/crt: Remove -mode_set callback drm/i915/sdvo: Remove -mode_set callback Imre Deak (25): drm/i915: vlv: clean up GTLC wake control/status register macros drm/i915: vlv: clear master interrupt flag when disabling interrupts drm/i915: vlv: add RC6 residency counters drm/i915: fix the RC6 status debug print drm/i915: remove the i915_dpio debugfs entry drm/i915: get a runtime PM ref for debugfs entries where needed drm/i915: move getting struct_mutex lower in the callstack during GPU reset drm/i915: get a runtime PM ref for the deferred GT powersave enabling drm/i915: get a runtime PM ref for the deferred GPU reset work drm/i915: gen2: move error capture of IER to its correct place drm/i915: add missing error capturing of the PIPESTAT reg drm/i915: vlv: check port power domain instead of only D0 for eDP VDD on drm/i915: fix unbalanced GT powersave enable / disable calls drm/i915: sanitize enable_rc6 option drm/i915: disable runtime PM if RC6 is disabled drm/i915: make runtime PM interrupt enable/disable platform independent drm/i915: factor out gen6_update_ring_freq drm/i915: make runtime PM swizzling/ring_freq init platform independent drm/i915: reinit GT power save during resume drm/i915: vlv: setup RPS min/max frequencies once during init time drm/i915: vlv: factor out vlv_force_gfx_clock and check for pending force-off drm/i915: vlv: increase timeout when forcing on the GFX clock drm/i915: remove extraneous VGA power domain put calls drm/i915: bdw: fix RC6 enabled status reporting and disable runtime PM drm/i915: vlv: init only needed state during early power well enabling Jan Moskyto Matejka (1): Revert drm/i915: fix build warning on 32-bit (v2) Jesse Barnes (1): drm/i915: remove unexplained vblank wait in the DP off code Ville Syrjälä (11): drm/i915: Fix deadlock
Re: [Intel-gfx] 830GM still woes
On Fri, May 16, 2014 at 07:04:54PM +0300, Ville Syrjälä wrote:
On Fri, May 16, 2014 at 05:09:53PM +0200, Daniel Vetter wrote:
On Fri, May 16, 2014 at 03:41:05PM +0100, Chris Wilson wrote:
On Fri, May 16, 2014 at 04:02:48PM +0200, Thomas Richter wrote:
It's not that I haven't had a patch for it. Really trivial. I wonder
what keeps you from adding this to the kernel and just make things
working?
You mean this patch?
diff --git a/drivers/gpu/drm/i915/intel_pm.c
b/drivers/gpu/drm/i915/intel_pm.c
index f671aca..3981898 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -944,7 +944,7 @@ static const struct intel_watermark_params
i915_wm_info = {
static const struct intel_watermark_params i830_wm_info = {
I855GM_FIFO_SIZE,
I915_MAX_WM,
- 1,
+ 8,
2,
I830_FIFO_LINE_SIZE
};
@@ -1001,7 +1001,7 @@ static unsigned long intel_calculate_wm(unsigned
long clock_in_khz,
/* Don't promote wm_size to unsigned... */
if (wm_size (long)wm-max_wm)
wm_size = wm-max_wm;
- if (wm_size = 0)
+ if (wm_size (long)wm-default_wm)
wm_size = wm-default_wm;
return wm_size;
}
I haven't spotted any explanation as to why that is, but a rough guess
would be that we program it to read in blocks of 8 superwords and that
it tries and fails to read from memory when the fifo only has room for 1
superword.
I have it - we need to proper align watermark limits and fifo sizes and
round them apparently. Bspec at least strongly suggests that, and it would
perfectly fit Thomas' symptoms.
Where have you seen that? And how should they be aligned? I've never
seen anything like that in the spec. Also based on tests on my 830
it doesn't need special alignment, it just needs some kind of minumum
value that's always somewhere around 6-8 (IIRC).
I do see this note Up to FIFO Size minus burst length + 32 bytes
in one of the tables in the display doc. I can't tell if that means
'fifo_size - (burst_size + 32B)' or 'fifo_size - burst_size + 32B'.
But in any case would actually make the minimum allowed value 7 or 9
since we always configure the burst size to 8.
On Gen3 the units change to 64B but it still has the same note with
the +32B, so I'm not sure what should be done there. I guess it's
just a copy paste fumble and maybe the same minimum value should
still apply.
Yeah the burst size stuff - afaiu we should select the biggest one
possible and if that's not working out round the watermark up to match the
burst size. I didn't spot the +32/-32bytes anywhere though ... I guess
going with burst_size + 1 should be safest, especially if we make the code
more flexible to also allow a burst size of 4 for the really high-res
stuff.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Sanitize cursors on driver load
On Fri, May 16, 2014 at 05:36:59PM +0100, Chris Wilson wrote:
Extremely unlikely to ever be required, but BIOSes do like to attack in
unexpected ways.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
---
drivers/gpu/drm/i915/intel_display.c |2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index a943ea7..5583e9b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11817,6 +11817,8 @@ static void intel_sanitize_crtc(struct intel_crtc
*crtc)
/* Adjust the state of the output pipe according to whether we
* have active connectors/encoders. */
intel_crtc_update_dpms(crtc-base);
+ intel_crtc_update_cursor(crtc,
+ intel_crtc-active intel_crtc-cursor_bo);
Should we do this for sprite planes, too? That way it would be nice fodder
for Matt to clean up later on ;-)
-Daniel
if (crtc-active != crtc-base.enabled) {
struct intel_encoder *encoder;
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/5] drm/i915: Convert uncleared FIFO underrun message to errors
On Fri, May 16, 2014 at 07:40:23PM +0300, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
Some platforms have a shared error interrupt, so if FIFO underrun
reporting gets disabled for one pipe/transcoder it gets disabled
for all pipes/transcoders.
When we disable FIFO underrun reporting we check whether the
interrupt was enabled or not. If it wasn't we might have missed
an underrun and we perform one last check right there. Currently
we print a debug message when an underrun is detect using this
mechanism. Promote the message to DRM_ERROR() to match the other
underrun error messages.
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
Queued for -next, thanks for the patch.
-Daniel
---
drivers/gpu/drm/i915/i915_irq.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index fdce260..862964f 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -344,8 +344,8 @@ static void ivybridge_set_fifo_underrun_reporting(struct
drm_device *dev,
if (!was_enabled
(I915_READ(GEN7_ERR_INT) ERR_INT_FIFO_UNDERRUN(pipe))) {
- DRM_DEBUG_KMS(uncleared fifo underrun on pipe %c\n,
- pipe_name(pipe));
+ DRM_ERROR(uncleared fifo underrun on pipe %c\n,
+ pipe_name(pipe));
}
}
}
@@ -429,8 +429,8 @@ static void cpt_set_fifo_underrun_reporting(struct
drm_device *dev,
if (!was_enabled
(tmp SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder))) {
- DRM_DEBUG_KMS(uncleared pch fifo underrun on pch
transcoder %c\n,
- transcoder_name(pch_transcoder));
+ DRM_ERROR(uncleared pch fifo underrun on pch
transcoder %c\n,
+ transcoder_name(pch_transcoder));
}
}
}
--
1.8.5.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Sanitize cursors on driver load
On Fri, May 16, 2014 at 06:53:52PM +0200, Daniel Vetter wrote: On Fri, May 16, 2014 at 05:36:59PM +0100, Chris Wilson wrote: Extremely unlikely to ever be required, but BIOSes do like to attack in unexpected ways. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk --- drivers/gpu/drm/i915/intel_display.c |2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a943ea7..5583e9b 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11817,6 +11817,8 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) /* Adjust the state of the output pipe according to whether we * have active connectors/encoders. */ intel_crtc_update_dpms(crtc-base); + intel_crtc_update_cursor(crtc, +intel_crtc-active intel_crtc-cursor_bo); Should we do this for sprite planes, too? That way it would be nice fodder for Matt to clean up later on ;-) * pokes Ville ;-) -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Sanitize cursors on driver load
On Fri, May 16, 2014 at 06:02:29PM +0100, Chris Wilson wrote: On Fri, May 16, 2014 at 06:53:52PM +0200, Daniel Vetter wrote: On Fri, May 16, 2014 at 05:36:59PM +0100, Chris Wilson wrote: Extremely unlikely to ever be required, but BIOSes do like to attack in unexpected ways. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk --- drivers/gpu/drm/i915/intel_display.c |2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a943ea7..5583e9b 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11817,6 +11817,8 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) /* Adjust the state of the output pipe according to whether we * have active connectors/encoders. */ intel_crtc_update_dpms(crtc-base); + intel_crtc_update_cursor(crtc, + intel_crtc-active intel_crtc-cursor_bo); Should we do this for sprite planes, too? That way it would be nice fodder for Matt to clean up later on ;-) * pokes Ville ;-) The problem I see here is that we would end up restoring twice, first in sanitize while we're still using whatever mode we get handed, and later when we restore the mode we actually want. So if we start restoring in sanitize based on the state userspace requested before we suspended, we might end up in some weird plane flicker land. Just forcing all the extra planes off in sanitize should be OK. Or we do the restore only when force_restore==false, which means driver init and so we can't even have any user space state to worry about. So really the two options should both result in just turning all the extra planes off. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [3.14.0-rc4] regression: drm FIFO underruns
2014-05-16 13:53 GMT+02:00 Ville Syrjälä ville.syrj...@linux.intel.com: On Tue, May 13, 2014 at 06:38:32PM +0200, Daniel Vetter wrote: On Tue, May 13, 2014 at 05:21:49PM +0200, Jörg Otte wrote: 2014-05-13 15:22 GMT+02:00 Daniel Vetter dan...@ffwll.ch: On Tue, May 13, 2014 at 12:38:41PM +0200, Daniel Vetter wrote: On Tue, May 13, 2014 at 12:29 PM, Jörg Otte jrg.o...@gmail.com wrote: Branch drm-intel-nightly as of ed60c27 drm-intel-nightly: 2014y-05m-09d-21h-51m-45s integration manifest looks badly: - KDE splash screen on boot-up is not visible - x-windows don't have title and menu bars - KDE system menu is not visible - moving windows around destroys its content Ugh, that's ugly. Nothing else change like e.g. the version of xfree-video-intel? (II) Loading /usr/lib/xorg/modules/drivers/intel_drv.so (II) Module intel: vendor=X.Org Foundation compiled for 1.11.3, module version = 2.17.0 Module class: X.Org Video Driver ABI class: X.Org Video Driver, version 11.0 Chris, any ideas? It's an ivybridge apparently. For the fifo underruns I think we've fully confirmed that they only happen on boot-up. I'll try to come up with some ideas what could have gone wrong there. Please test the below patch. -Daniel diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index b10fbde1d5ee..63ced2dee027 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -427,9 +427,6 @@ bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev, ret = !intel_crtc-cpu_fifo_underrun_disabled; - if (enable == ret) - goto done; - intel_crtc-cpu_fifo_underrun_disabled = !enable; if (enable (INTEL_INFO(dev)-gen 5 || IS_VALLEYVIEW(dev))) @@ -441,7 +438,6 @@ bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev, else if (IS_GEN8(dev)) broadwell_set_fifo_underrun_reporting(dev, pipe, enable); -done: return ret; } -- Doesn't work for me, I still have an underrun at boot-up. I'm at a loss tbh with ideas. We successfully disable both pipes, then enable pipe A and it all works. Then we enable pipe B and _both_ pipes underrun immediately afterwards. Really strange. Can you please reproduce the issue again on drm-intel-nightly (latest -nightly should also have the display corruptions fixed, so good to retest anyway) and attach a new dmesg with drm.debug=0xe. I see a few underrun on my IVB as well. But it seems to be limited to cases that involve the VGA connector, which doesn't actually exist on this machine so I can't be sure if it's really properly set up on the PCH. But so far with just two HDMI connectors I was unable to reproduce it. Meanwhile I'll try to come up with new theories and ideas. I was thinking that we might frob with the PCH refclk during driver init and that might cause the PCH underrun for Jörg, but it looks like the underruns really happen at the modeset time which is much later than the PCH refclock init. For the 1-n pipe transition, I don't think we handle it correctly at the moment. I have a fix as part of my remaining watermark patches. I rebased those and I'll repost them soon. In the meantime I pushed them to [1]. Jörg, can you give that branch a go? [1] git://gitorious.org/vsyrjala/linux.git watermarks_intm_31_notrace Unfortunately not working for me. It gives me some more errors: [drm:ivybridge_set_fifo_underrun_reporting] *ERROR* uncleared fifo underrun on pipe A [drm:ivb_err_int_handler] *ERROR* Pipe A FIFO underrun [drm:ivybridge_set_fifo_underrun_reporting] *ERROR* uncleared fifo underrun on pipe B [drm:ivb_err_int_handler] *ERROR* Pipe B FIFO underrun [drm:cpt_set_fifo_underrun_reporting] *ERROR* uncleared pch fifo underrun on pch transcoder A [drm:cpt_serr_int_handler] *ERROR* PCH transcoder A FIFO underrun [drm:cpt_set_fifo_underrun_reporting] *ERROR* uncleared pch fifo underrun on pch transcoder B [drm:cpt_serr_int_handler] *ERROR* PCH transcoder B FIFO underrun Thanks, Jörg ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Only unpin the default ctx object if it exists
Since commit 691e6415c891b8b2b082a120b896b443531c4d45
Author: Chris Wilson ch...@chris-wilson.co.uk
Date: Wed Apr 9 09:07:36 2014 +0100
drm/i915: Always use kref tracking for all contexts.
we have contexts everywhere, and so we must be careful to distinguish
fake contexts, which do not have an associated bo, and real ones, which
do. In particular, we now need to be careful not to dereference NULL
pointers.
This is one such example, as the commit highlighted above failed to move
the unpinning of the default ctx object into the real-context-only
branch.
Reported-by: Daniel Vetter daniel.vet...@ffwll.ch
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78792
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: Ben Widawsky benjamin.widaw...@intel.com
Cc: Mika Kuoppala mika.kuopp...@intel.com
Cc: Jani Nikula jani.nik...@intel.com
---
drivers/gpu/drm/i915/i915_gem_context.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 55b3e52..4c7cd24 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -454,6 +454,8 @@ void i915_gem_context_fini(struct drm_device *dev)
i915_gem_context_unreference(dctx);
dev_priv-ring[RCS].last_context = NULL;
}
+
+ i915_gem_object_ggtt_unpin(dctx-obj);
}
for (i = 0; i I915_NUM_RINGS; i++) {
@@ -466,7 +468,6 @@ void i915_gem_context_fini(struct drm_device *dev)
ring-last_context = NULL;
}
- i915_gem_object_ggtt_unpin(dctx-obj);
i915_gem_context_unreference(dctx);
}
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
On Thu, 27 Mar 2014 16:22:44 -0700 Kenneth Graunke kenn...@whitecape.org wrote: On 03/27/2014 03:44 PM, Daniel Vetter wrote: On Thu, Mar 27, 2014 at 10:34 PM, Kenneth Graunke kenn...@whitecape.org wrote: Why are we parsing batches with I915_EXEC_SECURE at all? Secure batches are only issued from trusted code which is guaranteed to be running as root. I don't see any benefit to scanning those batches, and there's definitely overhead. I mean, sure, it may be reasonable in the short term as a way to test the command parser, but I certainly hope we don't *ship* that. Everyone runs X as root, but I kinda want X to also be able to run as non-root. The cmd parser has a special list of drm master register lists which should allow this, but if we just bypass the cmd parser for all normal X installs we'll have 0 test coverage on this. Which means broken like hell. Hence I actually intend to ship this, yes. Chris doesn't like it either really. -Daniel Seriously? Hurt performance on every user's system just so you can test things? That a classic case of the tail wagging the dog. Why not make a i915.enable_cmd_parser=2 value which enables it all the time and use that when running igt? Clearly being able to test this is valuable, but enabling it universally is *not* OK. Daniel, I'm not sure what you mean by 0 coverage. Surely DRI clients count for something? And X shouldn't be submitting all its batches with the secure bit set, right? If so, we ought to fix that and only use it for ones where it's necessary (e.g. wait events or similar). I agree with Ken and Chris here. Chris? -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
On Fri, May 16, 2014 at 12:05:45PM -0700, Jesse Barnes wrote: On Thu, 27 Mar 2014 16:22:44 -0700 Kenneth Graunke kenn...@whitecape.org wrote: On 03/27/2014 03:44 PM, Daniel Vetter wrote: On Thu, Mar 27, 2014 at 10:34 PM, Kenneth Graunke kenn...@whitecape.org wrote: Why are we parsing batches with I915_EXEC_SECURE at all? Secure batches are only issued from trusted code which is guaranteed to be running as root. I don't see any benefit to scanning those batches, and there's definitely overhead. I mean, sure, it may be reasonable in the short term as a way to test the command parser, but I certainly hope we don't *ship* that. Everyone runs X as root, but I kinda want X to also be able to run as non-root. The cmd parser has a special list of drm master register lists which should allow this, but if we just bypass the cmd parser for all normal X installs we'll have 0 test coverage on this. Which means broken like hell. Hence I actually intend to ship this, yes. Chris doesn't like it either really. -Daniel Seriously? Hurt performance on every user's system just so you can test things? That a classic case of the tail wagging the dog. Why not make a i915.enable_cmd_parser=2 value which enables it all the time and use that when running igt? Clearly being able to test this is valuable, but enabling it universally is *not* OK. Daniel, I'm not sure what you mean by 0 coverage. Surely DRI clients count for something? And X shouldn't be submitting all its batches with the secure bit set, right? If so, we ought to fix that and only use it for ones where it's necessary (e.g. wait events or similar). I agree with Ken and Chris here. Chris? We haven't even fixed the major regression from enabling FBC. What's another massive slowdown? Yes, X only sets the secure bit when it pokes the display registers, and those registers should be privileged even with a cmd parser in place (which they are). Daniel's argument presumes that we haven't been patching out the cmd parser all this time anyway. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
On Fri, 16 May 2014 20:20:50 +0100 Chris Wilson ch...@chris-wilson.co.uk wrote: On Fri, May 16, 2014 at 12:05:45PM -0700, Jesse Barnes wrote: On Thu, 27 Mar 2014 16:22:44 -0700 Kenneth Graunke kenn...@whitecape.org wrote: On 03/27/2014 03:44 PM, Daniel Vetter wrote: On Thu, Mar 27, 2014 at 10:34 PM, Kenneth Graunke kenn...@whitecape.org wrote: Why are we parsing batches with I915_EXEC_SECURE at all? Secure batches are only issued from trusted code which is guaranteed to be running as root. I don't see any benefit to scanning those batches, and there's definitely overhead. I mean, sure, it may be reasonable in the short term as a way to test the command parser, but I certainly hope we don't *ship* that. Everyone runs X as root, but I kinda want X to also be able to run as non-root. The cmd parser has a special list of drm master register lists which should allow this, but if we just bypass the cmd parser for all normal X installs we'll have 0 test coverage on this. Which means broken like hell. Hence I actually intend to ship this, yes. Chris doesn't like it either really. -Daniel Seriously? Hurt performance on every user's system just so you can test things? That a classic case of the tail wagging the dog. Why not make a i915.enable_cmd_parser=2 value which enables it all the time and use that when running igt? Clearly being able to test this is valuable, but enabling it universally is *not* OK. Daniel, I'm not sure what you mean by 0 coverage. Surely DRI clients count for something? And X shouldn't be submitting all its batches with the secure bit set, right? If so, we ought to fix that and only use it for ones where it's necessary (e.g. wait events or similar). I agree with Ken and Chris here. Chris? We haven't even fixed the major regression from enabling FBC. What's another massive slowdown? I thought you had that fixed in the X driver by avoiding front buffer rendering altogether. If that's the case we just need an ioctl to opt out of front buffer tracking, right? Presumably that flag would follow the current DRM_MASTER process... Yes, X only sets the secure bit when it pokes the display registers, and those registers should be privileged even with a cmd parser in place (which they are). Daniel's argument presumes that we haven't been patching out the cmd parser all this time anyway. Yeah I know we have some perf issues as it is; it would be nice if the overhead were so minimal that it didn't matter. But just on principle, scanning secure buffers seems wrong, and I'm trying to understand why Daniel would want it. -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Only unpin the default ctx object if it exists
On Fri, May 16, 2014 at 06:59:00PM +0100, Chris Wilson wrote:
Since commit 691e6415c891b8b2b082a120b896b443531c4d45
Author: Chris Wilson ch...@chris-wilson.co.uk
Date: Wed Apr 9 09:07:36 2014 +0100
drm/i915: Always use kref tracking for all contexts.
we have contexts everywhere, and so we must be careful to distinguish
fake contexts, which do not have an associated bo, and real ones, which
do. In particular, we now need to be careful not to dereference NULL
pointers.
This is one such example, as the commit highlighted above failed to move
the unpinning of the default ctx object into the real-context-only
branch.
Reported-by: Daniel Vetter daniel.vet...@ffwll.ch
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78792
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: Ben Widawsky benjamin.widaw...@intel.com
Cc: Mika Kuoppala mika.kuopp...@intel.com
Cc: Jani Nikula jani.nik...@intel.com
I'll leave it to Jani to decide whether this is justified for -fixes, it
has my t-d r-b. Queued for -next, thanks for the patch.
-Daniel
---
drivers/gpu/drm/i915/i915_gem_context.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 55b3e52..4c7cd24 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -454,6 +454,8 @@ void i915_gem_context_fini(struct drm_device *dev)
i915_gem_context_unreference(dctx);
dev_priv-ring[RCS].last_context = NULL;
}
+
+ i915_gem_object_ggtt_unpin(dctx-obj);
}
for (i = 0; i I915_NUM_RINGS; i++) {
@@ -466,7 +468,6 @@ void i915_gem_context_fini(struct drm_device *dev)
ring-last_context = NULL;
}
- i915_gem_object_ggtt_unpin(dctx-obj);
i915_gem_context_unreference(dctx);
}
--
1.7.9.5
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
On Fri, 16 May 2014 12:34:08 -0700 Jesse Barnes jbar...@virtuousgeek.org wrote: On Fri, 16 May 2014 20:20:50 +0100 Chris Wilson ch...@chris-wilson.co.uk wrote: Yes, X only sets the secure bit when it pokes the display registers, and those registers should be privileged even with a cmd parser in place (which they are). Daniel's argument presumes that we haven't been patching out the cmd parser all this time anyway. Yeah I know we have some perf issues as it is; it would be nice if the overhead were so minimal that it didn't matter. But just on principle, scanning secure buffers seems wrong, and I'm trying to understand why Daniel would want it. Ok Daniel explained on IRC that we actually have a special whitelist for the secure batch case. The idea is to allow a DRM_MASTER to submit secure batches, but still prevent a local root exploit. I suppose that means preventing access to most commands and registers, but allowing a few extra things like wait events and display register updates. I suppose it's not entirely unreasonable, but it does add complexity to the scanner and overhead to all users; not sure it's worth it. -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
On Fri, May 16, 2014 at 12:53:30PM -0700, Jesse Barnes wrote: On Fri, 16 May 2014 12:34:08 -0700 Jesse Barnes jbar...@virtuousgeek.org wrote: On Fri, 16 May 2014 20:20:50 +0100 Chris Wilson ch...@chris-wilson.co.uk wrote: Yes, X only sets the secure bit when it pokes the display registers, and those registers should be privileged even with a cmd parser in place (which they are). Daniel's argument presumes that we haven't been patching out the cmd parser all this time anyway. Yeah I know we have some perf issues as it is; it would be nice if the overhead were so minimal that it didn't matter. But just on principle, scanning secure buffers seems wrong, and I'm trying to understand why Daniel would want it. Ok Daniel explained on IRC that we actually have a special whitelist for the secure batch case. The idea is to allow a DRM_MASTER to submit secure batches, but still prevent a local root exploit. I suppose that means preventing access to most commands and registers, but allowing a few extra things like wait events and display register updates. Just to clarify further: the additional register whitelist and commands are only based on DRM_MASTER. Setting I915_EXEC_SECURE is not required. So I suppose we could stop scanning batches that have I915_EXEC_SECURE and userspace could stop sending such batches when the parser is fully enabled. Brad I suppose it's not entirely unreasonable, but it does add complexity to the scanner and overhead to all users; not sure it's worth it. -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
On Fri, 16 May 2014 20:49:30 +0100 Chris Wilson ch...@chris-wilson.co.uk wrote: On Fri, May 16, 2014 at 12:34:08PM -0700, Jesse Barnes wrote: On Fri, 16 May 2014 20:20:50 +0100 Chris Wilson ch...@chris-wilson.co.uk wrote: We haven't even fixed the major regression from enabling FBC. What's another massive slowdown? I thought you had that fixed in the X driver by avoiding front buffer rendering altogether. If that's the case we just need an ioctl to opt out of front buffer tracking, right? Presumably that flag would follow the current DRM_MASTER process... No. All rendering triggers FBC updates. Ville has patches to fix the majority of that with only nuking FBC when front buffer rendering. (Note that games aren't usually affected by this because FBC is disabled by pageflipping.) The overhead could probably be reduced further by periodic nuking the FBC like (ideally) PSR. And yes X can avoid front buffer rendering altogether. The remaining challenge is to know when to enable it (the kernel doesn't give us any information about FBC or PSR after setting a mode) and when not, i.e. there is already a pageflipping compositor. I thought there was a control bit for when the nuke occurred? If not, yeah I guess we have to go with a sw approach... -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
On Fri, 16 May 2014 13:12:27 -0700 Volkin, Bradley D bradley.d.vol...@intel.com wrote: On Fri, May 16, 2014 at 12:53:30PM -0700, Jesse Barnes wrote: On Fri, 16 May 2014 12:34:08 -0700 Jesse Barnes jbar...@virtuousgeek.org wrote: On Fri, 16 May 2014 20:20:50 +0100 Chris Wilson ch...@chris-wilson.co.uk wrote: Yes, X only sets the secure bit when it pokes the display registers, and those registers should be privileged even with a cmd parser in place (which they are). Daniel's argument presumes that we haven't been patching out the cmd parser all this time anyway. Yeah I know we have some perf issues as it is; it would be nice if the overhead were so minimal that it didn't matter. But just on principle, scanning secure buffers seems wrong, and I'm trying to understand why Daniel would want it. Ok Daniel explained on IRC that we actually have a special whitelist for the secure batch case. The idea is to allow a DRM_MASTER to submit secure batches, but still prevent a local root exploit. I suppose that means preventing access to most commands and registers, but allowing a few extra things like wait events and display register updates. Just to clarify further: the additional register whitelist and commands are only based on DRM_MASTER. Setting I915_EXEC_SECURE is not required. So I suppose we could stop scanning batches that have I915_EXEC_SECURE and userspace could stop sending such batches when the parser is fully enabled. Ah ok, yeah that's another option, but now I understand where Daniel is coming from with testing, since that's not how the current X driver behaves. -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Retire requests before creating a new one
On Thu, May 15, 2014 at 10:41:42AM +0100, Chris Wilson wrote: More fallout from commit c8725f3dc0911d4354315a65150aecd8b7d0d74a Author: Chris Wilson ch...@chris-wilson.co.uk Date: Mon Mar 17 12:21:55 2014 + drm/i915: Do not call retire_requests from wait_for_rendering is that we can completely fill all of memory using small objects, such that we exhaust the filp space, and spend all of our time evicting objects from the aperture. As such, we never fill the ring, and never trigger the last resort flushing in commit 1cf0ba14740d96fbf6f58a201f000a34b74f4725 Author: Chris Wilson ch...@chris-wilson.co.uk Date: Mon May 5 09:07:33 2014 +0100 drm/i915: Flush request queue when waiting for ring space and so all the requests are left active and the objects keep that last active reference. Eventually the system comes to a halt as it runs out of memory. The impact is mainly limited to test cases as regular userspace will trigger retirement by manually checking whether an object is active. Testcase: igt/gem_lut_handle Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78724 Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Tested-by: Guo Jinxian jinxianx@intel.com --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 26c9d66e4294..7ba517f1ce06 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -628,6 +628,8 @@ i915_gem_execbuffer_reserve(struct intel_ring_buffer *ring, bool has_fenced_gpu_access = INTEL_INFO(ring-dev)-gen 4; int retry; + i915_gem_retire_requests_ring(ring); + vm = list_first_entry(vmas, struct i915_vma, exec_list)-vm; INIT_LIST_HEAD(ordered_vmas); -- 2.0.0.rc2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot.
From: Clint Taylor clinton.a.tay...@intel.com
The panel power sequencer on vlv doesn't appear to accept changes to its T12
power down duration during warm reboots. This change forces a delay for warm
reboots to the T12 panel timing as defined in the VBT table for the connected
panel.
Ver2: removed redundant pr_crit(), commented magic value for pp_div_reg
Signed-off-by: Clint Taylor clinton.a.tay...@intel.com
---
drivers/gpu/drm/i915/intel_dp.c | 43 ++
drivers/gpu/drm/i915/intel_drv.h |1 +
2 files changed, 44 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5ca68aa9..023efda 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -28,6 +28,8 @@
#include linux/i2c.h
#include linux/slab.h
#include linux/export.h
+#include linux/notifier.h
+#include linux/reboot.h
#include drm/drmP.h
#include drm/drm_crtc.h
#include drm/drm_crtc_helper.h
@@ -302,6 +304,39 @@ static u32 _pp_stat_reg(struct intel_dp *intel_dp)
return VLV_PIPE_PP_STATUS(vlv_power_sequencer_pipe(intel_dp));
}
+/* Reboot notifier handler to shutdown panel power to gaurantee T12 timing */
+static int edp_notify_handler(struct notifier_block *this, unsigned long code,
+ void *unused)
+{
+ struct intel_dp *intel_dp = container_of(this, typeof(* intel_dp),
+edp_notifier);
+ struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+ struct drm_device *dev = intel_dig_port-base.base.dev;
+ struct drm_i915_private *dev_priv = dev-dev_private;
+ u32 pp;
+ u32 pp_ctrl_reg, pp_div_reg;
+ enum pipe pipe = vlv_power_sequencer_pipe(intel_dp);
+
+ if (!is_edp(intel_dp))
+ return 0;
+
+ if (IS_VALLEYVIEW(dev)) {
+ pp_ctrl_reg = VLV_PIPE_PP_CONTROL(pipe);
+ pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe);
+ if (code == SYS_RESTART) {
+ pp = I915_READ(VLV_PIPE_PP_DIVISOR(pipe));
+ pp = PP_REFERENCE_DIVIDER_MASK;
+ /* 0x1F write to PP_DIV_REG sets max cycle delay */
+ I915_WRITE(pp_div_reg , pp | 0x1F);
+ I915_WRITE(pp_ctrl_reg,
+ PANEL_UNLOCK_REGS | PANEL_POWER_OFF);
+ /* VBT entries are in tenths of uS convert to mS */
+ msleep(dev_priv-vbt.edp_pps.t11_t12 / 10);
+ }
+ }
+ return 0;
+}
+
static bool edp_have_panel_power(struct intel_dp *intel_dp)
{
struct drm_device *dev = intel_dp_to_dev(intel_dp);
@@ -3344,6 +3379,10 @@ void intel_dp_encoder_destroy(struct drm_encoder
*encoder)
mutex_lock(dev-mode_config.mutex);
edp_panel_vdd_off_sync(intel_dp);
mutex_unlock(dev-mode_config.mutex);
+ if (intel_dp-edp_notifier.notifier_call) {
+ unregister_reboot_notifier(intel_dp-edp_notifier);
+ intel_dp-edp_notifier.notifier_call = NULL;
+ }
}
kfree(intel_dig_port);
}
@@ -3782,6 +3821,10 @@ intel_dp_init_connector(struct intel_digital_port
*intel_dig_port,
if (is_edp(intel_dp)) {
intel_dp_init_panel_power_timestamps(intel_dp);
intel_dp_init_panel_power_sequencer(dev, intel_dp, power_seq);
+ if (IS_VALLEYVIEW(dev)) {
+ intel_dp-edp_notifier.notifier_call =
edp_notify_handler;
+ register_reboot_notifier(intel_dp-edp_notifier);
+ }
}
intel_dp_aux_init(intel_dp, intel_connector);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 328b1a7..1ea193a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -512,6 +512,7 @@ struct intel_dp {
bool psr_setup_done;
bool use_tps3;
struct intel_connector *attached_connector;
+ struct notifier_block edp_notifier;
uint32_t (*get_aux_clock_divider)(struct intel_dp *dp, int index);
/*
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] tests/kms_sink_crc_basic: Put into righ Makefile target
If it's a simple test, it needs to be in the simple lists. Tests with subtests go into the _M tests. Without that test enumeration is all screwed up. Cc: Rodrigo Vivi rodrigo.v...@gmail.com Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- tests/Makefile.sources | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Makefile.sources b/tests/Makefile.sources index 383a2098e768..fbf63e92c5b2 100644 --- a/tests/Makefile.sources +++ b/tests/Makefile.sources @@ -68,7 +68,6 @@ TESTS_progs_M = \ kms_plane \ kms_render \ kms_setmode \ - kms_sink_crc_basic \ pm_lpsp \ pm_rpm \ pm_rps \ @@ -135,6 +134,7 @@ TESTS_progs = \ gen3_render_tiledx_blits \ gen3_render_tiledy_blits \ gen7_forcewake_mt \ + kms_sink_crc_basic \ pm_psr \ pm_rc6_residency \ prime_udl \ -- 1.9.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 66/66] drm/i915: runtime PM support for DPMS
On Thu, 24 Apr 2014 23:55:42 +0200
Daniel Vetter daniel.vet...@ffwll.ch wrote:
+ if (enable) {
+ if (!intel_crtc-active) {
+ domains = get_crtc_power_domains(crtc);
+ for_each_power_domain(domain, domains)
+ intel_display_power_get(dev_priv, domain);
+ intel_crtc-enabled_power_domains = domains;
+
+ dev_priv-display.crtc_enable(crtc);
+ }
+ } else {
+ if (intel_crtc-active) {
+ dev_priv-display.crtc_disable(crtc);
+
+ domains = intel_crtc-enabled_power_domains;
+ for_each_power_domain(domain, domains)
+ intel_display_power_put(dev_priv, domain);
+ intel_crtc-enabled_power_domains = 0;
+ }
+ }
These branches could probably be cleaned up a bit.
But the power domain bits here got me thinking that maybe we can push
them down into the crtc_enable/disable functions instead. That would
let us do the right thing per-platform and save us the
get_crtc_power_domains call which may not make sense on all platforms.
I haven't thought it through for the other power wells, but that type
of approach may make more sense than trying to abstract the wells at
the high level we're doing today, especially since things are likely to
get finer grained over time rather than coarser.
--
Jesse Barnes, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 66/66] drm/i915: runtime PM support for DPMS
On Fri, May 16, 2014 at 02:48:27PM -0700, Jesse Barnes wrote:
On Thu, 24 Apr 2014 23:55:42 +0200
Daniel Vetter daniel.vet...@ffwll.ch wrote:
+ if (enable) {
+ if (!intel_crtc-active) {
+ domains = get_crtc_power_domains(crtc);
+ for_each_power_domain(domain, domains)
+ intel_display_power_get(dev_priv, domain);
+ intel_crtc-enabled_power_domains = domains;
+
+ dev_priv-display.crtc_enable(crtc);
+ }
+ } else {
+ if (intel_crtc-active) {
+ dev_priv-display.crtc_disable(crtc);
+
+ domains = intel_crtc-enabled_power_domains;
+ for_each_power_domain(domain, domains)
+ intel_display_power_put(dev_priv, domain);
+ intel_crtc-enabled_power_domains = 0;
+ }
+ }
These branches could probably be cleaned up a bit.
But the power domain bits here got me thinking that maybe we can push
them down into the crtc_enable/disable functions instead. That would
let us do the right thing per-platform and save us the
get_crtc_power_domains call which may not make sense on all platforms.
I haven't thought it through for the other power wells, but that type
of approach may make more sense than trying to abstract the wells at
the high level we're doing today, especially since things are likely to
get finer grained over time rather than coarser.
Had the same idea but then things get ugly since since the power domain
grabbing in the modeset sequence is a bit convoluted (for historical
reasons). So would require a bit of unwinding.
Also this gives us a much clearer bisect point imo.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/i915: enable VT switchless resume v3
On Mon, 21 Apr 2014 18:37:31 +0200 Knut Petersen knut_peter...@t-online.de wrote: + /* This driver doesn't need a VT switch to restore the mode on resume */ + info-skip_vt_switch = true; + drm_fb_helper_fill_fix(info, fb-pitches[0], fb-depth); drm_fb_helper_fill_var(info, ifbdev-helper, sizes-fb_width, sizes-fb_height); Is it sufficient to just revert this part? Or are the other bits needed too? Sorry for the delay on this, I've been traveling a lot the past month and buried in other stuff so out of touch with much of my email. Thanks, -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 66/66] drm/i915: runtime PM support for DPMS
On Sat, 17 May 2014 00:19:09 +0200
Daniel Vetter dan...@ffwll.ch wrote:
On Fri, May 16, 2014 at 02:48:27PM -0700, Jesse Barnes wrote:
On Thu, 24 Apr 2014 23:55:42 +0200
Daniel Vetter daniel.vet...@ffwll.ch wrote:
+ if (enable) {
+ if (!intel_crtc-active) {
+ domains = get_crtc_power_domains(crtc);
+ for_each_power_domain(domain, domains)
+ intel_display_power_get(dev_priv, domain);
+ intel_crtc-enabled_power_domains = domains;
+
+ dev_priv-display.crtc_enable(crtc);
+ }
+ } else {
+ if (intel_crtc-active) {
+ dev_priv-display.crtc_disable(crtc);
+
+ domains = intel_crtc-enabled_power_domains;
+ for_each_power_domain(domain, domains)
+ intel_display_power_put(dev_priv, domain);
+ intel_crtc-enabled_power_domains = 0;
+ }
+ }
These branches could probably be cleaned up a bit.
But the power domain bits here got me thinking that maybe we can push
them down into the crtc_enable/disable functions instead. That would
let us do the right thing per-platform and save us the
get_crtc_power_domains call which may not make sense on all platforms.
I haven't thought it through for the other power wells, but that type
of approach may make more sense than trying to abstract the wells at
the high level we're doing today, especially since things are likely to
get finer grained over time rather than coarser.
Had the same idea but then things get ugly since since the power domain
grabbing in the modeset sequence is a bit convoluted (for historical
reasons). So would require a bit of unwinding.
Also this gives us a much clearer bisect point imo.
Yeah no doubt, not suggesting you change it as part of this series...
but overall it's something to consider for a future rewrite of our
power well code. :)
--
Jesse Barnes, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/i915: enable VT switchless resume v3
On Sat, May 17, 2014 at 12:20 AM, Jesse Barnes jbar...@virtuousgeek.org wrote: On Mon, 21 Apr 2014 18:37:31 +0200 Knut Petersen knut_peter...@t-online.de wrote: + /* This driver doesn't need a VT switch to restore the mode on resume */ + info-skip_vt_switch = true; + drm_fb_helper_fill_fix(info, fb-pitches[0], fb-depth); drm_fb_helper_fill_var(info, ifbdev-helper, sizes-fb_width, sizes-fb_height); Is it sufficient to just revert this part? Or are the other bits needed too? Sorry for the delay on this, I've been traveling a lot the past month and buried in other stuff so out of touch with much of my email. Aren't we tracking this in a bugzilla now? At least I have memories of another bug where the output state is flip-flopping somehow ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm: Refactor setplane to allow internal use
Refactor DRM setplane code into a new setplane_internal() function that
takes DRM objects directly as parameters rather than looking them up by
ID. We'll use this in a future patch when we implement legacy cursor
ioctls on top of the universal plane interface.
Signed-off-by: Matt Roper matthew.d.ro...@intel.com
---
drivers/gpu/drm/drm_crtc.c | 178 +
1 file changed, 100 insertions(+), 78 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 1a1a5f4..201c317 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2075,48 +2075,39 @@ out:
return ret;
}
-/**
- * drm_mode_setplane - configure a plane's configuration
- * @dev: DRM device
- * @data: ioctl data*
- * @file_priv: DRM file info
+/*
+ * setplane_internal - setplane handler for internal callers
*
- * Set plane configuration, including placement, fb, scaling, and other
factors.
- * Or pass a NULL fb to disable.
+ * Note that we assume an extra reference has already been taken on fb. If the
+ * update fails, this reference will be dropped before return; if it succeeds,
+ * the previous framebuffer (if any) will be unreferenced instead.
*
- * Returns:
- * Zero on success, errno on failure.
+ * src_{x,y,w,h} are provided in 16.16 fixed point format
*/
-int drm_mode_setplane(struct drm_device *dev, void *data,
- struct drm_file *file_priv)
+static int setplane_internal(struct drm_crtc *crtc,
+struct drm_plane *plane,
+struct drm_framebuffer *fb,
+int32_t crtc_x, int32_t crtc_y,
+uint32_t crtc_w, uint32_t crtc_h,
+/* src_{x,y,w,h} values are 16.16 fixed point */
+uint32_t src_x, uint32_t src_y,
+uint32_t src_w, uint32_t src_h)
{
- struct drm_mode_set_plane *plane_req = data;
- struct drm_mode_object *obj;
- struct drm_plane *plane;
- struct drm_crtc *crtc;
- struct drm_framebuffer *fb = NULL, *old_fb = NULL;
+ struct drm_device *dev = crtc-dev;
+ struct drm_framebuffer *old_fb = NULL;
int ret = 0;
unsigned int fb_width, fb_height;
int i;
- if (!drm_core_check_feature(dev, DRIVER_MODESET))
- return -EINVAL;
-
- /*
-* First, find the plane, crtc, and fb objects. If not available,
-* we don't bother to call the driver.
-*/
- obj = drm_mode_object_find(dev, plane_req-plane_id,
- DRM_MODE_OBJECT_PLANE);
- if (!obj) {
- DRM_DEBUG_KMS(Unknown plane ID %d\n,
- plane_req-plane_id);
- return -ENOENT;
+ /* Check whether this plane is usable on this CRTC */
+ if (!(plane-possible_crtcs drm_crtc_mask(crtc))) {
+ DRM_DEBUG_KMS(Invalid crtc for plane\n);
+ ret = -EINVAL;
+ goto out;
}
- plane = obj_to_plane(obj);
/* No fb means shut it down */
- if (!plane_req-fb_id) {
+ if (!fb) {
drm_modeset_lock_all(dev);
old_fb = plane-fb;
ret = plane-funcs-disable_plane(plane);
@@ -2130,31 +2121,6 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
goto out;
}
- obj = drm_mode_object_find(dev, plane_req-crtc_id,
- DRM_MODE_OBJECT_CRTC);
- if (!obj) {
- DRM_DEBUG_KMS(Unknown crtc ID %d\n,
- plane_req-crtc_id);
- ret = -ENOENT;
- goto out;
- }
- crtc = obj_to_crtc(obj);
-
- /* Check whether this plane is usable on this CRTC */
- if (!(plane-possible_crtcs drm_crtc_mask(crtc))) {
- DRM_DEBUG_KMS(Invalid crtc for plane\n);
- ret = -EINVAL;
- goto out;
- }
-
- fb = drm_framebuffer_lookup(dev, plane_req-fb_id);
- if (!fb) {
- DRM_DEBUG_KMS(Unknown framebuffer ID %d\n,
- plane_req-fb_id);
- ret = -ENOENT;
- goto out;
- }
-
/* Check whether this plane supports the fb pixel format. */
for (i = 0; i plane-format_count; i++)
if (fb-pixel_format == plane-format_types[i])
@@ -2170,32 +2136,27 @@ int drm_mode_setplane(struct drm_device *dev, void
*data,
fb_height = fb-height 16;
/* Make sure source coordinates are inside the fb. */
- if (plane_req-src_w fb_width ||
- plane_req-src_x fb_width - plane_req-src_w ||
- plane_req-src_h fb_height ||
- plane_req-src_y fb_height - plane_req-src_h) {
+ if (src_w fb_width ||
+ src_x fb_width - src_w ||
+ src_h fb_height ||
+ src_y
[Intel-gfx] [PATCH 1/2] drm: Refactor framebuffer creation to allow internal use
Refactor DRM framebuffer creation into a new function that returns a
struct drm_framebuffer directly. The upcoming universal cursor support
will want to create framebuffers internally to wrap cursor buffers, so
we want to be able to share that framebuffer creation with the
drm_mode_addfb2 ioctl handler.
Signed-off-by: Matt Roper matthew.d.ro...@intel.com
---
drivers/gpu/drm/drm_crtc.c | 64 +++---
1 file changed, 38 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index b6d6c04..1a1a5f4 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2796,56 +2796,39 @@ static int framebuffer_check(const struct
drm_mode_fb_cmd2 *r)
return 0;
}
-/**
- * drm_mode_addfb2 - add an FB to the graphics configuration
- * @dev: drm device for the ioctl
- * @data: data pointer for the ioctl
- * @file_priv: drm file for the ioctl call
- *
- * Add a new FB to the specified CRTC, given a user request with format. This
is
- * the 2nd version of the addfb ioctl, which supports multi-planar framebuffers
- * and uses fourcc codes as pixel format specifiers.
- *
- * Called by the user via ioctl.
- *
- * Returns:
- * Zero on success, errno on failure.
- */
-int drm_mode_addfb2(struct drm_device *dev,
- void *data, struct drm_file *file_priv)
+static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
+ void *data,
+ struct drm_file
*file_priv)
{
struct drm_mode_fb_cmd2 *r = data;
struct drm_mode_config *config = dev-mode_config;
struct drm_framebuffer *fb;
int ret;
- if (!drm_core_check_feature(dev, DRIVER_MODESET))
- return -EINVAL;
-
if (r-flags ~DRM_MODE_FB_INTERLACED) {
DRM_DEBUG_KMS(bad framebuffer flags 0x%08x\n, r-flags);
- return -EINVAL;
+ return ERR_PTR(-EINVAL);
}
if ((config-min_width r-width) || (r-width config-max_width)) {
DRM_DEBUG_KMS(bad framebuffer width %d, should be = %d =
%d\n,
r-width, config-min_width, config-max_width);
- return -EINVAL;
+ return ERR_PTR(-EINVAL);
}
if ((config-min_height r-height) || (r-height
config-max_height)) {
DRM_DEBUG_KMS(bad framebuffer height %d, should be = %d =
%d\n,
r-height, config-min_height, config-max_height);
- return -EINVAL;
+ return ERR_PTR(-EINVAL);
}
ret = framebuffer_check(r);
if (ret)
- return ret;
+ return ERR_PTR(ret);
fb = dev-mode_config.funcs-fb_create(dev, file_priv, r);
if (IS_ERR(fb)) {
DRM_DEBUG_KMS(could not create framebuffer\n);
- return PTR_ERR(fb);
+ return fb;
}
mutex_lock(file_priv-fbs_lock);
@@ -2854,8 +2837,37 @@ int drm_mode_addfb2(struct drm_device *dev,
DRM_DEBUG_KMS([FB:%d]\n, fb-base.id);
mutex_unlock(file_priv-fbs_lock);
+ return fb;
+}
+
+/**
+ * drm_mode_addfb2 - add an FB to the graphics configuration
+ * @dev: drm device for the ioctl
+ * @data: data pointer for the ioctl
+ * @file_priv: drm file for the ioctl call
+ *
+ * Add a new FB to the specified CRTC, given a user request with format. This
is
+ * the 2nd version of the addfb ioctl, which supports multi-planar framebuffers
+ * and uses fourcc codes as pixel format specifiers.
+ *
+ * Called by the user via ioctl.
+ *
+ * Returns:
+ * Zero on success, errno on failure.
+ */
+int drm_mode_addfb2(struct drm_device *dev,
+ void *data, struct drm_file *file_priv)
+{
+ struct drm_framebuffer *fb;
- return ret;
+ if (!drm_core_check_feature(dev, DRIVER_MODESET))
+ return -EINVAL;
+
+ fb = add_framebuffer_internal(dev, data, file_priv);
+ if (IS_ERR(fb))
+ return PTR_ERR(fb);
+
+ return 0;
}
/**
--
1.8.5.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/4] drm: Support legacy cursor ioctls via universal planes when possible (v2)
If drivers support universal planes and have registered a cursor plane
with the DRM core, we should use that universal plane support when
handling legacy cursor ioctls. Drivers that transition to universal
planes won't have to maintain separate legacy ioctl handling; drivers
that don't transition to universal planes will continue to operate
without any change to behavior.
Note that there's a bit of a mismatch between the legacy cursor ioctls
and the universal plane API's --- legacy ioctl's use driver buffer
handles directly whereas the universal plane API takes drm_framebuffers.
Since there's no way to recover the driver handle from a
drm_framebuffer, we can implement legacy ioctl's in terms of universal
plane interfaces, but cannot implement universal plane interfaces in
terms of legacy ioctls. Specifically, there's no way to create a
general cursor helper in the way we previously created a primary plane
helper.
It's important to land this patch before any patches that add universal
cursor support to individual drivers so that drivers don't have to worry
about juggling two different styles of reference counting for cursor
buffers when userspace mixes and matches legacy and universal cursor
calls. With this patch, a driver that switches to universal cursor
support may assume that all cursor buffers are wrapped in a
drm_framebuffer and can rely on framebuffer reference counting for all
cursor operations.
v2:
- Use new add_framebuffer_internal() function to create framebuffer
rather than trying to call directly into the ioctl interface and
look up the handle returned.
- Use new setplane_internal() function to update the cursor plane
rather than calling through the ioctl interface. Note that since
we're no longer looking up an fb_id, no extra reference will be
taken here.
- Grab extra reference to fb under lock in !BO case to avoid issues
where racing userspace could cause the fb to be destroyed out from
under us after we grab the fb pointer.
Signed-off-by: Matt Roper matthew.d.ro...@intel.com
---
drivers/gpu/drm/drm_crtc.c | 108 +
include/drm/drm_crtc.h | 4 ++
2 files changed, 112 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 201c317..9f6f93c 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -40,6 +40,10 @@
#include drm_crtc_internal.h
+static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
+ void *data,
+ struct drm_file
*file_priv);
+
/**
* drm_modeset_lock_all - take all modeset locks
* @dev: drm device
@@ -2498,6 +2502,103 @@ out:
return ret;
}
+/**
+ * drm_mode_cursor_universal - translate legacy cursor ioctl call into a
+ * universal plane handler call
+ * @crtc: crtc to update cursor for
+ * @req: data pointer for the ioctl
+ * @file_priv: drm file for the ioctl call
+ *
+ * Legacy cursor ioctl's work directly with driver buffer handles. To
+ * translate legacy ioctl calls into universal plane handler calls, we need to
+ * wrap the native buffer handle in a drm_framebuffer.
+ *
+ * Note that we assume any handle passed to the legacy ioctls was a 32-bit ARGB
+ * buffer with a pitch of 4*width; the universal plane interface should be used
+ * directly in cases where the hardware can support other buffer settings and
+ * userspace wants to make use of these capabilities.
+ *
+ * Returns:
+ * Zero on success, errno on failure.
+ */
+static int drm_mode_cursor_universal(struct drm_crtc *crtc,
+struct drm_mode_cursor2 *req,
+struct drm_file *file_priv)
+{
+ struct drm_device *dev = crtc-dev;
+ struct drm_framebuffer *fb = NULL;
+ struct drm_mode_fb_cmd2 fbreq = {
+ .width = req-width,
+ .height = req-height,
+ .pixel_format = DRM_FORMAT_ARGB,
+ .pitches = { req-width * 4 },
+ .handles = { req-handle },
+ };
+ int32_t crtc_x, crtc_y;
+ uint32_t crtc_w = 0, crtc_h = 0;
+ uint32_t src_w = 0, src_h = 0;
+ int ret = 0;
+
+ BUG_ON(!crtc-cursor);
+
+ /*
+* Obtain fb we'll be using (either new or existing) and take an extra
+* reference to it if fb != null. setplane will take care of dropping
+* the reference if the plane update fails.
+*/
+ if (req-flags DRM_MODE_CURSOR_BO) {
+ if (req-handle) {
+ fb = add_framebuffer_internal(dev, fbreq, file_priv);
+ if (IS_ERR(fb)) {
+ DRM_DEBUG_KMS(failed to wrap cursor buffer in
drm framebuffer\n);
+ return PTR_ERR(fb);
+ }
+
+ drm_framebuffer_reference(fb);
+
Re: [Intel-gfx] [PATCH 3/4] drm/i915: enable VT switchless resume v3
On Fri, May 16, 2014 at 03:20:47PM -0700, Jesse Barnes wrote: On Mon, 21 Apr 2014 18:37:31 +0200 Knut Petersen knut_peter...@t-online.de wrote: + /* This driver doesn't need a VT switch to restore the mode on resume */ + info-skip_vt_switch = true; + drm_fb_helper_fill_fix(info, fb-pitches[0], fb-depth); drm_fb_helper_fill_var(info, ifbdev-helper, sizes-fb_width, sizes-fb_height); Is it sufficient to just revert this part? Or are the other bits needed too? Sorry for the delay on this, I've been traveling a lot the past month and buried in other stuff so out of touch with much of my email. The key step here is that X is restarted after resume. The slow down was due to X not finding any connected outputs and so disabling them all, setting up a dummy 1024x768 fb which really confused KDE. (KDE queries the config causing a forced reprobe of all outputs, setups the display for the native 1280x1024, but screws up KDE's own bookkeeping.) The impact has been fixed by handling the connector-status more robusting during initial output probing in X. What remains is the question whether connector-status can be set appropriately upon resume? It requires a detection cycle to be sure that the outputs are still there, which is arguably better deferred to userspace. To be consistent the BIOS take over code should mark connector-status as unknown for the CRTCs it takes over without doing a detection cycle (where we just presume that the CRTC/output being enabled means something is on the other end of the pipe and is still valid). -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/i915: enable VT switchless resume v3
On Fri, May 16, 2014 at 11:38:07PM +0100, Chris Wilson wrote: On Fri, May 16, 2014 at 03:20:47PM -0700, Jesse Barnes wrote: On Mon, 21 Apr 2014 18:37:31 +0200 Knut Petersen knut_peter...@t-online.de wrote: + /* This driver doesn't need a VT switch to restore the mode on resume */ + info-skip_vt_switch = true; + drm_fb_helper_fill_fix(info, fb-pitches[0], fb-depth); drm_fb_helper_fill_var(info, ifbdev-helper, sizes-fb_width, sizes-fb_height); Is it sufficient to just revert this part? Or are the other bits needed too? Sorry for the delay on this, I've been traveling a lot the past month and buried in other stuff so out of touch with much of my email. The key step here is that X is restarted after resume. The slow down was due to X not finding any connected outputs and so disabling them all, setting up a dummy 1024x768 fb which really confused KDE. (KDE queries the config causing a forced reprobe of all outputs, setups the display for the native 1280x1024, but screws up KDE's own bookkeeping.) The impact has been fixed by handling the connector-status more robusting during initial output probing in X. What remains is the question whether connector-status can be set appropriately upon resume? It requires a detection cycle to be sure that the outputs are still there, which is arguably better deferred to userspace. To be consistent the BIOS take over code should mark connector-status as unknown for the CRTCs it takes over without doing a detection cycle (where we just presume that the CRTC/output being enabled means something is on the other end of the pipe and is still valid). Hmm. Why didn't fbcon respond to the hotplug event on resume and perform a detection cycle before Knut was able to type startx on the console? That I think is the bug. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm: Support legacy cursor ioctls via universal planes when possible (v2)
On Sat, May 17, 2014 at 12:38 AM, Matt Roper matthew.d.ro...@intel.com wrote:
+ if (ret) {
+ if (req-flags DRM_MODE_CURSOR_BO)
+ drm_mode_rmfb(dev, fb-base.id, file_priv);
+ return ret;
+ }
With the new refcount logic an unconditional
drm_framebuffer_unreference should be here instead of this. I think,
but please double-check since it's late here ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/4] drm/i915: Add intel_crtc_cursor_set_obj() to set cursor buffer (v2)
Refactor cursor buffer setting such that the code to actually update the
cursor lives in a new function, intel_crtc_cursor_set_obj(), and takes
a GEM object as a parameter. The existing legacy cursor ioctl handler,
intel_crtc_cursor_set() will now perform the userspace handle lookup and
then call this new function.
This refactoring is in preparation for the universal plane cursor
support where we'll want to update the cursor with an actual GEM buffer
object (obtained via drm_framebuffer) rather than a userspace handle.
v2: Drop obvious kerneldoc and replace with note about function's
reference consumption
Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch
Signed-off-by: Matt Roper matthew.d.ro...@intel.com
---
drivers/gpu/drm/i915/intel_display.c | 42 ++--
1 file changed, 31 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index e9f196e..11c9493 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8015,21 +8015,26 @@ static void intel_crtc_update_cursor(struct drm_crtc
*crtc,
}
}
-static int intel_crtc_cursor_set(struct drm_crtc *crtc,
-struct drm_file *file,
-uint32_t handle,
-uint32_t width, uint32_t height)
+/*
+ * intel_crtc_cursor_set_obj - Set cursor to specified GEM object
+ *
+ * Note that the object's reference will be consumed if the update fails. If
+ * the update succeeds, the reference of the old object (if any) will be
+ * consumed.
+ */
+static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
+struct drm_i915_gem_object *obj,
+uint32_t width, uint32_t height)
{
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- struct drm_i915_gem_object *obj;
unsigned old_width;
uint32_t addr;
int ret;
/* if we want to turn off the cursor ignore width and height */
- if (!handle) {
+ if (!obj) {
DRM_DEBUG_KMS(cursor off\n);
addr = 0;
obj = NULL;
@@ -8045,12 +8050,8 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
return -EINVAL;
}
- obj = to_intel_bo(drm_gem_object_lookup(dev, file, handle));
- if (obj-base == NULL)
- return -ENOENT;
-
if (obj-base.size width * height * 4) {
- DRM_DEBUG_KMS(buffer is to small\n);
+ DRM_DEBUG_KMS(buffer is too small\n);
ret = -ENOMEM;
goto fail;
}
@@ -8138,6 +8139,25 @@ fail:
return ret;
}
+static int intel_crtc_cursor_set(struct drm_crtc *crtc,
+struct drm_file *file,
+uint32_t handle,
+uint32_t width, uint32_t height)
+{
+ struct drm_device *dev = crtc-dev;
+ struct drm_i915_gem_object *obj;
+
+ if (handle) {
+ obj = to_intel_bo(drm_gem_object_lookup(dev, file, handle));
+ if (obj-base == NULL)
+ return -ENOENT;
+ } else {
+ obj = NULL;
+ }
+
+ return intel_crtc_cursor_set_obj(crtc, obj, width, height);
+}
+
static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
{
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
--
1.8.5.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] i-g-t + GDB
Hi, I am trying to step through i-g-t libdrm source w/ GDB but single stepping seems erratic so I am guessing the build is optimized. Is changing CFLAGS = -g -O2 to CFLAGS = -g -O0 the right thing to do? If so, how can I do that globally one time versus touching every Makefile inside the igt and libdrm projects? Maybe pass an additional parameter to autogen.sh, I'm not sure ? Thanks, Mike ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] i-g-t + GDB
On 05/16/2014 04:29 PM, Michael H Nguyen wrote: Hi, I am trying to step through i-g-t libdrm source w/ GDB but single stepping seems erratic so I am guessing the build is optimized. Is changing CFLAGS = -g -O2 to CFLAGS = -g -O0 the right thing to do? If so, how can I do that globally one time versus touching every Makefile inside the igt and libdrm projects? Maybe pass an additional parameter to autogen.sh, I'm not sure ? Figured it out. For anyone interested... $ ./autogen.sh --prefix=my prefix CFLAGS=-g -O0 CFLAGS gets passed to ./configure which generates the make files w/ CFLAGS = -g -O0 Thanks, Mike ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
