On Mon, Mar 09, 2015 at 10:48:08AM -0700, Matt Roper wrote: > 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. intel_crtc_active() will come out as false in that case and we never end up looking at the fb. > > 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. I don't think it's worth looking at the old platform code at this point. We just want the ilk+ stuff converted to atomic first. I suspect once I get the VLV/CHV wm code sorted into a better shape we can leverage it for the other gmch platforms as well. -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
