Re: [PATCH 3/7] drm/amd/display: Use vblank control events for PSR enable/disable
On 2021-09-04 10:36 a.m., Mike Lothian wrote:
Hi
This patch is causing issues on my PRIME system
I've opened
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1700&data=04%7C01%7Cnicholas.kazlauskas%40amd.com%7Cd230db90a08d4b53011508d96fb15eb4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637663629862006687%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=fH8mvhyHDchXqxZ5dlsyp7KqrzxkuymV%2BHtBmzVUD3I%3D&reserved=0
to track
Cheers
Mike
We don't create the workqueue on headless configs so I guess all the
instances of flush need to be guarded with NULL checks first.
Thanks for reporting this!
Regards,
Nicholas Kazlauskas
On Fri, 13 Aug 2021 at 07:35, Wayne Lin wrote:
From: Nicholas Kazlauskas
[Why]
PSR can disable the HUBP along with the OTG when PSR is active.
We'll hit a pageflip timeout when the OTG is disable because we're no
longer updating the CRTC vblank counter and the pflip high IRQ will
not fire on the flip.
In order to flip the page flip timeout occur we should modify the
enter/exit conditions to match DRM requirements.
[How]
Use our deferred handlers for DRM vblank control to notify DMCU(B)
when it can enable or disable PSR based on whether vblank is disabled or
enabled respectively.
We'll need to pass along the stream with the notification now because
we want to access the CRTC state while the CRTC is locked to get the
stream state prior to the commit.
Retain a reference to the stream so it remains safe to continue to
access and release that reference once we're done with it.
Enable/disable logic follows what we were previously doing in
update_planes.
The workqueue has to be flushed before programming streams or planes
to ensure that we exit out of idle optimizations and PSR before
these events occur if necessary.
To keep the skip count logic the same to avoid FBCON PSR enablement
requires copying the allow condition onto the DM IRQ parameters - a
field that we can actually access from the worker.
Reviewed-by: Roman Li
Acked-by: Wayne Lin
Signed-off-by: Nicholas Kazlauskas
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 48 +++
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 2 +
.../display/amdgpu_dm/amdgpu_dm_irq_params.h | 1 +
3 files changed, 43 insertions(+), 8 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 f88b6c5b83cd..cebd663b6708 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1061,7 +1061,22 @@ static void vblank_control_worker(struct work_struct
*work)
DRM_DEBUG_KMS("Allow idle optimizations (MALL): %d\n",
dm->active_vblank_irq_count == 0);
+ /* Control PSR based on vblank requirements from OS */
+ if (vblank_work->stream && vblank_work->stream->link) {
+ if (vblank_work->enable) {
+ if
(vblank_work->stream->link->psr_settings.psr_allow_active)
+ amdgpu_dm_psr_disable(vblank_work->stream);
+ } else if (vblank_work->stream->link->psr_settings.psr_feature_enabled
&&
+ !vblank_work->stream->link->psr_settings.psr_allow_active
&&
+ vblank_work->acrtc->dm_irq_params.allow_psr_entry) {
+ amdgpu_dm_psr_enable(vblank_work->stream);
+ }
+ }
+
mutex_unlock(&dm->dc_lock);
+
+ dc_stream_release(vblank_work->stream);
+
kfree(vblank_work);
}
@@ -6018,6 +6033,11 @@ static inline int dm_set_vblank(struct drm_crtc *crtc,
bool enable)
work->acrtc = acrtc;
work->enable = enable;
+ if (acrtc_state->stream) {
+ dc_stream_retain(acrtc_state->stream);
+ work->stream = acrtc_state->stream;
+ }
+
queue_work(dm->vblank_control_workqueue, &work->work);
#endif
@@ -8623,6 +8643,12 @@ static void amdgpu_dm_commit_planes(struct
drm_atomic_state *state,
/* Update the planes if changed or disable if we don't have any. */
if ((planes_count || acrtc_state->active_planes == 0) &&
acrtc_state->stream) {
+ /*
+* If PSR or idle optimizations are enabled then flush out
+* any pending work before hardware programming.
+*/
+ flush_workqueue(dm->vblank_control_workqueue);
+
bundle->stream_update.stream = acrtc_state->stream;
if (new_pcrtc_state->mode_changed) {
bundle->stream_update.src = acrtc_state->stream->src;
@@ -8691,16 +8717,20 @@ static void amdgpu_dm_commit_planes(struct
drm_atomic_state *state,
acrtc_state->stream->link->psr_settings.psr_version !=
DC_PSR_VERSION_UNSUPPORTED &&
Re: [PATCH 3/7] drm/amd/display: Use vblank control events for PSR enable/disable
Hi
This patch is causing issues on my PRIME system
I've opened https://gitlab.freedesktop.org/drm/amd/-/issues/1700 to track
Cheers
Mike
On Fri, 13 Aug 2021 at 07:35, Wayne Lin wrote:
>
> From: Nicholas Kazlauskas
>
> [Why]
> PSR can disable the HUBP along with the OTG when PSR is active.
>
> We'll hit a pageflip timeout when the OTG is disable because we're no
> longer updating the CRTC vblank counter and the pflip high IRQ will
> not fire on the flip.
>
> In order to flip the page flip timeout occur we should modify the
> enter/exit conditions to match DRM requirements.
>
> [How]
> Use our deferred handlers for DRM vblank control to notify DMCU(B)
> when it can enable or disable PSR based on whether vblank is disabled or
> enabled respectively.
>
> We'll need to pass along the stream with the notification now because
> we want to access the CRTC state while the CRTC is locked to get the
> stream state prior to the commit.
>
> Retain a reference to the stream so it remains safe to continue to
> access and release that reference once we're done with it.
>
> Enable/disable logic follows what we were previously doing in
> update_planes.
>
> The workqueue has to be flushed before programming streams or planes
> to ensure that we exit out of idle optimizations and PSR before
> these events occur if necessary.
>
> To keep the skip count logic the same to avoid FBCON PSR enablement
> requires copying the allow condition onto the DM IRQ parameters - a
> field that we can actually access from the worker.
>
> Reviewed-by: Roman Li
> Acked-by: Wayne Lin
> Signed-off-by: Nicholas Kazlauskas
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 48 +++
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 2 +
> .../display/amdgpu_dm/amdgpu_dm_irq_params.h | 1 +
> 3 files changed, 43 insertions(+), 8 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 f88b6c5b83cd..cebd663b6708 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -1061,7 +1061,22 @@ static void vblank_control_worker(struct work_struct
> *work)
>
> DRM_DEBUG_KMS("Allow idle optimizations (MALL): %d\n",
> dm->active_vblank_irq_count == 0);
>
> + /* Control PSR based on vblank requirements from OS */
> + if (vblank_work->stream && vblank_work->stream->link) {
> + if (vblank_work->enable) {
> + if
> (vblank_work->stream->link->psr_settings.psr_allow_active)
> + amdgpu_dm_psr_disable(vblank_work->stream);
> + } else if
> (vblank_work->stream->link->psr_settings.psr_feature_enabled &&
> +
> !vblank_work->stream->link->psr_settings.psr_allow_active &&
> + vblank_work->acrtc->dm_irq_params.allow_psr_entry)
> {
> + amdgpu_dm_psr_enable(vblank_work->stream);
> + }
> + }
> +
> mutex_unlock(&dm->dc_lock);
> +
> + dc_stream_release(vblank_work->stream);
> +
> kfree(vblank_work);
> }
>
> @@ -6018,6 +6033,11 @@ static inline int dm_set_vblank(struct drm_crtc *crtc,
> bool enable)
> work->acrtc = acrtc;
> work->enable = enable;
>
> + if (acrtc_state->stream) {
> + dc_stream_retain(acrtc_state->stream);
> + work->stream = acrtc_state->stream;
> + }
> +
> queue_work(dm->vblank_control_workqueue, &work->work);
> #endif
>
> @@ -8623,6 +8643,12 @@ static void amdgpu_dm_commit_planes(struct
> drm_atomic_state *state,
> /* Update the planes if changed or disable if we don't have any. */
> if ((planes_count || acrtc_state->active_planes == 0) &&
> acrtc_state->stream) {
> + /*
> +* If PSR or idle optimizations are enabled then flush out
> +* any pending work before hardware programming.
> +*/
> + flush_workqueue(dm->vblank_control_workqueue);
> +
> bundle->stream_update.stream = acrtc_state->stream;
> if (new_pcrtc_state->mode_changed) {
> bundle->stream_update.src = acrtc_state->stream->src;
> @@ -8691,16 +8717,20 @@ static void amdgpu_dm_commit_planes(struct
> drm_atomic_state *state,
>
> acrtc_state->stream->link->psr_settings.psr_version !=
> DC_PSR_VERSION_UNSUPPORTED &&
>
> !acrtc_state->stream->link->psr_settings.psr_feature_enabled)
> amdgpu_dm_link_setup_psr(acrtc_state->stream);
> - else if ((acrtc_state->update_type == UPDATE_TYPE_FAST) &&
> -
> acrtc_state->stream->link->psr_settings.psr_feature_enabled &&
> -
> !acrtc_state->stream->link->psr_settings.psr_allow_
[PATCH 3/7] drm/amd/display: Use vblank control events for PSR enable/disable
From: Nicholas Kazlauskas
[Why]
PSR can disable the HUBP along with the OTG when PSR is active.
We'll hit a pageflip timeout when the OTG is disable because we're no
longer updating the CRTC vblank counter and the pflip high IRQ will
not fire on the flip.
In order to flip the page flip timeout occur we should modify the
enter/exit conditions to match DRM requirements.
[How]
Use our deferred handlers for DRM vblank control to notify DMCU(B)
when it can enable or disable PSR based on whether vblank is disabled or
enabled respectively.
We'll need to pass along the stream with the notification now because
we want to access the CRTC state while the CRTC is locked to get the
stream state prior to the commit.
Retain a reference to the stream so it remains safe to continue to
access and release that reference once we're done with it.
Enable/disable logic follows what we were previously doing in
update_planes.
The workqueue has to be flushed before programming streams or planes
to ensure that we exit out of idle optimizations and PSR before
these events occur if necessary.
To keep the skip count logic the same to avoid FBCON PSR enablement
requires copying the allow condition onto the DM IRQ parameters - a
field that we can actually access from the worker.
Reviewed-by: Roman Li
Acked-by: Wayne Lin
Signed-off-by: Nicholas Kazlauskas
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 48 +++
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 2 +
.../display/amdgpu_dm/amdgpu_dm_irq_params.h | 1 +
3 files changed, 43 insertions(+), 8 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 f88b6c5b83cd..cebd663b6708 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1061,7 +1061,22 @@ static void vblank_control_worker(struct work_struct
*work)
DRM_DEBUG_KMS("Allow idle optimizations (MALL): %d\n",
dm->active_vblank_irq_count == 0);
+ /* Control PSR based on vblank requirements from OS */
+ if (vblank_work->stream && vblank_work->stream->link) {
+ if (vblank_work->enable) {
+ if
(vblank_work->stream->link->psr_settings.psr_allow_active)
+ amdgpu_dm_psr_disable(vblank_work->stream);
+ } else if
(vblank_work->stream->link->psr_settings.psr_feature_enabled &&
+
!vblank_work->stream->link->psr_settings.psr_allow_active &&
+ vblank_work->acrtc->dm_irq_params.allow_psr_entry) {
+ amdgpu_dm_psr_enable(vblank_work->stream);
+ }
+ }
+
mutex_unlock(&dm->dc_lock);
+
+ dc_stream_release(vblank_work->stream);
+
kfree(vblank_work);
}
@@ -6018,6 +6033,11 @@ static inline int dm_set_vblank(struct drm_crtc *crtc,
bool enable)
work->acrtc = acrtc;
work->enable = enable;
+ if (acrtc_state->stream) {
+ dc_stream_retain(acrtc_state->stream);
+ work->stream = acrtc_state->stream;
+ }
+
queue_work(dm->vblank_control_workqueue, &work->work);
#endif
@@ -8623,6 +8643,12 @@ static void amdgpu_dm_commit_planes(struct
drm_atomic_state *state,
/* Update the planes if changed or disable if we don't have any. */
if ((planes_count || acrtc_state->active_planes == 0) &&
acrtc_state->stream) {
+ /*
+* If PSR or idle optimizations are enabled then flush out
+* any pending work before hardware programming.
+*/
+ flush_workqueue(dm->vblank_control_workqueue);
+
bundle->stream_update.stream = acrtc_state->stream;
if (new_pcrtc_state->mode_changed) {
bundle->stream_update.src = acrtc_state->stream->src;
@@ -8691,16 +8717,20 @@ static void amdgpu_dm_commit_planes(struct
drm_atomic_state *state,
acrtc_state->stream->link->psr_settings.psr_version !=
DC_PSR_VERSION_UNSUPPORTED &&
!acrtc_state->stream->link->psr_settings.psr_feature_enabled)
amdgpu_dm_link_setup_psr(acrtc_state->stream);
- else if ((acrtc_state->update_type == UPDATE_TYPE_FAST) &&
-
acrtc_state->stream->link->psr_settings.psr_feature_enabled &&
-
!acrtc_state->stream->link->psr_settings.psr_allow_active) {
- struct amdgpu_dm_connector *aconn = (struct
amdgpu_dm_connector *)
- acrtc_state->stream->dm_stream_context;
+
+ /* Decrement skip count when PSR is enabled and we're doing
fast updates. */
+ if (acrtc_state->update_type == UPDATE_TYPE_FAST &&
+
acrtc_state->stream->link->psr_settings.ps
[PATCH 3/7] drm/amd/display: Use vblank control events for PSR enable/disable
From: Nicholas Kazlauskas
[Why]
PSR can disable the HUBP along with the OTG when PSR is active.
We'll hit a pageflip timeout when the OTG is disable because we're no
longer updating the CRTC vblank counter and the pflip high IRQ will
not fire on the flip.
In order to flip the page flip timeout occur we should modify the
enter/exit conditions to match DRM requirements.
[How]
Use our deferred handlers for DRM vblank control to notify DMCU(B)
when it can enable or disable PSR based on whether vblank is disabled or
enabled respectively.
We'll need to pass along the stream with the notification now because
we want to access the CRTC state while the CRTC is locked to get the
stream state prior to the commit.
Retain a reference to the stream so it remains safe to continue to
access and release that reference once we're done with it.
Enable/disable logic follows what we were previously doing in
update_planes.
The workqueue has to be flushed before programming streams or planes
to ensure that we exit out of idle optimizations and PSR before
these events occur if necessary.
To keep the skip count logic the same to avoid FBCON PSR enablement
requires copying the allow condition onto the DM IRQ parameters - a
field that we can actually access from the worker.
Reviewed-by: Roman Li
Acked-by: Wayne Lin
Signed-off-by: Nicholas Kazlauskas
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 48 +++
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 2 +
.../display/amdgpu_dm/amdgpu_dm_irq_params.h | 1 +
3 files changed, 43 insertions(+), 8 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 f88b6c5b83cd..cebd663b6708 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1061,7 +1061,22 @@ static void vblank_control_worker(struct work_struct
*work)
DRM_DEBUG_KMS("Allow idle optimizations (MALL): %d\n",
dm->active_vblank_irq_count == 0);
+ /* Control PSR based on vblank requirements from OS */
+ if (vblank_work->stream && vblank_work->stream->link) {
+ if (vblank_work->enable) {
+ if
(vblank_work->stream->link->psr_settings.psr_allow_active)
+ amdgpu_dm_psr_disable(vblank_work->stream);
+ } else if
(vblank_work->stream->link->psr_settings.psr_feature_enabled &&
+
!vblank_work->stream->link->psr_settings.psr_allow_active &&
+ vblank_work->acrtc->dm_irq_params.allow_psr_entry) {
+ amdgpu_dm_psr_enable(vblank_work->stream);
+ }
+ }
+
mutex_unlock(&dm->dc_lock);
+
+ dc_stream_release(vblank_work->stream);
+
kfree(vblank_work);
}
@@ -6018,6 +6033,11 @@ static inline int dm_set_vblank(struct drm_crtc *crtc,
bool enable)
work->acrtc = acrtc;
work->enable = enable;
+ if (acrtc_state->stream) {
+ dc_stream_retain(acrtc_state->stream);
+ work->stream = acrtc_state->stream;
+ }
+
queue_work(dm->vblank_control_workqueue, &work->work);
#endif
@@ -8623,6 +8643,12 @@ static void amdgpu_dm_commit_planes(struct
drm_atomic_state *state,
/* Update the planes if changed or disable if we don't have any. */
if ((planes_count || acrtc_state->active_planes == 0) &&
acrtc_state->stream) {
+ /*
+* If PSR or idle optimizations are enabled then flush out
+* any pending work before hardware programming.
+*/
+ flush_workqueue(dm->vblank_control_workqueue);
+
bundle->stream_update.stream = acrtc_state->stream;
if (new_pcrtc_state->mode_changed) {
bundle->stream_update.src = acrtc_state->stream->src;
@@ -8691,16 +8717,20 @@ static void amdgpu_dm_commit_planes(struct
drm_atomic_state *state,
acrtc_state->stream->link->psr_settings.psr_version !=
DC_PSR_VERSION_UNSUPPORTED &&
!acrtc_state->stream->link->psr_settings.psr_feature_enabled)
amdgpu_dm_link_setup_psr(acrtc_state->stream);
- else if ((acrtc_state->update_type == UPDATE_TYPE_FAST) &&
-
acrtc_state->stream->link->psr_settings.psr_feature_enabled &&
-
!acrtc_state->stream->link->psr_settings.psr_allow_active) {
- struct amdgpu_dm_connector *aconn = (struct
amdgpu_dm_connector *)
- acrtc_state->stream->dm_stream_context;
+
+ /* Decrement skip count when PSR is enabled and we're doing
fast updates. */
+ if (acrtc_state->update_type == UPDATE_TYPE_FAST &&
+
acrtc_state->stream->link->psr_settings.ps
