Re: [Intel-gfx] [PATCH 15/18] drm: Remove transitional helpers

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Oct 03, 2018 at 11:18:24AM +0200, Daniel Vetter wrote:
> With armada the last bigger driver that realistically needed these to
> convert from legacy kms to atomic is converted. These helpers have
> been broken more often than not the past 2 years, and as this little
> patch series shows, tricked a bunch of people into using the wrong
> helpers for their functions.
> 
> Aside: I think a lot more drivers should be using the device-level
> drm_atomic_helper_shutdown/suspend/resume helpers and related
> functions. In almost all the cases they get things exactly right.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc_helper.c  | 115 -----------------
>  drivers/gpu/drm/drm_plane_helper.c | 197 -----------------------------
>  include/drm/drm_crtc_helper.h      |   6 -
>  include/drm/drm_plane_helper.h     |  14 --
>  4 files changed, 332 deletions(-)

Pretty nice diffstat.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index ce75e9506e85..a3c81850e755 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -984,118 +984,3 @@ void drm_helper_resume_force_mode(struct drm_device *dev)
>  	drm_modeset_unlock_all(dev);
>  }
>  EXPORT_SYMBOL(drm_helper_resume_force_mode);
> -
> -/**
> - * drm_helper_crtc_mode_set - mode_set implementation for atomic plane helpers
> - * @crtc: DRM CRTC
> - * @mode: DRM display mode which userspace requested
> - * @adjusted_mode: DRM display mode adjusted by ->mode_fixup callbacks
> - * @x: x offset of the CRTC scanout area on the underlying framebuffer
> - * @y: y offset of the CRTC scanout area on the underlying framebuffer
> - * @old_fb: previous framebuffer
> - *
> - * This function implements a callback useable as the ->mode_set callback
> - * required by the CRTC helpers. Besides the atomic plane helper functions for
> - * the primary plane the driver must also provide the ->mode_set_nofb callback
> - * to set up the CRTC.
> - *
> - * This is a transitional helper useful for converting drivers to the atomic
> - * interfaces.
> - */
> -int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
> -			     struct drm_display_mode *adjusted_mode, int x, int y,
> -			     struct drm_framebuffer *old_fb)
> -{
> -	struct drm_crtc_state *crtc_state;
> -	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
> -	int ret;
> -
> -	if (crtc->funcs->atomic_duplicate_state)
> -		crtc_state = crtc->funcs->atomic_duplicate_state(crtc);
> -	else {
> -		if (!crtc->state)
> -			drm_atomic_helper_crtc_reset(crtc);
> -
> -		crtc_state = drm_atomic_helper_crtc_duplicate_state(crtc);
> -	}
> -
> -	if (!crtc_state)
> -		return -ENOMEM;
> -
> -	crtc_state->planes_changed = true;
> -	crtc_state->mode_changed = true;
> -	ret = drm_atomic_set_mode_for_crtc(crtc_state, mode);
> -	if (ret)
> -		goto out;
> -	drm_mode_copy(&crtc_state->adjusted_mode, adjusted_mode);
> -
> -	if (crtc_funcs->atomic_check) {
> -		ret = crtc_funcs->atomic_check(crtc, crtc_state);
> -		if (ret)
> -			goto out;
> -	}
> -
> -	swap(crtc->state, crtc_state);
> -
> -	crtc_funcs->mode_set_nofb(crtc);
> -
> -	ret = drm_helper_crtc_mode_set_base(crtc, x, y, old_fb);
> -
> -out:
> -	if (crtc_state) {
> -		if (crtc->funcs->atomic_destroy_state)
> -			crtc->funcs->atomic_destroy_state(crtc, crtc_state);
> -		else
> -			drm_atomic_helper_crtc_destroy_state(crtc, crtc_state);
> -	}
> -
> -	return ret;
> -}
> -EXPORT_SYMBOL(drm_helper_crtc_mode_set);
> -
> -/**
> - * drm_helper_crtc_mode_set_base - mode_set_base implementation for atomic plane helpers
> - * @crtc: DRM CRTC
> - * @x: x offset of the CRTC scanout area on the underlying framebuffer
> - * @y: y offset of the CRTC scanout area on the underlying framebuffer
> - * @old_fb: previous framebuffer
> - *
> - * This function implements a callback useable as the ->mode_set_base used
> - * required by the CRTC helpers. The driver must provide the atomic plane helper
> - * functions for the primary plane.
> - *
> - * This is a transitional helper useful for converting drivers to the atomic
> - * interfaces.
> - */
> -int drm_helper_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
> -				  struct drm_framebuffer *old_fb)
> -{
> -	struct drm_plane_state *plane_state;
> -	struct drm_plane *plane = crtc->primary;
> -
> -	if (plane->funcs->atomic_duplicate_state)
> -		plane_state = plane->funcs->atomic_duplicate_state(plane);
> -	else {
> -		if (!plane->state)
> -			drm_atomic_helper_plane_reset(plane);
> -
> -		plane_state = drm_atomic_helper_plane_duplicate_state(plane);
> -	}
> -	if (!plane_state)
> -		return -ENOMEM;
> -	plane_state->plane = plane;
> -
> -	plane_state->crtc = crtc;
> -	drm_atomic_set_fb_for_plane(plane_state, crtc->primary->fb);
> -	plane_state->crtc_x = 0;
> -	plane_state->crtc_y = 0;
> -	plane_state->crtc_h = crtc->mode.vdisplay;
> -	plane_state->crtc_w = crtc->mode.hdisplay;
> -	plane_state->src_x = x << 16;
> -	plane_state->src_y = y << 16;
> -	plane_state->src_h = crtc->mode.vdisplay << 16;
> -	plane_state->src_w = crtc->mode.hdisplay << 16;
> -
> -	return drm_plane_helper_commit(plane, plane_state, old_fb);
> -}
> -EXPORT_SYMBOL(drm_helper_crtc_mode_set_base);
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index a393756b664e..965286231600 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -336,200 +336,3 @@ const struct drm_plane_funcs drm_primary_helper_funcs = {
>  	.destroy = drm_primary_helper_destroy,
>  };
>  EXPORT_SYMBOL(drm_primary_helper_funcs);
> -
> -int drm_plane_helper_commit(struct drm_plane *plane,
> -			    struct drm_plane_state *plane_state,
> -			    struct drm_framebuffer *old_fb)
> -{
> -	const struct drm_plane_helper_funcs *plane_funcs;
> -	struct drm_crtc *crtc[2];
> -	const struct drm_crtc_helper_funcs *crtc_funcs[2];
> -	int i, ret = 0;
> -
> -	plane_funcs = plane->helper_private;
> -
> -	/* Since this is a transitional helper we can't assume that plane->state
> -	 * is always valid. Hence we need to use plane->crtc instead of
> -	 * plane->state->crtc as the old crtc. */
> -	crtc[0] = plane->crtc;
> -	crtc[1] = crtc[0] != plane_state->crtc ? plane_state->crtc : NULL;
> -
> -	for (i = 0; i < 2; i++)
> -		crtc_funcs[i] = crtc[i] ? crtc[i]->helper_private : NULL;
> -
> -	if (plane_funcs->atomic_check) {
> -		ret = plane_funcs->atomic_check(plane, plane_state);
> -		if (ret)
> -			goto out;
> -	}
> -
> -	if (plane_funcs->prepare_fb && plane_state->fb != old_fb) {
> -		ret = plane_funcs->prepare_fb(plane,
> -					      plane_state);
> -		if (ret)
> -			goto out;
> -	}
> -
> -	/* Point of no return, commit sw state. */
> -	swap(plane->state, plane_state);
> -
> -	for (i = 0; i < 2; i++) {
> -		if (crtc_funcs[i] && crtc_funcs[i]->atomic_begin)
> -			crtc_funcs[i]->atomic_begin(crtc[i], crtc[i]->state);
> -	}
> -
> -	/*
> -	 * Drivers may optionally implement the ->atomic_disable callback, so
> -	 * special-case that here.
> -	 */
> -	if (drm_atomic_plane_disabling(plane_state, plane->state) &&
> -	    plane_funcs->atomic_disable)
> -		plane_funcs->atomic_disable(plane, plane_state);
> -	else
> -		plane_funcs->atomic_update(plane, plane_state);
> -
> -	for (i = 0; i < 2; i++) {
> -		if (crtc_funcs[i] && crtc_funcs[i]->atomic_flush)
> -			crtc_funcs[i]->atomic_flush(crtc[i], crtc[i]->state);
> -	}
> -
> -	/*
> -	 * If we only moved the plane and didn't change fb's, there's no need to
> -	 * wait for vblank.
> -	 */
> -	if (plane->state->fb == old_fb)
> -		goto out;
> -
> -	for (i = 0; i < 2; i++) {
> -		if (!crtc[i])
> -			continue;
> -
> -		if (crtc[i]->cursor == plane)
> -			continue;
> -
> -		/* There's no other way to figure out whether the crtc is running. */
> -		ret = drm_crtc_vblank_get(crtc[i]);
> -		if (ret == 0) {
> -			drm_crtc_wait_one_vblank(crtc[i]);
> -			drm_crtc_vblank_put(crtc[i]);
> -		}
> -
> -		ret = 0;
> -	}
> -
> -	if (plane_funcs->cleanup_fb)
> -		plane_funcs->cleanup_fb(plane, plane_state);
> -out:
> -	if (plane->funcs->atomic_destroy_state)
> -		plane->funcs->atomic_destroy_state(plane, plane_state);
> -	else
> -		drm_atomic_helper_plane_destroy_state(plane, plane_state);
> -
> -	return ret;
> -}
> -
> -/**
> - * drm_plane_helper_update() - Transitional helper for plane update
> - * @plane: plane object to update
> - * @crtc: owning CRTC of owning plane
> - * @fb: framebuffer to flip onto plane
> - * @crtc_x: x offset of primary plane on crtc
> - * @crtc_y: y offset of primary plane on crtc
> - * @crtc_w: width of primary plane rectangle on crtc
> - * @crtc_h: height of primary plane rectangle on crtc
> - * @src_x: x offset of @fb for panning
> - * @src_y: y offset of @fb for panning
> - * @src_w: width of source rectangle in @fb
> - * @src_h: height of source rectangle in @fb
> - * @ctx: lock acquire context, not used here
> - *
> - * Provides a default plane update handler using the atomic plane update
> - * functions. It is fully left to the driver to check plane constraints and
> - * handle corner-cases like a fully occluded or otherwise invisible plane.
> - *
> - * This is useful for piecewise transitioning of a driver to the atomic helpers.
> - *
> - * RETURNS:
> - * Zero on success, error code on failure
> - */
> -int drm_plane_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
> -			    struct drm_framebuffer *fb,
> -			    int crtc_x, int crtc_y,
> -			    unsigned int crtc_w, unsigned int crtc_h,
> -			    uint32_t src_x, uint32_t src_y,
> -			    uint32_t src_w, uint32_t src_h,
> -			    struct drm_modeset_acquire_ctx *ctx)
> -{
> -	struct drm_plane_state *plane_state;
> -
> -	if (plane->funcs->atomic_duplicate_state)
> -		plane_state = plane->funcs->atomic_duplicate_state(plane);
> -	else {
> -		if (!plane->state)
> -			drm_atomic_helper_plane_reset(plane);
> -
> -		plane_state = drm_atomic_helper_plane_duplicate_state(plane);
> -	}
> -	if (!plane_state)
> -		return -ENOMEM;
> -	plane_state->plane = plane;
> -
> -	plane_state->crtc = crtc;
> -	drm_atomic_set_fb_for_plane(plane_state, fb);
> -	plane_state->crtc_x = crtc_x;
> -	plane_state->crtc_y = crtc_y;
> -	plane_state->crtc_h = crtc_h;
> -	plane_state->crtc_w = crtc_w;
> -	plane_state->src_x = src_x;
> -	plane_state->src_y = src_y;
> -	plane_state->src_h = src_h;
> -	plane_state->src_w = src_w;
> -
> -	return drm_plane_helper_commit(plane, plane_state, plane->fb);
> -}
> -EXPORT_SYMBOL(drm_plane_helper_update);
> -
> -/**
> - * drm_plane_helper_disable() - Transitional helper for plane disable
> - * @plane: plane to disable
> - * @ctx: lock acquire context, not used here
> - *
> - * Provides a default plane disable handler using the atomic plane update
> - * functions. It is fully left to the driver to check plane constraints and
> - * handle corner-cases like a fully occluded or otherwise invisible plane.
> - *
> - * This is useful for piecewise transitioning of a driver to the atomic helpers.
> - *
> - * RETURNS:
> - * Zero on success, error code on failure
> - */
> -int drm_plane_helper_disable(struct drm_plane *plane,
> -			     struct drm_modeset_acquire_ctx *ctx)
> -{
> -	struct drm_plane_state *plane_state;
> -	struct drm_framebuffer *old_fb;
> -
> -	/* crtc helpers love to call disable functions for already disabled hw
> -	 * functions. So cope with that. */
> -	if (!plane->crtc)
> -		return 0;
> -
> -	if (plane->funcs->atomic_duplicate_state)
> -		plane_state = plane->funcs->atomic_duplicate_state(plane);
> -	else {
> -		if (!plane->state)
> -			drm_atomic_helper_plane_reset(plane);
> -
> -		plane_state = drm_atomic_helper_plane_duplicate_state(plane);
> -	}
> -	if (!plane_state)
> -		return -ENOMEM;
> -	plane_state->plane = plane;
> -
> -	plane_state->crtc = NULL;
> -	old_fb = plane_state->fb;
> -	drm_atomic_set_fb_for_plane(plane_state, NULL);
> -
> -	return drm_plane_helper_commit(plane, plane_state, old_fb);
> -}
> -EXPORT_SYMBOL(drm_plane_helper_disable);
> diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
> index 6914633037a5..d65f034843ce 100644
> --- a/include/drm/drm_crtc_helper.h
> +++ b/include/drm/drm_crtc_helper.h
> @@ -57,12 +57,6 @@ int drm_helper_connector_dpms(struct drm_connector *connector, int mode);
>  
>  void drm_helper_resume_force_mode(struct drm_device *dev);
>  
> -int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
> -			     struct drm_display_mode *adjusted_mode, int x, int y,
> -			     struct drm_framebuffer *old_fb);
> -int drm_helper_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
> -				  struct drm_framebuffer *old_fb);
> -
>  /* drm_probe_helper.c */
>  int drm_helper_probe_single_connector_modes(struct drm_connector
>  					    *connector, uint32_t maxX,
> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
> index 26cee2934781..1999781781c7 100644
> --- a/include/drm/drm_plane_helper.h
> +++ b/include/drm/drm_plane_helper.h
> @@ -62,18 +62,4 @@ int drm_primary_helper_disable(struct drm_plane *plane,
>  void drm_primary_helper_destroy(struct drm_plane *plane);
>  extern const struct drm_plane_funcs drm_primary_helper_funcs;
>  
> -int drm_plane_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
> -			    struct drm_framebuffer *fb,
> -			    int crtc_x, int crtc_y,
> -			    unsigned int crtc_w, unsigned int crtc_h,
> -			    uint32_t src_x, uint32_t src_y,
> -			    uint32_t src_w, uint32_t src_h,
> -			    struct drm_modeset_acquire_ctx *ctx);
> -int drm_plane_helper_disable(struct drm_plane *plane,
> -			     struct drm_modeset_acquire_ctx *ctx);
> -
> -/* For use by drm_crtc_helper.c */
> -int drm_plane_helper_commit(struct drm_plane *plane,
> -			    struct drm_plane_state *plane_state,
> -			    struct drm_framebuffer *old_fb);
>  #endif
> -- 
> 2.19.0.rc2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel