Re: [DPU PATCH 10/11] drm/msm: Switch to atomic_helper_commit()

[abhinavk]

On 2018-03-12 13:32, Sean Paul wrote:
On Thu, Mar 08, 2018 at 07:28:08PM -0800, abhin...@codeaurora.org 
wrote:

On 2018-02-28 11:19, Sean Paul wrote:
> Now that all of the msm-specific goo is tucked safely away we can switch
> over to using the atomic helper commit directly. \o/
>
[Abhinav] Can we say something like "Move remaining msm-specific 
operations

to private handles and switch to atomic helper commit for the rest.


Well, to be pedantic, that's not what's happening in this change. This 
change
simply replaces the msm atomic ops with the generic helper ops, which 
are

now identical due to the preceding 9 patches.

Sean


[Abhinav] Ok, I commented "remaining" because of msm_atomic_commit_tail
So something like this would be better "Replace the msm atomic ops with 
the generic helper ones

and move any exceptions to private handles" ?
But this is just trivial comment. So approving this.




> Change-Id: Ieab0bd0c526b2a9d3b3345eeba402ac4857fe418
> Signed-off-by: Sean Paul 

Reviewed-by: Abhinav Kumar 

> ---
>  drivers/gpu/drm/msm/msm_atomic.c | 120 +--
>  drivers/gpu/drm/msm/msm_drv.c|   7 +-
>  drivers/gpu/drm/msm/msm_drv.h|   3 +-
>  3 files changed, 8 insertions(+), 122 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c
> b/drivers/gpu/drm/msm/msm_atomic.c
> index 204c66cbfd31..91ffded576d8 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -18,8 +18,6 @@
>
>  #include "msm_drv.h"
>  #include "msm_kms.h"
> -#include "msm_gem.h"
> -#include "msm_fence.h"
>
>  static void msm_atomic_wait_for_commit_done(
>struct drm_device *dev,
> @@ -39,7 +37,7 @@ static void msm_atomic_wait_for_commit_done(
>}
>  }
>
> -static void msm_atomic_commit_tail(struct drm_atomic_state *state)
> +void msm_atomic_commit_tail(struct drm_atomic_state *state)
>  {
>struct drm_device *dev = state->dev;
>struct msm_drm_private *priv = dev->dev_private;
> @@ -81,119 +79,3 @@ static void msm_atomic_commit_tail(struct
> drm_atomic_state *state)
>
>kms->funcs->complete_commit(kms, state);
>  }
> -
> -/* The (potentially) asynchronous part of the commit.  At this point
> - * nothing can fail short of armageddon.
> - */
> -static void commit_tail(struct drm_atomic_state *state)
> -{
> -  drm_atomic_helper_wait_for_fences(state->dev, state, false);
> -
> -  drm_atomic_helper_wait_for_dependencies(state);
> -
> -  msm_atomic_commit_tail(state);
> -
> -  drm_atomic_helper_commit_cleanup_done(state);
> -
> -  drm_atomic_state_put(state);
> -}
> -
> -static void commit_work(struct work_struct *work)
> -{
> -  struct drm_atomic_state *state = container_of(work,
> -struct drm_atomic_state,
> -commit_work);
> -  commit_tail(state);
> -}
> -
> -/**
> - * drm_atomic_helper_commit - commit validated state object
> - * @dev: DRM device
> - * @state: the driver state object
> - * @nonblock: nonblocking commit
> - *
> - * This function commits a with drm_atomic_helper_check() pre-validated
> state
> - * object. This can still fail when e.g. the framebuffer reservation
> fails.
> - *
> - * RETURNS
> - * Zero for success or -errno.
> - */
> -int msm_atomic_commit(struct drm_device *dev,
> -  struct drm_atomic_state *state, bool nonblock)
> -{
> -  struct msm_drm_private *priv = dev->dev_private;
> -  struct drm_crtc *crtc;
> -  struct drm_crtc_state *crtc_state;
> -  struct drm_plane *plane;
> -  struct drm_plane_state *old_plane_state, *new_plane_state;
> -  int i, ret;
> -
> -  /*
> -   * Note that plane->atomic_async_check() should fail if we need
> -   * to re-assign hwpipe or anything that touches global atomic
> -   * state, so we'll never go down the async update path in those
> -   * cases.
> -   */
> -  if (state->async_update) {
> -  ret = drm_atomic_helper_prepare_planes(dev, state);
> -  if (ret)
> -  return ret;
> -
> -  drm_atomic_helper_async_commit(dev, state);
> -  drm_atomic_helper_cleanup_planes(dev, state);
> -  return 0;
> -  }
> -
> -  ret = drm_atomic_helper_setup_commit(state, nonblock);
> -  if (ret)
> -  return ret;
> -
> -  INIT_WORK(>commit_work, commit_work);
> -
> -  ret = drm_atomic_helper_prepare_planes(dev, state);
> -  if (ret)
> -  return ret;
> -
> -  if (!nonblock) {
> -  ret = drm_atomic_helper_wait_for_fences(dev, state, true);
> -  if (ret)
> -  goto error;
> -  }
> -
> -  /*
> -   * This is the point of no return - everything below never fails
> except
> -   * when the hw goes bonghits. Which means we can commit the new state
> on
> -   * the software side now.
> -   *
> -   * swap driver private state while still holding state_lock
> -   */
> -  BUG_ON(drm_atomic_helper_swap_state(state, false) < 0);
> -
> -  /*
> -   * Everything 

Re: [DPU PATCH 10/11] drm/msm: Switch to atomic_helper_commit()

[Sean Paul]

On Thu, Mar 08, 2018 at 07:28:08PM -0800, abhin...@codeaurora.org wrote:
> On 2018-02-28 11:19, Sean Paul wrote:
> > Now that all of the msm-specific goo is tucked safely away we can switch
> > over to using the atomic helper commit directly. \o/
> > 
> [Abhinav] Can we say something like "Move remaining msm-specific operations
> to private handles and switch to atomic helper commit for the rest.

Well, to be pedantic, that's not what's happening in this change. This change
simply replaces the msm atomic ops with the generic helper ops, which are
now identical due to the preceding 9 patches.

Sean

> 
> > Change-Id: Ieab0bd0c526b2a9d3b3345eeba402ac4857fe418
> > Signed-off-by: Sean Paul 
> > ---
> >  drivers/gpu/drm/msm/msm_atomic.c | 120 +--
> >  drivers/gpu/drm/msm/msm_drv.c|   7 +-
> >  drivers/gpu/drm/msm/msm_drv.h|   3 +-
> >  3 files changed, 8 insertions(+), 122 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/msm_atomic.c
> > b/drivers/gpu/drm/msm/msm_atomic.c
> > index 204c66cbfd31..91ffded576d8 100644
> > --- a/drivers/gpu/drm/msm/msm_atomic.c
> > +++ b/drivers/gpu/drm/msm/msm_atomic.c
> > @@ -18,8 +18,6 @@
> > 
> >  #include "msm_drv.h"
> >  #include "msm_kms.h"
> > -#include "msm_gem.h"
> > -#include "msm_fence.h"
> > 
> >  static void msm_atomic_wait_for_commit_done(
> > struct drm_device *dev,
> > @@ -39,7 +37,7 @@ static void msm_atomic_wait_for_commit_done(
> > }
> >  }
> > 
> > -static void msm_atomic_commit_tail(struct drm_atomic_state *state)
> > +void msm_atomic_commit_tail(struct drm_atomic_state *state)
> >  {
> > struct drm_device *dev = state->dev;
> > struct msm_drm_private *priv = dev->dev_private;
> > @@ -81,119 +79,3 @@ static void msm_atomic_commit_tail(struct
> > drm_atomic_state *state)
> > 
> > kms->funcs->complete_commit(kms, state);
> >  }
> > -
> > -/* The (potentially) asynchronous part of the commit.  At this point
> > - * nothing can fail short of armageddon.
> > - */
> > -static void commit_tail(struct drm_atomic_state *state)
> > -{
> > -   drm_atomic_helper_wait_for_fences(state->dev, state, false);
> > -
> > -   drm_atomic_helper_wait_for_dependencies(state);
> > -
> > -   msm_atomic_commit_tail(state);
> > -
> > -   drm_atomic_helper_commit_cleanup_done(state);
> > -
> > -   drm_atomic_state_put(state);
> > -}
> > -
> > -static void commit_work(struct work_struct *work)
> > -{
> > -   struct drm_atomic_state *state = container_of(work,
> > - struct drm_atomic_state,
> > - commit_work);
> > -   commit_tail(state);
> > -}
> > -
> > -/**
> > - * drm_atomic_helper_commit - commit validated state object
> > - * @dev: DRM device
> > - * @state: the driver state object
> > - * @nonblock: nonblocking commit
> > - *
> > - * This function commits a with drm_atomic_helper_check() pre-validated
> > state
> > - * object. This can still fail when e.g. the framebuffer reservation
> > fails.
> > - *
> > - * RETURNS
> > - * Zero for success or -errno.
> > - */
> > -int msm_atomic_commit(struct drm_device *dev,
> > -   struct drm_atomic_state *state, bool nonblock)
> > -{
> > -   struct msm_drm_private *priv = dev->dev_private;
> > -   struct drm_crtc *crtc;
> > -   struct drm_crtc_state *crtc_state;
> > -   struct drm_plane *plane;
> > -   struct drm_plane_state *old_plane_state, *new_plane_state;
> > -   int i, ret;
> > -
> > -   /*
> > -* Note that plane->atomic_async_check() should fail if we need
> > -* to re-assign hwpipe or anything that touches global atomic
> > -* state, so we'll never go down the async update path in those
> > -* cases.
> > -*/
> > -   if (state->async_update) {
> > -   ret = drm_atomic_helper_prepare_planes(dev, state);
> > -   if (ret)
> > -   return ret;
> > -
> > -   drm_atomic_helper_async_commit(dev, state);
> > -   drm_atomic_helper_cleanup_planes(dev, state);
> > -   return 0;
> > -   }
> > -
> > -   ret = drm_atomic_helper_setup_commit(state, nonblock);
> > -   if (ret)
> > -   return ret;
> > -
> > -   INIT_WORK(>commit_work, commit_work);
> > -
> > -   ret = drm_atomic_helper_prepare_planes(dev, state);
> > -   if (ret)
> > -   return ret;
> > -
> > -   if (!nonblock) {
> > -   ret = drm_atomic_helper_wait_for_fences(dev, state, true);
> > -   if (ret)
> > -   goto error;
> > -   }
> > -
> > -   /*
> > -* This is the point of no return - everything below never fails
> > except
> > -* when the hw goes bonghits. Which means we can commit the new state
> > on
> > -* the software side now.
> > -*
> > -* swap driver private state while still holding state_lock
> > -*/
> > -   BUG_ON(drm_atomic_helper_swap_state(state, false) < 0);
> > -
> > -   /*
> > -* Everything below can be run asynchronously without 

Re: [DPU PATCH 10/11] drm/msm: Switch to atomic_helper_commit()

[abhinavk]

On 2018-02-28 11:19, Sean Paul wrote:
Now that all of the msm-specific goo is tucked safely away we can 
switch

over to using the atomic helper commit directly. \o/

[Abhinav] Can we say something like "Move remaining msm-specific 
operations to private handles and switch to atomic helper commit for the 
rest.



Change-Id: Ieab0bd0c526b2a9d3b3345eeba402ac4857fe418
Signed-off-by: Sean Paul 
---
 drivers/gpu/drm/msm/msm_atomic.c | 120 +--
 drivers/gpu/drm/msm/msm_drv.c|   7 +-
 drivers/gpu/drm/msm/msm_drv.h|   3 +-
 3 files changed, 8 insertions(+), 122 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_atomic.c 
b/drivers/gpu/drm/msm/msm_atomic.c

index 204c66cbfd31..91ffded576d8 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -18,8 +18,6 @@

 #include "msm_drv.h"
 #include "msm_kms.h"
-#include "msm_gem.h"
-#include "msm_fence.h"

 static void msm_atomic_wait_for_commit_done(
struct drm_device *dev,
@@ -39,7 +37,7 @@ static void msm_atomic_wait_for_commit_done(
}
 }

-static void msm_atomic_commit_tail(struct drm_atomic_state *state)
+void msm_atomic_commit_tail(struct drm_atomic_state *state)
 {
struct drm_device *dev = state->dev;
struct msm_drm_private *priv = dev->dev_private;
@@ -81,119 +79,3 @@ static void msm_atomic_commit_tail(struct
drm_atomic_state *state)

kms->funcs->complete_commit(kms, state);
 }
-
-/* The (potentially) asynchronous part of the commit.  At this point
- * nothing can fail short of armageddon.
- */
-static void commit_tail(struct drm_atomic_state *state)
-{
-   drm_atomic_helper_wait_for_fences(state->dev, state, false);
-
-   drm_atomic_helper_wait_for_dependencies(state);
-
-   msm_atomic_commit_tail(state);
-
-   drm_atomic_helper_commit_cleanup_done(state);
-
-   drm_atomic_state_put(state);
-}
-
-static void commit_work(struct work_struct *work)
-{
-   struct drm_atomic_state *state = container_of(work,
- struct drm_atomic_state,
- commit_work);
-   commit_tail(state);
-}
-
-/**
- * drm_atomic_helper_commit - commit validated state object
- * @dev: DRM device
- * @state: the driver state object
- * @nonblock: nonblocking commit
- *
- * This function commits a with drm_atomic_helper_check() 
pre-validated state
- * object. This can still fail when e.g. the framebuffer reservation 
fails.

- *
- * RETURNS
- * Zero for success or -errno.
- */
-int msm_atomic_commit(struct drm_device *dev,
-   struct drm_atomic_state *state, bool nonblock)
-{
-   struct msm_drm_private *priv = dev->dev_private;
-   struct drm_crtc *crtc;
-   struct drm_crtc_state *crtc_state;
-   struct drm_plane *plane;
-   struct drm_plane_state *old_plane_state, *new_plane_state;
-   int i, ret;
-
-   /*
-* Note that plane->atomic_async_check() should fail if we need
-* to re-assign hwpipe or anything that touches global atomic
-* state, so we'll never go down the async update path in those
-* cases.
-*/
-   if (state->async_update) {
-   ret = drm_atomic_helper_prepare_planes(dev, state);
-   if (ret)
-   return ret;
-
-   drm_atomic_helper_async_commit(dev, state);
-   drm_atomic_helper_cleanup_planes(dev, state);
-   return 0;
-   }
-
-   ret = drm_atomic_helper_setup_commit(state, nonblock);
-   if (ret)
-   return ret;
-
-   INIT_WORK(>commit_work, commit_work);
-
-   ret = drm_atomic_helper_prepare_planes(dev, state);
-   if (ret)
-   return ret;
-
-   if (!nonblock) {
-   ret = drm_atomic_helper_wait_for_fences(dev, state, true);
-   if (ret)
-   goto error;
-   }
-
-   /*
-	 * This is the point of no return - everything below never fails 
except
-	 * when the hw goes bonghits. Which means we can commit the new state 
on

-* the software side now.
-*
-* swap driver private state while still holding state_lock
-*/
-   BUG_ON(drm_atomic_helper_swap_state(state, false) < 0);
-
-   /*
-	 * Everything below can be run asynchronously without the need to 
grab
-	 * any modeset locks at all under one conditions: It must be 
guaranteed
-	 * that the asynchronous work has either been cancelled (if the 
driver

-* supports it, which at least requires that the framebuffers get
-* cleaned up with drm_atomic_helper_cleanup_planes()) or completed
-* before the new state gets committed on the software side with
-* drm_atomic_helper_swap_state().
-*
-* This scheme allows new atomic state updates to be prepared and
-* checked in parallel to the asynchronous completion of the previous

[DPU PATCH 10/11] drm/msm: Switch to atomic_helper_commit()

[Sean Paul]

Now that all of the msm-specific goo is tucked safely away we can switch
over to using the atomic helper commit directly. \o/

Change-Id: Ieab0bd0c526b2a9d3b3345eeba402ac4857fe418
Signed-off-by: Sean Paul 
---
 drivers/gpu/drm/msm/msm_atomic.c | 120 +--
 drivers/gpu/drm/msm/msm_drv.c|   7 +-
 drivers/gpu/drm/msm/msm_drv.h|   3 +-
 3 files changed, 8 insertions(+), 122 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index 204c66cbfd31..91ffded576d8 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -18,8 +18,6 @@
 
 #include "msm_drv.h"
 #include "msm_kms.h"
-#include "msm_gem.h"
-#include "msm_fence.h"
 
 static void msm_atomic_wait_for_commit_done(
struct drm_device *dev,
@@ -39,7 +37,7 @@ static void msm_atomic_wait_for_commit_done(
}
 }
 
-static void msm_atomic_commit_tail(struct drm_atomic_state *state)
+void msm_atomic_commit_tail(struct drm_atomic_state *state)
 {
struct drm_device *dev = state->dev;
struct msm_drm_private *priv = dev->dev_private;
@@ -81,119 +79,3 @@ static void msm_atomic_commit_tail(struct drm_atomic_state 
*state)
 
kms->funcs->complete_commit(kms, state);
 }
-
-/* The (potentially) asynchronous part of the commit.  At this point
- * nothing can fail short of armageddon.
- */
-static void commit_tail(struct drm_atomic_state *state)
-{
-   drm_atomic_helper_wait_for_fences(state->dev, state, false);
-
-   drm_atomic_helper_wait_for_dependencies(state);
-
-   msm_atomic_commit_tail(state);
-
-   drm_atomic_helper_commit_cleanup_done(state);
-
-   drm_atomic_state_put(state);
-}
-
-static void commit_work(struct work_struct *work)
-{
-   struct drm_atomic_state *state = container_of(work,
- struct drm_atomic_state,
- commit_work);
-   commit_tail(state);
-}
-
-/**
- * drm_atomic_helper_commit - commit validated state object
- * @dev: DRM device
- * @state: the driver state object
- * @nonblock: nonblocking commit
- *
- * This function commits a with drm_atomic_helper_check() pre-validated state
- * object. This can still fail when e.g. the framebuffer reservation fails.
- *
- * RETURNS
- * Zero for success or -errno.
- */
-int msm_atomic_commit(struct drm_device *dev,
-   struct drm_atomic_state *state, bool nonblock)
-{
-   struct msm_drm_private *priv = dev->dev_private;
-   struct drm_crtc *crtc;
-   struct drm_crtc_state *crtc_state;
-   struct drm_plane *plane;
-   struct drm_plane_state *old_plane_state, *new_plane_state;
-   int i, ret;
-
-   /*
-* Note that plane->atomic_async_check() should fail if we need
-* to re-assign hwpipe or anything that touches global atomic
-* state, so we'll never go down the async update path in those
-* cases.
-*/
-   if (state->async_update) {
-   ret = drm_atomic_helper_prepare_planes(dev, state);
-   if (ret)
-   return ret;
-
-   drm_atomic_helper_async_commit(dev, state);
-   drm_atomic_helper_cleanup_planes(dev, state);
-   return 0;
-   }
-
-   ret = drm_atomic_helper_setup_commit(state, nonblock);
-   if (ret)
-   return ret;
-
-   INIT_WORK(>commit_work, commit_work);
-
-   ret = drm_atomic_helper_prepare_planes(dev, state);
-   if (ret)
-   return ret;
-
-   if (!nonblock) {
-   ret = drm_atomic_helper_wait_for_fences(dev, state, true);
-   if (ret)
-   goto error;
-   }
-
-   /*
-* This is the point of no return - everything below never fails except
-* when the hw goes bonghits. Which means we can commit the new state on
-* the software side now.
-*
-* swap driver private state while still holding state_lock
-*/
-   BUG_ON(drm_atomic_helper_swap_state(state, false) < 0);
-
-   /*
-* Everything below can be run asynchronously without the need to grab
-* any modeset locks at all under one conditions: It must be guaranteed
-* that the asynchronous work has either been cancelled (if the driver
-* supports it, which at least requires that the framebuffers get
-* cleaned up with drm_atomic_helper_cleanup_planes()) or completed
-* before the new state gets committed on the software side with
-* drm_atomic_helper_swap_state().
-*
-* This scheme allows new atomic state updates to be prepared and
-* checked in parallel to the asynchronous completion of the previous
-* update. Which is important since compositors need to figure out the
-* composition of the next frame right after having submitted the
-*