| Message ID | 20181003193601.16260-1-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] drm/i915/dp: Remove i915.enable_dp_mst module parameter | expand |
On Wed, Oct 03, 2018 at 12:36:01PM -0700, Dhinakaran Pandiyan wrote: > MST is enabled by default on all platforms that support it. I don't think > we should be providing a switch to work around MST issues as the feature > has been supported for a while now. Let's kill this module parameter > that we also do not test in CI. > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/i915_params.c | 3 --- > drivers/gpu/drm/i915/i915_params.h | 1 - > drivers/gpu/drm/i915/intel_dp.c | 6 ------ > 3 files changed, 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > index bd6bd8879cab..ea5961ae6803 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -156,9 +156,6 @@ i915_param_named_unsafe(huc_firmware_path, charp, 0400, > i915_param_named_unsafe(dmc_firmware_path, charp, 0400, > "DMC firmware path to use instead of the default one"); > > -i915_param_named_unsafe(enable_dp_mst, bool, 0600, > - "Enable multi-stream transport (MST) for new DisplayPort sinks. (default: true)"); > - > #if IS_ENABLED(CONFIG_DRM_I915_DEBUG) > i915_param_named_unsafe(inject_load_failure, uint, 0400, > "Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)"); > diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h > index 7e56c516c815..5d995af2ef58 100644 > --- a/drivers/gpu/drm/i915/i915_params.h > +++ b/drivers/gpu/drm/i915/i915_params.h > @@ -65,7 +65,6 @@ struct drm_printer; > param(bool, disable_display, false) \ > param(bool, verbose_state_checks, true) \ > param(bool, nuclear_pageflip, false) \ > - param(bool, enable_dp_mst, true) \ > param(bool, enable_dpcd_backlight, false) \ > param(bool, enable_gvt, false) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 15a981ef5966..b0d8baae6d96 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4054,9 +4054,6 @@ intel_dp_can_mst(struct intel_dp *intel_dp) > { > u8 mstm_cap; > > - if (!i915_modparams.enable_dp_mst) > - return false; > - > if (!intel_dp->can_mst) > return false; > > @@ -4072,9 +4069,6 @@ intel_dp_can_mst(struct intel_dp *intel_dp) > static void > intel_dp_configure_mst(struct intel_dp *intel_dp) > { > - if (!i915_modparams.enable_dp_mst) > - return; > - > if (!intel_dp->can_mst) > return; > > -- > 2.17.1 >
On Wed, 03 Oct 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote: > MST is enabled by default on all platforms that support it. I don't think > we should be providing a switch to work around MST issues as the feature > has been supported for a while now. Let's kill this module parameter > that we also do not test in CI. I agree we don't want to provide this to users to *work around* issues. But maybe we want something like this to *debug* issues? We have a plethora of unsafe/debug module parameters mainly because they're convenient and take effect from the beginning of the probe. If there was anything comparable for debugfs, I'd be all in. Should we have this in debugfs instead? Cc: Lyude for insights too. BR, Jani.
On Thu, Oct 04, 2018 at 10:03:39AM +0300, Jani Nikula wrote: > On Wed, 03 Oct 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote: > > MST is enabled by default on all platforms that support it. I don't think > > we should be providing a switch to work around MST issues as the feature > > has been supported for a while now. Let's kill this module parameter > > that we also do not test in CI. > > I agree we don't want to provide this to users to *work around* > issues. But maybe we want something like this to *debug* issues? Yes. I was using it for that just a few days ago when looking at a bug. Also the mst code lacks a bunch of features I think we'd want (remote dpcd, remote i2c write, maybe others). It's still the unloved stepchild with no one really focusing on improving it. So I think it's way too early to think about removing this outright. Not sure we should ever remove it really. What happens if in the future most of our ci displays are mst capable? Do we just not test sst at all? Granted a modparam is a probably a bit too coarse for that, but I think we may want *something* to force sst.
On Thu, Oct 04, 2018 at 01:59:52PM +0300, Ville Syrjälä wrote: > On Thu, Oct 04, 2018 at 10:03:39AM +0300, Jani Nikula wrote: > > On Wed, 03 Oct 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote: > > > MST is enabled by default on all platforms that support it. I don't think > > > we should be providing a switch to work around MST issues as the feature > > > has been supported for a while now. Let's kill this module parameter > > > that we also do not test in CI. > > > > I agree we don't want to provide this to users to *work around* > > issues. But maybe we want something like this to *debug* issues? > > Yes. I was using it for that just a few days ago when looking at a bug. so it seems useful and it means that we need to move to debugfs :) > > Also the mst code lacks a bunch of features I think we'd want (remote dpcd, > remote i2c write, maybe others). It's still the unloved stepchild with no > one really focusing on improving it. > > So I think it's way too early to think about removing this outright. > Not sure we should ever remove it really. What happens if in the future > most of our ci displays are mst capable? Do we just not test sst at all? > Granted a modparam is a probably a bit too coarse for that, but I think > we may want *something* to force sst. > > -- > Ville Syrjälä > Intel > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, 2018-10-04 at 08:54 -0700, Rodrigo Vivi wrote: > On Thu, Oct 04, 2018 at 01:59:52PM +0300, Ville Syrjälä wrote: > > On Thu, Oct 04, 2018 at 10:03:39AM +0300, Jani Nikula wrote: > > > On Wed, 03 Oct 2018, Dhinakaran Pandiyan < > > > dhinakaran.pandiyan@intel.com> wrote: > > > > MST is enabled by default on all platforms that support it. I > > > > don't think > > > > we should be providing a switch to work around MST issues as > > > > the feature > > > > has been supported for a while now. Let's kill this module > > > > parameter > > > > that we also do not test in CI. > > > > > > I agree we don't want to provide this to users to *work around* > > > issues. But maybe we want something like this to *debug* issues? > > > > Yes. I was using it for that just a few days ago when looking at a > > bug. > > so it seems useful and it means that we need to move to debugfs :) > It also allows us to have a switch for each primary connector instead of disabling MST completely. > > > > Also the mst code lacks a bunch of features I think we'd want > > (remote dpcd, > > remote i2c write, maybe others). It's still the unloved stepchild > > with no > > one really focusing on improving it. > > Because things work (mostly) without them :) But yeah, remote dpcd reads can be very useful for debugging. Why are remote i2c writes needed though? -DK > > So I think it's way too early to think about removing this > > outright. > > Not sure we should ever remove it really. What happens if in the > > future > > most of our ci displays are mst capable? Do we just not test sst at > > all? > > Granted a modparam is a probably a bit too coarse for that, but I > > think > > we may want *something* to force sst. > > > > -- > > Ville Syrjälä > > Intel > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index bd6bd8879cab..ea5961ae6803 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -156,9 +156,6 @@ i915_param_named_unsafe(huc_firmware_path, charp, 0400, i915_param_named_unsafe(dmc_firmware_path, charp, 0400, "DMC firmware path to use instead of the default one"); -i915_param_named_unsafe(enable_dp_mst, bool, 0600, - "Enable multi-stream transport (MST) for new DisplayPort sinks. (default: true)"); - #if IS_ENABLED(CONFIG_DRM_I915_DEBUG) i915_param_named_unsafe(inject_load_failure, uint, 0400, "Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)"); diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index 7e56c516c815..5d995af2ef58 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -65,7 +65,6 @@ struct drm_printer; param(bool, disable_display, false) \ param(bool, verbose_state_checks, true) \ param(bool, nuclear_pageflip, false) \ - param(bool, enable_dp_mst, true) \ param(bool, enable_dpcd_backlight, false) \ param(bool, enable_gvt, false) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 15a981ef5966..b0d8baae6d96 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4054,9 +4054,6 @@ intel_dp_can_mst(struct intel_dp *intel_dp) { u8 mstm_cap; - if (!i915_modparams.enable_dp_mst) - return false; - if (!intel_dp->can_mst) return false; @@ -4072,9 +4069,6 @@ intel_dp_can_mst(struct intel_dp *intel_dp) static void intel_dp_configure_mst(struct intel_dp *intel_dp) { - if (!i915_modparams.enable_dp_mst) - return; - if (!intel_dp->can_mst) return;
MST is enabled by default on all platforms that support it. I don't think we should be providing a switch to work around MST issues as the feature has been supported for a while now. Let's kill this module parameter that we also do not test in CI. Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Jani Nikula <jani.nikula@linux.intel.com> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/i915_params.c | 3 --- drivers/gpu/drm/i915/i915_params.h | 1 - drivers/gpu/drm/i915/intel_dp.c | 6 ------ 3 files changed, 10 deletions(-)