intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm/i915/display: fix display param dup for NULL char * params
@ 2024-04-02 15:55 Jani Nikula
  2024-04-02 16:24 ` Lucas De Marchi
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Jani Nikula @ 2024-04-02 15:55 UTC (permalink / raw)
  To: intel-gfx, intel-xe; +Cc: jani.nikula, Jouni Högander, Luca Coelho, stable

The display param duplication deviates from the original param
duplication in that it converts NULL params to to allocated empty
strings. This works for the vbt_firmware parameter, but not for
dmc_firmware_path, the user of which interprets NULL and the empty
string as distinct values. Specifically, the empty dmc_firmware_path
leads to DMC and PM being disabled.

Just remove the NULL check and pass it to kstrdup(), which safely
returns NULL for NULL input.

Fixes: 8015bee0bfec ("drm/i915/display: Add framework to add parameters specific to display")
Fixes: 0d82a0d6f556 ("drm/i915/display: move dmc_firmware_path to display params")
Cc: Jouni Högander <jouni.hogander@intel.com>
Cc: Luca Coelho <luciano.coelho@intel.com>
Cc: <stable@vger.kernel.org> # v6.8+
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display_params.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c
index c8e3d6892e23..49c6b42077dc 100644
--- a/drivers/gpu/drm/i915/display/intel_display_params.c
+++ b/drivers/gpu/drm/i915/display/intel_display_params.c
@@ -176,9 +176,9 @@ void intel_display_params_dump(struct drm_i915_private *i915, struct drm_printer
 #undef PRINT
 }
 
-__maybe_unused static void _param_dup_charp(char **valp)
+static void _param_dup_charp(char **valp)
 {
-	*valp = kstrdup(*valp ? *valp : "", GFP_ATOMIC);
+	*valp = kstrdup(*valp, GFP_ATOMIC);
 }
 
 __maybe_unused static void _param_nop(void *valp)
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] drm/i915/display: fix display param dup for NULL char * params
  2024-04-02 15:55 [PATCH] drm/i915/display: fix display param dup for NULL char * params Jani Nikula
@ 2024-04-02 16:24 ` Lucas De Marchi
  2024-04-02 16:49   ` Jani Nikula
  2024-04-02 18:14 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 6+ messages in thread
From: Lucas De Marchi @ 2024-04-02 16:24 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx, intel-xe, Jouni Högander, Luca Coelho, stable

On Tue, Apr 02, 2024 at 06:55:34PM +0300, Jani Nikula wrote:
>The display param duplication deviates from the original param
>duplication in that it converts NULL params to to allocated empty
>strings. This works for the vbt_firmware parameter, but not for
>dmc_firmware_path, the user of which interprets NULL and the empty
>string as distinct values. Specifically, the empty dmc_firmware_path
>leads to DMC and PM being disabled.
>
>Just remove the NULL check and pass it to kstrdup(), which safely
>returns NULL for NULL input.
>
>Fixes: 8015bee0bfec ("drm/i915/display: Add framework to add parameters specific to display")
>Fixes: 0d82a0d6f556 ("drm/i915/display: move dmc_firmware_path to display params")
>Cc: Jouni Högander <jouni.hogander@intel.com>
>Cc: Luca Coelho <luciano.coelho@intel.com>
>Cc: <stable@vger.kernel.org> # v6.8+
>Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

... but what's the purpose of the duplication?  How one is supposed to
use the dmc_firmware with e.g. LNL + BMG? Setting it later via debugfs
doesn´t change the behavior. AFAIR this was done to support multiple
devices, but I don't think it achieves its purpose or I'm missing
something.

By leaving a param writable and not duplicate it at all, we are at
least be allowed to:

1) disable autoprobe
2) load module
3) bind do LNL
4) set dmc_firmware param
5) bind to BMG

Yeah, it's manual and not intuitive, but should only be used by
developers with targeted debug.  How would we do something like that
with the current code?

I know that for params via sysfs, it's impossible to get them back to
NULL, so I think we should make sure NULL and empty is handled the same
way. Getting it back to empty is hard enough but at least possible (see
https://lore.kernel.org/igt-dev/20240228223134.3908035-4-lucas.demarchi@intel.com/),
but I think this is not the case for debugfs.

Lucas De Marchi

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] drm/i915/display: fix display param dup for NULL char * params
  2024-04-02 16:24 ` Lucas De Marchi
@ 2024-04-02 16:49   ` Jani Nikula
  0 siblings, 0 replies; 6+ messages in thread
From: Jani Nikula @ 2024-04-02 16:49 UTC (permalink / raw)
  To: Lucas De Marchi
  Cc: intel-gfx, intel-xe, Jouni Högander, Luca Coelho, stable

On Tue, 02 Apr 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Tue, Apr 02, 2024 at 06:55:34PM +0300, Jani Nikula wrote:
>>The display param duplication deviates from the original param
>>duplication in that it converts NULL params to to allocated empty
>>strings. This works for the vbt_firmware parameter, but not for
>>dmc_firmware_path, the user of which interprets NULL and the empty
>>string as distinct values. Specifically, the empty dmc_firmware_path
>>leads to DMC and PM being disabled.
>>
>>Just remove the NULL check and pass it to kstrdup(), which safely
>>returns NULL for NULL input.
>>
>>Fixes: 8015bee0bfec ("drm/i915/display: Add framework to add parameters specific to display")
>>Fixes: 0d82a0d6f556 ("drm/i915/display: move dmc_firmware_path to display params")
>>Cc: Jouni Högander <jouni.hogander@intel.com>
>>Cc: Luca Coelho <luciano.coelho@intel.com>
>>Cc: <stable@vger.kernel.org> # v6.8+
>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

Thanks!

> ... but what's the purpose of the duplication?  How one is supposed to
> use the dmc_firmware with e.g. LNL + BMG? Setting it later via debugfs
> doesn´t change the behavior. AFAIR this was done to support multiple
> devices, but I don't think it achieves its purpose or I'm missing
> something.
>
> By leaving a param writable and not duplicate it at all, we are at
> least be allowed to:
>
> 1) disable autoprobe
> 2) load module
> 3) bind do LNL
> 4) set dmc_firmware param
> 5) bind to BMG
>
> Yeah, it's manual and not intuitive, but should only be used by
> developers with targeted debug.  How would we do something like that
> with the current code?
>
> I know that for params via sysfs, it's impossible to get them back to
> NULL, so I think we should make sure NULL and empty is handled the same
> way. Getting it back to empty is hard enough but at least possible (see
> https://lore.kernel.org/igt-dev/20240228223134.3908035-4-lucas.demarchi@intel.com/),
> but I think this is not the case for debugfs.

There are a lot of angles to this. :)

First of all, I think when we do copy the params, they should be
preserved as they were, instead of changed. This patch fixes this part,
and the bug that currently disables DMC altogether.

We do copies for a few reasons. From module params to device params, and
from device params to error capture.

I agree that making a distinction between an unset parameter and a
parameter set to NULL is probably a mistake, because as you mention it's
not feasible to go back to NULL. In this case, NULL means default and ""
means disabled. No way to go back to default.

For params that only make sense at probe, we should perhaps leave the
module parameter writable, and the device parameter read only. Even in
this case, the duplication is a feature and makes sense: you can modify
the module parameter, but it only makes a difference to devices bound
after the change. For devices already bound, you can look up the value
that was used from debugfs.

BR,
Jani.



-- 
Jani Nikula, Intel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* ✗ Fi.CI.CHECKPATCH: warning for drm/i915/display: fix display param dup for NULL char * params
  2024-04-02 15:55 [PATCH] drm/i915/display: fix display param dup for NULL char * params Jani Nikula
  2024-04-02 16:24 ` Lucas De Marchi
@ 2024-04-02 18:14 ` Patchwork
  2024-04-02 18:28 ` ✗ Fi.CI.BAT: failure " Patchwork
  2024-04-03  8:03 ` [PATCH] " Jani Nikula
  3 siblings, 0 replies; 6+ messages in thread
From: Patchwork @ 2024-04-02 18:14 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/display: fix display param dup for NULL char * params
URL   : https://patchwork.freedesktop.org/series/131949/
State : warning

== Summary ==

Error: dim checkpatch failed
b15c6c58c760 drm/i915/display: fix display param dup for NULL char * params
-:11: WARNING:REPEATED_WORD: Possible repeated word: 'to'
#11: 
duplication in that it converts NULL params to to allocated empty

total: 0 errors, 1 warnings, 0 checks, 11 lines checked



^ permalink raw reply	[flat|nested] 6+ messages in thread

* ✗ Fi.CI.BAT: failure for drm/i915/display: fix display param dup for NULL char * params
  2024-04-02 15:55 [PATCH] drm/i915/display: fix display param dup for NULL char * params Jani Nikula
  2024-04-02 16:24 ` Lucas De Marchi
  2024-04-02 18:14 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
@ 2024-04-02 18:28 ` Patchwork
  2024-04-03  8:03 ` [PATCH] " Jani Nikula
  3 siblings, 0 replies; 6+ messages in thread
From: Patchwork @ 2024-04-02 18:28 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

[-- Attachment #1: Type: text/plain, Size: 11171 bytes --]

== Series Details ==

Series: drm/i915/display: fix display param dup for NULL char * params
URL   : https://patchwork.freedesktop.org/series/131949/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_14516 -> Patchwork_131949v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_131949v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_131949v1, please notify your bug team (I915-ci-infra@lists.freedesktop.org) 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_131949v1/index.html

Participating hosts (37 -> 32)
------------------------------

  Additional (1): fi-cfl-8109u 
  Missing    (6): bat-dg1-7 fi-snb-2520m bat-dg2-11 fi-bsw-nick bat-jsl-1 bat-arls-3 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_131949v1:

### IGT changes ###

#### Possible regressions ####

  * igt@debugfs_test@read_all_entries:
    - fi-kbl-x1275:       [PASS][1] -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/fi-kbl-x1275/igt@debugfs_test@read_all_entries.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/fi-kbl-x1275/igt@debugfs_test@read_all_entries.html
    - bat-adlp-11:        [PASS][3] -> [ABORT][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/bat-adlp-11/igt@debugfs_test@read_all_entries.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/bat-adlp-11/igt@debugfs_test@read_all_entries.html
    - fi-cfl-8109u:       NOTRUN -> [ABORT][5]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/fi-cfl-8109u/igt@debugfs_test@read_all_entries.html
    - fi-kbl-7567u:       [PASS][6] -> [ABORT][7]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/fi-kbl-7567u/igt@debugfs_test@read_all_entries.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/fi-kbl-7567u/igt@debugfs_test@read_all_entries.html
    - bat-adln-1:         [PASS][8] -> [ABORT][9]
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/bat-adln-1/igt@debugfs_test@read_all_entries.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/bat-adln-1/igt@debugfs_test@read_all_entries.html
    - fi-ivb-3770:        [PASS][10] -> [ABORT][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/fi-ivb-3770/igt@debugfs_test@read_all_entries.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/fi-ivb-3770/igt@debugfs_test@read_all_entries.html
    - bat-mtlp-8:         [PASS][12] -> [ABORT][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/bat-mtlp-8/igt@debugfs_test@read_all_entries.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/bat-mtlp-8/igt@debugfs_test@read_all_entries.html
    - fi-elk-e7500:       [PASS][14] -> [ABORT][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/fi-elk-e7500/igt@debugfs_test@read_all_entries.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/fi-elk-e7500/igt@debugfs_test@read_all_entries.html
    - bat-dg2-8:          [PASS][16] -> [ABORT][17]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/bat-dg2-8/igt@debugfs_test@read_all_entries.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/bat-dg2-8/igt@debugfs_test@read_all_entries.html
    - fi-kbl-guc:         [PASS][18] -> [ABORT][19]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/fi-kbl-guc/igt@debugfs_test@read_all_entries.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/fi-kbl-guc/igt@debugfs_test@read_all_entries.html
    - bat-adls-6:         [PASS][20] -> [ABORT][21]
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/bat-adls-6/igt@debugfs_test@read_all_entries.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/bat-adls-6/igt@debugfs_test@read_all_entries.html
    - bat-adlm-1:         [PASS][22] -> [ABORT][23]
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/bat-adlm-1/igt@debugfs_test@read_all_entries.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/bat-adlm-1/igt@debugfs_test@read_all_entries.html
    - fi-ilk-650:         [PASS][24] -> [ABORT][25]
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/fi-ilk-650/igt@debugfs_test@read_all_entries.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/fi-ilk-650/igt@debugfs_test@read_all_entries.html
    - fi-tgl-1115g4:      [PASS][26] -> [ABORT][27]
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/fi-tgl-1115g4/igt@debugfs_test@read_all_entries.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/fi-tgl-1115g4/igt@debugfs_test@read_all_entries.html
    - bat-arls-1:         [PASS][28] -> [ABORT][29]
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/bat-arls-1/igt@debugfs_test@read_all_entries.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/bat-arls-1/igt@debugfs_test@read_all_entries.html
    - fi-cfl-guc:         [PASS][30] -> [ABORT][31]
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/fi-cfl-guc/igt@debugfs_test@read_all_entries.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/fi-cfl-guc/igt@debugfs_test@read_all_entries.html
    - bat-mtlp-6:         [PASS][32] -> [ABORT][33]
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/bat-mtlp-6/igt@debugfs_test@read_all_entries.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/bat-mtlp-6/igt@debugfs_test@read_all_entries.html
    - bat-rpls-3:         [PASS][34] -> [ABORT][35]
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/bat-rpls-3/igt@debugfs_test@read_all_entries.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/bat-rpls-3/igt@debugfs_test@read_all_entries.html
    - fi-pnv-d510:        [PASS][36] -> [ABORT][37]
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/fi-pnv-d510/igt@debugfs_test@read_all_entries.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/fi-pnv-d510/igt@debugfs_test@read_all_entries.html
    - fi-glk-j4005:       [PASS][38] -> [ABORT][39]
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/fi-glk-j4005/igt@debugfs_test@read_all_entries.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/fi-glk-j4005/igt@debugfs_test@read_all_entries.html
    - bat-adlp-9:         [PASS][40] -> [ABORT][41]
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/bat-adlp-9/igt@debugfs_test@read_all_entries.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/bat-adlp-9/igt@debugfs_test@read_all_entries.html
    - fi-cfl-8700k:       [PASS][42] -> [ABORT][43]
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/fi-cfl-8700k/igt@debugfs_test@read_all_entries.html
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/fi-cfl-8700k/igt@debugfs_test@read_all_entries.html
    - bat-dg2-14:         [PASS][44] -> [ABORT][45]
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/bat-dg2-14/igt@debugfs_test@read_all_entries.html
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/bat-dg2-14/igt@debugfs_test@read_all_entries.html
    - bat-kbl-2:          [PASS][46] -> [ABORT][47]
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/bat-kbl-2/igt@debugfs_test@read_all_entries.html
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/bat-kbl-2/igt@debugfs_test@read_all_entries.html
    - bat-rplp-1:         [PASS][48] -> [ABORT][49]
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/bat-rplp-1/igt@debugfs_test@read_all_entries.html
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/bat-rplp-1/igt@debugfs_test@read_all_entries.html
    - fi-rkl-11600:       [PASS][50] -> [ABORT][51]
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/fi-rkl-11600/igt@debugfs_test@read_all_entries.html
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/fi-rkl-11600/igt@debugfs_test@read_all_entries.html
    - bat-jsl-3:          [PASS][52] -> [ABORT][53]
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/bat-jsl-3/igt@debugfs_test@read_all_entries.html
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/bat-jsl-3/igt@debugfs_test@read_all_entries.html
    - bat-dg2-9:          [PASS][54] -> [ABORT][55]
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/bat-dg2-9/igt@debugfs_test@read_all_entries.html
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/bat-dg2-9/igt@debugfs_test@read_all_entries.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@debugfs_test@read_all_entries:
    - {bat-arls-4}:       [PASS][56] -> [ABORT][57]
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/bat-arls-4/igt@debugfs_test@read_all_entries.html
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/bat-arls-4/igt@debugfs_test@read_all_entries.html
    - {bat-mtlp-9}:       [PASS][58] -> [ABORT][59]
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/bat-mtlp-9/igt@debugfs_test@read_all_entries.html
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/bat-mtlp-9/igt@debugfs_test@read_all_entries.html

  
Known issues
------------

  Here are the changes found in Patchwork_131949v1 that come from known issues:

### IGT changes ###

#### Possible fixes ####

  * igt@kms_chamelium_hpd@common-hpd-after-suspend:
    - bat-dg2-13:         [DMESG-WARN][60] ([i915#10651]) -> [PASS][61]
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14516/bat-dg2-13/igt@kms_chamelium_hpd@common-hpd-after-suspend.html
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/bat-dg2-13/igt@kms_chamelium_hpd@common-hpd-after-suspend.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#10651]: https://gitlab.freedesktop.org/drm/intel/issues/10651


Build changes
-------------

  * Linux: CI_DRM_14516 -> Patchwork_131949v1

  CI-20190529: 20190529
  CI_DRM_14516: 5100fcc57dc5d45b246a0aeb068f4f8062d29b09 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7796: 2cfed18f6aa776c1593d7cc328d23225dd61bdf9 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_131949v1: 5100fcc57dc5d45b246a0aeb068f4f8062d29b09 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

e5e94c472551 drm/i915/display: fix display param dup for NULL char * params

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131949v1/index.html

[-- Attachment #2: Type: text/html, Size: 11980 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] drm/i915/display: fix display param dup for NULL char * params
  2024-04-02 15:55 [PATCH] drm/i915/display: fix display param dup for NULL char * params Jani Nikula
                   ` (2 preceding siblings ...)
  2024-04-02 18:28 ` ✗ Fi.CI.BAT: failure " Patchwork
@ 2024-04-03  8:03 ` Jani Nikula
  3 siblings, 0 replies; 6+ messages in thread
From: Jani Nikula @ 2024-04-03  8:03 UTC (permalink / raw)
  To: intel-gfx, intel-xe
  Cc: Jouni Högander, Luca Coelho, Lucas De Marchi, Rodrigo Vivi

On Tue, 02 Apr 2024, Jani Nikula <jani.nikula@intel.com> wrote:
> The display param duplication deviates from the original param
> duplication in that it converts NULL params to to allocated empty
> strings. This works for the vbt_firmware parameter, but not for
> dmc_firmware_path, the user of which interprets NULL and the empty
> string as distinct values. Specifically, the empty dmc_firmware_path
> leads to DMC and PM being disabled.
>
> Just remove the NULL check and pass it to kstrdup(), which safely
> returns NULL for NULL input.

Turns out this fails miserably as well. The debugfs for display params
oopses for NULL value while the i915 core debugfs doesn't. I obviously
didn't realize how the param handling was subtly different between the
two.

Since the quick fix didn't cut it, I've opted to revert 0d82a0d6f556
("drm/i915/display: move dmc_firmware_path to display params") with
Rodrigo's and Lucas' acks. Back to the drawing board.

We probably want to first address the issue with NULL and "" being
distinct values for firmware path params and the fact that you can't set
them back to NULL via debugfs or module param sysfs.

BR,
Jani.


> Fixes: 8015bee0bfec ("drm/i915/display: Add framework to add parameters specific to display")
> Fixes: 0d82a0d6f556 ("drm/i915/display: move dmc_firmware_path to display params")
> Cc: Jouni Högander <jouni.hogander@intel.com>
> Cc: Luca Coelho <luciano.coelho@intel.com>
> Cc: <stable@vger.kernel.org> # v6.8+
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display_params.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c
> index c8e3d6892e23..49c6b42077dc 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_params.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
> @@ -176,9 +176,9 @@ void intel_display_params_dump(struct drm_i915_private *i915, struct drm_printer
>  #undef PRINT
>  }
>  
> -__maybe_unused static void _param_dup_charp(char **valp)
> +static void _param_dup_charp(char **valp)
>  {
> -	*valp = kstrdup(*valp ? *valp : "", GFP_ATOMIC);
> +	*valp = kstrdup(*valp, GFP_ATOMIC);
>  }
>  
>  __maybe_unused static void _param_nop(void *valp)

-- 
Jani Nikula, Intel

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-04-03  8:03 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-02 15:55 [PATCH] drm/i915/display: fix display param dup for NULL char * params Jani Nikula
2024-04-02 16:24 ` Lucas De Marchi
2024-04-02 16:49   ` Jani Nikula
2024-04-02 18:14 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2024-04-02 18:28 ` ✗ Fi.CI.BAT: failure " Patchwork
2024-04-03  8:03 ` [PATCH] " Jani Nikula

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).