* [PATCH 1/2] drm/i915: Calculate IPS linetime watermark based on future cdclk
@ 2016-04-29 14:31 ville.syrjala
2016-04-29 14:31 ` [PATCH 2/2] drm/i915: Add a FIXME about crtc !active vs. watermarks ville.syrjala
2016-04-29 16:24 ` ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Calculate IPS linetime watermark based on future cdclk Patchwork
0 siblings, 2 replies; 6+ messages in thread
From: ville.syrjala @ 2016-04-29 14:31 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/2] drm/i915: Add a FIXME about crtc !active vs. watermarks
2016-04-29 14:31 [PATCH 1/2] drm/i915: Calculate IPS linetime watermark based on future cdclk ville.syrjala
@ 2016-04-29 14:31 ` ville.syrjala
2016-05-02 9:13 ` Daniel Vetter
2016-05-04 12:30 ` Maarten Lankhorst
2016-04-29 16:24 ` ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Calculate IPS linetime watermark based on future cdclk Patchwork
1 sibling, 2 replies; 6+ messages in thread
From: ville.syrjala @ 2016-04-29 14:31 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/i915: Add a FIXME about crtc !active vs. watermarks
2016-04-29 14:31 ` [PATCH 2/2] drm/i915: Add a FIXME about crtc !active vs. watermarks ville.syrjala
@ 2016-05-02 9:13 ` Daniel Vetter
2016-05-04 12:30 ` Maarten Lankhorst
1 sibling, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2016-05-02 9:13 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/i915: Add a FIXME about crtc !active vs. watermarks
2016-04-29 14:31 ` [PATCH 2/2] drm/i915: Add a FIXME about crtc !active vs. watermarks ville.syrjala
2016-05-02 9:13 ` Daniel Vetter
@ 2016-05-04 12:30 ` Maarten Lankhorst
2016-05-09 16:27 ` Ville Syrjälä
1 sibling, 1 reply; 6+ messages in thread
From: Maarten Lankhorst @ 2016-05-04 12:30 UTC (permalink / raw)
To: ville.syrjala, intel-gfx
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/i915: Add a FIXME about crtc !active vs. watermarks
2016-05-04 12:30 ` Maarten Lankhorst
@ 2016-05-09 16:27 ` Ville Syrjälä
0 siblings, 0 replies; 6+ messages in thread
From: Ville Syrjälä @ 2016-05-09 16:27 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Calculate IPS linetime watermark based on future cdclk
2016-04-29 14:31 [PATCH 1/2] drm/i915: Calculate IPS linetime watermark based on future cdclk ville.syrjala
2016-04-29 14:31 ` [PATCH 2/2] drm/i915: Add a FIXME about crtc !active vs. watermarks ville.syrjala
@ 2016-04-29 16:24 ` Patchwork
1 sibling, 0 replies; 6+ messages in thread
From: Patchwork @ 2016-04-29 16:24 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx
== 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-05-09 16:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-04-29 14:31 [PATCH 1/2] drm/i915: Calculate IPS linetime watermark based on future cdclk ville.syrjala
2016-04-29 14:31 ` [PATCH 2/2] drm/i915: Add a FIXME about crtc !active vs. watermarks ville.syrjala
2016-05-02 9:13 ` Daniel Vetter
2016-05-04 12:30 ` Maarten Lankhorst
2016-05-09 16:27 ` Ville Syrjälä
2016-04-29 16:24 ` ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Calculate IPS linetime watermark based on future cdclk Patchwork
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox