On Wed, Aug 26, 2015 at 08:37:41AM -0700, Matt Roper wrote:
> On Wed, Aug 26, 2015 at 04:39:12PM +0300, Ville Syrjälä wrote:
> > On Thu, Aug 20, 2015 at 06:11:53PM -0700, Matt Roper wrote:
> > > Just pull the info out of the CRTC state structure rather than staging
> > > it in an additional structure.
> > >
> > > Signed-off-by: Matt Roper <matthew.d.ro...@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/intel_pm.c | 84
> > > ++++++++++++++---------------------------
> > > 1 file changed, 28 insertions(+), 56 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index c9cf7cf..d82ea82 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -1787,12 +1787,6 @@ struct skl_pipe_wm_parameters {
> > > struct intel_plane_wm_parameters cursor;
> > > };
> > >
> > > -struct ilk_pipe_wm_parameters {
> > > - bool active;
> > > - uint32_t pipe_htotal;
> > > - uint32_t pixel_rate;
> > > -};
> > > -
> > > struct ilk_wm_maximums {
> > > uint16_t pri;
> > > uint16_t spr;
> > > @@ -1811,7 +1805,7 @@ struct intel_wm_config {
> > > * For both WM_PIPE and WM_LP.
> > > * mem_value must be in 0.1us units.
> > > */
> > > -static uint32_t ilk_compute_pri_wm(const struct ilk_pipe_wm_parameters
> > > *params,
> > > +static uint32_t ilk_compute_pri_wm(const struct intel_crtc_state *cstate,
> > > const struct intel_plane_state *pstate,
> > > uint32_t mem_value,
> > > bool is_lp)
> > > @@ -1819,16 +1813,16 @@ static uint32_t ilk_compute_pri_wm(const struct
> > > ilk_pipe_wm_parameters *params,
> > > int bpp = pstate->base.fb ? pstate->base.fb->bits_per_pixel / 8 : 0;
> > > uint32_t method1, method2;
> > >
> > > - if (!params->active || !pstate->visible)
> > > + if (!cstate->base.active || !pstate->visible)
> > > return 0;
> >
> > Previously we lookd at crtc->active, now we look at state->base.active.
> > Since the atomic modeset code is a bit insane and doesn't have an
> > intermediate atomic state for the disable->re-enable case, I'm not sure
> > this will fly currently.
> >
> > I'd be much happier if someone added the intermediate atomic state to
> > handle pipe disabling in a nicer way. Afterwards we should be able to
> > elimiate all special cases dealing with watermarks and just do the wm
> > updates from the commit_planes (or whatever it was called).
>
> I think I'm missing some of the context for your comment. At the moment
> we update watermarks after vblank if the CRTC is being re-enabled and
> before the vblank in other cases, so it seems like if we use
> crtc->active here, then when we get to the post-vblank watermark update
> we won't have any watermark values since we'll be acting on the old data
> that says the CRTC is still off.
>
> When you talk about intermediate atomic state are you saying that you
> want to see CRTC enables take a two step process where the CRTC gets
> enabled with no planes first, then all the planes get turned on during
> the next vblank?
What I think should happen is something like this:
// whatever the user wanted
compute_final_atomic_state()
// include all crtcs in the intermediate state which are
// getting disabled (even temporarily to perform a modeset)
compute_intermediate_atomic_state()
ret = check_state_change(old, intermediate)
ret = check_state_change(intermediate, new)
// commit all planes in on go to make the pop out as atomically as
// possible
for_each_crtc_in(intermediate) {
commit_planes()
}
for_each_crtc_in(intermediate) {
disable_crtc()
}
for_each_crtc_in(new) {
if (!currently_active)
crtc_enable()
}
// commit all planes in on go to make the pop in as atomically as
// possible
for_each_crtc_in(new) {
commit_planes()
}
>
>
> Matt
>
> >
> > >
> > > - method1 = ilk_wm_method1(params->pixel_rate, bpp, mem_value);
> > > + method1 = ilk_wm_method1(ilk_pipe_pixel_rate(cstate), bpp, mem_value);
> > >
> > > if (!is_lp)
> > > return method1;
> > >
> > > - method2 = ilk_wm_method2(params->pixel_rate,
> > > - params->pipe_htotal,
> > > + method2 = ilk_wm_method2(ilk_pipe_pixel_rate(cstate),
> > > + cstate->base.adjusted_mode.crtc_htotal,
> > > drm_rect_width(&pstate->dst),
> > > bpp,
> > > mem_value);
> > > @@ -1840,19 +1834,19 @@ static uint32_t ilk_compute_pri_wm(const struct
> > > ilk_pipe_wm_parameters *params,
> > > * For both WM_PIPE and WM_LP.
> > > * mem_value must be in 0.1us units.
> > > */
> > > -static uint32_t ilk_compute_spr_wm(const struct ilk_pipe_wm_parameters
> > > *params,
> > > +static uint32_t ilk_compute_spr_wm(const struct intel_crtc_state *cstate,
> > > const struct intel_plane_state *pstate,
> > > uint32_t mem_value)
> > > {
> > > int bpp = pstate->base.fb ? pstate->base.fb->bits_per_pixel / 8 : 0;
> > > uint32_t method1, method2;
> > >
> > > - if (!params->active || !pstate->visible)
> > > + if (!cstate->base.active || !pstate->visible)
> > > return 0;
> > >
> > > - method1 = ilk_wm_method1(params->pixel_rate, bpp, mem_value);
> > > - method2 = ilk_wm_method2(params->pixel_rate,
> > > - params->pipe_htotal,
> > > + method1 = ilk_wm_method1(ilk_pipe_pixel_rate(cstate), bpp, mem_value);
> > > + method2 = ilk_wm_method2(ilk_pipe_pixel_rate(cstate),
> > > + cstate->base.adjusted_mode.crtc_htotal,
> > > drm_rect_width(&pstate->dst),
> > > bpp,
> > > mem_value);
> > > @@ -1863,30 +1857,30 @@ static uint32_t ilk_compute_spr_wm(const struct
> > > ilk_pipe_wm_parameters *params,
> > > * For both WM_PIPE and WM_LP.
> > > * mem_value must be in 0.1us units.
> > > */
> > > -static uint32_t ilk_compute_cur_wm(const struct ilk_pipe_wm_parameters
> > > *params,
> > > +static uint32_t ilk_compute_cur_wm(const struct intel_crtc_state *cstate,
> > > const struct intel_plane_state *pstate,
> > > uint32_t mem_value)
> > > {
> > > int bpp = pstate->base.fb ? pstate->base.fb->bits_per_pixel / 8 : 0;
> > >
> > > - if (!params->active || !pstate->visible)
> > > + if (!cstate->base.active || !pstate->visible)
> > > return 0;
> > >
> > > - return ilk_wm_method2(params->pixel_rate,
> > > - params->pipe_htotal,
> > > + return ilk_wm_method2(ilk_pipe_pixel_rate(cstate),
> > > + cstate->base.adjusted_mode.crtc_htotal,
> > > drm_rect_width(&pstate->dst),
> > > bpp,
> > > mem_value);
> > > }
> > >
> > > /* Only for WM_LP. */
> > > -static uint32_t ilk_compute_fbc_wm(const struct ilk_pipe_wm_parameters
> > > *params,
> > > +static uint32_t ilk_compute_fbc_wm(const struct intel_crtc_state *cstate,
> > > const struct intel_plane_state *pstate,
> > > uint32_t pri_val)
> > > {
> > > int bpp = pstate->base.fb ? pstate->base.fb->bits_per_pixel / 8 : 0;
> > >
> > > - if (!params->active || !pstate->visible)
> > > + if (!cstate->base.active || !pstate->visible)
> > > return 0;
> > >
> > > return ilk_wm_fbc(pri_val, drm_rect_width(&pstate->dst), bpp);
> > > @@ -2056,7 +2050,7 @@ static bool ilk_validate_wm_level(int level,
> > > static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
> > > const struct intel_crtc *intel_crtc,
> > > int level,
> > > - const struct ilk_pipe_wm_parameters *p,
> > > + struct intel_crtc_state *cstate,
> > > struct intel_wm_level *result)
> > > {
> > > struct intel_plane *intel_plane;
> > > @@ -2077,18 +2071,18 @@ static void ilk_compute_wm_level(const struct
> > > drm_i915_private *dev_priv,
> > >
> > > switch (intel_plane->base.type) {
> > > case DRM_PLANE_TYPE_PRIMARY:
> > > - result->pri_val = ilk_compute_pri_wm(p, pstate,
> > > + result->pri_val = ilk_compute_pri_wm(cstate, pstate,
> > > pri_latency,
> > > level);
> > > - result->fbc_val = ilk_compute_fbc_wm(p, pstate,
> > > + result->fbc_val = ilk_compute_fbc_wm(cstate, pstate,
> > > result->pri_val);
> > > break;
> > > case DRM_PLANE_TYPE_OVERLAY:
> > > - result->spr_val = ilk_compute_spr_wm(p, pstate,
> > > + result->spr_val = ilk_compute_spr_wm(cstate, pstate,
> > > spr_latency);
> > > break;
> > > case DRM_PLANE_TYPE_CURSOR:
> > > - result->cur_val = ilk_compute_cur_wm(p, pstate,
> > > + result->cur_val = ilk_compute_cur_wm(cstate, pstate,
> > > cur_latency);
> > > break;
> > > }
> > > @@ -2352,19 +2346,6 @@ static void skl_setup_wm_latency(struct drm_device
> > > *dev)
> > > intel_print_wm_latency(dev, "Gen9 Plane", dev_priv->wm.skl_latency);
> > > }
> > >
> > > -static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
> > > - struct ilk_pipe_wm_parameters *p)
> > > -{
> > > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > -
> > > - if (!intel_crtc->active)
> > > - return;
> > > -
> > > - p->active = true;
> > > - p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
> > > - p->pixel_rate = ilk_pipe_pixel_rate(intel_crtc->config);
> > > -}
> > > -
> > > static void ilk_compute_wm_config(struct drm_device *dev,
> > > struct intel_wm_config *config)
> > > {
> > > @@ -2384,10 +2365,10 @@ static void ilk_compute_wm_config(struct
> > > drm_device *dev,
> > > }
> > >
> > > /* Compute new watermarks for the pipe */
> > > -static bool intel_compute_pipe_wm(struct drm_crtc *crtc,
> > > - const struct ilk_pipe_wm_parameters *params,
> > > +static bool intel_compute_pipe_wm(struct intel_crtc_state *cstate,
> > > struct intel_pipe_wm *pipe_wm)
> > > {
> > > + struct drm_crtc *crtc = cstate->base.crtc;
> > > struct drm_device *dev = crtc->dev;
> > > const struct drm_i915_private *dev_priv = dev->dev_private;
> > > struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > @@ -2412,8 +2393,7 @@ static bool intel_compute_pipe_wm(struct drm_crtc
> > > *crtc,
> > > drm_rect_width(&sprstate->dst) !=
> > > drm_rect_width(&sprstate->src) >> 16 ||
> > > drm_rect_height(&sprstate->dst) !=
> > > drm_rect_height(&sprstate->src) >> 16;
> > >
> > > -
> > > - pipe_wm->pipe_enabled = params->active;
> > > + pipe_wm->pipe_enabled = cstate->base.active;
> > > pipe_wm->sprites_enabled = sprstate->visible;
> > > pipe_wm->sprites_scaled = config.sprites_scaled;
> > >
> > > @@ -2425,7 +2405,7 @@ static bool intel_compute_pipe_wm(struct drm_crtc
> > > *crtc,
> > > if (config.sprites_scaled)
> > > max_level = 0;
> > >
> > > - ilk_compute_wm_level(dev_priv, intel_crtc, 0, params, &pipe_wm->wm[0]);
> > > + ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate, &pipe_wm->wm[0]);
> > >
> > > if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > > pipe_wm->linetime = hsw_compute_linetime_wm(dev, crtc);
> > > @@ -2442,7 +2422,7 @@ static bool intel_compute_pipe_wm(struct drm_crtc
> > > *crtc,
> > > for (level = 1; level <= max_level; level++) {
> > > struct intel_wm_level wm = {};
> > >
> > > - ilk_compute_wm_level(dev_priv, intel_crtc, level, params, &wm);
> > > + ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate, &wm);
> > >
> > > /*
> > > * Disable any watermark level that exceeds the
> > > @@ -3748,19 +3728,17 @@ skl_update_sprite_wm(struct drm_plane *plane,
> > > struct drm_crtc *crtc,
> > > static void ilk_update_wm(struct drm_crtc *crtc)
> > > {
> > > struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > + struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> > > struct drm_device *dev = crtc->dev;
> > > struct drm_i915_private *dev_priv = dev->dev_private;
> > > struct ilk_wm_maximums max;
> > > - struct ilk_pipe_wm_parameters params = {};
> > > struct ilk_wm_values results = {};
> > > enum intel_ddb_partitioning partitioning;
> > > struct intel_pipe_wm pipe_wm = {};
> > > struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm;
> > > struct intel_wm_config config = {};
> > >
> > > - ilk_compute_wm_parameters(crtc, ¶ms);
> > > -
> > > - intel_compute_pipe_wm(crtc, ¶ms, &pipe_wm);
> > > + intel_compute_pipe_wm(cstate, &pipe_wm);
> > >
> > > if (!memcmp(&intel_crtc->wm.active, &pipe_wm, sizeof(pipe_wm)))
> > > return;
> > > @@ -3800,12 +3778,6 @@ ilk_update_sprite_wm(struct drm_plane *plane,
> > > struct drm_device *dev = plane->dev;
> > > struct intel_plane *intel_plane = to_intel_plane(plane);
> > >
> > > - intel_plane->wm.enabled = enabled;
> > > - intel_plane->wm.scaled = scaled;
> > > - intel_plane->wm.horiz_pixels = sprite_width;
> > > - intel_plane->wm.vert_pixels = sprite_width;
> > > - intel_plane->wm.bytes_per_pixel = pixel_size;
> > > -
> > > /*
> > > * IVB workaround: must disable low power watermarks for at least
> > > * one frame before enabling scaling. LP watermarks can be re-enabled
> > > --
> > > 2.1.4
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel OTC
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
