Re: [PATCH v2 03/11] drm/i915/gen9+: Use for_each_intel_plane_on_crtc in skl_print_wm_changes

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

On Wed, Oct 26, 2016 at 03:41:31PM +0200, Maarten Lankhorst wrote:
> This will allow us to find all allocations that may have changed,

What is the "this" you are referring to?

Ie. please don't treat the subject line as part of the commit message
itself, because it's confusing.

> not just the one added by the atomic state.
> 
> This is required to stop adding planes to state when its
> allocation changes, and is useful for finding bugs.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index bdb69582e7c5..c1520afb2360 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4090,41 +4090,35 @@ skl_print_wm_changes(const struct drm_atomic_state *state)
>  		to_intel_atomic_state(state);
>  	const struct drm_crtc *crtc;
>  	const struct drm_crtc_state *cstate;
> -	const struct drm_plane *plane;
>  	const struct intel_plane *intel_plane;
> -	const struct drm_plane_state *pstate;
>  	const struct skl_ddb_allocation *old_ddb = &dev_priv->wm.skl_hw.ddb;
>  	const struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
>  	enum pipe pipe;
>  	int id;
> -	int i, j;
> +	int i;
>  
>  	for_each_crtc_in_state(state, crtc, cstate, i) {

So what you're saying is that all crtcs whose DDB allocation may have
changed are part of the state, but not all planes on those crtcs may be
part of the state?

>  		pipe = to_intel_crtc(crtc)->pipe;
>  
> -		for_each_plane_in_state(state, plane, pstate, j) {
> +		for_each_intel_plane_on_crtc(dev, to_intel_crtc(crtc), intel_plane) {
>  			const struct skl_ddb_entry *old, *new;
>  
> -			intel_plane = to_intel_plane(plane);
>  			id = skl_wm_plane_id(intel_plane);
>  			old = &old_ddb->plane[pipe][id];
>  			new = &new_ddb->plane[pipe][id];
>  
> -			if (intel_plane->pipe != pipe)
> -				continue;
> -
>  			if (skl_ddb_entry_equal(old, new))
>  				continue;
>  
>  			if (id != PLANE_CURSOR) {
>  				DRM_DEBUG_ATOMIC("[PLANE:%d:plane %d%c] ddb (%d - %d) -> (%d - %d)\n",
> -						 plane->base.id, id + 1,
> +						 intel_plane->base.base.id, id + 1,

Slightly unrelated, but
 "[PLANE:%d:%s] ...", plane->base.id, plane->name
and then you can kill off this cursor special case entirely.

We should probably make some macros for those things so pople wouldn't
have to write them by hand.

>  						 pipe_name(pipe),
>  						 old->start, old->end,
>  						 new->start, new->end);
>  			} else {
>  				DRM_DEBUG_ATOMIC("[PLANE:%d:cursor %c] ddb (%d - %d) -> (%d - %d)\n",
> -						 plane->base.id,
> +						 intel_plane->base.base.id,
>  						 pipe_name(pipe),
>  						 old->start, old->end,
>  						 new->start, new->end);
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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