* [PATCH 1/2] drm/i915: Extract intel_wm_plane_visible()
@ 2017-03-14 15:10 ville.syrjala
2017-03-14 15:10 ` [PATCH 2/2] drm/i915: Fix SKL cursor watermarks ville.syrjala
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: ville.syrjala @ 2017-03-14 15:10 UTC (permalink / raw)
To: intel-gfx
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
All platforms that lack double buffered watermarks will need to
handle the legacy cursor updates in the same way. So let's extract the
logic to determine the plane visibility into a small helper. For
simplicity we'll make the function DTRT for any plane, but only apply
the special sauce for cursor planes.
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/intel_pm.c | 39 +++++++++++++++++++++++++++------------
1 file changed, 27 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2ca38ae4421e..3b0d379b6f38 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -655,6 +655,29 @@ static unsigned long intel_calculate_wm(unsigned long clock_in_khz,
return wm_size;
}
+static bool intel_wm_plane_visible(const struct intel_crtc_state *crtc_state,
+ const struct intel_plane_state *plane_state)
+{
+ struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
+
+ /* FIXME check the 'enable' instead */
+ if (!crtc_state->base.active)
+ return false;
+
+ /*
+ * Treat cursor with fb as always visible since cursor updates
+ * can happen faster than the vrefresh rate, and the current
+ * watermark code doesn't handle that correctly. Cursor updates
+ * which set/clear the fb or change the cursor size are going
+ * to get throttled by intel_legacy_cursor_update() to work
+ * around this problem with the watermark code.
+ */
+ if (plane->id == PLANE_CURSOR)
+ return plane_state->base.fb != NULL;
+ else
+ return plane_state->base.visible;
+}
+
static struct intel_crtc *single_enabled_crtc(struct drm_i915_private *dev_priv)
{
struct intel_crtc *crtc, *enabled = NULL;
@@ -1961,7 +1984,7 @@ static uint32_t ilk_compute_pri_wm(const struct intel_crtc_state *cstate,
uint32_t method1, method2;
int cpp;
- if (!cstate->base.active || !pstate->base.visible)
+ if (!intel_wm_plane_visible(cstate, pstate))
return 0;
cpp = pstate->base.fb->format->cpp[0];
@@ -1990,7 +2013,7 @@ static uint32_t ilk_compute_spr_wm(const struct intel_crtc_state *cstate,
uint32_t method1, method2;
int cpp;
- if (!cstate->base.active || !pstate->base.visible)
+ if (!intel_wm_plane_visible(cstate, pstate))
return 0;
cpp = pstate->base.fb->format->cpp[0];
@@ -2013,15 +2036,7 @@ static uint32_t ilk_compute_cur_wm(const struct intel_crtc_state *cstate,
{
int cpp;
- /*
- * Treat cursor with fb as always visible since cursor updates
- * can happen faster than the vrefresh rate, and the current
- * watermark code doesn't handle that correctly. Cursor updates
- * which set/clear the fb or change the cursor size are going
- * to get throttled by intel_legacy_cursor_update() to work
- * around this problem with the watermark code.
- */
- if (!cstate->base.active || !pstate->base.fb)
+ if (!intel_wm_plane_visible(cstate, pstate))
return 0;
cpp = pstate->base.fb->format->cpp[0];
@@ -2038,7 +2053,7 @@ static uint32_t ilk_compute_fbc_wm(const struct intel_crtc_state *cstate,
{
int cpp;
- if (!cstate->base.active || !pstate->base.visible)
+ if (!intel_wm_plane_visible(cstate, pstate))
return 0;
cpp = pstate->base.fb->format->cpp[0];
--
2.10.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 2/2] drm/i915: Fix SKL cursor watermarks 2017-03-14 15:10 [PATCH 1/2] drm/i915: Extract intel_wm_plane_visible() ville.syrjala @ 2017-03-14 15:10 ` ville.syrjala 2017-03-15 10:46 ` [2/2] " Tahvanainen, Jari 2017-03-22 9:01 ` [PATCH 2/2] " Maarten Lankhorst 2017-03-14 20:23 ` ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Extract intel_wm_plane_visible() Patchwork 2017-03-21 9:58 ` Dorota Czaplejewicz 2 siblings, 2 replies; 8+ messages in thread From: ville.syrjala @ 2017-03-14 15:10 UTC (permalink / raw) To: intel-gfx From: Ville Syrjälä <ville.syrjala@linux.intel.com> Use intel_wm_plane_visible() to determine cursor visibility for SKL+ also. Previously SKL+ would check the actual visibility which now conflicts with the assumptions in intel_legacy_cursor_update(). We also change SKL+ to compute the cursor watermarks based on the unclipped cursor size, just as we do on all the other platforms. Using the clipped size could now result in garbage results. Testcase: igt/kms_chv_cursor_fail Fixes: a5509abda48e ("drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW") Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100195 Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 44 +++++++++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 3b0d379b6f38..e51370cd5787 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3361,19 +3361,29 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv, * Caller should take care of dividing & rounding off the value. */ static uint32_t -skl_plane_downscale_amount(const struct intel_plane_state *pstate) +skl_plane_downscale_amount(const struct intel_crtc_state *cstate, + const struct intel_plane_state *pstate) { + struct intel_plane *plane = to_intel_plane(pstate->base.plane); uint32_t downscale_h, downscale_w; uint32_t src_w, src_h, dst_w, dst_h; - if (WARN_ON(!pstate->base.visible)) + if (WARN_ON(!intel_wm_plane_visible(cstate, pstate))) return DRM_PLANE_HELPER_NO_SCALING; /* n.b., src is 16.16 fixed point, dst is whole integer */ - src_w = drm_rect_width(&pstate->base.src); - src_h = drm_rect_height(&pstate->base.src); - dst_w = drm_rect_width(&pstate->base.dst); - dst_h = drm_rect_height(&pstate->base.dst); + if (plane->id == PLANE_CURSOR) { + src_w = pstate->base.src_w; + src_h = pstate->base.src_h; + dst_w = pstate->base.crtc_w; + dst_h = pstate->base.crtc_h; + } else { + src_w = drm_rect_width(&pstate->base.src); + src_h = drm_rect_height(&pstate->base.src); + dst_w = drm_rect_width(&pstate->base.dst); + dst_h = drm_rect_height(&pstate->base.dst); + } + if (drm_rotation_90_or_270(pstate->base.rotation)) swap(dst_w, dst_h); @@ -3389,6 +3399,7 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate, const struct drm_plane_state *pstate, int y) { + struct intel_plane *plane = to_intel_plane(pstate->plane); struct intel_plane_state *intel_pstate = to_intel_plane_state(pstate); uint32_t down_scale_amount, data_rate; uint32_t width = 0, height = 0; @@ -3401,7 +3412,7 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate, fb = pstate->fb; format = fb->format->format; - if (pstate->plane->type == DRM_PLANE_TYPE_CURSOR) + if (plane->id == PLANE_CURSOR) return 0; if (y && format != DRM_FORMAT_NV12) return 0; @@ -3425,7 +3436,7 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate, data_rate = width * height * fb->format->cpp[0]; } - down_scale_amount = skl_plane_downscale_amount(intel_pstate); + down_scale_amount = skl_plane_downscale_amount(cstate, intel_pstate); return (uint64_t)data_rate * down_scale_amount >> 16; } @@ -3717,7 +3728,7 @@ static uint32_t skl_adjusted_plane_pixel_rate(const struct intel_crtc_state *cst uint64_t pixel_rate; /* Shouldn't reach here on disabled planes... */ - if (WARN_ON(!pstate->base.visible)) + if (WARN_ON(!intel_wm_plane_visible(cstate, pstate))) return 0; /* @@ -3725,7 +3736,7 @@ static uint32_t skl_adjusted_plane_pixel_rate(const struct intel_crtc_state *cst * with additional adjustments for plane-specific scaling. */ adjusted_pixel_rate = cstate->pixel_rate; - downscale_amount = skl_plane_downscale_amount(pstate); + downscale_amount = skl_plane_downscale_amount(cstate, pstate); pixel_rate = adjusted_pixel_rate * downscale_amount >> 16; WARN_ON(pixel_rate != clamp_t(uint32_t, pixel_rate, 0, ~0)); @@ -3742,6 +3753,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, uint8_t *out_lines, /* out */ bool *enabled /* out */) { + struct intel_plane *plane = to_intel_plane(intel_pstate->base.plane); struct drm_plane_state *pstate = &intel_pstate->base; struct drm_framebuffer *fb = pstate->fb; uint32_t latency = dev_priv->wm.skl_latency[level]; @@ -3761,7 +3773,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state); bool y_tiled, x_tiled; - if (latency == 0 || !cstate->base.active || !intel_pstate->base.visible) { + if (latency == 0 || + !intel_wm_plane_visible(cstate, intel_pstate)) { *enabled = false; return 0; } @@ -3777,8 +3790,13 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, if (apply_memory_bw_wa && x_tiled) latency += 15; - width = drm_rect_width(&intel_pstate->base.src) >> 16; - height = drm_rect_height(&intel_pstate->base.src) >> 16; + if (plane->id == PLANE_CURSOR) { + width = intel_pstate->base.crtc_w; + height = intel_pstate->base.crtc_h; + } else { + width = drm_rect_width(&intel_pstate->base.src) >> 16; + height = drm_rect_height(&intel_pstate->base.src) >> 16; + } if (drm_rotation_90_or_270(pstate->rotation)) swap(width, height); -- 2.10.2 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [2/2] drm/i915: Fix SKL cursor watermarks 2017-03-14 15:10 ` [PATCH 2/2] drm/i915: Fix SKL cursor watermarks ville.syrjala @ 2017-03-15 10:46 ` Tahvanainen, Jari 2017-03-22 9:01 ` [PATCH 2/2] " Maarten Lankhorst 1 sibling, 0 replies; 8+ messages in thread From: Tahvanainen, Jari @ 2017-03-15 10:46 UTC (permalink / raw) To: ville.syrjala@linux.intel.com, intel-gfx@lists.freedesktop.org I applied this patch (and [1/2] drm/i915: Extract intel_wm_plane_visible()) on top of drm-tip: 2017y-03m-14d-16h-04m-56s UTC integration manifest and run all igt/kms_chv_cursor_fail cases producing [36/36] pass: 36. Patch fixes the problem https://bugs.freedesktop.org/show_bug.cgi?id=100195. Tested-by: Jari Tahvanainen <jari.tahvanainen@intel.com> BR, Jari -----Original Message----- From: ville.syrjala@linux.intel.com [mailto:ville.syrjala@linux.intel.com] Sent: Tuesday, March 14, 2017 5:11 PM To: intel-gfx@lists.freedesktop.org Subject: [2/2] drm/i915: Fix SKL cursor watermarks From: Ville Syrjälä <ville.syrjala@linux.intel.com> Use intel_wm_plane_visible() to determine cursor visibility for SKL+ also. Previously SKL+ would check the actual visibility which now conflicts with the assumptions in intel_legacy_cursor_update(). We also change SKL+ to compute the cursor watermarks based on the unclipped cursor size, just as we do on all the other platforms. Using the clipped size could now result in garbage results. Testcase: igt/kms_chv_cursor_fail Fixes: a5509abda48e ("drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW") Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100195 Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 44 +++++++++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 3b0d379b6f38..e51370cd5787 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3361,19 +3361,29 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv, * Caller should take care of dividing & rounding off the value. */ static uint32_t -skl_plane_downscale_amount(const struct intel_plane_state *pstate) +skl_plane_downscale_amount(const struct intel_crtc_state *cstate, + const struct intel_plane_state *pstate) { + struct intel_plane *plane = to_intel_plane(pstate->base.plane); uint32_t downscale_h, downscale_w; uint32_t src_w, src_h, dst_w, dst_h; - if (WARN_ON(!pstate->base.visible)) + if (WARN_ON(!intel_wm_plane_visible(cstate, pstate))) return DRM_PLANE_HELPER_NO_SCALING; /* n.b., src is 16.16 fixed point, dst is whole integer */ - src_w = drm_rect_width(&pstate->base.src); - src_h = drm_rect_height(&pstate->base.src); - dst_w = drm_rect_width(&pstate->base.dst); - dst_h = drm_rect_height(&pstate->base.dst); + if (plane->id == PLANE_CURSOR) { + src_w = pstate->base.src_w; + src_h = pstate->base.src_h; + dst_w = pstate->base.crtc_w; + dst_h = pstate->base.crtc_h; + } else { + src_w = drm_rect_width(&pstate->base.src); + src_h = drm_rect_height(&pstate->base.src); + dst_w = drm_rect_width(&pstate->base.dst); + dst_h = drm_rect_height(&pstate->base.dst); + } + if (drm_rotation_90_or_270(pstate->base.rotation)) swap(dst_w, dst_h); @@ -3389,6 +3399,7 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate, const struct drm_plane_state *pstate, int y) { + struct intel_plane *plane = to_intel_plane(pstate->plane); struct intel_plane_state *intel_pstate = to_intel_plane_state(pstate); uint32_t down_scale_amount, data_rate; uint32_t width = 0, height = 0; @@ -3401,7 +3412,7 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate, fb = pstate->fb; format = fb->format->format; - if (pstate->plane->type == DRM_PLANE_TYPE_CURSOR) + if (plane->id == PLANE_CURSOR) return 0; if (y && format != DRM_FORMAT_NV12) return 0; @@ -3425,7 +3436,7 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate, data_rate = width * height * fb->format->cpp[0]; } - down_scale_amount = skl_plane_downscale_amount(intel_pstate); + down_scale_amount = skl_plane_downscale_amount(cstate, intel_pstate); return (uint64_t)data_rate * down_scale_amount >> 16; } @@ -3717,7 +3728,7 @@ static uint32_t skl_adjusted_plane_pixel_rate(const struct intel_crtc_state *cst uint64_t pixel_rate; /* Shouldn't reach here on disabled planes... */ - if (WARN_ON(!pstate->base.visible)) + if (WARN_ON(!intel_wm_plane_visible(cstate, pstate))) return 0; /* @@ -3725,7 +3736,7 @@ static uint32_t skl_adjusted_plane_pixel_rate(const struct intel_crtc_state *cst * with additional adjustments for plane-specific scaling. */ adjusted_pixel_rate = cstate->pixel_rate; - downscale_amount = skl_plane_downscale_amount(pstate); + downscale_amount = skl_plane_downscale_amount(cstate, pstate); pixel_rate = adjusted_pixel_rate * downscale_amount >> 16; WARN_ON(pixel_rate != clamp_t(uint32_t, pixel_rate, 0, ~0)); @@ -3742,6 +3753,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, uint8_t *out_lines, /* out */ bool *enabled /* out */) { + struct intel_plane *plane = to_intel_plane(intel_pstate->base.plane); struct drm_plane_state *pstate = &intel_pstate->base; struct drm_framebuffer *fb = pstate->fb; uint32_t latency = dev_priv->wm.skl_latency[level]; @@ -3761,7 +3773,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state); bool y_tiled, x_tiled; - if (latency == 0 || !cstate->base.active || !intel_pstate->base.visible) { + if (latency == 0 || + !intel_wm_plane_visible(cstate, intel_pstate)) { *enabled = false; return 0; } @@ -3777,8 +3790,13 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, if (apply_memory_bw_wa && x_tiled) latency += 15; - width = drm_rect_width(&intel_pstate->base.src) >> 16; - height = drm_rect_height(&intel_pstate->base.src) >> 16; + if (plane->id == PLANE_CURSOR) { + width = intel_pstate->base.crtc_w; + height = intel_pstate->base.crtc_h; + } else { + width = drm_rect_width(&intel_pstate->base.src) >> 16; + height = drm_rect_height(&intel_pstate->base.src) >> 16; + } if (drm_rotation_90_or_270(pstate->rotation)) swap(width, height); _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] drm/i915: Fix SKL cursor watermarks 2017-03-14 15:10 ` [PATCH 2/2] drm/i915: Fix SKL cursor watermarks ville.syrjala 2017-03-15 10:46 ` [2/2] " Tahvanainen, Jari @ 2017-03-22 9:01 ` Maarten Lankhorst 2017-03-22 9:51 ` Ander Conselvan De Oliveira 1 sibling, 1 reply; 8+ messages in thread From: Maarten Lankhorst @ 2017-03-22 9:01 UTC (permalink / raw) To: ville.syrjala, intel-gfx Op 14-03-17 om 16:10 schreef ville.syrjala@linux.intel.com: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Use intel_wm_plane_visible() to determine cursor visibility for SKL+ > also. Previously SKL+ would check the actual visibility which now > conflicts with the assumptions in intel_legacy_cursor_update(). > > We also change SKL+ to compute the cursor watermarks based on the > unclipped cursor size, just as we do on all the other platforms. > Using the clipped size could now result in garbage results. > > Testcase: igt/kms_chv_cursor_fail > Fixes: a5509abda48e ("drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW") > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100195 > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> For patch 1 & 2: Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Should be the right way to fix it. :) _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] drm/i915: Fix SKL cursor watermarks 2017-03-22 9:01 ` [PATCH 2/2] " Maarten Lankhorst @ 2017-03-22 9:51 ` Ander Conselvan De Oliveira 2017-03-22 20:20 ` Ville Syrjälä 0 siblings, 1 reply; 8+ messages in thread From: Ander Conselvan De Oliveira @ 2017-03-22 9:51 UTC (permalink / raw) To: Maarten Lankhorst, ville.syrjala, intel-gfx On Wed, 2017-03-22 at 10:01 +0100, Maarten Lankhorst wrote: > Op 14-03-17 om 16:10 schreef ville.syrjala@linux.intel.com: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Use intel_wm_plane_visible() to determine cursor visibility for SKL+ > > also. Previously SKL+ would check the actual visibility which now > > conflicts with the assumptions in intel_legacy_cursor_update(). > > > > We also change SKL+ to compute the cursor watermarks based on the > > unclipped cursor size, just as we do on all the other platforms. > > Using the clipped size could now result in garbage results. > > > > Testcase: igt/kms_chv_cursor_fail > > Fixes: a5509abda48e ("drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW") > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100195 > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > For patch 1 & 2: > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Should be the right way to fix it. :) In intel_legacy_cursor_update(), I see the comment below, /* * If any parameters change that may affect watermarks, * take the slowpath. Only changing fb or position should be * in the fastpath. */ followed by a bunch of checks on the plane size and fb. My understanding is that the bug was caused by those assumptions being out of sync with the actual watermark code. So IMO, a more proper way to fix this would be to have intel_legacy_cursor_update() call into watermark code to ask if it can proceed or not, instead of making assumptions of what can cause watermarks to change. But since the duplicated assumptions were there before, this fix doesn't make the overall situation any worse. Acked-by: Ander Conselvan de Oliveira <conselvan2@gmail.com> _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] drm/i915: Fix SKL cursor watermarks 2017-03-22 9:51 ` Ander Conselvan De Oliveira @ 2017-03-22 20:20 ` Ville Syrjälä 0 siblings, 0 replies; 8+ messages in thread From: Ville Syrjälä @ 2017-03-22 20:20 UTC (permalink / raw) To: Ander Conselvan De Oliveira; +Cc: intel-gfx On Wed, Mar 22, 2017 at 11:51:56AM +0200, Ander Conselvan De Oliveira wrote: > On Wed, 2017-03-22 at 10:01 +0100, Maarten Lankhorst wrote: > > Op 14-03-17 om 16:10 schreef ville.syrjala@linux.intel.com: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Use intel_wm_plane_visible() to determine cursor visibility for SKL+ > > > also. Previously SKL+ would check the actual visibility which now > > > conflicts with the assumptions in intel_legacy_cursor_update(). > > > > > > We also change SKL+ to compute the cursor watermarks based on the > > > unclipped cursor size, just as we do on all the other platforms. > > > Using the clipped size could now result in garbage results. > > > > > > Testcase: igt/kms_chv_cursor_fail > > > Fixes: a5509abda48e ("drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW") > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100195 > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > For patch 1 & 2: > > > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > Should be the right way to fix it. :) > > In intel_legacy_cursor_update(), I see the comment below, > > /* > * If any parameters change that may affect watermarks, > * take the slowpath. Only changing fb or position should be > * in the fastpath. > */ > > followed by a bunch of checks on the plane size and fb. My understanding is that > the bug was caused by those assumptions being out of sync with the actual > watermark code. So IMO, a more proper way to fix this would be to have > intel_legacy_cursor_update() call into watermark code to ask if it can proceed > or not, instead of making assumptions of what can cause watermarks to change. Yeah, we do (at at least used to) have some assumptions about watermarks in other parts of the driver as well. That's potentially something we should fix, probably when someone finally starts doing the s/active/enable/ change for watermarks. > > But since the duplicated assumptions were there before, this fix doesn't make > the overall situation any worse. > > Acked-by: Ander Conselvan de Oliveira <conselvan2@gmail.com> Thank you all. Series pushed to dinq. -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Extract intel_wm_plane_visible() 2017-03-14 15:10 [PATCH 1/2] drm/i915: Extract intel_wm_plane_visible() ville.syrjala 2017-03-14 15:10 ` [PATCH 2/2] drm/i915: Fix SKL cursor watermarks ville.syrjala @ 2017-03-14 20:23 ` Patchwork 2017-03-21 9:58 ` Dorota Czaplejewicz 2 siblings, 0 replies; 8+ messages in thread From: Patchwork @ 2017-03-14 20:23 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx == Series Details == Series: series starting with [1/2] drm/i915: Extract intel_wm_plane_visible() URL : https://patchwork.freedesktop.org/series/21226/ State : success == Summary == Series 21226v1 Series without cover letter https://patchwork.freedesktop.org/api/1.0/series/21226/revisions/1/mbox/ Test gem_exec_flush: Subgroup basic-uc-prw-default: pass -> INCOMPLETE (fi-skl-6700hq) fdo#100130 Test kms_cursor_legacy: Subgroup basic-busy-flip-before-cursor-atomic: fail -> PASS (fi-snb-2520m) fdo#100130 https://bugs.freedesktop.org/show_bug.cgi?id=100130 fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time: 458s fi-bsw-n3050 total:278 pass:239 dwarn:0 dfail:0 fail:0 skip:39 time: 574s fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time: 534s fi-bxt-t5700 total:278 pass:258 dwarn:0 dfail:0 fail:0 skip:20 time: 565s fi-byt-j1900 total:278 pass:251 dwarn:0 dfail:0 fail:0 skip:27 time: 497s fi-byt-n2820 total:278 pass:247 dwarn:0 dfail:0 fail:0 skip:31 time: 501s fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 436s fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 432s fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time: 438s fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 519s fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 505s fi-kbl-7500u total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 487s fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 490s fi-skl-6700hq total:54 pass:46 dwarn:0 dfail:0 fail:0 skip:7 time: 0s fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time: 495s fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 524s fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time: 550s fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time: 421s c641417b70c6b78efca29ae732d7cbf5716ac6d5 drm-tip: 2017y-03m-14d-16h-04m-56s UTC integration manifest 3144fbe drm/i915: Fix SKL cursor watermarks 1b26d8d drm/i915: Extract intel_wm_plane_visible() == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4166/ _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [1/2] drm/i915: Extract intel_wm_plane_visible() 2017-03-14 15:10 [PATCH 1/2] drm/i915: Extract intel_wm_plane_visible() ville.syrjala 2017-03-14 15:10 ` [PATCH 2/2] drm/i915: Fix SKL cursor watermarks ville.syrjala 2017-03-14 20:23 ` ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Extract intel_wm_plane_visible() Patchwork @ 2017-03-21 9:58 ` Dorota Czaplejewicz 2 siblings, 0 replies; 8+ messages in thread From: Dorota Czaplejewicz @ 2017-03-21 9:58 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx I have tested this patch series (with [2/2] drm/i915: Fix SKL cursor watermarks, X-Patchwork-Id: 143965, 143966) and I can confirm it fixes https://bugs.freedesktop.org/show_bug.cgi?id=100195 Tested-by: Dorota Czaplejewicz <dorota.czaplejewicz@collabora.co.uk> On Tue, 14 Mar 2017 17:10:49 +0200 ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > All platforms that lack double buffered watermarks will need to > handle the legacy cursor updates in the same way. So let's extract the > logic to determine the plane visibility into a small helper. For > simplicity we'll make the function DTRT for any plane, but only apply > the special sauce for cursor planes. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 39 +++++++++++++++++++++++++++------------ > 1 file changed, 27 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 2ca38ae4421e..3b0d379b6f38 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -655,6 +655,29 @@ static unsigned long intel_calculate_wm(unsigned long clock_in_khz, > return wm_size; > } > > +static bool intel_wm_plane_visible(const struct intel_crtc_state *crtc_state, > + const struct intel_plane_state *plane_state) > +{ > + struct intel_plane *plane = to_intel_plane(plane_state->base.plane); > + > + /* FIXME check the 'enable' instead */ > + if (!crtc_state->base.active) > + return false; > + > + /* > + * Treat cursor with fb as always visible since cursor updates > + * can happen faster than the vrefresh rate, and the current > + * watermark code doesn't handle that correctly. Cursor updates > + * which set/clear the fb or change the cursor size are going > + * to get throttled by intel_legacy_cursor_update() to work > + * around this problem with the watermark code. > + */ > + if (plane->id == PLANE_CURSOR) > + return plane_state->base.fb != NULL; > + else > + return plane_state->base.visible; > +} > + > static struct intel_crtc *single_enabled_crtc(struct drm_i915_private *dev_priv) > { > struct intel_crtc *crtc, *enabled = NULL; > @@ -1961,7 +1984,7 @@ static uint32_t ilk_compute_pri_wm(const struct intel_crtc_state *cstate, > uint32_t method1, method2; > int cpp; > > - if (!cstate->base.active || !pstate->base.visible) > + if (!intel_wm_plane_visible(cstate, pstate)) > return 0; > > cpp = pstate->base.fb->format->cpp[0]; > @@ -1990,7 +2013,7 @@ static uint32_t ilk_compute_spr_wm(const struct intel_crtc_state *cstate, > uint32_t method1, method2; > int cpp; > > - if (!cstate->base.active || !pstate->base.visible) > + if (!intel_wm_plane_visible(cstate, pstate)) > return 0; > > cpp = pstate->base.fb->format->cpp[0]; > @@ -2013,15 +2036,7 @@ static uint32_t ilk_compute_cur_wm(const struct intel_crtc_state *cstate, > { > int cpp; > > - /* > - * Treat cursor with fb as always visible since cursor updates > - * can happen faster than the vrefresh rate, and the current > - * watermark code doesn't handle that correctly. Cursor updates > - * which set/clear the fb or change the cursor size are going > - * to get throttled by intel_legacy_cursor_update() to work > - * around this problem with the watermark code. > - */ > - if (!cstate->base.active || !pstate->base.fb) > + if (!intel_wm_plane_visible(cstate, pstate)) > return 0; > > cpp = pstate->base.fb->format->cpp[0]; > @@ -2038,7 +2053,7 @@ static uint32_t ilk_compute_fbc_wm(const struct intel_crtc_state *cstate, > { > int cpp; > > - if (!cstate->base.active || !pstate->base.visible) > + if (!intel_wm_plane_visible(cstate, pstate)) > return 0; > > cpp = pstate->base.fb->format->cpp[0]; _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-03-22 20:20 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-03-14 15:10 [PATCH 1/2] drm/i915: Extract intel_wm_plane_visible() ville.syrjala 2017-03-14 15:10 ` [PATCH 2/2] drm/i915: Fix SKL cursor watermarks ville.syrjala 2017-03-15 10:46 ` [2/2] " Tahvanainen, Jari 2017-03-22 9:01 ` [PATCH 2/2] " Maarten Lankhorst 2017-03-22 9:51 ` Ander Conselvan De Oliveira 2017-03-22 20:20 ` Ville Syrjälä 2017-03-14 20:23 ` ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Extract intel_wm_plane_visible() Patchwork 2017-03-21 9:58 ` Dorota Czaplejewicz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).