[Intel-gfx] [PATCH v2 0/7] Make GEN macros more similar
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu Nov 8 11:23:46 UTC 2018
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(-)
>>>
>
More information about the Intel-gfx
mailing list