| Message ID | 20220309090913.2820533-1-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/regs: move GEN12_SFC_DONE_MAX where it belongs | expand |
On Wed, Mar 09, 2022 at 11:09:13AM +0200, Jani Nikula wrote: > Commit ce2fce2513c5 ("drm/i915: Only include i915_reg.h from .c files") > moved GEN12_SFC_DONE_MAX from i915_regs.h to i915_reg_defs.h. Arguably > it belongs next to the GEN12_SFC_DONE() definition, as it describes the > number of GEN12_SFC_DONE instances. It should probably be replaced with an I915_MAX_SFC define instead, since what we really care about is the number of SFC units rather than the number of registers; there's always one register per SFC unit. Or maybe even get rid of this completely and replace it with I915_MAX_VCS/2 since we expect there to be a 2:1:1 ratio of VCS:VECS:SFC. Matt > > The unfortunate downside is obviously having to pull in intel_gt_regs.h > in i915_gpu_error.h. > > Cc: Matt Roper <matthew.d.roper@intel.com> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > --- > > I guess the alternative is to define GEN12_SFC_DONE_MAX in > i915_gpu_error.h, to avoid the extra include. No matter what, IMO it's > really out of place in i915_reg_defs.h. > --- > drivers/gpu/drm/i915/gt/intel_gt_regs.h | 1 + > drivers/gpu/drm/i915/i915_gpu_error.h | 1 + > drivers/gpu/drm/i915/i915_reg_defs.h | 2 -- > 3 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h > index 19cd34f24263..6bb987fe3289 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h > @@ -1502,5 +1502,6 @@ > #define GEN12_CCS2_CCS3_INTR_MASK _MMIO(0x190104) > > #define GEN12_SFC_DONE(n) _MMIO(0x1cc000 + (n) * 0x1000) > +#define GEN12_SFC_DONE_MAX 4 > > #endif /* __INTEL_GT_REGS__ */ > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.h b/drivers/gpu/drm/i915/i915_gpu_error.h > index 903d838e2e63..d29155aca84c 100644 > --- a/drivers/gpu/drm/i915/i915_gpu_error.h > +++ b/drivers/gpu/drm/i915/i915_gpu_error.h > @@ -15,6 +15,7 @@ > #include <drm/drm_mm.h> > > #include "gt/intel_engine.h" > +#include "gt/intel_gt_regs.h" > #include "gt/intel_gt_types.h" > #include "gt/uc/intel_uc_fw.h" > > diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h b/drivers/gpu/drm/i915/i915_reg_defs.h > index d78d78fce431..8f486f77609f 100644 > --- a/drivers/gpu/drm/i915/i915_reg_defs.h > +++ b/drivers/gpu/drm/i915/i915_reg_defs.h > @@ -123,6 +123,4 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) > > #define VLV_DISPLAY_BASE 0x180000 > > -#define GEN12_SFC_DONE_MAX 4 > - > #endif /* __I915_REG_DEFS__ */ > -- > 2.30.2 >
On Wed, 09 Mar 2022, Matt Roper <matthew.d.roper@intel.com> wrote: > On Wed, Mar 09, 2022 at 11:09:13AM +0200, Jani Nikula wrote: >> Commit ce2fce2513c5 ("drm/i915: Only include i915_reg.h from .c files") >> moved GEN12_SFC_DONE_MAX from i915_regs.h to i915_reg_defs.h. Arguably >> it belongs next to the GEN12_SFC_DONE() definition, as it describes the >> number of GEN12_SFC_DONE instances. > > It should probably be replaced with an I915_MAX_SFC define instead, > since what we really care about is the number of SFC units rather than > the number of registers; there's always one register per SFC unit. Or > maybe even get rid of this completely and replace it with I915_MAX_VCS/2 > since we expect there to be a 2:1:1 ratio of VCS:VECS:SFC. Even if we used I915_MAX_VCS/2 I think adding a macro for it would be warranted. So... #define I915_MAX_SFC (I915_MAX_VCS / 2) in intel_engine_types.h or i915_gpu_error.h or where? BR, Jani. > > > Matt > >> >> The unfortunate downside is obviously having to pull in intel_gt_regs.h >> in i915_gpu_error.h. >> >> Cc: Matt Roper <matthew.d.roper@intel.com> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> >> --- >> >> I guess the alternative is to define GEN12_SFC_DONE_MAX in >> i915_gpu_error.h, to avoid the extra include. No matter what, IMO it's >> really out of place in i915_reg_defs.h. >> --- >> drivers/gpu/drm/i915/gt/intel_gt_regs.h | 1 + >> drivers/gpu/drm/i915/i915_gpu_error.h | 1 + >> drivers/gpu/drm/i915/i915_reg_defs.h | 2 -- >> 3 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h >> index 19cd34f24263..6bb987fe3289 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h >> +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h >> @@ -1502,5 +1502,6 @@ >> #define GEN12_CCS2_CCS3_INTR_MASK _MMIO(0x190104) >> >> #define GEN12_SFC_DONE(n) _MMIO(0x1cc000 + (n) * 0x1000) >> +#define GEN12_SFC_DONE_MAX 4 >> >> #endif /* __INTEL_GT_REGS__ */ >> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.h b/drivers/gpu/drm/i915/i915_gpu_error.h >> index 903d838e2e63..d29155aca84c 100644 >> --- a/drivers/gpu/drm/i915/i915_gpu_error.h >> +++ b/drivers/gpu/drm/i915/i915_gpu_error.h >> @@ -15,6 +15,7 @@ >> #include <drm/drm_mm.h> >> >> #include "gt/intel_engine.h" >> +#include "gt/intel_gt_regs.h" >> #include "gt/intel_gt_types.h" >> #include "gt/uc/intel_uc_fw.h" >> >> diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h b/drivers/gpu/drm/i915/i915_reg_defs.h >> index d78d78fce431..8f486f77609f 100644 >> --- a/drivers/gpu/drm/i915/i915_reg_defs.h >> +++ b/drivers/gpu/drm/i915/i915_reg_defs.h >> @@ -123,6 +123,4 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) >> >> #define VLV_DISPLAY_BASE 0x180000 >> >> -#define GEN12_SFC_DONE_MAX 4 >> - >> #endif /* __I915_REG_DEFS__ */ >> -- >> 2.30.2 >>
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h index 19cd34f24263..6bb987fe3289 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h @@ -1502,5 +1502,6 @@ #define GEN12_CCS2_CCS3_INTR_MASK _MMIO(0x190104) #define GEN12_SFC_DONE(n) _MMIO(0x1cc000 + (n) * 0x1000) +#define GEN12_SFC_DONE_MAX 4 #endif /* __INTEL_GT_REGS__ */ diff --git a/drivers/gpu/drm/i915/i915_gpu_error.h b/drivers/gpu/drm/i915/i915_gpu_error.h index 903d838e2e63..d29155aca84c 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.h +++ b/drivers/gpu/drm/i915/i915_gpu_error.h @@ -15,6 +15,7 @@ #include <drm/drm_mm.h> #include "gt/intel_engine.h" +#include "gt/intel_gt_regs.h" #include "gt/intel_gt_types.h" #include "gt/uc/intel_uc_fw.h" diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h b/drivers/gpu/drm/i915/i915_reg_defs.h index d78d78fce431..8f486f77609f 100644 --- a/drivers/gpu/drm/i915/i915_reg_defs.h +++ b/drivers/gpu/drm/i915/i915_reg_defs.h @@ -123,6 +123,4 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) #define VLV_DISPLAY_BASE 0x180000 -#define GEN12_SFC_DONE_MAX 4 - #endif /* __I915_REG_DEFS__ */
Commit ce2fce2513c5 ("drm/i915: Only include i915_reg.h from .c files") moved GEN12_SFC_DONE_MAX from i915_regs.h to i915_reg_defs.h. Arguably it belongs next to the GEN12_SFC_DONE() definition, as it describes the number of GEN12_SFC_DONE instances. The unfortunate downside is obviously having to pull in intel_gt_regs.h in i915_gpu_error.h. Cc: Matt Roper <matthew.d.roper@intel.com> Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- I guess the alternative is to define GEN12_SFC_DONE_MAX in i915_gpu_error.h, to avoid the extra include. No matter what, IMO it's really out of place in i915_reg_defs.h. --- drivers/gpu/drm/i915/gt/intel_gt_regs.h | 1 + drivers/gpu/drm/i915/i915_gpu_error.h | 1 + drivers/gpu/drm/i915/i915_reg_defs.h | 2 -- 3 files changed, 2 insertions(+), 2 deletions(-)