Re: [PATCH] drm/amd/display: remove need of modeset flag for overlay planes

["S, Shirish" <sshankar-5C7GfCeVMHo@public.gmane.org>]

On 5/2/2018 7:21 PM, Harry Wentland wrote:
> On 2018-04-27 06:27 AM, Shirish S wrote:
>> This patch is in continuation to the
>> "843e3c7 drm/amd/display: defer modeset check in dm_update_planes_state"
>> where we started to eliminate the dependency on
>> DRM_MODE_ATOMIC_ALLOW_MODESET to be set by the user space,
>> which as such is not mandatory.
>>
>> After deferring, this patch eliminates the dependency on the flag
>> for overlay planes.
>>
> Apologies for the late response. I had to think about this patch for a long time since I'm not quite comfortable with it.
>
>> This has to be done in stages as its a pretty complex and requires thorough
>> testing before we free primary planes as well from dependency on modeset
>> flag.
>>
>> Signed-off-by: Shirish S <shirish.s@amd.com>
>> ---
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 1a63c04..87b661d 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -4174,7 +4174,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>>   		}
>>   		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>>   
>> -		if (!pflip_needed) {
>> +		if (!pflip_needed || plane->type == DRM_PLANE_TYPE_OVERLAY) {
> Does this mean that whenever we have an overlay plane we won't do amdgpu_dm_do_flip but commit_planes_to_stream instead? Is this really the behavior we want?
>
> commit_planes_to_stream was intended to program a new surface on a modeset whereas amdgpu_dm_do_flip was intended for pageflips.
Need of "modeset" flag to program new surface is what we want to fix in 
this patch for underlay plane and in next stages, fix manifestations 
caused by this approach as and when seen.
Since the user space doesn't send modeset flag for new surface, hence to 
program it, this patch checks the plane type to construct planes_count 
before calling commit_planes_to_stream().

  If we use commit_planes_to_stream we end up losing things like the immediate_flip flag, as well as the wait for the right moment to program the flip that amdgpu_dm_do_flip does.

 From the code, amdgpu_dm_do_flip does what you mentioned only for 
primary plane and hence either way its not set for underlay.

Regards,
Shirish S
>   Even more importantly we won't wait for fences (reservation_object_wait_timeout_rcu).
>
> Harry
>
>>   			WARN_ON(!dm_new_plane_state->dc_state);
>>   
>>   			plane_states_constructed[planes_count] = dm_new_plane_state->dc_state;
>> @@ -4884,7 +4884,8 @@ static int dm_update_planes_state(struct dc *dc,
>>   
>>   		/* Remove any changed/removed planes */
>>   		if (!enable) {
>> -			if (pflip_needed)
>> +			if (pflip_needed &&
>> +			    plane && plane->type != DRM_PLANE_TYPE_OVERLAY)
>>   				continue;
>>   
>>   			if (!old_plane_crtc)
>> @@ -4931,7 +4932,8 @@ static int dm_update_planes_state(struct dc *dc,
>>   			if (!dm_new_crtc_state->stream)
>>   				continue;
>>   
>> -			if (pflip_needed)
>> +			if (pflip_needed &&
>> +			    plane && plane->type != DRM_PLANE_TYPE_OVERLAY)
>>   				continue;
>>   
>>   			WARN_ON(dm_new_plane_state->dc_state);
>>

-- 
Regards,
Shirish S

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