[PATCH] drm/i915/display/debugfs: New entry "DRRS capable" to i915_drrs_status

[Modem, Bhanuprakash]

On 26-02-2024 07:50 pm, Jani Nikula wrote:
> On Mon, 26 Feb 2024, "Nautiyal, Ankit K" <ankit.k.nautiyal at intel.com> wrote:
>> On 2/22/2024 11:27 AM, Golani, Mitulkumar Ajitkumar wrote:
>>>
>>>> -----Original Message-----
>>>> From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of
>>>> Bhanuprakash Modem
>>>> Sent: Wednesday, February 21, 2024 4:42 PM
>>>> To: intel-gfx at lists.freedesktop.org
>>>> Cc: Modem, Bhanuprakash <bhanuprakash.modem at intel.com>
>>>> Subject: [PATCH] drm/i915/display/debugfs: New entry "DRRS capable" to
>>>> i915_drrs_status
>>>>
>>>> If the connected panel supports both DRRS & PSR, driver gives preference to
>>>> PSR ("DRRS enabled: no"). Even though the hardware supports DRRS, IGT
>>>> treats ("DRRS enabled: yes") as not capable.
>>>>
>>>> Introduce a new entry "DRRS capable" to debugfs i915_drrs_status, so that
>>>> IGT will read the DRRS capability as "DRRS capable: yes".
>>>>
>>>> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/display/intel_drrs.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_drrs.c
>>>> b/drivers/gpu/drm/i915/display/intel_drrs.c
>>>> index 6282ec0fc9b4..169ef38ff188 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_drrs.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_drrs.c
>>>> @@ -299,6 +299,7 @@ void intel_drrs_crtc_init(struct intel_crtc *crtc)  static
>>>> int intel_drrs_debugfs_status_show(struct seq_file *m, void *unused)  {
>>>>    	struct intel_crtc *crtc = m->private;
>>>> +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
>>>>    	const struct intel_crtc_state *crtc_state;
>>>>    	int ret;
>>>>
>>>> @@ -310,6 +311,11 @@ static int intel_drrs_debugfs_status_show(struct
>>>> seq_file *m, void *unused)
>>>>
>>>>    	mutex_lock(&crtc->drrs.mutex);
>>>>
>>>> +	seq_printf(m, "DRRS capable: %s\n",
>>>> +		   str_yes_no(crtc_state->has_drrs ||
>>>> +			      HAS_DOUBLE_BUFFERED_M_N(i915) ||
>>>> +			      intel_cpu_transcoder_has_m2_n2(i915,
>>>> +crtc_state->cpu_transcoder)));
> 
> Why would "capability" look at ->has_drrs?

IGT interprets the platform capability as "DRRS enabled: yes", which is 
represented by crtc_state->has_drrs.

However, if the connected panel supports both DRRS and PSR, the driver 
prioritizes PSR, causing crtc_state->has_drrs to become false. This 
leads to IGT incorrectly reading the DRRS capability as "DRRS enabled: no".

To rectify this we introduced a new entry "DRRS capable: yes/no".

> 
> Why didn't anyone question the duplication of the conditions of what
> "drrs capable" means?
> 
> And what *does* "drrs capable" mean here anyway? That the platform is
> capable? But what if the display isn't capable?

"DRRS capable: yes/no" is the platform capability. For display 
capability, there is another connector specific debugfs called 
"i915_drrs_type".

- Bhanu

> 
> 
> BR,
> Jani.
> 
> 
> 
>>>> +
>>> Adding DRRS capable property to debugfs.
>>>
>>> Change LGTM
>>> Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani at intel.com>
>>
>>
>> Thanks for the patch and review. Pushed to drm-intel-next.
>>
>> Regards,
>>
>> Ankit
>>
>>>>    	seq_printf(m, "DRRS enabled: %s\n",
>>>>    		   str_yes_no(crtc_state->has_drrs));
>>>>
>>>> --
>>>> 2.43.0
>