* [PATCH v1] drm/i915/bdb: Fix version check
@ 2021-09-20 14:11 ` Lukasz Majczak
0 siblings, 0 replies; 14+ messages in thread
From: Lukasz Majczak @ 2021-09-20 14:11 UTC (permalink / raw)
To: Jose Roberto de Souza, Shawn C Lee
Cc: Jani Nikula, Joonas Lahtinen, intel-gfx, dri-devel, upstream,
Lukasz Majczak, stable
With patch "drm/i915/vbt: Fix backlight parsing for VBT 234+"
the size of bdb_lfp_backlight_data structure has been increased,
causing if-statement in the parse_lfp_backlight function
that comapres this structure size to the one retrieved from BDB,
always to fail for older revisions.
This patch fixes it by comparing a total size of all fileds from
the structure (present before the change) with the value gathered from BDB.
Tested on Chromebook Pixelbook (Nocturne) (reports bdb->version = 221)
Cc: <stable@vger.kernel.org> # 5.4+
Tested-by: Lukasz Majczak <lma@semihalf.com>
Signed-off-by: Lukasz Majczak <lma@semihalf.com>
---
drivers/gpu/drm/i915/display/intel_bios.c | 4 +++-
drivers/gpu/drm/i915/display/intel_vbt_defs.h | 5 +++++
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
index 3c25926092de..052a19b455d1 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -452,7 +452,9 @@ parse_lfp_backlight(struct drm_i915_private *i915,
i915->vbt.backlight.type = INTEL_BACKLIGHT_DISPLAY_DDI;
if (bdb->version >= 191 &&
- get_blocksize(backlight_data) >= sizeof(*backlight_data)) {
+ get_blocksize(backlight_data) >= (sizeof(backlight_data->entry_size) +
+ sizeof(backlight_data->data) +
+ sizeof(backlight_data->level))) {
const struct lfp_backlight_control_method *method;
method = &backlight_data->backlight_control[panel_type];
diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
index 330077c2e588..fff456bf8783 100644
--- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
+++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
@@ -814,6 +814,11 @@ struct lfp_brightness_level {
u16 reserved;
} __packed;
+/*
+ * Changing struct bdb_lfp_backlight_data might affect its
+ * size comparation to the value hold in BDB.
+ * (e.g. in parse_lfp_backlight())
+ */
struct bdb_lfp_backlight_data {
u8 entry_size;
struct lfp_backlight_data_entry data[16];
--
2.33.0.464.g1972c5931b-goog
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Intel-gfx] [PATCH v1] drm/i915/bdb: Fix version check
@ 2021-09-20 14:11 ` Lukasz Majczak
0 siblings, 0 replies; 14+ messages in thread
From: Lukasz Majczak @ 2021-09-20 14:11 UTC (permalink / raw)
To: Jose Roberto de Souza, Shawn C Lee
Cc: Jani Nikula, Joonas Lahtinen, intel-gfx, dri-devel, upstream,
Lukasz Majczak, stable
With patch "drm/i915/vbt: Fix backlight parsing for VBT 234+"
the size of bdb_lfp_backlight_data structure has been increased,
causing if-statement in the parse_lfp_backlight function
that comapres this structure size to the one retrieved from BDB,
always to fail for older revisions.
This patch fixes it by comparing a total size of all fileds from
the structure (present before the change) with the value gathered from BDB.
Tested on Chromebook Pixelbook (Nocturne) (reports bdb->version = 221)
Cc: <stable@vger.kernel.org> # 5.4+
Tested-by: Lukasz Majczak <lma@semihalf.com>
Signed-off-by: Lukasz Majczak <lma@semihalf.com>
---
drivers/gpu/drm/i915/display/intel_bios.c | 4 +++-
drivers/gpu/drm/i915/display/intel_vbt_defs.h | 5 +++++
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
index 3c25926092de..052a19b455d1 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -452,7 +452,9 @@ parse_lfp_backlight(struct drm_i915_private *i915,
i915->vbt.backlight.type = INTEL_BACKLIGHT_DISPLAY_DDI;
if (bdb->version >= 191 &&
- get_blocksize(backlight_data) >= sizeof(*backlight_data)) {
+ get_blocksize(backlight_data) >= (sizeof(backlight_data->entry_size) +
+ sizeof(backlight_data->data) +
+ sizeof(backlight_data->level))) {
const struct lfp_backlight_control_method *method;
method = &backlight_data->backlight_control[panel_type];
diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
index 330077c2e588..fff456bf8783 100644
--- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
+++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
@@ -814,6 +814,11 @@ struct lfp_brightness_level {
u16 reserved;
} __packed;
+/*
+ * Changing struct bdb_lfp_backlight_data might affect its
+ * size comparation to the value hold in BDB.
+ * (e.g. in parse_lfp_backlight())
+ */
struct bdb_lfp_backlight_data {
u8 entry_size;
struct lfp_backlight_data_entry data[16];
--
2.33.0.464.g1972c5931b-goog
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v1] drm/i915/bdb: Fix version check
2021-09-20 14:11 ` [Intel-gfx] " Lukasz Majczak
@ 2021-09-20 16:55 ` Jani Nikula
-1 siblings, 0 replies; 14+ messages in thread
From: Jani Nikula @ 2021-09-20 16:55 UTC (permalink / raw)
To: Lukasz Majczak, Jose Roberto de Souza, Shawn C Lee
Cc: Joonas Lahtinen, intel-gfx, dri-devel, upstream, Lukasz Majczak, stable
On Mon, 20 Sep 2021, Lukasz Majczak <lma@semihalf.com> wrote:
> With patch "drm/i915/vbt: Fix backlight parsing for VBT 234+"
> the size of bdb_lfp_backlight_data structure has been increased,
> causing if-statement in the parse_lfp_backlight function
> that comapres this structure size to the one retrieved from BDB,
> always to fail for older revisions.
> This patch fixes it by comparing a total size of all fileds from
> the structure (present before the change) with the value gathered from BDB.
> Tested on Chromebook Pixelbook (Nocturne) (reports bdb->version = 221)
>
> Cc: <stable@vger.kernel.org> # 5.4+
Not a review of the patch, I'll leave that to José, but 5.4 is not
right.
The commit is d381baad29b4 ("drm/i915/vbt: Fix backlight parsing for VBT
234+") which was merged in v5.11 and AFAICT has not been backported to
any stable kernel.
So this would be:
Fixes: d381baad29b4 ("drm/i915/vbt: Fix backlight parsing for VBT 234+")
Cc: <stable@vger.kernel.org> # v5.11+
BR,
Jani.
> Tested-by: Lukasz Majczak <lma@semihalf.com>
> Signed-off-by: Lukasz Majczak <lma@semihalf.com>
> ---
> drivers/gpu/drm/i915/display/intel_bios.c | 4 +++-
> drivers/gpu/drm/i915/display/intel_vbt_defs.h | 5 +++++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index 3c25926092de..052a19b455d1 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -452,7 +452,9 @@ parse_lfp_backlight(struct drm_i915_private *i915,
>
> i915->vbt.backlight.type = INTEL_BACKLIGHT_DISPLAY_DDI;
> if (bdb->version >= 191 &&
> - get_blocksize(backlight_data) >= sizeof(*backlight_data)) {
> + get_blocksize(backlight_data) >= (sizeof(backlight_data->entry_size) +
> + sizeof(backlight_data->data) +
> + sizeof(backlight_data->level))) {
> const struct lfp_backlight_control_method *method;
>
> method = &backlight_data->backlight_control[panel_type];
> diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> index 330077c2e588..fff456bf8783 100644
> --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> @@ -814,6 +814,11 @@ struct lfp_brightness_level {
> u16 reserved;
> } __packed;
>
> +/*
> + * Changing struct bdb_lfp_backlight_data might affect its
> + * size comparation to the value hold in BDB.
> + * (e.g. in parse_lfp_backlight())
> + */
> struct bdb_lfp_backlight_data {
> u8 entry_size;
> struct lfp_backlight_data_entry data[16];
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-gfx] [PATCH v1] drm/i915/bdb: Fix version check
@ 2021-09-20 16:55 ` Jani Nikula
0 siblings, 0 replies; 14+ messages in thread
From: Jani Nikula @ 2021-09-20 16:55 UTC (permalink / raw)
To: Lukasz Majczak, Jose Roberto de Souza, Shawn C Lee
Cc: Joonas Lahtinen, intel-gfx, dri-devel, upstream, Lukasz Majczak, stable
On Mon, 20 Sep 2021, Lukasz Majczak <lma@semihalf.com> wrote:
> With patch "drm/i915/vbt: Fix backlight parsing for VBT 234+"
> the size of bdb_lfp_backlight_data structure has been increased,
> causing if-statement in the parse_lfp_backlight function
> that comapres this structure size to the one retrieved from BDB,
> always to fail for older revisions.
> This patch fixes it by comparing a total size of all fileds from
> the structure (present before the change) with the value gathered from BDB.
> Tested on Chromebook Pixelbook (Nocturne) (reports bdb->version = 221)
>
> Cc: <stable@vger.kernel.org> # 5.4+
Not a review of the patch, I'll leave that to José, but 5.4 is not
right.
The commit is d381baad29b4 ("drm/i915/vbt: Fix backlight parsing for VBT
234+") which was merged in v5.11 and AFAICT has not been backported to
any stable kernel.
So this would be:
Fixes: d381baad29b4 ("drm/i915/vbt: Fix backlight parsing for VBT 234+")
Cc: <stable@vger.kernel.org> # v5.11+
BR,
Jani.
> Tested-by: Lukasz Majczak <lma@semihalf.com>
> Signed-off-by: Lukasz Majczak <lma@semihalf.com>
> ---
> drivers/gpu/drm/i915/display/intel_bios.c | 4 +++-
> drivers/gpu/drm/i915/display/intel_vbt_defs.h | 5 +++++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index 3c25926092de..052a19b455d1 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -452,7 +452,9 @@ parse_lfp_backlight(struct drm_i915_private *i915,
>
> i915->vbt.backlight.type = INTEL_BACKLIGHT_DISPLAY_DDI;
> if (bdb->version >= 191 &&
> - get_blocksize(backlight_data) >= sizeof(*backlight_data)) {
> + get_blocksize(backlight_data) >= (sizeof(backlight_data->entry_size) +
> + sizeof(backlight_data->data) +
> + sizeof(backlight_data->level))) {
> const struct lfp_backlight_control_method *method;
>
> method = &backlight_data->backlight_control[panel_type];
> diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> index 330077c2e588..fff456bf8783 100644
> --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> @@ -814,6 +814,11 @@ struct lfp_brightness_level {
> u16 reserved;
> } __packed;
>
> +/*
> + * Changing struct bdb_lfp_backlight_data might affect its
> + * size comparation to the value hold in BDB.
> + * (e.g. in parse_lfp_backlight())
> + */
> struct bdb_lfp_backlight_data {
> u8 entry_size;
> struct lfp_backlight_data_entry data[16];
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] drm/i915/bdb: Fix version check
2021-09-20 14:11 ` [Intel-gfx] " Lukasz Majczak
@ 2021-09-20 20:47 ` Souza, Jose
-1 siblings, 0 replies; 14+ messages in thread
From: Souza, Jose @ 2021-09-20 20:47 UTC (permalink / raw)
To: Lee, Shawn C, lma
Cc: dri-devel, joonas.lahtinen, stable, intel-gfx, jani.nikula, upstream
On Mon, 2021-09-20 at 16:11 +0200, Lukasz Majczak wrote:
> With patch "drm/i915/vbt: Fix backlight parsing for VBT 234+"
> the size of bdb_lfp_backlight_data structure has been increased,
> causing if-statement in the parse_lfp_backlight function
> that comapres this structure size to the one retrieved from BDB,
> always to fail for older revisions.
> This patch fixes it by comparing a total size of all fileds from
> the structure (present before the change) with the value gathered from BDB.
> Tested on Chromebook Pixelbook (Nocturne) (reports bdb->version = 221)
>
> Cc: <stable@vger.kernel.org> # 5.4+
> Tested-by: Lukasz Majczak <lma@semihalf.com>
> Signed-off-by: Lukasz Majczak <lma@semihalf.com>
> ---
> drivers/gpu/drm/i915/display/intel_bios.c | 4 +++-
> drivers/gpu/drm/i915/display/intel_vbt_defs.h | 5 +++++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index 3c25926092de..052a19b455d1 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -452,7 +452,9 @@ parse_lfp_backlight(struct drm_i915_private *i915,
>
> i915->vbt.backlight.type = INTEL_BACKLIGHT_DISPLAY_DDI;
> if (bdb->version >= 191 &&
> - get_blocksize(backlight_data) >= sizeof(*backlight_data)) {
> + get_blocksize(backlight_data) >= (sizeof(backlight_data->entry_size) +
> + sizeof(backlight_data->data) +
> + sizeof(backlight_data->level))) {
Missing sizeof(backlight_data->backlight_control) but this is getting very verbose.
Would be better have a expected size variable set each version set in the beginning of this function.
something like:
switch (bdb->version) {
case 191:
expected_size = x;
break;
case 234:
expected_size = x;
break;
case 236:
default:
expected_size = x;
}
> const struct lfp_backlight_control_method *method;
>
> method = &backlight_data->backlight_control[panel_type];
> diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> index 330077c2e588..fff456bf8783 100644
> --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> @@ -814,6 +814,11 @@ struct lfp_brightness_level {
> u16 reserved;
> } __packed;
>
> +/*
> + * Changing struct bdb_lfp_backlight_data might affect its
> + * size comparation to the value hold in BDB.
> + * (e.g. in parse_lfp_backlight())
> + */
This is true for all the blocks so I don't think we need this comment.
> struct bdb_lfp_backlight_data {
> u8 entry_size;
> struct lfp_backlight_data_entry data[16];
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-gfx] [PATCH v1] drm/i915/bdb: Fix version check
@ 2021-09-20 20:47 ` Souza, Jose
0 siblings, 0 replies; 14+ messages in thread
From: Souza, Jose @ 2021-09-20 20:47 UTC (permalink / raw)
To: Lee, Shawn C, lma
Cc: dri-devel, joonas.lahtinen, stable, intel-gfx, jani.nikula, upstream
On Mon, 2021-09-20 at 16:11 +0200, Lukasz Majczak wrote:
> With patch "drm/i915/vbt: Fix backlight parsing for VBT 234+"
> the size of bdb_lfp_backlight_data structure has been increased,
> causing if-statement in the parse_lfp_backlight function
> that comapres this structure size to the one retrieved from BDB,
> always to fail for older revisions.
> This patch fixes it by comparing a total size of all fileds from
> the structure (present before the change) with the value gathered from BDB.
> Tested on Chromebook Pixelbook (Nocturne) (reports bdb->version = 221)
>
> Cc: <stable@vger.kernel.org> # 5.4+
> Tested-by: Lukasz Majczak <lma@semihalf.com>
> Signed-off-by: Lukasz Majczak <lma@semihalf.com>
> ---
> drivers/gpu/drm/i915/display/intel_bios.c | 4 +++-
> drivers/gpu/drm/i915/display/intel_vbt_defs.h | 5 +++++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index 3c25926092de..052a19b455d1 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -452,7 +452,9 @@ parse_lfp_backlight(struct drm_i915_private *i915,
>
> i915->vbt.backlight.type = INTEL_BACKLIGHT_DISPLAY_DDI;
> if (bdb->version >= 191 &&
> - get_blocksize(backlight_data) >= sizeof(*backlight_data)) {
> + get_blocksize(backlight_data) >= (sizeof(backlight_data->entry_size) +
> + sizeof(backlight_data->data) +
> + sizeof(backlight_data->level))) {
Missing sizeof(backlight_data->backlight_control) but this is getting very verbose.
Would be better have a expected size variable set each version set in the beginning of this function.
something like:
switch (bdb->version) {
case 191:
expected_size = x;
break;
case 234:
expected_size = x;
break;
case 236:
default:
expected_size = x;
}
> const struct lfp_backlight_control_method *method;
>
> method = &backlight_data->backlight_control[panel_type];
> diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> index 330077c2e588..fff456bf8783 100644
> --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> @@ -814,6 +814,11 @@ struct lfp_brightness_level {
> u16 reserved;
> } __packed;
>
> +/*
> + * Changing struct bdb_lfp_backlight_data might affect its
> + * size comparation to the value hold in BDB.
> + * (e.g. in parse_lfp_backlight())
> + */
This is true for all the blocks so I don't think we need this comment.
> struct bdb_lfp_backlight_data {
> u8 entry_size;
> struct lfp_backlight_data_entry data[16];
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/bdb: Fix version check
2021-09-20 14:11 ` [Intel-gfx] " Lukasz Majczak
` (2 preceding siblings ...)
(?)
@ 2021-09-20 21:57 ` Patchwork
-1 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2021-09-20 21:57 UTC (permalink / raw)
To: Lukasz Majczak; +Cc: intel-gfx
[-- Attachment #1: Type: text/plain, Size: 13928 bytes --]
== Series Details ==
Series: drm/i915/bdb: Fix version check
URL : https://patchwork.freedesktop.org/series/94871/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_10614 -> Patchwork_21101
====================================================
Summary
-------
**FAILURE**
Serious unknown changes coming with Patchwork_21101 absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_21101, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/index.html
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_21101:
### IGT changes ###
#### Possible regressions ####
* igt@i915_module_load@reload:
- fi-ilk-650: [PASS][1] -> [INCOMPLETE][2]
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10614/fi-ilk-650/igt@i915_module_load@reload.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-ilk-650/igt@i915_module_load@reload.html
- fi-ivb-3770: [PASS][3] -> [INCOMPLETE][4]
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10614/fi-ivb-3770/igt@i915_module_load@reload.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-ivb-3770/igt@i915_module_load@reload.html
- fi-bsw-nick: [PASS][5] -> [INCOMPLETE][6]
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10614/fi-bsw-nick/igt@i915_module_load@reload.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-bsw-nick/igt@i915_module_load@reload.html
#### Suppressed ####
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* igt@i915_module_load@reload:
- {fi-jsl-1}: [INCOMPLETE][7] ([i915#4136]) -> [INCOMPLETE][8]
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10614/fi-jsl-1/igt@i915_module_load@reload.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-jsl-1/igt@i915_module_load@reload.html
Known issues
------------
Here are the changes found in Patchwork_21101 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@amdgpu/amd_basic@cs-sdma:
- fi-cfl-8109u: NOTRUN -> [SKIP][9] ([fdo#109271]) +17 similar issues
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-cfl-8109u/igt@amdgpu/amd_basic@cs-sdma.html
* igt@amdgpu/amd_cs_nop@sync-fork-compute0:
- fi-snb-2600: NOTRUN -> [SKIP][10] ([fdo#109271]) +17 similar issues
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-snb-2600/igt@amdgpu/amd_cs_nop@sync-fork-compute0.html
* igt@amdgpu/amd_cs_nop@sync-fork-gfx0:
- fi-cfl-8700k: NOTRUN -> [SKIP][11] ([fdo#109271]) +17 similar issues
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-cfl-8700k/igt@amdgpu/amd_cs_nop@sync-fork-gfx0.html
* igt@amdgpu/amd_prime@i915-to-amd:
- fi-snb-2520m: NOTRUN -> [SKIP][12] ([fdo#109271]) +18 similar issues
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-snb-2520m/igt@amdgpu/amd_prime@i915-to-amd.html
* igt@core_hotunplug@unbind-rebind:
- fi-rkl-guc: [PASS][13] -> [INCOMPLETE][14] ([i915#4130])
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10614/fi-rkl-guc/igt@core_hotunplug@unbind-rebind.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-rkl-guc/igt@core_hotunplug@unbind-rebind.html
- fi-tgl-1115g4: NOTRUN -> [INCOMPLETE][15] ([i915#4130])
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-tgl-1115g4/igt@core_hotunplug@unbind-rebind.html
- fi-icl-u2: [PASS][16] -> [INCOMPLETE][17] ([i915#4130])
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10614/fi-icl-u2/igt@core_hotunplug@unbind-rebind.html
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-icl-u2/igt@core_hotunplug@unbind-rebind.html
* igt@gem_exec_suspend@basic-s3:
- fi-skl-6600u: [PASS][18] -> [INCOMPLETE][19] ([i915#146] / [i915#198])
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10614/fi-skl-6600u/igt@gem_exec_suspend@basic-s3.html
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-skl-6600u/igt@gem_exec_suspend@basic-s3.html
* igt@gem_huc_copy@huc-copy:
- fi-tgl-1115g4: NOTRUN -> [SKIP][20] ([i915#2190])
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-tgl-1115g4/igt@gem_huc_copy@huc-copy.html
* igt@i915_module_load@reload:
- fi-bxt-dsi: [PASS][21] -> [INCOMPLETE][22] ([i915#4130])
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10614/fi-bxt-dsi/igt@i915_module_load@reload.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-bxt-dsi/igt@i915_module_load@reload.html
- fi-icl-y: [PASS][23] -> [INCOMPLETE][24] ([i915#4130])
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10614/fi-icl-y/igt@i915_module_load@reload.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-icl-y/igt@i915_module_load@reload.html
- fi-kbl-7500u: NOTRUN -> [INCOMPLETE][25] ([i915#4130])
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-kbl-7500u/igt@i915_module_load@reload.html
* igt@i915_pm_backlight@basic-brightness:
- fi-tgl-1115g4: NOTRUN -> [SKIP][26] ([i915#1155])
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-tgl-1115g4/igt@i915_pm_backlight@basic-brightness.html
* igt@kms_chamelium@common-hpd-after-suspend:
- fi-tgl-1115g4: NOTRUN -> [SKIP][27] ([fdo#111827]) +8 similar issues
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-tgl-1115g4/igt@kms_chamelium@common-hpd-after-suspend.html
* igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
- fi-tgl-1115g4: NOTRUN -> [SKIP][28] ([i915#4103]) +1 similar issue
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-tgl-1115g4/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
* igt@kms_force_connector_basic@force-load-detect:
- fi-tgl-1115g4: NOTRUN -> [SKIP][29] ([fdo#109285])
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-tgl-1115g4/igt@kms_force_connector_basic@force-load-detect.html
* igt@kms_psr@primary_mmap_gtt:
- fi-tgl-1115g4: NOTRUN -> [SKIP][30] ([i915#1072]) +3 similar issues
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-tgl-1115g4/igt@kms_psr@primary_mmap_gtt.html
* igt@prime_vgem@basic-userptr:
- fi-tgl-1115g4: NOTRUN -> [SKIP][31] ([i915#3301])
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-tgl-1115g4/igt@prime_vgem@basic-userptr.html
* igt@runner@aborted:
- fi-ilk-650: NOTRUN -> [FAIL][32] ([i915#2426])
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-ilk-650/igt@runner@aborted.html
- fi-glk-dsi: NOTRUN -> [FAIL][33] ([i915#2426] / [i915#3363] / [k.org#202321])
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-glk-dsi/igt@runner@aborted.html
- fi-bsw-nick: NOTRUN -> [FAIL][34] ([i915#3690])
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-bsw-nick/igt@runner@aborted.html
- fi-kbl-7500u: NOTRUN -> [FAIL][35] ([i915#2426] / [i915#3363])
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-kbl-7500u/igt@runner@aborted.html
- fi-cml-u2: NOTRUN -> [FAIL][36] ([i915#2082] / [i915#2426] / [i915#3363])
[36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-cml-u2/igt@runner@aborted.html
- fi-ivb-3770: NOTRUN -> [FAIL][37] ([i915#2426])
[37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-ivb-3770/igt@runner@aborted.html
- fi-bxt-dsi: NOTRUN -> [FAIL][38] ([i915#2426] / [i915#3363])
[38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-bxt-dsi/igt@runner@aborted.html
- fi-tgl-1115g4: NOTRUN -> [FAIL][39] ([i915#1602] / [i915#2722])
[39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-tgl-1115g4/igt@runner@aborted.html
#### Possible fixes ####
* igt@core_hotunplug@unbind-rebind:
- fi-cfl-8700k: [INCOMPLETE][40] ([i915#4130]) -> [PASS][41]
[40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10614/fi-cfl-8700k/igt@core_hotunplug@unbind-rebind.html
[41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-cfl-8700k/igt@core_hotunplug@unbind-rebind.html
- fi-kbl-7500u: [INCOMPLETE][42] ([i915#4130]) -> [PASS][43]
[42]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10614/fi-kbl-7500u/igt@core_hotunplug@unbind-rebind.html
[43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-kbl-7500u/igt@core_hotunplug@unbind-rebind.html
- fi-cfl-8109u: [INCOMPLETE][44] ([i915#4130]) -> [PASS][45]
[44]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10614/fi-cfl-8109u/igt@core_hotunplug@unbind-rebind.html
[45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-cfl-8109u/igt@core_hotunplug@unbind-rebind.html
* igt@i915_module_load@reload:
- fi-kbl-r: [TIMEOUT][46] ([i915#4136]) -> [PASS][47]
[46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10614/fi-kbl-r/igt@i915_module_load@reload.html
[47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-kbl-r/igt@i915_module_load@reload.html
- fi-snb-2520m: [INCOMPLETE][48] -> [PASS][49]
[48]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10614/fi-snb-2520m/igt@i915_module_load@reload.html
[49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-snb-2520m/igt@i915_module_load@reload.html
* igt@i915_selftest@live@hangcheck:
- fi-snb-2600: [INCOMPLETE][50] ([i915#3921]) -> [PASS][51]
[50]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10614/fi-snb-2600/igt@i915_selftest@live@hangcheck.html
[51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-snb-2600/igt@i915_selftest@live@hangcheck.html
* igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
- fi-rkl-guc: [SKIP][52] ([i915#3919]) -> [PASS][53]
[52]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10614/fi-rkl-guc/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html
[53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-rkl-guc/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html
#### Warnings ####
* igt@i915_module_load@reload:
- fi-kbl-8809g: [INCOMPLETE][54] ([i915#4136]) -> [INCOMPLETE][55] ([i915#4130])
[54]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10614/fi-kbl-8809g/igt@i915_module_load@reload.html
[55]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-kbl-8809g/igt@i915_module_load@reload.html
- fi-cml-u2: [INCOMPLETE][56] ([i915#4136]) -> [INCOMPLETE][57] ([i915#4130])
[56]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10614/fi-cml-u2/igt@i915_module_load@reload.html
[57]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-cml-u2/igt@i915_module_load@reload.html
- fi-glk-dsi: [INCOMPLETE][58] -> [INCOMPLETE][59] ([i915#4130])
[58]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10614/fi-glk-dsi/igt@i915_module_load@reload.html
[59]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/fi-glk-dsi/igt@i915_module_load@reload.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
[fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
[i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
[i915#1155]: https://gitlab.freedesktop.org/drm/intel/issues/1155
[i915#146]: https://gitlab.freedesktop.org/drm/intel/issues/146
[i915#1602]: https://gitlab.freedesktop.org/drm/intel/issues/1602
[i915#198]: https://gitlab.freedesktop.org/drm/intel/issues/198
[i915#2082]: https://gitlab.freedesktop.org/drm/intel/issues/2082
[i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
[i915#2426]: https://gitlab.freedesktop.org/drm/intel/issues/2426
[i915#2722]: https://gitlab.freedesktop.org/drm/intel/issues/2722
[i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
[i915#3363]: https://gitlab.freedesktop.org/drm/intel/issues/3363
[i915#3690]: https://gitlab.freedesktop.org/drm/intel/issues/3690
[i915#3919]: https://gitlab.freedesktop.org/drm/intel/issues/3919
[i915#3921]: https://gitlab.freedesktop.org/drm/intel/issues/3921
[i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
[i915#4130]: https://gitlab.freedesktop.org/drm/intel/issues/4130
[i915#4136]: https://gitlab.freedesktop.org/drm/intel/issues/4136
[k.org#202321]: https://bugzilla.kernel.org/show_bug.cgi?id=202321
Participating hosts (37 -> 33)
------------------------------
Additional (1): fi-tgl-1115g4
Missing (5): bat-dg1-6 fi-bsw-cyan fi-ctg-p8600 bat-jsl-1 fi-bdw-samus
Build changes
-------------
* Linux: CI_DRM_10614 -> Patchwork_21101
CI-20190529: 20190529
CI_DRM_10614: 8c455bc1c06a7c99442881dadd10466cd5996bec @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_6213: e9ae59cb8b4f1e7bc61a9261f33fc7e52ae06c65 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_21101: 61b5fba9a321085bcb6823df096429c92c3f9c4e @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
61b5fba9a321 drm/i915/bdb: Fix version check
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21101/index.html
[-- Attachment #2: Type: text/html, Size: 17060 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] drm/i915/bdb: Fix version check
2021-09-20 20:47 ` [Intel-gfx] " Souza, Jose
(?)
@ 2021-09-21 9:01 ` Lukasz Majczak
-1 siblings, 0 replies; 14+ messages in thread
From: Lukasz Majczak @ 2021-09-21 9:01 UTC (permalink / raw)
To: Souza, Jose
Cc: Lee, Shawn C, dri-devel, joonas.lahtinen, stable, intel-gfx,
jani.nikula, upstream
pon., 20 wrz 2021 o 22:47 Souza, Jose <jose.souza@intel.com> napisał(a):
>
> On Mon, 2021-09-20 at 16:11 +0200, Lukasz Majczak wrote:
> > With patch "drm/i915/vbt: Fix backlight parsing for VBT 234+"
> > the size of bdb_lfp_backlight_data structure has been increased,
> > causing if-statement in the parse_lfp_backlight function
> > that comapres this structure size to the one retrieved from BDB,
> > always to fail for older revisions.
> > This patch fixes it by comparing a total size of all fileds from
> > the structure (present before the change) with the value gathered from BDB.
> > Tested on Chromebook Pixelbook (Nocturne) (reports bdb->version = 221)
> >
> > Cc: <stable@vger.kernel.org> # 5.4+
> > Tested-by: Lukasz Majczak <lma@semihalf.com>
> > Signed-off-by: Lukasz Majczak <lma@semihalf.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_bios.c | 4 +++-
> > drivers/gpu/drm/i915/display/intel_vbt_defs.h | 5 +++++
> > 2 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> > index 3c25926092de..052a19b455d1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > @@ -452,7 +452,9 @@ parse_lfp_backlight(struct drm_i915_private *i915,
> >
> > i915->vbt.backlight.type = INTEL_BACKLIGHT_DISPLAY_DDI;
> > if (bdb->version >= 191 &&
> > - get_blocksize(backlight_data) >= sizeof(*backlight_data)) {
> > + get_blocksize(backlight_data) >= (sizeof(backlight_data->entry_size) +
> > + sizeof(backlight_data->data) +
> > + sizeof(backlight_data->level))) {
>
> Missing sizeof(backlight_data->backlight_control) but this is getting very verbose.
> Would be better have a expected size variable set each version set in the beginning of this function.
>
> something like:
> switch (bdb->version) {
> case 191:
> expected_size = x;
> break;
> case 234:
> expected_size = x;
> break;
> case 236:
> default:
> expected_size = x;
> }
>
>
> > const struct lfp_backlight_control_method *method;
> >
> > method = &backlight_data->backlight_control[panel_type];
> > diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > index 330077c2e588..fff456bf8783 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > @@ -814,6 +814,11 @@ struct lfp_brightness_level {
> > u16 reserved;
> > } __packed;
> >
> > +/*
> > + * Changing struct bdb_lfp_backlight_data might affect its
> > + * size comparation to the value hold in BDB.
> > + * (e.g. in parse_lfp_backlight())
> > + */
>
> This is true for all the blocks so I don't think we need this comment.
>
> > struct bdb_lfp_backlight_data {
> > u8 entry_size;
> > struct lfp_backlight_data_entry data[16];
>
Hi Jose, Jani
Jani - you are right - I was working on 5.4 with a backported patch -
I'm sorry for this confusion.
Jose,
Regarding expected_size, I couldn't find documentation that could
described this structure size changes among revisions, so all I could
do is to do an educated guess, basing on comments at this structure,
like:
(gdb) ptype /o struct bdb_lfp_backlight_data
/* offset | size */ type = struct bdb_lfp_backlight_data {
/* 0 | 1 */ u8 entry_size;
/* 1 | 96 */ struct lfp_backlight_data_entry data[16];
/* 97 | 16 */ u8 level[16];
/* 113 | 16 */ struct lfp_backlight_control_method
backlight_control[16];
/* 129 | 64 */ struct lfp_brightness_level
brightness_level[16]; /* 234+ */
/* 193 | 64 */ struct lfp_brightness_level
brightness_min_level[16]; /* 234+ */
/* 257 | 16 */ u8 brightness_precision_bits[16]; /* 236+ */
/* total size (bytes): 273 */
}
if (revision <= 234)
expected_size = 129;
else if (revision > 234 && revision <=236)
expected_size = 257;
else /* revision > 236 */
expected_size = 273;
Is this approach ok? Otherwise I think I would need help from you to
get exact numbers for each revision...
Best regards,
Lukasz
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] drm/i915/bdb: Fix version check
@ 2021-09-21 9:01 ` Lukasz Majczak
0 siblings, 0 replies; 14+ messages in thread
From: Lukasz Majczak @ 2021-09-21 9:01 UTC (permalink / raw)
To: Souza, Jose
Cc: Lee, Shawn C, dri-devel, joonas.lahtinen, stable, intel-gfx,
jani.nikula, upstream
pon., 20 wrz 2021 o 22:47 Souza, Jose <jose.souza@intel.com> napisał(a):
>
> On Mon, 2021-09-20 at 16:11 +0200, Lukasz Majczak wrote:
> > With patch "drm/i915/vbt: Fix backlight parsing for VBT 234+"
> > the size of bdb_lfp_backlight_data structure has been increased,
> > causing if-statement in the parse_lfp_backlight function
> > that comapres this structure size to the one retrieved from BDB,
> > always to fail for older revisions.
> > This patch fixes it by comparing a total size of all fileds from
> > the structure (present before the change) with the value gathered from BDB.
> > Tested on Chromebook Pixelbook (Nocturne) (reports bdb->version = 221)
> >
> > Cc: <stable@vger.kernel.org> # 5.4+
> > Tested-by: Lukasz Majczak <lma@semihalf.com>
> > Signed-off-by: Lukasz Majczak <lma@semihalf.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_bios.c | 4 +++-
> > drivers/gpu/drm/i915/display/intel_vbt_defs.h | 5 +++++
> > 2 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> > index 3c25926092de..052a19b455d1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > @@ -452,7 +452,9 @@ parse_lfp_backlight(struct drm_i915_private *i915,
> >
> > i915->vbt.backlight.type = INTEL_BACKLIGHT_DISPLAY_DDI;
> > if (bdb->version >= 191 &&
> > - get_blocksize(backlight_data) >= sizeof(*backlight_data)) {
> > + get_blocksize(backlight_data) >= (sizeof(backlight_data->entry_size) +
> > + sizeof(backlight_data->data) +
> > + sizeof(backlight_data->level))) {
>
> Missing sizeof(backlight_data->backlight_control) but this is getting very verbose.
> Would be better have a expected size variable set each version set in the beginning of this function.
>
> something like:
> switch (bdb->version) {
> case 191:
> expected_size = x;
> break;
> case 234:
> expected_size = x;
> break;
> case 236:
> default:
> expected_size = x;
> }
>
>
> > const struct lfp_backlight_control_method *method;
> >
> > method = &backlight_data->backlight_control[panel_type];
> > diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > index 330077c2e588..fff456bf8783 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > @@ -814,6 +814,11 @@ struct lfp_brightness_level {
> > u16 reserved;
> > } __packed;
> >
> > +/*
> > + * Changing struct bdb_lfp_backlight_data might affect its
> > + * size comparation to the value hold in BDB.
> > + * (e.g. in parse_lfp_backlight())
> > + */
>
> This is true for all the blocks so I don't think we need this comment.
>
> > struct bdb_lfp_backlight_data {
> > u8 entry_size;
> > struct lfp_backlight_data_entry data[16];
>
Hi Jose, Jani
Jani - you are right - I was working on 5.4 with a backported patch -
I'm sorry for this confusion.
Jose,
Regarding expected_size, I couldn't find documentation that could
described this structure size changes among revisions, so all I could
do is to do an educated guess, basing on comments at this structure,
like:
(gdb) ptype /o struct bdb_lfp_backlight_data
/* offset | size */ type = struct bdb_lfp_backlight_data {
/* 0 | 1 */ u8 entry_size;
/* 1 | 96 */ struct lfp_backlight_data_entry data[16];
/* 97 | 16 */ u8 level[16];
/* 113 | 16 */ struct lfp_backlight_control_method
backlight_control[16];
/* 129 | 64 */ struct lfp_brightness_level
brightness_level[16]; /* 234+ */
/* 193 | 64 */ struct lfp_brightness_level
brightness_min_level[16]; /* 234+ */
/* 257 | 16 */ u8 brightness_precision_bits[16]; /* 236+ */
/* total size (bytes): 273 */
}
if (revision <= 234)
expected_size = 129;
else if (revision > 234 && revision <=236)
expected_size = 257;
else /* revision > 236 */
expected_size = 273;
Is this approach ok? Otherwise I think I would need help from you to
get exact numbers for each revision...
Best regards,
Lukasz
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-gfx] [PATCH v1] drm/i915/bdb: Fix version check
@ 2021-09-21 9:01 ` Lukasz Majczak
0 siblings, 0 replies; 14+ messages in thread
From: Lukasz Majczak @ 2021-09-21 9:01 UTC (permalink / raw)
To: Souza, Jose
Cc: Lee, Shawn C, dri-devel, joonas.lahtinen, stable, intel-gfx,
jani.nikula, upstream
pon., 20 wrz 2021 o 22:47 Souza, Jose <jose.souza@intel.com> napisał(a):
>
> On Mon, 2021-09-20 at 16:11 +0200, Lukasz Majczak wrote:
> > With patch "drm/i915/vbt: Fix backlight parsing for VBT 234+"
> > the size of bdb_lfp_backlight_data structure has been increased,
> > causing if-statement in the parse_lfp_backlight function
> > that comapres this structure size to the one retrieved from BDB,
> > always to fail for older revisions.
> > This patch fixes it by comparing a total size of all fileds from
> > the structure (present before the change) with the value gathered from BDB.
> > Tested on Chromebook Pixelbook (Nocturne) (reports bdb->version = 221)
> >
> > Cc: <stable@vger.kernel.org> # 5.4+
> > Tested-by: Lukasz Majczak <lma@semihalf.com>
> > Signed-off-by: Lukasz Majczak <lma@semihalf.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_bios.c | 4 +++-
> > drivers/gpu/drm/i915/display/intel_vbt_defs.h | 5 +++++
> > 2 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> > index 3c25926092de..052a19b455d1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > @@ -452,7 +452,9 @@ parse_lfp_backlight(struct drm_i915_private *i915,
> >
> > i915->vbt.backlight.type = INTEL_BACKLIGHT_DISPLAY_DDI;
> > if (bdb->version >= 191 &&
> > - get_blocksize(backlight_data) >= sizeof(*backlight_data)) {
> > + get_blocksize(backlight_data) >= (sizeof(backlight_data->entry_size) +
> > + sizeof(backlight_data->data) +
> > + sizeof(backlight_data->level))) {
>
> Missing sizeof(backlight_data->backlight_control) but this is getting very verbose.
> Would be better have a expected size variable set each version set in the beginning of this function.
>
> something like:
> switch (bdb->version) {
> case 191:
> expected_size = x;
> break;
> case 234:
> expected_size = x;
> break;
> case 236:
> default:
> expected_size = x;
> }
>
>
> > const struct lfp_backlight_control_method *method;
> >
> > method = &backlight_data->backlight_control[panel_type];
> > diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > index 330077c2e588..fff456bf8783 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > @@ -814,6 +814,11 @@ struct lfp_brightness_level {
> > u16 reserved;
> > } __packed;
> >
> > +/*
> > + * Changing struct bdb_lfp_backlight_data might affect its
> > + * size comparation to the value hold in BDB.
> > + * (e.g. in parse_lfp_backlight())
> > + */
>
> This is true for all the blocks so I don't think we need this comment.
>
> > struct bdb_lfp_backlight_data {
> > u8 entry_size;
> > struct lfp_backlight_data_entry data[16];
>
Hi Jose, Jani
Jani - you are right - I was working on 5.4 with a backported patch -
I'm sorry for this confusion.
Jose,
Regarding expected_size, I couldn't find documentation that could
described this structure size changes among revisions, so all I could
do is to do an educated guess, basing on comments at this structure,
like:
(gdb) ptype /o struct bdb_lfp_backlight_data
/* offset | size */ type = struct bdb_lfp_backlight_data {
/* 0 | 1 */ u8 entry_size;
/* 1 | 96 */ struct lfp_backlight_data_entry data[16];
/* 97 | 16 */ u8 level[16];
/* 113 | 16 */ struct lfp_backlight_control_method
backlight_control[16];
/* 129 | 64 */ struct lfp_brightness_level
brightness_level[16]; /* 234+ */
/* 193 | 64 */ struct lfp_brightness_level
brightness_min_level[16]; /* 234+ */
/* 257 | 16 */ u8 brightness_precision_bits[16]; /* 236+ */
/* total size (bytes): 273 */
}
if (revision <= 234)
expected_size = 129;
else if (revision > 234 && revision <=236)
expected_size = 257;
else /* revision > 236 */
expected_size = 273;
Is this approach ok? Otherwise I think I would need help from you to
get exact numbers for each revision...
Best regards,
Lukasz
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-gfx] [PATCH v1] drm/i915/bdb: Fix version check
2021-09-20 20:47 ` [Intel-gfx] " Souza, Jose
@ 2021-09-21 10:15 ` Radosław Biernacki
-1 siblings, 0 replies; 14+ messages in thread
From: Radosław Biernacki @ 2021-09-21 10:15 UTC (permalink / raw)
To: Souza, Jose
Cc: Lee, Shawn C, lma, dri-devel, joonas.lahtinen, intel-gfx,
jani.nikula, upstream
- dropping stable
...
> > diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > index 330077c2e588..fff456bf8783 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > @@ -814,6 +814,11 @@ struct lfp_brightness_level {
> > u16 reserved;
> > } __packed;
> >
> > +/*
> > + * Changing struct bdb_lfp_backlight_data might affect its
> > + * size comparation to the value hold in BDB.
> > + * (e.g. in parse_lfp_backlight())
> > + */
>
> This is true for all the blocks so I don't think we need this comment.
Lack of such comment was probable cause of this overlook.
As this is an example of the consequence (bricking platforms dependent
on mentioned conditions) IMO we need some comment here, or this will
probably happen again.
>
> > struct bdb_lfp_backlight_data {
> > u8 entry_size;
> > struct lfp_backlight_data_entry data[16];
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] drm/i915/bdb: Fix version check
@ 2021-09-21 10:15 ` Radosław Biernacki
0 siblings, 0 replies; 14+ messages in thread
From: Radosław Biernacki @ 2021-09-21 10:15 UTC (permalink / raw)
To: Souza, Jose
Cc: Lee, Shawn C, lma, dri-devel, joonas.lahtinen, intel-gfx,
jani.nikula, upstream
- dropping stable
...
> > diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > index 330077c2e588..fff456bf8783 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > @@ -814,6 +814,11 @@ struct lfp_brightness_level {
> > u16 reserved;
> > } __packed;
> >
> > +/*
> > + * Changing struct bdb_lfp_backlight_data might affect its
> > + * size comparation to the value hold in BDB.
> > + * (e.g. in parse_lfp_backlight())
> > + */
>
> This is true for all the blocks so I don't think we need this comment.
Lack of such comment was probable cause of this overlook.
As this is an example of the consequence (bricking platforms dependent
on mentioned conditions) IMO we need some comment here, or this will
probably happen again.
>
> > struct bdb_lfp_backlight_data {
> > u8 entry_size;
> > struct lfp_backlight_data_entry data[16];
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] drm/i915/bdb: Fix version check
2021-09-21 10:15 ` Radosław Biernacki
@ 2021-09-21 10:27 ` Jani Nikula
-1 siblings, 0 replies; 14+ messages in thread
From: Jani Nikula @ 2021-09-21 10:27 UTC (permalink / raw)
To: Radosław Biernacki, Souza, Jose
Cc: Lee, Shawn C, lma, dri-devel, joonas.lahtinen, intel-gfx, upstream
On Tue, 21 Sep 2021, Radosław Biernacki <rad@semihalf.com> wrote:
> - dropping stable
>
> ...
>
>> > diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
>> > index 330077c2e588..fff456bf8783 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
>> > +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
>> > @@ -814,6 +814,11 @@ struct lfp_brightness_level {
>> > u16 reserved;
>> > } __packed;
>> >
>> > +/*
>> > + * Changing struct bdb_lfp_backlight_data might affect its
>> > + * size comparation to the value hold in BDB.
>> > + * (e.g. in parse_lfp_backlight())
>> > + */
>>
>> This is true for all the blocks so I don't think we need this comment.
>
> Lack of such comment was probable cause of this overlook.
> As this is an example of the consequence (bricking platforms dependent
> on mentioned conditions) IMO we need some comment here, or this will
> probably happen again.
The whole file is full of __packed structs with the sole purpose of
parsing VBT data in memory. People are generally well aware of the
consequences of changing the size, and this is the only such mistake I
can recall.
BR,
Jani.
>
>
>>
>> > struct bdb_lfp_backlight_data {
>> > u8 entry_size;
>> > struct lfp_backlight_data_entry data[16];
>>
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-gfx] [PATCH v1] drm/i915/bdb: Fix version check
@ 2021-09-21 10:27 ` Jani Nikula
0 siblings, 0 replies; 14+ messages in thread
From: Jani Nikula @ 2021-09-21 10:27 UTC (permalink / raw)
To: Radosław Biernacki, Souza, Jose
Cc: Lee, Shawn C, lma, dri-devel, joonas.lahtinen, intel-gfx, upstream
On Tue, 21 Sep 2021, Radosław Biernacki <rad@semihalf.com> wrote:
> - dropping stable
>
> ...
>
>> > diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
>> > index 330077c2e588..fff456bf8783 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
>> > +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
>> > @@ -814,6 +814,11 @@ struct lfp_brightness_level {
>> > u16 reserved;
>> > } __packed;
>> >
>> > +/*
>> > + * Changing struct bdb_lfp_backlight_data might affect its
>> > + * size comparation to the value hold in BDB.
>> > + * (e.g. in parse_lfp_backlight())
>> > + */
>>
>> This is true for all the blocks so I don't think we need this comment.
>
> Lack of such comment was probable cause of this overlook.
> As this is an example of the consequence (bricking platforms dependent
> on mentioned conditions) IMO we need some comment here, or this will
> probably happen again.
The whole file is full of __packed structs with the sole purpose of
parsing VBT data in memory. People are generally well aware of the
consequences of changing the size, and this is the only such mistake I
can recall.
BR,
Jani.
>
>
>>
>> > struct bdb_lfp_backlight_data {
>> > u8 entry_size;
>> > struct lfp_backlight_data_entry data[16];
>>
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-09-22 6:35 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-20 14:11 [PATCH v1] drm/i915/bdb: Fix version check Lukasz Majczak
2021-09-20 14:11 ` [Intel-gfx] " Lukasz Majczak
2021-09-20 16:55 ` Jani Nikula
2021-09-20 16:55 ` [Intel-gfx] " Jani Nikula
2021-09-20 20:47 ` Souza, Jose
2021-09-20 20:47 ` [Intel-gfx] " Souza, Jose
2021-09-21 9:01 ` Lukasz Majczak
2021-09-21 9:01 ` [Intel-gfx] " Lukasz Majczak
2021-09-21 9:01 ` Lukasz Majczak
2021-09-21 10:15 ` [Intel-gfx] " Radosław Biernacki
2021-09-21 10:15 ` Radosław Biernacki
2021-09-21 10:27 ` Jani Nikula
2021-09-21 10:27 ` [Intel-gfx] " Jani Nikula
2021-09-20 21:57 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.