[Intel-gfx] [PATCH 2/9] drm/i915: Add function to set SSEU info per platform
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Wed Jul 24 12:50:15 UTC 2019
On 23/07/2019 16:49, Stuart Summers wrote:
> Add a new function to allow each platform to set maximum
> slice, subslice, and EU information to reduce code duplication.
>
> Signed-off-by: Stuart Summers <stuart.summers at intel.com>
> ---
> drivers/gpu/drm/i915/gt/intel_sseu.c | 8 +++++
> drivers/gpu/drm/i915/gt/intel_sseu.h | 3 ++
> drivers/gpu/drm/i915/i915_debugfs.c | 6 ++--
> drivers/gpu/drm/i915/intel_device_info.c | 39 +++++++++---------------
> 4 files changed, 28 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_sseu.c b/drivers/gpu/drm/i915/gt/intel_sseu.c
> index a0756f006f5f..08b74ae40739 100644
> --- a/drivers/gpu/drm/i915/gt/intel_sseu.c
> +++ b/drivers/gpu/drm/i915/gt/intel_sseu.c
> @@ -8,6 +8,14 @@
> #include "intel_lrc_reg.h"
> #include "intel_sseu.h"
>
> +void intel_sseu_set_info(struct sseu_dev_info *sseu, u8 max_slices,
> + u8 max_subslices, u8 max_eus_per_subslice)
> +{
> + sseu->max_slices = max_slices;
> + sseu->max_subslices = max_subslices;
> + sseu->max_eus_per_subslice = max_eus_per_subslice;
> +}
Static inline in header since it is so trivial?
Is the term "info" established in this space? I am thinking if just
intel_sseu_set would be enough? Or set_topology? Although it is not a
full topology.. don't know.
> +
> unsigned int
> intel_sseu_subslice_total(const struct sseu_dev_info *sseu)
> {
> diff --git a/drivers/gpu/drm/i915/gt/intel_sseu.h b/drivers/gpu/drm/i915/gt/intel_sseu.h
> index b50d0401a4e2..64e47dad07be 100644
> --- a/drivers/gpu/drm/i915/gt/intel_sseu.h
> +++ b/drivers/gpu/drm/i915/gt/intel_sseu.h
> @@ -63,6 +63,9 @@ intel_sseu_from_device_info(const struct sseu_dev_info *sseu)
> return value;
> }
>
> +void intel_sseu_set_info(struct sseu_dev_info *sseu, u8 max_slices,
> + u8 max_subslices, u8 max_eus_per_subslice);
> +
> unsigned int
> intel_sseu_subslice_total(const struct sseu_dev_info *sseu);
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 14eadab4209b..011ed2fd391d 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4034,9 +4034,9 @@ static int i915_sseu_status(struct seq_file *m, void *unused)
>
> seq_puts(m, "SSEU Device Status\n");
> memset(&sseu, 0, sizeof(sseu));
> - sseu.max_slices = info->sseu.max_slices;
> - sseu.max_subslices = info->sseu.max_subslices;
> - sseu.max_eus_per_subslice = info->sseu.max_eus_per_subslice;
> + intel_sseu_set_info(&sseu, info->sseu.max_slices,
> + info->sseu.max_subslices,
> + info->sseu.max_eus_per_subslice);
>
> with_intel_runtime_pm(&dev_priv->runtime_pm, wakeref) {
> if (IS_CHERRYVIEW(dev_priv))
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> index f99c9fd497b2..9a79d9d547c5 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -191,15 +191,10 @@ static void gen11_sseu_info_init(struct drm_i915_private *dev_priv)
> u8 eu_en;
> int s;
>
> - if (IS_ELKHARTLAKE(dev_priv)) {
> - sseu->max_slices = 1;
> - sseu->max_subslices = 4;
> - sseu->max_eus_per_subslice = 8;
> - } else {
> - sseu->max_slices = 1;
> - sseu->max_subslices = 8;
> - sseu->max_eus_per_subslice = 8;
> - }
> + if (IS_ELKHARTLAKE(dev_priv))
> + intel_sseu_set_info(sseu, 1, 4, 8);
> + else
> + intel_sseu_set_info(sseu, 1, 8, 8);
>
> s_en = I915_READ(GEN11_GT_SLICE_ENABLE) & GEN11_GT_S_ENA_MASK;
> ss_en = ~I915_READ(GEN11_GT_SUBSLICE_DISABLE);
> @@ -236,11 +231,10 @@ static void gen10_sseu_info_init(struct drm_i915_private *dev_priv)
> const int eu_mask = 0xff;
> u32 subslice_mask, eu_en;
>
> + intel_sseu_set_info(sseu, 6, 4, 8);
> +
> sseu->slice_mask = (fuse2 & GEN10_F2_S_ENA_MASK) >>
> GEN10_F2_S_ENA_SHIFT;
Right, so intel_sseu_set_info sets some bits but not all. I guess
topology is then definitely the wrong name. intel_sseu_set_sseu is what
it does :), so the final suffix might be redundant be it info or sseu.
As you prefer.
> - sseu->max_slices = 6;
> - sseu->max_subslices = 4;
> - sseu->max_eus_per_subslice = 8;
>
> subslice_mask = (1 << 4) - 1;
> subslice_mask &= ~((fuse2 & GEN10_F2_SS_DIS_MASK) >>
> @@ -314,9 +308,7 @@ static void cherryview_sseu_info_init(struct drm_i915_private *dev_priv)
> fuse = I915_READ(CHV_FUSE_GT);
>
> sseu->slice_mask = BIT(0);
> - sseu->max_slices = 1;
> - sseu->max_subslices = 2;
> - sseu->max_eus_per_subslice = 8;
> + intel_sseu_set_info(sseu, 1, 2, 8);
>
> if (!(fuse & CHV_FGT_DISABLE_SS0)) {
> u8 disabled_mask =
> @@ -372,9 +364,8 @@ static void gen9_sseu_info_init(struct drm_i915_private *dev_priv)
> sseu->slice_mask = (fuse2 & GEN8_F2_S_ENA_MASK) >> GEN8_F2_S_ENA_SHIFT;
>
> /* BXT has a single slice and at most 3 subslices. */
> - sseu->max_slices = IS_GEN9_LP(dev_priv) ? 1 : 3;
> - sseu->max_subslices = IS_GEN9_LP(dev_priv) ? 3 : 4;
> - sseu->max_eus_per_subslice = 8;
> + intel_sseu_set_info(sseu, IS_GEN9_LP(dev_priv) ? 1 : 3,
> + IS_GEN9_LP(dev_priv) ? 3 : 4, 8);
So far it all improved readability but not here. I think here
if (IS_GEN9_LP(dev_priv))
intel_sseu_set(...);
else
intel_sseu_set(...);
Would be cleaner.
>
> /*
> * The subslice disable field is global, i.e. it applies
> @@ -473,9 +464,7 @@ static void broadwell_sseu_info_init(struct drm_i915_private *dev_priv)
>
> fuse2 = I915_READ(GEN8_FUSE2);
> sseu->slice_mask = (fuse2 & GEN8_F2_S_ENA_MASK) >> GEN8_F2_S_ENA_SHIFT;
> - sseu->max_slices = 3;
> - sseu->max_subslices = 3;
> - sseu->max_eus_per_subslice = 8;
> + intel_sseu_set_info(sseu, 3, 3, 8);
>
> /*
> * The subslice disable field is global, i.e. it applies
> @@ -577,9 +566,6 @@ static void haswell_sseu_info_init(struct drm_i915_private *dev_priv)
> break;
> }
>
> - sseu->max_slices = hweight8(sseu->slice_mask);
> - sseu->max_subslices = hweight8(sseu->subslice_mask[0]);
> -
> fuse1 = I915_READ(HSW_PAVP_FUSE1);
> switch ((fuse1 & HSW_F1_EU_DIS_MASK) >> HSW_F1_EU_DIS_SHIFT) {
> default:
> @@ -596,7 +582,10 @@ static void haswell_sseu_info_init(struct drm_i915_private *dev_priv)
> sseu->eu_per_subslice = 6;
> break;
> }
> - sseu->max_eus_per_subslice = sseu->eu_per_subslice;
> +
> + intel_sseu_set_info(sseu, hweight8(sseu->slice_mask),
> + hweight8(sseu->subslice_mask[0]),
> + sseu->eu_per_subslice);
>
> for (s = 0; s < sseu->max_slices; s++) {
> for (ss = 0; ss < sseu->max_subslices; ss++) {
>
Overall looks like a readability improvement. Some tweaks or not as
mentioned above can be discussed.
Regards,
Tvrtko
More information about the Intel-gfx
mailing list