Re: [PATCH 13/13] drm/i915/xe3lpd: Use DMC wakelock by default

[Gustavo Sousa <gustavo.sousa@intel.com>]

Quoting Gustavo Sousa (2024-11-05 10:46:52-03:00)
>Quoting Luca Coelho (2024-11-01 11:27:10-03:00)
>>On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
>>> Although Bspec doesn't explicitly mentions that, as of Xe3_LPD, using
>>> DMC wakelock is the officially recommended way of accessing registers
>>> that would be off during DC5/DC6 and the legacy method (where the DMC
>>> intercepts MMIO to wake up the hardware) is to be avoided.
>>> 
>>> As such, update the driver to use the DMC wakelock by default starting
>>> with Xe3_LPD. Since the feature is somewhat new to the driver, also
>>> allow disabling it via a module parameter for debugging purposes.
>>> 
>>> For that, make the existing parameter allow values -1 (per-chip
>>> default), 0 (disabled) and 1 (enabled), similarly to what is done for
>>> other parameters.
>>> 
>>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/display/intel_display_params.c | 4 ++--
>>>  drivers/gpu/drm/i915/display/intel_display_params.h | 2 +-
>>>  drivers/gpu/drm/i915/display/intel_dmc_wl.c         | 6 +++++-
>>>  3 files changed, 8 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c
>>> index 024de8abcb1a..bf00e5f1f145 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_params.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
>>> @@ -123,10 +123,10 @@ intel_display_param_named_unsafe(enable_psr2_sel_fetch, bool, 0400,
>>>          "(0=disabled, 1=enabled) "
>>>          "Default: 1");
>>>  
>>> -intel_display_param_named_unsafe(enable_dmc_wl, bool, 0400,
>>> +intel_display_param_named_unsafe(enable_dmc_wl, int, 0400,
>>>          "Enable DMC wakelock "
>>>          "(0=disabled, 1=enabled) "
>>> -        "Default: 0");
>>> +        "Default: -1 (use per-chip default)");
>>
>>We're already explaining the possible values in the previous
>>parentheses, so maybe the -1 should also be explained there?
>
>Yep that makes sense. I was following the trend of what was done for
>enable_fbc and enable_psr, but I guess following other examples in this
>same file where we tag the default one with "[default]" is better.

Ended up simply doing this:

    diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c
    index bf00e5f1f145..dc666aefa362 100644
    --- a/drivers/gpu/drm/i915/display/intel_display_params.c
    +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
    @@ -125,8 +125,8 @@ intel_display_param_named_unsafe(enable_psr2_sel_fetch, bool, 0400,
     
     intel_display_param_named_unsafe(enable_dmc_wl, int, 0400,
     	"Enable DMC wakelock "
    -	"(0=disabled, 1=enabled) "
    -	"Default: -1 (use per-chip default)");
    +	"(-1=use per-chip default, 0=disabled, 1=enabled) "
    +	"Default: -1");
     
     __maybe_unused
     static void _param_print_bool(struct drm_printer *p, const char *driver_name,

, because repeating the word "default" in "(-1=use per-chip default
[default], 0=disabled, 1=enabled)" looked weird.

--
Gustavo Sousa

>
>Thanks! I'll update this on the next version.
>
>--
>Gustavo Sousa