>-----Original Message-----
>From: Navare, Manasi D
>Sent: Tuesday, November 6, 2018 6:40 PM
>To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>
>Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; Ville Syrjala
><ville.syrjala@xxxxxxxxxxxxxxx>; Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
>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@xxxxxxxxxxxxxxx>
>> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
>> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
>> Cc: Manasi Navare <manasi.d.navare@xxxxxxxxx>
>> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
>> ---
>> 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
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
