| Message ID | 20190122051227.8329-5-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Define MOCS table for Icelake | expand |
Quoting Lucas De Marchi (2019-01-22 05:12:24) > Let's use a macro to make tables smaller and at the same time allow us > to add fields that apply to all entries in future. > > For the sake of readability, I'm calling an exception on 80 chars limit. > Lines are aligned for easy comparison of the entry values. > + MOCS_ENTRY(I915_MOCS_UNCACHED, LE_1_UC | LE_TC_2_LLC_ELLC, \ > + L3_1_UC), \ MOCS_ENTRY(I915_MOCS_UNCACHED, LE_1_UC | LE_TC_2_LLC_ELLC, L3_1_UC), \ is even more readable with the visual clustering of attribute flags. -Chris
On Tue, Jan 22, 2019 at 6:32 AM Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Lucas De Marchi (2019-01-22 05:12:24) > > Let's use a macro to make tables smaller and at the same time allow us > > to add fields that apply to all entries in future. > > > > For the sake of readability, I'm calling an exception on 80 chars limit. > > Lines are aligned for easy comparison of the entry values. > > > + MOCS_ENTRY(I915_MOCS_UNCACHED, LE_1_UC | LE_TC_2_LLC_ELLC, \ > > + L3_1_UC), \ > > MOCS_ENTRY(I915_MOCS_UNCACHED, > LE_1_UC | LE_TC_2_LLC_ELLC, L3_1_UC), \ My intention was to split the lines for each *value*, so it's easy to see what control_value vs l3cc_value is set to (too difficult to spot mistakes on adding a comma rather than a |). But I'm not strongly against your version, so I'll switch to that. thanks Lucas De Marchi > > is even more readable with the visual clustering of attribute flags. > -Chris > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Lucas De Marchi (2019-01-22 21:33:25) > On Tue, Jan 22, 2019 at 6:32 AM Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > Quoting Lucas De Marchi (2019-01-22 05:12:24) > > > Let's use a macro to make tables smaller and at the same time allow us > > > to add fields that apply to all entries in future. > > > > > > For the sake of readability, I'm calling an exception on 80 chars limit. > > > Lines are aligned for easy comparison of the entry values. > > > > > + MOCS_ENTRY(I915_MOCS_UNCACHED, LE_1_UC | LE_TC_2_LLC_ELLC, \ > > > + L3_1_UC), \ > > > > MOCS_ENTRY(I915_MOCS_UNCACHED, > > LE_1_UC | LE_TC_2_LLC_ELLC, L3_1_UC), \ > > My intention was to split the lines for each *value*, so it's easy to > see what control_value vs l3cc_value is set to > (too difficult to spot mistakes on adding a comma rather than a |). > > But I'm not strongly against your version, so I'll switch to that. Have another new line :) Because you are right as I confused that \ for a |. MOCS_ENTRY(I915_MOCS_UNCACHED, \ LE_1_UC | LE_TC_2_LLC_ELLC, \ L3_1_UC), \ -Chris
On 2019-01-22 06:12, Lucas De Marchi wrote: > Let's use a macro to make tables smaller and at the same time allow us > to add fields that apply to all entries in future. > > For the sake of readability, I'm calling an exception on 80 chars limit. > Lines are aligned for easy comparison of the entry values. > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> Reviewed-by: Tomasz Lis <tomasz.lis@intel.com> > --- > drivers/gpu/drm/i915/intel_mocs.c | 39 +++++++++++-------------------- > 1 file changed, 14 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c > index c7a2a8d81d90..faae2eefc5cc 100644 > --- a/drivers/gpu/drm/i915/intel_mocs.c > +++ b/drivers/gpu/drm/i915/intel_mocs.c > @@ -71,6 +71,12 @@ struct drm_i915_mocs_table { > #define L3_2_RESERVED _L3_CACHEABILITY(2) > #define L3_3_WB _L3_CACHEABILITY(3) > > +#define MOCS_ENTRY(__idx, __control_value, __l3cc_value) \ > + [__idx] = { \ > + .control_value = __control_value, \ > + .l3cc_value = __l3cc_value, \ > + } > + > /* > * MOCS tables > * > @@ -93,40 +99,23 @@ struct drm_i915_mocs_table { > * may only be updated incrementally by adding entries at the > * end. > */ > - The idea behind this EOL was that the comment above relates to several statements below, not just the first one. But I'm not really sure what our commenting rules are in this case - a comment which bundles several definitions. -Tomasz > #define GEN9_MOCS_ENTRIES \ > - [I915_MOCS_UNCACHED] = { \ > - /* 0x00000009 */ \ > - .control_value = LE_1_UC | LE_TC_2_LLC_ELLC, \ > - /* 0x0010 */ \ > - .l3cc_value = L3_1_UC, \ > - }, \ > - [I915_MOCS_PTE] = { \ > - /* 0x00000038 */ \ > - .control_value = LE_0_PAGETABLE | LE_TC_2_LLC_ELLC | LE_LRUM(3), \ > - /* 0x0030 */ \ > - .l3cc_value = L3_3_WB, \ > - } > + MOCS_ENTRY(I915_MOCS_UNCACHED, LE_1_UC | LE_TC_2_LLC_ELLC, \ > + L3_1_UC), \ > + MOCS_ENTRY(I915_MOCS_PTE, LE_0_PAGETABLE | LE_TC_2_LLC_ELLC | LE_LRUM(3), \ > + L3_3_WB) \ > > static const struct drm_i915_mocs_entry skylake_mocs_table[] = { > GEN9_MOCS_ENTRIES, > - [I915_MOCS_CACHED] = { > - /* 0x0000003b */ > - .control_value = LE_3_WB | LE_TC_2_LLC_ELLC | LE_LRUM(3), > - /* 0x0030 */ > - .l3cc_value = L3_3_WB, > - }, > + MOCS_ENTRY(I915_MOCS_CACHED, LE_3_WB | LE_TC_2_LLC_ELLC | LE_LRUM(3), > + L3_3_WB) > }; > > /* NOTE: the LE_TGT_CACHE is not used on Broxton */ > static const struct drm_i915_mocs_entry broxton_mocs_table[] = { > GEN9_MOCS_ENTRIES, > - [I915_MOCS_CACHED] = { > - /* 0x00000039 */ > - .control_value = LE_1_UC | LE_TC_2_LLC_ELLC | LE_LRUM(3), > - /* 0x0030 */ > - .l3cc_value = L3_3_WB, > - }, > + MOCS_ENTRY(I915_MOCS_CACHED, LE_1_UC | LE_TC_2_LLC_ELLC | LE_LRUM(3), > + L3_3_WB) > }; > > /**
On Tue, Jan 22, 2019 at 09:37:02PM +0000, Chris Wilson wrote: >Quoting Lucas De Marchi (2019-01-22 21:33:25) >> On Tue, Jan 22, 2019 at 6:32 AM Chris Wilson <chris@chris-wilson.co.uk> wrote: >> > >> > Quoting Lucas De Marchi (2019-01-22 05:12:24) >> > > Let's use a macro to make tables smaller and at the same time allow us >> > > to add fields that apply to all entries in future. >> > > >> > > For the sake of readability, I'm calling an exception on 80 chars limit. >> > > Lines are aligned for easy comparison of the entry values. >> > >> > > + MOCS_ENTRY(I915_MOCS_UNCACHED, LE_1_UC | LE_TC_2_LLC_ELLC, \ >> > > + L3_1_UC), \ >> > >> > MOCS_ENTRY(I915_MOCS_UNCACHED, >> > LE_1_UC | LE_TC_2_LLC_ELLC, L3_1_UC), \ >> >> My intention was to split the lines for each *value*, so it's easy to >> see what control_value vs l3cc_value is set to >> (too difficult to spot mistakes on adding a comma rather than a |). >> >> But I'm not strongly against your version, so I'll switch to that. > >Have another new line :) > >Because you are right as I confused that \ for a |. > > MOCS_ENTRY(I915_MOCS_UNCACHED, \ > LE_1_UC | LE_TC_2_LLC_ELLC, \ > L3_1_UC), \ ok. The Ice Lake table is huge (see last patch) and this will mandate 3 lines per entry, but at least it will be clear. Lucas De Marchi > >-Chris
diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c index c7a2a8d81d90..faae2eefc5cc 100644 --- a/drivers/gpu/drm/i915/intel_mocs.c +++ b/drivers/gpu/drm/i915/intel_mocs.c @@ -71,6 +71,12 @@ struct drm_i915_mocs_table { #define L3_2_RESERVED _L3_CACHEABILITY(2) #define L3_3_WB _L3_CACHEABILITY(3) +#define MOCS_ENTRY(__idx, __control_value, __l3cc_value) \ + [__idx] = { \ + .control_value = __control_value, \ + .l3cc_value = __l3cc_value, \ + } + /* * MOCS tables * @@ -93,40 +99,23 @@ struct drm_i915_mocs_table { * may only be updated incrementally by adding entries at the * end. */ - #define GEN9_MOCS_ENTRIES \ - [I915_MOCS_UNCACHED] = { \ - /* 0x00000009 */ \ - .control_value = LE_1_UC | LE_TC_2_LLC_ELLC, \ - /* 0x0010 */ \ - .l3cc_value = L3_1_UC, \ - }, \ - [I915_MOCS_PTE] = { \ - /* 0x00000038 */ \ - .control_value = LE_0_PAGETABLE | LE_TC_2_LLC_ELLC | LE_LRUM(3), \ - /* 0x0030 */ \ - .l3cc_value = L3_3_WB, \ - } + MOCS_ENTRY(I915_MOCS_UNCACHED, LE_1_UC | LE_TC_2_LLC_ELLC, \ + L3_1_UC), \ + MOCS_ENTRY(I915_MOCS_PTE, LE_0_PAGETABLE | LE_TC_2_LLC_ELLC | LE_LRUM(3), \ + L3_3_WB) \ static const struct drm_i915_mocs_entry skylake_mocs_table[] = { GEN9_MOCS_ENTRIES, - [I915_MOCS_CACHED] = { - /* 0x0000003b */ - .control_value = LE_3_WB | LE_TC_2_LLC_ELLC | LE_LRUM(3), - /* 0x0030 */ - .l3cc_value = L3_3_WB, - }, + MOCS_ENTRY(I915_MOCS_CACHED, LE_3_WB | LE_TC_2_LLC_ELLC | LE_LRUM(3), + L3_3_WB) }; /* NOTE: the LE_TGT_CACHE is not used on Broxton */ static const struct drm_i915_mocs_entry broxton_mocs_table[] = { GEN9_MOCS_ENTRIES, - [I915_MOCS_CACHED] = { - /* 0x00000039 */ - .control_value = LE_1_UC | LE_TC_2_LLC_ELLC | LE_LRUM(3), - /* 0x0030 */ - .l3cc_value = L3_3_WB, - }, + MOCS_ENTRY(I915_MOCS_CACHED, LE_1_UC | LE_TC_2_LLC_ELLC | LE_LRUM(3), + L3_3_WB) }; /**
Let's use a macro to make tables smaller and at the same time allow us to add fields that apply to all entries in future. For the sake of readability, I'm calling an exception on 80 chars limit. Lines are aligned for easy comparison of the entry values. Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- drivers/gpu/drm/i915/intel_mocs.c | 39 +++++++++++-------------------- 1 file changed, 14 insertions(+), 25 deletions(-)