[PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active()

[PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active()

[Matt Roper]

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

[PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation

[Matt Roper]

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

Re: [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation

[Jani Nikula]

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

Re: [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active()

[Ville Syrjälä]

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

Re: [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active()

[Daniel Vetter]

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

Re: [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active()

[Daniel Vetter]

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

Re: [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation

[shuang.he]

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

Re: [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active()

[Tvrtko Ursulin]

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

Re: [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation

[Tvrtko Ursulin]

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

Re: [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active()

[Matt Roper]

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

Re: [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active()

[Ville Syrjälä]

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

Re: [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active()

[Daniel Vetter]

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

Re: [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active()

[Ville Syrjälä]

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

Re: [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active()

[Tvrtko Ursulin]

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

[PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation (v2)

[Matt Roper]

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

Re: [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation (v2)

[Tvrtko Ursulin]

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

Re: [PATCH 2/2] drm/i915: Don't assume primary & cursor are always on for wm calculation (v2)

[Ville Syrjälä]

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

Re: [PATCH 1/2] drm/i915: Don't require primary->fb in intel_crtc_active()

[Daniel Vetter]

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