Re: [Intel-gfx] [PATCH] drm/atomic: Move drm_crtc_commit to drm_crtc_state, v4.

[Daniel Vetter]

On Mon, Sep 04, 2017 at 05:04:56PM +0200, Maarten Lankhorst wrote:
> Most code only cares about the current commit or previous commit.
> Fortuantely we already have a place to track those. Move it to
> drm_crtc_state where it belongs. :)
> 
> The per-crtc commit_list is kept for places where we have to look
> deeper than the current or previous commit for checking whether to stall
> on unpin. This is used in drm_atomic_helper_setup_commit and
> intel_has_pending_fb_unpin.
> 
> Changes since v1:
> - Update kerneldoc for drm_crtc.commit_list. (danvet)
> Changes since v2:
> - Remove drm_atomic_helper_async_check hunk. (pinchartl)
> Changes since v3:
> - Fix use-after-free in drm_atomic_helper_commit_cleanup_done().
> 
> Signed-off-by: Maarten Lankhorst 
> Reviewed-by: Daniel Vetter 

Might be good to drop this, or at least annoate that the r-b is for v3,
just for next time around. But I looked at the patch again, r-b: me still
holds I think. But then I missed the bug in v4 ...
-Daniel


> ---
>  drivers/gpu/drm/drm_atomic.c|  7 
>  drivers/gpu/drm/drm_atomic_helper.c | 82 
> -
>  include/drm/drm_atomic.h|  1 -
>  include/drm/drm_crtc.h  | 23 +--
>  4 files changed, 54 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 2fd383d7253a..2cce48f203e0 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -163,13 +163,6 @@ void drm_atomic_state_default_clear(struct 
> drm_atomic_state *state)
>   crtc->funcs->atomic_destroy_state(crtc,
> state->crtcs[i].state);
>  
> - if (state->crtcs[i].commit) {
> - kfree(state->crtcs[i].commit->event);
> - state->crtcs[i].commit->event = NULL;
> - drm_crtc_commit_put(state->crtcs[i].commit);
> - }
> -
> - state->crtcs[i].commit = NULL;
>   state->crtcs[i].ptr = NULL;
>   state->crtcs[i].state = NULL;
>   }
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 4e53aae9a1fb..80c138cbde9a 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1262,12 +1262,12 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
>  void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
> struct drm_atomic_state *old_state)
>  {
> - struct drm_crtc_state *unused;
> + struct drm_crtc_state *new_crtc_state;
>   struct drm_crtc *crtc;
>   int i;
>  
> - for_each_new_crtc_in_state(old_state, crtc, unused, i) {
> - struct drm_crtc_commit *commit = old_state->crtcs[i].commit;
> + for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> + struct drm_crtc_commit *commit = new_crtc_state->commit;
>   int ret;
>  
>   if (!commit)
> @@ -1731,7 +1731,7 @@ int drm_atomic_helper_setup_commit(struct 
> drm_atomic_state *state,
>   kref_init(>ref);
>   commit->crtc = crtc;
>  
> - state->crtcs[i].commit = commit;
> + new_crtc_state->commit = commit;
>  
>   ret = stall_checks(crtc, nonblock);
>   if (ret)
> @@ -1769,22 +1769,6 @@ int drm_atomic_helper_setup_commit(struct 
> drm_atomic_state *state,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
>  
> -
> -static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
> -{
> - struct drm_crtc_commit *commit;
> - int i = 0;
> -
> - list_for_each_entry(commit, >commit_list, commit_entry) {
> - /* skip the first entry, that's the current commit */
> - if (i == 1)
> - return commit;
> - i++;
> - }
> -
> - return NULL;
> -}
> -
>  /**
>   * drm_atomic_helper_wait_for_dependencies - wait for required preceeding 
> commits
>   * @old_state: atomic state object with old state structures
> @@ -1800,17 +1784,13 @@ static struct drm_crtc_commit 
> *preceeding_commit(struct drm_crtc *crtc)
>  void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state 
> *old_state)
>  {
>   struct drm_crtc *crtc;
> - struct drm_crtc_state *new_crtc_state;
> + struct drm_crtc_state *old_crtc_state;
>   struct drm_crtc_commit *commit;
>   int i;
>   long ret;
>  
> - for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> - spin_lock(>commit_lock);
> - commit = preceeding_commit(crtc);
> - if (commit)
> - drm_crtc_commit_get(commit);
> - spin_unlock(>commit_lock);
> + for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> + commit = old_crtc_state->commit;
> 

Re: [Intel-gfx] [PATCH] drm/atomic: Move drm_crtc_commit to drm_crtc_state, v4.

[kbuild test robot]

Hi Maarten,

[auto build test ERROR on drm/drm-next]
[also build test ERROR on next-20170904]
[cannot apply to v4.13]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Maarten-Lankhorst/drm-atomic-Move-drm_crtc_commit-to-drm_crtc_state-v4/20170905-011023
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
config: x86_64-randconfig-x016-201736 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/gpu//drm/drm_atomic_helper.c: In function 
'drm_atomic_helper_commit_hw_done':
>> drivers/gpu//drm/drm_atomic_helper.c:1850:26: error: void value not ignored 
>> as it ought to be
  old_crtc_state->commit = drm_crtc_commit_get(commit);
 ^

vim +1850 drivers/gpu//drm/drm_atomic_helper.c

  1814  
  1815  /**
  1816   * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
  1817   * @old_state: atomic state object with old state structures
  1818   *
  1819   * This function is used to signal completion of the hardware commit 
step. After
  1820   * this step the driver is not allowed to read or change any permanent 
software
  1821   * or hardware modeset state. The only exception is state protected by 
other
  1822   * means than _modeset_lock locks.
  1823   *
  1824   * Drivers should try to postpone any expensive or delayed cleanup work 
after
  1825   * this function is called.
  1826   *
  1827   * This is part of the atomic helper support for nonblocking commits, 
see
  1828   * drm_atomic_helper_setup_commit() for an overview.
  1829   */
  1830  void drm_atomic_helper_commit_hw_done(struct drm_atomic_state 
*old_state)
  1831  {
  1832  struct drm_crtc *crtc;
  1833  struct drm_crtc_state *old_crtc_state, *new_crtc_state;
  1834  struct drm_crtc_commit *commit;
  1835  int i;
  1836  
  1837  for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, 
new_crtc_state, i) {
  1838  commit = new_crtc_state->commit;
  1839  if (!commit)
  1840  continue;
  1841  
  1842  /*
  1843   * copy new_crtc_state->commit to 
old_crtc_state->commit,
  1844   * it's unsafe to touch new_crtc_state after hw_done,
  1845   * but we still need to do so in cleanup_done().
  1846   */
  1847  if (old_crtc_state->commit)
  1848  drm_crtc_commit_put(old_crtc_state->commit);
  1849  
> 1850  old_crtc_state->commit = drm_crtc_commit_get(commit);
  1851  
  1852  /* backend must have consumed any event by now */
  1853  WARN_ON(new_crtc_state->event);
  1854  complete_all(>hw_done);
  1855  }
  1856  }
  1857  EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
  1858  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx