[Intel-gfx] [PATCH] [VPG HSW-A] drm/i915:Added HDMI Audio codec disable sequence for HSW.
From: Mukesh mukeshx.ar...@intel.com The code implements hsw_hdmi_audio_disable func which sets the relevant registers for disabling the audio codec in a call to intel_disable_ddi func.This audio codec disbale sequence is implemented as per the recommendation of the Bspec. Change-Id: If6eefbfe5ef821db547c759caa9ff5dc18980738 Signed-off-by: Mukesh Arora mukeshx.ar...@intel.com --- drivers/gpu/drm/i915/intel_ddi.c | 41 1 file changed, 41 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 0de236e..2718d9a 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1119,6 +1119,43 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder) I915_WRITE(PORT_CLK_SEL(port), PORT_CLK_SEL_NONE); } +/* Sets the registers for audio codec disable sequence as +* mentioned in the Haswell Bspec. +*/ +void hsw_hdmi_audio_disable(struct drm_encoder *encoder) +{ + u32 temp; + struct intel_crtc *intel_crtc = to_intel_crtc(encoder-crtc); + struct drm_device *dev = encoder-dev; + struct drm_i915_private *dev_priv = dev-dev_private; + int aud_config = HSW_AUD_CFG(intel_crtc-pipe); + + /* HDMI audio disable sequence for Haswell*/ + if (intel_crtc-eld_vld) { + /* disable timestamps */ + temp = I915_READ(aud_config); + /* write 0 for HDMI */ + temp = ~AUD_CONFIG_N_VALUE_INDEX; + /* Set N_programming_enable */ + temp |= AUD_CONFIG_N_PROG_ENABLE; + /* Set Upper_N_value and Lower_N_value + (bits 27:20, 15:4) to all 0s */ + temp = ~(AUD_CONFIG_UPPER_N_VALUE|AUD_CONFIG_LOWER_N_VALUE); + I915_WRITE(aud_config, temp); + /* Disable ELDV and ELD buffer */ + temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); + temp = ~(AUDIO_ELD_VALID_A (intel_crtc-pipe * 4)); + I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, temp); + /* Wait for 2 vertical blanks */ + intel_wait_for_vblank(dev, intel_crtc-pipe); + intel_wait_for_vblank(dev, intel_crtc-pipe); + /* Disable audio PD. This is optional as per Bspec. */ + temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); + temp = ~(AUDIO_OUTPUT_ENABLE_A (intel_crtc-pipe * 4)); + I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, temp); + } + +} static void intel_enable_ddi(struct intel_encoder *intel_encoder) { struct drm_encoder *encoder = intel_encoder-base; @@ -1173,6 +1210,10 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder) tmp = ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) (pipe * 4)); I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp); + if (IS_HASWELL(dev) (type == INTEL_OUTPUT_HDMI)) { + /*HDMI audio codec disable sequence.*/ + hsw_hdmi_audio_disable(encoder); + } } if (type == INTEL_OUTPUT_EDP) { -- 1.8.1.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2] drm/i915/hsw: Add display Audio codec disable sequence for Haswell
From: Mukesh mukeshx.ar...@intel.com The code defines a new function intel_disable_audio() to implement the entire audio codec disable sequence, called by intel_disable_ddi() in DDI port disablement. This audio codec disbale sequence is implemented as per the recommendation of the Bspec. [Patch modified by Mendong to apply to both HDMI and DP port.] Signed-off-by: Mukesh Arora mukeshx.ar...@intel.com Signed-off-by: Mengdong Lin mengdong@intel.com diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index b042ee5..2946fe7 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1088,6 +1088,45 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder) I915_WRITE(PORT_CLK_SEL(port), PORT_CLK_SEL_NONE); } +/* audio codec disable sequence, as per Bspec */ +void intel_disable_audio(struct intel_encoder *intel_encoder) +{ + int type = intel_encoder-type; + struct drm_encoder *encoder = intel_encoder-base; + struct drm_device *dev = encoder-dev; + struct drm_i915_private *dev_priv = dev-dev_private; + struct intel_crtc *intel_crtc = to_intel_crtc(encoder-crtc); + int pipe = intel_crtc-pipe; + int aud_config = HSW_AUD_CFG(pipe); + u32 temp; + + /* disable timestamps */ + temp = I915_READ(aud_config); + if (type == INTEL_OUTPUT_HDMI) + temp = ~AUD_CONFIG_N_VALUE_INDEX; + else if (type == INTEL_OUTPUT_DISPLAYPORT) + temp |= AUD_CONFIG_N_VALUE_INDEX; + else + return; + temp |= AUD_CONFIG_N_PROG_ENABLE; + temp = ~(AUD_CONFIG_UPPER_N_VALUE|AUD_CONFIG_LOWER_N_VALUE); + I915_WRITE(aud_config, temp); + + /* Disable ELDV and ELD buffer */ + temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); + temp = ~(AUDIO_ELD_VALID_A (pipe * 4)); + I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, temp); + + /* Wait for 2 vertical blanks */ + intel_wait_for_vblank(dev, pipe); + intel_wait_for_vblank(dev, pipe); + + /* Disable audio PD. This is optional as per Bspec. */ + temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); + temp = ~(AUDIO_OUTPUT_ENABLE_A (pipe * 4)); + I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, temp); +} + static void intel_enable_ddi(struct intel_encoder *intel_encoder) { struct drm_encoder *encoder = intel_encoder-base; @@ -1132,18 +1171,10 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder) struct drm_encoder *encoder = intel_encoder-base; struct drm_crtc *crtc = encoder-crtc; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - int pipe = intel_crtc-pipe; int type = intel_encoder-type; - struct drm_device *dev = encoder-dev; - struct drm_i915_private *dev_priv = dev-dev_private; - uint32_t tmp; - if (intel_crtc-eld_vld type != INTEL_OUTPUT_EDP) { - tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); - tmp = ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) -(pipe * 4)); - I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp); - } + if (intel_crtc-eld_vld type != INTEL_OUTPUT_EDP) + intel_disable_audio(intel_encoder); if (type == INTEL_OUTPUT_EDP) { struct intel_dp *intel_dp = enc_to_intel_dp(encoder); -- 1.8.1.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] [VPG HSW-A] drm/i915:Added HDMI Audio codec disable sequence for HSW.
Hi Daniel and Mukesh, This patch is submitted on behalf of Arora Mukesh. Please have a review. And the 2nd version of patch is also submitted with subject: [PATCH v2] drm/i915/hsw: Add display Audio codec disable sequence for Haswell. Please also have a review. http://lists.freedesktop.org/archives/intel-gfx/2013-August/032488.html In the new patch, I did some modification to this original patch, to apply to both HDMI and DP port: - rename the function from hsw_hdmi_audio_disable() to intel_disable_audio() - let intel_disable_ddi() call this new function for both HDMI and DP port. - remove redundant device check, since only Haswell and its successors use DDI port operations in intel_ddi.c Thanks Mengdong -Original Message- From: intel-gfx-bounces+mengdong.lin=intel@lists.freedesktop.org [mailto:intel-gfx-bounces+mengdong.lin=intel@lists.freedesktop.org] On Behalf Of mengdong@intel.com Sent: Friday, August 30, 2013 7:48 AM To: intel-gfx@lists.freedesktop.org Cc: Arora, MukeshX Subject: [Intel-gfx] [PATCH] [VPG HSW-A] drm/i915:Added HDMI Audio codec disable sequence for HSW. From: Mukesh mukeshx.ar...@intel.com The code implements hsw_hdmi_audio_disable func which sets the relevant registers for disabling the audio codec in a call to intel_disable_ddi func.This audio codec disbale sequence is implemented as per the recommendation of the Bspec. Change-Id: If6eefbfe5ef821db547c759caa9ff5dc18980738 Signed-off-by: Mukesh Arora mukeshx.ar...@intel.com --- drivers/gpu/drm/i915/intel_ddi.c | 41 1 file changed, 41 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 0de236e..2718d9a 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1119,6 +1119,43 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder) I915_WRITE(PORT_CLK_SEL(port), PORT_CLK_SEL_NONE); } +/* Sets the registers for audio codec disable sequence as +* mentioned in the Haswell Bspec. +*/ +void hsw_hdmi_audio_disable(struct drm_encoder *encoder) { + u32 temp; + struct intel_crtc *intel_crtc = to_intel_crtc(encoder-crtc); + struct drm_device *dev = encoder-dev; + struct drm_i915_private *dev_priv = dev-dev_private; + int aud_config = HSW_AUD_CFG(intel_crtc-pipe); + + /* HDMI audio disable sequence for Haswell*/ + if (intel_crtc-eld_vld) { + /* disable timestamps */ + temp = I915_READ(aud_config); + /* write 0 for HDMI */ + temp = ~AUD_CONFIG_N_VALUE_INDEX; + /* Set N_programming_enable */ + temp |= AUD_CONFIG_N_PROG_ENABLE; + /* Set Upper_N_value and Lower_N_value + (bits 27:20, 15:4) to all 0s */ + temp = ~(AUD_CONFIG_UPPER_N_VALUE|AUD_CONFIG_LOWER_N_VALUE); + I915_WRITE(aud_config, temp); + /* Disable ELDV and ELD buffer */ + temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); + temp = ~(AUDIO_ELD_VALID_A (intel_crtc-pipe * 4)); + I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, temp); + /* Wait for 2 vertical blanks */ + intel_wait_for_vblank(dev, intel_crtc-pipe); + intel_wait_for_vblank(dev, intel_crtc-pipe); + /* Disable audio PD. This is optional as per Bspec. */ + temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); + temp = ~(AUDIO_OUTPUT_ENABLE_A (intel_crtc-pipe * 4)); + I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, temp); + } + +} static void intel_enable_ddi(struct intel_encoder *intel_encoder) { struct drm_encoder *encoder = intel_encoder-base; @@ -1173,6 +1210,10 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder) tmp = ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) (pipe * 4)); I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp); + if (IS_HASWELL(dev) (type == INTEL_OUTPUT_HDMI)) { + /*HDMI audio codec disable sequence.*/ + hsw_hdmi_audio_disable(encoder); + } } if (type == INTEL_OUTPUT_EDP) { -- 1.8.1.2 ___ 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 1/2] drm/i915: Modify DP set clock to accomodate more eDP timings.
On Sat, Aug 31, 2013 at 01:57:44AM +0800, Chon Ming Lee wrote: eDP 1.4 supports 4-5 extra link rates in additional to current 2 link rate. Create a structure to store the DPLL divisor data to improve readability. Signed-off-by: Chon Ming Lee chon.ming@intel.com This is a neat idea and I agree it'll be much better when we add edp link rates. Small comments below: --- drivers/gpu/drm/i915/intel_dp.c | 48 +++--- 1 files changed, 24 insertions(+), 24 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 2151d13..ab8a5ff 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -38,6 +38,19 @@ #define DP_LINK_CHECK_TIMEOUT(10 * 1000) +struct dp_link_dpll{ + int link_bw; + struct dpll dpll; +}; + +static const struct dp_link_dpll gen4_dpll[] = + {{ DP_LINK_BW_1_62, {2,23,8,2,10,0,0,0,0}}, + { DP_LINK_BW_2_7, {1,14,2,1,10,0,0,0,0}}}; Usually we only indent by one tab here. Also please use c99 initializers and you don't need to explicitly initialize to 0. And a bit more space helps readbility further imo: static const struct dp_link_dpll gen4_dpll[] = { { DP_LINK_BW_1_62, { .p1 = 2, .p2 = 10, .n = 2, .m1 = 23, .m2 = 8 }}, /* more ... */ }; + +static const struct dp_link_dpll pch_dpll[] = + {{ DP_LINK_BW_1_62, {1,12,9,2,10,0,0,0,0}}, + { DP_LINK_BW_2_7, {2,14,8,1,10,0,0,0,0}}}; + /** * is_edp - is the given port attached to an eDP panel (either CPU or PCH) * @intel_dp: DP struct @@ -649,37 +662,24 @@ intel_dp_set_clock(struct intel_encoder *encoder, struct intel_crtc_config *pipe_config, int link_bw) { struct drm_device *dev = encoder-base.dev; + int i; if (IS_G4X(dev)) { - if (link_bw == DP_LINK_BW_1_62) { - pipe_config-dpll.p1 = 2; - pipe_config-dpll.p2 = 10; - pipe_config-dpll.n = 2; - pipe_config-dpll.m1 = 23; - pipe_config-dpll.m2 = 8; - } else { - pipe_config-dpll.p1 = 1; - pipe_config-dpll.p2 = 10; - pipe_config-dpll.n = 1; - pipe_config-dpll.m1 = 14; - pipe_config-dpll.m2 = 2; + for(i = 0; i sizeof(gen4_dpll) / sizeof(struct dp_link_dpll); i++) { + if (link_bw == gen4_dpll[i].link_bw){ + pipe_config-dpll = gen4_dpll[i].dpll; + break; + } } pipe_config-clock_set = true; } else if (IS_HASWELL(dev)) { /* Haswell has special-purpose DP DDI clocks. */ } else if (HAS_PCH_SPLIT(dev)) { - if (link_bw == DP_LINK_BW_1_62) { - pipe_config-dpll.n = 1; - pipe_config-dpll.p1 = 2; - pipe_config-dpll.p2 = 10; - pipe_config-dpll.m1 = 12; - pipe_config-dpll.m2 = 9; - } else { - pipe_config-dpll.n = 2; - pipe_config-dpll.p1 = 1; - pipe_config-dpll.p2 = 10; - pipe_config-dpll.m1 = 14; - pipe_config-dpll.m2 = 8; + for(i = 0; i sizeof(pch_dpll) / sizeof(struct dp_link_dpll); i++) { + if (link_bw == pch_dpll[i].link_bw){ + pipe_config-dpll = pch_dpll[i].dpll; + break; + } I'd add temporary variables here to first select the right platform (and size of the array) and then only have one for loop to fish out the right value. Cheers, Daniel } pipe_config-clock_set = true; } else if (IS_VALLEYVIEW(dev)) { -- 1.7.7.6 ___ 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] drm/i915: Modify DP set clock to accomodate more eDP timings.
On Fri, 30 Aug 2013, Chon Ming Lee chon.ming@intel.com wrote: eDP 1.4 supports 4-5 extra link rates in additional to current 2 link rate. Create a structure to store the DPLL divisor data to improve readability. DP 1.2 already supports 3 link rates, right? Signed-off-by: Chon Ming Lee chon.ming@intel.com --- drivers/gpu/drm/i915/intel_dp.c | 48 +++--- 1 files changed, 24 insertions(+), 24 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 2151d13..ab8a5ff 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -38,6 +38,19 @@ #define DP_LINK_CHECK_TIMEOUT(10 * 1000) +struct dp_link_dpll{ Missing space before {. + int link_bw; + struct dpll dpll; +}; + +static const struct dp_link_dpll gen4_dpll[] = + {{ DP_LINK_BW_1_62, {2,23,8,2,10,0,0,0,0}}, + { DP_LINK_BW_2_7, {1,14,2,1,10,0,0,0,0}}}; + +static const struct dp_link_dpll pch_dpll[] = + {{ DP_LINK_BW_1_62, {1,12,9,2,10,0,0,0,0}}, + { DP_LINK_BW_2_7, {2,14,8,1,10,0,0,0,0}}}; + Please switch these to use C99 designated initializers for clarity, something like this: static const struct dp_link_dpll gen4_dpll[] = { { .link_bw = DP_LINK_BW_1_62, .dpll = { .n = 2, .m1 = 23, m2 = 8, p1 = 2, p2 = 10, }, }, { .link_bw = DP_LINK_BW_2_7, .dpll = { .n = 1, .m1 = 14, m2 = 2, p1 = 1, p2 = 10, }, } }; /** * is_edp - is the given port attached to an eDP panel (either CPU or PCH) * @intel_dp: DP struct @@ -649,37 +662,24 @@ intel_dp_set_clock(struct intel_encoder *encoder, struct intel_crtc_config *pipe_config, int link_bw) { struct drm_device *dev = encoder-base.dev; + int i; if (IS_G4X(dev)) { - if (link_bw == DP_LINK_BW_1_62) { - pipe_config-dpll.p1 = 2; - pipe_config-dpll.p2 = 10; - pipe_config-dpll.n = 2; - pipe_config-dpll.m1 = 23; - pipe_config-dpll.m2 = 8; - } else { - pipe_config-dpll.p1 = 1; - pipe_config-dpll.p2 = 10; - pipe_config-dpll.n = 1; - pipe_config-dpll.m1 = 14; - pipe_config-dpll.m2 = 2; + for(i = 0; i sizeof(gen4_dpll) / sizeof(struct dp_link_dpll); i++) { Please use i ARRAY_SIZE(gen4_dpll). + if (link_bw == gen4_dpll[i].link_bw){ + pipe_config-dpll = gen4_dpll[i].dpll; + break; + } } The old if-else used some values even if link_bw was bogus. I'm not sure how likely that is. But if the link_bw is not found in the table for some obscure reason (e.g. the hw doesn't support the link rate), this fails. Perhaps we should have a test for i == ARRAY_SIZE(gen4_dpll) here and complain loudly if we hit that, and perhaps use a fallback value. pipe_config-clock_set = true; } else if (IS_HASWELL(dev)) { /* Haswell has special-purpose DP DDI clocks. */ } else if (HAS_PCH_SPLIT(dev)) { - if (link_bw == DP_LINK_BW_1_62) { - pipe_config-dpll.n = 1; - pipe_config-dpll.p1 = 2; - pipe_config-dpll.p2 = 10; - pipe_config-dpll.m1 = 12; - pipe_config-dpll.m2 = 9; - } else { - pipe_config-dpll.n = 2; - pipe_config-dpll.p1 = 1; - pipe_config-dpll.p2 = 10; - pipe_config-dpll.m1 = 14; - pipe_config-dpll.m2 = 8; + for(i = 0; i sizeof(pch_dpll) / sizeof(struct dp_link_dpll); i++) { + if (link_bw == pch_dpll[i].link_bw){ + pipe_config-dpll = pch_dpll[i].dpll; + break; + } } Same here. BR, Jani. pipe_config-clock_set = true; } else if (IS_VALLEYVIEW(dev)) { -- 1.7.7.6 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, 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 2/2] drm/i915: Move Valleyview DP DPLL divisor calc to intel_dp_set_clock
[Okay, I missed Daniel's review, and noticed I hadn't actually hit send on this one either... but here goes anyway...] On Fri, 30 Aug 2013, Chon Ming Lee chon.ming@intel.com wrote: For DP pll settings, there is only two golden configs. Instead of running through the algorithm to determine it, hardcode the value and get it determine in intel_dp_set_clock. Signed-off-by: Chon Ming Lee chon.ming@intel.com --- drivers/gpu/drm/i915/intel_display.c | 22 -- drivers/gpu/drm/i915/intel_dp.c | 12 +++- 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f526ea9..453fa16 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -339,19 +339,6 @@ static const intel_limit_t intel_limits_vlv_hdmi = { .p2_slow = 2, .p2_fast = 20 }, }; -static const intel_limit_t intel_limits_vlv_dp = { - .dot = { .min = 25000, .max = 27 }, - .vco = { .min = 400, .max = 600 }, - .n = { .min = 1, .max = 7 }, - .m = { .min = 22, .max = 450 }, - .m1 = { .min = 2, .max = 3 }, - .m2 = { .min = 11, .max = 156 }, - .p = { .min = 10, .max = 30 }, - .p1 = { .min = 1, .max = 3 }, - .p2 = { .dot_limit = 27, - .p2_slow = 2, .p2_fast = 20 }, -}; - static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc, int refclk) { @@ -414,10 +401,8 @@ static const intel_limit_t *intel_limit(struct drm_crtc *crtc, int refclk) } else if (IS_VALLEYVIEW(dev)) { if (intel_pipe_has_type(crtc, INTEL_OUTPUT_ANALOG)) limit = intel_limits_vlv_dac; - else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI)) + else limit = intel_limits_vlv_hdmi; - else - limit = intel_limits_vlv_dp; } else if (!IS_GEN2(dev)) { if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) limit = intel_limits_i9xx_lvds; @@ -4889,15 +4874,16 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, } refclk = i9xx_get_refclk(crtc, num_connectors); + + limit = intel_limit(crtc, refclk); Did you move this here just to avoid the warning about uninitialized limit? It's a bit ugly... but then again the the whole is_dsi vs. not is rather ugly already. *shrug*. - if (!is_dsi) { + if (!is_dsi !intel_crtc-config.clock_set) { /* * Returns a set of divisors for the desired target clock with * the given refclk, or FALSE. The returned values represent * the clock equation: reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + * 2) / p1 / p2. */ - limit = intel_limit(crtc, refclk); ok = dev_priv-display.find_dpll(limit, crtc, intel_crtc-config.port_clock, refclk, NULL, clock); diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index ab8a5ff..89a2606 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -51,6 +51,10 @@ static const struct dp_link_dpll pch_dpll[] = {{ DP_LINK_BW_1_62, {1,12,9,2,10,0,0,0,0}}, { DP_LINK_BW_2_7, {2,14,8,1,10,0,0,0,0}}}; +static const struct dp_link_dpll vlv_dpll[] = + {{ DP_LINK_BW_1_62, {5,3,81,3,2,0,0,0,0}}, + { DP_LINK_BW_2_7, {1,2,27,2,2,0,0,0,0}}}; + /** * is_edp - is the given port attached to an eDP panel (either CPU or PCH) * @intel_dp: DP struct @@ -683,7 +687,13 @@ intel_dp_set_clock(struct intel_encoder *encoder, } pipe_config-clock_set = true; } else if (IS_VALLEYVIEW(dev)) { - /* FIXME: Need to figure out optimized DP clocks for vlv. */ + for(i = 0; i sizeof(vlv_dpll) / sizeof(struct dp_link_dpll); i++) { + if (link_bw == vlv_dpll[i].link_bw){ + pipe_config-dpll = vlv_dpll[i].dpll; + break; + } + } + pipe_config-clock_set = true; You now have three similar loops in the function. A follow-up patch could pick the table to use in the if branches, and have a single loop at the end. You could handle the array size by having .link_bw = 0 in the last entry as a stop condition, and using that as the fallback entry too (see my review of patch 1 about unknown link_bw values). BR, Jani. } } -- 1.7.7.6 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org
[Intel-gfx] [QA] Testing report for `drm-intel-testing` (was: Updated -next) on ww35
Summary We covered the platform: Haswell mobile, HSW desktop, HSW ULT, IvyBridge, SandyBridge, IronLake. in this circle, 5 new bugs are filed, 31 bugs are still opened, 1 WONTFIX bugs, 1 NOTABUG bugs,2 NOTOURBUG bugs, 0 Duplicated bugs,1 REOPENED bugs, 3 bugs are fixed, 13 bugs are closed. Test Environment Kernel: (drm-intel-testing)ff7528fe24d3576a7472d37ebdf665ad092ba6a2 Some additional commit info: Merge: a6e7e3d 6f1e120 Author: Daniel Vetter daniel.vet...@ffwll.ch Date: Fri Aug 23 14:53:34 2013 +0200 Finding New Bugs:5 bugs Bug 68643https://bugs.freedesktop.org/show_bug.cgi?id=68643 - [SNB DP]Some modes can't light up Bug 68640https://bugs.freedesktop.org/show_bug.cgi?id=68640 - [HSW nv]igt/prime_nv_pcopy/test3_1 timeout with debug kernel Bug 68638https://bugs.freedesktop.org/show_bug.cgi?id=68638 - [HSW nv]igt/prime_nv_test/i915_import_cpu_mmap fails with debug kernel Bug 68462https://bugs.freedesktop.org/show_bug.cgi?id=68462 - [SNB]IGT/gem_evict_alignment system hang Bug 68251https://bugs.freedesktop.org/show_bug.cgi?id=68251 - [IVB Apple MacBook pro] dmesg of machine boot up with call trace and *ERROR* mismatch in pch_pfit.pos (expected 0, found 80) Opened Bugs: 31 bugs Bug 68165https://bugs.freedesktop.org/show_bug.cgi?id=68165 - [SNB IVB HSW eDP bisected]I-G-T/testdisplay -f causes eDP black Bug 68004https://bugs.freedesktop.org/show_bug.cgi?id=68004 - igt/prime_self_import/with_one_bo_two_files aborted Bug 67891https://bugs.freedesktop.org/show_bug.cgi?id=67891 - igt/prime_self_import/export-vs-gem_close-race fail and causes call trace Bug 67889https://bugs.freedesktop.org/show_bug.cgi?id=67889 - [Baytrail-M] Blackscreen occurred after running xrandr setting twice Bug 67813https://bugs.freedesktop.org/show_bug.cgi?id=67813 - [HSW bisected]igt/module_reload causes [drm:hsw_unclaimed_reg_check] *ERROR* Unclaimed write to 44004 and system hang with headless, with power well disabled Bug 67734https://bugs.freedesktop.org/show_bug.cgi?id=67734 - [Baytrail-M] gt_RP0_freq_mhz value is equal to 0 Bug 67732https://bugs.freedesktop.org/show_bug.cgi?id=67732 - [Baytrail-M] eDP and VGA can't light up simultaneously Bug 67520https://bugs.freedesktop.org/show_bug.cgi?id=67520 - [HSW ULT] System boots with *ERROR* conflict detected with stolen region: [0xada0 - 0xafa0] Bug 67462https://bugs.freedesktop.org/show_bug.cgi?id=67462 - [SNB] Call trace appears after system boots with DP Bug 67345https://bugs.freedesktop.org/show_bug.cgi?id=67345 - [BayTrail-M] [drm:intel_pipe_config_compare] *ERROR* mismatch in clock (expected 146250, found 0) Bug 67287https://bugs.freedesktop.org/show_bug.cgi?id=67287 - igt/gem_flink_race fails Bug 67243https://bugs.freedesktop.org/show_bug.cgi?id=67243 - [ILK]igt/kms_render/gpu-blit randomly causes system hang Bug 67030https://bugs.freedesktop.org/show_bug.cgi?id=67030 - [HSW] no modes bigger than 1080p in the parsed EDID for 4k TV on both HDMI/VGA Bug 67027https://bugs.freedesktop.org/show_bug.cgi?id=67027 - [HSW] some modes oversan, on a 4K HDMI TV Bug 66301https://bugs.freedesktop.org/show_bug.cgi?id=66301 - [HSW mobile] resume from s4 sporadically causes call trace and machine is reachable Bug 65995https://bugs.freedesktop.org/show_bug.cgi?id=65995 - [HSW mobile] eDP can't light up when boot up Bug 65927https://bugs.freedesktop.org/show_bug.cgi?id=65927 - [HSW] crash when vga output set to mirror mode Bug 65700https://bugs.freedesktop.org/show_bug.cgi?id=65700 - [SNB]3[drm:ironlake_disable_pch_transcoder] *ERROR* failed to disable transcoder 0 Bug 65496https://bugs.freedesktop.org/show_bug.cgi?id=65496 - [HSW] Probabilistic resume from s4 causes call trace and system hang, with warm boot Bug 64415https://bugs.freedesktop.org/show_bug.cgi?id=64415 - [ILK] Some modes unable to light up on DP display Bug 64379https://bugs.freedesktop.org/show_bug.cgi?id=64379 - [HSW desktop bisected] ddi pll tracking botch-up due to vt-switchless resume Bug 63981https://bugs.freedesktop.org/show_bug.cgi?id=63981 - MacBook Pro retina 13 inch early 2013 jittery display Bug 61041https://bugs.freedesktop.org/show_bug.cgi?id=61041 - [Pineview]I-G-T/testdisplay VGA 1600x1200 65Hz messing the screen Bug 60002https://bugs.freedesktop.org/show_bug.cgi?id=60002 - [PNV]I-G-T/kms_flip subtest:'blocking-absolute-wf_vblank' fail Bug 5https://bugs.freedesktop.org/show_bug.cgi?id=5 - [ILK,IVB]kms_flip error: inter-flip ts jitter Bug 59836https://bugs.freedesktop.org/show_bug.cgi?id=59836 - [PNV]igt/kms_flip/absolute-wf_vblank fail Bug 59321https://bugzilla.kernel.org/show_bug.cgi?id=59321 - S4 broken with Haswell Bug 58701https://bugs.freedesktop.org/show_bug.cgi?id=58701 - [SNB DP]I-G-T/testdisplay 1920x1080i showing not full Bug 57441https://bugs.freedesktop.org/show_bug.cgi?id=57441 - [HSW]I-G-T/sysfs_l3_parity fail Bug 54687https://bugs.freedesktop.org/show_bug.cgi?id=54687 - [ilk] pipe
[Intel-gfx] [PATCH 00/19] drm/i915: More HSW watermark prep work
Another big set of prep work to get the watermarks ready for the atomic age. The main themes here are some new structures to allow us to eventually pre-compute the watermarks and trying to reduce the overhead of the watermark update itself. My plan has been to attempt to update the watermarks from the vblank irq handler, but I'm not 100% sure that I can do that. I have a big patch set at [1] that does it (and more), and for the most part it is working. But I am seeing an occasional underrun (very rarely though). I'm not sure that's caused by missing the dealine in the vblank handler or something else. In my big patch set, the next step is adding the vblank update mechanism, and after that convert ILK/SNB/IVB over to the HSW watermark code. As I'm not yet 100% convinced that the vblank thing will work out, I think I will reverse those steps, so that we can get the ILK+ stuff in while I keep looking into the underrun issue a bit more. The big patchset also has some other random bits and pieces (mainly sprite related), and I'll probably be sending some of that out as well. [1] git://gitorious.org/vsyrjala/linux.git watermarks_full ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 01/19] drm/i915: Pass crtc to intel_update_watermarks()
From: Ville Syrjälä ville.syrj...@linux.intel.com Passing the appropriate crtc to intel_update_watermarks() should help in avoiding needless work in the future. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/intel_display.c | 20 - drivers/gpu/drm/i915/intel_drv.h | 2 +- drivers/gpu/drm/i915/intel_pm.c | 43 +--- drivers/gpu/drm/i915/intel_sprite.c | 6 ++--- 5 files changed, 40 insertions(+), 33 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 7594356..ab46eb3 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -357,7 +357,7 @@ struct drm_i915_display_funcs { int target, int refclk, struct dpll *match_clock, struct dpll *best_clock); - void (*update_wm)(struct drm_device *dev); + void (*update_wm)(struct drm_crtc *crtc); void (*update_sprite_wm)(struct drm_plane *plane, struct drm_crtc *crtc, uint32_t sprite_width, int pixel_size, diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f526ea9..13856ec 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3278,7 +3278,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) intel_set_cpu_fifo_underrun_reporting(dev, pipe, true); intel_set_pch_fifo_underrun_reporting(dev, pipe, true); - intel_update_watermarks(dev); + intel_update_watermarks(crtc); for_each_encoder_on_crtc(dev, crtc, encoder) if (encoder-pre_enable) @@ -3388,7 +3388,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc) if (intel_crtc-config.has_pch_encoder) intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_A, true); - intel_update_watermarks(dev); + intel_update_watermarks(crtc); if (intel_crtc-config.has_pch_encoder) dev_priv-display.fdi_link_train(crtc); @@ -3520,7 +3520,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) } intel_crtc-active = false; - intel_update_watermarks(dev); + intel_update_watermarks(crtc); mutex_lock(dev-struct_mutex); intel_update_fbc(dev); @@ -3577,7 +3577,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) } intel_crtc-active = false; - intel_update_watermarks(dev); + intel_update_watermarks(crtc); mutex_lock(dev-struct_mutex); intel_update_fbc(dev); @@ -3677,7 +3677,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc) return; intel_crtc-active = true; - intel_update_watermarks(dev); + intel_update_watermarks(crtc); for_each_encoder_on_crtc(dev, crtc, encoder) if (encoder-pre_pll_enable) @@ -3722,7 +3722,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) return; intel_crtc-active = true; - intel_update_watermarks(dev); + intel_update_watermarks(crtc); for_each_encoder_on_crtc(dev, crtc, encoder) if (encoder-pre_enable) @@ -3806,7 +3806,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) intel_crtc-active = false; intel_update_fbc(dev); - intel_update_watermarks(dev); + intel_update_watermarks(crtc); } static void i9xx_crtc_off(struct drm_crtc *crtc) @@ -4972,7 +4972,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, ret = intel_pipe_set_base(crtc, x, y, fb); - intel_update_watermarks(dev); + intel_update_watermarks(crtc); return ret; } @@ -5860,7 +5860,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, ret = intel_pipe_set_base(crtc, x, y, fb); - intel_update_watermarks(dev); + intel_update_watermarks(crtc); return ret; } @@ -6316,7 +6316,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc, ret = intel_pipe_set_base(crtc, x, y, fb); - intel_update_watermarks(dev); + intel_update_watermarks(crtc); return ret; } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index a38f5b2..8b132e2 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -714,7 +714,7 @@ extern void hsw_fdi_link_train(struct drm_crtc *crtc); extern void intel_ddi_init(struct drm_device *dev, enum port port); /* For use by IVB LP watermark workaround in intel_sprite.c */ -extern void intel_update_watermarks(struct drm_device *dev); +extern void intel_update_watermarks(struct drm_crtc *crtc); extern void intel_update_sprite_watermarks(struct drm_plane *plane,
[Intel-gfx] [PATCH 02/19] drm/i915: Call intel_update_watermarks() in specific place during modeset
From: Ville Syrjälä ville.syrj...@linux.intel.com Make the call to intel_update_watermarks() just once or twice during modeset. Ideally it should happen independently when each plane gets enabled/disabled, but for now it seems better to keep it in central place. We can improve things when we get all the planes sorted out in a better way. When enabling set up the watermarks just before the pipe is enabled. And when disabling we need to wait until we've marked the crtc as inactive. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/intel_display.c | 19 ++- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 13856ec..a5181fe 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3278,8 +3278,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) intel_set_cpu_fifo_underrun_reporting(dev, pipe, true); intel_set_pch_fifo_underrun_reporting(dev, pipe, true); - intel_update_watermarks(crtc); - for_each_encoder_on_crtc(dev, crtc, encoder) if (encoder-pre_enable) encoder-pre_enable(encoder); @@ -3302,6 +3300,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) */ intel_crtc_load_lut(crtc); + intel_update_watermarks(crtc); intel_enable_pipe(dev_priv, pipe, intel_crtc-config.has_pch_encoder, false); intel_enable_plane(dev_priv, plane, pipe); @@ -3388,8 +3387,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc) if (intel_crtc-config.has_pch_encoder) intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_A, true); - intel_update_watermarks(crtc); - if (intel_crtc-config.has_pch_encoder) dev_priv-display.fdi_link_train(crtc); @@ -3410,6 +3407,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc) intel_ddi_set_pipe_settings(crtc); intel_ddi_enable_transcoder_func(crtc); + intel_update_watermarks(crtc); intel_enable_pipe(dev_priv, pipe, intel_crtc-config.has_pch_encoder, false); intel_enable_plane(dev_priv, plane, pipe); @@ -3677,7 +3675,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc) return; intel_crtc-active = true; - intel_update_watermarks(crtc); for_each_encoder_on_crtc(dev, crtc, encoder) if (encoder-pre_pll_enable) @@ -3696,6 +3693,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc) intel_crtc_load_lut(crtc); + intel_update_watermarks(crtc); intel_enable_pipe(dev_priv, pipe, false, is_dsi); intel_enable_plane(dev_priv, plane, pipe); intel_enable_planes(crtc); @@ -3722,7 +3720,6 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) return; intel_crtc-active = true; - intel_update_watermarks(crtc); for_each_encoder_on_crtc(dev, crtc, encoder) if (encoder-pre_enable) @@ -3734,6 +3731,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) intel_crtc_load_lut(crtc); + intel_update_watermarks(crtc); intel_enable_pipe(dev_priv, pipe, false, false); intel_enable_plane(dev_priv, plane, pipe); intel_enable_planes(crtc); @@ -3805,8 +3803,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) i9xx_disable_pll(dev_priv, pipe); intel_crtc-active = false; - intel_update_fbc(dev); intel_update_watermarks(crtc); + + intel_update_fbc(dev); } static void i9xx_crtc_off(struct drm_crtc *crtc) @@ -4972,8 +4971,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, ret = intel_pipe_set_base(crtc, x, y, fb); - intel_update_watermarks(crtc); - return ret; } @@ -5860,8 +5857,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, ret = intel_pipe_set_base(crtc, x, y, fb); - intel_update_watermarks(crtc); - return ret; } @@ -6316,8 +6311,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc, ret = intel_pipe_set_base(crtc, x, y, fb); - intel_update_watermarks(crtc); - return ret; } -- 1.8.1.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 03/19] drm/i915: Constify some watermark data
From: Ville Syrjälä ville.syrj...@linux.intel.com hsw_pipe_wm_parameters and hsw_wm_maximums typically are read only. Make them const. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/intel_pm.c | 20 ++-- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 774c57f..96493dc 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2202,7 +2202,7 @@ struct intel_wm_config { * For both WM_PIPE and WM_LP. * mem_value must be in 0.1us units. */ -static uint32_t ilk_compute_pri_wm(struct hsw_pipe_wm_parameters *params, +static uint32_t ilk_compute_pri_wm(const struct hsw_pipe_wm_parameters *params, uint32_t mem_value, bool is_lp) { @@ -2231,7 +2231,7 @@ static uint32_t ilk_compute_pri_wm(struct hsw_pipe_wm_parameters *params, * For both WM_PIPE and WM_LP. * mem_value must be in 0.1us units. */ -static uint32_t ilk_compute_spr_wm(struct hsw_pipe_wm_parameters *params, +static uint32_t ilk_compute_spr_wm(const struct hsw_pipe_wm_parameters *params, uint32_t mem_value) { uint32_t method1, method2; @@ -2254,7 +2254,7 @@ static uint32_t ilk_compute_spr_wm(struct hsw_pipe_wm_parameters *params, * For both WM_PIPE and WM_LP. * mem_value must be in 0.1us units. */ -static uint32_t ilk_compute_cur_wm(struct hsw_pipe_wm_parameters *params, +static uint32_t ilk_compute_cur_wm(const struct hsw_pipe_wm_parameters *params, uint32_t mem_value) { if (!params-active || !params-cur.enabled) @@ -2268,7 +2268,7 @@ static uint32_t ilk_compute_cur_wm(struct hsw_pipe_wm_parameters *params, } /* Only for WM_LP. */ -static uint32_t ilk_compute_fbc_wm(struct hsw_pipe_wm_parameters *params, +static uint32_t ilk_compute_fbc_wm(const struct hsw_pipe_wm_parameters *params, uint32_t pri_val) { if (!params-active || !params-pri.enabled) @@ -2419,7 +2419,7 @@ static bool ilk_check_wm(int level, static void ilk_compute_wm_level(struct drm_i915_private *dev_priv, int level, -struct hsw_pipe_wm_parameters *p, +const struct hsw_pipe_wm_parameters *p, struct intel_wm_level *result) { uint16_t pri_latency = dev_priv-wm.pri_latency[level]; @@ -2441,8 +2441,8 @@ static void ilk_compute_wm_level(struct drm_i915_private *dev_priv, } static bool hsw_compute_lp_wm(struct drm_i915_private *dev_priv, - int level, struct hsw_wm_maximums *max, - struct hsw_pipe_wm_parameters *params, + int level, const struct hsw_wm_maximums *max, + const struct hsw_pipe_wm_parameters *params, struct intel_wm_level *result) { enum pipe pipe; @@ -2462,7 +2462,7 @@ static bool hsw_compute_lp_wm(struct drm_i915_private *dev_priv, static uint32_t hsw_compute_wm_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, - struct hsw_pipe_wm_parameters *params) + const struct hsw_pipe_wm_parameters *params) { uint32_t pri_val, cur_val, spr_val; /* WM0 latency values stored in 0.1us units */ @@ -2670,8 +2670,8 @@ static void hsw_compute_wm_parameters(struct drm_device *dev, } static void hsw_compute_wm_results(struct drm_device *dev, - struct hsw_pipe_wm_parameters *params, - struct hsw_wm_maximums *lp_maximums, + const struct hsw_pipe_wm_parameters *params, + const struct hsw_wm_maximums *lp_maximums, struct hsw_wm_values *results) { struct drm_i915_private *dev_priv = dev-dev_private; -- 1.8.1.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 04/19] drm/i915: Use ilk_compute_wm_level to compute WM_PIPE values
From: Ville Syrjälä ville.syrj...@linux.intel.com Unify the code a bit to use ilk_compute_wm_level for all watermark levels. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/intel_pm.c | 44 - 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 96493dc..d118ee1 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2460,33 +2460,31 @@ static bool hsw_compute_lp_wm(struct drm_i915_private *dev_priv, return ilk_check_wm(level, max, result); } -static uint32_t hsw_compute_wm_pipe(struct drm_i915_private *dev_priv, - enum pipe pipe, + +static uint32_t hsw_compute_wm_pipe(struct drm_device *dev, const struct hsw_pipe_wm_parameters *params) { - uint32_t pri_val, cur_val, spr_val; - /* WM0 latency values stored in 0.1us units */ - uint16_t pri_latency = dev_priv-wm.pri_latency[0]; - uint16_t spr_latency = dev_priv-wm.spr_latency[0]; - uint16_t cur_latency = dev_priv-wm.cur_latency[0]; + struct drm_i915_private *dev_priv = dev-dev_private; + struct intel_wm_config config = { + .num_pipes_active = 1, + .sprites_enabled = params-spr.enabled, + .sprites_scaled = params-spr.scaled, + }; + struct hsw_wm_maximums max; + struct intel_wm_level res; + + if (!params-active) + return 0; + + ilk_wm_max(dev, 0, config, INTEL_DDB_PART_1_2, max); - pri_val = ilk_compute_pri_wm(params, pri_latency, false); - spr_val = ilk_compute_spr_wm(params, spr_latency); - cur_val = ilk_compute_cur_wm(params, cur_latency); + ilk_compute_wm_level(dev_priv, 0, params, res); - WARN(pri_val 127, -Primary WM error, mode not supported for pipe %c\n, -pipe_name(pipe)); - WARN(spr_val 127, -Sprite WM error, mode not supported for pipe %c\n, -pipe_name(pipe)); - WARN(cur_val 63, -Cursor WM error, mode not supported for pipe %c\n, -pipe_name(pipe)); + ilk_check_wm(0, max, res); - return (pri_val WM0_PIPE_PLANE_SHIFT) | - (spr_val WM0_PIPE_SPRITE_SHIFT) | - cur_val; + return (res.pri_val WM0_PIPE_PLANE_SHIFT) | + (res.spr_val WM0_PIPE_SPRITE_SHIFT) | + res.cur_val; } static uint32_t @@ -2715,7 +2713,7 @@ static void hsw_compute_wm_results(struct drm_device *dev, } for_each_pipe(pipe) - results-wm_pipe[pipe] = hsw_compute_wm_pipe(dev_priv, pipe, + results-wm_pipe[pipe] = hsw_compute_wm_pipe(dev, params[pipe]); for_each_pipe(pipe) { -- 1.8.1.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 05/19] drm/i915: Refactor max WM level
From: Ville Syrjälä ville.syrj...@linux.intel.com Pull the expected max WM level determinations out to a separate function. Will have another user soon. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/intel_pm.c | 19 +++ 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index d118ee1..163ba74 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2558,19 +2558,22 @@ static void intel_fixup_cur_wm_latency(struct drm_device *dev, uint16_t wm[5]) wm[3] *= 2; } -static void intel_print_wm_latency(struct drm_device *dev, - const char *name, - const uint16_t wm[5]) +static int ilk_wm_max_level(const struct drm_device *dev) { - int level, max_level; - /* how many WM levels are we expecting */ if (IS_HASWELL(dev)) - max_level = 4; + return 4; else if (INTEL_INFO(dev)-gen = 6) - max_level = 3; + return 3; else - max_level = 2; + return 2; +} + +static void intel_print_wm_latency(struct drm_device *dev, + const char *name, + const uint16_t wm[5]) +{ + int level, max_level = ilk_wm_max_level(dev); for (level = 0; level = max_level; level++) { unsigned int latency = wm[level]; -- 1.8.1.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 06/19] drm/i915: Add intel_pipe_wm and prepare for watermark pre-compute
From: Ville Syrjälä ville.syrj...@linux.intel.com Introduce a new struct intel_pipe_wm which contains all the watermarks for a single pipe. Use it to unify the LP0 and LP1+ watermark computations so that we can just iterate through the watermark levels neatly and call ilk_compute_wm_level() for each. Also add another tool ilk_wm_merge() that merges the LP1+ watermarks from all pipes. For that, embed one intel_pipe_wm inside intel_crtc that contains the currently valid watermarks for each pipe. This is mainly preparatory work for pre-computing the watermarks for each pipe and merging them at a later time. For now the merging still happens immediately. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/intel_drv.h | 12 +++ drivers/gpu/drm/i915/intel_pm.c | 190 +++ 2 files changed, 126 insertions(+), 76 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 8b132e2..664df77 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -293,6 +293,12 @@ struct intel_crtc_config { bool ips_enabled; }; +struct intel_pipe_wm { + struct intel_wm_level wm[5]; + uint32_t linetime; + bool fbc_wm_enabled; +}; + struct intel_crtc { struct drm_crtc base; enum pipe pipe; @@ -333,6 +339,12 @@ struct intel_crtc { /* Access to these should be protected by dev_priv-irq_lock. */ bool cpu_fifo_underrun_disabled; bool pch_fifo_underrun_disabled; + + /* per-pipe watermark state */ + struct { + /* watermarks currently being used */ + struct intel_pipe_wm active; + } wm; }; struct intel_plane_wm_parameters { diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 163ba74..c6f105f 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2440,53 +2440,6 @@ static void ilk_compute_wm_level(struct drm_i915_private *dev_priv, result-enable = true; } -static bool hsw_compute_lp_wm(struct drm_i915_private *dev_priv, - int level, const struct hsw_wm_maximums *max, - const struct hsw_pipe_wm_parameters *params, - struct intel_wm_level *result) -{ - enum pipe pipe; - struct intel_wm_level res[3]; - - for (pipe = PIPE_A; pipe = PIPE_C; pipe++) - ilk_compute_wm_level(dev_priv, level, params[pipe], res[pipe]); - - result-pri_val = max3(res[0].pri_val, res[1].pri_val, res[2].pri_val); - result-spr_val = max3(res[0].spr_val, res[1].spr_val, res[2].spr_val); - result-cur_val = max3(res[0].cur_val, res[1].cur_val, res[2].cur_val); - result-fbc_val = max3(res[0].fbc_val, res[1].fbc_val, res[2].fbc_val); - result-enable = true; - - return ilk_check_wm(level, max, result); -} - - -static uint32_t hsw_compute_wm_pipe(struct drm_device *dev, - const struct hsw_pipe_wm_parameters *params) -{ - struct drm_i915_private *dev_priv = dev-dev_private; - struct intel_wm_config config = { - .num_pipes_active = 1, - .sprites_enabled = params-spr.enabled, - .sprites_scaled = params-spr.scaled, - }; - struct hsw_wm_maximums max; - struct intel_wm_level res; - - if (!params-active) - return 0; - - ilk_wm_max(dev, 0, config, INTEL_DDB_PART_1_2, max); - - ilk_compute_wm_level(dev_priv, 0, params, res); - - ilk_check_wm(0, max, res); - - return (res.pri_val WM0_PIPE_PLANE_SHIFT) | - (res.spr_val WM0_PIPE_SPRITE_SHIFT) | - res.cur_val; -} - static uint32_t hsw_compute_linetime_wm(struct drm_device *dev, struct drm_crtc *crtc) { @@ -2670,44 +2623,121 @@ static void hsw_compute_wm_parameters(struct drm_device *dev, *lp_max_5_6 = *lp_max_1_2; } +/* Compute new watermarks for the pipe */ +static bool intel_compute_pipe_wm(struct drm_crtc *crtc, + const struct hsw_pipe_wm_parameters *params, + struct intel_pipe_wm *pipe_wm) +{ + struct drm_device *dev = crtc-dev; + struct drm_i915_private *dev_priv = dev-dev_private; + int level, max_level = ilk_wm_max_level(dev); + struct intel_wm_config config = { + .num_pipes_active = 1, + .sprites_enabled = params-spr.enabled, + .sprites_scaled = params-spr.scaled, + }; + struct hsw_wm_maximums max; + + memset(pipe_wm, 0, sizeof(*pipe_wm)); + + ilk_wm_max(dev, 0, config, INTEL_DDB_PART_1_2, max); + + for (level = 0; level = max_level; level++) + ilk_compute_wm_level(dev_priv, level, params, +pipe_wm-wm[level]); + + pipe_wm-linetime =
[Intel-gfx] [PATCH 07/19] drm/i915: Don't re-compute pipe watermarks except for the affected pipe
From: Ville Syrjälä ville.syrj...@linux.intel.com No point in re-computing the watermarks for all pipes, when only one pipe has changed. The watermarks stored under intel_crtc.wm.active are still valid for the other pipes. We just need to redo the merging. We can also skip the merge/update procedure completely if the new watermarks for the affected pipe come out unchanged. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/intel_pm.c | 67 + 1 file changed, 28 insertions(+), 39 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index c6f105f..1fb904a 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2566,29 +2566,19 @@ static void intel_setup_wm_latency(struct drm_device *dev) intel_print_wm_latency(dev, Cursor, dev_priv-wm.cur_latency); } -static void hsw_compute_wm_parameters(struct drm_device *dev, - struct hsw_pipe_wm_parameters *params, +static void hsw_compute_wm_parameters(struct drm_crtc *crtc, + struct hsw_pipe_wm_parameters *p, struct hsw_wm_maximums *lp_max_1_2, struct hsw_wm_maximums *lp_max_5_6) { - struct drm_crtc *crtc; - struct drm_plane *plane; - enum pipe pipe; + struct drm_device *dev = crtc-dev; + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + enum pipe pipe = intel_crtc-pipe; struct intel_wm_config config = {}; + struct drm_plane *plane; - list_for_each_entry(crtc, dev-mode_config.crtc_list, head) { - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - struct hsw_pipe_wm_parameters *p; - - pipe = intel_crtc-pipe; - p = params[pipe]; - - p-active = intel_crtc_active(crtc); - if (!p-active) - continue; - - config.num_pipes_active++; - + p-active = intel_crtc_active(crtc); + if (p-active) { p-pipe_htotal = intel_crtc-config.adjusted_mode.htotal; p-pixel_rate = ilk_pipe_pixel_rate(dev, crtc); p-pri.bytes_per_pixel = crtc-fb-bits_per_pixel / 8; @@ -2601,17 +2591,17 @@ static void hsw_compute_wm_parameters(struct drm_device *dev, p-cur.enabled = true; } + list_for_each_entry(crtc, dev-mode_config.crtc_list, head) + config.num_pipes_active += intel_crtc_active(crtc); + list_for_each_entry(plane, dev-mode_config.plane_list, head) { struct intel_plane *intel_plane = to_intel_plane(plane); - struct hsw_pipe_wm_parameters *p; - - pipe = intel_plane-pipe; - p = params[pipe]; - p-spr = intel_plane-wm; + if (intel_plane-pipe == pipe) + p-spr = intel_plane-wm; - config.sprites_enabled |= p-spr.enabled; - config.sprites_scaled |= p-spr.scaled; + config.sprites_enabled |= intel_plane-wm.enabled; + config.sprites_scaled |= intel_plane-wm.scaled; } ilk_wm_max(dev, 1, config, INTEL_DDB_PART_1_2, lp_max_1_2); @@ -2638,8 +2628,6 @@ static bool intel_compute_pipe_wm(struct drm_crtc *crtc, }; struct hsw_wm_maximums max; - memset(pipe_wm, 0, sizeof(*pipe_wm)); - ilk_wm_max(dev, 0, config, INTEL_DDB_PART_1_2, max); for (level = 0; level = max_level; level++) @@ -2709,7 +2697,6 @@ static void ilk_wm_merge(struct drm_device *dev, } static void hsw_compute_wm_results(struct drm_device *dev, - const struct hsw_pipe_wm_parameters *params, const struct hsw_wm_maximums *lp_maximums, struct hsw_wm_values *results) { @@ -2717,11 +2704,6 @@ static void hsw_compute_wm_results(struct drm_device *dev, int level, wm_lp; struct intel_pipe_wm lp_wm = {}; - list_for_each_entry(intel_crtc, dev-mode_config.crtc_list, base.head) - intel_compute_pipe_wm(intel_crtc-base, - params[intel_crtc-pipe], - intel_crtc-wm.active); - ilk_wm_merge(dev, lp_maximums, lp_wm); memset(results, 0, sizeof(*results)); @@ -2888,20 +2870,27 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv, static void haswell_update_wm(struct drm_crtc *crtc) { + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct drm_device *dev = crtc-dev; struct drm_i915_private *dev_priv = dev-dev_private; struct hsw_wm_maximums lp_max_1_2, lp_max_5_6; - struct hsw_pipe_wm_parameters params[3]; + struct hsw_pipe_wm_parameters params = {};
[Intel-gfx] [PATCH 09/19] drm/i915: Use intel_pipe_wm in hsw_find_best_results
From: Ville Syrjälä ville.syrj...@linux.intel.com Let's try to keep using the intermediate intel_pipe_wm representation for as long as possible. It avoids subtle knowledge about the internals of the hardware registers when trying to choose the best watermark configuration. While at it replace the memset() w/ zero initialization. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/intel_pm.c | 42 - 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index b538d09..d1184f9 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2703,8 +2703,6 @@ static void hsw_compute_wm_results(struct drm_device *dev, struct intel_crtc *intel_crtc; int level, wm_lp; - memset(results, 0, sizeof(*results)); - results-enable_fbc_wm = lp_wm-fbc_wm_enabled; /* LP1+ register values */ @@ -2744,24 +2742,26 @@ static void hsw_compute_wm_results(struct drm_device *dev, /* Find the result with the highest level enabled. Check for enable_fbc_wm in * case both are at the same level. Prefer r1 in case they're the same. */ -static struct hsw_wm_values *hsw_find_best_result(struct hsw_wm_values *r1, - struct hsw_wm_values *r2) +static struct intel_pipe_wm *hsw_find_best_result(struct drm_device *dev, + struct intel_pipe_wm *r1, + struct intel_pipe_wm *r2) { - int i, val_r1 = 0, val_r2 = 0; + int level, max_level = ilk_wm_max_level(dev); + int level1 = 0, level2 = 0; - for (i = 0; i 3; i++) { - if (r1-wm_lp[i] WM3_LP_EN) - val_r1 = r1-wm_lp[i] WM1_LP_LATENCY_MASK; - if (r2-wm_lp[i] WM3_LP_EN) - val_r2 = r2-wm_lp[i] WM1_LP_LATENCY_MASK; + for (level = 1; level = max_level; level++) { + if (r1-wm[level].enable) + level1 = level; + if (r2-wm[level].enable) + level2 = level; } - if (val_r1 == val_r2) { - if (r2-enable_fbc_wm !r1-enable_fbc_wm) + if (level1 == level2) { + if (r2-fbc_wm_enabled !r1-fbc_wm_enabled) return r2; else return r1; - } else if (val_r1 val_r2) { + } else if (level1 level2) { return r1; } else { return r2; @@ -2872,10 +2872,10 @@ static void haswell_update_wm(struct drm_crtc *crtc) struct drm_i915_private *dev_priv = dev-dev_private; struct hsw_wm_maximums lp_max_1_2, lp_max_5_6; struct hsw_pipe_wm_parameters params = {}; - struct hsw_wm_values results_1_2, results_5_6, *best_results; + struct hsw_wm_values results = {}; enum intel_ddb_partitioning partitioning; struct intel_pipe_wm pipe_wm = {}; - struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}; + struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm; hsw_compute_wm_parameters(crtc, params, lp_max_1_2, lp_max_5_6); @@ -2889,18 +2889,18 @@ static void haswell_update_wm(struct drm_crtc *crtc) ilk_wm_merge(dev, lp_max_1_2, lp_wm_1_2); ilk_wm_merge(dev, lp_max_5_6, lp_wm_5_6); - hsw_compute_wm_results(dev, lp_wm_1_2, results_1_2); if (lp_max_1_2.pri != lp_max_5_6.pri) { - hsw_compute_wm_results(dev, lp_wm_5_6, results_5_6); - best_results = hsw_find_best_result(results_1_2, results_5_6); + best_lp_wm = hsw_find_best_result(dev, lp_wm_1_2, lp_wm_5_6); } else { - best_results = results_1_2; + best_lp_wm = lp_wm_1_2; } - partitioning = (best_results == results_1_2) ? + hsw_compute_wm_results(dev, best_lp_wm, results); + + partitioning = (best_lp_wm == lp_wm_1_2) ? INTEL_DDB_PART_1_2 : INTEL_DDB_PART_5_6; - hsw_write_wm_values(dev_priv, best_results, partitioning); + hsw_write_wm_values(dev_priv, results, partitioning); } static void haswell_update_sprite_wm(struct drm_plane *plane, -- 1.8.1.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 10/19] drm/i915: Move some computations out from hsw_compute_wm_parameters()
From: Ville Syrjälä ville.syrj...@linux.intel.com Move the watermark max computations into haswell_update_wm(). This allows keeping the 1/2 vs. 5/6 split code in one place, and avoid having to pass around so many things. We also save a bit of stack space by only requiring one copy of struct hsw_wm_maximums. Also move the intel_wm_config out from hsw_compute_wm_parameters() and pass it it. We'll have some need for it in haswell_update_wm() later. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/intel_pm.c | 33 ++--- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index d1184f9..5c5fba4 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2568,13 +2568,11 @@ static void intel_setup_wm_latency(struct drm_device *dev) static void hsw_compute_wm_parameters(struct drm_crtc *crtc, struct hsw_pipe_wm_parameters *p, - struct hsw_wm_maximums *lp_max_1_2, - struct hsw_wm_maximums *lp_max_5_6) + struct intel_wm_config *config) { struct drm_device *dev = crtc-dev; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); enum pipe pipe = intel_crtc-pipe; - struct intel_wm_config config = {}; struct drm_plane *plane; p-active = intel_crtc_active(crtc); @@ -2592,7 +2590,7 @@ static void hsw_compute_wm_parameters(struct drm_crtc *crtc, } list_for_each_entry(crtc, dev-mode_config.crtc_list, head) - config.num_pipes_active += intel_crtc_active(crtc); + config-num_pipes_active += intel_crtc_active(crtc); list_for_each_entry(plane, dev-mode_config.plane_list, head) { struct intel_plane *intel_plane = to_intel_plane(plane); @@ -2600,17 +2598,9 @@ static void hsw_compute_wm_parameters(struct drm_crtc *crtc, if (intel_plane-pipe == pipe) p-spr = intel_plane-wm; - config.sprites_enabled |= intel_plane-wm.enabled; - config.sprites_scaled |= intel_plane-wm.scaled; + config-sprites_enabled |= intel_plane-wm.enabled; + config-sprites_scaled |= intel_plane-wm.scaled; } - - ilk_wm_max(dev, 1, config, INTEL_DDB_PART_1_2, lp_max_1_2); - - /* 5/6 split only in single pipe config on IVB+ */ - if (INTEL_INFO(dev)-gen = 7 config.num_pipes_active = 1) - ilk_wm_max(dev, 1, config, INTEL_DDB_PART_5_6, lp_max_5_6); - else - *lp_max_5_6 = *lp_max_1_2; } /* Compute new watermarks for the pipe */ @@ -2870,14 +2860,15 @@ static void haswell_update_wm(struct drm_crtc *crtc) struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct drm_device *dev = crtc-dev; struct drm_i915_private *dev_priv = dev-dev_private; - struct hsw_wm_maximums lp_max_1_2, lp_max_5_6; + struct hsw_wm_maximums max; struct hsw_pipe_wm_parameters params = {}; struct hsw_wm_values results = {}; enum intel_ddb_partitioning partitioning; struct intel_pipe_wm pipe_wm = {}; struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm; + struct intel_wm_config config = {}; - hsw_compute_wm_parameters(crtc, params, lp_max_1_2, lp_max_5_6); + hsw_compute_wm_parameters(crtc, params, config); intel_compute_pipe_wm(crtc, params, pipe_wm); @@ -2886,10 +2877,14 @@ static void haswell_update_wm(struct drm_crtc *crtc) intel_crtc-wm.active = pipe_wm; - ilk_wm_merge(dev, lp_max_1_2, lp_wm_1_2); - ilk_wm_merge(dev, lp_max_5_6, lp_wm_5_6); + ilk_wm_max(dev, 1, config, INTEL_DDB_PART_1_2, max); + ilk_wm_merge(dev, max, lp_wm_1_2); + + /* 5/6 split only in single pipe config on IVB+ */ + if (INTEL_INFO(dev)-gen = 7 config.num_pipes_active = 1) { + ilk_wm_max(dev, 1, config, INTEL_DDB_PART_5_6, max); + ilk_wm_merge(dev, max, lp_wm_5_6); - if (lp_max_1_2.pri != lp_max_5_6.pri) { best_lp_wm = hsw_find_best_result(dev, lp_wm_1_2, lp_wm_5_6); } else { best_lp_wm = lp_wm_1_2; -- 1.8.1.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 12/19] drm/i915: Refactor wm_lp to level calculation
From: Ville Syrjälä ville.syrj...@linux.intel.com On HSW the LP1,LP2,LP3 levels are either 1,2,3 or 1,3,4. We make the conversion from LPn to to the level at one point current. Later we're going to do it in a few places, so move it to a separate function. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/intel_pm.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 7b4f7d9..ee74352 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2686,6 +2686,12 @@ static void ilk_wm_merge(struct drm_device *dev, } } +static int ilk_wm_lp_to_level(int wm_lp, const struct intel_pipe_wm *pipe_wm) +{ + /* LP1,LP2,LP3 levels are either 1,2,3 or 1,3,4 */ + return wm_lp + (wm_lp = 2 pipe_wm-wm[4].enable); +} + static void hsw_compute_wm_results(struct drm_device *dev, const struct intel_pipe_wm *lp_wm, struct hsw_wm_values *results) @@ -2699,7 +2705,7 @@ static void hsw_compute_wm_results(struct drm_device *dev, for (wm_lp = 1; wm_lp = 3; wm_lp++) { const struct intel_wm_level *r; - level = wm_lp + (wm_lp = 2 lp_wm-wm[4].enable); + level = ilk_wm_lp_to_level(wm_lp, lp_wm); r = lp_wm-wm[level]; if (!r-enable) -- 1.8.1.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 13/19] drm/i915: Kill fbc_wm_enabled from intel_wm_config
From: Ville Syrjälä ville.syrj...@linux.intel.com The fbc_wm_enabled member in intel_wm_config is useless for the time being. The original idea for it was that we'd pre-compute it and so that the WM merging process could know whether it needs to worry about FBC watermarks at all. But we don't have a convenient way to pre-check for the possibility of FBC being used. intel_update_fbc() should be split up for that. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/intel_pm.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index ee74352..c0f2837 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2195,7 +2195,6 @@ struct intel_wm_config { unsigned int num_pipes_active; bool sprites_enabled; bool sprites_scaled; - bool fbc_wm_enabled; }; /* -- 1.8.1.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 08/19] drm/i915: Move LP1+ watermark merging out from hsw_compute_wm_results()
From: Ville Syrjälä ville.syrj...@linux.intel.com I want to convert hsw_find_best_result() to use intel_pipe_wm, so we need to move the merging to happen outside hsw_compute_wm_results(). Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/intel_pm.c | 19 ++- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 1fb904a..b538d09 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2697,26 +2697,23 @@ static void ilk_wm_merge(struct drm_device *dev, } static void hsw_compute_wm_results(struct drm_device *dev, - const struct hsw_wm_maximums *lp_maximums, + const struct intel_pipe_wm *lp_wm, struct hsw_wm_values *results) { struct intel_crtc *intel_crtc; int level, wm_lp; - struct intel_pipe_wm lp_wm = {}; - - ilk_wm_merge(dev, lp_maximums, lp_wm); memset(results, 0, sizeof(*results)); - results-enable_fbc_wm = lp_wm.fbc_wm_enabled; + results-enable_fbc_wm = lp_wm-fbc_wm_enabled; /* LP1+ register values */ for (wm_lp = 1; wm_lp = 3; wm_lp++) { const struct intel_wm_level *r; - level = wm_lp + (wm_lp = 2 lp_wm.wm[4].enable); + level = wm_lp + (wm_lp = 2 lp_wm-wm[4].enable); - r = lp_wm.wm[level]; + r = lp_wm-wm[level]; if (!r-enable) break; @@ -2878,6 +2875,7 @@ static void haswell_update_wm(struct drm_crtc *crtc) struct hsw_wm_values results_1_2, results_5_6, *best_results; enum intel_ddb_partitioning partitioning; struct intel_pipe_wm pipe_wm = {}; + struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}; hsw_compute_wm_parameters(crtc, params, lp_max_1_2, lp_max_5_6); @@ -2888,9 +2886,12 @@ static void haswell_update_wm(struct drm_crtc *crtc) intel_crtc-wm.active = pipe_wm; - hsw_compute_wm_results(dev, lp_max_1_2, results_1_2); + ilk_wm_merge(dev, lp_max_1_2, lp_wm_1_2); + ilk_wm_merge(dev, lp_max_5_6, lp_wm_5_6); + + hsw_compute_wm_results(dev, lp_wm_1_2, results_1_2); if (lp_max_1_2.pri != lp_max_5_6.pri) { - hsw_compute_wm_results(dev, lp_max_5_6, results_5_6); + hsw_compute_wm_results(dev, lp_wm_5_6, results_5_6); best_results = hsw_find_best_result(results_1_2, results_5_6); } else { best_results = results_1_2; -- 1.8.1.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 11/19] drm/i915: Don't compute 5/6 DDB split w/ zero active pipes
From: Ville Syrjälä ville.syrj...@linux.intel.com When there are zero active pipes, all the watermarks should be zero also. No point in wasting time w/ computing the 5/6 split watermark config. Signed-off-by: Ville Syrjälä ville.syrj...@linux.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 5c5fba4..7b4f7d9 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2881,7 +2881,7 @@ static void haswell_update_wm(struct drm_crtc *crtc) ilk_wm_merge(dev, max, lp_wm_1_2); /* 5/6 split only in single pipe config on IVB+ */ - if (INTEL_INFO(dev)-gen = 7 config.num_pipes_active = 1) { + if (INTEL_INFO(dev)-gen = 7 config.num_pipes_active == 1) { ilk_wm_max(dev, 1, config, INTEL_DDB_PART_5_6, max); ilk_wm_merge(dev, max, lp_wm_5_6); -- 1.8.1.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 14/19] drm/i915: Store current watermark state in dev_priv-wm
From: Ville Syrjälä ville.syrj...@linux.intel.com To make it easier to check what watermark updates are actually necessary, keep copies of the relevant bits that match the current hardware state. Also add DDB partitioning into hsw_wm_values as that's another piece of state we want to track. We don't read out the hardware state on init yet, so we can't really start using this yet, but it will be used later. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/i915_drv.h | 12 +++ drivers/gpu/drm/i915/intel_pm.c | 46 ++--- 2 files changed, 27 insertions(+), 31 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index ab46eb3..35db44b 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1088,6 +1088,15 @@ struct intel_wm_level { uint32_t fbc_val; }; +struct hsw_wm_values { + uint32_t wm_pipe[3]; + uint32_t wm_lp[3]; + uint32_t wm_lp_spr[3]; + uint32_t wm_linetime[3]; + bool enable_fbc_wm; + enum intel_ddb_partitioning partitioning; +}; + /* * This struct tracks the state needed for the Package C8+ feature. * @@ -1339,6 +1348,9 @@ typedef struct drm_i915_private { uint16_t spr_latency[5]; /* cursor */ uint16_t cur_latency[5]; + + /* current hardware state */ + struct hsw_wm_values hw; } wm; struct i915_package_c8 pc8; diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index c0f2837..27cc69d 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2182,14 +2182,6 @@ struct hsw_wm_maximums { uint16_t fbc; }; -struct hsw_wm_values { - uint32_t wm_pipe[3]; - uint32_t wm_lp[3]; - uint32_t wm_lp_spr[3]; - uint32_t wm_linetime[3]; - bool enable_fbc_wm; -}; - /* used in computing the new watermarks state */ struct intel_wm_config { unsigned int num_pipes_active; @@ -2693,12 +2685,14 @@ static int ilk_wm_lp_to_level(int wm_lp, const struct intel_pipe_wm *pipe_wm) static void hsw_compute_wm_results(struct drm_device *dev, const struct intel_pipe_wm *lp_wm, + enum intel_ddb_partitioning partitioning, struct hsw_wm_values *results) { struct intel_crtc *intel_crtc; int level, wm_lp; results-enable_fbc_wm = lp_wm-fbc_wm_enabled; + results-partitioning = partitioning; /* LP1+ register values */ for (wm_lp = 1; wm_lp = 3; wm_lp++) { @@ -2768,13 +2762,10 @@ static struct intel_pipe_wm *hsw_find_best_result(struct drm_device *dev, * causes WMs to be re-evaluated, expending some power. */ static void hsw_write_wm_values(struct drm_i915_private *dev_priv, - struct hsw_wm_values *results, - enum intel_ddb_partitioning partitioning) + struct hsw_wm_values *results) { struct hsw_wm_values previous; uint32_t val; - enum intel_ddb_partitioning prev_partitioning; - bool prev_enable_fbc_wm; previous.wm_pipe[0] = I915_READ(WM0_PIPEA_ILK); previous.wm_pipe[1] = I915_READ(WM0_PIPEB_ILK); @@ -2789,21 +2780,12 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv, previous.wm_linetime[1] = I915_READ(PIPE_WM_LINETIME(PIPE_B)); previous.wm_linetime[2] = I915_READ(PIPE_WM_LINETIME(PIPE_C)); - prev_partitioning = (I915_READ(WM_MISC) WM_MISC_DATA_PARTITION_5_6) ? + previous.partitioning = (I915_READ(WM_MISC) WM_MISC_DATA_PARTITION_5_6) ? INTEL_DDB_PART_5_6 : INTEL_DDB_PART_1_2; - prev_enable_fbc_wm = !(I915_READ(DISP_ARB_CTL) DISP_FBC_WM_DIS); - - if (memcmp(results-wm_pipe, previous.wm_pipe, - sizeof(results-wm_pipe)) == 0 - memcmp(results-wm_lp, previous.wm_lp, - sizeof(results-wm_lp)) == 0 - memcmp(results-wm_lp_spr, previous.wm_lp_spr, - sizeof(results-wm_lp_spr)) == 0 - memcmp(results-wm_linetime, previous.wm_linetime, - sizeof(results-wm_linetime)) == 0 - partitioning == prev_partitioning - results-enable_fbc_wm == prev_enable_fbc_wm) + previous.enable_fbc_wm = !(I915_READ(DISP_ARB_CTL) DISP_FBC_WM_DIS); + + if (memcmp(results, previous, sizeof(*results)) == 0) return; if (previous.wm_lp[2] != 0) @@ -2827,16 +2809,16 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv, if (previous.wm_linetime[2] != results-wm_linetime[2]) I915_WRITE(PIPE_WM_LINETIME(PIPE_C), results-wm_linetime[2]); - if (prev_partitioning != partitioning) { + if
[Intel-gfx] [PATCH 15/19] drm/i915: Improve watermark dirtyness checks
From: Ville Syrjälä ville.syrj...@linux.intel.com Currently hsw_write_vm_values() may write to certain watermark registers needlessly. For instance it will disable LP1+ watermarks around WM_PIPE changes even though that's not needed. Also if only, say, LP3 changes, the current code will again disable all LP1+ watermarks even though only LP3 needs to be reconfigured. Add an easy to read function that will compute the dirtyness of the watermarks, and use that information to further optimize the watermark programming. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/intel_pm.c | 95 + 1 file changed, 77 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 27cc69d..82e1af6 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2757,6 +2757,63 @@ static struct intel_pipe_wm *hsw_find_best_result(struct drm_device *dev, } } +/* dirty bits used to track which watermarks need changes */ +#define WM_DIRTY_PIPE(pipe) (1 (pipe)) +#define WM_DIRTY_LINETIME(pipe) (1 (8 + (pipe))) +#define WM_DIRTY_LP(wm_lp) (1 (15 + (wm_lp))) +#define WM_DIRTY_LP_ALL (WM_DIRTY_LP(1) | WM_DIRTY_LP(2) | WM_DIRTY_LP(3)) +#define WM_DIRTY_FBC (1 24) +#define WM_DIRTY_DDB (1 25) + +static unsigned int ilk_compute_wm_dirty(struct drm_device *dev, +const struct hsw_wm_values *old, +const struct hsw_wm_values *new) +{ + unsigned int dirty = 0; + enum pipe pipe; + int wm_lp; + + for_each_pipe(pipe) { + if (old-wm_linetime[pipe] != new-wm_linetime[pipe]) { + dirty |= WM_DIRTY_LINETIME(pipe); + /* Must disable LP1+ watermarks too */ + dirty |= WM_DIRTY_LP_ALL; + } + + if (old-wm_pipe[pipe] != new-wm_pipe[pipe]) + dirty |= WM_DIRTY_PIPE(pipe); + } + + if (old-enable_fbc_wm != new-enable_fbc_wm) { + dirty |= WM_DIRTY_FBC; + /* Must disable LP1+ watermarks too */ + dirty |= WM_DIRTY_LP_ALL; + } + + if (old-partitioning != new-partitioning) { + dirty |= WM_DIRTY_DDB; + /* Must disable LP1+ watermarks too */ + dirty |= WM_DIRTY_LP_ALL; + } + + /* LP1+ watermarks already deemed dirty, no need to continue */ + if (dirty WM_DIRTY_LP_ALL) + return dirty; + + /* Find the lowest numbered LP1+ watermark in need of an update... */ + for (wm_lp = 1; wm_lp = 3; wm_lp++) { + if (old-wm_lp[wm_lp - 1] != new-wm_lp[wm_lp - 1] || + old-wm_lp_spr[wm_lp - 1] != new-wm_lp_spr[wm_lp - 1]) + break; + } + + /* ...and mark it and all higher numbered LP1+ watermarks as dirty */ + for (; wm_lp = 3; wm_lp++) + dirty |= WM_DIRTY_LP(wm_lp); + + return dirty; +} + /* * The spec says we shouldn't write when we don't need, because every write * causes WMs to be re-evaluated, expending some power. @@ -2765,6 +2822,7 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv, struct hsw_wm_values *results) { struct hsw_wm_values previous; + unsigned int dirty; uint32_t val; previous.wm_pipe[0] = I915_READ(WM0_PIPEA_ILK); @@ -2785,31 +2843,32 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv, previous.enable_fbc_wm = !(I915_READ(DISP_ARB_CTL) DISP_FBC_WM_DIS); - if (memcmp(results, previous, sizeof(*results)) == 0) + dirty = ilk_compute_wm_dirty(dev_priv-dev, previous, results); + if (!dirty) return; - if (previous.wm_lp[2] != 0) + if (dirty WM_DIRTY_LP(3) previous.wm_lp[2] != 0) I915_WRITE(WM3_LP_ILK, 0); - if (previous.wm_lp[1] != 0) + if (dirty WM_DIRTY_LP(2) previous.wm_lp[1] != 0) I915_WRITE(WM2_LP_ILK, 0); - if (previous.wm_lp[0] != 0) + if (dirty WM_DIRTY_LP(1) previous.wm_lp[0] != 0) I915_WRITE(WM1_LP_ILK, 0); - if (previous.wm_pipe[0] != results-wm_pipe[0]) + if (dirty WM_DIRTY_PIPE(PIPE_A)) I915_WRITE(WM0_PIPEA_ILK, results-wm_pipe[0]); - if (previous.wm_pipe[1] != results-wm_pipe[1]) + if (dirty WM_DIRTY_PIPE(PIPE_B)) I915_WRITE(WM0_PIPEB_ILK, results-wm_pipe[1]); - if (previous.wm_pipe[2] != results-wm_pipe[2]) + if (dirty WM_DIRTY_PIPE(PIPE_C)) I915_WRITE(WM0_PIPEC_IVB, results-wm_pipe[2]); - if (previous.wm_linetime[0] != results-wm_linetime[0]) + if (dirty WM_DIRTY_LINETIME(PIPE_A)) I915_WRITE(PIPE_WM_LINETIME(PIPE_A), results-wm_linetime[0]); - if
[Intel-gfx] [PATCH 17/19] drm/i915: Remove a somewhat silly debug print from watermark code
From: Ville Syrjälä ville.syrj...@linux.intel.com This debug print just adds overhead to the watermark merging process, and doesn't really give enough information to be useful. Just kill and let's add something much better a bit later. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/intel_pm.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index fe44e86..62c85d9 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2403,8 +2403,6 @@ static bool ilk_check_wm(int level, result-enable = true; } - DRM_DEBUG_KMS(WM%d: %sabled\n, level, result-enable ? en : dis); - return ret; } -- 1.8.1.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 16/19] drm/i915: Init HSW watermark tracking in intel_modeset_setup_hw_state()
From: Ville Syrjälä ville.syrj...@linux.intel.com Fill out the HSW watermark s/w tracking structures with the current hardware state in intel_modeset_setup_hw_state(). This allows us to skip the HW state readback during watermark programming and just use the values we keep around in dev_priv-wm. Reduces the overhead of the watermark programming quite a bit. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/intel_display.c | 3 + drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_pm.c | 104 ++- 3 files changed, 82 insertions(+), 26 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a5181fe..738e285 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -10473,6 +10473,9 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, pll-on = false; } + if (IS_HASWELL(dev)) + ilk_init_wm(dev); + if (force_restore) { /* * We need to use raw interfaces for restoring state to avoid diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 664df77..a684d5e 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -809,5 +809,6 @@ extern void hsw_pc8_disable_interrupts(struct drm_device *dev); extern void hsw_pc8_restore_interrupts(struct drm_device *dev); extern void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv); extern void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv); +extern void ilk_init_wm(struct drm_device *dev); #endif /* __INTEL_DRV_H__ */ diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 82e1af6..fe44e86 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2821,37 +2821,19 @@ static unsigned int ilk_compute_wm_dirty(struct drm_device *dev, static void hsw_write_wm_values(struct drm_i915_private *dev_priv, struct hsw_wm_values *results) { - struct hsw_wm_values previous; + struct hsw_wm_values *previous = dev_priv-wm.hw; unsigned int dirty; uint32_t val; - previous.wm_pipe[0] = I915_READ(WM0_PIPEA_ILK); - previous.wm_pipe[1] = I915_READ(WM0_PIPEB_ILK); - previous.wm_pipe[2] = I915_READ(WM0_PIPEC_IVB); - previous.wm_lp[0] = I915_READ(WM1_LP_ILK); - previous.wm_lp[1] = I915_READ(WM2_LP_ILK); - previous.wm_lp[2] = I915_READ(WM3_LP_ILK); - previous.wm_lp_spr[0] = I915_READ(WM1S_LP_ILK); - previous.wm_lp_spr[1] = I915_READ(WM2S_LP_IVB); - previous.wm_lp_spr[2] = I915_READ(WM3S_LP_IVB); - previous.wm_linetime[0] = I915_READ(PIPE_WM_LINETIME(PIPE_A)); - previous.wm_linetime[1] = I915_READ(PIPE_WM_LINETIME(PIPE_B)); - previous.wm_linetime[2] = I915_READ(PIPE_WM_LINETIME(PIPE_C)); - - previous.partitioning = (I915_READ(WM_MISC) WM_MISC_DATA_PARTITION_5_6) ? - INTEL_DDB_PART_5_6 : INTEL_DDB_PART_1_2; - - previous.enable_fbc_wm = !(I915_READ(DISP_ARB_CTL) DISP_FBC_WM_DIS); - - dirty = ilk_compute_wm_dirty(dev_priv-dev, previous, results); + dirty = ilk_compute_wm_dirty(dev_priv-dev, previous, results); if (!dirty) return; - if (dirty WM_DIRTY_LP(3) previous.wm_lp[2] != 0) + if (dirty WM_DIRTY_LP(3) previous-wm_lp[2] != 0) I915_WRITE(WM3_LP_ILK, 0); - if (dirty WM_DIRTY_LP(2) previous.wm_lp[1] != 0) + if (dirty WM_DIRTY_LP(2) previous-wm_lp[1] != 0) I915_WRITE(WM2_LP_ILK, 0); - if (dirty WM_DIRTY_LP(1) previous.wm_lp[0] != 0) + if (dirty WM_DIRTY_LP(1) previous-wm_lp[0] != 0) I915_WRITE(WM1_LP_ILK, 0); if (dirty WM_DIRTY_PIPE(PIPE_A)) @@ -2886,11 +2868,11 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv, I915_WRITE(DISP_ARB_CTL, val); } - if (dirty WM_DIRTY_LP(1) previous.wm_lp_spr[0] != results-wm_lp_spr[0]) + if (dirty WM_DIRTY_LP(1) previous-wm_lp_spr[0] != results-wm_lp_spr[0]) I915_WRITE(WM1S_LP_ILK, results-wm_lp_spr[0]); - if (dirty WM_DIRTY_LP(2) previous.wm_lp_spr[1] != results-wm_lp_spr[1]) + if (dirty WM_DIRTY_LP(2) previous-wm_lp_spr[1] != results-wm_lp_spr[1]) I915_WRITE(WM2S_LP_IVB, results-wm_lp_spr[1]); - if (dirty WM_DIRTY_LP(3) previous.wm_lp_spr[2] != results-wm_lp_spr[2]) + if (dirty WM_DIRTY_LP(3) previous-wm_lp_spr[2] != results-wm_lp_spr[2]) I915_WRITE(WM3S_LP_IVB, results-wm_lp_spr[2]); if (dirty WM_DIRTY_LP(1) results-wm_lp[0] != 0) @@ -3123,6 +3105,76 @@ static void sandybridge_update_sprite_wm(struct drm_plane *plane, I915_WRITE(WM3S_LP_IVB,
[Intel-gfx] [PATCH 18/19] drm/i915: Adjust watermark register masks
From: Ville Syrjälä ville.syrj...@linux.intel.com We want to be able to use the masks to decode the register contents regardless of the hardware generation. So just expand the masks to cover all available bits, even if those are reserved on some generations. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/i915_reg.h | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 35a6c74..ff23680 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -3219,11 +3219,11 @@ /* define the Watermark register on Ironlake */ #define WM0_PIPEA_ILK 0x45100 -#define WM0_PIPE_PLANE_MASK (0x7f16) +#define WM0_PIPE_PLANE_MASK (0x16) #define WM0_PIPE_PLANE_SHIFT 16 -#define WM0_PIPE_SPRITE_MASK (0x3f8) +#define WM0_PIPE_SPRITE_MASK (0xff8) #define WM0_PIPE_SPRITE_SHIFT 8 -#define WM0_PIPE_CURSOR_MASK (0x1f) +#define WM0_PIPE_CURSOR_MASK (0xff) #define WM0_PIPEB_ILK 0x45104 #define WM0_PIPEC_IVB 0x45200 @@ -3233,9 +3233,9 @@ #define WM1_LP_LATENCY_MASK (0x7f24) #define WM1_LP_FBC_MASK (0xf20) #define WM1_LP_FBC_SHIFT 20 -#define WM1_LP_SR_MASK(0x1ff8) +#define WM1_LP_SR_MASK(0xfff8) #define WM1_LP_SR_SHIFT 8 -#define WM1_LP_CURSOR_MASK(0x3f) +#define WM1_LP_CURSOR_MASK(0xff) #define WM2_LP_ILK 0x4510c #define WM2_LP_EN (131) #define WM3_LP_ILK 0x45110 -- 1.8.1.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 19/19] drm/i915: Add watermark tracepoints
From: Ville Syrjälä ville.syrj...@linux.intel.com We may want to know what kind of watermarks got computed and programmed into the hardware. Using tracepoints is much leaner than debug prints. Also add trace call for the watermark state we read out of the hardware during init, though I;m not sure there's any way to see that trace as the events aren't available until the module is loaded. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/i915_trace.h | 181 ++ drivers/gpu/drm/i915/intel_pm.c | 42 - 2 files changed, 220 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h index e2c5ee6..6dc9a91 100644 --- a/drivers/gpu/drm/i915/i915_trace.h +++ b/drivers/gpu/drm/i915/i915_trace.h @@ -14,6 +14,187 @@ #define TRACE_SYSTEM_STRING __stringify(TRACE_SYSTEM) #define TRACE_INCLUDE_FILE i915_trace +/* watermark */ + +TRACE_EVENT(i915_wm_update_start, + TP_PROTO(enum pipe pipe), + TP_ARGS(pipe), + + TP_STRUCT__entry( +__field(enum pipe, pipe) +), + + TP_fast_assign( + __entry-pipe = pipe; + ), + + TP_printk(pipe %c, pipe_name(__entry-pipe)) +); + +TRACE_EVENT(i915_wm_update_end, + TP_PROTO(enum pipe pipe, bool changed), + TP_ARGS(pipe, changed), + + TP_STRUCT__entry( +__field(enum pipe, pipe) +__field(bool, changed) +), + + TP_fast_assign( + __entry-pipe = pipe; + __entry-changed = changed; + ), + + TP_printk(pipe %c, changed=%s, + pipe_name(__entry-pipe), __entry-changed ? yes : no) +); + +TRACE_EVENT_CONDITION(i915_wm_misc, + TP_PROTO(const struct hsw_wm_values *hw, bool trace), + TP_ARGS(hw, trace), + + TP_CONDITION(trace), + + TP_STRUCT__entry( + __field(bool, enable_fbc_wm) + __field(enum intel_ddb_partitioning, partitioning) + ), + + TP_fast_assign( + __entry-enable_fbc_wm = hw-enable_fbc_wm; + __entry-partitioning = hw-partitioning; + ), + + TP_printk(fbc=%s, ddb partitioning=%s, + __entry-enable_fbc_wm ? yes : no, + __entry-partitioning == INTEL_DDB_PART_5_6 ? 5/6 : 1/2) +); + +TRACE_EVENT_CONDITION(i915_wm_pipe, + TP_PROTO(struct drm_device *dev, enum pipe pipe, const struct hsw_wm_values *hw, bool trace), + TP_ARGS(dev, pipe, hw, trace), + + TP_CONDITION(pipe INTEL_INFO(dev)-num_pipes trace), + + TP_STRUCT__entry( + __field(enum pipe, pipe) + __field(uint32_t, wm_pipe) + ), + + TP_fast_assign( + __entry-pipe = pipe; + __entry-wm_pipe = hw-wm_pipe[pipe]; + ), + + TP_printk(pipe %c: pri=%u, spr=%u, cur=%u, + pipe_name(__entry-pipe), + (__entry-wm_pipe WM0_PIPE_PLANE_MASK) WM0_PIPE_PLANE_SHIFT, + (__entry-wm_pipe WM0_PIPE_SPRITE_MASK) WM0_PIPE_SPRITE_SHIFT, + __entry-wm_pipe WM0_PIPE_CURSOR_MASK) +); + +TRACE_EVENT_CONDITION(i915_wm_linetime, + TP_PROTO(struct drm_device *dev, enum pipe pipe, const struct hsw_wm_values *hw, bool trace), + TP_ARGS(dev, pipe, hw, trace), + + TP_CONDITION(IS_HASWELL(dev) pipe INTEL_INFO(dev)-num_pipes trace), + + TP_STRUCT__entry( + __field(enum pipe, pipe) + __field(uint32_t, wm_linetime) + ), + + TP_fast_assign( + __entry-pipe = pipe; + __entry-wm_linetime = hw-wm_linetime[pipe]; + ), + + TP_printk(pipe %c: linetime=%u, ips linetime=%u, + pipe_name(__entry-pipe), + __entry-wm_linetime PIPE_WM_LINETIME_MASK, + (__entry-wm_linetime PIPE_WM_LINETIME_IPS_LINETIME_MASK) 16) +); + + +TRACE_EVENT_CONDITION(i915_wm_lp1_ilk, + TP_PROTO(struct drm_device *dev, const struct hsw_wm_values *hw, bool trace), + TP_ARGS(dev, hw, trace), + + TP_CONDITION(INTEL_INFO(dev)-gen = 6 trace), + + TP_STRUCT__entry( + __field(uint32_t, wm_lp) + __field(uint32_t, wm_lp_spr) + ), + + TP_fast_assign( + __entry-wm_lp = hw-wm_lp[0]; + __entry-wm_lp_spr = hw-wm_lp_spr[0]; + ), + + TP_printk(LP1: en=%s, lat=%u, fbc=%u, pri=%u, cur=%u, spr=%u, spr en=%s, + __entry-wm_lp WM1_LP_SR_EN ? yes : no, + (__entry-wm_lp WM1_LP_LATENCY_MASK) WM1_LP_LATENCY_SHIFT, + (__entry-wm_lp WM1_LP_FBC_MASK) WM1_LP_FBC_SHIFT, +
Re: [Intel-gfx] [PATCH 0/2] vgaarb: Fixes for partial VGA opt-out
On Thu, Aug 15, 2013 at 04:37:47PM -0600, Alex Williamson wrote: I'm trying to add support for VGA arbitration on newer Intel graphics devices. The existing code attempts to do this, but appear to have not been updated since GMCH devices roamed the Earth. On newer devices like Haswell, we can disable VGA memory through an MSR on the device, but we rely on the VGA arbiter to manage VGA IO using the PCI COMMAND register. In trying to unregister legacy VGA memory, I found that the VGA arbiter still wanted to disable both memory and IO on the device and that it forgot to actually program the device to disable IO when the decoding is updated. This series attempts to fix both of those. Thanks, The series looks good to me. Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com Alex --- Alex Williamson (2): vgaarb: Don't disable resources that are not owned vgaarb: Fix VGA decodes changes drivers/gpu/vga/vgaarb.c | 50 -- 1 file changed, 22 insertions(+), 28 deletions(-) -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] drm/i915: ban badly behaving contexts
Now when we have mechanism in place to track which context was guilty of hanging the gpu, it is possible to punish for bad behaviour. If context has recently submitted a faulty batchbuffers guilty of gpu hang and submits another batch which hangs gpu in quick succession, ban it permanently. If ctx is banned, no more batchbuffers will be queued for execution. There is no need for global wedge machinery anymore and it would be unwise to wedge the whole gpu if we have multiple hanging batches queued for execution. Instead just ban the guilty ones and carry on. v2: Store guilty ban status bool in gpu_error instead of pointers that might become danling before hang is declared. v3: Use return value for banned status instead of stashing state into gpu_error (Chris Wilson) v4: - rebase on top of fixed hang stats api - add define for ban period - rename commit and improve commit msg v5: - rely context banning instead of wedging the gpu - beautification and fix for ban calculation (Chris) Signed-off-by: Mika Kuoppala mika.kuopp...@intel.com --- drivers/gpu/drm/i915/i915_drv.c| 29 drivers/gpu/drm/i915/i915_drv.h| 11 +-- drivers/gpu/drm/i915/i915_gem.c| 22 +++-- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 12 4 files changed, 53 insertions(+), 21 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index eb56f68..a3beef3 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -719,24 +719,19 @@ int i915_reset(struct drm_device *dev) simulated = dev_priv-gpu_error.stop_rings != 0; - if (!simulated get_seconds() - dev_priv-gpu_error.last_reset 5) { - DRM_ERROR(GPU hanging too fast, declaring wedged!\n); - ret = -ENODEV; - } else { - ret = intel_gpu_reset(dev); - - /* Also reset the gpu hangman. */ - if (simulated) { - DRM_INFO(Simulated gpu hang, resetting stop_rings\n); - dev_priv-gpu_error.stop_rings = 0; - if (ret == -ENODEV) { - DRM_ERROR(Reset not implemented, but ignoring - error for simulated gpu hangs\n); - ret = 0; - } - } else - dev_priv-gpu_error.last_reset = get_seconds(); + ret = intel_gpu_reset(dev); + + /* Also reset the gpu hangman. */ + if (simulated) { + DRM_INFO(Simulated gpu hang, resetting stop_rings\n); + dev_priv-gpu_error.stop_rings = 0; + if (ret == -ENODEV) { + DRM_ERROR(Reset not implemented, but ignoring + error for simulated gpu hangs\n); + ret = 0; + } } + if (ret) { DRM_ERROR(Failed to reset chip.\n); mutex_unlock(dev-struct_mutex); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 4da1f86..2c5f3bc 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -583,6 +583,12 @@ struct i915_ctx_hang_stats { /* This context had batch active when hang was declared */ unsigned batch_active; + + /* Time when this context was last blamed for a GPU reset */ + unsigned long guilty_ts; + + /* This context is banned to submit more work */ + bool banned; }; /* This must match up with the value previously used for execbuf2.rsvd1. */ @@ -984,6 +990,9 @@ struct i915_gpu_error { /* For hangcheck timer */ #define DRM_I915_HANGCHECK_PERIOD 1500 /* in ms */ #define DRM_I915_HANGCHECK_JIFFIES msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD) + /* Hang gpu twice in this window and your context gets banned */ +#define DRM_I915_CTX_BAN_PERIOD DIV_ROUND_UP(8*DRM_I915_HANGCHECK_PERIOD, 1000) + struct timer_list hangcheck_timer; /* For reset and error_state handling. */ @@ -992,8 +1001,6 @@ struct i915_gpu_error { struct drm_i915_error_state *first_error; struct work_struct work; - unsigned long last_reset; - /** * State variable and reset counter controlling the reset flow * diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 7392f64..754f875 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2201,6 +2201,21 @@ static bool i915_request_guilty(struct drm_i915_gem_request *request, return false; } +static bool i915_context_is_banned(const struct i915_ctx_hang_stats *hs) +{ + const unsigned long elapsed = get_seconds() - hs-guilty_ts; + + if (hs-banned) + return true; + + if (elapsed =
[Intel-gfx] [PATCH 2/2] drm/i915: add hangcheck action and score to error state
Score and action reveal what the rings were doing when hang was declared. Signed-off-by: Mika Kuoppala mika.kuopp...@intel.com --- drivers/gpu/drm/i915/i915_drv.h |2 ++ drivers/gpu/drm/i915/i915_gpu_error.c | 22 ++ 2 files changed, 24 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 2c5f3bc..16629cc 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -326,6 +326,8 @@ struct drm_i915_error_state { u32 *active_bo_count, *pinned_bo_count; struct intel_overlay_error_state *overlay; struct intel_display_error_state *display; + int hangcheck_score[I915_NUM_RINGS]; + enum intel_ring_hangcheck_action hangcheck_action[I915_NUM_RINGS]; }; struct intel_crtc_config; diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index aba9d74..7e92ba8 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -213,6 +213,22 @@ static void print_error_buffers(struct drm_i915_error_state_buf *m, } } +static const char *hangcheck_action_to_str(enum intel_ring_hangcheck_action a) +{ + switch (a) { + case HANGCHECK_WAIT: + return wait; + case HANGCHECK_ACTIVE: + return active; + case HANGCHECK_KICK: + return kick; + case HANGCHECK_HUNG: + return hung; + } + + return unknown; +} + static void i915_ring_error_state(struct drm_i915_error_state_buf *m, struct drm_device *dev, struct drm_i915_error_state *error, @@ -253,6 +269,9 @@ static void i915_ring_error_state(struct drm_i915_error_state_buf *m, err_printf(m, waiting: %s\n, yesno(error-waiting[ring])); err_printf(m, ring-head: 0x%08x\n, error-cpu_ring_head[ring]); err_printf(m, ring-tail: 0x%08x\n, error-cpu_ring_tail[ring]); + err_printf(m, hangcheck_action: %s\n, + hangcheck_action_to_str(error-hangcheck_action[ring])); + err_printf(m, hangcheck_score: %d\n, error-hangcheck_score[ring]); } void i915_error_printf(struct drm_i915_error_state_buf *e, const char *f, ...) @@ -718,6 +737,9 @@ static void i915_record_ring_state(struct drm_device *dev, error-cpu_ring_head[ring-id] = ring-head; error-cpu_ring_tail[ring-id] = ring-tail; + + error-hangcheck_score[ring-id] = ring-hangcheck.score; + error-hangcheck_action[ring-id] = ring-hangcheck.action; } -- 1.7.9.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: It's its!
Signed-off-by: Damien Lespiau damien.lesp...@intel.com --- drivers/gpu/drm/i915/i915_gem_context.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 403309c..e038513 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -73,7 +73,7 @@ * * There are two confusing terms used above: * The current context means the context which is currently running on the - * GPU. The GPU has loaded it's state already and has stored away the gtt + * GPU. The GPU has loaded its state already and has stored away the gtt * offset of the BO. The GPU is not actively referencing the data at this * offset, but it will on the next context switch. The only way to avoid this * is to do a GPU reset. -- 1.8.3.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/17] drm/i915: Do not add an interrupt for a context switch
On Mon, Aug 26, 2013 at 07:50:53PM -0300, Rodrigo Vivi wrote: From: Chris Wilson ch...@chris-wilson.co.uk We use the request to ensure we hold a reference to the context for the duration that it remains in use by the ring. Each request only holds a reference to the current context, hence we emit a request after switching contexts with the final reference to the old context. However, the extra interrupt caused by that request is not useful (no timing critical function will wait for the context object), instead the overhead of servicing the IRQ shows up in some (lightweight) benchmarks. In order to keep the useful property of using the request to manage the context lifetime, we want to add a dummy request that is associated with the interrupt from the subsequent real request following the batch. The extra interrupt was added as a side-effect of using i915_add_request() in commit 112522f6789581824903f6f72082b5b841a7f0f9 Author: Chris Wilson ch...@chris-wilson.co.uk Date: Thu May 2 16:48:07 2013 +0300 drm/i915: put context upon switching v2: Daniel convinced me that the request here was solely for context lifetime tracking and that we have the active ref to keep the object alive whilst the MI_SET_CONTEXT. So the only concern then is which context should get the blame for MI_SET_CONTEXT failing. The old scheme added a request for the old context so that any hang upto and including the switch away would mark the old context as guilty. Now any hang here implicates the new context. However since we have already gone through a complete flush with the last context in its last request, and all that lies in no-man's-land is an invalidate flush and the MI_SET_CONTEXT, we should be safe in not unduly placing blame on the new context. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Ben Widawsky b...@bwidawsk.net Cc: Paulo Zanoni paulo.r.zan...@intel.com I'm somewhat new to the core GEM code, but I'm convinced by the commit message and the patch does what it says. So: Reviewed-by: Damien Lespiau damien.lesp...@intel.com -- Damien --- drivers/gpu/drm/i915/i915_gem.c | 1 - drivers/gpu/drm/i915/i915_gem_context.c | 12 +--- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 2d1cb10..56c9104 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2046,7 +2046,6 @@ int __i915_add_request(struct intel_ring_buffer *ring, if (request == NULL) return -ENOMEM; - /* Record the position of the start of the request so that * should we detect the updated seqno part-way through the * GPU processing the request, we never over-estimate the diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 403309c..b6da70b 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -451,17 +451,7 @@ static int do_switch(struct i915_hw_context *to) from-obj-dirty = 1; BUG_ON(from-obj-ring != ring); - ret = i915_add_request(ring, NULL); - if (ret) { - /* Too late, we've already scheduled a context switch. - * Try to undo the change so that the hw state is - * consistent with out tracking. In case of emergency, - * scream. - */ - WARN_ON(mi_set_context(ring, from, MI_RESTORE_INHIBIT)); - return ret; - } - + /* obj is kept alive until the next request by its active ref */ i915_gem_object_unpin(from-obj); i915_gem_context_unreference(from); } -- 1.8.1.4 ___ 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 03/17] drm/i915: Pin pages whilst mapping the dma-buf
On Mon, Aug 26, 2013 at 07:50:55PM -0300, Rodrigo Vivi wrote: From: Chris Wilson ch...@chris-wilson.co.uk As we attempt to kmalloc after calling get_pages, there is a possibility that the shrinker may reap the pages we just acquired. To prevent this we need to increment the pages_pin_count early, so rearrange the code and error paths to make it so. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Reviewed-by: Damien Lespiau damien.lesp...@intel.com -- Damien --- drivers/gpu/drm/i915/i915_gem_dmabuf.c | 41 ++ 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index e918b05..7d5752f 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -42,27 +42,24 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme ret = i915_mutex_lock_interruptible(obj-base.dev); if (ret) - return ERR_PTR(ret); + goto err; ret = i915_gem_object_get_pages(obj); - if (ret) { - st = ERR_PTR(ret); - goto out; - } + if (ret) + goto err_unlock; + + i915_gem_object_pin_pages(obj); /* Copy sg so that we make an independent mapping */ st = kmalloc(sizeof(struct sg_table), GFP_KERNEL); if (st == NULL) { - st = ERR_PTR(-ENOMEM); - goto out; + ret = -ENOMEM; + goto err_unpin; } ret = sg_alloc_table(st, obj-pages-nents, GFP_KERNEL); - if (ret) { - kfree(st); - st = ERR_PTR(ret); - goto out; - } + if (ret) + goto err_free; src = obj-pages-sgl; dst = st-sgl; @@ -73,17 +70,23 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme } if (!dma_map_sg(attachment-dev, st-sgl, st-nents, dir)) { - sg_free_table(st); - kfree(st); - st = ERR_PTR(-ENOMEM); - goto out; + ret =-ENOMEM; + goto err_free_sg; } - i915_gem_object_pin_pages(obj); - -out: mutex_unlock(obj-base.dev-struct_mutex); return st; + +err_free_sg: + sg_free_table(st); +err_free: + kfree(st); +err_unpin: + i915_gem_object_unpin_pages(obj); +err_unlock: + mutex_unlock(obj-base.dev-struct_mutex); +err: + return ERR_PTR(ret); } static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, -- 1.8.1.4 ___ 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: Don't call sg_free_table() if sg_alloc_table() fails
On Fri, Aug 30, 2013 at 03:39:26PM +0100, Damien Lespiau wrote: One needs to call __sg_free_table() if __sg_alloc_table() fails, but sg_alloc_table() does that for us already. Signed-off-by: Damien Lespiau damien.lesp...@intel.com Reviewd-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
[Intel-gfx] [PATCH] drm/i915: Don't call sg_free_table() if sg_alloc_table() fails
One needs to call __sg_free_table() if __sg_alloc_table() fails, but sg_alloc_table() does that for us already. Signed-off-by: Damien Lespiau damien.lesp...@intel.com --- drivers/gpu/drm/i915/i915_gem.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index f705314..8eb8e17 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1772,7 +1772,6 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj) page_count = obj-base.size / PAGE_SIZE; if (sg_alloc_table(st, page_count, GFP_KERNEL)) { - sg_free_table(st); kfree(st); return -ENOMEM; } -- 1.8.3.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/6] drm/i915: add opregion function to notify bios of adapter power state
Notifying the bios lets it enter power saving states. Signed-off-by: Jani Nikula jani.nik...@intel.com --- drivers/gpu/drm/i915/i915_drv.h |7 +++ drivers/gpu/drm/i915/intel_opregion.c | 27 +++ 2 files changed, 34 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 5339297..7daae2a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2190,6 +2190,8 @@ extern void intel_opregion_fini(struct drm_device *dev); extern void intel_opregion_asle_intr(struct drm_device *dev); extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, bool enable); +extern int intel_opregion_notify_adapter(struct drm_device *dev, +pci_power_t state); #else static inline void intel_opregion_init(struct drm_device *dev) { return; } static inline void intel_opregion_fini(struct drm_device *dev) { return; } @@ -2199,6 +2201,11 @@ intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, bool enable) { return 0; } +static inline int +intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state) +{ + return 0; +} #endif /* intel_acpi.c */ diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c index 791991b..05243df 100644 --- a/drivers/gpu/drm/i915/intel_opregion.c +++ b/drivers/gpu/drm/i915/intel_opregion.c @@ -342,6 +342,33 @@ int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, return swsci(dev, SWSCI_SBCB_DISPLAY_POWER_STATE, parm, NULL); } +static const struct { + pci_power_t pci_power_state; + u32 parm; +} power_state_map[] = { + { PCI_D0, 0x00 }, + { PCI_D1, 0x01 }, + { PCI_D2, 0x02 }, + { PCI_D3hot,0x04 }, + { PCI_D3cold, 0x04 }, +}; + +int intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state) +{ + int i; + + if (!HAS_DDI(dev)) + return 0; + + for (i = 0; i ARRAY_SIZE(power_state_map); i++) { + if (state == power_state_map[i].pci_power_state) + return swsci(dev, SWSCI_SBCB_ADAPTER_POWER_STATE, +power_state_map[i].parm, NULL); + } + + return -EINVAL; +} + static u32 asle_set_backlight(struct drm_device *dev, u32 bclp) { struct drm_i915_private *dev_priv = dev-dev_private; -- 1.7.10.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 5/6] drm/i915: do display power state notification on crtc enable/disable
The spec says to notify prior to power down and after power up. It is unclear whether it makes a difference. Signed-off-by: Jani Nikula jani.nik...@intel.com Reviewed-by: Paulo Zanoni paulo.r.zan...@intel.com --- Paulo, still okay with the r-b? --- drivers/gpu/drm/i915/intel_display.c |8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9222e570..83d853f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3425,8 +3425,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc) intel_update_fbc(dev); mutex_unlock(dev-struct_mutex); - for_each_encoder_on_crtc(dev, crtc, encoder) + for_each_encoder_on_crtc(dev, crtc, encoder) { encoder-enable(encoder); + intel_opregion_notify_encoder(encoder, true); + } /* * There seems to be a race in PCH platform hw (at least on some @@ -3540,8 +3542,10 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) if (!intel_crtc-active) return; - for_each_encoder_on_crtc(dev, crtc, encoder) + for_each_encoder_on_crtc(dev, crtc, encoder) { + intel_opregion_notify_encoder(encoder, false); encoder-disable(encoder); + } intel_crtc_wait_for_pending_flips(crtc); drm_vblank_off(dev, pipe); -- 1.7.10.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 6/6] DRAFT: drm/i915: do adapter power state notification on PC8+ enable/disable
v2: - go to PCI_D3cold - shuffle the call site a bit Signed-off-by: Jani Nikula jani.nik...@intel.com --- 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 83d853f..e33fa6d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6126,6 +6126,7 @@ void hsw_enable_pc8_work(struct work_struct *__work) lpt_disable_clkout_dp(dev); hsw_pc8_disable_interrupts(dev); + intel_opregion_notify_adapter(dev, PCI_D3cold); hsw_disable_lcpll(dev_priv, true, true); } @@ -6163,6 +6164,7 @@ static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv) DRM_DEBUG_KMS(Disabling package C8+\n); hsw_restore_lcpll(dev_priv); + intel_opregion_notify_adapter(dev, PCI_D0); hsw_pc8_restore_interrupts(dev); lpt_init_pch_refclk(dev); -- 1.7.10.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/6] drm/i915: BIOS display/adapter power state notifications
Hi all, a new version of [1] addressing Paulo's review comments. I hope I didn't rush the changes too much; the new info about requested vs. supported callbacks is confusing to say the least... I'm sure Daniel will appreciate any Tested-by's! BR, Jani. [1] http://mid.gmane.org/cover.1377246881.git.jani.nik...@intel.com Jani Nikula (6): drm/i915: expose intel_ddi_get_encoder_port() drm/i915: add plumbing for SWSCI drm/i915: add opregion function to notify bios of encoder enable/disable drm/i915: add opregion function to notify bios of adapter power state drm/i915: do display power state notification on crtc enable/disable DRAFT: drm/i915: do adapter power state notification on PC8+ enable/disable drivers/gpu/drm/i915/i915_drv.h | 17 ++ drivers/gpu/drm/i915/intel_ddi.c |2 +- drivers/gpu/drm/i915/intel_display.c | 10 +- drivers/gpu/drm/i915/intel_drv.h |1 + drivers/gpu/drm/i915/intel_opregion.c | 276 - 5 files changed, 300 insertions(+), 6 deletions(-) -- 1.7.10.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/6] drm/i915: add plumbing for SWSCI
SWSCI is a driver to bios call interface. This checks for SWSCI availability and bios requested callbacks, and filters out any calls that shouldn't happen. This way the callers don't need to do the checks all over the place. v2: silence some checkpatch nagging v3: set PCI_SWSCI bit 0 to trigger interrupt (Mengdong Lin) v4: remove an extra #define (Jesse) v5: spec says s/w is responsible for clearing PCI_SWSCI bit 0 too v6: per Paulo's review and more: - fix sub-function mask - add exit parameter - add define for set panel details call - return more errors from swsci - clean up the supported/requested callbacks bit masks mess - use DSLP for timeout - fix build for CONFIG_ACPI=n Signed-off-by: Jani Nikula jani.nik...@intel.com --- drivers/gpu/drm/i915/i915_drv.h |2 + drivers/gpu/drm/i915/intel_opregion.c | 198 - 2 files changed, 197 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 0602c4b..67673e2 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -225,6 +225,8 @@ struct intel_opregion { struct opregion_header __iomem *header; struct opregion_acpi __iomem *acpi; struct opregion_swsci __iomem *swsci; + u32 swsci_gbda_sub_functions; + u32 swsci_sbcb_sub_functions; struct opregion_asle __iomem *asle; void __iomem *vbt; u32 __iomem *lid_state; diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c index cfb8fb6..c6a8d61 100644 --- a/drivers/gpu/drm/i915/intel_opregion.c +++ b/drivers/gpu/drm/i915/intel_opregion.c @@ -36,8 +36,11 @@ #include i915_drv.h #include intel_drv.h -#define PCI_ASLE 0xe4 -#define PCI_ASLS 0xfc +#define PCI_ASLE 0xe4 +#define PCI_ASLS 0xfc +#define PCI_SWSCI 0xe8 +#define PCI_SWSCI_SCISEL (1 15) +#define PCI_SWSCI_GSSCIE (1 0) #define OPREGION_HEADER_OFFSET 0 #define OPREGION_ACPI_OFFSET 0x100 @@ -151,6 +154,51 @@ struct opregion_asle { #define ASLE_CBLV_VALID (131) +/* Software System Control Interrupt (SWSCI) */ +#define SWSCI_SCIC_INDICATOR (1 0) +#define SWSCI_SCIC_MAIN_FUNCTION_SHIFT 1 +#define SWSCI_SCIC_MAIN_FUNCTION_MASK (0xf 1) +#define SWSCI_SCIC_SUB_FUNCTION_SHIFT 8 +#define SWSCI_SCIC_SUB_FUNCTION_MASK (0xff 8) +#define SWSCI_SCIC_EXIT_PARAMETER_SHIFT8 +#define SWSCI_SCIC_EXIT_PARAMETER_MASK (0xff 8) +#define SWSCI_SCIC_EXIT_STATUS_SHIFT 5 +#define SWSCI_SCIC_EXIT_STATUS_MASK(7 5) +#define SWSCI_SCIC_EXIT_STATUS_SUCCESS 1 + +#define SWSCI_FUNCTION_CODE(main, sub) \ + ((main) SWSCI_SCIC_MAIN_FUNCTION_SHIFT | \ +(sub) SWSCI_SCIC_SUB_FUNCTION_SHIFT) + +/* SWSCI: Get BIOS Data (GBDA) */ +#define SWSCI_GBDA 4 +#define SWSCI_GBDA_SUPPORTED_CALLS SWSCI_FUNCTION_CODE(SWSCI_GBDA, 0) +#define SWSCI_GBDA_REQUESTED_CALLBACKS SWSCI_FUNCTION_CODE(SWSCI_GBDA, 1) +#define SWSCI_GBDA_BOOT_DISPLAY_PREF SWSCI_FUNCTION_CODE(SWSCI_GBDA, 4) +#define SWSCI_GBDA_PANEL_DETAILS SWSCI_FUNCTION_CODE(SWSCI_GBDA, 5) +#define SWSCI_GBDA_TV_STANDARD SWSCI_FUNCTION_CODE(SWSCI_GBDA, 6) +#define SWSCI_GBDA_INTERNAL_GRAPHICS SWSCI_FUNCTION_CODE(SWSCI_GBDA, 7) +#define SWSCI_GBDA_SPREAD_SPECTRUM SWSCI_FUNCTION_CODE(SWSCI_GBDA, 10) + +/* SWSCI: System BIOS Callbacks (SBCB) */ +#define SWSCI_SBCB 6 +#define SWSCI_SBCB_SUPPORTED_CALLBACKS SWSCI_FUNCTION_CODE(SWSCI_SBCB, 0) +#define SWSCI_SBCB_INIT_COMPLETION SWSCI_FUNCTION_CODE(SWSCI_SBCB, 1) +#define SWSCI_SBCB_PRE_HIRES_SET_MODE SWSCI_FUNCTION_CODE(SWSCI_SBCB, 3) +#define SWSCI_SBCB_POST_HIRES_SET_MODE SWSCI_FUNCTION_CODE(SWSCI_SBCB, 4) +#define SWSCI_SBCB_DISPLAY_SWITCH SWSCI_FUNCTION_CODE(SWSCI_SBCB, 5) +#define SWSCI_SBCB_SET_TV_FORMAT SWSCI_FUNCTION_CODE(SWSCI_SBCB, 6) +#define SWSCI_SBCB_ADAPTER_POWER_STATE SWSCI_FUNCTION_CODE(SWSCI_SBCB, 7) +#define SWSCI_SBCB_DISPLAY_POWER_STATE SWSCI_FUNCTION_CODE(SWSCI_SBCB, 8) +#define SWSCI_SBCB_SET_BOOT_DISPLAYSWSCI_FUNCTION_CODE(SWSCI_SBCB, 9) +#define SWSCI_SBCB_SET_PANEL_DETAILS SWSCI_FUNCTION_CODE(SWSCI_SBCB, 10) +#define SWSCI_SBCB_SET_INTERNAL_GFXSWSCI_FUNCTION_CODE(SWSCI_SBCB, 11) +#define SWSCI_SBCB_POST_HIRES_TO_DOS_FSSWSCI_FUNCTION_CODE(SWSCI_SBCB, 16) +#define SWSCI_SBCB_SUSPEND_RESUME SWSCI_FUNCTION_CODE(SWSCI_SBCB, 17) +#define SWSCI_SBCB_SET_SPREAD_SPECTRUM SWSCI_FUNCTION_CODE(SWSCI_SBCB, 18) +#define SWSCI_SBCB_POST_VBE_PM SWSCI_FUNCTION_CODE(SWSCI_SBCB, 19) +#define SWSCI_SBCB_ENABLE_DISABLE_AUDIOSWSCI_FUNCTION_CODE(SWSCI_SBCB, 21) + #define ACPI_OTHER_OUTPUT (08) #define ACPI_VGA_OUTPUT (18) #define ACPI_TV_OUTPUT (28) @@ -158,6 +206,91 @@ struct opregion_asle { #define ACPI_LVDS_OUTPUT (48) #ifdef CONFIG_ACPI +static int swsci(struct drm_device *dev, u32 function, u32 parm, u32 *parm_out) +{ +
[Intel-gfx] [PATCH 3/6] drm/i915: add opregion function to notify bios of encoder enable/disable
The bios interface seems messy, and it's hard to tell what the bios really wants. At first, only add support for DDI based machines (hsw+), and see how it turns out. The spec says to notify prior to power down and after power up. It is unclear whether it makes a difference. v2: - squash notification function and callers patches together (Daniel) - move callers to haswell_crtc_{enable,disable} (Daniel) - rename notification function (Chris) v3: - separate notification function and callers again, as it's not clear whether the display power state notification is the right thing to do after all v4: per Paulo's review: - drop LVDS - WARN on unsupported encoder types Signed-off-by: Jani Nikula jani.nik...@intel.com Reviewed-by: Paulo Zanoni paulo.r.zan...@intel.com --- drivers/gpu/drm/i915/i915_drv.h |8 ++ drivers/gpu/drm/i915/intel_opregion.c | 51 + 2 files changed, 59 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 67673e2..5339297 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2182,15 +2182,23 @@ static inline bool intel_gmbus_is_forced_bit(struct i2c_adapter *adapter) extern void intel_i2c_reset(struct drm_device *dev); /* intel_opregion.c */ +struct intel_encoder; extern int intel_opregion_setup(struct drm_device *dev); #ifdef CONFIG_ACPI extern void intel_opregion_init(struct drm_device *dev); extern void intel_opregion_fini(struct drm_device *dev); extern void intel_opregion_asle_intr(struct drm_device *dev); +extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, +bool enable); #else static inline void intel_opregion_init(struct drm_device *dev) { return; } static inline void intel_opregion_fini(struct drm_device *dev) { return; } static inline void intel_opregion_asle_intr(struct drm_device *dev) { return; } +static inline int +intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, bool enable) +{ + return 0; +} #endif /* intel_acpi.c */ diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c index c6a8d61..791991b 100644 --- a/drivers/gpu/drm/i915/intel_opregion.c +++ b/drivers/gpu/drm/i915/intel_opregion.c @@ -291,6 +291,57 @@ static int swsci(struct drm_device *dev, u32 function, u32 parm, u32 *parm_out) #undef C } +#define DISPLAY_TYPE_CRT 0 +#define DISPLAY_TYPE_TV1 +#define DISPLAY_TYPE_EXTERNAL_FLAT_PANEL 2 +#define DISPLAY_TYPE_INTERNAL_FLAT_PANEL 3 + +int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, + bool enable) +{ + struct drm_device *dev = intel_encoder-base.dev; + u32 parm = 0; + u32 type = 0; + u32 port; + + /* don't care about old stuff for now */ + if (!HAS_DDI(dev)) + return 0; + + port = intel_ddi_get_encoder_port(intel_encoder); + if (port == PORT_E) { + port = 0; + } else { + parm |= 1 port; + port++; + } + + if (!enable) + parm |= 4 8; + + switch (intel_encoder-type) { + case INTEL_OUTPUT_ANALOG: + type = DISPLAY_TYPE_CRT; + break; + case INTEL_OUTPUT_UNKNOWN: + case INTEL_OUTPUT_DISPLAYPORT: + case INTEL_OUTPUT_HDMI: + type = DISPLAY_TYPE_EXTERNAL_FLAT_PANEL; + break; + case INTEL_OUTPUT_EDP: + type = DISPLAY_TYPE_INTERNAL_FLAT_PANEL; + break; + default: + WARN_ONCE(1, unsupported intel_encoder type %d\n, + intel_encoder-type); + return -EINVAL; + } + + parm |= type (16 + port * 3); + + return swsci(dev, SWSCI_SBCB_DISPLAY_POWER_STATE, parm, NULL); +} + static u32 asle_set_backlight(struct drm_device *dev, u32 bclp) { struct drm_i915_private *dev_priv = dev-dev_private; -- 1.7.10.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/6] drm/i915: expose intel_ddi_get_encoder_port()
In preparation for followup work. Signed-off-by: Jani Nikula jani.nik...@intel.com Reviewed-by: Paulo Zanoni paulo.r.zan...@intel.com --- drivers/gpu/drm/i915/intel_ddi.c |2 +- drivers/gpu/drm/i915/intel_drv.h |1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 63aca49..060ea50 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -58,7 +58,7 @@ static const u32 hsw_ddi_translations_fdi[] = { 0x00FF, 0x00040006 /* HDMI parameters */ }; -static enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder) +enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder) { struct drm_encoder *encoder = intel_encoder-base; int type = intel_encoder-type; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index a38f5b2..5efb844 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -712,6 +712,7 @@ extern void intel_write_eld(struct drm_encoder *encoder, extern void intel_prepare_ddi(struct drm_device *dev); extern void hsw_fdi_link_train(struct drm_crtc *crtc); extern void intel_ddi_init(struct drm_device *dev, enum port port); +extern enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder); /* For use by IVB LP watermark workaround in intel_sprite.c */ extern void intel_update_watermarks(struct drm_device *dev); -- 1.7.10.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: enable trickle feed on Haswell
On Fri, Aug 23, 2013 at 07:51:28PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com We shouldn't disable the trickle feed bits on Haswell. Our documentation explicitly says the trickle feed bits of PRI_CTL and CUR_CTL should not be programmed to 1, and the hardware engineer also asked us to not program the SPR_CTL field to 1. Leaving the bits as 1 could cause underflows. Reported-by: Arthur Runyan arthur.j.run...@intel.com Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_display.c | 10 +++--- drivers/gpu/drm/i915/intel_pm.c | 2 -- drivers/gpu/drm/i915/intel_sprite.c | 7 +-- 4 files changed, 13 insertions(+), 7 deletions(-) Some side discussions: 1 - It seems we always do read-modify-write with the PRI/SPR/CUR registers. I think this is dangerous since we may forget to disable something and keep our code in a bugged state forever. Shouldn't we try to completely set the full state of the bits from scratch every time we enable PRI/SPR/CUR? Yeah I dislike the RMW. In my atomic branch I think I got rid of it, if for no other reason than less overhead. I think the reason for the RMW was that the whole plane handling has been spread around all over the place. We should just centralize it nicely as part of the all planes are drm_planes change. The base address RMW is an open question though since it was explicitly made that way. But on HSW that's supposedly bugged and can revert the based address update from a command streamer flip. I haven't actually tried which way it's bugged: does it immediately revert and cause the display to scan out garbage until the new value is latches on the next vblank, or does the revert also need a vblank to latch. The latter option is the less dangerous since we'd need to get a vblank between the read and write to see garbage. I should write a test now that there's a hsw machine on my desk... 2 - We also have code to enable the trickle feed bits at init_clock_gating. Isn't this dangerous? Trickle fee changes the memory is read, my first guess would be that it's probably not safe to change this bit when things are enabled. Also, we lose the state of these bits when the power well is disabled, so the code in init_clock_gating really makes no sense on Haswell. I don't recall seeing any garbage when toggling trickle feed on the fly. I assume that if the watermarks are sufficiently high toggling trickle feed should be safe, well apparently before HSW that is :) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index bbbc177..e92bb47 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -3312,6 +3312,7 @@ #define MCURSOR_PIPE_A 0x00 #define MCURSOR_PIPE_B (1 28) #define MCURSOR_GAMMA_ENABLE (1 26) +#define CURSOR_TRICKLE_FEED_DISABLE(1 14) #define _CURABASE(dev_priv-info-display_mmio_offset + 0x70084) #define _CURAPOS (dev_priv-info-display_mmio_offset + 0x70088) #define CURSOR_POS_MASK 0x007FF diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index c64631d..d5f038e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2077,8 +2077,10 @@ static int ironlake_update_plane(struct drm_crtc *crtc, else dspcntr = ~DISPPLANE_TILED; - /* must disable */ - dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE; + if (IS_HASWELL(dev)) + dspcntr = ~DISPPLANE_TRICKLE_FEED_DISABLE; + else + dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE; I915_WRITE(reg, dspcntr); @@ -6768,8 +6770,10 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base) cntl = ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE); cntl |= CURSOR_MODE_DISABLE; } - if (IS_HASWELL(dev)) + if (IS_HASWELL(dev)) { cntl |= CURSOR_PIPE_CSC_ENABLE; + cntl = ~CURSOR_TRICKLE_FEED_DISABLE; + } I915_WRITE(CURCNTR_IVB(pipe), cntl); intel_crtc-cursor_visible = visible; diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 4605682..3a5c0bb 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4950,8 +4950,6 @@ static void haswell_init_clock_gating(struct drm_device *dev) I915_READ(GEN7_SQ_CHICKEN_MBCUNIT_CONFIG) | GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB); - g4x_disable_trickle_feed(dev); - /* WaVSRefCountFullforceMissDisable:hsw */ gen7_setup_fixed_func_scheduler(dev_priv); diff --git a/drivers/gpu/drm/i915/intel_sprite.c
Re: [Intel-gfx] [PATCH 2/6] drm/i915: add plumbing for SWSCI
2013/8/30 Jani Nikula jani.nik...@intel.com: SWSCI is a driver to bios call interface. This checks for SWSCI availability and bios requested callbacks, and filters out any calls that shouldn't happen. This way the callers don't need to do the checks all over the place. v2: silence some checkpatch nagging v3: set PCI_SWSCI bit 0 to trigger interrupt (Mengdong Lin) v4: remove an extra #define (Jesse) v5: spec says s/w is responsible for clearing PCI_SWSCI bit 0 too v6: per Paulo's review and more: - fix sub-function mask - add exit parameter - add define for set panel details call - return more errors from swsci - clean up the supported/requested callbacks bit masks mess - use DSLP for timeout - fix build for CONFIG_ACPI=n Signed-off-by: Jani Nikula jani.nik...@intel.com --- drivers/gpu/drm/i915/i915_drv.h |2 + drivers/gpu/drm/i915/intel_opregion.c | 198 - 2 files changed, 197 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 0602c4b..67673e2 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -225,6 +225,8 @@ struct intel_opregion { struct opregion_header __iomem *header; struct opregion_acpi __iomem *acpi; struct opregion_swsci __iomem *swsci; + u32 swsci_gbda_sub_functions; + u32 swsci_sbcb_sub_functions; struct opregion_asle __iomem *asle; void __iomem *vbt; u32 __iomem *lid_state; diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c index cfb8fb6..c6a8d61 100644 --- a/drivers/gpu/drm/i915/intel_opregion.c +++ b/drivers/gpu/drm/i915/intel_opregion.c @@ -36,8 +36,11 @@ #include i915_drv.h #include intel_drv.h -#define PCI_ASLE 0xe4 -#define PCI_ASLS 0xfc +#define PCI_ASLE 0xe4 +#define PCI_ASLS 0xfc +#define PCI_SWSCI 0xe8 +#define PCI_SWSCI_SCISEL (1 15) +#define PCI_SWSCI_GSSCIE (1 0) #define OPREGION_HEADER_OFFSET 0 #define OPREGION_ACPI_OFFSET 0x100 @@ -151,6 +154,51 @@ struct opregion_asle { #define ASLE_CBLV_VALID (131) +/* Software System Control Interrupt (SWSCI) */ +#define SWSCI_SCIC_INDICATOR (1 0) +#define SWSCI_SCIC_MAIN_FUNCTION_SHIFT 1 +#define SWSCI_SCIC_MAIN_FUNCTION_MASK (0xf 1) +#define SWSCI_SCIC_SUB_FUNCTION_SHIFT 8 +#define SWSCI_SCIC_SUB_FUNCTION_MASK (0xff 8) +#define SWSCI_SCIC_EXIT_PARAMETER_SHIFT8 +#define SWSCI_SCIC_EXIT_PARAMETER_MASK (0xff 8) +#define SWSCI_SCIC_EXIT_STATUS_SHIFT 5 +#define SWSCI_SCIC_EXIT_STATUS_MASK(7 5) +#define SWSCI_SCIC_EXIT_STATUS_SUCCESS 1 + +#define SWSCI_FUNCTION_CODE(main, sub) \ + ((main) SWSCI_SCIC_MAIN_FUNCTION_SHIFT | \ +(sub) SWSCI_SCIC_SUB_FUNCTION_SHIFT) + +/* SWSCI: Get BIOS Data (GBDA) */ +#define SWSCI_GBDA 4 +#define SWSCI_GBDA_SUPPORTED_CALLS SWSCI_FUNCTION_CODE(SWSCI_GBDA, 0) +#define SWSCI_GBDA_REQUESTED_CALLBACKS SWSCI_FUNCTION_CODE(SWSCI_GBDA, 1) +#define SWSCI_GBDA_BOOT_DISPLAY_PREF SWSCI_FUNCTION_CODE(SWSCI_GBDA, 4) +#define SWSCI_GBDA_PANEL_DETAILS SWSCI_FUNCTION_CODE(SWSCI_GBDA, 5) +#define SWSCI_GBDA_TV_STANDARD SWSCI_FUNCTION_CODE(SWSCI_GBDA, 6) +#define SWSCI_GBDA_INTERNAL_GRAPHICS SWSCI_FUNCTION_CODE(SWSCI_GBDA, 7) +#define SWSCI_GBDA_SPREAD_SPECTRUM SWSCI_FUNCTION_CODE(SWSCI_GBDA, 10) + +/* SWSCI: System BIOS Callbacks (SBCB) */ +#define SWSCI_SBCB 6 +#define SWSCI_SBCB_SUPPORTED_CALLBACKS SWSCI_FUNCTION_CODE(SWSCI_SBCB, 0) +#define SWSCI_SBCB_INIT_COMPLETION SWSCI_FUNCTION_CODE(SWSCI_SBCB, 1) +#define SWSCI_SBCB_PRE_HIRES_SET_MODE SWSCI_FUNCTION_CODE(SWSCI_SBCB, 3) +#define SWSCI_SBCB_POST_HIRES_SET_MODE SWSCI_FUNCTION_CODE(SWSCI_SBCB, 4) +#define SWSCI_SBCB_DISPLAY_SWITCH SWSCI_FUNCTION_CODE(SWSCI_SBCB, 5) +#define SWSCI_SBCB_SET_TV_FORMAT SWSCI_FUNCTION_CODE(SWSCI_SBCB, 6) +#define SWSCI_SBCB_ADAPTER_POWER_STATE SWSCI_FUNCTION_CODE(SWSCI_SBCB, 7) +#define SWSCI_SBCB_DISPLAY_POWER_STATE SWSCI_FUNCTION_CODE(SWSCI_SBCB, 8) +#define SWSCI_SBCB_SET_BOOT_DISPLAYSWSCI_FUNCTION_CODE(SWSCI_SBCB, 9) +#define SWSCI_SBCB_SET_PANEL_DETAILS SWSCI_FUNCTION_CODE(SWSCI_SBCB, 10) +#define SWSCI_SBCB_SET_INTERNAL_GFXSWSCI_FUNCTION_CODE(SWSCI_SBCB, 11) +#define SWSCI_SBCB_POST_HIRES_TO_DOS_FS SWSCI_FUNCTION_CODE(SWSCI_SBCB, 16) +#define SWSCI_SBCB_SUSPEND_RESUME SWSCI_FUNCTION_CODE(SWSCI_SBCB, 17) +#define SWSCI_SBCB_SET_SPREAD_SPECTRUM SWSCI_FUNCTION_CODE(SWSCI_SBCB, 18) +#define SWSCI_SBCB_POST_VBE_PM SWSCI_FUNCTION_CODE(SWSCI_SBCB, 19) +#define SWSCI_SBCB_ENABLE_DISABLE_AUDIO SWSCI_FUNCTION_CODE(SWSCI_SBCB, 21) + #define ACPI_OTHER_OUTPUT (08) #define ACPI_VGA_OUTPUT (18) #define ACPI_TV_OUTPUT (28) @@ -158,6 +206,91 @@ struct
Re: [Intel-gfx] [PATCH 6/6] DRAFT: drm/i915: do adapter power state notification on PC8+ enable/disable
2013/8/30 Jani Nikula jani.nik...@intel.com: v2: - go to PCI_D3cold - shuffle the call site a bit Ok, so I know I'm the one who requested to shuffle the call site, but it's because I thought that when we disable LCPLL we actually put the device in D3. After some experimentation last week, we discovered we need to write a PCI config register to actually enable D3, so your call to intel_opregion_notify_adapter should probably be glued to that write (which we don't have yet). The problem is that if we really put the device in D3, all the registers go away, so we need to properly recover from that. I think it makes sense to postpone this patch until we have the actual code to put our device in D3 and properly restore from it. So my suggestion is to merge patches 1-5 so we can work on the D3 feature on top of the work you already completed. Thanks, Paulo Signed-off-by: Jani Nikula jani.nik...@intel.com --- 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 83d853f..e33fa6d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6126,6 +6126,7 @@ void hsw_enable_pc8_work(struct work_struct *__work) lpt_disable_clkout_dp(dev); hsw_pc8_disable_interrupts(dev); + intel_opregion_notify_adapter(dev, PCI_D3cold); hsw_disable_lcpll(dev_priv, true, true); } @@ -6163,6 +6164,7 @@ static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv) DRM_DEBUG_KMS(Disabling package C8+\n); hsw_restore_lcpll(dev_priv); + intel_opregion_notify_adapter(dev, PCI_D0); hsw_pc8_restore_interrupts(dev); lpt_init_pch_refclk(dev); -- 1.7.10.4 -- Paulo Zanoni ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/19] drm/i915: Pass crtc to intel_update_watermarks()
2013/8/30 ville.syrj...@linux.intel.com: From: Ville Syrjälä ville.syrj...@linux.intel.com Passing the appropriate crtc to intel_update_watermarks() should help in avoiding needless work in the future. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com I like the fact that now we're passing the CRTC, but bikeshed my only worry is that now some functions overwrite the crtc we pass as argument, so this might be confusing and maybe lead to bugs in the future: perhaps the argument could be called unused_crtc or ignored_crtc, then we'd keep the crtc variables they already have /bikeshed. But it's just a bikeshed, so with or without changes: Reviewed-by: Paulo Zanoni paulo.r.zan...@intel.com. --- drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/intel_display.c | 20 - drivers/gpu/drm/i915/intel_drv.h | 2 +- drivers/gpu/drm/i915/intel_pm.c | 43 +--- drivers/gpu/drm/i915/intel_sprite.c | 6 ++--- 5 files changed, 40 insertions(+), 33 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 7594356..ab46eb3 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -357,7 +357,7 @@ struct drm_i915_display_funcs { int target, int refclk, struct dpll *match_clock, struct dpll *best_clock); - void (*update_wm)(struct drm_device *dev); + void (*update_wm)(struct drm_crtc *crtc); void (*update_sprite_wm)(struct drm_plane *plane, struct drm_crtc *crtc, uint32_t sprite_width, int pixel_size, diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f526ea9..13856ec 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3278,7 +3278,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) intel_set_cpu_fifo_underrun_reporting(dev, pipe, true); intel_set_pch_fifo_underrun_reporting(dev, pipe, true); - intel_update_watermarks(dev); + intel_update_watermarks(crtc); for_each_encoder_on_crtc(dev, crtc, encoder) if (encoder-pre_enable) @@ -3388,7 +3388,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc) if (intel_crtc-config.has_pch_encoder) intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_A, true); - intel_update_watermarks(dev); + intel_update_watermarks(crtc); if (intel_crtc-config.has_pch_encoder) dev_priv-display.fdi_link_train(crtc); @@ -3520,7 +3520,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) } intel_crtc-active = false; - intel_update_watermarks(dev); + intel_update_watermarks(crtc); mutex_lock(dev-struct_mutex); intel_update_fbc(dev); @@ -3577,7 +3577,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) } intel_crtc-active = false; - intel_update_watermarks(dev); + intel_update_watermarks(crtc); mutex_lock(dev-struct_mutex); intel_update_fbc(dev); @@ -3677,7 +3677,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc) return; intel_crtc-active = true; - intel_update_watermarks(dev); + intel_update_watermarks(crtc); for_each_encoder_on_crtc(dev, crtc, encoder) if (encoder-pre_pll_enable) @@ -3722,7 +3722,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) return; intel_crtc-active = true; - intel_update_watermarks(dev); + intel_update_watermarks(crtc); for_each_encoder_on_crtc(dev, crtc, encoder) if (encoder-pre_enable) @@ -3806,7 +3806,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) intel_crtc-active = false; intel_update_fbc(dev); - intel_update_watermarks(dev); + intel_update_watermarks(crtc); } static void i9xx_crtc_off(struct drm_crtc *crtc) @@ -4972,7 +4972,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, ret = intel_pipe_set_base(crtc, x, y, fb); - intel_update_watermarks(dev); + intel_update_watermarks(crtc); return ret; } @@ -5860,7 +5860,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, ret = intel_pipe_set_base(crtc, x, y, fb); - intel_update_watermarks(dev); + intel_update_watermarks(crtc); return ret; } @@ -6316,7 +6316,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc, ret = intel_pipe_set_base(crtc, x, y, fb); - intel_update_watermarks(dev); + intel_update_watermarks(crtc); return ret; } diff --git a/drivers/gpu/drm/i915/intel_drv.h
Re: [Intel-gfx] [PATCH 4/6] drm/i915: add opregion function to notify bios of adapter power state
2013/8/30 Jani Nikula jani.nik...@intel.com: Notifying the bios lets it enter power saving states. Signed-off-by: Jani Nikula jani.nik...@intel.com Reviewed-by: Paulo Zanoni paulo.r.zan...@intel.com --- drivers/gpu/drm/i915/i915_drv.h |7 +++ drivers/gpu/drm/i915/intel_opregion.c | 27 +++ 2 files changed, 34 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 5339297..7daae2a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2190,6 +2190,8 @@ extern void intel_opregion_fini(struct drm_device *dev); extern void intel_opregion_asle_intr(struct drm_device *dev); extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, bool enable); +extern int intel_opregion_notify_adapter(struct drm_device *dev, +pci_power_t state); #else static inline void intel_opregion_init(struct drm_device *dev) { return; } static inline void intel_opregion_fini(struct drm_device *dev) { return; } @@ -2199,6 +2201,11 @@ intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, bool enable) { return 0; } +static inline int +intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state) +{ + return 0; +} #endif /* intel_acpi.c */ diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c index 791991b..05243df 100644 --- a/drivers/gpu/drm/i915/intel_opregion.c +++ b/drivers/gpu/drm/i915/intel_opregion.c @@ -342,6 +342,33 @@ int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder, return swsci(dev, SWSCI_SBCB_DISPLAY_POWER_STATE, parm, NULL); } +static const struct { + pci_power_t pci_power_state; + u32 parm; +} power_state_map[] = { + { PCI_D0, 0x00 }, + { PCI_D1, 0x01 }, + { PCI_D2, 0x02 }, + { PCI_D3hot,0x04 }, + { PCI_D3cold, 0x04 }, +}; + +int intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state) +{ + int i; + + if (!HAS_DDI(dev)) + return 0; + + for (i = 0; i ARRAY_SIZE(power_state_map); i++) { + if (state == power_state_map[i].pci_power_state) + return swsci(dev, SWSCI_SBCB_ADAPTER_POWER_STATE, +power_state_map[i].parm, NULL); + } + + return -EINVAL; +} + static u32 asle_set_backlight(struct drm_device *dev, u32 bclp) { struct drm_i915_private *dev_priv = dev-dev_private; -- 1.7.10.4 -- Paulo Zanoni ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/19] drm/i915: Call intel_update_watermarks() in specific place during modeset
2013/8/30 ville.syrj...@linux.intel.com: From: Ville Syrjälä ville.syrj...@linux.intel.com Make the call to intel_update_watermarks() just once or twice during modeset. Ideally it should happen independently when each plane gets enabled/disabled, but for now it seems better to keep it in central place. We can improve things when we get all the planes sorted out in a better way. When enabling set up the watermarks just before the pipe is enabled. And when disabling we need to wait until we've marked the crtc as inactive. Why do we need to wait until we've marked the CRTC as inactive? (Daniel/Ville should put the answer in the commit message) The patch looks correct, but bikeshed I'd separate the don't update watermarks at crtc-mode_set part from the current patch, since if we start getting regressions we won't know if they're caused by the fact that now we don't adjust the watermarks before enabling the PCH and doing link training, or if they're caused by the fact that we don't update them anymore at mode_set /bikeshed. But since it looks correct, we shouldn't really be getting regressions... With or without changes: Reviewed-by: Paulo Zanoni paulo.r.zan...@intel.com Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/intel_display.c | 19 ++- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 13856ec..a5181fe 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3278,8 +3278,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) intel_set_cpu_fifo_underrun_reporting(dev, pipe, true); intel_set_pch_fifo_underrun_reporting(dev, pipe, true); - intel_update_watermarks(crtc); - for_each_encoder_on_crtc(dev, crtc, encoder) if (encoder-pre_enable) encoder-pre_enable(encoder); @@ -3302,6 +3300,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) */ intel_crtc_load_lut(crtc); + intel_update_watermarks(crtc); intel_enable_pipe(dev_priv, pipe, intel_crtc-config.has_pch_encoder, false); intel_enable_plane(dev_priv, plane, pipe); @@ -3388,8 +3387,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc) if (intel_crtc-config.has_pch_encoder) intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_A, true); - intel_update_watermarks(crtc); - if (intel_crtc-config.has_pch_encoder) dev_priv-display.fdi_link_train(crtc); @@ -3410,6 +3407,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc) intel_ddi_set_pipe_settings(crtc); intel_ddi_enable_transcoder_func(crtc); + intel_update_watermarks(crtc); intel_enable_pipe(dev_priv, pipe, intel_crtc-config.has_pch_encoder, false); intel_enable_plane(dev_priv, plane, pipe); @@ -3677,7 +3675,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc) return; intel_crtc-active = true; - intel_update_watermarks(crtc); for_each_encoder_on_crtc(dev, crtc, encoder) if (encoder-pre_pll_enable) @@ -3696,6 +3693,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc) intel_crtc_load_lut(crtc); + intel_update_watermarks(crtc); intel_enable_pipe(dev_priv, pipe, false, is_dsi); intel_enable_plane(dev_priv, plane, pipe); intel_enable_planes(crtc); @@ -3722,7 +3720,6 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) return; intel_crtc-active = true; - intel_update_watermarks(crtc); for_each_encoder_on_crtc(dev, crtc, encoder) if (encoder-pre_enable) @@ -3734,6 +3731,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) intel_crtc_load_lut(crtc); + intel_update_watermarks(crtc); intel_enable_pipe(dev_priv, pipe, false, false); intel_enable_plane(dev_priv, plane, pipe); intel_enable_planes(crtc); @@ -3805,8 +3803,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) i9xx_disable_pll(dev_priv, pipe); intel_crtc-active = false; - intel_update_fbc(dev); intel_update_watermarks(crtc); + + intel_update_fbc(dev); } static void i9xx_crtc_off(struct drm_crtc *crtc) @@ -4972,8 +4971,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, ret = intel_pipe_set_base(crtc, x, y, fb); - intel_update_watermarks(crtc); - return ret; } @@ -5860,8 +5857,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, ret = intel_pipe_set_base(crtc, x, y, fb); - intel_update_watermarks(crtc); - return ret; } @@ -6316,8 +6311,6 @@ static int
Re: [Intel-gfx] [PATCH 5/6] drm/i915: do display power state notification on crtc enable/disable
2013/8/30 Jani Nikula jani.nik...@intel.com: The spec says to notify prior to power down and after power up. It is unclear whether it makes a difference. Signed-off-by: Jani Nikula jani.nik...@intel.com Reviewed-by: Paulo Zanoni paulo.r.zan...@intel.com --- Paulo, still okay with the r-b? Yes :) --- drivers/gpu/drm/i915/intel_display.c |8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9222e570..83d853f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3425,8 +3425,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc) intel_update_fbc(dev); mutex_unlock(dev-struct_mutex); - for_each_encoder_on_crtc(dev, crtc, encoder) + for_each_encoder_on_crtc(dev, crtc, encoder) { encoder-enable(encoder); + intel_opregion_notify_encoder(encoder, true); + } /* * There seems to be a race in PCH platform hw (at least on some @@ -3540,8 +3542,10 @@ static void haswell_crtc_disable(struct drm_crtc *crtc) if (!intel_crtc-active) return; - for_each_encoder_on_crtc(dev, crtc, encoder) + for_each_encoder_on_crtc(dev, crtc, encoder) { + intel_opregion_notify_encoder(encoder, false); encoder-disable(encoder); + } intel_crtc_wait_for_pending_flips(crtc); drm_vblank_off(dev, pipe); -- 1.7.10.4 -- Paulo Zanoni ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/19] drm/i915: Pass crtc to intel_update_watermarks()
On Fri, Aug 30, 2013 at 05:09:49PM -0300, Paulo Zanoni wrote: 2013/8/30 ville.syrj...@linux.intel.com: From: Ville Syrjälä ville.syrj...@linux.intel.com Passing the appropriate crtc to intel_update_watermarks() should help in avoiding needless work in the future. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com I like the fact that now we're passing the CRTC, but bikeshed my only worry is that now some functions overwrite the crtc we pass as argument, so this might be confusing and maybe lead to bugs in the future: perhaps the argument could be called unused_crtc or ignored_crtc, then we'd keep the crtc variables they already have /bikeshed. But it's just a bikeshed, so with or without changes: Reviewed-by: Paulo Zanoni paulo.r.zan...@intel.com. Right. It could be a bit confusing. I'll change it. -- 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 03/19] drm/i915: Constify some watermark data
2013/8/30 ville.syrj...@linux.intel.com: From: Ville Syrjälä ville.syrj...@linux.intel.com hsw_pipe_wm_parameters and hsw_wm_maximums typically are read only. Make them const. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com Reviewed-by: GCC 4.7.3 Reviewed-by: Paulo Zanoni paulo.r.zan...@intel.com --- drivers/gpu/drm/i915/intel_pm.c | 20 ++-- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 774c57f..96493dc 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2202,7 +2202,7 @@ struct intel_wm_config { * For both WM_PIPE and WM_LP. * mem_value must be in 0.1us units. */ -static uint32_t ilk_compute_pri_wm(struct hsw_pipe_wm_parameters *params, +static uint32_t ilk_compute_pri_wm(const struct hsw_pipe_wm_parameters *params, uint32_t mem_value, bool is_lp) { @@ -2231,7 +2231,7 @@ static uint32_t ilk_compute_pri_wm(struct hsw_pipe_wm_parameters *params, * For both WM_PIPE and WM_LP. * mem_value must be in 0.1us units. */ -static uint32_t ilk_compute_spr_wm(struct hsw_pipe_wm_parameters *params, +static uint32_t ilk_compute_spr_wm(const struct hsw_pipe_wm_parameters *params, uint32_t mem_value) { uint32_t method1, method2; @@ -2254,7 +2254,7 @@ static uint32_t ilk_compute_spr_wm(struct hsw_pipe_wm_parameters *params, * For both WM_PIPE and WM_LP. * mem_value must be in 0.1us units. */ -static uint32_t ilk_compute_cur_wm(struct hsw_pipe_wm_parameters *params, +static uint32_t ilk_compute_cur_wm(const struct hsw_pipe_wm_parameters *params, uint32_t mem_value) { if (!params-active || !params-cur.enabled) @@ -2268,7 +2268,7 @@ static uint32_t ilk_compute_cur_wm(struct hsw_pipe_wm_parameters *params, } /* Only for WM_LP. */ -static uint32_t ilk_compute_fbc_wm(struct hsw_pipe_wm_parameters *params, +static uint32_t ilk_compute_fbc_wm(const struct hsw_pipe_wm_parameters *params, uint32_t pri_val) { if (!params-active || !params-pri.enabled) @@ -2419,7 +2419,7 @@ static bool ilk_check_wm(int level, static void ilk_compute_wm_level(struct drm_i915_private *dev_priv, int level, -struct hsw_pipe_wm_parameters *p, +const struct hsw_pipe_wm_parameters *p, struct intel_wm_level *result) { uint16_t pri_latency = dev_priv-wm.pri_latency[level]; @@ -2441,8 +2441,8 @@ static void ilk_compute_wm_level(struct drm_i915_private *dev_priv, } static bool hsw_compute_lp_wm(struct drm_i915_private *dev_priv, - int level, struct hsw_wm_maximums *max, - struct hsw_pipe_wm_parameters *params, + int level, const struct hsw_wm_maximums *max, + const struct hsw_pipe_wm_parameters *params, struct intel_wm_level *result) { enum pipe pipe; @@ -2462,7 +2462,7 @@ static bool hsw_compute_lp_wm(struct drm_i915_private *dev_priv, static uint32_t hsw_compute_wm_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, - struct hsw_pipe_wm_parameters *params) + const struct hsw_pipe_wm_parameters *params) { uint32_t pri_val, cur_val, spr_val; /* WM0 latency values stored in 0.1us units */ @@ -2670,8 +2670,8 @@ static void hsw_compute_wm_parameters(struct drm_device *dev, } static void hsw_compute_wm_results(struct drm_device *dev, - struct hsw_pipe_wm_parameters *params, - struct hsw_wm_maximums *lp_maximums, + const struct hsw_pipe_wm_parameters *params, + const struct hsw_wm_maximums *lp_maximums, struct hsw_wm_values *results) { struct drm_i915_private *dev_priv = dev-dev_private; -- 1.8.1.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/19] drm/i915: Call intel_update_watermarks() in specific place during modeset
On Fri, Aug 30, 2013 at 05:26:29PM -0300, Paulo Zanoni wrote: 2013/8/30 ville.syrj...@linux.intel.com: From: Ville Syrjälä ville.syrj...@linux.intel.com Make the call to intel_update_watermarks() just once or twice during modeset. Ideally it should happen independently when each plane gets enabled/disabled, but for now it seems better to keep it in central place. We can improve things when we get all the planes sorted out in a better way. When enabling set up the watermarks just before the pipe is enabled. And when disabling we need to wait until we've marked the crtc as inactive. Why do we need to wait until we've marked the CRTC as inactive? (Daniel/Ville should put the answer in the commit message) Because the watermark compute code looks at intel_crtc-active. If we compute the watermarks before, the code thinks the pipe is active. Hmm. BTW now that I look at intel_crtc_active() I start to wonder why it looks at the clock in the user specified mode. In fact most (maybe all?) of the pre-hsw watermark code is fscked up and it looks at the wrong mode. Sigh. Suppose I need to make a quick for all that as well... -- 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 04/19] drm/i915: Use ilk_compute_wm_level to compute WM_PIPE values
2013/8/30 ville.syrj...@linux.intel.com: From: Ville Syrjälä ville.syrj...@linux.intel.com Unify the code a bit to use ilk_compute_wm_level for all watermark levels. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com Looks correct and doesn't change the WM values set on my specific eDP+HDMI config here... Reviewed-by: Paulo Zanoni paulo.r.zan...@intel.com --- drivers/gpu/drm/i915/intel_pm.c | 44 - 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 96493dc..d118ee1 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2460,33 +2460,31 @@ static bool hsw_compute_lp_wm(struct drm_i915_private *dev_priv, return ilk_check_wm(level, max, result); } -static uint32_t hsw_compute_wm_pipe(struct drm_i915_private *dev_priv, - enum pipe pipe, + +static uint32_t hsw_compute_wm_pipe(struct drm_device *dev, const struct hsw_pipe_wm_parameters *params) { - uint32_t pri_val, cur_val, spr_val; - /* WM0 latency values stored in 0.1us units */ - uint16_t pri_latency = dev_priv-wm.pri_latency[0]; - uint16_t spr_latency = dev_priv-wm.spr_latency[0]; - uint16_t cur_latency = dev_priv-wm.cur_latency[0]; + struct drm_i915_private *dev_priv = dev-dev_private; + struct intel_wm_config config = { + .num_pipes_active = 1, + .sprites_enabled = params-spr.enabled, + .sprites_scaled = params-spr.scaled, + }; + struct hsw_wm_maximums max; + struct intel_wm_level res; + + if (!params-active) + return 0; + + ilk_wm_max(dev, 0, config, INTEL_DDB_PART_1_2, max); - pri_val = ilk_compute_pri_wm(params, pri_latency, false); - spr_val = ilk_compute_spr_wm(params, spr_latency); - cur_val = ilk_compute_cur_wm(params, cur_latency); + ilk_compute_wm_level(dev_priv, 0, params, res); - WARN(pri_val 127, -Primary WM error, mode not supported for pipe %c\n, -pipe_name(pipe)); - WARN(spr_val 127, -Sprite WM error, mode not supported for pipe %c\n, -pipe_name(pipe)); - WARN(cur_val 63, -Cursor WM error, mode not supported for pipe %c\n, -pipe_name(pipe)); + ilk_check_wm(0, max, res); - return (pri_val WM0_PIPE_PLANE_SHIFT) | - (spr_val WM0_PIPE_SPRITE_SHIFT) | - cur_val; + return (res.pri_val WM0_PIPE_PLANE_SHIFT) | + (res.spr_val WM0_PIPE_SPRITE_SHIFT) | + res.cur_val; } static uint32_t @@ -2715,7 +2713,7 @@ static void hsw_compute_wm_results(struct drm_device *dev, } for_each_pipe(pipe) - results-wm_pipe[pipe] = hsw_compute_wm_pipe(dev_priv, pipe, + results-wm_pipe[pipe] = hsw_compute_wm_pipe(dev, params[pipe]); for_each_pipe(pipe) { -- 1.8.1.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 05/19] drm/i915: Refactor max WM level
2013/8/30 ville.syrj...@linux.intel.com: From: Ville Syrjälä ville.syrj...@linux.intel.com Pull the expected max WM level determinations out to a separate function. Will have another user soon. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com Reviewed-by: Paulo Zanoni paulo.r.zan...@intel.com --- drivers/gpu/drm/i915/intel_pm.c | 19 +++ 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index d118ee1..163ba74 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2558,19 +2558,22 @@ static void intel_fixup_cur_wm_latency(struct drm_device *dev, uint16_t wm[5]) wm[3] *= 2; } -static void intel_print_wm_latency(struct drm_device *dev, - const char *name, - const uint16_t wm[5]) +static int ilk_wm_max_level(const struct drm_device *dev) { - int level, max_level; - /* how many WM levels are we expecting */ if (IS_HASWELL(dev)) - max_level = 4; + return 4; else if (INTEL_INFO(dev)-gen = 6) - max_level = 3; + return 3; else - max_level = 2; + return 2; +} + +static void intel_print_wm_latency(struct drm_device *dev, + const char *name, + const uint16_t wm[5]) +{ + int level, max_level = ilk_wm_max_level(dev); for (level = 0; level = max_level; level++) { unsigned int latency = wm[level]; -- 1.8.1.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: It's its!
On Fri, Aug 30, 2013 at 02:40:26PM +0100, Damien Lespiau wrote: Signed-off-by: Damien Lespiau damien.lesp...@intel.com --- drivers/gpu/drm/i915/i915_gem_context.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 403309c..e038513 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -73,7 +73,7 @@ * * There are two confusing terms used above: * The current context means the context which is currently running on the - * GPU. The GPU has loaded it's state already and has stored away the gtt + * GPU. The GPU has loaded its state already and has stored away the gtt * offset of the BO. The GPU is not actively referencing the data at this * offset, but it will on the next context switch. The only way to avoid this * is to do a GPU reset. Acked-by: Ben Widawsky b...@bwidawsk.net By the way, I plan to rewrite a lot of this once we land full PPGTT. -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/8] [RESEND] VMA patches
Now that [hopefully all] of the fallout from the last round of VMA merging has been taken care of - Resending the remaining VMA patches which are required for the full PPGTT support. Some of these patches are new as a result of the last round of review. For the ones which are just a RESEND, I've only rebased and un on an IVB. Ben Widawsky (8): drm/i915: Synchronize pread/pwrite with wait_rendering drm/i915: Extract vm specific part of eviction drm/i915: evict VM instead of everything drm/1915: trace vm eviction instead of everything drm/i915: Convert active API to VMA drm/i915: Add bind/unbind object functions to VM drm/i915: Use the new vm [un]bind functions drm/i915: eliminate vm-insert_entries() drivers/gpu/drm/i915/i915_drv.h| 84 ++--- drivers/gpu/drm/i915/i915_gem.c| 61 +++ drivers/gpu/drm/i915/i915_gem_context.c| 16 ++-- drivers/gpu/drm/i915/i915_gem_evict.c | 47 ++-- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 40 +- drivers/gpu/drm/i915/i915_gem_gtt.c| 116 - drivers/gpu/drm/i915/i915_trace.h | 12 +-- 7 files changed, 228 insertions(+), 148 deletions(-) -- 1.8.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/8] drm/i915: Extract vm specific part of eviction
As we'll see in the next patch, being able to evict for just 1 VM is handy. Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem_evict.c | 28 ++-- 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c index cc8974f..e9033f0 100644 --- a/drivers/gpu/drm/i915/i915_gem_evict.c +++ b/drivers/gpu/drm/i915/i915_gem_evict.c @@ -155,12 +155,31 @@ found: return ret; } +static int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle) +{ + struct i915_vma *vma, *next; + int ret; + + if (do_idle) { + ret = i915_gpu_idle(vm-dev); + if (ret) + return ret; + + i915_gem_retire_requests(vm-dev); + } + + list_for_each_entry_safe(vma, next, vm-inactive_list, mm_list) + if (vma-obj-pin_count == 0) + WARN_ON(i915_vma_unbind(vma)); + + return 0; +} + int i915_gem_evict_everything(struct drm_device *dev) { drm_i915_private_t *dev_priv = dev-dev_private; struct i915_address_space *vm; - struct i915_vma *vma, *next; bool lists_empty = true; int ret; @@ -187,11 +206,8 @@ i915_gem_evict_everything(struct drm_device *dev) i915_gem_retire_requests(dev); /* Having flushed everything, unbind() should never raise an error */ - list_for_each_entry(vm, dev_priv-vm_list, global_link) { - list_for_each_entry_safe(vma, next, vm-inactive_list, mm_list) - if (vma-obj-pin_count == 0) - WARN_ON(i915_vma_unbind(vma)); - } + list_for_each_entry(vm, dev_priv-vm_list, global_link) + WARN_ON(i915_gem_evict_vm(vm, false)); return 0; } -- 1.8.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/8] drm/i915: Synchronize pread/pwrite with wait_rendering
lifted from Daniel: pread/pwrite isn't about the object's domain at all, but purely about synchronizing for outstanding rendering. Replacing the call to set_to_gtt_domain with a wait_rendering would imo improve code readability. Furthermore we could pimp pread to only block for outstanding writes and not for reads. Since you're not the first one to trip over this: Can I volunteer you for a follow-up patch to fix this? Recommended-by: Daniel Vetter daniel.vet...@ffwll.ch Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem.c | 19 +-- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index f026153..a839bcb 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -41,6 +41,9 @@ static void i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *o static void i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj, bool force); static __must_check int +i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj, + bool readonly); +static __must_check int i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj, struct i915_address_space *vm, unsigned alignment, @@ -430,11 +433,9 @@ i915_gem_shmem_pread(struct drm_device *dev, * optimizes for the case when the gpu will dirty the data * anyway again before the next pread happens. */ needs_clflush = !cpu_cache_is_coherent(dev, obj-cache_level); - if (i915_gem_obj_bound_any(obj)) { - ret = i915_gem_object_set_to_gtt_domain(obj, false); - if (ret) - return ret; - } + ret = i915_gem_object_wait_rendering(obj, false); + if (ret) + return ret; } ret = i915_gem_object_get_pages(obj); @@ -746,11 +747,9 @@ i915_gem_shmem_pwrite(struct drm_device *dev, * optimizes for the case when the gpu will use the data * right away and we therefore have to clflush anyway. */ needs_clflush_after = cpu_write_needs_clflush(obj); - if (i915_gem_obj_bound_any(obj)) { - ret = i915_gem_object_set_to_gtt_domain(obj, true); - if (ret) - return ret; - } + ret = i915_gem_object_wait_rendering(obj, false); + if (ret) + return ret; } /* Same trick applies to invalidate partially written cachelines read * before writing. */ -- 1.8.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/8] drm/i915: evict VM instead of everything
When reserving objects during execbuf, it is possible to come across an object which will not fit given the current fragmentation of the address space. We do not have any defragment in drm_mm, so the strategy is to instead evict everything, and reallocate objects. With the upcoming addition of multiple VMs, there is no point to evict everything since doing so is overkill for the specific case mentioned above. Recommended-by: Daniel Vetter daniel.vet...@ffwll.ch Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_drv.h| 1 + drivers/gpu/drm/i915/i915_gem_evict.c | 17 - drivers/gpu/drm/i915/i915_gem_execbuffer.c | 8 +++- 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 0602c4b..6e8ade0 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2088,6 +2088,7 @@ int __must_check i915_gem_evict_something(struct drm_device *dev, unsigned cache_level, bool mappable, bool nonblock); +int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle); int i915_gem_evict_everything(struct drm_device *dev); /* i915_gem_stolen.c */ diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c index e9033f0..a3e279d 100644 --- a/drivers/gpu/drm/i915/i915_gem_evict.c +++ b/drivers/gpu/drm/i915/i915_gem_evict.c @@ -155,7 +155,22 @@ found: return ret; } -static int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle) +/** + * i915_gem_evict_vm - Try to free up VM space + * + * @vm: Address space to evict from + * @do_idle: Boolean directing whether to idle first. + * + * VM eviction is about freeing up virtual address space. If one wants fine + * grained eviction, they should see evict something for more details. In terms + * of freeing up actual system memory, this function may not accomplish the + * desired result. An object may be shared in multiple address space, and this + * function will not assert those objects be freed. + * + * Using do_idle will result in a more complete eviction because it retires, and + * inactivates current BOs. + */ +int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle) { struct i915_vma *vma, *next; int ret; diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index ea4bacb..f0a0bd7 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -546,10 +546,16 @@ i915_gem_execbuffer_reserve(struct intel_ring_buffer *ring, { struct drm_i915_gem_object *obj; struct i915_vma *vma; + struct i915_address_space *vm; struct list_head ordered_vmas; bool has_fenced_gpu_access = INTEL_INFO(ring-dev)-gen 4; int retry; + if (list_empty(vmas)) + return 0; + + vm = list_first_entry(vmas, struct i915_vma, exec_list)-vm; + INIT_LIST_HEAD(ordered_vmas); while (!list_empty(vmas)) { struct drm_i915_gem_exec_object2 *entry; @@ -638,7 +644,7 @@ err:/* Decrement pin count for bound objects */ if (ret != -ENOSPC || retry++) return ret; - ret = i915_gem_evict_everything(ring-dev); + ret = i915_gem_evict_vm(vm, true); if (ret) return ret; } while (1); -- 1.8.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/8] drm/i915: trace vm eviction instead of everything
Tracing vm eviction is really the event we care about. For the cases we evict everything, we still will get the trace. Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem_evict.c | 4 ++-- drivers/gpu/drm/i915/i915_trace.h | 12 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c index a3e279d..ff91ac6 100644 --- a/drivers/gpu/drm/i915/i915_gem_evict.c +++ b/drivers/gpu/drm/i915/i915_gem_evict.c @@ -175,6 +175,8 @@ int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle) struct i915_vma *vma, *next; int ret; + trace_i915_gem_evict_vm(vm); + if (do_idle) { ret = i915_gpu_idle(vm-dev); if (ret) @@ -208,8 +210,6 @@ i915_gem_evict_everything(struct drm_device *dev) if (lists_empty) return -ENOSPC; - trace_i915_gem_evict_everything(dev); - /* The gpu_idle will flush everything in the write domain to the * active list. Then we must move everything off the active list * with retire requests. diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h index e2c5ee6..d2a5502 100644 --- a/drivers/gpu/drm/i915/i915_trace.h +++ b/drivers/gpu/drm/i915/i915_trace.h @@ -218,19 +218,19 @@ TRACE_EVENT(i915_gem_evict, __entry-mappable ? , mappable : ) ); -TRACE_EVENT(i915_gem_evict_everything, - TP_PROTO(struct drm_device *dev), - TP_ARGS(dev), +TRACE_EVENT(i915_gem_evict_vm, + TP_PROTO(struct i915_address_space *vm), + TP_ARGS(vm), TP_STRUCT__entry( -__field(u32, dev) +__field(struct i915_address_space *, vm) ), TP_fast_assign( - __entry-dev = dev-primary-index; + __entry-vm = vm; ), - TP_printk(dev=%d, __entry-dev) + TP_printk(vm=%p, __entry-vm) ); TRACE_EVENT(i915_gem_ring_dispatch, -- 1.8.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 5/8] drm/i915: Convert active API to VMA
From: Ben Widawsky b...@bwidawsk.net Even though we track object activity and not VMA, because we have the active_list be based on the VM, it makes the most sense to use VMAs in the APIs. NOTE: Daniel intends to eventually rip out active/inactive LRUs, but for now, leave them be. Signed-off-by: Ben Widawsky b...@bwidawsk.net Conflicts: drivers/gpu/drm/i915/i915_gem_execbuffer.c --- drivers/gpu/drm/i915/i915_drv.h| 5 ++--- drivers/gpu/drm/i915/i915_gem.c| 11 +-- drivers/gpu/drm/i915/i915_gem_context.c| 8 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 3 +-- 4 files changed, 16 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 6e8ade0..c9ed77a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1866,9 +1866,8 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj) int __must_check i915_mutex_lock_interruptible(struct drm_device *dev); int i915_gem_object_sync(struct drm_i915_gem_object *obj, struct intel_ring_buffer *to); -void i915_gem_object_move_to_active(struct drm_i915_gem_object *obj, - struct intel_ring_buffer *ring); - +void i915_vma_move_to_active(struct i915_vma *vma, +struct intel_ring_buffer *ring); int i915_gem_dumb_create(struct drm_file *file_priv, struct drm_device *dev, struct drm_mode_create_dumb *args); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index a839bcb..8547b97 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1881,11 +1881,11 @@ i915_gem_object_get_pages(struct drm_i915_gem_object *obj) return 0; } -void +static void i915_gem_object_move_to_active(struct drm_i915_gem_object *obj, struct intel_ring_buffer *ring) { - struct drm_device *dev = obj-base.dev; + struct drm_device *dev = ring-dev; struct drm_i915_private *dev_priv = dev-dev_private; u32 seqno = intel_ring_get_seqno(ring); @@ -1920,6 +1920,13 @@ i915_gem_object_move_to_active(struct drm_i915_gem_object *obj, } } +void i915_vma_move_to_active(struct i915_vma *vma, +struct intel_ring_buffer *ring) +{ + list_move_tail(vma-mm_list, vma-vm-active_list); + return i915_gem_object_move_to_active(vma-obj, ring); +} + static void i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj) { diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 403309c..6a0f568 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -436,11 +436,11 @@ static int do_switch(struct i915_hw_context *to) * MI_SET_CONTEXT instead of when the next seqno has completed. */ if (from != NULL) { - struct drm_i915_private *dev_priv = from-obj-base.dev-dev_private; - struct i915_address_space *ggtt = dev_priv-gtt.base; + struct drm_i915_private *dev_priv = ring-dev-dev_private; + struct i915_vma *vma = + i915_gem_obj_to_vma(from-obj, dev_priv-gtt.base); from-obj-base.read_domains = I915_GEM_DOMAIN_INSTRUCTION; - list_move_tail(i915_gem_obj_to_vma(from-obj, ggtt)-mm_list, ggtt-active_list); - i915_gem_object_move_to_active(from-obj, ring); + i915_vma_move_to_active(vma, ring); /* As long as MI_SET_CONTEXT is serializing, ie. it flushes the * whole damn pipeline, we don't need to explicitly mark the * object dirty. The only exception is that the context must be diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index f0a0bd7..928d37b 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -869,8 +869,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas, obj-base.read_domains = obj-base.pending_read_domains; obj-fenced_gpu_access = obj-pending_fenced_gpu_access; - list_move_tail(vma-mm_list, vma-vm-active_list); - i915_gem_object_move_to_active(obj, ring); + i915_vma_move_to_active(vma, ring); if (obj-base.write_domain) { obj-dirty = 1; obj-last_write_seqno = intel_ring_get_seqno(ring); -- 1.8.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 6/8] drm/i915: Add bind/unbind object functions to VM
From: Ben Widawsky b...@bwidawsk.net As we plumb the code with more VM information, it has become more obvious that the easiest way to deal with bind and unbind is to simply put the function pointers in the vm, and let those choose the correct way to handle the page table updates. This change allows many places in the code to simply be vm-bind, and not have to worry about distinguishing PPGTT vs GGTT. Notice that this patch has no impact on functionality. I've decided to save the actual change until the next patch because I think it's easier to review that way. I'm happy to squash the two, or let Daniel do it on merge. v2: Make ggtt handle the quirky aliasing ppgtt Add flags to bind object to support above Don't ever call bind/unbind directly for PPGTT until we have real, full PPGTT (use NULLs to assert this) Make sure we rebind the ggtt if there already is a ggtt binding. This happens on set cache levels Use VMA for bind/unbind (Daniel, Ben) Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_drv.h | 69 +--- drivers/gpu/drm/i915/i915_gem_gtt.c | 101 2 files changed, 140 insertions(+), 30 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index c9ed77a..377ca63 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -461,6 +461,36 @@ enum i915_cache_level { typedef uint32_t gen6_gtt_pte_t; +/** + * A VMA represents a GEM BO that is bound into an address space. Therefore, a + * VMA's presence cannot be guaranteed before binding, or after unbinding the + * object into/from the address space. + * + * To make things as simple as possible (ie. no refcounting), a VMA's lifetime + * will always be = an objects lifetime. So object refcounting should cover us. + */ +struct i915_vma { + struct drm_mm_node node; + struct drm_i915_gem_object *obj; + struct i915_address_space *vm; + + /** This object's place on the active/inactive lists */ + struct list_head mm_list; + + struct list_head vma_link; /* Link in the object's VMA list */ + + /** This vma's place in the batchbuffer or on the eviction list */ + struct list_head exec_list; + + /** +* Used for performing relocations during execbuffer insertion. +*/ + struct hlist_node exec_node; + unsigned long exec_handle; + struct drm_i915_gem_exec_object2 *exec_entry; + +}; + struct i915_address_space { struct drm_mm mm; struct drm_device *dev; @@ -499,9 +529,18 @@ struct i915_address_space { /* FIXME: Need a more generic return type */ gen6_gtt_pte_t (*pte_encode)(dma_addr_t addr, enum i915_cache_level level); + + /** Unmap an object from an address space. This usually consists of +* setting the valid PTE entries to a reserved scratch page. */ + void (*unbind_vma)(struct i915_vma *vma); void (*clear_range)(struct i915_address_space *vm, unsigned int first_entry, unsigned int num_entries); + /* Map an object into an address space with the given cache flags. */ +#define GLOBAL_BIND (10) + void (*bind_vma)(struct i915_vma *vma, +enum i915_cache_level cache_level, +u32 flags); void (*insert_entries)(struct i915_address_space *vm, struct sg_table *st, unsigned int first_entry, @@ -548,36 +587,6 @@ struct i915_hw_ppgtt { int (*enable)(struct drm_device *dev); }; -/** - * A VMA represents a GEM BO that is bound into an address space. Therefore, a - * VMA's presence cannot be guaranteed before binding, or after unbinding the - * object into/from the address space. - * - * To make things as simple as possible (ie. no refcounting), a VMA's lifetime - * will always be = an objects lifetime. So object refcounting should cover us. - */ -struct i915_vma { - struct drm_mm_node node; - struct drm_i915_gem_object *obj; - struct i915_address_space *vm; - - /** This object's place on the active/inactive lists */ - struct list_head mm_list; - - struct list_head vma_link; /* Link in the object's VMA list */ - - /** This vma's place in the batchbuffer or on the eviction list */ - struct list_head exec_list; - - /** -* Used for performing relocations during execbuffer insertion. -*/ - struct hlist_node exec_node; - unsigned long exec_handle; - struct drm_i915_gem_exec_object2 *exec_entry; - -}; - struct i915_ctx_hang_stats { /* This context had batch pending when hang was declared */ unsigned batch_pending; diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 212f6d8..d5a4580 100644 ---
[Intel-gfx] [PATCH 7/8] drm/i915: Use the new vm [un]bind functions
From: Ben Widawsky b...@bwidawsk.net Building on the last patch which created the new function pointers in the VM for bind/unbind, here we actually put those new function pointers to use. Split out as a separate patch to aid in review. I'm fine with squashing into the previous patch if people request it. v2: Updated to address the smart ggtt which can do aliasing as needed Make sure we bind to global gtt when mappable and fenceable. I thought we could get away without this initialy, but we cannot. Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_drv.h| 9 -- drivers/gpu/drm/i915/i915_gem.c| 31 --- drivers/gpu/drm/i915/i915_gem_context.c| 8 +++-- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 29 -- drivers/gpu/drm/i915/i915_gem_gtt.c| 48 ++ 5 files changed, 34 insertions(+), 91 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 377ca63..94cbd0a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2065,17 +2065,8 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data, /* i915_gem_gtt.c */ void i915_gem_cleanup_aliasing_ppgtt(struct drm_device *dev); -void i915_ppgtt_bind_object(struct i915_hw_ppgtt *ppgtt, - struct drm_i915_gem_object *obj, - enum i915_cache_level cache_level); -void i915_ppgtt_unbind_object(struct i915_hw_ppgtt *ppgtt, - struct drm_i915_gem_object *obj); - void i915_gem_restore_gtt_mappings(struct drm_device *dev); int __must_check i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj); -void i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj, - enum i915_cache_level cache_level); -void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj); void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj); void i915_gem_init_global_gtt(struct drm_device *dev); void i915_gem_setup_global_gtt(struct drm_device *dev, unsigned long start, diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 8547b97..0082c81 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2645,12 +2645,8 @@ int i915_vma_unbind(struct i915_vma *vma) trace_i915_vma_unbind(vma); - if (obj-has_global_gtt_mapping) - i915_gem_gtt_unbind_object(obj); - if (obj-has_aliasing_ppgtt_mapping) { - i915_ppgtt_unbind_object(dev_priv-mm.aliasing_ppgtt, obj); - obj-has_aliasing_ppgtt_mapping = 0; - } + vma-vm-unbind_vma(vma); + i915_gem_gtt_finish_object(obj); i915_gem_object_unpin_pages(obj); @@ -3377,7 +3373,6 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj, enum i915_cache_level cache_level) { struct drm_device *dev = obj-base.dev; - drm_i915_private_t *dev_priv = dev-dev_private; struct i915_vma *vma; int ret; @@ -3416,11 +3411,8 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj, return ret; } - if (obj-has_global_gtt_mapping) - i915_gem_gtt_bind_object(obj, cache_level); - if (obj-has_aliasing_ppgtt_mapping) - i915_ppgtt_bind_object(dev_priv-mm.aliasing_ppgtt, - obj, cache_level); + list_for_each_entry(vma, obj-vma_list, vma_link) + vma-vm-bind_vma(vma, cache_level, 0); } list_for_each_entry(vma, obj-vma_list, vma_link) @@ -3748,6 +3740,7 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj, bool map_and_fenceable, bool nonblocking) { + const u32 flags = map_and_fenceable ? GLOBAL_BIND : 0; struct i915_vma *vma; int ret; @@ -3776,20 +3769,22 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj, } if (!i915_gem_obj_bound(obj, vm)) { - struct drm_i915_private *dev_priv = obj-base.dev-dev_private; - ret = i915_gem_object_bind_to_vm(obj, vm, alignment, map_and_fenceable, nonblocking); if (ret) return ret; - if (!dev_priv-mm.aliasing_ppgtt) - i915_gem_gtt_bind_object(obj, obj-cache_level); - } + vma = i915_gem_obj_to_vma(obj, vm); + vm-bind_vma(vma, obj-cache_level, flags); + } else + vma = i915_gem_obj_to_vma(obj, vm); + /* Objects are created map and fenceable. If we bind an object +* the first time, and we had aliasing PPGTT
[Intel-gfx] [PATCH 8/8] drm/i915: eliminate vm-insert_entries()
From: Ben Widawsky b...@bwidawsk.net With bind/unbind function pointers in place, we no longer need insert_entries. We could, and want, to remove clear_range, however it's not totally easy at this point. Since it's used in a couple of place still that don't only deal in objects: setup, ppgtt init, and restore gtt mappings. Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem_gtt.c | 17 + 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 3c752ad..72c8b8a 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -339,8 +339,8 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) ppgtt-enable = gen6_ppgtt_enable; ppgtt-base.unbind_vma = NULL; ppgtt-base.clear_range = gen6_ppgtt_clear_range; - ppgtt-base.bind_vma = NULL; ppgtt-base.insert_entries = gen6_ppgtt_insert_entries; + ppgtt-base.bind_vma = NULL; ppgtt-base.cleanup = gen6_ppgtt_cleanup; ppgtt-base.scratch = dev_priv-gtt.base.scratch; ppgtt-pt_pages = kzalloc(sizeof(struct page *)*ppgtt-num_pd_entries, @@ -591,19 +591,6 @@ static void gen6_ggtt_clear_range(struct i915_address_space *vm, readl(gtt_base); } - -static void i915_ggtt_insert_entries(struct i915_address_space *vm, -struct sg_table *st, -unsigned int pg_start, -enum i915_cache_level cache_level) -{ - unsigned int flags = (cache_level == I915_CACHE_NONE) ? - AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY; - - intel_gtt_insert_sg_entries(st, pg_start, flags); - -} - static void i915_ggtt_bind_vma(struct i915_vma *vma, enum i915_cache_level cache_level, u32 unused) @@ -916,7 +903,6 @@ static int gen6_gmch_probe(struct drm_device *dev, dev_priv-gtt.base.clear_range = gen6_ggtt_clear_range; dev_priv-gtt.base.unbind_vma = gen6_ggtt_unbind_vma; - dev_priv-gtt.base.insert_entries = gen6_ggtt_insert_entries; dev_priv-gtt.base.bind_vma = gen6_ggtt_bind_vma; return ret; @@ -950,7 +936,6 @@ static int i915_gmch_probe(struct drm_device *dev, dev_priv-gtt.do_idle_maps = needs_idle_maps(dev_priv-dev); dev_priv-gtt.base.clear_range = i915_ggtt_clear_range; dev_priv-gtt.base.unbind_vma = i915_ggtt_unbind_vma; - dev_priv-gtt.base.insert_entries = i915_ggtt_insert_entries; dev_priv-gtt.base.bind_vma = i915_ggtt_bind_vma; return 0; -- 1.8.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/8] drm/i915: Synchronize pread/pwrite with wait_rendering
On Fri, Aug 30, 2013 at 04:43:54PM -0700, Ben Widawsky wrote: lifted from Daniel: pread/pwrite isn't about the object's domain at all, but purely about synchronizing for outstanding rendering. Replacing the call to set_to_gtt_domain with a wait_rendering would imo improve code readability. Furthermore we could pimp pread to only block for outstanding writes and not for reads. Since you're not the first one to trip over this: Can I volunteer you for a follow-up patch to fix this? Recommended-by: Daniel Vetter daniel.vet...@ffwll.ch Signed-off-by: Ben Widawsky b...@bwidawsk.net This should fail i-g-t... -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 2/8] drm/i915: Extract vm specific part of eviction
On Fri, Aug 30, 2013 at 04:43:55PM -0700, Ben Widawsky wrote: As we'll see in the next patch, being able to evict for just 1 VM is handy. Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem_evict.c | 28 ++-- 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c index cc8974f..e9033f0 100644 --- a/drivers/gpu/drm/i915/i915_gem_evict.c +++ b/drivers/gpu/drm/i915/i915_gem_evict.c @@ -155,12 +155,31 @@ found: return ret; } +static int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle) +{ + struct i915_vma *vma, *next; + int ret; + + if (do_idle) { + ret = i915_gpu_idle(vm-dev); + if (ret) + return ret; + + i915_gem_retire_requests(vm-dev); + } + + list_for_each_entry_safe(vma, next, vm-inactive_list, mm_list) + if (vma-obj-pin_count == 0) + WARN_ON(i915_vma_unbind(vma)); + + return 0; +} + int i915_gem_evict_everything(struct drm_device *dev) { drm_i915_private_t *dev_priv = dev-dev_private; struct i915_address_space *vm; - struct i915_vma *vma, *next; bool lists_empty = true; int ret; @@ -187,11 +206,8 @@ i915_gem_evict_everything(struct drm_device *dev) i915_gem_retire_requests(dev); /* Having flushed everything, unbind() should never raise an error */ - list_for_each_entry(vm, dev_priv-vm_list, global_link) { - list_for_each_entry_safe(vma, next, vm-inactive_list, mm_list) - if (vma-obj-pin_count == 0) - WARN_ON(i915_vma_unbind(vma)); - } + list_for_each_entry(vm, dev_priv-vm_list, global_link) + WARN_ON(i915_gem_evict_vm(vm, false)); Wny not use do_idle here? -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/8] drm/i915: evict VM instead of everything
On Fri, Aug 30, 2013 at 04:43:56PM -0700, Ben Widawsky wrote: When reserving objects during execbuf, it is possible to come across an object which will not fit given the current fragmentation of the address space. We do not have any defragment in drm_mm, so the strategy is to instead evict everything, and reallocate objects. With the upcoming addition of multiple VMs, there is no point to evict everything since doing so is overkill for the specific case mentioned above. Recommended-by: Daniel Vetter daniel.vet...@ffwll.ch Signed-off-by: Ben Widawsky b...@bwidawsk.net This loses the ENOSPC notification, and the tracepoints. I've just seen you add tracepoints later.. I'm pretty certain this will be caught by an extra pass through reserve, and probably worth it for the simplification. Fortunately, we do have i-g-t coverage for this. -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 4/8] drm/i915: trace vm eviction instead of everything
On Fri, Aug 30, 2013 at 04:43:57PM -0700, Ben Widawsky wrote: Tracing vm eviction is really the event we care about. For the cases we evict everything, we still will get the trace. Keep both until you retire evict_everything for good. Sometimes you need flashing neon lights to explain things in a trace. -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 5/8] drm/i915: Convert active API to VMA
On Fri, Aug 30, 2013 at 04:43:58PM -0700, Ben Widawsky wrote: From: Ben Widawsky b...@bwidawsk.net Even though we track object activity and not VMA, because we have the active_list be based on the VM, it makes the most sense to use VMAs in the APIs. NOTE: Daniel intends to eventually rip out active/inactive LRUs, but for now, leave them be. Signed-off-by: Ben Widawsky b...@bwidawsk.net Conflicts: drivers/gpu/drm/i915/i915_gem_execbuffer.c There's a silly chunk here, but otherwise ok. --- drivers/gpu/drm/i915/i915_drv.h| 5 ++--- drivers/gpu/drm/i915/i915_gem.c| 11 +-- drivers/gpu/drm/i915/i915_gem_context.c| 8 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 3 +-- 4 files changed, 16 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 6e8ade0..c9ed77a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1866,9 +1866,8 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj) int __must_check i915_mutex_lock_interruptible(struct drm_device *dev); int i915_gem_object_sync(struct drm_i915_gem_object *obj, struct intel_ring_buffer *to); -void i915_gem_object_move_to_active(struct drm_i915_gem_object *obj, - struct intel_ring_buffer *ring); - +void i915_vma_move_to_active(struct i915_vma *vma, + struct intel_ring_buffer *ring); int i915_gem_dumb_create(struct drm_file *file_priv, struct drm_device *dev, struct drm_mode_create_dumb *args); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index a839bcb..8547b97 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1881,11 +1881,11 @@ i915_gem_object_get_pages(struct drm_i915_gem_object *obj) return 0; } -void +static void i915_gem_object_move_to_active(struct drm_i915_gem_object *obj, struct intel_ring_buffer *ring) { - struct drm_device *dev = obj-base.dev; + struct drm_device *dev = ring-dev; ? Are you trying to anger the static checkers? -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 6/8] drm/i915: Add bind/unbind object functions to VM
On Fri, Aug 30, 2013 at 04:43:59PM -0700, Ben Widawsky wrote: From: Ben Widawsky b...@bwidawsk.net As we plumb the code with more VM information, it has become more obvious that the easiest way to deal with bind and unbind is to simply put the function pointers in the vm, and let those choose the correct way to handle the page table updates. This change allows many places in the code to simply be vm-bind, and not have to worry about distinguishing PPGTT vs GGTT. Notice that this patch has no impact on functionality. I've decided to save the actual change until the next patch because I think it's easier to review that way. I'm happy to squash the two, or let Daniel do it on merge. v2: Make ggtt handle the quirky aliasing ppgtt Add flags to bind object to support above Don't ever call bind/unbind directly for PPGTT until we have real, full PPGTT (use NULLs to assert this) Make sure we rebind the ggtt if there already is a ggtt binding. This happens on set cache levels Use VMA for bind/unbind (Daniel, Ben) Signed-off-by: Ben Widawsky b...@bwidawsk.net You like pokkadot paint for its inconsistency? Other than interesting alternation of styles, I see nothing wrong with the logic. -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] Avoid i915 flip unpin/HPD event handler deadlock.
Both of these were taking the mode_config mutex but executed from the same work queue. If intel_crtc_page_flip happened to flush a work queue containing an HPD event handler work item, deadlock resulted, since the mutex required by the HPD handler was taken before the flush. Instead use a separate work queue for the flip unpin work. Signed-off-by: sabercrom...@chromium.org Reviewed-by: marc...@chromium.org --- drivers/gpu/drm/i915/i915_dma.c | 21 - drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_display.c | 4 ++-- 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 4f129bb..9215360 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1558,6 +1558,22 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) goto out_mtrrfree; } + /* intel_crtc_page_flip runs with the mode_config mutex having been +* taken in the DRM layer. It synchronously waits for pending unpin +* work items while holding this mutex. Therefore this queue cannot +* contain work items that take this mutex, such as HPD event +* handling, or we deadlock. There is also no reason for flipping to +* wait on such events. Therefore put flip unpinning in its own +* work queue. +*/ + dev_priv-flip_unpin_wq = alloc_ordered_workqueue(i915, 0); + if (dev_priv-flip_unpin_wq == NULL) { + DRM_ERROR(Failed to create flip unpin workqueue.\n); + destroy_workqueue(dev_priv-wq); + ret = -ENOMEM; + goto out_mtrrfree; + } + /* This must be called before any calls to HAS_PCH_* */ intel_detect_pch(dev); @@ -1628,6 +1644,7 @@ out_gem_unload: intel_teardown_gmbus(dev); intel_teardown_mchbar(dev); destroy_workqueue(dev_priv-wq); + destroy_workqueue(dev_priv-flip_unpin_wq); out_mtrrfree: if (dev_priv-mm.gtt_mtrr = 0) { mtrr_del(dev_priv-mm.gtt_mtrr, @@ -1709,7 +1726,8 @@ int i915_driver_unload(struct drm_device *dev) intel_opregion_fini(dev); if (drm_core_check_feature(dev, DRIVER_MODESET)) { - /* Flush any outstanding unpin_work. */ + /* Flush any outstanding unpin, HPD, etc. work. */ + flush_workqueue(dev_priv-flip_unpin_wq); flush_workqueue(dev_priv-wq); mutex_lock(dev-struct_mutex); @@ -1734,6 +1752,7 @@ int i915_driver_unload(struct drm_device *dev) intel_teardown_mchbar(dev); destroy_workqueue(dev_priv-wq); + destroy_workqueue(dev_priv-flip_unpin_wq); pci_dev_put(dev_priv-bridge_dev); kfree(dev-dev_private); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 4130e6d..7eb4619 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -766,6 +766,7 @@ typedef struct drm_i915_private { struct work_struct error_work; struct completion error_completion; struct workqueue_struct *wq; + struct workqueue_struct *flip_unpin_wq; /* Display functions */ struct drm_i915_display_funcs display; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e204913..acd07dd 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6937,7 +6937,7 @@ static void do_intel_finish_page_flip(struct drm_device *dev, obj-pending_flip.counter); wake_up(dev_priv-pending_flip_queue); - queue_work(dev_priv-wq, work-work); + queue_work(dev_priv-flip_unpin_wq, work-work); trace_i915_flip_complete(intel_crtc-plane, work-pending_flip_obj); } @@ -7305,7 +7305,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, spin_unlock_irqrestore(dev-event_lock, flags); if (atomic_read(intel_crtc-unpin_work_count) = 2) - flush_workqueue(dev_priv-wq); + flush_workqueue(dev_priv-flip_unpin_wq); ret = i915_mutex_lock_interruptible(dev); if (ret) -- 1.8.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/8] drm/i915: Synchronize pread/pwrite with wait_rendering
On Sat, Aug 31, 2013 at 12:50:30AM +0100, Chris Wilson wrote: On Fri, Aug 30, 2013 at 04:43:54PM -0700, Ben Widawsky wrote: lifted from Daniel: pread/pwrite isn't about the object's domain at all, but purely about synchronizing for outstanding rendering. Replacing the call to set_to_gtt_domain with a wait_rendering would imo improve code readability. Furthermore we could pimp pread to only block for outstanding writes and not for reads. Since you're not the first one to trip over this: Can I volunteer you for a follow-up patch to fix this? Recommended-by: Daniel Vetter daniel.vet...@ffwll.ch Signed-off-by: Ben Widawsky b...@bwidawsk.net This should fail i-g-t... -Chris Daniel, how have I failed your plan? -- Ben Widawsky, 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 2/8] drm/i915: Extract vm specific part of eviction
On Sat, Aug 31, 2013 at 12:52:03AM +0100, Chris Wilson wrote: On Fri, Aug 30, 2013 at 04:43:55PM -0700, Ben Widawsky wrote: As we'll see in the next patch, being able to evict for just 1 VM is handy. Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem_evict.c | 28 ++-- 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c index cc8974f..e9033f0 100644 --- a/drivers/gpu/drm/i915/i915_gem_evict.c +++ b/drivers/gpu/drm/i915/i915_gem_evict.c @@ -155,12 +155,31 @@ found: return ret; } +static int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle) +{ + struct i915_vma *vma, *next; + int ret; + + if (do_idle) { + ret = i915_gpu_idle(vm-dev); + if (ret) + return ret; + + i915_gem_retire_requests(vm-dev); + } + + list_for_each_entry_safe(vma, next, vm-inactive_list, mm_list) + if (vma-obj-pin_count == 0) + WARN_ON(i915_vma_unbind(vma)); + + return 0; +} + int i915_gem_evict_everything(struct drm_device *dev) { drm_i915_private_t *dev_priv = dev-dev_private; struct i915_address_space *vm; - struct i915_vma *vma, *next; bool lists_empty = true; int ret; @@ -187,11 +206,8 @@ i915_gem_evict_everything(struct drm_device *dev) i915_gem_retire_requests(dev); /* Having flushed everything, unbind() should never raise an error */ - list_for_each_entry(vm, dev_priv-vm_list, global_link) { - list_for_each_entry_safe(vma, next, vm-inactive_list, mm_list) - if (vma-obj-pin_count == 0) - WARN_ON(i915_vma_unbind(vma)); - } + list_for_each_entry(vm, dev_priv-vm_list, global_link) + WARN_ON(i915_gem_evict_vm(vm, false)); Wny not use do_idle here? -Chris The point of the extraction was to exactly avoid repeated call to idle + retire, although in looking back at it, I suppose the cost wouldn't be much since the first idle would make subsequent calls to idle + retire just about free). Just dropping the do_idle parameter, and always idling is what you probably want. -- Ben Widawsky, 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 4/8] drm/i915: trace vm eviction instead of everything
On Sat, Aug 31, 2013 at 01:06:02AM +0100, Chris Wilson wrote: On Fri, Aug 30, 2013 at 04:43:57PM -0700, Ben Widawsky wrote: Tracing vm eviction is really the event we care about. For the cases we evict everything, we still will get the trace. Keep both until you retire evict_everything for good. Sometimes you need flashing neon lights to explain things in a trace. -Chris Got it. Will resend after Daniel has a chance to look. Do you want this squashed in with the previous patch to keep the trace events as well? -- Ben Widawsky, 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 6/8] drm/i915: Add bind/unbind object functions to VM
On Sat, Aug 31, 2013 at 01:12:55AM +0100, Chris Wilson wrote: On Fri, Aug 30, 2013 at 04:43:59PM -0700, Ben Widawsky wrote: From: Ben Widawsky b...@bwidawsk.net As we plumb the code with more VM information, it has become more obvious that the easiest way to deal with bind and unbind is to simply put the function pointers in the vm, and let those choose the correct way to handle the page table updates. This change allows many places in the code to simply be vm-bind, and not have to worry about distinguishing PPGTT vs GGTT. Notice that this patch has no impact on functionality. I've decided to save the actual change until the next patch because I think it's easier to review that way. I'm happy to squash the two, or let Daniel do it on merge. v2: Make ggtt handle the quirky aliasing ppgtt Add flags to bind object to support above Don't ever call bind/unbind directly for PPGTT until we have real, full PPGTT (use NULLs to assert this) Make sure we rebind the ggtt if there already is a ggtt binding. This happens on set cache levels Use VMA for bind/unbind (Daniel, Ben) Signed-off-by: Ben Widawsky b...@bwidawsk.net You like pokkadot paint for its inconsistency? Other than interesting alternation of styles, I see nothing wrong with the logic. -Chris To what are you referring? I'm probably more than willing to change whatever displeases you. -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx