Re: [PATCH 03/16] drm/atomic: Move __drm_atomic_helper_disable_plane/set_config()

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Mar 27, 2019 at 2:42 PM Noralf Trønnes <noralf@tronnes.org> wrote:
>
>
>
> Den 26.03.2019 21.48, skrev Daniel Vetter:
> > On Tue, Mar 26, 2019 at 06:55:33PM +0100, Noralf Trønnes wrote:
> >> Prepare for moving drm_fb_helper modesetting code to drm_client.
> >> drm_client will be linked to drm.ko, so move
> >> __drm_atomic_helper_disable_plane() and __drm_atomic_helper_set_config()
> >> out of drm_kms_helper.ko.
> >>
> >> While at it, fix two checkpatch complaints:
> >> - WARNING: Block comments use a trailing */ on a separate line
> >> - CHECK: Alignment should match open parenthesis
> >>
> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >
> > Triggers a bit my midlayer ocd, but I can't come up with a better idea
> > either.
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
>
> The Intel CI informed me that this patch didn't apply..
>
> Turns out there's a patch in drm-next by Sean:
>
> drm: Merge __drm_atomic_helper_disable_all() into
> drm_atomic_helper_disable_all()
>
> https://cgit.freedesktop.org/drm/drm/commit?id=37406a60fac7de336dec331a8707a94462ac5a5e
>
> Should I move the whole function with the kerneldoc to drm_atomic.c, or
> should I keep the doc and function in drm_atomic_helper.c and just
> recreate __drm_atomic_helper_disable_all() and call that ?

So my longterm goal with all these compat helpers is to move them into
the core anyway, and make them at least the default, if not only the
only accepted choice for atomic drivers. We've already done that for
the dpms hook. We can't make it the enforced default yet for planes
because of cursors, but I think we could just move both of these into
the core (disable_plane and update_plane), make it the default option,
and remove all driver users that just set it as the default hook.

Would need to be renamed to something like drm_atomic_default_* ofc.

Not sure why you're talking about disable_all here though, since that
doesn't seem to be touched in this patch at all?
-Daniel

>
> Noralf.
>
> >> ---
> >>  drivers/gpu/drm/drm_atomic.c        | 168 ++++++++++++++++++++++++++++
> >>  drivers/gpu/drm/drm_atomic_helper.c | 164 ---------------------------
> >>  drivers/gpu/drm/drm_crtc_internal.h |   5 +
> >>  include/drm/drm_atomic_helper.h     |   4 -
> >>  4 files changed, 173 insertions(+), 168 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> index 5eb40130fafb..c3a9ffbf2310 100644
> >> --- a/drivers/gpu/drm/drm_atomic.c
> >> +++ b/drivers/gpu/drm/drm_atomic.c
> >> @@ -1130,6 +1130,174 @@ int drm_atomic_nonblocking_commit(struct drm_atomic_state *state)
> >>  }
> >>  EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
> >>
> >> +/* just used from drm-client and atomic-helper: */
> >> +int __drm_atomic_helper_disable_plane(struct drm_plane *plane,
> >> +                                  struct drm_plane_state *plane_state)
> >> +{
> >> +    int ret;
> >> +
> >> +    ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> >> +    if (ret != 0)
> >> +            return ret;
> >> +
> >> +    drm_atomic_set_fb_for_plane(plane_state, NULL);
> >> +    plane_state->crtc_x = 0;
> >> +    plane_state->crtc_y = 0;
> >> +    plane_state->crtc_w = 0;
> >> +    plane_state->crtc_h = 0;
> >> +    plane_state->src_x = 0;
> >> +    plane_state->src_y = 0;
> >> +    plane_state->src_w = 0;
> >> +    plane_state->src_h = 0;
> >> +
> >> +    return 0;
> >> +}
> >> +EXPORT_SYMBOL(__drm_atomic_helper_disable_plane);
> >> +
> >> +static int update_output_state(struct drm_atomic_state *state,
> >> +                           struct drm_mode_set *set)
> >> +{
> >> +    struct drm_device *dev = set->crtc->dev;
> >> +    struct drm_crtc *crtc;
> >> +    struct drm_crtc_state *new_crtc_state;
> >> +    struct drm_connector *connector;
> >> +    struct drm_connector_state *new_conn_state;
> >> +    int ret, i;
> >> +
> >> +    ret = drm_modeset_lock(&dev->mode_config.connection_mutex,
> >> +                           state->acquire_ctx);
> >> +    if (ret)
> >> +            return ret;
> >> +
> >> +    /* First disable all connectors on the target crtc. */
> >> +    ret = drm_atomic_add_affected_connectors(state, set->crtc);
> >> +    if (ret)
> >> +            return ret;
> >> +
> >> +    for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> >> +            if (new_conn_state->crtc == set->crtc) {
> >> +                    ret = drm_atomic_set_crtc_for_connector(new_conn_state,
> >> +                                                            NULL);
> >> +                    if (ret)
> >> +                            return ret;
> >> +
> >> +                    /* Make sure legacy setCrtc always re-trains */
> >> +                    new_conn_state->link_status = DRM_LINK_STATUS_GOOD;
> >> +            }
> >> +    }
> >> +
> >> +    /* Then set all connectors from set->connectors on the target crtc */
> >> +    for (i = 0; i < set->num_connectors; i++) {
> >> +            new_conn_state = drm_atomic_get_connector_state(state,
> >> +                                                            set->connectors[i]);
> >> +            if (IS_ERR(new_conn_state))
> >> +                    return PTR_ERR(new_conn_state);
> >> +
> >> +            ret = drm_atomic_set_crtc_for_connector(new_conn_state,
> >> +                                                    set->crtc);
> >> +            if (ret)
> >> +                    return ret;
> >> +    }
> >> +
> >> +    for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> >> +            /*
> >> +             * Don't update ->enable for the CRTC in the set_config request,
> >> +             * since a mismatch would indicate a bug in the upper layers.
> >> +             * The actual modeset code later on will catch any
> >> +             * inconsistencies here.
> >> +             */
> >> +            if (crtc == set->crtc)
> >> +                    continue;
> >> +
> >> +            if (!new_crtc_state->connector_mask) {
> >> +                    ret = drm_atomic_set_mode_prop_for_crtc(new_crtc_state,
> >> +                                                            NULL);
> >> +                    if (ret < 0)
> >> +                            return ret;
> >> +
> >> +                    new_crtc_state->active = false;
> >> +            }
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +/* just used from drm-client and atomic-helper: */
> >> +int __drm_atomic_helper_set_config(struct drm_mode_set *set,
> >> +                               struct drm_atomic_state *state)
> >> +{
> >> +    struct drm_crtc_state *crtc_state;
> >> +    struct drm_plane_state *primary_state;
> >> +    struct drm_crtc *crtc = set->crtc;
> >> +    int hdisplay, vdisplay;
> >> +    int ret;
> >> +
> >> +    crtc_state = drm_atomic_get_crtc_state(state, crtc);
> >> +    if (IS_ERR(crtc_state))
> >> +            return PTR_ERR(crtc_state);
> >> +
> >> +    primary_state = drm_atomic_get_plane_state(state, crtc->primary);
> >> +    if (IS_ERR(primary_state))
> >> +            return PTR_ERR(primary_state);
> >> +
> >> +    if (!set->mode) {
> >> +            WARN_ON(set->fb);
> >> +            WARN_ON(set->num_connectors);
> >> +
> >> +            ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
> >> +            if (ret != 0)
> >> +                    return ret;
> >> +
> >> +            crtc_state->active = false;
> >> +
> >> +            ret = drm_atomic_set_crtc_for_plane(primary_state, NULL);
> >> +            if (ret != 0)
> >> +                    return ret;
> >> +
> >> +            drm_atomic_set_fb_for_plane(primary_state, NULL);
> >> +
> >> +            goto commit;
> >> +    }
> >> +
> >> +    WARN_ON(!set->fb);
> >> +    WARN_ON(!set->num_connectors);
> >> +
> >> +    ret = drm_atomic_set_mode_for_crtc(crtc_state, set->mode);
> >> +    if (ret != 0)
> >> +            return ret;
> >> +
> >> +    crtc_state->active = true;
> >> +
> >> +    ret = drm_atomic_set_crtc_for_plane(primary_state, crtc);
> >> +    if (ret != 0)
> >> +            return ret;
> >> +
> >> +    drm_mode_get_hv_timing(set->mode, &hdisplay, &vdisplay);
> >> +
> >> +    drm_atomic_set_fb_for_plane(primary_state, set->fb);
> >> +    primary_state->crtc_x = 0;
> >> +    primary_state->crtc_y = 0;
> >> +    primary_state->crtc_w = hdisplay;
> >> +    primary_state->crtc_h = vdisplay;
> >> +    primary_state->src_x = set->x << 16;
> >> +    primary_state->src_y = set->y << 16;
> >> +    if (drm_rotation_90_or_270(primary_state->rotation)) {
> >> +            primary_state->src_w = vdisplay << 16;
> >> +            primary_state->src_h = hdisplay << 16;
> >> +    } else {
> >> +            primary_state->src_w = hdisplay << 16;
> >> +            primary_state->src_h = vdisplay << 16;
> >> +    }
> >> +
> >> +commit:
> >> +    ret = update_output_state(state, set);
> >> +    if (ret)
> >> +            return ret;
> >> +
> >> +    return 0;
> >> +}
> >> +EXPORT_SYMBOL(__drm_atomic_helper_set_config);
> >> +
> >>  void drm_atomic_print_state(const struct drm_atomic_state *state)
> >>  {
> >>      struct drm_printer p = drm_info_printer(state->dev->dev);
> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >> index 2453678d1186..b0d960da53cb 100644
> >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >> @@ -2831,95 +2831,6 @@ int drm_atomic_helper_disable_plane(struct drm_plane *plane,
> >>  }
> >>  EXPORT_SYMBOL(drm_atomic_helper_disable_plane);
> >>
> >> -/* just used from fb-helper and atomic-helper: */
> >> -int __drm_atomic_helper_disable_plane(struct drm_plane *plane,
> >> -            struct drm_plane_state *plane_state)
> >> -{
> >> -    int ret;
> >> -
> >> -    ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> >> -    if (ret != 0)
> >> -            return ret;
> >> -
> >> -    drm_atomic_set_fb_for_plane(plane_state, NULL);
> >> -    plane_state->crtc_x = 0;
> >> -    plane_state->crtc_y = 0;
> >> -    plane_state->crtc_w = 0;
> >> -    plane_state->crtc_h = 0;
> >> -    plane_state->src_x = 0;
> >> -    plane_state->src_y = 0;
> >> -    plane_state->src_w = 0;
> >> -    plane_state->src_h = 0;
> >> -
> >> -    return 0;
> >> -}
> >> -
> >> -static int update_output_state(struct drm_atomic_state *state,
> >> -                           struct drm_mode_set *set)
> >> -{
> >> -    struct drm_device *dev = set->crtc->dev;
> >> -    struct drm_crtc *crtc;
> >> -    struct drm_crtc_state *new_crtc_state;
> >> -    struct drm_connector *connector;
> >> -    struct drm_connector_state *new_conn_state;
> >> -    int ret, i;
> >> -
> >> -    ret = drm_modeset_lock(&dev->mode_config.connection_mutex,
> >> -                           state->acquire_ctx);
> >> -    if (ret)
> >> -            return ret;
> >> -
> >> -    /* First disable all connectors on the target crtc. */
> >> -    ret = drm_atomic_add_affected_connectors(state, set->crtc);
> >> -    if (ret)
> >> -            return ret;
> >> -
> >> -    for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> >> -            if (new_conn_state->crtc == set->crtc) {
> >> -                    ret = drm_atomic_set_crtc_for_connector(new_conn_state,
> >> -                                                            NULL);
> >> -                    if (ret)
> >> -                            return ret;
> >> -
> >> -                    /* Make sure legacy setCrtc always re-trains */
> >> -                    new_conn_state->link_status = DRM_LINK_STATUS_GOOD;
> >> -            }
> >> -    }
> >> -
> >> -    /* Then set all connectors from set->connectors on the target crtc */
> >> -    for (i = 0; i < set->num_connectors; i++) {
> >> -            new_conn_state = drm_atomic_get_connector_state(state,
> >> -                                                        set->connectors[i]);
> >> -            if (IS_ERR(new_conn_state))
> >> -                    return PTR_ERR(new_conn_state);
> >> -
> >> -            ret = drm_atomic_set_crtc_for_connector(new_conn_state,
> >> -                                                    set->crtc);
> >> -            if (ret)
> >> -                    return ret;
> >> -    }
> >> -
> >> -    for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> >> -            /* Don't update ->enable for the CRTC in the set_config request,
> >> -             * since a mismatch would indicate a bug in the upper layers.
> >> -             * The actual modeset code later on will catch any
> >> -             * inconsistencies here. */
> >> -            if (crtc == set->crtc)
> >> -                    continue;
> >> -
> >> -            if (!new_crtc_state->connector_mask) {
> >> -                    ret = drm_atomic_set_mode_prop_for_crtc(new_crtc_state,
> >> -                                                            NULL);
> >> -                    if (ret < 0)
> >> -                            return ret;
> >> -
> >> -                    new_crtc_state->active = false;
> >> -            }
> >> -    }
> >> -
> >> -    return 0;
> >> -}
> >> -
> >>  /**
> >>   * drm_atomic_helper_set_config - set a new config from userspace
> >>   * @set: mode set configuration
> >> @@ -2964,81 +2875,6 @@ int drm_atomic_helper_set_config(struct drm_mode_set *set,
> >>  }
> >>  EXPORT_SYMBOL(drm_atomic_helper_set_config);
> >>
> >> -/* just used from fb-helper and atomic-helper: */
> >> -int __drm_atomic_helper_set_config(struct drm_mode_set *set,
> >> -            struct drm_atomic_state *state)
> >> -{
> >> -    struct drm_crtc_state *crtc_state;
> >> -    struct drm_plane_state *primary_state;
> >> -    struct drm_crtc *crtc = set->crtc;
> >> -    int hdisplay, vdisplay;
> >> -    int ret;
> >> -
> >> -    crtc_state = drm_atomic_get_crtc_state(state, crtc);
> >> -    if (IS_ERR(crtc_state))
> >> -            return PTR_ERR(crtc_state);
> >> -
> >> -    primary_state = drm_atomic_get_plane_state(state, crtc->primary);
> >> -    if (IS_ERR(primary_state))
> >> -            return PTR_ERR(primary_state);
> >> -
> >> -    if (!set->mode) {
> >> -            WARN_ON(set->fb);
> >> -            WARN_ON(set->num_connectors);
> >> -
> >> -            ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
> >> -            if (ret != 0)
> >> -                    return ret;
> >> -
> >> -            crtc_state->active = false;
> >> -
> >> -            ret = drm_atomic_set_crtc_for_plane(primary_state, NULL);
> >> -            if (ret != 0)
> >> -                    return ret;
> >> -
> >> -            drm_atomic_set_fb_for_plane(primary_state, NULL);
> >> -
> >> -            goto commit;
> >> -    }
> >> -
> >> -    WARN_ON(!set->fb);
> >> -    WARN_ON(!set->num_connectors);
> >> -
> >> -    ret = drm_atomic_set_mode_for_crtc(crtc_state, set->mode);
> >> -    if (ret != 0)
> >> -            return ret;
> >> -
> >> -    crtc_state->active = true;
> >> -
> >> -    ret = drm_atomic_set_crtc_for_plane(primary_state, crtc);
> >> -    if (ret != 0)
> >> -            return ret;
> >> -
> >> -    drm_mode_get_hv_timing(set->mode, &hdisplay, &vdisplay);
> >> -
> >> -    drm_atomic_set_fb_for_plane(primary_state, set->fb);
> >> -    primary_state->crtc_x = 0;
> >> -    primary_state->crtc_y = 0;
> >> -    primary_state->crtc_w = hdisplay;
> >> -    primary_state->crtc_h = vdisplay;
> >> -    primary_state->src_x = set->x << 16;
> >> -    primary_state->src_y = set->y << 16;
> >> -    if (drm_rotation_90_or_270(primary_state->rotation)) {
> >> -            primary_state->src_w = vdisplay << 16;
> >> -            primary_state->src_h = hdisplay << 16;
> >> -    } else {
> >> -            primary_state->src_w = hdisplay << 16;
> >> -            primary_state->src_h = vdisplay << 16;
> >> -    }
> >> -
> >> -commit:
> >> -    ret = update_output_state(state, set);
> >> -    if (ret)
> >> -            return ret;
> >> -
> >> -    return 0;
> >> -}
> >> -
> >>  static int __drm_atomic_helper_disable_all(struct drm_device *dev,
> >>                                         struct drm_modeset_acquire_ctx *ctx,
> >>                                         bool clean_old_fbs)
> >> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> >> index 216f2a9ee3d4..22a63a544ec7 100644
> >> --- a/drivers/gpu/drm/drm_crtc_internal.h
> >> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> >> @@ -207,6 +207,11 @@ struct drm_minor;
> >>  int drm_atomic_debugfs_init(struct drm_minor *minor);
> >>  #endif
> >>
> >> +int __drm_atomic_helper_disable_plane(struct drm_plane *plane,
> >> +                                  struct drm_plane_state *plane_state);
> >> +int __drm_atomic_helper_set_config(struct drm_mode_set *set,
> >> +                               struct drm_atomic_state *state);
> >> +
> >>  void drm_atomic_print_state(const struct drm_atomic_state *state);
> >>
> >>  /* drm_atomic_uapi.c */
> >> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> >> index 58214be3bf3d..bf4e07141d81 100644
> >> --- a/include/drm/drm_atomic_helper.h
> >> +++ b/include/drm/drm_atomic_helper.h
> >> @@ -117,12 +117,8 @@ int drm_atomic_helper_update_plane(struct drm_plane *plane,
> >>                                 struct drm_modeset_acquire_ctx *ctx);
> >>  int drm_atomic_helper_disable_plane(struct drm_plane *plane,
> >>                                  struct drm_modeset_acquire_ctx *ctx);
> >> -int __drm_atomic_helper_disable_plane(struct drm_plane *plane,
> >> -            struct drm_plane_state *plane_state);
> >>  int drm_atomic_helper_set_config(struct drm_mode_set *set,
> >>                               struct drm_modeset_acquire_ctx *ctx);
> >> -int __drm_atomic_helper_set_config(struct drm_mode_set *set,
> >> -            struct drm_atomic_state *state);
> >>
> >>  int drm_atomic_helper_disable_all(struct drm_device *dev,
> >>                                struct drm_modeset_acquire_ctx *ctx);
> >> --
> >> 2.20.1
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx