[PATCH 3/4] drm/amd/display: Switch to using atomic_helper for flip.

[Michel Dänzer]

On 17/01/17 07:16 AM, Laurent Pinchart wrote:
> On Monday 16 Jan 2017 10:44:57 Andrey Grodzovsky wrote:
>> Change-Id: Iad3e0b9b3546e4e4dc79be9233daf4fe4dba83e0
>> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky at amd.com>
>> ---
>>  .../drm/amd/display/amdgpu_dm/amdgpu_dm_types.c    | 92 ++----------------
>>  1 file changed, 6 insertions(+), 86 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_types.c
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_types.c index
>> a443b70..d4664bf 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_types.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_types.c
>> @@ -1060,83 +1060,6 @@ static int dm_crtc_funcs_atomic_set_property(
>>  	return 0;
>>  }
>>
>> -
>> -static int amdgpu_atomic_helper_page_flip(struct drm_crtc *crtc,
>> -				struct drm_framebuffer *fb,
>> -				struct drm_pending_vblank_event *event,
>> -				uint32_t flags)
>> -{
>> -	struct drm_plane *plane = crtc->primary;
>> -	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>> -	struct drm_atomic_state *state;
>> -	struct drm_plane_state *plane_state;
>> -	struct drm_crtc_state *crtc_state;
>> -	int ret = 0;
>> -
>> -	state = drm_atomic_state_alloc(plane->dev);
>> -	if (!state)
>> -		return -ENOMEM;
>> -
>> -	ret = drm_crtc_vblank_get(crtc);
> 
> The DRM core's atomic page flip helper doesn't get/put vblank. Have you 
> double-checked that removing them isn't a problem ?

This patch makes the amdgpu DM code use the page_flip_target hook.
drm_mode_page_flip_ioctl calls drm_crtc_vblank_get before the
page_flip_target hook.

You're right though that the fact that drm_atomic_helper_page_flip
doesn't call drm_crtc_vblank_get is a bit alarming. From the
DRM_IOCTL_MODE_PAGE_FLIP POV, drm_crtc_vblank_get must be called when
userspace calls the ioctl (either by drm_mode_page_flip_ioctl or the
page_flip hook implementation), and drm_crtc_vblank_put must be called
when the flip completes and the event is sent to userspace. How is this
supposed to be handled with atomic?

Andrey, did you check via code audit and/or testing that the vblank
reference count is still balanced after this change?


>> @@ -3143,8 +3064,7 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
>>  			 * 1. This commit is not a page flip.
>>  			 * 2. This commit is a page flip, and targets are 
> created.
>>  			 */
>> -			if (!page_flip_needed(plane_state, old_plane_state,
>> -					      true) ||
>> +			if (!page_flip_needed(plane_state, old_plane_state, 
> true) ||
> 
> This seems to be an unrelated change.

Yeah, such whitespace-only changes should be dropped.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer