Re: [Freedreno] [Intel-gfx] [PATCH] drm/vblank: drop the mode argument from drm_calc_vbltimestamp_from_scanoutpos
On Thu, Mar 30, 2017 at 08:27:40PM +0200, Daniel Vetter wrote: > On Thu, Mar 30, 2017 at 04:41:57PM +0300, Ville Syrjälä wrote: > > On Thu, Mar 30, 2017 at 03:27:57PM +0200, Daniel Vetter wrote: > > > On Thu, Mar 30, 2017 at 2:03 PM, Ville Syrjälä > > > wrote: > > > > On Wed, Mar 22, 2017 at 09:56:12PM +0100, Daniel Vetter wrote: > > > >> If we restrict this helper to only kms drivers (which is the case) we > > > >> can look up the correct mode easily ourselves. But it's a bit tricky: > > > >> > > > >> - All legacy drivers look at crtc->hwmode. But that is update already > > > >> at the beginning of the modeset helper, which means when we disable > > > >> a pipe. Hence the final timestamps might be a bit off. But since > > > >> this is an existing bug I'm not going to change it, but just try to > > > >> be bug-for-bug compatible with the current code. This only applies > > > >> to radeon&amdgpu. > > > >> > > > >> - i915 tries to get it perfect by updating crtc->hwmode when the pipe > > > >> is off (i.e. vblank->enabled = false). > > > >> > > > >> - All other atomic drivers look at crtc->state->adjusted_mode. Those > > > >> that look at state->requested_mode simply don't adjust their mode, > > > >> so it's the same. That has two problems: Accessing crtc->state from > > > >> interrupt handling code is unsafe, and it's updated before we shut > > > >> down the pipe. For nonblocking modesets it's even worse. > > > >> > > > >> For atomic drivers try to implement what i915 does. To do that we add > > > >> a new hwmode field to the vblank structure, and update it from > > > >> drm_calc_timestamping_constants(). > > > > > > > > i915 clear crtc->hwmode.crtc_clock when turning the crtc off, which > > > > this does not do for the new vblank->hwmode. I guess no one should > > > > really end up in these codepaths with a disabled crtc, but parts of > > > > drm_irq.c sort of look like they're expecting it to happen. > > > > > > > > So should we have some way to clear out the vblank->hwmode.crtc_clock > > > > for disabled crtcs? And then maybe make some of these crtc_clock checks > > > > WARN and eventually just nuke it all if it looks like nothing is hitting > > > > those? > > > > > > So the trouble is that with a pile of dpms on/off/on/off you could run > > > drm_crtc_vblank_on/off a lot, without ever calling the > > > drm_calc_vbltimestamps helper again to re-upload the mode. So I don't > > > think we can clear vblank->hwmode.crtc_clock unfortunately in > > > drm_crtc_vblank_off. > > > > I was thinking that we'd just try to avoid making pontetially functional > > changes here. Ie. reset where we currently reset, which I think is somewhere > > in the atomic commit for i915, but definitely not in drm_crtc_vblank_off(). > > Hm, sounds like a good idea. I'll try to shuffle things around. Upon > re-reading all that stuff I also noticed that atm i915 only calls the > helper that would update vblank->hwmode when we do a full modeset, but not > when we just fastset. Shouldn't matter, because the hwmode will match, but > better safe than sorry. > > I'll re-read all that stuff again and will try to lock it down fully, like > we have now, and remove the i915 crtc->hwmode access maybe entirely while > at it. Ok, I stitched something together which I think should work well. Done as a follow-up patch though since this refactoring patch here was already pretty big. Resend the entire pile for CI to double-check, review from you on the vblank stuff would be much appreciated. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [Intel-gfx] [PATCH] drm/vblank: drop the mode argument from drm_calc_vbltimestamp_from_scanoutpos
On Thu, Mar 30, 2017 at 04:41:57PM +0300, Ville Syrjälä wrote: > On Thu, Mar 30, 2017 at 03:27:57PM +0200, Daniel Vetter wrote: > > On Thu, Mar 30, 2017 at 2:03 PM, Ville Syrjälä > > wrote: > > > On Wed, Mar 22, 2017 at 09:56:12PM +0100, Daniel Vetter wrote: > > >> If we restrict this helper to only kms drivers (which is the case) we > > >> can look up the correct mode easily ourselves. But it's a bit tricky: > > >> > > >> - All legacy drivers look at crtc->hwmode. But that is update already > > >> at the beginning of the modeset helper, which means when we disable > > >> a pipe. Hence the final timestamps might be a bit off. But since > > >> this is an existing bug I'm not going to change it, but just try to > > >> be bug-for-bug compatible with the current code. This only applies > > >> to radeon&amdgpu. > > >> > > >> - i915 tries to get it perfect by updating crtc->hwmode when the pipe > > >> is off (i.e. vblank->enabled = false). > > >> > > >> - All other atomic drivers look at crtc->state->adjusted_mode. Those > > >> that look at state->requested_mode simply don't adjust their mode, > > >> so it's the same. That has two problems: Accessing crtc->state from > > >> interrupt handling code is unsafe, and it's updated before we shut > > >> down the pipe. For nonblocking modesets it's even worse. > > >> > > >> For atomic drivers try to implement what i915 does. To do that we add > > >> a new hwmode field to the vblank structure, and update it from > > >> drm_calc_timestamping_constants(). > > > > > > i915 clear crtc->hwmode.crtc_clock when turning the crtc off, which > > > this does not do for the new vblank->hwmode. I guess no one should > > > really end up in these codepaths with a disabled crtc, but parts of > > > drm_irq.c sort of look like they're expecting it to happen. > > > > > > So should we have some way to clear out the vblank->hwmode.crtc_clock > > > for disabled crtcs? And then maybe make some of these crtc_clock checks > > > WARN and eventually just nuke it all if it looks like nothing is hitting > > > those? > > > > So the trouble is that with a pile of dpms on/off/on/off you could run > > drm_crtc_vblank_on/off a lot, without ever calling the > > drm_calc_vbltimestamps helper again to re-upload the mode. So I don't > > think we can clear vblank->hwmode.crtc_clock unfortunately in > > drm_crtc_vblank_off. > > I was thinking that we'd just try to avoid making pontetially functional > changes here. Ie. reset where we currently reset, which I think is somewhere > in the atomic commit for i915, but definitely not in drm_crtc_vblank_off(). Hm, sounds like a good idea. I'll try to shuffle things around. Upon re-reading all that stuff I also noticed that atm i915 only calls the helper that would update vblank->hwmode when we do a full modeset, but not when we just fastset. Shouldn't matter, because the hwmode will match, but better safe than sorry. I'll re-read all that stuff again and will try to lock it down fully, like we have now, and remove the i915 crtc->hwmode access maybe entirely while at it. -Daniel > > > > > But what we could do (at least with atomic) is WARN in the vblank > > helper if it's called outside of drm_vblank_on/off ... Not sure how > > useful that is (it won't catch when a driver outright forgets to call > > these) or whether we have enough checks already. Would be a separate > > patch (can do ofc if we agree on what exactly). > > -Daniel > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > > -- > Ville Syrjälä > Intel OTC -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [Intel-gfx] [PATCH] drm/vblank: drop the mode argument from drm_calc_vbltimestamp_from_scanoutpos
On Thu, Mar 30, 2017 at 03:27:57PM +0200, Daniel Vetter wrote: > On Thu, Mar 30, 2017 at 2:03 PM, Ville Syrjälä > wrote: > > On Wed, Mar 22, 2017 at 09:56:12PM +0100, Daniel Vetter wrote: > >> If we restrict this helper to only kms drivers (which is the case) we > >> can look up the correct mode easily ourselves. But it's a bit tricky: > >> > >> - All legacy drivers look at crtc->hwmode. But that is update already > >> at the beginning of the modeset helper, which means when we disable > >> a pipe. Hence the final timestamps might be a bit off. But since > >> this is an existing bug I'm not going to change it, but just try to > >> be bug-for-bug compatible with the current code. This only applies > >> to radeon&amdgpu. > >> > >> - i915 tries to get it perfect by updating crtc->hwmode when the pipe > >> is off (i.e. vblank->enabled = false). > >> > >> - All other atomic drivers look at crtc->state->adjusted_mode. Those > >> that look at state->requested_mode simply don't adjust their mode, > >> so it's the same. That has two problems: Accessing crtc->state from > >> interrupt handling code is unsafe, and it's updated before we shut > >> down the pipe. For nonblocking modesets it's even worse. > >> > >> For atomic drivers try to implement what i915 does. To do that we add > >> a new hwmode field to the vblank structure, and update it from > >> drm_calc_timestamping_constants(). > > > > i915 clear crtc->hwmode.crtc_clock when turning the crtc off, which > > this does not do for the new vblank->hwmode. I guess no one should > > really end up in these codepaths with a disabled crtc, but parts of > > drm_irq.c sort of look like they're expecting it to happen. > > > > So should we have some way to clear out the vblank->hwmode.crtc_clock > > for disabled crtcs? And then maybe make some of these crtc_clock checks > > WARN and eventually just nuke it all if it looks like nothing is hitting > > those? > > So the trouble is that with a pile of dpms on/off/on/off you could run > drm_crtc_vblank_on/off a lot, without ever calling the > drm_calc_vbltimestamps helper again to re-upload the mode. So I don't > think we can clear vblank->hwmode.crtc_clock unfortunately in > drm_crtc_vblank_off. I was thinking that we'd just try to avoid making pontetially functional changes here. Ie. reset where we currently reset, which I think is somewhere in the atomic commit for i915, but definitely not in drm_crtc_vblank_off(). > > But what we could do (at least with atomic) is WARN in the vblank > helper if it's called outside of drm_vblank_on/off ... Not sure how > useful that is (it won't catch when a driver outright forgets to call > these) or whether we have enough checks already. Would be a separate > patch (can do ofc if we agree on what exactly). > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Ville Syrjälä Intel OTC ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [Intel-gfx] [PATCH] drm/vblank: drop the mode argument from drm_calc_vbltimestamp_from_scanoutpos
On Thu, Mar 30, 2017 at 2:03 PM, Ville Syrjälä wrote: > On Wed, Mar 22, 2017 at 09:56:12PM +0100, Daniel Vetter wrote: >> If we restrict this helper to only kms drivers (which is the case) we >> can look up the correct mode easily ourselves. But it's a bit tricky: >> >> - All legacy drivers look at crtc->hwmode. But that is update already >> at the beginning of the modeset helper, which means when we disable >> a pipe. Hence the final timestamps might be a bit off. But since >> this is an existing bug I'm not going to change it, but just try to >> be bug-for-bug compatible with the current code. This only applies >> to radeon&amdgpu. >> >> - i915 tries to get it perfect by updating crtc->hwmode when the pipe >> is off (i.e. vblank->enabled = false). >> >> - All other atomic drivers look at crtc->state->adjusted_mode. Those >> that look at state->requested_mode simply don't adjust their mode, >> so it's the same. That has two problems: Accessing crtc->state from >> interrupt handling code is unsafe, and it's updated before we shut >> down the pipe. For nonblocking modesets it's even worse. >> >> For atomic drivers try to implement what i915 does. To do that we add >> a new hwmode field to the vblank structure, and update it from >> drm_calc_timestamping_constants(). > > i915 clear crtc->hwmode.crtc_clock when turning the crtc off, which > this does not do for the new vblank->hwmode. I guess no one should > really end up in these codepaths with a disabled crtc, but parts of > drm_irq.c sort of look like they're expecting it to happen. > > So should we have some way to clear out the vblank->hwmode.crtc_clock > for disabled crtcs? And then maybe make some of these crtc_clock checks > WARN and eventually just nuke it all if it looks like nothing is hitting > those? So the trouble is that with a pile of dpms on/off/on/off you could run drm_crtc_vblank_on/off a lot, without ever calling the drm_calc_vbltimestamps helper again to re-upload the mode. So I don't think we can clear vblank->hwmode.crtc_clock unfortunately in drm_crtc_vblank_off. But what we could do (at least with atomic) is WARN in the vblank helper if it's called outside of drm_vblank_on/off ... Not sure how useful that is (it won't catch when a driver outright forgets to call these) or whether we have enough checks already. Would be a separate patch (can do ofc if we agree on what exactly). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [Intel-gfx] [PATCH] drm/vblank: drop the mode argument from drm_calc_vbltimestamp_from_scanoutpos
On Wed, Mar 22, 2017 at 09:56:12PM +0100, Daniel Vetter wrote:
> If we restrict this helper to only kms drivers (which is the case) we
> can look up the correct mode easily ourselves. But it's a bit tricky:
>
> - All legacy drivers look at crtc->hwmode. But that is update already
> at the beginning of the modeset helper, which means when we disable
> a pipe. Hence the final timestamps might be a bit off. But since
> this is an existing bug I'm not going to change it, but just try to
> be bug-for-bug compatible with the current code. This only applies
> to radeon&amdgpu.
>
> - i915 tries to get it perfect by updating crtc->hwmode when the pipe
> is off (i.e. vblank->enabled = false).
>
> - All other atomic drivers look at crtc->state->adjusted_mode. Those
> that look at state->requested_mode simply don't adjust their mode,
> so it's the same. That has two problems: Accessing crtc->state from
> interrupt handling code is unsafe, and it's updated before we shut
> down the pipe. For nonblocking modesets it's even worse.
>
> For atomic drivers try to implement what i915 does. To do that we add
> a new hwmode field to the vblank structure, and update it from
> drm_calc_timestamping_constants().
i915 clear crtc->hwmode.crtc_clock when turning the crtc off, which
this does not do for the new vblank->hwmode. I guess no one should
really end up in these codepaths with a disabled crtc, but parts of
drm_irq.c sort of look like they're expecting it to happen.
So should we have some way to clear out the vblank->hwmode.crtc_clock
for disabled crtcs? And then maybe make some of these crtc_clock checks
WARN and eventually just nuke it all if it looks like nothing is hitting
those?
> For atomic drivers that's called
> from the right spot by the helper library already, so all fine. But
> for safety let's enforce that.
>
> For legacy driver this function is only called at the end (oh the
> fun), which is broken, so again let's not bother and just stay
> bug-for-bug compatible.
>
> The benefit is that we can use drm_calc_vbltimestamp_from_scanoutpos
> directly to implement ->get_vblank_timestamp in every driver, deleting
> a lot of code.
>
> v2: Completely new approach, trying to mimick the i915 solution.
>
> v3: Fixup kerneldoc.
>
> v4: Drop the WARN_ON to check that the vblank is off, atomic helpers
> currently unconditionally call this. Recomputing the same stuff should
> be harmless.
>
> Cc: Mario Kleiner
> Cc: Eric Anholt
> Cc: Rob Clark
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
> Cc: Alex Deucher
> Cc: Christian König
> Cc: Ben Skeggs
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 41
> ---
> drivers/gpu/drm/drm_irq.c | 27 +---
> drivers/gpu/drm/i915/i915_irq.c | 33 +
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 26 +---
> drivers/gpu/drm/nouveau/nouveau_display.c | 22 -
> drivers/gpu/drm/nouveau/nouveau_display.h | 2 --
> drivers/gpu/drm/nouveau/nouveau_drm.c | 2 +-
> drivers/gpu/drm/radeon/radeon_drv.c | 6 +
> drivers/gpu/drm/radeon/radeon_kms.c | 37
> drivers/gpu/drm/vc4/vc4_crtc.c| 13 --
> drivers/gpu/drm/vc4/vc4_drv.c | 2 +-
> drivers/gpu/drm/vc4/vc4_drv.h | 3 ---
> include/drm/drm_irq.h | 15 +--
> 15 files changed, 42 insertions(+), 193 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index edb3bb83e1a9..61bef9609b24 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1768,10 +1768,6 @@ int amdgpu_device_resume(struct drm_device *dev, bool
> resume, bool fbcon);
> u32 amdgpu_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe);
> int amdgpu_enable_vblank_kms(struct drm_device *dev, unsigned int pipe);
> void amdgpu_disable_vblank_kms(struct drm_device *dev, unsigned int pipe);
> -bool amdgpu_get_vblank_timestamp_kms(struct drm_device *dev, unsigned int
> pipe,
> - int *max_error,
> - struct timeval *vblank_time,
> - bool in_vblank_irq);
> long amdgpu_kms_compat_ioctl(struct file *filp, unsigned int cmd,
>unsigned long arg);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 8a61296fd4cc..ba169a0699d5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -692,7 +692,7 @@ static struct drm_driver kms_driver = {
> .get_vblank_counter = amdgpu_get_vblank_counter_kms,
> .
