drm/i915: Don't assume primary & cursor are always on for wm calculation (v4) (c9f038a1) · Commits · 戴 / test

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@intel.com> Date: Fri Feb 27 10:12:01 2015 -0800 drm/i915: Use plane->state->fb in watermark code (v2) and commit Author: Tvrtko Ursulin <tvrtko.ursulin@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. 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) v4: Only add FB checks for ILK/SKL codepaths. Older platforms still use intel_crtc_active() and will shortcircuit out of watermark calculations before ever trying to dereference the primary plane's framebuffer. Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Reported-by:

Michael Leuchtenburg <michael@slashhome.org> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89388 Signed-off-by:

Matt Roper <matthew.d.roper@intel.com> Reviewed-by:

Tvrtko Ursulin <tvrtko.ursulin@intel.com> Signed-off-by:

Daniel Vetter <daniel.vetter@ffwll.ch>

drivers/gpu/drm/i915/intel_pm.c

+39 −23

Original line number	Diff line number	Diff line
		@@ -2034,13 +2034,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;

		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);
		@@ -2806,27 +2818,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.
		*/
		fb = crtc->primary->state->fb;
		if (fb) {
		p->plane[0].enabled = true;
		p->plane[0].bytes_per_pixel =
		crtc->primary->state->fb->bits_per_pixel / 8;
		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->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];

		fb = crtc->cursor->state->fb;
		if (fb) {
		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;
		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) {

Admin message