[PATCH] drm/amdgpu: For virtual_display feature, define a variable for vblank count of cpu vsync timer.

[Deng, Emily]

> -----Original Message-----
> From: Michel Dänzer [mailto:michel at daenzer.net]
> Sent: Monday, August 15, 2016 9:46 AM
> To: Deng, Emily <Emily.Deng at amd.com>
> Cc: amd-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: For virtual_display feature, define a variable
> for vblank count of cpu vsync timer.
> 
> 
> [ Adding the dri-devel list ]
> 
> On 11/08/16 12:46 PM, Emily Deng wrote:
> > The adev->ddev->vblank[crtc].count couldn't be used here, so define
> > another variable to compute the vblank count.
> >
> > Signed-off-by: Emily Deng <Emily.Deng at amd.com>
> 
> [...]
> 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_virtual.c
> > b/drivers/gpu/drm/amd/amdgpu/dce_virtual.c
> > index 2ce5f90..d616ab9 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/dce_virtual.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/dce_virtual.c
> > @@ -58,7 +58,7 @@ static u32 dce_virtual_vblank_get_counter(struct
> amdgpu_device *adev, int crtc)
> >  	if (crtc >= adev->mode_info.num_crtc)
> >  		return 0;
> >  	else
> > -		return adev->ddev->vblank[crtc].count;
> > +		return adev->mode_info.timer_vblank_count;
> >  }
> 
> AFAIK the vblank_get_counter hook is supposed to always return 0 when
> there's no hardware frame counter which can be used. That's what
> drm_vblank_no_hw_counter was created for.
>
[[EmilyD]] Sorry, I don't know much about drm_vblank_no_hw_counter, can it support vsync when 
return to 0?
> 
> You mentioned internally that you ran into trouble when trying this though.
> Please provide more information about that, e.g.: Which base kernel version did
> you test it with? What values did the DRM_IOCTL_WAIT_VBLANK ioctl return to
> userspace? ...
> 
[[EmilyD]] I run the driver on kernel version 4.6, and run glxgears with vsync enabled. It is hard to detail the issue, I will try my best to description 
the issue I found. 
After I double checked, it is not segment fault in libGL.so  when run glxgears with vsync, but the glxgears will  be stucked in waiting for the before
swap buffers to complete. This is because when enable vsync, every time swap buffers, the DDX driver will call DRM_IOCTL_WAIT_VBLANK  to 
queue the vblank event, and the vbl.request.sequence will be set to current_vblank_count + swap_interval. Then in kernel driver, when timer 
interrupt occurs, it will call drm_handle_vblank_events, it will call drm_vblank_count_and_time to get current seq, and only seq >= vbl.request.sequence,
then will call send_vblank_event. So it will never call send_vblank_event.
For example, the DDX driver call DRM_IOCTL_WAIT_VBLANK , then kernel driver will call drm_queue_vblank_event, and current vblank_count is 1
(As we only return 0 in vblank_get_counter, so the vblank_count will never change except calling drm_reset_vblank_timestamp which will make  
adev->ddev->vblank[crtc].count++), and swap_interval is 1, then vbl.request.sequence will be 2, but the drm_vblank_count_and_time will always
 return 1 except calling drm_reset_vblank_timestamp(The function drm_reset_vblank_timestamp will only be called in drm_vblank_post_modeset
 and drm_vblank_on ), so the send_vblank_event will never be called, and swap buffers won't complete, so the glxgears will be stucked. 
> 
> > @@ -353,7 +353,6 @@ static int dce_virtual_crtc_init(struct
> > amdgpu_device *adev, int index)  static int
> > dce_virtual_early_init(void *handle)  {
> >  	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > -
> >  	adev->mode_info.vsync_timer_enabled =
> AMDGPU_IRQ_STATE_DISABLE;
> >  	dce_virtual_set_display_funcs(adev);
> >  	dce_virtual_set_irq_funcs(adev);
> 
> BTW, this hunk would break the kernel coding style.
> 
[[EmilyD]] Thanks, I will modify this.
> 
> > @@ -653,7 +653,7 @@ static enum hrtimer_restart
> dce_virtual_vblank_timer_handle(struct hrtimer *vbla
> >  	struct amdgpu_mode_info *mode_info = container_of(vblank_timer,
> struct amdgpu_mode_info ,vblank_timer);
> >  	struct amdgpu_device *adev = container_of(mode_info, struct
> amdgpu_device ,mode_info);
> >  	unsigned crtc = 0;
> > -	adev->ddev->vblank[0].count++;
> > +	adev->mode_info.timer_vblank_count++;
> >  	drm_handle_vblank(adev->ddev, crtc);
> >  	dce_virtual_pageflip_irq(adev, NULL, NULL);
> >  	hrtimer_start(vblank_timer, ktime_set(0,
> DCE_VIRTUAL_VBLANK_PERIOD),
> > HRTIMER_MODE_REL);
> 
> [...]
> 
> > @@ -718,7 +716,7 @@ static int dce_virtual_crtc_irq(struct amdgpu_device
> *adev,
> >  	unsigned crtc = 0;
> >  	unsigned irq_type = AMDGPU_CRTC_IRQ_VBLANK1;
> >
> > -	adev->ddev->vblank[crtc].count++;
> > +	adev->mode_info.timer_vblank_count++;
> >  	dce_virtual_crtc_vblank_int_ack(adev, crtc);
> >
> >  	if (amdgpu_irq_enabled(adev, source, irq_type)) {
> >
> 
> Wouldn't this result in adev->mode_info.timer_vblank_count getting
> incremented twice every time the virtual interrupt timer fires?
> 
[[EmilyD]] Sorry, I don't think so, the dce_virtual_crtc_irq will only be called when hardware vsync timer interrupt occurs.

> Anyway, this approach means that the virtual interrupt timer can never be
> disabled, even while userspace isn't using the vblank functionality, which is
> undesirable.
> 
[[EmilyD]] No, it could be disabled by calling dce_virtual_set_crtc_vblank_interrupt_state.
> 
> --
> Earthling Michel Dänzer               |               http://www.amd.com
> Libre software enthusiast             |             Mesa and X developer