| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z368DnfCrpYOPbav@phenom.ffwll.local>
Date: Wed, 8 Jan 2025 18:55:26 +0100
From: Simona Vetter <simona.vetter@...ll.ch>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
Rob Clark <robdclark@...il.com>,
Abhinav Kumar <quic_abhinavk@...cinc.com>,
Sean Paul <sean@...rly.run>,
Marijn Suijten <marijn.suijten@...ainline.org>,
Chandan Uddaraju <chandanu@...eaurora.org>,
Jeykumar Sankaran <jsanka@...eaurora.org>,
Jordan Crouse <jordan@...micpenguin.net>,
Sravanthi Kollukuduru <skolluku@...eaurora.org>,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
Archit Taneja <architt@...eaurora.org>,
Rajesh Yadav <ryadav@...eaurora.org>, linux-arm-msm@...r.kernel.org,
freedreno@...ts.freedesktop.org,
Simona Vetter <simona.vetter@...ll.ch>
Subject: Re: [PATCH 6/6] drm/msm/dpu: don't set crtc_state->mode_changed from
atomic_check()
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.vetter@...ll.ch>
> Closes: https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.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
Powered by blists - more mailing lists