Re: [Intel-gfx] [PATCH v4 2/5] drm/i915/display: handle migration for dpt

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel