[Intel-gfx] [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation (v2)
Ville Syrjälä
ville.syrjala at linux.intel.com
Fri Mar 6 04:57:41 PST 2015
On Thu, Mar 05, 2015 at 05:31:18PM -0800, Matt Roper wrote:
> Current ILK-style watermark code assumes the primary plane and cursor
> plane are always enabled. This assumption, along with the combination
> of two independent commits that got merged at the same time, results in
> a NULL dereference. The offending commits are:
>
> commit fd2d61341bf39d1054256c07d6eddd624ebc4241
> Author: Matt Roper <matthew.d.roper at intel.com>
> Date: Fri Feb 27 10:12:01 2015 -0800
>
> drm/i915: Use plane->state->fb in watermark code (v2)
>
> and
>
> commit 0fda65680e92545caea5be7805a7f0a617fb6c20
> Author: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Date: Fri Feb 27 15:12:35 2015 +0000
>
> drm/i915/skl: Update watermarks for Y tiling
>
> The first commit causes us to use the FB from plane->state->fb rather
> than the legacy plane->fb, which is updated a bit later in the process.
>
> The second commit includes a change that now triggers watermark
> reprogramming on primary plane enable/disable where we didn't have one
> before (which wasn't really correct, but we had been getting lucky
> because we always calculated as if the primary plane was on).
>
> Together, these two commits cause the watermark calculation to
> (properly) see plane->state->fb = NULL when we're in the process of
> disabling the primary plane. However the existing watermark code
> assumes there's always a primary fb and tries to dereference it to find
> out pixel format / bpp information.
>
> The fix is to make ILK-style watermark calculation actually check the
> true status of primary & cursor planes and adjust our watermark logic
> accordingly.
I think you need to change intel_crtc_active() to check
primary->state->fb instead of primary->fb as well. Otherwise I'll exepct
some explosions on old platforms.
>
> v2: Update unchecked uses of state->fb for other platforms (pnv, skl,
> etc.). Note that this is just a temporary fix. Ultimately the
> useful information is going to be computed at check time and stored
> right in the state structures so that we don't have to figure this
> all out while we're supposed to be programming the watermarks.
> (caught by Tvrtko)
>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Reported-by: Michael Leuchtenburg <michael at slashhome.org>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89388
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> ---
> drivers/gpu/drm/i915/intel_pm.c | 116 +++++++++++++++++++++++++++++-----------
> 1 file changed, 85 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index e2e8414..09e210c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -553,12 +553,17 @@ static void pineview_update_wm(struct drm_crtc *unused_crtc)
> crtc = single_enabled_crtc(dev);
> if (crtc) {
> const struct drm_display_mode *adjusted_mode;
> - int pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> + int pixel_size;
> int clock;
>
> adjusted_mode = &to_intel_crtc(crtc)->config->base.adjusted_mode;
> clock = adjusted_mode->crtc_clock;
>
> + if (crtc->primary->state->fb)
> + pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> + else
> + pixel_size = 4;
> +
> /* Display SR */
> wm = intel_calculate_wm(clock, &pineview_display_wm,
> pineview_display_wm.fifo_size,
> @@ -629,7 +634,11 @@ static bool g4x_compute_wm0(struct drm_device *dev,
> clock = adjusted_mode->crtc_clock;
> htotal = adjusted_mode->crtc_htotal;
> hdisplay = to_intel_crtc(crtc)->config->pipe_src_w;
> - pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> +
> + if (crtc->primary->state->fb)
> + pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> + else
> + pixel_size = 4;
>
> /* Use the small buffer method to calculate plane watermark */
> entries = ((clock * pixel_size / 1000) * display_latency_ns) / 1000;
> @@ -716,7 +725,11 @@ static bool g4x_compute_srwm(struct drm_device *dev,
> clock = adjusted_mode->crtc_clock;
> htotal = adjusted_mode->crtc_htotal;
> hdisplay = to_intel_crtc(crtc)->config->pipe_src_w;
> - pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> +
> + if (crtc->primary->state->fb)
> + pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> + else
> + pixel_size = 4;
>
> line_time_us = max(htotal * 1000 / clock, 1);
> line_count = (latency_ns / line_time_us + 1000) / 1000;
> @@ -799,7 +812,10 @@ static void vlv_update_drain_latency(struct drm_crtc *crtc)
> }
>
> /* Primary plane Drain Latency */
> - pixel_size = crtc->primary->state->fb->bits_per_pixel / 8; /* BPP */
> + if (crtc->primary->state->fb)
> + pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> + else
> + pixel_size = 4;
> if (vlv_compute_drain_latency(crtc, pixel_size, &prec_mult, &drain_latency)) {
> plane_prec = (prec_mult == high_precision) ?
> DDL_PLANE_PRECISION_HIGH :
> @@ -1080,10 +1096,15 @@ static void i965_update_wm(struct drm_crtc *unused_crtc)
> int clock = adjusted_mode->crtc_clock;
> int htotal = adjusted_mode->crtc_htotal;
> int hdisplay = to_intel_crtc(crtc)->config->pipe_src_w;
> - int pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> + int pixel_size;
> unsigned long line_time_us;
> int entries;
>
> + if (crtc->primary->state->fb)
> + pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> + else
> + pixel_size = 4;
> +
> line_time_us = max(htotal * 1000 / clock, 1);
>
> /* Use ns/us then divide to preserve precision */
> @@ -1157,7 +1178,13 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
> crtc = intel_get_crtc_for_plane(dev, 0);
> if (intel_crtc_active(crtc)) {
> const struct drm_display_mode *adjusted_mode;
> - int cpp = crtc->primary->state->fb->bits_per_pixel / 8;
> + int cpp;
> +
> + if (crtc->primary->state->fb)
> + cpp = crtc->primary->state->fb->bits_per_pixel / 8;
> + else
> + cpp = 4;
> +
> if (IS_GEN2(dev))
> cpp = 4;
>
> @@ -1179,7 +1206,13 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
> crtc = intel_get_crtc_for_plane(dev, 1);
> if (intel_crtc_active(crtc)) {
> const struct drm_display_mode *adjusted_mode;
> - int cpp = crtc->primary->state->fb->bits_per_pixel / 8;
> + int cpp;
> +
> + if (crtc->primary->state->fb)
> + cpp = crtc->primary->state->fb->bits_per_pixel / 8;
> + else
> + cpp = 4;
> +
> if (IS_GEN2(dev))
> cpp = 4;
>
> @@ -1205,7 +1238,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
> obj = intel_fb_obj(enabled->primary->state->fb);
>
> /* self-refresh seems busted with untiled */
> - if (obj->tiling_mode == I915_TILING_NONE)
> + if (!obj || obj->tiling_mode == I915_TILING_NONE)
> enabled = NULL;
> }
>
> @@ -1226,10 +1259,15 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
> int clock = adjusted_mode->crtc_clock;
> int htotal = adjusted_mode->crtc_htotal;
> int hdisplay = to_intel_crtc(enabled)->config->pipe_src_w;
> - int pixel_size = enabled->primary->state->fb->bits_per_pixel / 8;
> + int pixel_size;
> unsigned long line_time_us;
> int entries;
>
> + if (enabled->primary->state->fb)
> + pixel_size = enabled->primary->state->fb->bits_per_pixel / 8;
> + else
> + pixel_size = 4;
> +
> line_time_us = max(htotal * 1000 / clock, 1);
>
> /* Use ns/us then divide to preserve precision */
> @@ -1924,13 +1962,25 @@ static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
> p->active = true;
> p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
> p->pixel_rate = ilk_pipe_pixel_rate(dev, crtc);
> - p->pri.bytes_per_pixel = crtc->primary->state->fb->bits_per_pixel / 8;
> - p->cur.bytes_per_pixel = 4;
> +
> + if (crtc->primary->state->fb) {
> + p->pri.enabled = true;
> + p->pri.bytes_per_pixel =
> + crtc->primary->state->fb->bits_per_pixel / 8;
> + } else {
> + p->pri.enabled = false;
> + p->pri.bytes_per_pixel = 0;
> + }
> +
> + if (crtc->cursor->state->fb) {
> + p->cur.enabled = true;
> + p->cur.bytes_per_pixel = 4;
> + } else {
> + p->cur.enabled = false;
> + p->cur.bytes_per_pixel = 0;
> + }
> p->pri.horiz_pixels = intel_crtc->config->pipe_src_w;
> p->cur.horiz_pixels = intel_crtc->base.cursor->state->crtc_w;
> - /* TODO: for now, assume primary and cursor planes are always enabled. */
> - p->pri.enabled = true;
> - p->cur.enabled = true;
>
> drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
> struct intel_plane *intel_plane = to_intel_plane(plane);
> @@ -2696,27 +2746,31 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
> p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
> p->pixel_rate = skl_pipe_pixel_rate(intel_crtc->config);
>
> - /*
> - * For now, assume primary and cursor planes are always enabled.
> - */
> - p->plane[0].enabled = true;
> - p->plane[0].bytes_per_pixel =
> - crtc->primary->state->fb->bits_per_pixel / 8;
> + fb = crtc->primary->state->fb;
> + if (fb) {
> + p->plane[0].enabled = true;
> + p->plane[0].bytes_per_pixel = fb->bits_per_pixel / 8;
> + p->plane[0].tiling = fb->modifier[0];
> + } else {
> + p->plane[0].enabled = false;
> + p->plane[0].bytes_per_pixel = 0;
> + }
> p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
> p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
> p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
> - fb = crtc->primary->state->fb;
> - /*
> - * Framebuffer can be NULL on plane disable, but it does not
> - * matter for watermarks if we assume no tiling in that case.
> - */
> - if (fb)
> - p->plane[0].tiling = fb->modifier[0];
>
> - p->cursor.enabled = true;
> - p->cursor.bytes_per_pixel = 4;
> - p->cursor.horiz_pixels = intel_crtc->base.cursor->state->crtc_w ?
> - intel_crtc->base.cursor->state->crtc_w : 64;
> + fb = crtc->primary->state->fb;
> + if (fb) {
> + p->cursor.enabled = true;
> + p->cursor.bytes_per_pixel = fb->bits_per_pixel / 8;
> + p->cursor.horiz_pixels = crtc->cursor->state->crtc_w;
> + p->cursor.vert_pixels = crtc->cursor->state->crtc_h;
> + } else {
> + p->cursor.enabled = false;
> + p->cursor.bytes_per_pixel = 0;
> + p->cursor.horiz_pixels = 64;
> + p->cursor.vert_pixels = 64;
> + }
> }
>
> list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> --
> 1.8.5.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list