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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 27 Feb 2024, "Modem, Bhanuprakash" <bhanuprakash.modem@intel.com> wrote:
> On 26-02-2024 07:50 pm, Jani Nikula wrote:
>> On Mon, 26 Feb 2024, "Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com> wrote:
>>> On 2/22/2024 11:27 AM, Golani, Mitulkumar Ajitkumar wrote:
>>>>
>>>>> -----Original Message-----
>>>>> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
>>>>> Bhanuprakash Modem
>>>>> Sent: Wednesday, February 21, 2024 4:42 PM
>>>>> To: intel-gfx@lists.freedesktop.org
>>>>> Cc: Modem, Bhanuprakash <bhanuprakash.modem@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@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.

That doesn't answer the question.

> 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?

Please remove the duplication. There should be a single point of truth
on what "drrs capable" means. One function.

BR,
Jani.


>> 
>> 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@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
>> 

-- 
Jani Nikula, Intel