On 3/4/2025 12:42 PM, Dmitry Baryshkov wrote:
On Tue, Mar 04, 2025 at 11:38:24AM -0800, Abhinav Kumar wrote:
On 3/3/2025 9:32 PM, Dmitry Baryshkov wrote:
On Tue, 4 Mar 2025 at 03:44, Jessica Zhang <quic_jesszhan@xxxxxxxxxxx> wrote:
On 3/3/2025 3:49 PM, Dmitry Baryshkov wrote:
On Mon, Mar 03, 2025 at 10:28:00AM -0800, Jessica Zhang wrote:
If new CTLs are reserved by CRTC but atomic_enable() is skipped, the
encoders will configure the stale CTL instead of the newly reserved one.
The CTLs are propagates in .atomic_mode_set(), not in .atomic_enable().
Hi Dmitry,
Yes, sorry mixed up the two function ops here and in my reply in the CWB
thread.
Avoid this by setting mode_changed to true if new CTLs have been
reserved by CRTC.
This looks very strange. First we reserve new CTLs when there is a
modeset requested. Then on one of the next commits we detect that
encoder has stale CTLs and try to upgrade the commit to full modeset
(while the user might not have .allow_modeset set to true for whatever
reason, e.g. because only ACTIVE is changed).
Ah I see what you mean. I think this is an issue with how/when we're
calling dpu_rm_reserve(). Since RM reservation is tied to
atomic_check(), we aren't able to force a modeset based on HW block
reservation. The only reason we were able to avoid this issue with
needs_cdm is because needs_cdm didn't depend on the CDM HW block index.
I think there's not really a good way to avoid this other than flipping
the order of the msm_atomic_check to drm_helper_atomic_check ->
dpu_kms.check_mode_changed -> drm_atomic_helper_check_modeset().
No-no-no. This would require a full drm_atomic_helper_check() call
again, after the check_mode_changed() callback. But again, this should
not be required at all. The whole point of .check_mode_changed() is to
be called before performing full atomic_check() chains.
Right but the documentation also allows calling
drm_atomic_helper_check_modeset() again. We are looking at all options even
moving forward and not just this issue.
It does. But I'd rather not do it. Especially not in this case: we
perfectly know in advance if hw resources were reallocated or not.
What do you think? It seems to be valid given the examples in the DRM
docs [1]
[1]
https://elixir.bootlin.com/linux/v6.13.5/source/drivers/gpu/drm/drm_atomic_helper.c#L610
Could you please check if the following change fixes the issue: in
crtc_set_mode() replace the raw !new_crtc_state->mode_changed check with
the drm_atomic_crtc_needs_modeset() call?
This also fixes the DPMS failures. IIRC Abhinav had suggested a similar
change to fix a different issue [2] and you gave some feedback on
avoiding mode_set() for enable/disable calls which don't have mode_changed.
After reading the documentation for
drm_encoder_helper_funcs.atomic_mode_set() and looking around, I think
the issue is in the handling of the DPMS functions. I might have a fix
for the issue.
Also, while this may fix the CWB CI failures, wouldn't the issue still
remain regarding how to force modeset for changes in HW block reservation?
I think it is the other way around: HW block reservation is only
changed if there is a modeset. I'm currently testing my theory :-) We
were performing HW reassignment if drm_atomic_crtc_needs_modeset() was
true. However this function returns true in one of the cases, where
there is no actual modeset happening (and it's even documented this
way) - when only DPMS call has happened (in other words, when
.active_changed = true, but two other bits are false). It is required
not to reassign HW resources in such a case. So, I think, a correct
fix is to change the condition in dpu_crtc_atomic_check().
Yes, Jessica had also suggested this option. This will work because now the
resource re-assignment will not happen and hence will avoid the issue. The
documentation of DPMS was not fully clear. So it said, the same thing you
mentioned, that when active has changed there is no need to reassign
hardware resources but I was not sure if that would impact normal
suspend/resume because across suspend/resume hardware resources need to be
cleared / re-assigned.
Suspend / resume is handled via the helpers, which commit an
all-disabled state and then commit a previous state. Other than that,
there is no need to clear resource assignments during suspend resume.
They should be disabled, but there is no need to drop the assignment.
We will need to do some testing to make sure this does not introduce
some other regressions. You can post your change, we can discuss it there.
I do still think that, even if this also works, we
will still run into issues when we will need to force a mode_changed based
on resource assignment of other encoder based blocks such as DSC or PP etc.
This is what dpu_encoder_needs_modeset() is for. On the other hand,
note, if we reassigned resources, it means that either mode_changes or
connectors_changed is set. And the DRM framework will call encoder's
atomic_mode_set() in such a case.
Like we wrote, dpu_encoder_needs_modeset() is not useful without the
sequence change in msm_atomic_check because dpu_encoder_needs_modeset()
is called before the resource assignments are done today.
[2] https://gitlab.freedesktop.org/drm/msm/-/issues/59
Thanks,
Jessica Zhang
Note: This patch only adds tracking for the CTL reservation, but eventually
all HW blocks used by encoders (i.e. DSC, PINGPONG, CWB) should have a
similar check to avoid the same issue.
Suggested-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
Closes: https://lists.freedesktop.org/archives/freedreno/2025-February/036719.html
Signed-off-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 13 +++++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12 ++++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 1 +
3 files changed, 26 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 4073d821158c0..a1a8be8f5ab9f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1406,19 +1406,32 @@ int dpu_crtc_check_mode_changed(struct drm_crtc_state *old_crtc_state,
struct drm_crtc *crtc = new_crtc_state->crtc;
bool clone_mode_enabled = drm_crtc_in_clone_mode(old_crtc_state);
bool clone_mode_requested = drm_crtc_in_clone_mode(new_crtc_state);
+ struct dpu_crtc_state *cstate = to_dpu_crtc_state(new_crtc_state);
+ uint32_t enc_ctl_mask = 0;
+ uint32_t crtc_ctl_mask = 0;
+ struct dpu_crtc_mixer *m;
DRM_DEBUG_ATOMIC("%d\n", crtc->base.id);
+ for (int i = 0; i < cstate->num_mixers; i++) {
+ m = &cstate->mixers[i];
+ crtc_ctl_mask |= BIT(m->lm_ctl->idx - CTL_0);
+ }
+
/* there might be cases where encoder needs a modeset too */
drm_for_each_encoder_mask(drm_enc, crtc->dev, new_crtc_state->encoder_mask) {
if (dpu_encoder_needs_modeset(drm_enc, new_crtc_state->state))
new_crtc_state->mode_changed = true;
+ enc_ctl_mask |= dpu_encoder_get_ctls(drm_enc);
}
if ((clone_mode_requested && !clone_mode_enabled) ||
(!clone_mode_requested && clone_mode_enabled))
new_crtc_state->mode_changed = true;
+ if (crtc_ctl_mask != enc_ctl_mask)
+ new_crtc_state->mode_changed = true;
+
return 0;
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index a61598710acda..2f3101caeba91 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -188,6 +188,7 @@ struct dpu_encoder_virt {
unsigned int dsc_mask;
unsigned int cwb_mask;
+ unsigned int ctl_mask;
bool intfs_swapped;
@@ -707,6 +708,13 @@ void dpu_encoder_update_topology(struct drm_encoder *drm_enc,
}
}
+uint32_t dpu_encoder_get_ctls(struct drm_encoder *drm_enc)
+{
+ struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
+
+ return dpu_enc->ctl_mask;
+}
+
bool dpu_encoder_needs_modeset(struct drm_encoder *drm_enc, struct drm_atomic_state *state)
{
struct drm_connector *connector;
@@ -1155,6 +1163,7 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
bool is_cwb_encoder;
unsigned int dsc_mask = 0;
unsigned int cwb_mask = 0;
+ unsigned int ctl_mask = 0;
int i;
if (!drm_enc) {
@@ -1245,11 +1254,14 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
"no ctl block assigned at idx: %d\n", i);
return;
}
+ ctl_mask |= BIT(phys->hw_ctl->idx - CTL_0);
phys->cached_mode = crtc_state->adjusted_mode;
if (phys->ops.atomic_mode_set)
phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
}
+
+ dpu_enc->ctl_mask = ctl_mask;
}
static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index ca1ca2e51d7ea..70b03743dc346 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -91,6 +91,7 @@ bool dpu_encoder_needs_modeset(struct drm_encoder *drm_enc, struct drm_atomic_st
void dpu_encoder_prepare_wb_job(struct drm_encoder *drm_enc,
struct drm_writeback_job *job);
+uint32_t dpu_encoder_get_ctls(struct drm_encoder *drm_enc);
void dpu_encoder_cleanup_wb_job(struct drm_encoder *drm_enc,
struct drm_writeback_job *job);
---
base-commit: 866e43b945bf98f8e807dfa45eca92f931f3a032
change-id: 20250228-force-modeset-hw-ctl-d02b80a2bb4c
prerequisite-change-id: 20241222-drm-dirty-modeset-88079bd27ae6:v2
prerequisite-patch-id: 0c61aabfcd13651203f476985380cbf4d3c299e6
prerequisite-patch-id: c6026f08011c288fd301676e9fa6f46d0cc1dab7
prerequisite-patch-id: b0cb06d5c88791d6e4755d879ced0d5050aa3cbf
prerequisite-patch-id: fd72ddde9dba0df053113bc505c213961a9760da
prerequisite-change-id: 20250209-dpu-c3fac78fc617:v2
prerequisite-patch-id: c84d2b4b06be06384968429085d1e8ebae23a583
prerequisite-patch-id: fb8ea7b9e7c85fabd27589c6551108382a235002
prerequisite-change-id: 20250211-dither-disable-b77b1e31977f:v1
prerequisite-patch-id: 079e04296212b4b83d51394b5a9b5eea6870d98a
prerequisite-change-id: 20240618-concurrent-wb-97d62387f952:v6
prerequisite-patch-id: b52034179741dc182aea9411fd446e270fdc69d1
prerequisite-patch-id: bc472765a7d5214691f3d92696cc8b0119f3252e
prerequisite-patch-id: c959bc480e96b04297ebaf30fea3a68bbac69da6
prerequisite-patch-id: f7db8449b241a41faac357d9257f8c7cb16503ec
prerequisite-patch-id: 7beb73131d0ab100f266fcd3c1f67c818a3263f4
prerequisite-patch-id: c08cbb5cf4e67e308afd61fdad6684b89429d3b6
prerequisite-patch-id: a4e343143b8fbe98ae4aa068cc459c750105eb9d
prerequisite-patch-id: 1d09edcf12ef7e7ab43547eefacae5b604b698e9
prerequisite-patch-id: 0008f9802bfd3c5877267666cceb7608203e5830
prerequisite-patch-id: 49402eb767c97915faf2378c5f5d05ced2dcfdac
prerequisite-patch-id: 522be2a6b5fe4e3a2d609526bb1539f9bc6f828f
prerequisite-patch-id: 031da00d0fffd522f74d682a551362f3ecda0c71
prerequisite-patch-id: 9454cec22231a8f3f01c33d52a5df3e26dd88287
prerequisite-patch-id: 7edbeaace3549332e581bee3183a76b0e4d18163
Best regards,
--
Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>
--
With best wishes
Dmitry
