On 07/02/2020 20:18, 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 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. This is easy since we have
> access to the whole atomic state when updating the CRTC configuration.
>
While implementing this fix it caught me by surprise that
crtc->state->state (pointer up to full atomic state) is NULL when
crtc_enable() or -flush() is called. So I take the plane-state directly
from the plane->state and just assume that it is pointing to the same
atomic state with the crtc state I am having. I that alraight?
Why is the crtc->state->state NULL? Is it a bug or is there some reason
to it?
Best regards,
Jyri
> Signed-off-by: Jyri Sarha <jsarha@xxxxxx>
> ---
> drivers/gpu/drm/tidss/tidss_crtc.c | 2 +-
> drivers/gpu/drm/tidss/tidss_dispc.c | 66 ++++++++++++++++++++---------
> 2 files changed, 47 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
> index 032c31ee2820..367efdebe2f8 100644
> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
> @@ -143,7 +143,7 @@ static void tidss_crtc_atomic_flush(struct drm_crtc *crtc,
> if (WARN_ON(!crtc->state->event))
> return;
>
> - /* Write vp properties to HW if needed. */
> + /* Write vp properties and plane positions to HW if needed. */
> dispc_vp_setup(tidss->dispc, tcrtc->hw_videoport, crtc->state, false);
>
> WARN_ON(drm_crtc_vblank_get(crtc) != 0);
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index eeb160dc047b..cfc230d2a88a 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -20,6 +20,7 @@
> #include <linux/pm_runtime.h>
> #include <linux/regmap.h>
>
> +#include <drm/drm_atomic.h>
> #include <drm/drm_fourcc.h>
> #include <drm/drm_fb_cma_helper.h>
> #include <drm/drm_gem_cma_helper.h>
> @@ -281,11 +282,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 +303,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;
>
> @@ -1301,13 +1295,54 @@ 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)
> +static 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);
> }
>
> +static void dispc_vp_position_planes(struct dispc_device *dispc,
> + u32 hw_videoport,
> + const struct drm_crtc_state *cstate,
> + bool newmodeset)
> +{
> + struct drm_device *ddev = &dispc->tidss->ddev;
> + int zpos;
> +
> + if (!cstate->zpos_changed && !cstate->planes_changed && !newmodeset)
> + return;
> +
> + for (zpos = 0; zpos < dispc->feat->num_planes; zpos++) {
> + struct drm_plane *plane;
> + bool zpos_taken = false;
> +
> + drm_for_each_plane_mask(plane, ddev, cstate->plane_mask) {
> + if (WARN_ON(!plane->state))
> + continue;
> +
> + if (plane->state->normalized_zpos == zpos) {
> + zpos_taken = true;
> + break;
> + }
> + }
> +
> + if (zpos_taken) {
> + struct tidss_plane *tplane = to_tidss_plane(plane);
> + const struct drm_plane_state *pstate = plane->state;
> +
> + dispc_ovr_set_plane(dispc, tplane->hw_plane_id,
> + hw_videoport,
> + pstate->crtc_x, pstate->crtc_y,
> + zpos);
> + }
> + dispc_ovr_enable_layer(dispc, hw_videoport, zpos, zpos_taken);
> + }
> +}
> +
> /* CSC */
> enum csc_ctm {
> CSC_RR, CSC_RG, CSC_RB,
> @@ -2070,21 +2105,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;
> @@ -2566,6 +2591,7 @@ void dispc_vp_setup(struct dispc_device *dispc, u32 hw_videoport,
> {
> dispc_vp_set_default_color(dispc, hw_videoport, 0);
> dispc_vp_set_color_mgmt(dispc, hw_videoport, state, newmodeset);
> + dispc_vp_position_planes(dispc, hw_videoport, state, newmodeset);
> }
>
> int dispc_runtime_suspend(struct dispc_device *dispc)
>
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
