On Fri, Apr 03, 2020 at 11:40:04PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Replace the use of mode->private_flags with a truly private bitmaks
> in our own crtc state. We also need a copy in the crtc itself so the
> vblank code can get at it. We already have scanline_offset in there
> for a similar reason, as well as the vblank->hwmode which is assigned
> via drm_calc_timestamping_constants(). Fortunately we now have a
> nice place for doing the crtc_state->crtc copy in
> intel_crtc_update_active_timings() which gets called both for
> modesets and init/resume readout.
>
> The one slightly iffy spot is the INHERITED flag which we want to
> preserve until userspace/fb_helper does the first proper commit after
> actually calling .detecti() on the connectors. Otherwise we don't have
> the full sink capabilities (audio,infoframes,etc.) when .compute_config()
> gets called and thus we will fail to enable those features when the
> first userspace commit happens. The only internal commit we do prior to
> that should be from intel_initial_commit() and there we can simply
> preserve the INHERITED flag from the readout.
>
> CC: Sam Ravnborg <sam@xxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Emil Velikov <emil.l.velikov@xxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/icl_dsi.c | 13 ++++------
> drivers/gpu/drm/i915/display/intel_atomic.c | 1 +
> drivers/gpu/drm/i915/display/intel_display.c | 24 +++++++++++++------
> .../drm/i915/display/intel_display_types.h | 9 ++++++-
> drivers/gpu/drm/i915/display/intel_tv.c | 4 ++--
> drivers/gpu/drm/i915/display/vlv_dsi.c | 6 ++---
> drivers/gpu/drm/i915/i915_irq.c | 4 ++--
> 7 files changed, 37 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c
> index 99a25c0bb08f..4d6788ef2e5e 100644
> --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> @@ -1469,8 +1469,7 @@ static void gen11_dsi_get_config(struct intel_encoder *encoder,
> pipe_config->pipe_bpp = bdw_get_pipemisc_bpp(crtc);
>
> if (gen11_dsi_is_periodic_cmd_mode(intel_dsi))
> - pipe_config->hw.adjusted_mode.private_flags |=
> - I915_MODE_FLAG_DSI_PERIODIC_CMD_MODE;
> + pipe_config->mode_flags |= I915_MODE_FLAG_DSI_PERIODIC_CMD_MODE;
> }
>
> static int gen11_dsi_dsc_compute_config(struct intel_encoder *encoder,
> @@ -1555,10 +1554,6 @@ static int gen11_dsi_compute_config(struct intel_encoder *encoder,
>
> pipe_config->port_clock = afe_clk(encoder, pipe_config) / 5;
>
> - /* We would not operate in periodic command mode */
> - pipe_config->hw.adjusted_mode.private_flags &=
> - ~I915_MODE_FLAG_DSI_PERIODIC_CMD_MODE;
> -
Since you delete this here, but not above (and then you could also detel
gen11_dsi_is_periodic_cmd_mode I think): It's dead code, maybe prep patch
to just garbage collect I915_MODE_FLAG_DSI_PERIODIC_CMD_MODE?
I think the proper replacement is the mode flag stuff below, this is just
interim stuff that fell through the review.
With that prep patch to get rid of these 2 hunks above:
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
Also surplus Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx> on the patch
to delete I915_MODE_FLAG_DSI_PERIODIC_CMD_MODE in case I miss the new
version.
> /*
> * In case of TE GATE cmd mode, we
> * receive TE from the slave if
> @@ -1566,14 +1561,14 @@ static int gen11_dsi_compute_config(struct intel_encoder *encoder,
> */
> if (is_cmd_mode(intel_dsi)) {
> if (intel_dsi->ports == (BIT(PORT_B) | BIT(PORT_A)))
> - pipe_config->hw.adjusted_mode.private_flags |=
> + pipe_config->mode_flags |=
> I915_MODE_FLAG_DSI_USE_TE1 |
> I915_MODE_FLAG_DSI_USE_TE0;
> else if (intel_dsi->ports == BIT(PORT_B))
> - pipe_config->hw.adjusted_mode.private_flags |=
> + pipe_config->mode_flags |=
> I915_MODE_FLAG_DSI_USE_TE1;
> else
> - pipe_config->hw.adjusted_mode.private_flags |=
> + pipe_config->mode_flags |=
> I915_MODE_FLAG_DSI_USE_TE0;
> }
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> index d043057d2fa0..5863e339a426 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -252,6 +252,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
> crtc_state->wm.need_postvbl_update = false;
> crtc_state->fb_bits = 0;
> crtc_state->update_planes = 0;
> + crtc_state->mode_flags &= ~I915_MODE_FLAG_INHERITED;
>
> return &crtc_state->uapi;
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index bcb5d754f20d..d88cade45c35 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -6414,7 +6414,7 @@ static bool hsw_post_update_enable_ips(const struct intel_crtc_state *old_crtc_s
> * forcibly enable IPS on the first fastset.
> */
> if (new_crtc_state->update_pipe &&
> - old_crtc_state->hw.adjusted_mode.private_flags & I915_MODE_FLAG_INHERITED)
> + old_crtc_state->mode_flags & I915_MODE_FLAG_INHERITED)
> return true;
>
> return !old_crtc_state->ips_enabled;
> @@ -13516,8 +13516,8 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
> bool ret = true;
> u32 bp_gamma = 0;
> bool fixup_inherited = fastset &&
> - (current_config->hw.mode.private_flags & I915_MODE_FLAG_INHERITED) &&
> - !(pipe_config->hw.mode.private_flags & I915_MODE_FLAG_INHERITED);
> + (current_config->mode_flags & I915_MODE_FLAG_INHERITED) &&
> + !(pipe_config->mode_flags & I915_MODE_FLAG_INHERITED);
>
> if (fixup_inherited && !fastboot_enabled(dev_priv)) {
> drm_dbg_kms(&dev_priv->drm,
> @@ -14307,6 +14307,8 @@ intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state)
>
> drm_calc_timestamping_constants(&crtc->base, adjusted_mode);
>
> + crtc->mode_flags = crtc_state->mode_flags;
> +
> /*
> * The scanline counter increments at the leading edge of hsync.
> *
> @@ -14668,8 +14670,7 @@ static int intel_atomic_check(struct drm_device *dev,
> /* Catch I915_MODE_FLAG_INHERITED */
> for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> new_crtc_state, i) {
> - if (new_crtc_state->uapi.mode.private_flags !=
> - old_crtc_state->uapi.mode.private_flags)
> + if (new_crtc_state->mode_flags != old_crtc_state->mode_flags)
> new_crtc_state->uapi.mode_changed = true;
> }
>
> @@ -15015,7 +15016,7 @@ static void intel_update_crtc(struct intel_atomic_state *state,
> * of enabling them on the CRTC's first fastset.
> */
> if (new_crtc_state->update_pipe && !modeset &&
> - old_crtc_state->hw.mode.private_flags & I915_MODE_FLAG_INHERITED)
> + old_crtc_state->mode_flags & I915_MODE_FLAG_INHERITED)
> intel_crtc_arm_fifo_underrun(crtc, new_crtc_state);
> }
>
> @@ -17486,6 +17487,15 @@ static int intel_initial_commit(struct drm_device *dev)
> }
>
> if (crtc_state->hw.active) {
> + /*
> + * We've not yet detected sink capabilities
> + * (audio,infoframes,etc.) and thus we don't want to
> + * force a full state recomputation yet. We want that to
> + * happen only for the first real commit from userspace.
> + * So preserve the inherited flag for the time being.
> + */
> + crtc_state->mode_flags |= I915_MODE_FLAG_INHERITED;
> +
> ret = drm_atomic_add_affected_planes(state, &crtc->base);
> if (ret)
> goto out;
> @@ -18256,7 +18266,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> * set a flag to indicate that a full recalculation is
> * needed on the next commit.
> */
> - mode->private_flags = I915_MODE_FLAG_INHERITED;
> + crtc_state->mode_flags |= I915_MODE_FLAG_INHERITED;
>
> intel_crtc_compute_pixel_rate(crtc_state);
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 2bedd626c686..26df856f8b72 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -641,7 +641,7 @@ struct intel_crtc_scaler_state {
> int scaler_id;
> };
>
> -/* drm_mode->private_flags */
> +/* {crtc,crtc_state}->mode_flags */
> #define I915_MODE_FLAG_INHERITED (1<<0)
> /* Flag to get scanline using frame time stamps */
> #define I915_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP (1<<1)
> @@ -952,6 +952,9 @@ struct intel_crtc_state {
> /* Used by SDVO (and if we ever fix it, HDMI). */
> unsigned pixel_multiplier;
>
> + /* I915_MODE_FLAG_* */
> + u8 mode_flags;
> +
> u8 lane_count;
>
> /*
> @@ -1115,6 +1118,10 @@ struct intel_crtc {
> */
> bool active;
> u8 plane_ids_mask;
> +
> + /* I915_MODE_FLAG_* */
> + u8 mode_flags;
> +
> unsigned long long enabled_power_domains;
> struct intel_overlay *overlay;
>
> diff --git a/drivers/gpu/drm/i915/display/intel_tv.c b/drivers/gpu/drm/i915/display/intel_tv.c
> index abc67207f2f3..777032d9697b 100644
> --- a/drivers/gpu/drm/i915/display/intel_tv.c
> +++ b/drivers/gpu/drm/i915/display/intel_tv.c
> @@ -1158,7 +1158,7 @@ intel_tv_get_config(struct intel_encoder *encoder,
>
> /* pixel counter doesn't work on i965gm TV output */
> if (IS_I965GM(dev_priv))
> - adjusted_mode->private_flags |=
> + pipe_config->mode_flags |=
> I915_MODE_FLAG_USE_SCANLINE_COUNTER;
> }
>
> @@ -1328,7 +1328,7 @@ intel_tv_compute_config(struct intel_encoder *encoder,
>
> /* pixel counter doesn't work on i965gm TV output */
> if (IS_I965GM(dev_priv))
> - adjusted_mode->private_flags |=
> + pipe_config->mode_flags |=
> I915_MODE_FLAG_USE_SCANLINE_COUNTER;
>
> return 0;
> diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c b/drivers/gpu/drm/i915/display/vlv_dsi.c
> index 4e18d4627065..d8b1c12cb21c 100644
> --- a/drivers/gpu/drm/i915/display/vlv_dsi.c
> +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
> @@ -299,7 +299,7 @@ static int intel_dsi_compute_config(struct intel_encoder *encoder,
>
> if (IS_GEN9_LP(dev_priv)) {
> /* Enable Frame time stamp based scanline reporting */
> - adjusted_mode->private_flags |=
> + pipe_config->mode_flags |=
> I915_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP;
>
> /* Dual link goes to DSI transcoder A. */
> @@ -1098,8 +1098,8 @@ static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder,
> pipe_config->pipe_bpp = bdw_get_pipemisc_bpp(crtc);
>
> /* Enable Frame time stamo based scanline reporting */
> - adjusted_mode->private_flags |=
> - I915_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP;
> + pipe_config->mode_flags |=
> + I915_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP;
>
> /* In terms of pixels */
> adjusted_mode->crtc_hdisplay =
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 1502ab44f1a5..55ed9516bfd3 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -735,7 +735,7 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
> vblank = &crtc->base.dev->vblank[drm_crtc_index(&crtc->base)];
> mode = &vblank->hwmode;
>
> - if (mode->private_flags & I915_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP)
> + if (crtc->mode_flags & I915_MODE_FLAG_GET_SCANLINE_FROM_TIMESTAMP)
> return __intel_get_crtc_scanline_from_timestamp(crtc);
>
> vtotal = mode->crtc_vtotal;
> @@ -794,7 +794,7 @@ static bool i915_get_crtc_scanoutpos(struct drm_crtc *_crtc,
> unsigned long irqflags;
> bool use_scanline_counter = INTEL_GEN(dev_priv) >= 5 ||
> IS_G4X(dev_priv) || IS_GEN(dev_priv, 2) ||
> - mode->private_flags & I915_MODE_FLAG_USE_SCANLINE_COUNTER;
> + crtc->mode_flags & I915_MODE_FLAG_USE_SCANLINE_COUNTER;
>
> if (drm_WARN_ON(&dev_priv->drm, !mode->crtc_clock)) {
> drm_dbg(&dev_priv->drm,
> --
> 2.24.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
