Re: [PATCH v3 1/4] drm/i915: Enable NULL PTE support for vm scratch

[Andrzej Hajda <andrzej.hajda@xxxxxxxxx>]

On 23.10.2023 14:23, Nirmoy Das wrote:
> On 10/23/2023 9:41 AM, Andrzej Hajda wrote:
> > From: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
> >
> > Enable NULL PTE support for vm scratch pages.
> >
> > The use of NULL PTEs in vm scratch pages requires us to change how
> > the i915 gem_contexts live selftest perform vm_isolation: instead of
> > checking the scratch pages are isolated and don't affect each other, we
> > check that all changes to the scratch pages are voided.
> >
> > v2: fixed order of definitions
> > v3: fixed typo
> >
> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
> > Suggested-by: Chris Wilson <chris.p.wilson@xxxxxxxxxxxxxxx>
> > Signed-off-by: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
> > Reviewed-by: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c | 6 ++++++
> >   drivers/gpu/drm/i915/gt/gen8_ppgtt.c                  | 3 +++
> >   drivers/gpu/drm/i915/gt/intel_gtt.h                   | 1 +
> >   drivers/gpu/drm/i915/i915_drv.h                       | 2 ++
> >   drivers/gpu/drm/i915/i915_pci.c                       | 2 ++
> >   drivers/gpu/drm/i915/intel_device_info.h              | 1 +
> >   6 files changed, 15 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c 
> > b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> > index 7021b6e9b219ef..48fc5990343bc7 100644
> > --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> > @@ -1751,6 +1751,12 @@ static int check_scratch_page(struct 
> > i915_gem_context *ctx, u32 *out)
> >       if (!vm)
> >           return -ENODEV;
> > +    if (HAS_NULL_PAGE(vm->i915)) {
> > +        if (out)
> > +            *out = 0;
> > +        return 0;
> > +    }
> > +
> >       if (!vm->scratch[0]) {
> >           pr_err("No scratch page!\n");
> >           return -EINVAL;
> > diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c 
> > b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> > index 9895e18df0435a..84aa29715e0aca 100644
> > --- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> > +++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> > @@ -855,6 +855,9 @@ static int gen8_init_scratch(struct 
> > i915_address_space *vm)
> >                                 I915_CACHE_NONE),
> >                      pte_flags);
> > +    if (HAS_NULL_PAGE(vm->i915))
> > +        vm->scratch[0]->encode |= PTE_NULL_PAGE;
> > +
> >       for (i = 1; i <= vm->top; i++) {
> >           struct drm_i915_gem_object *obj;
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h 
> > b/drivers/gpu/drm/i915/gt/intel_gtt.h
> > index b471edac269920..15c71da14d1d27 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gtt.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
> > @@ -151,6 +151,7 @@ typedef u64 gen8_pte_t;
> >   #define GEN8_PAGE_PRESENT        BIT_ULL(0)
> >   #define GEN8_PAGE_RW            BIT_ULL(1)
> > +#define PTE_NULL_PAGE            BIT_ULL(9)
> >   #define GEN8_PDE_IPS_64K BIT(11)
> >   #define GEN8_PDE_PS_2M   BIT(7)
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index cb60fc9cf87373..8f61137deb6cef 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -776,6 +776,8 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
> >    */
> >   #define HAS_FLAT_CCS(i915)   (INTEL_INFO(i915)->has_flat_ccs)
> > +#define HAS_NULL_PAGE(dev_priv) (INTEL_INFO(dev_priv)->has_null_page)
> > +
> >   #define HAS_GT_UC(i915)    (INTEL_INFO(i915)->has_gt_uc)
> >   #define HAS_POOLED_EU(i915)    (RUNTIME_INFO(i915)->has_pooled_eu)
> > diff --git a/drivers/gpu/drm/i915/i915_pci.c 
> > b/drivers/gpu/drm/i915/i915_pci.c
> > index 454467cfa52b9d..aa6e4559b0f0c7 100644
> > --- a/drivers/gpu/drm/i915/i915_pci.c
> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > @@ -642,6 +642,7 @@ static const struct intel_device_info jsl_info = {
> >       GEN(12), \
> >       TGL_CACHELEVEL, \
> >       .has_global_mocs = 1, \
> > +    .has_null_page = 1, \
> >       .has_pxp = 1, \
> >       .max_pat_index = 3
> > @@ -719,6 +720,7 @@ static const struct intel_device_info adl_p_info = {
> >       .has_logical_ring_contexts = 1, \
> >       .has_logical_ring_elsq = 1, \
> >       .has_mslice_steering = 1, \
> > +    .has_null_page = 1, \
> >       .has_oa_bpc_reporting = 1, \
> >       .has_oa_slice_contrib_limits = 1, \
> >       .has_oam = 1, \
>
> Why only above platforms are picked, it is not clear from the commit 
> message.

This is git issue, it fails to parse #define, and provides incorrect 
owner hints, here we have actually:
1st: #define GEN12_FEATURES
2nd: #define XE_HP_FEATURES, which is included in all later gens.

So IIRC all gen12+.
Just for information - this patch will be dropped anyway.

Regards
Andrzej



> Regards,
>
> Nirmoy
>
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.h 
> > b/drivers/gpu/drm/i915/intel_device_info.h
> > index 39817490b13fd4..36e169695cd61b 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.h
> > +++ b/drivers/gpu/drm/i915/intel_device_info.h
> > @@ -160,6 +160,7 @@ enum intel_ppgtt_type {
> >       func(has_logical_ring_elsq); \
> >       func(has_media_ratio_mode); \
> >       func(has_mslice_steering); \
> > +    func(has_null_page); \
> >       func(has_oa_bpc_reporting); \
> >       func(has_oa_slice_contrib_limits); \
> >       func(has_oam); \