Re: [Intel-gfx] [PATCH 2/2] drm/i915: Remove delayed FBC activation.
On Thu, Jun 28, 2018 at 04:10:09PM +0200, Maarten Lankhorst wrote: > Op 27-06-18 om 16:14 schreef Ville Syrjälä: > > On Wed, Jun 27, 2018 at 01:45:04PM +0200, Maarten Lankhorst wrote: > >> Op 26-06-18 om 19:59 schreef Ville Syrjälä: > >>> On Mon, Jun 25, 2018 at 06:37:58PM +0200, Maarten Lankhorst wrote: > The only time we should start FBC is when we have waited a vblank > after the atomic update. > >>> What about front buffer tracking? Is this going to leave FBC permanently > >>> disabled unless there are flips/plane updates? > >> No, see intel_fbc_flush. If there's a race with frontbuffer tracking and > >> page flip, > >> we will not enable FBC in intel_fbc_flush(), but in that case we would > >> enable it from > >> intel_fbc_post_update(). > > I guess what I haven't quite figured out is why the pre_plane_update() > > even leaves fbc logically enabled. I would think we should just disable > > fbc there if anything important is going to change, and then re-enable > > at post_plane_update() after reallocating the cfb. > Indeed. > >> Or the other way around, intel_fbc_post_update won't enable FBC if the fb > >> is dirty, but > >> intel_fbc_flush() afterwards will. > >>> I think there are a few cases we need to consider: > >>> 1. plane update which doesn't need fbc disable > >>> 2. plane update which needs to disable fbc but can re-enable it after the > >>> flip > >>>has happended (eg. need to reallocate the cfb due to plane size/format > >>> change) > >>> 3. plane update which needs to disable fbc and can't re-enable it (eg. > >>> the new > >>>plane state no longer fbc compatible) > >>> 4. front buffer invalidate + flush > >>> > >>> HW nuke will handle case 1. Case 2 could do the fbc re-enable after the > >>> post-flip vblank wait. Case 3 would ideally let us move FBC to another > >>> plane (thinking of pre-HSW hardware here). And case 4 must re-enable fbc > >>> after the flush has happened. > >> I don't see how this code will break any case. :) > > OK, so I guess you just remove the delayed activation at > > flush/post_plane_update. So now it'll enable it immediately. > > > > Hmm, and if we just get a flush without the invalidate then we're going > > to just use the nuke msg register anyway. So I suppose this shouldn't > > cause much additional fbc on<->off ping pong. > > > > Actually, are we now effecitively forcing a synchronous vblank wait > > in pre_plane_update for every frame via the hw_deactivate() polling > > for fbc to stop? AFAICS with the re-enable now being immediate we're > > going to have to disable fbc every single time. I think the correct > > fix for that would involve a) not disabling fbc around flips when the > > hw flip nuke will suffice, and b) convert the "wait for fbc to disable" > > thing into a post_plane_update time assert (and verify whether the hw > > is actually happy with disabling both fbc and the plane at the same > > time). > Could we not block in hw_deactivate then? But only sure how much we still care about i8xx fbc since it's disabled by default. Hmm. I thought everyone waits there. Apparently I'm mistaken. Never mind then. We should probably remove the poll from the fbc1 code too then, and convert it to a post_plane_update assert. But we can defer to that to when we fix the other remaining issues in that code. Still a bit of redundant work to deactive+re-activate, but whatever. OK, I think this series should be fine as is: Reviewed-by: Ville Syrjälä -- Ville Syrjälä Intel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Remove delayed FBC activation.
Op 27-06-18 om 16:14 schreef Ville Syrjälä: > On Wed, Jun 27, 2018 at 01:45:04PM +0200, Maarten Lankhorst wrote: >> Op 26-06-18 om 19:59 schreef Ville Syrjälä: >>> On Mon, Jun 25, 2018 at 06:37:58PM +0200, Maarten Lankhorst wrote: The only time we should start FBC is when we have waited a vblank after the atomic update. >>> What about front buffer tracking? Is this going to leave FBC permanently >>> disabled unless there are flips/plane updates? >> No, see intel_fbc_flush. If there's a race with frontbuffer tracking and >> page flip, >> we will not enable FBC in intel_fbc_flush(), but in that case we would >> enable it from >> intel_fbc_post_update(). > I guess what I haven't quite figured out is why the pre_plane_update() > even leaves fbc logically enabled. I would think we should just disable > fbc there if anything important is going to change, and then re-enable > at post_plane_update() after reallocating the cfb. Indeed. >> Or the other way around, intel_fbc_post_update won't enable FBC if the fb is >> dirty, but >> intel_fbc_flush() afterwards will. >>> I think there are a few cases we need to consider: >>> 1. plane update which doesn't need fbc disable >>> 2. plane update which needs to disable fbc but can re-enable it after the >>> flip >>>has happended (eg. need to reallocate the cfb due to plane size/format >>> change) >>> 3. plane update which needs to disable fbc and can't re-enable it (eg. the >>> new >>>plane state no longer fbc compatible) >>> 4. front buffer invalidate + flush >>> >>> HW nuke will handle case 1. Case 2 could do the fbc re-enable after the >>> post-flip vblank wait. Case 3 would ideally let us move FBC to another >>> plane (thinking of pre-HSW hardware here). And case 4 must re-enable fbc >>> after the flush has happened. >> I don't see how this code will break any case. :) > OK, so I guess you just remove the delayed activation at > flush/post_plane_update. So now it'll enable it immediately. > > Hmm, and if we just get a flush without the invalidate then we're going > to just use the nuke msg register anyway. So I suppose this shouldn't > cause much additional fbc on<->off ping pong. > > Actually, are we now effecitively forcing a synchronous vblank wait > in pre_plane_update for every frame via the hw_deactivate() polling > for fbc to stop? AFAICS with the re-enable now being immediate we're > going to have to disable fbc every single time. I think the correct > fix for that would involve a) not disabling fbc around flips when the > hw flip nuke will suffice, and b) convert the "wait for fbc to disable" > thing into a post_plane_update time assert (and verify whether the hw > is actually happy with disabling both fbc and the plane at the same > time). Could we not block in hw_deactivate then? But only https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Remove delayed FBC activation.
On Wed, Jun 27, 2018 at 01:45:04PM +0200, Maarten Lankhorst wrote:
> Op 26-06-18 om 19:59 schreef Ville Syrjälä:
> > On Mon, Jun 25, 2018 at 06:37:58PM +0200, Maarten Lankhorst wrote:
> >> The only time we should start FBC is when we have waited a vblank
> >> after the atomic update.
> > What about front buffer tracking? Is this going to leave FBC permanently
> > disabled unless there are flips/plane updates?
> No, see intel_fbc_flush. If there's a race with frontbuffer tracking and page
> flip,
> we will not enable FBC in intel_fbc_flush(), but in that case we would enable
> it from
> intel_fbc_post_update().
I guess what I haven't quite figured out is why the pre_plane_update()
even leaves fbc logically enabled. I would think we should just disable
fbc there if anything important is going to change, and then re-enable
at post_plane_update() after reallocating the cfb.
>
> Or the other way around, intel_fbc_post_update won't enable FBC if the fb is
> dirty, but
> intel_fbc_flush() afterwards will.
> > I think there are a few cases we need to consider:
> > 1. plane update which doesn't need fbc disable
> > 2. plane update which needs to disable fbc but can re-enable it after the
> > flip
> >has happended (eg. need to reallocate the cfb due to plane size/format
> > change)
> > 3. plane update which needs to disable fbc and can't re-enable it (eg. the
> > new
> >plane state no longer fbc compatible)
> > 4. front buffer invalidate + flush
> >
> > HW nuke will handle case 1. Case 2 could do the fbc re-enable after the
> > post-flip vblank wait. Case 3 would ideally let us move FBC to another
> > plane (thinking of pre-HSW hardware here). And case 4 must re-enable fbc
> > after the flush has happened.
> I don't see how this code will break any case. :)
OK, so I guess you just remove the delayed activation at
flush/post_plane_update. So now it'll enable it immediately.
Hmm, and if we just get a flush without the invalidate then we're going
to just use the nuke msg register anyway. So I suppose this shouldn't
cause much additional fbc on<->off ping pong.
Actually, are we now effecitively forcing a synchronous vblank wait
in pre_plane_update for every frame via the hw_deactivate() polling
for fbc to stop? AFAICS with the re-enable now being immediate we're
going to have to disable fbc every single time. I think the correct
fix for that would involve a) not disabling fbc around flips when the
hw flip nuke will suffice, and b) convert the "wait for fbc to disable"
thing into a post_plane_update time assert (and verify whether the hw
is actually happy with disabling both fbc and the plane at the same
time).
>
> ~Maarten
>
> >> We've already forced a vblank wait by doing
> >> wait_for_flip_done before intel_post_plane_update(), so we don't need
> >> to wait a second time before enabling.
> >>
> >> Removing the worker simplifies the code and removes possible race
> >> conditions, like happening in 103167.
> >>
> >> Cc: Paulo Zanoni
> >> Cc: Rodrigo Vivi
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103167
> >> Signed-off-by: Maarten Lankhorst
> >> ---
> >> drivers/gpu/drm/i915/i915_debugfs.c | 5 --
> >> drivers/gpu/drm/i915/i915_drv.h | 6 --
> >> drivers/gpu/drm/i915/intel_fbc.c| 96 +
> >> 3 files changed, 1 insertion(+), 106 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> >> b/drivers/gpu/drm/i915/i915_debugfs.c
> >> index c400f42a54ec..48a57c0636bf 100644
> >> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> >> @@ -1659,11 +1659,6 @@ static int i915_fbc_status(struct seq_file *m, void
> >> *unused)
> >>else
> >>seq_printf(m, "FBC disabled: %s\n", fbc->no_fbc_reason);
> >>
> >> - if (fbc->work.scheduled)
> >> - seq_printf(m, "FBC worker scheduled on vblank %llu, now %llu\n",
> >> - fbc->work.scheduled_vblank,
> >> - drm_crtc_vblank_count(>crtc->base));
> >> -
> >>if (intel_fbc_is_active(dev_priv)) {
> >>u32 mask;
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >> b/drivers/gpu/drm/i915/i915_drv.h
> >> index 328d4312c438..9645dcb30454 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -580,12 +580,6 @@ struct intel_fbc {
> >>unsigned int gen9_wa_cfb_stride;
> >>} params;
> >>
> >> - struct intel_fbc_work {
> >> - bool scheduled;
> >> - u64 scheduled_vblank;
> >> - struct work_struct work;
> >> - } work;
> >> -
> >>const char *no_fbc_reason;
> >> };
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> >> b/drivers/gpu/drm/i915/intel_fbc.c
> >> index 9f9ea0b5452f..01d1d2088f04 100644
> >> --- a/drivers/gpu/drm/i915/intel_fbc.c
> >> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> >> @@ -399,89 +399,6 @@ bool intel_fbc_is_active(struct drm_i915_private
> >> *dev_priv)
> >>
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Remove delayed FBC activation.
Op 26-06-18 om 19:59 schreef Ville Syrjälä:
> On Mon, Jun 25, 2018 at 06:37:58PM +0200, Maarten Lankhorst wrote:
>> The only time we should start FBC is when we have waited a vblank
>> after the atomic update.
> What about front buffer tracking? Is this going to leave FBC permanently
> disabled unless there are flips/plane updates?
No, see intel_fbc_flush. If there's a race with frontbuffer tracking and page
flip,
we will not enable FBC in intel_fbc_flush(), but in that case we would enable
it from
intel_fbc_post_update().
Or the other way around, intel_fbc_post_update won't enable FBC if the fb is
dirty, but
intel_fbc_flush() afterwards will.
> I think there are a few cases we need to consider:
> 1. plane update which doesn't need fbc disable
> 2. plane update which needs to disable fbc but can re-enable it after the flip
>has happended (eg. need to reallocate the cfb due to plane size/format
> change)
> 3. plane update which needs to disable fbc and can't re-enable it (eg. the new
>plane state no longer fbc compatible)
> 4. front buffer invalidate + flush
>
> HW nuke will handle case 1. Case 2 could do the fbc re-enable after the
> post-flip vblank wait. Case 3 would ideally let us move FBC to another
> plane (thinking of pre-HSW hardware here). And case 4 must re-enable fbc
> after the flush has happened.
I don't see how this code will break any case. :)
~Maarten
>> We've already forced a vblank wait by doing
>> wait_for_flip_done before intel_post_plane_update(), so we don't need
>> to wait a second time before enabling.
>>
>> Removing the worker simplifies the code and removes possible race
>> conditions, like happening in 103167.
>>
>> Cc: Paulo Zanoni
>> Cc: Rodrigo Vivi
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103167
>> Signed-off-by: Maarten Lankhorst
>> ---
>> drivers/gpu/drm/i915/i915_debugfs.c | 5 --
>> drivers/gpu/drm/i915/i915_drv.h | 6 --
>> drivers/gpu/drm/i915/intel_fbc.c| 96 +
>> 3 files changed, 1 insertion(+), 106 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index c400f42a54ec..48a57c0636bf 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1659,11 +1659,6 @@ static int i915_fbc_status(struct seq_file *m, void
>> *unused)
>> else
>> seq_printf(m, "FBC disabled: %s\n", fbc->no_fbc_reason);
>>
>> -if (fbc->work.scheduled)
>> -seq_printf(m, "FBC worker scheduled on vblank %llu, now %llu\n",
>> - fbc->work.scheduled_vblank,
>> - drm_crtc_vblank_count(>crtc->base));
>> -
>> if (intel_fbc_is_active(dev_priv)) {
>> u32 mask;
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 328d4312c438..9645dcb30454 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -580,12 +580,6 @@ struct intel_fbc {
>> unsigned int gen9_wa_cfb_stride;
>> } params;
>>
>> -struct intel_fbc_work {
>> -bool scheduled;
>> -u64 scheduled_vblank;
>> -struct work_struct work;
>> -} work;
>> -
>> const char *no_fbc_reason;
>> };
>>
>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c
>> b/drivers/gpu/drm/i915/intel_fbc.c
>> index 9f9ea0b5452f..01d1d2088f04 100644
>> --- a/drivers/gpu/drm/i915/intel_fbc.c
>> +++ b/drivers/gpu/drm/i915/intel_fbc.c
>> @@ -399,89 +399,6 @@ bool intel_fbc_is_active(struct drm_i915_private
>> *dev_priv)
>> return dev_priv->fbc.active;
>> }
>>
>> -static void intel_fbc_work_fn(struct work_struct *__work)
>> -{
>> -struct drm_i915_private *dev_priv =
>> -container_of(__work, struct drm_i915_private, fbc.work.work);
>> -struct intel_fbc *fbc = _priv->fbc;
>> -struct intel_fbc_work *work = >work;
>> -struct intel_crtc *crtc = fbc->crtc;
>> -struct drm_vblank_crtc *vblank = _priv->drm.vblank[crtc->pipe];
>> -
>> -if (drm_crtc_vblank_get(>base)) {
>> -/* CRTC is now off, leave FBC deactivated */
>> -mutex_lock(>lock);
>> -work->scheduled = false;
>> -mutex_unlock(>lock);
>> -return;
>> -}
>> -
>> -retry:
>> -/* Delay the actual enabling to let pageflipping cease and the
>> - * display to settle before starting the compression. Note that
>> - * this delay also serves a second purpose: it allows for a
>> - * vblank to pass after disabling the FBC before we attempt
>> - * to modify the control registers.
>> - *
>> - * WaFbcWaitForVBlankBeforeEnable:ilk,snb
>> - *
>> - * It is also worth mentioning that since work->scheduled_vblank can be
>> - * updated multiple times by the other threads, hitting the timeout is
>> - * not an error condition. We'll just end up hitting the "goto retry"
>> - * case below.
>> -
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Remove delayed FBC activation.
On Mon, Jun 25, 2018 at 06:37:58PM +0200, Maarten Lankhorst wrote:
> The only time we should start FBC is when we have waited a vblank
> after the atomic update.
What about front buffer tracking? Is this going to leave FBC permanently
disabled unless there are flips/plane updates?
I think there are a few cases we need to consider:
1. plane update which doesn't need fbc disable
2. plane update which needs to disable fbc but can re-enable it after the flip
has happended (eg. need to reallocate the cfb due to plane size/format
change)
3. plane update which needs to disable fbc and can't re-enable it (eg. the new
plane state no longer fbc compatible)
4. front buffer invalidate + flush
HW nuke will handle case 1. Case 2 could do the fbc re-enable after the
post-flip vblank wait. Case 3 would ideally let us move FBC to another
plane (thinking of pre-HSW hardware here). And case 4 must re-enable fbc
after the flush has happened.
> We've already forced a vblank wait by doing
> wait_for_flip_done before intel_post_plane_update(), so we don't need
> to wait a second time before enabling.
>
> Removing the worker simplifies the code and removes possible race
> conditions, like happening in 103167.
>
> Cc: Paulo Zanoni
> Cc: Rodrigo Vivi
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103167
> Signed-off-by: Maarten Lankhorst
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 5 --
> drivers/gpu/drm/i915/i915_drv.h | 6 --
> drivers/gpu/drm/i915/intel_fbc.c| 96 +
> 3 files changed, 1 insertion(+), 106 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index c400f42a54ec..48a57c0636bf 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1659,11 +1659,6 @@ static int i915_fbc_status(struct seq_file *m, void
> *unused)
> else
> seq_printf(m, "FBC disabled: %s\n", fbc->no_fbc_reason);
>
> - if (fbc->work.scheduled)
> - seq_printf(m, "FBC worker scheduled on vblank %llu, now %llu\n",
> -fbc->work.scheduled_vblank,
> -drm_crtc_vblank_count(>crtc->base));
> -
> if (intel_fbc_is_active(dev_priv)) {
> u32 mask;
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 328d4312c438..9645dcb30454 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -580,12 +580,6 @@ struct intel_fbc {
> unsigned int gen9_wa_cfb_stride;
> } params;
>
> - struct intel_fbc_work {
> - bool scheduled;
> - u64 scheduled_vblank;
> - struct work_struct work;
> - } work;
> -
> const char *no_fbc_reason;
> };
>
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> b/drivers/gpu/drm/i915/intel_fbc.c
> index 9f9ea0b5452f..01d1d2088f04 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -399,89 +399,6 @@ bool intel_fbc_is_active(struct drm_i915_private
> *dev_priv)
> return dev_priv->fbc.active;
> }
>
> -static void intel_fbc_work_fn(struct work_struct *__work)
> -{
> - struct drm_i915_private *dev_priv =
> - container_of(__work, struct drm_i915_private, fbc.work.work);
> - struct intel_fbc *fbc = _priv->fbc;
> - struct intel_fbc_work *work = >work;
> - struct intel_crtc *crtc = fbc->crtc;
> - struct drm_vblank_crtc *vblank = _priv->drm.vblank[crtc->pipe];
> -
> - if (drm_crtc_vblank_get(>base)) {
> - /* CRTC is now off, leave FBC deactivated */
> - mutex_lock(>lock);
> - work->scheduled = false;
> - mutex_unlock(>lock);
> - return;
> - }
> -
> -retry:
> - /* Delay the actual enabling to let pageflipping cease and the
> - * display to settle before starting the compression. Note that
> - * this delay also serves a second purpose: it allows for a
> - * vblank to pass after disabling the FBC before we attempt
> - * to modify the control registers.
> - *
> - * WaFbcWaitForVBlankBeforeEnable:ilk,snb
> - *
> - * It is also worth mentioning that since work->scheduled_vblank can be
> - * updated multiple times by the other threads, hitting the timeout is
> - * not an error condition. We'll just end up hitting the "goto retry"
> - * case below.
> - */
> - wait_event_timeout(vblank->queue,
> - drm_crtc_vblank_count(>base) != work->scheduled_vblank,
> - msecs_to_jiffies(50));
> -
> - mutex_lock(>lock);
> -
> - /* Were we cancelled? */
> - if (!work->scheduled)
> - goto out;
> -
> - /* Were we delayed again while this function was sleeping? */
> - if (drm_crtc_vblank_count(>base) == work->scheduled_vblank) {
> - mutex_unlock(>lock);
> - goto retry;
> - }
> -
