| Message ID | 1430819164-22609-1-git-send-email-sonika.jindal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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 '*'
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 >
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 >>
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))
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(-)