| Message ID | 20170314151050.12194-2-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. :)
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>
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.
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);