Re: [PATCH 2/2] drm/amd/display: Move iteration out of dm_update_crtcs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 1/7/19 10:53 AM, sunpeng.li@xxxxxxx wrote:
> From: Leo Li <sunpeng.li@xxxxxxx>
> 
> [Why]
> To reduce indent in dm_update_crtcs, and to make it operate on single
> instances.
> 
> [How]
> Move iteration of plane states into atomic_check.
> No functional change is intended.
> 
> Signed-off-by: Leo Li <sunpeng.li@xxxxxxx>

Series is:

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

Looks good to me. I guess we don't need the other two patches from the 
last series if we're not going to be making use of the helper.

Nicholas Kazlauskas

> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 341 +++++++++++-----------
>   1 file changed, 175 insertions(+), 166 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 0b6bce5..70cd8bc 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5550,15 +5550,15 @@ static void reset_freesync_config_for_crtc(
>   	       sizeof(new_crtc_state->vrr_infopacket));
>   }
>   
> -static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
> -				 struct drm_atomic_state *state,
> -				 bool enable,
> -				 bool *lock_and_validation_needed)
> +static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
> +				struct drm_atomic_state *state,
> +				struct drm_crtc *crtc,
> +				struct drm_crtc_state *old_crtc_state,
> +				struct drm_crtc_state *new_crtc_state,
> +				bool enable,
> +				bool *lock_and_validation_needed)
>   {
>   	struct dm_atomic_state *dm_state = NULL;
> -	struct drm_crtc *crtc;
> -	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> -	int i;
>   	struct dm_crtc_state *dm_old_crtc_state, *dm_new_crtc_state;
>   	struct dc_stream_state *new_stream;
>   	int ret = 0;
> @@ -5567,200 +5567,199 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
>   	 * TODO Move this code into dm_crtc_atomic_check once we get rid of dc_validation_set
>   	 * update changed items
>   	 */
> -	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> -		struct amdgpu_crtc *acrtc = NULL;
> -		struct amdgpu_dm_connector *aconnector = NULL;
> -		struct drm_connector_state *drm_new_conn_state = NULL, *drm_old_conn_state = NULL;
> -		struct dm_connector_state *dm_new_conn_state = NULL, *dm_old_conn_state = NULL;
> -		struct drm_plane_state *new_plane_state = NULL;
> +	struct amdgpu_crtc *acrtc = NULL;
> +	struct amdgpu_dm_connector *aconnector = NULL;
> +	struct drm_connector_state *drm_new_conn_state = NULL, *drm_old_conn_state = NULL;
> +	struct dm_connector_state *dm_new_conn_state = NULL, *dm_old_conn_state = NULL;
> +	struct drm_plane_state *new_plane_state = NULL;
>   
> -		new_stream = NULL;
> +	new_stream = NULL;
>   
> -		dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
> -		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
> -		acrtc = to_amdgpu_crtc(crtc);
> +	dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
> +	dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
> +	acrtc = to_amdgpu_crtc(crtc);
>   
> -		new_plane_state = drm_atomic_get_new_plane_state(state, new_crtc_state->crtc->primary);
> +	new_plane_state = drm_atomic_get_new_plane_state(state, new_crtc_state->crtc->primary);
>   
> -		if (new_crtc_state->enable && new_plane_state && !new_plane_state->fb) {
> -			ret = -EINVAL;
> -			goto fail;
> -		}
> +	if (new_crtc_state->enable && new_plane_state && !new_plane_state->fb) {
> +		ret = -EINVAL;
> +		goto fail;
> +	}
>   
> -		aconnector = amdgpu_dm_find_first_crtc_matching_connector(state, crtc);
> +	aconnector = amdgpu_dm_find_first_crtc_matching_connector(state, crtc);
>   
> -		/* TODO This hack should go away */
> -		if (aconnector && enable) {
> -			/* Make sure fake sink is created in plug-in scenario */
> -			drm_new_conn_state = drm_atomic_get_new_connector_state(state,
> - 								    &aconnector->base);
> -			drm_old_conn_state = drm_atomic_get_old_connector_state(state,
> -								    &aconnector->base);
> +	/* TODO This hack should go away */
> +	if (aconnector && enable) {
> +		/* Make sure fake sink is created in plug-in scenario */
> +		drm_new_conn_state = drm_atomic_get_new_connector_state(state,
> +							    &aconnector->base);
> +		drm_old_conn_state = drm_atomic_get_old_connector_state(state,
> +							    &aconnector->base);
>   
> -			if (IS_ERR(drm_new_conn_state)) {
> -				ret = PTR_ERR_OR_ZERO(drm_new_conn_state);
> -				break;
> -			}
> +		if (IS_ERR(drm_new_conn_state)) {
> +			ret = PTR_ERR_OR_ZERO(drm_new_conn_state);
> +			goto fail;
> +		}
>   
> -			dm_new_conn_state = to_dm_connector_state(drm_new_conn_state);
> -			dm_old_conn_state = to_dm_connector_state(drm_old_conn_state);
> +		dm_new_conn_state = to_dm_connector_state(drm_new_conn_state);
> +		dm_old_conn_state = to_dm_connector_state(drm_old_conn_state);
>   
> -			new_stream = create_stream_for_sink(aconnector,
> -							     &new_crtc_state->mode,
> -							    dm_new_conn_state,
> -							    dm_old_crtc_state->stream);
> +		new_stream = create_stream_for_sink(aconnector,
> +						     &new_crtc_state->mode,
> +						    dm_new_conn_state,
> +						    dm_old_crtc_state->stream);
>   
> -			/*
> -			 * we can have no stream on ACTION_SET if a display
> -			 * was disconnected during S3, in this case it is not an
> -			 * error, the OS will be updated after detection, and
> -			 * will do the right thing on next atomic commit
> -			 */
> +		/*
> +		 * we can have no stream on ACTION_SET if a display
> +		 * was disconnected during S3, in this case it is not an
> +		 * error, the OS will be updated after detection, and
> +		 * will do the right thing on next atomic commit
> +		 */
>   
> -			if (!new_stream) {
> -				DRM_DEBUG_DRIVER("%s: Failed to create new stream for crtc %d\n",
> -						__func__, acrtc->base.base.id);
> -				break;
> -			}
> +		if (!new_stream) {
> +			DRM_DEBUG_DRIVER("%s: Failed to create new stream for crtc %d\n",
> +					__func__, acrtc->base.base.id);
> +			ret = -ENOMEM;
> +			goto fail;
> +		}
>   
> -			dm_new_crtc_state->abm_level = dm_new_conn_state->abm_level;
> +		dm_new_crtc_state->abm_level = dm_new_conn_state->abm_level;
>   
> -			if (dc_is_stream_unchanged(new_stream, dm_old_crtc_state->stream) &&
> -			    dc_is_stream_scaling_unchanged(new_stream, dm_old_crtc_state->stream)) {
> -				new_crtc_state->mode_changed = false;
> -				DRM_DEBUG_DRIVER("Mode change not required, setting mode_changed to %d",
> -						 new_crtc_state->mode_changed);
> -			}
> +		if (dc_is_stream_unchanged(new_stream, dm_old_crtc_state->stream) &&
> +		    dc_is_stream_scaling_unchanged(new_stream, dm_old_crtc_state->stream)) {
> +			new_crtc_state->mode_changed = false;
> +			DRM_DEBUG_DRIVER("Mode change not required, setting mode_changed to %d",
> +					 new_crtc_state->mode_changed);
>   		}
> +	}
>   
> -		if (!drm_atomic_crtc_needs_modeset(new_crtc_state))
> -			goto next_crtc;
> -
> -		DRM_DEBUG_DRIVER(
> -			"amdgpu_crtc id:%d crtc_state_flags: enable:%d, active:%d, "
> -			"planes_changed:%d, mode_changed:%d,active_changed:%d,"
> -			"connectors_changed:%d\n",
> -			acrtc->crtc_id,
> -			new_crtc_state->enable,
> -			new_crtc_state->active,
> -			new_crtc_state->planes_changed,
> -			new_crtc_state->mode_changed,
> -			new_crtc_state->active_changed,
> -			new_crtc_state->connectors_changed);
> +	if (!drm_atomic_crtc_needs_modeset(new_crtc_state))
> +		goto skip_modeset;
>   
> -		/* Remove stream for any changed/disabled CRTC */
> -		if (!enable) {
> +	DRM_DEBUG_DRIVER(
> +		"amdgpu_crtc id:%d crtc_state_flags: enable:%d, active:%d, "
> +		"planes_changed:%d, mode_changed:%d,active_changed:%d,"
> +		"connectors_changed:%d\n",
> +		acrtc->crtc_id,
> +		new_crtc_state->enable,
> +		new_crtc_state->active,
> +		new_crtc_state->planes_changed,
> +		new_crtc_state->mode_changed,
> +		new_crtc_state->active_changed,
> +		new_crtc_state->connectors_changed);
>   
> -			if (!dm_old_crtc_state->stream)
> -				goto next_crtc;
> +	/* Remove stream for any changed/disabled CRTC */
> +	if (!enable) {
>   
> -			ret = dm_atomic_get_state(state, &dm_state);
> -			if (ret)
> -				goto fail;
> +		if (!dm_old_crtc_state->stream)
> +			goto skip_modeset;
>   
> -			DRM_DEBUG_DRIVER("Disabling DRM crtc: %d\n",
> -					crtc->base.id);
> +		ret = dm_atomic_get_state(state, &dm_state);
> +		if (ret)
> +			goto fail;
>   
> -			/* i.e. reset mode */
> -			if (dc_remove_stream_from_ctx(
> -					dm->dc,
> -					dm_state->context,
> -					dm_old_crtc_state->stream) != DC_OK) {
> -				ret = -EINVAL;
> -				goto fail;
> -			}
> +		DRM_DEBUG_DRIVER("Disabling DRM crtc: %d\n",
> +				crtc->base.id);
>   
> -			dc_stream_release(dm_old_crtc_state->stream);
> -			dm_new_crtc_state->stream = NULL;
> +		/* i.e. reset mode */
> +		if (dc_remove_stream_from_ctx(
> +				dm->dc,
> +				dm_state->context,
> +				dm_old_crtc_state->stream) != DC_OK) {
> +			ret = -EINVAL;
> +			goto fail;
> +		}
>   
> -			reset_freesync_config_for_crtc(dm_new_crtc_state);
> +		dc_stream_release(dm_old_crtc_state->stream);
> +		dm_new_crtc_state->stream = NULL;
>   
> -			*lock_and_validation_needed = true;
> +		reset_freesync_config_for_crtc(dm_new_crtc_state);
>   
> -		} else {/* Add stream for any updated/enabled CRTC */
> -			/*
> -			 * Quick fix to prevent NULL pointer on new_stream when
> -			 * added MST connectors not found in existing crtc_state in the chained mode
> -			 * TODO: need to dig out the root cause of that
> -			 */
> -			if (!aconnector || (!aconnector->dc_sink && aconnector->mst_port))
> -				goto next_crtc;
> +		*lock_and_validation_needed = true;
>   
> -			if (modereset_required(new_crtc_state))
> -				goto next_crtc;
> +	} else {/* Add stream for any updated/enabled CRTC */
> +		/*
> +		 * Quick fix to prevent NULL pointer on new_stream when
> +		 * added MST connectors not found in existing crtc_state in the chained mode
> +		 * TODO: need to dig out the root cause of that
> +		 */
> +		if (!aconnector || (!aconnector->dc_sink && aconnector->mst_port))
> +			goto skip_modeset;
>   
> -			if (modeset_required(new_crtc_state, new_stream,
> -					     dm_old_crtc_state->stream)) {
> +		if (modereset_required(new_crtc_state))
> +			goto skip_modeset;
>   
> -				WARN_ON(dm_new_crtc_state->stream);
> +		if (modeset_required(new_crtc_state, new_stream,
> +				     dm_old_crtc_state->stream)) {
>   
> -				ret = dm_atomic_get_state(state, &dm_state);
> -				if (ret)
> -					goto fail;
> +			WARN_ON(dm_new_crtc_state->stream);
>   
> -				dm_new_crtc_state->stream = new_stream;
> +			ret = dm_atomic_get_state(state, &dm_state);
> +			if (ret)
> +				goto fail;
>   
> -				dc_stream_retain(new_stream);
> +			dm_new_crtc_state->stream = new_stream;
>   
> -				DRM_DEBUG_DRIVER("Enabling DRM crtc: %d\n",
> -							crtc->base.id);
> +			dc_stream_retain(new_stream);
>   
> -				if (dc_add_stream_to_ctx(
> -						dm->dc,
> -						dm_state->context,
> -						dm_new_crtc_state->stream) != DC_OK) {
> -					ret = -EINVAL;
> -					goto fail;
> -				}
> +			DRM_DEBUG_DRIVER("Enabling DRM crtc: %d\n",
> +						crtc->base.id);
>   
> -				*lock_and_validation_needed = true;
> +			if (dc_add_stream_to_ctx(
> +					dm->dc,
> +					dm_state->context,
> +					dm_new_crtc_state->stream) != DC_OK) {
> +				ret = -EINVAL;
> +				goto fail;
>   			}
> -		}
>   
> -next_crtc:
> -		/* Release extra reference */
> -		if (new_stream)
> -			 dc_stream_release(new_stream);
> +			*lock_and_validation_needed = true;
> +		}
> +	}
>   
> -		/*
> -		 * We want to do dc stream updates that do not require a
> -		 * full modeset below.
> -		 */
> -		if (!(enable && aconnector && new_crtc_state->enable &&
> -		      new_crtc_state->active))
> -			continue;
> -		/*
> -		 * Given above conditions, the dc state cannot be NULL because:
> -		 * 1. We're in the process of enabling CRTCs (just been added
> -		 *    to the dc context, or already is on the context)
> -		 * 2. Has a valid connector attached, and
> -		 * 3. Is currently active and enabled.
> -		 * => The dc stream state currently exists.
> -		 */
> -		BUG_ON(dm_new_crtc_state->stream == NULL);
> +skip_modeset:
> +	/* Release extra reference */
> +	if (new_stream)
> +		 dc_stream_release(new_stream);
>   
> -		/* Scaling or underscan settings */
> -		if (is_scaling_state_different(dm_old_conn_state, dm_new_conn_state))
> -			update_stream_scaling_settings(
> -				&new_crtc_state->mode, dm_new_conn_state, dm_new_crtc_state->stream);
> +	/*
> +	 * We want to do dc stream updates that do not require a
> +	 * full modeset below.
> +	 */
> +	if (!(enable && aconnector && new_crtc_state->enable &&
> +	      new_crtc_state->active))
> +		return 0;
> +	/*
> +	 * Given above conditions, the dc state cannot be NULL because:
> +	 * 1. We're in the process of enabling CRTCs (just been added
> +	 *    to the dc context, or already is on the context)
> +	 * 2. Has a valid connector attached, and
> +	 * 3. Is currently active and enabled.
> +	 * => The dc stream state currently exists.
> +	 */
> +	BUG_ON(dm_new_crtc_state->stream == NULL);
>   
> -		/*
> -		 * Color management settings. We also update color properties
> -		 * when a modeset is needed, to ensure it gets reprogrammed.
> -		 */
> -		if (dm_new_crtc_state->base.color_mgmt_changed ||
> -		    drm_atomic_crtc_needs_modeset(new_crtc_state)) {
> -			ret = amdgpu_dm_set_regamma_lut(dm_new_crtc_state);
> -			if (ret)
> -				goto fail;
> -			amdgpu_dm_set_ctm(dm_new_crtc_state);
> -		}
> +	/* Scaling or underscan settings */
> +	if (is_scaling_state_different(dm_old_conn_state, dm_new_conn_state))
> +		update_stream_scaling_settings(
> +			&new_crtc_state->mode, dm_new_conn_state, dm_new_crtc_state->stream);
>   
> -		/* Update Freesync settings. */
> -		get_freesync_config_for_crtc(dm_new_crtc_state,
> -					     dm_new_conn_state);
> +	/*
> +	 * Color management settings. We also update color properties
> +	 * when a modeset is needed, to ensure it gets reprogrammed.
> +	 */
> +	if (dm_new_crtc_state->base.color_mgmt_changed ||
> +	    drm_atomic_crtc_needs_modeset(new_crtc_state)) {
> +		ret = amdgpu_dm_set_regamma_lut(dm_new_crtc_state);
> +		if (ret)
> +			goto fail;
> +		amdgpu_dm_set_ctm(dm_new_crtc_state);
>   	}
>   
> +	/* Update Freesync settings. */
> +	get_freesync_config_for_crtc(dm_new_crtc_state,
> +				     dm_new_conn_state);
> +
>   	return ret;
>   
>   fail:
> @@ -6108,15 +6107,25 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>   	}
>   
>   	/* Disable all crtcs which require disable */
> -	ret = dm_update_crtcs_state(&adev->dm, state, false, &lock_and_validation_needed);
> -	if (ret) {
> -		goto fail;
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		ret = dm_update_crtc_state(&adev->dm, state, crtc,
> +					   old_crtc_state,
> +					   new_crtc_state,
> +					   false,
> +					   &lock_and_validation_needed);
> +		if (ret)
> +			goto fail;
>   	}
>   
>   	/* Enable all crtcs which require enable */
> -	ret = dm_update_crtcs_state(&adev->dm, state, true, &lock_and_validation_needed);
> -	if (ret) {
> -		goto fail;
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		ret = dm_update_crtc_state(&adev->dm, state, crtc,
> +					   old_crtc_state,
> +					   new_crtc_state,
> +					   true,
> +					   &lock_and_validation_needed);
> +		if (ret)
> +			goto fail;
>   	}
>   
>   	/* Add new/modified planes */
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux