Reviewed-by: Rodrigo Siqueira <rodrigo.sique...@amd.com>
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.wentl...@amd.com>
> Cc: Bhawanpreet Lakha <bhawanpreet.la...@amd.com>
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlaus...@amd.com>
> ---
> .../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-...@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7CRodrigo.Siqueira%40amd.com%7C0b11d89281b84b13c2ba08d834c866ba%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637317382661579264&sdata=0%2BiC8Fx2pHp5cCo2p5TovGSRrrbnYH867lad%2B5ZeXqM%3D&reserved=0-- Rodrigo Siqueira https://siqueira.tech
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
