| Message ID | 20181015215037.13901-7-anusha.srivatsa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Forward Error Correction | expand |
Hi Anusha, Find my comments below: On Mon, Oct 15, 2018 at 02:50:37PM -0700, Anusha Srivatsa wrote: > Set the suitable bits in DP_TP_CTL to stop > bit correction when DSC is disabled. > > - rebased. > - Add additional check for compression state. (Gaurav) > > v3: rebased. > > Cc: Gaurav K Singh <gaurav.k.singh@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Manasi Navare <manasi.d.navare@intel.com> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 2 ++ > drivers/gpu/drm/i915/intel_dp.c | 18 ++++++++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > 3 files changed, 22 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 67c013ea4d39..fefa92070b2d 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -3245,6 +3245,8 @@ static void intel_disable_ddi_dp(struct intel_encoder *encoder, > /* Disable the decompression in DP Sink */ > intel_dp_sink_set_decompression_state(intel_dp, old_crtc_state, > ~DP_DECOMPRESSION_EN); > + /* Disable FEC in DP Sink */ > + intel_dp_disable_fec_state(intel_dp, old_crtc_state); > } > > static void intel_disable_ddi_hdmi(struct intel_encoder *encoder, > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index b9f85502d9ff..1db1a738c85f 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3044,6 +3044,24 @@ void intel_dp_enable_fec_state(struct intel_dp *intel_dp, > DRM_ERROR("Timed out waiting for FEC Enable Status\n"); > } > > +void intel_dp_disable_fec_state(struct intel_dp *intel_dp, > + const struct intel_crtc_state *crtc_state) > +{ > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + enum port port = intel_dig_port->base.port; > + u32 val; > + > + if (crtc_state->dsc_params.compression_enable) > + DRM_DEBUG_KMS("Compression still enabled\n"); This check is wrong. The crtc_state->dsc_params.compression_enable only indicates that compression enable is allowed in atomic check. This does not indicate whether it is enabled or disabled in source/sink Infact this function shd return if !crtc_state->dsc_params.compression_enable, see what intel_dp_sink_set_decompression_state does. After that to make sure DSC is disabled, you can read DP_DSC_ENABLE DPCD register and check DP_DECOMPRESSION_EN bit before proceeding to disabling FEC. Manasi > + return; > + > + val = I915_READ(DP_TP_CTL(port)); > + val &= ~DP_TP_CTL_FEC_ENABLE; > + I915_WRITE(DP_TP_CTL(port), val); > + POSTING_READ(DP_TP_CTL(port)); > +} > + > /* If the sink supports it, try to set the power state appropriately */ > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode) > { > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index e51d612a9f42..0c2429f7cc35 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1713,6 +1713,8 @@ void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp, > int state); > void intel_dp_enable_fec_state(struct intel_dp *intel_dp, > const struct intel_crtc_state *crtc_state); > +void intel_dp_disable_fec_state(struct intel_dp *intel_dp, > + const struct intel_crtc_state *crtc_state); > void intel_dp_encoder_reset(struct drm_encoder *encoder); > void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder); > void intel_dp_encoder_destroy(struct drm_encoder *encoder); > -- > 2.17.1 >
>-----Original Message----- >From: Navare, Manasi D >Sent: Thursday, October 18, 2018 4:16 PM >To: Srivatsa, Anusha <anusha.srivatsa@intel.com> >Cc: intel-gfx@lists.freedesktop.org; Singh, Gaurav K <gaurav.k.singh@intel.com>; >Jani Nikula <jani.nikula@linux.intel.com>; Ville Syrjala ><ville.syrjala@linux.intel.com> >Subject: Re: [v2 6/6] drm/i915/fec: Disable FEC state. > >Hi Anusha, > >Find my comments below: > >On Mon, Oct 15, 2018 at 02:50:37PM -0700, Anusha Srivatsa wrote: >> Set the suitable bits in DP_TP_CTL to stop bit correction when DSC is >> disabled. >> > >> - rebased. >> - Add additional check for compression state. (Gaurav) >> >> v3: rebased. >> >> Cc: Gaurav K Singh <gaurav.k.singh@intel.com> >> Cc: Jani Nikula <jani.nikula@linux.intel.com> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> Cc: Manasi Navare <manasi.d.navare@intel.com> >> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> >> --- >> drivers/gpu/drm/i915/intel_ddi.c | 2 ++ >> drivers/gpu/drm/i915/intel_dp.c | 18 ++++++++++++++++++ >> drivers/gpu/drm/i915/intel_drv.h | 2 ++ >> 3 files changed, 22 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c >> b/drivers/gpu/drm/i915/intel_ddi.c >> index 67c013ea4d39..fefa92070b2d 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -3245,6 +3245,8 @@ static void intel_disable_ddi_dp(struct intel_encoder >*encoder, >> /* Disable the decompression in DP Sink */ >> intel_dp_sink_set_decompression_state(intel_dp, old_crtc_state, >> ~DP_DECOMPRESSION_EN); >> + /* Disable FEC in DP Sink */ >> + intel_dp_disable_fec_state(intel_dp, old_crtc_state); >> } >> >> static void intel_disable_ddi_hdmi(struct intel_encoder *encoder, >> diff --git a/drivers/gpu/drm/i915/intel_dp.c >> b/drivers/gpu/drm/i915/intel_dp.c index b9f85502d9ff..1db1a738c85f >> 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -3044,6 +3044,24 @@ void intel_dp_enable_fec_state(struct intel_dp >*intel_dp, >> DRM_ERROR("Timed out waiting for FEC Enable Status\n"); } >> >> +void intel_dp_disable_fec_state(struct intel_dp *intel_dp, >> + const struct intel_crtc_state *crtc_state) { >> + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); >> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); >> + enum port port = intel_dig_port->base.port; >> + u32 val; >> + >> + if (crtc_state->dsc_params.compression_enable) >> + DRM_DEBUG_KMS("Compression still enabled\n"); > >This check is wrong. The crtc_state->dsc_params.compression_enable only >indicates that compression enable is allowed in atomic check. This does not >indicate whether it is enabled or disabled in source/sink > >Infact this function shd return if !crtc_state->dsc_params.compression_enable, >see what intel_dp_sink_set_decompression_state does. > >After that to make sure DSC is disabled, you can read DP_DSC_ENABLE DPCD >register and check DP_DECOMPRESSION_EN bit before proceeding to disabling >FEC. That is a good point. I will make the changes. Thanks For the review. Anusha >Manasi > >> + return; >> + >> + val = I915_READ(DP_TP_CTL(port)); >> + val &= ~DP_TP_CTL_FEC_ENABLE; >> + I915_WRITE(DP_TP_CTL(port), val); >> + POSTING_READ(DP_TP_CTL(port)); >> +} >> + >> /* If the sink supports it, try to set the power state appropriately >> */ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode) { >> diff --git a/drivers/gpu/drm/i915/intel_drv.h >> b/drivers/gpu/drm/i915/intel_drv.h >> index e51d612a9f42..0c2429f7cc35 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -1713,6 +1713,8 @@ void intel_dp_sink_set_fec_ready(struct intel_dp >*intel_dp, >> int state); >> void intel_dp_enable_fec_state(struct intel_dp *intel_dp, >> const struct intel_crtc_state *crtc_state); >> +void intel_dp_disable_fec_state(struct intel_dp *intel_dp, >> + const struct intel_crtc_state *crtc_state); >> void intel_dp_encoder_reset(struct drm_encoder *encoder); void >> intel_dp_encoder_suspend(struct intel_encoder *intel_encoder); void >> intel_dp_encoder_destroy(struct drm_encoder *encoder); >> -- >> 2.17.1 >>
On Mon, Oct 15, 2018 at 02:50:37PM -0700, Anusha Srivatsa wrote: > Set the suitable bits in DP_TP_CTL to stop > bit correction when DSC is disabled. > > v2: > - rebased. > - Add additional check for compression state. (Gaurav) > > v3: rebased. > > Cc: Gaurav K Singh <gaurav.k.singh@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Manasi Navare <manasi.d.navare@intel.com> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 2 ++ > drivers/gpu/drm/i915/intel_dp.c | 18 ++++++++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > 3 files changed, 22 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 67c013ea4d39..fefa92070b2d 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -3245,6 +3245,8 @@ static void intel_disable_ddi_dp(struct intel_encoder *encoder, > /* Disable the decompression in DP Sink */ > intel_dp_sink_set_decompression_state(intel_dp, old_crtc_state, > ~DP_DECOMPRESSION_EN); > + /* Disable FEC in DP Sink */ > + intel_dp_disable_fec_state(intel_dp, old_crtc_state); This looks like the wrong spot as well. Should be in intel_disable_ddi_buf() if I'm reading the spec correctly. > } > > static void intel_disable_ddi_hdmi(struct intel_encoder *encoder, > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index b9f85502d9ff..1db1a738c85f 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3044,6 +3044,24 @@ void intel_dp_enable_fec_state(struct intel_dp *intel_dp, > DRM_ERROR("Timed out waiting for FEC Enable Status\n"); > } > > +void intel_dp_disable_fec_state(struct intel_dp *intel_dp, > + const struct intel_crtc_state *crtc_state) > +{ > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + enum port port = intel_dig_port->base.port; > + u32 val; > + > + if (crtc_state->dsc_params.compression_enable) > + DRM_DEBUG_KMS("Compression still enabled\n"); > + return; > + > + val = I915_READ(DP_TP_CTL(port)); > + val &= ~DP_TP_CTL_FEC_ENABLE; > + I915_WRITE(DP_TP_CTL(port), val); > + POSTING_READ(DP_TP_CTL(port)); > +} Same comments as for the other patch. > + > /* If the sink supports it, try to set the power state appropriately */ > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode) > { > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index e51d612a9f42..0c2429f7cc35 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1713,6 +1713,8 @@ void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp, > int state); > void intel_dp_enable_fec_state(struct intel_dp *intel_dp, > const struct intel_crtc_state *crtc_state); > +void intel_dp_disable_fec_state(struct intel_dp *intel_dp, > + const struct intel_crtc_state *crtc_state); > void intel_dp_encoder_reset(struct drm_encoder *encoder); > void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder); > void intel_dp_encoder_destroy(struct drm_encoder *encoder); > -- > 2.17.1
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 67c013ea4d39..fefa92070b2d 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -3245,6 +3245,8 @@ static void intel_disable_ddi_dp(struct intel_encoder *encoder, /* Disable the decompression in DP Sink */ intel_dp_sink_set_decompression_state(intel_dp, old_crtc_state, ~DP_DECOMPRESSION_EN); + /* Disable FEC in DP Sink */ + intel_dp_disable_fec_state(intel_dp, old_crtc_state); } static void intel_disable_ddi_hdmi(struct intel_encoder *encoder, diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index b9f85502d9ff..1db1a738c85f 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3044,6 +3044,24 @@ void intel_dp_enable_fec_state(struct intel_dp *intel_dp, DRM_ERROR("Timed out waiting for FEC Enable Status\n"); } +void intel_dp_disable_fec_state(struct intel_dp *intel_dp, + const struct intel_crtc_state *crtc_state) +{ + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); + enum port port = intel_dig_port->base.port; + u32 val; + + if (crtc_state->dsc_params.compression_enable) + DRM_DEBUG_KMS("Compression still enabled\n"); + return; + + val = I915_READ(DP_TP_CTL(port)); + val &= ~DP_TP_CTL_FEC_ENABLE; + I915_WRITE(DP_TP_CTL(port), val); + POSTING_READ(DP_TP_CTL(port)); +} + /* If the sink supports it, try to set the power state appropriately */ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode) { diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index e51d612a9f42..0c2429f7cc35 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1713,6 +1713,8 @@ void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp, int state); void intel_dp_enable_fec_state(struct intel_dp *intel_dp, const struct intel_crtc_state *crtc_state); +void intel_dp_disable_fec_state(struct intel_dp *intel_dp, + const struct intel_crtc_state *crtc_state); void intel_dp_encoder_reset(struct drm_encoder *encoder); void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder); void intel_dp_encoder_destroy(struct drm_encoder *encoder);
Set the suitable bits in DP_TP_CTL to stop bit correction when DSC is disabled. v2: - rebased. - Add additional check for compression state. (Gaurav) v3: rebased. Cc: Gaurav K Singh <gaurav.k.singh@intel.com> Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: Manasi Navare <manasi.d.navare@intel.com> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 2 ++ drivers/gpu/drm/i915/intel_dp.c | 18 ++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 2 ++ 3 files changed, 22 insertions(+)