Re: [PATCH 11/13] drm/i915/dmc_wl: Add and use HAS_DMC_WAKELOCK()

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

Quoting Jani Nikula (2024-11-06 06:25:52-03:00)
>On Tue, 05 Nov 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>> Quoting Gustavo Sousa (2024-10-22 08:03:39-03:00)
>>>Quoting Jani Nikula (2024-10-22 06:37:51-03:00)
>>>>On Mon, 21 Oct 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>>>>> In order to be able to use the DMC wakelock, we also need to know that
>>>>> the display hardware has support for DMC, which is a runtime info.
>>>>> Define HAS_DMC_WAKELOCK(), which checks for both DMC availability and IP
>>>>> version, and use it in place of directly checking the display version.
>>>>>
>>>>> Since we depend on runtime info, also make sure to call
>>>>> intel_dmc_wl_init() only after we have probed the hardware for such info
>>>>> (i.e. after intel_display_device_info_runtime_init()).
>>>>
>>>>Non-functional changes combined with functional changes. Please split.
>>>
>>>Do you mean changing the call site of intel_dmc_wl_init() as being
>>>non-functional? Or is it something else?
>>
>> Jani, I'll send a v2 soon-ish. I'll go ahead and assume the anwser for
>> the above is the former. Please stop me if otherwise :-)
>
>Sorry, inbox overflowing. I think I meant that adding HAS_DMC_WAKELOCK()
>is a non-functional change.

Ah, okay.

Well, I think the use of HAS_DMC() in the definition of
HAS_DMC_WAKELOCK() makes it a functional change when intel_dmc_wl.c uses
it (because we were not checking HAS_DMC() before). So, for an earlier
"non-functional" patch, maybe the way to go is something like the
following then?

- A patch defining HAS_DMC_WAKELOCK() with only DISPLAY_VER(i915) >=
  20 and use that macro in the DMC wakelock code.
- A modified version of this patch discarding stuff already done in the
  patch above.

Is that what you meant?

--
Gustavo Sousa

>
>BR,
>Jani.
>
>>
>> --
>> Gustavo Sousa
>>
>>>
>>>If this is about the former, I would argue that's not really
>>>non-functional, because we are changing the order of how things are
>>>done... But if making that a standalone patch is preferred, I can do
>>>that.
>>>
>>>--
>>>Gustavo Sousa
>>>
>>>>
>>>>BR,
>>>>Jani.
>>>>
>>>>>
>>>>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>>>>> ---
>>>>>  drivers/gpu/drm/i915/display/intel_display_device.h | 1 +
>>>>>  drivers/gpu/drm/i915/display/intel_display_driver.c | 2 +-
>>>>>  drivers/gpu/drm/i915/display/intel_dmc_wl.c         | 4 ++--
>>>>>  3 files changed, 4 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
>>>>> index 071a36b51f79..5f78fd127fe0 100644
>>>>> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
>>>>> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
>>>>> @@ -128,6 +128,7 @@ enum intel_display_subplatform {
>>>>>  #define HAS_DDI(i915)                        (DISPLAY_INFO(i915)->has_ddi)
>>>>>  #define HAS_DISPLAY(i915)                (DISPLAY_RUNTIME_INFO(i915)->pipe_mask != 0)
>>>>>  #define HAS_DMC(i915)                        (DISPLAY_RUNTIME_INFO(i915)->has_dmc)
>>>>> +#define HAS_DMC_WAKELOCK(i915)                (HAS_DMC(i915) && DISPLAY_VER(i915) >= 20)
>>>>>  #define HAS_DOUBLE_BUFFERED_M_N(i915)        (DISPLAY_VER(i915) >= 9 || IS_BROADWELL(i915))
>>>>>  #define HAS_DP_MST(i915)                (DISPLAY_INFO(i915)->has_dp_mst)
>>>>>  #define HAS_DP20(i915)                        (IS_DG2(i915) || DISPLAY_VER(i915) >= 14)
>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
>>>>> index 673f9b965494..8afaa9cb89d2 100644
>>>>> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
>>>>> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
>>>>> @@ -200,7 +200,6 @@ void intel_display_driver_early_probe(struct drm_i915_private *i915)
>>>>>          intel_dpll_init_clock_hook(i915);
>>>>>          intel_init_display_hooks(i915);
>>>>>          intel_fdi_init_hook(i915);
>>>>> -        intel_dmc_wl_init(&i915->display);
>>>>>  }
>>>>>  
>>>>>  /* part #1: call before irq install */
>>>>> @@ -238,6 +237,7 @@ int intel_display_driver_probe_noirq(struct drm_i915_private *i915)
>>>>>                  return 0;
>>>>>  
>>>>>          intel_dmc_init(display);
>>>>> +        intel_dmc_wl_init(display);
>>>>>  
>>>>>          i915->display.wq.modeset = alloc_ordered_workqueue("i915_modeset", 0);
>>>>>          i915->display.wq.flip = alloc_workqueue("i915_flip", WQ_HIGHPRI |
>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>>>>> index 8283b607aac4..f6ec79b0e39d 100644
>>>>> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>>>>> @@ -250,7 +250,7 @@ static bool intel_dmc_wl_check_range(struct intel_display *display, u32 address)
>>>>>  
>>>>>  static bool __intel_dmc_wl_supported(struct intel_display *display)
>>>>>  {
>>>>> -        if (DISPLAY_VER(display) < 20 ||
>>>>> +        if (!HAS_DMC_WAKELOCK(display) ||
>>>>>              !intel_dmc_has_payload(display) ||
>>>>>              !display->params.enable_dmc_wl)
>>>>>                  return false;
>>>>> @@ -263,7 +263,7 @@ void intel_dmc_wl_init(struct intel_display *display)
>>>>>          struct intel_dmc_wl *wl = &display->wl;
>>>>>  
>>>>>          /* don't call __intel_dmc_wl_supported(), DMC is not loaded yet */
>>>>> -        if (DISPLAY_VER(display) < 20 || !display->params.enable_dmc_wl)
>>>>> +        if (!HAS_DMC_WAKELOCK(display) || !display->params.enable_dmc_wl)
>>>>>                  return;
>>>>>  
>>>>>          INIT_DELAYED_WORK(&wl->work, intel_dmc_wl_work);
>>>>
>>>>-- 
>>>>Jani Nikula, Intel
>
>-- 
>Jani Nikula, Intel