diff mbox

[v3,1/9] drm/i915/skl: pass pipe_wm in skl_compute_(wm_level/plane_wm) functions

Message ID 20160909080106.17506-2-mahesh1.kumar@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kumar, Mahesh Sept. 9, 2016, 8 a.m. UTC 
From: Mahesh Kumar <mahesh1.kumar@intel.com>

This patch make use of plane_wm variable directly instead of passing
skl_plane_wm struct. this way reduces number of argument requirement
in watermark calculation functions.

It also gives more freedom of decision making to implement Bspec WM
workarounds.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

Comments

Zanoni, Paulo R Sept. 20, 2016, 12:17 p.m. UTC | #1 
Em Sex, 2016-09-09 às 13:30 +0530, Kumar, Mahesh escreveu:
> From: Mahesh Kumar <mahesh1.kumar@intel.com>
> 
> This patch make use of plane_wm variable directly instead of passing
> skl_plane_wm struct. this way reduces number of argument requirement
> in watermark calculation functions.
> 
> It also gives more freedom of decision making to implement Bspec WM
> workarounds.

This is just my personal opinion, but I don't think this patch is an
improvement to the codebase. The way things are currently organized,
there's no risk that we may end up reading some variable that's still
not set/computed, so there's less risk that some later patch may end up
using information it shouldn't. Also, by having these explicit out
parameters, we clearly highlight what's the goal of the function:
output those 3 values, instead of writing I-don't-know-what to a huge
struct.

Besides, the patch even keeps the out_ variables as local variables
instead of parameters, which makes things even more confusing IMHO,
since in_ and out_ variables are usually function parameters.

I also see that this patch is not necessary for the series. We kinda
use the new pipe_wm variable at patch 9, but we could just go with the
variables we have.

So, unless some new arguments are given, I'd suggest to just drop this
patch.

> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 86c6d66..3fdec4d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3542,9 +3542,7 @@ static int skl_compute_plane_wm(const struct
> drm_i915_private *dev_priv,
>  				struct intel_plane_state
> *intel_pstate,
>  				uint16_t ddb_allocation,
>  				int level,
> -				uint16_t *out_blocks, /* out */
> -				uint8_t *out_lines, /* out */
> -				bool *enabled /* out */)
> +				struct skl_pipe_wm *pipe_wm)
>  {
>  	struct drm_plane_state *pstate = &intel_pstate->base;
>  	struct drm_framebuffer *fb = pstate->fb;
> @@ -3557,6 +3555,11 @@ static int skl_compute_plane_wm(const struct
> drm_i915_private *dev_priv,
>  	uint32_t width = 0, height = 0;
>  	uint32_t plane_pixel_rate;
>  	uint32_t y_tile_minimum, y_min_scanlines;
> +	int id = skl_wm_plane_id(to_intel_plane(pstate->plane));
> +	struct skl_wm_level *result = &pipe_wm->wm[level];
> +	uint16_t *out_blocks = &result->plane_res_b[id];
> +	uint8_t *out_lines = &result->plane_res_l[id];
> +	bool *enabled = &result->plane_en[id];
>  
>  	if (latency == 0 || !cstate->base.active || !intel_pstate-
> >base.visible) {
>  		*enabled = false;
> @@ -3673,7 +3676,7 @@ skl_compute_wm_level(const struct
> drm_i915_private *dev_priv,
>  		     struct skl_ddb_allocation *ddb,
>  		     struct intel_crtc_state *cstate,
>  		     int level,
> -		     struct skl_wm_level *result)
> +		     struct skl_pipe_wm *pipe_wm)
>  {
>  	struct drm_atomic_state *state = cstate->base.state;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
> >base.crtc);
> @@ -3684,12 +3687,6 @@ skl_compute_wm_level(const struct
> drm_i915_private *dev_priv,
>  	enum pipe pipe = intel_crtc->pipe;
>  	int ret;
>  
> -	/*
> -	 * We'll only calculate watermarks for planes that are
> actually
> -	 * enabled, so make sure all other planes are set as
> disabled.
> -	 */
> -	memset(result, 0, sizeof(*result));
> -
>  	for_each_intel_plane_mask(&dev_priv->drm,
>  				  intel_plane,
>  				  cstate->base.plane_mask) {
> @@ -3727,9 +3724,7 @@ skl_compute_wm_level(const struct
> drm_i915_private *dev_priv,
>  					   intel_pstate,
>  					   ddb_blocks,
>  					   level,
> -					   &result->plane_res_b[i],
> -					   &result->plane_res_l[i],
> -					   &result->plane_en[i]);
> +					   pipe_wm);
>  		if (ret)
>  			return ret;
>  	}
> @@ -3777,9 +3772,15 @@ static int skl_build_pipe_wm(struct
> intel_crtc_state *cstate,
>  	int level, max_level = ilk_wm_max_level(dev);
>  	int 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, 0, sizeof(*pipe_wm));
> +
>  	for (level = 0; level <= max_level; level++) {
>  		ret = skl_compute_wm_level(dev_priv, ddb, cstate,
> -					   level, &pipe_wm-
> >wm[level]);
> +					   level, pipe_wm);
>  		if (ret)
>  			return ret;
>  	}
Kumar, Mahesh Sept. 21, 2016, 1:48 p.m. UTC | #2 
there are two main reason I want to pass complete pipe_wm to compute 
function

1. for transition WM calculation, else we need to pass 
trans_wm.plane_res_b &  trans_wm.plane_res_l also as a parameter,

     that will increase the parameter-list. And trans_wm will be used 
(calculation will happen) only during level-0. All other levels (1-7) 
these will be extra unused variables.

     If we want to calculate transition WM separately (after plane WM 
calculation) there we will have to duplicate some of the code for WM 
level-0 calculation.

2. This will be used in Render compression WA for (Gen9 & CNL) where we 
have to make sure WM level-(1-7) result blocks/lines are >= WM level-0 
result blocks/lines.


agree, out_ variable should not be local variables.
If you think above points can be addressed, let me know, will update 
patches accordingly.

thanks,
-Mahesh

On Tuesday 20 September 2016 05:47 PM, Paulo Zanoni wrote:
> Em Sex, 2016-09-09 às 13:30 +0530, Kumar, Mahesh escreveu:
>> From: Mahesh Kumar <mahesh1.kumar@intel.com>
>>
>> This patch make use of plane_wm variable directly instead of passing
>> skl_plane_wm struct. this way reduces number of argument requirement
>> in watermark calculation functions.
>>
>> It also gives more freedom of decision making to implement Bspec WM
>> workarounds.
> This is just my personal opinion, but I don't think this patch is an
> improvement to the codebase. The way things are currently organized,
> there's no risk that we may end up reading some variable that's still
> not set/computed, so there's less risk that some later patch may end up
> using information it shouldn't. Also, by having these explicit out
> parameters, we clearly highlight what's the goal of the function:
> output those 3 values, instead of writing I-don't-know-what to a huge
> struct.
>
> Besides, the patch even keeps the out_ variables as local variables
> instead of parameters, which makes things even more confusing IMHO,
> since in_ and out_ variables are usually function parameters.
>
> I also see that this patch is not necessary for the series. We kinda
> use the new pipe_wm variable at patch 9, but we could just go with the
> variables we have.
>
> So, unless some new arguments are given, I'd suggest to just drop this
> patch.
>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_pm.c | 29 +++++++++++++++--------------
>>   1 file changed, 15 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>> b/drivers/gpu/drm/i915/intel_pm.c
>> index 86c6d66..3fdec4d 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3542,9 +3542,7 @@ static int skl_compute_plane_wm(const struct
>> drm_i915_private *dev_priv,
>>   				struct intel_plane_state
>> *intel_pstate,
>>   				uint16_t ddb_allocation,
>>   				int level,
>> -				uint16_t *out_blocks, /* out */
>> -				uint8_t *out_lines, /* out */
>> -				bool *enabled /* out */)
>> +				struct skl_pipe_wm *pipe_wm)
>>   {
>>   	struct drm_plane_state *pstate = &intel_pstate->base;
>>   	struct drm_framebuffer *fb = pstate->fb;
>> @@ -3557,6 +3555,11 @@ static int skl_compute_plane_wm(const struct
>> drm_i915_private *dev_priv,
>>   	uint32_t width = 0, height = 0;
>>   	uint32_t plane_pixel_rate;
>>   	uint32_t y_tile_minimum, y_min_scanlines;
>> +	int id = skl_wm_plane_id(to_intel_plane(pstate->plane));
>> +	struct skl_wm_level *result = &pipe_wm->wm[level];
>> +	uint16_t *out_blocks = &result->plane_res_b[id];
>> +	uint8_t *out_lines = &result->plane_res_l[id];
>> +	bool *enabled = &result->plane_en[id];
>>   
>>   	if (latency == 0 || !cstate->base.active || !intel_pstate-
>>> base.visible) {
>>   		*enabled = false;
>> @@ -3673,7 +3676,7 @@ skl_compute_wm_level(const struct
>> drm_i915_private *dev_priv,
>>   		     struct skl_ddb_allocation *ddb,
>>   		     struct intel_crtc_state *cstate,
>>   		     int level,
>> -		     struct skl_wm_level *result)
>> +		     struct skl_pipe_wm *pipe_wm)
>>   {
>>   	struct drm_atomic_state *state = cstate->base.state;
>>   	struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
>>> base.crtc);
>> @@ -3684,12 +3687,6 @@ skl_compute_wm_level(const struct
>> drm_i915_private *dev_priv,
>>   	enum pipe pipe = intel_crtc->pipe;
>>   	int ret;
>>   
>> -	/*
>> -	 * We'll only calculate watermarks for planes that are
>> actually
>> -	 * enabled, so make sure all other planes are set as
>> disabled.
>> -	 */
>> -	memset(result, 0, sizeof(*result));
>> -
>>   	for_each_intel_plane_mask(&dev_priv->drm,
>>   				  intel_plane,
>>   				  cstate->base.plane_mask) {
>> @@ -3727,9 +3724,7 @@ skl_compute_wm_level(const struct
>> drm_i915_private *dev_priv,
>>   					   intel_pstate,
>>   					   ddb_blocks,
>>   					   level,
>> -					   &result->plane_res_b[i],
>> -					   &result->plane_res_l[i],
>> -					   &result->plane_en[i]);
>> +					   pipe_wm);
>>   		if (ret)
>>   			return ret;
>>   	}
>> @@ -3777,9 +3772,15 @@ static int skl_build_pipe_wm(struct
>> intel_crtc_state *cstate,
>>   	int level, max_level = ilk_wm_max_level(dev);
>>   	int 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, 0, sizeof(*pipe_wm));
>> +
>>   	for (level = 0; level <= max_level; level++) {
>>   		ret = skl_compute_wm_level(dev_priv, ddb, cstate,
>> -					   level, &pipe_wm-
>>> wm[level]);
>> +					   level, pipe_wm);
>>   		if (ret)
>>   			return ret;
>>   	}
Zanoni, Paulo R Sept. 21, 2016, 1:59 p.m. UTC | #3 
Em Qua, 2016-09-21 às 19:18 +0530, Mahesh Kumar escreveu:
> there are two main reason I want to pass complete pipe_wm to compute 
> function
> 
> 1. for transition WM calculation, else we need to pass 
> trans_wm.plane_res_b &  trans_wm.plane_res_l also as a parameter,
> 
>      that will increase the parameter-list. And trans_wm will be
> used 
> (calculation will happen) only during level-0. All other levels (1-
> 7) 
> these will be extra unused variables.
> 
>      If we want to calculate transition WM separately (after plane
> WM 
> calculation) there we will have to duplicate some of the code for WM 
> level-0 calculation.
> 
> 2. This will be used in Render compression WA for (Gen9 & CNL) where
> we 
> have to make sure WM level-(1-7) result blocks/lines are >= WM level-
> 0 
> result blocks/lines.

Well, I tried to check against the series and I really thought they
were not necessary.

Considering there's also some discussion whether we want transition WMs
or not (i.e., the HW guys say we don't want), can you please at least
move this patch to be right before the point where it is needed? This
way we'll be able to discuss & merge the others before.

Thanks,
Paulo

> 
> 
> agree, out_ variable should not be local variables.
> If you think above points can be addressed, let me know, will update 
> patches accordingly.
> 
> thanks,
> -Mahesh
> 
> On Tuesday 20 September 2016 05:47 PM, Paulo Zanoni wrote:
> > 
> > Em Sex, 2016-09-09 às 13:30 +0530, Kumar, Mahesh escreveu:
> > > 
> > > From: Mahesh Kumar <mahesh1.kumar@intel.com>
> > > 
> > > This patch make use of plane_wm variable directly instead of
> > > passing
> > > skl_plane_wm struct. this way reduces number of argument
> > > requirement
> > > in watermark calculation functions.
> > > 
> > > It also gives more freedom of decision making to implement Bspec
> > > WM
> > > workarounds.
> > This is just my personal opinion, but I don't think this patch is
> > an
> > improvement to the codebase. The way things are currently
> > organized,
> > there's no risk that we may end up reading some variable that's
> > still
> > not set/computed, so there's less risk that some later patch may
> > end up
> > using information it shouldn't. Also, by having these explicit out
> > parameters, we clearly highlight what's the goal of the function:
> > output those 3 values, instead of writing I-don't-know-what to a
> > huge
> > struct.
> > 
> > Besides, the patch even keeps the out_ variables as local variables
> > instead of parameters, which makes things even more confusing IMHO,
> > since in_ and out_ variables are usually function parameters.
> > 
> > I also see that this patch is not necessary for the series. We
> > kinda
> > use the new pipe_wm variable at patch 9, but we could just go with
> > the
> > variables we have.
> > 
> > So, unless some new arguments are given, I'd suggest to just drop
> > this
> > patch.
> > 
> > > 
> > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_pm.c | 29 +++++++++++++++-----------
> > > ---
> > >   1 file changed, 15 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index 86c6d66..3fdec4d 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3542,9 +3542,7 @@ static int skl_compute_plane_wm(const
> > > struct
> > > drm_i915_private *dev_priv,
> > >   				struct intel_plane_state
> > > *intel_pstate,
> > >   				uint16_t ddb_allocation,
> > >   				int level,
> > > -				uint16_t *out_blocks, /* out */
> > > -				uint8_t *out_lines, /* out */
> > > -				bool *enabled /* out */)
> > > +				struct skl_pipe_wm *pipe_wm)
> > >   {
> > >   	struct drm_plane_state *pstate = &intel_pstate->base;
> > >   	struct drm_framebuffer *fb = pstate->fb;
> > > @@ -3557,6 +3555,11 @@ static int skl_compute_plane_wm(const
> > > struct
> > > drm_i915_private *dev_priv,
> > >   	uint32_t width = 0, height = 0;
> > >   	uint32_t plane_pixel_rate;
> > >   	uint32_t y_tile_minimum, y_min_scanlines;
> > > +	int id = skl_wm_plane_id(to_intel_plane(pstate->plane));
> > > +	struct skl_wm_level *result = &pipe_wm->wm[level];
> > > +	uint16_t *out_blocks = &result->plane_res_b[id];
> > > +	uint8_t *out_lines = &result->plane_res_l[id];
> > > +	bool *enabled = &result->plane_en[id];
> > >   
> > >   	if (latency == 0 || !cstate->base.active ||
> > > !intel_pstate-
> > > > 
> > > > base.visible) {
> > >   		*enabled = false;
> > > @@ -3673,7 +3676,7 @@ skl_compute_wm_level(const struct
> > > drm_i915_private *dev_priv,
> > >   		     struct skl_ddb_allocation *ddb,
> > >   		     struct intel_crtc_state *cstate,
> > >   		     int level,
> > > -		     struct skl_wm_level *result)
> > > +		     struct skl_pipe_wm *pipe_wm)
> > >   {
> > >   	struct drm_atomic_state *state = cstate->base.state;
> > >   	struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
> > > > 
> > > > base.crtc);
> > > @@ -3684,12 +3687,6 @@ skl_compute_wm_level(const struct
> > > drm_i915_private *dev_priv,
> > >   	enum pipe pipe = intel_crtc->pipe;
> > >   	int ret;
> > >   
> > > -	/*
> > > -	 * We'll only calculate watermarks for planes that are
> > > actually
> > > -	 * enabled, so make sure all other planes are set as
> > > disabled.
> > > -	 */
> > > -	memset(result, 0, sizeof(*result));
> > > -
> > >   	for_each_intel_plane_mask(&dev_priv->drm,
> > >   				  intel_plane,
> > >   				  cstate->base.plane_mask) {
> > > @@ -3727,9 +3724,7 @@ skl_compute_wm_level(const struct
> > > drm_i915_private *dev_priv,
> > >   					   intel_pstate,
> > >   					   ddb_blocks,
> > >   					   level,
> > > -					   &result-
> > > >plane_res_b[i],
> > > -					   &result-
> > > >plane_res_l[i],
> > > -					   &result-
> > > >plane_en[i]);
> > > +					   pipe_wm);
> > >   		if (ret)
> > >   			return ret;
> > >   	}
> > > @@ -3777,9 +3772,15 @@ static int skl_build_pipe_wm(struct
> > > intel_crtc_state *cstate,
> > >   	int level, max_level = ilk_wm_max_level(dev);
> > >   	int 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, 0, sizeof(*pipe_wm));
> > > +
> > >   	for (level = 0; level <= max_level; level++) {
> > >   		ret = skl_compute_wm_level(dev_priv, ddb,
> > > cstate,
> > > -					   level, &pipe_wm-
> > > > 
> > > > wm[level]);
> > > +					   level, pipe_wm);
> > >   		if (ret)
> > >   			return ret;
> > >   	}
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 86c6d66..3fdec4d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3542,9 +3542,7 @@  static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 				struct intel_plane_state *intel_pstate,
 				uint16_t ddb_allocation,
 				int level,
-				uint16_t *out_blocks, /* out */
-				uint8_t *out_lines, /* out */
-				bool *enabled /* out */)
+				struct skl_pipe_wm *pipe_wm)
 {
 	struct drm_plane_state *pstate = &intel_pstate->base;
 	struct drm_framebuffer *fb = pstate->fb;
@@ -3557,6 +3555,11 @@  static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	uint32_t width = 0, height = 0;
 	uint32_t plane_pixel_rate;
 	uint32_t y_tile_minimum, y_min_scanlines;
+	int id = skl_wm_plane_id(to_intel_plane(pstate->plane));
+	struct skl_wm_level *result = &pipe_wm->wm[level];
+	uint16_t *out_blocks = &result->plane_res_b[id];
+	uint8_t *out_lines = &result->plane_res_l[id];
+	bool *enabled = &result->plane_en[id];
 
 	if (latency == 0 || !cstate->base.active || !intel_pstate->base.visible) {
 		*enabled = false;
@@ -3673,7 +3676,7 @@  skl_compute_wm_level(const struct drm_i915_private *dev_priv,
 		     struct skl_ddb_allocation *ddb,
 		     struct intel_crtc_state *cstate,
 		     int level,
-		     struct skl_wm_level *result)
+		     struct skl_pipe_wm *pipe_wm)
 {
 	struct drm_atomic_state *state = cstate->base.state;
 	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
@@ -3684,12 +3687,6 @@  skl_compute_wm_level(const struct drm_i915_private *dev_priv,
 	enum pipe pipe = intel_crtc->pipe;
 	int ret;
 
-	/*
-	 * We'll only calculate watermarks for planes that are actually
-	 * enabled, so make sure all other planes are set as disabled.
-	 */
-	memset(result, 0, sizeof(*result));
-
 	for_each_intel_plane_mask(&dev_priv->drm,
 				  intel_plane,
 				  cstate->base.plane_mask) {
@@ -3727,9 +3724,7 @@  skl_compute_wm_level(const struct drm_i915_private *dev_priv,
 					   intel_pstate,
 					   ddb_blocks,
 					   level,
-					   &result->plane_res_b[i],
-					   &result->plane_res_l[i],
-					   &result->plane_en[i]);
+					   pipe_wm);
 		if (ret)
 			return ret;
 	}
@@ -3777,9 +3772,15 @@  static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
 	int level, max_level = ilk_wm_max_level(dev);
 	int 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, 0, sizeof(*pipe_wm));
+
 	for (level = 0; level <= max_level; level++) {
 		ret = skl_compute_wm_level(dev_priv, ddb, cstate,
-					   level, &pipe_wm->wm[level]);
+					   level, pipe_wm);
 		if (ret)
 			return ret;
 	}