Re: [PATCH 06/18] drm/amd/display: Drop underlay plane support

From: "Kazlauskas, Nicholas" <Nicholas.Kazlauskas-5C7GfCeVMHo@public.gmane.org>
To: "Michel Dänzer" <michel-otUistvHUpPR7s880joybQ@public.gmane.org>
Cc: "amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org"
	<amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
Subject: Re: [PATCH 06/18] drm/amd/display: Drop underlay plane support
Date: Tue, 5 Mar 2019 17:15:24 +0000	[thread overview]
Message-ID: <0090d693-da7d-ec97-4d2e-5a109d2bc4fc@amd.com> (raw)
In-Reply-To: <daf986ad-a939-05d2-8ee5-e103727d6b1d-otUistvHUpPR7s880joybQ@public.gmane.org>

On 3/5/19 12:09 PM, Michel Dänzer wrote:
> On 2019-02-25 11:46 p.m., Bhawanpreet Lakha wrote:
>> From: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>>
>> [Why]
>> Primary and underlay planes were previously exposed to DRM by using
>> max_planes and max_slave_planes.
>>
>> The value for max_planes was always pipe_count + has_underlay.
>> If there was an underlay pipe, then max_slave_planes = 1.
>>
>> Raven has pipe_count = 4, max_planes = 4, and max_slave_planes = 1.
>> So during plane initialziation it was actually "creating"
>> 1 overlay plane and 3 primary planes... or it would be, had its
>> plane_type array not been dm_plane_type_default, which will only create
>> DRM_PLANE_TYPE_PRIMARY planes.
>>
>> We can expose primary planes as supporting more than one CRTC at a time
>> to more closely resemble plane behavior on DCN but userspace doesn't
>> really expect planes to be used in this manner and will either
>> ignore the planes or crash.
>>
>> Planes with index greater than max_streams are marked as supporting
>> all CRTCs. No ASIC currently has primary plane count greater than the
>> stream count but we shouldn't expose more than necessary.
>>
>> [How]
>> Drop support for underlay planes. They aren't well tested and don't
>> fully work right at the moment.
>>
>> Only create one primary plane per CRTC so we're not creating overlays.
>>
>> Initialize plane types directly instead of referencing a misleading
>> array of plane types.
>>
>> Change-Id: Ibdf1caa8a2be42df6f7574dae97c49734fa44151
>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>> Reviewed-by: Tony Cheng <Tony.Cheng@amd.com>
>> Acked-by: Bhawanpreet Lakha <Bhawanpreet.Lakha@amd.com>
>>
>>
>> [...]
>>
>> @@ -1925,21 +1901,17 @@ static int amdgpu_dm_initialize_drm_device(struct amdgpu_device *adev)
>>   		return -EINVAL;
>>   	}
>>   
>> -	/* Identify the number of planes to be initialized */
>> -	total_overlay_planes = dm->dc->caps.max_slave_planes;
>> -	total_primary_planes = dm->dc->caps.max_planes - dm->dc->caps.max_slave_planes;
>> +	/* There is one primary plane per CRTC */
>> +	primary_planes = dm->dc->caps.max_streams;
>> +	ASSERT(primary_planes < AMDGPU_MAX_PLANES);
> 
> This assertion fails for me with Bonaire. In fact, since
> AMDGPU_MAX_PLANES is 6, and primary_planes seems to be the number of
> CRTCs, it'll fail with most GPUs?
> 
> Maybe it was meant to be <= instead of < ?
> 
> 

Yeah, that should have been a <= . It would fail on anything with 6 at 
least. I think because it happens early in the init process it doesn't 
show any warnings or errors in IGT testing.

I'll go ahead and fix this one up, thanks.

I think I could probably drop the whole mode_info->planes array in a 
separate patch too since it doesn't really serve much of a purpose now 
(it was only used for checking the type during initialization).

Nicholas Kazlauskas
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx