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.ro...@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vet...@ffwll.ch>
Yeah, I agree this is probably a better way to go.
Reviewed-by: Matt Roper <matthew.d.ro...@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
