On Mon, May 11, 2015 at 04:25:01PM +0200, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
