Re: [PATCH 4/6] drm/i915/gen9: Make skl_wm_level per-plane

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

Op 05-10-16 om 22:33 schreef Paulo Zanoni:
> Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu:
>> Having skl_wm_level contain all of the watermarks for each plane is
>> annoying since it prevents us from having any sort of object to
>> represent a single watermark level, something we take advantage of in
>> the next commit to cut down on all of the copy paste code in here.
> I'd like to start my review pointing that I really like this patch. I
> agree the current form is annoying.
>
> See below for some details.
>
>
>> Signed-off-by: Lyude <cpaul@redhat.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Matt Roper <matthew.d.roper@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h  |   6 +-
>>  drivers/gpu/drm/i915/intel_drv.h |   6 +-
>>  drivers/gpu/drm/i915/intel_pm.c  | 208 +++++++++++++++++----------
>> ------------
>>  3 files changed, 100 insertions(+), 120 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index d26e5999..0f97d43 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1648,9 +1648,9 @@ struct skl_wm_values {
>>  };
>>  
>>  struct skl_wm_level {
>> -	bool plane_en[I915_MAX_PLANES];
>> -	uint16_t plane_res_b[I915_MAX_PLANES];
>> -	uint8_t plane_res_l[I915_MAX_PLANES];
>> +	bool plane_en;
>> +	uint16_t plane_res_b;
>> +	uint8_t plane_res_l;
>>  };
>>  
>>  /*
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 35ba282..d684f4f 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -468,9 +468,13 @@ struct intel_pipe_wm {
>>  	bool sprites_scaled;
>>  };
>>  
>> -struct skl_pipe_wm {
>> +struct skl_plane_wm {
>>  	struct skl_wm_level wm[8];
>>  	struct skl_wm_level trans_wm;
>> +};
>> +
>> +struct skl_pipe_wm {
>> +	struct skl_plane_wm planes[I915_MAX_PLANES];
>>  	uint32_t linetime;
>>  };
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>> b/drivers/gpu/drm/i915/intel_pm.c
>> index af96888..250f12d 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3668,67 +3668,52 @@ static int
>>  skl_compute_wm_level(const struct drm_i915_private *dev_priv,
>>  		     struct skl_ddb_allocation *ddb,
>>  		     struct intel_crtc_state *cstate,
>> +		     struct intel_plane *intel_plane,
>>  		     int level,
>>  		     struct skl_wm_level *result)
>>  {
>>  	struct drm_atomic_state *state = cstate->base.state;
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
>>> base.crtc);
>> -	struct drm_plane *plane;
>> -	struct intel_plane *intel_plane;
>> -	struct intel_plane_state *intel_pstate;
>> +	struct drm_plane *plane = &intel_plane->base;
>> +	struct intel_plane_state *intel_pstate = NULL;
>>  	uint16_t ddb_blocks;
>>  	enum pipe pipe = intel_crtc->pipe;
>>  	int ret;
>> +	int i = skl_wm_plane_id(intel_plane);
>> +
>> +	if (state)
>> +		intel_pstate =
>> +			intel_atomic_get_existing_plane_state(state,
>> +							      intel_
>> plane);
>>  
>>  	/*
>> -	 * We'll only calculate watermarks for planes that are
>> actually
>> -	 * enabled, so make sure all other planes are set as
>> disabled.
>> +	 * Note: If we start supporting multiple pending atomic
>> commits against
>> +	 * the same planes/CRTC's in the future, plane->state will
>> no longer be
>> +	 * the correct pre-state to use for the calculations here
>> and we'll
>> +	 * need to change where we get the 'unchanged' plane data
>> from.
>> +	 *
>> +	 * For now this is fine because we only allow one queued
>> commit against
>> +	 * a CRTC.  Even if the plane isn't modified by this
>> transaction and we
>> +	 * don't have a plane lock, we still have the CRTC's lock,
>> so we know
>> +	 * that no other transactions are racing with us to update
>> it.
>>  	 */
>> -	memset(result, 0, sizeof(*result));
>> -
>> -	for_each_intel_plane_mask(&dev_priv->drm,
>> -				  intel_plane,
>> -				  cstate->base.plane_mask) {
>> -		int i = skl_wm_plane_id(intel_plane);
>> -
>> -		plane = &intel_plane->base;
>> -		intel_pstate = NULL;
>> -		if (state)
>> -			intel_pstate =
>> -				intel_atomic_get_existing_plane_stat
>> e(state,
>> -								    
>>   intel_plane);
>> +	if (!intel_pstate)
>> +		intel_pstate = to_intel_plane_state(plane->state);
>>  
>> -		/*
>> -		 * Note: If we start supporting multiple pending
>> atomic commits
>> -		 * against the same planes/CRTC's in the future,
>> plane->state
>> -		 * will no longer be the correct pre-state to use
>> for the
>> -		 * calculations here and we'll need to change where
>> we get the
>> -		 * 'unchanged' plane data from.
>> -		 *
>> -		 * For now this is fine because we only allow one
>> queued commit
>> -		 * against a CRTC.  Even if the plane isn't modified
>> by this
>> -		 * transaction and we don't have a plane lock, we
>> still have
>> -		 * the CRTC's lock, so we know that no other
>> transactions are
>> -		 * racing with us to update it.
>> -		 */
>> -		if (!intel_pstate)
>> -			intel_pstate = to_intel_plane_state(plane-
>>> state);
>> +	WARN_ON(!intel_pstate->base.fb);
>>  
>> -		WARN_ON(!intel_pstate->base.fb);
>> +	ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);
>>  
>> -		ddb_blocks = skl_ddb_entry_size(&ddb-
>>> plane[pipe][i]);
>> -
>> -		ret = skl_compute_plane_wm(dev_priv,
>> -					   cstate,
>> -					   intel_pstate,
>> -					   ddb_blocks,
>> -					   level,
>> -					   &result->plane_res_b[i],
>> -					   &result->plane_res_l[i],
>> -					   &result->plane_en[i]);
>> -		if (ret)
>> -			return ret;
>> -	}
>> +	ret = skl_compute_plane_wm(dev_priv,
>> +				   cstate,
>> +				   intel_pstate,
>> +				   ddb_blocks,
>> +				   level,
>> +				   &result->plane_res_b,
>> +				   &result->plane_res_l,
>> +				   &result->plane_en);
>> +	if (ret)
>> +		return ret;
>>  
>>  	return 0;
>>  }
>> @@ -3749,19 +3734,11 @@ skl_compute_linetime_wm(struct
>> intel_crtc_state *cstate)
>>  static void skl_compute_transition_wm(struct intel_crtc_state
>> *cstate,
>>  				      struct skl_wm_level *trans_wm
>> /* out */)
>>  {
>> -	struct drm_crtc *crtc = cstate->base.crtc;
>> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> -	struct intel_plane *intel_plane;
>> -
>>  	if (!cstate->base.active)
>>  		return;
>>  
>>  	/* Until we know more, just disable transition WMs */
>> -	for_each_intel_plane_on_crtc(crtc->dev, intel_crtc,
>> intel_plane) {
>> -		int i = skl_wm_plane_id(intel_plane);
>> -
>> -		trans_wm->plane_en[i] = false;
>> -	}
>> +	trans_wm->plane_en = false;
>>  }
>>  
>>  static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
>> @@ -3770,19 +3747,33 @@ static int skl_build_pipe_wm(struct
>> intel_crtc_state *cstate,
>>  {
>>  	struct drm_device *dev = cstate->base.crtc->dev;
>>  	const struct drm_i915_private *dev_priv = to_i915(dev);
>> +	struct intel_plane *intel_plane;
>> +	struct skl_plane_wm *wm;
>>  	int level, max_level = ilk_wm_max_level(dev);
>>  	int ret;
>>  
>> -	for (level = 0; level <= max_level; level++) {
>> -		ret = skl_compute_wm_level(dev_priv, ddb, cstate,
>> -					   level, &pipe_wm-
>>> wm[level]);
>> -		if (ret)
>> -			return ret;
>> +	/*
>> +	 * We'll only calculate watermarks for planes that are
>> actually
>> +	 * enabled, so make sure all other planes are set as
>> disabled.
>> +	 */
>> +	memset(pipe_wm->planes, 0, sizeof(pipe_wm->planes));
>> +
>> +	for_each_intel_plane_mask(&dev_priv->drm,
>> +				  intel_plane,
>> +				  cstate->base.plane_mask) {
>> +		wm = &pipe_wm->planes[skl_wm_plane_id(intel_plane)];
>> +
>> +		for (level = 0; level <= max_level; level++) {
>> +			ret = skl_compute_wm_level(dev_priv, ddb,
>> cstate,
>> +						   intel_plane,
>> level,
>> +						   &wm->wm[level]);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +		skl_compute_transition_wm(cstate, &wm->trans_wm);
>>  	}
>>  	pipe_wm->linetime = skl_compute_linetime_wm(cstate);
>>  
>> -	skl_compute_transition_wm(cstate, &pipe_wm->trans_wm);
>> -
>>  	return 0;
>>  }
>>  
>> @@ -3792,50 +3783,56 @@ static void skl_compute_wm_results(struct
>> drm_device *dev,
>>  				   struct intel_crtc *intel_crtc)
>>  {
>>  	int level, max_level = ilk_wm_max_level(dev);
>> +	struct skl_plane_wm *plane_wm;
>>  	enum pipe pipe = intel_crtc->pipe;
>>  	uint32_t temp;
>>  	int i;
>>  
>> -	for (level = 0; level <= max_level; level++) {
>> -		for (i = 0; i < intel_num_planes(intel_crtc); i++) {
>> +	for (i = 0; i < intel_num_planes(intel_crtc); i++) {
>> +		plane_wm = &p_wm->planes[i];
>> +
>> +		for (level = 0; level <= max_level; level++) {
>>  			temp = 0;
>>  
>> -			temp |= p_wm->wm[level].plane_res_l[i] <<
>> +			temp |= plane_wm->wm[level].plane_res_l <<
>>  					PLANE_WM_LINES_SHIFT;
>> -			temp |= p_wm->wm[level].plane_res_b[i];
>> -			if (p_wm->wm[level].plane_en[i])
>> +			temp |= plane_wm->wm[level].plane_res_b;
>> +			if (plane_wm->wm[level].plane_en)
>>  				temp |= PLANE_WM_EN;
>>  
>>  			r->plane[pipe][i][level] = temp;
>>  		}
>>  
>> -		temp = 0;
>> -
>> -		temp |= p_wm->wm[level].plane_res_l[PLANE_CURSOR] <<
>> PLANE_WM_LINES_SHIFT;
>> -		temp |= p_wm->wm[level].plane_res_b[PLANE_CURSOR];
>> +	}
>>  
>> -		if (p_wm->wm[level].plane_en[PLANE_CURSOR])
>> +	for (level = 0; level <= max_level; level++) {
>> +		plane_wm = &p_wm->planes[PLANE_CURSOR];
>> +		temp = 0;
>> +		temp |= plane_wm->wm[level].plane_res_l <<
>> PLANE_WM_LINES_SHIFT;
>> +		temp |= plane_wm->wm[level].plane_res_b;
>> +		if (plane_wm->wm[level].plane_en)
>>  			temp |= PLANE_WM_EN;
>>  
>>  		r->plane[pipe][PLANE_CURSOR][level] = temp;
>> -
>>  	}
>>  
>>  	/* transition WMs */
>>  	for (i = 0; i < intel_num_planes(intel_crtc); i++) {
>> +		plane_wm = &p_wm->planes[i];
>>  		temp = 0;
>> -		temp |= p_wm->trans_wm.plane_res_l[i] <<
>> PLANE_WM_LINES_SHIFT;
>> -		temp |= p_wm->trans_wm.plane_res_b[i];
>> -		if (p_wm->trans_wm.plane_en[i])
>> +		temp |= plane_wm->trans_wm.plane_res_l <<
>> PLANE_WM_LINES_SHIFT;
>> +		temp |= plane_wm->trans_wm.plane_res_b;
>> +		if (plane_wm->trans_wm.plane_en)
>>  			temp |= PLANE_WM_EN;
>>  
>>  		r->plane_trans[pipe][i] = temp;
>>  	}
>>  
>> +	plane_wm = &p_wm->planes[PLANE_CURSOR];
>>  	temp = 0;
>> -	temp |= p_wm->trans_wm.plane_res_l[PLANE_CURSOR] <<
>> PLANE_WM_LINES_SHIFT;
>> -	temp |= p_wm->trans_wm.plane_res_b[PLANE_CURSOR];
>> -	if (p_wm->trans_wm.plane_en[PLANE_CURSOR])
>> +	temp |= plane_wm->trans_wm.plane_res_l <<
>> PLANE_WM_LINES_SHIFT;
>> +	temp |= plane_wm->trans_wm.plane_res_b;
>> +	if (plane_wm->trans_wm.plane_en)
>>  		temp |= PLANE_WM_EN;
>>  
>>  	r->plane_trans[pipe][PLANE_CURSOR] = temp;
>> @@ -4262,44 +4259,24 @@ static void ilk_optimize_watermarks(struct
>> intel_crtc_state *cstate)
>>  static void skl_pipe_wm_active_state(uint32_t val,
>>  				     struct skl_pipe_wm *active,
>>  				     bool is_transwm,
>> -				     bool is_cursor,
>>  				     int i,
>>  				     int level)
>>  {
>> +	struct skl_plane_wm *plane_wm = &active->planes[i];
>>  	bool is_enabled = (val & PLANE_WM_EN) != 0;
>>  
>>  	if (!is_transwm) {
>> -		if (!is_cursor) {
>> -			active->wm[level].plane_en[i] = is_enabled;
>> -			active->wm[level].plane_res_b[i] =
>> -					val & PLANE_WM_BLOCKS_MASK;
>> -			active->wm[level].plane_res_l[i] =
>> -					(val >>
>> PLANE_WM_LINES_SHIFT) &
>> -						PLANE_WM_LINES_MASK;
>> -		} else {
>> -			active->wm[level].plane_en[PLANE_CURSOR] =
>> is_enabled;
>> -			active->wm[level].plane_res_b[PLANE_CURSOR]
>> =
>> -					val & PLANE_WM_BLOCKS_MASK;
>> -			active->wm[level].plane_res_l[PLANE_CURSOR]
>> =
>> -					(val >>
>> PLANE_WM_LINES_SHIFT) &
>> -						PLANE_WM_LINES_MASK;
>> -		}
>> +		plane_wm->wm[level].plane_en = is_enabled;
>> +		plane_wm->wm[level].plane_res_b = val &
>> PLANE_WM_BLOCKS_MASK;
>> +		plane_wm->wm[level].plane_res_l =
>> +			(val >> PLANE_WM_LINES_SHIFT) &
>> +			PLANE_WM_LINES_MASK;
> Nitpick: you can join the two lines above and still stay under 80
> columns.
>
>
>>  	} else {
>> -		if (!is_cursor) {
>> -			active->trans_wm.plane_en[i] = is_enabled;
>> -			active->trans_wm.plane_res_b[i] =
>> -					val & PLANE_WM_BLOCKS_MASK;
>> -			active->trans_wm.plane_res_l[i] =
>> -					(val >>
>> PLANE_WM_LINES_SHIFT) &
>> -						PLANE_WM_LINES_MASK;
>> -		} else {
>> -			active->trans_wm.plane_en[PLANE_CURSOR] =
>> is_enabled;
>> -			active->trans_wm.plane_res_b[PLANE_CURSOR] =
>> -					val & PLANE_WM_BLOCKS_MASK;
>> -			active->trans_wm.plane_res_l[PLANE_CURSOR] =
>> -					(val >>
>> PLANE_WM_LINES_SHIFT) &
>> -						PLANE_WM_LINES_MASK;
>> -		}
>> +		plane_wm->trans_wm.plane_en = is_enabled;
>> +		plane_wm->trans_wm.plane_res_b = val &
>> PLANE_WM_BLOCKS_MASK;
>> +		plane_wm->trans_wm.plane_res_l =
>> +			(val >> PLANE_WM_LINES_SHIFT) &
>> +			PLANE_WM_LINES_MASK;
> Same here.
>
>
>>  	}
>>  }
>>  
>> @@ -4338,20 +4315,19 @@ static void skl_pipe_wm_get_hw_state(struct
>> drm_crtc *crtc)
>>  	for (level = 0; level <= max_level; level++) {
>>  		for (i = 0; i < intel_num_planes(intel_crtc); i++) {
>>  			temp = hw->plane[pipe][i][level];
>> -			skl_pipe_wm_active_state(temp, active,
>> false,
>> -						false, i, level);
>> +			skl_pipe_wm_active_state(temp, active,
>> false, i, level);
>>  		}
>>  		temp = hw->plane[pipe][PLANE_CURSOR][level];
>> -		skl_pipe_wm_active_state(temp, active, false, true,
>> i, level);
>> +		skl_pipe_wm_active_state(temp, active, false, i,
>> level);
> While this is not wrong today, history shows that the number of planes
> increases over time, so we may at some point in the future add PLANE_D
> and more, so the code will become wrong. Just pass PLANE_CURSOR instead
> of "i" here and below. Also, this simplification could have been a
> separate patch.
Agreed, but I want to note that PLANE_CURSOR is always supposed to be the last member.
Unless you have sprite planes covering the cursor, which doesn't ever happen.

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx