Re: [RFC PATCH 04/11] drm: Add __drm_framebuffer_remove_atomic

[Daniel Vetter]

On Tue, Oct 11, 2016 at 03:54:01PM +0100, Brian Starkey wrote:
> Implement the CRTC/Plane disable functionality of drm_framebuffer_remove
> using the atomic API, and use it if possible.
> 
> For atomic drivers, this removes the possibility of several commits when
> a framebuffer is in use by more than one CRTC/plane.
> 
> Additionally, this will provide a suitable place to support the removal
> of a framebuffer from a writeback connector, in the case that a
> writeback connector is still actively using a framebuffer when it is
> removed by userspace.
> 
> Signed-off-by: Brian Starkey 

Just the small comment here: Last time around I wanted toland an atomic
disable function for fb remove code it blew up. Need to check out git
history to make sure we've addressed those isses.  Caveat emperor ;-)
-Daniel

> ---
>  drivers/gpu/drm/drm_framebuffer.c |  154 
> -
>  1 file changed, 152 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_framebuffer.c 
> b/drivers/gpu/drm/drm_framebuffer.c
> index 528f75d..b02cf73 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -22,6 +22,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -795,6 +796,148 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
>  EXPORT_SYMBOL(drm_framebuffer_cleanup);
>  
>  /**
> + * __drm_framebuffer_remove_atomic - atomic version of 
> __drm_framebuffer_remove
> + * @dev: drm device
> + * @fb: framebuffer to remove
> + *
> + * If the driver implements the atomic API, we can handle the disabling of 
> all
> + * CRTCs/planes which use a framebuffer which is going away in a single 
> atomic
> + * commit.
> + *
> + * This scans all CRTCs and planes in @dev's mode_config. If they're using 
> @fb,
> + * it is removed and the CRTC/plane disabled.
> + * The legacy references are dropped and the ->fb pointers set to NULL
> + * accordingly.
> + *
> + * Returns:
> + * true if the framebuffer was successfully removed from use
> + */
> +static bool __drm_framebuffer_remove_atomic(struct drm_device *dev,
> + struct drm_framebuffer *fb)
> +{
> + struct drm_modeset_acquire_ctx ctx;
> + struct drm_atomic_state *state;
> + struct drm_connector_state *conn_state;
> + struct drm_connector *connector;
> + struct drm_plane *plane;
> + struct drm_crtc *crtc;
> + unsigned plane_mask;
> + int i, ret;
> +
> + drm_modeset_acquire_init(, 0);
> +
> + state = drm_atomic_state_alloc(dev);
> + if (!state)
> + return false;
> +
> + state->acquire_ctx = 
> +
> +retry:
> + drm_for_each_crtc(crtc, dev) {
> + struct drm_plane_state *primary_state;
> + struct drm_crtc_state *crtc_state;
> +
> + primary_state = drm_atomic_get_plane_state(state, 
> crtc->primary);
> + if (IS_ERR(primary_state)) {
> + ret = PTR_ERR(primary_state);
> + goto fail;
> + }
> +
> + if (primary_state->fb != fb)
> + continue;
> +
> + crtc_state = drm_atomic_get_crtc_state(state, crtc);
> + if (IS_ERR(crtc_state)) {
> + ret = PTR_ERR(crtc_state);
> + goto fail;
> + }
> +
> + /* Only handle the CRTC itself here, handle the plane later */
> + ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
> + if (ret != 0)
> + goto fail;
> +
> + crtc_state->active = false;
> +
> + /* Get the connectors in order to disable them */
> + ret = drm_atomic_add_affected_connectors(state, crtc);
> + if (ret)
> + goto fail;
> + }
> +
> + plane_mask = 0;
> + drm_for_each_plane(plane, dev) {
> + struct drm_plane_state *plane_state;
> +
> + plane_state = drm_atomic_get_plane_state(state, plane);
> + if (IS_ERR(plane_state)) {
> + ret = PTR_ERR(plane_state);
> + goto fail;
> + }
> +
> + if (plane_state->fb != fb)
> + continue;
> +
> + plane->old_fb = plane->fb;
> + plane_mask |= 1 << drm_plane_index(plane);
> +
> + /*
> +  * Open-coded copy of __drm_atomic_helper_disable_plane to avoid
> +  * a dependency on atomic-helper
> +  */
> + ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> + if (ret != 0)
> + goto fail;
> +
> + drm_atomic_set_fb_for_plane(plane_state, NULL);
> + plane_state->crtc_x = 0;
> + plane_state->crtc_y = 0;
> + plane_state->crtc_w = 0;
> + plane_state->crtc_h = 0;
> + plane_state->src_x = 0;
> + plane_state->src_y = 0;
> +  

[RFC PATCH 04/11] drm: Add __drm_framebuffer_remove_atomic

[Brian Starkey]

Implement the CRTC/Plane disable functionality of drm_framebuffer_remove
using the atomic API, and use it if possible.

For atomic drivers, this removes the possibility of several commits when
a framebuffer is in use by more than one CRTC/plane.

Additionally, this will provide a suitable place to support the removal
of a framebuffer from a writeback connector, in the case that a
writeback connector is still actively using a framebuffer when it is
removed by userspace.

Signed-off-by: Brian Starkey 
---
 drivers/gpu/drm/drm_framebuffer.c |  154 -
 1 file changed, 152 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_framebuffer.c 
b/drivers/gpu/drm/drm_framebuffer.c
index 528f75d..b02cf73 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -22,6 +22,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -795,6 +796,148 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
 EXPORT_SYMBOL(drm_framebuffer_cleanup);
 
 /**
+ * __drm_framebuffer_remove_atomic - atomic version of __drm_framebuffer_remove
+ * @dev: drm device
+ * @fb: framebuffer to remove
+ *
+ * If the driver implements the atomic API, we can handle the disabling of all
+ * CRTCs/planes which use a framebuffer which is going away in a single atomic
+ * commit.
+ *
+ * This scans all CRTCs and planes in @dev's mode_config. If they're using @fb,
+ * it is removed and the CRTC/plane disabled.
+ * The legacy references are dropped and the ->fb pointers set to NULL
+ * accordingly.
+ *
+ * Returns:
+ * true if the framebuffer was successfully removed from use
+ */
+static bool __drm_framebuffer_remove_atomic(struct drm_device *dev,
+   struct drm_framebuffer *fb)
+{
+   struct drm_modeset_acquire_ctx ctx;
+   struct drm_atomic_state *state;
+   struct drm_connector_state *conn_state;
+   struct drm_connector *connector;
+   struct drm_plane *plane;
+   struct drm_crtc *crtc;
+   unsigned plane_mask;
+   int i, ret;
+
+   drm_modeset_acquire_init(, 0);
+
+   state = drm_atomic_state_alloc(dev);
+   if (!state)
+   return false;
+
+   state->acquire_ctx = 
+
+retry:
+   drm_for_each_crtc(crtc, dev) {
+   struct drm_plane_state *primary_state;
+   struct drm_crtc_state *crtc_state;
+
+   primary_state = drm_atomic_get_plane_state(state, 
crtc->primary);
+   if (IS_ERR(primary_state)) {
+   ret = PTR_ERR(primary_state);
+   goto fail;
+   }
+
+   if (primary_state->fb != fb)
+   continue;
+
+   crtc_state = drm_atomic_get_crtc_state(state, crtc);
+   if (IS_ERR(crtc_state)) {
+   ret = PTR_ERR(crtc_state);
+   goto fail;
+   }
+
+   /* Only handle the CRTC itself here, handle the plane later */
+   ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
+   if (ret != 0)
+   goto fail;
+
+   crtc_state->active = false;
+
+   /* Get the connectors in order to disable them */
+   ret = drm_atomic_add_affected_connectors(state, crtc);
+   if (ret)
+   goto fail;
+   }
+
+   plane_mask = 0;
+   drm_for_each_plane(plane, dev) {
+   struct drm_plane_state *plane_state;
+
+   plane_state = drm_atomic_get_plane_state(state, plane);
+   if (IS_ERR(plane_state)) {
+   ret = PTR_ERR(plane_state);
+   goto fail;
+   }
+
+   if (plane_state->fb != fb)
+   continue;
+
+   plane->old_fb = plane->fb;
+   plane_mask |= 1 << drm_plane_index(plane);
+
+   /*
+* Open-coded copy of __drm_atomic_helper_disable_plane to avoid
+* a dependency on atomic-helper
+*/
+   ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
+   if (ret != 0)
+   goto fail;
+
+   drm_atomic_set_fb_for_plane(plane_state, NULL);
+   plane_state->crtc_x = 0;
+   plane_state->crtc_y = 0;
+   plane_state->crtc_w = 0;
+   plane_state->crtc_h = 0;
+   plane_state->src_x = 0;
+   plane_state->src_y = 0;
+   plane_state->src_w = 0;
+   plane_state->src_h = 0;
+   }
+
+   /* All of the connectors in state need disabling */
+   for_each_connector_in_state(state, connector, conn_state, i) {
+   ret = drm_atomic_set_crtc_for_connector(conn_state,
+   NULL);
+   if (ret)
+   goto fail;
+   }
+
+   if