Re: [PATCH 09/13] drm/i915: Remove some unneeded checks from check_crtc_state.

From: Daniel Vetter <daniel@ffwll.ch>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 09/13] drm/i915: Remove some unneeded checks from check_crtc_state.
Date: Thu, 16 Jul 2015 11:24:51 +0200	[thread overview]
Message-ID: <20150716092451.GF6223@phenom.ffwll.local> (raw)
In-Reply-To: <1437037166-9339-10-git-send-email-maarten.lankhorst@linux.intel.com>

On Thu, Jul 16, 2015 at 10:59:22AM +0200, Maarten Lankhorst wrote:
> This is handled by the atomic core now, no need to check this for ourself.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

For all these "Remove ..." patches I think it'd be better to rewrite the
changed code to use atomic state for whatever it does directly and stop
using any of the legacy state (whether drm core or i915 legacy state). If
we do that conversion it's possible to review whether there's any cases
we're no longer checking. Trying to do that while we just rip out code
makes that harder.

hw state checker would then only compare hw state against atomic state,
and it would be the job of update_legacy_state and friends to make sure
atomic state matches up with legacy state.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2ca50e589ea4..30ece88703f0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12715,8 +12715,7 @@ check_crtc_state(struct drm_device *dev)
>  	struct intel_crtc_state pipe_config;
>  
>  	for_each_intel_crtc(dev, crtc) {
> -		bool enabled = false;
> -		bool active = false;
> +		bool active;
>  
>  		memset(&pipe_config, 0, sizeof(pipe_config));
>  
> @@ -12726,22 +12725,6 @@ check_crtc_state(struct drm_device *dev)
>  		I915_STATE_WARN(crtc->active && !crtc->base.state->enable,
>  		     "active crtc, but not enabled in sw tracking\n");
>  
> -		for_each_intel_encoder(dev, encoder) {
> -			if (encoder->base.crtc != &crtc->base)
> -				continue;
> -			enabled = true;
> -			if (encoder->connectors_active)
> -				active = true;
> -		}
> -
> -		I915_STATE_WARN(active != crtc->active,
> -		     "crtc's computed active state doesn't match tracked active state "
> -		     "(expected %i, found %i)\n", active, crtc->active);
> -		I915_STATE_WARN(enabled != crtc->base.state->enable,
> -		     "crtc's computed enabled state doesn't match tracked enabled state "
> -		     "(expected %i, found %i)\n", enabled,
> -				crtc->base.state->enable);
> -
>  		active = dev_priv->display.get_pipe_config(crtc,
>  							   &pipe_config);
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx