On 03/06/2015 01:31 AM, 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)
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 | 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;
Last line should go in the else block above.
- 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;
+ }
There can be an active crtc with no primary plane but cursor then cannot
be active?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
