[PATCH] drm/i915: Make DP fast link training a module parameter

[PATCH] drm/i915: Make DP fast link training a module parameter

[Mika Kahola]

There is a bug report

https://bugs.freedesktop.org/show_bug.cgi?id=91393

indicating that there are panels that do not support
link training starting with non-zero voltage swing and
pre-emphasis values.

This patch proposes to make this fast link training
feature as one module parameter. To take more conservative
approach this feature is disabled by default.

Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h               |  1 +
 drivers/gpu/drm/i915/i915_params.c            |  4 ++++
 drivers/gpu/drm/i915/intel_dp_link_training.c | 11 ++++++++++-
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a47e0f4..dc2d5a0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2669,6 +2669,7 @@ struct i915_params {
 	bool verbose_state_checks;
 	bool nuclear_pageflip;
 	int edp_vswing;
+	bool enable_dp_flt;
 };
 extern struct i915_params i915 __read_mostly;
 
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 835d609..aea5d47 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -56,6 +56,7 @@ struct i915_params i915 __read_mostly = {
 	.edp_vswing = 0,
 	.enable_guc_submission = false,
 	.guc_log_level = -1,
+	.enable_dp_flt = false,
 };
 
 module_param_named(modeset, i915.modeset, int, 0400);
@@ -202,3 +203,6 @@ MODULE_PARM_DESC(enable_guc_submission, "Enable GuC submission (default:false)")
 module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
 MODULE_PARM_DESC(guc_log_level,
 	"GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
+
+module_param_named_unsafe(enable_dp_flt, i915.enable_dp_flt, bool, 0400);
+MODULE_PARM_DESC(enable_dp_flt, "Enable DP fast link training (default:false)");
diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 8888793..f8b6d69 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -85,8 +85,17 @@ static bool
 intel_dp_reset_link_train(struct intel_dp *intel_dp,
 			uint8_t dp_train_pat)
 {
-	if (!intel_dp->train_set_valid)
+	if (i915.enable_dp_flt) {
+		DRM_DEBUG_KMS("DP flt enabled, train set valid: %s\n",
+			      intel_dp->train_set_valid ? "true" : "false");
+
+		if (!intel_dp->train_set_valid)
+			memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
+	} else {
+		DRM_DEBUG_KMS("DP flt disabled\n");
 		memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
+	}
+
 	intel_dp_set_signal_levels(intel_dp);
 	return intel_dp_set_link_train(intel_dp, dp_train_pat);
 }
-- 
1.9.1

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

Re: [PATCH] drm/i915: Make DP fast link training a module parameter

[Jani Nikula]

On Fri, 20 Nov 2015, Mika Kahola <mika.kahola@intel.com> wrote:
> There is a bug report
>
> https://bugs.freedesktop.org/show_bug.cgi?id=91393
>
> indicating that there are panels that do not support
> link training starting with non-zero voltage swing and
> pre-emphasis values.
>
> This patch proposes to make this fast link training
> feature as one module parameter. To take more conservative
> approach this feature is disabled by default.

Please let's not add any more of these. If we can't make this work
automatically (we should still keep trying) I'd say we remove it.

Adding features behind module parameters means that most people will
never use them. Theoretically it doubles our testing efforts because we
would have to test with the feature on and off; in practise that won't
happen and only the default will be tested (because we already have so
many features behind parameters). The code bitrots and breaks even for
the platforms on which it did work originally. Then there are a minority
of people who will enable all possible knobs they read about on forums
and think will improve performance or power savings or something, and
report bugs when they fail.

IMO there's just too much effort for too little gain if we can't enable
this. I just wish we would have been able to block more of the module
parameters earlier...


BR,
Jani.


>
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h               |  1 +
>  drivers/gpu/drm/i915/i915_params.c            |  4 ++++
>  drivers/gpu/drm/i915/intel_dp_link_training.c | 11 ++++++++++-
>  3 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a47e0f4..dc2d5a0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2669,6 +2669,7 @@ struct i915_params {
>  	bool verbose_state_checks;
>  	bool nuclear_pageflip;
>  	int edp_vswing;
> +	bool enable_dp_flt;
>  };
>  extern struct i915_params i915 __read_mostly;
>  
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 835d609..aea5d47 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -56,6 +56,7 @@ struct i915_params i915 __read_mostly = {
>  	.edp_vswing = 0,
>  	.enable_guc_submission = false,
>  	.guc_log_level = -1,
> +	.enable_dp_flt = false,
>  };
>  
>  module_param_named(modeset, i915.modeset, int, 0400);
> @@ -202,3 +203,6 @@ MODULE_PARM_DESC(enable_guc_submission, "Enable GuC submission (default:false)")
>  module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
>  MODULE_PARM_DESC(guc_log_level,
>  	"GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
> +
> +module_param_named_unsafe(enable_dp_flt, i915.enable_dp_flt, bool, 0400);
> +MODULE_PARM_DESC(enable_dp_flt, "Enable DP fast link training (default:false)");
> diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> index 8888793..f8b6d69 100644
> --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> @@ -85,8 +85,17 @@ static bool
>  intel_dp_reset_link_train(struct intel_dp *intel_dp,
>  			uint8_t dp_train_pat)
>  {
> -	if (!intel_dp->train_set_valid)
> +	if (i915.enable_dp_flt) {
> +		DRM_DEBUG_KMS("DP flt enabled, train set valid: %s\n",
> +			      intel_dp->train_set_valid ? "true" : "false");
> +
> +		if (!intel_dp->train_set_valid)
> +			memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
> +	} else {
> +		DRM_DEBUG_KMS("DP flt disabled\n");
>  		memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
> +	}
> +
>  	intel_dp_set_signal_levels(intel_dp);
>  	return intel_dp_set_link_train(intel_dp, dp_train_pat);
>  }

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

Re: [PATCH] drm/i915: Make DP fast link training a module parameter

[Kahola, Mika]

> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> Sent: Friday, November 20, 2015 3:47 PM
> To: Kahola, Mika; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Make DP fast link training a
> module parameter
> 
> On Fri, 20 Nov 2015, Mika Kahola <mika.kahola@intel.com> wrote:
> > There is a bug report
> >
> > https://bugs.freedesktop.org/show_bug.cgi?id=91393
> >
> > indicating that there are panels that do not support link training
> > starting with non-zero voltage swing and pre-emphasis values.
> >
> > This patch proposes to make this fast link training feature as one
> > module parameter. To take more conservative approach this feature is
> > disabled by default.
> 
> Please let's not add any more of these. If we can't make this work
> automatically (we should still keep trying) I'd say we remove it.
> 
> Adding features behind module parameters means that most people will
> never use them. Theoretically it doubles our testing efforts because we
> would have to test with the feature on and off; in practise that won't happen
> and only the default will be tested (because we already have so many
> features behind parameters). The code bitrots and breaks even for the
> platforms on which it did work originally. Then there are a minority of people
> who will enable all possible knobs they read about on forums and think will
> improve performance or power savings or something, and report bugs when
> they fail.
> 
> IMO there's just too much effort for too little gain if we can't enable this. I
> just wish we would have been able to block more of the module parameters
> earlier...
> 
The idea that I had in mind was to keep this feature as selectable item until we get this working for all panel combinations. That said, I consider this would have been only a first aid to the problem.

Anyway, back to the drawing board...

Cheers,
Mika

> 
> BR,
> Jani.
> 
> 
> >
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h               |  1 +
> >  drivers/gpu/drm/i915/i915_params.c            |  4 ++++
> >  drivers/gpu/drm/i915/intel_dp_link_training.c | 11 ++++++++++-
> >  3 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h index a47e0f4..dc2d5a0 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2669,6 +2669,7 @@ struct i915_params {
> >  	bool verbose_state_checks;
> >  	bool nuclear_pageflip;
> >  	int edp_vswing;
> > +	bool enable_dp_flt;
> >  };
> >  extern struct i915_params i915 __read_mostly;
> >
> > diff --git a/drivers/gpu/drm/i915/i915_params.c
> > b/drivers/gpu/drm/i915/i915_params.c
> > index 835d609..aea5d47 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -56,6 +56,7 @@ struct i915_params i915 __read_mostly = {
> >  	.edp_vswing = 0,
> >  	.enable_guc_submission = false,
> >  	.guc_log_level = -1,
> > +	.enable_dp_flt = false,
> >  };
> >
> >  module_param_named(modeset, i915.modeset, int, 0400); @@ -202,3
> > +203,6 @@ MODULE_PARM_DESC(enable_guc_submission, "Enable GuC
> > submission (default:false)")  module_param_named(guc_log_level,
> > i915.guc_log_level, int, 0400);  MODULE_PARM_DESC(guc_log_level,
> >  	"GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
> > +
> > +module_param_named_unsafe(enable_dp_flt, i915.enable_dp_flt, bool,
> > +0400); MODULE_PARM_DESC(enable_dp_flt, "Enable DP fast link training
> > +(default:false)");
> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > index 8888793..f8b6d69 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > @@ -85,8 +85,17 @@ static bool
> >  intel_dp_reset_link_train(struct intel_dp *intel_dp,
> >  			uint8_t dp_train_pat)
> >  {
> > -	if (!intel_dp->train_set_valid)
> > +	if (i915.enable_dp_flt) {
> > +		DRM_DEBUG_KMS("DP flt enabled, train set valid: %s\n",
> > +			      intel_dp->train_set_valid ? "true" : "false");
> > +
> > +		if (!intel_dp->train_set_valid)
> > +			memset(intel_dp->train_set, 0, sizeof(intel_dp-
> >train_set));
> > +	} else {
> > +		DRM_DEBUG_KMS("DP flt disabled\n");
> >  		memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
> > +	}
> > +
> >  	intel_dp_set_signal_levels(intel_dp);
> >  	return intel_dp_set_link_train(intel_dp, dp_train_pat);  }
> 
> --
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Make DP fast link training a module parameter

[Jani Nikula]

On Fri, 20 Nov 2015, "Kahola, Mika" <mika.kahola@intel.com> wrote:
>> -----Original Message-----
>> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>> Sent: Friday, November 20, 2015 3:47 PM
>> To: Kahola, Mika; intel-gfx@lists.freedesktop.org
>> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Make DP fast link training a
>> module parameter
>> 
>> On Fri, 20 Nov 2015, Mika Kahola <mika.kahola@intel.com> wrote:
>> > There is a bug report
>> >
>> > https://bugs.freedesktop.org/show_bug.cgi?id=91393
>> >
>> > indicating that there are panels that do not support link training
>> > starting with non-zero voltage swing and pre-emphasis values.
>> >
>> > This patch proposes to make this fast link training feature as one
>> > module parameter. To take more conservative approach this feature is
>> > disabled by default.
>> 
>> Please let's not add any more of these. If we can't make this work
>> automatically (we should still keep trying) I'd say we remove it.
>> 
>> Adding features behind module parameters means that most people will
>> never use them. Theoretically it doubles our testing efforts because we
>> would have to test with the feature on and off; in practise that won't happen
>> and only the default will be tested (because we already have so many
>> features behind parameters). The code bitrots and breaks even for the
>> platforms on which it did work originally. Then there are a minority of people
>> who will enable all possible knobs they read about on forums and think will
>> improve performance or power savings or something, and report bugs when
>> they fail.
>> 
>> IMO there's just too much effort for too little gain if we can't enable this. I
>> just wish we would have been able to block more of the module parameters
>> earlier...
>> 
> The idea that I had in mind was to keep this feature as selectable
> item until we get this working for all panel combinations. That said,
> I consider this would have been only a first aid to the problem.

I know. It's the same logic behind roughly all of the feature enable
module parameters we have. :(

BR,
Jani.

>
> Anyway, back to the drawing board...
>
> Cheers,
> Mika
>
>> 
>> BR,
>> Jani.
>> 
>> 
>> >
>> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_drv.h               |  1 +
>> >  drivers/gpu/drm/i915/i915_params.c            |  4 ++++
>> >  drivers/gpu/drm/i915/intel_dp_link_training.c | 11 ++++++++++-
>> >  3 files changed, 15 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> > b/drivers/gpu/drm/i915/i915_drv.h index a47e0f4..dc2d5a0 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > @@ -2669,6 +2669,7 @@ struct i915_params {
>> >  	bool verbose_state_checks;
>> >  	bool nuclear_pageflip;
>> >  	int edp_vswing;
>> > +	bool enable_dp_flt;
>> >  };
>> >  extern struct i915_params i915 __read_mostly;
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_params.c
>> > b/drivers/gpu/drm/i915/i915_params.c
>> > index 835d609..aea5d47 100644
>> > --- a/drivers/gpu/drm/i915/i915_params.c
>> > +++ b/drivers/gpu/drm/i915/i915_params.c
>> > @@ -56,6 +56,7 @@ struct i915_params i915 __read_mostly = {
>> >  	.edp_vswing = 0,
>> >  	.enable_guc_submission = false,
>> >  	.guc_log_level = -1,
>> > +	.enable_dp_flt = false,
>> >  };
>> >
>> >  module_param_named(modeset, i915.modeset, int, 0400); @@ -202,3
>> > +203,6 @@ MODULE_PARM_DESC(enable_guc_submission, "Enable GuC
>> > submission (default:false)")  module_param_named(guc_log_level,
>> > i915.guc_log_level, int, 0400);  MODULE_PARM_DESC(guc_log_level,
>> >  	"GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
>> > +
>> > +module_param_named_unsafe(enable_dp_flt, i915.enable_dp_flt, bool,
>> > +0400); MODULE_PARM_DESC(enable_dp_flt, "Enable DP fast link training
>> > +(default:false)");
>> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c
>> > b/drivers/gpu/drm/i915/intel_dp_link_training.c
>> > index 8888793..f8b6d69 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
>> > @@ -85,8 +85,17 @@ static bool
>> >  intel_dp_reset_link_train(struct intel_dp *intel_dp,
>> >  			uint8_t dp_train_pat)
>> >  {
>> > -	if (!intel_dp->train_set_valid)
>> > +	if (i915.enable_dp_flt) {
>> > +		DRM_DEBUG_KMS("DP flt enabled, train set valid: %s\n",
>> > +			      intel_dp->train_set_valid ? "true" : "false");
>> > +
>> > +		if (!intel_dp->train_set_valid)
>> > +			memset(intel_dp->train_set, 0, sizeof(intel_dp-
>> >train_set));
>> > +	} else {
>> > +		DRM_DEBUG_KMS("DP flt disabled\n");
>> >  		memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
>> > +	}
>> > +
>> >  	intel_dp_set_signal_levels(intel_dp);
>> >  	return intel_dp_set_link_train(intel_dp, dp_train_pat);  }
>> 
>> --
>> Jani Nikula, Intel Open Source Technology Center

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