Re: [Intel-gfx] [PATCH] drm/i915: Rename full ppgtt configuration to be more generic
On Fri, 31 Aug 2018 13:21:40 -0700
Rodrigo Vivi wrote:
> On Fri, Aug 31, 2018 at 04:51:29PM +0100, Chris Wilson wrote:
> > Quoting Bob Paauwe (2018-08-31 16:47:04)
> > > For ppgtt, what we're really interested in is the number of page
> > > walk levels for each platform. Rename the device info fields to
> > > reflect this:
> > >
> > > .has_full_48b_ppgtt -> .has_full_4lvl_ppgtt
> > > .has_full_ppgtt -> .has_full_3lvl_ppgtt
> > >
> > > Also add a new field, full_ppgtt_bits, that defines the actual
> > > address range. This gives us more flexibility and will work for
> > > cases where we have platforms with different address ranges but
> > > share the same page walk levels.
> > >
> > > Signed-off-by: Bob Paauwe
> > > CC: Rodrigo Vivi
> > > CC: Michel Thierry
> > > ---
> > > drivers/gpu/drm/i915/i915_drv.h | 4 +--
> > > drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
> > > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
> > > drivers/gpu/drm/i915/i915_gem_gtt.c | 34
> > > +---
> > > drivers/gpu/drm/i915/i915_params.c | 3 ++-
> > > drivers/gpu/drm/i915/i915_pci.c | 17 +++-
> > > drivers/gpu/drm/i915/intel_device_info.h | 7 +++--
> > > drivers/gpu/drm/i915/selftests/huge_pages.c | 2 +-
> > > drivers/gpu/drm/i915/selftests/i915_gem_evict.c | 2 +-
> > > drivers/gpu/drm/i915/selftests/i915_gem_gtt.c| 2 +-
> > > drivers/gpu/drm/i915/selftests/mock_gem_device.c | 2 ++
> > > 11 files changed, 45 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index e5b9d3c77139..b9f7903e60d1 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -2569,8 +2569,8 @@ intel_info(const struct drm_i915_private *dev_priv)
> > > #define HAS_EXECLISTS(dev_priv) HAS_LOGICAL_RING_CONTEXTS(dev_priv)
> > >
> > > #define USES_PPGTT(dev_priv) (i915_modparams.enable_ppgtt)
> > > -#define USES_FULL_PPGTT(dev_priv) (i915_modparams.enable_ppgtt >= 2)
> > > -#define USES_FULL_48BIT_PPGTT(dev_priv)
> > > (i915_modparams.enable_ppgtt == 3)
> > > +#define USES_FULL_3LVL_PPGTT(dev_priv) (i915_modparams.enable_ppgtt >= 2)
> > > +#define USES_FULL_4LVL_PPGTT(dev_priv) (i915_modparams.enable_ppgtt == 3)
> > > #define HAS_PAGE_SIZES(dev_priv, sizes) ({ \
> > > GEM_BUG_ON((sizes) == 0); \
> > > ((sizes) & ~(dev_priv)->info.page_sizes) == 0; \
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> > > b/drivers/gpu/drm/i915/i915_gem_context.c
> > > index f15a039772db..a0dc3170b358 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > > @@ -361,7 +361,7 @@ i915_gem_create_context(struct drm_i915_private
> > > *dev_priv,
> > > if (IS_ERR(ctx))
> > > return ctx;
> > >
> > > - if (USES_FULL_PPGTT(dev_priv)) {
> > > + if (USES_FULL_3LVL_PPGTT(dev_priv)) {
> >
> > That is not an improvement. It really is a question of whether or not
> > full-ppgtt is enabled.
>
> I think we do need this change, but only with USES_FULL_PPGTT macro
> and the rest should be checked with the full_ppgtt number of bits if
> needed.
>
USES_FULL_PPGTT() is currently used to mean at least 3 level, but could
be 4 level so I get Chris's point that USES_FULL_3LVL_PPGTT doesn't
really work, at least not as a simple substitution.
There are only a couple of places where we care that it's actually 4
level so changing those to actually check the number of bits seems to
make sense to me. Either
#define USES_FULL_4LVL_PPGTT(i915) (i915.number_of_bits > 32)
or just use the condition without the macro where needed.
> >
> > > struct i915_hw_ppgtt *ppgtt;
> > >
> > > ppgtt = i915_ppgtt_create(dev_priv, file_priv);
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > index a926d7d47183..166f1ea1786f 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > @@ -2201,7 +2201,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
> > > eb.flags = (unsigned int *)(eb.vma + args->buffer_count + 1);
> > >
> > > eb.invalid_flags = __EXEC_OBJECT_UNKNOWN_FLAGS;
> > > - if (USES_FULL_PPGTT(eb.i915))
> > > + if (USES_FULL_3LVL_PPGTT(eb.i915))
> >
> > Again the same complaint.
> >
> > I think you need to rethink the semantics carefully.
> > -Chris
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
--
Bob Paauwe
bob.j.paa...@intel.com
IOTG / PED Software Organization
Intel Corp. Folsom, CA
(916) 356-6193
Re: [Intel-gfx] [PATCH] drm/i915: Rename full ppgtt configuration to be more generic
On Fri, Aug 31, 2018 at 04:51:29PM +0100, Chris Wilson wrote:
> Quoting Bob Paauwe (2018-08-31 16:47:04)
> > For ppgtt, what we're really interested in is the number of page
> > walk levels for each platform. Rename the device info fields to
> > reflect this:
> >
> > .has_full_48b_ppgtt -> .has_full_4lvl_ppgtt
> > .has_full_ppgtt -> .has_full_3lvl_ppgtt
> >
> > Also add a new field, full_ppgtt_bits, that defines the actual
> > address range. This gives us more flexibility and will work for
> > cases where we have platforms with different address ranges but
> > share the same page walk levels.
> >
> > Signed-off-by: Bob Paauwe
> > CC: Rodrigo Vivi
> > CC: Michel Thierry
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 4 +--
> > drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
> > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
> > drivers/gpu/drm/i915/i915_gem_gtt.c | 34
> > +---
> > drivers/gpu/drm/i915/i915_params.c | 3 ++-
> > drivers/gpu/drm/i915/i915_pci.c | 17 +++-
> > drivers/gpu/drm/i915/intel_device_info.h | 7 +++--
> > drivers/gpu/drm/i915/selftests/huge_pages.c | 2 +-
> > drivers/gpu/drm/i915/selftests/i915_gem_evict.c | 2 +-
> > drivers/gpu/drm/i915/selftests/i915_gem_gtt.c| 2 +-
> > drivers/gpu/drm/i915/selftests/mock_gem_device.c | 2 ++
> > 11 files changed, 45 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index e5b9d3c77139..b9f7903e60d1 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2569,8 +2569,8 @@ intel_info(const struct drm_i915_private *dev_priv)
> > #define HAS_EXECLISTS(dev_priv) HAS_LOGICAL_RING_CONTEXTS(dev_priv)
> >
> > #define USES_PPGTT(dev_priv) (i915_modparams.enable_ppgtt)
> > -#define USES_FULL_PPGTT(dev_priv) (i915_modparams.enable_ppgtt >= 2)
> > -#define USES_FULL_48BIT_PPGTT(dev_priv)
> > (i915_modparams.enable_ppgtt == 3)
> > +#define USES_FULL_3LVL_PPGTT(dev_priv) (i915_modparams.enable_ppgtt >= 2)
> > +#define USES_FULL_4LVL_PPGTT(dev_priv) (i915_modparams.enable_ppgtt == 3)
> > #define HAS_PAGE_SIZES(dev_priv, sizes) ({ \
> > GEM_BUG_ON((sizes) == 0); \
> > ((sizes) & ~(dev_priv)->info.page_sizes) == 0; \
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> > b/drivers/gpu/drm/i915/i915_gem_context.c
> > index f15a039772db..a0dc3170b358 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -361,7 +361,7 @@ i915_gem_create_context(struct drm_i915_private
> > *dev_priv,
> > if (IS_ERR(ctx))
> > return ctx;
> >
> > - if (USES_FULL_PPGTT(dev_priv)) {
> > + if (USES_FULL_3LVL_PPGTT(dev_priv)) {
>
> That is not an improvement. It really is a question of whether or not
> full-ppgtt is enabled.
I think we do need this change, but only with USES_FULL_PPGTT macro
and the rest should be checked with the full_ppgtt number of bits if
needed.
>
> > struct i915_hw_ppgtt *ppgtt;
> >
> > ppgtt = i915_ppgtt_create(dev_priv, file_priv);
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index a926d7d47183..166f1ea1786f 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -2201,7 +2201,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
> > eb.flags = (unsigned int *)(eb.vma + args->buffer_count + 1);
> >
> > eb.invalid_flags = __EXEC_OBJECT_UNKNOWN_FLAGS;
> > - if (USES_FULL_PPGTT(eb.i915))
> > + if (USES_FULL_3LVL_PPGTT(eb.i915))
>
> Again the same complaint.
>
> I think you need to rethink the semantics carefully.
> -Chris
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Rename full ppgtt configuration to be more generic
On Fri, 31 Aug 2018 16:51:29 +0100
Chris Wilson wrote:
> Quoting Bob Paauwe (2018-08-31 16:47:04)
> > For ppgtt, what we're really interested in is the number of page
> > walk levels for each platform. Rename the device info fields to
> > reflect this:
> >
> > .has_full_48b_ppgtt -> .has_full_4lvl_ppgtt
> > .has_full_ppgtt -> .has_full_3lvl_ppgtt
> >
> > Also add a new field, full_ppgtt_bits, that defines the actual
> > address range. This gives us more flexibility and will work for
> > cases where we have platforms with different address ranges but
> > share the same page walk levels.
> >
> > Signed-off-by: Bob Paauwe
> > CC: Rodrigo Vivi
> > CC: Michel Thierry
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 4 +--
> > drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
> > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
> > drivers/gpu/drm/i915/i915_gem_gtt.c | 34
> > +---
> > drivers/gpu/drm/i915/i915_params.c | 3 ++-
> > drivers/gpu/drm/i915/i915_pci.c | 17 +++-
> > drivers/gpu/drm/i915/intel_device_info.h | 7 +++--
> > drivers/gpu/drm/i915/selftests/huge_pages.c | 2 +-
> > drivers/gpu/drm/i915/selftests/i915_gem_evict.c | 2 +-
> > drivers/gpu/drm/i915/selftests/i915_gem_gtt.c| 2 +-
> > drivers/gpu/drm/i915/selftests/mock_gem_device.c | 2 ++
> > 11 files changed, 45 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index e5b9d3c77139..b9f7903e60d1 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2569,8 +2569,8 @@ intel_info(const struct drm_i915_private *dev_priv)
> > #define HAS_EXECLISTS(dev_priv) HAS_LOGICAL_RING_CONTEXTS(dev_priv)
> >
> > #define USES_PPGTT(dev_priv) (i915_modparams.enable_ppgtt)
> > -#define USES_FULL_PPGTT(dev_priv) (i915_modparams.enable_ppgtt >= 2)
> > -#define USES_FULL_48BIT_PPGTT(dev_priv)
> > (i915_modparams.enable_ppgtt == 3)
> > +#define USES_FULL_3LVL_PPGTT(dev_priv) (i915_modparams.enable_ppgtt >= 2)
> > +#define USES_FULL_4LVL_PPGTT(dev_priv) (i915_modparams.enable_ppgtt == 3)
> > #define HAS_PAGE_SIZES(dev_priv, sizes) ({ \
> > GEM_BUG_ON((sizes) == 0); \
> > ((sizes) & ~(dev_priv)->info.page_sizes) == 0; \
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> > b/drivers/gpu/drm/i915/i915_gem_context.c
> > index f15a039772db..a0dc3170b358 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -361,7 +361,7 @@ i915_gem_create_context(struct drm_i915_private
> > *dev_priv,
> > if (IS_ERR(ctx))
> > return ctx;
> >
> > - if (USES_FULL_PPGTT(dev_priv)) {
> > + if (USES_FULL_3LVL_PPGTT(dev_priv)) {
>
> That is not an improvement. It really is a question of whether or not
> full-ppgtt is enabled.
>
> > struct i915_hw_ppgtt *ppgtt;
> >
> > ppgtt = i915_ppgtt_create(dev_priv, file_priv);
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index a926d7d47183..166f1ea1786f 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -2201,7 +2201,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
> > eb.flags = (unsigned int *)(eb.vma + args->buffer_count + 1);
> >
> > eb.invalid_flags = __EXEC_OBJECT_UNKNOWN_FLAGS;
> > - if (USES_FULL_PPGTT(eb.i915))
> > + if (USES_FULL_3LVL_PPGTT(eb.i915))
>
> Again the same complaint.
>
> I think you need to rethink the semantics carefully.
> -Chris
Would USES_FULL_PPGTT() and USES_EXTENDED_PPGTT() make more sense
then? I think the biggest issue is with the FULL_48BIT_PPGTT name
going forward.
Bob
--
--
Bob Paauwe
bob.j.paa...@intel.com
IOTG / PED Software Organization
Intel Corp. Folsom, CA
(916) 356-6193
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Rename full ppgtt configuration to be more generic
Quoting Bob Paauwe (2018-08-31 16:47:04)
> For ppgtt, what we're really interested in is the number of page
> walk levels for each platform. Rename the device info fields to
> reflect this:
>
> .has_full_48b_ppgtt -> .has_full_4lvl_ppgtt
> .has_full_ppgtt -> .has_full_3lvl_ppgtt
>
> Also add a new field, full_ppgtt_bits, that defines the actual
> address range. This gives us more flexibility and will work for
> cases where we have platforms with different address ranges but
> share the same page walk levels.
>
> Signed-off-by: Bob Paauwe
> CC: Rodrigo Vivi
> CC: Michel Thierry
> ---
> drivers/gpu/drm/i915/i915_drv.h | 4 +--
> drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
> drivers/gpu/drm/i915/i915_gem_gtt.c | 34
> +---
> drivers/gpu/drm/i915/i915_params.c | 3 ++-
> drivers/gpu/drm/i915/i915_pci.c | 17 +++-
> drivers/gpu/drm/i915/intel_device_info.h | 7 +++--
> drivers/gpu/drm/i915/selftests/huge_pages.c | 2 +-
> drivers/gpu/drm/i915/selftests/i915_gem_evict.c | 2 +-
> drivers/gpu/drm/i915/selftests/i915_gem_gtt.c| 2 +-
> drivers/gpu/drm/i915/selftests/mock_gem_device.c | 2 ++
> 11 files changed, 45 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e5b9d3c77139..b9f7903e60d1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2569,8 +2569,8 @@ intel_info(const struct drm_i915_private *dev_priv)
> #define HAS_EXECLISTS(dev_priv) HAS_LOGICAL_RING_CONTEXTS(dev_priv)
>
> #define USES_PPGTT(dev_priv) (i915_modparams.enable_ppgtt)
> -#define USES_FULL_PPGTT(dev_priv) (i915_modparams.enable_ppgtt >= 2)
> -#define USES_FULL_48BIT_PPGTT(dev_priv)(i915_modparams.enable_ppgtt
> == 3)
> +#define USES_FULL_3LVL_PPGTT(dev_priv) (i915_modparams.enable_ppgtt >= 2)
> +#define USES_FULL_4LVL_PPGTT(dev_priv) (i915_modparams.enable_ppgtt == 3)
> #define HAS_PAGE_SIZES(dev_priv, sizes) ({ \
> GEM_BUG_ON((sizes) == 0); \
> ((sizes) & ~(dev_priv)->info.page_sizes) == 0; \
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index f15a039772db..a0dc3170b358 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -361,7 +361,7 @@ i915_gem_create_context(struct drm_i915_private *dev_priv,
> if (IS_ERR(ctx))
> return ctx;
>
> - if (USES_FULL_PPGTT(dev_priv)) {
> + if (USES_FULL_3LVL_PPGTT(dev_priv)) {
That is not an improvement. It really is a question of whether or not
full-ppgtt is enabled.
> struct i915_hw_ppgtt *ppgtt;
>
> ppgtt = i915_ppgtt_create(dev_priv, file_priv);
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index a926d7d47183..166f1ea1786f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -2201,7 +2201,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
> eb.flags = (unsigned int *)(eb.vma + args->buffer_count + 1);
>
> eb.invalid_flags = __EXEC_OBJECT_UNKNOWN_FLAGS;
> - if (USES_FULL_PPGTT(eb.i915))
> + if (USES_FULL_3LVL_PPGTT(eb.i915))
Again the same complaint.
I think you need to rethink the semantics carefully.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
