On Tue, Mar 10, 2015 at 10:51:34AM +0000, Tvrtko Ursulin wrote: > > On 03/09/2015 06:06 PM, 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.ro...@intel.com> > > 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.ursu...@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.ursu...@intel.com> > >Reported-by: Michael Leuchtenburg <mich...@slashhome.org> > >Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89388 > >Signed-off-by: Matt Roper <matthew.d.ro...@intel.com> > >--- > > drivers/gpu/drm/i915/intel_pm.c | 62 > > ++++++++++++++++++++++++++--------------- > > 1 file changed, 39 insertions(+), 23 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/intel_pm.c > >b/drivers/gpu/drm/i915/intel_pm.c > >index a06a2c7..7566cec 100644 > >--- a/drivers/gpu/drm/i915/intel_pm.c > >+++ b/drivers/gpu/drm/i915/intel_pm.c > >@@ -1962,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); > >@@ -2734,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; > >- 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) { > > > > I was nervous about a new possibility of bytes_per_pixel being zero so > looked if someone could divide by it. ilk_wm_fbc can, but is gated by > pri.enabled so that is fine and I didn't find any other similar places. > > Given that, it looks fine to me. > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
Queued for -next, thanks for the patch. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
