On Fri, Nov 02, 2018 at 03:12:18PM -0700, Rodrigo Vivi wrote:
> On Fri, Nov 02, 2018 at 12:47:28PM -0700, Lucas De Marchi wrote:
> > On Fri, Nov 02, 2018 at 12:19:13PM -0700, Rodrigo Vivi wrote:
> > > On Fri, Nov 02, 2018 at 11:10:10AM -0700, Lucas De Marchi wrote:
> > > > On Thu, Nov 01, 2018 at 11:31:25AM +0000, Tvrtko Ursulin wrote:
> > > > >
> > > > > On 01/11/2018 08:35, Lucas De Marchi wrote:
> > > > > > Instead of using several IS_GEN(), it's possible to pass the
> > > > > > range as argument. By code inspection these were the ranges deemed
> > > > > > necessary for spatch:
> > > > > >
> > > > > > @@
> > > > > > expression e;
> > > > > > @@
> > > > > > (
> > > > > > - IS_GEN(e, 3) || IS_GEN(e, 4)
> > > > > > + IS_GEN(e, 3, 4)
> > > > > > |
> > > > > > - IS_GEN(e, 5) || IS_GEN(e, 6)
> > > > > > + IS_GEN(e, 5, 6)
> > > > > > |
> > > > > > - IS_GEN(e, 6) || IS_GEN(e, 7)
> > > > > > + IS_GEN(e, 6, 7)
> > > > > > |
> > > > > > - IS_GEN(e, 7) || IS_GEN(e, 8)
> > > > > > + IS_GEN(e, 7, 8)
> > > > > > |
> > > > > > - IS_GEN(e, 8) || IS_GEN(e, 9)
> > > > > > + IS_GEN(e, 8, 9)
> > > > > > |
> > > > > > - IS_GEN(e, 10) || IS_GEN(e, 9)
> > > > > > + IS_GEN(e, 9, 10)
> > > > > > |
> > > > > > - IS_GEN(e, 9) || IS_GEN(e, 10)
> > > > > > + IS_GEN(e, 9, 10)
> > > > > > )
> > > > > >
> > > > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/i915_debugfs.c | 6 +++---
> > > > > > drivers/gpu/drm/i915/i915_gpu_error.c | 2 +-
> > > > > > drivers/gpu/drm/i915/i915_perf.c | 2 +-
> > > > > > drivers/gpu/drm/i915/intel_crt.c | 2 +-
> > > > > > drivers/gpu/drm/i915/intel_device_info.c | 2 +-
> > > > > > drivers/gpu/drm/i915/intel_display.c | 2 +-
> > > > > > drivers/gpu/drm/i915/intel_engine_cs.c | 2 +-
> > > > > > drivers/gpu/drm/i915/intel_fifo_underrun.c | 2 +-
> > > > > > drivers/gpu/drm/i915/intel_pipe_crc.c | 4 ++--
> > > > > > drivers/gpu/drm/i915/intel_uncore.c | 6 +++---
> > > > > > 10 files changed, 15 insertions(+), 15 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > > index 28d95f9d0b0e..f2fbc016bd7f 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > > @@ -2030,7 +2030,7 @@ static int i915_swizzle_info(struct seq_file *m, void *data)
> > > > > > seq_printf(m, "bit6 swizzle for Y-tiling = %s\n",
> > > > > > swizzle_string(dev_priv->mm.bit_6_swizzle_y));
> > > > > > - if (IS_GEN(dev_priv, 3) || IS_GEN(dev_priv, 4)) {
> > > > > > + if (IS_GEN(dev_priv, 3, 4)) {
> > > > >
> > > > > I can see value in it but think it would read better with IS_GEN_RANGE.
> > > >
> > > > Ok, it seems there's a rough consensus of s/IS_GEN/IS_GEN_RANGE/ an then
> > > > bring the patches that make sense here. There was a recent patch from Rodrigo
> > > > doing that. I'll include it in next version.
> > >
> > > I liked the double args idea but after reading I believe
> > > it gets clear IS_GEN_RANGE.
> > >
> > > >
> > > > >
> > > > > Are there any cases of or-ed IS_GEN checks with something sandwiched in
> > > > > between then, which the above spatch would miss?
> > > >
> > > > By manually inspecting the result of ``git grep -ne "IS_GEN(.*" -- drivers/gpu/drm/i915/``
> > > > I didn't find any. The only thing I found was a missed case for gen3 || gen2
> > > > that was not covered by the spatch.
> > > >
> > > > >
> > > > > How many non-consecutive IS_GEN gen checks are there? To give us some ideas
> > > > > if the usual pattern is range, or perhaps checks against a list of gens also
> > > > > exists? (Gut feeling says no.)
> > > >
> > > > only cases of <=, <, >=, >.
> > >
> > > For these cases on patches 4 and 5::
> > >
> > > What about converting all < n to <= n-1 and all > n to >= n + 1
> > > get FORVER back and introduce IS_GEN_UNTIL ?
> > >
> > > IS_GEN_UNTIL(dev_priv, e)
> > > IS_GEN_RANGE(dev_priv, s, FOREVER)
> > >
> > > so we can also kill INTEL_GEN.
> > >
> > > Another different idea on top of that.
> > >
> > > What about removing all "IS_"?
> > >
> > > so end result could be something like that:
> > >
> > > INTEL_GEN(dev_priv, n)
> > > DISPLAY_GEN(dev_priv, n)
> > > INTEL_GEN_RANGE(dev_priv, s, e) #or e = FOREVER
> > > DISPLAY_GEN_RANGE(dev_priv, s, e) #or e = FOREVER
> > > INTEL_GEN_UNTIL(dev_priv, e)
> > > DISPLAY_GEN_UNTIL(dev_priv, e)
> > >
> > > (maybe s/INTEL/GT)
> >
> > I like it. I'm just not sure about UNTIL, because I will always have doubts if
> > it's inclusive or not. But I guess we have the same today with RANGE and we
> > just get used to it. By making all of them inclusive, it will be easier.
> >
> > Anyway, my preference is:
> >
> > GT_GEN(dev_priv, n)
> > GT_GEN_RANGE(dev_priv, s, e)
> > and e can be GEN_FOREVER, aka -1. The macro has enough knowledge to work out
>
> one niptik
> I prefer FOREVER alone than GEN_FOREVER because macro already has "GEN"
humn... but then the macro loses context and may conflict with other use cases
(not only present ones, but when we decide to add something and will need to
rename FOREVER so it doesn't clutter the namespace. Today we already have
REVID_FOREVER properly namespaced.
>
> > the mask, e.g. s == 10, e == FOREVER => mask == ~(BIT(s) | (BIT(s) - 1))
> >
> > And the DISPLAY_GEN* counterparts.
> >
> > IMO there's no need to have _UNTIL because it can also be expressed as
> > GT_GEN_RANGE(dev_priv, GEN_FOREVER, 10).
>
> Yeap, the GEN_FOREVER I had just killed last week covered this case,
> but my brain refuses to use and accept FOREVER to something in the past.
>
> maybe GT_GEN_RANGE(dev_priv, 0, 10) instead of UNTIL ?
yeah, that's a great idea, very clear and easy to implement.
>
> or GT_GEN_RANGE(dev_priv, BEGIN, 10) ?
>
> >
> > This way we can also kill the comparisons INTEL_GEN(dev_priv) == x, so we always
> > work with the gen_mask field. Which means the compiler can do a single comparison
> > (/me hoping it actually generates good code)
>
> yeap!
>
> >
> > There are corner cases though:
> >
> > What should we do with e.g. IS_GEN9_LP() and friends?
>
> #define GT_GEN_LP(dev_priv, n) GT_GEN(dev_priv, n) && IS_LP(dev_priv)
> #define DISPLAY_GEN_LP(dev_priv, n) DISPLAY_GEN(dev_priv, n) && IS_LP(dev_priv)
> #define GT_GEN_BC(dev_priv, n) GT_GEN(dev_priv, n) && !IS_LP(dev_priv)
> #define DISPLAY_GEN_BC(dev_priv, n) DISPLAY_GEN(dev_priv, n) && !IS_LP(dev_priv)
>
> I think this would be useful to replace many IS_VALLEYVIEW || IS_CHERRYVIEW
> to DISPLAY_GEN_LP(dev_priv, 7)
that works for me, too. So this means in the end we have:
GT_GEN(dev_priv, n)
DISPLAY_GEN(dev_priv, n)
GT_GEN_RANGE(dev_priv, s, e)
DISPLAY_GEN_RANGE(dev_priv, s, e)
With these macros implementing the special case for s == 0 and e == GEN_FOREVER
GT_GEN_LP(dev_priv, n)
DISPLAY_GEN_LP(dev_priv, n)
thanks
Lucas De Marchi
>
> either this above or simply kill it in favor of a more verbose version everywhere
> GT_GEN(dev_priv, 9) && IS_LP(dev_priv) ?! :/
>
> >
> >
> > Any conversion like these will create a lot of noise, not only for inflight patches,
> > but also for stable backports. So IMO anything we do needs to be scriptable.
>
> I fully agree.
>
> Thanks,
> Rodrigo.
>
> >
> > Lucas De Marchi
> >
> > >
> > > and group all of these together, because today
> > > they are spreadded apart.
> > >
> > > Thanks,
> > > Rodrigo.
> > >
> > > >
> > > >
> > > > thanks
> > > > Lucas De Marchi
> > > >
> > > > >
> > > > > Regards,
> > > > >
> > > > > Tvrtko
> > > > >
> > > > > > seq_printf(m, "DDC = 0x%08x\n",
> > > > > > I915_READ(DCC));
> > > > > > seq_printf(m, "DDC2 = 0x%08x\n",
> > > > > > @@ -4260,7 +4260,7 @@ i915_cache_sharing_get(void *data, u64 *val)
> > > > > > struct drm_i915_private *dev_priv = data;
> > > > > > u32 snpcr;
> > > > > > - if (!(IS_GEN(dev_priv, 6) || IS_GEN(dev_priv, 7)))
> > > > > > + if (!(IS_GEN(dev_priv, 6, 7)))
> > > > > > return -ENODEV;
> > > > > > intel_runtime_pm_get(dev_priv);
> > > > > > @@ -4280,7 +4280,7 @@ i915_cache_sharing_set(void *data, u64 val)
> > > > > > struct drm_i915_private *dev_priv = data;
> > > > > > u32 snpcr;
> > > > > > - if (!(IS_GEN(dev_priv, 6) || IS_GEN(dev_priv, 7)))
> > > > > > + if (!(IS_GEN(dev_priv, 6, 7)))
> > > > > > return -ENODEV;
> > > > > > if (val > 3)
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > > > > index c9f8aa47005a..969691e50c04 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > > > > @@ -1673,7 +1673,7 @@ static void capture_reg_state(struct i915_gpu_state *error)
> > > > > > error->ccid = I915_READ(CCID);
> > > > > > /* 3: Feature specific registers */
> > > > > > - if (IS_GEN(dev_priv, 6) || IS_GEN(dev_priv, 7)) {
> > > > > > + if (IS_GEN(dev_priv, 6, 7)) {
> > > > > > error->gam_ecochk = I915_READ(GAM_ECOCHK);
> > > > > > error->gac_eco = I915_READ(GAC_ECO_BITS);
> > > > > > }
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > > > > > index 92daddf79cb0..baaa7b70ffa0 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_perf.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > > > > > @@ -3449,7 +3449,7 @@ void i915_perf_init(struct drm_i915_private *dev_priv)
> > > > > > dev_priv->perf.oa.ops.read = gen8_oa_read;
> > > > > > dev_priv->perf.oa.ops.oa_hw_tail_read = gen8_oa_hw_tail_read;
> > > > > > - if (IS_GEN(dev_priv, 8) || IS_GEN(dev_priv, 9)) {
> > > > > > + if (IS_GEN(dev_priv, 8, 9)) {
> > > > > > dev_priv->perf.oa.ops.is_valid_b_counter_reg =
> > > > > > gen7_is_valid_b_counter_addr;
> > > > > > dev_priv->perf.oa.ops.is_valid_mux_reg =
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> > > > > > index bf4fd739b68c..1822dccb1914 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_crt.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_crt.c
> > > > > > @@ -322,7 +322,7 @@ intel_crt_mode_valid(struct drm_connector *connector,
> > > > > > * DAC limit supposedly 355 MHz.
> > > > > > */
> > > > > > max_clock = 270000;
> > > > > > - else if (IS_GEN(dev_priv, 3) || IS_GEN(dev_priv, 4))
> > > > > > + else if (IS_GEN(dev_priv, 3, 4))
> > > > > > max_clock = 400000;
> > > > > > else
> > > > > > max_clock = 350000;
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> > > > > > index 873f37b7b796..a1b046c322d5 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_device_info.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_device_info.c
> > > > > > @@ -783,7 +783,7 @@ void intel_device_info_runtime_init(struct intel_device_info *info)
> > > > > > DRM_INFO("Display disabled (module parameter)\n");
> > > > > > info->num_pipes = 0;
> > > > > > } else if (info->num_pipes > 0 &&
> > > > > > - (IS_GEN(dev_priv, 7) || IS_GEN(dev_priv, 8)) &&
> > > > > > + (IS_GEN(dev_priv, 7, 8)) &&
> > > > > > HAS_PCH_SPLIT(dev_priv)) {
> > > > > > u32 fuse_strap = I915_READ(FUSE_STRAP);
> > > > > > u32 sfuse_strap = I915_READ(SFUSE_STRAP);
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > > > index 84c432cbdf5b..02b338b1d8be 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > > @@ -10677,7 +10677,7 @@ int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_crtc_stat
> > > > > > * the w/a on all three platforms.
> > > > > > */
> > > > > > if (plane->id == PLANE_SPRITE0 &&
> > > > > > - (IS_GEN(dev_priv, 5) || IS_GEN(dev_priv, 6) ||
> > > > > > + (IS_GEN(dev_priv, 5, 6) ||
> > > > > > IS_IVYBRIDGE(dev_priv)) &&
> > > > > > (turn_on || (!needs_scaling(old_plane_state) &&
> > > > > > needs_scaling(to_intel_plane_state(plane_state)))))
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> > > > > > index 7642f6634f7b..779c683b48ab 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > > > > > @@ -438,7 +438,7 @@ void intel_engine_init_global_seqno(struct intel_engine_cs *engine, u32 seqno)
> > > > > > * the semaphore value, then when the seqno moves backwards all
> > > > > > * future waits will complete instantly (causing rendering corruption).
> > > > > > */
> > > > > > - if (IS_GEN(dev_priv, 6) || IS_GEN(dev_priv, 7)) {
> > > > > > + if (IS_GEN(dev_priv, 6, 7)) {
> > > > > > I915_WRITE(RING_SYNC_0(engine->mmio_base), 0);
> > > > > > I915_WRITE(RING_SYNC_1(engine->mmio_base), 0);
> > > > > > if (HAS_VEBOX(dev_priv))
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > > > > > index ff2743ccbece..a16b463a527d 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > > > > > @@ -260,7 +260,7 @@ static bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
> > > > > > if (HAS_GMCH_DISPLAY(dev_priv))
> > > > > > i9xx_set_fifo_underrun_reporting(dev, pipe, enable, old);
> > > > > > - else if (IS_GEN(dev_priv, 5) || IS_GEN(dev_priv, 6))
> > > > > > + else if (IS_GEN(dev_priv, 5, 6))
> > > > > > ironlake_set_fifo_underrun_reporting(dev, pipe, enable);
> > > > > > else if (IS_GEN(dev_priv, 7))
> > > > > > ivybridge_set_fifo_underrun_reporting(dev, pipe, enable, old);
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > > index 9e870caf8104..3e06570337b6 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > > @@ -433,7 +433,7 @@ static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
> > > > > > return i9xx_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
> > > > > > else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > > > return vlv_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
> > > > > > - else if (IS_GEN(dev_priv, 5) || IS_GEN(dev_priv, 6))
> > > > > > + else if (IS_GEN(dev_priv, 5, 6))
> > > > > > return ilk_pipe_crc_ctl_reg(source, val);
> > > > > > else
> > > > > > return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
> > > > > > @@ -550,7 +550,7 @@ intel_is_valid_crc_source(struct drm_i915_private *dev_priv,
> > > > > > return i9xx_crc_source_valid(dev_priv, source);
> > > > > > else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > > > return vlv_crc_source_valid(dev_priv, source);
> > > > > > - else if (IS_GEN(dev_priv, 5) || IS_GEN(dev_priv, 6))
> > > > > > + else if (IS_GEN(dev_priv, 5, 6))
> > > > > > return ilk_crc_source_valid(dev_priv, source);
> > > > > > else
> > > > > > return ivb_crc_source_valid(dev_priv, source);
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > > > > > index c36453d66d93..88cbd32d6964 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > > > > @@ -528,7 +528,7 @@ check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
> > > > > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > > > ret |= vlv_check_for_unclaimed_mmio(dev_priv);
> > > > > > - if (IS_GEN(dev_priv, 6) || IS_GEN(dev_priv, 7))
> > > > > > + if (IS_GEN(dev_priv, 6, 7))
> > > > > > ret |= gen6_check_for_fifo_debug(dev_priv);
> > > > > > return ret;
> > > > > > @@ -556,7 +556,7 @@ static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
> > > > > > dev_priv->uncore.funcs.force_wake_get(dev_priv,
> > > > > > restore_forcewake);
> > > > > > - if (IS_GEN(dev_priv, 6) || IS_GEN(dev_priv, 7))
> > > > > > + if (IS_GEN(dev_priv, 6, 7))
> > > > > > dev_priv->uncore.fifo_count =
> > > > > > fifo_free_entries(dev_priv);
> > > > > > spin_unlock_irq(&dev_priv->uncore.lock);
> > > > > > @@ -1437,7 +1437,7 @@ static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv)
> > > > > > FORCEWAKE_MEDIA_VEBOX_GEN11(i),
> > > > > > FORCEWAKE_ACK_MEDIA_VEBOX_GEN11(i));
> > > > > > }
> > > > > > - } else if (IS_GEN(dev_priv, 10) || IS_GEN(dev_priv, 9)) {
> > > > > > + } else if (IS_GEN(dev_priv, 9, 10)) {
> > > > > > dev_priv->uncore.funcs.force_wake_get =
> > > > > > fw_domains_get_with_fallback;
> > > > > > dev_priv->uncore.funcs.force_wake_put = fw_domains_put;
> > > > > >
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
