On Fri, Sep 21, 2018 at 07:39:40PM +0200, Maarten Lankhorst wrote:
> To make NV12 working on icl, we need to update 2 planes simultaneously.
> I've chosen to do this in the CRTC step after plane validation is done,
> so we know what planes are (in)visible. The linked Y plane will get
> updated in intel_plane_update_planes_on_crtc(), by the call to
> update_slave, which gets the master's plane_state as argument.
>
> The link requires both planes for atomic_update to work,
> so make sure skl_ddb_add_affected_planes() adds both states.
>
> Changes since v1:
> - Introduce icl_is_nv12_y_plane(), instead of hardcoding sprite numbers.
> - Put all the state updating login in intel_plane_atomic_check_with_state().
> - Clean up changes in intel_plane_atomic_check().
> Changes since v2:
> - Fix intel_atomic_get_old_plane_state() to actually return old state.
> - Move visibility changes to preparation patch.
> - Only try to find a Y plane on gen11, earlier platforms only require a single plane.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>
> fixup Y/UV Linkage
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++++++----
> drivers/gpu/drm/i915/intel_display.c | 57 ++++++++++++
> drivers/gpu/drm/i915/intel_drv.h | 53 +++++++++++
> drivers/gpu/drm/i915/intel_pm.c | 12 ++-
> 4 files changed, 210 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 984bc1f26625..522699085a59 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> crtc_state->nv12_planes &= ~BIT(intel_plane->id);
> intel_state->base.visible = false;
>
> - /* If this is a cursor plane, no further checks are needed. */
> + /* Destroy the link */
> + intel_state->linked_plane = NULL;
> + intel_state->slave = false;
> +
> + /* If this is a cursor or Y plane, no further checks are needed. */
> if (!intel_state->base.crtc && !old_plane_state->base.crtc)
> return 0;
>
> @@ -142,27 +146,76 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> state);
> }
>
> -static int intel_plane_atomic_check(struct drm_plane *plane,
> - struct drm_plane_state *new_plane_state)
> +static int intel_plane_atomic_check(struct drm_plane *drm_plane,
> + struct drm_plane_state *new_drm_plane_state)
> {
> - struct drm_atomic_state *state = new_plane_state->state;
> - const struct drm_plane_state *old_plane_state =
> - drm_atomic_get_old_plane_state(state, plane);
> - struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc;
> - const struct drm_crtc_state *old_crtc_state;
> - struct drm_crtc_state *new_crtc_state;
> -
> - new_plane_state->visible = false;
> + struct intel_atomic_state *state =
> + to_intel_atomic_state(new_drm_plane_state->state);
> + struct intel_plane *plane = to_intel_plane(drm_plane);
> + const struct intel_plane_state *old_plane_state =
> + intel_atomic_get_old_plane_state(state, plane);
> + struct intel_plane_state *new_plane_state =
> + to_intel_plane_state(new_drm_plane_state);
> + struct intel_crtc *crtc = to_intel_crtc(
> + new_plane_state->base.crtc ?:
> + old_plane_state->base.crtc);
> + const struct intel_crtc_state *old_crtc_state;
> + struct intel_crtc_state *new_crtc_state;
> + struct intel_plane *linked = old_plane_state->linked_plane;
> + int ret;
> + const struct intel_plane_state *old_linked_state;
> + struct intel_plane_state *new_linked_state = NULL;
> +
> + if (linked) {
> + /*
> + * Make sure a previously linked plane (and implicitly, the CRTC)
> + * is part of the atomic commit.
> + */
> + if (!intel_atomic_get_new_plane_state(state, linked)) {
> + new_linked_state = intel_atomic_get_plane_state(state, linked);
> + if (IS_ERR(new_linked_state))
> + return PTR_ERR(new_linked_state);
> + }
> +
> + old_linked_state =
> + intel_atomic_get_old_plane_state(state, linked);
> +
> + /*
> + * This will happen when we're the Y plane. In which case
> + * old/new_state->crtc are both NULL. We still need to perform
> + * updates on the linked plane.
> + */
> + if (!crtc)
> + crtc = to_intel_crtc(old_linked_state->base.crtc);
> +
> + WARN_ON(!crtc);
> + }
> +
> + new_plane_state->base.visible = false;
> if (!crtc)
> return 0;
>
> - old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
> - new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> + old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc);
> + new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
> +
> + if (new_linked_state &&
> + drm_plane_index(&linked->base) < drm_plane_index(&plane->base)) {
> + /*
> + * This function is called from drm_atomic_helper_check_planes(), which
> + * will normally check the newly added plane for us, but since we're
> + * already in that function, it won't check the plane if our index
> + * is bigger than the linked index because of the
> + * for_each_oldnew_plane_in_state() call.
> + */
> + new_crtc_state->base.planes_changed = true;
> + ret = intel_plane_atomic_check_with_state(old_crtc_state, new_crtc_state,
> + old_linked_state, new_linked_state);
> + if (ret)
> + return ret;
> + }
This is all rather confusing. Can't we just do a preprocessing step
before check_planes() to add the linked planes as needed, and then
let the normal check_planes() do its thing?
>
> - return intel_plane_atomic_check_with_state(to_intel_crtc_state(old_crtc_state),
> - to_intel_crtc_state(new_crtc_state),
> - to_intel_plane_state(old_plane_state),
> - to_intel_plane_state(new_plane_state));
> + return intel_plane_atomic_check_with_state(old_crtc_state, new_crtc_state,
> + old_plane_state, new_plane_state);
> }
>
> void intel_update_planes_on_crtc(struct intel_atomic_state *old_state,
> @@ -187,6 +240,25 @@ void intel_update_planes_on_crtc(struct intel_atomic_state *old_state,
> trace_intel_update_plane(&plane->base, crtc);
>
> plane->update_plane(plane, new_crtc_state, new_plane_state);
> + } else if (new_plane_state->slave) {
> + struct intel_plane *master =
> + new_plane_state->linked_plane;
> +
> + /*
> + * We update the slave plane from this function because
> + * programming it from the master plane's update_plane
> + * callback runs into issues when the Y plane is
> + * reassigned, disabled or used by a different plane.
> + *
> + * The slave plane is updated with the master plane's
> + * plane_state.
> + */
> + new_plane_state =
> + intel_atomic_get_new_plane_state(old_state, master);
> +
> + trace_intel_update_plane(&plane->base, crtc);
> +
> + plane->update_slave(plane, new_crtc_state, new_plane_state);
> } else {
> trace_intel_disable_plane(&plane->base, crtc);
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 390907d77ecd..19cd6bbb43c4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10726,6 +10726,60 @@ static bool check_single_encoder_cloning(struct drm_atomic_state *state,
> return true;
> }
>
> +static int icl_check_nv12_planes(struct drm_i915_private *dev_priv,
> + struct intel_crtc *crtc,
> + struct intel_crtc_state *crtc_state)
> +{
> + struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->base.state);
> + struct intel_plane *plane, *aux;
> +
> + if (INTEL_GEN(dev_priv) < 11 || !crtc_state->nv12_planes)
> + return 0;
> +
> + for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
> + struct intel_plane_state *plane_state, *aux_state;
> + struct drm_plane_state *drm_aux_state = NULL;
> +
> + if (!(crtc_state->nv12_planes & BIT(plane->id)))
> + continue;
> +
> + plane_state = intel_atomic_get_new_plane_state(state, plane);
> + if (!plane_state)
> + continue;
> +
> + for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, aux) {
> + if (!icl_is_nv12_y_plane(aux->id))
> + continue;
> +
> + if (crtc_state->active_planes & BIT(aux->id))
> + continue;
> +
> + drm_aux_state = drm_atomic_get_plane_state(&state->base, &aux->base);
> + if (IS_ERR(drm_aux_state))
> + return PTR_ERR(drm_aux_state);
> +
> + break;
> + }
> +
> + if (!drm_aux_state) {
> + DRM_DEBUG_KMS("Need %d free Y planes for NV12\n",
> + hweight8(crtc_state->nv12_planes));
> +
> + return -EINVAL;
> + }
> +
> + plane_state->linked_plane = aux;
> +
> + aux_state = to_intel_plane_state(drm_aux_state);
Don't really like the aux name here. Each plane can have an aux surface
so now having aux planes is getting a bit overloady. You called it slave
elsewhere so I'd stick to that everywhere.
> + aux_state->slave = true;
> + aux_state->linked_plane = plane;
> + crtc_state->active_planes |= BIT(aux->id);
> + DRM_DEBUG_KMS("Using %s as Y plane for %s\n", aux->base.name, plane->base.name);
> + }
> +
> + return 0;
> +}
> +
> static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> struct drm_crtc_state *crtc_state)
> {
> @@ -10797,6 +10851,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> if (mode_changed)
> ret = skl_update_scaler_crtc(pipe_config);
>
> + if (!ret)
> + ret = icl_check_nv12_planes(dev_priv, intel_crtc,
> + pipe_config);
> if (!ret)
> ret = skl_check_pipe_max_pixel_rate(intel_crtc,
> pipe_config);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 8073a85d7178..29c7a4bb484d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -539,6 +539,26 @@ struct intel_plane_state {
> */
> int scaler_id;
>
> + /*
> + * linked_plane:
> + *
> + * ICL planar formats require 2 planes that are updated as pairs.
> + * This member is used to make sure the other plane is also updated
> + * when required, and for update_slave() to find the correct
> + * plane_state to pass as argument.
> + */
> + struct intel_plane *linked_plane;
> +
> + /*
> + * slave:
> + * If set don't update use the linked plane's state for updating
> + * this plane during atomic commit with the update_slave() callback.
> + *
> + * It's also used by the watermark code to ignore wm calculations on
> + * this plane. They're calculated by the linked plane's wm code.
> + */
> + bool slave;
Wondering if separate *slave and *master pointers would be make things
more obvioius which one we're looking at in each piece of code.
> +
> struct drm_intel_sprite_colorkey ckey;
> };
>
> @@ -973,6 +993,9 @@ struct intel_plane {
> void (*update_plane)(struct intel_plane *plane,
> const struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state);
> + void (*update_slave)(struct intel_plane *plane,
> + const struct intel_crtc_state *crtc_state,
> + const struct intel_plane_state *plane_state);
> void (*disable_plane)(struct intel_plane *plane,
> struct intel_crtc *crtc);
> bool (*get_hw_state)(struct intel_plane *plane, enum pipe *pipe);
> @@ -1330,6 +1353,27 @@ hdmi_to_dig_port(struct intel_hdmi *intel_hdmi)
> return container_of(intel_hdmi, struct intel_digital_port, hdmi);
> }
>
> +static inline struct intel_plane_state *
> +intel_atomic_get_plane_state(struct intel_atomic_state *state,
> + struct intel_plane *plane)
> +{
> + struct drm_plane_state *ret =
> + drm_atomic_get_plane_state(&state->base, &plane->base);
> +
> + if (IS_ERR(ret))
> + return ERR_CAST(ret);
> +
> + return to_intel_plane_state(ret);
> +}
> +
> +static inline struct intel_plane_state *
> +intel_atomic_get_old_plane_state(struct intel_atomic_state *state,
> + struct intel_plane *plane)
> +{
> + return to_intel_plane_state(drm_atomic_get_old_plane_state(&state->base,
> + &plane->base));
> +}
> +
> static inline struct intel_plane_state *
> intel_atomic_get_new_plane_state(struct intel_atomic_state *state,
> struct intel_plane *plane)
> @@ -2143,6 +2187,15 @@ int skl_plane_check(struct intel_crtc_state *crtc_state,
> int intel_plane_check_src_coordinates(struct intel_plane_state *plane_state);
> int chv_plane_check_rotation(const struct intel_plane_state *plane_state);
>
> +static inline bool icl_is_nv12_y_plane(enum plane_id id)
> +{
> + /* Don't need to do a gen check, these planes are only available on gen11 */
> + if (id == PLANE_SPRITE4 || id == PLANE_SPRITE5)
> + return true;
> +
> + return false;
> +}
> +
> /* intel_tv.c */
> void intel_tv_init(struct drm_i915_private *dev_priv);
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 1db9b8328275..d76d93452137 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5137,11 +5137,12 @@ skl_ddb_add_affected_planes(struct intel_crtc_state *cstate)
> struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
> struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
> - struct drm_plane_state *plane_state;
> struct drm_plane *plane;
> enum pipe pipe = intel_crtc->pipe;
>
> drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) {
> + struct drm_plane_state *plane_state;
> + struct intel_plane *linked;
> enum plane_id plane_id = to_intel_plane(plane)->id;
>
> if (skl_ddb_entry_equal(&cur_ddb->plane[pipe][plane_id],
> @@ -5153,6 +5154,15 @@ skl_ddb_add_affected_planes(struct intel_crtc_state *cstate)
> plane_state = drm_atomic_get_plane_state(state, plane);
> if (IS_ERR(plane_state))
> return PTR_ERR(plane_state);
> +
> + /* Make sure linked plane is updated too */
> + linked = to_intel_plane_state(plane_state)->linked_plane;
> + if (!linked)
> + continue;
> +
> + plane_state = drm_atomic_get_plane_state(state, &linked->base);
> + if (IS_ERR(plane_state))
> + return PTR_ERR(plane_state);
> }
>
> return 0;
> --
> 2.18.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
