| Message ID | 1432717471-7879-2-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27.05.2015 18:04, Daniel Vetter wrote: > These should be functionally equivalent to the older per/post modeset > functions, except that they block out drm_vblank_get right away. > There's only the clock adjusting code (outside of pageflips) in > readone which uses drm_vblank_get. But that code doesn't synchronize > against concurrent modesets and instead handles any such races by > waiting for the right vblank to arrive with a short timetout. > > The longer-term plan here is to switch all kms drivers to > drm_vblank_on/off so that common code like pending event cleanup can > be done there, while drm_vblank_pre/post_modeset will be purely > drm internal for the old UMS ioctl. > > Note that the kerneldoc for pre/post_modeset is wrong since as Michel > Dänzer correctly pointed out it works if only using pre/post_modeset. > The trouble that lead to this comment is the very old version of > drm_vblank_off to clear out pending events when disabling a pipe, > which did seem to wreak havoc with the trick used by pre/post_modeset. > Michel also expressed dissatisfaction with intel folks pushing new > interfaces with bogus justifications. I still maintain that having a > consistent set of vblank behaviour across kms drivers, separate from > any old UMS functions is a useful goal. > > Cc: Michel Dänzer <michel.daenzer@amd.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Can you describe at least one tangible benefit this change provides for the radeon driver? Because I'm afraid that this might cause subtle breakage, and since we don't have any rigorous tests for this like in intel-gpu-tools (yet?), it might be painful to track it down. So, I'd like to have a good reason for taking the risk. > drivers/gpu/drm/radeon/atombios_crtc.c | 4 ++-- > drivers/gpu/drm/radeon/radeon_legacy_crtc.c | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c > index 42b2ea3fdcf3..77912fd48b69 100644 > --- a/drivers/gpu/drm/radeon/atombios_crtc.c > +++ b/drivers/gpu/drm/radeon/atombios_crtc.c > @@ -274,13 +274,13 @@ void atombios_crtc_dpms(struct drm_crtc *crtc, int mode) > if (ASIC_IS_DCE3(rdev) && !ASIC_IS_DCE6(rdev)) > atombios_enable_crtc_memreq(crtc, ATOM_ENABLE); > atombios_blank_crtc(crtc, ATOM_DISABLE); > - drm_vblank_post_modeset(dev, radeon_crtc->crtc_id); > + drm_vblank_on(dev, radeon_crtc->crtc_id); > radeon_crtc_load_lut(crtc); > break; > case DRM_MODE_DPMS_STANDBY: > case DRM_MODE_DPMS_SUSPEND: > case DRM_MODE_DPMS_OFF: > - drm_vblank_pre_modeset(dev, radeon_crtc->crtc_id); > + drm_vblank_off(dev, radeon_crtc->crtc_id); > if (radeon_crtc->enabled) > atombios_blank_crtc(crtc, ATOM_ENABLE); > if (ASIC_IS_DCE3(rdev) && !ASIC_IS_DCE6(rdev)) > diff --git a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c > index 678b4386540d..4259e27f3983 100644 > --- a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c > +++ b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c > @@ -330,13 +330,13 @@ static void radeon_crtc_dpms(struct drm_crtc *crtc, int mode) > RADEON_CRTC_DISP_REQ_EN_B)); > WREG32_P(RADEON_CRTC_EXT_CNTL, crtc_ext_cntl, ~(mask | crtc_ext_cntl)); > } > - drm_vblank_post_modeset(dev, radeon_crtc->crtc_id); > + drm_vblank_on(dev, radeon_crtc->crtc_id); > radeon_crtc_load_lut(crtc); > break; > case DRM_MODE_DPMS_STANDBY: > case DRM_MODE_DPMS_SUSPEND: > case DRM_MODE_DPMS_OFF: > - drm_vblank_pre_modeset(dev, radeon_crtc->crtc_id); > + drm_vblank_off(dev, radeon_crtc->crtc_id); > if (radeon_crtc->crtc_id) > WREG32_P(RADEON_CRTC2_GEN_CNTL, mask, ~(RADEON_CRTC2_EN | mask)); > else { >
On Wed, May 27, 2015 at 06:21:24PM +0900, Michel Dänzer wrote: > On 27.05.2015 18:04, Daniel Vetter wrote: > > These should be functionally equivalent to the older per/post modeset > > functions, except that they block out drm_vblank_get right away. > > There's only the clock adjusting code (outside of pageflips) in > > readone which uses drm_vblank_get. But that code doesn't synchronize > > against concurrent modesets and instead handles any such races by > > waiting for the right vblank to arrive with a short timetout. > > > > The longer-term plan here is to switch all kms drivers to > > drm_vblank_on/off so that common code like pending event cleanup can > > be done there, while drm_vblank_pre/post_modeset will be purely > > drm internal for the old UMS ioctl. > > > > Note that the kerneldoc for pre/post_modeset is wrong since as Michel > > Dänzer correctly pointed out it works if only using pre/post_modeset. > > The trouble that lead to this comment is the very old version of > > drm_vblank_off to clear out pending events when disabling a pipe, > > which did seem to wreak havoc with the trick used by pre/post_modeset. > > Michel also expressed dissatisfaction with intel folks pushing new > > interfaces with bogus justifications. I still maintain that having a > > consistent set of vblank behaviour across kms drivers, separate from > > any old UMS functions is a useful goal. > > > > Cc: Michel Dänzer <michel.daenzer@amd.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Can you describe at least one tangible benefit this change provides for > the radeon driver? > > Because I'm afraid that this might cause subtle breakage, and since we > don't have any rigorous tests for this like in intel-gpu-tools (yet?), > it might be painful to track it down. > > So, I'd like to have a good reason for taking the risk. right now at most a bit of code to clean out pending events on modeset disable, for somewhat consistent behaviour with other drivers. But in general it's fairly ill-defined what happens with vblank events. Longer-term Rob&I have a pile of patches and refactorings for drm_irq.c planned so that drivers don't need to open-code all the event handling and cleanup code (for both vblank and pageflips) and maybe also some infrastructure for vblank callbacks (which also a lot of drivers hand-roll). But drm_irq.c has the trouble of being used by both kms and ums drivers, and I don't want to change any of the ums code too much. Hence first step (which has been ongoing for a while now) is trying to create a clean split between functions/features used by kms drivers and those used by the ums ioctls. Anyway that's just my overall plan, there's not going to be a problem if you decide not to apply these patches. I just want to make sure that any such drm core refactoring projects aren't seen as intel-only, hence I send around these patches. And ofc I'd be nice to know if this does unintentionally break something, to make sure the new stuff is as widely suitable as possible. But in the end it's your call. -Daniel > > drivers/gpu/drm/radeon/atombios_crtc.c | 4 ++-- > > drivers/gpu/drm/radeon/radeon_legacy_crtc.c | 4 ++-- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c > > index 42b2ea3fdcf3..77912fd48b69 100644 > > --- a/drivers/gpu/drm/radeon/atombios_crtc.c > > +++ b/drivers/gpu/drm/radeon/atombios_crtc.c > > @@ -274,13 +274,13 @@ void atombios_crtc_dpms(struct drm_crtc *crtc, int mode) > > if (ASIC_IS_DCE3(rdev) && !ASIC_IS_DCE6(rdev)) > > atombios_enable_crtc_memreq(crtc, ATOM_ENABLE); > > atombios_blank_crtc(crtc, ATOM_DISABLE); > > - drm_vblank_post_modeset(dev, radeon_crtc->crtc_id); > > + drm_vblank_on(dev, radeon_crtc->crtc_id); > > radeon_crtc_load_lut(crtc); > > break; > > case DRM_MODE_DPMS_STANDBY: > > case DRM_MODE_DPMS_SUSPEND: > > case DRM_MODE_DPMS_OFF: > > - drm_vblank_pre_modeset(dev, radeon_crtc->crtc_id); > > + drm_vblank_off(dev, radeon_crtc->crtc_id); > > if (radeon_crtc->enabled) > > atombios_blank_crtc(crtc, ATOM_ENABLE); > > if (ASIC_IS_DCE3(rdev) && !ASIC_IS_DCE6(rdev)) > > diff --git a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c > > index 678b4386540d..4259e27f3983 100644 > > --- a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c > > +++ b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c > > @@ -330,13 +330,13 @@ static void radeon_crtc_dpms(struct drm_crtc *crtc, int mode) > > RADEON_CRTC_DISP_REQ_EN_B)); > > WREG32_P(RADEON_CRTC_EXT_CNTL, crtc_ext_cntl, ~(mask | crtc_ext_cntl)); > > } > > - drm_vblank_post_modeset(dev, radeon_crtc->crtc_id); > > + drm_vblank_on(dev, radeon_crtc->crtc_id); > > radeon_crtc_load_lut(crtc); > > break; > > case DRM_MODE_DPMS_STANDBY: > > case DRM_MODE_DPMS_SUSPEND: > > case DRM_MODE_DPMS_OFF: > > - drm_vblank_pre_modeset(dev, radeon_crtc->crtc_id); > > + drm_vblank_off(dev, radeon_crtc->crtc_id); > > if (radeon_crtc->crtc_id) > > WREG32_P(RADEON_CRTC2_GEN_CNTL, mask, ~(RADEON_CRTC2_EN | mask)); > > else { > > > > > -- > Earthling Michel Dänzer | http://www.amd.com > Libre software enthusiast | Mesa and X developer
On 27.05.2015 18:41, Daniel Vetter wrote: > On Wed, May 27, 2015 at 06:21:24PM +0900, Michel Dänzer wrote: >> On 27.05.2015 18:04, Daniel Vetter wrote: >>> These should be functionally equivalent to the older per/post modeset >>> functions, except that they block out drm_vblank_get right away. >>> There's only the clock adjusting code (outside of pageflips) in >>> readone which uses drm_vblank_get. But that code doesn't synchronize >>> against concurrent modesets and instead handles any such races by >>> waiting for the right vblank to arrive with a short timetout. >>> >>> The longer-term plan here is to switch all kms drivers to >>> drm_vblank_on/off so that common code like pending event cleanup can >>> be done there, while drm_vblank_pre/post_modeset will be purely >>> drm internal for the old UMS ioctl. >>> >>> Note that the kerneldoc for pre/post_modeset is wrong since as Michel >>> Dänzer correctly pointed out it works if only using pre/post_modeset. >>> The trouble that lead to this comment is the very old version of >>> drm_vblank_off to clear out pending events when disabling a pipe, >>> which did seem to wreak havoc with the trick used by pre/post_modeset. >>> Michel also expressed dissatisfaction with intel folks pushing new >>> interfaces with bogus justifications. I still maintain that having a >>> consistent set of vblank behaviour across kms drivers, separate from >>> any old UMS functions is a useful goal. >>> >>> Cc: Michel Dänzer <michel.daenzer@amd.com> >>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >> >> Can you describe at least one tangible benefit this change provides for >> the radeon driver? >> >> Because I'm afraid that this might cause subtle breakage, and since we >> don't have any rigorous tests for this like in intel-gpu-tools (yet?), >> it might be painful to track it down. >> >> So, I'd like to have a good reason for taking the risk. > > right now at most a bit of code to clean out pending events on modeset > disable, for somewhat consistent behaviour with other drivers. But in > general it's fairly ill-defined what happens with vblank events. Yeah, while that's nice to have, I don't think it makes too much difference in practice. Anyway, I'm giving this patch a spin, and it does indeed cause userspace fallout, at least with DRI3/Present enabled, because the vblank and pageflip ioctls now return -EINVAL while the CRTC is off. However, it looks like fixing that up might not be too bad, so I'm cautiously optimistic for this change. But I'd like some more time for testing and fixing userspace.
On Thu, May 28, 2015 at 04:11:53PM +0900, Michel Dänzer wrote: > On 27.05.2015 18:41, Daniel Vetter wrote: > > On Wed, May 27, 2015 at 06:21:24PM +0900, Michel Dänzer wrote: > >> On 27.05.2015 18:04, Daniel Vetter wrote: > >>> These should be functionally equivalent to the older per/post modeset > >>> functions, except that they block out drm_vblank_get right away. > >>> There's only the clock adjusting code (outside of pageflips) in > >>> readone which uses drm_vblank_get. But that code doesn't synchronize > >>> against concurrent modesets and instead handles any such races by > >>> waiting for the right vblank to arrive with a short timetout. > >>> > >>> The longer-term plan here is to switch all kms drivers to > >>> drm_vblank_on/off so that common code like pending event cleanup can > >>> be done there, while drm_vblank_pre/post_modeset will be purely > >>> drm internal for the old UMS ioctl. > >>> > >>> Note that the kerneldoc for pre/post_modeset is wrong since as Michel > >>> Dänzer correctly pointed out it works if only using pre/post_modeset. > >>> The trouble that lead to this comment is the very old version of > >>> drm_vblank_off to clear out pending events when disabling a pipe, > >>> which did seem to wreak havoc with the trick used by pre/post_modeset. > >>> Michel also expressed dissatisfaction with intel folks pushing new > >>> interfaces with bogus justifications. I still maintain that having a > >>> consistent set of vblank behaviour across kms drivers, separate from > >>> any old UMS functions is a useful goal. > >>> > >>> Cc: Michel Dänzer <michel.daenzer@amd.com> > >>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > >> > >> Can you describe at least one tangible benefit this change provides for > >> the radeon driver? > >> > >> Because I'm afraid that this might cause subtle breakage, and since we > >> don't have any rigorous tests for this like in intel-gpu-tools (yet?), > >> it might be painful to track it down. > >> > >> So, I'd like to have a good reason for taking the risk. > > > > right now at most a bit of code to clean out pending events on modeset > > disable, for somewhat consistent behaviour with other drivers. But in > > general it's fairly ill-defined what happens with vblank events. > > Yeah, while that's nice to have, I don't think it makes too much > difference in practice. > > Anyway, I'm giving this patch a spin, and it does indeed cause userspace > fallout, at least with DRI3/Present enabled, because the vblank and > pageflip ioctls now return -EINVAL while the CRTC is off. However, it > looks like fixing that up might not be too bad, so I'm cautiously > optimistic for this change. But I'd like some more time for testing and > fixing userspace. Hm that's a bummer, since it means the abi userspace sees has already diverged between drivers :( And because drm_irq.c is still a giant mess with no clear drm core -> driver interface there's also no way for drivers to overwrite the normal behaviour to keep old, driver-specific userspace happy. Adding such a hook for kms drivers is definitely something I want to do. With all the modeset entry points and atomic helpers you can do that and so keep old userspace working even if the more strict semantics of atomic would break it (which was an important goal with all the super-flexible helper library). Otoh asking for a vblank event on a dead pipe smells like a userspace bug and could result in stuck compositors. Not sure what's best here really. -Daniel
On 28.05.2015 17:38, Daniel Vetter wrote: > On Thu, May 28, 2015 at 04:11:53PM +0900, Michel Dänzer wrote: >> On 27.05.2015 18:41, Daniel Vetter wrote: >>> On Wed, May 27, 2015 at 06:21:24PM +0900, Michel Dänzer wrote: >>>> On 27.05.2015 18:04, Daniel Vetter wrote: >>>>> These should be functionally equivalent to the older per/post modeset >>>>> functions, except that they block out drm_vblank_get right away. >>>>> There's only the clock adjusting code (outside of pageflips) in >>>>> readone which uses drm_vblank_get. But that code doesn't synchronize >>>>> against concurrent modesets and instead handles any such races by >>>>> waiting for the right vblank to arrive with a short timetout. >>>>> >>>>> The longer-term plan here is to switch all kms drivers to >>>>> drm_vblank_on/off so that common code like pending event cleanup can >>>>> be done there, while drm_vblank_pre/post_modeset will be purely >>>>> drm internal for the old UMS ioctl. >>>>> >>>>> Note that the kerneldoc for pre/post_modeset is wrong since as Michel >>>>> Dänzer correctly pointed out it works if only using pre/post_modeset. >>>>> The trouble that lead to this comment is the very old version of >>>>> drm_vblank_off to clear out pending events when disabling a pipe, >>>>> which did seem to wreak havoc with the trick used by pre/post_modeset. >>>>> Michel also expressed dissatisfaction with intel folks pushing new >>>>> interfaces with bogus justifications. I still maintain that having a >>>>> consistent set of vblank behaviour across kms drivers, separate from >>>>> any old UMS functions is a useful goal. >>>>> >>>>> Cc: Michel Dänzer <michel.daenzer@amd.com> >>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >>>> >>>> Can you describe at least one tangible benefit this change provides for >>>> the radeon driver? >>>> >>>> Because I'm afraid that this might cause subtle breakage, and since we >>>> don't have any rigorous tests for this like in intel-gpu-tools (yet?), >>>> it might be painful to track it down. >>>> >>>> So, I'd like to have a good reason for taking the risk. >>> >>> right now at most a bit of code to clean out pending events on modeset >>> disable, for somewhat consistent behaviour with other drivers. But in >>> general it's fairly ill-defined what happens with vblank events. >> >> Yeah, while that's nice to have, I don't think it makes too much >> difference in practice. >> >> Anyway, I'm giving this patch a spin, and it does indeed cause userspace >> fallout, at least with DRI3/Present enabled, because the vblank and >> pageflip ioctls now return -EINVAL while the CRTC is off. However, it >> looks like fixing that up might not be too bad, so I'm cautiously >> optimistic for this change. But I'd like some more time for testing and >> fixing userspace. [...] > Otoh asking for a vblank event on a dead pipe smells like a userspace bug > and could result in stuck compositors. Not sure what's best here really. Agreed, and we're already careful not to do that with DRI2, just not yet with DRI3/Present (which isn't in any xf86-video-ati release yet).
On 28.05.2015 18:03, Michel Dänzer wrote: > On 28.05.2015 17:38, Daniel Vetter wrote: >> On Thu, May 28, 2015 at 04:11:53PM +0900, Michel Dänzer wrote: >>> On 27.05.2015 18:41, Daniel Vetter wrote: >>>> On Wed, May 27, 2015 at 06:21:24PM +0900, Michel Dänzer wrote: >>>>> On 27.05.2015 18:04, Daniel Vetter wrote: >>>>>> These should be functionally equivalent to the older per/post modeset >>>>>> functions, except that they block out drm_vblank_get right away. >>>>>> There's only the clock adjusting code (outside of pageflips) in >>>>>> readone which uses drm_vblank_get. But that code doesn't synchronize >>>>>> against concurrent modesets and instead handles any such races by >>>>>> waiting for the right vblank to arrive with a short timetout. >>>>>> >>>>>> The longer-term plan here is to switch all kms drivers to >>>>>> drm_vblank_on/off so that common code like pending event cleanup can >>>>>> be done there, while drm_vblank_pre/post_modeset will be purely >>>>>> drm internal for the old UMS ioctl. >>>>>> >>>>>> Note that the kerneldoc for pre/post_modeset is wrong since as Michel >>>>>> Dänzer correctly pointed out it works if only using pre/post_modeset. >>>>>> The trouble that lead to this comment is the very old version of >>>>>> drm_vblank_off to clear out pending events when disabling a pipe, >>>>>> which did seem to wreak havoc with the trick used by pre/post_modeset. >>>>>> Michel also expressed dissatisfaction with intel folks pushing new >>>>>> interfaces with bogus justifications. I still maintain that having a >>>>>> consistent set of vblank behaviour across kms drivers, separate from >>>>>> any old UMS functions is a useful goal. >>>>>> >>>>>> Cc: Michel Dänzer <michel.daenzer@amd.com> >>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >>>>> >>>>> Can you describe at least one tangible benefit this change provides for >>>>> the radeon driver? >>>>> >>>>> Because I'm afraid that this might cause subtle breakage, and since we >>>>> don't have any rigorous tests for this like in intel-gpu-tools (yet?), >>>>> it might be painful to track it down. >>>>> >>>>> So, I'd like to have a good reason for taking the risk. >>>> >>>> right now at most a bit of code to clean out pending events on modeset >>>> disable, for somewhat consistent behaviour with other drivers. But in >>>> general it's fairly ill-defined what happens with vblank events. >>> >>> Yeah, while that's nice to have, I don't think it makes too much >>> difference in practice. >>> >>> Anyway, I'm giving this patch a spin, and it does indeed cause userspace >>> fallout, at least with DRI3/Present enabled, because the vblank and >>> pageflip ioctls now return -EINVAL while the CRTC is off. However, it >>> looks like fixing that up might not be too bad, so I'm cautiously >>> optimistic for this change. But I'd like some more time for testing and >>> fixing userspace. > > [...] > >> Otoh asking for a vblank event on a dead pipe smells like a userspace bug >> and could result in stuck compositors. Not sure what's best here really. > > Agreed, and we're already careful not to do that with DRI2, just not yet > with DRI3/Present (which isn't in any xf86-video-ati release yet). I've fixed up the DRI3/Present code as well. This patch (with maybe some cleanups to the commit log, in particular I'm not sure the third paragraph should be there) is Acked-and-Tested-by: Michel Dänzer <michel.daenzer@amd.com>
diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c index 42b2ea3fdcf3..77912fd48b69 100644 --- a/drivers/gpu/drm/radeon/atombios_crtc.c +++ b/drivers/gpu/drm/radeon/atombios_crtc.c @@ -274,13 +274,13 @@ void atombios_crtc_dpms(struct drm_crtc *crtc, int mode) if (ASIC_IS_DCE3(rdev) && !ASIC_IS_DCE6(rdev)) atombios_enable_crtc_memreq(crtc, ATOM_ENABLE); atombios_blank_crtc(crtc, ATOM_DISABLE); - drm_vblank_post_modeset(dev, radeon_crtc->crtc_id); + drm_vblank_on(dev, radeon_crtc->crtc_id); radeon_crtc_load_lut(crtc); break; case DRM_MODE_DPMS_STANDBY: case DRM_MODE_DPMS_SUSPEND: case DRM_MODE_DPMS_OFF: - drm_vblank_pre_modeset(dev, radeon_crtc->crtc_id); + drm_vblank_off(dev, radeon_crtc->crtc_id); if (radeon_crtc->enabled) atombios_blank_crtc(crtc, ATOM_ENABLE); if (ASIC_IS_DCE3(rdev) && !ASIC_IS_DCE6(rdev)) diff --git a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c index 678b4386540d..4259e27f3983 100644 --- a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c +++ b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c @@ -330,13 +330,13 @@ static void radeon_crtc_dpms(struct drm_crtc *crtc, int mode) RADEON_CRTC_DISP_REQ_EN_B)); WREG32_P(RADEON_CRTC_EXT_CNTL, crtc_ext_cntl, ~(mask | crtc_ext_cntl)); } - drm_vblank_post_modeset(dev, radeon_crtc->crtc_id); + drm_vblank_on(dev, radeon_crtc->crtc_id); radeon_crtc_load_lut(crtc); break; case DRM_MODE_DPMS_STANDBY: case DRM_MODE_DPMS_SUSPEND: case DRM_MODE_DPMS_OFF: - drm_vblank_pre_modeset(dev, radeon_crtc->crtc_id); + drm_vblank_off(dev, radeon_crtc->crtc_id); if (radeon_crtc->crtc_id) WREG32_P(RADEON_CRTC2_GEN_CNTL, mask, ~(RADEON_CRTC2_EN | mask)); else {
These should be functionally equivalent to the older per/post modeset functions, except that they block out drm_vblank_get right away. There's only the clock adjusting code (outside of pageflips) in readone which uses drm_vblank_get. But that code doesn't synchronize against concurrent modesets and instead handles any such races by waiting for the right vblank to arrive with a short timetout. The longer-term plan here is to switch all kms drivers to drm_vblank_on/off so that common code like pending event cleanup can be done there, while drm_vblank_pre/post_modeset will be purely drm internal for the old UMS ioctl. Note that the kerneldoc for pre/post_modeset is wrong since as Michel Dänzer correctly pointed out it works if only using pre/post_modeset. The trouble that lead to this comment is the very old version of drm_vblank_off to clear out pending events when disabling a pipe, which did seem to wreak havoc with the trick used by pre/post_modeset. Michel also expressed dissatisfaction with intel folks pushing new interfaces with bogus justifications. I still maintain that having a consistent set of vblank behaviour across kms drivers, separate from any old UMS functions is a useful goal. Cc: Michel Dänzer <michel.daenzer@amd.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/radeon/atombios_crtc.c | 4 ++-- drivers/gpu/drm/radeon/radeon_legacy_crtc.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)