[Intel-gfx] [PATCH 3/3] drm/i915: Don't assume primary & cursor are always on for wm calculation (v4)
Daniel Vetter
daniel at ffwll.ch
Tue Mar 10 06:09:57 PDT 2015
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.roper at 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.ursulin at 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 at intel.com>
> >Reported-by: Michael Leuchtenburg <michael at slashhome.org>
> >Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89388
> >Signed-off-by: Matt Roper <matthew.d.roper at 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.ursulin at 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
More information about the Intel-gfx
mailing list