On Mon, Mar 09, 2015 at 10:19:25AM -0700, 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@xxxxxxxxx> > 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@xxxxxxxxx> > 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. > > 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) > > v3: Fix a couple copy/paste mistakes in SKL code. (Tvrtko) > > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> > Reported-by: Michael Leuchtenburg <michael@xxxxxxxxxxxxx> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89388 > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx> > --- > drivers/gpu/drm/i915/intel_pm.c | 113 +++++++++++++++++++++++++++++----------- > 1 file changed, 82 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index a06a2c7..207c973 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -608,12 +608,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, > @@ -684,7 +689,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; > @@ -771,7 +780,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; > @@ -1118,10 +1131,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 */ > @@ -1195,7 +1213,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; > > @@ -1217,7 +1241,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; > > @@ -1243,7 +1273,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; > } > > @@ -1264,10 +1294,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 */ With the change to intel_crtc_active() to look at state->fb I think you could drop all of the changes above, and just keep the changes below. > @@ -1962,13 +1997,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); > @@ -2734,27 +2781,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; > - 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) > + 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].tiling = DRM_FORMAT_MOD_NONE; > + } > + p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w; > + p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h; > > - 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->cursor->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@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
