| Message ID | 20221004103311.194409-2-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/5] drm/i915: remove the TODO in pin_and_fence_fb_obj | expand |
On Tue, Oct 04, 2022 at 11:33:08AM +0100, Matthew Auld wrote: > On platforms like DG2, it looks like the dpt path here is missing the > migrate-to-lmem step on discrete platforms. > > Fixes: 33e7a975103c ("drm/i915/xelpd: First stab at DPT support") > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Jianshui Yu <jianshui.yu@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Nirmoy Das <nirmoy.das@intel.com> > --- > drivers/gpu/drm/i915/display/intel_fb_pin.c | 23 ++++++++++++++++++--- > 1 file changed, 20 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c > index 0cd9e8cb078b..32206bd359da 100644 > --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c > +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c > @@ -26,10 +26,17 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb, > struct drm_device *dev = fb->dev; > struct drm_i915_private *dev_priv = to_i915(dev); > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > + struct i915_gem_ww_ctx ww; > struct i915_vma *vma; > u32 alignment; > int ret; > > + /* > + * We are not syncing against the binding (and potential migrations) > + * below, so this vm must never be async. > + */ > + GEM_WARN_ON(vm->bind_async_flags); Not sure why this is different between the dpt and non-dpt paths? > + > if (WARN_ON(!i915_gem_object_is_framebuffer(obj))) > return ERR_PTR(-EINVAL); > > @@ -37,10 +44,20 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb, > > atomic_inc(&dev_priv->gpu_error.pending_fb_pin); > > - ret = i915_gem_object_lock_interruptible(obj, NULL); > - if (!ret) { > + for_i915_gem_ww(&ww, ret, true) { > + ret = i915_gem_object_lock(obj, &ww); > + if (ret) > + continue; > + > + if (HAS_LMEM(dev_priv)) { > + ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0); > + if (ret) > + continue; > + } > + > ret = i915_gem_object_set_cache_level(obj, I915_CACHE_NONE); > - i915_gem_object_unlock(obj); > + if (ret) > + continue; > } The non-dpt path has the whole thing under the same lock. Is there a reason we're not doing the same thing for both? I guess some kind of unification effort would be nice to avoid the codepaths diverging for no good reason. Maybe even some refactoring would be nice to share more code, but IIRC all the fence/mappable stuff in the lower levels of the ggtt paths is what got in the way of just reusing more of the ggtt code directly. > if (ret) { > vma = ERR_PTR(ret); > -- > 2.37.3
On 04/10/2022 12:22, Ville Syrjälä wrote: > On Tue, Oct 04, 2022 at 11:33:08AM +0100, Matthew Auld wrote: >> On platforms like DG2, it looks like the dpt path here is missing the >> migrate-to-lmem step on discrete platforms. >> >> Fixes: 33e7a975103c ("drm/i915/xelpd: First stab at DPT support") >> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >> Cc: Jianshui Yu <jianshui.yu@intel.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Nirmoy Das <nirmoy.das@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_fb_pin.c | 23 ++++++++++++++++++--- >> 1 file changed, 20 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c >> index 0cd9e8cb078b..32206bd359da 100644 >> --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c >> +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c >> @@ -26,10 +26,17 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb, >> struct drm_device *dev = fb->dev; >> struct drm_i915_private *dev_priv = to_i915(dev); >> struct drm_i915_gem_object *obj = intel_fb_obj(fb); >> + struct i915_gem_ww_ctx ww; >> struct i915_vma *vma; >> u32 alignment; >> int ret; >> >> + /* >> + * We are not syncing against the binding (and potential migrations) >> + * below, so this vm must never be async. >> + */ >> + GEM_WARN_ON(vm->bind_async_flags); > > Not sure why this is different between the dpt and non-dpt paths? It looks like dpt is using vma_pin() below which doesn't have the wait_for_bind() stuff, like we do for ggtt_pin(). > >> + >> if (WARN_ON(!i915_gem_object_is_framebuffer(obj))) >> return ERR_PTR(-EINVAL); >> >> @@ -37,10 +44,20 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb, >> >> atomic_inc(&dev_priv->gpu_error.pending_fb_pin); >> >> - ret = i915_gem_object_lock_interruptible(obj, NULL); >> - if (!ret) { >> + for_i915_gem_ww(&ww, ret, true) { >> + ret = i915_gem_object_lock(obj, &ww); >> + if (ret) >> + continue; >> + >> + if (HAS_LMEM(dev_priv)) { >> + ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0); >> + if (ret) >> + continue; >> + } >> + >> ret = i915_gem_object_set_cache_level(obj, I915_CACHE_NONE); >> - i915_gem_object_unlock(obj); >> + if (ret) >> + continue; >> } > > The non-dpt path has the whole thing under the same lock. > Is there a reason we're not doing the same thing for both? > > I guess some kind of unification effort would be nice to > avoid the codepaths diverging for no good reason. Can do, I'll take a look. > > Maybe even some refactoring would be nice to share more code, > but IIRC all the fence/mappable stuff in the lower levels > of the ggtt paths is what got in the way of just reusing > more of the ggtt code directly. > >> if (ret) { >> vma = ERR_PTR(ret); >> -- >> 2.37.3 >
On Tue, Oct 04, 2022 at 12:54:33PM +0100, Matthew Auld wrote: > On 04/10/2022 12:22, Ville Syrjälä wrote: > > On Tue, Oct 04, 2022 at 11:33:08AM +0100, Matthew Auld wrote: > >> On platforms like DG2, it looks like the dpt path here is missing the > >> migrate-to-lmem step on discrete platforms. > >> > >> Fixes: 33e7a975103c ("drm/i915/xelpd: First stab at DPT support") > >> Signed-off-by: Matthew Auld <matthew.auld@intel.com> > >> Cc: Jianshui Yu <jianshui.yu@intel.com> > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> Cc: Nirmoy Das <nirmoy.das@intel.com> > >> --- > >> drivers/gpu/drm/i915/display/intel_fb_pin.c | 23 ++++++++++++++++++--- > >> 1 file changed, 20 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c > >> index 0cd9e8cb078b..32206bd359da 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c > >> +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c > >> @@ -26,10 +26,17 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb, > >> struct drm_device *dev = fb->dev; > >> struct drm_i915_private *dev_priv = to_i915(dev); > >> struct drm_i915_gem_object *obj = intel_fb_obj(fb); > >> + struct i915_gem_ww_ctx ww; > >> struct i915_vma *vma; > >> u32 alignment; > >> int ret; > >> > >> + /* > >> + * We are not syncing against the binding (and potential migrations) > >> + * below, so this vm must never be async. > >> + */ > >> + GEM_WARN_ON(vm->bind_async_flags); > > > > Not sure why this is different between the dpt and non-dpt paths? > > It looks like dpt is using vma_pin() below which doesn't have the > wait_for_bind() stuff, like we do for ggtt_pin(). So I guess more accident than by design. > > > > >> + > >> if (WARN_ON(!i915_gem_object_is_framebuffer(obj))) > >> return ERR_PTR(-EINVAL); > >> > >> @@ -37,10 +44,20 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb, > >> > >> atomic_inc(&dev_priv->gpu_error.pending_fb_pin); > >> > >> - ret = i915_gem_object_lock_interruptible(obj, NULL); > >> - if (!ret) { > >> + for_i915_gem_ww(&ww, ret, true) { > >> + ret = i915_gem_object_lock(obj, &ww); > >> + if (ret) > >> + continue; > >> + > >> + if (HAS_LMEM(dev_priv)) { > >> + ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0); > >> + if (ret) > >> + continue; > >> + } > >> + > >> ret = i915_gem_object_set_cache_level(obj, I915_CACHE_NONE); > >> - i915_gem_object_unlock(obj); > >> + if (ret) > >> + continue; > >> } > > > > The non-dpt path has the whole thing under the same lock. > > Is there a reason we're not doing the same thing for both? > > > > I guess some kind of unification effort would be nice to > > avoid the codepaths diverging for no good reason. > > Can do, I'll take a look. BTW I'm not saying we have to do it right now, but at least something to keep in mind. And to that end would be nice to understand what are the real differences vs. differnces due to accidents of the development history. It would be nice to not have to maintain two totally separate codepaths if possible. > > > > Maybe even some refactoring would be nice to share more code, > > but IIRC all the fence/mappable stuff in the lower levels > > of the ggtt paths is what got in the way of just reusing > > more of the ggtt code directly. > > > >> if (ret) { > >> vma = ERR_PTR(ret); > >> -- > >> 2.37.3 > >
diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c index 0cd9e8cb078b..32206bd359da 100644 --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c @@ -26,10 +26,17 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb, struct drm_device *dev = fb->dev; struct drm_i915_private *dev_priv = to_i915(dev); struct drm_i915_gem_object *obj = intel_fb_obj(fb); + struct i915_gem_ww_ctx ww; struct i915_vma *vma; u32 alignment; int ret; + /* + * We are not syncing against the binding (and potential migrations) + * below, so this vm must never be async. + */ + GEM_WARN_ON(vm->bind_async_flags); + if (WARN_ON(!i915_gem_object_is_framebuffer(obj))) return ERR_PTR(-EINVAL); @@ -37,10 +44,20 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb, atomic_inc(&dev_priv->gpu_error.pending_fb_pin); - ret = i915_gem_object_lock_interruptible(obj, NULL); - if (!ret) { + for_i915_gem_ww(&ww, ret, true) { + ret = i915_gem_object_lock(obj, &ww); + if (ret) + continue; + + if (HAS_LMEM(dev_priv)) { + ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0); + if (ret) + continue; + } + ret = i915_gem_object_set_cache_level(obj, I915_CACHE_NONE); - i915_gem_object_unlock(obj); + if (ret) + continue; } if (ret) { vma = ERR_PTR(ret);
On platforms like DG2, it looks like the dpt path here is missing the migrate-to-lmem step on discrete platforms. Fixes: 33e7a975103c ("drm/i915/xelpd: First stab at DPT support") Signed-off-by: Matthew Auld <matthew.auld@intel.com> Cc: Jianshui Yu <jianshui.yu@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Nirmoy Das <nirmoy.das@intel.com> --- drivers/gpu/drm/i915/display/intel_fb_pin.c | 23 ++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)