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

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

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?

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?


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