Re: [PATCH] drm/plane-helper: Don't fake-implement primary plane disabling

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

On Tue, Apr 15, 2014 at 10:02:43AM +0200, Daniel Vetter wrote:
> After thinking about this topic a bit more I've reached the conclusion
> that implementing this doesn't make sense:
> 
> - The locking is all wrong: set_config(NULL) will also unlink encoders
>   and connectors, but those links are protected with the mode_config
>   mutex. In the ->disable_plane callback we only hold all modeset
>   locks, but eventually we want to switch to just grabbing the
>   per-crtc (and maybe per-plane) locks as needed, maybe based on
>   ww_mutexes. Having a callback which absolutely needs all modeset
>   locks is bad for this conversion.
> 
>   Note that the same isn't true for the provided ->update_plane since
>   we've audited the crtc helpers to make sure that not encoder or
>   connector links are changed.
> 
> - There's no way to re-enable the plane with an ->update_plane: The
>   connectors/encoder links are lost and so we can't re-enable the
>   CRTC. Even without that issue the driver might have reassigned some
>   shared resources (as opposed to e.g. DPMS off, where drivers are not
>   allowed to do that to make sure the CRTC can be enabled again).
> 
> - The semantics don't make much sense: Userspace asked to scan out
>   black (or some other color if the driver supports a background
>   color), not that the screen be disabled.
> 
> - Implementing proper primary plane support (i.e. actually disabling
>   the primary plane without disabling the CRTC) is really simple, at
>   least if all the hw needs is flipping a bit. The big task is
>   auditing all the interactions with other ioctls when the CRTC is on
>   but there's no primary plane (e.g. pageflips). And some of that work
>   still needs to be done.
> 
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

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

Although I'm not sure EINVAL is the best choice here. Maybe ENOSYS?

> ---
>  drivers/gpu/drm/drm_plane_helper.c | 33 +++++----------------------------
>  1 file changed, 5 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index 9263fd5efa58..9540ff9f97fe 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -205,9 +205,9 @@ EXPORT_SYMBOL(drm_primary_helper_update);
>   *
>   * Provides a default plane disable handler for primary planes.  This is handler
>   * is called in response to a userspace SetPlane operation on the plane with a
> - * NULL framebuffer parameter.  We call the driver's modeset handler with a NULL
> - * framebuffer to disable the CRTC if no other planes are currently enabled.
> - * If other planes are still enabled on the same CRTC, we return -EBUSY.
> + * NULL framebuffer parameter.  It unconditionally fails the disable call with
> + * -EINVAL the only way to disable the primary plane without driver support is
> + * to disable the entier CRTC. Which does not match the plane ->disable hook.
>   *
>   * Note that some hardware may be able to disable the primary plane without
>   * disabling the whole CRTC.  Drivers for such hardware should provide their
> @@ -216,34 +216,11 @@ EXPORT_SYMBOL(drm_primary_helper_update);
>   * disabled primary plane).
>   *
>   * RETURNS:
> - * Zero on success, error code on failure
> + * Unconditionally returns -EINVAL.
>   */
>  int drm_primary_helper_disable(struct drm_plane *plane)
>  {
> -	struct drm_plane *p;
> -	struct drm_mode_set set = {
> -		.crtc = plane->crtc,
> -		.fb = NULL,
> -	};
> -
> -	if (plane->crtc == NULL || plane->fb == NULL)
> -		/* Already disabled */
> -		return 0;
> -
> -	list_for_each_entry(p, &plane->dev->mode_config.plane_list, head)
> -		if (p != plane && p->fb) {
> -			DRM_DEBUG_KMS("Cannot disable primary plane while other planes are still active on CRTC.\n");
> -			return -EBUSY;
> -		}
> -
> -	/*
> -	 * N.B.  We call set_config() directly here rather than
> -	 * drm_mode_set_config_internal() since drm_mode_setplane() already
> -	 * handles the basic refcounting and we don't need the special
> -	 * cross-CRTC refcounting (no chance of stealing connectors from
> -	 * other CRTC's with this update).
> -	 */
> -	return plane->crtc->funcs->set_config(&set);
> +	return -EINVAL;
>  }
>  EXPORT_SYMBOL(drm_primary_helper_disable);
>  
> -- 
> 1.9.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC