| Message ID | 20190207162953.158854-3-bob.j.paauwe@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915: Make 48bit full ppgtt configuration generic (v11) | expand |
Quoting Bob Paauwe (2019-02-07 16:29:53) > With the address range being specified for each platform, we can use > that instead of the .ppgtt enum to handle the differences between > 3 level and 4 level PPGTT. In most cases, we really only care if the > platform supports PPGTT or not. Because of this, we can now remove > the HAS_FULL_PPGTT macro and the device info ppgtt field. > > Aliasing PPGTT used by GEN 6 is a bit of an exception. For those cases, > it makes just as much sense to check if we're running on GEN 6 as it > does to check a device info flag. > > v2: Reword the commit message to make it correct wrt aliasing ppgtt (Chris) > v3: Rebase on current drm-tip The point of adding the type into the device_info was that it was included in the error state so that I didn't have to remember which w/a applied to which gen. gen6 has full-ppgtt, we can't enabled it as no one has solved how to make it work. -Chris
On Thu, 7 Feb 2019 16:41:58 +0000 Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Bob Paauwe (2019-02-07 16:29:53) > > With the address range being specified for each platform, we can use > > that instead of the .ppgtt enum to handle the differences between > > 3 level and 4 level PPGTT. In most cases, we really only care if the > > platform supports PPGTT or not. Because of this, we can now remove > > the HAS_FULL_PPGTT macro and the device info ppgtt field. > > > > Aliasing PPGTT used by GEN 6 is a bit of an exception. For those cases, > > it makes just as much sense to check if we're running on GEN 6 as it > > does to check a device info flag. > > > > v2: Reword the commit message to make it correct wrt aliasing ppgtt (Chris) > > v3: Rebase on current drm-tip > > The point of adding the type into the device_info was that it was > included in the error state so that I didn't have to remember which w/a > applied to which gen. gen6 has full-ppgtt, we can't enabled it as no one > has solved how to make it work. > -Chris OK, I had assumed that the point was to control the code flow so I'm not sure how best to proceed. This patch doesn't really change any program logic so we can drop it and keep both the ppgtt type and size in device info. Alternately, I could add the ppgtt size to error state output. But I'm not sure that really provides what you're looking for wrt gen6. If you have a better idea or something specific you'd like to see me do with this, I could use some direction. Thanks, Bob
Chris, Any thoughts on how I can best address your comment on this patch? Bob On Thu, 7 Feb 2019 11:13:15 -0800 Bob Paauwe <bob.j.paauwe@intel.com> wrote: > On Thu, 7 Feb 2019 16:41:58 +0000 > Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > Quoting Bob Paauwe (2019-02-07 16:29:53) > > > With the address range being specified for each platform, we can use > > > that instead of the .ppgtt enum to handle the differences between > > > 3 level and 4 level PPGTT. In most cases, we really only care if the > > > platform supports PPGTT or not. Because of this, we can now remove > > > the HAS_FULL_PPGTT macro and the device info ppgtt field. > > > > > > Aliasing PPGTT used by GEN 6 is a bit of an exception. For those cases, > > > it makes just as much sense to check if we're running on GEN 6 as it > > > does to check a device info flag. > > > > > > v2: Reword the commit message to make it correct wrt aliasing ppgtt (Chris) > > > v3: Rebase on current drm-tip > > > > The point of adding the type into the device_info was that it was > > included in the error state so that I didn't have to remember which w/a > > applied to which gen. gen6 has full-ppgtt, we can't enabled it as no one > > has solved how to make it work. > > -Chris > > OK, I had assumed that the point was to control the code flow so I'm > not sure how best to proceed. > > This patch doesn't really change any program logic so we can drop it > and keep both the ppgtt type and size in device info. > > Alternately, I could add the ppgtt size to error state output. But I'm > not sure that really provides what you're looking for wrt gen6. > > If you have a better idea or something specific you'd like to see me do > with this, I could use some direction. > > Thanks, > Bob > -- Bob Paauwe Bob.J.Paauwe@intel.com IOTG / PED Software Organization Intel Corp. Folsom, CA (916) 356-6193
Quoting Bob Paauwe (2019-03-14 20:09:57) > Chris, > > Any thoughts on how I can best address your comment on this patch? I do not see the value in this patch as it is. Don't remove intel_device_info.ppgtt; rename it to ppgtt_mode. Put ppgtt_size alongside. Ie keep using intel_device_info.ppgtt_mode to differentiate between none/aliasing/full for initialisation, then the normal pointer dance from i915_gem_context to determine which is active. -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 2a7bd202f7d8..aab84baac8bc 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -346,7 +346,12 @@ static int i915_getparam_ioctl(struct drm_device *dev, void *data, value = HAS_WT(dev_priv); break; case I915_PARAM_HAS_ALIASING_PPGTT: - value = min_t(int, INTEL_PPGTT(dev_priv), I915_GEM_PPGTT_FULL); + if (INTEL_GEN(dev_priv) < 6) + value = I915_GEM_PPGTT_NONE; + else if (INTEL_GEN(dev_priv) == 6) + value = I915_GEM_PPGTT_ALIASING; + else + value = I915_GEM_PPGTT_FULL; break; case I915_PARAM_HAS_SEMAPHORES: value = 0; diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 641b4ced3725..9255b2e3375f 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2449,11 +2449,13 @@ static inline unsigned int i915_sg_segment_size(void) #define HAS_EXECLISTS(dev_priv) HAS_LOGICAL_RING_CONTEXTS(dev_priv) -#define INTEL_PPGTT(dev_priv) (INTEL_INFO(dev_priv)->ppgtt) +#define INTEL_PPGTT_BITS(dev_priv) (INTEL_INFO(dev_priv)->ppgtt_bits) #define HAS_PPGTT(dev_priv) \ - (INTEL_PPGTT(dev_priv) != INTEL_PPGTT_NONE) + (INTEL_PPGTT_BITS(dev_priv) != 0) +/* #define HAS_FULL_PPGTT(dev_priv) \ - (INTEL_PPGTT(dev_priv) >= INTEL_PPGTT_FULL) + (INTEL_PPGTT_BITS(dev_priv) >= 31) +*/ #define HAS_PAGE_SIZES(dev_priv, sizes) ({ \ GEM_BUG_ON((sizes) == 0); \ diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 42e1248a90da..ea71a5a7a483 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -437,7 +437,7 @@ i915_gem_create_context(struct drm_i915_private *dev_priv, if (IS_ERR(ctx)) return ctx; - if (HAS_FULL_PPGTT(dev_priv)) { + if (INTEL_GEN(dev_priv) > 6) { struct i915_hw_ppgtt *ppgtt; ppgtt = i915_ppgtt_create(dev_priv, file_priv); diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index f2b634e6cf9d..fa59b7992d80 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -2754,7 +2754,7 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv) /* And finally clear the reserved guard page */ ggtt->vm.clear_range(&ggtt->vm, ggtt->vm.total - PAGE_SIZE, PAGE_SIZE); - if (INTEL_PPGTT(dev_priv) == INTEL_PPGTT_ALIASING) { + if (INTEL_GEN(dev_priv) == 6) { ret = i915_gem_init_aliasing_ppgtt(dev_priv); if (ret) goto err; diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c index f18929637bd5..bd8cd790e1b1 100644 --- a/drivers/gpu/drm/i915/i915_pci.c +++ b/drivers/gpu/drm/i915/i915_pci.c @@ -288,7 +288,6 @@ static const struct intel_device_info intel_ironlake_m_info = { .has_llc = 1, \ .has_rc6 = 1, \ .has_rc6p = 1, \ - .ppgtt = INTEL_PPGTT_ALIASING, \ .ppgtt_bits = 31, \ GEN_DEFAULT_PIPEOFFSETS, \ GEN_DEFAULT_PAGE_SIZES, \ @@ -334,7 +333,6 @@ static const struct intel_device_info intel_sandybridge_m_gt2_info = { .has_llc = 1, \ .has_rc6 = 1, \ .has_rc6p = 1, \ - .ppgtt = INTEL_PPGTT_FULL, \ .ppgtt_bits = 31, \ GEN_DEFAULT_PIPEOFFSETS, \ GEN_DEFAULT_PAGE_SIZES, \ @@ -388,7 +386,6 @@ static const struct intel_device_info intel_valleyview_info = { .has_rc6 = 1, .display.has_gmch = 1, .display.has_hotplug = 1, - .ppgtt = INTEL_PPGTT_FULL, .ppgtt_bits = 31, .has_snoop = true, .has_coherent_ggtt = false, @@ -436,7 +433,6 @@ static const struct intel_device_info intel_haswell_gt3_info = { .page_sizes = I915_GTT_PAGE_SIZE_4K | \ I915_GTT_PAGE_SIZE_2M, \ .has_logical_ring_contexts = 1, \ - .ppgtt = INTEL_PPGTT_FULL, \ .ppgtt_bits = 48, \ .has_64bit_reloc = 1, \ .has_reset_engine = 1 @@ -481,7 +477,6 @@ static const struct intel_device_info intel_cherryview_info = { .has_rc6 = 1, .has_logical_ring_contexts = 1, .display.has_gmch = 1, - .ppgtt = INTEL_PPGTT_FULL, .ppgtt_bits = 32, .has_reset_engine = 1, .has_snoop = true, @@ -557,7 +552,6 @@ static const struct intel_device_info intel_skylake_gt4_info = { .has_logical_ring_contexts = 1, \ .has_logical_ring_preemption = 1, \ .has_guc = 1, \ - .ppgtt = INTEL_PPGTT_FULL, \ .ppgtt_bits = 48, \ .has_reset_engine = 1, \ .has_snoop = true, \ diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c index 855a5074ad77..80f0e905f381 100644 --- a/drivers/gpu/drm/i915/intel_device_info.c +++ b/drivers/gpu/drm/i915/intel_device_info.c @@ -844,7 +844,7 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv) if (IS_GEN(dev_priv, 6) && intel_vtd_active()) { DRM_INFO("Disabling ppGTT for VT-d support\n"); - info->ppgtt = INTEL_PPGTT_NONE; + info->ppgtt_bits = 0; } /* Initialize command stream timestamp frequency */ diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h index f6a4ce0f9cb5..836c099fe254 100644 --- a/drivers/gpu/drm/i915/intel_device_info.h +++ b/drivers/gpu/drm/i915/intel_device_info.h @@ -76,12 +76,6 @@ enum intel_platform { INTEL_MAX_PLATFORMS }; -enum intel_ppgtt { - INTEL_PPGTT_NONE = I915_GEM_PPGTT_NONE, - INTEL_PPGTT_ALIASING = I915_GEM_PPGTT_ALIASING, - INTEL_PPGTT_FULL = I915_GEM_PPGTT_FULL, -}; - #define DEV_INFO_FOR_EACH_FLAG(func) \ func(is_mobile); \ func(is_lp); \ @@ -161,7 +155,6 @@ struct intel_device_info { enum intel_platform platform; u32 platform_mask; - enum intel_ppgtt ppgtt; unsigned int page_sizes; /* page sizes supported by the HW */ u32 display_mmio_offset; diff --git a/drivers/gpu/drm/i915/selftests/huge_pages.c b/drivers/gpu/drm/i915/selftests/huge_pages.c index 9f90179df185..08c98db4d961 100644 --- a/drivers/gpu/drm/i915/selftests/huge_pages.c +++ b/drivers/gpu/drm/i915/selftests/huge_pages.c @@ -1708,8 +1708,8 @@ int i915_gem_huge_page_mock_selftests(void) if (!dev_priv) return -ENOMEM; - /* Pretend to be a device which supports the 48b PPGTT */ - mkwrite_device_info(dev_priv)->ppgtt = INTEL_PPGTT_FULL; + /* Pretend to be a device which supports the 63b PPGTT */ + mkwrite_device_info(dev_priv)->ppgtt_bits = 63; mutex_lock(&dev_priv->drm.struct_mutex); ppgtt = i915_ppgtt_create(dev_priv, ERR_PTR(-ENODEV)); diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_evict.c b/drivers/gpu/drm/i915/selftests/i915_gem_evict.c index 32dce7176f63..d1614b261b71 100644 --- a/drivers/gpu/drm/i915/selftests/i915_gem_evict.c +++ b/drivers/gpu/drm/i915/selftests/i915_gem_evict.c @@ -393,7 +393,7 @@ static int igt_evict_contexts(void *arg) * where the GTT space of the request is separate from the GGTT * allocation required to build the request. */ - if (!HAS_FULL_PPGTT(i915)) + if (INTEL_GEN(i915) <= 6) return 0; mutex_lock(&i915->drm.struct_mutex); diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c index 3850ef4a5ec8..9faffbe64480 100644 --- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c @@ -1002,7 +1002,7 @@ static int exercise_ppgtt(struct drm_i915_private *dev_priv, IGT_TIMEOUT(end_time); int err; - if (!HAS_FULL_PPGTT(dev_priv)) + if (INTEL_GEN(dev_priv) <= 6) return 0; file = mock_file(dev_priv);
With the address range being specified for each platform, we can use that instead of the .ppgtt enum to handle the differences between 3 level and 4 level PPGTT. In most cases, we really only care if the platform supports PPGTT or not. Because of this, we can now remove the HAS_FULL_PPGTT macro and the device info ppgtt field. Aliasing PPGTT used by GEN 6 is a bit of an exception. For those cases, it makes just as much sense to check if we're running on GEN 6 as it does to check a device info flag. v2: Reword the commit message to make it correct wrt aliasing ppgtt (Chris) v3: Rebase on current drm-tip Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com> CC: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_drv.c | 7 ++++++- drivers/gpu/drm/i915/i915_drv.h | 8 +++++--- drivers/gpu/drm/i915/i915_gem_context.c | 2 +- drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +- drivers/gpu/drm/i915/i915_pci.c | 6 ------ drivers/gpu/drm/i915/intel_device_info.c | 2 +- drivers/gpu/drm/i915/intel_device_info.h | 7 ------- drivers/gpu/drm/i915/selftests/huge_pages.c | 4 ++-- drivers/gpu/drm/i915/selftests/i915_gem_evict.c | 2 +- drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 2 +- 10 files changed, 18 insertions(+), 24 deletions(-)