Re: [PATCH] drm: Introduce a drm_crtc_commit_wait helper

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Dec 17, 2020 at 3:43 PM Maxime Ripard <maxime@xxxxxxxxxx> wrote:
>
> There's currently four users of the same logic to wait for a commit to
> be flipped: three for the CRTCs, connectors and planes in
> drm_atomic_helper_wait_for_dependencies, and one in vc4.
>
> Let's consolidate this a bit to avoid any code duplication.
>
> Suggested-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>

Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>

I did ponder for a bit whether this is in a good spot or not, maybe a
drm_crtc_commit.[hc] would be neat but still feels a bit like
overkill. Maybe add a quick sentence to the struct drm_crtc_commit
pointing at this new function, like "See also drm_crtc_commit_wait()"?
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic.c        | 39 ++++++++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c | 61 +++++------------------------
>  drivers/gpu/drm/vc4/vc4_kms.c       | 17 ++------
>  include/drm/drm_atomic.h            |  2 +
>  4 files changed, 54 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index b2d20eb6c807..e2ab6564535c 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -52,6 +52,45 @@ void __drm_crtc_commit_free(struct kref *kref)
>  }
>  EXPORT_SYMBOL(__drm_crtc_commit_free);
>
> +/**
> + * drm_crtc_commit_wait - Waits for a commit to complete
> + * @commit: &drm_crtc_commit to wait for
> + *
> + * Waits for a given &drm_crtc_commit to be programmed into the
> + * hardware and flipped to.
> + *
> + * Returns:
> + *
> + * 0 on success, a negative error code otherwise.
> + */
> +int drm_crtc_commit_wait(struct drm_crtc_commit *commit)
> +{
> +       unsigned long timeout = 10 * HZ;
> +       int ret;
> +
> +       if (!commit)
> +               return 0;
> +
> +       ret = wait_for_completion_timeout(&commit->hw_done, timeout);
> +       if (!ret) {
> +               DRM_ERROR("hw_done timed out\n");
> +               return -ETIMEDOUT;
> +       }
> +
> +       /*
> +        * Currently no support for overwriting flips, hence
> +        * stall for previous one to execute completely.
> +        */
> +       ret = wait_for_completion_timeout(&commit->flip_done, timeout);
> +       if (!ret) {
> +               DRM_ERROR("flip_done timed out\n");
> +               return -ETIMEDOUT;
> +       }
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(drm_crtc_commit_wait);
> +
>  /**
>   * drm_atomic_state_default_release -
>   * release memory initialized by drm_atomic_state_init
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index a84dc427cf82..9fa3f97223a1 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2202,70 +2202,27 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
>         struct drm_plane_state *old_plane_state;
>         struct drm_connector *conn;
>         struct drm_connector_state *old_conn_state;
> -       struct drm_crtc_commit *commit;
>         int i;
>         long ret;
>
>         for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> -               commit = old_crtc_state->commit;
> -
> -               if (!commit)
> -                       continue;
> -
> -               ret = wait_for_completion_timeout(&commit->hw_done,
> -                                                 10*HZ);
> -               if (ret == 0)
> -                       DRM_ERROR("[CRTC:%d:%s] hw_done timed out\n",
> -                                 crtc->base.id, crtc->name);
> -
> -               /* Currently no support for overwriting flips, hence
> -                * stall for previous one to execute completely. */
> -               ret = wait_for_completion_timeout(&commit->flip_done,
> -                                                 10*HZ);
> -               if (ret == 0)
> -                       DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
> +               ret = drm_crtc_commit_wait(old_crtc_state->commit);
> +               if (ret)
> +                       DRM_ERROR("[CRTC:%d:%s] commit wait timed out\n",
>                                   crtc->base.id, crtc->name);
>         }
>
>         for_each_old_connector_in_state(old_state, conn, old_conn_state, i) {
> -               commit = old_conn_state->commit;
> -
> -               if (!commit)
> -                       continue;
> -
> -               ret = wait_for_completion_timeout(&commit->hw_done,
> -                                                 10*HZ);
> -               if (ret == 0)
> -                       DRM_ERROR("[CONNECTOR:%d:%s] hw_done timed out\n",
> -                                 conn->base.id, conn->name);
> -
> -               /* Currently no support for overwriting flips, hence
> -                * stall for previous one to execute completely. */
> -               ret = wait_for_completion_timeout(&commit->flip_done,
> -                                                 10*HZ);
> -               if (ret == 0)
> -                       DRM_ERROR("[CONNECTOR:%d:%s] flip_done timed out\n",
> +               ret = drm_crtc_commit_wait(old_conn_state->commit);
> +               if (ret)
> +                       DRM_ERROR("[CONNECTOR:%d:%s] commit wait timed out\n",
>                                   conn->base.id, conn->name);
>         }
>
>         for_each_old_plane_in_state(old_state, plane, old_plane_state, i) {
> -               commit = old_plane_state->commit;
> -
> -               if (!commit)
> -                       continue;
> -
> -               ret = wait_for_completion_timeout(&commit->hw_done,
> -                                                 10*HZ);
> -               if (ret == 0)
> -                       DRM_ERROR("[PLANE:%d:%s] hw_done timed out\n",
> -                                 plane->base.id, plane->name);
> -
> -               /* Currently no support for overwriting flips, hence
> -                * stall for previous one to execute completely. */
> -               ret = wait_for_completion_timeout(&commit->flip_done,
> -                                                 10*HZ);
> -               if (ret == 0)
> -                       DRM_ERROR("[PLANE:%d:%s] flip_done timed out\n",
> +               ret = drm_crtc_commit_wait(old_plane_state->commit);
> +               if (ret)
> +                       DRM_ERROR("[PLANE:%d:%s] commit wait timed out\n",
>                                   plane->base.id, plane->name);
>         }
>  }
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index f09254c2497d..bb5529a7a9c2 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -363,9 +363,8 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
>         for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>                 struct vc4_crtc_state *vc4_crtc_state =
>                         to_vc4_crtc_state(old_crtc_state);
> -               struct drm_crtc_commit *commit;
>                 unsigned int channel = vc4_crtc_state->assigned_channel;
> -               unsigned long done;
> +               int ret;
>
>                 if (channel == VC4_HVS_CHANNEL_DISABLED)
>                         continue;
> @@ -373,17 +372,9 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
>                 if (!old_hvs_state->fifo_state[channel].in_use)
>                         continue;
>
> -               commit = old_hvs_state->fifo_state[i].pending_commit;
> -               if (!commit)
> -                       continue;
> -
> -               done = wait_for_completion_timeout(&commit->hw_done, 10 * HZ);
> -               if (!done)
> -                       drm_err(dev, "Timed out waiting for hw_done\n");
> -
> -               done = wait_for_completion_timeout(&commit->flip_done, 10 * HZ);
> -               if (!done)
> -                       drm_err(dev, "Timed out waiting for flip_done\n");
> +               ret = drm_crtc_commit_wait(old_hvs_state->fifo_state[i].pending_commit);
> +               if (ret)
> +                       drm_err(dev, "Timed out waiting for commit\n");
>         }
>
>         drm_atomic_helper_commit_modeset_disables(dev, state);
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index ce7023e9115d..79ef992c433d 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -436,6 +436,8 @@ static inline void drm_crtc_commit_put(struct drm_crtc_commit *commit)
>         kref_put(&commit->ref, __drm_crtc_commit_free);
>  }
>
> +int drm_crtc_commit_wait(struct drm_crtc_commit *commit);
> +
>  struct drm_atomic_state * __must_check
>  drm_atomic_state_alloc(struct drm_device *dev);
>  void drm_atomic_state_clear(struct drm_atomic_state *state);
> --
> 2.29.2
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux