On Tue, 02 Apr 2024, Jani Nikula <jani.nikula@xxxxxxxxx> 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@xxxxxxxxx>
> Cc: Luca Coelho <luciano.coelho@xxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> # v6.8+
> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> ---
> 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
