From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 07/24] drm/i915: Introduce intel_atomic_get_plane_state_after_check()
Date: Tue, 8 Oct 2019 20:03:18 +0300 [thread overview]
Message-ID: <20191008170318.GS1208@intel.com> (raw)
In-Reply-To: <20191004113514.17064-8-maarten.lankhorst@linux.intel.com>
On Fri, Oct 04, 2019 at 01:34:57PM +0200, Maarten Lankhorst wrote:
> Use this in all the places where we try to acquire planes after the planes
> atomic_check().
>
> In case of intel_modeset_all_pipes() this is not yet done after atomic_check,
> but seems like it will be in the future. To add some paranoia, add all planes
> rather than active planes, because of bigjoiner and planar YUV support having
> extra planes outside of the core's view that wouldn't be added otherwise.
If the plane isn't active what good does adding it do?
Maybe the only real exception I can think of is the watermarks
and the primary vs. gamma/csc_enable on pre-skl, but those are
already handled correctly.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_atomic.c | 41 +++++++++----------
> .../gpu/drm/i915/display/intel_atomic_plane.c | 19 +++++++++
> drivers/gpu/drm/i915/display/intel_cdclk.c | 15 ++++---
> drivers/gpu/drm/i915/display/intel_color.c | 7 ++--
> .../drm/i915/display/intel_display_types.h | 6 +++
> drivers/gpu/drm/i915/intel_pm.c | 14 ++++---
> 6 files changed, 66 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> index c5a552a69752..e6cb85d41c8d 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -313,13 +313,10 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
> struct intel_crtc *intel_crtc,
> struct intel_crtc_state *crtc_state)
> {
> - struct drm_plane *plane = NULL;
> - struct intel_plane *intel_plane;
> - struct intel_plane_state *plane_state = NULL;
> struct intel_crtc_scaler_state *scaler_state =
> &crtc_state->scaler_state;
> struct drm_atomic_state *drm_state = crtc_state->base.state;
> - struct intel_atomic_state *intel_state = to_intel_atomic_state(drm_state);
> + struct intel_atomic_state *state = to_intel_atomic_state(drm_state);
> int num_scalers_need;
> int i;
>
> @@ -346,6 +343,7 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
>
> /* walkthrough scaler_users bits and start assigning scalers */
> for (i = 0; i < sizeof(scaler_state->scaler_users) * 8; i++) {
> + struct intel_plane_state *plane_state = NULL;
> int *scaler_id;
> const char *name;
> int idx;
> @@ -361,19 +359,16 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
> /* panel fitter case: assign as a crtc scaler */
> scaler_id = &scaler_state->scaler_id;
> } else {
> - name = "PLANE";
> + struct intel_plane *plane;
>
> /* plane scaler case: assign as a plane scaler */
> /* find the plane that set the bit as scaler_user */
> - plane = drm_state->planes[i].ptr;
>
> /*
> * to enable/disable hq mode, add planes that are using scaler
> * into this transaction
> */
> - if (!plane) {
> - struct drm_plane_state *state;
> -
> + if (!drm_state->planes[i].ptr) {
> /*
> * GLK+ scalers don't have a HQ mode so it
> * isn't necessary to change between HQ and dyn mode
> @@ -382,24 +377,28 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
> if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> continue;
>
> - plane = drm_plane_from_index(&dev_priv->drm, i);
> - state = drm_atomic_get_plane_state(drm_state, plane);
> - if (IS_ERR(state)) {
> - DRM_DEBUG_KMS("Failed to add [PLANE:%d] to drm_state\n",
> - plane->base.id);
> - return PTR_ERR(state);
> + plane = to_intel_plane(drm_plane_from_index(&dev_priv->drm, i));
> + plane_state =
> + intel_atomic_get_plane_state_after_check(state,
> + crtc_state,
> + plane);
> + if (IS_ERR(plane_state)) {
> + DRM_DEBUG_KMS("Failed to add [PLANE:%d] to drm_state: %li\n",
> + plane->base.base.id, PTR_ERR(plane_state));
> + return PTR_ERR(plane_state);
> }
> + } else {
> + plane = to_intel_plane(drm_state->planes[i].ptr);
> + plane_state = intel_atomic_get_new_plane_state(state,
> + plane);
> }
>
> - intel_plane = to_intel_plane(plane);
> - idx = plane->base.id;
> -
> /* plane on different crtc cannot be a scaler user of this crtc */
> - if (WARN_ON(intel_plane->pipe != intel_crtc->pipe))
> + if (WARN_ON(plane->pipe != intel_crtc->pipe))
> continue;
>
> - plane_state = intel_atomic_get_new_plane_state(intel_state,
> - intel_plane);
> + name = "PLANE";
> + idx = plane->base.base.id;
> scaler_id = &plane_state->scaler_id;
> }
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index 98b7766eaa7a..4eaab0de98bf 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -366,6 +366,25 @@ void i9xx_update_planes_on_crtc(struct intel_atomic_state *state,
> }
> }
>
> +struct intel_plane_state *
> +intel_atomic_get_plane_state_after_check(struct intel_atomic_state *state,
> + struct intel_crtc_state *new_crtc_state,
> + struct intel_plane *plane)
> +{
> + struct intel_plane_state *plane_state =
> + intel_atomic_get_new_plane_state(state, plane);
> +
> + if (plane_state)
> + return plane_state;
> +
> + plane_state = intel_atomic_get_plane_state(state, plane);
> + if (IS_ERR(plane_state))
> + return plane_state;
> +
> + new_crtc_state->update_planes |= BIT(plane->id);
I also don't really like burying this all the way down. Seems
rather non-obvious from the caller. Also seems wrong to not set
the flag if the plane was already part of the state.
I'd say we just need to hand roll add_affected_planes() for the
cdclk code. And there is should definitely be sufficient to
add just the active planes.
> + return plane_state;
> +}
> +
> const struct drm_plane_helper_funcs intel_plane_helper_funcs = {
> .prepare_fb = intel_prepare_plane_fb,
> .cleanup_fb = intel_cleanup_plane_fb,
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 43564295b864..42bd02638d32 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -2249,6 +2249,7 @@ static int intel_modeset_all_pipes(struct intel_atomic_state *state)
> */
> for_each_intel_crtc(&dev_priv->drm, crtc) {
> struct intel_crtc_state *crtc_state;
> + struct intel_plane *plane;
> int ret;
>
> crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
> @@ -2266,12 +2267,14 @@ static int intel_modeset_all_pipes(struct intel_atomic_state *state)
> if (ret)
> return ret;
>
> - ret = drm_atomic_add_affected_planes(&state->base,
> - &crtc->base);
> - if (ret)
> - return ret;
> -
> - crtc_state->update_planes |= crtc_state->active_planes;
> + for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
> + struct intel_plane_state *plane_state =
> + intel_atomic_get_plane_state_after_check(state,
> + crtc_state,
> + plane);
> + if (IS_ERR(plane_state))
> + return PTR_ERR(plane_state);
> + }
> }
>
> return 0;
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index 9ab34902663e..1e3a623eaf82 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1077,11 +1077,12 @@ intel_color_add_affected_planes(struct intel_crtc_state *new_crtc_state)
> if (!need_plane_update(plane, new_crtc_state))
> continue;
>
> - plane_state = intel_atomic_get_plane_state(state, plane);
> + plane_state =
> + intel_atomic_get_plane_state_after_check(state,
> + new_crtc_state,
> + plane);
> if (IS_ERR(plane_state))
> return PTR_ERR(plane_state);
> -
> - new_crtc_state->update_planes |= BIT(plane->id);
> }
>
> return 0;
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 976669f01a8c..526423437f63 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1541,4 +1541,10 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state)
> return i915_ggtt_offset(state->vma);
> }
>
> +/* intel_atomic_plane.c */
> +struct intel_plane_state *
> +intel_atomic_get_plane_state_after_check(struct intel_atomic_state *state,
> + struct intel_crtc_state *crtc_state,
> + struct intel_plane *plane);
> +
> #endif /* __INTEL_DISPLAY_TYPES_H__ */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 53358e33df1b..5e6e54cb22fe 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5242,11 +5242,12 @@ skl_ddb_add_affected_planes(const struct intel_crtc_state *old_crtc_state,
> &new_crtc_state->wm.skl.plane_ddb_uv[plane_id]))
> continue;
>
> - plane_state = intel_atomic_get_plane_state(state, plane);
> + plane_state =
> + intel_atomic_get_plane_state_after_check(state,
> + new_crtc_state,
> + plane);
> if (IS_ERR(plane_state))
> return PTR_ERR(plane_state);
> -
> - new_crtc_state->update_planes |= BIT(plane_id);
> }
>
> return 0;
> @@ -5534,11 +5535,12 @@ static int skl_wm_add_affected_planes(struct intel_atomic_state *state,
> &new_crtc_state->wm.skl.optimal.planes[plane_id]))
> continue;
>
> - plane_state = intel_atomic_get_plane_state(state, plane);
> + plane_state =
> + intel_atomic_get_plane_state_after_check(state,
> + new_crtc_state,
> + plane);
> if (IS_ERR(plane_state))
> return PTR_ERR(plane_state);
> -
> - new_crtc_state->update_planes |= BIT(plane_id);
> }
>
> return 0;
> --
> 2.23.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2019-10-08 17:03 UTC|newest]
Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-04 11:34 [PATCH 00/24] Enable bigjoiner support, second approach Maarten Lankhorst
2019-10-04 11:34 ` [PATCH 01/24] HAX to make DSC work on the icelake test system Maarten Lankhorst
2019-10-04 11:34 ` [PATCH 02/24] drm/i915: Fix for_each_intel_plane_mask definition Maarten Lankhorst
2019-10-04 13:14 ` Ville Syrjälä
2019-10-07 19:37 ` Matt Roper
2019-10-04 11:34 ` [PATCH 03/24] drm/i915: Introduce and use intel_atomic_crtc_state_for_each_plane_state Maarten Lankhorst
2019-10-04 13:18 ` Ville Syrjälä
2019-10-07 19:37 ` Matt Roper
2019-10-04 11:34 ` [PATCH 04/24] drm/i915: Remove cursor use of properties for coordinates Maarten Lankhorst
2019-10-04 13:22 ` Ville Syrjälä
2019-10-07 19:37 ` Matt Roper
2019-10-10 12:10 ` Maarten Lankhorst
2019-10-10 14:04 ` Maarten Lankhorst
2019-10-04 11:34 ` [PATCH 05/24] drm/i915: Use intel_plane_state in prepare and cleanup plane_fb Maarten Lankhorst
2019-10-04 13:23 ` Ville Syrjälä
2019-10-07 19:37 ` Matt Roper
2019-10-04 11:34 ` [PATCH 06/24] drm/i915: Remove begin/finish_crtc_commit, v4 Maarten Lankhorst
2019-10-07 19:43 ` Matt Roper
2019-10-04 11:34 ` [PATCH 07/24] drm/i915: Introduce intel_atomic_get_plane_state_after_check() Maarten Lankhorst
2019-10-08 17:03 ` Ville Syrjälä [this message]
2019-10-10 11:56 ` Maarten Lankhorst
2019-10-10 12:39 ` Ville Syrjälä
2019-10-10 13:01 ` Maarten Lankhorst
2019-10-04 11:34 ` [PATCH 08/24] drm/i915: Prepare to split crtc state in uapi and hw state Maarten Lankhorst
2019-10-08 17:06 ` Ville Syrjälä
2019-10-10 14:21 ` Maarten Lankhorst
2019-10-10 14:47 ` Ville Syrjälä
2019-10-14 8:20 ` Maarten Lankhorst
2019-10-04 11:34 ` [PATCH 09/24] drm/i915: Handle a few more cases for crtc hw/uapi split Maarten Lankhorst
2019-10-04 13:31 ` Ville Syrjälä
2019-10-04 15:51 ` Maarten Lankhorst
2019-10-04 15:56 ` Ville Syrjälä
2019-10-04 11:35 ` [PATCH 10/24] drm/i915: Complete crtc hw/uapi split, v2 Maarten Lankhorst
2019-10-04 11:35 ` [PATCH 11/24] drm/i915: Preparation for plane split Maarten Lankhorst
2019-10-04 11:35 ` [PATCH 12/24] drm/i915: Split plane hw and uapi state Maarten Lankhorst
2019-10-08 17:42 ` Ville Syrjälä
2019-10-09 12:13 ` Maarten Lankhorst
2019-10-09 12:23 ` Ville Syrjälä
2019-10-09 12:31 ` Maarten Lankhorst
2019-10-09 12:41 ` Ville Syrjälä
2019-10-09 12:58 ` Maarten Lankhorst
2019-10-04 11:35 ` [PATCH 13/24] drm/i915: Stop using drm_atomic_helper_check_planes() Maarten Lankhorst
2019-10-04 11:35 ` [PATCH 14/24] drm/i915/dp: Allow big joiner modes in intel_dp_mode_valid(), v2 Maarten Lankhorst
2019-10-08 17:50 ` Ville Syrjälä
2019-10-04 11:35 ` [PATCH 15/24] drm/i915: Try to make bigjoiner work in atomic check, v2 Maarten Lankhorst
2019-10-08 19:40 ` Ville Syrjälä
2019-10-10 12:42 ` Maarten Lankhorst
2019-10-04 11:35 ` [PATCH 16/24] drm/i915: Enable big joiner support in enable and disable sequences Maarten Lankhorst
2019-10-04 11:35 ` [PATCH 17/24] drm/i915: Make hardware readout work on i915 Maarten Lankhorst
2019-10-04 11:35 ` [PATCH 18/24] drm/i915: Remove special case slave handling during hw programming Maarten Lankhorst
2019-10-04 11:35 ` [PATCH 19/24] drm/i915: Link planes in a bigjoiner configuration, v2 Maarten Lankhorst
2019-10-04 11:35 ` [PATCH 20/24] drm/i915: Add bigjoiner aware plane clipping checks Maarten Lankhorst
2019-10-04 11:35 ` [PATCH 21/24] drm/i915: Ensure color blobs are copied to slave before planes are checked Maarten Lankhorst
2019-10-04 11:35 ` [PATCH 22/24] drm/i915: Add intel_update_bigjoiner handling Maarten Lankhorst
2019-10-04 11:35 ` [PATCH 23/24] drm/i915: Add debugfs dumping for bigjoiner, v2 Maarten Lankhorst
2019-10-04 11:35 ` [PATCH 24/24] semi-hax: drm/i915: Always verify ddb allocation Maarten Lankhorst
2019-10-04 14:23 ` [PATCH] " Maarten Lankhorst
2019-10-04 13:10 ` ✗ Fi.CI.BUILD: failure for Enable bigjoiner support, second approach Patchwork
2019-10-04 18:03 ` ✗ Fi.CI.BUILD: failure for Enable bigjoiner support, second approach. (rev2) Patchwork
2019-10-10 16:25 ` ✗ Fi.CI.BUILD: failure for Enable bigjoiner support, second approach. (rev3) Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191008170318.GS1208@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=maarten.lankhorst@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).