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