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

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

[Daniel Vetter]

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>
---
 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

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

[Ville Syrjälä]

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

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

[Daniel Vetter]

On Tue, Apr 15, 2014 at 12:22 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> Although I'm not sure EINVAL is the best choice here. Maybe ENOSYS?

We return -EINVAL everywhere else where we don't support a giving
config, e.g. lack of dpll, unsupported plane scaling, pixel format
mismatch of the primary plane (since we don't yet have per-gen lists
of supported formats for the primary plane) and so on. So I've figured
-EINVAL is good enough until we have something better with the test
mode for atomic modesets.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Matt Roper]

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>

Yeah, I agree this is probably a better way to go.

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


Matt

> ---
>  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
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795