* [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active() @ 2015-03-04 2:15 Matt Roper 2015-03-04 2:15 ` [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation Matt Roper ` (3 more replies) 0 siblings, 4 replies; 18+ messages in thread From: Matt Roper @ 2015-03-04 2:15 UTC (permalink / raw) To: intel-gfx Universal planes allow us to have an active CRTC without a primary plane framebuffer bound. Drop the test for primary->fb from intel_crtc_active() since we can clearly have active CRTC's without a framebuffer, and this check is now interfering with watermark calculations when we try to re-enable the primary plane. Note that commit commit 0fda65680e92545caea5be7805a7f0a617fb6c20 Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Date: Fri Feb 27 15:12:35 2015 +0000 drm/i915/skl: Update watermarks for Y tiling adds a test for primary plane enable/disable to trigger a watermark update (previously we ignored updates to primary planes, which wasn't really correct, but we got lucky since we always pretended the primary plane was on). Tvrtko's patch tries to update watermarks when we re-enable the primary plane, but that watermark computation gets aborted early because intel_crtc_active() always returns false when the primary plane is disabled; this leads to watermark underruns (at least on platforms with ILK-style watermark code). Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 589addf..92cb2ff 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -893,11 +893,8 @@ bool intel_crtc_active(struct drm_crtc *crtc) * * We can ditch the adjusted_mode.crtc_clock check as soon * as Haswell has gained clock readout/fastboot support. - * - * We can ditch the crtc->primary->fb check as soon as we can - * properly reconstruct framebuffers. */ - return intel_crtc->active && crtc->primary->fb && + return intel_crtc->active && intel_crtc->config->base.adjusted_mode.crtc_clock; } -- 1.8.5.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation 2015-03-04 2:15 [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active() Matt Roper @ 2015-03-04 2:15 ` Matt Roper 2015-03-04 8:18 ` Jani Nikula ` (2 more replies) 2015-03-04 9:45 ` [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active() Ville Syrjälä ` (2 subsequent siblings) 3 siblings, 3 replies; 18+ messages in thread From: Matt Roper @ 2015-03-04 2:15 UTC (permalink / raw) To: intel-gfx; +Cc: Michael Leuchtenburg 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 0fda65680e92545caea5be7805a7f0a617fb6c20 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. Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: 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> --- drivers/gpu/drm/i915/intel_pm.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index e710b43..93fd15f 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -1924,13 +1924,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); -- 1.8.5.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation 2015-03-04 2:15 ` [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation Matt Roper @ 2015-03-04 8:18 ` Jani Nikula 2015-03-04 16:17 ` shuang.he 2015-03-04 17:29 ` Tvrtko Ursulin 2 siblings, 0 replies; 18+ messages in thread From: Jani Nikula @ 2015-03-04 8:18 UTC (permalink / raw) To: Matt Roper, intel-gfx; +Cc: Michael Leuchtenburg On Wed, 04 Mar 2015, Matt Roper <matthew.d.roper@intel.com> 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@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@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. > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: 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> Tested-by: lu hua <huax.lu@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 22 +++++++++++++++++----- > 1 file changed, 17 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index e710b43..93fd15f 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -1924,13 +1924,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); > -- > 1.8.5.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation 2015-03-04 2:15 ` [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation Matt Roper 2015-03-04 8:18 ` Jani Nikula @ 2015-03-04 16:17 ` shuang.he 2015-03-04 17:29 ` Tvrtko Ursulin 2 siblings, 0 replies; 18+ messages in thread From: shuang.he @ 2015-03-04 16:17 UTC (permalink / raw) To: shuang.he, ethan.gao, intel-gfx, matthew.d.roper Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com) Task id: 5883 -------------------------------------Summary------------------------------------- Platform Delta drm-intel-nightly Series Applied PNV -8 278/278 270/278 ILK 308/308 308/308 SNB -1 284/284 283/284 IVB 380/380 380/380 BYT 294/294 294/294 HSW 387/387 387/387 BDW -1 316/316 315/316 -------------------------------------Detailed------------------------------------- Platform Test drm-intel-nightly Series Applied PNV igt_gem_userptr_blits_coherency-sync NO_RESULT(1)CRASH(8)NRUN(1)PASS(8) CRASH(1)PASS(1) PNV igt_gem_userptr_blits_coherency-unsync FAIL(1)NO_RESULT(1)CRASH(6)PASS(7) CRASH(1)PASS(1) *PNV igt_gem_userptr_blits_forbidden-operations PASS(2) NRUN(1)PASS(1) *PNV igt_gem_userptr_blits_forked-access PASS(2) NRUN(1)PASS(1) *PNV igt_gem_userptr_blits_forked-sync-interruptible PASS(2) NRUN(1)PASS(1) PNV igt_gen3_render_linear_blits FAIL(6)NRUN(1)DMESG_WARN(1)PASS(9) FAIL(2) PNV igt_gen3_render_mixed_blits FAIL(10)PASS(9) FAIL(2) PNV igt_gem_fence_thrash_bo-write-verify-threaded-none FAIL(2)CRASH(5)PASS(7) CRASH(2) *SNB igt_gem_flink_bad-flink PASS(3) DMESG_WARN(1)PASS(1) *BDW igt_gem_gtt_hog PASS(19) DMESG_WARN(1)PASS(1) Note: You need to pay more attention to line start with '*' _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation 2015-03-04 2:15 ` [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation Matt Roper 2015-03-04 8:18 ` Jani Nikula 2015-03-04 16:17 ` shuang.he @ 2015-03-04 17:29 ` Tvrtko Ursulin 2015-03-06 1:31 ` [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation (v2) Matt Roper 2 siblings, 1 reply; 18+ messages in thread From: Tvrtko Ursulin @ 2015-03-04 17:29 UTC (permalink / raw) To: Matt Roper, intel-gfx; +Cc: Michael Leuchtenburg On 03/04/2015 02:15 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@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@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. > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: 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> > --- > drivers/gpu/drm/i915/intel_pm.c | 22 +++++++++++++++++----- > 1 file changed, 17 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index e710b43..93fd15f 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -1924,13 +1924,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); > Hm there are other place which dereference crtc->primary->state->fb, like i9xx_update_wm and skl_compute_wm_pipe_parameters - they won't suffer the same problem? Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation (v2) 2015-03-04 17:29 ` Tvrtko Ursulin @ 2015-03-06 1:31 ` Matt Roper 2015-03-06 9:58 ` Tvrtko Ursulin 2015-03-06 12:57 ` Ville Syrjälä 0 siblings, 2 replies; 18+ messages in thread From: Matt Roper @ 2015-03-06 1:31 UTC (permalink / raw) To: intel-gfx 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 0fda65680e92545caea5be7805a7f0a617fb6c20 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) 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> --- 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; - 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; + } } list_for_each_entry(plane, &dev->mode_config.plane_list, head) { -- 1.8.5.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation (v2) 2015-03-06 1:31 ` [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation (v2) Matt Roper @ 2015-03-06 9:58 ` Tvrtko Ursulin 2015-03-06 12:57 ` Ville Syrjälä 1 sibling, 0 replies; 18+ messages in thread From: Tvrtko Ursulin @ 2015-03-06 9:58 UTC (permalink / raw) To: Matt Roper, intel-gfx 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@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@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) > > 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> > --- > 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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation (v2) 2015-03-06 1:31 ` [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation (v2) Matt Roper 2015-03-06 9:58 ` Tvrtko Ursulin @ 2015-03-06 12:57 ` Ville Syrjälä 1 sibling, 0 replies; 18+ messages in thread From: Ville Syrjälä @ 2015-03-06 12:57 UTC (permalink / raw) To: Matt Roper; +Cc: intel-gfx On Thu, Mar 05, 2015 at 05:31:18PM -0800, 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@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@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. I think you need to change intel_crtc_active() to check primary->state->fb instead of primary->fb as well. Otherwise I'll exepct some explosions on old platforms. > > 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@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> > --- > 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; > - 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; > + } > } > > list_for_each_entry(plane, &dev->mode_config.plane_list, head) { > -- > 1.8.5.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active() 2015-03-04 2:15 [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active() Matt Roper 2015-03-04 2:15 ` [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation Matt Roper @ 2015-03-04 9:45 ` Ville Syrjälä 2015-03-04 16:13 ` Daniel Vetter 2015-03-04 16:14 ` Daniel Vetter 2015-03-04 17:26 ` Tvrtko Ursulin 3 siblings, 1 reply; 18+ messages in thread From: Ville Syrjälä @ 2015-03-04 9:45 UTC (permalink / raw) To: Matt Roper; +Cc: intel-gfx On Tue, Mar 03, 2015 at 06:15:12PM -0800, Matt Roper wrote: > Universal planes allow us to have an active CRTC without a primary plane > framebuffer bound. Drop the test for primary->fb from > intel_crtc_active() since we can clearly have active CRTC's without a > framebuffer, and this check is now interfering with watermark > calculations when we try to re-enable the primary plane. > > Note that commit > > commit 0fda65680e92545caea5be7805a7f0a617fb6c20 > Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Date: Fri Feb 27 15:12:35 2015 +0000 > > drm/i915/skl: Update watermarks for Y tiling > > adds a test for primary plane enable/disable to trigger a watermark > update (previously we ignored updates to primary planes, which wasn't > really correct, but we got lucky since we always pretended the primary > plane was on). Tvrtko's patch tries to update watermarks when we > re-enable the primary plane, but that watermark computation gets aborted > early because intel_crtc_active() always returns false when the primary > plane is disabled; this leads to watermark underruns (at least on > platforms with ILK-style watermark code). I think this will make a bunch of the old platforms blow up. Well, I think they might already blow up since they now look at crtc->state->fb based on the result of intel_crtc_active(). So I believe more fixes are needed all over the wm code at least. In fact looking at the code, I think most (or all?) of the call sites in the in the ilk+ wm code should just be using crtc->active. So for some extra safety it might make sense to do leave intel_crtc_active() alone for now, or in fact we should maybe change it to also look at primary->state->fb instead. That should more or less keep the old platforms in the same state as they were before, while letting new platforms handle each plane properly. > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 589addf..92cb2ff 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -893,11 +893,8 @@ bool intel_crtc_active(struct drm_crtc *crtc) > * > * We can ditch the adjusted_mode.crtc_clock check as soon > * as Haswell has gained clock readout/fastboot support. > - * > - * We can ditch the crtc->primary->fb check as soon as we can > - * properly reconstruct framebuffers. > */ > - return intel_crtc->active && crtc->primary->fb && > + return intel_crtc->active && > intel_crtc->config->base.adjusted_mode.crtc_clock; > } > > -- > 1.8.5.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active() 2015-03-04 9:45 ` [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active() Ville Syrjälä @ 2015-03-04 16:13 ` Daniel Vetter 2015-03-04 18:21 ` Ville Syrjälä 0 siblings, 1 reply; 18+ messages in thread From: Daniel Vetter @ 2015-03-04 16:13 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Wed, Mar 04, 2015 at 11:45:42AM +0200, Ville Syrjälä wrote: > On Tue, Mar 03, 2015 at 06:15:12PM -0800, Matt Roper wrote: > > Universal planes allow us to have an active CRTC without a primary plane > > framebuffer bound. Drop the test for primary->fb from > > intel_crtc_active() since we can clearly have active CRTC's without a > > framebuffer, and this check is now interfering with watermark > > calculations when we try to re-enable the primary plane. > > > > Note that commit > > > > commit 0fda65680e92545caea5be7805a7f0a617fb6c20 > > Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Date: Fri Feb 27 15:12:35 2015 +0000 > > > > drm/i915/skl: Update watermarks for Y tiling > > > > adds a test for primary plane enable/disable to trigger a watermark > > update (previously we ignored updates to primary planes, which wasn't > > really correct, but we got lucky since we always pretended the primary > > plane was on). Tvrtko's patch tries to update watermarks when we > > re-enable the primary plane, but that watermark computation gets aborted > > early because intel_crtc_active() always returns false when the primary > > plane is disabled; this leads to watermark underruns (at least on > > platforms with ILK-style watermark code). > > I think this will make a bunch of the old platforms blow up. Well, I > think they might already blow up since they now look at crtc->state->fb > based on the result of intel_crtc_active(). So I believe more fixes are > needed all over the wm code at least. > > In fact looking at the code, I think most (or all?) of the call sites in > the in the ilk+ wm code should just be using crtc->active. So for some > extra safety it might make sense to do leave intel_crtc_active() alone > for now, or in fact we should maybe change it to also look at > primary->state->fb instead. That should more or less keep the old > platforms in the same state as they were before, while letting new > platforms handle each plane properly. intel_crtc->active shouldn't be used anywhere in state computation code. The only thing you're allowed to look at is crtc_state->enable in the atomic world. Otherwise resource allocation isn't properly done when the pipe is in dpms off state and then stuff blows up when userspace tries to dpms on. Which is a big no-no, at least for proper backwards compat. -Daniel > > > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 5 +---- > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 589addf..92cb2ff 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -893,11 +893,8 @@ bool intel_crtc_active(struct drm_crtc *crtc) > > * > > * We can ditch the adjusted_mode.crtc_clock check as soon > > * as Haswell has gained clock readout/fastboot support. > > - * > > - * We can ditch the crtc->primary->fb check as soon as we can > > - * properly reconstruct framebuffers. > > */ > > - return intel_crtc->active && crtc->primary->fb && > > + return intel_crtc->active && > > intel_crtc->config->base.adjusted_mode.crtc_clock; > > } > > > > -- > > 1.8.5.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active() 2015-03-04 16:13 ` Daniel Vetter @ 2015-03-04 18:21 ` Ville Syrjälä 2015-03-05 12:20 ` Daniel Vetter 0 siblings, 1 reply; 18+ messages in thread From: Ville Syrjälä @ 2015-03-04 18:21 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx On Wed, Mar 04, 2015 at 05:13:07PM +0100, Daniel Vetter wrote: > On Wed, Mar 04, 2015 at 11:45:42AM +0200, Ville Syrjälä wrote: > > On Tue, Mar 03, 2015 at 06:15:12PM -0800, Matt Roper wrote: > > > Universal planes allow us to have an active CRTC without a primary plane > > > framebuffer bound. Drop the test for primary->fb from > > > intel_crtc_active() since we can clearly have active CRTC's without a > > > framebuffer, and this check is now interfering with watermark > > > calculations when we try to re-enable the primary plane. > > > > > > Note that commit > > > > > > commit 0fda65680e92545caea5be7805a7f0a617fb6c20 > > > Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > Date: Fri Feb 27 15:12:35 2015 +0000 > > > > > > drm/i915/skl: Update watermarks for Y tiling > > > > > > adds a test for primary plane enable/disable to trigger a watermark > > > update (previously we ignored updates to primary planes, which wasn't > > > really correct, but we got lucky since we always pretended the primary > > > plane was on). Tvrtko's patch tries to update watermarks when we > > > re-enable the primary plane, but that watermark computation gets aborted > > > early because intel_crtc_active() always returns false when the primary > > > plane is disabled; this leads to watermark underruns (at least on > > > platforms with ILK-style watermark code). > > > > I think this will make a bunch of the old platforms blow up. Well, I > > think they might already blow up since they now look at crtc->state->fb > > based on the result of intel_crtc_active(). So I believe more fixes are > > needed all over the wm code at least. > > > > In fact looking at the code, I think most (or all?) of the call sites in > > the in the ilk+ wm code should just be using crtc->active. So for some > > extra safety it might make sense to do leave intel_crtc_active() alone > > for now, or in fact we should maybe change it to also look at > > primary->state->fb instead. That should more or less keep the old > > platforms in the same state as they were before, while letting new > > platforms handle each plane properly. > > intel_crtc->active shouldn't be used anywhere in state computation code. > The only thing you're allowed to look at is crtc_state->enable in the > atomic world. We'll want crtc->active in a bunch of places in the ilk+ wm code since it's actually intersted in the current state, not the future state. The way it should work (after getting my remaining ilk+ wm patches reworked+merged): 1. Compute new watermarks for this specific pipe using the future plane/crtc state. We don't consider at all how many pipes will be active or are active currently, as those are not variables in our actual watermark equations. 2. Fail the operation if LP0 comes out invalid. LP0 limits do not depend on the number of active pipes, so a failure to meet the LP0 limits is always fatal. LP1+ limits we can ignore at this stage since LP1+ are optional. 3. Merge the watermarks computed at step 1 with the current watermarks on the pipe and store the result as the current watermarks on this pipe. These are what I called "intermediate watermarks" and are needed due to lack of double buffered watermark registers. 4. Merge the current watermarks from all currently active pipes, applying whatever extra limits are imposed by the number of active pipes (eg. disallow LP1+ on ilk/snb/ivb with multiple pipes) and write the result to the hardware registers 5. Commit the plane/crtc state 6. Wait until the hardware plane/crtc state has changed (ie. until the next vblank) 7. Store the watermarks computed at step 1 as the current watermarks of the pipe, replacing the "intermediate watermarks" from step 3 8. <same as step 4> So step 1 is pretty much the only place where we should consider the future state as opposed to the current state of the hardware. -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active() 2015-03-04 18:21 ` Ville Syrjälä @ 2015-03-05 12:20 ` Daniel Vetter 2015-03-05 12:48 ` Ville Syrjälä 0 siblings, 1 reply; 18+ messages in thread From: Daniel Vetter @ 2015-03-05 12:20 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Wed, Mar 04, 2015 at 08:21:16PM +0200, Ville Syrjälä wrote: > On Wed, Mar 04, 2015 at 05:13:07PM +0100, Daniel Vetter wrote: > > On Wed, Mar 04, 2015 at 11:45:42AM +0200, Ville Syrjälä wrote: > > > On Tue, Mar 03, 2015 at 06:15:12PM -0800, Matt Roper wrote: > > > > Universal planes allow us to have an active CRTC without a primary plane > > > > framebuffer bound. Drop the test for primary->fb from > > > > intel_crtc_active() since we can clearly have active CRTC's without a > > > > framebuffer, and this check is now interfering with watermark > > > > calculations when we try to re-enable the primary plane. > > > > > > > > Note that commit > > > > > > > > commit 0fda65680e92545caea5be7805a7f0a617fb6c20 > > > > Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > Date: Fri Feb 27 15:12:35 2015 +0000 > > > > > > > > drm/i915/skl: Update watermarks for Y tiling > > > > > > > > adds a test for primary plane enable/disable to trigger a watermark > > > > update (previously we ignored updates to primary planes, which wasn't > > > > really correct, but we got lucky since we always pretended the primary > > > > plane was on). Tvrtko's patch tries to update watermarks when we > > > > re-enable the primary plane, but that watermark computation gets aborted > > > > early because intel_crtc_active() always returns false when the primary > > > > plane is disabled; this leads to watermark underruns (at least on > > > > platforms with ILK-style watermark code). > > > > > > I think this will make a bunch of the old platforms blow up. Well, I > > > think they might already blow up since they now look at crtc->state->fb > > > based on the result of intel_crtc_active(). So I believe more fixes are > > > needed all over the wm code at least. > > > > > > In fact looking at the code, I think most (or all?) of the call sites in > > > the in the ilk+ wm code should just be using crtc->active. So for some > > > extra safety it might make sense to do leave intel_crtc_active() alone > > > for now, or in fact we should maybe change it to also look at > > > primary->state->fb instead. That should more or less keep the old > > > platforms in the same state as they were before, while letting new > > > platforms handle each plane properly. > > > > intel_crtc->active shouldn't be used anywhere in state computation code. > > The only thing you're allowed to look at is crtc_state->enable in the > > atomic world. > > We'll want crtc->active in a bunch of places in the ilk+ wm code > since it's actually intersted in the current state, not the future > state. > > The way it should work (after getting my remaining ilk+ wm patches > reworked+merged): > > 1. Compute new watermarks for this specific pipe using the future plane/crtc state. > We don't consider at all how many pipes will be active or are active currently, > as those are not variables in our actual watermark equations. > 2. Fail the operation if LP0 comes out invalid. LP0 limits do not depend > on the number of active pipes, so a failure to meet the LP0 limits is > always fatal. LP1+ limits we can ignore at this stage since LP1+ are > optional. > 3. Merge the watermarks computed at step 1 with the current watermarks > on the pipe and store the result as the current watermarks on this pipe. > These are what I called "intermediate watermarks" and are needed due > to lack of double buffered watermark registers. > 4. Merge the current watermarks from all currently active pipes, applying > whatever extra limits are imposed by the number of active pipes (eg. > disallow LP1+ on ilk/snb/ivb with multiple pipes) and write the result > to the hardware registers > 5. Commit the plane/crtc state > 6. Wait until the hardware plane/crtc state has changed (ie. until the > next vblank) > 7. Store the watermarks computed at step 1 as the current watermarks > of the pipe, replacing the "intermediate watermarks" from step 3 > 8. <same as step 4> > > So step 1 is pretty much the only place where we should consider the > future state as opposed to the current state of the hardware. Yeah that's a valid use-case, but you instead want to look at crtc_state->active. intel_crtc->active will just be a consistency check in the end really. And since crtc_state->active is set correctly even in the check phase of the modeset we could compute the final watermarks right away. intel_crtc->active is only set while committing the state to hw. This is important from a locking pov since state objects must be invariant once committed to foo->state pointers. So you'd end up with some interesting book-keeping problems I expect. We'll see. -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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active() 2015-03-05 12:20 ` Daniel Vetter @ 2015-03-05 12:48 ` Ville Syrjälä 0 siblings, 0 replies; 18+ messages in thread From: Ville Syrjälä @ 2015-03-05 12:48 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx On Thu, Mar 05, 2015 at 01:20:17PM +0100, Daniel Vetter wrote: > On Wed, Mar 04, 2015 at 08:21:16PM +0200, Ville Syrjälä wrote: > > On Wed, Mar 04, 2015 at 05:13:07PM +0100, Daniel Vetter wrote: > > > On Wed, Mar 04, 2015 at 11:45:42AM +0200, Ville Syrjälä wrote: > > > > On Tue, Mar 03, 2015 at 06:15:12PM -0800, Matt Roper wrote: > > > > > Universal planes allow us to have an active CRTC without a primary plane > > > > > framebuffer bound. Drop the test for primary->fb from > > > > > intel_crtc_active() since we can clearly have active CRTC's without a > > > > > framebuffer, and this check is now interfering with watermark > > > > > calculations when we try to re-enable the primary plane. > > > > > > > > > > Note that commit > > > > > > > > > > commit 0fda65680e92545caea5be7805a7f0a617fb6c20 > > > > > Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > > Date: Fri Feb 27 15:12:35 2015 +0000 > > > > > > > > > > drm/i915/skl: Update watermarks for Y tiling > > > > > > > > > > adds a test for primary plane enable/disable to trigger a watermark > > > > > update (previously we ignored updates to primary planes, which wasn't > > > > > really correct, but we got lucky since we always pretended the primary > > > > > plane was on). Tvrtko's patch tries to update watermarks when we > > > > > re-enable the primary plane, but that watermark computation gets aborted > > > > > early because intel_crtc_active() always returns false when the primary > > > > > plane is disabled; this leads to watermark underruns (at least on > > > > > platforms with ILK-style watermark code). > > > > > > > > I think this will make a bunch of the old platforms blow up. Well, I > > > > think they might already blow up since they now look at crtc->state->fb > > > > based on the result of intel_crtc_active(). So I believe more fixes are > > > > needed all over the wm code at least. > > > > > > > > In fact looking at the code, I think most (or all?) of the call sites in > > > > the in the ilk+ wm code should just be using crtc->active. So for some > > > > extra safety it might make sense to do leave intel_crtc_active() alone > > > > for now, or in fact we should maybe change it to also look at > > > > primary->state->fb instead. That should more or less keep the old > > > > platforms in the same state as they were before, while letting new > > > > platforms handle each plane properly. > > > > > > intel_crtc->active shouldn't be used anywhere in state computation code. > > > The only thing you're allowed to look at is crtc_state->enable in the > > > atomic world. > > > > We'll want crtc->active in a bunch of places in the ilk+ wm code > > since it's actually intersted in the current state, not the future > > state. > > > > The way it should work (after getting my remaining ilk+ wm patches > > reworked+merged): > > > > 1. Compute new watermarks for this specific pipe using the future plane/crtc state. > > We don't consider at all how many pipes will be active or are active currently, > > as those are not variables in our actual watermark equations. > > 2. Fail the operation if LP0 comes out invalid. LP0 limits do not depend > > on the number of active pipes, so a failure to meet the LP0 limits is > > always fatal. LP1+ limits we can ignore at this stage since LP1+ are > > optional. > > 3. Merge the watermarks computed at step 1 with the current watermarks > > on the pipe and store the result as the current watermarks on this pipe. > > These are what I called "intermediate watermarks" and are needed due > > to lack of double buffered watermark registers. > > 4. Merge the current watermarks from all currently active pipes, applying > > whatever extra limits are imposed by the number of active pipes (eg. > > disallow LP1+ on ilk/snb/ivb with multiple pipes) and write the result > > to the hardware registers > > 5. Commit the plane/crtc state > > 6. Wait until the hardware plane/crtc state has changed (ie. until the > > next vblank) > > 7. Store the watermarks computed at step 1 as the current watermarks > > of the pipe, replacing the "intermediate watermarks" from step 3 > > 8. <same as step 4> > > > > So step 1 is pretty much the only place where we should consider the > > future state as opposed to the current state of the hardware. > > Yeah that's a valid use-case, but you instead want to look at > crtc_state->active. intel_crtc->active will just be a consistency check in > the end really. And since crtc_state->active is set correctly even in the > check phase of the modeset we could compute the final watermarks right > away. During crtc enable/disable we may need to do a watermark updates like so: enable: 1. active=true 2. update watermarks 3. enable pipe disable: 1. disable pipe 2. active=false 3. update watermarks So if crtc_state->active works for that then we can use it. But I think we still want to keep doing what I listed since we don't want to go recomputing the watermarks for every pipe when only one pipe needs updating. For one, that would involve taking locks on all the crtcs/planes which sounds like something to avoid. There are also other factors besides the number of active pipes affecting how we merge the watermarks so I can't accept any "but all locks are held at modeset" excuse here :) If we recompute the watermarks only for the specific pipe we're changing we already have the required locks. Any shared wm state will be protected by dev_priv->wm.mutex. > intel_crtc->active is only set while committing the state to hw. > > This is important from a locking pov since state objects must be invariant > once committed to foo->state pointers. So you'd end up with some > interesting book-keeping problems I expect. We'll see. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active() 2015-03-04 2:15 [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active() Matt Roper 2015-03-04 2:15 ` [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation Matt Roper 2015-03-04 9:45 ` [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active() Ville Syrjälä @ 2015-03-04 16:14 ` Daniel Vetter 2015-03-04 17:26 ` Tvrtko Ursulin 3 siblings, 0 replies; 18+ messages in thread From: Daniel Vetter @ 2015-03-04 16:14 UTC (permalink / raw) To: Matt Roper; +Cc: intel-gfx On Tue, Mar 03, 2015 at 06:15:12PM -0800, Matt Roper wrote: > Universal planes allow us to have an active CRTC without a primary plane > framebuffer bound. Drop the test for primary->fb from > intel_crtc_active() since we can clearly have active CRTC's without a > framebuffer, and this check is now interfering with watermark > calculations when we try to re-enable the primary plane. > > Note that commit > > commit 0fda65680e92545caea5be7805a7f0a617fb6c20 > Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Date: Fri Feb 27 15:12:35 2015 +0000 > > drm/i915/skl: Update watermarks for Y tiling > > adds a test for primary plane enable/disable to trigger a watermark > update (previously we ignored updates to primary planes, which wasn't > really correct, but we got lucky since we always pretended the primary > plane was on). Tvrtko's patch tries to update watermarks when we > re-enable the primary plane, but that watermark computation gets aborted > early because intel_crtc_active() always returns false when the primary > plane is disabled; this leads to watermark underruns (at least on > platforms with ILK-style watermark code). That pretty much means the igt test coverage isn't exhaustive enough and doesn't properly check that the right stuff shows up on the screens with CRCs. Tvrtko can you please add relevant tests for this scenario? Thanks, Daniel > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 589addf..92cb2ff 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -893,11 +893,8 @@ bool intel_crtc_active(struct drm_crtc *crtc) > * > * We can ditch the adjusted_mode.crtc_clock check as soon > * as Haswell has gained clock readout/fastboot support. > - * > - * We can ditch the crtc->primary->fb check as soon as we can > - * properly reconstruct framebuffers. > */ > - return intel_crtc->active && crtc->primary->fb && > + return intel_crtc->active && > intel_crtc->config->base.adjusted_mode.crtc_clock; > } > > -- > 1.8.5.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active() 2015-03-04 2:15 [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active() Matt Roper ` (2 preceding siblings ...) 2015-03-04 16:14 ` Daniel Vetter @ 2015-03-04 17:26 ` Tvrtko Ursulin 2015-03-04 17:42 ` Matt Roper 3 siblings, 1 reply; 18+ messages in thread From: Tvrtko Ursulin @ 2015-03-04 17:26 UTC (permalink / raw) To: Matt Roper, intel-gfx On 03/04/2015 02:15 AM, Matt Roper wrote: > Universal planes allow us to have an active CRTC without a primary plane > framebuffer bound. Drop the test for primary->fb from > intel_crtc_active() since we can clearly have active CRTC's without a > framebuffer, and this check is now interfering with watermark > calculations when we try to re-enable the primary plane. > > Note that commit > > commit 0fda65680e92545caea5be7805a7f0a617fb6c20 > Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Date: Fri Feb 27 15:12:35 2015 +0000 > > drm/i915/skl: Update watermarks for Y tiling > > adds a test for primary plane enable/disable to trigger a watermark > update (previously we ignored updates to primary planes, which wasn't > really correct, but we got lucky since we always pretended the primary > plane was on). Tvrtko's patch tries to update watermarks when we > re-enable the primary plane, but that watermark computation gets aborted > early because intel_crtc_active() always returns false when the primary > plane is disabled; this leads to watermark underruns (at least on > platforms with ILK-style watermark code). > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 589addf..92cb2ff 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -893,11 +893,8 @@ bool intel_crtc_active(struct drm_crtc *crtc) > * > * We can ditch the adjusted_mode.crtc_clock check as soon > * as Haswell has gained clock readout/fastboot support. > - * > - * We can ditch the crtc->primary->fb check as soon as we can > - * properly reconstruct framebuffers. > */ > - return intel_crtc->active && crtc->primary->fb && > + return intel_crtc->active && > intel_crtc->config->base.adjusted_mode.crtc_clock; Struggling to paint the whole picture here.. Why it was correct to replace primary->fb with primary->state->fb elsewhere, but not here? Does the commit message mean there can be an active crtc with an active plane, but crtc->primary->fb == NULL so the wm recompute incorrectly configures for disabled plane? Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active() 2015-03-04 17:26 ` Tvrtko Ursulin @ 2015-03-04 17:42 ` Matt Roper 2015-03-05 15:07 ` Tvrtko Ursulin 0 siblings, 1 reply; 18+ messages in thread From: Matt Roper @ 2015-03-04 17:42 UTC (permalink / raw) To: Tvrtko Ursulin; +Cc: intel-gfx On Wed, Mar 04, 2015 at 05:26:36PM +0000, Tvrtko Ursulin wrote: > > On 03/04/2015 02:15 AM, Matt Roper wrote: > >Universal planes allow us to have an active CRTC without a primary plane > >framebuffer bound. Drop the test for primary->fb from > >intel_crtc_active() since we can clearly have active CRTC's without a > >framebuffer, and this check is now interfering with watermark > >calculations when we try to re-enable the primary plane. > > > >Note that commit > > > > commit 0fda65680e92545caea5be7805a7f0a617fb6c20 > > Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Date: Fri Feb 27 15:12:35 2015 +0000 > > > > drm/i915/skl: Update watermarks for Y tiling > > > >adds a test for primary plane enable/disable to trigger a watermark > >update (previously we ignored updates to primary planes, which wasn't > >really correct, but we got lucky since we always pretended the primary > >plane was on). Tvrtko's patch tries to update watermarks when we > >re-enable the primary plane, but that watermark computation gets aborted > >early because intel_crtc_active() always returns false when the primary > >plane is disabled; this leads to watermark underruns (at least on > >platforms with ILK-style watermark code). > > > >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > >--- > > drivers/gpu/drm/i915/intel_display.c | 5 +---- > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >index 589addf..92cb2ff 100644 > >--- a/drivers/gpu/drm/i915/intel_display.c > >+++ b/drivers/gpu/drm/i915/intel_display.c > >@@ -893,11 +893,8 @@ bool intel_crtc_active(struct drm_crtc *crtc) > > * > > * We can ditch the adjusted_mode.crtc_clock check as soon > > * as Haswell has gained clock readout/fastboot support. > >- * > >- * We can ditch the crtc->primary->fb check as soon as we can > >- * properly reconstruct framebuffers. > > */ > >- return intel_crtc->active && crtc->primary->fb && > >+ return intel_crtc->active && > > intel_crtc->config->base.adjusted_mode.crtc_clock; > > Struggling to paint the whole picture here.. > > Why it was correct to replace primary->fb with primary->state->fb > elsewhere, but not here? > > Does the commit message mean there can be an active crtc with an > active plane, but crtc->primary->fb == NULL so the wm recompute > incorrectly configures for disabled plane? Back when this code was first written, we didn't have universal planes yet and the fb pointer was directly in the CRTC. So at that time, (crtc->fb == NULL) meant that the entire CRTC was off. When I added universal plane support, I used Coccinelle to globally do a s/crtc->fb/crtc->primary->fb/, so that change didn't really have any special thought going into it. However at that point we started allowing you to have an explicitly disabled primary plane with an active CRTC (something that had never been possible before with the legacy API's). In this case, crtc->primary->fb = NULL, but the CRTC is still running. So only considering a CRTC to be active if its primary plane has a framebuffer isn't really correct in the universal plane and atomic world. Whether we use primary->fb or primary->state->fb doesn't really matter in this case. My understanding is that the reason we also checked fb and mode in addition to intel_crtc->active had to do with quickboot logic and the driver's ability to inherit state setup by the boot firmware. I think Jesse or Ville might be able to explain that part better and describe any quickboot problems that this change might cause. This patch is still sort of a bandaid; ultimately our watermark code should all be state-based and we'll be looking at crtc->state->active and plane->state->FOO and such to figure out how/when to update our watermarks. But the watermark rework isn't done yet and we're also sort of in a weird limbo stage where planes are pretty much converted to atomic, but CRTC's are still being worked on. Matt > > Regards, > > Tvrtko -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active() 2015-03-04 17:42 ` Matt Roper @ 2015-03-05 15:07 ` Tvrtko Ursulin 2015-03-06 16:44 ` Daniel Vetter 0 siblings, 1 reply; 18+ messages in thread From: Tvrtko Ursulin @ 2015-03-05 15:07 UTC (permalink / raw) To: Matt Roper; +Cc: intel-gfx On 03/04/2015 05:42 PM, Matt Roper wrote: > On Wed, Mar 04, 2015 at 05:26:36PM +0000, Tvrtko Ursulin wrote: >> >> On 03/04/2015 02:15 AM, Matt Roper wrote: >>> Universal planes allow us to have an active CRTC without a primary plane >>> framebuffer bound. Drop the test for primary->fb from >>> intel_crtc_active() since we can clearly have active CRTC's without a >>> framebuffer, and this check is now interfering with watermark >>> calculations when we try to re-enable the primary plane. >>> >>> Note that commit >>> >>> commit 0fda65680e92545caea5be7805a7f0a617fb6c20 >>> Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> Date: Fri Feb 27 15:12:35 2015 +0000 >>> >>> drm/i915/skl: Update watermarks for Y tiling >>> >>> adds a test for primary plane enable/disable to trigger a watermark >>> update (previously we ignored updates to primary planes, which wasn't >>> really correct, but we got lucky since we always pretended the primary >>> plane was on). Tvrtko's patch tries to update watermarks when we >>> re-enable the primary plane, but that watermark computation gets aborted >>> early because intel_crtc_active() always returns false when the primary >>> plane is disabled; this leads to watermark underruns (at least on >>> platforms with ILK-style watermark code). >>> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_display.c | 5 +---- >>> 1 file changed, 1 insertion(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>> index 589addf..92cb2ff 100644 >>> --- a/drivers/gpu/drm/i915/intel_display.c >>> +++ b/drivers/gpu/drm/i915/intel_display.c >>> @@ -893,11 +893,8 @@ bool intel_crtc_active(struct drm_crtc *crtc) >>> * >>> * We can ditch the adjusted_mode.crtc_clock check as soon >>> * as Haswell has gained clock readout/fastboot support. >>> - * >>> - * We can ditch the crtc->primary->fb check as soon as we can >>> - * properly reconstruct framebuffers. >>> */ >>> - return intel_crtc->active && crtc->primary->fb && >>> + return intel_crtc->active && >>> intel_crtc->config->base.adjusted_mode.crtc_clock; >> >> Struggling to paint the whole picture here.. >> >> Why it was correct to replace primary->fb with primary->state->fb >> elsewhere, but not here? >> >> Does the commit message mean there can be an active crtc with an >> active plane, but crtc->primary->fb == NULL so the wm recompute >> incorrectly configures for disabled plane? > > Back when this code was first written, we didn't have universal planes > yet and the fb pointer was directly in the CRTC. So at that time, > (crtc->fb == NULL) meant that the entire CRTC was off. > > When I added universal plane support, I used Coccinelle to globally do a > s/crtc->fb/crtc->primary->fb/, so that change didn't really have any > special thought going into it. However at that point we started > allowing you to have an explicitly disabled primary plane with an active > CRTC (something that had never been possible before with the legacy > API's). In this case, crtc->primary->fb = NULL, but the CRTC is still > running. > > So only considering a CRTC to be active if its primary plane has a > framebuffer isn't really correct in the universal plane and atomic > world. Whether we use primary->fb or primary->state->fb doesn't really > matter in this case. > > My understanding is that the reason we also checked fb and mode in > addition to intel_crtc->active had to do with quickboot logic and the > driver's ability to inherit state setup by the boot firmware. I think > Jesse or Ville might be able to explain that part better and describe > any quickboot problems that this change might cause. > > This patch is still sort of a bandaid; ultimately our watermark code > should all be state-based and we'll be looking at crtc->state->active > and plane->state->FOO and such to figure out how/when to update our > watermarks. But the watermark rework isn't done yet and we're also sort > of in a weird limbo stage where planes are pretty much converted to > atomic, but CRTC's are still being worked on. Yeah.. well, I don't see that anything will go bad with this, but this is with like 50% confidence. And the comment said check can be removed when we can "properly reconstruct the framebuffer" anyway. But what does "properly" mean there? Firmware take over looks there to me. So maybe it could really had been removed even regardless of this fallout? Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active() 2015-03-05 15:07 ` Tvrtko Ursulin @ 2015-03-06 16:44 ` Daniel Vetter 0 siblings, 0 replies; 18+ messages in thread From: Daniel Vetter @ 2015-03-06 16:44 UTC (permalink / raw) To: Tvrtko Ursulin; +Cc: intel-gfx On Thu, Mar 05, 2015 at 03:07:52PM +0000, Tvrtko Ursulin wrote: > > On 03/04/2015 05:42 PM, Matt Roper wrote: > >On Wed, Mar 04, 2015 at 05:26:36PM +0000, Tvrtko Ursulin wrote: > >> > >>On 03/04/2015 02:15 AM, Matt Roper wrote: > >>>Universal planes allow us to have an active CRTC without a primary plane > >>>framebuffer bound. Drop the test for primary->fb from > >>>intel_crtc_active() since we can clearly have active CRTC's without a > >>>framebuffer, and this check is now interfering with watermark > >>>calculations when we try to re-enable the primary plane. > >>> > >>>Note that commit > >>> > >>> commit 0fda65680e92545caea5be7805a7f0a617fb6c20 > >>> Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>> Date: Fri Feb 27 15:12:35 2015 +0000 > >>> > >>> drm/i915/skl: Update watermarks for Y tiling > >>> > >>>adds a test for primary plane enable/disable to trigger a watermark > >>>update (previously we ignored updates to primary planes, which wasn't > >>>really correct, but we got lucky since we always pretended the primary > >>>plane was on). Tvrtko's patch tries to update watermarks when we > >>>re-enable the primary plane, but that watermark computation gets aborted > >>>early because intel_crtc_active() always returns false when the primary > >>>plane is disabled; this leads to watermark underruns (at least on > >>>platforms with ILK-style watermark code). > >>> > >>>Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>>Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > >>>--- > >>> drivers/gpu/drm/i915/intel_display.c | 5 +---- > >>> 1 file changed, 1 insertion(+), 4 deletions(-) > >>> > >>>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >>>index 589addf..92cb2ff 100644 > >>>--- a/drivers/gpu/drm/i915/intel_display.c > >>>+++ b/drivers/gpu/drm/i915/intel_display.c > >>>@@ -893,11 +893,8 @@ bool intel_crtc_active(struct drm_crtc *crtc) > >>> * > >>> * We can ditch the adjusted_mode.crtc_clock check as soon > >>> * as Haswell has gained clock readout/fastboot support. > >>>- * > >>>- * We can ditch the crtc->primary->fb check as soon as we can > >>>- * properly reconstruct framebuffers. > >>> */ > >>>- return intel_crtc->active && crtc->primary->fb && > >>>+ return intel_crtc->active && > >>> intel_crtc->config->base.adjusted_mode.crtc_clock; > >> > >>Struggling to paint the whole picture here.. > >> > >>Why it was correct to replace primary->fb with primary->state->fb > >>elsewhere, but not here? > >> > >>Does the commit message mean there can be an active crtc with an > >>active plane, but crtc->primary->fb == NULL so the wm recompute > >>incorrectly configures for disabled plane? > > > >Back when this code was first written, we didn't have universal planes > >yet and the fb pointer was directly in the CRTC. So at that time, > >(crtc->fb == NULL) meant that the entire CRTC was off. > > > >When I added universal plane support, I used Coccinelle to globally do a > >s/crtc->fb/crtc->primary->fb/, so that change didn't really have any > >special thought going into it. However at that point we started > >allowing you to have an explicitly disabled primary plane with an active > >CRTC (something that had never been possible before with the legacy > >API's). In this case, crtc->primary->fb = NULL, but the CRTC is still > >running. > > > >So only considering a CRTC to be active if its primary plane has a > >framebuffer isn't really correct in the universal plane and atomic > >world. Whether we use primary->fb or primary->state->fb doesn't really > >matter in this case. > > > >My understanding is that the reason we also checked fb and mode in > >addition to intel_crtc->active had to do with quickboot logic and the > >driver's ability to inherit state setup by the boot firmware. I think > >Jesse or Ville might be able to explain that part better and describe > >any quickboot problems that this change might cause. > > > >This patch is still sort of a bandaid; ultimately our watermark code > >should all be state-based and we'll be looking at crtc->state->active > >and plane->state->FOO and such to figure out how/when to update our > >watermarks. But the watermark rework isn't done yet and we're also sort > >of in a weird limbo stage where planes are pretty much converted to > >atomic, but CRTC's are still being worked on. > > Yeah.. well, I don't see that anything will go bad with this, but this is > with like 50% confidence. > > And the comment said check can be removed when we can "properly reconstruct > the framebuffer" anyway. > > But what does "properly" mean there? Firmware take over looks there to me. > So maybe it could really had been removed even regardless of this fallout? Comment is outdated, what we want in the end is to just disable the primary plane and make sure the hw can cope. Since if someone boots in vga mode there is no primary plane (the vga plane is some entirely separate beast). -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 ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2015-03-06 16:42 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-03-04 2:15 [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active() Matt Roper 2015-03-04 2:15 ` [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation Matt Roper 2015-03-04 8:18 ` Jani Nikula 2015-03-04 16:17 ` shuang.he 2015-03-04 17:29 ` Tvrtko Ursulin 2015-03-06 1:31 ` [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation (v2) Matt Roper 2015-03-06 9:58 ` Tvrtko Ursulin 2015-03-06 12:57 ` Ville Syrjälä 2015-03-04 9:45 ` [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active() Ville Syrjälä 2015-03-04 16:13 ` Daniel Vetter 2015-03-04 18:21 ` Ville Syrjälä 2015-03-05 12:20 ` Daniel Vetter 2015-03-05 12:48 ` Ville Syrjälä 2015-03-04 16:14 ` Daniel Vetter 2015-03-04 17:26 ` Tvrtko Ursulin 2015-03-04 17:42 ` Matt Roper 2015-03-05 15:07 ` Tvrtko Ursulin 2015-03-06 16:44 ` Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.