| Message ID | 20240221111223.2313692-1-bhanuprakash.modem@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/display/debugfs: New entry "DRRS capable" to i915_drrs_status | expand |
> -----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))); > + Adding DRRS capable property to debugfs. Change LGTM Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com> > seq_printf(m, "DRRS enabled: %s\n", > str_yes_no(crtc_state->has_drrs)); > > -- > 2.43.0
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))); >> + > 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
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
On 2/26/2024 7: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? > > 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? As I understand, for the display there is another debugfs for connector i915_drrs_type which can be none, seamless, static. Here drrs capable means whether the platform is capable, which depends on cpu_transcoder_has_drrs() which in turn depends on HAS_DOUBLE_BUFFERED_M_N and intel_cpu_transcoder_has_m2_n2. Regards, Ankit > > > 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
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. 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@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 >
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 >>
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))); + seq_printf(m, "DRRS enabled: %s\n", str_yes_no(crtc_state->has_drrs));
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(+)