[PATCH v4] drm/tidss: dispc: Rewrite naive plane positioning code
Tomi Valkeinen
tomi.valkeinen at ti.com
Fri Feb 14 11:20:31 UTC 2020
On 13/02/2020 21:37, Jyri Sarha wrote:
> The old implementation of placing planes on the CRTC while configuring
> the planes was naive and relied on the order in which the planes were
> configured, enabled, and disabled. The situation where a plane's zpos
> was changed on the fly was completely broken. The usual symptoms of
> this problem was scrambled display and a flood of sync lost errors,
> when a plane was active in two layers at the same time, or a missing
> plane, in case when a layer was accidentally disabled.
>
> The rewrite takes a more straight forward approach when HW is
> concerned. The plane positioning registers are in the CRTC (or
> actually OVR) register space and it is more natural to configure them
> in a one go when configuring the CRTC. To do this we need make sure we
> have all the planes on the updated CRTCs in the new atomic state. The
> untouched planes on CRTCs that need plane position update are added to
> the atomic state in tidss_atomic_check().
The subject needs updating. This is a fix for a bug, and subject needs to reflect that.
> Signed-off-by: Jyri Sarha <jsarha at ti.com>
> ---
> drivers/gpu/drm/tidss/tidss_crtc.c | 55 +++++++++++++++++++++++++++++
> drivers/gpu/drm/tidss/tidss_crtc.h | 2 ++
> drivers/gpu/drm/tidss/tidss_dispc.c | 55 +++++++++++------------------
> drivers/gpu/drm/tidss/tidss_dispc.h | 5 +++
> drivers/gpu/drm/tidss/tidss_kms.c | 49 ++++++++++++++++++++++++-
> 5 files changed, 130 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
> index 032c31ee2820..631ec61b086a 100644
> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
> @@ -17,6 +17,7 @@
> #include "tidss_dispc.h"
> #include "tidss_drv.h"
> #include "tidss_irq.h"
> +#include "tidss_plane.h"
>
> /* Page flip and frame done IRQs */
>
> @@ -111,6 +112,54 @@ static int tidss_crtc_atomic_check(struct drm_crtc *crtc,
> return dispc_vp_bus_check(dispc, hw_videoport, state);
> }
>
> +/*
> + * This needs all affected planes to be present in the atomic
> + * state. The untouched planes are added to the state in
> + * tidss_atomic_check().
> + */
> +static void tidss_crtc_position_planes(struct tidss_device *tidss,
> + struct drm_crtc *crtc,
> + struct drm_crtc_state *old_state,
> + bool newmodeset)
> +{
> + struct drm_atomic_state *ostate = old_state->state;
> + struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
> + struct drm_crtc_state *cstate = crtc->state;
> + int zpos;
> +
> + if (!newmodeset && !cstate->zpos_changed &&
> + !to_tidss_crtc_state(cstate)->plane_pos_changed)
> + return;
> +
> + for (zpos = 0; zpos < tidss->feat->num_planes; zpos++) {
> + struct drm_plane_state *pstate;
> + struct drm_plane *plane;
> + bool zpos_taken = false;
> + int i;
> +
> + for_each_new_plane_in_state(ostate, plane, pstate, i) {
> + if (pstate->crtc != crtc || !pstate->visible)
> + continue;
> +
> + if (pstate->normalized_zpos == zpos) {
> + zpos_taken = true;
> + break;
> + }
> + }
> +
> + if (zpos_taken) {
> + struct tidss_plane *tplane = to_tidss_plane(plane);
> +
> + dispc_ovr_set_plane(tidss->dispc, tplane->hw_plane_id,
> + tcrtc->hw_videoport,
> + pstate->crtc_x, pstate->crtc_y,
> + zpos);
> + }
> + dispc_ovr_enable_layer(tidss->dispc, tcrtc->hw_videoport, zpos,
> + zpos_taken);
> + }
> +}
Nitpicking, but... I think the "zpos" above is really "layer". Even the params, to which you pass
"zpos", in the ovr functions are named "layer".
"zpos_taken" sounds like it's reserved and not available for us, or something like that. Maybe
"layer_active" conveys better that we're just collecting which layers are active and which are not.
> +
> static void tidss_crtc_atomic_flush(struct drm_crtc *crtc,
> struct drm_crtc_state *old_crtc_state)
> {
> @@ -146,6 +195,9 @@ static void tidss_crtc_atomic_flush(struct drm_crtc *crtc,
> /* Write vp properties to HW if needed. */
> dispc_vp_setup(tidss->dispc, tcrtc->hw_videoport, crtc->state, false);
>
> + /* Update plane positions if needed. */
> + tidss_crtc_position_planes(tidss, crtc, old_crtc_state, false);
> +
> WARN_ON(drm_crtc_vblank_get(crtc) != 0);
>
> spin_lock_irqsave(&ddev->event_lock, flags);
> @@ -183,6 +235,7 @@ static void tidss_crtc_atomic_enable(struct drm_crtc *crtc,
> return;
>
> dispc_vp_setup(tidss->dispc, tcrtc->hw_videoport, crtc->state, true);
> + tidss_crtc_position_planes(tidss, crtc, old_state, true);
>
> /* Turn vertical blanking interrupt reporting on. */
> drm_crtc_vblank_on(crtc);
> @@ -318,6 +371,8 @@ static struct drm_crtc_state *tidss_crtc_duplicate_state(struct drm_crtc *crtc)
>
> __drm_atomic_helper_crtc_duplicate_state(crtc, &state->base);
>
> + state->plane_pos_changed = false;
> +
> state->bus_format = current_state->bus_format;
> state->bus_flags = current_state->bus_flags;
>
> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.h b/drivers/gpu/drm/tidss/tidss_crtc.h
> index df9d90b1ad2d..09e773666228 100644
> --- a/drivers/gpu/drm/tidss/tidss_crtc.h
> +++ b/drivers/gpu/drm/tidss/tidss_crtc.h
> @@ -32,6 +32,8 @@ struct tidss_crtc_state {
> /* Must be first. */
> struct drm_crtc_state base;
>
> + bool plane_pos_changed;
> +
> u32 bus_format;
> u32 bus_flags;
> };
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index eeb160dc047b..e79dad246b1e 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -281,11 +281,6 @@ struct dss_vp_data {
> u32 *gamma_table;
> };
>
> -struct dss_plane_data {
> - u32 zorder;
> - u32 hw_videoport;
> -};
> -
> struct dispc_device {
> struct tidss_device *tidss;
> struct device *dev;
> @@ -307,8 +302,6 @@ struct dispc_device {
>
> struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
>
> - struct dss_plane_data plane_data[TIDSS_MAX_PLANES];
> -
> u32 *fourccs;
> u32 num_fourccs;
>
> @@ -1247,7 +1240,7 @@ int dispc_vp_set_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
> /* OVR */
> static void dispc_k2g_ovr_set_plane(struct dispc_device *dispc,
> u32 hw_plane, u32 hw_videoport,
> - u32 x, u32 y, u32 zpos)
> + u32 x, u32 y, u32 layer)
> {
> /* On k2g there is only one plane and no need for ovr */
> dispc_vid_write(dispc, hw_plane, DISPC_VID_K2G_POSITION,
> @@ -1256,44 +1249,43 @@ static void dispc_k2g_ovr_set_plane(struct dispc_device *dispc,
>
> static void dispc_am65x_ovr_set_plane(struct dispc_device *dispc,
> u32 hw_plane, u32 hw_videoport,
> - u32 x, u32 y, u32 zpos)
> + u32 x, u32 y, u32 layer)
> {
> - OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(zpos),
> + OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(layer),
> hw_plane, 4, 1);
> - OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(zpos),
> + OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(layer),
> x, 17, 6);
> - OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(zpos),
> + OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(layer),
> y, 30, 19);
> }
>
> static void dispc_j721e_ovr_set_plane(struct dispc_device *dispc,
> u32 hw_plane, u32 hw_videoport,
> - u32 x, u32 y, u32 zpos)
> + u32 x, u32 y, u32 layer)
> {
> - OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(zpos),
> + OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(layer),
> hw_plane, 4, 1);
> - OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES2(zpos),
> + OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES2(layer),
> x, 13, 0);
> - OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES2(zpos),
> + OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES2(layer),
> y, 29, 16);
> }
>
> -static void dispc_ovr_set_plane(struct dispc_device *dispc,
> - u32 hw_plane, u32 hw_videoport,
> - u32 x, u32 y, u32 zpos)
> +void dispc_ovr_set_plane(struct dispc_device *dispc, u32 hw_plane,
> + u32 hw_videoport, u32 x, u32 y, u32 layer)
> {
> switch (dispc->feat->subrev) {
> case DISPC_K2G:
> dispc_k2g_ovr_set_plane(dispc, hw_plane, hw_videoport,
> - x, y, zpos);
> + x, y, layer);
> break;
> case DISPC_AM65X:
> dispc_am65x_ovr_set_plane(dispc, hw_plane, hw_videoport,
> - x, y, zpos);
> + x, y, layer);
> break;
> case DISPC_J721E:
> dispc_j721e_ovr_set_plane(dispc, hw_plane, hw_videoport,
> - x, y, zpos);
> + x, y, layer);
> break;
> default:
> WARN_ON(1);
> @@ -1301,10 +1293,13 @@ static void dispc_ovr_set_plane(struct dispc_device *dispc,
> }
> }
>
> -static void dispc_ovr_enable_plane(struct dispc_device *dispc,
> - u32 hw_videoport, u32 zpos, bool enable)
> +void dispc_ovr_enable_layer(struct dispc_device *dispc,
> + u32 hw_videoport, u32 layer, bool enable)
> {
> - OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(zpos),
> + if (dispc->feat->subrev == DISPC_K2G)
> + return;
> +
> + OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(layer),
> !!enable, 0, 0);
> }
>
> @@ -2070,21 +2065,11 @@ int dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane,
> VID_REG_FLD_MOD(dispc, hw_plane, DISPC_VID_ATTRIBUTES, 0,
> 28, 28);
>
> - dispc_ovr_set_plane(dispc, hw_plane, hw_videoport,
> - state->crtc_x, state->crtc_y,
> - state->normalized_zpos);
> -
> - dispc->plane_data[hw_plane].zorder = state->normalized_zpos;
> - dispc->plane_data[hw_plane].hw_videoport = hw_videoport;
> -
> return 0;
> }
>
> int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable)
> {
> - dispc_ovr_enable_plane(dispc, dispc->plane_data[hw_plane].hw_videoport,
> - dispc->plane_data[hw_plane].zorder, enable);
> -
> VID_REG_FLD_MOD(dispc, hw_plane, DISPC_VID_ATTRIBUTES, !!enable, 0, 0);
>
> return 0;
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
> index e65e6a2bb821..a4a68249e44b 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
> @@ -94,6 +94,11 @@ extern const struct dispc_features dispc_j721e_feats;
> void dispc_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask);
> dispc_irq_t dispc_read_and_clear_irqstatus(struct dispc_device *dispc);
>
> +void dispc_ovr_set_plane(struct dispc_device *dispc, u32 hw_plane,
> + u32 hw_videoport, u32 x, u32 y, u32 layer);
> +void dispc_ovr_enable_layer(struct dispc_device *dispc,
> + u32 hw_videoport, u32 layer, bool enable);
> +
> void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport,
> const struct drm_crtc_state *state);
> void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
> index 5311e0f1c551..24b3f02b90c6 100644
> --- a/drivers/gpu/drm/tidss/tidss_kms.c
> +++ b/drivers/gpu/drm/tidss/tidss_kms.c
> @@ -47,9 +47,56 @@ static const struct drm_mode_config_helper_funcs mode_config_helper_funcs = {
> .atomic_commit_tail = tidss_atomic_commit_tail,
> };
>
> +static int tidss_atomic_check(struct drm_device *ddev,
> + struct drm_atomic_state *state)
> +{
> + struct drm_plane_state *opstate;
> + struct drm_plane_state *npstate;
> + struct drm_plane *plane;
> + struct drm_crtc_state *cstate;
> + struct drm_crtc *crtc;
> + int ret, i;
> +
> + ret = drm_atomic_helper_check(ddev, state);
> + if (ret)
> + return ret;
> +
> + /*
> + * If a plane on a CRTC changes add all active planes on that
Only if the plane's position (x/y/z) changes.
> + * CRTC to the atomic state. This is needed for updating the
> + * plane positions in tidss_crtc_position_planes() which is
> + * called from crtc_atomic_enable() and crtc_atomic_flush().
> + * The update is needed for x,y-position changes too, so
> + * zpos_changed condition is not enough and we need add the
> + * our own plane_pos_changed flag.
Strictly speaking, we don't need to add all active planes if only plane's x/y pos changes. In that
case it's enough to just update the OVR layer's x & y.
And when a plane is enabled or disabled, we need to update OVR. Does the code handled that? Keeping
plane's x/y/z pos the same, but enabling and disabling it. I think both zpos_changed and pos_changed
could be false in that case.
> + */
> + for_each_oldnew_plane_in_state(state, plane, opstate, npstate, i) {
> + if (npstate->crtc && npstate->visible &&
> + (!opstate->crtc ||
> + opstate->crtc_x != npstate->crtc_x ||
> + opstate->crtc_y != npstate->crtc_y)) {
The above becomes a bit easier to read if you first:
if (!npstate->crtc || !npstate->visible)
continue;
> + cstate = drm_atomic_get_crtc_state(state,
> + npstate->crtc);
> + if (IS_ERR(cstate))
> + return PTR_ERR(cstate);
> + to_tidss_crtc_state(cstate)->plane_pos_changed = true;
> + }
> + }
Add blank line here.
> + for_each_new_crtc_in_state(state, crtc, cstate, i) {
> + if (to_tidss_crtc_state(cstate)->plane_pos_changed ||
> + cstate->zpos_changed) {
> + ret = drm_atomic_add_affected_planes(state, crtc);
> + if (ret)
> + return ret;
> + }
> + }
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
More information about the dri-devel
mailing list