On Mon, Mar 09, 2015 at 07:44:00PM +0200, Ville Syrjälä wrote: > 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. Are you sure? If intel_crtc->active is true, but the primary plane is disabled (plane->state->fb == NULL), then I think we're still going to wind up trying to dereference plane->state->fb to get bits_per_pixel. In my other branch I do have a patch that just moves all that wm_param calculation out of the watermark code here and sticks it in the check_plane code, so that we always have up-to-date enabled/bpp/etc values collected for the plane state. If I can extract that from my other branch cleanly, that might be a slightly cleaner solution than just adding all the fb tests here. Matt > > > @@ -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 -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
