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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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

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