[PATCH] drm/tidss: dispc: Rewrite naive plane positioning code

[Jyri Sarha]

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 at ti.com>
> ---
>  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