Re: [v3 4/7] i915/dp/fec: Add can_fec to the crtc state.

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

On Thu, Oct 25, 2018 at 09:49:40PM -0700, Anusha Srivatsa wrote:
> 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.
> 
> Suggested-by: Ville Syrjala <ville.syrjala@linux.intel.com>
> 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  | 22 ++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  3 +++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index cfcef9e4b5d9..4776ce6f2174 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2045,6 +2045,23 @@ intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
>  	return false;
>  }
>  
> +static bool intel_dp_can_fec(struct intel_dp *intel_dp,
> +			     struct intel_crtc_state *pipe_config)

I would prefer naming it as intel_dp_supports_fec since as per Ville's feedback
on adding helper function for DSC that will be called intel_dp_supports_dsc

> +{
> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);

You dont need dig_port, you can obtain dev_priv from intel_dp directly:
struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);

> +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> +	if (INTEL_GEN(dev_priv) < 11 || intel_dp_is_edp(intel_dp))
> +		return false;
> +
> +	/* On Gen 11, FEC is Supported Only for DP SST modes.
> +	 * Let us start by enabling FEC for Compressed streams.
> +	 */

Where is the check for sink FEC support?

> +	if (pipe_config->dsc_params.compression_enable)
> +		return true;

I think this logic also looks reversed. The compression_enable state should
be set based on whether can_fec is true and not the vice versa.
IMHO, can_fec is decided solely on the platform support.

Infact since can_fec or fec_support solely depends on platform and sink FEC support,
we dont need a can_fec state , that can just be obtained using a helper
intel_dp_supports_fec() which will check platform support and sink support.

Now if we are saying that we want to enable fec based on mode, then we need fec_enable
state in crtc and that can be set if intel_dp_supports_fec and for now only if compression_en
is set.

Jani, Ville what are your thoughts on this?

> +	else
> +		return false;

You can swap these and have false earlier and avoid else
Just return true at the end.

Manasi


> +
> +}
>  static bool intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  					struct intel_crtc_state *pipe_config,
>  					struct link_config_limits *limits)
> @@ -2129,6 +2146,11 @@ static bool intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  		      pipe_config->dsc_params.compressed_bpp,
>  		      pipe_config->dsc_params.slice_count);
>  
> +	/* For DP 1.4, Enable DSC if FEC can be configured */
> +	pipe_config->can_fec = intel_dp_can_fec(intel_dp, pipe_config);
> +	if (!pipe_config->can_fec)
> +		return false;
> +
>  	return true;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9a94c6544bf5..9dac242ead12 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -940,6 +940,9 @@ struct intel_crtc_state {
>  		u8 slice_count;
>  	} dsc_params;
>  	struct drm_dsc_config dp_dsc_cfg;
> +
> +	/* Forward Error correction State */
> +	bool can_fec;
>  };
>  
>  struct intel_crtc {
> -- 
> 2.17.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx