Re: [PATCH 1/2] drm/amd/display: Use private obj helpers for dm_atomic_state
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
>> Cc: Leo Li
>> Signed-off-by: Nicholas Kazlauskas
>
> Patch 1/2 is
> Reviewed-by: Leo Li
>
> 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, >base) < 0)
>> -goto fail;
>> -
>> -return >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
Re: [PATCH 1/2] drm/amd/display: Use private obj helpers for dm_atomic_state
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
> Cc: Leo Li
> Signed-off-by: Nicholas Kazlauskas
Patch 1/2 is
Reviewed-by: Leo Li
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, >base) < 0)
> - goto fail;
> -
> - return >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 = >dm;
> + struct drm_private_state *priv_state;
> + int ret;
> +
> + if (*dm_state)
> + return 0;
> +
> + ret = drm_modeset_lock(>atomic_obj_lock, state->acquire_ctx);
> + if (ret)
> + return ret;
> +
> + priv_state = drm_atomic_get_private_obj_state(state, >atomic_obj);
> + if (IS_ERR(priv_state))
> + return PTR_ERR(priv_state);
> +
> + *dm_state = to_dm_atomic_state(priv_state);
> +
> + return 0;
> +}
> +
> +struct
[PATCH 1/2] drm/amd/display: Use private obj helpers for dm_atomic_state
[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
Cc: Leo Li
Signed-off-by: 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, >base) < 0)
- goto fail;
-
- return >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 = >dm;
+ struct drm_private_state *priv_state;
+ int ret;
+
+ if (*dm_state)
+ return 0;
+
+ ret = drm_modeset_lock(>atomic_obj_lock, state->acquire_ctx);
+ if (ret)
+ return ret;
+
+ priv_state = drm_atomic_get_private_obj_state(state, >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 = >dm;
+ struct drm_private_obj *obj;
+ struct drm_private_state *new_obj_state;
+
