[PATCH 04/19] drm/amd/display: Fix handling of scaling and underscan.
Kai Wasserbäch
kai at dev.carbon-project.org
Wed May 31 16:39:14 UTC 2017
Hey Harry, hey Andrey,
Harry Wentland wrote on 31.05.2017 17:52:
> From: Andrey Grodzovsky <Andrey.Grodzovsky at amd.com>
>
> Summury of changes:
s/Summury/Summary/
> 1: Both in check and commit Connector properties were handled as
s/in// ?
> part of for_each(crtc) loop while they shoud have been handled
s/part of/part of a/
s/shoud/should/
> in a dedicated for_each(connector)
> loop since they are connector properties. Moved.
>
> 2: Removed hacky plane add in amdgpu_dm_connector_atomic_set_property
> to force iteration on plane forconnector property. This was
> causing double call to commit_surface_for_stream both in crtc loop
> and plane loop.
> 3: Remove middleman DC interface and call dc_commit_surfaces_to_stream
> directly to increase code clarity.
This description makes it sound like three independent issues, that should be
split into their own patches. But I'm probably missing an interdependency here?
If so, it'd be awesome if this could be mentioned here.
Cheers&Thanks,
Kai
> Remove it from atomic_commit.
>
> Change-Id: I1337b7abe4a2c6812c7500a5bf22f6c6f01890ca
> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky at amd.com>
> Reviewed-by: Harry Wentland <Harry.Wentland at amd.com>
> ---
> .../drm/amd/display/amdgpu_dm/amdgpu_dm_types.c | 207 ++++++++++-----------
> 1 file changed, 99 insertions(+), 108 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_types.c
> index a929df2e690d..15204bf7d9a8 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_types.c
> @@ -683,9 +683,8 @@ struct amdgpu_connector *aconnector_from_drm_crtc_id(
> static void update_stream_scaling_settings(
> const struct drm_display_mode *mode,
> const struct dm_connector_state *dm_state,
> - const struct dc_stream *stream)
> + struct dc_stream *stream)
> {
> - struct amdgpu_device *adev = dm_state->base.crtc->dev->dev_private;
> enum amdgpu_rmx_type rmx_type;
>
> struct rect src = { 0 }; /* viewport in composition space*/
> @@ -727,7 +726,8 @@ static void update_stream_scaling_settings(
> dst.height -= dm_state->underscan_vborder;
> }
>
> - adev->dm.dc->stream_funcs.stream_update_scaling(adev->dm.dc, stream, &src, &dst);
> + stream->src = src;
> + stream->dst = dst;
>
> DRM_DEBUG_KMS("Destination Rectangle x:%d y:%d width:%d height:%d\n",
> dst.x, dst.y, dst.width, dst.height);
> @@ -1287,9 +1287,6 @@ int amdgpu_dm_connector_atomic_set_property(
> struct dm_connector_state *dm_new_state =
> to_dm_connector_state(connector_state);
>
> - struct drm_crtc_state *new_crtc_state;
> - struct drm_crtc *crtc;
> - int i;
> int ret = -EINVAL;
>
> if (property == dev->mode_config.scaling_mode_property) {
> @@ -1335,34 +1332,6 @@ int amdgpu_dm_connector_atomic_set_property(
> return ret;
> }
>
> -
> -
> - for_each_crtc_in_state(
> - connector_state->state,
> - crtc,
> - new_crtc_state,
> - i) {
> -
> - if (crtc == connector_state->crtc) {
> - struct drm_plane_state *plane_state;
> -
> - /*
> - * Bit of magic done here. We need to ensure
> - * that planes get update after mode is set.
> - * So, we need to add primary plane to state,
> - * and this way atomic_update would be called
> - * for it
> - */
> - plane_state =
> - drm_atomic_get_plane_state(
> - connector_state->state,
> - crtc->primary);
> -
> - if (!plane_state)
> - return -EINVAL;
> - }
> - }
> -
> return ret;
> }
>
> @@ -2582,28 +2551,19 @@ static void amdgpu_dm_commit_surfaces(struct drm_atomic_state *state,
> struct amdgpu_crtc *acrtc_attach = to_amdgpu_crtc(crtc);
> struct drm_framebuffer *fb = plane_state->fb;
> struct drm_connector *connector;
> - struct dm_connector_state *dm_state = NULL;
> -
> - enum dm_commit_action action;
> + struct dm_connector_state *con_state = NULL;
> bool pflip_needed;
>
> if (!fb || !crtc || !crtc->state->active)
> continue;
>
> - action = get_dm_commit_action(crtc->state);
> -
> - /*
> - * TODO - TO decide if it's a flip or surface update
> - * stop relying on allow_modeset flag and query DC
> - * using dc_check_update_surfaces_for_stream.
> - */
> pflip_needed = !state->allow_modeset;
> if (!pflip_needed) {
> list_for_each_entry(connector,
> &dev->mode_config.connector_list,
> head) {
> if (connector->state->crtc == crtc) {
> - dm_state = to_dm_connector_state(
> + con_state = to_dm_connector_state(
> connector->state);
> break;
> }
> @@ -2623,8 +2583,10 @@ static void amdgpu_dm_commit_surfaces(struct drm_atomic_state *state,
> * Also it should be needed when used with actual
> * drm_atomic_commit ioctl in future
> */
> - if (!dm_state)
> + if (!con_state)
> continue;
> +
> +
> if (crtc == pcrtc) {
> add_surface(dm->dc, crtc, plane,
> &dc_surfaces_constructed[planes_count]);
> @@ -2676,7 +2638,8 @@ void amdgpu_dm_atomic_commit_tail(
> const struct dc_stream *new_stream;
> unsigned long flags;
> bool wait_for_vblank = true;
> -
> + struct drm_connector *connector;
> + struct drm_connector_state *old_conn_state;
>
> drm_atomic_helper_update_legacy_modeset_state(dev, state);
>
> @@ -2756,21 +2719,6 @@ void amdgpu_dm_atomic_commit_tail(
>
> break;
> }
> -
> - case DM_COMMIT_ACTION_NOTHING: {
> - struct dm_connector_state *dm_state = NULL;
> -
> - if (!aconnector)
> - break;
> -
> - dm_state = to_dm_connector_state(aconnector->base.state);
> -
> - /* Scaling update */
> - update_stream_scaling_settings(&crtc->state->mode,
> - dm_state, acrtc->stream);
> -
> - break;
> - }
> case DM_COMMIT_ACTION_DPMS_OFF:
> case DM_COMMIT_ACTION_RESET:
> DRM_INFO("Atomic commit: RESET. crtc id %d:[%p]\n", acrtc->crtc_id, acrtc);
> @@ -2778,9 +2726,48 @@ void amdgpu_dm_atomic_commit_tail(
> if (acrtc->stream)
> remove_stream(adev, acrtc);
> break;
> +
> + /*TODO retire */
> + case DM_COMMIT_ACTION_NOTHING:
> + continue;
> } /* switch() */
> } /* for_each_crtc_in_state() */
>
> + /* Handle scaling and undersacn changes*/
> + for_each_connector_in_state(state, connector, old_conn_state, i) {
> + struct amdgpu_connector *aconnector = to_amdgpu_connector(connector);
> + struct dm_connector_state *con_new_state =
> + to_dm_connector_state(aconnector->base.state);
> + struct dm_connector_state *con_old_state =
> + to_dm_connector_state(old_conn_state);
> + struct amdgpu_crtc *acrtc = to_amdgpu_crtc(con_new_state->base.crtc);
> + const struct dc_stream_status *status = NULL;
> +
> + /* Skip any modesets/resets */
> + if (!acrtc ||
> + get_dm_commit_action(acrtc->base.state) != DM_COMMIT_ACTION_NOTHING)
> + continue;
> +
> + /* Skip any thing not scale or underscan chnages */
> + if (!is_scaling_state_different(con_new_state, con_old_state))
> + continue;
> +
> + update_stream_scaling_settings(&con_new_state->base.crtc->mode,
> + con_new_state, (struct dc_stream *)acrtc->stream);
> +
> + status = dc_stream_get_status(acrtc->stream);
> + WARN_ON(!status);
> + WARN_ON(!status->surface_count);
> +
> + /*TODO How it works with MPO ?*/
> + if (!dc_commit_surfaces_to_stream(
> + dm->dc,
> + (const struct dc_surface **)status->surfaces,
> + status->surface_count,
> + acrtc->stream))
> + dm_error("%s: Failed to update stream scaling!\n", __func__);
> + }
> +
> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>
> struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
> @@ -3118,6 +3105,8 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
> struct dc *dc = adev->dm.dc;
> bool need_to_validate = false;
> struct validate_context *context;
> + struct drm_connector *connector;
> + struct drm_connector_state *conn_state;
> /*
> * This bool will be set for true for any modeset/reset
> * or surface update which implies non fast surfae update.
> @@ -3203,52 +3192,6 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
> break;
> }
>
> - case DM_COMMIT_ACTION_NOTHING: {
> - const struct drm_connector *drm_connector = NULL;
> - struct drm_connector_state *conn_state = NULL;
> - struct dm_connector_state *dm_state = NULL;
> - struct dm_connector_state *old_dm_state = NULL;
> - struct dc_stream *new_stream;
> -
> - if (!aconnector)
> - break;
> -
> - for_each_connector_in_state(
> - state, drm_connector, conn_state, j) {
> - if (&aconnector->base == drm_connector)
> - break;
> - }
> -
> - old_dm_state = to_dm_connector_state(drm_connector->state);
> - dm_state = to_dm_connector_state(conn_state);
> -
> - /* Support underscan adjustment*/
> - if (!is_scaling_state_different(dm_state, old_dm_state))
> - break;
> -
> - new_stream = create_stream_for_sink(aconnector, &crtc_state->mode, dm_state);
> -
> - if (!new_stream) {
> - DRM_ERROR("%s: Failed to create new stream for crtc %d\n",
> - __func__, acrtc->base.base.id);
> - break;
> - }
> -
> - new_streams[new_stream_count] = new_stream;
> - set_count = update_in_val_sets_stream(
> - set,
> - crtc_set,
> - set_count,
> - acrtc->stream,
> - new_stream,
> - crtc);
> -
> - new_stream_count++;
> - need_to_validate = true;
> - wait_for_prev_commits = true;
> -
> - break;
> - }
> case DM_COMMIT_ACTION_DPMS_OFF:
> case DM_COMMIT_ACTION_RESET:
> /* i.e. reset mode */
> @@ -3260,6 +3203,10 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
> wait_for_prev_commits = true;
> }
> break;
> +
> + /*TODO retire */
> + case DM_COMMIT_ACTION_NOTHING:
> + continue;
> }
>
> /*
> @@ -3276,6 +3223,50 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
> ret = -EINVAL;
> }
>
> + /* Check scaling and undersacn changes*/
> + for_each_connector_in_state(state, connector, conn_state, i) {
> + struct amdgpu_connector *aconnector = to_amdgpu_connector(connector);
> + struct dm_connector_state *con_old_state =
> + to_dm_connector_state(aconnector->base.state);
> + struct dm_connector_state *con_new_state =
> + to_dm_connector_state(conn_state);
> + struct amdgpu_crtc *acrtc = to_amdgpu_crtc(con_new_state->base.crtc);
> + struct dc_stream *new_stream;
> +
> + /* Skip any modesets/resets */
> + if (!acrtc ||
> + get_dm_commit_action(acrtc->base.state) != DM_COMMIT_ACTION_NOTHING)
> + continue;
> +
> + /* Skip any thing not scale or underscan chnages */
> + if (!is_scaling_state_different(con_new_state, con_old_state))
> + continue;
> +
> + new_stream = create_stream_for_sink(
> + aconnector,
> + &acrtc->base.state->mode,
> + con_new_state);
> +
> + if (!new_stream) {
> + DRM_ERROR("%s: Failed to create new stream for crtc %d\n",
> + __func__, acrtc->base.base.id);
> + continue;
> + }
> +
> + new_streams[new_stream_count] = new_stream;
> + set_count = update_in_val_sets_stream(
> + set,
> + crtc_set,
> + set_count,
> + acrtc->stream,
> + new_stream,
> + &acrtc->base);
> +
> + new_stream_count++;
> + need_to_validate = true;
> + wait_for_prev_commits = true;
> + }
> +
> for (i = 0; i < set_count; i++) {
> for_each_plane_in_state(state, plane, plane_state, j) {
> struct drm_crtc *crtc = plane_state->crtc;
>
--
Kai Wasserbäch (Kai Wasserbaech)
E-Mail: kai at dev.carbon-project.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20170531/b592dfa2/attachment.sig>
More information about the amd-gfx
mailing list