Re: [PATCH 1/2] drm/i915: Do not enable FEC without DSC

[Manasi Navare <manasi.d.navare@intel.com>]

On Tue, Mar 26, 2019 at 04:49:02PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently we enable FEC even when DSC is no used. While that is
> theoretically valid supposedly there isn't much of a benefit from
> this. But more importantly we do not account for the FEC link
> bandwidth overhead (2.4%) in the non-DSC link bandwidth computations.
> So the code may think we have enough bandwidth when we in fact
> do not.
> 
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.comk>
> Fixes: 240999cf339f ("i915/dp/fec: Add fec_enable to the crtc state.")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 326de12c3f44..bbf678561509 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1925,6 +1925,9 @@ static int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  	int pipe_bpp;
>  	int ret;
>  
> +	pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) &&
> +		intel_dp_supports_fec(intel_dp, pipe_config);
> +

We could still not enable DSC after this point since it has more checks in this
function. Even though in that case we would fail the encoder config so wouldnt
matter if we have enabled FEC or not, but its less intutive.
IMHO, the ideal place to set the fec enable is in intel_dp_compute_link_config()
after the all to dsc_compute_config and set it only if pipe_config->dsc_params.compression_enable

Manasi

>  	if (!intel_dp_supports_dsc(intel_dp, pipe_config))
>  		return -EINVAL;
>  
> @@ -2168,9 +2171,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
>  		return -EINVAL;
>  
> -	pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) &&
> -				  intel_dp_supports_fec(intel_dp, pipe_config);
> -
>  	ret = intel_dp_compute_link_config(encoder, pipe_config, conn_state);
>  	if (ret < 0)
>  		return ret;
> -- 
> 2.19.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx