[Intel-gfx] [PATCH 2/3] drm/i915/dp: Make port sync mode assignments only if all tiles present

[Matt Roper]

On Wed, Dec 11, 2019 at 01:14:24PM -0800, Manasi Navare wrote:
> Add an extra check before making master slave assignments for tiled
> displays to make sure we make these assignments only if all tiled
> connectors are present. If not then initialize the state to defaults
> so it does a normal non tiled modeset without transcoder port sync.
> 
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 28 ++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 7263eaa66cda..c0a2dab3fe67 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -12048,6 +12048,12 @@ static bool c8_planes_changed(const struct intel_crtc_state *new_crtc_state)
>  	return !old_crtc_state->c8_planes != !new_crtc_state->c8_planes;
>  }
>  
> +static void initialize_trans_port_sync_mode_state(struct intel_crtc_state *crtc_state)

"reset" rather than "initialize" might be more consistent with similar
things elsewhere in the driver?

> +{
> +	crtc_state->master_transcoder = INVALID_TRANSCODER;
> +	crtc_state->sync_mode_slaves_mask = 0;
> +}
> +
>  static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
>  {
>  	struct drm_crtc *crtc = crtc_state->uapi.crtc;
> @@ -12059,11 +12065,22 @@ static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
>  	struct drm_crtc *master_crtc = NULL;
>  	struct drm_crtc_state *master_crtc_state;
>  	struct intel_crtc_state *master_pipe_config;
> -	int i, tile_group_id;
> +	int i, tile_group_id = 0, num_tiled_conns = 0;
>  
>  	if (INTEL_GEN(dev_priv) < 11)
>  		return 0;
>  
> +	/* If all tiles not present do not make master slave assignments

This comment seems like it should go farther down the function; this
block isn't directly responsible for master/slave assignments, so it's a
bit confusing here.  Also, you might want to clarify what "present"
means in this case.  E.g., explain that since you've already added all
tiles of the same monitor to the transaction (earlier in
intel_atomic_check), then if the number of tiles in the tile group is
smaller than the size of the tile group it means that at least one of
the tiles isn't active.

> +	 * Here we assume all tiles belong to the same tile group for now.

Should this sentence be a FIXME:?  If we plug in 2x 2-tile monitors on
TGL, this would be problematic, right?

The logic changes seem correct (if we assume that only a single tiled
monitor is in use), so

Reviewed-by: Matt Roper <matthew.d.roper at intel.com>


Matt

> +	 */
> +	for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> +		if (connector->has_tile) {
> +			if (!tile_group_id)
> +				tile_group_id = connector->tile_group->id;
> +			num_tiled_conns++;
> +		}
> +	}
> +
>  	/*
>  	 * In case of tiled displays there could be one or more slaves but there is
>  	 * only one master. Lets make the CRTC used by the connector corresponding
> @@ -12077,8 +12094,15 @@ static int icl_add_sync_mode_crtcs(struct intel_crtc_state *crtc_state)
>  		if (!connector->has_tile)
>  			continue;
>  		if (crtc_state->hw.mode.hdisplay != connector->tile_h_size ||
> -		    crtc_state->hw.mode.vdisplay != connector->tile_v_size)
> +		    crtc_state->hw.mode.vdisplay != connector->tile_v_size) {
> +			initialize_trans_port_sync_mode_state(crtc_state);
>  			return 0;
> +		}
> +		if (connector->tile_group->id == tile_group_id &&
> +		    num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
> +			initialize_trans_port_sync_mode_state(crtc_state);
> +			return 0;
> +		}
>  		if (connector->tile_h_loc == connector->num_h_tile - 1 &&
>  		    connector->tile_v_loc == connector->num_v_tile - 1)
>  			continue;
> -- 
> 2.19.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795