On 1/8/2025 8:26 PM, Dmitry Baryshkov wrote:
On Wed, Jan 08, 2025 at 08:11:27PM -0800, Abhinav Kumar wrote:
On 1/8/2025 6:27 PM, Abhinav Kumar wrote:
On 12/21/2024 9:00 PM, Dmitry Baryshkov wrote:
The MSM driver uses drm_atomic_helper_check() which mandates that none
of the atomic_check() callbacks toggles crtc_state->mode_changed.
Perform corresponding check before calling the drm_atomic_helper_check()
function.
Fixes: 8b45a26f2ba9 ("drm/msm/dpu: reserve cdm blocks for writeback
in case of YUV output")
Reported-by: Simona Vetter <simona.vetter@xxxxxxxx>
Closes:
https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 32
+++++++++++++++++++++++++----
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 4 ++++
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 26
+++++++++++++++++++++++
drivers/gpu/drm/msm/msm_atomic.c | 13 +++++++++++-
drivers/gpu/drm/msm/msm_kms.h | 7 +++++++
5 files changed, 77 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 209e6fb605b2d8724935b62001032e7d39540366..b7c3aa8d0e2ca58091deacdeaccb0819d2bf045c
100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -753,6 +753,34 @@ static void
dpu_encoder_assign_crtc_resources(struct dpu_kms *dpu_kms,
cstate->num_mixers = num_lm;
}
+/**
+ * dpu_encoder_virt_check_mode_changed: check if full modeset is
required
+ * @drm_enc: Pointer to drm encoder structure
+ * @crtc_state: Corresponding CRTC state to be checked
+ * @conn_state: Corresponding Connector's state to be checked
+ *
+ * Check if the changes in the object properties demand full mode set.
+ */
+int dpu_encoder_virt_check_mode_changed(struct drm_encoder *drm_enc,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state)
+{
+ struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
+ struct msm_display_topology topology;
+
+ DPU_DEBUG_ENC(dpu_enc, "\n");
+
+ /* Using mode instead of adjusted_mode as it wasn't computed yet */
+ topology = dpu_encoder_get_topology(dpu_enc, &crtc_state->mode,
crtc_state, conn_state);
+
+ if (topology.needs_cdm && !dpu_enc->cur_master->hw_cdm)
+ crtc_state->mode_changed = true;
+ else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
+ crtc_state->mode_changed = true;
+
+ return 0;
+}
How will this work exactly?
needs_cdm is set in the encoder's atomic_check which is called inside
drm_atomic_helper_check(). But this function is called before that.
So needs_cdm will never hit.
Sorry, my bad. after change (4) of this series needs_cdm is also populated
within dpu_encoder_get_topology().
To follow up on https://patchwork.freedesktop.org/patch/629231/?series=137975&rev=4#comment_1148651
So is the plan for CWB to add a dpu_crtc_check_mode_changed() like
dpu_encoder's and call it?
I think dpu_encoder_virt_check_mode_changed() would transform into the
dpu_crtc_check_mode_changed() together with one of the patches that
moves resource allocation and refactors topology handling.
hmm we need the cur_master for cdm. That will not be accessible in
dpu_crtc.c so we will end up with a separate
dpu_crtc_check_mode_changed() for CWB from what I see. We will discuss
it further when we re-post CWB.
But overall, I think we can make CWB work on top of this.
Hence,
Reviewed-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
I do not know how important patch 2 is for this series and I would
prefer not delaying CWB even more than what it already has been.
If we cannot reach a conclusion on patch 2, can you break that one out
of this series so that the rest of it is ready to land?
+
static int dpu_encoder_virt_atomic_check(
struct drm_encoder *drm_enc,
struct drm_crtc_state *crtc_state,
