linux-kernel - Re: [v2] drm/msm/dpu: Fix reservation failures in modeset

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAF6AEGtY8xdy=KoJVWr1hMOdduv5b0EwENL1Txcs7R_2AFEK3Q@mail.gmail.com>
Date:   Fri, 7 Aug 2020 14:46:20 -0700
From:   Rob Clark <robdclark@...il.com>
To:     Kalyan Thota <kalyan_t@...eaurora.org>
Cc:     dri-devel <dri-devel@...ts.freedesktop.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        freedreno <freedreno@...ts.freedesktop.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Sean Paul <seanpaul@...omium.org>,
        "Kristian H. Kristensen" <hoegsberg@...omium.org>,
        Douglas Anderson <dianders@...omium.org>,
        Krishna Manikandan <mkrishn@...eaurora.org>,
        Raviteja Tamatam <travitej@...eaurora.org>,
        nganji@...eaurora.org, Stephen Boyd <swboyd@...omium.org>,
        Abhinav Kumar <abhinavk@...eaurora.org>,
        Drew Davenport <ddavenport@...omium.org>,
        Stephen Boyd <sboyd@...nel.org>
Subject: Re: [v2] drm/msm/dpu: Fix reservation failures in modeset

On Fri, Aug 7, 2020 at 6:10 AM Kalyan Thota <kalyan_t@...eaurora.org> wrote:
>
> In TEST_ONLY commit, rm global_state will duplicate the
> object and request for new reservations, once they pass
> then the new state will be swapped with the old and will
> be available for the Atomic Commit.
>
> This patch fixes some of missing links in the resource
> reservation sequence mentioned above.
>
> 1) Creation of duplicate state in test_only commit (Rob)
> 2) Allocate and release the resources on every modeset.
> 3) Avoid allocation only when active is false.
>
> In a modeset operation, swap state happens well before
> disable. Hence clearing reservations in disable will
> cause failures in modeset enable.
>
> Allow reservations to be cleared/allocated before swap,
> such that only newly committed resources are pushed to HW.
>
> Changes in v1:
>  - Move the rm release to atomic_check.
>  - Ensure resource allocation and free happens when active
>    is not changed i.e only when mode is changed.(Rob)
>
> Signed-off-by: Kalyan Thota <kalyan_t@...eaurora.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 63976dc..50a98d1 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -582,7 +582,7 @@ static int dpu_encoder_virt_atomic_check(
>         dpu_kms = to_dpu_kms(priv->kms);
>         mode = &crtc_state->mode;
>         adj_mode = &crtc_state->adjusted_mode;
> -       global_state = dpu_kms_get_existing_global_state(dpu_kms);
> +       global_state = dpu_kms_get_global_state(crtc_state->state);

sboyd noticed with lock debug enabled, that you are missing:

   if (IS_ERR(global_state))
     return PTR_ERR(global_state);

since drm_modeset_lock() can fail (and if you have
CONFIG_DEBUG_WW_MUTEX_SLOWPATH enabled, it frequently does return
-EDEADLK, to test the backoff path)

BR,
-R

>         trace_dpu_enc_atomic_check(DRMID(drm_enc));
>
>         /*
> @@ -617,12 +617,15 @@ static int dpu_encoder_virt_atomic_check(
>         /* Reserve dynamic resources now. */
>         if (!ret) {
>                 /*
> -                * Avoid reserving resources when mode set is pending. Topology
> -                * info may not be available to complete reservation.
> +                * Release and Allocate resources on every modeset
> +                * Dont allocate when active is false.
>                  */
>                 if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> -                       ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
> -                                       drm_enc, crtc_state, topology);
> +                       dpu_rm_release(global_state, drm_enc);
> +
> +                       if (!crtc_state->active_changed || crtc_state->active)
> +                               ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
> +                                               drm_enc, crtc_state, topology);
>                 }
>         }
>
> @@ -1171,7 +1174,6 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
>         struct dpu_encoder_virt *dpu_enc = NULL;
>         struct msm_drm_private *priv;
>         struct dpu_kms *dpu_kms;
> -       struct dpu_global_state *global_state;
>         int i = 0;
>
>         if (!drm_enc) {
> @@ -1190,7 +1192,6 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
>
>         priv = drm_enc->dev->dev_private;
>         dpu_kms = to_dpu_kms(priv->kms);
> -       global_state = dpu_kms_get_existing_global_state(dpu_kms);
>
>         trace_dpu_enc_disable(DRMID(drm_enc));
>
> @@ -1220,8 +1221,6 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
>
>         DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
>
> -       dpu_rm_release(global_state, drm_enc);
> -
>         mutex_unlock(&dpu_enc->enc_lock);
>  }
>
> --
> 1.9.1
>