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

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

 




Den 27.03.2019 14.55, skrev Daniel Vetter:
> On Wed, Mar 27, 2019 at 2:42 PM Noralf Trønnes <noralf@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>>>
>>> Triggers a bit my midlayer ocd, but I can't come up with a better idea
>>> either.
>>>
>>> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>>>
>>
>> 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?

Sorry my bad, I was just confused. Looking at it again, I see that I can
just rebase on top of that patch.

Noralf.

> -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@xxxxxxxxxxxxxxxxxxxxx
>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
> 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux