From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Souza, Jose" Subject: Re: [PATCH 2/5] drm/i915: Refactor PSR status debugfs Date: Tue, 11 Dec 2018 12:44:30 +0000 Message-ID: References: <20181204230032.6352-1-jose.souza@intel.com> <20181204230032.6352-2-jose.souza@intel.com> <4d0a8bd1fd7f37e66936ef380d02da9a4c140552.camel@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1158929564==" Return-path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by gabe.freedesktop.org (Postfix) with ESMTPS id E50416E11D for ; Tue, 11 Dec 2018 12:44:32 +0000 (UTC) In-Reply-To: <4d0a8bd1fd7f37e66936ef380d02da9a4c140552.camel@intel.com> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: "intel-gfx@lists.freedesktop.org" , "Pandiyan, Dhinakaran" Cc: "Vivi, Rodrigo" List-Id: intel-gfx@lists.freedesktop.org --===============1158929564== Content-Language: en-US Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-Rz0whMCgw43jNQ3VGuUt" --=-Rz0whMCgw43jNQ3VGuUt Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, 2018-12-10 at 22:51 -0800, Dhinakaran Pandiyan wrote: > On Tue, 2018-12-04 at 15:00 -0800, Jos=C3=A9 Roberto de Souza wrote: > > The old debugfs fields was not following a naming partern and it > > was > > a bit confusing. > >=20 > > So it went from: > > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status > > Sink_Support: yes > > PSR mode: PSR1 > > Enabled: yes > > Busy frontbuffer bits: 0x000 > > Main link in standby mode: no > > HW Enabled & Active bit: yes > > Source PSR status: 0x24050006 [SRDONACK] > >=20 > > To: > > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status > > Sink support: yes [0x00000003] > > Status: PSR1 enabled > > Source PSR ctl: enabled [0x81f00e26] > > Source PSR status: SRDENT [0x40040006] > > Busy frontbuffer bits: 0x00000000 > >=20 > > The 'Main link in standby mode' was removed as it is not useful but > > if needed by someone the information is still in the register value > > of 'Source PSR ctl' inside of the brackets, PSR mode and Enabled > > was > > squashed into Status, some renames and reorders and we have this > > cleaner version. This will also make easy to parse debugfs for IGT > > tests. > >=20 > > Cc: Rodrigo Vivi > > Cc: Dhinakaran Pandiyan > > Suggested-by: Dhinakaran Pandiyan > > Signed-off-by: Jos=C3=A9 Roberto de Souza > > --- > > drivers/gpu/drm/i915/i915_debugfs.c | 96 +++++++++++++++---------- > > ---- > > 1 file changed, 49 insertions(+), 47 deletions(-) > >=20 > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > b/drivers/gpu/drm/i915/i915_debugfs.c > > index 38dcee1ca062..86303ba02666 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -2665,7 +2665,8 @@ DEFINE_SHOW_ATTRIBUTE(i915_psr_sink_status); > > static void > > psr_source_status(struct drm_i915_private *dev_priv, struct > > seq_file > > *m) > > { > > - u32 val, psr_status; > > + u32 val, status_val; > > + const char *status =3D "unknown"; > > =20 > > if (dev_priv->psr.psr2_enabled) { > > static const char * const live_status[] =3D { > > @@ -2681,14 +2682,11 @@ psr_source_status(struct drm_i915_private > > *dev_priv, struct seq_file *m) > > "BUF_ON", > > "TG_ON" > > }; > > - psr_status =3D I915_READ(EDP_PSR2_STATUS); > > - val =3D (psr_status & EDP_PSR2_STATUS_STATE_MASK) >> > > - EDP_PSR2_STATUS_STATE_SHIFT; > > - if (val < ARRAY_SIZE(live_status)) { > > - seq_printf(m, "Source PSR status: 0x%x [%s]\n", > > - psr_status, live_status[val]); > > - return; > > - } > > + val =3D I915_READ(EDP_PSR2_STATUS); > > + status_val =3D (val & EDP_PSR2_STATUS_STATE_MASK) >> > > + EDP_PSR2_STATUS_STATE_SHIFT; > > + if (status_val < ARRAY_SIZE(live_status)) > > + status =3D live_status[status_val]; > > } else { > > static const char * const live_status[] =3D { > > "IDLE", > > @@ -2700,74 +2698,78 @@ psr_source_status(struct drm_i915_private > > *dev_priv, struct seq_file *m) > > "SRDOFFACK", > > "SRDENT_ON", > > }; > > - psr_status =3D I915_READ(EDP_PSR_STATUS); > > - val =3D (psr_status & EDP_PSR_STATUS_STATE_MASK) >> > > - EDP_PSR_STATUS_STATE_SHIFT; > > - if (val < ARRAY_SIZE(live_status)) { > > - seq_printf(m, "Source PSR status: 0x%x [%s]\n", > > - psr_status, live_status[val]); > > - return; > > - } > > + val =3D I915_READ(EDP_PSR_STATUS); > > + status_val =3D (val & EDP_PSR_STATUS_STATE_MASK) >> > > + EDP_PSR_STATUS_STATE_SHIFT; > > + if (status_val < ARRAY_SIZE(live_status)) > > + status =3D live_status[status_val]; > > } > > =20 > > - seq_printf(m, "Source PSR status: 0x%x [%s]\n", psr_status, > > "unknown"); > > + seq_printf(m, "Source PSR status: %s [0x%08x]\n", status, val); > > } > > =20 > > static int i915_edp_psr_status(struct seq_file *m, void *data) > > { > > struct drm_i915_private *dev_priv =3D node_to_i915(m->private); > > - u32 psrperf =3D 0; > > - bool enabled =3D false; > > - bool sink_support; > > + struct i915_psr *psr =3D &dev_priv->psr; > > + const char *status; > > + bool enabled; > > + u32 val; > > =20 > > if (!HAS_PSR(dev_priv)) > > return -ENODEV; > > =20 > > - sink_support =3D dev_priv->psr.sink_support; > > - seq_printf(m, "Sink_Support: %s\n", yesno(sink_support)); > > - if (!sink_support) > > + seq_printf(m, "Sink support: %s", yesno(psr->sink_support)); > > + if (!psr->sink_support) { > > + seq_puts(m, "\n"); > > return 0; > > + } > > =20 > > intel_runtime_pm_get(dev_priv); > > + mutex_lock(&psr->lock); > > =20 > > - mutex_lock(&dev_priv->psr.lock); > > - seq_printf(m, "PSR mode: %s\n", > > - dev_priv->psr.psr2_enabled ? "PSR2" : "PSR1"); > > - seq_printf(m, "Enabled: %s\n", yesno(dev_priv->psr.enabled)); > > - seq_printf(m, "Busy frontbuffer bits: 0x%03x\n", > > - dev_priv->psr.busy_frontbuffer_bits); > > + seq_printf(m, " [0x%08x]\n", psr->dp->psr_dpcd[0]); > This can be moved closer to where "Sink support" is printed. > Also, the > extra zeroes that get printed due to "%08x" look odd, please consider > changing it to "%02x". Done >=20 > > =20 > > - if (dev_priv->psr.psr2_enabled) > > - enabled =3D I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE; > > + if (psr->enabled) > > + status =3D psr->psr2_enabled ? "PSR2 enabled" : "PSR1 > > enabled"; > > else > > - enabled =3D I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE; > > + status =3D "disabled"; > > + seq_printf(m, "Status: %s\n", status); > Can we just use the ctl field and get rid of this? This status and > and > the one below can be confusing to those not familiar with the code > base. >=20 > We should be able to parse all the information we need from two lines > Source PSR ctl: [PSR1,PSR2] enabled > Source PSR status: I'm fine in having just one but when there is frontbuffer modifications Status is kept as enabled and Source PSR ctl and Source PSR status are set to idle states, we know that but those not familiar with the code could think it is disabled but it usualy happen so fast that I guess it is not a problem. And when PSR is disabled we print something else other than Sink support? >=20 >=20 >=20 > > =20 > > - seq_printf(m, "Main link in standby mode: %s\n", > > - yesno(dev_priv->psr.link_standby)); > > + if (!psr->enabled) > > + goto unlock; > > =20 > > - seq_printf(m, "HW Enabled & Active bit: %s\n", yesno(enabled)); > > + if (psr->psr2_enabled) { > > + val =3D I915_READ(EDP_PSR2_CTL); > > + enabled =3D val & EDP_PSR2_ENABLE; > > + } else { > > + val =3D I915_READ(EDP_PSR_CTL); > > + enabled =3D val & EDP_PSR_ENABLE; > > + } > > + seq_printf(m, "Source PSR ctl: %s [0x%08x]\n", > > + enableddisabled(enabled), val); > > + psr_source_status(dev_priv, m); > > + seq_printf(m, "Busy frontbuffer bits: 0x%08x\n", > > + psr->busy_frontbuffer_bits); > > =20 > > /* > > * SKL+ Perf counter is reset to 0 everytime DC state is > > entered > > */ > > if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) { > > - psrperf =3D I915_READ(EDP_PSR_PERF_CNT) & > > - EDP_PSR_PERF_CNT_MASK; > > - > > - seq_printf(m, "Performance_Counter: %u\n", psrperf); > > + val =3D I915_READ(EDP_PSR_PERF_CNT) & > > EDP_PSR_PERF_CNT_MASK; > > + seq_printf(m, "Performance counter: %u\n", val); > > } > > =20 > > - psr_source_status(dev_priv, m); > > - mutex_unlock(&dev_priv->psr.lock); > > - > > - if (READ_ONCE(dev_priv->psr.debug) & I915_PSR_DEBUG_IRQ) { > > + if (psr->debug & I915_PSR_DEBUG_IRQ) { > > seq_printf(m, "Last attempted entry at: %lld\n", > > - dev_priv->psr.last_entry_attempt); > > - seq_printf(m, "Last exit at: %lld\n", > > - dev_priv->psr.last_exit); > > + psr->last_entry_attempt); > > + seq_printf(m, "Last exit at: %lld\n", psr->last_exit); > > } > > =20 > > +unlock: > > + mutex_unlock(&psr->lock); > > intel_runtime_pm_put(dev_priv); > > + > > return 0; > > } > > =20 --=-Rz0whMCgw43jNQ3VGuUt Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEVNG051EijGa0MiaQVenbO/mOWkkFAlwPsSkACgkQVenbO/mO Wkm6Mwf9EqgpJnSPRcq0u0zcTWNpi2ZFI9gFnM3in3TqNDvIWyxL7BCrZfmTNuWf +2g2k3DuwQVi9s+mD4efdsjT5huLSTwg++zIppk8/y9j9LjJavSZve+UFbBk8sKM yx0l9vml/F57xtiDa6CulgjVkPWlbf4cyr+S0FFm7hzrznaw1zJQBl+HWfehTQOI C3HeTBQoK3g8n32wb5MwnrEvN4PeSDYRPLzAw7j5MTbbzkxn/0+NB67dY8Yw864S jwFIYyBf/GhUiKgebWX8bOrN95vEmMJGV1/fYfB0yEJTteFTIc2iE2HtCEH7CPWi vkgfruZfN1LT5Pje/LnIZKKDGrks3g== =Z3ql -----END PGP SIGNATURE----- --=-Rz0whMCgw43jNQ3VGuUt-- --===============1158929564== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg== --===============1158929564==--