[Intel-gfx] [PATCH 2/8] drm/i915/skl+: Remove data_rate from watermark struct.

[Maarten Lankhorst]

Op 20-10-16 om 19:18 schreef Paulo Zanoni:
> Em Qua, 2016-10-19 às 15:13 -0700, Matt Roper escreveu:
>> On Wed, Oct 12, 2016 at 03:28:15PM +0200, Maarten Lankhorst wrote:
>>> It's only used in one function, and can be calculated without
>>> caching it
>>> in the global struct by using
>>> drm_atomic_crtc_state_for_each_plane_state.
>>>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com
>>> ---
>>>  drivers/gpu/drm/i915/intel_drv.h |  4 ----
>>>  drivers/gpu/drm/i915/intel_pm.c  | 44 +++++++++++++++++++---------
>>> ------------
>>>  2 files changed, 21 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>>> b/drivers/gpu/drm/i915/intel_drv.h
>>> index bb468c974e14..888054518f3c 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -502,10 +502,6 @@ struct intel_crtc_wm_state {
>>>  			struct skl_pipe_wm optimal;
>>>  			struct skl_ddb_entry ddb;
>>>  
>>> -			/* cached plane data rate */
>>> -			unsigned plane_data_rate[I915_MAX_PLANES];
>>> -			unsigned
>>> plane_y_data_rate[I915_MAX_PLANES];
>>> -
>>>  			/* minimum block allocation */
>>>  			uint16_t minimum_blocks[I915_MAX_PLANES];
>>>  			uint16_t
>>> minimum_y_blocks[I915_MAX_PLANES];
>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>>> b/drivers/gpu/drm/i915/intel_pm.c
>>> index b96a899c899d..97b6202c4097 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -3236,12 +3236,13 @@ skl_plane_relative_data_rate(const struct
>>> intel_crtc_state *cstate,
>>>   *   3 * 4096 * 8192  * 4 < 2^32
>>>   */
>>>  static unsigned int
>>> -skl_get_total_relative_data_rate(struct intel_crtc_state
>>> *intel_cstate)
>>> +skl_get_total_relative_data_rate(struct intel_crtc_state
>>> *intel_cstate,
>>> +				 unsigned *plane_data_rate,
>>> +				 unsigned *plane_y_data_rate)
>>>  {
>>>  	struct drm_crtc_state *cstate = &intel_cstate->base;
>>>  	struct drm_atomic_state *state = cstate->state;
>>>  	struct drm_crtc *crtc = cstate->crtc;
>>> -	struct drm_device *dev = crtc->dev;
>>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>>  	struct drm_plane *plane;
>>>  	const struct intel_plane *intel_plane;
>>> @@ -3263,21 +3264,16 @@ skl_get_total_relative_data_rate(struct
>>> intel_crtc_state *intel_cstate)
>>>  		/* packed/uv */
>>>  		rate = skl_plane_relative_data_rate(intel_cstate,
>>>  						    pstate, 0);
>>> -		intel_cstate->wm.skl.plane_data_rate[id] = rate;
>>> +		plane_data_rate[id] = rate;
>>> +
>>> +		total_data_rate += rate;
>>>  
>>>  		/* y-plane */
>>>  		rate = skl_plane_relative_data_rate(intel_cstate,
>>>  						    pstate, 1);
>>> -		intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
>>> -	}
>>> -
>>> -	/* Calculate CRTC's total data rate from cached values */
>>> -	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane)
>>> {
>>> -		int id = skl_wm_plane_id(intel_plane);
>>> +		plane_y_data_rate[id] = rate;
>>>  
>>> -		/* packed/uv */
>>> -		total_data_rate += intel_cstate-
>>>> wm.skl.plane_data_rate[id];
>>> -		total_data_rate += intel_cstate-
>>>> wm.skl.plane_y_data_rate[id];
>>> +		total_data_rate += rate;
>>>  	}
>>>  
>>>  	return total_data_rate;
>>> @@ -3366,6 +3362,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
>>> *cstate,
>>>  	int num_active;
>>>  	int id, i;
>>>  
>>> +	unsigned data_rate[I915_MAX_PLANES] = {};
>>> +	unsigned y_data_rate[I915_MAX_PLANES] = {};
>>> +
>> Minor nitpick; if you picked a different names here (e.g.,
>> plane_data_rate[]) then you could leave the local variables farther
>> down
>> named 'data_rate' and 'y_data_rate' which would reduce the diff
>> changes
>> and result in a slightly smaller patch.
>>
>> Whether or not you feel like making that change, killing the caching
>> is
>> good so,
>>
>> Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
>>
>>
>>>  	/* Clear the partitioning for disabled planes. */
>>>  	memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
>>>  	memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
>>> @@ -3425,29 +3424,28 @@ 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);
>>> +	total_data_rate = skl_get_total_relative_data_rate(cstate,
>>> data_rate, y_data_rate);
>>>  	if (total_data_rate == 0)
>>>  		return 0;
>>>  
>>>  	start = alloc->start;
>>> -	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane)
>>> {
>>> -		unsigned int data_rate, y_data_rate;
>>> +	for (id = 0; id < I915_MAX_PLANES; id++) {
> Can we please use a different kind of iteration? Although this is
> correct today, history shows that the number of planes increases over
> time and the code may suddenly break when if we ever introduce PLANE_D.
>
> Perhaps:
> for_each_intel_plane_on_crtc(...) {
> 	id = skl_wm_plane_id(intel_plane);
> 	...
> }
>
> With that fixed (and, in case you want, Matt's suggestion):
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
Why would that break with PLANE_D? in that case rate = 0 and nothing happens. The hooks for it will never be called anyway, since it only updates up to max_planes.

~Maarten