| Message ID | 1431354318-11995-26-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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);
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(-)