Re: [PATCH 2/2] drm/i915/psr: Use DMC wakelock instead of DC state for VBI

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

Gustavo Sousa <gustavo.sousa@intel.com> writes:

> Dibin Moolakadan Subrahmanian <dibin.moolakadan.subrahmanian@intel.com>
> writes:
>
>> From display version 30+, use the DMC wakelock mechanism for
>> preventing DC entry. Older platforms continue to use
>> set_target_dc_state() to disable DC entry during active vblank.
>>
>> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/work_items/2296
>> Fixes: acfc688bc51b ("drm/i915/dmc: Reduce wakelock hold time")
>> Signed-off-by: Dibin Moolakadan Subrahmanian <dibin.moolakadan.subrahmanian@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_psr.c | 26 +++++++++++++++---------
>>  1 file changed, 16 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>> index 9acd47392192..d01b4013f33f 100644
>> --- a/drivers/gpu/drm/i915/display/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>> @@ -4151,16 +4151,22 @@ void intel_psr_notify_vblank_enable_disable(struct intel_display *display,
>>  		mutex_unlock(&intel_dp->psr.lock);
>>  		break;
>>  	}
>> -
>> -	/*
>> -	 * NOTE: intel_display_power_set_target_dc_state is used
>> -	 * only by PSR * code for DC3CO handling. DC3CO target
>> -	 * state is currently disabled in * PSR code. If DC3CO
>> -	 * is taken into use we need take that into account here
>> -	 * as well.
>> -	 */
>> -	intel_display_power_set_target_dc_state(display, enable ? DC_STATE_DISABLE :
>> -						DC_STATE_EN_UPTO_DC6);
>> +	if (DISPLAY_VER(display) < 30) {
>
> Drive-by comment:
>
> I think we should avoid using open-coded version check for testing
> features.  It is usually better to use the HAS_SOME_FEATURE() macros
> that we expose in intel_display_device.h.
>
> In this case though, it is better to use to expose and use
> __intel_dmc_wl_supported (dropping the "__") instead of doing a display
> version check here, since that is what will actually tell whether the
> driver is using the wakelock or not.
>
> I even think we can even drop the HAS_DMC_WAKELOCK, since it is not used
> outside of intel_dmc_wl.c.

For this last part:
https://lore.kernel.org/all/20260429-drop-has_dmc_wakelock-v1-1-62cb6fab1da0@intel.com

--
Gustavo Sousa

>
> --
> Gustavo Sousa
>
>> +		/*
>> +		 * NOTE: intel_display_power_set_target_dc_state is used
>> +		 * only by PSR code for DC3CO handling. DC3CO target
>> +		 * state is currently disabled in PSR code. If DC3CO
>> +		 * is taken into use we need take that into account here
>> +		 * as well.
>> +		 */
>> +		intel_display_power_set_target_dc_state(display, enable ? DC_STATE_DISABLE :
>> +							DC_STATE_EN_UPTO_DC6);
>> +	} else {
>> +		if (enable)
>> +			intel_dmc_wl_get_noreg(display);
>> +		else
>> +			intel_dmc_wl_put_noreg(display);
>> +	}
>>  }
>>  
>>  static void
>> -- 
>> 2.43.0