diff mbox series

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

Message ID 20181026044943.29334-5-anusha.srivatsa@intel.com (mailing list archive)
State New, archived
Headers show
Series Forward Error Correction | expand

Commit Message

Srivatsa, Anusha Oct. 26, 2018, 4:49 a.m. UTC 
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(+)

Comments

Navare, Manasi Oct. 30, 2018, 2:44 a.m. UTC | #1 
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
>
Ville Syrjälä Oct. 30, 2018, 6:57 p.m. UTC | #2 
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)
> +{
> +	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);
> +	if (INTEL_GEN(dev_priv) < 11 || intel_dp_is_edp(intel_dp))
> +		return false;

We don't need FEC for eDP DSC?

> +
> +	/* On Gen 11, FEC is Supported Only for DP SST modes.
> +	 * Let us start by enabling FEC for Compressed streams.
> +	 */
> +	if (pipe_config->dsc_params.compression_enable)
> +		return true;
> +	else
> +		return false;
> +
> +}
>  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;

'fec_enable' is what I'd call this.

We're rather inconsistent with  foo_enable vs. foo_enabled vs.
enable_foo, but one has to pick something.

>  };
>  
>  struct intel_crtc {
> -- 
> 2.17.1
Ville Syrjälä Oct. 30, 2018, 7:38 p.m. UTC | #3 
On Tue, Oct 30, 2018 at 08:57:12PM +0200, Ville Syrjälä wrote:
> 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)
> > +{
> > +	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);
> > +	if (INTEL_GEN(dev_priv) < 11 || intel_dp_is_edp(intel_dp))
> > +		return false;
> 
> We don't need FEC for eDP DSC?

This stuff maybe needs another sink vs. source split and whatnot.

Maybe along these lines:

source_supportes_fec()
{
	return GEN >= 11 && port != PORT_A;
}

sink_supports_fec()
{
	return whatever;
}

supports_fec()
{
	return source_supports_fec() &&
		sink_supports_fec();
}

compute_dsc()
{
	/* eDP DSC does not need FEC */
	fec_enable = !is_edp();

	if (fec_enable && !supports_fec())
		return false;

	if (!supports_dsc())
		return false;

	...
}


> 
> > +
> > +	/* On Gen 11, FEC is Supported Only for DP SST modes.
> > +	 * Let us start by enabling FEC for Compressed streams.
> > +	 */
> > +	if (pipe_config->dsc_params.compression_enable)
> > +		return true;
> > +	else
> > +		return false;
> > +
> > +}
> >  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;
> 
> 'fec_enable' is what I'd call this.
> 
> We're rather inconsistent with  foo_enable vs. foo_enabled vs.
> enable_foo, but one has to pick something.
> 
> >  };
> >  
> >  struct intel_crtc {
> > -- 
> > 2.17.1
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

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)
+{
+	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);
+	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.
+	 */
+	if (pipe_config->dsc_params.compression_enable)
+		return true;
+	else
+		return false;
+
+}
 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 {