diff mbox series

[2/9] drm/i915: Reinstate an early latency==0 check for skl+

Message ID 20181221171436.8218-3-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series skl+ watermark stuff | expand

Commit Message

Ville Syrjälä Dec. 21, 2018, 5:14 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

I thought we could remove all the early latency==0 checks
and rely on skl_wm_method{1,2}() checking for it. But
skl_compute_plane_wm() applies a bunch of workarounds to bump
up the latency before calling those guys so clearly it won't
end up doing the right thing. Also not sure if the calculations
based on the method1/2 results are safe agaisnt overflows so
it might not work all that well in any case. Let's put the
early check back.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Stanislav Lisovskiy Jan. 28, 2019, 8:34 a.m. UTC | #1 
On Fri, 2018-12-21 at 19:14 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I thought we could remove all the early latency==0 checks
> and rely on skl_wm_method{1,2}() checking for it. But
> skl_compute_plane_wm() applies a bunch of workarounds to bump
> up the latency before calling those guys so clearly it won't
> end up doing the right thing. Also not sure if the calculations
> based on the method1/2 results are safe agaisnt overflows so
> it might not work all that well in any case. Let's put the
> early check back.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index d132ef10fa60..0aac7e7b660f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4701,6 +4701,9 @@ static void skl_compute_plane_wm(const struct
> intel_crtc_state *cstate,
>  		to_intel_atomic_state(cstate->base.state);
>  	bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
>  
> +	if (latency == 0)
> +		return;
> +
>  	/* Display WA #1141: kbl,cfl */
>  	if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv) ||
>  	    IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_B0)) &&

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Matt Roper Jan. 29, 2019, 11:54 p.m. UTC | #2 
On Fri, Dec 21, 2018 at 07:14:29PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I thought we could remove all the early latency==0 checks
> and rely on skl_wm_method{1,2}() checking for it. But
> skl_compute_plane_wm() applies a bunch of workarounds to bump
> up the latency before calling those guys so clearly it won't
> end up doing the right thing. Also not sure if the calculations
> based on the method1/2 results are safe agaisnt overflows so
> it might not work all that well in any case. Let's put the
> early check back.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Should we remove the tests from skl_wm_method{1,2}() now?  I suppose
someone could still use the debugfs interface to set a latency value of
exactly (UINT32_MAX - workaround amount) to make latency wrap around and
hit 0, but I'm not sure if that's really any worse than if they shoot
themselves in the foot by setting a too-low non-zero latency.  I don't
think we divide by latency anywhere.

Either way,

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


> ---
>  drivers/gpu/drm/i915/intel_pm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d132ef10fa60..0aac7e7b660f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4701,6 +4701,9 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *cstate,
>  		to_intel_atomic_state(cstate->base.state);
>  	bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
>  
> +	if (latency == 0)
> +		return;
> +
>  	/* Display WA #1141: kbl,cfl */
>  	if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv) ||
>  	    IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_B0)) &&
> -- 
> 2.19.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Jan. 30, 2019, 2:25 p.m. UTC | #3 
On Tue, Jan 29, 2019 at 03:54:30PM -0800, Matt Roper wrote:
> On Fri, Dec 21, 2018 at 07:14:29PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > I thought we could remove all the early latency==0 checks
> > and rely on skl_wm_method{1,2}() checking for it. But
> > skl_compute_plane_wm() applies a bunch of workarounds to bump
> > up the latency before calling those guys so clearly it won't
> > end up doing the right thing. Also not sure if the calculations
> > based on the method1/2 results are safe agaisnt overflows so
> > it might not work all that well in any case. Let's put the
> > early check back.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Should we remove the tests from skl_wm_method{1,2}() now?  I suppose
> someone could still use the debugfs interface to set a latency value of
> exactly (UINT32_MAX - workaround amount) to make latency wrap around and
> hit 0, but I'm not sure if that's really any worse than if they shoot
> themselves in the foot by setting a too-low non-zero latency.  I don't
> think we divide by latency anywhere.

I'd probably prefer to fix the overflows somehow so that the code
could work the same way as the pre-skl code (ie. just check for 0
latency in the method1/2 functions). But that would require some
actual thinking.

> 
> Either way,
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

Thanks for the reviews. Series pushed to dinq.

> 
> 
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index d132ef10fa60..0aac7e7b660f 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4701,6 +4701,9 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *cstate,
> >  		to_intel_atomic_state(cstate->base.state);
> >  	bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
> >  
> > +	if (latency == 0)
> > +		return;
> > +
> >  	/* Display WA #1141: kbl,cfl */
> >  	if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv) ||
> >  	    IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_B0)) &&
> > -- 
> > 2.19.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d132ef10fa60..0aac7e7b660f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4701,6 +4701,9 @@  static void skl_compute_plane_wm(const struct intel_crtc_state *cstate,
 		to_intel_atomic_state(cstate->base.state);
 	bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
 
+	if (latency == 0)
+		return;
+
 	/* Display WA #1141: kbl,cfl */
 	if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv) ||
 	    IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_B0)) &&