Re: [PATCH] drm/atomic-helper: Add option to update planes only on active crtc

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 22-07-15 om 18:02 schreef Daniel Vetter:
> With drivers supporting runtime pm it's generally not a good idea to
> touch the hardware when it's off. Add an option to the commit_planes
> helper to support this case.
>
> Note that the helpers already add all planes on a crtc when a modeset
> happens, hence plane updates will not be lost if drivers set this to
> true.
>
> v2: Check for NULL state->crtc before chasing the pointer. Also check
> both old and new crtc if there's a switch. Finally just outright
> disallow switching crtcs for a plane if the plane is in active use, on
> most hardware that doesn't make sense.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c    | 29 +++++++++++++++++++++++++++--
>  drivers/gpu/drm/exynos/exynos_drm_fb.c |  2 +-
>  drivers/gpu/drm/msm/msm_atomic.c       |  2 +-
>  drivers/gpu/drm/omapdrm/omap_drv.c     |  2 +-
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  2 +-
>  drivers/gpu/drm/sti/sti_drm_drv.c      |  2 +-
>  drivers/gpu/drm/tegra/drm.c            |  2 +-
>  include/drm/drm_atomic_helper.h        |  3 ++-
>  8 files changed, 35 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 99656815641d..2122c2b844da 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -471,6 +471,14 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
>  		if (!funcs || !funcs->atomic_check)
>  			continue;
>  
> +		if (plane->state->crtc && plane_state->crtc &&
> +		    plane->state->crtc != plane_state->crtc &&
> +		    plane->state->crtc->state->active) {
> +			DRM_DEBUG_ATOMIC("[PLANE:%d] changing crtc while still active\n",
> +					 plane->base.id);
> +			return -EINVAL;
> +		}
> +
>  		ret = funcs->atomic_check(plane, plane_state);
>  		if (ret) {
>  			DRM_DEBUG_ATOMIC("[PLANE:%d] atomic driver check failed\n",
> @@ -995,7 +1003,7 @@ int drm_atomic_helper_commit(struct drm_device *dev,
>  
>  	drm_atomic_helper_commit_modeset_disables(dev, state);
>  
> -	drm_atomic_helper_commit_planes(dev, state);
> +	drm_atomic_helper_commit_planes(dev, state, false);
>  
>  	drm_atomic_helper_commit_modeset_enables(dev, state);
>  
> @@ -1110,10 +1118,16 @@ fail:
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_prepare_planes);
>  
> +bool plane_crtc_active(struct drm_plane_state *state)
> +{
> +	return state->crtc && state->crtc->state->active;
> +}
> +
>  /**
>   * drm_atomic_helper_commit_planes - commit plane state
>   * @dev: DRM device
>   * @old_state: atomic state object with old state structures
> + * @active_only: Only commit on active CRTC if set
>   *
>   * This function commits the new plane state using the plane and atomic helper
>   * functions for planes and crtcs. It assumes that the atomic state has already
> @@ -1128,7 +1142,8 @@ EXPORT_SYMBOL(drm_atomic_helper_prepare_planes);
>   * drm_atomic_helper_commit_planes_on_crtc() instead.
>   */
>  void drm_atomic_helper_commit_planes(struct drm_device *dev,
> -				     struct drm_atomic_state *old_state)
> +				     struct drm_atomic_state *old_state,
> +				     bool active_only)
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *old_crtc_state;
> @@ -1144,6 +1159,9 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>  		if (!funcs || !funcs->atomic_begin)
>  			continue;
>  
> +		if (active_only && !crtc->state->active)
> +			continue;
> +
>  		funcs->atomic_begin(crtc);
>  	}
>  
> @@ -1155,6 +1173,10 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>  		if (!funcs)
>  			continue;
>  
> +		if (active_only && !plane_crtc_active(plane->state) &&
> +		    !plane_crtc_active(old_plane_state))
> +			continue;
> +
>  		/*
>  		 * Special-case disabling the plane if drivers support it.
>  		 */
>
The check is still wrong. It should only check if the new plane_state->crtc is active, if it's not call the disable_plane hook, but only when get_existing_crtc_state(old_plane_state->crtc)->active && !needs_modeset(old_plane_state->crtc->state).

The other helper should be a disable_planes helper that blindly calls disable_plane for planes that are on crtc's and require modeset for the crtc on the previous plane_state.

~Maarten
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel