Re: [PATCH 6/7] drm/amd/display: Drop dm_determine_update_type_for_commit

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

 



Reviewed-by: Rodrigo Siqueira <Rodrigo.Siqueira@xxxxxxx>

On 07/30, Nicholas Kazlauskas wrote:
> [Why]
> This was added in the past to solve the issue of not knowing when
> to stall for medium and full updates in DM.
> 
> Since DC is ultimately decides what requires bandwidth changes we
> wanted to make use of it directly to determine this.
> 
> The problem is that we can't actually pass any of the stream or surface
> updates into DC global validation, so we don't actually check if the new
> configuration is valid - we just validate the old existing config
> instead and stall for outstanding commits to finish.
> 
> There's also the problem of grabbing the DRM private object for
> pageflips which can lead to page faults in the case where commits
> execute out of order and free a DRM private object state that was
> still required for commit tail.
> 
> [How]
> Now that we reset the plane in DM with the same conditions DC checks
> we can have planes go through DC validation and we know when we need
> to check and stall based on whether the stream or planes changed.
> 
> We mark lock_and_validation_needed whenever we've done this, so just
> go back to using that instead of dm_determine_update_type_for_commit.
> 
> Since we'll skip resetting the plane for a pageflip we will no longer
> grab the DRM private object for pageflips as well, avoiding the
> page fault issued caused by pageflipping under load with commits
> executing out of order.
> 
> Cc: Harry Wentland <harry.wentland@xxxxxxx>
> Cc: Bhawanpreet Lakha <Bhawanpreet.Lakha@xxxxxxx>
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 199 ++----------------
>  1 file changed, 17 insertions(+), 182 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 2cbb29199e61..59829ec81694 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -8542,161 +8542,6 @@ static int dm_update_plane_state(struct dc *dc,
>  	return ret;
>  }
>  
> -static int
> -dm_determine_update_type_for_commit(struct amdgpu_display_manager *dm,
> -				    struct drm_atomic_state *state,
> -				    enum surface_update_type *out_type)
> -{
> -	struct dc *dc = dm->dc;
> -	struct dm_atomic_state *dm_state = NULL, *old_dm_state = NULL;
> -	int i, j, num_plane, ret = 0;
> -	struct drm_plane_state *old_plane_state, *new_plane_state;
> -	struct dm_plane_state *new_dm_plane_state, *old_dm_plane_state;
> -	struct drm_crtc *new_plane_crtc;
> -	struct drm_plane *plane;
> -
> -	struct drm_crtc *crtc;
> -	struct drm_crtc_state *new_crtc_state, *old_crtc_state;
> -	struct dm_crtc_state *new_dm_crtc_state, *old_dm_crtc_state;
> -	struct dc_stream_status *status = NULL;
> -	enum surface_update_type update_type = UPDATE_TYPE_FAST;
> -	struct surface_info_bundle {
> -		struct dc_surface_update surface_updates[MAX_SURFACES];
> -		struct dc_plane_info plane_infos[MAX_SURFACES];
> -		struct dc_scaling_info scaling_infos[MAX_SURFACES];
> -		struct dc_flip_addrs flip_addrs[MAX_SURFACES];
> -		struct dc_stream_update stream_update;
> -	} *bundle;
> -
> -	bundle = kzalloc(sizeof(*bundle), GFP_KERNEL);
> -
> -	if (!bundle) {
> -		DRM_ERROR("Failed to allocate update bundle\n");
> -		/* Set type to FULL to avoid crashing in DC*/
> -		update_type = UPDATE_TYPE_FULL;
> -		goto cleanup;
> -	}
> -
> -	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> -
> -		memset(bundle, 0, sizeof(struct surface_info_bundle));
> -
> -		new_dm_crtc_state = to_dm_crtc_state(new_crtc_state);
> -		old_dm_crtc_state = to_dm_crtc_state(old_crtc_state);
> -		num_plane = 0;
> -
> -		if (new_dm_crtc_state->stream != old_dm_crtc_state->stream) {
> -			update_type = UPDATE_TYPE_FULL;
> -			goto cleanup;
> -		}
> -
> -		if (!new_dm_crtc_state->stream)
> -			continue;
> -
> -		for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, j) {
> -			struct dc_plane_info *plane_info = &bundle->plane_infos[num_plane];
> -			struct dc_flip_addrs *flip_addr = &bundle->flip_addrs[num_plane];
> -			struct dc_scaling_info *scaling_info = &bundle->scaling_infos[num_plane];
> -
> -			new_plane_crtc = new_plane_state->crtc;
> -			new_dm_plane_state = to_dm_plane_state(new_plane_state);
> -			old_dm_plane_state = to_dm_plane_state(old_plane_state);
> -
> -			if (plane->type == DRM_PLANE_TYPE_CURSOR)
> -				continue;
> -
> -			if (new_dm_plane_state->dc_state != old_dm_plane_state->dc_state) {
> -				update_type = UPDATE_TYPE_FULL;
> -				goto cleanup;
> -			}
> -
> -			if (crtc != new_plane_crtc)
> -				continue;
> -
> -			bundle->surface_updates[num_plane].surface =
> -					new_dm_plane_state->dc_state;
> -
> -			if (new_crtc_state->mode_changed) {
> -				bundle->stream_update.dst = new_dm_crtc_state->stream->dst;
> -				bundle->stream_update.src = new_dm_crtc_state->stream->src;
> -			}
> -
> -			if (new_crtc_state->color_mgmt_changed) {
> -				bundle->surface_updates[num_plane].gamma =
> -						new_dm_plane_state->dc_state->gamma_correction;
> -				bundle->surface_updates[num_plane].in_transfer_func =
> -						new_dm_plane_state->dc_state->in_transfer_func;
> -				bundle->surface_updates[num_plane].gamut_remap_matrix =
> -						&new_dm_plane_state->dc_state->gamut_remap_matrix;
> -				bundle->stream_update.gamut_remap =
> -						&new_dm_crtc_state->stream->gamut_remap_matrix;
> -				bundle->stream_update.output_csc_transform =
> -						&new_dm_crtc_state->stream->csc_color_matrix;
> -				bundle->stream_update.out_transfer_func =
> -						new_dm_crtc_state->stream->out_transfer_func;
> -			}
> -
> -			ret = fill_dc_scaling_info(new_plane_state,
> -						   scaling_info);
> -			if (ret)
> -				goto cleanup;
> -
> -			bundle->surface_updates[num_plane].scaling_info = scaling_info;
> -
> -			if (new_plane_state->fb) {
> -				ret = fill_dc_plane_info_and_addr(
> -					dm->adev, new_plane_state,
> -					new_dm_plane_state->tiling_flags,
> -					plane_info, &flip_addr->address,
> -					new_dm_plane_state->tmz_surface, false);
> -				if (ret)
> -					goto cleanup;
> -
> -				bundle->surface_updates[num_plane].plane_info = plane_info;
> -				bundle->surface_updates[num_plane].flip_addr = flip_addr;
> -			}
> -
> -			num_plane++;
> -		}
> -
> -		if (num_plane == 0)
> -			continue;
> -
> -		ret = dm_atomic_get_state(state, &dm_state);
> -		if (ret)
> -			goto cleanup;
> -
> -		old_dm_state = dm_atomic_get_old_state(state);
> -		if (!old_dm_state) {
> -			ret = -EINVAL;
> -			goto cleanup;
> -		}
> -
> -		status = dc_stream_get_status_from_state(old_dm_state->context,
> -							 new_dm_crtc_state->stream);
> -		bundle->stream_update.stream = new_dm_crtc_state->stream;
> -		/*
> -		 * TODO: DC modifies the surface during this call so we need
> -		 * to lock here - find a way to do this without locking.
> -		 */
> -		mutex_lock(&dm->dc_lock);
> -		update_type = dc_check_update_surfaces_for_stream(
> -				dc,	bundle->surface_updates, num_plane,
> -				&bundle->stream_update, status);
> -		mutex_unlock(&dm->dc_lock);
> -
> -		if (update_type > UPDATE_TYPE_MED) {
> -			update_type = UPDATE_TYPE_FULL;
> -			goto cleanup;
> -		}
> -	}
> -
> -cleanup:
> -	kfree(bundle);
> -
> -	*out_type = update_type;
> -	return ret;
> -}
>  #if defined(CONFIG_DRM_AMD_DC_DCN)
>  static int add_affected_mst_dsc_crtcs(struct drm_atomic_state *state, struct drm_crtc *crtc)
>  {
> @@ -8737,8 +8582,7 @@ static int add_affected_mst_dsc_crtcs(struct drm_atomic_state *state, struct drm
>   * acquired. For full updates case which removes/adds/updates streams on one
>   * CRTC while flipping on another CRTC, acquiring global lock will guarantee
>   * that any such full update commit will wait for completion of any outstanding
> - * flip using DRMs synchronization events. See
> - * dm_determine_update_type_for_commit()
> + * flip using DRMs synchronization events.
>   *
>   * Note that DM adds the affected connectors for all CRTCs in state, when that
>   * might not seem necessary. This is because DC stream creation requires the
> @@ -8759,15 +8603,8 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>  	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>  	struct drm_plane *plane;
>  	struct drm_plane_state *old_plane_state, *new_plane_state;
> -	enum surface_update_type update_type = UPDATE_TYPE_FAST;
> -	enum surface_update_type overall_update_type = UPDATE_TYPE_FAST;
>  	enum dc_status status;
>  	int ret, i;
> -
> -	/*
> -	 * This bool will be set for true for any modeset/reset
> -	 * or plane update which implies non fast surface update.
> -	 */
>  	bool lock_and_validation_needed = false;
>  
>  	ret = drm_atomic_helper_check_modeset(dev, state);
> @@ -8961,27 +8798,23 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>  		if (!is_scaling_state_different(dm_new_con_state, dm_old_con_state))
>  			continue;
>  
> -		overall_update_type = UPDATE_TYPE_FULL;
>  		lock_and_validation_needed = true;
>  	}
>  
> -	ret = dm_determine_update_type_for_commit(&adev->dm, state, &update_type);
> -	if (ret)
> -		goto fail;
> -
> -	if (overall_update_type < update_type)
> -		overall_update_type = update_type;
> -
> -	/*
> -	 * lock_and_validation_needed was an old way to determine if we need to set
> -	 * the global lock. Leaving it in to check if we broke any corner cases
> -	 * lock_and_validation_needed true = UPDATE_TYPE_FULL or UPDATE_TYPE_MED
> -	 * lock_and_validation_needed false = UPDATE_TYPE_FAST
> +	/**
> +	 * Streams and planes are reset when there are changes that affect
> +	 * bandwidth. Anything that affects bandwidth needs to go through
> +	 * DC global validation to ensure that the configuration can be applied
> +	 * to hardware.
> +	 *
> +	 * We have to currently stall out here in atomic_check for outstanding
> +	 * commits to finish in this case because our IRQ handlers reference
> +	 * DRM state directly - we can end up disabling interrupts too early
> +	 * if we don't.
> +	 *
> +	 * TODO: Remove this stall and drop DM state private objects.
>  	 */
> -	if (lock_and_validation_needed && overall_update_type <= UPDATE_TYPE_FAST)
> -		WARN(1, "Global lock should be Set, overall_update_type should be UPDATE_TYPE_MED or UPDATE_TYPE_FULL");
> -
> -	if (overall_update_type > UPDATE_TYPE_FAST) {
> +	if (lock_and_validation_needed) {
>  		ret = dm_atomic_get_state(state, &dm_state);
>  		if (ret)
>  			goto fail;
> @@ -9063,7 +8896,9 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>  		struct dm_crtc_state *dm_new_crtc_state =
>  			to_dm_crtc_state(new_crtc_state);
>  
> -		dm_new_crtc_state->update_type = (int)overall_update_type;
> +		dm_new_crtc_state->update_type = lock_and_validation_needed ?
> +							 UPDATE_TYPE_FULL :
> +							 UPDATE_TYPE_FAST;
>  	}
>  
>  	/* Must be success */
> -- 
> 2.25.1
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7CRodrigo.Siqueira%40amd.com%7C0b11d89281b84b13c2ba08d834c866ba%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637317382661579264&amp;sdata=0%2BiC8Fx2pHp5cCo2p5TovGSRrrbnYH867lad%2B5ZeXqM%3D&amp;reserved=0

-- 
Rodrigo Siqueira
https://siqueira.tech

Attachment: signature.asc
Description: PGP signature

_______________________________________________
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