Re: [PATCH] [intel-gfx] drm/i915/csr Added DC5 and DC6 counter register for ICL in debugfs entry.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Oct 03, 2018 at 07:51:24AM -0700, Rodrigo Vivi wrote:
> On Wed, Oct 03, 2018 at 11:27:42AM +0530, Yadav, Jyoti R wrote:
> > 
> > 
> > On 10/3/2018 10:36 AM, Vivi, Rodrigo wrote:
> > > 
> > > > On Oct 2, 2018, at 9:20 PM, Yadav, Jyoti R <jyoti.r.yadav@intel.com> wrote:
> > > > 
> > > > DC5 and DC6 counter register tells about residency of DC5 and DC6.
> > > > These registers are same for SKL and ICL.
> > > > 
> > > > v2 : Remove csr_version check.
> > > >      Added generic check regarding DC counters for  Gen9 onwards. (Rodrigo)
> > > > v3 : Simplified gen checks. (Chris)
> > > > 
> > > > Signed-off-by: Jyoti Yadav <jyoti.r.yadav@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/i915_debugfs.c | 5 ++---
> > > > drivers/gpu/drm/i915/i915_reg.h     | 1 +
> > > > 2 files changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > index a5265c2..af13077 100644
> > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > @@ -2897,13 +2897,12 @@ static int i915_dmc_info(struct seq_file *m, void *unused)
> > > >     seq_printf(m, "version: %d.%d\n", CSR_VERSION_MAJOR(csr->version),
> > > >            CSR_VERSION_MINOR(csr->version));
> > > > 
> > > > -    if (IS_KABYLAKE(dev_priv) ||
> > > > -        (IS_SKYLAKE(dev_priv) && csr->version >= CSR_VERSION(1, 6))) {
> > > > +    if ((!IS_BROXTON(dev_priv)) && IS_GEN(dev_priv, 9, 11)) {
> > > >         seq_printf(m, "DC3 -> DC5 count: %d\n",
> > > >                I915_READ(SKL_CSR_DC3_DC5_COUNT));
> > > >         seq_printf(m, "DC5 -> DC6 count: %d\n",
> > > >                I915_READ(SKL_CSR_DC5_DC6_COUNT));
> > > > -    } else if (IS_BROXTON(dev_priv) && csr->version >= CSR_VERSION(1, 4)) {
> > > > +    } else if (IS_BROXTON(dev_priv)) {
> > > >         seq_printf(m, "DC3 -> DC5 count: %d\n",
> > > >                I915_READ(BXT_CSR_DC3_DC5_COUNT));
> > > >     }
> > > Please do the other way around...
> > > If is broxton {
> > > } else if gen(9,11) {
> > > }
> > > 
> > > So no need for repetition with nots...
> > I was thinking of doing the same earlier, but then thought we should
> > maintain Platform hierarchy. Same trend is followed in other files as well.
> 
> Hmm... general guidance is to leave newer platforms at the top indeed.
> But I think we cannot enforce that as a hard rule when code cleanliness is
> compromised.
> Besides that we already have other exceptions around and this case is a range
> against only platform who is exception, not actually a platform order. BXT is
> inside the range(9, 11) and is the exception... SKL is older than BXT and in
> your proposed block it comes before bxt what breaks that rule anyways right?!

I think the "new first, old last" is a good general guideline. But I
agree with Rodrigo that sometimes that would make things too messy.
In which case I think we can extend the guideline to "helpful
exceptions first, then follow the new->old order". Eg. VLV/CHV
have a habit of needing such exceptions.

I think generally if you have to stop and really think what the
if-statement is saying you should do something to simplify it.
Sometimes just reordering the if ladder a bit is enough, other
times it can be helpful to just extract the entire condition
into a small helper function that has a descriptive name.

Eg.
 if (IS_A || (gen >= x && !IS_B))
vs.
 if (has_something())

The first one almost needs pen and paper to decypher. With the
second one you can just keep on reading without having to
waste further brain cells.

> 
> > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > index 8534f88..573d5f3 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -6985,6 +6985,7 @@ enum {
> > > > /* MMIO address range for CSR program (0x80000 - 0x82FFF) */
> > > > #define CSR_MMIO_START_RANGE    0x80000
> > > > #define CSR_MMIO_END_RANGE    0x8FFFF
> > > > +/* DC3_DC5 count and DC5_DC6 count registers are same for SKL and ICL */
> > > > #define SKL_CSR_DC3_DC5_COUNT    _MMIO(0x80030)
> > > > #define SKL_CSR_DC5_DC6_COUNT    _MMIO(0x8002C)
> > > > #define BXT_CSR_DC3_DC5_COUNT    _MMIO(0x80038)
> > > > -- 
> > > > 1.9.1
> > > > 
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx