| Message ID | 20190717223451.2595-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 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
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> 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(-)