Re: [PATCH 06/14] drm/i915/skl: Eliminate usage of pipe_wm_parameters from SKL-style WM (v2)

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Hey,

Op 15-09-15 om 04:19 schreef Matt Roper:
> Just pull the info out of the state structures rather than staging
> it in an additional set of structures.  To make this more
> straightforward, we change the signature of several internal WM
> functions to take the crtc state as a parameter.
>
> v2:
>  - Don't forget to skip cursor planes on a loop in the DDB allocation
>    function to match original behavior.  (Ander)
>  - Change a use of intel_crtc->active to cstate->active.  They should
>    be identical, but it's better to be consistent.  (Ander)
>  - Rework more function signatures to pass states rather than crtc for
>    consistency. (Ander)
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 326 +++++++++++++++++++---------------------
>  1 file changed, 151 insertions(+), 175 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 7115368..51dbda7 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1787,13 +1787,6 @@ static uint32_t ilk_wm_fbc(uint32_t pri_val, uint32_t horiz_pixels,
>  	return DIV_ROUND_UP(pri_val * 64, horiz_pixels * bytes_per_pixel) + 2;
>  }
>  
> -struct skl_pipe_wm_parameters {
> -	bool active;
> -	uint32_t pipe_htotal;
> -	uint32_t pixel_rate; /* in KHz */
> -	struct intel_plane_wm_parameters plane[I915_MAX_PLANES];
> -};
> -
>  struct ilk_wm_maximums {
>  	uint16_t pri;
>  	uint16_t spr;
> @@ -2834,18 +2827,40 @@ static bool ilk_disable_lp_wm(struct drm_device *dev)
>  #define SKL_DDB_SIZE		896	/* in blocks */
>  #define BXT_DDB_SIZE		512
>  
> +/*
> + * Return the index of a plane in the SKL DDB and wm result arrays.  Primary
> + * plane is always in slot 0, cursor is always in slot I915_MAX_PLANES-1, and
> + * other universal planes are in indices 1..n.  Note that this may leave unused
> + * indices between the top "sprite" plane and the cursor.
> + */
> +static int
> +skl_wm_plane_id(const struct intel_plane *plane)
> +{
> +	switch (plane->base.type) {
> +	case DRM_PLANE_TYPE_PRIMARY:
> +		return 0;
> +	case DRM_PLANE_TYPE_CURSOR:
> +		return PLANE_CURSOR;
> +	case DRM_PLANE_TYPE_OVERLAY:
> +		return plane->plane;
Shouldn't this be plane + 1 on skylake? Though it's not the only code in i915 that gets it wrong,
assert_sprites_disabled has the same issue on SKL.. O:-)

> +	default:
> +		MISSING_CASE(plane->base.type);
> +		return plane->plane;
> +	}
> +}
> +
>  static void
>  skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
> -				   struct drm_crtc *for_crtc,
> +				   const struct intel_crtc_state *cstate,
>  				   const struct intel_wm_config *config,
> -				   const struct skl_pipe_wm_parameters *params,
>  				   struct skl_ddb_entry *alloc /* out */)
>  {
> +	struct drm_crtc *for_crtc = cstate->base.crtc;
>  	struct drm_crtc *crtc;
>  	unsigned int pipe_size, ddb_size;
>  	int nth_active_pipe;
>  
> -	if (!params->active) {
> +	if (!cstate->base.active) {
>  		alloc->start = 0;
>  		alloc->end = 0;
>  		return;
> @@ -2911,19 +2926,29 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>  }
>  
>  static unsigned int
> -skl_plane_relative_data_rate(const struct intel_plane_wm_parameters *p, int y)
> +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 drm_framebuffer *fb = pstate->fb;
>  
>  	/* for planar format */
> -	if (p->y_bytes_per_pixel) {
> +	if (fb->pixel_format == DRM_FORMAT_NV12) {
>  		if (y)  /* y-plane data rate */
> -			return p->horiz_pixels * p->vert_pixels * p->y_bytes_per_pixel;
> +			return intel_crtc->config->pipe_src_w *
> +				intel_crtc->config->pipe_src_h *
> +				drm_format_plane_cpp(fb->pixel_format, 0);
>  		else    /* uv-plane data rate */
> -			return (p->horiz_pixels/2) * (p->vert_pixels/2) * p->bytes_per_pixel;
> +			return (intel_crtc->config->pipe_src_w/2) *
> +				(intel_crtc->config->pipe_src_h/2) *
> +				drm_format_plane_cpp(fb->pixel_format, 1);
>  	}
>  
>  	/* for packed formats */
> -	return p->horiz_pixels * p->vert_pixels * p->bytes_per_pixel;
> +	return intel_crtc->config->pipe_src_w *
> +		intel_crtc->config->pipe_src_h *
> +		drm_format_plane_cpp(fb->pixel_format, 1);
^Gives a nasty divide by zero error on my skl when total_data_rate ends up being 0..
You want to change last argument to 0 here. ;-)
>  }
>  
>  /*
> @@ -2932,46 +2957,51 @@ skl_plane_relative_data_rate(const struct intel_plane_wm_parameters *p, int y)
>   *   3 * 4096 * 8192  * 4 < 2^32
>   */
>  static unsigned int
> -skl_get_total_relative_data_rate(struct intel_crtc *intel_crtc,
> -				 const struct skl_pipe_wm_parameters *params)
> +skl_get_total_relative_data_rate(const struct intel_crtc_state *cstate)
>  {
> +	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
> +	struct drm_device *dev = intel_crtc->base.dev;
> +	const struct intel_plane *intel_plane;
>  	unsigned int total_data_rate = 0;
> -	int plane;
>  
> -	for (plane = 0; plane < intel_num_planes(intel_crtc); plane++) {
> -		const struct intel_plane_wm_parameters *p;
> +	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> +		const struct drm_plane_state *pstate = intel_plane->base.state;
>  
> -		p = &params->plane[plane];
> -		if (!p->enabled)
> +		if (WARN_ON(pstate->fb == NULL))
>  			continue;
^Why is this a WARN_ON?

All in all, the series looks good.

With those fixes feel free to add this to all patches:
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx