[PATCH 0/5] Forward Error Correction

[PATCH 0/5] Forward Error Correction

[Anusha Srivatsa]

With Display Stream Compression, bit error
in pixel stream can turn into a significant
corruption on the screen. DP1.4 adds support for
Forward Eror correction which uses Reed-Solomon
codes with which the sink can detect small
numbers of bit errors in compressed stream.

This series is rebased on top of:
https://patchwork.freedesktop.org/series/47514/

Srivatsa, Anusha (5):
  drm/dp/fec: DRM helper for Forward Error Correction
  i915/dp/fec: Check for FEC Support
  drm/i915/fec: Set FEC_READY in FEC_CONFIGURATION
  i915/dp/fec: Configure the Forward Error Correction bits.
  drm/i915/fec: Disable FEC state.

 drivers/gpu/drm/drm_dp_helper.c  | 14 ++++++++
 drivers/gpu/drm/i915/i915_reg.h  |  2 ++
 drivers/gpu/drm/i915/intel_ddi.c |  5 +++
 drivers/gpu/drm/i915/intel_dp.c  | 74 ++++++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h |  8 ++++-
 include/drm/drm_dp_helper.h      |  3 ++
 6 files changed, 103 insertions(+), 3 deletions(-)

-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/5] drm/dp/fec: DRM helper for Forward Error Correction

[Anusha Srivatsa]

From: "Srivatsa, Anusha" <anusha.srivatsa@intel.com>

DP 1.4 has Forward Error Correction Support(FEC).
Add helper function to check if the sink device
supports FEC.

v2: Separate the helper and the code that uses the helper into
two separate patches. (Manasi)

v3:
- Move the code to drm_dp_helper.c (Manasi)
- change the return type, code style changes (Gaurav)
- Use drm_dp_dpcd_readb instead of drm_dp_dpcd_read. (Jani)

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/drm_dp_helper.c | 14 ++++++++++++++
 include/drm/drm_dp_helper.h     |  3 +++
 2 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 7dc61d1..2e127b9 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -1425,3 +1425,17 @@ u8 drm_dp_dsc_sink_max_color_depth(const u8 dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE])
 	return 0;
 }
 EXPORT_SYMBOL(drm_dp_dsc_sink_max_color_depth);
+
+/* Forward Error Correction support for DP 1.4 */
+int drm_dp_sink_supports_fec(struct drm_dp_aux *aux)
+{
+	int fec_err;
+	u8 fec_cap;
+
+	fec_err = drm_dp_dpcd_readb(aux, DP_FEC_CAPABILITY, &fec_cap);
+	if (fec_err < 0)
+		return fec_err;
+
+	return fec_cap & DP_FEC_CAPABLE;
+}
+EXPORT_SYMBOL(drm_dp_sink_supports_fec);
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index f178933..d958c7d 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1270,6 +1270,9 @@ void drm_dp_aux_unregister(struct drm_dp_aux *aux);
 int drm_dp_start_crc(struct drm_dp_aux *aux, struct drm_crtc *crtc);
 int drm_dp_stop_crc(struct drm_dp_aux *aux);
 
+/* Forward Error Correction Support on DP 1.4 */
+int drm_dp_sink_supports_fec(struct drm_dp_aux *aux);
+
 struct drm_dp_dpcd_ident {
 	u8 oui[3];
 	u8 device_id[6];
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/dp/fec: DRM helper for Forward Error Correction

[Jani Nikula]

On Tue, 07 Aug 2018, Anusha Srivatsa <anusha.srivatsa@intel.com> wrote:
> From: "Srivatsa, Anusha" <anusha.srivatsa@intel.com>
>
> DP 1.4 has Forward Error Correction Support(FEC).
> Add helper function to check if the sink device
> supports FEC.
>
> v2: Separate the helper and the code that uses the helper into
> two separate patches. (Manasi)
>
> v3:
> - Move the code to drm_dp_helper.c (Manasi)
> - change the return type, code style changes (Gaurav)
> - Use drm_dp_dpcd_readb instead of drm_dp_dpcd_read. (Jani)
>
> 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/drm_dp_helper.c | 14 ++++++++++++++
>  include/drm/drm_dp_helper.h     |  3 +++

Neither of these is i915, thus intel-gfx is not enough.

>  2 files changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 7dc61d1..2e127b9 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -1425,3 +1425,17 @@ u8 drm_dp_dsc_sink_max_color_depth(const u8 dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE])
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_dp_dsc_sink_max_color_depth);
> +
> +/* Forward Error Correction support for DP 1.4 */
> +int drm_dp_sink_supports_fec(struct drm_dp_aux *aux)
> +{
> +	int fec_err;
> +	u8 fec_cap;
> +
> +	fec_err = drm_dp_dpcd_readb(aux, DP_FEC_CAPABILITY, &fec_cap);
> +	if (fec_err < 0)
> +		return fec_err;
> +
> +	return fec_cap & DP_FEC_CAPABLE;

I can't help but think this function feels wrong in so many ways.

First, how does this function fit with the rest of the helpers? Most
helpers operate on previously read data. At the very least the function
name should reflect the fact that this *reads* DPCD *if* this continues
to read the DPCD.

Second, what are you going to do when you need to read the other bits in
the same register? Read it again in the driver? Add more helpers? But
you only want to read the register *once*. This one seems too
specialized.

Third, the name implies a boolean return, but it's not. An unsuspecting
caller will use this and get a "supports fec" return on errors. This is
hard to use. Patch 2/5 in this series makes that exact mistake twice,
it's the first user, and sets the example.

I'm not convinced of the usefulness of this particular helper.

BR,
Jani.



> +}
> +EXPORT_SYMBOL(drm_dp_sink_supports_fec);
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index f178933..d958c7d 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1270,6 +1270,9 @@ void drm_dp_aux_unregister(struct drm_dp_aux *aux);
>  int drm_dp_start_crc(struct drm_dp_aux *aux, struct drm_crtc *crtc);
>  int drm_dp_stop_crc(struct drm_dp_aux *aux);
>  
> +/* Forward Error Correction Support on DP 1.4 */
> +int drm_dp_sink_supports_fec(struct drm_dp_aux *aux);
> +
>  struct drm_dp_dpcd_ident {
>  	u8 oui[3];
>  	u8 device_id[6];

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/dp/fec: DRM helper for Forward Error Correction

[Manasi Navare]

Hi Jani,

Thanks for your feedback. Please see my comments below:

On Fri, Aug 17, 2018 at 12:51:03PM +0300, Jani Nikula wrote:
> On Tue, 07 Aug 2018, Anusha Srivatsa <anusha.srivatsa@intel.com> wrote:
> > From: "Srivatsa, Anusha" <anusha.srivatsa@intel.com>
> >
> > DP 1.4 has Forward Error Correction Support(FEC).
> > Add helper function to check if the sink device
> > supports FEC.
> >
> > v2: Separate the helper and the code that uses the helper into
> > two separate patches. (Manasi)
> >
> > v3:
> > - Move the code to drm_dp_helper.c (Manasi)
> > - change the return type, code style changes (Gaurav)
> > - Use drm_dp_dpcd_readb instead of drm_dp_dpcd_read. (Jani)
> >
> > 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/drm_dp_helper.c | 14 ++++++++++++++
> >  include/drm/drm_dp_helper.h     |  3 +++
> 
> Neither of these is i915, thus intel-gfx is not enough.
> 
> >  2 files changed, 17 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > index 7dc61d1..2e127b9 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -1425,3 +1425,17 @@ u8 drm_dp_dsc_sink_max_color_depth(const u8 dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE])
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL(drm_dp_dsc_sink_max_color_depth);
> > +
> > +/* Forward Error Correction support for DP 1.4 */
> > +int drm_dp_sink_supports_fec(struct drm_dp_aux *aux)
> > +{
> > +	int fec_err;
> > +	u8 fec_cap;
> > +
> > +	fec_err = drm_dp_dpcd_readb(aux, DP_FEC_CAPABILITY, &fec_cap);
> > +	if (fec_err < 0)
> > +		return fec_err;
> > +
> > +	return fec_cap & DP_FEC_CAPABLE;
> 
> I can't help but think this function feels wrong in so many ways.
> 
> First, how does this function fit with the rest of the helpers? Most
> helpers operate on previously read data. At the very least the function
> name should reflect the fact that this *reads* DPCD *if* this continues
> to read the DPCD.

I agree that this does not fit with rest of the helpers. All the other
helpers that get any information from DPCD registers actually work
on the cached set of registers.
So here to follow the same format, we could cache FEC DPCD registers -
FEC_SUPPORT, FEC_CONFIGURATION, FEC_STATUS, FEC_ERROR_COUNT.
Out fo which we really for now need to read FEC_SUPPORT but might
need other registers in the future.
One way to correct this would be to cache these at the same time when
we cache the DSC DPCD registers into say fec_dpcd[].
That way we dont trigger the aux reads everytime we need to get fec_support()

Jani, does this sound like a good solution?

> 
> Second, what are you going to do when you need to read the other bits in
> the same register? Read it again in the driver? Add more helpers? But
> you only want to read the register *once*. This one seems too
> specialized.
> 
> Third, the name implies a boolean return, but it's not. An unsuspecting
> caller will use this and get a "supports fec" return on errors. This is
> hard to use. Patch 2/5 in this series makes that exact mistake twice,
> it's the first user, and sets the example.

Yes I think this should follow the same format as drm_dp_sink_supports_dsc() where
it returns a bool based on the cached value.

Manasi


> 
> I'm not convinced of the usefulness of this particular helper.
> 
> BR,
> Jani.
> 
> 
> 
> > +}
> > +EXPORT_SYMBOL(drm_dp_sink_supports_fec);
> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > index f178933..d958c7d 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -1270,6 +1270,9 @@ void drm_dp_aux_unregister(struct drm_dp_aux *aux);
> >  int drm_dp_start_crc(struct drm_dp_aux *aux, struct drm_crtc *crtc);
> >  int drm_dp_stop_crc(struct drm_dp_aux *aux);
> >  
> > +/* Forward Error Correction Support on DP 1.4 */
> > +int drm_dp_sink_supports_fec(struct drm_dp_aux *aux);
> > +
> >  struct drm_dp_dpcd_ident {
> >  	u8 oui[3];
> >  	u8 device_id[6];
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/dp/fec: DRM helper for Forward Error Correction

[Srivatsa, Anusha]

>-----Original Message-----
>From: Navare, Manasi D
>Sent: Monday, August 20, 2018 12:32 PM
>To: Jani Nikula <jani.nikula@linux.intel.com>
>Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel-
>gfx@lists.freedesktop.org; Ville Syrjala <ville.syrjala@linux.intel.com>
>Subject: Re: [PATCH 1/5] drm/dp/fec: DRM helper for Forward Error Correction
>
>Hi Jani,
>
>Thanks for your feedback. Please see my comments below:
>
>On Fri, Aug 17, 2018 at 12:51:03PM +0300, Jani Nikula wrote:
>> On Tue, 07 Aug 2018, Anusha Srivatsa <anusha.srivatsa@intel.com> wrote:
>> > From: "Srivatsa, Anusha" <anusha.srivatsa@intel.com>
>> >
>> > DP 1.4 has Forward Error Correction Support(FEC).
>> > Add helper function to check if the sink device supports FEC.
>> >
>> > v2: Separate the helper and the code that uses the helper into two
>> > separate patches. (Manasi)
>> >
>> > v3:
>> > - Move the code to drm_dp_helper.c (Manasi)
>> > - change the return type, code style changes (Gaurav)
>> > - Use drm_dp_dpcd_readb instead of drm_dp_dpcd_read. (Jani)
>> >
>> > 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/drm_dp_helper.c | 14 ++++++++++++++
>> >  include/drm/drm_dp_helper.h     |  3 +++
>>
>> Neither of these is i915, thus intel-gfx is not enough.
>>
>> >  2 files changed, 17 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/drm_dp_helper.c
>> > b/drivers/gpu/drm/drm_dp_helper.c index 7dc61d1..2e127b9 100644
>> > --- a/drivers/gpu/drm/drm_dp_helper.c
>> > +++ b/drivers/gpu/drm/drm_dp_helper.c
>> > @@ -1425,3 +1425,17 @@ u8 drm_dp_dsc_sink_max_color_depth(const u8
>dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE])
>> >  	return 0;
>> >  }
>> >  EXPORT_SYMBOL(drm_dp_dsc_sink_max_color_depth);
>> > +
>> > +/* Forward Error Correction support for DP 1.4 */ int
>> > +drm_dp_sink_supports_fec(struct drm_dp_aux *aux) {
>> > +	int fec_err;
>> > +	u8 fec_cap;
>> > +
>> > +	fec_err = drm_dp_dpcd_readb(aux, DP_FEC_CAPABILITY, &fec_cap);
>> > +	if (fec_err < 0)
>> > +		return fec_err;
>> > +
>> > +	return fec_cap & DP_FEC_CAPABLE;
>>
>> I can't help but think this function feels wrong in so many ways.
>>
>> First, how does this function fit with the rest of the helpers? Most
>> helpers operate on previously read data. At the very least the
>> function name should reflect the fact that this *reads* DPCD *if* this
>> continues to read the DPCD.
>
>I agree that this does not fit with rest of the helpers. All the other helpers that get
>any information from DPCD registers actually work on the cached set of registers.
>So here to follow the same format, we could cache FEC DPCD registers -
>FEC_SUPPORT, FEC_CONFIGURATION, FEC_STATUS, FEC_ERROR_COUNT.
>Out fo which we really for now need to read FEC_SUPPORT but might need other
>registers in the future.
>One way to correct this would be to cache these at the same time when we
>cache the DSC DPCD registers into say fec_dpcd[].
>That way we dont trigger the aux reads everytime we need to get fec_support()

Sounds about right, Jani,any thougts?

>Jani, does this sound like a good solution?
>
>>
>> Second, what are you going to do when you need to read the other bits
>> in the same register? Read it again in the driver? Add more helpers?
>> But you only want to read the register *once*. This one seems too
>> specialized.
>>
>> Third, the name implies a boolean return, but it's not. An
>> unsuspecting caller will use this and get a "supports fec" return on
>> errors. This is hard to use. Patch 2/5 in this series makes that exact
>> mistake twice, it's the first user, and sets the example.
>
>Yes I think this should follow the same format as drm_dp_sink_supports_dsc()
>where it returns a bool based on the cached value.

Sure. Makes sense.

Thanks Jani, Manasi for the feedback.

Anusha 
>Manasi
>
>
>>
>> I'm not convinced of the usefulness of this particular helper.
>>
>> BR,
>> Jani.
>>
>>
>>
>> > +}
>> > +EXPORT_SYMBOL(drm_dp_sink_supports_fec);
>> > diff --git a/include/drm/drm_dp_helper.h
>> > b/include/drm/drm_dp_helper.h index f178933..d958c7d 100644
>> > --- a/include/drm/drm_dp_helper.h
>> > +++ b/include/drm/drm_dp_helper.h
>> > @@ -1270,6 +1270,9 @@ void drm_dp_aux_unregister(struct drm_dp_aux
>> > *aux);  int drm_dp_start_crc(struct drm_dp_aux *aux, struct drm_crtc
>> > *crtc);  int drm_dp_stop_crc(struct drm_dp_aux *aux);
>> >
>> > +/* Forward Error Correction Support on DP 1.4 */ int
>> > +drm_dp_sink_supports_fec(struct drm_dp_aux *aux);
>> > +
>> >  struct drm_dp_dpcd_ident {
>> >  	u8 oui[3];
>> >  	u8 device_id[6];
>>
>> --
>> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/dp/fec: DRM helper for Forward Error Correction

[Srivatsa, Anusha]

>-----Original Message-----
>From: Navare, Manasi D
>Sent: Monday, August 20, 2018 12:32 PM
>To: Jani Nikula <jani.nikula@linux.intel.com>
>Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel-
>gfx@lists.freedesktop.org; Ville Syrjala <ville.syrjala@linux.intel.com>
>Subject: Re: [PATCH 1/5] drm/dp/fec: DRM helper for Forward Error Correction
>
>Hi Jani,
>
>Thanks for your feedback. Please see my comments below:
>
>On Fri, Aug 17, 2018 at 12:51:03PM +0300, Jani Nikula wrote:
>> On Tue, 07 Aug 2018, Anusha Srivatsa <anusha.srivatsa@intel.com> wrote:
>> > From: "Srivatsa, Anusha" <anusha.srivatsa@intel.com>
>> >
>> > DP 1.4 has Forward Error Correction Support(FEC).
>> > Add helper function to check if the sink device supports FEC.
>> >
>> > v2: Separate the helper and the code that uses the helper into two
>> > separate patches. (Manasi)
>> >
>> > v3:
>> > - Move the code to drm_dp_helper.c (Manasi)
>> > - change the return type, code style changes (Gaurav)
>> > - Use drm_dp_dpcd_readb instead of drm_dp_dpcd_read. (Jani)
>> >
>> > 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/drm_dp_helper.c | 14 ++++++++++++++
>> >  include/drm/drm_dp_helper.h     |  3 +++
>>
>> Neither of these is i915, thus intel-gfx is not enough.
>>
>> >  2 files changed, 17 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/drm_dp_helper.c
>> > b/drivers/gpu/drm/drm_dp_helper.c index 7dc61d1..2e127b9 100644
>> > --- a/drivers/gpu/drm/drm_dp_helper.c
>> > +++ b/drivers/gpu/drm/drm_dp_helper.c
>> > @@ -1425,3 +1425,17 @@ u8 drm_dp_dsc_sink_max_color_depth(const u8
>dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE])
>> >  	return 0;
>> >  }
>> >  EXPORT_SYMBOL(drm_dp_dsc_sink_max_color_depth);
>> > +
>> > +/* Forward Error Correction support for DP 1.4 */ int
>> > +drm_dp_sink_supports_fec(struct drm_dp_aux *aux) {
>> > +	int fec_err;
>> > +	u8 fec_cap;
>> > +
>> > +	fec_err = drm_dp_dpcd_readb(aux, DP_FEC_CAPABILITY, &fec_cap);
>> > +	if (fec_err < 0)
>> > +		return fec_err;
>> > +
>> > +	return fec_cap & DP_FEC_CAPABLE;
>>
>> I can't help but think this function feels wrong in so many ways.
>>
>> First, how does this function fit with the rest of the helpers? Most
>> helpers operate on previously read data. At the very least the
>> function name should reflect the fact that this *reads* DPCD *if* this
>> continues to read the DPCD.
>
>I agree that this does not fit with rest of the helpers. All the other helpers that get
>any information from DPCD registers actually work on the cached set of registers.
>So here to follow the same format, we could cache FEC DPCD registers -
>FEC_SUPPORT, FEC_CONFIGURATION, FEC_STATUS, FEC_ERROR_COUNT.
>Out fo which we really for now need to read FEC_SUPPORT but might need other
>registers in the future.
>One way to correct this would be to cache these at the same time when we
>cache the DSC DPCD registers into say fec_dpcd[].
>That way we dont trigger the aux reads everytime we need to get fec_support()
>
>Jani, does this sound like a good solution?
>
>>
>> Second, what are you going to do when you need to read the other bits
>> in the same register? Read it again in the driver? Add more helpers?
>> But you only want to read the register *once*. This one seems too
>> specialized.
>>
>> Third, the name implies a boolean return, but it's not. An
>> unsuspecting caller will use this and get a "supports fec" return on
>> errors. This is hard to use. Patch 2/5 in this series makes that exact
>> mistake twice, it's the first user, and sets the example.
>
>Yes I think this should follow the same format as drm_dp_sink_supports_dsc()
>where it returns a bool based on the cached value.
>
>Manasi
>
>
>>
>> I'm not convinced of the usefulness of this particular helper.

Jani, Manasi,

Other than FEC_CAPABLE we don't read any other register. We do a bunch of writes, but that's about it. Also The registers are not consecutive. So, In case we decide to go ahead and cache all registers, it has to be three different cache. I think it is good approach  to cache the FEC_CAPABLE register and  *not* do aux operations everytime.

Sound good?

Anusha 
>> BR,
>> Jani.
>>
>>
>>
>> > +}
>> > +EXPORT_SYMBOL(drm_dp_sink_supports_fec);
>> > diff --git a/include/drm/drm_dp_helper.h
>> > b/include/drm/drm_dp_helper.h index f178933..d958c7d 100644
>> > --- a/include/drm/drm_dp_helper.h
>> > +++ b/include/drm/drm_dp_helper.h
>> > @@ -1270,6 +1270,9 @@ void drm_dp_aux_unregister(struct drm_dp_aux
>> > *aux);  int drm_dp_start_crc(struct drm_dp_aux *aux, struct drm_crtc
>> > *crtc);  int drm_dp_stop_crc(struct drm_dp_aux *aux);
>> >
>> > +/* Forward Error Correction Support on DP 1.4 */ int
>> > +drm_dp_sink_supports_fec(struct drm_dp_aux *aux);
>> > +
>> >  struct drm_dp_dpcd_ident {
>> >  	u8 oui[3];
>> >  	u8 device_id[6];
>>
>> --
>> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/5] i915/dp/fec: Check for FEC Support

[Anusha Srivatsa]

From: "Srivatsa, Anusha" <anusha.srivatsa@intel.com>

For DP 1.4 and above, Display Stream compression can be
enabled only if Forward Error Correctin can be performed.

If FEC support exists, write to the FEC_READY bit
in the FEC_CONFIGURATION DPCD register.

v2: Mention External DP where ever FEC is mentioned
in the code.Check return status of dpcd reads. (Gaurav)
- Do regular mode check even if FEC is not supported. (manasi)

v3: Do not perform any dpcd writes in the atomic
check phase. (DK, Manasi)

v4: Use debug level logging for scenario where sink does
not support a feature. (DK)

Cc: Gaurav K Singh <gaurav.k.singh@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>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1a8329c..cb8b63e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -652,7 +652,7 @@ intel_dp_mode_valid(struct drm_connector *connector,
 			dsc_max_output_bpp = drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4;
 			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->aux)) {
 			dsc_max_output_bpp = intel_dp_dsc_get_output_bpp(max_link_clock,
 									 max_lanes,
 									 target_clock,
@@ -660,7 +660,8 @@ intel_dp_mode_valid(struct drm_connector *connector,
 			dsc_slice_count = intel_dp_dsc_get_slice_count(intel_dp,
 								       target_clock,
 								       mode->hdisplay);
-		}
+		} else
+			DRM_DEBUG_KMS("Sink device does not Support FEC\n");
 	}
 
 	if ((mode_rate > max_rate && !(dsc_max_output_bpp && dsc_slice_count)) ||
@@ -2000,6 +2001,13 @@ static bool intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 	if (pipe == PIPE_A && !intel_dp_is_edp(intel_dp))
 		return false;
 
+	/* DSC not supported if external DP sink does not support FEC */
+	if (!intel_dp_is_edp(intel_dp) &&
+	    !drm_dp_sink_supports_fec(&intel_dp->aux)) {
+		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");
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/5] drm/i915/fec: Set FEC_READY in FEC_CONFIGURATION

[Anusha Srivatsa]

From: "Srivatsa, Anusha" <anusha.srivatsa@intel.com>

If the panel supports FEC, the driver has to
set the FEC_READY bit in the dpcd register:
FEC_CONFIGURATION.

This has to happen before link training.

v2: s/intel_dp_set_fec_ready/intel_dp_sink_set_fec_ready
   - change commit message. (Gaurav)

Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ville Syrjala <ville.syrjala@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_ddi.c |  1 +
 drivers/gpu/drm/i915/intel_dp.c  | 17 +++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  3 +++
 3 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 5e8c891..9cdcc31 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2827,6 +2827,7 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
 		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
 	intel_dp_sink_set_decompression_state(intel_dp, crtc_state,
 					      DP_DECOMPRESSION_EN);
+	intel_dp_sink_set_fec_ready(intel_dp, crtc_state, DP_FEC_READY);
 	intel_dp_start_link_train(intel_dp);
 	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
 		intel_dp_stop_link_train(intel_dp);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index cb8b63e..5ef9005 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2948,6 +2948,23 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
 			      state == DP_DECOMPRESSION_EN ? "enable" : "disable");
 }
 
+void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
+				 const struct intel_crtc_state *crtc_state,
+				 int state)
+{
+	int ret;
+
+	if (!crtc_state->dsc_params.compression_enable)
+		return;
+
+	if (intel_dp_is_edp(intel_dp))
+		return;
+
+	ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_FEC_CONFIGURATION, state);
+	if (ret < 0)
+		DRM_DEBUG_KMS("Failed to get FEC enabled in sink\n");
+}
+
 /* If the sink supports it, try to set the power state appropriately */
 void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0ecfacf..1ab9b20 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1696,6 +1696,9 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
 void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
 					   const struct intel_crtc_state *crtc_state,
 					   int state);
+void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
+				 const struct intel_crtc_state *crtc_state,
+				 int state);
 void intel_dp_encoder_reset(struct drm_encoder *encoder);
 void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder);
 void intel_dp_encoder_destroy(struct drm_encoder *encoder);
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] i915/dp/fec: Configure the Forward Error Correction bits.

[Anusha Srivatsa]

From: "Srivatsa, Anusha" <anusha.srivatsa@intel.com>

If FEC is supported, the corresponding
DP_TP_CTL register bits have to be configured.

The driver has to program the FEC_ENABLE in DP_TP_CTL[30] register
and wait till FEC_STATUS in DP_TP_CTL[28] is 1.
Also add the warn message to make sure that the control
register is already active while enabling FEC.

v2:
- Change commit message. Configure fec state after
  link training (Manasi, Gaurav)
- Remove redundent checks (Manasi)
- Remove the registers that get added automagically (Anusha)

v3: s/intel_dp_set_fec_state()/intel_dp_enable_fec_state() (Gaurav)

Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  |  2 ++
 drivers/gpu/drm/i915/intel_ddi.c |  2 ++
 drivers/gpu/drm/i915/intel_dp.c  | 27 +++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  3 ++-
 4 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0ae38b6..80a10969 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -9151,6 +9151,7 @@ enum skl_power_gate {
 #define _DP_TP_CTL_B			0x64140
 #define DP_TP_CTL(port) _MMIO_PORT(port, _DP_TP_CTL_A, _DP_TP_CTL_B)
 #define  DP_TP_CTL_ENABLE			(1 << 31)
+#define  DP_TP_CTL_FEC_ENABLE			(1 << 30)
 #define  DP_TP_CTL_MODE_SST			(0 << 27)
 #define  DP_TP_CTL_MODE_MST			(1 << 27)
 #define  DP_TP_CTL_FORCE_ACT			(1 << 25)
@@ -9169,6 +9170,7 @@ enum skl_power_gate {
 #define _DP_TP_STATUS_A			0x64044
 #define _DP_TP_STATUS_B			0x64144
 #define DP_TP_STATUS(port) _MMIO_PORT(port, _DP_TP_STATUS_A, _DP_TP_STATUS_B)
+#define  DP_TP_STATUS_FEC_ENABLE_LIVE		(1 << 28)
 #define  DP_TP_STATUS_IDLE_DONE			(1 << 25)
 #define  DP_TP_STATUS_ACT_SENT			(1 << 24)
 #define  DP_TP_STATUS_MODE_STATUS_MST		(1 << 23)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 9cdcc31..27fbfb7 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2829,6 +2829,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
 					      DP_DECOMPRESSION_EN);
 	intel_dp_sink_set_fec_ready(intel_dp, crtc_state, DP_FEC_READY);
 	intel_dp_start_link_train(intel_dp);
+	intel_dp_enable_fec_state(intel_dp, crtc_state);
+
 	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
 		intel_dp_stop_link_train(intel_dp);
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5ef9005..1651da4 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2965,6 +2965,33 @@ void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
 		DRM_DEBUG_KMS("Failed to get FEC enabled in sink\n");
 }
 
+void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
+			       const struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	enum port port = intel_dig_port->base.port;
+	u32 val;
+
+	/* FEC support exists for DP 1.4 only */
+	if (intel_dp_is_edp(intel_dp))
+		return;
+
+	/* If Display Compression is not enabled, FEC need not be configured */
+	if (!crtc_state->dsc_params.compression_enable)
+		return;
+
+	val = I915_READ(DP_TP_CTL(port));
+	val |= DP_TP_CTL_FEC_ENABLE;
+	I915_WRITE(DP_TP_CTL(port), val);
+
+	if (intel_wait_for_register(dev_priv, DP_TP_STATUS(port),
+				    DP_TP_STATUS_FEC_ENABLE_LIVE,
+				    DP_TP_STATUS_FEC_ENABLE_LIVE,
+				    1))
+		DRM_ERROR("Timed out waiting for FEC Enable Status\n");
+}
+
 /* If the sink supports it, try to set the power state appropriately */
 void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1ab9b20..117ddaf 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1519,7 +1519,6 @@ intel_encoder_current_mode(struct intel_encoder *encoder);
 bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port port);
 enum tc_port intel_port_to_tc(struct drm_i915_private *dev_priv,
 			      enum port port);
-
 enum pipe intel_get_pipe_from_connector(struct intel_connector *connector);
 int intel_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void *data,
 				      struct drm_file *file_priv);
@@ -1699,6 +1698,8 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
 void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
 				 const struct intel_crtc_state *crtc_state,
 				 int state);
+void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
+			       const struct intel_crtc_state *crtc_state);
 void intel_dp_encoder_reset(struct drm_encoder *encoder);
 void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder);
 void intel_dp_encoder_destroy(struct drm_encoder *encoder);
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/5] drm/i915/fec: Disable FEC state.

[Anusha Srivatsa]

From: "Srivatsa, Anusha" <anusha.srivatsa@intel.com>

Set the suitable bits in DP_TP_CTL to stop
bit correction when DSC is disabled.

v2:
- rebased.
- Add additional check for compression state. (Gaurav)

Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ville Syrjala <ville.syrjala@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_ddi.c |  2 ++
 drivers/gpu/drm/i915/intel_dp.c  | 18 ++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 3 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 27fbfb7..0227a00 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -3162,6 +3162,8 @@ static void intel_disable_ddi_dp(struct intel_encoder *encoder,
 	/* Disable the decompression in DP Sink */
 	intel_dp_sink_set_decompression_state(intel_dp, old_crtc_state,
 					      ~DP_DECOMPRESSION_EN);
+	/* Disable FEC in DP Sink */
+	intel_dp_disable_fec_state(intel_dp, old_crtc_state);
 }
 
 static void intel_disable_ddi_hdmi(struct intel_encoder *encoder,
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1651da4..fe36318 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2992,6 +2992,24 @@ void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
 		DRM_ERROR("Timed out waiting for FEC Enable Status\n");
 }
 
+void intel_dp_disable_fec_state(struct intel_dp *intel_dp,
+				const struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	enum port port = intel_dig_port->base.port;
+	u32 val;
+
+	if (crtc_state->dsc_params.compression_enable)
+		DRM_DEBUG_KMS("Compression still enabled\n");
+		return;
+
+	val = I915_READ(DP_TP_CTL(port));
+	val &= ~DP_TP_CTL_FEC_ENABLE;
+	I915_WRITE(DP_TP_CTL(port), val);
+	POSTING_READ(DP_TP_CTL(port));
+}
+
 /* If the sink supports it, try to set the power state appropriately */
 void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 117ddaf..fd6d1d3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1700,6 +1700,8 @@ void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
 				 int state);
 void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
 			       const struct intel_crtc_state *crtc_state);
+void intel_dp_disable_fec_state(struct intel_dp *intel_dp,
+				const struct intel_crtc_state *crtc_state);
 void intel_dp_encoder_reset(struct drm_encoder *encoder);
 void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder);
 void intel_dp_encoder_destroy(struct drm_encoder *encoder);
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915/fec: Disable FEC state.

[Chris Wilson]

Quoting Anusha Srivatsa (2018-08-08 00:05:32)
> +void intel_dp_disable_fec_state(struct intel_dp *intel_dp,
> +                               const struct intel_crtc_state *crtc_state)
> +{
> +       struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> +       struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +       enum port port = intel_dig_port->base.port;
> +       u32 val;
> +
> +       if (crtc_state->dsc_params.compression_enable)
> +               DRM_DEBUG_KMS("Compression still enabled\n");
> +               return;

Time for a new compiler?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for Forward Error Correction

[Patchwork]

== Series Details ==

Series: Forward Error Correction
URL   : https://patchwork.freedesktop.org/series/47848/
State : failure

== Summary ==

Applying: drm/dp/fec: DRM helper for Forward Error Correction
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/drm_dp_helper.c
M	include/drm/drm_dp_helper.h
Falling back to patching base and 3-way merge...
Auto-merging include/drm/drm_dp_helper.h
Auto-merging drivers/gpu/drm/drm_dp_helper.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/drm_dp_helper.c
error: Failed to merge in the changes.
Patch failed at 0001 drm/dp/fec: DRM helper for Forward Error Correction
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx