| Message ID | 20201119193614.25914-1-uma.shankar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/2] drm/i915/display/tgl: Disable FBC with PSR2 | expand |
On Fri, Nov 20, 2020 at 01:06:14AM +0530, Uma Shankar wrote: > There are some corner cases wrt underrun when we enable > FBC with PSR2 on TGL. Recommendation from hardware is to > keep this combination disabled. > > Bspec: 50422 HSD: 14010260002 > > v2: Added psr2 enabled check from crtc_state (Anshuman) > Added Bspec link and HSD referneces (Jose) > > v3: Moved the logic to disable fbc to intel_fbc_update_state_cache > and removed the crtc->config usages, as per Ville's recommendation. > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_fbc.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > index a5b072816a7b..cb29c6f068f9 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > @@ -701,6 +701,15 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, > struct drm_framebuffer *fb = plane_state->hw.fb; > > cache->plane.visible = plane_state->uapi.visible; > + > + /* > + * Tigerlake is not supporting FBC with PSR2. > + * Recommendation is to keep this combination disabled > + * Bspec: 50422 HSD: 14010260002 > + */ > + if (crtc_state->has_psr2 && IS_TIGERLAKE(dev_priv)) > + cache->plane.visible = false; > + > if (!cache->plane.visible) > return; > > -- > 2.26.2
On Fri, 2020-11-20 at 01:06 +0530, Uma Shankar wrote: > There are some corner cases wrt underrun when we enable > FBC with PSR2 on TGL. Recommendation from hardware is to > keep this combination disabled. > > Bspec: 50422 HSD: 14010260002 > > v2: Added psr2 enabled check from crtc_state (Anshuman) > Added Bspec link and HSD referneces (Jose) > > v3: Moved the logic to disable fbc to intel_fbc_update_state_cache > and removed the crtc->config usages, as per Ville's recommendation. > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > --- > drivers/gpu/drm/i915/display/intel_fbc.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > index a5b072816a7b..cb29c6f068f9 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > @@ -701,6 +701,15 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, > struct drm_framebuffer *fb = plane_state->hw.fb; > > > > > cache->plane.visible = plane_state->uapi.visible; > + > + /* > + * Tigerlake is not supporting FBC with PSR2. > + * Recommendation is to keep this combination disabled > + * Bspec: 50422 HSD: 14010260002 > + */ > + if (crtc_state->has_psr2 && IS_TIGERLAKE(dev_priv)) > + cache->plane.visible = false; Looks like a hack to me, would be better add a psr2 variable in intel_fbc_state_cache. We also would need have a PSR2 reason set in no_fbc_reason and handle it in IGT. > + > if (!cache->plane.visible) > return; > > > >
On Tue, Nov 24, 2020 at 10:03:35PM +0000, Souza, Jose wrote: > On Fri, 2020-11-20 at 01:06 +0530, Uma Shankar wrote: > > There are some corner cases wrt underrun when we enable > > FBC with PSR2 on TGL. Recommendation from hardware is to > > keep this combination disabled. > > > > Bspec: 50422 HSD: 14010260002 > > > > v2: Added psr2 enabled check from crtc_state (Anshuman) > > Added Bspec link and HSD referneces (Jose) > > > > v3: Moved the logic to disable fbc to intel_fbc_update_state_cache > > and removed the crtc->config usages, as per Ville's recommendation. > > > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_fbc.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > > index a5b072816a7b..cb29c6f068f9 100644 > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > @@ -701,6 +701,15 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, > > struct drm_framebuffer *fb = plane_state->hw.fb; > > > > > > > > > > cache->plane.visible = plane_state->uapi.visible; > > + > > + /* > > + * Tigerlake is not supporting FBC with PSR2. > > + * Recommendation is to keep this combination disabled > > + * Bspec: 50422 HSD: 14010260002 > > + */ > > + if (crtc_state->has_psr2 && IS_TIGERLAKE(dev_priv)) > > + cache->plane.visible = false; > > Looks like a hack to me, would be better add a psr2 variable in intel_fbc_state_cache. The plan is to remove most things from that cache anyway since it's mostly pointless stuff that should just be handled directly via the plane/crtc states. Not really convinced it makes sense to add more crap to it at this time. So IMO this is good enough for now. > We also would need have a PSR2 reason set in no_fbc_reason and handle it in IGT. I think that no_fbc_reason is rather pointless as well. Would make life a lot simpler if we didn't have to worry about it. So tempted to just nuke it. > > > + > > if (!cache->plane.visible) > > return; > > > > > > > > >
On Wed, 2020-11-25 at 18:17 +0200, Ville Syrjälä wrote: > On Tue, Nov 24, 2020 at 10:03:35PM +0000, Souza, Jose wrote: > > On Fri, 2020-11-20 at 01:06 +0530, Uma Shankar wrote: > > > There are some corner cases wrt underrun when we enable > > > FBC with PSR2 on TGL. Recommendation from hardware is to > > > keep this combination disabled. > > > > > > Bspec: 50422 HSD: 14010260002 > > > > > > v2: Added psr2 enabled check from crtc_state (Anshuman) > > > Added Bspec link and HSD referneces (Jose) > > > > > > v3: Moved the logic to disable fbc to intel_fbc_update_state_cache > > > and removed the crtc->config usages, as per Ville's recommendation. > > > > > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_fbc.c | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > > > index a5b072816a7b..cb29c6f068f9 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > > @@ -701,6 +701,15 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, > > > struct drm_framebuffer *fb = plane_state->hw.fb; > > > > > > > > > > > > > > > cache->plane.visible = plane_state->uapi.visible; > > > + > > > + /* > > > + * Tigerlake is not supporting FBC with PSR2. > > > + * Recommendation is to keep this combination disabled > > > + * Bspec: 50422 HSD: 14010260002 > > > + */ > > > + if (crtc_state->has_psr2 && IS_TIGERLAKE(dev_priv)) > > > + cache->plane.visible = false; > > > > Looks like a hack to me, would be better add a psr2 variable in intel_fbc_state_cache. > > The plan is to remove most things from that cache anyway since it's > mostly pointless stuff that should just be handled directly via > the plane/crtc states. Not really convinced it makes sense to add > more crap to it at this time. So IMO this is good enough for now. When this will happen? if soon okay. If there is no ETA IMHO is better do the right thing. > > > We also would need have a PSR2 reason set in no_fbc_reason and handle it in IGT. > > I think that no_fbc_reason is rather pointless as well. Would make > life a lot simpler if we didn't have to worry about it. So tempted > to just nuke it. > > > > > > + > > > if (!cache->plane.visible) > > > return; > > > > > > > > > > > > > > >
On Wed, Nov 25, 2020 at 05:52:10PM +0000, Souza, Jose wrote: > On Wed, 2020-11-25 at 18:17 +0200, Ville Syrjälä wrote: > > On Tue, Nov 24, 2020 at 10:03:35PM +0000, Souza, Jose wrote: > > > On Fri, 2020-11-20 at 01:06 +0530, Uma Shankar wrote: > > > > There are some corner cases wrt underrun when we enable > > > > FBC with PSR2 on TGL. Recommendation from hardware is to > > > > keep this combination disabled. > > > > > > > > Bspec: 50422 HSD: 14010260002 > > > > > > > > v2: Added psr2 enabled check from crtc_state (Anshuman) > > > > Added Bspec link and HSD referneces (Jose) > > > > > > > > v3: Moved the logic to disable fbc to intel_fbc_update_state_cache > > > > and removed the crtc->config usages, as per Ville's recommendation. > > > > > > > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_fbc.c | 9 +++++++++ > > > > 1 file changed, 9 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > index a5b072816a7b..cb29c6f068f9 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > @@ -701,6 +701,15 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, > > > > struct drm_framebuffer *fb = plane_state->hw.fb; > > > > > > > > > > > > > > > > > > > > cache->plane.visible = plane_state->uapi.visible; > > > > + > > > > + /* > > > > + * Tigerlake is not supporting FBC with PSR2. > > > > + * Recommendation is to keep this combination disabled > > > > + * Bspec: 50422 HSD: 14010260002 > > > > + */ > > > > + if (crtc_state->has_psr2 && IS_TIGERLAKE(dev_priv)) > > > > + cache->plane.visible = false; > > > > > > Looks like a hack to me, would be better add a psr2 variable in intel_fbc_state_cache. > > > > The plan is to remove most things from that cache anyway since it's > > mostly pointless stuff that should just be handled directly via > > the plane/crtc states. Not really convinced it makes sense to add > > more crap to it at this time. So IMO this is good enough for now. > > When this will happen? if soon okay. > If there is no ETA IMHO is better do the right thing. I was hoping to get back to it soon, but looks like there's quite a bit more urgent work ahead for the moment. So don't know when I'll get back to this. So I guess path of least resitance would be for Uma to respin with your suggested approach. It was one of the solutions I also suggested originally, but I did also suggest this simpler version Uma actually did.
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Friday, November 27, 2020 8:16 PM > To: Souza, Jose <jose.souza@intel.com> > Cc: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [v3 1/2] drm/i915/display/tgl: Disable FBC with PSR2 > > On Wed, Nov 25, 2020 at 05:52:10PM +0000, Souza, Jose wrote: > > On Wed, 2020-11-25 at 18:17 +0200, Ville Syrjälä wrote: > > > On Tue, Nov 24, 2020 at 10:03:35PM +0000, Souza, Jose wrote: > > > > On Fri, 2020-11-20 at 01:06 +0530, Uma Shankar wrote: > > > > > There are some corner cases wrt underrun when we enable FBC with > > > > > PSR2 on TGL. Recommendation from hardware is to keep this > > > > > combination disabled. > > > > > > > > > > Bspec: 50422 HSD: 14010260002 > > > > > > > > > > v2: Added psr2 enabled check from crtc_state (Anshuman) Added > > > > > Bspec link and HSD referneces (Jose) > > > > > > > > > > v3: Moved the logic to disable fbc to > > > > > intel_fbc_update_state_cache and removed the crtc->config usages, as > per Ville's recommendation. > > > > > > > > > > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_fbc.c | 9 +++++++++ > > > > > 1 file changed, 9 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c > > > > > b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > > index a5b072816a7b..cb29c6f068f9 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > > @@ -701,6 +701,15 @@ static void intel_fbc_update_state_cache(struct > intel_crtc *crtc, > > > > > struct drm_framebuffer *fb = plane_state->hw.fb; > > > > > > > > > > > > > > > > > > > > > > > > > cache->plane.visible = plane_state->uapi.visible; > > > > > + > > > > > + /* > > > > > + * Tigerlake is not supporting FBC with PSR2. > > > > > + * Recommendation is to keep this combination disabled > > > > > + * Bspec: 50422 HSD: 14010260002 > > > > > + */ > > > > > + if (crtc_state->has_psr2 && IS_TIGERLAKE(dev_priv)) > > > > > + cache->plane.visible = false; > > > > > > > > Looks like a hack to me, would be better add a psr2 variable in > intel_fbc_state_cache. > > > > > > The plan is to remove most things from that cache anyway since it's > > > mostly pointless stuff that should just be handled directly via the > > > plane/crtc states. Not really convinced it makes sense to add more > > > crap to it at this time. So IMO this is good enough for now. > > > > When this will happen? if soon okay. > > If there is no ETA IMHO is better do the right thing. > > I was hoping to get back to it soon, but looks like there's quite a bit more urgent > work ahead for the moment. So don't know when I'll get back to this. > > So I guess path of least resitance would be for Uma to respin with your > suggested approach. It was one of the solutions I also suggested originally, but I > did also suggest this simpler version Uma actually did. Ok, let me send out a version with psr2 variable in state cache. Please review if that looks better. Regards, Uma Shankar > -- > 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 a5b072816a7b..cb29c6f068f9 100644 --- a/drivers/gpu/drm/i915/display/intel_fbc.c +++ b/drivers/gpu/drm/i915/display/intel_fbc.c @@ -701,6 +701,15 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, struct drm_framebuffer *fb = plane_state->hw.fb; cache->plane.visible = plane_state->uapi.visible; + + /* + * Tigerlake is not supporting FBC with PSR2. + * Recommendation is to keep this combination disabled + * Bspec: 50422 HSD: 14010260002 + */ + if (crtc_state->has_psr2 && IS_TIGERLAKE(dev_priv)) + cache->plane.visible = false; + if (!cache->plane.visible) return;
There are some corner cases wrt underrun when we enable FBC with PSR2 on TGL. Recommendation from hardware is to keep this combination disabled. Bspec: 50422 HSD: 14010260002 v2: Added psr2 enabled check from crtc_state (Anshuman) Added Bspec link and HSD referneces (Jose) v3: Moved the logic to disable fbc to intel_fbc_update_state_cache and removed the crtc->config usages, as per Ville's recommendation. Signed-off-by: Uma Shankar <uma.shankar@intel.com> --- drivers/gpu/drm/i915/display/intel_fbc.c | 9 +++++++++ 1 file changed, 9 insertions(+)