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

[Michel Dänzer]

[ 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.

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? ...


> @@ -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.


> @@ -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?

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.


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