[Intel-gfx] [PATCH 3/3] drm/i915: Don't assume primary & cursor are always on for wm calculation (v3)
Matt Roper
matthew.d.roper at intel.com
Mon Mar 9 10:48:08 PDT 2015
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 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)
> >
> > 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 | 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.
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.
Matt
>
> > @@ -1962,13 +1997,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 +2781,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) {
> > --
> > 1.8.5.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
More information about the Intel-gfx
mailing list