[PATCH 0/3] Fix recent watermark breakage (v2)

[PATCH 0/3] Fix recent watermark breakage (v2)

[Matt Roper]

Simplified the series of fixes; we leave intel_crtc->active alone for now
(since it's tightly tied to our current legacy modeset pipeline) and use it in
place of intel_crtc_active() in the watermark code.  As Ander's work on atomic
CRTC state progresses, some of the places we use intel_crtc->active will need
to be converted to crtc->state->active instead.

Matt Roper (3):
  drm/i915: Update intel_crtc_active() to use state values (v2)
  drm/i915: Use crtc->state->active in ilk/skl watermark calculations
    (v3)
  drm/i915: Don't assume primary & cursor are always on for wm
    calculation (v3)

 drivers/gpu/drm/i915/intel_display.c |   6 +-
 drivers/gpu/drm/i915/intel_pm.c      | 129 ++++++++++++++++++++++++-----------
 2 files changed, 95 insertions(+), 40 deletions(-)

-- 
1.8.5.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/3] drm/i915: Update intel_crtc_active() to use state values (v2)

[Matt Roper]

With the switch to atomic plumbing for planes, some of our commit-time
work (e.g., watermarks) is done after the new atomic state is swapped
into the relevant DRM object, but before the DRM core has a chance to
update its legacy state values.  Switch intel_crtc_active() to look at
the state objects rather than legacy fields to ensure we operate on the
proper values.

Note that we're continuing to use intel_crtc->active here for the time
being since crtc->state isn't really hooked up yet.  Once CRTC states
are wired up properly, we'll want to switch this over to use
crtc->state->active instead.

v2: Switch back to intel_crtc->active for now; when Ander's work on CRTC
    states is ready, we can flip this over to use crtc->state->active
    instead. (Ville)

Cc: Ander Conselvan De Oliveira <conselvan2@gmail.com>
Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 43d3575..e35fd7f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -896,8 +896,12 @@ bool intel_crtc_active(struct drm_crtc *crtc)
 	 *
 	 * We can ditch the crtc->primary->fb check as soon as we can
 	 * properly reconstruct framebuffers.
+	 *
+	 * FIXME: The intel_crtc->active here should be switched to
+	 * crtc->state->active once we have proper CRTC states wired up
+	 * for atomic.
 	 */
-	return intel_crtc->active && crtc->primary->fb &&
+	return intel_crtc->active && crtc->primary->state->fb &&
 		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/3] drm/i915: Use crtc->state->active in ilk/skl watermark calculations (v3)

[Matt Roper]

Existing watermark code calls intel_crtc_active() to determine whether a CRTC
is active for the purpose of watermark calculations (and bails out early if it
determines the CRTC is not active).  However intel_crtc_active() only returns
true if crtc->primary->fb is non-NULL, which isn't appropriate in the modern
age of universal planes and atomic modeset since userspace can now disable the
primary plane, but leave the CRTC (and other planes) running.

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() returns
false due to the disabled primary plane.

Switch the ILK and SKL watermark code over to use crtc->state->active rather
than calling intel_crtc_active() so that we'll properly compute watermarks when
re-enabling the primary plane.

Note that this commit doesn't touch callsites in the watermark code for
older platforms since there were concerns that doing so would lead to
other types of breakage.

Also note that all of the watermark calculation at the moment takes place after
new crtc/plane states are swapped into the DRM objects.  This will change in
the future, so we'll be working with in-flight state objects, but for the time
being, crtc->state is what we want to operate on.

v2: Don't drop primary->fb check from intel_crtc_active(), but rather replace
    ILK/SKL callsites with direct tests of crtc->state->active.  There is
    concern that messing with intel_crtc_active() will lead to other breakage for
    old hardware platforms.  (Ville)

v3: Use intel_crtc->active for now rather than crtc->state->active since
    we don't have CRTC states properly hooked up and initialized yet.
    We'll defer the switch to crtc->state->active until the atomic CRTC
    state work is farther along. (Ville)

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4f04fab..a06a2c7 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1701,7 +1701,7 @@ hsw_compute_linetime_wm(struct drm_device *dev, struct drm_crtc *crtc)
 	struct drm_display_mode *mode = &intel_crtc->config->base.adjusted_mode;
 	u32 linetime, ips_linetime;
 
-	if (!intel_crtc_active(crtc))
+	if (!intel_crtc->active)
 		return 0;
 
 	/* The WM are computed with base on how long it takes to fill a single
@@ -1956,7 +1956,7 @@ static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
 	enum pipe pipe = intel_crtc->pipe;
 	struct drm_plane *plane;
 
-	if (!intel_crtc_active(crtc))
+	if (!intel_crtc->active)
 		return;
 
 	p->active = true;
@@ -2468,7 +2468,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 
 	nth_active_pipe = 0;
 	for_each_crtc(dev, crtc) {
-		if (!intel_crtc_active(crtc))
+		if (!to_intel_crtc(crtc)->active)
 			continue;
 
 		if (crtc == for_crtc)
@@ -2708,7 +2708,7 @@ static void skl_compute_wm_global_parameters(struct drm_device *dev,
 	struct drm_plane *plane;
 
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
-		config->num_pipes_active += intel_crtc_active(crtc);
+		config->num_pipes_active += to_intel_crtc(crtc)->active;
 
 	/* FIXME: I don't think we need those two global parameters on SKL */
 	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
@@ -2729,7 +2729,7 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
 	struct drm_framebuffer *fb;
 	int i = 1; /* Index for sprite planes start */
 
-	p->active = intel_crtc_active(crtc);
+	p->active = intel_crtc->active;
 	if (p->active) {
 		p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
 		p->pixel_rate = skl_pipe_pixel_rate(intel_crtc->config);
@@ -2860,7 +2860,7 @@ static void skl_compute_wm_level(const struct drm_i915_private *dev_priv,
 static uint32_t
 skl_compute_linetime_wm(struct drm_crtc *crtc, struct skl_pipe_wm_parameters *p)
 {
-	if (!intel_crtc_active(crtc))
+	if (!to_intel_crtc(crtc)->active)
 		return 0;
 
 	return DIV_ROUND_UP(8 * p->pipe_htotal * 1000, p->pixel_rate);
@@ -3407,7 +3407,7 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
 		hw->plane_trans[pipe][i] = I915_READ(PLANE_WM_TRANS(pipe, i));
 	hw->cursor_trans[pipe] = I915_READ(CUR_WM_TRANS(pipe));
 
-	if (!intel_crtc_active(crtc))
+	if (!intel_crtc->active)
 		return;
 
 	hw->dirty[pipe] = true;
@@ -3462,7 +3462,7 @@ static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
 	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		hw->wm_linetime[pipe] = I915_READ(PIPE_WM_LINETIME(pipe));
 
-	active->pipe_enabled = intel_crtc_active(crtc);
+	active->pipe_enabled = intel_crtc->active;
 
 	if (active->pipe_enabled) {
 		u32 tmp = hw->wm_pipe[pipe];
-- 
1.8.5.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Use crtc->state->active in ilk/skl watermark calculations (v3)

[Daniel Vetter]

On Mon, Mar 09, 2015 at 10:19:24AM -0700, Matt Roper wrote:
> Existing watermark code calls intel_crtc_active() to determine whether a CRTC
> is active for the purpose of watermark calculations (and bails out early if it
> determines the CRTC is not active).  However intel_crtc_active() only returns
> true if crtc->primary->fb is non-NULL, which isn't appropriate in the modern
> age of universal planes and atomic modeset since userspace can now disable the
> primary plane, but leave the CRTC (and other planes) running.
> 
> 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() returns
> false due to the disabled primary plane.
> 
> Switch the ILK and SKL watermark code over to use crtc->state->active rather
> than calling intel_crtc_active() so that we'll properly compute watermarks when
> re-enabling the primary plane.
> 
> Note that this commit doesn't touch callsites in the watermark code for
> older platforms since there were concerns that doing so would lead to
> other types of breakage.
> 
> Also note that all of the watermark calculation at the moment takes place after
> new crtc/plane states are swapped into the DRM objects.  This will change in
> the future, so we'll be working with in-flight state objects, but for the time
> being, crtc->state is what we want to operate on.
> 
> v2: Don't drop primary->fb check from intel_crtc_active(), but rather replace
>     ILK/SKL callsites with direct tests of crtc->state->active.  There is
>     concern that messing with intel_crtc_active() will lead to other breakage for
>     old hardware platforms.  (Ville)
> 
> v3: Use intel_crtc->active for now rather than crtc->state->active since
>     we don't have CRTC states properly hooked up and initialized yet.
>     We'll defer the switch to crtc->state->active until the atomic CRTC
>     state work is farther along. (Ville)
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Merged up to this one, thanks.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 4f04fab..a06a2c7 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1701,7 +1701,7 @@ hsw_compute_linetime_wm(struct drm_device *dev, struct drm_crtc *crtc)
>  	struct drm_display_mode *mode = &intel_crtc->config->base.adjusted_mode;
>  	u32 linetime, ips_linetime;
>  
> -	if (!intel_crtc_active(crtc))
> +	if (!intel_crtc->active)
>  		return 0;
>  
>  	/* The WM are computed with base on how long it takes to fill a single
> @@ -1956,7 +1956,7 @@ static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
>  	enum pipe pipe = intel_crtc->pipe;
>  	struct drm_plane *plane;
>  
> -	if (!intel_crtc_active(crtc))
> +	if (!intel_crtc->active)
>  		return;
>  
>  	p->active = true;
> @@ -2468,7 +2468,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>  
>  	nth_active_pipe = 0;
>  	for_each_crtc(dev, crtc) {
> -		if (!intel_crtc_active(crtc))
> +		if (!to_intel_crtc(crtc)->active)
>  			continue;
>  
>  		if (crtc == for_crtc)
> @@ -2708,7 +2708,7 @@ static void skl_compute_wm_global_parameters(struct drm_device *dev,
>  	struct drm_plane *plane;
>  
>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> -		config->num_pipes_active += intel_crtc_active(crtc);
> +		config->num_pipes_active += to_intel_crtc(crtc)->active;
>  
>  	/* FIXME: I don't think we need those two global parameters on SKL */
>  	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> @@ -2729,7 +2729,7 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>  	struct drm_framebuffer *fb;
>  	int i = 1; /* Index for sprite planes start */
>  
> -	p->active = intel_crtc_active(crtc);
> +	p->active = intel_crtc->active;
>  	if (p->active) {
>  		p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
>  		p->pixel_rate = skl_pipe_pixel_rate(intel_crtc->config);
> @@ -2860,7 +2860,7 @@ static void skl_compute_wm_level(const struct drm_i915_private *dev_priv,
>  static uint32_t
>  skl_compute_linetime_wm(struct drm_crtc *crtc, struct skl_pipe_wm_parameters *p)
>  {
> -	if (!intel_crtc_active(crtc))
> +	if (!to_intel_crtc(crtc)->active)
>  		return 0;
>  
>  	return DIV_ROUND_UP(8 * p->pipe_htotal * 1000, p->pixel_rate);
> @@ -3407,7 +3407,7 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
>  		hw->plane_trans[pipe][i] = I915_READ(PLANE_WM_TRANS(pipe, i));
>  	hw->cursor_trans[pipe] = I915_READ(CUR_WM_TRANS(pipe));
>  
> -	if (!intel_crtc_active(crtc))
> +	if (!intel_crtc->active)
>  		return;
>  
>  	hw->dirty[pipe] = true;
> @@ -3462,7 +3462,7 @@ static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
>  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		hw->wm_linetime[pipe] = I915_READ(PIPE_WM_LINETIME(pipe));
>  
> -	active->pipe_enabled = intel_crtc_active(crtc);
> +	active->pipe_enabled = intel_crtc->active;
>  
>  	if (active->pipe_enabled) {
>  		u32 tmp = hw->wm_pipe[pipe];
> -- 
> 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

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

[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)

v3: Fix a couple copy/paste mistakes in SKL code. (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 | 113 +++++++++++++++++++++++++++++-----------
 1 file changed, 82 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a06a2c7..207c973 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -608,12 +608,17 @@ static void pineview_update_wm(struct drm_crtc *unused_crtc)
 	crtc = single_enabled_crtc(dev);
 	if (crtc) {
 		const struct drm_display_mode *adjusted_mode;
-		int pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
+		int pixel_size;
 		int clock;
 
 		adjusted_mode = &to_intel_crtc(crtc)->config->base.adjusted_mode;
 		clock = adjusted_mode->crtc_clock;
 
+		if (crtc->primary->state->fb)
+			pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
+		else
+			pixel_size = 4;
+
 		/* Display SR */
 		wm = intel_calculate_wm(clock, &pineview_display_wm,
 					pineview_display_wm.fifo_size,
@@ -684,7 +689,11 @@ static bool g4x_compute_wm0(struct drm_device *dev,
 	clock = adjusted_mode->crtc_clock;
 	htotal = adjusted_mode->crtc_htotal;
 	hdisplay = to_intel_crtc(crtc)->config->pipe_src_w;
-	pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
+
+	if (crtc->primary->state->fb)
+		pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
+	else
+		pixel_size = 4;
 
 	/* Use the small buffer method to calculate plane watermark */
 	entries = ((clock * pixel_size / 1000) * display_latency_ns) / 1000;
@@ -771,7 +780,11 @@ static bool g4x_compute_srwm(struct drm_device *dev,
 	clock = adjusted_mode->crtc_clock;
 	htotal = adjusted_mode->crtc_htotal;
 	hdisplay = to_intel_crtc(crtc)->config->pipe_src_w;
-	pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
+
+	if (crtc->primary->state->fb)
+		pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
+	else
+		pixel_size = 4;
 
 	line_time_us = max(htotal * 1000 / clock, 1);
 	line_count = (latency_ns / line_time_us + 1000) / 1000;
@@ -1118,10 +1131,15 @@ static void i965_update_wm(struct drm_crtc *unused_crtc)
 		int clock = adjusted_mode->crtc_clock;
 		int htotal = adjusted_mode->crtc_htotal;
 		int hdisplay = to_intel_crtc(crtc)->config->pipe_src_w;
-		int pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
+		int pixel_size;
 		unsigned long line_time_us;
 		int entries;
 
+		if (crtc->primary->state->fb)
+			pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
+		else
+			pixel_size = 4;
+
 		line_time_us = max(htotal * 1000 / clock, 1);
 
 		/* Use ns/us then divide to preserve precision */
@@ -1195,7 +1213,13 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
 	crtc = intel_get_crtc_for_plane(dev, 0);
 	if (intel_crtc_active(crtc)) {
 		const struct drm_display_mode *adjusted_mode;
-		int cpp = crtc->primary->state->fb->bits_per_pixel / 8;
+		int cpp;
+
+		if (crtc->primary->state->fb)
+			cpp = crtc->primary->state->fb->bits_per_pixel / 8;
+		else
+			cpp = 4;
+
 		if (IS_GEN2(dev))
 			cpp = 4;
 
@@ -1217,7 +1241,13 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
 	crtc = intel_get_crtc_for_plane(dev, 1);
 	if (intel_crtc_active(crtc)) {
 		const struct drm_display_mode *adjusted_mode;
-		int cpp = crtc->primary->state->fb->bits_per_pixel / 8;
+		int cpp;
+
+		if (crtc->primary->state->fb)
+			cpp = crtc->primary->state->fb->bits_per_pixel / 8;
+		else
+			cpp = 4;
+
 		if (IS_GEN2(dev))
 			cpp = 4;
 
@@ -1243,7 +1273,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
 		obj = intel_fb_obj(enabled->primary->state->fb);
 
 		/* self-refresh seems busted with untiled */
-		if (obj->tiling_mode == I915_TILING_NONE)
+		if (!obj || obj->tiling_mode == I915_TILING_NONE)
 			enabled = NULL;
 	}
 
@@ -1264,10 +1294,15 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
 		int clock = adjusted_mode->crtc_clock;
 		int htotal = adjusted_mode->crtc_htotal;
 		int hdisplay = to_intel_crtc(enabled)->config->pipe_src_w;
-		int pixel_size = enabled->primary->state->fb->bits_per_pixel / 8;
+		int pixel_size;
 		unsigned long line_time_us;
 		int entries;
 
+		if (enabled->primary->state->fb)
+			pixel_size = enabled->primary->state->fb->bits_per_pixel / 8;
+		else
+			pixel_size = 4;
+
 		line_time_us = max(htotal * 1000 / clock, 1);
 
 		/* Use ns/us then divide to preserve precision */
@@ -1962,13 +1997,25 @@ static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
 	p->active = true;
 	p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
 	p->pixel_rate = ilk_pipe_pixel_rate(dev, crtc);
-	p->pri.bytes_per_pixel = crtc->primary->state->fb->bits_per_pixel / 8;
-	p->cur.bytes_per_pixel = 4;
+
+	if (crtc->primary->state->fb) {
+		p->pri.enabled = true;
+		p->pri.bytes_per_pixel =
+			crtc->primary->state->fb->bits_per_pixel / 8;
+	} else {
+		p->pri.enabled = false;
+		p->pri.bytes_per_pixel = 0;
+	}
+
+	if (crtc->cursor->state->fb) {
+		p->cur.enabled = true;
+		p->cur.bytes_per_pixel = 4;
+	} else {
+		p->cur.enabled = false;
+		p->cur.bytes_per_pixel = 0;
+	}
 	p->pri.horiz_pixels = intel_crtc->config->pipe_src_w;
 	p->cur.horiz_pixels = intel_crtc->base.cursor->state->crtc_w;
-	/* TODO: for now, assume primary and cursor planes are always enabled. */
-	p->pri.enabled = true;
-	p->cur.enabled = true;
 
 	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
 		struct intel_plane *intel_plane = to_intel_plane(plane);
@@ -2734,27 +2781,31 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
 		p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
 		p->pixel_rate = skl_pipe_pixel_rate(intel_crtc->config);
 
-		/*
-		 * For now, assume primary and cursor planes are always enabled.
-		 */
-		p->plane[0].enabled = true;
-		p->plane[0].bytes_per_pixel =
-			crtc->primary->state->fb->bits_per_pixel / 8;
-		p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
-		p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
-		p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
 		fb = crtc->primary->state->fb;
-		/*
-		 * Framebuffer can be NULL on plane disable, but it does not
-		 * matter for watermarks if we assume no tiling in that case.
-		 */
-		if (fb)
+		if (fb) {
+			p->plane[0].enabled = true;
+			p->plane[0].bytes_per_pixel = fb->bits_per_pixel / 8;
 			p->plane[0].tiling = fb->modifier[0];
+		} else {
+			p->plane[0].enabled = false;
+			p->plane[0].bytes_per_pixel = 0;
+			p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
+		}
+		p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
+		p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
 
-		p->cursor.enabled = true;
-		p->cursor.bytes_per_pixel = 4;
-		p->cursor.horiz_pixels = intel_crtc->base.cursor->state->crtc_w ?
-					 intel_crtc->base.cursor->state->crtc_w : 64;
+		fb = crtc->cursor->state->fb;
+		if (fb) {
+			p->cursor.enabled = true;
+			p->cursor.bytes_per_pixel = fb->bits_per_pixel / 8;
+			p->cursor.horiz_pixels = crtc->cursor->state->crtc_w;
+			p->cursor.vert_pixels = crtc->cursor->state->crtc_h;
+		} else {
+			p->cursor.enabled = false;
+			p->cursor.bytes_per_pixel = 0;
+			p->cursor.horiz_pixels = 64;
+			p->cursor.vert_pixels = 64;
+		}
 	}
 
 	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
-- 
1.8.5.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Mon, Mar 09, 2015 at 10:19:25AM -0700, Matt Roper wrote:
> Current ILK-style watermark code assumes the primary plane and cursor
> plane are always enabled.  This assumption, along with the combination
> of two independent commits that got merged at the same time, results in
> a NULL dereference.  The offending commits are:
> 
>         commit fd2d61341bf39d1054256c07d6eddd624ebc4241
>         Author: Matt Roper <matthew.d.roper@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)
> 
> v3: Fix a couple copy/paste mistakes in SKL code. (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 | 113 +++++++++++++++++++++++++++++-----------
>  1 file changed, 82 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a06a2c7..207c973 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -608,12 +608,17 @@ static void pineview_update_wm(struct drm_crtc *unused_crtc)
>  	crtc = single_enabled_crtc(dev);
>  	if (crtc) {
>  		const struct drm_display_mode *adjusted_mode;
> -		int pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> +		int pixel_size;
>  		int clock;
>  
>  		adjusted_mode = &to_intel_crtc(crtc)->config->base.adjusted_mode;
>  		clock = adjusted_mode->crtc_clock;
>  
> +		if (crtc->primary->state->fb)
> +			pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> +		else
> +			pixel_size = 4;
> +
>  		/* Display SR */
>  		wm = intel_calculate_wm(clock, &pineview_display_wm,
>  					pineview_display_wm.fifo_size,
> @@ -684,7 +689,11 @@ static bool g4x_compute_wm0(struct drm_device *dev,
>  	clock = adjusted_mode->crtc_clock;
>  	htotal = adjusted_mode->crtc_htotal;
>  	hdisplay = to_intel_crtc(crtc)->config->pipe_src_w;
> -	pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> +
> +	if (crtc->primary->state->fb)
> +		pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> +	else
> +		pixel_size = 4;
>  
>  	/* Use the small buffer method to calculate plane watermark */
>  	entries = ((clock * pixel_size / 1000) * display_latency_ns) / 1000;
> @@ -771,7 +780,11 @@ static bool g4x_compute_srwm(struct drm_device *dev,
>  	clock = adjusted_mode->crtc_clock;
>  	htotal = adjusted_mode->crtc_htotal;
>  	hdisplay = to_intel_crtc(crtc)->config->pipe_src_w;
> -	pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> +
> +	if (crtc->primary->state->fb)
> +		pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> +	else
> +		pixel_size = 4;
>  
>  	line_time_us = max(htotal * 1000 / clock, 1);
>  	line_count = (latency_ns / line_time_us + 1000) / 1000;
> @@ -1118,10 +1131,15 @@ static void i965_update_wm(struct drm_crtc *unused_crtc)
>  		int clock = adjusted_mode->crtc_clock;
>  		int htotal = adjusted_mode->crtc_htotal;
>  		int hdisplay = to_intel_crtc(crtc)->config->pipe_src_w;
> -		int pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> +		int pixel_size;
>  		unsigned long line_time_us;
>  		int entries;
>  
> +		if (crtc->primary->state->fb)
> +			pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> +		else
> +			pixel_size = 4;
> +
>  		line_time_us = max(htotal * 1000 / clock, 1);
>  
>  		/* Use ns/us then divide to preserve precision */
> @@ -1195,7 +1213,13 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
>  	crtc = intel_get_crtc_for_plane(dev, 0);
>  	if (intel_crtc_active(crtc)) {
>  		const struct drm_display_mode *adjusted_mode;
> -		int cpp = crtc->primary->state->fb->bits_per_pixel / 8;
> +		int cpp;
> +
> +		if (crtc->primary->state->fb)
> +			cpp = crtc->primary->state->fb->bits_per_pixel / 8;
> +		else
> +			cpp = 4;
> +
>  		if (IS_GEN2(dev))
>  			cpp = 4;
>  
> @@ -1217,7 +1241,13 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
>  	crtc = intel_get_crtc_for_plane(dev, 1);
>  	if (intel_crtc_active(crtc)) {
>  		const struct drm_display_mode *adjusted_mode;
> -		int cpp = crtc->primary->state->fb->bits_per_pixel / 8;
> +		int cpp;
> +
> +		if (crtc->primary->state->fb)
> +			cpp = crtc->primary->state->fb->bits_per_pixel / 8;
> +		else
> +			cpp = 4;
> +
>  		if (IS_GEN2(dev))
>  			cpp = 4;
>  
> @@ -1243,7 +1273,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
>  		obj = intel_fb_obj(enabled->primary->state->fb);
>  
>  		/* self-refresh seems busted with untiled */
> -		if (obj->tiling_mode == I915_TILING_NONE)
> +		if (!obj || obj->tiling_mode == I915_TILING_NONE)
>  			enabled = NULL;
>  	}
>  
> @@ -1264,10 +1294,15 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
>  		int clock = adjusted_mode->crtc_clock;
>  		int htotal = adjusted_mode->crtc_htotal;
>  		int hdisplay = to_intel_crtc(enabled)->config->pipe_src_w;
> -		int pixel_size = enabled->primary->state->fb->bits_per_pixel / 8;
> +		int pixel_size;
>  		unsigned long line_time_us;
>  		int entries;
>  
> +		if (enabled->primary->state->fb)
> +			pixel_size = enabled->primary->state->fb->bits_per_pixel / 8;
> +		else
> +			pixel_size = 4;
> +
>  		line_time_us = max(htotal * 1000 / clock, 1);
>  
>  		/* Use ns/us then divide to preserve precision */

With the change to intel_crtc_active() to look at state->fb I think you
could drop all of the changes above, and just keep the changes below.

> @@ -1962,13 +1997,25 @@ static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
>  	p->active = true;
>  	p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
>  	p->pixel_rate = ilk_pipe_pixel_rate(dev, crtc);
> -	p->pri.bytes_per_pixel = crtc->primary->state->fb->bits_per_pixel / 8;
> -	p->cur.bytes_per_pixel = 4;
> +
> +	if (crtc->primary->state->fb) {
> +		p->pri.enabled = true;
> +		p->pri.bytes_per_pixel =
> +			crtc->primary->state->fb->bits_per_pixel / 8;
> +	} else {
> +		p->pri.enabled = false;
> +		p->pri.bytes_per_pixel = 0;
> +	}
> +
> +	if (crtc->cursor->state->fb) {
> +		p->cur.enabled = true;
> +		p->cur.bytes_per_pixel = 4;
> +	} else {
> +		p->cur.enabled = false;
> +		p->cur.bytes_per_pixel = 0;
> +	}
>  	p->pri.horiz_pixels = intel_crtc->config->pipe_src_w;
>  	p->cur.horiz_pixels = intel_crtc->base.cursor->state->crtc_w;
> -	/* TODO: for now, assume primary and cursor planes are always enabled. */
> -	p->pri.enabled = true;
> -	p->cur.enabled = true;
>  
>  	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
>  		struct intel_plane *intel_plane = to_intel_plane(plane);
> @@ -2734,27 +2781,31 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>  		p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
>  		p->pixel_rate = skl_pipe_pixel_rate(intel_crtc->config);
>  
> -		/*
> -		 * For now, assume primary and cursor planes are always enabled.
> -		 */
> -		p->plane[0].enabled = true;
> -		p->plane[0].bytes_per_pixel =
> -			crtc->primary->state->fb->bits_per_pixel / 8;
> -		p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
> -		p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
> -		p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
>  		fb = crtc->primary->state->fb;
> -		/*
> -		 * Framebuffer can be NULL on plane disable, but it does not
> -		 * matter for watermarks if we assume no tiling in that case.
> -		 */
> -		if (fb)
> +		if (fb) {
> +			p->plane[0].enabled = true;
> +			p->plane[0].bytes_per_pixel = fb->bits_per_pixel / 8;
>  			p->plane[0].tiling = fb->modifier[0];
> +		} else {
> +			p->plane[0].enabled = false;
> +			p->plane[0].bytes_per_pixel = 0;
> +			p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
> +		}
> +		p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
> +		p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
>  
> -		p->cursor.enabled = true;
> -		p->cursor.bytes_per_pixel = 4;
> -		p->cursor.horiz_pixels = intel_crtc->base.cursor->state->crtc_w ?
> -					 intel_crtc->base.cursor->state->crtc_w : 64;
> +		fb = crtc->cursor->state->fb;
> +		if (fb) {
> +			p->cursor.enabled = true;
> +			p->cursor.bytes_per_pixel = fb->bits_per_pixel / 8;
> +			p->cursor.horiz_pixels = crtc->cursor->state->crtc_w;
> +			p->cursor.vert_pixels = crtc->cursor->state->crtc_h;
> +		} else {
> +			p->cursor.enabled = false;
> +			p->cursor.bytes_per_pixel = 0;
> +			p->cursor.horiz_pixels = 64;
> +			p->cursor.vert_pixels = 64;
> +		}
>  	}
>  
>  	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@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 3/3] drm/i915: Don't assume primary & cursor are always on for wm calculation (v3)

[Matt Roper]

On Mon, Mar 09, 2015 at 07:44:00PM +0200, Ville Syrjälä wrote:
> On Mon, Mar 09, 2015 at 10:19:25AM -0700, Matt Roper wrote:
> > Current ILK-style watermark code assumes the primary plane and cursor
> > plane are always enabled.  This assumption, along with the combination
> > of two independent commits that got merged at the same time, results in
> > a NULL dereference.  The offending commits are:
> > 
> >         commit fd2d61341bf39d1054256c07d6eddd624ebc4241
> >         Author: Matt Roper <matthew.d.roper@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)
> > 
> > v3: Fix a couple copy/paste mistakes in SKL code. (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 | 113 +++++++++++++++++++++++++++++-----------
> >  1 file changed, 82 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index a06a2c7..207c973 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -608,12 +608,17 @@ static void pineview_update_wm(struct drm_crtc *unused_crtc)
> >  	crtc = single_enabled_crtc(dev);
> >  	if (crtc) {
> >  		const struct drm_display_mode *adjusted_mode;
> > -		int pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> > +		int pixel_size;
> >  		int clock;
> >  
> >  		adjusted_mode = &to_intel_crtc(crtc)->config->base.adjusted_mode;
> >  		clock = adjusted_mode->crtc_clock;
> >  
> > +		if (crtc->primary->state->fb)
> > +			pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> > +		else
> > +			pixel_size = 4;
> > +
> >  		/* Display SR */
> >  		wm = intel_calculate_wm(clock, &pineview_display_wm,
> >  					pineview_display_wm.fifo_size,
> > @@ -684,7 +689,11 @@ static bool g4x_compute_wm0(struct drm_device *dev,
> >  	clock = adjusted_mode->crtc_clock;
> >  	htotal = adjusted_mode->crtc_htotal;
> >  	hdisplay = to_intel_crtc(crtc)->config->pipe_src_w;
> > -	pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> > +
> > +	if (crtc->primary->state->fb)
> > +		pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> > +	else
> > +		pixel_size = 4;
> >  
> >  	/* Use the small buffer method to calculate plane watermark */
> >  	entries = ((clock * pixel_size / 1000) * display_latency_ns) / 1000;
> > @@ -771,7 +780,11 @@ static bool g4x_compute_srwm(struct drm_device *dev,
> >  	clock = adjusted_mode->crtc_clock;
> >  	htotal = adjusted_mode->crtc_htotal;
> >  	hdisplay = to_intel_crtc(crtc)->config->pipe_src_w;
> > -	pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> > +
> > +	if (crtc->primary->state->fb)
> > +		pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> > +	else
> > +		pixel_size = 4;
> >  
> >  	line_time_us = max(htotal * 1000 / clock, 1);
> >  	line_count = (latency_ns / line_time_us + 1000) / 1000;
> > @@ -1118,10 +1131,15 @@ static void i965_update_wm(struct drm_crtc *unused_crtc)
> >  		int clock = adjusted_mode->crtc_clock;
> >  		int htotal = adjusted_mode->crtc_htotal;
> >  		int hdisplay = to_intel_crtc(crtc)->config->pipe_src_w;
> > -		int pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> > +		int pixel_size;
> >  		unsigned long line_time_us;
> >  		int entries;
> >  
> > +		if (crtc->primary->state->fb)
> > +			pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> > +		else
> > +			pixel_size = 4;
> > +
> >  		line_time_us = max(htotal * 1000 / clock, 1);
> >  
> >  		/* Use ns/us then divide to preserve precision */
> > @@ -1195,7 +1213,13 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
> >  	crtc = intel_get_crtc_for_plane(dev, 0);
> >  	if (intel_crtc_active(crtc)) {
> >  		const struct drm_display_mode *adjusted_mode;
> > -		int cpp = crtc->primary->state->fb->bits_per_pixel / 8;
> > +		int cpp;
> > +
> > +		if (crtc->primary->state->fb)
> > +			cpp = crtc->primary->state->fb->bits_per_pixel / 8;
> > +		else
> > +			cpp = 4;
> > +
> >  		if (IS_GEN2(dev))
> >  			cpp = 4;
> >  
> > @@ -1217,7 +1241,13 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
> >  	crtc = intel_get_crtc_for_plane(dev, 1);
> >  	if (intel_crtc_active(crtc)) {
> >  		const struct drm_display_mode *adjusted_mode;
> > -		int cpp = crtc->primary->state->fb->bits_per_pixel / 8;
> > +		int cpp;
> > +
> > +		if (crtc->primary->state->fb)
> > +			cpp = crtc->primary->state->fb->bits_per_pixel / 8;
> > +		else
> > +			cpp = 4;
> > +
> >  		if (IS_GEN2(dev))
> >  			cpp = 4;
> >  
> > @@ -1243,7 +1273,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
> >  		obj = intel_fb_obj(enabled->primary->state->fb);
> >  
> >  		/* self-refresh seems busted with untiled */
> > -		if (obj->tiling_mode == I915_TILING_NONE)
> > +		if (!obj || obj->tiling_mode == I915_TILING_NONE)
> >  			enabled = NULL;
> >  	}
> >  
> > @@ -1264,10 +1294,15 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
> >  		int clock = adjusted_mode->crtc_clock;
> >  		int htotal = adjusted_mode->crtc_htotal;
> >  		int hdisplay = to_intel_crtc(enabled)->config->pipe_src_w;
> > -		int pixel_size = enabled->primary->state->fb->bits_per_pixel / 8;
> > +		int pixel_size;
> >  		unsigned long line_time_us;
> >  		int entries;
> >  
> > +		if (enabled->primary->state->fb)
> > +			pixel_size = enabled->primary->state->fb->bits_per_pixel / 8;
> > +		else
> > +			pixel_size = 4;
> > +
> >  		line_time_us = max(htotal * 1000 / clock, 1);
> >  
> >  		/* Use ns/us then divide to preserve precision */
> 
> With the change to intel_crtc_active() to look at state->fb I think you
> could drop all of the changes above, and just keep the changes below.

Are you sure?  If intel_crtc->active is true, but the primary plane is
disabled (plane->state->fb == NULL), then I think we're still going to
wind up trying to dereference plane->state->fb to get bits_per_pixel.

In my other branch I do have a patch that just moves all that wm_param
calculation out of the watermark code here and sticks it in the
check_plane code, so that we always have up-to-date enabled/bpp/etc
values collected for the plane state.  If I can extract that from my
other branch cleanly, that might be a slightly cleaner solution than
just adding all the fb tests here.


Matt

> 
> > @@ -1962,13 +1997,25 @@ static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
> >  	p->active = true;
> >  	p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
> >  	p->pixel_rate = ilk_pipe_pixel_rate(dev, crtc);
> > -	p->pri.bytes_per_pixel = crtc->primary->state->fb->bits_per_pixel / 8;
> > -	p->cur.bytes_per_pixel = 4;
> > +
> > +	if (crtc->primary->state->fb) {
> > +		p->pri.enabled = true;
> > +		p->pri.bytes_per_pixel =
> > +			crtc->primary->state->fb->bits_per_pixel / 8;
> > +	} else {
> > +		p->pri.enabled = false;
> > +		p->pri.bytes_per_pixel = 0;
> > +	}
> > +
> > +	if (crtc->cursor->state->fb) {
> > +		p->cur.enabled = true;
> > +		p->cur.bytes_per_pixel = 4;
> > +	} else {
> > +		p->cur.enabled = false;
> > +		p->cur.bytes_per_pixel = 0;
> > +	}
> >  	p->pri.horiz_pixels = intel_crtc->config->pipe_src_w;
> >  	p->cur.horiz_pixels = intel_crtc->base.cursor->state->crtc_w;
> > -	/* TODO: for now, assume primary and cursor planes are always enabled. */
> > -	p->pri.enabled = true;
> > -	p->cur.enabled = true;
> >  
> >  	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
> >  		struct intel_plane *intel_plane = to_intel_plane(plane);
> > @@ -2734,27 +2781,31 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
> >  		p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
> >  		p->pixel_rate = skl_pipe_pixel_rate(intel_crtc->config);
> >  
> > -		/*
> > -		 * For now, assume primary and cursor planes are always enabled.
> > -		 */
> > -		p->plane[0].enabled = true;
> > -		p->plane[0].bytes_per_pixel =
> > -			crtc->primary->state->fb->bits_per_pixel / 8;
> > -		p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
> > -		p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
> > -		p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
> >  		fb = crtc->primary->state->fb;
> > -		/*
> > -		 * Framebuffer can be NULL on plane disable, but it does not
> > -		 * matter for watermarks if we assume no tiling in that case.
> > -		 */
> > -		if (fb)
> > +		if (fb) {
> > +			p->plane[0].enabled = true;
> > +			p->plane[0].bytes_per_pixel = fb->bits_per_pixel / 8;
> >  			p->plane[0].tiling = fb->modifier[0];
> > +		} else {
> > +			p->plane[0].enabled = false;
> > +			p->plane[0].bytes_per_pixel = 0;
> > +			p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
> > +		}
> > +		p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
> > +		p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
> >  
> > -		p->cursor.enabled = true;
> > -		p->cursor.bytes_per_pixel = 4;
> > -		p->cursor.horiz_pixels = intel_crtc->base.cursor->state->crtc_w ?
> > -					 intel_crtc->base.cursor->state->crtc_w : 64;
> > +		fb = crtc->cursor->state->fb;
> > +		if (fb) {
> > +			p->cursor.enabled = true;
> > +			p->cursor.bytes_per_pixel = fb->bits_per_pixel / 8;
> > +			p->cursor.horiz_pixels = crtc->cursor->state->crtc_w;
> > +			p->cursor.vert_pixels = crtc->cursor->state->crtc_h;
> > +		} else {
> > +			p->cursor.enabled = false;
> > +			p->cursor.bytes_per_pixel = 0;
> > +			p->cursor.horiz_pixels = 64;
> > +			p->cursor.vert_pixels = 64;
> > +		}
> >  	}
> >  
> >  	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> > -- 
> > 1.8.5.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Mon, Mar 09, 2015 at 10:48:08AM -0700, Matt Roper wrote:
> On Mon, Mar 09, 2015 at 07:44:00PM +0200, Ville Syrjälä wrote:
> > On Mon, Mar 09, 2015 at 10:19:25AM -0700, Matt Roper wrote:
> > > Current ILK-style watermark code assumes the primary plane and cursor
> > > plane are always enabled.  This assumption, along with the combination
> > > of two independent commits that got merged at the same time, results in
> > > a NULL dereference.  The offending commits are:
> > > 
> > >         commit fd2d61341bf39d1054256c07d6eddd624ebc4241
> > >         Author: Matt Roper <matthew.d.roper@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)
> > > 
> > > v3: Fix a couple copy/paste mistakes in SKL code. (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 | 113 +++++++++++++++++++++++++++++-----------
> > >  1 file changed, 82 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index a06a2c7..207c973 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -608,12 +608,17 @@ static void pineview_update_wm(struct drm_crtc *unused_crtc)
> > >  	crtc = single_enabled_crtc(dev);
> > >  	if (crtc) {
> > >  		const struct drm_display_mode *adjusted_mode;
> > > -		int pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> > > +		int pixel_size;
> > >  		int clock;
> > >  
> > >  		adjusted_mode = &to_intel_crtc(crtc)->config->base.adjusted_mode;
> > >  		clock = adjusted_mode->crtc_clock;
> > >  
> > > +		if (crtc->primary->state->fb)
> > > +			pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> > > +		else
> > > +			pixel_size = 4;
> > > +
> > >  		/* Display SR */
> > >  		wm = intel_calculate_wm(clock, &pineview_display_wm,
> > >  					pineview_display_wm.fifo_size,
> > > @@ -684,7 +689,11 @@ static bool g4x_compute_wm0(struct drm_device *dev,
> > >  	clock = adjusted_mode->crtc_clock;
> > >  	htotal = adjusted_mode->crtc_htotal;
> > >  	hdisplay = to_intel_crtc(crtc)->config->pipe_src_w;
> > > -	pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> > > +
> > > +	if (crtc->primary->state->fb)
> > > +		pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> > > +	else
> > > +		pixel_size = 4;
> > >  
> > >  	/* Use the small buffer method to calculate plane watermark */
> > >  	entries = ((clock * pixel_size / 1000) * display_latency_ns) / 1000;
> > > @@ -771,7 +780,11 @@ static bool g4x_compute_srwm(struct drm_device *dev,
> > >  	clock = adjusted_mode->crtc_clock;
> > >  	htotal = adjusted_mode->crtc_htotal;
> > >  	hdisplay = to_intel_crtc(crtc)->config->pipe_src_w;
> > > -	pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> > > +
> > > +	if (crtc->primary->state->fb)
> > > +		pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> > > +	else
> > > +		pixel_size = 4;
> > >  
> > >  	line_time_us = max(htotal * 1000 / clock, 1);
> > >  	line_count = (latency_ns / line_time_us + 1000) / 1000;
> > > @@ -1118,10 +1131,15 @@ static void i965_update_wm(struct drm_crtc *unused_crtc)
> > >  		int clock = adjusted_mode->crtc_clock;
> > >  		int htotal = adjusted_mode->crtc_htotal;
> > >  		int hdisplay = to_intel_crtc(crtc)->config->pipe_src_w;
> > > -		int pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> > > +		int pixel_size;
> > >  		unsigned long line_time_us;
> > >  		int entries;
> > >  
> > > +		if (crtc->primary->state->fb)
> > > +			pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> > > +		else
> > > +			pixel_size = 4;
> > > +
> > >  		line_time_us = max(htotal * 1000 / clock, 1);
> > >  
> > >  		/* Use ns/us then divide to preserve precision */
> > > @@ -1195,7 +1213,13 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
> > >  	crtc = intel_get_crtc_for_plane(dev, 0);
> > >  	if (intel_crtc_active(crtc)) {
> > >  		const struct drm_display_mode *adjusted_mode;
> > > -		int cpp = crtc->primary->state->fb->bits_per_pixel / 8;
> > > +		int cpp;
> > > +
> > > +		if (crtc->primary->state->fb)
> > > +			cpp = crtc->primary->state->fb->bits_per_pixel / 8;
> > > +		else
> > > +			cpp = 4;
> > > +
> > >  		if (IS_GEN2(dev))
> > >  			cpp = 4;
> > >  
> > > @@ -1217,7 +1241,13 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
> > >  	crtc = intel_get_crtc_for_plane(dev, 1);
> > >  	if (intel_crtc_active(crtc)) {
> > >  		const struct drm_display_mode *adjusted_mode;
> > > -		int cpp = crtc->primary->state->fb->bits_per_pixel / 8;
> > > +		int cpp;
> > > +
> > > +		if (crtc->primary->state->fb)
> > > +			cpp = crtc->primary->state->fb->bits_per_pixel / 8;
> > > +		else
> > > +			cpp = 4;
> > > +
> > >  		if (IS_GEN2(dev))
> > >  			cpp = 4;
> > >  
> > > @@ -1243,7 +1273,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
> > >  		obj = intel_fb_obj(enabled->primary->state->fb);
> > >  
> > >  		/* self-refresh seems busted with untiled */
> > > -		if (obj->tiling_mode == I915_TILING_NONE)
> > > +		if (!obj || obj->tiling_mode == I915_TILING_NONE)
> > >  			enabled = NULL;
> > >  	}
> > >  
> > > @@ -1264,10 +1294,15 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
> > >  		int clock = adjusted_mode->crtc_clock;
> > >  		int htotal = adjusted_mode->crtc_htotal;
> > >  		int hdisplay = to_intel_crtc(enabled)->config->pipe_src_w;
> > > -		int pixel_size = enabled->primary->state->fb->bits_per_pixel / 8;
> > > +		int pixel_size;
> > >  		unsigned long line_time_us;
> > >  		int entries;
> > >  
> > > +		if (enabled->primary->state->fb)
> > > +			pixel_size = enabled->primary->state->fb->bits_per_pixel / 8;
> > > +		else
> > > +			pixel_size = 4;
> > > +
> > >  		line_time_us = max(htotal * 1000 / clock, 1);
> > >  
> > >  		/* Use ns/us then divide to preserve precision */
> > 
> > With the change to intel_crtc_active() to look at state->fb I think you
> > could drop all of the changes above, and just keep the changes below.
> 
> Are you sure?  If intel_crtc->active is true, but the primary plane is
> disabled (plane->state->fb == NULL), then I think we're still going to
> wind up trying to dereference plane->state->fb to get bits_per_pixel.

intel_crtc_active() will come out as false in that case and we never end
up looking at the fb.

> 
> In my other branch I do have a patch that just moves all that wm_param
> calculation out of the watermark code here and sticks it in the
> check_plane code, so that we always have up-to-date enabled/bpp/etc
> values collected for the plane state.  If I can extract that from my
> other branch cleanly, that might be a slightly cleaner solution than
> just adding all the fb tests here.

I don't think it's worth looking at the old platform code at this point.
We just want the ilk+ stuff converted to atomic first. I suspect once I
get the VLV/CHV wm code sorted into a better shape we can leverage it
for the other gmch platforms as well.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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)

v3: Fix a couple copy/paste mistakes in SKL code. (Tvrtko)

v4: Only add FB checks for ILK/SKL codepaths.  Older platforms still use
    intel_crtc_active() and will shortcircuit out of watermark
    calculations before ever trying to dereference the primary plane's
    framebuffer.

Cc: Tvrtko Ursulin <tvrtko.ursulin@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 | 62 ++++++++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a06a2c7..7566cec 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1962,13 +1962,25 @@ static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
 	p->active = true;
 	p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
 	p->pixel_rate = ilk_pipe_pixel_rate(dev, crtc);
-	p->pri.bytes_per_pixel = crtc->primary->state->fb->bits_per_pixel / 8;
-	p->cur.bytes_per_pixel = 4;
+
+	if (crtc->primary->state->fb) {
+		p->pri.enabled = true;
+		p->pri.bytes_per_pixel =
+			crtc->primary->state->fb->bits_per_pixel / 8;
+	} else {
+		p->pri.enabled = false;
+		p->pri.bytes_per_pixel = 0;
+	}
+
+	if (crtc->cursor->state->fb) {
+		p->cur.enabled = true;
+		p->cur.bytes_per_pixel = 4;
+	} else {
+		p->cur.enabled = false;
+		p->cur.bytes_per_pixel = 0;
+	}
 	p->pri.horiz_pixels = intel_crtc->config->pipe_src_w;
 	p->cur.horiz_pixels = intel_crtc->base.cursor->state->crtc_w;
-	/* TODO: for now, assume primary and cursor planes are always enabled. */
-	p->pri.enabled = true;
-	p->cur.enabled = true;
 
 	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
 		struct intel_plane *intel_plane = to_intel_plane(plane);
@@ -2734,27 +2746,31 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
 		p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
 		p->pixel_rate = skl_pipe_pixel_rate(intel_crtc->config);
 
-		/*
-		 * For now, assume primary and cursor planes are always enabled.
-		 */
-		p->plane[0].enabled = true;
-		p->plane[0].bytes_per_pixel =
-			crtc->primary->state->fb->bits_per_pixel / 8;
-		p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
-		p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
-		p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
 		fb = crtc->primary->state->fb;
-		/*
-		 * Framebuffer can be NULL on plane disable, but it does not
-		 * matter for watermarks if we assume no tiling in that case.
-		 */
-		if (fb)
+		if (fb) {
+			p->plane[0].enabled = true;
+			p->plane[0].bytes_per_pixel = fb->bits_per_pixel / 8;
 			p->plane[0].tiling = fb->modifier[0];
+		} else {
+			p->plane[0].enabled = false;
+			p->plane[0].bytes_per_pixel = 0;
+			p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
+		}
+		p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
+		p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
 
-		p->cursor.enabled = true;
-		p->cursor.bytes_per_pixel = 4;
-		p->cursor.horiz_pixels = intel_crtc->base.cursor->state->crtc_w ?
-					 intel_crtc->base.cursor->state->crtc_w : 64;
+		fb = crtc->cursor->state->fb;
+		if (fb) {
+			p->cursor.enabled = true;
+			p->cursor.bytes_per_pixel = fb->bits_per_pixel / 8;
+			p->cursor.horiz_pixels = crtc->cursor->state->crtc_w;
+			p->cursor.vert_pixels = crtc->cursor->state->crtc_h;
+		} else {
+			p->cursor.enabled = false;
+			p->cursor.bytes_per_pixel = 0;
+			p->cursor.horiz_pixels = 64;
+			p->cursor.vert_pixels = 64;
+		}
 	}
 
 	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
-- 
1.8.5.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 03/09/2015 06:06 PM, Matt Roper wrote:
> Current ILK-style watermark code assumes the primary plane and cursor
> plane are always enabled.  This assumption, along with the combination
> of two independent commits that got merged at the same time, results in
> a NULL dereference.  The offending commits are:
>
>          commit fd2d61341bf39d1054256c07d6eddd624ebc4241
>          Author: Matt Roper <matthew.d.roper@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)
>
> v3: Fix a couple copy/paste mistakes in SKL code. (Tvrtko)
>
> v4: Only add FB checks for ILK/SKL codepaths.  Older platforms still use
>      intel_crtc_active() and will shortcircuit out of watermark
>      calculations before ever trying to dereference the primary plane's
>      framebuffer.
>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@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 | 62 ++++++++++++++++++++++++++---------------
>   1 file changed, 39 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a06a2c7..7566cec 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1962,13 +1962,25 @@ static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
>   	p->active = true;
>   	p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
>   	p->pixel_rate = ilk_pipe_pixel_rate(dev, crtc);
> -	p->pri.bytes_per_pixel = crtc->primary->state->fb->bits_per_pixel / 8;
> -	p->cur.bytes_per_pixel = 4;
> +
> +	if (crtc->primary->state->fb) {
> +		p->pri.enabled = true;
> +		p->pri.bytes_per_pixel =
> +			crtc->primary->state->fb->bits_per_pixel / 8;
> +	} else {
> +		p->pri.enabled = false;
> +		p->pri.bytes_per_pixel = 0;
> +	}
> +
> +	if (crtc->cursor->state->fb) {
> +		p->cur.enabled = true;
> +		p->cur.bytes_per_pixel = 4;
> +	} else {
> +		p->cur.enabled = false;
> +		p->cur.bytes_per_pixel = 0;
> +	}
>   	p->pri.horiz_pixels = intel_crtc->config->pipe_src_w;
>   	p->cur.horiz_pixels = intel_crtc->base.cursor->state->crtc_w;
> -	/* TODO: for now, assume primary and cursor planes are always enabled. */
> -	p->pri.enabled = true;
> -	p->cur.enabled = true;
>
>   	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
>   		struct intel_plane *intel_plane = to_intel_plane(plane);
> @@ -2734,27 +2746,31 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>   		p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
>   		p->pixel_rate = skl_pipe_pixel_rate(intel_crtc->config);
>
> -		/*
> -		 * For now, assume primary and cursor planes are always enabled.
> -		 */
> -		p->plane[0].enabled = true;
> -		p->plane[0].bytes_per_pixel =
> -			crtc->primary->state->fb->bits_per_pixel / 8;
> -		p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
> -		p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
> -		p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
>   		fb = crtc->primary->state->fb;
> -		/*
> -		 * Framebuffer can be NULL on plane disable, but it does not
> -		 * matter for watermarks if we assume no tiling in that case.
> -		 */
> -		if (fb)
> +		if (fb) {
> +			p->plane[0].enabled = true;
> +			p->plane[0].bytes_per_pixel = fb->bits_per_pixel / 8;
>   			p->plane[0].tiling = fb->modifier[0];
> +		} else {
> +			p->plane[0].enabled = false;
> +			p->plane[0].bytes_per_pixel = 0;
> +			p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
> +		}
> +		p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
> +		p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
>
> -		p->cursor.enabled = true;
> -		p->cursor.bytes_per_pixel = 4;
> -		p->cursor.horiz_pixels = intel_crtc->base.cursor->state->crtc_w ?
> -					 intel_crtc->base.cursor->state->crtc_w : 64;
> +		fb = crtc->cursor->state->fb;
> +		if (fb) {
> +			p->cursor.enabled = true;
> +			p->cursor.bytes_per_pixel = fb->bits_per_pixel / 8;
> +			p->cursor.horiz_pixels = crtc->cursor->state->crtc_w;
> +			p->cursor.vert_pixels = crtc->cursor->state->crtc_h;
> +		} else {
> +			p->cursor.enabled = false;
> +			p->cursor.bytes_per_pixel = 0;
> +			p->cursor.horiz_pixels = 64;
> +			p->cursor.vert_pixels = 64;
> +		}
>   	}
>
>   	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
>

I was nervous about a new possibility of bytes_per_pixel being zero so 
looked if someone could divide by it. ilk_wm_fbc can, but is gated by 
pri.enabled so that is fine and I didn't find any other similar places.

Given that, it looks fine to me.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Tue, Mar 10, 2015 at 10:51:34AM +0000, Tvrtko Ursulin wrote:
> 
> On 03/09/2015 06:06 PM, Matt Roper wrote:
> >Current ILK-style watermark code assumes the primary plane and cursor
> >plane are always enabled.  This assumption, along with the combination
> >of two independent commits that got merged at the same time, results in
> >a NULL dereference.  The offending commits are:
> >
> >         commit fd2d61341bf39d1054256c07d6eddd624ebc4241
> >         Author: Matt Roper <matthew.d.roper@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)
> >
> >v3: Fix a couple copy/paste mistakes in SKL code. (Tvrtko)
> >
> >v4: Only add FB checks for ILK/SKL codepaths.  Older platforms still use
> >     intel_crtc_active() and will shortcircuit out of watermark
> >     calculations before ever trying to dereference the primary plane's
> >     framebuffer.
> >
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@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 | 62 ++++++++++++++++++++++++++---------------
> >  1 file changed, 39 insertions(+), 23 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >index a06a2c7..7566cec 100644
> >--- a/drivers/gpu/drm/i915/intel_pm.c
> >+++ b/drivers/gpu/drm/i915/intel_pm.c
> >@@ -1962,13 +1962,25 @@ static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
> >  	p->active = true;
> >  	p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
> >  	p->pixel_rate = ilk_pipe_pixel_rate(dev, crtc);
> >-	p->pri.bytes_per_pixel = crtc->primary->state->fb->bits_per_pixel / 8;
> >-	p->cur.bytes_per_pixel = 4;
> >+
> >+	if (crtc->primary->state->fb) {
> >+		p->pri.enabled = true;
> >+		p->pri.bytes_per_pixel =
> >+			crtc->primary->state->fb->bits_per_pixel / 8;
> >+	} else {
> >+		p->pri.enabled = false;
> >+		p->pri.bytes_per_pixel = 0;
> >+	}
> >+
> >+	if (crtc->cursor->state->fb) {
> >+		p->cur.enabled = true;
> >+		p->cur.bytes_per_pixel = 4;
> >+	} else {
> >+		p->cur.enabled = false;
> >+		p->cur.bytes_per_pixel = 0;
> >+	}
> >  	p->pri.horiz_pixels = intel_crtc->config->pipe_src_w;
> >  	p->cur.horiz_pixels = intel_crtc->base.cursor->state->crtc_w;
> >-	/* TODO: for now, assume primary and cursor planes are always enabled. */
> >-	p->pri.enabled = true;
> >-	p->cur.enabled = true;
> >
> >  	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
> >  		struct intel_plane *intel_plane = to_intel_plane(plane);
> >@@ -2734,27 +2746,31 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
> >  		p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
> >  		p->pixel_rate = skl_pipe_pixel_rate(intel_crtc->config);
> >
> >-		/*
> >-		 * For now, assume primary and cursor planes are always enabled.
> >-		 */
> >-		p->plane[0].enabled = true;
> >-		p->plane[0].bytes_per_pixel =
> >-			crtc->primary->state->fb->bits_per_pixel / 8;
> >-		p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
> >-		p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
> >-		p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
> >  		fb = crtc->primary->state->fb;
> >-		/*
> >-		 * Framebuffer can be NULL on plane disable, but it does not
> >-		 * matter for watermarks if we assume no tiling in that case.
> >-		 */
> >-		if (fb)
> >+		if (fb) {
> >+			p->plane[0].enabled = true;
> >+			p->plane[0].bytes_per_pixel = fb->bits_per_pixel / 8;
> >  			p->plane[0].tiling = fb->modifier[0];
> >+		} else {
> >+			p->plane[0].enabled = false;
> >+			p->plane[0].bytes_per_pixel = 0;
> >+			p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
> >+		}
> >+		p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
> >+		p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
> >
> >-		p->cursor.enabled = true;
> >-		p->cursor.bytes_per_pixel = 4;
> >-		p->cursor.horiz_pixels = intel_crtc->base.cursor->state->crtc_w ?
> >-					 intel_crtc->base.cursor->state->crtc_w : 64;
> >+		fb = crtc->cursor->state->fb;
> >+		if (fb) {
> >+			p->cursor.enabled = true;
> >+			p->cursor.bytes_per_pixel = fb->bits_per_pixel / 8;
> >+			p->cursor.horiz_pixels = crtc->cursor->state->crtc_w;
> >+			p->cursor.vert_pixels = crtc->cursor->state->crtc_h;
> >+		} else {
> >+			p->cursor.enabled = false;
> >+			p->cursor.bytes_per_pixel = 0;
> >+			p->cursor.horiz_pixels = 64;
> >+			p->cursor.vert_pixels = 64;
> >+		}
> >  	}
> >
> >  	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> >
> 
> I was nervous about a new possibility of bytes_per_pixel being zero so
> looked if someone could divide by it. ilk_wm_fbc can, but is gated by
> pri.enabled so that is fine and I didn't find any other similar places.
> 
> Given that, it looks fine to me.
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5919
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  282/282              282/282
ILK                                  308/308              308/308
SNB                                  307/307              307/307
IVB                                  375/375              375/375
BYT                                  294/294              294/294
HSW                 -1              385/385              384/385
BDW                                  315/315              315/315
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*HSW  igt_gem_storedw_batches_loop_normal      PASS(4)      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