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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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