| Message ID | 20191127201222.16669-15-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/fbc: Fix FBC for glk+ | expand |
Op 27-11-2019 om 21:12 schreef Ville Syrjala: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > The code assumes we can omit the cfb allocation once fbc > has been enabled once. That's nonsense. Let's try to > reallocate it if we need to. > > The code is still a mess, but maybe this is enough to get > fbc going in some cases where it initially underallocates > the cfb and there's no full modeset to fix it up. > > Cc: Daniel Drake <drake@endlessm.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Jian-Hong Pan <jian-hong@endlessm.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_fbc.c | 22 +++++++++++++++------- > 1 file changed, 15 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > index c976698b0729..928059a5da80 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > @@ -672,6 +672,14 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, > cache->fence_id = -1; > } > > +static bool intel_fbc_cfb_size_changed(struct drm_i915_private *dev_priv) > +{ > + struct intel_fbc *fbc = &dev_priv->fbc; > + > + return intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) > > + fbc->compressed_fb.size * fbc->threshold; > +} > + > static bool intel_fbc_can_activate(struct intel_crtc *crtc) > { > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > @@ -757,8 +765,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) > * we didn't get any invalidate/deactivate calls, but this would require > * a lot of tracking just for a specific case. If we conclude it's an > * important case, we can implement it later. */ > - if (intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) > > - fbc->compressed_fb.size * fbc->threshold) { > + if (intel_fbc_cfb_size_changed(dev_priv)) { > fbc->no_fbc_reason = "CFB requirements changed"; > return false; > } > @@ -1112,12 +1119,12 @@ void intel_fbc_enable(struct intel_crtc *crtc, > mutex_lock(&fbc->lock); > > if (fbc->crtc) { > - WARN_ON(fbc->crtc == crtc && !crtc_state->enable_fbc); > - goto out; > - } > + if (fbc->crtc != crtc || > + !intel_fbc_cfb_size_changed(dev_priv)) > + goto out; > > - if (!crtc_state->enable_fbc) > - goto out; > + __intel_fbc_disable(dev_priv); > + } > > WARN_ON(fbc->active); > > @@ -1130,6 +1137,7 @@ void intel_fbc_enable(struct intel_crtc *crtc, > if (intel_fbc_alloc_cfb(dev_priv, > intel_fbc_calculate_cfb_size(dev_priv, cache), > fb->format->cpp[0])) { > + cache->plane.visible = false; > fbc->no_fbc_reason = "not enough stolen memory"; > goto out; > } Makes sense, unfortunately kms_cursor_legacy starts failing on this series. :( For 1-11, 14 Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> We should probably get rid of the FBC disable on frontbuffer disable as well. I had some patches but nothing upstream-worthy yet. :( 12 and 13 need more thought for now, kms_cursor_legacy is failing.
On Thu, Nov 28, 2019 at 04:48:04PM +0100, Maarten Lankhorst wrote: > Op 27-11-2019 om 21:12 schreef Ville Syrjala: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > The code assumes we can omit the cfb allocation once fbc > > has been enabled once. That's nonsense. Let's try to > > reallocate it if we need to. > > > > The code is still a mess, but maybe this is enough to get > > fbc going in some cases where it initially underallocates > > the cfb and there's no full modeset to fix it up. > > > > Cc: Daniel Drake <drake@endlessm.com> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Cc: Jian-Hong Pan <jian-hong@endlessm.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_fbc.c | 22 +++++++++++++++------- > > 1 file changed, 15 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > > index c976698b0729..928059a5da80 100644 > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > @@ -672,6 +672,14 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, > > cache->fence_id = -1; > > } > > > > +static bool intel_fbc_cfb_size_changed(struct drm_i915_private *dev_priv) > > +{ > > + struct intel_fbc *fbc = &dev_priv->fbc; > > + > > + return intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) > > > + fbc->compressed_fb.size * fbc->threshold; > > +} > > + > > static bool intel_fbc_can_activate(struct intel_crtc *crtc) > > { > > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > @@ -757,8 +765,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) > > * we didn't get any invalidate/deactivate calls, but this would require > > * a lot of tracking just for a specific case. If we conclude it's an > > * important case, we can implement it later. */ > > - if (intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) > > > - fbc->compressed_fb.size * fbc->threshold) { > > + if (intel_fbc_cfb_size_changed(dev_priv)) { > > fbc->no_fbc_reason = "CFB requirements changed"; > > return false; > > } > > @@ -1112,12 +1119,12 @@ void intel_fbc_enable(struct intel_crtc *crtc, > > mutex_lock(&fbc->lock); > > > > if (fbc->crtc) { > > - WARN_ON(fbc->crtc == crtc && !crtc_state->enable_fbc); > > - goto out; > > - } > > + if (fbc->crtc != crtc || > > + !intel_fbc_cfb_size_changed(dev_priv)) > > + goto out; > > > > - if (!crtc_state->enable_fbc) > > - goto out; > > + __intel_fbc_disable(dev_priv); > > + } > > > > WARN_ON(fbc->active); > > > > @@ -1130,6 +1137,7 @@ void intel_fbc_enable(struct intel_crtc *crtc, > > if (intel_fbc_alloc_cfb(dev_priv, > > intel_fbc_calculate_cfb_size(dev_priv, cache), > > fb->format->cpp[0])) { > > + cache->plane.visible = false; > > fbc->no_fbc_reason = "not enough stolen memory"; > > goto out; > > } > > Makes sense, unfortunately kms_cursor_legacy starts failing on this series. :( > > For 1-11, 14 > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > We should probably get rid of the FBC disable on frontbuffer disable as well. I had some patches but nothing upstream-worthy yet. :( How would we get rid of the disable there? By triggering nukes at some predefined interval? Doesn't sound all that great. > > 12 and 13 need more thought for now, kms_cursor_legacy is failing. Already posted the v2 that fixes it.
Op 28-11-2019 om 16:59 schreef Ville Syrjälä: > On Thu, Nov 28, 2019 at 04:48:04PM +0100, Maarten Lankhorst wrote: >> Op 27-11-2019 om 21:12 schreef Ville Syrjala: >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> >>> The code assumes we can omit the cfb allocation once fbc >>> has been enabled once. That's nonsense. Let's try to >>> reallocate it if we need to. >>> >>> The code is still a mess, but maybe this is enough to get >>> fbc going in some cases where it initially underallocates >>> the cfb and there's no full modeset to fix it up. >>> >>> Cc: Daniel Drake <drake@endlessm.com> >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> >>> Cc: Jian-Hong Pan <jian-hong@endlessm.com> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> --- >>> drivers/gpu/drm/i915/display/intel_fbc.c | 22 +++++++++++++++------- >>> 1 file changed, 15 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c >>> index c976698b0729..928059a5da80 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_fbc.c >>> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c >>> @@ -672,6 +672,14 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, >>> cache->fence_id = -1; >>> } >>> >>> +static bool intel_fbc_cfb_size_changed(struct drm_i915_private *dev_priv) >>> +{ >>> + struct intel_fbc *fbc = &dev_priv->fbc; >>> + >>> + return intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) > >>> + fbc->compressed_fb.size * fbc->threshold; >>> +} >>> + >>> static bool intel_fbc_can_activate(struct intel_crtc *crtc) >>> { >>> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >>> @@ -757,8 +765,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) >>> * we didn't get any invalidate/deactivate calls, but this would require >>> * a lot of tracking just for a specific case. If we conclude it's an >>> * important case, we can implement it later. */ >>> - if (intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) > >>> - fbc->compressed_fb.size * fbc->threshold) { >>> + if (intel_fbc_cfb_size_changed(dev_priv)) { >>> fbc->no_fbc_reason = "CFB requirements changed"; >>> return false; >>> } >>> @@ -1112,12 +1119,12 @@ void intel_fbc_enable(struct intel_crtc *crtc, >>> mutex_lock(&fbc->lock); >>> >>> if (fbc->crtc) { >>> - WARN_ON(fbc->crtc == crtc && !crtc_state->enable_fbc); >>> - goto out; >>> - } >>> + if (fbc->crtc != crtc || >>> + !intel_fbc_cfb_size_changed(dev_priv)) >>> + goto out; >>> >>> - if (!crtc_state->enable_fbc) >>> - goto out; >>> + __intel_fbc_disable(dev_priv); >>> + } >>> >>> WARN_ON(fbc->active); >>> >>> @@ -1130,6 +1137,7 @@ void intel_fbc_enable(struct intel_crtc *crtc, >>> if (intel_fbc_alloc_cfb(dev_priv, >>> intel_fbc_calculate_cfb_size(dev_priv, cache), >>> fb->format->cpp[0])) { >>> + cache->plane.visible = false; >>> fbc->no_fbc_reason = "not enough stolen memory"; >>> goto out; >>> } >> Makes sense, unfortunately kms_cursor_legacy starts failing on this series. :( >> >> For 1-11, 14 >> >> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> >> We should probably get rid of the FBC disable on frontbuffer disable as well. I had some patches but nothing upstream-worthy yet. :( > How would we get rid of the disable there? By triggering nukes at some > predefined interval? Doesn't sound all that great. Not touching FBC on frontbuffer write at all, and forcing userspace to use the dirtyfb api. I think the whole implicit tracking should be removed. > >> 12 and 13 need more thought for now, kms_cursor_legacy is failing. > Already posted the v2 that fixes it. > >
On Fri, Nov 29, 2019 at 09:48:45AM +0100, Maarten Lankhorst wrote: > Op 28-11-2019 om 16:59 schreef Ville Syrjälä: > > On Thu, Nov 28, 2019 at 04:48:04PM +0100, Maarten Lankhorst wrote: > >> Op 27-11-2019 om 21:12 schreef Ville Syrjala: > >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> > >>> > >>> The code assumes we can omit the cfb allocation once fbc > >>> has been enabled once. That's nonsense. Let's try to > >>> reallocate it if we need to. > >>> > >>> The code is still a mess, but maybe this is enough to get > >>> fbc going in some cases where it initially underallocates > >>> the cfb and there's no full modeset to fix it up. > >>> > >>> Cc: Daniel Drake <drake@endlessm.com> > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > >>> Cc: Jian-Hong Pan <jian-hong@endlessm.com> > >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >>> --- > >>> drivers/gpu/drm/i915/display/intel_fbc.c | 22 +++++++++++++++------- > >>> 1 file changed, 15 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > >>> index c976698b0729..928059a5da80 100644 > >>> --- a/drivers/gpu/drm/i915/display/intel_fbc.c > >>> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > >>> @@ -672,6 +672,14 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, > >>> cache->fence_id = -1; > >>> } > >>> > >>> +static bool intel_fbc_cfb_size_changed(struct drm_i915_private *dev_priv) > >>> +{ > >>> + struct intel_fbc *fbc = &dev_priv->fbc; > >>> + > >>> + return intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) > > >>> + fbc->compressed_fb.size * fbc->threshold; > >>> +} > >>> + > >>> static bool intel_fbc_can_activate(struct intel_crtc *crtc) > >>> { > >>> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > >>> @@ -757,8 +765,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) > >>> * we didn't get any invalidate/deactivate calls, but this would require > >>> * a lot of tracking just for a specific case. If we conclude it's an > >>> * important case, we can implement it later. */ > >>> - if (intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) > > >>> - fbc->compressed_fb.size * fbc->threshold) { > >>> + if (intel_fbc_cfb_size_changed(dev_priv)) { > >>> fbc->no_fbc_reason = "CFB requirements changed"; > >>> return false; > >>> } > >>> @@ -1112,12 +1119,12 @@ void intel_fbc_enable(struct intel_crtc *crtc, > >>> mutex_lock(&fbc->lock); > >>> > >>> if (fbc->crtc) { > >>> - WARN_ON(fbc->crtc == crtc && !crtc_state->enable_fbc); > >>> - goto out; > >>> - } > >>> + if (fbc->crtc != crtc || > >>> + !intel_fbc_cfb_size_changed(dev_priv)) > >>> + goto out; > >>> > >>> - if (!crtc_state->enable_fbc) > >>> - goto out; > >>> + __intel_fbc_disable(dev_priv); > >>> + } > >>> > >>> WARN_ON(fbc->active); > >>> > >>> @@ -1130,6 +1137,7 @@ void intel_fbc_enable(struct intel_crtc *crtc, > >>> if (intel_fbc_alloc_cfb(dev_priv, > >>> intel_fbc_calculate_cfb_size(dev_priv, cache), > >>> fb->format->cpp[0])) { > >>> + cache->plane.visible = false; > >>> fbc->no_fbc_reason = "not enough stolen memory"; > >>> goto out; > >>> } > >> Makes sense, unfortunately kms_cursor_legacy starts failing on this series. :( > >> > >> For 1-11, 14 > >> > >> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> > >> We should probably get rid of the FBC disable on frontbuffer disable as well. I had some patches but nothing upstream-worthy yet. :( > > How would we get rid of the disable there? By triggering nukes at some > > predefined interval? Doesn't sound all that great. > Not touching FBC on frontbuffer write at all, and forcing userspace to use the dirtyfb api. I think the whole implicit tracking should be removed. Perhaps. Not sure userspace is ready for that though. I guess the only long lasting frontbuffer invalidate is the one from set_domain. Everything else is bounded and so we know the flush is going to come in a somewhat timely manner. So for those cases I guess we could perhaps skip the invalidate. Hmm. Also looks like ORIGIN_GTT has been neutered and now we treat everyting as ORIGIN_CPU. That's maybe not so great. Should probably reinstate ORIGIN_GTT so we can actually benefit from the hw gtt tracking. Or we just try to kill that off as well. Also I wonder where is the flush counterpart to the invalidate in i915_gem_object_prepare_write()?
Op 29-11-2019 om 12:37 schreef Ville Syrjälä: > On Fri, Nov 29, 2019 at 09:48:45AM +0100, Maarten Lankhorst wrote: >> Op 28-11-2019 om 16:59 schreef Ville Syrjälä: >>> On Thu, Nov 28, 2019 at 04:48:04PM +0100, Maarten Lankhorst wrote: >>>> Op 27-11-2019 om 21:12 schreef Ville Syrjala: >>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >>>>> >>>>> The code assumes we can omit the cfb allocation once fbc >>>>> has been enabled once. That's nonsense. Let's try to >>>>> reallocate it if we need to. >>>>> >>>>> The code is still a mess, but maybe this is enough to get >>>>> fbc going in some cases where it initially underallocates >>>>> the cfb and there's no full modeset to fix it up. >>>>> >>>>> Cc: Daniel Drake <drake@endlessm.com> >>>>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> >>>>> Cc: Jian-Hong Pan <jian-hong@endlessm.com> >>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >>>>> --- >>>>> drivers/gpu/drm/i915/display/intel_fbc.c | 22 +++++++++++++++------- >>>>> 1 file changed, 15 insertions(+), 7 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c >>>>> index c976698b0729..928059a5da80 100644 >>>>> --- a/drivers/gpu/drm/i915/display/intel_fbc.c >>>>> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c >>>>> @@ -672,6 +672,14 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, >>>>> cache->fence_id = -1; >>>>> } >>>>> >>>>> +static bool intel_fbc_cfb_size_changed(struct drm_i915_private *dev_priv) >>>>> +{ >>>>> + struct intel_fbc *fbc = &dev_priv->fbc; >>>>> + >>>>> + return intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) > >>>>> + fbc->compressed_fb.size * fbc->threshold; >>>>> +} >>>>> + >>>>> static bool intel_fbc_can_activate(struct intel_crtc *crtc) >>>>> { >>>>> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >>>>> @@ -757,8 +765,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) >>>>> * we didn't get any invalidate/deactivate calls, but this would require >>>>> * a lot of tracking just for a specific case. If we conclude it's an >>>>> * important case, we can implement it later. */ >>>>> - if (intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) > >>>>> - fbc->compressed_fb.size * fbc->threshold) { >>>>> + if (intel_fbc_cfb_size_changed(dev_priv)) { >>>>> fbc->no_fbc_reason = "CFB requirements changed"; >>>>> return false; >>>>> } >>>>> @@ -1112,12 +1119,12 @@ void intel_fbc_enable(struct intel_crtc *crtc, >>>>> mutex_lock(&fbc->lock); >>>>> >>>>> if (fbc->crtc) { >>>>> - WARN_ON(fbc->crtc == crtc && !crtc_state->enable_fbc); >>>>> - goto out; >>>>> - } >>>>> + if (fbc->crtc != crtc || >>>>> + !intel_fbc_cfb_size_changed(dev_priv)) >>>>> + goto out; >>>>> >>>>> - if (!crtc_state->enable_fbc) >>>>> - goto out; >>>>> + __intel_fbc_disable(dev_priv); >>>>> + } >>>>> >>>>> WARN_ON(fbc->active); >>>>> >>>>> @@ -1130,6 +1137,7 @@ void intel_fbc_enable(struct intel_crtc *crtc, >>>>> if (intel_fbc_alloc_cfb(dev_priv, >>>>> intel_fbc_calculate_cfb_size(dev_priv, cache), >>>>> fb->format->cpp[0])) { >>>>> + cache->plane.visible = false; >>>>> fbc->no_fbc_reason = "not enough stolen memory"; >>>>> goto out; >>>>> } >>>> Makes sense, unfortunately kms_cursor_legacy starts failing on this series. :( >>>> >>>> For 1-11, 14 >>>> >>>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> >>>> We should probably get rid of the FBC disable on frontbuffer disable as well. I had some patches but nothing upstream-worthy yet. :( >>> How would we get rid of the disable there? By triggering nukes at some >>> predefined interval? Doesn't sound all that great. >> Not touching FBC on frontbuffer write at all, and forcing userspace to use the dirtyfb api. I think the whole implicit tracking should be removed. > Perhaps. Not sure userspace is ready for that though. We have to audit that DirtyFB is called on all gen9+ userspace, because FBC is only enabled by default on those platforms. I know the modesetting ddx does, I believe xf86-video-intel as well. So it should be safe to do. We could hide the old behavior behind a kernel parameter for now for 1 or 2 releases, so we can chicken out if needed. > I guess the only long lasting frontbuffer invalidate is the > one from set_domain. Everything else is bounded and so we > know the flush is going to come in a somewhat timely manner. > So for those cases I guess we could perhaps skip the invalidate. > > Hmm. Also looks like ORIGIN_GTT has been neutered and now > we treat everyting as ORIGIN_CPU. That's maybe not so great. > Should probably reinstate ORIGIN_GTT so we can actually benefit > from the hw gtt tracking. Or we just try to kill that off as well. HW tracking has been buggy for a long time, and is no longer available on current hw because of those bugs. > Also I wonder where is the flush counterpart to the invalidate > in i915_gem_object_prepare_write()? > Not sure.
On Tue, Dec 03, 2019 at 09:45:19AM +0100, Maarten Lankhorst wrote: > Op 29-11-2019 om 12:37 schreef Ville Syrjälä: > > On Fri, Nov 29, 2019 at 09:48:45AM +0100, Maarten Lankhorst wrote: > >> Op 28-11-2019 om 16:59 schreef Ville Syrjälä: > >>> On Thu, Nov 28, 2019 at 04:48:04PM +0100, Maarten Lankhorst wrote: > >>>> Op 27-11-2019 om 21:12 schreef Ville Syrjala: > >>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> > >>>>> > >>>>> The code assumes we can omit the cfb allocation once fbc > >>>>> has been enabled once. That's nonsense. Let's try to > >>>>> reallocate it if we need to. > >>>>> > >>>>> The code is still a mess, but maybe this is enough to get > >>>>> fbc going in some cases where it initially underallocates > >>>>> the cfb and there's no full modeset to fix it up. > >>>>> > >>>>> Cc: Daniel Drake <drake@endlessm.com> > >>>>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > >>>>> Cc: Jian-Hong Pan <jian-hong@endlessm.com> > >>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >>>>> --- > >>>>> drivers/gpu/drm/i915/display/intel_fbc.c | 22 +++++++++++++++------- > >>>>> 1 file changed, 15 insertions(+), 7 deletions(-) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > >>>>> index c976698b0729..928059a5da80 100644 > >>>>> --- a/drivers/gpu/drm/i915/display/intel_fbc.c > >>>>> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > >>>>> @@ -672,6 +672,14 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, > >>>>> cache->fence_id = -1; > >>>>> } > >>>>> > >>>>> +static bool intel_fbc_cfb_size_changed(struct drm_i915_private *dev_priv) > >>>>> +{ > >>>>> + struct intel_fbc *fbc = &dev_priv->fbc; > >>>>> + > >>>>> + return intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) > > >>>>> + fbc->compressed_fb.size * fbc->threshold; > >>>>> +} > >>>>> + > >>>>> static bool intel_fbc_can_activate(struct intel_crtc *crtc) > >>>>> { > >>>>> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > >>>>> @@ -757,8 +765,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) > >>>>> * we didn't get any invalidate/deactivate calls, but this would require > >>>>> * a lot of tracking just for a specific case. If we conclude it's an > >>>>> * important case, we can implement it later. */ > >>>>> - if (intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) > > >>>>> - fbc->compressed_fb.size * fbc->threshold) { > >>>>> + if (intel_fbc_cfb_size_changed(dev_priv)) { > >>>>> fbc->no_fbc_reason = "CFB requirements changed"; > >>>>> return false; > >>>>> } > >>>>> @@ -1112,12 +1119,12 @@ void intel_fbc_enable(struct intel_crtc *crtc, > >>>>> mutex_lock(&fbc->lock); > >>>>> > >>>>> if (fbc->crtc) { > >>>>> - WARN_ON(fbc->crtc == crtc && !crtc_state->enable_fbc); > >>>>> - goto out; > >>>>> - } > >>>>> + if (fbc->crtc != crtc || > >>>>> + !intel_fbc_cfb_size_changed(dev_priv)) > >>>>> + goto out; > >>>>> > >>>>> - if (!crtc_state->enable_fbc) > >>>>> - goto out; > >>>>> + __intel_fbc_disable(dev_priv); > >>>>> + } > >>>>> > >>>>> WARN_ON(fbc->active); > >>>>> > >>>>> @@ -1130,6 +1137,7 @@ void intel_fbc_enable(struct intel_crtc *crtc, > >>>>> if (intel_fbc_alloc_cfb(dev_priv, > >>>>> intel_fbc_calculate_cfb_size(dev_priv, cache), > >>>>> fb->format->cpp[0])) { > >>>>> + cache->plane.visible = false; > >>>>> fbc->no_fbc_reason = "not enough stolen memory"; > >>>>> goto out; > >>>>> } > >>>> Makes sense, unfortunately kms_cursor_legacy starts failing on this series. :( > >>>> > >>>> For 1-11, 14 > >>>> > >>>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>> > >>>> We should probably get rid of the FBC disable on frontbuffer disable as well. I had some patches but nothing upstream-worthy yet. :( > >>> How would we get rid of the disable there? By triggering nukes at some > >>> predefined interval? Doesn't sound all that great. > >> Not touching FBC on frontbuffer write at all, and forcing userspace to use the dirtyfb api. I think the whole implicit tracking should be removed. > > Perhaps. Not sure userspace is ready for that though. > > We have to audit that DirtyFB is called on all gen9+ userspace, because FBC is only enabled by default on those platforms. I'll probably enable it across the board once I get it fixed. > > I know the modesetting ddx does, I believe xf86-video-intel as well. So it should be safe to do. We could hide the old behavior behind a kernel parameter for now for 1 or 2 releases, > > so we can chicken out if needed. > > > I guess the only long lasting frontbuffer invalidate is the > > one from set_domain. Everything else is bounded and so we > > know the flush is going to come in a somewhat timely manner. > > So for those cases I guess we could perhaps skip the invalidate. > > > > Hmm. Also looks like ORIGIN_GTT has been neutered and now > > we treat everyting as ORIGIN_CPU. That's maybe not so great. > > Should probably reinstate ORIGIN_GTT so we can actually benefit > > from the hw gtt tracking. Or we just try to kill that off as well. > HW tracking has been buggy for a long time, and is no longer available on current hw because of those bugs. Which bugs? We still enable it on all platforms so I don't know what you mean by it not being available. > > Also I wonder where is the flush counterpart to the invalidate > > in i915_gem_object_prepare_write()? > > > Not sure.
On Thu, Nov 28, 2019 at 04:48:04PM +0100, Maarten Lankhorst wrote: > Op 27-11-2019 om 21:12 schreef Ville Syrjala: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > The code assumes we can omit the cfb allocation once fbc > > has been enabled once. That's nonsense. Let's try to > > reallocate it if we need to. > > > > The code is still a mess, but maybe this is enough to get > > fbc going in some cases where it initially underallocates > > the cfb and there's no full modeset to fix it up. > > > > Cc: Daniel Drake <drake@endlessm.com> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Cc: Jian-Hong Pan <jian-hong@endlessm.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_fbc.c | 22 +++++++++++++++------- > > 1 file changed, 15 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > > index c976698b0729..928059a5da80 100644 > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > @@ -672,6 +672,14 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, > > cache->fence_id = -1; > > } > > > > +static bool intel_fbc_cfb_size_changed(struct drm_i915_private *dev_priv) > > +{ > > + struct intel_fbc *fbc = &dev_priv->fbc; > > + > > + return intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) > > > + fbc->compressed_fb.size * fbc->threshold; > > +} > > + > > static bool intel_fbc_can_activate(struct intel_crtc *crtc) > > { > > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > @@ -757,8 +765,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) > > * we didn't get any invalidate/deactivate calls, but this would require > > * a lot of tracking just for a specific case. If we conclude it's an > > * important case, we can implement it later. */ > > - if (intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) > > > - fbc->compressed_fb.size * fbc->threshold) { > > + if (intel_fbc_cfb_size_changed(dev_priv)) { > > fbc->no_fbc_reason = "CFB requirements changed"; > > return false; > > } > > @@ -1112,12 +1119,12 @@ void intel_fbc_enable(struct intel_crtc *crtc, > > mutex_lock(&fbc->lock); > > > > if (fbc->crtc) { > > - WARN_ON(fbc->crtc == crtc && !crtc_state->enable_fbc); > > - goto out; > > - } > > + if (fbc->crtc != crtc || > > + !intel_fbc_cfb_size_changed(dev_priv)) > > + goto out; > > > > - if (!crtc_state->enable_fbc) > > - goto out; > > + __intel_fbc_disable(dev_priv); > > + } > > > > WARN_ON(fbc->active); > > > > @@ -1130,6 +1137,7 @@ void intel_fbc_enable(struct intel_crtc *crtc, > > if (intel_fbc_alloc_cfb(dev_priv, > > intel_fbc_calculate_cfb_size(dev_priv, cache), > > fb->format->cpp[0])) { > > + cache->plane.visible = false; > > fbc->no_fbc_reason = "not enough stolen memory"; > > goto out; > > } > > Makes sense, unfortunately kms_cursor_legacy starts failing on this series. :( > > For 1-11, 14 > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Entire series pushed with Maarten's irc r-b for the rest (thanks). I also tracked down the lag I'm seeing on my laptop. It's caused by the intel ddx never issuing dirtyfb ioctls because it fails to correctly detect the capability. I'll post fixes for that shortly. I suspect it was working better before we got WC mmap because then the hw gtt tracking should have dealt with it. But with WC mmap that no longer works and we actually depend on dirtyfb to flush things.
diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c index c976698b0729..928059a5da80 100644 --- a/drivers/gpu/drm/i915/display/intel_fbc.c +++ b/drivers/gpu/drm/i915/display/intel_fbc.c @@ -672,6 +672,14 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, cache->fence_id = -1; } +static bool intel_fbc_cfb_size_changed(struct drm_i915_private *dev_priv) +{ + struct intel_fbc *fbc = &dev_priv->fbc; + + return intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) > + fbc->compressed_fb.size * fbc->threshold; +} + static bool intel_fbc_can_activate(struct intel_crtc *crtc) { struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); @@ -757,8 +765,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) * we didn't get any invalidate/deactivate calls, but this would require * a lot of tracking just for a specific case. If we conclude it's an * important case, we can implement it later. */ - if (intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) > - fbc->compressed_fb.size * fbc->threshold) { + if (intel_fbc_cfb_size_changed(dev_priv)) { fbc->no_fbc_reason = "CFB requirements changed"; return false; } @@ -1112,12 +1119,12 @@ void intel_fbc_enable(struct intel_crtc *crtc, mutex_lock(&fbc->lock); if (fbc->crtc) { - WARN_ON(fbc->crtc == crtc && !crtc_state->enable_fbc); - goto out; - } + if (fbc->crtc != crtc || + !intel_fbc_cfb_size_changed(dev_priv)) + goto out; - if (!crtc_state->enable_fbc) - goto out; + __intel_fbc_disable(dev_priv); + } WARN_ON(fbc->active); @@ -1130,6 +1137,7 @@ void intel_fbc_enable(struct intel_crtc *crtc, if (intel_fbc_alloc_cfb(dev_priv, intel_fbc_calculate_cfb_size(dev_priv, cache), fb->format->cpp[0])) { + cache->plane.visible = false; fbc->no_fbc_reason = "not enough stolen memory"; goto out; }