diff mbox series

[RFC] drm/i915/dp: Remove i915.enable_dp_mst module parameter

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

Commit Message

Dhinakaran Pandiyan Oct. 3, 2018, 7:36 p.m. UTC
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(-)

Comments

Rodrigo Vivi Oct. 3, 2018, 8:29 p.m. UTC | #1
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
>
Jani Nikula Oct. 4, 2018, 7:03 a.m. UTC | #2
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.
Ville Syrjälä Oct. 4, 2018, 10:59 a.m. UTC | #3
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.
Rodrigo Vivi Oct. 4, 2018, 3:54 p.m. UTC | #4
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
Dhinakaran Pandiyan Oct. 4, 2018, 5:36 p.m. UTC | #5
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 mbox series

Patch

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;