On Mon, Nov 19, 2018 at 02:20:55PM -0800, Lucas De Marchi wrote: > 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: specially on display side... > > > > if (gen >= 11 || IS_PLATFORM) My goal is exactly to organize the gen numbers in a way that we minimize this mix as much as possible. > > > > 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 personally don't like it... but maybe it is just my bad english?! 1. if gen 9 2. if is gen 9 3. if this is a gen 9 platform I like more the option 1... > > > > 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? I don't like mixed styles much. If we don't kill the macro we will continue having mixed cases. So I'm in favor of the approach of this series here. Thanks, 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
