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

[Matthew Auld <matthew.auld@intel.com>]

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
>