drm/i915/skl: Add module parameter to select edp vswing table
diff mbox

Message ID 1430819164-22609-1-git-send-email-sonika.jindal@intel.com
State New
Headers show

Commit Message

sonika.jindal@intel.com May 5, 2015, 9:46 a.m. UTC
This provides an option to override the value set by VBT
for selecting edp Vswing Pre-emph setting table.

https://bugs.freedesktop.org/show_bug.cgi?id=89554
Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h    |    4 +++-
 drivers/gpu/drm/i915/i915_params.c |    7 +++++++
 drivers/gpu/drm/i915/intel_bios.c  |    9 +++++++--
 drivers/gpu/drm/i915/intel_ddi.c   |    2 +-
 drivers/gpu/drm/i915/intel_dp.c    |    2 +-
 5 files changed, 19 insertions(+), 5 deletions(-)

Comments

Jani Nikula May 5, 2015, 11:39 a.m. UTC | #1
On Tue, 05 May 2015, Sonika Jindal <sonika.jindal@intel.com> wrote:
> This provides an option to override the value set by VBT
> for selecting edp Vswing Pre-emph setting table.
>
> https://bugs.freedesktop.org/show_bug.cgi?id=89554
> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h    |    4 +++-
>  drivers/gpu/drm/i915/i915_params.c |    7 +++++++
>  drivers/gpu/drm/i915/intel_bios.c  |    9 +++++++--
>  drivers/gpu/drm/i915/intel_ddi.c   |    2 +-
>  drivers/gpu/drm/i915/intel_dp.c    |    2 +-
>  5 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 136d42a..1c3ee66 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1364,7 +1364,6 @@ struct intel_vbt_data {
>  	bool edp_initialized;
>  	bool edp_support;
>  	int edp_bpp;
> -	bool edp_low_vswing;
>  	struct edp_power_seq edp_pps;
>  
>  	struct {
> @@ -1846,6 +1845,8 @@ struct drm_i915_private {
>  		void (*stop_ring)(struct intel_engine_cs *ring);
>  	} gt;
>  
> +	bool edp_low_vswing;
> +
>  	/*
>  	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
>  	 * will be rejected. Instead look for a better place.
> @@ -2515,6 +2516,7 @@ struct i915_params {
>  	int mmio_debug;
>  	bool verbose_state_checks;
>  	bool nuclear_pageflip;
> +	int edp_vswing;
>  };
>  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 bb64415..d245ac5 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -53,6 +53,7 @@ struct i915_params i915 __read_mostly = {
>  	.mmio_debug = 0,
>  	.verbose_state_checks = 1,
>  	.nuclear_pageflip = 0,
> +	.edp_vswing = 0,
>  };
>  
>  module_param_named(modeset, i915.modeset, int, 0400);
> @@ -184,3 +185,9 @@ MODULE_PARM_DESC(verbose_state_checks,
>  module_param_named_unsafe(nuclear_pageflip, i915.nuclear_pageflip, bool, 0600);
>  MODULE_PARM_DESC(nuclear_pageflip,
>  		 "Force atomic modeset functionality; only planes work for now (default: false).");
> +
> +module_param_named_unsafe(edp_vswing, i915.edp_vswing, int, 0600);
> +MODULE_PARM_DESC(edp_vswing,
> +		 "Ignore/Override vswing pre-emph table selection from VBT "
> +		 "(0=use value from vbt [default], 1=low power swing(200mV),"
> +		 "2=default swing(400mV))");
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index c08368c..cee596d 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -672,8 +672,13 @@ parse_edp(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
>  	if (bdb->version >= 173) {
>  		uint8_t vswing;
>  
> -		vswing = (edp->edp_vswing_preemph >> (panel_type * 4)) & 0xF;
> -		dev_priv->vbt.edp_low_vswing = vswing == 0;
> +		/* Don't read from VBT if module parameter has valid value*/
> +		if (i915.edp_vswing) {
> +			dev_priv->edp_low_vswing = i915.edp_vswing == 1;
> +		} else {
> +			vswing = (edp->edp_vswing_preemph >> (panel_type * 4)) & 0xF;
> +			dev_priv->edp_low_vswing = vswing == 0;
> +		}

Please keep dev_priv->vbt.edp_low_vswing as it is, and check the module
parameter where that is used. If we're adding a module parameter, it
will be immensely useful to be able to change the value at runtime to
check if it makes a difference. What you suggest requires a module
reload.

There's still the question whether we can default to using the vbt value
if that can be broken...

BR,
Jani.


>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 9c1e74a..2539f22 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -282,7 +282,7 @@ static void intel_prepare_ddi_buffers(struct drm_device *dev, enum port port,
>  		ddi_translations_fdi = NULL;
>  		ddi_translations_dp = skl_ddi_translations_dp;
>  		n_dp_entries = ARRAY_SIZE(skl_ddi_translations_dp);
> -		if (dev_priv->vbt.edp_low_vswing) {
> +		if (dev_priv->edp_low_vswing) {
>  			ddi_translations_edp = skl_ddi_translations_edp;
>  			n_edp_entries = ARRAY_SIZE(skl_ddi_translations_edp);
>  		} else {
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index bacdec5..5f53666 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2894,7 +2894,7 @@ intel_dp_voltage_max(struct intel_dp *intel_dp)
>  	if (IS_BROXTON(dev))
>  		return DP_TRAIN_VOLTAGE_SWING_LEVEL_3;
>  	else if (INTEL_INFO(dev)->gen >= 9) {
> -		if (dev_priv->vbt.edp_low_vswing && port == PORT_A)
> +		if (dev_priv->edp_low_vswing && port == PORT_A)
>  			return DP_TRAIN_VOLTAGE_SWING_LEVEL_3;
>  		return DP_TRAIN_VOLTAGE_SWING_LEVEL_2;
>  	} else if (IS_VALLEYVIEW(dev))
> -- 
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Lespiau, Damien May 5, 2015, 1 p.m. UTC | #2
On Tue, May 05, 2015 at 02:39:48PM +0300, Jani Nikula wrote:
> There's still the question whether we can default to using the vbt value
> if that can be broken...

This is a workaround for SDPs loaded with a "default" VBT which doesn't
correspond to the full fleet of SPDs. For actual product, it's really
expected to have a VBT with a correct low vswing bit set, otherwise the
panel won't lit up (even on Windows!).

It's probably a good idea to add a comment saying that's it's a
workaround for early platform with generic-ish VBT. Something that we
should remove in a bit.
Jani Nikula May 5, 2015, 1:58 p.m. UTC | #3
On Tue, 05 May 2015, Damien Lespiau <damien.lespiau@intel.com> wrote:
> On Tue, May 05, 2015 at 02:39:48PM +0300, Jani Nikula wrote:
>> There's still the question whether we can default to using the vbt value
>> if that can be broken...
>
> This is a workaround for SDPs loaded with a "default" VBT which doesn't
> correspond to the full fleet of SPDs. For actual product, it's really
> expected to have a VBT with a correct low vswing bit set, otherwise the
> panel won't lit up (even on Windows!).
>
> It's probably a good idea to add a comment saying that's it's a
> workaround for early platform with generic-ish VBT. Something that we
> should remove in a bit.

Thanks for the clarification, Damien. That settles the default then. I'd
still like the module parameter to be effective even if changed runtime.

BR,
Jani.



>
> -- 
> Damien
Shuang He May 5, 2015, 4:52 p.m. UTC | #4
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6319
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                                  316/316              316/316
IVB                                  342/342              342/342
BYT                                  286/286              286/286
BDW                                  318/318              318/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
Note: You need to pay more attention to line start with '*'
sonika.jindal@intel.com May 6, 2015, 5:43 a.m. UTC | #5
On 5/5/2015 7:28 PM, Jani Nikula wrote:
> On Tue, 05 May 2015, Damien Lespiau <damien.lespiau@intel.com> wrote:
>> On Tue, May 05, 2015 at 02:39:48PM +0300, Jani Nikula wrote:
>>> There's still the question whether we can default to using the vbt value
>>> if that can be broken...
>>
>> This is a workaround for SDPs loaded with a "default" VBT which doesn't
>> correspond to the full fleet of SPDs. For actual product, it's really
>> expected to have a VBT with a correct low vswing bit set, otherwise the
>> panel won't lit up (even on Windows!).
>>
>> It's probably a good idea to add a comment saying that's it's a
>> workaround for early platform with generic-ish VBT. Something that we
>> should remove in a bit.
>
> Thanks for the clarification, Damien. That settles the default then. I'd
> still like the module parameter to be effective even if changed runtime.
>
This is used while preparing ddi buffers to program ddi_buf_trans during 
driver load. It will not really have any effect at runtime until power 
well is disabled/re-enabled.
PLease let me know if you still see the need of moving the check 
directly in prepare_ddi_buffers.

Regards,
Sonika
> BR,
> Jani.
>
>
>
>>
>> --
>> Damien
>
Jani Nikula May 6, 2015, 6:57 a.m. UTC | #6
On Wed, 06 May 2015, "Jindal, Sonika" <sonika.jindal@intel.com> wrote:
> On 5/5/2015 7:28 PM, Jani Nikula wrote:
>> On Tue, 05 May 2015, Damien Lespiau <damien.lespiau@intel.com> wrote:
>>> On Tue, May 05, 2015 at 02:39:48PM +0300, Jani Nikula wrote:
>>>> There's still the question whether we can default to using the vbt value
>>>> if that can be broken...
>>>
>>> This is a workaround for SDPs loaded with a "default" VBT which doesn't
>>> correspond to the full fleet of SPDs. For actual product, it's really
>>> expected to have a VBT with a correct low vswing bit set, otherwise the
>>> panel won't lit up (even on Windows!).
>>>
>>> It's probably a good idea to add a comment saying that's it's a
>>> workaround for early platform with generic-ish VBT. Something that we
>>> should remove in a bit.
>>
>> Thanks for the clarification, Damien. That settles the default then. I'd
>> still like the module parameter to be effective even if changed runtime.
>>
> This is used while preparing ddi buffers to program ddi_buf_trans during 
> driver load. It will not really have any effect at runtime until power 
> well is disabled/re-enabled.

Right, missed that part, sorry.

> PLease let me know if you still see the need of moving the check 
> directly in prepare_ddi_buffers.

No, it would be confusing. And I guess we'll remove this parameter
afterwards anyway?

Please just change the module parameter permissions to read only, so
it's not possible to change it dynamically via sysfs.

BR,
Jani.


>
> Regards,
> Sonika
>> BR,
>> Jani.
>>
>>
>>
>>>
>>> --
>>> Damien
>>

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 136d42a..1c3ee66 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1364,7 +1364,6 @@  struct intel_vbt_data {
 	bool edp_initialized;
 	bool edp_support;
 	int edp_bpp;
-	bool edp_low_vswing;
 	struct edp_power_seq edp_pps;
 
 	struct {
@@ -1846,6 +1845,8 @@  struct drm_i915_private {
 		void (*stop_ring)(struct intel_engine_cs *ring);
 	} gt;
 
+	bool edp_low_vswing;
+
 	/*
 	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
 	 * will be rejected. Instead look for a better place.
@@ -2515,6 +2516,7 @@  struct i915_params {
 	int mmio_debug;
 	bool verbose_state_checks;
 	bool nuclear_pageflip;
+	int edp_vswing;
 };
 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 bb64415..d245ac5 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -53,6 +53,7 @@  struct i915_params i915 __read_mostly = {
 	.mmio_debug = 0,
 	.verbose_state_checks = 1,
 	.nuclear_pageflip = 0,
+	.edp_vswing = 0,
 };
 
 module_param_named(modeset, i915.modeset, int, 0400);
@@ -184,3 +185,9 @@  MODULE_PARM_DESC(verbose_state_checks,
 module_param_named_unsafe(nuclear_pageflip, i915.nuclear_pageflip, bool, 0600);
 MODULE_PARM_DESC(nuclear_pageflip,
 		 "Force atomic modeset functionality; only planes work for now (default: false).");
+
+module_param_named_unsafe(edp_vswing, i915.edp_vswing, int, 0600);
+MODULE_PARM_DESC(edp_vswing,
+		 "Ignore/Override vswing pre-emph table selection from VBT "
+		 "(0=use value from vbt [default], 1=low power swing(200mV),"
+		 "2=default swing(400mV))");
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index c08368c..cee596d 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -672,8 +672,13 @@  parse_edp(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
 	if (bdb->version >= 173) {
 		uint8_t vswing;
 
-		vswing = (edp->edp_vswing_preemph >> (panel_type * 4)) & 0xF;
-		dev_priv->vbt.edp_low_vswing = vswing == 0;
+		/* Don't read from VBT if module parameter has valid value*/
+		if (i915.edp_vswing) {
+			dev_priv->edp_low_vswing = i915.edp_vswing == 1;
+		} else {
+			vswing = (edp->edp_vswing_preemph >> (panel_type * 4)) & 0xF;
+			dev_priv->edp_low_vswing = vswing == 0;
+		}
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 9c1e74a..2539f22 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -282,7 +282,7 @@  static void intel_prepare_ddi_buffers(struct drm_device *dev, enum port port,
 		ddi_translations_fdi = NULL;
 		ddi_translations_dp = skl_ddi_translations_dp;
 		n_dp_entries = ARRAY_SIZE(skl_ddi_translations_dp);
-		if (dev_priv->vbt.edp_low_vswing) {
+		if (dev_priv->edp_low_vswing) {
 			ddi_translations_edp = skl_ddi_translations_edp;
 			n_edp_entries = ARRAY_SIZE(skl_ddi_translations_edp);
 		} else {
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index bacdec5..5f53666 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2894,7 +2894,7 @@  intel_dp_voltage_max(struct intel_dp *intel_dp)
 	if (IS_BROXTON(dev))
 		return DP_TRAIN_VOLTAGE_SWING_LEVEL_3;
 	else if (INTEL_INFO(dev)->gen >= 9) {
-		if (dev_priv->vbt.edp_low_vswing && port == PORT_A)
+		if (dev_priv->edp_low_vswing && port == PORT_A)
 			return DP_TRAIN_VOLTAGE_SWING_LEVEL_3;
 		return DP_TRAIN_VOLTAGE_SWING_LEVEL_2;
 	} else if (IS_VALLEYVIEW(dev))