| Message ID | 20190716220321.32192-1-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/vbt: Fix VBT parsing for the PSR section | expand |
On Tue, 2019-07-16 at 15:03 -0700, Dhinakaran Pandiyan wrote: > A single 32-bit PSR2 training pattern field follows the sixteen element > array of PSR table entries in the VBT spec. But, we incorrectly define > this PSR2 field for each of the PSR table entries. As a result, the PSR1 > training pattern duration for any panel_type != 0 will be parsed > incorrectly. Secondly, PSR2 training pattern durations for VBTs with bdb > version >= 226 will also be wrong. This was reported and bisected by Aliaksei Urbanski here - https://bugzilla.kernel.org/show_bug.cgi?id=204183 I'll add Bugzilla after the fix is confirmed. Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: José Roberto de Souza <jose.souza@intel.com> Fixes: 88a0d9606aff ("drm/i915/vbt: Parse and use the new field with PSR2 TP2/3 wakeup time")z Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/display/intel_bios.c | 2 +- drivers/gpu/drm/i915/display/intel_vbt_defs.h | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c index 21501d565327..b416b394b641 100644 --- a/drivers/gpu/drm/i915/display/intel_bios.c +++ b/drivers/gpu/drm/i915/display/intel_bios.c @@ -766,7 +766,7 @@ parse_psr(struct drm_i915_private *dev_priv, const struct bdb_header *bdb) } if (bdb->version >= 226) { - u32 wakeup_time = psr_table->psr2_tp2_tp3_wakeup_time; + u32 wakeup_time = psr->psr2_tp2_tp3_wakeup_time; wakeup_time = (wakeup_time >> (2 * panel_type)) & 0x3; switch (wakeup_time) { diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h index 93f5c9d204d6..09cd37fb0b1c 100644 --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h @@ -481,13 +481,13 @@ struct psr_table { /* TP wake up time in multiple of 100 */ u16 tp1_wakeup_time; u16 tp2_tp3_wakeup_time; - - /* PSR2 TP2/TP3 wakeup time for 16 panels */ - u32 psr2_tp2_tp3_wakeup_time; } __packed; struct bdb_psr { struct psr_table psr_table[16]; + + /* PSR2 TP2/TP3 wakeup time for 16 panels */ + u32 psr2_tp2_tp3_wakeup_time; } __packed; /*
On Tue, Jul 16, 2019 at 03:03:21PM -0700, Dhinakaran Pandiyan wrote: > A single 32-bit PSR2 training pattern field follows the sixteen element > array of PSR table entries in the VBT spec. But, we incorrectly define > this PSR2 field for each of the PSR table entries. As a result, the PSR1 > training pattern duration for any panel_type != 0 will be parsed > incorrectly. Secondly, PSR2 training pattern durations for VBTs with bdb > version >= 226 will also be wrong. > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: José Roberto de Souza <jose.souza@intel.com> > Fixes: 88a0d9606aff ("drm/i915/vbt: Parse and use the new field with PSR2 TP2/3 wakeup time") > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Please sync the igt copy after landing this. > --- > drivers/gpu/drm/i915/display/intel_bios.c | 2 +- > drivers/gpu/drm/i915/display/intel_vbt_defs.h | 6 +++--- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c > index 21501d565327..b416b394b641 100644 > --- a/drivers/gpu/drm/i915/display/intel_bios.c > +++ b/drivers/gpu/drm/i915/display/intel_bios.c > @@ -766,7 +766,7 @@ parse_psr(struct drm_i915_private *dev_priv, const struct bdb_header *bdb) > } > > if (bdb->version >= 226) { > - u32 wakeup_time = psr_table->psr2_tp2_tp3_wakeup_time; > + u32 wakeup_time = psr->psr2_tp2_tp3_wakeup_time; > > wakeup_time = (wakeup_time >> (2 * panel_type)) & 0x3; > switch (wakeup_time) { > diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h > index 93f5c9d204d6..09cd37fb0b1c 100644 > --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h > +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h > @@ -481,13 +481,13 @@ struct psr_table { > /* TP wake up time in multiple of 100 */ > u16 tp1_wakeup_time; > u16 tp2_tp3_wakeup_time; > - > - /* PSR2 TP2/TP3 wakeup time for 16 panels */ > - u32 psr2_tp2_tp3_wakeup_time; > } __packed; > > struct bdb_psr { > struct psr_table psr_table[16]; > + > + /* PSR2 TP2/TP3 wakeup time for 16 panels */ > + u32 psr2_tp2_tp3_wakeup_time; > } __packed; > > /* > -- > 2.17.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 2019-07-16 at 15:10 -0700, Pandiyan, Dhinakaran wrote: > On Tue, 2019-07-16 at 15:03 -0700, Dhinakaran Pandiyan wrote: > > A single 32-bit PSR2 training pattern field follows the sixteen > > element > > array of PSR table entries in the VBT spec. But, we incorrectly > > define > > this PSR2 field for each of the PSR table entries. As a result, the > > PSR1 > > training pattern duration for any panel_type != 0 will be parsed > > incorrectly. Secondly, PSR2 training pattern durations for VBTs > > with bdb > > version >= 226 will also be wrong. > > This was reported and bisected by > Aliaksei Urbanski here - > https://bugzilla.kernel.org/show_bug.cgi?id=204183 > > I'll add Bugzilla after the fix is confirmed. > Oohh it makes sense, thanks for spotting it. And 2 users reported that it works: https://bugzilla.kernel.org/show_bug.cgi?id=204183 https://bugs.freedesktop.org/show_bug.cgi?id=111088 Can you also CC kernel stable so the fix is cherry-picked? Reviewed-by: José Roberto de Souza <jose.souza@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: José Roberto de Souza <jose.souza@intel.com> > Fixes: 88a0d9606aff ("drm/i915/vbt: Parse and use the new field with > PSR2 TP2/3 wakeup time")z > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/display/intel_bios.c | 2 +- > drivers/gpu/drm/i915/display/intel_vbt_defs.h | 6 +++--- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c > b/drivers/gpu/drm/i915/display/intel_bios.c > index 21501d565327..b416b394b641 100644 > --- a/drivers/gpu/drm/i915/display/intel_bios.c > +++ b/drivers/gpu/drm/i915/display/intel_bios.c > @@ -766,7 +766,7 @@ parse_psr(struct drm_i915_private *dev_priv, > const struct bdb_header *bdb) > } > > if (bdb->version >= 226) { > - u32 wakeup_time = psr_table->psr2_tp2_tp3_wakeup_time; > + u32 wakeup_time = psr->psr2_tp2_tp3_wakeup_time; > > wakeup_time = (wakeup_time >> (2 * panel_type)) & 0x3; > switch (wakeup_time) { > diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h > b/drivers/gpu/drm/i915/display/intel_vbt_defs.h > index 93f5c9d204d6..09cd37fb0b1c 100644 > --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h > +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h > @@ -481,13 +481,13 @@ struct psr_table { > /* TP wake up time in multiple of 100 */ > u16 tp1_wakeup_time; > u16 tp2_tp3_wakeup_time; > - > - /* PSR2 TP2/TP3 wakeup time for 16 panels */ > - u32 psr2_tp2_tp3_wakeup_time; > } __packed; > > struct bdb_psr { > struct psr_table psr_table[16]; > + > + /* PSR2 TP2/TP3 wakeup time for 16 panels */ > + u32 psr2_tp2_tp3_wakeup_time; > } __packed; > > /*
On Wed, Jul 17, 2019 at 09:41:13AM -0700, Souza, Jose wrote: > On Tue, 2019-07-16 at 15:10 -0700, Pandiyan, Dhinakaran wrote: > > On Tue, 2019-07-16 at 15:03 -0700, Dhinakaran Pandiyan wrote: > > > A single 32-bit PSR2 training pattern field follows the sixteen > > > element > > > array of PSR table entries in the VBT spec. But, we incorrectly > > > define > > > this PSR2 field for each of the PSR table entries. As a result, the > > > PSR1 > > > training pattern duration for any panel_type != 0 will be parsed > > > incorrectly. Secondly, PSR2 training pattern durations for VBTs > > > with bdb > > > version >= 226 will also be wrong. > > > > This was reported and bisected by > > Aliaksei Urbanski here - > > https://bugzilla.kernel.org/show_bug.cgi?id=204183 > > > > I'll add Bugzilla after the fix is confirmed. > > > > Oohh it makes sense, thanks for spotting it. > > And 2 users reported that it works: > https://bugzilla.kernel.org/show_bug.cgi?id=204183 > https://bugs.freedesktop.org/show_bug.cgi?id=111088 Please add this before merging: Reference: https://bugzilla.kernel.org/show_bug.cgi?id=204183 Reference: https://bugs.freedesktop.org/show_bug.cgi?id=111088 > > Can you also CC kernel stable so the fix is cherry-picked? and this: Cc: stable@vger.kernel.org #v5.2 > > Reviewed-by: José Roberto de Souza <jose.souza@intel.com> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Fixes: 88a0d9606aff ("drm/i915/vbt: Parse and use the new field with > > PSR2 TP2/3 wakeup time")z > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_bios.c | 2 +- > > drivers/gpu/drm/i915/display/intel_vbt_defs.h | 6 +++--- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c > > b/drivers/gpu/drm/i915/display/intel_bios.c > > index 21501d565327..b416b394b641 100644 > > --- a/drivers/gpu/drm/i915/display/intel_bios.c > > +++ b/drivers/gpu/drm/i915/display/intel_bios.c > > @@ -766,7 +766,7 @@ parse_psr(struct drm_i915_private *dev_priv, > > const struct bdb_header *bdb) > > } > > > > if (bdb->version >= 226) { > > - u32 wakeup_time = psr_table->psr2_tp2_tp3_wakeup_time; > > + u32 wakeup_time = psr->psr2_tp2_tp3_wakeup_time; > > > > wakeup_time = (wakeup_time >> (2 * panel_type)) & 0x3; > > switch (wakeup_time) { > > diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h > > b/drivers/gpu/drm/i915/display/intel_vbt_defs.h > > index 93f5c9d204d6..09cd37fb0b1c 100644 > > --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h > > +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h > > @@ -481,13 +481,13 @@ struct psr_table { > > /* TP wake up time in multiple of 100 */ > > u16 tp1_wakeup_time; > > u16 tp2_tp3_wakeup_time; > > - > > - /* PSR2 TP2/TP3 wakeup time for 16 panels */ > > - u32 psr2_tp2_tp3_wakeup_time; > > } __packed; > > > > struct bdb_psr { > > struct psr_table psr_table[16]; > > + > > + /* PSR2 TP2/TP3 wakeup time for 16 panels */ > > + u32 psr2_tp2_tp3_wakeup_time; > > } __packed; > > > > /*
Tested-by: François Guerraz <kubrick@fgv6.net>
Tested-by: François Guerraz <kubrick@fgv6.net>
On Dell XPS 9350
Tested-by: François Guerraz <kubrick@fgv6.net>
On Dell XPS 9350
On Wed, 2019-07-17 at 14:35 +0300, Ville Syrjälä wrote: > On Tue, Jul 16, 2019 at 03:03:21PM -0700, Dhinakaran Pandiyan wrote: > > A single 32-bit PSR2 training pattern field follows the sixteen element > > array of PSR table entries in the VBT spec. But, we incorrectly define > > this PSR2 field for each of the PSR table entries. As a result, the PSR1 > > training pattern duration for any panel_type != 0 will be parsed > > incorrectly. Secondly, PSR2 training pattern durations for VBTs with bdb > > version >= 226 will also be wrong. > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Fixes: 88a0d9606aff ("drm/i915/vbt: Parse and use the new field with PSR2 TP2/3 wakeup time") > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Please sync the igt copy after landing this. Will do, thanks! -DK > > > --- > > drivers/gpu/drm/i915/display/intel_bios.c | 2 +- > > drivers/gpu/drm/i915/display/intel_vbt_defs.h | 6 +++--- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c > > b/drivers/gpu/drm/i915/display/intel_bios.c > > index 21501d565327..b416b394b641 100644 > > --- a/drivers/gpu/drm/i915/display/intel_bios.c > > +++ b/drivers/gpu/drm/i915/display/intel_bios.c > > @@ -766,7 +766,7 @@ parse_psr(struct drm_i915_private *dev_priv, const struct bdb_header *bdb) > > } > > > > if (bdb->version >= 226) { > > - u32 wakeup_time = psr_table->psr2_tp2_tp3_wakeup_time; > > + u32 wakeup_time = psr->psr2_tp2_tp3_wakeup_time; > > > > wakeup_time = (wakeup_time >> (2 * panel_type)) & 0x3; > > switch (wakeup_time) { > > diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h > > b/drivers/gpu/drm/i915/display/intel_vbt_defs.h > > index 93f5c9d204d6..09cd37fb0b1c 100644 > > --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h > > +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h > > @@ -481,13 +481,13 @@ struct psr_table { > > /* TP wake up time in multiple of 100 */ > > u16 tp1_wakeup_time; > > u16 tp2_tp3_wakeup_time; > > - > > - /* PSR2 TP2/TP3 wakeup time for 16 panels */ > > - u32 psr2_tp2_tp3_wakeup_time; > > } __packed; > > > > struct bdb_psr { > > struct psr_table psr_table[16]; > > + > > + /* PSR2 TP2/TP3 wakeup time for 16 panels */ > > + u32 psr2_tp2_tp3_wakeup_time; > > } __packed; > > > > /* > > -- > > 2.17.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c index 21501d565327..b416b394b641 100644 --- a/drivers/gpu/drm/i915/display/intel_bios.c +++ b/drivers/gpu/drm/i915/display/intel_bios.c @@ -766,7 +766,7 @@ parse_psr(struct drm_i915_private *dev_priv, const struct bdb_header *bdb) } if (bdb->version >= 226) { - u32 wakeup_time = psr_table->psr2_tp2_tp3_wakeup_time; + u32 wakeup_time = psr->psr2_tp2_tp3_wakeup_time; wakeup_time = (wakeup_time >> (2 * panel_type)) & 0x3; switch (wakeup_time) { diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h index 93f5c9d204d6..09cd37fb0b1c 100644 --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h @@ -481,13 +481,13 @@ struct psr_table { /* TP wake up time in multiple of 100 */ u16 tp1_wakeup_time; u16 tp2_tp3_wakeup_time; - - /* PSR2 TP2/TP3 wakeup time for 16 panels */ - u32 psr2_tp2_tp3_wakeup_time; } __packed; struct bdb_psr { struct psr_table psr_table[16]; + + /* PSR2 TP2/TP3 wakeup time for 16 panels */ + u32 psr2_tp2_tp3_wakeup_time; } __packed; /*
A single 32-bit PSR2 training pattern field follows the sixteen element array of PSR table entries in the VBT spec. But, we incorrectly define this PSR2 field for each of the PSR table entries. As a result, the PSR1 training pattern duration for any panel_type != 0 will be parsed incorrectly. Secondly, PSR2 training pattern durations for VBTs with bdb version >= 226 will also be wrong. Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: José Roberto de Souza <jose.souza@intel.com> Fixes: 88a0d9606aff ("drm/i915/vbt: Parse and use the new field with PSR2 TP2/3 wakeup time") Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/display/intel_bios.c | 2 +- drivers/gpu/drm/i915/display/intel_vbt_defs.h | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-)