| Message ID | 20200910121153.6749-1-uma.shankar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/display: Add a vblank wait for FBC activation within same frame | expand |
On Thu, 10 Sep 2020, Uma Shankar <uma.shankar@intel.com> wrote: > Add a vblank wait when fbc activation request comes for the > same frame on TGL. This helps fix underrun related to fbc. > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > --- > drivers/gpu/drm/i915/display/intel_fbc.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > index 135f5e8a4d70..3e1d715e4a4e 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > @@ -1055,6 +1055,7 @@ static void __intel_fbc_post_update(struct intel_crtc *crtc) > { > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > struct intel_fbc *fbc = &dev_priv->fbc; > + static u32 old_frame_cnt, new_frame_cnt; In the strongest terms, do not add static data. These would be shared across devices, which is certainly a bug. BR, Jani. > > drm_WARN_ON(&dev_priv->drm, !mutex_is_locked(&fbc->lock)); > > @@ -1075,10 +1076,18 @@ static void __intel_fbc_post_update(struct intel_crtc *crtc) > if (!intel_fbc_can_activate(crtc)) > return; > > - if (!fbc->busy_bits) > + old_frame_cnt = new_frame_cnt; > + new_frame_cnt = intel_crtc_get_vblank_counter(crtc); > + if (!fbc->busy_bits) { > + if (IS_TIGERLAKE(dev_priv) && > + old_frame_cnt == new_frame_cnt) { > + drm_dbg_kms(&dev_priv->drm, "Wait for vblank before Activating FBC"); > + intel_wait_for_vblank_if_active(dev_priv, crtc->pipe); > + } > intel_fbc_hw_activate(dev_priv); > - else > + } else { > intel_fbc_deactivate(dev_priv, "frontbuffer write"); > + } > } > > void intel_fbc_post_update(struct intel_atomic_state *state,
> -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: Thursday, September 10, 2020 7:04 PM > To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Add a vblank wait for FBC > activation within same frame > > On Thu, 10 Sep 2020, Uma Shankar <uma.shankar@intel.com> wrote: > > Add a vblank wait when fbc activation request comes for the same frame > > on TGL. This helps fix underrun related to fbc. > > > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_fbc.c | 13 +++++++++++-- > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c > > b/drivers/gpu/drm/i915/display/intel_fbc.c > > index 135f5e8a4d70..3e1d715e4a4e 100644 > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > @@ -1055,6 +1055,7 @@ static void __intel_fbc_post_update(struct > > intel_crtc *crtc) { > > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > struct intel_fbc *fbc = &dev_priv->fbc; > > + static u32 old_frame_cnt, new_frame_cnt; > > In the strongest terms, do not add static data. These would be shared across > devices, which is certainly a bug. Oh ok, yeah will remove this. I guess adding them to intel_fbc should be ok right ? Thanks & Regards, Uma Shankar > BR, > Jani. > > > > > drm_WARN_ON(&dev_priv->drm, !mutex_is_locked(&fbc->lock)); > > > > @@ -1075,10 +1076,18 @@ static void __intel_fbc_post_update(struct > intel_crtc *crtc) > > if (!intel_fbc_can_activate(crtc)) > > return; > > > > - if (!fbc->busy_bits) > > + old_frame_cnt = new_frame_cnt; > > + new_frame_cnt = intel_crtc_get_vblank_counter(crtc); > > + if (!fbc->busy_bits) { > > + if (IS_TIGERLAKE(dev_priv) && > > + old_frame_cnt == new_frame_cnt) { > > + drm_dbg_kms(&dev_priv->drm, "Wait for vblank before > Activating FBC"); > > + intel_wait_for_vblank_if_active(dev_priv, crtc->pipe); > > + } > > intel_fbc_hw_activate(dev_priv); > > - else > > + } else { > > intel_fbc_deactivate(dev_priv, "frontbuffer write"); > > + } > > } > > > > void intel_fbc_post_update(struct intel_atomic_state *state, > > -- > Jani Nikula, Intel Open Source Graphics Center
On Thu, Sep 10, 2020 at 01:42:23PM +0000, Shankar, Uma wrote: > > > > -----Original Message----- > > From: Jani Nikula <jani.nikula@linux.intel.com> > > Sent: Thursday, September 10, 2020 7:04 PM > > To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Add a vblank wait for FBC > > activation within same frame > > > > On Thu, 10 Sep 2020, Uma Shankar <uma.shankar@intel.com> wrote: > > > Add a vblank wait when fbc activation request comes for the same frame > > > on TGL. This helps fix underrun related to fbc. > > > > > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_fbc.c | 13 +++++++++++-- > > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c > > > b/drivers/gpu/drm/i915/display/intel_fbc.c > > > index 135f5e8a4d70..3e1d715e4a4e 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > > @@ -1055,6 +1055,7 @@ static void __intel_fbc_post_update(struct > > > intel_crtc *crtc) { > > > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > > struct intel_fbc *fbc = &dev_priv->fbc; > > > + static u32 old_frame_cnt, new_frame_cnt; > > > > In the strongest terms, do not add static data. These would be shared across > > devices, which is certainly a bug. > > Oh ok, yeah will remove this. I guess adding them to intel_fbc should be ok right ? I didn't think we found what's really going on here. So this still seems like random duct tape which just changs some timings. > > Thanks & Regards, > Uma Shankar > > > BR, > > Jani. > > > > > > > > drm_WARN_ON(&dev_priv->drm, !mutex_is_locked(&fbc->lock)); > > > > > > @@ -1075,10 +1076,18 @@ static void __intel_fbc_post_update(struct > > intel_crtc *crtc) > > > if (!intel_fbc_can_activate(crtc)) > > > return; > > > > > > - if (!fbc->busy_bits) > > > + old_frame_cnt = new_frame_cnt; > > > + new_frame_cnt = intel_crtc_get_vblank_counter(crtc); > > > + if (!fbc->busy_bits) { > > > + if (IS_TIGERLAKE(dev_priv) && > > > + old_frame_cnt == new_frame_cnt) { > > > + drm_dbg_kms(&dev_priv->drm, "Wait for vblank before > > Activating FBC"); > > > + intel_wait_for_vblank_if_active(dev_priv, crtc->pipe); > > > + } > > > intel_fbc_hw_activate(dev_priv); > > > - else > > > + } else { > > > intel_fbc_deactivate(dev_priv, "frontbuffer write"); > > > + } > > > } > > > > > > void intel_fbc_post_update(struct intel_atomic_state *state, > > > > -- > > Jani Nikula, Intel Open Source Graphics Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Thursday, September 10, 2020 7:22 PM > To: Shankar, Uma <uma.shankar@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com>; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Add a vblank wait for FBC > activation within same frame > > On Thu, Sep 10, 2020 at 01:42:23PM +0000, Shankar, Uma wrote: > > > > > > > -----Original Message----- > > > From: Jani Nikula <jani.nikula@linux.intel.com> > > > Sent: Thursday, September 10, 2020 7:04 PM > > > To: Shankar, Uma <uma.shankar@intel.com>; > > > intel-gfx@lists.freedesktop.org > > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Add a vblank wait > > > for FBC activation within same frame > > > > > > On Thu, 10 Sep 2020, Uma Shankar <uma.shankar@intel.com> wrote: > > > > Add a vblank wait when fbc activation request comes for the same > > > > frame on TGL. This helps fix underrun related to fbc. > > > > > > > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_fbc.c | 13 +++++++++++-- > > > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c > > > > b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > index 135f5e8a4d70..3e1d715e4a4e 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > @@ -1055,6 +1055,7 @@ static void __intel_fbc_post_update(struct > > > > intel_crtc *crtc) { > > > > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > > > struct intel_fbc *fbc = &dev_priv->fbc; > > > > + static u32 old_frame_cnt, new_frame_cnt; > > > > > > In the strongest terms, do not add static data. These would be > > > shared across devices, which is certainly a bug. > > > > Oh ok, yeah will remove this. I guess adding them to intel_fbc should be ok > right ? > > I didn't think we found what's really going on here. So this still seems like > random duct tape which just changs some timings. Yeah Ville, this still is a WA to avoid multiple FBC activation within frame. Adding the frame counter checks to avoid waits on all updates. More analysis still ongoing and issue is raised with hw team as well for further analysis. My idea was to get the health of this WA on CI and general feedback if this can be considered as a fallback option. Regards, Uma Shankar > > > > > > > drm_WARN_ON(&dev_priv->drm, !mutex_is_locked(&fbc->lock)); > > > > > > > > @@ -1075,10 +1076,18 @@ static void __intel_fbc_post_update(struct > > > intel_crtc *crtc) > > > > if (!intel_fbc_can_activate(crtc)) > > > > return; > > > > > > > > - if (!fbc->busy_bits) > > > > + old_frame_cnt = new_frame_cnt; > > > > + new_frame_cnt = intel_crtc_get_vblank_counter(crtc); > > > > + if (!fbc->busy_bits) { > > > > + if (IS_TIGERLAKE(dev_priv) && > > > > + old_frame_cnt == new_frame_cnt) { > > > > + drm_dbg_kms(&dev_priv->drm, "Wait for vblank before > > > Activating FBC"); > > > > + intel_wait_for_vblank_if_active(dev_priv, crtc->pipe); > > > > + } > > > > intel_fbc_hw_activate(dev_priv); > > > > - else > > > > + } else { > > > > intel_fbc_deactivate(dev_priv, "frontbuffer write"); > > > > + } > > > > } > > > > > > > > void intel_fbc_post_update(struct intel_atomic_state *state, > > > > > > -- > > > Jani Nikula, Intel Open Source Graphics Center > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c index 135f5e8a4d70..3e1d715e4a4e 100644 --- a/drivers/gpu/drm/i915/display/intel_fbc.c +++ b/drivers/gpu/drm/i915/display/intel_fbc.c @@ -1055,6 +1055,7 @@ static void __intel_fbc_post_update(struct intel_crtc *crtc) { struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); struct intel_fbc *fbc = &dev_priv->fbc; + static u32 old_frame_cnt, new_frame_cnt; drm_WARN_ON(&dev_priv->drm, !mutex_is_locked(&fbc->lock)); @@ -1075,10 +1076,18 @@ static void __intel_fbc_post_update(struct intel_crtc *crtc) if (!intel_fbc_can_activate(crtc)) return; - if (!fbc->busy_bits) + old_frame_cnt = new_frame_cnt; + new_frame_cnt = intel_crtc_get_vblank_counter(crtc); + if (!fbc->busy_bits) { + if (IS_TIGERLAKE(dev_priv) && + old_frame_cnt == new_frame_cnt) { + drm_dbg_kms(&dev_priv->drm, "Wait for vblank before Activating FBC"); + intel_wait_for_vblank_if_active(dev_priv, crtc->pipe); + } intel_fbc_hw_activate(dev_priv); - else + } else { intel_fbc_deactivate(dev_priv, "frontbuffer write"); + } } void intel_fbc_post_update(struct intel_atomic_state *state,
Add a vblank wait when fbc activation request comes for the same frame on TGL. This helps fix underrun related to fbc. Signed-off-by: Uma Shankar <uma.shankar@intel.com> --- drivers/gpu/drm/i915/display/intel_fbc.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)