Re: [PATCH 01/13] drm/amd/display: Add MST atomic routines

["Kazlauskas, Nicholas" <Nicholas.Kazlauskas@amd.com>]

On 2019-10-30 3:24 p.m., mikita.lipski@amd.com wrote:
> From: Mikita Lipski <mikita.lipski@amd.com>
> 
> - Adding encoder atomic check to find vcpi slots for a connector
> - Using DRM helper functions to calculate PBN
> - Adding connector atomic check to release vcpi slots if connector
> loses CRTC
> - Calculate  PBN and VCPI slots only once during atomic
> check and store them on crtc_state to eliminate
> redundant calculation
> - Call drm_dp_mst_atomic_check to verify validity of MST topology
> during state atomic check
> 
> v2: squashed previous 3 separate patches, removed DSC PBN calculation,
> and added PBN and VCPI slots properties to amdgpu connector
> 
> v3:
> - moved vcpi_slots and pbn properties to dm_crtc_state and dc_stream_state
> - updates stream's vcpi_slots and pbn on commit
> - separated patch from the DSC MST series
> 
> v4:
> - set vcpi_slots and pbn properties to dm_connector_state
> - copy porperties from connector state on to crtc state
> 
> v5:
> - keep the pbn and vcpi values only on connnector state
> - added a void pointer to the stream state instead on two ints,
> because dc_stream_state is OS agnostic. Pointer points to the
> current dm_connector_state.
> 
> v6:
> - Remove new param from stream
> 
> v7:
> - Fix error with using max capable bpc
> 
> Cc: Jun Lei <Jun.Lei@amd.com>
> Cc: Jerry Zuo <Jerry.Zuo@amd.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Mikita Lipski <mikita.lipski@amd.com>

Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

You might want to verify that this still works as you expect when 
changing "max bpc" on an MST display.

My understanding is that it'd still force a new modeset before encoder 
atomic check is called so you *should* have the correct bpc value during 
MST calculations.

> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 65 ++++++++++++++++++-
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  2 +
>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 51 ++++-----------
>   .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 32 +++++++++
>   4 files changed, 109 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 48f5b43e2698..d75726013436 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4180,7 +4180,8 @@ void amdgpu_dm_connector_funcs_reset(struct drm_connector *connector)
>   		state->underscan_hborder = 0;
>   		state->underscan_vborder = 0;
>   		state->base.max_requested_bpc = 8;
> -
> +		state->vcpi_slots = 0;
> +		state->pbn = 0;
>   		if (connector->connector_type == DRM_MODE_CONNECTOR_eDP)
>   			state->abm_level = amdgpu_dm_abm_level;
>   
> @@ -4209,7 +4210,8 @@ amdgpu_dm_connector_atomic_duplicate_state(struct drm_connector *connector)
>   	new_state->underscan_enable = state->underscan_enable;
>   	new_state->underscan_hborder = state->underscan_hborder;
>   	new_state->underscan_vborder = state->underscan_vborder;
> -
> +	new_state->vcpi_slots = state->vcpi_slots;
> +	new_state->pbn = state->pbn;
>   	return &new_state->base;
>   }
>   
> @@ -4606,10 +4608,64 @@ static void dm_encoder_helper_disable(struct drm_encoder *encoder)
>   
>   }
>   
> +static int convert_dc_color_depth_into_bpc (enum dc_color_depth display_color_depth)
> +{
> +	switch (display_color_depth) {
> +		case COLOR_DEPTH_666:
> +			return 6;
> +		case COLOR_DEPTH_888:
> +			return 8;
> +		case COLOR_DEPTH_101010:
> +			return 10;
> +		case COLOR_DEPTH_121212:
> +			return 12;
> +		case COLOR_DEPTH_141414:
> +			return 14;
> +		case COLOR_DEPTH_161616:
> +			return 16;
> +		default:
> +			break;
> +		}
> +	return 0;
> +}
> +
>   static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder,
>   					  struct drm_crtc_state *crtc_state,
>   					  struct drm_connector_state *conn_state)
>   {
> +	struct drm_atomic_state *state = crtc_state->state;
> +	struct drm_connector *connector = conn_state->connector;
> +	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
> +	struct dm_connector_state *dm_new_connector_state = to_dm_connector_state(conn_state);
> +	const struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
> +	struct drm_dp_mst_topology_mgr *mst_mgr;
> +	struct drm_dp_mst_port *mst_port;
> +	enum dc_color_depth color_depth;
> +	int clock, bpp = 0;
> +
> +	if (!aconnector->port || !aconnector->dc_sink)
> +		return 0;
> +
> +	mst_port = aconnector->port;
> +	mst_mgr = &aconnector->mst_port->mst_mgr;
> +
> +	if (!crtc_state->connectors_changed && !crtc_state->mode_changed)
> +		return 0;
> +
> +	if (!state->duplicated) {
> +		color_depth = convert_color_depth_from_display_info(connector, conn_state);
> +		bpp = convert_dc_color_depth_into_bpc(color_depth) * 3;

Minor nitpick here - this is converting from a numerical bpc to a DC 
enum and then from the DC enum back to a numerical bpc. Almost seems 
like we should just have a helper to get the numerical bpc.

Logic seems fine though.

> +		clock = adjusted_mode->clock;
> +		dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp);
> +	}
> +	dm_new_connector_state->vcpi_slots = drm_dp_atomic_find_vcpi_slots(state,
> +									   mst_mgr,
> +									   mst_port,
> +									   dm_new_connector_state->pbn);
> +	if (dm_new_connector_state->vcpi_slots < 0) {
> +		DRM_DEBUG_ATOMIC("failed finding vcpi slots: %d\n", (int)dm_new_connector_state->vcpi_slots);
> +		return dm_new_connector_state->vcpi_slots;
> +	}
>   	return 0;
>   }
>   
> @@ -7651,6 +7707,11 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>   	if (ret)
>   		goto fail;
>   
> +	/* Perform validation of MST topology in the state*/
> +	ret = drm_dp_mst_atomic_check(state);
> +	if (ret)
> +		goto fail;
> +
>   	if (state->legacy_cursor_update) {
>   		/*
>   		 * This is a fast cursor update coming from the plane update
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index c6fdebee7189..910c8598faf9 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -360,6 +360,8 @@ struct dm_connector_state {
>   	bool freesync_enable;
>   	bool freesync_capable;
>   	uint8_t abm_level;
> +	uint64_t vcpi_slots;
> +	uint64_t pbn;
>   };
>   
>   #define to_dm_connector_state(x)\
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index 11e5784aa62a..1b2cc85b4815 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> @@ -182,15 +182,20 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
>   		bool enable)
>   {
>   	struct amdgpu_dm_connector *aconnector;
> +	struct drm_connector *connector;
> +	struct dm_connector_state *dm_conn_state;
>   	struct drm_dp_mst_topology_mgr *mst_mgr;
>   	struct drm_dp_mst_port *mst_port;
> -	int slots = 0;
>   	bool ret;
> -	int clock;
> -	int bpp = 0;
> -	int pbn = 0;
>   
>   	aconnector = (struct amdgpu_dm_connector *)stream->dm_stream_context;
> +	/* Accessing the connector state is required for vcpi_slots allocation
> +	 * and directly relies on behaviour in commit check
> +	 * that blocks before commit guaranteeing that the state
> +	 * is not gonna be swapped while still in use in commit tail */
> +
> +	dm_conn_state = to_dm_connector_state(aconnector->base.state);
> +
>   
>   	if (!aconnector || !aconnector->mst_port)
>   		return false;
> @@ -203,42 +208,10 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
>   	mst_port = aconnector->port;
>   
>   	if (enable) {
> -		clock = stream->timing.pix_clk_100hz / 10;
> -
> -		switch (stream->timing.display_color_depth) {
> -
> -		case COLOR_DEPTH_666:
> -			bpp = 6;
> -			break;
> -		case COLOR_DEPTH_888:
> -			bpp = 8;
> -			break;
> -		case COLOR_DEPTH_101010:
> -			bpp = 10;
> -			break;
> -		case COLOR_DEPTH_121212:
> -			bpp = 12;
> -			break;
> -		case COLOR_DEPTH_141414:
> -			bpp = 14;
> -			break;
> -		case COLOR_DEPTH_161616:
> -			bpp = 16;
> -			break;
> -		default:
> -			ASSERT(bpp != 0);
> -			break;
> -		}
> -
> -		bpp = bpp * 3;
> -
> -		/* TODO need to know link rate */
> -
> -		pbn = drm_dp_calc_pbn_mode(clock, bpp);
> -
> -		slots = drm_dp_find_vcpi_slots(mst_mgr, pbn);
> -		ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port, pbn, slots);
>   
> +		ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port,
> +					       dm_conn_state->pbn,
> +					       dm_conn_state->vcpi_slots);
>   		if (!ret)
>   			return false;
>   
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 779d0b60cac9..1a17ea1b42e0 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -251,10 +251,42 @@ dm_mst_atomic_best_encoder(struct drm_connector *connector,
>   	return &to_amdgpu_dm_connector(connector)->mst_encoder->base;
>   }
>   
> +static int dm_dp_mst_atomic_check(struct drm_connector *connector,
> +				struct drm_atomic_state *state)
> +{
> +	struct drm_connector_state *new_conn_state =
> +			drm_atomic_get_new_connector_state(state, connector);
> +	struct drm_connector_state *old_conn_state =
> +			drm_atomic_get_old_connector_state(state, connector);
> +	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
> +	struct drm_crtc_state *new_crtc_state;
> +	struct drm_dp_mst_topology_mgr *mst_mgr;
> +	struct drm_dp_mst_port *mst_port;
> +
> +	mst_port = aconnector->port;
> +	mst_mgr = &aconnector->mst_port->mst_mgr;
> +
> +	if (!old_conn_state->crtc)
> +		return 0;
> +
> +	if (new_conn_state->crtc) {
> +		new_crtc_state = drm_atomic_get_old_crtc_state(state, new_conn_state->crtc);
> +		if (!new_crtc_state ||
> +		    !drm_atomic_crtc_needs_modeset(new_crtc_state) ||
> +		    new_crtc_state->enable)
> +			return 0;
> +		}
> +
> +	return drm_dp_atomic_release_vcpi_slots(state,
> +						mst_mgr,
> +						mst_port);
> +}
> +
>   static const struct drm_connector_helper_funcs dm_dp_mst_connector_helper_funcs = {
>   	.get_modes = dm_dp_mst_get_modes,
>   	.mode_valid = amdgpu_dm_connector_mode_valid,
>   	.atomic_best_encoder = dm_mst_atomic_best_encoder,
> +	.atomic_check = dm_dp_mst_atomic_check,
>   };
>   
>   static void amdgpu_dm_encoder_destroy(struct drm_encoder *encoder)
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel