[Intel-gfx] [PATCH] drm/i915/skl: Use proper plane dimensions for DDB and WM calculations

[Matt Roper]

On Mon, Jan 11, 2016 at 09:31:03PM +0200, Ville Syrjälä wrote:
> On Mon, Dec 21, 2015 at 07:31:17AM -0800, Matt Roper wrote:
> > In commit
> > 
> >         commit 024c9045221fe45482863c47c4b4c47d37f97cbf
> >         Author: Matt Roper <matthew.d.roper at intel.com>
> >         Date:   Thu Sep 24 15:53:11 2015 -0700
> > 
> >             drm/i915/skl: Eliminate usage of pipe_wm_parameters from SKL-style WM (v4)
> > 
> > I fumbled while converting the dimensions stored in the plane_parameters
> > structure to the values stored in plane state and accidentally replaced
> > the plane dimensions with the pipe dimensions in both the DDB allocation
> > function and the WM calculation function.  On the DDB side this is
> > harmless since we effectively treat all of our non-cursor planes as
> > full-screen which may not be optimal, but generally won't cause any
> > problems either (and in 99% of the cases where there's no sprite plane
> > usage or primary plane windowing, there's no effect at all).  On the WM
> > calculation side there's more potential for this fumble to cause actual
> > problems since cursors also get miscalculated.
> > 
> > Cc: Ville Syrjälä <ville.syrjala at intel.com>
> > Cc: "Kondapally, Kalyan" <kalyan.kondapally at intel.com>
> > Cc: Radhakrishna Sripada <radhakrishna.sripada at intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 24 +++++++++++++-----------
> >  1 file changed, 13 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 8d0d6f5..f4d4cc7 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2845,25 +2845,22 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
> >  			     const struct drm_plane_state *pstate,
> >  			     int y)
> >  {
> > -	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
> > +	struct intel_plane_state *intel_pstate = to_intel_plane_state(pstate);
> >  	struct drm_framebuffer *fb = pstate->fb;
> > +	unsigned w = drm_rect_width(&intel_pstate->dst);
> > +	unsigned h = drm_rect_height(&intel_pstate->dst);
> 
> I think you're supposed to use the src dimensions in most places.

Hmm, just went back to double check the bpsec and if I'm interpreting it
correctly, it looks like we actually need to use the larger of the two:
"Down scaling effectively increases the pixel rate. Up scaling does not
reduce the pixel rate."

Thanks for pointing that out; I'll send an updated patch.



Matt

> 
> >  
> >  	/* for planar format */
> >  	if (fb->pixel_format == DRM_FORMAT_NV12) {
> >  		if (y)  /* y-plane data rate */
> > -			return intel_crtc->config->pipe_src_w *
> > -				intel_crtc->config->pipe_src_h *
> > -				drm_format_plane_cpp(fb->pixel_format, 0);
> > +			return w * h * drm_format_plane_cpp(fb->pixel_format, 0);
> >  		else    /* uv-plane data rate */
> > -			return (intel_crtc->config->pipe_src_w/2) *
> > -				(intel_crtc->config->pipe_src_h/2) *
> > +			return (w/2) * (h/2) *
> >  				drm_format_plane_cpp(fb->pixel_format, 1);
> >  	}
> >  
> >  	/* for packed formats */
> > -	return intel_crtc->config->pipe_src_w *
> > -		intel_crtc->config->pipe_src_h *
> > -		drm_format_plane_cpp(fb->pixel_format, 0);
> > +	return w * h * drm_format_plane_cpp(fb->pixel_format, 0);
> >  }
> >  
> >  /*
> > @@ -2960,6 +2957,8 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> >  	 * FIXME: we may not allocate every single block here.
> >  	 */
> >  	total_data_rate = skl_get_total_relative_data_rate(cstate);
> > +	if (!total_data_rate)
> > +		return;
> >  
> >  	start = alloc->start;
> >  	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> > @@ -3093,12 +3092,15 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> >  {
> >  	struct drm_plane *plane = &intel_plane->base;
> >  	struct drm_framebuffer *fb = plane->state->fb;
> > +	struct intel_plane_state *intel_pstate =
> > +		to_intel_plane_state(plane->state);
> >  	uint32_t latency = dev_priv->wm.skl_latency[level];
> >  	uint32_t method1, method2;
> >  	uint32_t plane_bytes_per_line, plane_blocks_per_line;
> >  	uint32_t res_blocks, res_lines;
> >  	uint32_t selected_result;
> >  	uint8_t bytes_per_pixel;
> > +	unsigned w = drm_rect_width(&intel_pstate->dst);
> >  
> >  	if (latency == 0 || !cstate->base.active || !fb)
> >  		return false;
> > @@ -3109,12 +3111,12 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> >  				 latency);
> >  	method2 = skl_wm_method2(skl_pipe_pixel_rate(cstate),
> >  				 cstate->base.adjusted_mode.crtc_htotal,
> > -				 cstate->pipe_src_w,
> > +				 w,
> >  				 bytes_per_pixel,
> >  				 fb->modifier[0],
> >  				 latency);
> >  
> > -	plane_bytes_per_line = cstate->pipe_src_w * bytes_per_pixel;
> > +	plane_bytes_per_line = w * bytes_per_pixel;
> >  	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
> >  
> >  	if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> > -- 
> > 2.1.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795