| Message ID | 20181106215123.27568-1-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Make GEN macros more similar | expand |
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. > 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. 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!?) 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(-) >
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? 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. 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) 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) > 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. > > 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(-) > >
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(-) >>> >
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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 19/11/2018 22:20, Lucas De Marchi wrote: > On Thu, Nov 08, 2018 at 11:23:46AM +0000, Tvrtko Ursulin wrote: [snip] >>> 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. I forgot about the per-SKU builds when replying on this point. So I think we need to add them as the benefit of using range masks throughout. Question of whether people can stomach the ugliness still remains. But at least for me personally, I think if we want to get to per-SKU builds one day, then we'll have to do 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). We agreed there that gen in runtime should be avoided if at all possible. It seems that the consensus is to have macros to access device info, whether the current, or two flavours of it in the future. So I think only the question of naming remains on this item. Regards, Tvrtko
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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 21/11/2018 22:19, Rodrigo Vivi wrote: > 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. Including the removal of INTEL_GEN macro? Or what do you propose for that one? Can't be called GT_GEN now since that has been used up... Regards, Tvrtko
On Mon, 19 Nov 2018, Lucas De Marchi <lucas.de.marchi@gmail.com> wrote: > On Thu, Nov 08, 2018 at 11:23:46AM +0000, Tvrtko Ursulin wrote: >> 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? Please hold until all aspects have been discussed. Thanks, Jani.
On Thu, Nov 22, 2018 at 08:54:30AM +0000, Tvrtko Ursulin wrote: > > > On 21/11/2018 22:19, Rodrigo Vivi wrote: > > 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. > > Including the removal of INTEL_GEN macro? Or what do you propose for that > one? Can't be called GT_GEN now since that has been used up... Yes, including the removal of INTEL_GEN macro. I don't like the mixed styles like using INTEL_GEN(d) > 7 in one place and INTEL_GEN_RANGE(d, 7, FOREVER) in another place. The meaning is the same so we should stick with one of the usages. I see that there were many cases where having the info->gen number directly is useful. But I'd prefer to use that on a direct fashion rather than keeping this macro. Because if we keep the macro developers will eventually end up adding the mixed style back. Using direct info->gen as Lucas already showed has almost same number of chars than the macro, but requires more attention what is a good thing. > > Regards, > > Tvrtko
On Mon, 26 Nov 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > On Thu, Nov 22, 2018 at 08:54:30AM +0000, Tvrtko Ursulin wrote: >> >> >> On 21/11/2018 22:19, Rodrigo Vivi wrote: >> > 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. >> >> Including the removal of INTEL_GEN macro? Or what do you propose for that >> one? Can't be called GT_GEN now since that has been used up... > > Yes, including the removal of INTEL_GEN macro. > > I don't like the mixed styles like using INTEL_GEN(d) > 7 in one place > and INTEL_GEN_RANGE(d, 7, FOREVER) in another place. > > The meaning is the same so we should stick with one of the usages. > > I see that there were many cases where having the info->gen number > directly is useful. But I'd prefer to use that on a direct fashion > rather than keeping this macro. > > Because if we keep the macro developers will eventually end up > adding the mixed style back. > > Using direct info->gen as Lucas already showed has almost same number > of chars than the macro, but requires more attention what is a good > thing. I prefer using INTEL_GEN() because it hides where the gen comes from, and we can change it on a whim. If we keep using dev_priv->info.gen we'll need to change that already when info becomes a pointer, i.e. dev_priv->info->gen. Throughout the codebase. With INTEL_GEN() it's just a couple of places. If mixed use is a problem, then rename gen to __gen. I even sent a patch for it, but didn't get it merged. BR, Jani.
On Tue, Nov 27, 2018 at 10:37:21AM +0200, Jani Nikula wrote: > On Mon, 26 Nov 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > > On Thu, Nov 22, 2018 at 08:54:30AM +0000, Tvrtko Ursulin wrote: > >> > >> > >> On 21/11/2018 22:19, Rodrigo Vivi wrote: > >> > 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. > >> > >> Including the removal of INTEL_GEN macro? Or what do you propose for that > >> one? Can't be called GT_GEN now since that has been used up... > > > > Yes, including the removal of INTEL_GEN macro. > > > > I don't like the mixed styles like using INTEL_GEN(d) > 7 in one place > > and INTEL_GEN_RANGE(d, 7, FOREVER) in another place. > > > > The meaning is the same so we should stick with one of the usages. > > > > I see that there were many cases where having the info->gen number > > directly is useful. But I'd prefer to use that on a direct fashion > > rather than keeping this macro. > > > > Because if we keep the macro developers will eventually end up > > adding the mixed style back. > > > > Using direct info->gen as Lucas already showed has almost same number > > of chars than the macro, but requires more attention what is a good > > thing. > > I prefer using INTEL_GEN() because it hides where the gen comes from, > and we can change it on a whim. If we keep using dev_priv->info.gen > we'll need to change that already when info becomes a pointer, > i.e. dev_priv->info->gen. Throughout the codebase. With INTEL_GEN() it's > just a couple of places. patch 7 actually does: - INTEL_GEN(E) + INTEL_INFO(E)->gen So still using the macro and if info becomes a pointer it would still be correct. My main motivation for removing it is not INTEL_GEN() itself, but because that would force us to keep the IS_ prefix in the other macros. IS_GT_GEN_RANGE is too big and ugly IMO. If my previous proposal of using a single macro for both range and single gen would be accepted, I think keeping the IS_ prefix would not be so ugly. Anyway, considering the addition of display macros, do you think it should be like below? IS_GEN -> IS_GT_GEN() `> IS_GT_GEN_RANGE() (dialing down on the changes as suggested by Tvrtko to preserve plain if ladders and simple unbound ranges) IS_GEN<N> -> IS_GT_GEN(..., N) INTEL_GEN -> GT_GEN() IS_DISPLAY_GEN() IS_DISPLAY_GEN_RANGE() DISPLAY_GEN() Other option: just do the IS_GEN<N> IS_GEN(..., N), that was the original motivation for this series, and remember that gen here refers to GT. Lucas De Marchi > > If mixed use is a problem, then rename gen to __gen. I even sent a patch > for it, but didn't get it merged. > > BR, > Jani. > > > -- > Jani Nikula, Intel Open Source Graphics Center
On 27/11/2018 09:36, Lucas De Marchi wrote: > On Tue, Nov 27, 2018 at 10:37:21AM +0200, Jani Nikula wrote: >> On Mon, 26 Nov 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: >>> On Thu, Nov 22, 2018 at 08:54:30AM +0000, Tvrtko Ursulin wrote: >>>> >>>> >>>> On 21/11/2018 22:19, Rodrigo Vivi wrote: >>>>> 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. >>>> >>>> Including the removal of INTEL_GEN macro? Or what do you propose for that >>>> one? Can't be called GT_GEN now since that has been used up... >>> >>> Yes, including the removal of INTEL_GEN macro. >>> >>> I don't like the mixed styles like using INTEL_GEN(d) > 7 in one place >>> and INTEL_GEN_RANGE(d, 7, FOREVER) in another place. >>> >>> The meaning is the same so we should stick with one of the usages. >>> >>> I see that there were many cases where having the info->gen number >>> directly is useful. But I'd prefer to use that on a direct fashion >>> rather than keeping this macro. >>> >>> Because if we keep the macro developers will eventually end up >>> adding the mixed style back. >>> >>> Using direct info->gen as Lucas already showed has almost same number >>> of chars than the macro, but requires more attention what is a good >>> thing. >> >> I prefer using INTEL_GEN() because it hides where the gen comes from, >> and we can change it on a whim. If we keep using dev_priv->info.gen >> we'll need to change that already when info becomes a pointer, >> i.e. dev_priv->info->gen. Throughout the codebase. With INTEL_GEN() it's >> just a couple of places. > > patch 7 actually does: > > - INTEL_GEN(E) > + INTEL_INFO(E)->gen > > So still using the macro and if info becomes a pointer it would still be > correct. My main motivation for removing it is not INTEL_GEN() itself, > but because that would force us to keep the IS_ prefix in the other > macros. IS_GT_GEN_RANGE is too big and ugly IMO. If my > previous proposal of using a single macro for both range and single gen > would be accepted, I think keeping the IS_ prefix would not be so ugly. > > Anyway, considering the addition of display macros, do you think it should > be like below? > > IS_GEN -> IS_GT_GEN() > `> IS_GT_GEN_RANGE() (dialing down on the changes as suggested by > Tvrtko to preserve plain if ladders and > simple unbound ranges) More range macro usage will help future per SKU builds work. So as said in my previous reply, if people can stomach this, I am okay with having this conversion as well. (From arithmetic gen comparison to ...GEN_RANGE everywhere.) That would also makes the question of INTEL_GEN mostly irrelevant. So I think above is the main point to clarify first. Then on the question of IS_ prefix or not, I don't feel very strongly about it. IS_ has a nice parallel with HAS_ and IS_platform, but I agree it doesn't look the prettiest (IS_GT_GEN). So don't know, whatever the vote ends up being. Regards, Tvrtko > IS_GEN<N> -> IS_GT_GEN(..., N) > INTEL_GEN -> GT_GEN() > > IS_DISPLAY_GEN() > IS_DISPLAY_GEN_RANGE() > DISPLAY_GEN() > > > Other option: just do the IS_GEN<N> IS_GEN(..., N), > that was the original motivation for this series, and remember that > gen here refers to GT. > > Lucas De Marchi > >> >> If mixed use is a problem, then rename gen to __gen. I even sent a patch >> for it, but didn't get it merged. >> >> BR, >> Jani. >> >> >> -- >> Jani Nikula, Intel Open Source Graphics Center >
On Tue, Nov 27, 2018 at 11:35:54AM +0000, Tvrtko Ursulin wrote: > > On 27/11/2018 09:36, Lucas De Marchi wrote: > > On Tue, Nov 27, 2018 at 10:37:21AM +0200, Jani Nikula wrote: > > > On Mon, 26 Nov 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > > > > On Thu, Nov 22, 2018 at 08:54:30AM +0000, Tvrtko Ursulin wrote: > > > > > > > > > > > > > > > On 21/11/2018 22:19, Rodrigo Vivi wrote: > > > > > > 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. > > > > > > > > > > Including the removal of INTEL_GEN macro? Or what do you propose for that > > > > > one? Can't be called GT_GEN now since that has been used up... > > > > > > > > Yes, including the removal of INTEL_GEN macro. > > > > > > > > I don't like the mixed styles like using INTEL_GEN(d) > 7 in one place > > > > and INTEL_GEN_RANGE(d, 7, FOREVER) in another place. > > > > > > > > The meaning is the same so we should stick with one of the usages. > > > > > > > > I see that there were many cases where having the info->gen number > > > > directly is useful. But I'd prefer to use that on a direct fashion > > > > rather than keeping this macro. > > > > > > > > Because if we keep the macro developers will eventually end up > > > > adding the mixed style back. > > > > > > > > Using direct info->gen as Lucas already showed has almost same number > > > > of chars than the macro, but requires more attention what is a good > > > > thing. > > > > > > I prefer using INTEL_GEN() because it hides where the gen comes from, > > > and we can change it on a whim. If we keep using dev_priv->info.gen > > > we'll need to change that already when info becomes a pointer, > > > i.e. dev_priv->info->gen. Throughout the codebase. With INTEL_GEN() it's > > > just a couple of places. > > > > patch 7 actually does: > > > > - INTEL_GEN(E) > > + INTEL_INFO(E)->gen > > > > So still using the macro and if info becomes a pointer it would still be > > correct. My main motivation for removing it is not INTEL_GEN() itself, > > but because that would force us to keep the IS_ prefix in the other > > macros. IS_GT_GEN_RANGE is too big and ugly IMO. If my > > previous proposal of using a single macro for both range and single gen > > would be accepted, I think keeping the IS_ prefix would not be so ugly. > > > > Anyway, considering the addition of display macros, do you think it should > > be like below? > > > > IS_GEN -> IS_GT_GEN() > > `> IS_GT_GEN_RANGE() (dialing down on the changes as suggested by > > Tvrtko to preserve plain if ladders and > > simple unbound ranges) > > More range macro usage will help future per SKU builds work. So as said in > my previous reply, if people can stomach this, I am okay with having this > conversion as well. (From arithmetic gen comparison to ...GEN_RANGE > everywhere.) > > That would also makes the question of INTEL_GEN mostly irrelevant. So I > think above is the main point to clarify first. > > Then on the question of IS_ prefix or not, I don't feel very strongly about > it. IS_ has a nice parallel with HAS_ and IS_platform, but I agree it > doesn't look the prettiest (IS_GT_GEN). So don't know, whatever the vote > ends up being. okay, the HAS_ parallel is a good point... but still in that case my brain prefers if HAS_FEATURE than if FEATURE because "FEATURE what?" Like if feature was more "transitive" requiring something else. while for "is" my brain prefers if PLATFORM than if IS_PLATFORM because here it seems more "intransitive"... like... self contained meaning where "is" can be implicit. > > Regards, > > Tvrtko > > > IS_GEN<N> -> IS_GT_GEN(..., N) > > INTEL_GEN -> GT_GEN() > > > > IS_DISPLAY_GEN() > > IS_DISPLAY_GEN_RANGE() > > DISPLAY_GEN() > > > > > > Other option: just do the IS_GEN<N> IS_GEN(..., N), > > that was the original motivation for this series, and remember that > > gen here refers to GT. > > > > Lucas De Marchi > > > > > > > > If mixed use is a problem, then rename gen to __gen. I even sent a patch > > > for it, but didn't get it merged. > > > > > > BR, > > > Jani. > > > > > > > > > -- > > > Jani Nikula, Intel Open Source Graphics Center > >
On Tue, Nov 27, 2018 at 04:19:23PM -0800, Rodrigo Vivi wrote: > > Then on the question of IS_ prefix or not, I don't feel very strongly about > > it. IS_ has a nice parallel with HAS_ and IS_platform, but I agree it > > doesn't look the prettiest (IS_GT_GEN). So don't know, whatever the vote > > ends up being. > > okay, the HAS_ parallel is a good point... > > but still in that case my brain prefers > > if HAS_FEATURE > than > if FEATURE > > because "FEATURE what?" Like if feature was more "transitive" requiring something else. > > while for "is" my brain prefers > > if PLATFORM > than > if IS_PLATFORM > > because here it seems more "intransitive"... > like... self contained meaning where "is" can be implicit. for me both IS_PLATFORM and PLATFORM make sense. IS_ prefix is used in several other places for things like that. I just don't like the outcome of having it: gigantic horrendous macros like IS_GT_GEN_RANGE(). Lucas De Marchi