On Sun, Dec 22, 2024 at 07:00:46AM +0200, 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.vet...@ffwll.ch>
> Closes: https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
> Signed-off-by: Dmitry Baryshkov <dmitry.barysh...@linaro.org>
> ---
> 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;
> +}
> +
> static int dpu_encoder_virt_atomic_check(
> struct drm_encoder *drm_enc,
> struct drm_crtc_state *crtc_state,
> @@ -786,10 +814,6 @@ static int dpu_encoder_virt_atomic_check(
>
> topology = dpu_encoder_get_topology(dpu_enc, adj_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;
> /*
> * Release and Allocate resources on every modeset
> */
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index
> 92b5ee390788d16e85e195a664417896a2bf1cae..da133ee4701a329f566f6f9a7255f2f6d050f891
> 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -88,4 +88,8 @@ void dpu_encoder_cleanup_wb_job(struct drm_encoder *drm_enc,
>
> bool dpu_encoder_is_valid_for_commit(struct drm_encoder *drm_enc);
>
> +int dpu_encoder_virt_check_mode_changed(struct drm_encoder *drm_enc,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state);
> +
> #endif /* __DPU_ENCODER_H__ */
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index
> dae8a94d3366abfb8937d5f44d8968f1d0691c2d..e2d822f7d785dc0debcb28595029a3e2050b0cf4
> 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -446,6 +446,31 @@ static void dpu_kms_disable_commit(struct msm_kms *kms)
> pm_runtime_put_sync(&dpu_kms->pdev->dev);
> }
>
> +static int dpu_kms_check_mode_changed(struct msm_kms *kms, struct
> drm_atomic_state *state)
> +{
> + struct drm_crtc_state *new_crtc_state;
> + struct drm_connector *connector;
> + struct drm_connector_state *new_conn_state;
> + int i;
> +
> + for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> + struct drm_encoder *encoder;
> +
> + WARN_ON(!!new_conn_state->best_encoder !=
> !!new_conn_state->crtc);
> +
> + if (!new_conn_state->crtc || !new_conn_state->best_encoder)
> + continue;
> +
> + new_crtc_state = drm_atomic_get_new_crtc_state(state,
> new_conn_state->crtc);
> +
> + encoder = new_conn_state->best_encoder;
> +
> + dpu_encoder_virt_check_mode_changed(encoder, new_crtc_state,
> new_conn_state);
> + }
> +
> + return 0;
> +}
> +
> static void dpu_kms_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
> {
> struct dpu_kms *dpu_kms = to_dpu_kms(kms);
> @@ -1049,6 +1074,7 @@ static const struct msm_kms_funcs kms_funcs = {
> .irq = dpu_core_irq,
> .enable_commit = dpu_kms_enable_commit,
> .disable_commit = dpu_kms_disable_commit,
> + .check_mode_changed = dpu_kms_check_mode_changed,
> .flush_commit = dpu_kms_flush_commit,
> .wait_flush = dpu_kms_wait_flush,
> .complete_commit = dpu_kms_complete_commit,
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c
> b/drivers/gpu/drm/msm/msm_atomic.c
> index
> a7a2384044ffdb13579cc9a10f56f8de9beca761..364df245e3a209094782ca1b47b752a729b32a5b
> 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -183,10 +183,16 @@ static unsigned get_crtc_mask(struct drm_atomic_state
> *state)
>
> int msm_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
> {
> + struct msm_drm_private *priv = dev->dev_private;
> + struct msm_kms *kms = priv->kms;
> struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> struct drm_crtc *crtc;
> - int i;
> + int i, ret = 0;
>
> + /*
> + * FIXME: stop setting allow_modeset and move this check to the DPU
> + * driver.
> + */
I guess there's more work to stop setting allow_modeset? Or was the issue
there that it breaks userspace that expects ctm changes to be doable
without modesets?
Either way msm patches lgtm, but don't feel confident enough for acks
except on the first one that reworks the active_change logic to use
crtc->enable instead for resource allocation.
-Sima
> for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
> new_crtc_state, i) {
> if ((old_crtc_state->ctm && !new_crtc_state->ctm) ||
> @@ -196,6 +202,11 @@ int msm_atomic_check(struct drm_device *dev, struct
> drm_atomic_state *state)
> }
> }
>
> + if (kms && kms->funcs && kms->funcs->check_mode_changed)
> + ret = kms->funcs->check_mode_changed(kms, state);
> + if (ret)
> + return ret;
> +
> return drm_atomic_helper_check(dev, state);
> }
>
> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> index
> e60162744c669773b6e5aef824a173647626ab4e..ec2a75af89b09754faef1a07adc9338f7d78161e
> 100644
> --- a/drivers/gpu/drm/msm/msm_kms.h
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -59,6 +59,13 @@ struct msm_kms_funcs {
> void (*enable_commit)(struct msm_kms *kms);
> void (*disable_commit)(struct msm_kms *kms);
>
> + /**
> + * @check_mode_changed:
> + *
> + * Verify if the commit requires a full modeset on one of CRTCs.
> + */
> + int (*check_mode_changed)(struct msm_kms *kms, struct drm_atomic_state
> *state);
> +
> /**
> * Prepare for atomic commit. This is called after any previous
> * (async or otherwise) commit has completed.
>
> --
> 2.39.5
>
--
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
