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

Message ID 1431354318-11995-26-git-send-email-maarten.lankhorst@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst May 11, 2015, 2:25 p.m. UTC
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(-)

Comments

Daniel Vetter May 12, 2015, 9:20 a.m. UTC | #1
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
Maarten Lankhorst May 12, 2015, 1:41 p.m. UTC | #2
Op 12-05-15 om 11:20 schreef Daniel Vetter:
> 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.
>
Ok.

Patch
diff mbox

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)
 {
 	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);