Re: [PATCH 6/9] drm/i915: Use intel state as much as possible in wm code

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

On Thu, Jun 20, 2019 at 11:46:10PM +0200, Maarten Lankhorst wrote:
> Instead of directly referencing drm_crtc_state, convert to
> intel_ctc_state and use the base struct. This is useful when we're
> making the split between uapi and hw state, and also makes the
> code slightly more readable.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 112 ++++++++++++++------------------
>  1 file changed, 50 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 4116de2a77fd..afa069f0dc70 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3857,8 +3857,8 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
>  	struct drm_atomic_state *state = cstate->base.state;
>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	struct drm_crtc *for_crtc = cstate->base.crtc;
> -	const struct drm_crtc_state *crtc_state;
> -	const struct drm_crtc *crtc;
> +	const struct intel_crtc_state *crtc_state;
> +	const struct intel_crtc *crtc;
>  	u32 pipe_width = 0, total_width = 0, width_before_pipe = 0;
>  	enum pipe for_pipe = to_intel_crtc(for_crtc)->pipe;
>  	u16 ddb_size;
> @@ -3901,16 +3901,16 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
>  	 * framebuffer, So instead of allocating DDB equally among pipes
>  	 * distribute DDB based on resolution/width of the display.
>  	 */
> -	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> +	for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i) {
>  		const struct drm_display_mode *adjusted_mode;
>  		int hdisplay, vdisplay;
>  		enum pipe pipe;
>  
> -		if (!crtc_state->enable)
> +		if (!crtc_state->base.enable)
>  			continue;
>  
> -		pipe = to_intel_crtc(crtc)->pipe;
> -		adjusted_mode = &crtc_state->adjusted_mode;
> +		pipe = crtc->pipe;
> +		adjusted_mode = &crtc_state->base.adjusted_mode;

Those two could be done when declaring the variables.

>  		drm_mode_get_hv_timing(adjusted_mode, &hdisplay, &vdisplay);
>  		total_width += hdisplay;
>  
> @@ -4139,11 +4139,9 @@ int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
>  				  struct intel_crtc_state *cstate)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
> -	struct drm_crtc_state *crtc_state = &cstate->base;
> -	struct drm_atomic_state *state = crtc_state->state;
> +	struct drm_atomic_state *state = cstate->base.state;
>  	struct drm_plane *plane;
> -	const struct drm_plane_state *pstate;
> -	struct intel_plane_state *intel_pstate;
> +	const struct drm_plane_state *drm_pstate;
>  	int crtc_clock, dotclk;
>  	u32 pipe_max_pixel_rate;
>  	uint_fixed_16_16_t pipe_downscale;
> @@ -4152,22 +4150,21 @@ int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
>  	if (!cstate->base.enable)
>  		return 0;
>  
> -	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, crtc_state) {
> +	drm_atomic_crtc_state_for_each_plane_state(plane, drm_pstate, &cstate->base) {
>  		uint_fixed_16_16_t plane_downscale;
>  		uint_fixed_16_16_t fp_9_div_8 = div_fixed16(9, 8);
>  		int bpp;
> +		const struct intel_plane_state *pstate =
> +			to_intel_plane_state(drm_pstate);
>  
> -		if (!intel_wm_plane_visible(cstate,
> -					    to_intel_plane_state(pstate)))
> +		if (!intel_wm_plane_visible(cstate, pstate))
>  			continue;
>  
> -		if (WARN_ON(!pstate->fb))
> +		if (WARN_ON(!pstate->base.fb))
>  			return -EINVAL;
>  
> -		intel_pstate = to_intel_plane_state(pstate);
> -		plane_downscale = skl_plane_downscale_amount(cstate,
> -							     intel_pstate);
> -		bpp = pstate->fb->format->cpp[0] * 8;
> +		plane_downscale = skl_plane_downscale_amount(cstate, pstate);
> +		bpp = pstate->base.fb->format->cpp[0] * 8;
>  		if (bpp == 64)
>  			plane_downscale = mul_fixed16(plane_downscale,
>  						      fp_9_div_8);
> @@ -4178,7 +4175,7 @@ int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
>  
>  	pipe_downscale = mul_fixed16(pipe_downscale, max_downscale);
>  
> -	crtc_clock = crtc_state->adjusted_mode.crtc_clock;
> +	crtc_clock = cstate->base.adjusted_mode.crtc_clock;
>  	dotclk = to_intel_atomic_state(state)->cdclk.logical.cdclk;
>  
>  	if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 10)
> @@ -4196,11 +4193,10 @@ int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
>  
>  static u64
>  skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
> -			     const struct intel_plane_state *intel_pstate,
> +			     const struct intel_plane_state *pstate,
>  			     const int plane)

Hmm. Didn't I rename that 'plane' to 'color_plane'? Maybe it was some
other instance, or the patch never got in. Anyways I'd like to see that
done and then we can use 'plane' for the intel_plane.

>  {
> -	struct intel_plane *intel_plane =
> -		to_intel_plane(intel_pstate->base.plane);
> +	struct intel_plane *intel_plane = to_intel_plane(pstate->base.plane);
>  	u32 data_rate;
>  	u32 width = 0, height = 0;
>  	struct drm_framebuffer *fb;
> @@ -4208,10 +4204,10 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
>  	uint_fixed_16_16_t down_scale_amount;
>  	u64 rate;
>  
> -	if (!intel_pstate->base.visible)
> +	if (!pstate->base.visible)
>  		return 0;
>  
> -	fb = intel_pstate->base.fb;
> +	fb = pstate->base.fb;
>  	format = fb->format->format;
>  
>  	if (intel_plane->id == PLANE_CURSOR)
> @@ -4224,8 +4220,8 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
>  	 * the 90/270 degree plane rotation cases (to match the
>  	 * GTT mapping), hence no need to account for rotation here.
>  	 */
> -	width = drm_rect_width(&intel_pstate->base.src) >> 16;
> -	height = drm_rect_height(&intel_pstate->base.src) >> 16;
> +	width = drm_rect_width(&pstate->base.src) >> 16;
> +	height = drm_rect_height(&pstate->base.src) >> 16;
>  
>  	/* UV plane does 1/2 pixel sub-sampling */
>  	if (plane == 1 && is_planar_yuv_format(format)) {
> @@ -4235,7 +4231,7 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
>  
>  	data_rate = width * height;
>  
> -	down_scale_amount = skl_plane_downscale_amount(cstate, intel_pstate);
> +	down_scale_amount = skl_plane_downscale_amount(cstate, pstate);
>  
>  	rate = mul_round_up_u32_fixed16(data_rate, down_scale_amount);
>  
> @@ -4244,35 +4240,32 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
>  }
>  
>  static u64
> -skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate,
> +skl_get_total_relative_data_rate(struct intel_crtc_state *cstate,
>  				 u64 *plane_data_rate,
>  				 u64 *uv_plane_data_rate)
>  {
> -	struct drm_crtc_state *cstate = &intel_cstate->base;
> -	struct drm_atomic_state *state = cstate->state;
> +	struct drm_atomic_state *state = cstate->base.state;

s/cstate/crtc_state/ etc. might be nice in a bunch of these functions.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  	struct drm_plane *plane;
> -	const struct drm_plane_state *pstate;
> +	const struct drm_plane_state *drm_pstate;
>  	u64 total_data_rate = 0;
>  
>  	if (WARN_ON(!state))
>  		return 0;
>  
>  	/* Calculate and cache data rate for each plane */
> -	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, cstate) {
> +	drm_atomic_crtc_state_for_each_plane_state(plane, drm_pstate, &cstate->base) {
>  		enum plane_id plane_id = to_intel_plane(plane)->id;
> +		const struct intel_plane_state *pstate =
> +			to_intel_plane_state(drm_pstate);
>  		u64 rate;
> -		const struct intel_plane_state *intel_pstate =
> -			to_intel_plane_state(pstate);
>  
>  		/* packed/y */
> -		rate = skl_plane_relative_data_rate(intel_cstate,
> -						    intel_pstate, 0);
> +		rate = skl_plane_relative_data_rate(cstate, pstate, 0);
>  		plane_data_rate[plane_id] = rate;
>  		total_data_rate += rate;
>  
>  		/* uv-plane */
> -		rate = skl_plane_relative_data_rate(intel_cstate,
> -						    intel_pstate, 1);
> +		rate = skl_plane_relative_data_rate(cstate, pstate, 1);
>  		uv_plane_data_rate[plane_id] = rate;
>  		total_data_rate += rate;
>  	}
> @@ -4281,28 +4274,25 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate,
>  }
>  
>  static u64
> -icl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate,
> +icl_get_total_relative_data_rate(struct intel_crtc_state *cstate,
>  				 u64 *plane_data_rate)
>  {
> -	struct drm_crtc_state *cstate = &intel_cstate->base;
> -	struct drm_atomic_state *state = cstate->state;
>  	struct drm_plane *plane;
> -	const struct drm_plane_state *pstate;
> +	const struct drm_plane_state *drm_pstate;
>  	u64 total_data_rate = 0;
>  
> -	if (WARN_ON(!state))
> +	if (WARN_ON(!cstate->base.state))
>  		return 0;
>  
>  	/* Calculate and cache data rate for each plane */
> -	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, cstate) {
> -		const struct intel_plane_state *intel_pstate =
> -			to_intel_plane_state(pstate);
> +	drm_atomic_crtc_state_for_each_plane_state(plane, drm_pstate, &cstate->base) {
> +		const struct intel_plane_state *pstate =
> +			to_intel_plane_state(drm_pstate);
>  		enum plane_id plane_id = to_intel_plane(plane)->id;
>  		u64 rate;
>  
> -		if (!intel_pstate->linked_plane) {
> -			rate = skl_plane_relative_data_rate(intel_cstate,
> -							    intel_pstate, 0);
> +		if (!pstate->linked_plane) {
> +			rate = skl_plane_relative_data_rate(cstate, pstate, 0);
>  			plane_data_rate[plane_id] = rate;
>  			total_data_rate += rate;
>  		} else {
> @@ -4315,18 +4305,16 @@ icl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate,
>  			 * NULL if we try get_new_plane_state(), so we
>  			 * always calculate from the master.
>  			 */
> -			if (intel_pstate->slave)
> +			if (pstate->slave)
>  				continue;
>  
>  			/* Y plane rate is calculated on the slave */
> -			rate = skl_plane_relative_data_rate(intel_cstate,
> -							    intel_pstate, 0);
> -			y_plane_id = intel_pstate->linked_plane->id;
> +			rate = skl_plane_relative_data_rate(cstate, pstate, 0);
> +			y_plane_id = pstate->linked_plane->id;
>  			plane_data_rate[y_plane_id] = rate;
>  			total_data_rate += rate;
>  
> -			rate = skl_plane_relative_data_rate(intel_cstate,
> -							    intel_pstate, 1);
> +			rate = skl_plane_relative_data_rate(cstate, pstate, 1);
>  			plane_data_rate[plane_id] = rate;
>  			total_data_rate += rate;
>  		}
> @@ -5095,9 +5083,8 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
>  	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
> -	struct drm_crtc_state *crtc_state = &cstate->base;
>  	struct drm_plane *plane;
> -	const struct drm_plane_state *pstate;
> +	const struct drm_plane_state *drm_pstate;
>  	int ret;
>  
>  	/*
> @@ -5106,14 +5093,15 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate)
>  	 */
>  	memset(pipe_wm->planes, 0, sizeof(pipe_wm->planes));
>  
> -	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, crtc_state) {
> -		const struct intel_plane_state *intel_pstate =
> -						to_intel_plane_state(pstate);
> +	drm_atomic_crtc_state_for_each_plane_state(plane, drm_pstate,
> +						   &cstate->base) {
> +		const struct intel_plane_state *pstate =
> +			to_intel_plane_state(drm_pstate);
>  
>  		if (INTEL_GEN(dev_priv) >= 11)
> -			ret = icl_build_plane_wm(cstate, intel_pstate);
> +			ret = icl_build_plane_wm(cstate, pstate);
>  		else
> -			ret = skl_build_plane_wm(cstate, intel_pstate);
> +			ret = skl_build_plane_wm(cstate, pstate);
>  		if (ret)
>  			return ret;
>  	}
> -- 
> 2.20.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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