Re: [Intel-gfx] [PATCH 02/23] drm/atomic-helper: Make drm_atomic_helper_disable_all() update the plane->fb pointers

[Ville Syrjälä]

On Mon, Mar 26, 2018 at 10:28:06PM +0200, Daniel Vetter wrote:
> On Thu, Mar 22, 2018 at 05:22:52PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä 
> > 
> > drm_atomic_helper_shutdown() needs to release the reference held by
> > plane->fb, so we want to use drm_atomic_clean_old_fb() in
> > drm_atomic_helper_disable_all(). However during suspend/resume, gpu
> > reset and load detection we should probably leave that stuff alone,
> > as otherwise we'd have to make sure we put them back again when
> > we restore the duplicated state to the device. Seems simpler to me
> > to not touch any of it anyway.
> > 
> > v2: Don't inflict the clean_old_fbs bool to drivers (Daniel)
> > 
> > Cc: martin.pe...@free.fr
> > Cc: ch...@chris-wilson.co.uk
> > Cc: Dave Airlie  (v1)
> > Cc: Maarten Lankhorst 
> > Cc: Daniel Vetter 
> > Signed-off-by: Ville Syrjälä 
> 
> I think this would be cleaner diff to read if you squash the first 2
> patches together. Also avoids the bisect fail. With that (and I trust you
> to come up with a suitably merged commit message):
> 
> Reviewed-by: Daniel Vetter 

Squashed, and commit message rescribbeled. And with sufficient confidence
from a local smoke test I proceeded to push the easy ones 1-13 (except msm), 
and 22-23 (the load detect stuff for i915). I'll have to figure out the
correct merge order for the rest next week.

Thanks for the reviews.

> 
> I reviewed this by re-reading the analysis from 49d70aeaeca8f62b72b77 and
> trusting my former self :-)
> 
> Cheers, Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 67 
> > ++---
> >  1 file changed, 40 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index c48f187d08de..39a69508d8c9 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -2881,31 +2881,9 @@ int __drm_atomic_helper_set_config(struct 
> > drm_mode_set *set,
> > return 0;
> >  }
> >  
> > -/**
> > - * drm_atomic_helper_disable_all - disable all currently active outputs
> > - * @dev: DRM device
> > - * @ctx: lock acquisition context
> > - *
> > - * Loops through all connectors, finding those that aren't turned off and 
> > then
> > - * turns them off by setting their DPMS mode to OFF and deactivating the 
> > CRTC
> > - * that they are connected to.
> > - *
> > - * This is used for example in suspend/resume to disable all currently 
> > active
> > - * functions when suspending. If you just want to shut down everything at 
> > e.g.
> > - * driver unload, look at drm_atomic_helper_shutdown().
> > - *
> > - * Note that if callers haven't already acquired all modeset locks this 
> > might
> > - * return -EDEADLK, which must be handled by calling drm_modeset_backoff().
> > - *
> > - * Returns:
> > - * 0 on success or a negative error code on failure.
> > - *
> > - * See also:
> > - * drm_atomic_helper_suspend(), drm_atomic_helper_resume() and
> > - * drm_atomic_helper_shutdown().
> > - */
> > -int drm_atomic_helper_disable_all(struct drm_device *dev,
> > - struct drm_modeset_acquire_ctx *ctx)
> > +static int __drm_atomic_helper_disable_all(struct drm_device *dev,
> > +  struct drm_modeset_acquire_ctx *ctx,
> > +  bool clean_old_fbs)
> >  {
> > struct drm_atomic_state *state;
> > struct drm_connector_state *conn_state;
> > @@ -2914,6 +2892,7 @@ int drm_atomic_helper_disable_all(struct drm_device 
> > *dev,
> > struct drm_plane *plane;
> > struct drm_crtc_state *crtc_state;
> > struct drm_crtc *crtc;
> > +   unsigned int plane_mask = 0;
> > int ret, i;
> >  
> > state = drm_atomic_state_alloc(dev);
> > @@ -2956,14 +2935,48 @@ int drm_atomic_helper_disable_all(struct drm_device 
> > *dev,
> > goto free;
> >  
> > drm_atomic_set_fb_for_plane(plane_state, NULL);
> > +
> > +   if (clean_old_fbs) {
> > +   plane->old_fb = plane->fb;
> > +   plane_mask |= BIT(drm_plane_index(plane));
> > +   }
> > }
> >  
> > ret = drm_atomic_commit(state);
> >  free:
> > +   drm_atomic_clean_old_fb(dev, plane_mask, ret);
> > +
> > drm_atomic_state_put(state);
> > return ret;
> >  }
> > -
> > +/**
> > + * drm_atomic_helper_disable_all - disable all currently active outputs
> > + * @dev: DRM device
> > + * @ctx: lock acquisition context
> > + *
> > + * Loops through all connectors, finding those that aren't turned off and 
> > then
> > + * turns them off by setting their DPMS mode to OFF and deactivating the 
> > CRTC
> > + * that they are connected to.
> > + *
> > + * This is used for example in suspend/resume to disable all currently 
> > 

Re: [Intel-gfx] [PATCH 02/23] drm/atomic-helper: Make drm_atomic_helper_disable_all() update the plane->fb pointers

[Daniel Vetter]

On Thu, Mar 22, 2018 at 05:22:52PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> drm_atomic_helper_shutdown() needs to release the reference held by
> plane->fb, so we want to use drm_atomic_clean_old_fb() in
> drm_atomic_helper_disable_all(). However during suspend/resume, gpu
> reset and load detection we should probably leave that stuff alone,
> as otherwise we'd have to make sure we put them back again when
> we restore the duplicated state to the device. Seems simpler to me
> to not touch any of it anyway.
> 
> v2: Don't inflict the clean_old_fbs bool to drivers (Daniel)
> 
> Cc: martin.pe...@free.fr
> Cc: ch...@chris-wilson.co.uk
> Cc: Dave Airlie  (v1)

More funny (v1) I just noticed ...
-Daniel

> Cc: Maarten Lankhorst 
> Cc: Daniel Vetter 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 67 
> ++---
>  1 file changed, 40 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index c48f187d08de..39a69508d8c9 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2881,31 +2881,9 @@ int __drm_atomic_helper_set_config(struct drm_mode_set 
> *set,
>   return 0;
>  }
>  
> -/**
> - * drm_atomic_helper_disable_all - disable all currently active outputs
> - * @dev: DRM device
> - * @ctx: lock acquisition context
> - *
> - * Loops through all connectors, finding those that aren't turned off and 
> then
> - * turns them off by setting their DPMS mode to OFF and deactivating the CRTC
> - * that they are connected to.
> - *
> - * This is used for example in suspend/resume to disable all currently active
> - * functions when suspending. If you just want to shut down everything at 
> e.g.
> - * driver unload, look at drm_atomic_helper_shutdown().
> - *
> - * Note that if callers haven't already acquired all modeset locks this might
> - * return -EDEADLK, which must be handled by calling drm_modeset_backoff().
> - *
> - * Returns:
> - * 0 on success or a negative error code on failure.
> - *
> - * See also:
> - * drm_atomic_helper_suspend(), drm_atomic_helper_resume() and
> - * drm_atomic_helper_shutdown().
> - */
> -int drm_atomic_helper_disable_all(struct drm_device *dev,
> -   struct drm_modeset_acquire_ctx *ctx)
> +static int __drm_atomic_helper_disable_all(struct drm_device *dev,
> +struct drm_modeset_acquire_ctx *ctx,
> +bool clean_old_fbs)
>  {
>   struct drm_atomic_state *state;
>   struct drm_connector_state *conn_state;
> @@ -2914,6 +2892,7 @@ int drm_atomic_helper_disable_all(struct drm_device 
> *dev,
>   struct drm_plane *plane;
>   struct drm_crtc_state *crtc_state;
>   struct drm_crtc *crtc;
> + unsigned int plane_mask = 0;
>   int ret, i;
>  
>   state = drm_atomic_state_alloc(dev);
> @@ -2956,14 +2935,48 @@ int drm_atomic_helper_disable_all(struct drm_device 
> *dev,
>   goto free;
>  
>   drm_atomic_set_fb_for_plane(plane_state, NULL);
> +
> + if (clean_old_fbs) {
> + plane->old_fb = plane->fb;
> + plane_mask |= BIT(drm_plane_index(plane));
> + }
>   }
>  
>   ret = drm_atomic_commit(state);
>  free:
> + drm_atomic_clean_old_fb(dev, plane_mask, ret);
> +
>   drm_atomic_state_put(state);
>   return ret;
>  }
> -
> +/**
> + * drm_atomic_helper_disable_all - disable all currently active outputs
> + * @dev: DRM device
> + * @ctx: lock acquisition context
> + *
> + * Loops through all connectors, finding those that aren't turned off and 
> then
> + * turns them off by setting their DPMS mode to OFF and deactivating the CRTC
> + * that they are connected to.
> + *
> + * This is used for example in suspend/resume to disable all currently active
> + * functions when suspending. If you just want to shut down everything at 
> e.g.
> + * driver unload, look at drm_atomic_helper_shutdown().
> + *
> + * Note that if callers haven't already acquired all modeset locks this might
> + * return -EDEADLK, which must be handled by calling drm_modeset_backoff().
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + *
> + * See also:
> + * drm_atomic_helper_suspend(), drm_atomic_helper_resume() and
> + * drm_atomic_helper_shutdown().
> + */
> +int drm_atomic_helper_disable_all(struct drm_device *dev,
> +   struct drm_modeset_acquire_ctx *ctx)
> +{
> + return __drm_atomic_helper_disable_all(dev, ctx, false);
> +}
>  EXPORT_SYMBOL(drm_atomic_helper_disable_all);
>  
>  /**
> @@ -2986,7 +2999,7 @@ void drm_atomic_helper_shutdown(struct drm_device *dev)
>   while (1) {
>

Re: [Intel-gfx] [PATCH 02/23] drm/atomic-helper: Make drm_atomic_helper_disable_all() update the plane->fb pointers

[Daniel Vetter]

On Thu, Mar 22, 2018 at 05:22:52PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> drm_atomic_helper_shutdown() needs to release the reference held by
> plane->fb, so we want to use drm_atomic_clean_old_fb() in
> drm_atomic_helper_disable_all(). However during suspend/resume, gpu
> reset and load detection we should probably leave that stuff alone,
> as otherwise we'd have to make sure we put them back again when
> we restore the duplicated state to the device. Seems simpler to me
> to not touch any of it anyway.
> 
> v2: Don't inflict the clean_old_fbs bool to drivers (Daniel)
> 
> Cc: martin.pe...@free.fr
> Cc: ch...@chris-wilson.co.uk
> Cc: Dave Airlie  (v1)
> Cc: Maarten Lankhorst 
> Cc: Daniel Vetter 
> Signed-off-by: Ville Syrjälä 

I think this would be cleaner diff to read if you squash the first 2
patches together. Also avoids the bisect fail. With that (and I trust you
to come up with a suitably merged commit message):

Reviewed-by: Daniel Vetter 

I reviewed this by re-reading the analysis from 49d70aeaeca8f62b72b77 and
trusting my former self :-)

Cheers, Daniel

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 67 
> ++---
>  1 file changed, 40 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index c48f187d08de..39a69508d8c9 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2881,31 +2881,9 @@ int __drm_atomic_helper_set_config(struct drm_mode_set 
> *set,
>   return 0;
>  }
>  
> -/**
> - * drm_atomic_helper_disable_all - disable all currently active outputs
> - * @dev: DRM device
> - * @ctx: lock acquisition context
> - *
> - * Loops through all connectors, finding those that aren't turned off and 
> then
> - * turns them off by setting their DPMS mode to OFF and deactivating the CRTC
> - * that they are connected to.
> - *
> - * This is used for example in suspend/resume to disable all currently active
> - * functions when suspending. If you just want to shut down everything at 
> e.g.
> - * driver unload, look at drm_atomic_helper_shutdown().
> - *
> - * Note that if callers haven't already acquired all modeset locks this might
> - * return -EDEADLK, which must be handled by calling drm_modeset_backoff().
> - *
> - * Returns:
> - * 0 on success or a negative error code on failure.
> - *
> - * See also:
> - * drm_atomic_helper_suspend(), drm_atomic_helper_resume() and
> - * drm_atomic_helper_shutdown().
> - */
> -int drm_atomic_helper_disable_all(struct drm_device *dev,
> -   struct drm_modeset_acquire_ctx *ctx)
> +static int __drm_atomic_helper_disable_all(struct drm_device *dev,
> +struct drm_modeset_acquire_ctx *ctx,
> +bool clean_old_fbs)
>  {
>   struct drm_atomic_state *state;
>   struct drm_connector_state *conn_state;
> @@ -2914,6 +2892,7 @@ int drm_atomic_helper_disable_all(struct drm_device 
> *dev,
>   struct drm_plane *plane;
>   struct drm_crtc_state *crtc_state;
>   struct drm_crtc *crtc;
> + unsigned int plane_mask = 0;
>   int ret, i;
>  
>   state = drm_atomic_state_alloc(dev);
> @@ -2956,14 +2935,48 @@ int drm_atomic_helper_disable_all(struct drm_device 
> *dev,
>   goto free;
>  
>   drm_atomic_set_fb_for_plane(plane_state, NULL);
> +
> + if (clean_old_fbs) {
> + plane->old_fb = plane->fb;
> + plane_mask |= BIT(drm_plane_index(plane));
> + }
>   }
>  
>   ret = drm_atomic_commit(state);
>  free:
> + drm_atomic_clean_old_fb(dev, plane_mask, ret);
> +
>   drm_atomic_state_put(state);
>   return ret;
>  }
> -
> +/**
> + * drm_atomic_helper_disable_all - disable all currently active outputs
> + * @dev: DRM device
> + * @ctx: lock acquisition context
> + *
> + * Loops through all connectors, finding those that aren't turned off and 
> then
> + * turns them off by setting their DPMS mode to OFF and deactivating the CRTC
> + * that they are connected to.
> + *
> + * This is used for example in suspend/resume to disable all currently active
> + * functions when suspending. If you just want to shut down everything at 
> e.g.
> + * driver unload, look at drm_atomic_helper_shutdown().
> + *
> + * Note that if callers haven't already acquired all modeset locks this might
> + * return -EDEADLK, which must be handled by calling drm_modeset_backoff().
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + *
> + * See also:
> + * drm_atomic_helper_suspend(), drm_atomic_helper_resume() and
> + * drm_atomic_helper_shutdown().
> + */
> +int drm_atomic_helper_disable_all(struct 

[Intel-gfx] [PATCH 02/23] drm/atomic-helper: Make drm_atomic_helper_disable_all() update the plane->fb pointers

[Ville Syrjala]

From: Ville Syrjälä 

drm_atomic_helper_shutdown() needs to release the reference held by
plane->fb, so we want to use drm_atomic_clean_old_fb() in
drm_atomic_helper_disable_all(). However during suspend/resume, gpu
reset and load detection we should probably leave that stuff alone,
as otherwise we'd have to make sure we put them back again when
we restore the duplicated state to the device. Seems simpler to me
to not touch any of it anyway.

v2: Don't inflict the clean_old_fbs bool to drivers (Daniel)

Cc: martin.pe...@free.fr
Cc: ch...@chris-wilson.co.uk
Cc: Dave Airlie  (v1)
Cc: Maarten Lankhorst 
Cc: Daniel Vetter 
Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/drm_atomic_helper.c | 67 ++---
 1 file changed, 40 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index c48f187d08de..39a69508d8c9 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2881,31 +2881,9 @@ int __drm_atomic_helper_set_config(struct drm_mode_set 
*set,
return 0;
 }
 
-/**
- * drm_atomic_helper_disable_all - disable all currently active outputs
- * @dev: DRM device
- * @ctx: lock acquisition context
- *
- * Loops through all connectors, finding those that aren't turned off and then
- * turns them off by setting their DPMS mode to OFF and deactivating the CRTC
- * that they are connected to.
- *
- * This is used for example in suspend/resume to disable all currently active
- * functions when suspending. If you just want to shut down everything at e.g.
- * driver unload, look at drm_atomic_helper_shutdown().
- *
- * Note that if callers haven't already acquired all modeset locks this might
- * return -EDEADLK, which must be handled by calling drm_modeset_backoff().
- *
- * Returns:
- * 0 on success or a negative error code on failure.
- *
- * See also:
- * drm_atomic_helper_suspend(), drm_atomic_helper_resume() and
- * drm_atomic_helper_shutdown().
- */
-int drm_atomic_helper_disable_all(struct drm_device *dev,
- struct drm_modeset_acquire_ctx *ctx)
+static int __drm_atomic_helper_disable_all(struct drm_device *dev,
+  struct drm_modeset_acquire_ctx *ctx,
+  bool clean_old_fbs)
 {
struct drm_atomic_state *state;
struct drm_connector_state *conn_state;
@@ -2914,6 +2892,7 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
struct drm_plane *plane;
struct drm_crtc_state *crtc_state;
struct drm_crtc *crtc;
+   unsigned int plane_mask = 0;
int ret, i;
 
state = drm_atomic_state_alloc(dev);
@@ -2956,14 +2935,48 @@ int drm_atomic_helper_disable_all(struct drm_device 
*dev,
goto free;
 
drm_atomic_set_fb_for_plane(plane_state, NULL);
+
+   if (clean_old_fbs) {
+   plane->old_fb = plane->fb;
+   plane_mask |= BIT(drm_plane_index(plane));
+   }
}
 
ret = drm_atomic_commit(state);
 free:
+   drm_atomic_clean_old_fb(dev, plane_mask, ret);
+
drm_atomic_state_put(state);
return ret;
 }
-
+/**
+ * drm_atomic_helper_disable_all - disable all currently active outputs
+ * @dev: DRM device
+ * @ctx: lock acquisition context
+ *
+ * Loops through all connectors, finding those that aren't turned off and then
+ * turns them off by setting their DPMS mode to OFF and deactivating the CRTC
+ * that they are connected to.
+ *
+ * This is used for example in suspend/resume to disable all currently active
+ * functions when suspending. If you just want to shut down everything at e.g.
+ * driver unload, look at drm_atomic_helper_shutdown().
+ *
+ * Note that if callers haven't already acquired all modeset locks this might
+ * return -EDEADLK, which must be handled by calling drm_modeset_backoff().
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ *
+ * See also:
+ * drm_atomic_helper_suspend(), drm_atomic_helper_resume() and
+ * drm_atomic_helper_shutdown().
+ */
+int drm_atomic_helper_disable_all(struct drm_device *dev,
+ struct drm_modeset_acquire_ctx *ctx)
+{
+   return __drm_atomic_helper_disable_all(dev, ctx, false);
+}
 EXPORT_SYMBOL(drm_atomic_helper_disable_all);
 
 /**
@@ -2986,7 +2999,7 @@ void drm_atomic_helper_shutdown(struct drm_device *dev)
while (1) {
ret = drm_modeset_lock_all_ctx(dev, );
if (!ret)
-   ret = drm_atomic_helper_disable_all(dev, );
+   ret = __drm_atomic_helper_disable_all(dev, , true);
 
if (ret != -EDEADLK)
break;
-- 
2.16.1