drm/i915/vbt: Fix VBT parsing for the PSR section

Submitted by Pandiyan, Dhinakaran on July 17, 2019, 10:34 p.m.

Details

Message ID 20190717223451.2595-1-dhinakaran.pandiyan@intel.com
State New
Headers show
Series "drm/i915/vbt: Fix VBT parsing for the PSR section" ( rev: 3 ) in Intel GFX

Browsing this patch as part of:
"drm/i915/vbt: Fix VBT parsing for the PSR section" rev 3 in Intel GFX
<< prev patch [1/1] next patch >>

Commit Message

Pandiyan, Dhinakaran July 17, 2019, 10:34 p.m.
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>
Cc: stable@vger.kernel.org
Cc: stable@vger.kernel.org #v5.2
Fixes: 88a0d9606aff ("drm/i915/vbt: Parse and use the new field with PSR2 TP2/3 wakeup time")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111088
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204183
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Tested-by: François Guerraz <kubrick@fgv6.net>
---
 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(-)

Patch hide | download patch | download mbox

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;
 
 /*

Comments

On Wed, Jul 17, 2019 at 03:34:51PM -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>
> Cc: stable@vger.kernel.org
> Cc: stable@vger.kernel.org #v5.2
> Fixes: 88a0d9606aff ("drm/i915/vbt: Parse and use the new field with PSR2 TP2/3 wakeup time")
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111088
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204183
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Tested-by: François Guerraz <kubrick@fgv6.net>

pushed, thanks for the patches, reviews and tests.

> ---
>  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
Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 88a0d9606aff drm/i915/vbt: Parse and use the new field with PSR2 TP2/3 wakeup time.

The bot has tested the following trees: v5.2.1.

v5.2.1: Failed to apply! Possible dependencies:
    231dcffc234f ("drm/i915/bios: add BDB block comments before definitions")
    843444ed1301 ("drm/i915/bios: sort BDB block definitions using block ID")
    f87f6599c843 ("drm/i915/bios: reserve struct bdb_ prefix for BDB blocks")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

--
Thanks,
Sasha
Hi Sasha,

On Thu, Jul 18, 2019 at 5:45 PM Sasha Levin <sashal@kernel.org> wrote:
>
> Hi,
>
> [This is an automated email]

Where did you get this patch from? Since stable needs patches merged
on Linus tree,
shouldn't your scripts run to try backporting only patches from there?

Thanks,
Rodrigo.

>
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: 88a0d9606aff drm/i915/vbt: Parse and use the new field with PSR2 TP2/3 wakeup time.
>
> The bot has tested the following trees: v5.2.1.
> v5.2.1: Failed to apply! Possible dependencies:
>     231dcffc234f ("drm/i915/bios: add BDB block comments before definitions")
>     843444ed1301 ("drm/i915/bios: sort BDB block definitions using block ID")
>     f87f6599c843 ("drm/i915/bios: reserve struct bdb_ prefix for BDB blocks")
>
>
> NOTE: The patch will not be queued to stable trees until it is upstream.
>
> How should we proceed with this patch?
>
> --
> Thanks,
> Sasha
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Jul 30, 2019 at 01:42:45PM -0700, Rodrigo Vivi wrote:
>Hi Sasha,

Hello!

>On Thu, Jul 18, 2019 at 5:45 PM Sasha Levin <sashal@kernel.org> wrote:
>>
>> Hi,
>>
>> [This is an automated email]
>
>Where did you get this patch from? Since stable needs patches merged

This bot grabs them from various mailing lists.

>on Linus tree,
>shouldn't your scripts run to try backporting only patches from there?

There's a note a few lines down that says:

    "NOTE: The patch will not be queued to stable trees until it is upstream."

Otherwise, no, there's no rule that says we can't look at patches before
they are upstream. We can't queue them up, but we sure can poke them.

The reasoning behind this is that it's easier to get replies (and
backports) from folks who are actively working on the patch now, rather
than a few weeks later when Greg sends his "FAILED:" mails and gets
ignored because said folks have moved on.

--
Thanks,
Sasha
> On Jul 30, 2019, at 2:48 PM, Sasha Levin <sashal@kernel.org> wrote:
> 
> On Tue, Jul 30, 2019 at 01:42:45PM -0700, Rodrigo Vivi wrote:
>> Hi Sasha,
> 
> Hello!
> 
>> On Thu, Jul 18, 2019 at 5:45 PM Sasha Levin <sashal@kernel.org> wrote:
>>> 
>>> Hi,
>>> 
>>> [This is an automated email]
>> 
>> Where did you get this patch from? Since stable needs patches merged
> 
> This bot grabs them from various mailing lists.
> 
>> on Linus tree,
>> shouldn't your scripts run to try backporting only patches from there?
> 
> There's a note a few lines down that says:
> 
>   "NOTE: The patch will not be queued to stable trees until it is upstream."
> 
> Otherwise, no, there's no rule that says we can't look at patches before
> they are upstream. We can't queue them up, but we sure can poke them.
> 
> The reasoning behind this is that it's easier to get replies (and
> backports) from folks who are actively working on the patch now,


This is a very good reason indeed...

> rather
> than a few weeks later when Greg sends his "FAILED:" mails and gets
> ignored because said folks have moved on.

however this could potentially cause extra work and confusion like we can see on this
thread where the developer immediately responded to your email and sent the
backported patch to the stable mailing list.

Maybe it is just because we are used to Greg's failed to apply email or maybe
it was just a matter of education... 

But I wonder if there isn't something that could be improved on the automated
message here. Some message clearly stating:

- No action required at this point
- you can work to prepare the backport in advance
-  don't send it to stable before requested by Greg

Anyway, just few ideas. I just reached you to understand the flow and I'm already
happy to understand what happened here.

Thanks a lot for that,
Rodrigo.


> 
> --
> Thanks,
> Sasha
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jul 31, 2019 at 05:14:38PM +0000, Vivi, Rodrigo wrote:
>> On Jul 30, 2019, at 2:48 PM, Sasha Levin <sashal@kernel.org> wrote:
>> rather
>> than a few weeks later when Greg sends his "FAILED:" mails and gets
>> ignored because said folks have moved on.
>
>however this could potentially cause extra work and confusion like we can see on this
>thread where the developer immediately responded to your email and sent the
>backported patch to the stable mailing list.
>
>Maybe it is just because we are used to Greg's failed to apply email or maybe
>it was just a matter of education...

I think that there were a few things here that ended up causing
confusion, but I'm not quite sure how to address them.

I think that stable should have a clearer rules as to how backports
should be sent. Right now we weed through mails to stable@ to figure out
what are backport requests, what are upstream patches, and what are just
confused folks.

We have gotten pretty good at this, but still not perfect...

>But I wonder if there isn't something that could be improved on the automated
>message here. Some message clearly stating:
>
>- No action required at this point

One *could* send a backport at this point. My understanding is that when
Greg sees a failure to apply a commit tagged for stable he'll grep
through his mailbox, hopefully finding the backport as a result of this
bot bugging people.

>- you can work to prepare the backport in advance
>-  don't send it to stable before requested by Greg

Why not? I think it's fine to put it on the mailing list, specially
under the same thread, and let us deal with it after the patch goes
upstream.

--
Thanks,
Sasha