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

[Ville Syrjälä]

On Mon, Jan 11, 2016 at 02:13:06PM -0800, Matt Roper wrote:
> 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."

The pixel rate is "downscale factor * pixel clock"

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

-- 
Ville Syrjälä
Intel OTC