| Message ID | 1538540431-6660-1-git-send-email-jyoti.r.yadav@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/csr Added DC5 and DC6 counter register for ICL in debugfs entry. | expand |
> 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... > 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 >
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. > >> 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 >>
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 > > > >
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
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)); } 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)
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(-)