Re: [PATCH v2 0/7] Make GEN macros more similar

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Nov 08, 2018 at 11:23:46AM +0000, Tvrtko Ursulin wrote:
> 
> On 08/11/2018 00:57, Lucas De Marchi wrote:
> > On Wed, Nov 07, 2018 at 10:05:19AM +0000, Tvrtko Ursulin wrote:
> > > 
> > > On 06/11/2018 21:51, Lucas De Marchi wrote:
> > > > This is the second version of the series trying to make GEN checks
> > > > more similar. These or roughly the changes from v1:
> > > > 
> > > > - We don't have a single macro receiving 2 or 3 parameters. Now there
> > > >     is GT_GEN_RANGE(), and GT_GEN(). The firs is the conversion from
> > > >     IS_GEN() while the second is the conversion from IS_GEN<N>()
> > > > - Bring GEN_FOREVER back to be used with above macros
> > > > - Patch converting <, <=, ==, >, >=  checks using INTEL_GEN() to
> > > >     use the macros above was added
> > > > - INTEL_GEN() is removed to avoid it being used when we should prefer
> > > >     the new macros
> > > > 
> > > > The idea of the names is to pave the way for checks of the display version,
> > > > which would be named DISPLAY_GEN(), DISPLAY_GEN_RANGE().
> > > > 
> > > > In the commit messages we have the scripts to regenerate the patch to make
> > > > it easier to apply after the discussions and also to be able to convert
> > > > inflight patches.
> > > > 
> > > > Sorry in advance for the noise this causes in the codebase, but hopefully
> > > > it is for the greater good.
> > > > 
> > > > 
> > > > Lucas De Marchi (6):
> > > >     Revert "drm/i915: Kill GEN_FOREVER"
> > > >     drm/i915: replace IS_GEN<N> with GT_GEN(..., N)
> > > >     drm/i915: rename IS_GEN9_* to GT_GEN9_*
> > > >     drm/i915: replace gen checks using operators by GT_GEN/GT_GEN_RANGE
> > > 
> > > I have reservations about this patch, where I think forcing only one flavour
> > > maybe is not the best thing. Because for plain if-ladder cases it feels more
> > > readable to stick with the current scheme of arithmetic comparisons. And it
> > > is more efficient in code as well.
> > > 
> > > Range checks are on the other hand useful either when combined in the same
> > > conditional as some other bitmask based test, or when both ends of the
> > > comparison edge are bound.
> > 
> > So are you against changing the == to use the macros, changing the >=, >, <, <=,
> > or all of them?
> 
> Definitely not all of them. Just plain if ladders I think are definitely
> more readable in source and result in better code in the normal fashion of:
> 
>    if (gen >= 11)
>    else if (gen >= 9)
>    else if (gen >= 7)
>    ... etc ...
> 
> Where I think it makes sense is when either both edges are bound, like:
> 
>   if (gen < 11 || gen >= 8)
>   if (gen >= 10 || gen == 8)

ok, I will take a look before respinning this.

> 
> But not sure how many of those there are.
> 
> What definitely exists in large-ish numbers are:
> 
>    if (gen >= 11 ||  IS_PLATFORM)
> 
> At some point I had a prototype which puts the gen and platform masks into a
> bag of bits and then, depending on bits locality, this too can be compressed
> to a single bitmask test. However I felt that was going too far, and the
> issue is achieving interesting bits locality for it too work. But I digress.
> 
> > Looking at the changes to ==, they seem very reasonable and in a few cases it allowed
> > the next patch to merge them in a GT_GEN_RANGE() -- yes the patch ordering was on
> > purpose to allow that.
> 
> Yep those are the good ones.
> 
> > The others are indeed debatable. However IMO for the cases it makes sense,
> > there's always the fallback
> > 
> > if (dev_priv->info.gen == 10)
> >      ...
> > else if (dev_priv->info.gen == 11)
> >      ...
> > else if (dev_priv->info.gen < 5)
> >      ...
> > 
> > We can go on a case by case manner in this patch rather than the mass conversion
> > for these.
> > 
> > > 
> > > >     drm/i915: merge gen checks to use range
> > > >     drm/i915: remove INTEL_GEN macro
> > > 
> > > I have reservations about this as as well, especially considering the
> > > previous paragraph. But even on it's own I am not sure we want to go back to
> > > more verbose.
> > 
> > see above. IMO it's not actually so verbose as one would think.
> > 
> >      if (INTEL_GEN(dev_priv) == 11)
> >      vs
> >      if (dev_priv->info.gen == 11)
> 
> I think it should be:
> 
>        if (INTEL_INFO(dev_priv)->gen == 11)
> 
> Which is a tiny bit longer..

If it's longer, why bother? We could just as well do for the if ladders:

gen = dev_priv->info.gen;
or
gen = INTEL_INFO(dev_priv)->gen

In your other series you would be moving gen to a runtime info, so this
would cause the same amount of churn (although I disagree with moving gen to a runtime
info just because of the mock struct).


> 
> > The "verbose" version is actually one character less and one lookup less
> > to what the macro is doing underneath. Of course that means a lot of churn
> > to the codebase when/if we change where the gen number is located, but that
> > number is at the same place since its introduction in 2010
> > (commit c96c3a8cb7fadcb33d9a5ebe35fcee8b7d0a7946)
> 
> I am pretty sure we went through patches to a) move towards INTEL_INFO and
> b) replace INTEL_INFO(dev_priv)->gen with INTEL_GEN. So I don't see an
> advantage of reverting that, just so that we can lose the IS_ prefix below.
> 
> > > Perhaps in the new scheme of things it should be renamed to INTEL_GT_GEN? I
> > > know it doesn't fit nicely with the naming scheme of GT/DISPLAY_GEN.. so
> > > maybe:
> > > 
> > > GT_GEN -> IS_GT_GEN
> > > GT_GEN_RANGE -> IS_GT_GEN_RANGE
> > > INTEL_GEN -> GT_GEN (but churn!?)
> > 
> > I still think INTEL_GEN() is not bringing much clarity and forcing
> > the other macros to have the IS_ prefix.
> 
> Is the IS_ prefix that bad?
> 
> I agree my idea does not decrease the amount of churn, since it said to
> replace INTEL_GEN with INTEL_GT_GEN. But in the light of the GT/DISPLAY
> split, and if we agree to leave a lot of the arithmetic comparison in
> (dialing down of "drm/i915: replace gen checks using operators by
> GT_GEN/GT_GEN_RANGE"), then it feels going back to INTEL_INFO(dev_priv)->gen
> throughout the code is undoing some work, just so you can remove the
> INTEL_GEN macro instead of renaming it INTEL_GT_GEN.
> 
> Don't know, it's my opinion at least and more people are welcome to chime in
> with theirs.

Any others to chime in on this? Jani, Ville, Rodrigo?

thanks
Lucas De Marchi

> 
> Regards,
> 
> Tvrtko
> 
> P.S. Reminded me of this tangential attempt as well:
> https://patchwork.freedesktop.org/patch/206168/, which I thought was a good
> way to make the code more elegant.
> 
> 
> > > 
> > > This leaves GT and DISPLAY namespaces completely parallel in syntax and
> > > semantics.
> > > 
> > > Regards,
> > > 
> > > Tvrtko
> > > 
> > > 
> > > 
> > > > 
> > > > Rodrigo Vivi (1):
> > > >     drm/i915: Rename IS_GEN to GT_RANGE
> > > > 
> > > >    drivers/gpu/drm/i915/gvt/gtt.c                |   4 +-
> > > >    drivers/gpu/drm/i915/gvt/handlers.c           |   2 +-
> > > >    drivers/gpu/drm/i915/gvt/vgpu.c               |   4 +-
> > > >    drivers/gpu/drm/i915/i915_cmd_parser.c        |   2 +-
> > > >    drivers/gpu/drm/i915/i915_debugfs.c           | 145 +++++----
> > > >    drivers/gpu/drm/i915/i915_drv.c               |  50 +--
> > > >    drivers/gpu/drm/i915/i915_drv.h               |  67 ++--
> > > >    drivers/gpu/drm/i915/i915_gem.c               |  30 +-
> > > >    drivers/gpu/drm/i915/i915_gem_context.c       |   4 +-
> > > >    drivers/gpu/drm/i915/i915_gem_execbuffer.c    |  10 +-
> > > >    drivers/gpu/drm/i915/i915_gem_fence_reg.c     |  16 +-
> > > >    drivers/gpu/drm/i915/i915_gem_gtt.c           |  36 +-
> > > >    drivers/gpu/drm/i915/i915_gem_render_state.c  |   2 +-
> > > >    drivers/gpu/drm/i915/i915_gem_stolen.c        |  15 +-
> > > >    drivers/gpu/drm/i915/i915_gem_tiling.c        |  12 +-
> > > >    drivers/gpu/drm/i915/i915_gpu_error.c         |  62 ++--
> > > >    drivers/gpu/drm/i915/i915_irq.c               | 120 +++----
> > > >    drivers/gpu/drm/i915/i915_perf.c              |  14 +-
> > > >    drivers/gpu/drm/i915/i915_pmu.c               |   6 +-
> > > >    drivers/gpu/drm/i915/i915_reg.h               |   8 +-
> > > >    drivers/gpu/drm/i915/i915_suspend.c           |  24 +-
> > > >    drivers/gpu/drm/i915/i915_sysfs.c             |   2 +-
> > > >    drivers/gpu/drm/i915/intel_atomic.c           |   4 +-
> > > >    drivers/gpu/drm/i915/intel_audio.c            |   4 +-
> > > >    drivers/gpu/drm/i915/intel_bios.c             |  12 +-
> > > >    drivers/gpu/drm/i915/intel_cdclk.c            |  28 +-
> > > >    drivers/gpu/drm/i915/intel_color.c            |   6 +-
> > > >    drivers/gpu/drm/i915/intel_crt.c              |  12 +-
> > > >    drivers/gpu/drm/i915/intel_csr.c              |   2 +-
> > > >    drivers/gpu/drm/i915/intel_ddi.c              |  58 ++--
> > > >    drivers/gpu/drm/i915/intel_device_info.c      |  46 +--
> > > >    drivers/gpu/drm/i915/intel_display.c          | 308 +++++++++---------
> > > >    drivers/gpu/drm/i915/intel_dp.c               |  82 ++---
> > > >    drivers/gpu/drm/i915/intel_dp_mst.c           |   2 +-
> > > >    drivers/gpu/drm/i915/intel_dpll_mgr.c         |  10 +-
> > > >    drivers/gpu/drm/i915/intel_drv.h              |   2 +-
> > > >    drivers/gpu/drm/i915/intel_engine_cs.c        |  44 +--
> > > >    drivers/gpu/drm/i915/intel_fbc.c              |  52 +--
> > > >    drivers/gpu/drm/i915/intel_fifo_underrun.c    |   8 +-
> > > >    drivers/gpu/drm/i915/intel_guc_fw.c           |   4 +-
> > > >    drivers/gpu/drm/i915/intel_hangcheck.c        |   6 +-
> > > >    drivers/gpu/drm/i915/intel_hdcp.c             |   2 +-
> > > >    drivers/gpu/drm/i915/intel_hdmi.c             |  18 +-
> > > >    drivers/gpu/drm/i915/intel_i2c.c              |  14 +-
> > > >    drivers/gpu/drm/i915/intel_lrc.c              |  32 +-
> > > >    drivers/gpu/drm/i915/intel_lvds.c             |  14 +-
> > > >    drivers/gpu/drm/i915/intel_mocs.c             |   8 +-
> > > >    drivers/gpu/drm/i915/intel_overlay.c          |  12 +-
> > > >    drivers/gpu/drm/i915/intel_panel.c            |  20 +-
> > > >    drivers/gpu/drm/i915/intel_pipe_crc.c         |  12 +-
> > > >    drivers/gpu/drm/i915/intel_pm.c               | 196 +++++------
> > > >    drivers/gpu/drm/i915/intel_psr.c              |  26 +-
> > > >    drivers/gpu/drm/i915/intel_ringbuffer.c       |  68 ++--
> > > >    drivers/gpu/drm/i915/intel_ringbuffer.h       |   4 +-
> > > >    drivers/gpu/drm/i915/intel_runtime_pm.c       |  32 +-
> > > >    drivers/gpu/drm/i915/intel_sdvo.c             |  14 +-
> > > >    drivers/gpu/drm/i915/intel_sprite.c           |  34 +-
> > > >    drivers/gpu/drm/i915/intel_tv.c               |   2 +-
> > > >    drivers/gpu/drm/i915/intel_uc.c               |   4 +-
> > > >    drivers/gpu/drm/i915/intel_uncore.c           |  60 ++--
> > > >    drivers/gpu/drm/i915/intel_wopcm.c            |   8 +-
> > > >    drivers/gpu/drm/i915/intel_workarounds.c      |  20 +-
> > > >    drivers/gpu/drm/i915/selftests/huge_pages.c   |   2 +-
> > > >    .../drm/i915/selftests/i915_gem_coherency.c   |   4 +-
> > > >    .../gpu/drm/i915/selftests/i915_gem_context.c |  10 +-
> > > >    .../gpu/drm/i915/selftests/i915_gem_object.c  |  12 +-
> > > >    drivers/gpu/drm/i915/selftests/i915_request.c |   2 +-
> > > >    .../gpu/drm/i915/selftests/intel_hangcheck.c  |   8 +-
> > > >    drivers/gpu/drm/i915/selftests/intel_lrc.c    |   2 +-
> > > >    drivers/gpu/drm/i915/selftests/intel_uncore.c |   2 +-
> > > >    .../drm/i915/selftests/intel_workarounds.c    |   2 +-
> > > >    drivers/gpu/drm/i915/vlv_dsi.c                |  40 +--
> > > >    72 files changed, 1003 insertions(+), 1006 deletions(-)
> > > > 
> > 
> _______________________________________________
> 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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux