* [PATCH 1/2] drm/amd/display: Use private obj helpers for dm_atomic_state
@ 2018-11-22 17:34 Nicholas Kazlauskas
[not found] ` <20181122173437.13538-1-nicholas.kazlauskas-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 7+ messages in thread
From: Nicholas Kazlauskas @ 2018-11-22 17:34 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Leo Li, Harry Wentland, Nicholas Kazlauskas
[Why]
Two non-blocking commits in succession can result in a sequence where
the same dc->current_state is queried for both commits.
1. 1st commit -> check -> commit -> swaps atomic state -> queues work
2. 2nd commit -> check -> commit -> swaps atomic state -> queues work
3. 1st commit work finishes
The issue with this sequence is that the same dc->current_state is
read in both atomic checks. If the first commit modifies streams or
planes those will be missing from the dc->current_state for the
second atomic check. This result in many stream and plane errors in
atomic commit tail.
[How]
The driver still needs to track old to new state to determine if the
commit in its current implementation. Updating the dc_state in
atomic tail is wrong since the dc_state swap should be happening as
part of drm_atomic_helper_swap_state *before* the worker queue kicks
its work off.
The simplest replacement for the subclassing (which doesn't properly
manage the old to new atomic state swap) is to use the drm private
object helpers. While some of the dc_state members could be merged
into dm_crtc_state or dm_plane_state and copied over that way it is
easier for now to just treat the whole dc_state structure as a single
private object.
This allows amdgpu_dm to drop the dc->current_state copy from within
atomic check. It's replaced by a copy from the current atomic state
which is propagated correctly for the sequence described above.
Since access to the dm_state private object is now locked this should
also fix issues that could arise if submitting non-blocking commits
from different threads.
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Leo Li <sunpeng.li@amd.com>
Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 290 ++++++++++++++----
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 13 +-
2 files changed, 234 insertions(+), 69 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 8433d31cdea8..3ae438d9849f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -968,45 +968,6 @@ const struct amdgpu_ip_block_version dm_ip_block =
};
-static struct drm_atomic_state *
-dm_atomic_state_alloc(struct drm_device *dev)
-{
- struct dm_atomic_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
-
- if (!state)
- return NULL;
-
- if (drm_atomic_state_init(dev, &state->base) < 0)
- goto fail;
-
- return &state->base;
-
-fail:
- kfree(state);
- return NULL;
-}
-
-static void
-dm_atomic_state_clear(struct drm_atomic_state *state)
-{
- struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
-
- if (dm_state->context) {
- dc_release_state(dm_state->context);
- dm_state->context = NULL;
- }
-
- drm_atomic_state_default_clear(state);
-}
-
-static void
-dm_atomic_state_alloc_free(struct drm_atomic_state *state)
-{
- struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
- drm_atomic_state_default_release(state);
- kfree(dm_state);
-}
-
/**
* DOC: atomic
*
@@ -1018,9 +979,6 @@ static const struct drm_mode_config_funcs amdgpu_dm_mode_funcs = {
.output_poll_changed = drm_fb_helper_output_poll_changed,
.atomic_check = amdgpu_dm_atomic_check,
.atomic_commit = amdgpu_dm_atomic_commit,
- .atomic_state_alloc = dm_atomic_state_alloc,
- .atomic_state_clear = dm_atomic_state_clear,
- .atomic_state_free = dm_atomic_state_alloc_free
};
static struct drm_mode_config_helper_funcs amdgpu_dm_mode_config_helperfuncs = {
@@ -1542,8 +1500,117 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
}
#endif
+/*
+ * Acquires the lock for the atomic state object and returns
+ * the new atomic state.
+ *
+ * This should only be called during atomic check.
+ */
+static int dm_atomic_get_state(struct drm_atomic_state *state,
+ struct dm_atomic_state **dm_state)
+{
+ struct drm_device *dev = state->dev;
+ struct amdgpu_device *adev = dev->dev_private;
+ struct amdgpu_display_manager *dm = &adev->dm;
+ struct drm_private_state *priv_state;
+ int ret;
+
+ if (*dm_state)
+ return 0;
+
+ ret = drm_modeset_lock(&dm->atomic_obj_lock, state->acquire_ctx);
+ if (ret)
+ return ret;
+
+ priv_state = drm_atomic_get_private_obj_state(state, &dm->atomic_obj);
+ if (IS_ERR(priv_state))
+ return PTR_ERR(priv_state);
+
+ *dm_state = to_dm_atomic_state(priv_state);
+
+ return 0;
+}
+
+struct dm_atomic_state *
+dm_atomic_get_new_state(struct drm_atomic_state *state)
+{
+ struct drm_device *dev = state->dev;
+ struct amdgpu_device *adev = dev->dev_private;
+ struct amdgpu_display_manager *dm = &adev->dm;
+ struct drm_private_obj *obj;
+ struct drm_private_state *new_obj_state;
+ int i;
+
+ for_each_new_private_obj_in_state(state, obj, new_obj_state, i) {
+ if (obj->funcs == dm->atomic_obj.funcs)
+ return to_dm_atomic_state(new_obj_state);
+ }
+
+ return NULL;
+}
+
+struct dm_atomic_state *
+dm_atomic_get_old_state(struct drm_atomic_state *state)
+{
+ struct drm_device *dev = state->dev;
+ struct amdgpu_device *adev = dev->dev_private;
+ struct amdgpu_display_manager *dm = &adev->dm;
+ struct drm_private_obj *obj;
+ struct drm_private_state *old_obj_state;
+ int i;
+
+ for_each_old_private_obj_in_state(state, obj, old_obj_state, i) {
+ if (obj->funcs == dm->atomic_obj.funcs)
+ return to_dm_atomic_state(old_obj_state);
+ }
+
+ return NULL;
+}
+
+static struct drm_private_state *
+dm_atomic_duplicate_state(struct drm_private_obj *obj)
+{
+ struct dm_atomic_state *old_state, *new_state;
+
+ new_state = kzalloc(sizeof(*new_state), GFP_KERNEL);
+ if (!new_state)
+ return NULL;
+
+ __drm_atomic_helper_private_obj_duplicate_state(obj, &new_state->base);
+
+ new_state->context = dc_create_state();
+ if (!new_state->context) {
+ kfree(new_state);
+ return NULL;
+ }
+
+ old_state = to_dm_atomic_state(obj->state);
+ if (old_state && old_state->context)
+ dc_resource_state_copy_construct(old_state->context,
+ new_state->context);
+
+ return &new_state->base;
+}
+
+static void dm_atomic_destroy_state(struct drm_private_obj *obj,
+ struct drm_private_state *state)
+{
+ struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
+
+ if (dm_state && dm_state->context)
+ dc_release_state(dm_state->context);
+
+ kfree(dm_state);
+}
+
+static struct drm_private_state_funcs dm_atomic_state_funcs = {
+ .atomic_duplicate_state = dm_atomic_duplicate_state,
+ .atomic_destroy_state = dm_atomic_destroy_state,
+};
+
static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
{
+ struct dm_atomic_state *state;
int r;
adev->mode_info.mode_config_initialized = true;
@@ -1561,6 +1628,24 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
adev->ddev->mode_config.fb_base = adev->gmc.aper_base;
+ drm_modeset_lock_init(&adev->dm.atomic_obj_lock);
+
+ state = kzalloc(sizeof(*state), GFP_KERNEL);
+ if (!state)
+ return -ENOMEM;
+
+ state->context = dc_create_state();
+ if (!state->context) {
+ kfree(state);
+ return -ENOMEM;
+ }
+
+ dc_resource_state_copy_construct_current(adev->dm.dc, state->context);
+
+ drm_atomic_private_obj_init(&adev->dm.atomic_obj,
+ &state->base,
+ &dm_atomic_state_funcs);
+
r = amdgpu_display_modeset_create_props(adev);
if (r)
return r;
@@ -1849,6 +1934,7 @@ static int amdgpu_dm_initialize_drm_device(struct amdgpu_device *adev)
static void amdgpu_dm_destroy_drm_device(struct amdgpu_display_manager *dm)
{
drm_mode_config_cleanup(dm->ddev);
+ drm_atomic_private_obj_fini(&dm->atomic_obj);
return;
}
@@ -4274,6 +4360,20 @@ static void prepare_flip_isr(struct amdgpu_crtc *acrtc)
acrtc->crtc_id);
}
+struct dc_stream_status *dc_state_get_stream_status(
+ struct dc_state *state,
+ struct dc_stream_state *stream)
+{
+ uint8_t i;
+
+ for (i = 0; i < state->stream_count; i++) {
+ if (stream == state->streams[i])
+ return &state->stream_status[i];
+ }
+
+ return NULL;
+}
+
/*
* Executes flip
*
@@ -4477,6 +4577,7 @@ static bool commit_planes_to_stream(
}
static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
+ struct dc_state *dc_state,
struct drm_device *dev,
struct amdgpu_display_manager *dm,
struct drm_crtc *pcrtc,
@@ -4493,7 +4594,6 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
struct dm_crtc_state *acrtc_state = to_dm_crtc_state(new_pcrtc_state);
struct dm_crtc_state *dm_old_crtc_state =
to_dm_crtc_state(drm_atomic_get_old_crtc_state(state, pcrtc));
- struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
int planes_count = 0;
unsigned long flags;
@@ -4554,7 +4654,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
crtc,
fb,
(uint32_t)drm_crtc_vblank_count(crtc) + *wait_for_vblank,
- dm_state->context);
+ dc_state);
}
}
@@ -4579,7 +4679,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
planes_count,
acrtc_state,
dm_old_crtc_state,
- dm_state->context))
+ dc_state))
dm_error("%s: Failed to attach plane!\n", __func__);
} else {
/*TODO BUG Here should go disable planes on CRTC. */
@@ -4647,6 +4747,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
struct amdgpu_device *adev = dev->dev_private;
struct amdgpu_display_manager *dm = &adev->dm;
struct dm_atomic_state *dm_state;
+ struct dc_state *dc_state = NULL, *dc_state_temp = NULL;
uint32_t i, j;
struct drm_crtc *crtc;
struct drm_crtc_state *old_crtc_state, *new_crtc_state;
@@ -4659,7 +4760,16 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
drm_atomic_helper_update_legacy_modeset_state(dev, state);
- dm_state = to_dm_atomic_state(state);
+ dm_state = dm_atomic_get_new_state(state);
+ if (dm_state && dm_state->context) {
+ dc_state = dm_state->context;
+ } else {
+ /* No state changes, retain current state. */
+ dc_state_temp = dc_create_state();
+ ASSERT(dc_state_temp);
+ dc_state = dc_state_temp;
+ dc_resource_state_copy_construct_current(dm->dc, dc_state);
+ }
/* update changed items */
for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
@@ -4732,9 +4842,9 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
}
} /* for_each_crtc_in_state() */
- if (dm_state->context) {
- dm_enable_per_frame_crtc_master_sync(dm_state->context);
- WARN_ON(!dc_commit_state(dm->dc, dm_state->context));
+ if (dc_state) {
+ dm_enable_per_frame_crtc_master_sync(dc_state);
+ WARN_ON(!dc_commit_state(dm->dc, dc_state));
}
for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
@@ -4746,6 +4856,10 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
const struct dc_stream_status *status =
dc_stream_get_status(dm_new_crtc_state->stream);
+ if (!status)
+ status = dc_state_get_stream_status(dc_state,
+ dm_new_crtc_state->stream);
+
if (!status)
DC_ERR("got no status for stream %p on acrtc%p\n", dm_new_crtc_state->stream, acrtc);
else
@@ -4828,7 +4942,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
if (dm_new_crtc_state->stream)
- amdgpu_dm_commit_planes(state, dev, dm, crtc, &wait_for_vblank);
+ amdgpu_dm_commit_planes(state, dc_state, dev,
+ dm, crtc, &wait_for_vblank);
}
@@ -4868,6 +4983,9 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
for (i = 0; i < crtc_disable_count; i++)
pm_runtime_put_autosuspend(dev->dev);
pm_runtime_mark_last_busy(dev->dev);
+
+ if (dc_state_temp)
+ dc_release_state(dc_state_temp);
}
@@ -5054,11 +5172,11 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
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 dm_atomic_state *dm_state = to_dm_atomic_state(state);
struct dc_stream_state *new_stream;
int ret = 0;
@@ -5156,6 +5274,10 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
if (!dm_old_crtc_state->stream)
goto next_crtc;
+ ret = dm_atomic_get_state(state, &dm_state);
+ if (ret)
+ goto fail;
+
DRM_DEBUG_DRIVER("Disabling DRM crtc: %d\n",
crtc->base.id);
@@ -5190,6 +5312,10 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
WARN_ON(dm_new_crtc_state->stream);
+ ret = dm_atomic_get_state(state, &dm_state);
+ if (ret)
+ goto fail;
+
dm_new_crtc_state->stream = new_stream;
dc_stream_retain(new_stream);
@@ -5264,12 +5390,13 @@ static int dm_update_planes_state(struct dc *dc,
bool enable,
bool *lock_and_validation_needed)
{
+
+ struct dm_atomic_state *dm_state = NULL;
struct drm_crtc *new_plane_crtc, *old_plane_crtc;
struct drm_crtc_state *old_crtc_state, *new_crtc_state;
struct drm_plane *plane;
struct drm_plane_state *old_plane_state, *new_plane_state;
struct dm_crtc_state *dm_new_crtc_state, *dm_old_crtc_state;
- struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
struct dm_plane_state *dm_new_plane_state, *dm_old_plane_state;
int i ;
/* TODO return page_flip_needed() function */
@@ -5307,6 +5434,10 @@ static int dm_update_planes_state(struct dc *dc,
DRM_DEBUG_ATOMIC("Disabling DRM plane: %d on DRM crtc %d\n",
plane->base.id, old_plane_crtc->base.id);
+ ret = dm_atomic_get_state(state, &dm_state);
+ if (ret)
+ return ret;
+
if (!dc_remove_plane_from_context(
dc,
dm_old_crtc_state->stream,
@@ -5361,6 +5492,12 @@ static int dm_update_planes_state(struct dc *dc,
return ret;
}
+ ret = dm_atomic_get_state(state, &dm_state);
+ if (ret) {
+ dc_plane_state_release(dc_new_plane_state);
+ return ret;
+ }
+
/*
* Any atomic check errors that occur after this will
* not need a release. The plane state will be attached
@@ -5392,11 +5529,14 @@ static int dm_update_planes_state(struct dc *dc,
return ret;
}
-enum surface_update_type dm_determine_update_type_for_commit(struct dc *dc, struct drm_atomic_state *state)
-{
-
- int i, j, num_plane;
+static int
+dm_determine_update_type_for_commit(struct dc *dc,
+ struct drm_atomic_state *state,
+ enum surface_update_type *out_type)
+{
+ 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, *old_plane_crtc;
@@ -5416,7 +5556,7 @@ enum surface_update_type dm_determine_update_type_for_commit(struct dc *dc, stru
DRM_ERROR("Plane or surface update failed to allocate");
/* Set type to FULL to avoid crashing in DC*/
update_type = UPDATE_TYPE_FULL;
- goto ret;
+ goto cleanup;
}
for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
@@ -5470,27 +5610,40 @@ enum surface_update_type dm_determine_update_type_for_commit(struct dc *dc, stru
}
if (num_plane > 0) {
- status = dc_stream_get_status(new_dm_crtc_state->stream);
+ 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_state_get_stream_status(old_dm_state->context,
+ new_dm_crtc_state->stream);
+
update_type = dc_check_update_surfaces_for_stream(dc, updates, num_plane,
&stream_update, status);
if (update_type > UPDATE_TYPE_MED) {
update_type = UPDATE_TYPE_FULL;
- goto ret;
+ goto cleanup;
}
}
} else if (!new_dm_crtc_state->stream && old_dm_crtc_state->stream) {
update_type = UPDATE_TYPE_FULL;
- goto ret;
+ goto cleanup;
}
}
-ret:
+cleanup:
kfree(updates);
kfree(surface);
- return update_type;
+ *out_type = update_type;
+ return ret;
}
/**
@@ -5522,8 +5675,8 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
struct drm_atomic_state *state)
{
struct amdgpu_device *adev = dev->dev_private;
+ struct dm_atomic_state *dm_state = NULL;
struct dc *dc = adev->dm.dc;
- struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
struct drm_connector *connector;
struct drm_connector_state *old_con_state, *new_con_state;
struct drm_crtc *crtc;
@@ -5564,10 +5717,6 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
goto fail;
}
- dm_state->context = dc_create_state();
- ASSERT(dm_state->context);
- dc_resource_state_copy_construct_current(dc, dm_state->context);
-
/* Remove exiting planes if they are modified */
ret = dm_update_planes_state(dc, state, false, &lock_and_validation_needed);
if (ret) {
@@ -5620,7 +5769,9 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
lock_and_validation_needed = true;
}
- update_type = dm_determine_update_type_for_commit(dc, state);
+ ret = dm_determine_update_type_for_commit(dc, state, &update_type);
+ if (ret)
+ goto fail;
if (overall_update_type < update_type)
overall_update_type = update_type;
@@ -5638,6 +5789,9 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
if (overall_update_type > UPDATE_TYPE_FAST) {
+ ret = dm_atomic_get_state(state, &dm_state);
+ if (ret)
+ goto fail;
ret = do_aquire_global_lock(dev, state);
if (ret)
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 607c3cdd7d0c..c8fd95950d0c 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -111,6 +111,17 @@ struct amdgpu_display_manager {
struct drm_device *ddev;
u16 display_indexes_num;
+ /**
+ * @atomic_obj
+ *
+ * In combination with &dm_atomic_state it helps manage
+ * global atomic state that doesn't map cleanly into existing
+ * drm resources, like &dc_context.
+ */
+ struct drm_private_obj atomic_obj;
+
+ struct drm_modeset_lock atomic_obj_lock;
+
/**
* @irq_handler_list_low_tab:
*
@@ -239,7 +250,7 @@ struct dm_crtc_state {
#define to_dm_crtc_state(x) container_of(x, struct dm_crtc_state, base)
struct dm_atomic_state {
- struct drm_atomic_state base;
+ struct drm_private_state base;
struct dc_state *context;
};
--
2.17.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] drm/amd/display: Remove wait for hw/flip done in atomic check
[not found] ` <20181122173437.13538-1-nicholas.kazlauskas-5C7GfCeVMHo@public.gmane.org>
@ 2018-11-22 17:34 ` Nicholas Kazlauskas
[not found] ` <20181122173437.13538-2-nicholas.kazlauskas-5C7GfCeVMHo@public.gmane.org>
2018-11-27 21:20 ` [PATCH 1/2] drm/amd/display: Use private obj helpers for dm_atomic_state Li, Sun peng (Leo)
1 sibling, 1 reply; 7+ messages in thread
From: Nicholas Kazlauskas @ 2018-11-22 17:34 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Leo Li, Harry Wentland, Nicholas Kazlauskas
[Why]
Atomic check can't truly be non-blocking if amdgpu_dm is waiting for
hw_done and flip_done in atomic check. This introduces waits when
any previous non-blocking commits queued work on a worker thread and
a new atomic commit attempts to do another full update/modeset.
[How]
Drop the waits and move the global lock acqusition into atomic check.
This is fine to do since commit tail waits for these dependencies
before calling amdgpu_dm_atomic_commit_tail.
This is only safe as long as DC never queries anything from within
current_state when doing validation in atomic_check. This is the
case as of writing, but any future uses of dc->current_state from
within atomic_check should be considered incorrect.
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Leo Li <sunpeng.li@amd.com>
Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 58 ++-----------------
1 file changed, 6 insertions(+), 52 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 3ae438d9849f..fe21bb86b66a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5078,57 +5078,6 @@ void dm_restore_drm_connector_state(struct drm_device *dev,
dm_force_atomic_commit(&aconnector->base);
}
-/*
- * Grabs all modesetting locks to serialize against any blocking commits,
- * Waits for completion of all non blocking commits.
- */
-static int do_aquire_global_lock(struct drm_device *dev,
- struct drm_atomic_state *state)
-{
- struct drm_crtc *crtc;
- struct drm_crtc_commit *commit;
- long ret;
-
- /*
- * Adding all modeset locks to aquire_ctx will
- * ensure that when the framework release it the
- * extra locks we are locking here will get released to
- */
- ret = drm_modeset_lock_all_ctx(dev, state->acquire_ctx);
- if (ret)
- return ret;
-
- list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
- spin_lock(&crtc->commit_lock);
- commit = list_first_entry_or_null(&crtc->commit_list,
- struct drm_crtc_commit, commit_entry);
- if (commit)
- drm_crtc_commit_get(commit);
- spin_unlock(&crtc->commit_lock);
-
- if (!commit)
- continue;
-
- /*
- * Make sure all pending HW programming completed and
- * page flips done
- */
- ret = wait_for_completion_interruptible_timeout(&commit->hw_done, 10*HZ);
-
- if (ret > 0)
- ret = wait_for_completion_interruptible_timeout(
- &commit->flip_done, 10*HZ);
-
- if (ret == 0)
- DRM_ERROR("[CRTC:%d:%s] hw_done or flip_done "
- "timed out\n", crtc->base.id, crtc->name);
-
- drm_crtc_commit_put(commit);
- }
-
- return ret < 0 ? ret : 0;
-}
-
void set_freesync_on_stream(struct amdgpu_display_manager *dm,
struct dm_crtc_state *new_crtc_state,
struct dm_connector_state *new_con_state,
@@ -5793,7 +5742,12 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
if (ret)
goto fail;
- ret = do_aquire_global_lock(dev, state);
+ /*
+ * This should be replaced with finer locking on the
+ * on the appropriate resources when possible.
+ * For now it's safer to grab everything.
+ */
+ ret = drm_modeset_lock_all_ctx(dev, state->acquire_ctx);
if (ret)
goto fail;
--
2.17.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm/amd/display: Remove wait for hw/flip done in atomic check
[not found] ` <20181122173437.13538-2-nicholas.kazlauskas-5C7GfCeVMHo@public.gmane.org>
@ 2018-11-22 19:39 ` Grodzovsky, Andrey
[not found] ` <9b4c08d4-4aac-02dc-ce86-ae7152ae67c0-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 7+ messages in thread
From: Grodzovsky, Andrey @ 2018-11-22 19:39 UTC (permalink / raw)
To: Kazlauskas, Nicholas, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Li, Sun peng (Leo), Wentland, Harry
On 11/22/2018 12:34 PM, Nicholas Kazlauskas wrote:
> [Why]
> Atomic check can't truly be non-blocking if amdgpu_dm is waiting for
> hw_done and flip_done in atomic check. This introduces waits when
> any previous non-blocking commits queued work on a worker thread and
> a new atomic commit attempts to do another full update/modeset.
>
> [How]
> Drop the waits and move the global lock acqusition into atomic check.
>
> This is fine to do since commit tail waits for these dependencies
> before calling amdgpu_dm_atomic_commit_tail.
Note that drm_atomic_helper_wait_for_dependencies waits only on all
preceeding commits that touch the same CRTC
while our custom do_aquire_global_lock wait on ALL previous commits in
the system - even on other CRTCS. As far as I can
remember that was important because we have global dc_state context
which must be protected for access/modifications while
looks like DRM assumes unrelated CRTCs can be modified concurrently.
Try to verify that latest display code will not be broken by this
relaxation of synchronization.
Andrey
>
> This is only safe as long as DC never queries anything from within
> current_state when doing validation in atomic_check. This is the
> case as of writing, but any future uses of dc->current_state from
> within atomic_check should be considered incorrect.
>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Leo Li <sunpeng.li@amd.com>
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 58 ++-----------------
> 1 file changed, 6 insertions(+), 52 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 3ae438d9849f..fe21bb86b66a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5078,57 +5078,6 @@ void dm_restore_drm_connector_state(struct drm_device *dev,
> dm_force_atomic_commit(&aconnector->base);
> }
>
> -/*
> - * Grabs all modesetting locks to serialize against any blocking commits,
> - * Waits for completion of all non blocking commits.
> - */
> -static int do_aquire_global_lock(struct drm_device *dev,
> - struct drm_atomic_state *state)
> -{
> - struct drm_crtc *crtc;
> - struct drm_crtc_commit *commit;
> - long ret;
> -
> - /*
> - * Adding all modeset locks to aquire_ctx will
> - * ensure that when the framework release it the
> - * extra locks we are locking here will get released to
> - */
> - ret = drm_modeset_lock_all_ctx(dev, state->acquire_ctx);
> - if (ret)
> - return ret;
> -
> - list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> - spin_lock(&crtc->commit_lock);
> - commit = list_first_entry_or_null(&crtc->commit_list,
> - struct drm_crtc_commit, commit_entry);
> - if (commit)
> - drm_crtc_commit_get(commit);
> - spin_unlock(&crtc->commit_lock);
> -
> - if (!commit)
> - continue;
> -
> - /*
> - * Make sure all pending HW programming completed and
> - * page flips done
> - */
> - ret = wait_for_completion_interruptible_timeout(&commit->hw_done, 10*HZ);
> -
> - if (ret > 0)
> - ret = wait_for_completion_interruptible_timeout(
> - &commit->flip_done, 10*HZ);
> -
> - if (ret == 0)
> - DRM_ERROR("[CRTC:%d:%s] hw_done or flip_done "
> - "timed out\n", crtc->base.id, crtc->name);
> -
> - drm_crtc_commit_put(commit);
> - }
> -
> - return ret < 0 ? ret : 0;
> -}
> -
> void set_freesync_on_stream(struct amdgpu_display_manager *dm,
> struct dm_crtc_state *new_crtc_state,
> struct dm_connector_state *new_con_state,
> @@ -5793,7 +5742,12 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
> if (ret)
> goto fail;
>
> - ret = do_aquire_global_lock(dev, state);
> + /*
> + * This should be replaced with finer locking on the
> + * on the appropriate resources when possible.
> + * For now it's safer to grab everything.
> + */
> + ret = drm_modeset_lock_all_ctx(dev, state->acquire_ctx);
> if (ret)
> goto fail;
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm/amd/display: Remove wait for hw/flip done in atomic check
[not found] ` <9b4c08d4-4aac-02dc-ce86-ae7152ae67c0-5C7GfCeVMHo@public.gmane.org>
@ 2018-11-22 19:43 ` Kazlauskas, Nicholas
[not found] ` <66d5f064-61fc-83d1-bb4a-2e0227dca826-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 7+ messages in thread
From: Kazlauskas, Nicholas @ 2018-11-22 19:43 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
On 11/22/18 2:39 PM, Grodzovsky, Andrey wrote:
>
>
> On 11/22/2018 12:34 PM, Nicholas Kazlauskas wrote:
>> [Why]
>> Atomic check can't truly be non-blocking if amdgpu_dm is waiting for
>> hw_done and flip_done in atomic check. This introduces waits when
>> any previous non-blocking commits queued work on a worker thread and
>> a new atomic commit attempts to do another full update/modeset.
>>
>> [How]
>> Drop the waits and move the global lock acqusition into atomic check.
>>
>> This is fine to do since commit tail waits for these dependencies
>> before calling amdgpu_dm_atomic_commit_tail.
>
> Note that drm_atomic_helper_wait_for_dependencies waits only on all
> preceeding commits that touch the same CRTC
> while our custom do_aquire_global_lock wait on ALL previous commits in
> the system - even on other CRTCS. As far as I can
> remember that was important because we have global dc_state context
> which must be protected for access/modifications while
> looks like DRM assumes unrelated CRTCs can be modified concurrently.
> Try to verify that latest display code will not be broken by this
> relaxation of synchronization.
>
> Andrey
This is a good point.
What we should really be doing instead here is locking anything that can
modify dc->current_state from within atomic commit tail. Serializing
commits into a queue could work too.
Either should fix non-blocking support.
Nicholas Kazlauskas
>
>>
>> This is only safe as long as DC never queries anything from within
>> current_state when doing validation in atomic_check. This is the
>> case as of writing, but any future uses of dc->current_state from
>> within atomic_check should be considered incorrect.
>>
>> Cc: Harry Wentland <harry.wentland@amd.com>
>> Cc: Leo Li <sunpeng.li@amd.com>
>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>> ---
>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 58 ++-----------------
>> 1 file changed, 6 insertions(+), 52 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 3ae438d9849f..fe21bb86b66a 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -5078,57 +5078,6 @@ void dm_restore_drm_connector_state(struct drm_device *dev,
>> dm_force_atomic_commit(&aconnector->base);
>> }
>>
>> -/*
>> - * Grabs all modesetting locks to serialize against any blocking commits,
>> - * Waits for completion of all non blocking commits.
>> - */
>> -static int do_aquire_global_lock(struct drm_device *dev,
>> - struct drm_atomic_state *state)
>> -{
>> - struct drm_crtc *crtc;
>> - struct drm_crtc_commit *commit;
>> - long ret;
>> -
>> - /*
>> - * Adding all modeset locks to aquire_ctx will
>> - * ensure that when the framework release it the
>> - * extra locks we are locking here will get released to
>> - */
>> - ret = drm_modeset_lock_all_ctx(dev, state->acquire_ctx);
>> - if (ret)
>> - return ret;
>> -
>> - list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>> - spin_lock(&crtc->commit_lock);
>> - commit = list_first_entry_or_null(&crtc->commit_list,
>> - struct drm_crtc_commit, commit_entry);
>> - if (commit)
>> - drm_crtc_commit_get(commit);
>> - spin_unlock(&crtc->commit_lock);
>> -
>> - if (!commit)
>> - continue;
>> -
>> - /*
>> - * Make sure all pending HW programming completed and
>> - * page flips done
>> - */
>> - ret = wait_for_completion_interruptible_timeout(&commit->hw_done, 10*HZ);
>> -
>> - if (ret > 0)
>> - ret = wait_for_completion_interruptible_timeout(
>> - &commit->flip_done, 10*HZ);
>> -
>> - if (ret == 0)
>> - DRM_ERROR("[CRTC:%d:%s] hw_done or flip_done "
>> - "timed out\n", crtc->base.id, crtc->name);
>> -
>> - drm_crtc_commit_put(commit);
>> - }
>> -
>> - return ret < 0 ? ret : 0;
>> -}
>> -
>> void set_freesync_on_stream(struct amdgpu_display_manager *dm,
>> struct dm_crtc_state *new_crtc_state,
>> struct dm_connector_state *new_con_state,
>> @@ -5793,7 +5742,12 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>> if (ret)
>> goto fail;
>>
>> - ret = do_aquire_global_lock(dev, state);
>> + /*
>> + * This should be replaced with finer locking on the
>> + * on the appropriate resources when possible.
>> + * For now it's safer to grab everything.
>> + */
>> + ret = drm_modeset_lock_all_ctx(dev, state->acquire_ctx);
>> if (ret)
>> goto fail;
>>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm/amd/display: Remove wait for hw/flip done in atomic check
[not found] ` <66d5f064-61fc-83d1-bb4a-2e0227dca826-5C7GfCeVMHo@public.gmane.org>
@ 2018-11-22 19:49 ` Grodzovsky, Andrey
0 siblings, 0 replies; 7+ messages in thread
From: Grodzovsky, Andrey @ 2018-11-22 19:49 UTC (permalink / raw)
To: Kazlauskas, Nicholas, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
On 11/22/2018 02:43 PM, Kazlauskas, Nicholas wrote:
> On 11/22/18 2:39 PM, Grodzovsky, Andrey wrote:
>>
>> On 11/22/2018 12:34 PM, Nicholas Kazlauskas wrote:
>>> [Why]
>>> Atomic check can't truly be non-blocking if amdgpu_dm is waiting for
>>> hw_done and flip_done in atomic check. This introduces waits when
>>> any previous non-blocking commits queued work on a worker thread and
>>> a new atomic commit attempts to do another full update/modeset.
>>>
>>> [How]
>>> Drop the waits and move the global lock acqusition into atomic check.
>>>
>>> This is fine to do since commit tail waits for these dependencies
>>> before calling amdgpu_dm_atomic_commit_tail.
>> Note that drm_atomic_helper_wait_for_dependencies waits only on all
>> preceeding commits that touch the same CRTC
>> while our custom do_aquire_global_lock wait on ALL previous commits in
>> the system - even on other CRTCS. As far as I can
>> remember that was important because we have global dc_state context
>> which must be protected for access/modifications while
>> looks like DRM assumes unrelated CRTCs can be modified concurrently.
>> Try to verify that latest display code will not be broken by this
>> relaxation of synchronization.
>>
>> Andrey
> This is a good point.
>
> What we should really be doing instead here is locking anything that can
> modify dc->current_state from within atomic commit tail. Serializing
> commits into a queue could work too.
This is a possibility by then you have to duplicate
drm_atomic_helper_commit with only once change which
is to substitute system_unbound_wq with you costume WQ. Alternatively
propose a change to update drm_atomic_helper_commit
to allow WQs as parameter.
Andrey
>
> Either should fix non-blocking support.
>
> Nicholas Kazlauskas
>
>>> This is only safe as long as DC never queries anything from within
>>> current_state when doing validation in atomic_check. This is the
>>> case as of writing, but any future uses of dc->current_state from
>>> within atomic_check should be considered incorrect.
>>>
>>> Cc: Harry Wentland <harry.wentland@amd.com>
>>> Cc: Leo Li <sunpeng.li@amd.com>
>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>>> ---
>>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 58 ++-----------------
>>> 1 file changed, 6 insertions(+), 52 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 3ae438d9849f..fe21bb86b66a 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -5078,57 +5078,6 @@ void dm_restore_drm_connector_state(struct drm_device *dev,
>>> dm_force_atomic_commit(&aconnector->base);
>>> }
>>>
>>> -/*
>>> - * Grabs all modesetting locks to serialize against any blocking commits,
>>> - * Waits for completion of all non blocking commits.
>>> - */
>>> -static int do_aquire_global_lock(struct drm_device *dev,
>>> - struct drm_atomic_state *state)
>>> -{
>>> - struct drm_crtc *crtc;
>>> - struct drm_crtc_commit *commit;
>>> - long ret;
>>> -
>>> - /*
>>> - * Adding all modeset locks to aquire_ctx will
>>> - * ensure that when the framework release it the
>>> - * extra locks we are locking here will get released to
>>> - */
>>> - ret = drm_modeset_lock_all_ctx(dev, state->acquire_ctx);
>>> - if (ret)
>>> - return ret;
>>> -
>>> - list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>>> - spin_lock(&crtc->commit_lock);
>>> - commit = list_first_entry_or_null(&crtc->commit_list,
>>> - struct drm_crtc_commit, commit_entry);
>>> - if (commit)
>>> - drm_crtc_commit_get(commit);
>>> - spin_unlock(&crtc->commit_lock);
>>> -
>>> - if (!commit)
>>> - continue;
>>> -
>>> - /*
>>> - * Make sure all pending HW programming completed and
>>> - * page flips done
>>> - */
>>> - ret = wait_for_completion_interruptible_timeout(&commit->hw_done, 10*HZ);
>>> -
>>> - if (ret > 0)
>>> - ret = wait_for_completion_interruptible_timeout(
>>> - &commit->flip_done, 10*HZ);
>>> -
>>> - if (ret == 0)
>>> - DRM_ERROR("[CRTC:%d:%s] hw_done or flip_done "
>>> - "timed out\n", crtc->base.id, crtc->name);
>>> -
>>> - drm_crtc_commit_put(commit);
>>> - }
>>> -
>>> - return ret < 0 ? ret : 0;
>>> -}
>>> -
>>> void set_freesync_on_stream(struct amdgpu_display_manager *dm,
>>> struct dm_crtc_state *new_crtc_state,
>>> struct dm_connector_state *new_con_state,
>>> @@ -5793,7 +5742,12 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>>> if (ret)
>>> goto fail;
>>>
>>> - ret = do_aquire_global_lock(dev, state);
>>> + /*
>>> + * This should be replaced with finer locking on the
>>> + * on the appropriate resources when possible.
>>> + * For now it's safer to grab everything.
>>> + */
>>> + ret = drm_modeset_lock_all_ctx(dev, state->acquire_ctx);
>>> if (ret)
>>> goto fail;
>>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] drm/amd/display: Use private obj helpers for dm_atomic_state
[not found] ` <20181122173437.13538-1-nicholas.kazlauskas-5C7GfCeVMHo@public.gmane.org>
2018-11-22 17:34 ` [PATCH 2/2] drm/amd/display: Remove wait for hw/flip done in atomic check Nicholas Kazlauskas
@ 2018-11-27 21:20 ` Li, Sun peng (Leo)
[not found] ` <8d527989-117c-020f-ee05-dfc76aed3221-5C7GfCeVMHo@public.gmane.org>
1 sibling, 1 reply; 7+ messages in thread
From: Li, Sun peng (Leo) @ 2018-11-27 21:20 UTC (permalink / raw)
To: Kazlauskas, Nicholas, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Wentland, Harry
On 2018-11-22 12:34 p.m., Nicholas Kazlauskas wrote:
> [Why]
> Two non-blocking commits in succession can result in a sequence where
> the same dc->current_state is queried for both commits.
>
> 1. 1st commit -> check -> commit -> swaps atomic state -> queues work
> 2. 2nd commit -> check -> commit -> swaps atomic state -> queues work
> 3. 1st commit work finishes
>
> The issue with this sequence is that the same dc->current_state is
> read in both atomic checks. If the first commit modifies streams or
> planes those will be missing from the dc->current_state for the
> second atomic check. This result in many stream and plane errors in
> atomic commit tail.
>
> [How]
> The driver still needs to track old to new state to determine if the
> commit in its current implementation. Updating the dc_state in
> atomic tail is wrong since the dc_state swap should be happening as
> part of drm_atomic_helper_swap_state *before* the worker queue kicks
> its work off.
>
> The simplest replacement for the subclassing (which doesn't properly
> manage the old to new atomic state swap) is to use the drm private
> object helpers. While some of the dc_state members could be merged
> into dm_crtc_state or dm_plane_state and copied over that way it is
> easier for now to just treat the whole dc_state structure as a single
> private object.
>
> This allows amdgpu_dm to drop the dc->current_state copy from within
> atomic check. It's replaced by a copy from the current atomic state
> which is propagated correctly for the sequence described above.
>
> Since access to the dm_state private object is now locked this should
> also fix issues that could arise if submitting non-blocking commits
> from different threads.
>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Leo Li <sunpeng.li@amd.com>
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Patch 1/2 is
Reviewed-by: Leo Li <sunpeng.li@amd.com>
Leo
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 290 ++++++++++++++----
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 13 +-
> 2 files changed, 234 insertions(+), 69 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 8433d31cdea8..3ae438d9849f 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -968,45 +968,6 @@ const struct amdgpu_ip_block_version dm_ip_block =
> };
>
>
> -static struct drm_atomic_state *
> -dm_atomic_state_alloc(struct drm_device *dev)
> -{
> - struct dm_atomic_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
> -
> - if (!state)
> - return NULL;
> -
> - if (drm_atomic_state_init(dev, &state->base) < 0)
> - goto fail;
> -
> - return &state->base;
> -
> -fail:
> - kfree(state);
> - return NULL;
> -}
> -
> -static void
> -dm_atomic_state_clear(struct drm_atomic_state *state)
> -{
> - struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
> -
> - if (dm_state->context) {
> - dc_release_state(dm_state->context);
> - dm_state->context = NULL;
> - }
> -
> - drm_atomic_state_default_clear(state);
> -}
> -
> -static void
> -dm_atomic_state_alloc_free(struct drm_atomic_state *state)
> -{
> - struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
> - drm_atomic_state_default_release(state);
> - kfree(dm_state);
> -}
> -
> /**
> * DOC: atomic
> *
> @@ -1018,9 +979,6 @@ static const struct drm_mode_config_funcs amdgpu_dm_mode_funcs = {
> .output_poll_changed = drm_fb_helper_output_poll_changed,
> .atomic_check = amdgpu_dm_atomic_check,
> .atomic_commit = amdgpu_dm_atomic_commit,
> - .atomic_state_alloc = dm_atomic_state_alloc,
> - .atomic_state_clear = dm_atomic_state_clear,
> - .atomic_state_free = dm_atomic_state_alloc_free
> };
>
> static struct drm_mode_config_helper_funcs amdgpu_dm_mode_config_helperfuncs = {
> @@ -1542,8 +1500,117 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
> }
> #endif
>
> +/*
> + * Acquires the lock for the atomic state object and returns
> + * the new atomic state.
> + *
> + * This should only be called during atomic check.
> + */
> +static int dm_atomic_get_state(struct drm_atomic_state *state,
> + struct dm_atomic_state **dm_state)
> +{
> + struct drm_device *dev = state->dev;
> + struct amdgpu_device *adev = dev->dev_private;
> + struct amdgpu_display_manager *dm = &adev->dm;
> + struct drm_private_state *priv_state;
> + int ret;
> +
> + if (*dm_state)
> + return 0;
> +
> + ret = drm_modeset_lock(&dm->atomic_obj_lock, state->acquire_ctx);
> + if (ret)
> + return ret;
> +
> + priv_state = drm_atomic_get_private_obj_state(state, &dm->atomic_obj);
> + if (IS_ERR(priv_state))
> + return PTR_ERR(priv_state);
> +
> + *dm_state = to_dm_atomic_state(priv_state);
> +
> + return 0;
> +}
> +
> +struct dm_atomic_state *
> +dm_atomic_get_new_state(struct drm_atomic_state *state)
> +{
> + struct drm_device *dev = state->dev;
> + struct amdgpu_device *adev = dev->dev_private;
> + struct amdgpu_display_manager *dm = &adev->dm;
> + struct drm_private_obj *obj;
> + struct drm_private_state *new_obj_state;
> + int i;
> +
> + for_each_new_private_obj_in_state(state, obj, new_obj_state, i) {
> + if (obj->funcs == dm->atomic_obj.funcs)
> + return to_dm_atomic_state(new_obj_state);
> + }
> +
> + return NULL;
> +}
> +
> +struct dm_atomic_state *
> +dm_atomic_get_old_state(struct drm_atomic_state *state)
> +{
> + struct drm_device *dev = state->dev;
> + struct amdgpu_device *adev = dev->dev_private;
> + struct amdgpu_display_manager *dm = &adev->dm;
> + struct drm_private_obj *obj;
> + struct drm_private_state *old_obj_state;
> + int i;
> +
> + for_each_old_private_obj_in_state(state, obj, old_obj_state, i) {
> + if (obj->funcs == dm->atomic_obj.funcs)
> + return to_dm_atomic_state(old_obj_state);
> + }
> +
> + return NULL;
> +}
> +
> +static struct drm_private_state *
> +dm_atomic_duplicate_state(struct drm_private_obj *obj)
> +{
> + struct dm_atomic_state *old_state, *new_state;
> +
> + new_state = kzalloc(sizeof(*new_state), GFP_KERNEL);
> + if (!new_state)
> + return NULL;
> +
> + __drm_atomic_helper_private_obj_duplicate_state(obj, &new_state->base);
> +
> + new_state->context = dc_create_state();
> + if (!new_state->context) {
> + kfree(new_state);
> + return NULL;
> + }
> +
> + old_state = to_dm_atomic_state(obj->state);
> + if (old_state && old_state->context)
> + dc_resource_state_copy_construct(old_state->context,
> + new_state->context);
> +
> + return &new_state->base;
> +}
> +
> +static void dm_atomic_destroy_state(struct drm_private_obj *obj,
> + struct drm_private_state *state)
> +{
> + struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
> +
> + if (dm_state && dm_state->context)
> + dc_release_state(dm_state->context);
> +
> + kfree(dm_state);
> +}
> +
> +static struct drm_private_state_funcs dm_atomic_state_funcs = {
> + .atomic_duplicate_state = dm_atomic_duplicate_state,
> + .atomic_destroy_state = dm_atomic_destroy_state,
> +};
> +
> static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
> {
> + struct dm_atomic_state *state;
> int r;
>
> adev->mode_info.mode_config_initialized = true;
> @@ -1561,6 +1628,24 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
>
> adev->ddev->mode_config.fb_base = adev->gmc.aper_base;
>
> + drm_modeset_lock_init(&adev->dm.atomic_obj_lock);
> +
> + state = kzalloc(sizeof(*state), GFP_KERNEL);
> + if (!state)
> + return -ENOMEM;
> +
> + state->context = dc_create_state();
> + if (!state->context) {
> + kfree(state);
> + return -ENOMEM;
> + }
> +
> + dc_resource_state_copy_construct_current(adev->dm.dc, state->context);
> +
> + drm_atomic_private_obj_init(&adev->dm.atomic_obj,
> + &state->base,
> + &dm_atomic_state_funcs);
> +
> r = amdgpu_display_modeset_create_props(adev);
> if (r)
> return r;
> @@ -1849,6 +1934,7 @@ static int amdgpu_dm_initialize_drm_device(struct amdgpu_device *adev)
> static void amdgpu_dm_destroy_drm_device(struct amdgpu_display_manager *dm)
> {
> drm_mode_config_cleanup(dm->ddev);
> + drm_atomic_private_obj_fini(&dm->atomic_obj);
> return;
> }
>
> @@ -4274,6 +4360,20 @@ static void prepare_flip_isr(struct amdgpu_crtc *acrtc)
> acrtc->crtc_id);
> }
>
> +struct dc_stream_status *dc_state_get_stream_status(
> + struct dc_state *state,
> + struct dc_stream_state *stream)
> +{
> + uint8_t i;
> +
> + for (i = 0; i < state->stream_count; i++) {
> + if (stream == state->streams[i])
> + return &state->stream_status[i];
> + }
> +
> + return NULL;
> +}
> +
> /*
> * Executes flip
> *
> @@ -4477,6 +4577,7 @@ static bool commit_planes_to_stream(
> }
>
> static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
> + struct dc_state *dc_state,
> struct drm_device *dev,
> struct amdgpu_display_manager *dm,
> struct drm_crtc *pcrtc,
> @@ -4493,7 +4594,6 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
> struct dm_crtc_state *acrtc_state = to_dm_crtc_state(new_pcrtc_state);
> struct dm_crtc_state *dm_old_crtc_state =
> to_dm_crtc_state(drm_atomic_get_old_crtc_state(state, pcrtc));
> - struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
> int planes_count = 0;
> unsigned long flags;
>
> @@ -4554,7 +4654,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
> crtc,
> fb,
> (uint32_t)drm_crtc_vblank_count(crtc) + *wait_for_vblank,
> - dm_state->context);
> + dc_state);
> }
>
> }
> @@ -4579,7 +4679,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
> planes_count,
> acrtc_state,
> dm_old_crtc_state,
> - dm_state->context))
> + dc_state))
> dm_error("%s: Failed to attach plane!\n", __func__);
> } else {
> /*TODO BUG Here should go disable planes on CRTC. */
> @@ -4647,6 +4747,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
> struct amdgpu_device *adev = dev->dev_private;
> struct amdgpu_display_manager *dm = &adev->dm;
> struct dm_atomic_state *dm_state;
> + struct dc_state *dc_state = NULL, *dc_state_temp = NULL;
> uint32_t i, j;
> struct drm_crtc *crtc;
> struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> @@ -4659,7 +4760,16 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>
> drm_atomic_helper_update_legacy_modeset_state(dev, state);
>
> - dm_state = to_dm_atomic_state(state);
> + dm_state = dm_atomic_get_new_state(state);
> + if (dm_state && dm_state->context) {
> + dc_state = dm_state->context;
> + } else {
> + /* No state changes, retain current state. */
> + dc_state_temp = dc_create_state();
> + ASSERT(dc_state_temp);
> + dc_state = dc_state_temp;
> + dc_resource_state_copy_construct_current(dm->dc, dc_state);
> + }
>
> /* update changed items */
> for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> @@ -4732,9 +4842,9 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
> }
> } /* for_each_crtc_in_state() */
>
> - if (dm_state->context) {
> - dm_enable_per_frame_crtc_master_sync(dm_state->context);
> - WARN_ON(!dc_commit_state(dm->dc, dm_state->context));
> + if (dc_state) {
> + dm_enable_per_frame_crtc_master_sync(dc_state);
> + WARN_ON(!dc_commit_state(dm->dc, dc_state));
> }
>
> for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> @@ -4746,6 +4856,10 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
> const struct dc_stream_status *status =
> dc_stream_get_status(dm_new_crtc_state->stream);
>
> + if (!status)
> + status = dc_state_get_stream_status(dc_state,
> + dm_new_crtc_state->stream);
> +
> if (!status)
> DC_ERR("got no status for stream %p on acrtc%p\n", dm_new_crtc_state->stream, acrtc);
> else
> @@ -4828,7 +4942,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
> dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
>
> if (dm_new_crtc_state->stream)
> - amdgpu_dm_commit_planes(state, dev, dm, crtc, &wait_for_vblank);
> + amdgpu_dm_commit_planes(state, dc_state, dev,
> + dm, crtc, &wait_for_vblank);
> }
>
>
> @@ -4868,6 +4983,9 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
> for (i = 0; i < crtc_disable_count; i++)
> pm_runtime_put_autosuspend(dev->dev);
> pm_runtime_mark_last_busy(dev->dev);
> +
> + if (dc_state_temp)
> + dc_release_state(dc_state_temp);
> }
>
>
> @@ -5054,11 +5172,11 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
> 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 dm_atomic_state *dm_state = to_dm_atomic_state(state);
> struct dc_stream_state *new_stream;
> int ret = 0;
>
> @@ -5156,6 +5274,10 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
> if (!dm_old_crtc_state->stream)
> goto next_crtc;
>
> + ret = dm_atomic_get_state(state, &dm_state);
> + if (ret)
> + goto fail;
> +
> DRM_DEBUG_DRIVER("Disabling DRM crtc: %d\n",
> crtc->base.id);
>
> @@ -5190,6 +5312,10 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
>
> WARN_ON(dm_new_crtc_state->stream);
>
> + ret = dm_atomic_get_state(state, &dm_state);
> + if (ret)
> + goto fail;
> +
> dm_new_crtc_state->stream = new_stream;
>
> dc_stream_retain(new_stream);
> @@ -5264,12 +5390,13 @@ static int dm_update_planes_state(struct dc *dc,
> bool enable,
> bool *lock_and_validation_needed)
> {
> +
> + struct dm_atomic_state *dm_state = NULL;
> struct drm_crtc *new_plane_crtc, *old_plane_crtc;
> struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> struct drm_plane *plane;
> struct drm_plane_state *old_plane_state, *new_plane_state;
> struct dm_crtc_state *dm_new_crtc_state, *dm_old_crtc_state;
> - struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
> struct dm_plane_state *dm_new_plane_state, *dm_old_plane_state;
> int i ;
> /* TODO return page_flip_needed() function */
> @@ -5307,6 +5434,10 @@ static int dm_update_planes_state(struct dc *dc,
> DRM_DEBUG_ATOMIC("Disabling DRM plane: %d on DRM crtc %d\n",
> plane->base.id, old_plane_crtc->base.id);
>
> + ret = dm_atomic_get_state(state, &dm_state);
> + if (ret)
> + return ret;
> +
> if (!dc_remove_plane_from_context(
> dc,
> dm_old_crtc_state->stream,
> @@ -5361,6 +5492,12 @@ static int dm_update_planes_state(struct dc *dc,
> return ret;
> }
>
> + ret = dm_atomic_get_state(state, &dm_state);
> + if (ret) {
> + dc_plane_state_release(dc_new_plane_state);
> + return ret;
> + }
> +
> /*
> * Any atomic check errors that occur after this will
> * not need a release. The plane state will be attached
> @@ -5392,11 +5529,14 @@ static int dm_update_planes_state(struct dc *dc,
>
> return ret;
> }
> -enum surface_update_type dm_determine_update_type_for_commit(struct dc *dc, struct drm_atomic_state *state)
> -{
> -
>
> - int i, j, num_plane;
> +static int
> +dm_determine_update_type_for_commit(struct dc *dc,
> + struct drm_atomic_state *state,
> + enum surface_update_type *out_type)
> +{
> + 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, *old_plane_crtc;
> @@ -5416,7 +5556,7 @@ enum surface_update_type dm_determine_update_type_for_commit(struct dc *dc, stru
> DRM_ERROR("Plane or surface update failed to allocate");
> /* Set type to FULL to avoid crashing in DC*/
> update_type = UPDATE_TYPE_FULL;
> - goto ret;
> + goto cleanup;
> }
>
> for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> @@ -5470,27 +5610,40 @@ enum surface_update_type dm_determine_update_type_for_commit(struct dc *dc, stru
> }
>
> if (num_plane > 0) {
> - status = dc_stream_get_status(new_dm_crtc_state->stream);
> + 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_state_get_stream_status(old_dm_state->context,
> + new_dm_crtc_state->stream);
> +
> update_type = dc_check_update_surfaces_for_stream(dc, updates, num_plane,
> &stream_update, status);
>
> if (update_type > UPDATE_TYPE_MED) {
> update_type = UPDATE_TYPE_FULL;
> - goto ret;
> + goto cleanup;
> }
> }
>
> } else if (!new_dm_crtc_state->stream && old_dm_crtc_state->stream) {
> update_type = UPDATE_TYPE_FULL;
> - goto ret;
> + goto cleanup;
> }
> }
>
> -ret:
> +cleanup:
> kfree(updates);
> kfree(surface);
>
> - return update_type;
> + *out_type = update_type;
> + return ret;
> }
>
> /**
> @@ -5522,8 +5675,8 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
> struct drm_atomic_state *state)
> {
> struct amdgpu_device *adev = dev->dev_private;
> + struct dm_atomic_state *dm_state = NULL;
> struct dc *dc = adev->dm.dc;
> - struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
> struct drm_connector *connector;
> struct drm_connector_state *old_con_state, *new_con_state;
> struct drm_crtc *crtc;
> @@ -5564,10 +5717,6 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
> goto fail;
> }
>
> - dm_state->context = dc_create_state();
> - ASSERT(dm_state->context);
> - dc_resource_state_copy_construct_current(dc, dm_state->context);
> -
> /* Remove exiting planes if they are modified */
> ret = dm_update_planes_state(dc, state, false, &lock_and_validation_needed);
> if (ret) {
> @@ -5620,7 +5769,9 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
> lock_and_validation_needed = true;
> }
>
> - update_type = dm_determine_update_type_for_commit(dc, state);
> + ret = dm_determine_update_type_for_commit(dc, state, &update_type);
> + if (ret)
> + goto fail;
>
> if (overall_update_type < update_type)
> overall_update_type = update_type;
> @@ -5638,6 +5789,9 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>
>
> if (overall_update_type > UPDATE_TYPE_FAST) {
> + ret = dm_atomic_get_state(state, &dm_state);
> + if (ret)
> + goto fail;
>
> ret = do_aquire_global_lock(dev, state);
> if (ret)
> 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 607c3cdd7d0c..c8fd95950d0c 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -111,6 +111,17 @@ struct amdgpu_display_manager {
> struct drm_device *ddev;
> u16 display_indexes_num;
>
> + /**
> + * @atomic_obj
> + *
> + * In combination with &dm_atomic_state it helps manage
> + * global atomic state that doesn't map cleanly into existing
> + * drm resources, like &dc_context.
> + */
> + struct drm_private_obj atomic_obj;
> +
> + struct drm_modeset_lock atomic_obj_lock;
> +
> /**
> * @irq_handler_list_low_tab:
> *
> @@ -239,7 +250,7 @@ struct dm_crtc_state {
> #define to_dm_crtc_state(x) container_of(x, struct dm_crtc_state, base)
>
> struct dm_atomic_state {
> - struct drm_atomic_state base;
> + struct drm_private_state base;
>
> struct dc_state *context;
> };
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] drm/amd/display: Use private obj helpers for dm_atomic_state
[not found] ` <8d527989-117c-020f-ee05-dfc76aed3221-5C7GfCeVMHo@public.gmane.org>
@ 2018-11-27 21:26 ` Kazlauskas, Nicholas
0 siblings, 0 replies; 7+ messages in thread
From: Kazlauskas, Nicholas @ 2018-11-27 21:26 UTC (permalink / raw)
To: Li, Sun peng (Leo), amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Wentland, Harry
On 11/27/18 4:20 PM, Li, Sun peng (Leo) wrote:
>
>
> On 2018-11-22 12:34 p.m., Nicholas Kazlauskas wrote:
>> [Why]
>> Two non-blocking commits in succession can result in a sequence where
>> the same dc->current_state is queried for both commits.
>>
>> 1. 1st commit -> check -> commit -> swaps atomic state -> queues work
>> 2. 2nd commit -> check -> commit -> swaps atomic state -> queues work
>> 3. 1st commit work finishes
>>
>> The issue with this sequence is that the same dc->current_state is
>> read in both atomic checks. If the first commit modifies streams or
>> planes those will be missing from the dc->current_state for the
>> second atomic check. This result in many stream and plane errors in
>> atomic commit tail.
>>
>> [How]
>> The driver still needs to track old to new state to determine if the
>> commit in its current implementation. Updating the dc_state in
>> atomic tail is wrong since the dc_state swap should be happening as
>> part of drm_atomic_helper_swap_state *before* the worker queue kicks
>> its work off.
>>
>> The simplest replacement for the subclassing (which doesn't properly
>> manage the old to new atomic state swap) is to use the drm private
>> object helpers. While some of the dc_state members could be merged
>> into dm_crtc_state or dm_plane_state and copied over that way it is
>> easier for now to just treat the whole dc_state structure as a single
>> private object.
>>
>> This allows amdgpu_dm to drop the dc->current_state copy from within
>> atomic check. It's replaced by a copy from the current atomic state
>> which is propagated correctly for the sequence described above.
>>
>> Since access to the dm_state private object is now locked this should
>> also fix issues that could arise if submitting non-blocking commits
>> from different threads.
>>
>> Cc: Harry Wentland <harry.wentland@amd.com>
>> Cc: Leo Li <sunpeng.li@amd.com>
>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>
> Patch 1/2 is
> Reviewed-by: Leo Li <sunpeng.li@amd.com>
>
> Leo
Thanks for the review.
Since the second patch is actually independent of this one (they solve
different problems) I'm fine with pushing this one first and putting
some more thought into the second. I'm thinking that the locking
solution might be what we want in the end for that but it'll need some
more time.
Nicholas Kazlauskas
>> ---
>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 290 ++++++++++++++----
>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 13 +-
>> 2 files changed, 234 insertions(+), 69 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 8433d31cdea8..3ae438d9849f 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -968,45 +968,6 @@ const struct amdgpu_ip_block_version dm_ip_block =
>> };
>>
>>
>> -static struct drm_atomic_state *
>> -dm_atomic_state_alloc(struct drm_device *dev)
>> -{
>> - struct dm_atomic_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
>> -
>> - if (!state)
>> - return NULL;
>> -
>> - if (drm_atomic_state_init(dev, &state->base) < 0)
>> - goto fail;
>> -
>> - return &state->base;
>> -
>> -fail:
>> - kfree(state);
>> - return NULL;
>> -}
>> -
>> -static void
>> -dm_atomic_state_clear(struct drm_atomic_state *state)
>> -{
>> - struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
>> -
>> - if (dm_state->context) {
>> - dc_release_state(dm_state->context);
>> - dm_state->context = NULL;
>> - }
>> -
>> - drm_atomic_state_default_clear(state);
>> -}
>> -
>> -static void
>> -dm_atomic_state_alloc_free(struct drm_atomic_state *state)
>> -{
>> - struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
>> - drm_atomic_state_default_release(state);
>> - kfree(dm_state);
>> -}
>> -
>> /**
>> * DOC: atomic
>> *
>> @@ -1018,9 +979,6 @@ static const struct drm_mode_config_funcs amdgpu_dm_mode_funcs = {
>> .output_poll_changed = drm_fb_helper_output_poll_changed,
>> .atomic_check = amdgpu_dm_atomic_check,
>> .atomic_commit = amdgpu_dm_atomic_commit,
>> - .atomic_state_alloc = dm_atomic_state_alloc,
>> - .atomic_state_clear = dm_atomic_state_clear,
>> - .atomic_state_free = dm_atomic_state_alloc_free
>> };
>>
>> static struct drm_mode_config_helper_funcs amdgpu_dm_mode_config_helperfuncs = {
>> @@ -1542,8 +1500,117 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
>> }
>> #endif
>>
>> +/*
>> + * Acquires the lock for the atomic state object and returns
>> + * the new atomic state.
>> + *
>> + * This should only be called during atomic check.
>> + */
>> +static int dm_atomic_get_state(struct drm_atomic_state *state,
>> + struct dm_atomic_state **dm_state)
>> +{
>> + struct drm_device *dev = state->dev;
>> + struct amdgpu_device *adev = dev->dev_private;
>> + struct amdgpu_display_manager *dm = &adev->dm;
>> + struct drm_private_state *priv_state;
>> + int ret;
>> +
>> + if (*dm_state)
>> + return 0;
>> +
>> + ret = drm_modeset_lock(&dm->atomic_obj_lock, state->acquire_ctx);
>> + if (ret)
>> + return ret;
>> +
>> + priv_state = drm_atomic_get_private_obj_state(state, &dm->atomic_obj);
>> + if (IS_ERR(priv_state))
>> + return PTR_ERR(priv_state);
>> +
>> + *dm_state = to_dm_atomic_state(priv_state);
>> +
>> + return 0;
>> +}
>> +
>> +struct dm_atomic_state *
>> +dm_atomic_get_new_state(struct drm_atomic_state *state)
>> +{
>> + struct drm_device *dev = state->dev;
>> + struct amdgpu_device *adev = dev->dev_private;
>> + struct amdgpu_display_manager *dm = &adev->dm;
>> + struct drm_private_obj *obj;
>> + struct drm_private_state *new_obj_state;
>> + int i;
>> +
>> + for_each_new_private_obj_in_state(state, obj, new_obj_state, i) {
>> + if (obj->funcs == dm->atomic_obj.funcs)
>> + return to_dm_atomic_state(new_obj_state);
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +struct dm_atomic_state *
>> +dm_atomic_get_old_state(struct drm_atomic_state *state)
>> +{
>> + struct drm_device *dev = state->dev;
>> + struct amdgpu_device *adev = dev->dev_private;
>> + struct amdgpu_display_manager *dm = &adev->dm;
>> + struct drm_private_obj *obj;
>> + struct drm_private_state *old_obj_state;
>> + int i;
>> +
>> + for_each_old_private_obj_in_state(state, obj, old_obj_state, i) {
>> + if (obj->funcs == dm->atomic_obj.funcs)
>> + return to_dm_atomic_state(old_obj_state);
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +static struct drm_private_state *
>> +dm_atomic_duplicate_state(struct drm_private_obj *obj)
>> +{
>> + struct dm_atomic_state *old_state, *new_state;
>> +
>> + new_state = kzalloc(sizeof(*new_state), GFP_KERNEL);
>> + if (!new_state)
>> + return NULL;
>> +
>> + __drm_atomic_helper_private_obj_duplicate_state(obj, &new_state->base);
>> +
>> + new_state->context = dc_create_state();
>> + if (!new_state->context) {
>> + kfree(new_state);
>> + return NULL;
>> + }
>> +
>> + old_state = to_dm_atomic_state(obj->state);
>> + if (old_state && old_state->context)
>> + dc_resource_state_copy_construct(old_state->context,
>> + new_state->context);
>> +
>> + return &new_state->base;
>> +}
>> +
>> +static void dm_atomic_destroy_state(struct drm_private_obj *obj,
>> + struct drm_private_state *state)
>> +{
>> + struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
>> +
>> + if (dm_state && dm_state->context)
>> + dc_release_state(dm_state->context);
>> +
>> + kfree(dm_state);
>> +}
>> +
>> +static struct drm_private_state_funcs dm_atomic_state_funcs = {
>> + .atomic_duplicate_state = dm_atomic_duplicate_state,
>> + .atomic_destroy_state = dm_atomic_destroy_state,
>> +};
>> +
>> static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
>> {
>> + struct dm_atomic_state *state;
>> int r;
>>
>> adev->mode_info.mode_config_initialized = true;
>> @@ -1561,6 +1628,24 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
>>
>> adev->ddev->mode_config.fb_base = adev->gmc.aper_base;
>>
>> + drm_modeset_lock_init(&adev->dm.atomic_obj_lock);
>> +
>> + state = kzalloc(sizeof(*state), GFP_KERNEL);
>> + if (!state)
>> + return -ENOMEM;
>> +
>> + state->context = dc_create_state();
>> + if (!state->context) {
>> + kfree(state);
>> + return -ENOMEM;
>> + }
>> +
>> + dc_resource_state_copy_construct_current(adev->dm.dc, state->context);
>> +
>> + drm_atomic_private_obj_init(&adev->dm.atomic_obj,
>> + &state->base,
>> + &dm_atomic_state_funcs);
>> +
>> r = amdgpu_display_modeset_create_props(adev);
>> if (r)
>> return r;
>> @@ -1849,6 +1934,7 @@ static int amdgpu_dm_initialize_drm_device(struct amdgpu_device *adev)
>> static void amdgpu_dm_destroy_drm_device(struct amdgpu_display_manager *dm)
>> {
>> drm_mode_config_cleanup(dm->ddev);
>> + drm_atomic_private_obj_fini(&dm->atomic_obj);
>> return;
>> }
>>
>> @@ -4274,6 +4360,20 @@ static void prepare_flip_isr(struct amdgpu_crtc *acrtc)
>> acrtc->crtc_id);
>> }
>>
>> +struct dc_stream_status *dc_state_get_stream_status(
>> + struct dc_state *state,
>> + struct dc_stream_state *stream)
>> +{
>> + uint8_t i;
>> +
>> + for (i = 0; i < state->stream_count; i++) {
>> + if (stream == state->streams[i])
>> + return &state->stream_status[i];
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> /*
>> * Executes flip
>> *
>> @@ -4477,6 +4577,7 @@ static bool commit_planes_to_stream(
>> }
>>
>> static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>> + struct dc_state *dc_state,
>> struct drm_device *dev,
>> struct amdgpu_display_manager *dm,
>> struct drm_crtc *pcrtc,
>> @@ -4493,7 +4594,6 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>> struct dm_crtc_state *acrtc_state = to_dm_crtc_state(new_pcrtc_state);
>> struct dm_crtc_state *dm_old_crtc_state =
>> to_dm_crtc_state(drm_atomic_get_old_crtc_state(state, pcrtc));
>> - struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
>> int planes_count = 0;
>> unsigned long flags;
>>
>> @@ -4554,7 +4654,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>> crtc,
>> fb,
>> (uint32_t)drm_crtc_vblank_count(crtc) + *wait_for_vblank,
>> - dm_state->context);
>> + dc_state);
>> }
>>
>> }
>> @@ -4579,7 +4679,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>> planes_count,
>> acrtc_state,
>> dm_old_crtc_state,
>> - dm_state->context))
>> + dc_state))
>> dm_error("%s: Failed to attach plane!\n", __func__);
>> } else {
>> /*TODO BUG Here should go disable planes on CRTC. */
>> @@ -4647,6 +4747,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>> struct amdgpu_device *adev = dev->dev_private;
>> struct amdgpu_display_manager *dm = &adev->dm;
>> struct dm_atomic_state *dm_state;
>> + struct dc_state *dc_state = NULL, *dc_state_temp = NULL;
>> uint32_t i, j;
>> struct drm_crtc *crtc;
>> struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>> @@ -4659,7 +4760,16 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>>
>> drm_atomic_helper_update_legacy_modeset_state(dev, state);
>>
>> - dm_state = to_dm_atomic_state(state);
>> + dm_state = dm_atomic_get_new_state(state);
>> + if (dm_state && dm_state->context) {
>> + dc_state = dm_state->context;
>> + } else {
>> + /* No state changes, retain current state. */
>> + dc_state_temp = dc_create_state();
>> + ASSERT(dc_state_temp);
>> + dc_state = dc_state_temp;
>> + dc_resource_state_copy_construct_current(dm->dc, dc_state);
>> + }
>>
>> /* update changed items */
>> for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>> @@ -4732,9 +4842,9 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>> }
>> } /* for_each_crtc_in_state() */
>>
>> - if (dm_state->context) {
>> - dm_enable_per_frame_crtc_master_sync(dm_state->context);
>> - WARN_ON(!dc_commit_state(dm->dc, dm_state->context));
>> + if (dc_state) {
>> + dm_enable_per_frame_crtc_master_sync(dc_state);
>> + WARN_ON(!dc_commit_state(dm->dc, dc_state));
>> }
>>
>> for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
>> @@ -4746,6 +4856,10 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>> const struct dc_stream_status *status =
>> dc_stream_get_status(dm_new_crtc_state->stream);
>>
>> + if (!status)
>> + status = dc_state_get_stream_status(dc_state,
>> + dm_new_crtc_state->stream);
>> +
>> if (!status)
>> DC_ERR("got no status for stream %p on acrtc%p\n", dm_new_crtc_state->stream, acrtc);
>> else
>> @@ -4828,7 +4942,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>> dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
>>
>> if (dm_new_crtc_state->stream)
>> - amdgpu_dm_commit_planes(state, dev, dm, crtc, &wait_for_vblank);
>> + amdgpu_dm_commit_planes(state, dc_state, dev,
>> + dm, crtc, &wait_for_vblank);
>> }
>>
>>
>> @@ -4868,6 +4983,9 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>> for (i = 0; i < crtc_disable_count; i++)
>> pm_runtime_put_autosuspend(dev->dev);
>> pm_runtime_mark_last_busy(dev->dev);
>> +
>> + if (dc_state_temp)
>> + dc_release_state(dc_state_temp);
>> }
>>
>>
>> @@ -5054,11 +5172,11 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
>> 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 dm_atomic_state *dm_state = to_dm_atomic_state(state);
>> struct dc_stream_state *new_stream;
>> int ret = 0;
>>
>> @@ -5156,6 +5274,10 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
>> if (!dm_old_crtc_state->stream)
>> goto next_crtc;
>>
>> + ret = dm_atomic_get_state(state, &dm_state);
>> + if (ret)
>> + goto fail;
>> +
>> DRM_DEBUG_DRIVER("Disabling DRM crtc: %d\n",
>> crtc->base.id);
>>
>> @@ -5190,6 +5312,10 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
>>
>> WARN_ON(dm_new_crtc_state->stream);
>>
>> + ret = dm_atomic_get_state(state, &dm_state);
>> + if (ret)
>> + goto fail;
>> +
>> dm_new_crtc_state->stream = new_stream;
>>
>> dc_stream_retain(new_stream);
>> @@ -5264,12 +5390,13 @@ static int dm_update_planes_state(struct dc *dc,
>> bool enable,
>> bool *lock_and_validation_needed)
>> {
>> +
>> + struct dm_atomic_state *dm_state = NULL;
>> struct drm_crtc *new_plane_crtc, *old_plane_crtc;
>> struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>> struct drm_plane *plane;
>> struct drm_plane_state *old_plane_state, *new_plane_state;
>> struct dm_crtc_state *dm_new_crtc_state, *dm_old_crtc_state;
>> - struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
>> struct dm_plane_state *dm_new_plane_state, *dm_old_plane_state;
>> int i ;
>> /* TODO return page_flip_needed() function */
>> @@ -5307,6 +5434,10 @@ static int dm_update_planes_state(struct dc *dc,
>> DRM_DEBUG_ATOMIC("Disabling DRM plane: %d on DRM crtc %d\n",
>> plane->base.id, old_plane_crtc->base.id);
>>
>> + ret = dm_atomic_get_state(state, &dm_state);
>> + if (ret)
>> + return ret;
>> +
>> if (!dc_remove_plane_from_context(
>> dc,
>> dm_old_crtc_state->stream,
>> @@ -5361,6 +5492,12 @@ static int dm_update_planes_state(struct dc *dc,
>> return ret;
>> }
>>
>> + ret = dm_atomic_get_state(state, &dm_state);
>> + if (ret) {
>> + dc_plane_state_release(dc_new_plane_state);
>> + return ret;
>> + }
>> +
>> /*
>> * Any atomic check errors that occur after this will
>> * not need a release. The plane state will be attached
>> @@ -5392,11 +5529,14 @@ static int dm_update_planes_state(struct dc *dc,
>>
>> return ret;
>> }
>> -enum surface_update_type dm_determine_update_type_for_commit(struct dc *dc, struct drm_atomic_state *state)
>> -{
>> -
>>
>> - int i, j, num_plane;
>> +static int
>> +dm_determine_update_type_for_commit(struct dc *dc,
>> + struct drm_atomic_state *state,
>> + enum surface_update_type *out_type)
>> +{
>> + 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, *old_plane_crtc;
>> @@ -5416,7 +5556,7 @@ enum surface_update_type dm_determine_update_type_for_commit(struct dc *dc, stru
>> DRM_ERROR("Plane or surface update failed to allocate");
>> /* Set type to FULL to avoid crashing in DC*/
>> update_type = UPDATE_TYPE_FULL;
>> - goto ret;
>> + goto cleanup;
>> }
>>
>> for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>> @@ -5470,27 +5610,40 @@ enum surface_update_type dm_determine_update_type_for_commit(struct dc *dc, stru
>> }
>>
>> if (num_plane > 0) {
>> - status = dc_stream_get_status(new_dm_crtc_state->stream);
>> + 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_state_get_stream_status(old_dm_state->context,
>> + new_dm_crtc_state->stream);
>> +
>> update_type = dc_check_update_surfaces_for_stream(dc, updates, num_plane,
>> &stream_update, status);
>>
>> if (update_type > UPDATE_TYPE_MED) {
>> update_type = UPDATE_TYPE_FULL;
>> - goto ret;
>> + goto cleanup;
>> }
>> }
>>
>> } else if (!new_dm_crtc_state->stream && old_dm_crtc_state->stream) {
>> update_type = UPDATE_TYPE_FULL;
>> - goto ret;
>> + goto cleanup;
>> }
>> }
>>
>> -ret:
>> +cleanup:
>> kfree(updates);
>> kfree(surface);
>>
>> - return update_type;
>> + *out_type = update_type;
>> + return ret;
>> }
>>
>> /**
>> @@ -5522,8 +5675,8 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>> struct drm_atomic_state *state)
>> {
>> struct amdgpu_device *adev = dev->dev_private;
>> + struct dm_atomic_state *dm_state = NULL;
>> struct dc *dc = adev->dm.dc;
>> - struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
>> struct drm_connector *connector;
>> struct drm_connector_state *old_con_state, *new_con_state;
>> struct drm_crtc *crtc;
>> @@ -5564,10 +5717,6 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>> goto fail;
>> }
>>
>> - dm_state->context = dc_create_state();
>> - ASSERT(dm_state->context);
>> - dc_resource_state_copy_construct_current(dc, dm_state->context);
>> -
>> /* Remove exiting planes if they are modified */
>> ret = dm_update_planes_state(dc, state, false, &lock_and_validation_needed);
>> if (ret) {
>> @@ -5620,7 +5769,9 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>> lock_and_validation_needed = true;
>> }
>>
>> - update_type = dm_determine_update_type_for_commit(dc, state);
>> + ret = dm_determine_update_type_for_commit(dc, state, &update_type);
>> + if (ret)
>> + goto fail;
>>
>> if (overall_update_type < update_type)
>> overall_update_type = update_type;
>> @@ -5638,6 +5789,9 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>>
>>
>> if (overall_update_type > UPDATE_TYPE_FAST) {
>> + ret = dm_atomic_get_state(state, &dm_state);
>> + if (ret)
>> + goto fail;
>>
>> ret = do_aquire_global_lock(dev, state);
>> if (ret)
>> 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 607c3cdd7d0c..c8fd95950d0c 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> @@ -111,6 +111,17 @@ struct amdgpu_display_manager {
>> struct drm_device *ddev;
>> u16 display_indexes_num;
>>
>> + /**
>> + * @atomic_obj
>> + *
>> + * In combination with &dm_atomic_state it helps manage
>> + * global atomic state that doesn't map cleanly into existing
>> + * drm resources, like &dc_context.
>> + */
>> + struct drm_private_obj atomic_obj;
>> +
>> + struct drm_modeset_lock atomic_obj_lock;
>> +
>> /**
>> * @irq_handler_list_low_tab:
>> *
>> @@ -239,7 +250,7 @@ struct dm_crtc_state {
>> #define to_dm_crtc_state(x) container_of(x, struct dm_crtc_state, base)
>>
>> struct dm_atomic_state {
>> - struct drm_atomic_state base;
>> + struct drm_private_state base;
>>
>> struct dc_state *context;
>> };
>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-11-27 21:26 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-22 17:34 [PATCH 1/2] drm/amd/display: Use private obj helpers for dm_atomic_state Nicholas Kazlauskas
[not found] ` <20181122173437.13538-1-nicholas.kazlauskas-5C7GfCeVMHo@public.gmane.org>
2018-11-22 17:34 ` [PATCH 2/2] drm/amd/display: Remove wait for hw/flip done in atomic check Nicholas Kazlauskas
[not found] ` <20181122173437.13538-2-nicholas.kazlauskas-5C7GfCeVMHo@public.gmane.org>
2018-11-22 19:39 ` Grodzovsky, Andrey
[not found] ` <9b4c08d4-4aac-02dc-ce86-ae7152ae67c0-5C7GfCeVMHo@public.gmane.org>
2018-11-22 19:43 ` Kazlauskas, Nicholas
[not found] ` <66d5f064-61fc-83d1-bb4a-2e0227dca826-5C7GfCeVMHo@public.gmane.org>
2018-11-22 19:49 ` Grodzovsky, Andrey
2018-11-27 21:20 ` [PATCH 1/2] drm/amd/display: Use private obj helpers for dm_atomic_state Li, Sun peng (Leo)
[not found] ` <8d527989-117c-020f-ee05-dfc76aed3221-5C7GfCeVMHo@public.gmane.org>
2018-11-27 21:26 ` Kazlauskas, Nicholas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.