| Message ID | 1448025942-3623-1-git-send-email-mika.kahola@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); > }
> -----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
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
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); }
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(-)