| Message ID | 20181107003122.25462-2-anusha.srivatsa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Forward Error Correction | expand |
On Tue, Nov 06, 2018 at 04:31:19PM -0800, Anusha Srivatsa wrote: > For DP 1.4 and above, Display Stream compression can be > enabled only if Forward Error Correctin can be performed. > > Add a crtc state for FEC. Currently, the state > is determined by platform, DP and DSC being > enabled. Moving forward we can use the state > to have error correction on other scenarios too > if needed. > > v2: > - Control compression_enable with the fec_enable > parameter in crtc state and with intel_dp_supports_fec() > (Ville) > > - intel_dp_can_fec()/intel_dp_supports_fec()(manasi) > > v3: Check for FEC support along with setting crtc state. > > v4: add checks to intel_dp_source_supports_dsc.(manasi) > - Move intel_dp_supports_fec() closer to > intel_dp_supports_dsc() (Anusha) > > v5: Move fec check to intel_dp_supports_dsc(Ville) > > v6: Remove warning. rebase. > > v7: change crtc state to include DP sink and fec capability > of source.(Manasi) > > Suggested-by: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: dri-devel@lists.freedesktop.org > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Jani Nikula <jani.nikula@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_dp.c | 31 +++++++++++++++++++++++++++++-- > drivers/gpu/drm/i915/intel_drv.h | 3 +++ > 2 files changed, 32 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 73c00c5acf14..f764c45deaab 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -545,7 +545,7 @@ intel_dp_mode_valid(struct drm_connector *connector, > dsc_slice_count = > drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd, > true); > - } else { > + } else if (drm_dp_sink_supports_fec(intel_dp->fec_capable)) { > dsc_max_output_bpp = > intel_dp_dsc_get_output_bpp(max_link_clock, > max_lanes, > @@ -1710,12 +1710,27 @@ struct link_config_limits { > int min_bpp, max_bpp; > }; > > +static bool intel_dp_source_supports_fec(struct intel_dp *intel_dp, > + const struct intel_crtc_state *pipe_config) > +{ > + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > + > + return INTEL_GEN(dev_priv) >= 11 && pipe_config->cpu_transcoder != TRANSCODER_A; > +} > + > +static bool intel_dp_supports_fec(struct intel_dp *intel_dp, > + const struct intel_crtc_state *pipe_config) > +{ > + return intel_dp_source_supports_fec(intel_dp, pipe_config) && > + drm_dp_sink_supports_fec(intel_dp->fec_capable); > +} > + > static bool intel_dp_source_supports_dsc(struct intel_dp *intel_dp, > const struct intel_crtc_state *pipe_config) > { > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > - /* FIXME: FEC needed for external DP until then reject DSC on DP */ > if (!intel_dp_is_edp(intel_dp)) > return false; No point in keeping this !edp condition here, thats supposed to go with FEC check, move that to where fec_supports is added. > > @@ -1726,6 +1741,9 @@ static bool intel_dp_source_supports_dsc(struct intel_dp *intel_dp, > static bool intel_dp_supports_dsc(struct intel_dp *intel_dp, > const struct intel_crtc_state *pipe_config) > { > + if (!pipe_config->fec_enable) > + return false; I think its better to use intel_dp_supports_fec(intel_dp, pipe_config) && !edp instead of pipe_config->fec_enable because pipe_config->fec_enable state will be set only in dp_compute_config > + > if (!intel_dp_source_supports_dsc(intel_dp, pipe_config) || > !drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) > return false; > @@ -1886,9 +1904,18 @@ static bool intel_dp_dsc_compute_config(struct intel_dp *intel_dp, > u16 dsc_max_output_bpp = 0; > u8 dsc_dp_slice_count = 0; > > + pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) && > + intel_dp_supports_fec(intel_dp, pipe_config); Why is pipe_config->fec_enable set in dsc_compute_config()? This state is totally indepenedent of dsc state and we are treating this as an independent feature. I think the earlier place of setting this in intel_dp_compute_link_config() is correct Manasi > + > if (!intel_dp_supports_dsc(intel_dp, pipe_config)) > return false; > > + /* DSC not supported if external DP sink does not support FEC */ > + if (!pipe_config->fec_enable) { > + DRM_DEBUG_KMS("Sink does not support Forward Error Correction, disabling Display Compression\n"); > + return false; > + } > + > /* DSC not supported for DSC sink BPC < 8 */ > if (limits->max_bpp < 3 * DP_DSC_MIN_SUPPORTED_BPC) { > DRM_DEBUG_KMS("No DSC support for less than 8bpc\n"); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index dd22cdeaa673..997bea5fdf16 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -945,6 +945,9 @@ struct intel_crtc_state { > u8 slice_count; > } dsc_params; > struct drm_dsc_config dp_dsc_cfg; > + > + /* Forward Error correction State */ > + bool fec_enable; > }; > > struct intel_crtc { > -- > 2.19.1 >
>-----Original Message----- >From: Navare, Manasi D >Sent: Tuesday, November 6, 2018 6:40 PM >To: Srivatsa, Anusha <anusha.srivatsa@intel.com> >Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Ville Syrjala ><ville.syrjala@linux.intel.com>; Jani Nikula <jani.nikula@linux.intel.com> >Subject: Re: [v7 1/4] i915/dp/fec: Add fec_enable to the crtc state. > >On Tue, Nov 06, 2018 at 04:31:19PM -0800, Anusha Srivatsa wrote: >> For DP 1.4 and above, Display Stream compression can be enabled only >> if Forward Error Correctin can be performed. >> >> Add a crtc state for FEC. Currently, the state is determined by >> platform, DP and DSC being enabled. Moving forward we can use the >> state to have error correction on other scenarios too if needed. >> >> v2: >> - Control compression_enable with the fec_enable parameter in crtc >> state and with intel_dp_supports_fec() >> (Ville) >> >> - intel_dp_can_fec()/intel_dp_supports_fec()(manasi) >> >> v3: Check for FEC support along with setting crtc state. >> >> v4: add checks to intel_dp_source_supports_dsc.(manasi) >> - Move intel_dp_supports_fec() closer to >> intel_dp_supports_dsc() (Anusha) >> >> v5: Move fec check to intel_dp_supports_dsc(Ville) >> >> v6: Remove warning. rebase. >> >> v7: change crtc state to include DP sink and fec capability of >> source.(Manasi) >> >> Suggested-by: Ville Syrjala <ville.syrjala@linux.intel.com> >> Cc: dri-devel@lists.freedesktop.org >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> Cc: Jani Nikula <jani.nikula@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_dp.c | 31 +++++++++++++++++++++++++++++-- >> drivers/gpu/drm/i915/intel_drv.h | 3 +++ >> 2 files changed, 32 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c >> b/drivers/gpu/drm/i915/intel_dp.c index 73c00c5acf14..f764c45deaab >> 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -545,7 +545,7 @@ intel_dp_mode_valid(struct drm_connector >*connector, >> dsc_slice_count = >> drm_dp_dsc_sink_max_slice_count(intel_dp- >>dsc_dpcd, >> true); >> - } else { >> + } else if (drm_dp_sink_supports_fec(intel_dp->fec_capable)) { >> dsc_max_output_bpp = >> intel_dp_dsc_get_output_bpp(max_link_clock, >> max_lanes, >> @@ -1710,12 +1710,27 @@ struct link_config_limits { >> int min_bpp, max_bpp; >> }; >> >> +static bool intel_dp_source_supports_fec(struct intel_dp *intel_dp, >> + const struct intel_crtc_state >*pipe_config) { >> + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); >> + struct drm_i915_private *dev_priv = >> +to_i915(dig_port->base.base.dev); >> + >> + return INTEL_GEN(dev_priv) >= 11 && pipe_config->cpu_transcoder != >> +TRANSCODER_A; } >> + >> +static bool intel_dp_supports_fec(struct intel_dp *intel_dp, >> + const struct intel_crtc_state *pipe_config) { >> + return intel_dp_source_supports_fec(intel_dp, pipe_config) && >> + drm_dp_sink_supports_fec(intel_dp->fec_capable); >> +} >> + >> static bool intel_dp_source_supports_dsc(struct intel_dp *intel_dp, >> const struct intel_crtc_state >*pipe_config) { >> struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); >> >> - /* FIXME: FEC needed for external DP until then reject DSC on DP */ >> if (!intel_dp_is_edp(intel_dp)) >> return false; > >No point in keeping this !edp condition here, thats supposed to go with FEC >check, move that to where fec_supports is added. > >> >> @@ -1726,6 +1741,9 @@ static bool intel_dp_source_supports_dsc(struct >> intel_dp *intel_dp, static bool intel_dp_supports_dsc(struct intel_dp *intel_dp, >> const struct intel_crtc_state *pipe_config) { >> + if (!pipe_config->fec_enable) >> + return false; > >I think its better to use intel_dp_supports_fec(intel_dp, pipe_config) && !edp >instead of pipe_config->fec_enable because pipe_config->fec_enable state will >be set only in dp_compute_config > >> + >> if (!intel_dp_source_supports_dsc(intel_dp, pipe_config) || >> !drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) >> return false; >> @@ -1886,9 +1904,18 @@ static bool intel_dp_dsc_compute_config(struct >intel_dp *intel_dp, >> u16 dsc_max_output_bpp = 0; >> u8 dsc_dp_slice_count = 0; >> >> + pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) && >> + intel_dp_supports_fec(intel_dp, pipe_config); > >Why is pipe_config->fec_enable set in dsc_compute_config()? This state is totally >indepenedent of dsc state and we are treating this as an independent feature. I >think the earlier place of setting this in intel_dp_compute_link_config() is correct How about, maybe adding the fec_enable state in intel_dp_compute_config(). Most of the pipe_config parameters are being set there. So, by the time we first check for dsc, we know if FEC support is there or not. >Manasi > >> + >> if (!intel_dp_supports_dsc(intel_dp, pipe_config)) >> return false; >> >> + /* DSC not supported if external DP sink does not support FEC */ >> + if (!pipe_config->fec_enable) { >> + DRM_DEBUG_KMS("Sink does not support Forward Error >Correction, disabling Display Compression\n"); >> + return false; >> + } >> + >> /* DSC not supported for DSC sink BPC < 8 */ >> if (limits->max_bpp < 3 * DP_DSC_MIN_SUPPORTED_BPC) { >> DRM_DEBUG_KMS("No DSC support for less than 8bpc\n"); diff - >-git >> a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index dd22cdeaa673..997bea5fdf16 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -945,6 +945,9 @@ struct intel_crtc_state { >> u8 slice_count; >> } dsc_params; >> struct drm_dsc_config dp_dsc_cfg; >> + >> + /* Forward Error correction State */ >> + bool fec_enable; >> }; >> >> struct intel_crtc { >> -- >> 2.19.1 >>
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 73c00c5acf14..f764c45deaab 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -545,7 +545,7 @@ intel_dp_mode_valid(struct drm_connector *connector, dsc_slice_count = drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd, true); - } else { + } else if (drm_dp_sink_supports_fec(intel_dp->fec_capable)) { dsc_max_output_bpp = intel_dp_dsc_get_output_bpp(max_link_clock, max_lanes, @@ -1710,12 +1710,27 @@ struct link_config_limits { int min_bpp, max_bpp; }; +static bool intel_dp_source_supports_fec(struct intel_dp *intel_dp, + const struct intel_crtc_state *pipe_config) +{ + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); + + return INTEL_GEN(dev_priv) >= 11 && pipe_config->cpu_transcoder != TRANSCODER_A; +} + +static bool intel_dp_supports_fec(struct intel_dp *intel_dp, + const struct intel_crtc_state *pipe_config) +{ + return intel_dp_source_supports_fec(intel_dp, pipe_config) && + drm_dp_sink_supports_fec(intel_dp->fec_capable); +} + static bool intel_dp_source_supports_dsc(struct intel_dp *intel_dp, const struct intel_crtc_state *pipe_config) { struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); - /* FIXME: FEC needed for external DP until then reject DSC on DP */ if (!intel_dp_is_edp(intel_dp)) return false; @@ -1726,6 +1741,9 @@ static bool intel_dp_source_supports_dsc(struct intel_dp *intel_dp, static bool intel_dp_supports_dsc(struct intel_dp *intel_dp, const struct intel_crtc_state *pipe_config) { + if (!pipe_config->fec_enable) + return false; + if (!intel_dp_source_supports_dsc(intel_dp, pipe_config) || !drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) return false; @@ -1886,9 +1904,18 @@ static bool intel_dp_dsc_compute_config(struct intel_dp *intel_dp, u16 dsc_max_output_bpp = 0; u8 dsc_dp_slice_count = 0; + pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) && + intel_dp_supports_fec(intel_dp, pipe_config); + if (!intel_dp_supports_dsc(intel_dp, pipe_config)) return false; + /* DSC not supported if external DP sink does not support FEC */ + if (!pipe_config->fec_enable) { + DRM_DEBUG_KMS("Sink does not support Forward Error Correction, disabling Display Compression\n"); + return false; + } + /* DSC not supported for DSC sink BPC < 8 */ if (limits->max_bpp < 3 * DP_DSC_MIN_SUPPORTED_BPC) { DRM_DEBUG_KMS("No DSC support for less than 8bpc\n"); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index dd22cdeaa673..997bea5fdf16 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -945,6 +945,9 @@ struct intel_crtc_state { u8 slice_count; } dsc_params; struct drm_dsc_config dp_dsc_cfg; + + /* Forward Error correction State */ + bool fec_enable; }; struct intel_crtc {
For DP 1.4 and above, Display Stream compression can be enabled only if Forward Error Correctin can be performed. Add a crtc state for FEC. Currently, the state is determined by platform, DP and DSC being enabled. Moving forward we can use the state to have error correction on other scenarios too if needed. v2: - Control compression_enable with the fec_enable parameter in crtc state and with intel_dp_supports_fec() (Ville) - intel_dp_can_fec()/intel_dp_supports_fec()(manasi) v3: Check for FEC support along with setting crtc state. v4: add checks to intel_dp_source_supports_dsc.(manasi) - Move intel_dp_supports_fec() closer to intel_dp_supports_dsc() (Anusha) v5: Move fec check to intel_dp_supports_dsc(Ville) v6: Remove warning. rebase. v7: change crtc state to include DP sink and fec capability of source.(Manasi) Suggested-by: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: dri-devel@lists.freedesktop.org Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: Jani Nikula <jani.nikula@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_dp.c | 31 +++++++++++++++++++++++++++++-- drivers/gpu/drm/i915/intel_drv.h | 3 +++ 2 files changed, 32 insertions(+), 2 deletions(-)