| Message ID | 1445514996-18733-4-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 22, 2015 at 01:56:28PM +0200, Maarten Lankhorst wrote: > By handling this after the atomic helper waits for vblanks there will > be one less wait for vblank in the atomic path. > > Also get rid of the double wait_for_vblank on broadwell, looks like > it's a bug doing the vblank wait twice. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Problem is that the next plane update should be possible right after this one happens, which is the potentially racing with the post_plane_update code. What's your plan to prevent this? -Daniel > --- > drivers/gpu/drm/i915/intel_display.c | 28 +++------------------------- > drivers/gpu/drm/i915/intel_drv.h | 1 - > 2 files changed, 3 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 051a1e2b1c55..b8e1a5471bed 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4672,14 +4672,6 @@ intel_post_enable_primary(struct drm_crtc *crtc) > int pipe = intel_crtc->pipe; > > /* > - * BDW signals flip done immediately if the plane > - * is disabled, even if the plane enable is already > - * armed to occur at the next vblank :( > - */ > - if (IS_BROADWELL(dev)) > - intel_wait_for_vblank(dev, pipe); > - > - /* > * FIXME IPS should be fine as long as one plane is > * enabled, but in practice it seems to have problems > * when going from primary only to sprite only and vice > @@ -4760,9 +4752,6 @@ static void intel_post_plane_update(struct intel_crtc *crtc) > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_plane *plane; > > - if (atomic->wait_vblank) > - intel_wait_for_vblank(dev, crtc->pipe); > - > intel_frontbuffer_flip(dev, atomic->fb_bits); > > if (atomic->disable_cxsr) > @@ -11661,15 +11650,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, > if (plane->type != DRM_PLANE_TYPE_CURSOR) { > intel_crtc->atomic.disable_cxsr = true; > /* to potentially re-enable cxsr */ > - intel_crtc->atomic.wait_vblank = true; > intel_crtc->atomic.update_wm_post = true; > } > } else if (turn_off) { > intel_crtc->atomic.update_wm_post = true; > /* must disable cxsr around plane enable/disable */ > if (plane->type != DRM_PLANE_TYPE_CURSOR) { > - if (is_crtc_enabled) > - intel_crtc->atomic.wait_vblank = true; > intel_crtc->atomic.disable_cxsr = true; > } > } else if (intel_wm_need_update(plane, plane_state)) { > @@ -11716,21 +11702,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, > plane_state->rotation != BIT(DRM_ROTATE_0)) > intel_crtc->atomic.disable_fbc = true; > > - /* > - * BDW signals flip done immediately if the plane > - * is disabled, even if the plane enable is already > - * armed to occur at the next vblank :( > - */ > - if (turn_on && IS_BROADWELL(dev)) > - intel_crtc->atomic.wait_vblank = true; > - > intel_crtc->atomic.update_fbc |= visible || mode_changed; > break; > case DRM_PLANE_TYPE_CURSOR: > break; > case DRM_PLANE_TYPE_OVERLAY: > if (turn_off && !mode_changed) { > - intel_crtc->atomic.wait_vblank = true; > intel_crtc->atomic.update_sprite_watermarks |= > 1 << i; > } > @@ -13271,14 +13248,15 @@ static int intel_atomic_commit(struct drm_device *dev, > > if (put_domains) > modeset_put_power_domains(dev_priv, put_domains); > - > - intel_post_plane_update(intel_crtc); > } > > /* FIXME: add subpixel order */ > > drm_atomic_helper_wait_for_vblanks(dev, state); > > + for_each_crtc_in_state(state, crtc, crtc_state, i) > + intel_post_plane_update(to_intel_crtc(crtc)); > + > mutex_lock(&dev->struct_mutex); > drm_atomic_helper_cleanup_planes(dev, state); > mutex_unlock(&dev->struct_mutex); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 54a2c0da2ece..4b6bb1adfddd 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -522,7 +522,6 @@ struct intel_crtc_atomic_commit { > > /* Sleepable operations to perform after commit */ > unsigned fb_bits; > - bool wait_vblank; > bool update_fbc; > bool post_enable_primary; > unsigned update_sprite_watermarks; > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Oct 22, 2015 at 01:56:28PM +0200, Maarten Lankhorst wrote: > By handling this after the atomic helper waits for vblanks there will > be one less wait for vblank in the atomic path. > > Also get rid of the double wait_for_vblank on broadwell, looks like > it's a bug doing the vblank wait twice. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 28 +++------------------------- > drivers/gpu/drm/i915/intel_drv.h | 1 - > 2 files changed, 3 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 051a1e2b1c55..b8e1a5471bed 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4672,14 +4672,6 @@ intel_post_enable_primary(struct drm_crtc *crtc) > int pipe = intel_crtc->pipe; > > /* > - * BDW signals flip done immediately if the plane > - * is disabled, even if the plane enable is already > - * armed to occur at the next vblank :( > - */ > - if (IS_BROADWELL(dev)) > - intel_wait_for_vblank(dev, pipe); The right fix for this crap is to not use the flip done interrupt. But apparently no one wants to fix that. So now I'm worried that we now depend on the atomic helper doing synchornous vblank waits here. Are we expecting those vblank waits to remain there? I would have expected to get rid of all synchronois vblank waits in plane codepaths (apart from ones for hw workarounds). Also does the helper actually wait for vblanks when the plane gets enabled w/o the framebuffer actually changing? > - > - /* > * FIXME IPS should be fine as long as one plane is > * enabled, but in practice it seems to have problems > * when going from primary only to sprite only and vice > @@ -4760,9 +4752,6 @@ static void intel_post_plane_update(struct intel_crtc *crtc) > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_plane *plane; > > - if (atomic->wait_vblank) > - intel_wait_for_vblank(dev, crtc->pipe); > - > intel_frontbuffer_flip(dev, atomic->fb_bits); > > if (atomic->disable_cxsr) > @@ -11661,15 +11650,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, > if (plane->type != DRM_PLANE_TYPE_CURSOR) { > intel_crtc->atomic.disable_cxsr = true; > /* to potentially re-enable cxsr */ > - intel_crtc->atomic.wait_vblank = true; What's there to make sure cxsr doesn't get enabled/disabled at the wrong time now? I guess this is the same question as for the BDW w/a, ie. does the plane getting enabled/disabled (or rather becoming visible/invisible) always imply a vblank wait? > intel_crtc->atomic.update_wm_post = true; > } > } else if (turn_off) { > intel_crtc->atomic.update_wm_post = true; > /* must disable cxsr around plane enable/disable */ > if (plane->type != DRM_PLANE_TYPE_CURSOR) { > - if (is_crtc_enabled) > - intel_crtc->atomic.wait_vblank = true; > intel_crtc->atomic.disable_cxsr = true; > } > } else if (intel_wm_need_update(plane, plane_state)) { > @@ -11716,21 +11702,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, > plane_state->rotation != BIT(DRM_ROTATE_0)) > intel_crtc->atomic.disable_fbc = true; > > - /* > - * BDW signals flip done immediately if the plane > - * is disabled, even if the plane enable is already > - * armed to occur at the next vblank :( > - */ > - if (turn_on && IS_BROADWELL(dev)) > - intel_crtc->atomic.wait_vblank = true; > - > intel_crtc->atomic.update_fbc |= visible || mode_changed; > break; > case DRM_PLANE_TYPE_CURSOR: > break; > case DRM_PLANE_TYPE_OVERLAY: > if (turn_off && !mode_changed) { > - intel_crtc->atomic.wait_vblank = true; > intel_crtc->atomic.update_sprite_watermarks |= > 1 << i; > } > @@ -13271,14 +13248,15 @@ static int intel_atomic_commit(struct drm_device *dev, > > if (put_domains) > modeset_put_power_domains(dev_priv, put_domains); > - > - intel_post_plane_update(intel_crtc); > } > > /* FIXME: add subpixel order */ > > drm_atomic_helper_wait_for_vblanks(dev, state); > > + for_each_crtc_in_state(state, crtc, crtc_state, i) > + intel_post_plane_update(to_intel_crtc(crtc)); > + > mutex_lock(&dev->struct_mutex); > drm_atomic_helper_cleanup_planes(dev, state); > mutex_unlock(&dev->struct_mutex); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 54a2c0da2ece..4b6bb1adfddd 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -522,7 +522,6 @@ struct intel_crtc_atomic_commit { > > /* Sleepable operations to perform after commit */ > unsigned fb_bits; > - bool wait_vblank; > bool update_fbc; > bool post_enable_primary; > unsigned update_sprite_watermarks; > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 22-10-15 om 15:30 schreef Ville Syrjälä: > On Thu, Oct 22, 2015 at 01:56:28PM +0200, Maarten Lankhorst wrote: >> By handling this after the atomic helper waits for vblanks there will >> be one less wait for vblank in the atomic path. >> >> Also get rid of the double wait_for_vblank on broadwell, looks like >> it's a bug doing the vblank wait twice. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 28 +++------------------------- >> drivers/gpu/drm/i915/intel_drv.h | 1 - >> 2 files changed, 3 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 051a1e2b1c55..b8e1a5471bed 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -4672,14 +4672,6 @@ intel_post_enable_primary(struct drm_crtc *crtc) >> int pipe = intel_crtc->pipe; >> >> /* >> - * BDW signals flip done immediately if the plane >> - * is disabled, even if the plane enable is already >> - * armed to occur at the next vblank :( >> - */ >> - if (IS_BROADWELL(dev)) >> - intel_wait_for_vblank(dev, pipe); > The right fix for this crap is to not use the flip done interrupt. But > apparently no one wants to fix that. This is fixed later on when converting page_flip to atomic. I've modified page_flip_finished to ignore flips when visible_changed happens on broadwell. > So now I'm worried that we now depend on the atomic helper doing > synchornous vblank waits here. Are we expecting those vblank waits to > remain there? I would have expected to get rid of all synchronois vblank > waits in plane codepaths (apart from ones for hw workarounds). In my unify page flip and atomic series I first add all post_plane_update and unpinning to unpin_work_fn. After that I have a patch to convert the last part of atomic after wait_for_vblanks to schedule unpin_work_fn, next I'll get more bold and zap the wait_for_vblanks and run it async. It depends on work->can_async_unpin, if it's false it has to wait for everything to finish before queueing the next flip, so it will finish before next one starts. > Also does the helper actually wait for vblanks when the plane gets > enabled w/o the framebuffer actually changing? afaict it does, as long as the crtc is enabled. I'm guessing that drm_atomic_helper_wait_for_vblanks should be fixed to look at crtc->state->active instead of crtc->state->enable.. >> - >> - /* >> * FIXME IPS should be fine as long as one plane is >> * enabled, but in practice it seems to have problems >> * when going from primary only to sprite only and vice >> @@ -4760,9 +4752,6 @@ static void intel_post_plane_update(struct intel_crtc *crtc) >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct drm_plane *plane; >> >> - if (atomic->wait_vblank) >> - intel_wait_for_vblank(dev, crtc->pipe); >> - >> intel_frontbuffer_flip(dev, atomic->fb_bits); >> >> if (atomic->disable_cxsr) >> @@ -11661,15 +11650,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, >> if (plane->type != DRM_PLANE_TYPE_CURSOR) { >> intel_crtc->atomic.disable_cxsr = true; >> /* to potentially re-enable cxsr */ >> - intel_crtc->atomic.wait_vblank = true; > What's there to make sure cxsr doesn't get enabled/disabled at the wrong > time now? I guess this is the same question as for the BDW w/a, ie. does > the plane getting enabled/disabled (or rather becoming > visible/invisible) always imply a vblank wait? crtc mutex lock and in the future atomic wm's.. >> intel_crtc->atomic.update_wm_post = true; >> } >> } else if (turn_off) { >> intel_crtc->atomic.update_wm_post = true; >> /* must disable cxsr around plane enable/disable */ >> if (plane->type != DRM_PLANE_TYPE_CURSOR) { >> - if (is_crtc_enabled) >> - intel_crtc->atomic.wait_vblank = true; >> intel_crtc->atomic.disable_cxsr = true; >> } >> } else if (intel_wm_need_update(plane, plane_state)) { >> @@ -11716,21 +11702,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, >> plane_state->rotation != BIT(DRM_ROTATE_0)) >> intel_crtc->atomic.disable_fbc = true; >> >> - /* >> - * BDW signals flip done immediately if the plane >> - * is disabled, even if the plane enable is already >> - * armed to occur at the next vblank :( >> - */ >> - if (turn_on && IS_BROADWELL(dev)) >> - intel_crtc->atomic.wait_vblank = true; >> - >> intel_crtc->atomic.update_fbc |= visible || mode_changed; >> break; >> case DRM_PLANE_TYPE_CURSOR: >> break; >> case DRM_PLANE_TYPE_OVERLAY: >> if (turn_off && !mode_changed) { >> - intel_crtc->atomic.wait_vblank = true; >> intel_crtc->atomic.update_sprite_watermarks |= >> 1 << i; >> } >> @@ -13271,14 +13248,15 @@ static int intel_atomic_commit(struct drm_device *dev, >> >> if (put_domains) >> modeset_put_power_domains(dev_priv, put_domains); >> - >> - intel_post_plane_update(intel_crtc); >> } >> >> /* FIXME: add subpixel order */ >> >> drm_atomic_helper_wait_for_vblanks(dev, state); >> >> + for_each_crtc_in_state(state, crtc, crtc_state, i) >> + intel_post_plane_update(to_intel_crtc(crtc)); >> + >> mutex_lock(&dev->struct_mutex); >> drm_atomic_helper_cleanup_planes(dev, state); >> mutex_unlock(&dev->struct_mutex); >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 54a2c0da2ece..4b6bb1adfddd 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -522,7 +522,6 @@ struct intel_crtc_atomic_commit { >> >> /* Sleepable operations to perform after commit */ >> unsigned fb_bits; >> - bool wait_vblank; >> bool update_fbc; >> bool post_enable_primary; >> unsigned update_sprite_watermarks; >> -- >> 2.1.0 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Oct 22, 2015 at 04:50:05PM +0200, Maarten Lankhorst wrote: > Op 22-10-15 om 15:30 schreef Ville Syrjälä: > > On Thu, Oct 22, 2015 at 01:56:28PM +0200, Maarten Lankhorst wrote: > >> By handling this after the atomic helper waits for vblanks there will > >> be one less wait for vblank in the atomic path. > >> > >> Also get rid of the double wait_for_vblank on broadwell, looks like > >> it's a bug doing the vblank wait twice. > >> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_display.c | 28 +++------------------------- > >> drivers/gpu/drm/i915/intel_drv.h | 1 - > >> 2 files changed, 3 insertions(+), 26 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> index 051a1e2b1c55..b8e1a5471bed 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -4672,14 +4672,6 @@ intel_post_enable_primary(struct drm_crtc *crtc) > >> int pipe = intel_crtc->pipe; > >> > >> /* > >> - * BDW signals flip done immediately if the plane > >> - * is disabled, even if the plane enable is already > >> - * armed to occur at the next vblank :( > >> - */ > >> - if (IS_BROADWELL(dev)) > >> - intel_wait_for_vblank(dev, pipe); > > The right fix for this crap is to not use the flip done interrupt. But > > apparently no one wants to fix that. > This is fixed later on when converting page_flip to atomic. I've modified > page_flip_finished to ignore flips when visible_changed happens on broadwell. Sounds like another hack. > > So now I'm worried that we now depend on the atomic helper doing > > synchornous vblank waits here. Are we expecting those vblank waits to > > remain there? I would have expected to get rid of all synchronois vblank > > waits in plane codepaths (apart from ones for hw workarounds). > In my unify page flip and atomic series I first add all post_plane_update and > unpinning to unpin_work_fn. > > After that I have a patch to convert the last part of atomic after wait_for_vblanks > to schedule unpin_work_fn, next I'll get more bold and zap the wait_for_vblanks and run it async. > It depends on work->can_async_unpin, if it's false it has to wait for everything to finish before queueing the next flip, > so it will finish before next one starts. > > > Also does the helper actually wait for vblanks when the plane gets > > enabled w/o the framebuffer actually changing? > afaict it does, as long as the crtc is enabled.a Where is that code? All I see is a old_fb != new_fb check. > > I'm guessing that drm_atomic_helper_wait_for_vblanks should be fixed to look at crtc->state->active instead of crtc->state->enable.. > > >> - > >> - /* > >> * FIXME IPS should be fine as long as one plane is > >> * enabled, but in practice it seems to have problems > >> * when going from primary only to sprite only and vice > >> @@ -4760,9 +4752,6 @@ static void intel_post_plane_update(struct intel_crtc *crtc) > >> struct drm_i915_private *dev_priv = dev->dev_private; > >> struct drm_plane *plane; > >> > >> - if (atomic->wait_vblank) > >> - intel_wait_for_vblank(dev, crtc->pipe); > >> - > >> intel_frontbuffer_flip(dev, atomic->fb_bits); > >> > >> if (atomic->disable_cxsr) > >> @@ -11661,15 +11650,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, > >> if (plane->type != DRM_PLANE_TYPE_CURSOR) { > >> intel_crtc->atomic.disable_cxsr = true; > >> /* to potentially re-enable cxsr */ > >> - intel_crtc->atomic.wait_vblank = true; > > What's there to make sure cxsr doesn't get enabled/disabled at the wrong > > time now? I guess this is the same question as for the BDW w/a, ie. does > > the plane getting enabled/disabled (or rather becoming > > visible/invisible) always imply a vblank wait? > crtc mutex lock and in the future atomic wm's.. > >> intel_crtc->atomic.update_wm_post = true; > >> } > >> } else if (turn_off) { > >> intel_crtc->atomic.update_wm_post = true; > >> /* must disable cxsr around plane enable/disable */ > >> if (plane->type != DRM_PLANE_TYPE_CURSOR) { > >> - if (is_crtc_enabled) > >> - intel_crtc->atomic.wait_vblank = true; > >> intel_crtc->atomic.disable_cxsr = true; > >> } > >> } else if (intel_wm_need_update(plane, plane_state)) { > >> @@ -11716,21 +11702,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, > >> plane_state->rotation != BIT(DRM_ROTATE_0)) > >> intel_crtc->atomic.disable_fbc = true; > >> > >> - /* > >> - * BDW signals flip done immediately if the plane > >> - * is disabled, even if the plane enable is already > >> - * armed to occur at the next vblank :( > >> - */ > >> - if (turn_on && IS_BROADWELL(dev)) > >> - intel_crtc->atomic.wait_vblank = true; > >> - > >> intel_crtc->atomic.update_fbc |= visible || mode_changed; > >> break; > >> case DRM_PLANE_TYPE_CURSOR: > >> break; > >> case DRM_PLANE_TYPE_OVERLAY: > >> if (turn_off && !mode_changed) { > >> - intel_crtc->atomic.wait_vblank = true; > >> intel_crtc->atomic.update_sprite_watermarks |= > >> 1 << i; > >> } > >> @@ -13271,14 +13248,15 @@ static int intel_atomic_commit(struct drm_device *dev, > >> > >> if (put_domains) > >> modeset_put_power_domains(dev_priv, put_domains); > >> - > >> - intel_post_plane_update(intel_crtc); > >> } > >> > >> /* FIXME: add subpixel order */ > >> > >> drm_atomic_helper_wait_for_vblanks(dev, state); > >> > >> + for_each_crtc_in_state(state, crtc, crtc_state, i) > >> + intel_post_plane_update(to_intel_crtc(crtc)); > >> + > >> mutex_lock(&dev->struct_mutex); > >> drm_atomic_helper_cleanup_planes(dev, state); > >> mutex_unlock(&dev->struct_mutex); > >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > >> index 54a2c0da2ece..4b6bb1adfddd 100644 > >> --- a/drivers/gpu/drm/i915/intel_drv.h > >> +++ b/drivers/gpu/drm/i915/intel_drv.h > >> @@ -522,7 +522,6 @@ struct intel_crtc_atomic_commit { > >> > >> /* Sleepable operations to perform after commit */ > >> unsigned fb_bits; > >> - bool wait_vblank; > >> bool update_fbc; > >> bool post_enable_primary; > >> unsigned update_sprite_watermarks; > >> -- > >> 2.1.0 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 22-10-15 om 17:15 schreef Ville Syrjälä: > On Thu, Oct 22, 2015 at 04:50:05PM +0200, Maarten Lankhorst wrote: >> Op 22-10-15 om 15:30 schreef Ville Syrjälä: >>> On Thu, Oct 22, 2015 at 01:56:28PM +0200, Maarten Lankhorst wrote: >>>> By handling this after the atomic helper waits for vblanks there will >>>> be one less wait for vblank in the atomic path. >>>> >>>> Also get rid of the double wait_for_vblank on broadwell, looks like >>>> it's a bug doing the vblank wait twice. >>>> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_display.c | 28 +++------------------------- >>>> drivers/gpu/drm/i915/intel_drv.h | 1 - >>>> 2 files changed, 3 insertions(+), 26 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>> index 051a1e2b1c55..b8e1a5471bed 100644 >>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>> @@ -4672,14 +4672,6 @@ intel_post_enable_primary(struct drm_crtc *crtc) >>>> int pipe = intel_crtc->pipe; >>>> >>>> /* >>>> - * BDW signals flip done immediately if the plane >>>> - * is disabled, even if the plane enable is already >>>> - * armed to occur at the next vblank :( >>>> - */ >>>> - if (IS_BROADWELL(dev)) >>>> - intel_wait_for_vblank(dev, pipe); >>> The right fix for this crap is to not use the flip done interrupt. But >>> apparently no one wants to fix that. >> This is fixed later on when converting page_flip to atomic. I've modified >> page_flip_finished to ignore flips when visible_changed happens on broadwell. > Sounds like another hack. It's not a hack when it's a workaround for buggy hw. ;-) > >>> So now I'm worried that we now depend on the atomic helper doing >>> synchornous vblank waits here. Are we expecting those vblank waits to >>> remain there? I would have expected to get rid of all synchronois vblank >>> waits in plane codepaths (apart from ones for hw workarounds). >> In my unify page flip and atomic series I first add all post_plane_update and >> unpinning to unpin_work_fn. >> >> After that I have a patch to convert the last part of atomic after wait_for_vblanks >> to schedule unpin_work_fn, next I'll get more bold and zap the wait_for_vblanks and run it async. >> It depends on work->can_async_unpin, if it's false it has to wait for everything to finish before queueing the next flip, >> so it will finish before next one starts. >> >>> Also does the helper actually wait for vblanks when the plane gets >>> enabled w/o the framebuffer actually changing? >> afaict it does, as long as the crtc is enabled.a > Where is that code? All I see is a old_fb != new_fb check. Oh indeed, I missed that one. That would make it unreliable to use though. :-/ I think drm_atomic_helper_wait_for_vblanks should be fixed to look at crtc->state->active and maybe take a force flag to force a vblank wait.. Daniel would you be ok with such a patch? ~Maarten
On Mon, Oct 26, 2015 at 09:31:48AM +0100, Maarten Lankhorst wrote: > Op 22-10-15 om 17:15 schreef Ville Syrjälä: > > On Thu, Oct 22, 2015 at 04:50:05PM +0200, Maarten Lankhorst wrote: > >> Op 22-10-15 om 15:30 schreef Ville Syrjälä: > >>> On Thu, Oct 22, 2015 at 01:56:28PM +0200, Maarten Lankhorst wrote: > >>>> By handling this after the atomic helper waits for vblanks there will > >>>> be one less wait for vblank in the atomic path. > >>>> > >>>> Also get rid of the double wait_for_vblank on broadwell, looks like > >>>> it's a bug doing the vblank wait twice. > >>>> > >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>> --- > >>>> drivers/gpu/drm/i915/intel_display.c | 28 +++------------------------- > >>>> drivers/gpu/drm/i915/intel_drv.h | 1 - > >>>> 2 files changed, 3 insertions(+), 26 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >>>> index 051a1e2b1c55..b8e1a5471bed 100644 > >>>> --- a/drivers/gpu/drm/i915/intel_display.c > >>>> +++ b/drivers/gpu/drm/i915/intel_display.c > >>>> @@ -4672,14 +4672,6 @@ intel_post_enable_primary(struct drm_crtc *crtc) > >>>> int pipe = intel_crtc->pipe; > >>>> > >>>> /* > >>>> - * BDW signals flip done immediately if the plane > >>>> - * is disabled, even if the plane enable is already > >>>> - * armed to occur at the next vblank :( > >>>> - */ > >>>> - if (IS_BROADWELL(dev)) > >>>> - intel_wait_for_vblank(dev, pipe); > >>> The right fix for this crap is to not use the flip done interrupt. But > >>> apparently no one wants to fix that. > >> This is fixed later on when converting page_flip to atomic. I've modified > >> page_flip_finished to ignore flips when visible_changed happens on broadwell. > > Sounds like another hack. > It's not a hack when it's a workaround for buggy hw. ;-) > > > >>> So now I'm worried that we now depend on the atomic helper doing > >>> synchornous vblank waits here. Are we expecting those vblank waits to > >>> remain there? I would have expected to get rid of all synchronois vblank > >>> waits in plane codepaths (apart from ones for hw workarounds). > >> In my unify page flip and atomic series I first add all post_plane_update and > >> unpinning to unpin_work_fn. > >> > >> After that I have a patch to convert the last part of atomic after wait_for_vblanks > >> to schedule unpin_work_fn, next I'll get more bold and zap the wait_for_vblanks and run it async. > >> It depends on work->can_async_unpin, if it's false it has to wait for everything to finish before queueing the next flip, > >> so it will finish before next one starts. > >> > >>> Also does the helper actually wait for vblanks when the plane gets > >>> enabled w/o the framebuffer actually changing? > >> afaict it does, as long as the crtc is enabled.a > > Where is that code? All I see is a old_fb != new_fb check. > Oh indeed, I missed that one. That would make it unreliable to use though. :-/ > > I think drm_atomic_helper_wait_for_vblanks should be fixed to look at crtc->state->active and maybe take a force flag to force a vblank wait.. > > Daniel would you be ok with such a patch? The helper vblank wait is just to avoid tearing, which can't happen if the fb doesn't change. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 051a1e2b1c55..b8e1a5471bed 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4672,14 +4672,6 @@ intel_post_enable_primary(struct drm_crtc *crtc) int pipe = intel_crtc->pipe; /* - * BDW signals flip done immediately if the plane - * is disabled, even if the plane enable is already - * armed to occur at the next vblank :( - */ - if (IS_BROADWELL(dev)) - intel_wait_for_vblank(dev, pipe); - - /* * FIXME IPS should be fine as long as one plane is * enabled, but in practice it seems to have problems * when going from primary only to sprite only and vice @@ -4760,9 +4752,6 @@ static void intel_post_plane_update(struct intel_crtc *crtc) struct drm_i915_private *dev_priv = dev->dev_private; struct drm_plane *plane; - if (atomic->wait_vblank) - intel_wait_for_vblank(dev, crtc->pipe); - intel_frontbuffer_flip(dev, atomic->fb_bits); if (atomic->disable_cxsr) @@ -11661,15 +11650,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, if (plane->type != DRM_PLANE_TYPE_CURSOR) { intel_crtc->atomic.disable_cxsr = true; /* to potentially re-enable cxsr */ - intel_crtc->atomic.wait_vblank = true; intel_crtc->atomic.update_wm_post = true; } } else if (turn_off) { intel_crtc->atomic.update_wm_post = true; /* must disable cxsr around plane enable/disable */ if (plane->type != DRM_PLANE_TYPE_CURSOR) { - if (is_crtc_enabled) - intel_crtc->atomic.wait_vblank = true; intel_crtc->atomic.disable_cxsr = true; } } else if (intel_wm_need_update(plane, plane_state)) { @@ -11716,21 +11702,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, plane_state->rotation != BIT(DRM_ROTATE_0)) intel_crtc->atomic.disable_fbc = true; - /* - * BDW signals flip done immediately if the plane - * is disabled, even if the plane enable is already - * armed to occur at the next vblank :( - */ - if (turn_on && IS_BROADWELL(dev)) - intel_crtc->atomic.wait_vblank = true; - intel_crtc->atomic.update_fbc |= visible || mode_changed; break; case DRM_PLANE_TYPE_CURSOR: break; case DRM_PLANE_TYPE_OVERLAY: if (turn_off && !mode_changed) { - intel_crtc->atomic.wait_vblank = true; intel_crtc->atomic.update_sprite_watermarks |= 1 << i; } @@ -13271,14 +13248,15 @@ static int intel_atomic_commit(struct drm_device *dev, if (put_domains) modeset_put_power_domains(dev_priv, put_domains); - - intel_post_plane_update(intel_crtc); } /* FIXME: add subpixel order */ drm_atomic_helper_wait_for_vblanks(dev, state); + for_each_crtc_in_state(state, crtc, crtc_state, i) + intel_post_plane_update(to_intel_crtc(crtc)); + mutex_lock(&dev->struct_mutex); drm_atomic_helper_cleanup_planes(dev, state); mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 54a2c0da2ece..4b6bb1adfddd 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -522,7 +522,6 @@ struct intel_crtc_atomic_commit { /* Sleepable operations to perform after commit */ unsigned fb_bits; - bool wait_vblank; bool update_fbc; bool post_enable_primary; unsigned update_sprite_watermarks;
By handling this after the atomic helper waits for vblanks there will be one less wait for vblank in the atomic path. Also get rid of the double wait_for_vblank on broadwell, looks like it's a bug doing the vblank wait twice. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 28 +++------------------------- drivers/gpu/drm/i915/intel_drv.h | 1 - 2 files changed, 3 insertions(+), 26 deletions(-)