[PATCH 1/2] drm/i915: Calculate IPS linetime watermark based on future cdclk

[PATCH 1/2] drm/i915: Calculate IPS linetime watermark based on future cdclk

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Use the cdclk we're going to be using when the pipe gets enabled to
compute the IPS linetime watermark. The current cdclk frequency is
irrelevant at this point since it can still change.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2422ac38ce5d..964b4eff3087 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2012,10 +2012,10 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
 }
 
 static uint32_t
-hsw_compute_linetime_wm(struct drm_device *dev,
-			struct intel_crtc_state *cstate)
+hsw_compute_linetime_wm(const struct intel_crtc_state *cstate)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	const struct intel_atomic_state *intel_state =
+		to_intel_atomic_state(cstate->base.state);
 	const struct drm_display_mode *adjusted_mode =
 		&cstate->base.adjusted_mode;
 	u32 linetime, ips_linetime;
@@ -2024,7 +2024,7 @@ hsw_compute_linetime_wm(struct drm_device *dev,
 		return 0;
 	if (WARN_ON(adjusted_mode->crtc_clock == 0))
 		return 0;
-	if (WARN_ON(dev_priv->cdclk_freq == 0))
+	if (WARN_ON(intel_state->cdclk == 0))
 		return 0;
 
 	/* The WM are computed with base on how long it takes to fill a single
@@ -2033,7 +2033,7 @@ hsw_compute_linetime_wm(struct drm_device *dev,
 	linetime = DIV_ROUND_CLOSEST(adjusted_mode->crtc_htotal * 1000 * 8,
 				     adjusted_mode->crtc_clock);
 	ips_linetime = DIV_ROUND_CLOSEST(adjusted_mode->crtc_htotal * 1000 * 8,
-					 dev_priv->cdclk_freq);
+					 intel_state->cdclk);
 
 	return PIPE_WM_LINETIME_IPS_LINETIME(ips_linetime) |
 	       PIPE_WM_LINETIME_TIME(linetime);
@@ -2352,7 +2352,7 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
 	pipe_wm->wm[0] = pipe_wm->raw_wm[0];
 
 	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
-		pipe_wm->linetime = hsw_compute_linetime_wm(dev, cstate);
+		pipe_wm->linetime = hsw_compute_linetime_wm(cstate);
 
 	if (!ilk_validate_pipe_wm(dev, pipe_wm))
 		return -EINVAL;
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: Add a FIXME about crtc !active vs. watermarks

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When the crtc is enabled but !active, we should still compute the
watermarks as if the planes were visible. That would make it more
likely that the we can later transition to active without errors.

Add a FIXME to remind people that we're doing the wrong thing now.
We should perhaps just move the wm computation for each individual plane
into the .check_plane hook, and later we'd just combine the results from
all active planes.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5ffccf6e7f36..00059d18b0f0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11833,6 +11833,11 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	 * Visibility is calculated as if the crtc was on, but
 	 * after scaler setup everything depends on it being off
 	 * when the crtc isn't active.
+	 *
+	 * FIXME this is wrong for watermarks. Watermarks should also
+	 * be computed as if the pipe would be active. Perhaps move
+	 * per-plane wm computation to the .check_plane() hook, and
+	 * only combine the results from all planes in the current place?
 	 */
 	if (!is_crtc_enabled)
 		to_intel_plane_state(plane_state)->visible = visible = false;
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Calculate IPS linetime watermark based on future cdclk

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Calculate IPS linetime watermark based on future cdclk
URL   : https://patchwork.freedesktop.org/series/6544/
State : success

== Summary ==

Series 6544v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/6544/revisions/1/mbox/


bdw-nuci7-2      total:201  pass:189  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:201  pass:176  dwarn:0   dfail:0   fail:0   skip:25 
bsw-nuc-2        total:200  pass:159  dwarn:0   dfail:0   fail:0   skip:41 
byt-nuc          total:200  pass:159  dwarn:0   dfail:0   fail:0   skip:41 
hsw-brixbox      total:201  pass:175  dwarn:0   dfail:0   fail:0   skip:26 
hsw-gt2          total:201  pass:179  dwarn:0   dfail:0   fail:1   skip:21 
ivb-t430s        total:201  pass:170  dwarn:0   dfail:0   fail:0   skip:31 
skl-i7k-2        total:201  pass:174  dwarn:0   dfail:0   fail:0   skip:27 
skl-nuci5        total:201  pass:190  dwarn:0   dfail:0   fail:0   skip:11 
snb-x220t        total:201  pass:159  dwarn:0   dfail:0   fail:1   skip:41 
snb-dellxps failed to collect. IGT log at Patchwork_2116/snb-dellxps/igt.log

Results at /archive/results/CI_IGT_test/Patchwork_2116/

e88c818eb230a7b07d71843f5eaca1786544c709 drm-intel-nightly: 2016y-04m-29d-13h-05m-14s UTC integration manifest
f89860a drm/i915: Add a FIXME about crtc !active vs. watermarks
2555e64 drm/i915: Calculate IPS linetime watermark based on future cdclk

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Add a FIXME about crtc !active vs. watermarks

[Daniel Vetter]

On Fri, Apr 29, 2016 at 05:31:18PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> When the crtc is enabled but !active, we should still compute the
> watermarks as if the planes were visible. That would make it more
> likely that the we can later transition to active without errors.
> 
> Add a FIXME to remind people that we're doing the wrong thing now.
> We should perhaps just move the wm computation for each individual plane
> into the .check_plane hook, and later we'd just combine the results from
> all active planes.
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

In general atomic check code shouldn't ever look at crtc->active, except
in special cases (like when checking whether a transition would work). We
still have a bit a mess in that regard.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5ffccf6e7f36..00059d18b0f0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11833,6 +11833,11 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  	 * Visibility is calculated as if the crtc was on, but
>  	 * after scaler setup everything depends on it being off
>  	 * when the crtc isn't active.
> +	 *
> +	 * FIXME this is wrong for watermarks. Watermarks should also
> +	 * be computed as if the pipe would be active. Perhaps move
> +	 * per-plane wm computation to the .check_plane() hook, and
> +	 * only combine the results from all planes in the current place?
>  	 */
>  	if (!is_crtc_enabled)
>  		to_intel_plane_state(plane_state)->visible = visible = false;
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Add a FIXME about crtc !active vs. watermarks

[Maarten Lankhorst]

Op 29-04-16 om 16:31 schreef ville.syrjala@linux.intel.com:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> When the crtc is enabled but !active, we should still compute the
> watermarks as if the planes were visible. That would make it more
> likely that the we can later transition to active without errors.
>
> Add a FIXME to remind people that we're doing the wrong thing now.
> We should perhaps just move the wm computation for each individual plane
> into the .check_plane hook, and later we'd just combine the results from
> all active planes.
For both patches:

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Not sure how much use there is to calculate watermarks for disabled planes, and then ignore them,
but definitely something to keep in mind.

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Add a FIXME about crtc !active vs. watermarks

[Ville Syrjälä]

On Wed, May 04, 2016 at 02:30:28PM +0200, Maarten Lankhorst wrote:
> Op 29-04-16 om 16:31 schreef ville.syrjala@linux.intel.com:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > When the crtc is enabled but !active, we should still compute the
> > watermarks as if the planes were visible. That would make it more
> > likely that the we can later transition to active without errors.
> >
> > Add a FIXME to remind people that we're doing the wrong thing now.
> > We should perhaps just move the wm computation for each individual plane
> > into the .check_plane hook, and later we'd just combine the results from
> > all active planes.
> For both patches:
> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> Not sure how much use there is to calculate watermarks for disabled planes, and then ignore them,
> but definitely something to keep in mind.

I guess you didn't read what I wrote? ;)

Series pushed to dinq. Thanks for the reviews.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx