Re: [PATCH 25/42] drm/i915: Remove use of crtc->config from intel_psr.c

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, May 11, 2015 at 04:25:01PM +0200, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 5ee0fa57ed19..868817402c11 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -73,14 +73,15 @@ static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
>  }
>  
>  static void intel_psr_write_vsc(struct intel_dp *intel_dp,
> -				    struct edp_vsc_psr *vsc_psr)
> +				struct edp_vsc_psr *vsc_psr)
>  {
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_device *dev = dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *crtc = to_intel_crtc(dig_port->base.base.crtc);
> -	u32 ctl_reg = HSW_TVIDEO_DIP_CTL(crtc->config->cpu_transcoder);
> -	u32 data_reg = HSW_TVIDEO_DIP_VSC_DATA(crtc->config->cpu_transcoder);
> +	struct intel_crtc_state *pipe_config =
> +		to_intel_crtc_state(dig_port->base.base.crtc->state);
> +	u32 ctl_reg = HSW_TVIDEO_DIP_CTL(pipe_config->cpu_transcoder);
> +	u32 data_reg = HSW_TVIDEO_DIP_VSC_DATA(pipe_config->cpu_transcoder);
>  	uint32_t *data = (uint32_t *) vsc_psr;
>  	unsigned int i;
>  
> @@ -282,13 +283,13 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
>  				EDP_SU_TRACK_ENABLE | EDP_PSR2_TP2_TIME_100);
>  }
>  
> -static bool intel_psr_match_conditions(struct intel_dp *intel_dp)
> +static bool intel_psr_match_conditions(struct intel_dp *intel_dp,
> +				       struct intel_crtc_state *pipe_config)

I spotted this pattern in a few other places as well already, where you
add a local variable to avoid the dereference dance again. But this is
called from a pre_enable hook, i.e. we can just directly access
crtc->state to get at the right pipe config. If you instead pass it as a
parameter I have to hunt around to make sure it's the right one.

Imo passing pipe_config should only be done if the code can be called in
the compute_config/check phase or in the disable phase. That then gives
reviewers a nice heads-up about the potential trickiness.
-Daniel

>  {
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_device *dev = dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc = dig_port->base.base.crtc;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  
>  	lockdep_assert_held(&dev_priv->psr.lock);
>  	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> @@ -307,14 +308,14 @@ static bool intel_psr_match_conditions(struct intel_dp *intel_dp)
>  	}
>  
>  	if (IS_HASWELL(dev) &&
> -	    I915_READ(HSW_STEREO_3D_CTL(intel_crtc->config->cpu_transcoder)) &
> +	    I915_READ(HSW_STEREO_3D_CTL(pipe_config->cpu_transcoder)) &
>  		      S3D_ENABLE) {
>  		DRM_DEBUG_KMS("PSR condition failed: Stereo 3D is Enabled\n");
>  		return false;
>  	}
>  
>  	if (IS_HASWELL(dev) &&
> -	    intel_crtc->config->base.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE) {
> +	    pipe_config->base.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE) {
>  		DRM_DEBUG_KMS("PSR condition failed: Interlaced is Enabled\n");
>  		return false;
>  	}
> @@ -364,6 +365,8 @@ void intel_psr_enable(struct intel_dp *intel_dp)
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
> +	struct intel_crtc_state *pipe_config =
> +		to_intel_crtc_state(crtc->base.state);
>  
>  	if (!HAS_PSR(dev)) {
>  		DRM_DEBUG_KMS("PSR not supported on this platform\n");
> @@ -381,7 +384,7 @@ void intel_psr_enable(struct intel_dp *intel_dp)
>  		goto unlock;
>  	}
>  
> -	if (!intel_psr_match_conditions(intel_dp))
> +	if (!intel_psr_match_conditions(intel_dp, pipe_config))
>  		goto unlock;
>  
>  	dev_priv->psr.busy_frontbuffer_bits = 0;
> @@ -391,8 +394,8 @@ void intel_psr_enable(struct intel_dp *intel_dp)
>  
>  		if (dev_priv->psr.psr2_support) {
>  			/* PSR2 is restricted to work with panel resolutions upto 3200x2000 */
> -			if (crtc->config->pipe_src_w > 3200 ||
> -				crtc->config->pipe_src_h > 2000)
> +			if (pipe_config->pipe_src_w > 3200 ||
> +				pipe_config->pipe_src_h > 2000)
>  				dev_priv->psr.psr2_support = false;
>  			else
>  				skl_psr_setup_su_vsc(intel_dp);
> -- 
> 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