[PATCH 1/3] drm/amdgpu: Fix hard hang for S/G display BOs.

[Grodzovsky, Andrey]

On 7/23/19 12:18 PM, Michel Dänzer wrote:
> On 2019-07-23 6:04 p.m., Andrey Grodzovsky wrote:
>> HW requires for caching to be unset for scanout BO
>> mappings when the BO placement is in GTT memory.
>> Usually the flag to unset is passed from user mode
>> but for FB mode this was missing.
>>
>> Suggested-by: Alex Deucher <alexander.deucher at amd.com>
>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky at amd.com>
>> Reviewed-by: Alex Deucher <alexander.deucher at amd.com>
>> Tested-by: Shirish S <shirish.s at amd.com>
>> ---
>>   [...]
>>
>> @@ -166,6 +166,14 @@ static int amdgpufb_create_pinned_object(struct amdgpu_fbdev *rfbdev,
>>   			dev_err(adev->dev, "FB failed to set tiling flags\n");
>>   	}
>>   
>> +	/*
>> +	 * If the AMDGPU_GEM_CREATE_CPU_GTT_USWC flag was removed during BO
>> +	 * creation it means that USWC is not supported for this board and
>> +	 * so to avoid hang caused by placement of scanout BO in GTT on certain
>> +	 * APUs and still light up, force the BO placement to VRAM.
>> +	 */
>> +	if (abo->flags & ~AMDGPU_GEM_CREATE_CPU_GTT_USWC)
>> +		domain  = AMDGPU_GEM_DOMAIN_VRAM;
> The comment sounds like you meant
>
> 	if (!(abo->flags & AMDGPU_GEM_CREATE_CPU_GTT_USWC))
> 		domain  = AMDGPU_GEM_DOMAIN_VRAM;
>
> ?


Yes, not sure how this happened to me...


>
> Anyway, this should be handled in amdgpu_display_supported_domains
> instead (e.g. by not allowing GTT if CONFIG_X86_32 is defined),


We have drm_arch_can_wc_memory function to cover all the cases when USWC 
mapping is not allowed, why the CONFIG_X86_32 here ?


> otherwise the BO could still be pinned to GTT later.

The only other later place I know is dm_plane_helper_prepare_fb of which 
I take care in patch 3. What other places you have in mind ?


Andrey


>
>
>> @@ -761,6 +762,7 @@ int amdgpu_mode_dumb_create(struct drm_file *file_priv,
>>   	args->size = ALIGN(args->size, PAGE_SIZE);
>>   	domain = amdgpu_bo_get_preferred_pin_domain(adev,
>>   				amdgpu_display_supported_domains(adev));
>> +
>>   	r = amdgpu_gem_object_create(adev, args->size, 0, domain, flags,
>>   				     ttm_bo_type_device, NULL, &gobj);
>>   	if (r)
>>
> Drop this hunk with only whitespace changes.
>
>