Re: [Intel-gfx] [PATCH v2 0/7] Make GEN macros more similar
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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 0/7] Make GEN macros more similar
On Tue, Nov 27, 2018 at 11:35:54AM +, 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 wrote: > > > > On Thu, Nov 22, 2018 at 08:54:30AM +, 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 +, Tvrtko Ursulin wrote: > > > > > > > > > > > > > > > > On 08/11/2018 00:57, Lucas De Marchi wrote: > > > > > > > > > On Wed, Nov 07, 2018 at 10:05:19AM +, 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() > > > > > > > > > > > - 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 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
Re: [Intel-gfx] [PATCH v2 0/7] Make GEN macros more similar
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 wrote: On Thu, Nov 22, 2018 at 08:54:30AM +, 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 +, Tvrtko Ursulin wrote: On 08/11/2018 00:57, Lucas De Marchi wrote: On Wed, Nov 07, 2018 at 10:05:19AM +, 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() - 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 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
Re: [Intel-gfx] [PATCH v2 0/7] Make GEN macros more similar
On Tue, Nov 27, 2018 at 10:37:21AM +0200, Jani Nikula wrote: > On Mon, 26 Nov 2018, Rodrigo Vivi wrote: > > On Thu, Nov 22, 2018 at 08:54:30AM +, 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 +, Tvrtko Ursulin wrote: > >> > > > > >> > > > On 08/11/2018 00:57, Lucas De Marchi wrote: > >> > > > > On Wed, Nov 07, 2018 at 10:05:19AM +, 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() > >> > > > > > > - 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 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
Re: [Intel-gfx] [PATCH v2 0/7] Make GEN macros more similar
On Mon, 26 Nov 2018, Rodrigo Vivi wrote: > On Thu, Nov 22, 2018 at 08:54:30AM +, 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 +, Tvrtko Ursulin wrote: >> > > > >> > > > On 08/11/2018 00:57, Lucas De Marchi wrote: >> > > > > On Wed, Nov 07, 2018 at 10:05:19AM +, 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() >> > > > > > > - 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 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 <
Re: [Intel-gfx] [PATCH v2 0/7] Make GEN macros more similar
On Thu, Nov 22, 2018 at 08:54:30AM +, 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 +, Tvrtko Ursulin wrote: > > > > > > > > On 08/11/2018 00:57, Lucas De Marchi wrote: > > > > > On Wed, Nov 07, 2018 at 10:05:19AM +, 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() > > > > > > > - 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 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. > > > > > > > > > > > > > > > > > >
Re: [Intel-gfx] [PATCH v2 0/7] Make GEN macros more similar
On Mon, 19 Nov 2018, Lucas De Marchi wrote: > On Thu, Nov 08, 2018 at 11:23:46AM +, 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. -- Jani Nikula, Intel Open Source Graphics Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 0/7] Make GEN macros more similar
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 +, Tvrtko Ursulin wrote: On 08/11/2018 00:57, Lucas De Marchi wrote: On Wed, Nov 07, 2018 at 10:05:19AM +, 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() - 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 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
Re: [Intel-gfx] [PATCH v2 0/7] Make GEN macros more similar
On Mon, Nov 19, 2018 at 02:20:55PM -0800, Lucas De Marchi wrote: > On Thu, Nov 08, 2018 at 11:23:46AM +, Tvrtko Ursulin wrote: > > > > On 08/11/2018 00:57, Lucas De Marchi wrote: > > > On Wed, Nov 07, 2018 at 10:05:19AM +, 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() > > > > > - 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 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;
Re: [Intel-gfx] [PATCH v2 0/7] Make GEN macros more similar
On 19/11/2018 22:20, Lucas De Marchi wrote: On Thu, Nov 08, 2018 at 11:23:46AM +, 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 0/7] Make GEN macros more similar
On Thu, Nov 08, 2018 at 11:23:46AM +, Tvrtko Ursulin wrote: > > On 08/11/2018 00:57, Lucas De Marchi wrote: > > On Wed, Nov 07, 2018 at 10:05:19AM +, 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() > > > > - 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 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 > >
Re: [Intel-gfx] [PATCH v2 0/7] Make GEN macros more similar
On 08/11/2018 00:57, Lucas De Marchi wrote: On Wed, Nov 07, 2018 at 10:05:19AM +, 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() - 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 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
Re: [Intel-gfx] [PATCH v2 0/7] Make GEN macros more similar
On Wed, Nov 07, 2018 at 10:05:19AM +, 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() > > - 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 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 +- > >
Re: [Intel-gfx] [PATCH v2 0/7] Make GEN macros more similar
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() - 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 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 |
[Intel-gfx] [PATCH v2 0/7] Make GEN macros more similar
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() - 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 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 drm/i915: merge gen checks to use range drm/i915: remove INTEL_GEN macro 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
