[PATCH] drm/amdgpu: For virtual_display feature, define a variable for vblank count of cpu vsync timer.
On 16/08/16 04:00 PM, Deng, Emily wrote: >> From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On Behalf Of >> Michel D?nzer >> Sent: Tuesday, August 16, 2016 2:43 PM >> On 16/08/16 03:28 PM, Deng, Emily wrote: From: Michel Dänzer [mailto:michel at daenzer.net] Sent: Tuesday, August 16, 2016 12:01 PM On 16/08/16 12:49 PM, Deng, Emily wrote: > Hi Michel, Thanks, I still couldn't see the issue that use a > variable to calculate the vblank count when vsync timer interrupt > occurs. I just think it only emulates the hardware behavior. And the > code change will only in virtual display files which won't touch drm > layer. I incline to not modify the code in drm layer or amdgpu other > codes, because it may lead to other issues . I see it basically the other way around: We are currently pretending to the DRM core code that we have a reliable hardware vblank counter and timing even in the virtual DCE case, when we really don't. We should stop pretending and instead communicate the lack of these hardware facilities in the virtual DCE case as intended, otherwise we'll >> probably run into issues sooner or later. >>> [[EmilyD]] Hi Michel, yes, you are right, we are pretending a hardware >>> vblank counter in virtual DCE case to drm layer. But can you specific some >> cases that we must communicate with drm layer that we don't have the vblank >> counter. >> >> I described all the necessary steps (that I know of; there may be more) in >> https://lists.freedesktop.org/archives/amd-gfx/2016-August/001342.html . >> > [[EmilyD]] Hi Michel, I means can you detail the reasons or cases that we > should communicate with drm layer > when don't have the vblank counter instead of pretending a hardware vblank > counter in virtual DCE case. The DRM core code expects the get_vblank_counter hook to use a reliable hardware counter. If that is not the case, it should always return 0. Similarly, drm_calc_vbltimestamp_from_scanoutpos expects the get_scanout_position hook to return accurate scanout position values based on reliable hardware registers. If we pretend to return accurate values from these when we actually can't, the DRM core code may provide incorrect vblank counter / timestamp values to userspace, or worse. Those are just off the top of my head, there may be more along the same lines. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer
[PATCH] drm/amdgpu: For virtual_display feature, define a variable for vblank count of cpu vsync timer.
On 16/08/16 03:28 PM, Deng, Emily wrote: >> From: Michel Dänzer [mailto:michel at daenzer.net] >> Sent: Tuesday, August 16, 2016 12:01 PM >> On 16/08/16 12:49 PM, Deng, Emily wrote: >>> Hi Michel, Thanks, I still couldn't see the issue that use a variable >>> to calculate the vblank count when vsync timer interrupt occurs. I >>> just think it only emulates the hardware behavior. And the code change >>> will only in virtual display files which won't touch drm layer. I >>> incline to not modify the code in drm layer or amdgpu other codes, >>> because it may lead to other issues . >> >> I see it basically the other way around: We are currently pretending to the >> DRM >> core code that we have a reliable hardware vblank counter and timing even in >> the virtual DCE case, when we really don't. We should stop pretending and >> instead communicate the lack of these hardware facilities in the virtual DCE >> case >> as intended, otherwise we'll probably run into issues sooner or later. >> > [[EmilyD]] Hi Michel, yes, you are right, we are pretending a hardware vblank > counter in virtual DCE case to drm > layer. But can you specific some cases that we must communicate with drm > layer that we don't have the vblank counter. I described all the necessary steps (that I know of; there may be more) in https://lists.freedesktop.org/archives/amd-gfx/2016-August/001342.html . -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer
[PATCH] drm/amdgpu: For virtual_display feature, define a variable for vblank count of cpu vsync timer.
On 16/08/16 12:49 PM, Deng, Emily wrote: > Hi Michel, Thanks, I still couldn't see the issue that use a variable > to calculate the vblank count when vsync timer interrupt occurs. I > just think it only emulates the hardware behavior. And the code > change will only in virtual display files which won't touch drm > layer. I incline to not modify the code in drm layer or amdgpu other > codes, because it may lead to other issues . I see it basically the other way around: We are currently pretending to the DRM core code that we have a reliable hardware vblank counter and timing even in the virtual DCE case, when we really don't. We should stop pretending and instead communicate the lack of these hardware facilities in the virtual DCE case as intended, otherwise we'll probably run into issues sooner or later. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer
[PATCH] drm/amdgpu: For virtual_display feature, define a variable for vblank count of cpu vsync timer.
On 15/08/16 03:45 PM, Deng, Emily wrote: >> From: Michel Dänzer [mailto:michel at daenzer.net] >> Sent: Monday, August 15, 2016 9:46 AM >> 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 >> >> [...] >> >>> 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? That's its purpose. BTW, I realized in the meantime that we can't use drm_vblank_no_hw_counter directly, because there's a single struct drm_driver used by all amdgpu driver instances. >> 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. Looking at the drm_update_vblank_count() code, you also need to do the following in the virtual DCE case: * Set dev->max_vblank_count = 0 * Make amdgpu_get_vblank_timestamp_kms either return values based on when the virtual vblank interrupt timer last fired, or just return a negative error code immediately, instead of calling drm_calc_vbltimestamp_from_scanoutpos * Make amdgpu_get_vblank_counter_kms take the else path (or just return 0 immediately), not run any of the scanout position related code -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer
[PATCH] drm/amdgpu: For virtual_display feature, define a variable for vblank count of cpu vsync timer.
Hi Michel and Daniel, I just tryed Michel's advice: return 0 in vblank_get_counter, and set dev->max_vblank_count = 0 , and found the adev->ddev->vblank[crtc].count also can increase which makes the 3D app with vsync can work properly as well. But I don't know the principle. Anyway I will take Michel's advice about vblank count, and send a patch later. Best Wishes, Emily Deng > -Original Message- > From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On Behalf Of > Deng, Emily > Sent: Tuesday, August 16, 2016 5:14 PM > To: Michel Dänzer ; Daniel Vetter > > Cc: dri-devel at lists.freedesktop.org; amd-gfx at lists.freedesktop.org > Subject: RE: [PATCH] drm/amdgpu: For virtual_display feature, define a > variable > for vblank count of cpu vsync timer. > > Hi Michel and Daniel, > Return the fake vblank count or return 0 in vblank_get_counter is only > the > virtual display feature's behavior, and the virtual display feature need to be > enabled by set module parameter, so won't affect normal case. And I think the > vblank counter will be increased every time when the vsync timer interrupt > occurs in virtual display feature is reasonable, so that the 3D app about > vblank > count could work normally. For the time stamp could be set to limitations for > virtual display feature. > > Best Wishes, > Emily Deng > > -Original Message- > > From: Michel Dänzer [mailto:michel at daenzer.net] > > Sent: Tuesday, August 16, 2016 3:18 PM > > To: Deng, Emily > > 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. > > > > On 16/08/16 04:00 PM, Deng, Emily wrote: > > >> From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On > > >> Behalf Of Michel D?nzer > > >> Sent: Tuesday, August 16, 2016 2:43 PM On 16/08/16 03:28 PM, Deng, > > >> Emily wrote: > > From: Michel Dänzer [mailto:michel at daenzer.net] > > Sent: Tuesday, August 16, 2016 12:01 PM On 16/08/16 12:49 PM, > > Deng, Emily wrote: > > > Hi Michel, Thanks, I still couldn't see the issue that use a > > > variable to calculate the vblank count when vsync timer > > > interrupt occurs. I just think it only emulates the hardware > > > behavior. And the code change will only in virtual display files > > > which won't touch drm layer. I incline to not modify the code in > > > drm layer or amdgpu other codes, because it may lead to other issues . > > > > I see it basically the other way around: We are currently > > pretending to the DRM core code that we have a reliable hardware > > vblank counter and timing even in the virtual DCE case, when we > > really don't. We should stop pretending and instead communicate > > the lack of these hardware facilities in the virtual DCE case as > > intended, otherwise we'll > > >> probably run into issues sooner or later. > > > > >>> [[EmilyD]] Hi Michel, yes, you are right, we are pretending a > > >>> hardware vblank counter in virtual DCE case to drm layer. But can > > >>> you specific some > > >> cases that we must communicate with drm layer that we don't have > > >> the vblank counter. > > >> > > >> I described all the necessary steps (that I know of; there may be > > >> more) in https://lists.freedesktop.org/archives/amd-gfx/2016- > > August/001342.html . > > >> > > > [[EmilyD]] Hi Michel, I means can you detail the reasons or cases > > > that we should communicate with drm layer when don't have the vblank > > > counter > > instead of pretending a hardware vblank counter in virtual DCE case. > > > > The DRM core code expects the get_vblank_counter hook to use a > > reliable hardware counter. If that is not the case, it should always return > > 0. > > > > Similarly, drm_calc_vbltimestamp_from_scanoutpos expects the > > get_scanout_position hook to return accurate scanout position values > > based on reliable hardware registers. > > > > If we pretend to return accurate values from these when we actually > > can't, the DRM core code may provide incorrect vblank counter / > > timestamp values to userspace, or worse. > > > > Those are just off the top of my head, there may be more along the same > > lines. > > > > > > -- > > Earthling Michel Dänzer | http://www.amd.com > > Libre software enthusiast | Mesa and X developer > ___ > amd-gfx mailing list > amd-gfx at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amdgpu: For virtual_display feature, define a variable for vblank count of cpu vsync timer.
On Tue, Aug 16, 2016 at 03:43:07PM +0900, Michel Dänzer wrote: > On 16/08/16 03:28 PM, Deng, Emily wrote: > >> From: Michel Dänzer [mailto:michel at daenzer.net] > >> Sent: Tuesday, August 16, 2016 12:01 PM > >> On 16/08/16 12:49 PM, Deng, Emily wrote: > >>> Hi Michel, Thanks, I still couldn't see the issue that use a variable > >>> to calculate the vblank count when vsync timer interrupt occurs. I > >>> just think it only emulates the hardware behavior. And the code change > >>> will only in virtual display files which won't touch drm layer. I > >>> incline to not modify the code in drm layer or amdgpu other codes, > >>> because it may lead to other issues . > >> > >> I see it basically the other way around: We are currently pretending to > >> the DRM > >> core code that we have a reliable hardware vblank counter and timing even > >> in > >> the virtual DCE case, when we really don't. We should stop pretending and > >> instead communicate the lack of these hardware facilities in the virtual > >> DCE case > >> as intended, otherwise we'll probably run into issues sooner or later. Yes please don't lie to the vblank core. In the future we might depend more on this, and that might lead to surprises. > > [[EmilyD]] Hi Michel, yes, you are right, we are pretending a hardware > > vblank counter in virtual DCE case to drm > > layer. But can you specific some cases that we must communicate with drm > > layer that we don't have the vblank counter. > > I described all the necessary steps (that I know of; there may be more) > in https://lists.freedesktop.org/archives/amd-gfx/2016-August/001342.html . Yeah I missed the accurate timestamp part, since on i915 on gen2 we can still do that part. Your todo list looks accurate from what I can tell. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[PATCH] drm/amdgpu: For virtual_display feature, define a variable for vblank count of cpu vsync timer.
Hi Michel and Daniel, Return the fake vblank count or return 0 in vblank_get_counter is only the virtual display feature's behavior, and the virtual display feature need to be enabled by set module parameter, so won't affect normal case. And I think the vblank counter will be increased every time when the vsync timer interrupt occurs in virtual display feature is reasonable, so that the 3D app about vblank count could work normally. For the time stamp could be set to limitations for virtual display feature. Best Wishes, Emily Deng > -Original Message- > From: Michel Dänzer [mailto:michel at daenzer.net] > Sent: Tuesday, August 16, 2016 3:18 PM > To: Deng, Emily > 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. > > On 16/08/16 04:00 PM, Deng, Emily wrote: > >> From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On > >> Behalf Of Michel D?nzer > >> Sent: Tuesday, August 16, 2016 2:43 PM On 16/08/16 03:28 PM, Deng, > >> Emily wrote: > From: Michel Dänzer [mailto:michel at daenzer.net] > Sent: Tuesday, August 16, 2016 12:01 PM On 16/08/16 12:49 PM, Deng, > Emily wrote: > > Hi Michel, Thanks, I still couldn't see the issue that use a > > variable to calculate the vblank count when vsync timer interrupt > > occurs. I just think it only emulates the hardware behavior. And > > the code change will only in virtual display files which won't > > touch drm layer. I incline to not modify the code in drm layer or > > amdgpu other codes, because it may lead to other issues . > > I see it basically the other way around: We are currently > pretending to the DRM core code that we have a reliable hardware > vblank counter and timing even in the virtual DCE case, when we > really don't. We should stop pretending and instead communicate the > lack of these hardware facilities in the virtual DCE case as > intended, otherwise we'll > >> probably run into issues sooner or later. > > >>> [[EmilyD]] Hi Michel, yes, you are right, we are pretending a > >>> hardware vblank counter in virtual DCE case to drm layer. But can > >>> you specific some > >> cases that we must communicate with drm layer that we don't have the > >> vblank counter. > >> > >> I described all the necessary steps (that I know of; there may be > >> more) in https://lists.freedesktop.org/archives/amd-gfx/2016- > August/001342.html . > >> > > [[EmilyD]] Hi Michel, I means can you detail the reasons or cases that > > we should communicate with drm layer when don't have the vblank counter > instead of pretending a hardware vblank counter in virtual DCE case. > > The DRM core code expects the get_vblank_counter hook to use a reliable > hardware counter. If that is not the case, it should always return 0. > > Similarly, drm_calc_vbltimestamp_from_scanoutpos expects the > get_scanout_position hook to return accurate scanout position values based on > reliable hardware registers. > > If we pretend to return accurate values from these when we actually can't, the > DRM core code may provide incorrect vblank counter / timestamp values to > userspace, or worse. > > Those are just off the top of my head, there may be more along the same lines. > > > -- > Earthling Michel Dänzer | http://www.amd.com > Libre software enthusiast | Mesa and X developer
[PATCH] drm/amdgpu: For virtual_display feature, define a variable for vblank count of cpu vsync timer.
> -Original Message- > From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On Behalf Of > Michel D?nzer > Sent: Tuesday, August 16, 2016 2:43 PM > To: Deng, Emily > Cc: dri-devel at lists.freedesktop.org; amd-gfx at lists.freedesktop.org > Subject: Re: [PATCH] drm/amdgpu: For virtual_display feature, define a > variable > for vblank count of cpu vsync timer. > > On 16/08/16 03:28 PM, Deng, Emily wrote: > >> From: Michel Dänzer [mailto:michel at daenzer.net] > >> Sent: Tuesday, August 16, 2016 12:01 PM On 16/08/16 12:49 PM, Deng, > >> Emily wrote: > >>> Hi Michel, Thanks, I still couldn't see the issue that use a > >>> variable to calculate the vblank count when vsync timer interrupt > >>> occurs. I just think it only emulates the hardware behavior. And the > >>> code change will only in virtual display files which won't touch drm > >>> layer. I incline to not modify the code in drm layer or amdgpu other > >>> codes, because it may lead to other issues . > >> > >> I see it basically the other way around: We are currently pretending > >> to the DRM core code that we have a reliable hardware vblank counter > >> and timing even in the virtual DCE case, when we really don't. We > >> should stop pretending and instead communicate the lack of these > >> hardware facilities in the virtual DCE case as intended, otherwise we'll > probably run into issues sooner or later. > >> > > [[EmilyD]] Hi Michel, yes, you are right, we are pretending a hardware > > vblank counter in virtual DCE case to drm layer. But can you specific some > cases that we must communicate with drm layer that we don't have the vblank > counter. > > I described all the necessary steps (that I know of; there may be more) in > https://lists.freedesktop.org/archives/amd-gfx/2016-August/001342.html . > [[EmilyD]] Hi Michel, I means can you detail the reasons or cases that we should communicate with drm layer when don't have the vblank counter instead of pretending a hardware vblank counter in virtual DCE case. > > -- > Earthling Michel Dänzer | http://www.amd.com > Libre software enthusiast | Mesa and X developer > ___ > amd-gfx mailing list > amd-gfx at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amdgpu: For virtual_display feature, define a variable for vblank count of cpu vsync timer.
> -Original Message- > From: Michel Dänzer [mailto:michel at daenzer.net] > Sent: Tuesday, August 16, 2016 12:01 PM > To: Deng, Emily > Cc: dri-devel at lists.freedesktop.org; amd-gfx at lists.freedesktop.org > Subject: Re: [PATCH] drm/amdgpu: For virtual_display feature, define a > variable > for vblank count of cpu vsync timer. > > On 16/08/16 12:49 PM, Deng, Emily wrote: > > Hi Michel, Thanks, I still couldn't see the issue that use a variable > > to calculate the vblank count when vsync timer interrupt occurs. I > > just think it only emulates the hardware behavior. And the code change > > will only in virtual display files which won't touch drm layer. I > > incline to not modify the code in drm layer or amdgpu other codes, > > because it may lead to other issues . > > I see it basically the other way around: We are currently pretending to the > DRM > core code that we have a reliable hardware vblank counter and timing even in > the virtual DCE case, when we really don't. We should stop pretending and > instead communicate the lack of these hardware facilities in the virtual DCE > case > as intended, otherwise we'll probably run into issues sooner or later. > [[EmilyD]] Hi Michel, yes, you are right, we are pretending a hardware vblank counter in virtual DCE case to drm layer. But can you specific some cases that we must communicate with drm layer that we don't have the vblank counter. > > -- > Earthling Michel Dänzer | http://www.amd.com > Libre software enthusiast | Mesa and X developer
[PATCH] drm/amdgpu: For virtual_display feature, define a variable for vblank count of cpu vsync timer.
Hi Michel, Thanks, I still couldn't see the issue that use a variable to calculate the vblank count when vsync timer interrupt occurs. I just think it only emulates the hardware behavior. And the code change will only in virtual display files which won't touch drm layer. I incline to not modify the code in drm layer or amdgpu other codes, because it may lead to other issues . Best Wishes, Emily Deng > -Original Message- > From: Michel Dänzer [mailto:michel at daenzer.net] > Sent: Tuesday, August 16, 2016 11:12 AM > To: Deng, Emily > 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. > > On 15/08/16 03:45 PM, Deng, Emily wrote: > >> From: Michel Dänzer [mailto:michel at daenzer.net] > >> Sent: Monday, August 15, 2016 9:46 AM 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 > >> > >> [...] > >> > >>> 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? > > That's its purpose. BTW, I realized in the meantime that we can't use > drm_vblank_no_hw_counter directly, because there's a single struct > drm_driver used by all amdgpu driver instances. > > > >> 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 > > adev->ddev->vbl.request.sequence will be 2, but the > > adev->ddev->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. > > Looking at the drm_update_vblank_count() code, you also need to do the > following in the virtual DCE case: > > * Set dev->max_vblank_count = 0 > * Make amdgpu_get_vblank_timestamp_kms either return values based on > when the virtual vblank interrupt timer last fired, or just return a > negative error code immediately, instead of calling > drm_calc_vbltimestamp_from_scanoutpos > * Make amdgpu_get_vblank_counter_kms take the else path (or just return > 0 immediately), not run any of the scanout position related code > > > -- > Earthling Michel Dänzer | http://www.amd.com > Libre software enthusiast | Mesa and X developer
[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
[...]
> 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
[PATCH] drm/amdgpu: For virtual_display feature, define a variable for vblank count of cpu vsync timer.
> -Original Message-
> From: Michel Dänzer [mailto:michel at daenzer.net]
> Sent: Monday, August 15, 2016 9:46 AM
> To: Deng, Emily
> 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
>
> [...]
>
> > 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
