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

[Jani Nikula <jani.nikula@linux.intel.com>]

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