Message ID | 20190909034325.20006-6-manasi.d.navare@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Remaining patches to enable Transcoder Port Sync for tiled displays | expand |
Op 09-09-2019 om 05:43 schreef Manasi Navare: > This clears the transcoder port sync bits of the TRANS_DDI_FUNC_CTL2 > register during crtc_disable(). > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 23 ++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 351c90ad7059..07deb0b93f5c 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -4438,6 +4438,26 @@ static void icl_enable_trans_port_sync(const struct intel_crtc_state *crtc_state > trans_ddi_func_ctl2_val); > } > > +static void icl_disable_transcoder_port_sync(const struct intel_crtc_state *old_crtc_state) > +{ > + struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc); > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + i915_reg_t reg; > + u32 trans_ddi_func_ctl2_val; > + > + if (old_crtc_state->master_transcoder == INVALID_TRANSCODER) > + return; > + > + DRM_DEBUG_KMS("Disabling Transcoder Port Sync on Slave Transcoder %s\n", > + transcoder_name(old_crtc_state->cpu_transcoder)); > + > + reg = TRANS_DDI_FUNC_CTL2(old_crtc_state->cpu_transcoder); > + trans_ddi_func_ctl2_val = I915_READ(reg); > + trans_ddi_func_ctl2_val &= ~(PORT_SYNC_MODE_ENABLE | > + PORT_SYNC_MODE_MASTER_SELECT_MASK); > + I915_WRITE(reg, trans_ddi_func_ctl2_val); > +} > + Would anything break if we just wrote 0 here? > static void intel_update_pipe_config(const struct intel_crtc_state *old_crtc_state, > const struct intel_crtc_state *new_crtc_state) > { > @@ -6687,6 +6707,9 @@ static void haswell_crtc_disable(struct intel_crtc_state *old_crtc_state, > if (intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST)) > intel_ddi_set_vc_payload_alloc(old_crtc_state, false); > > + if (INTEL_GEN(dev_priv) >= 11) > + icl_disable_transcoder_port_sync(old_crtc_state); > + > if (!transcoder_is_dsi(cpu_transcoder)) > intel_ddi_disable_transcoder_func(old_crtc_state); >
On Tue, Sep 17, 2019 at 05:04:28PM +0200, Maarten Lankhorst wrote: > Op 09-09-2019 om 05:43 schreef Manasi Navare: > > This clears the transcoder port sync bits of the TRANS_DDI_FUNC_CTL2 > > register during crtc_disable(). > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Matt Roper <matthew.d.roper@intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 23 ++++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index 351c90ad7059..07deb0b93f5c 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -4438,6 +4438,26 @@ static void icl_enable_trans_port_sync(const struct intel_crtc_state *crtc_state > > trans_ddi_func_ctl2_val); > > } > > > > +static void icl_disable_transcoder_port_sync(const struct intel_crtc_state *old_crtc_state) > > +{ > > + struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc); > > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > + i915_reg_t reg; > > + u32 trans_ddi_func_ctl2_val; > > + > > + if (old_crtc_state->master_transcoder == INVALID_TRANSCODER) > > + return; > > + > > + DRM_DEBUG_KMS("Disabling Transcoder Port Sync on Slave Transcoder %s\n", > > + transcoder_name(old_crtc_state->cpu_transcoder)); > > + > > + reg = TRANS_DDI_FUNC_CTL2(old_crtc_state->cpu_transcoder); > > + trans_ddi_func_ctl2_val = I915_READ(reg); > > + trans_ddi_func_ctl2_val &= ~(PORT_SYNC_MODE_ENABLE | > > + PORT_SYNC_MODE_MASTER_SELECT_MASK); > > + I915_WRITE(reg, trans_ddi_func_ctl2_val); > > +} > > + > Would anything break if we just wrote 0 here? We dont want to accidently reset other bits in the register which are for DSI and not used currently but to make this function more future proof, I have avoided writing a 0 But if you strongly feel against this, I can switch this to writing 0 directly. Manasi > > static void intel_update_pipe_config(const struct intel_crtc_state *old_crtc_state, > > const struct intel_crtc_state *new_crtc_state) > > { > > @@ -6687,6 +6707,9 @@ static void haswell_crtc_disable(struct intel_crtc_state *old_crtc_state, > > if (intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST)) > > intel_ddi_set_vc_payload_alloc(old_crtc_state, false); > > > > + if (INTEL_GEN(dev_priv) >= 11) > > + icl_disable_transcoder_port_sync(old_crtc_state); > > + > > if (!transcoder_is_dsi(cpu_transcoder)) > > intel_ddi_disable_transcoder_func(old_crtc_state); > > > >
Op 17-09-2019 om 18:37 schreef Manasi Navare: > On Tue, Sep 17, 2019 at 05:04:28PM +0200, Maarten Lankhorst wrote: >> Op 09-09-2019 om 05:43 schreef Manasi Navare: >>> This clears the transcoder port sync bits of the TRANS_DDI_FUNC_CTL2 >>> register during crtc_disable(). >>> >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> Cc: Matt Roper <matthew.d.roper@intel.com> >>> Cc: Jani Nikula <jani.nikula@intel.com> >>> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> >>> --- >>> drivers/gpu/drm/i915/display/intel_display.c | 23 ++++++++++++++++++++ >>> 1 file changed, 23 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >>> index 351c90ad7059..07deb0b93f5c 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_display.c >>> +++ b/drivers/gpu/drm/i915/display/intel_display.c >>> @@ -4438,6 +4438,26 @@ static void icl_enable_trans_port_sync(const struct intel_crtc_state *crtc_state >>> trans_ddi_func_ctl2_val); >>> } >>> >>> +static void icl_disable_transcoder_port_sync(const struct intel_crtc_state *old_crtc_state) >>> +{ >>> + struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc); >>> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >>> + i915_reg_t reg; >>> + u32 trans_ddi_func_ctl2_val; >>> + >>> + if (old_crtc_state->master_transcoder == INVALID_TRANSCODER) >>> + return; >>> + >>> + DRM_DEBUG_KMS("Disabling Transcoder Port Sync on Slave Transcoder %s\n", >>> + transcoder_name(old_crtc_state->cpu_transcoder)); >>> + >>> + reg = TRANS_DDI_FUNC_CTL2(old_crtc_state->cpu_transcoder); >>> + trans_ddi_func_ctl2_val = I915_READ(reg); >>> + trans_ddi_func_ctl2_val &= ~(PORT_SYNC_MODE_ENABLE | >>> + PORT_SYNC_MODE_MASTER_SELECT_MASK); >>> + I915_WRITE(reg, trans_ddi_func_ctl2_val); >>> +} >>> + >> Would anything break if we just wrote 0 here? > We dont want to accidently reset other bits in the register which are for DSI and not used currently but > to make this function more future proof, I have avoided writing a 0 > > But if you strongly feel against this, I can switch this to writing 0 directly. We overwrite func_ctl2 in enable_port_sync so it makes sense to do the same in disable. :)
On Wed, Sep 18, 2019 at 10:51:54AM +0200, Maarten Lankhorst wrote: > Op 17-09-2019 om 18:37 schreef Manasi Navare: > > On Tue, Sep 17, 2019 at 05:04:28PM +0200, Maarten Lankhorst wrote: > >> Op 09-09-2019 om 05:43 schreef Manasi Navare: > >>> This clears the transcoder port sync bits of the TRANS_DDI_FUNC_CTL2 > >>> register during crtc_disable(). > >>> > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>> Cc: Matt Roper <matthew.d.roper@intel.com> > >>> Cc: Jani Nikula <jani.nikula@intel.com> > >>> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > >>> --- > >>> drivers/gpu/drm/i915/display/intel_display.c | 23 ++++++++++++++++++++ > >>> 1 file changed, 23 insertions(+) > >>> > >>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > >>> index 351c90ad7059..07deb0b93f5c 100644 > >>> --- a/drivers/gpu/drm/i915/display/intel_display.c > >>> +++ b/drivers/gpu/drm/i915/display/intel_display.c > >>> @@ -4438,6 +4438,26 @@ static void icl_enable_trans_port_sync(const struct intel_crtc_state *crtc_state > >>> trans_ddi_func_ctl2_val); > >>> } > >>> > >>> +static void icl_disable_transcoder_port_sync(const struct intel_crtc_state *old_crtc_state) > >>> +{ > >>> + struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc); > >>> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > >>> + i915_reg_t reg; > >>> + u32 trans_ddi_func_ctl2_val; > >>> + > >>> + if (old_crtc_state->master_transcoder == INVALID_TRANSCODER) > >>> + return; > >>> + > >>> + DRM_DEBUG_KMS("Disabling Transcoder Port Sync on Slave Transcoder %s\n", > >>> + transcoder_name(old_crtc_state->cpu_transcoder)); > >>> + > >>> + reg = TRANS_DDI_FUNC_CTL2(old_crtc_state->cpu_transcoder); > >>> + trans_ddi_func_ctl2_val = I915_READ(reg); > >>> + trans_ddi_func_ctl2_val &= ~(PORT_SYNC_MODE_ENABLE | > >>> + PORT_SYNC_MODE_MASTER_SELECT_MASK); > >>> + I915_WRITE(reg, trans_ddi_func_ctl2_val); > >>> +} > >>> + > >> Would anything break if we just wrote 0 here? > > We dont want to accidently reset other bits in the register which are for DSI and not used currently but > > to make this function more future proof, I have avoided writing a 0 > > > > But if you strongly feel against this, I can switch this to writing 0 directly. > > We overwrite func_ctl2 in enable_port_sync so it makes sense to do the same in disable. :) Yes as per the reviews on the enable patch, the RMW was not recommended, so even here I will just overwrite with ~(PORT_SYNC_MODE_ENABLE | PORT_SYNC_MODE_MASTER_SELECT_MASK); This sounds good? Manasi >
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 351c90ad7059..07deb0b93f5c 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -4438,6 +4438,26 @@ static void icl_enable_trans_port_sync(const struct intel_crtc_state *crtc_state trans_ddi_func_ctl2_val); } +static void icl_disable_transcoder_port_sync(const struct intel_crtc_state *old_crtc_state) +{ + struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc); + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); + i915_reg_t reg; + u32 trans_ddi_func_ctl2_val; + + if (old_crtc_state->master_transcoder == INVALID_TRANSCODER) + return; + + DRM_DEBUG_KMS("Disabling Transcoder Port Sync on Slave Transcoder %s\n", + transcoder_name(old_crtc_state->cpu_transcoder)); + + reg = TRANS_DDI_FUNC_CTL2(old_crtc_state->cpu_transcoder); + trans_ddi_func_ctl2_val = I915_READ(reg); + trans_ddi_func_ctl2_val &= ~(PORT_SYNC_MODE_ENABLE | + PORT_SYNC_MODE_MASTER_SELECT_MASK); + I915_WRITE(reg, trans_ddi_func_ctl2_val); +} + static void intel_update_pipe_config(const struct intel_crtc_state *old_crtc_state, const struct intel_crtc_state *new_crtc_state) { @@ -6687,6 +6707,9 @@ static void haswell_crtc_disable(struct intel_crtc_state *old_crtc_state, if (intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST)) intel_ddi_set_vc_payload_alloc(old_crtc_state, false); + if (INTEL_GEN(dev_priv) >= 11) + icl_disable_transcoder_port_sync(old_crtc_state); + if (!transcoder_is_dsi(cpu_transcoder)) intel_ddi_disable_transcoder_func(old_crtc_state);
This clears the transcoder port sync bits of the TRANS_DDI_FUNC_CTL2 register during crtc_disable(). Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 23 ++++++++++++++++++++ 1 file changed, 23 insertions(+)