Re: [Intel-gfx] [PATCH 2/2] drm/i915: Rename gt to gt0
Hi Chris,
> > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > @@ -817,7 +817,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane,
> > * maximum clocks following a vblank miss (see do_rps_boost()).
> > */
> > if (!state->rps_interactive) {
> > - intel_rps_mark_interactive(&dev_priv->gt.rps, true);
> > + intel_rps_mark_interactive(&dev_priv->gt0.rps, true);
>
> This should be across all gt, so probably wants a fresh interface that
> takes i915 and does for_each_gt in a later patch. (Since we could be
> rendering on a remote tile to present on a display.)
To make it more generic this can be done by adding in rps a:
/* the original intel_rps_mark_interactive */
intel_rps_mark_interactive_gt(struct intel_rps *rps, bool interactive)
{
...
}
intel_rps_mark_interactive(struct drm_i915_private *i915, bool interactive)
{
for_each_gt(...)
intel_rps_mark_interactive_gt(...)
}
but I think this should go on a different patch.
> > state->rps_interactive = true;
> > }
> >
> > @@ -851,7 +851,7 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
> > return;
> >
> > if (state->rps_interactive) {
> > - intel_rps_mark_interactive(&dev_priv->gt.rps, false);
> > + intel_rps_mark_interactive(&dev_priv->gt0.rps, false);
> > state->rps_interactive = false;
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 0ceee8ac66717..d4fcd8f236476 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -838,7 +838,7 @@ __intel_display_resume(struct drm_device *dev,
> > static bool gpu_reset_clobbers_display(struct drm_i915_private *dev_priv)
> > {
> > return (INTEL_INFO(dev_priv)->gpu_reset_clobbers_display &&
> > - intel_has_gpu_reset(&dev_priv->gt));
> > + intel_has_gpu_reset(&dev_priv->gt0));
>
> All these display consumers probably want to use
> dev_priv->ggtt->vm.gt, since the scanout capable GGTT would seem to be
> the defining feature.
>
> to_scanout_gt(i915) ?
OK
> > static bool pxp_is_borked(struct drm_i915_gem_object *obj)
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > index ebd775cb1661c..c62253d0af044 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > @@ -237,7 +237,7 @@ static int proto_context_set_persistence(struct
> > drm_i915_private *i915,
> > * colateral damage, and we should not pretend we can by
> > * exposing the interface.
> > */
> > - if (!intel_has_reset_engine(&i915->gt))
> > + if (!intel_has_reset_engine(&i915->gt0))
> > return -ENODEV;
>
> Prep for all gt. A lot of these need an all-gt interface so we don't
> have for_each_gt spread all other the place.
agree... I think, though, this should go in a different patch.
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
> > index ef22d4ed66ad6..69ad407eb15f3 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
> > @@ -166,7 +166,7 @@ static struct dma_fence *i915_ttm_accel_move(struct
> > ttm_buffer_object *bo,
> > enum i915_cache_level src_level, dst_level;
> > int ret;
> >
> > - if (!i915->gt.migrate.context || intel_gt_is_wedged(&i915->gt))
> > + if (!i915->gt0.migrate.context || intel_gt_is_wedged(&i915->gt0))
>
> This should already be looking at lmem->gt
Thanks... I will note this down for a different patch, as well.
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c
> > b/drivers/gpu/drm/i915/gt/intel_engine_user.c
> > index 8f8bea08e734d..176ea5c7d422f 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_user.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c
> > @@ -116,7 +116,7 @@ static void set_scheduler_caps(struct drm_i915_private
> > *i915)
> > disabled |= (I915_SCHEDULER_CAP_ENABLED |
> > I915_SCHEDULER_CAP_PRIORITY);
> >
> > - if (intel_uc_uses_guc_submission(&i915->gt.uc))
> > + if (intel_uc_uses_guc_submission(&i915->gt0.uc))
>
> This shouldn't be looking at gt at all, but if it must, that information
> must be coming via engine->gt. Kind of renders the mapping moot
> currently.
OK
> > diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c
> > b/drivers/gpu/drm/i915/gt/intel_rps.c
> > index 07ff7ba7b2b71..63089e671a242 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_rps.c
> > +++
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Rename gt to gt0
On Wed, Nov 17, 2021 at 02:16:13PM +, Chris Wilson wrote:
Quoting Andi Shyti (2021-11-17 13:34:56)
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index a9727447c0379..4bfedc04f5c70 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -936,7 +936,7 @@ hsw_gt_workarounds_init(struct intel_gt *gt, struct
i915_wa_list *wal)
static void
gen9_wa_init_mcr(struct drm_i915_private *i915, struct i915_wa_list *wal)
{
- const struct sseu_dev_info *sseu = &i915->gt.info.sseu;
+ const struct sseu_dev_info *sseu = &i915->gt0.info.sseu;
This feels like it should be pulling from uncore->gt, since the MCR is
across an uncore.
Overall though, rather than introduce bare &i915->gt0, how about pulling in
the patch for to_gt(i915)?
agreed, why are we going a different route rather than using the to_gt()
patches Michal prepared? If the to_gt() is not what we want, then please
sync with Michal on the proper direction
Lucas De Marchi
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Rename gt to gt0
Quoting Andi Shyti (2021-11-17 13:34:56)
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index 089fb4658b216..0bbf8c0c42eac 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -817,7 +817,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane,
> * maximum clocks following a vblank miss (see do_rps_boost()).
> */
> if (!state->rps_interactive) {
> - intel_rps_mark_interactive(&dev_priv->gt.rps, true);
> + intel_rps_mark_interactive(&dev_priv->gt0.rps, true);
This should be across all gt, so probably wants a fresh interface that
takes i915 and does for_each_gt in a later patch. (Since we could be
rendering on a remote tile to present on a display.)
> state->rps_interactive = true;
> }
>
> @@ -851,7 +851,7 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
> return;
>
> if (state->rps_interactive) {
> - intel_rps_mark_interactive(&dev_priv->gt.rps, false);
> + intel_rps_mark_interactive(&dev_priv->gt0.rps, false);
> state->rps_interactive = false;
> }
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 0ceee8ac66717..d4fcd8f236476 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -838,7 +838,7 @@ __intel_display_resume(struct drm_device *dev,
> static bool gpu_reset_clobbers_display(struct drm_i915_private *dev_priv)
> {
> return (INTEL_INFO(dev_priv)->gpu_reset_clobbers_display &&
> - intel_has_gpu_reset(&dev_priv->gt));
> + intel_has_gpu_reset(&dev_priv->gt0));
All these display consumers probably want to use
dev_priv->ggtt->vm.gt, since the scanout capable GGTT would seem to be
the defining feature.
to_scanout_gt(i915) ?
> static bool pxp_is_borked(struct drm_i915_gem_object *obj)
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index ebd775cb1661c..c62253d0af044 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -237,7 +237,7 @@ static int proto_context_set_persistence(struct
> drm_i915_private *i915,
> * colateral damage, and we should not pretend we can by
> * exposing the interface.
> */
> - if (!intel_has_reset_engine(&i915->gt))
> + if (!intel_has_reset_engine(&i915->gt0))
> return -ENODEV;
Prep for all gt. A lot of these need an all-gt interface so we don't
have for_each_gt spread all other the place.
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
> b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
> index ef22d4ed66ad6..69ad407eb15f3 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
> @@ -166,7 +166,7 @@ static struct dma_fence *i915_ttm_accel_move(struct
> ttm_buffer_object *bo,
> enum i915_cache_level src_level, dst_level;
> int ret;
>
> - if (!i915->gt.migrate.context || intel_gt_is_wedged(&i915->gt))
> + if (!i915->gt0.migrate.context || intel_gt_is_wedged(&i915->gt0))
This should already be looking at lmem->gt
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c
> b/drivers/gpu/drm/i915/gt/intel_engine_user.c
> index 8f8bea08e734d..176ea5c7d422f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_user.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c
> @@ -116,7 +116,7 @@ static void set_scheduler_caps(struct drm_i915_private
> *i915)
> disabled |= (I915_SCHEDULER_CAP_ENABLED |
> I915_SCHEDULER_CAP_PRIORITY);
>
> - if (intel_uc_uses_guc_submission(&i915->gt.uc))
> + if (intel_uc_uses_guc_submission(&i915->gt0.uc))
This shouldn't be looking at gt at all, but if it must, that information
must be coming via engine->gt. Kind of renders the mapping moot
currently.
> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c
> b/drivers/gpu/drm/i915/gt/intel_rps.c
> index 07ff7ba7b2b71..63089e671a242 100644
> --- a/drivers/gpu/drm/i915/gt/intel_rps.c
> +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
> @@ -2302,7 +2302,7 @@ unsigned long i915_read_mch_val(void)
> return 0;
>
> with_intel_runtime_pm(&i915->runtime_pm, wakeref) {
> - struct intel_ips *ips = &i915->gt.rps.ips;
> + struct intel_ips *ips = &i915->gt0.rps.ips;
Make mchdev_get() return the gt or rps, at the slight cost of making the
drm_dev_put() more complicated (but can be pushed into a mchdev_put for
symmetry).
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> b/drivers/gp
