[PATCH v2 0/7] drm/atomic: Remove deprecated atomic iterator macros, v2.

[PATCH v2 0/7] drm/atomic: Remove deprecated atomic iterator macros, v2.

[Maarten Lankhorst]

It's time to kill off for_each_obj_in_state, and convert
the remainder to for_each_(old/new/both)_obj_in_state.

Some patches have been upstreamed, these are the remaining few
with compile fixes.

Maarten Lankhorst (7):
  drm/atomic: Use new iterator macros in
  drm_atomic_helper_wait_for_flip_done, again.
  drm/atomic: Clean up drm_atomic_helper_async_check
  drm/rcar-du: Use new iterator macros, v2.
  drm/omapdrm: Fix omap_atomic_wait_for_completion
  drm/msm: Convert to use new iterator macros, v2.
  drm/nouveau: Convert nouveau to use new iterator macros, v2.
  drm/atomic: Remove deprecated accessor macros

 drivers/gpu/drm/drm_atomic_helper.c     | 58 +++++++------------------
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c |  4 +-
 drivers/gpu/drm/msm/msm_atomic.c        | 18 ++++----
 drivers/gpu/drm/nouveau/nv50_display.c  | 72 ++++++++++++++++---------------
 drivers/gpu/drm/omapdrm/omap_drv.c      |  6 +--
 drivers/gpu/drm/rcar-du/rcar_du_plane.c | 57 ++++++++++++-------------
 include/drm/drm_atomic.h                | 75 ---------------------------------
 include/drm/drm_connector.h             |  3 +-
 include/drm/drm_crtc.h                  |  8 ++--
 include/drm/drm_plane.h                 |  8 ++--
 10 files changed, 104 insertions(+), 205 deletions(-)

-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/7] drm/atomic: Use new iterator macros in drm_atomic_helper_wait_for_flip_done, again.

[Maarten Lankhorst]

for_each_obj_in_state is about to be removed, so use the correct new
iterator macro.

I renamed the variable to 'unused', but forgot to convert
drm_atomic_helper_wait_for_flip_done to the new iterator macro,
so make it work this time.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: David Airlie <airlied@linux.ie>
---
 drivers/gpu/drm/drm_atomic_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 70146f809db5..a46b51291006 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1270,7 +1270,7 @@ void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	int i;
 
-	for_each_crtc_in_state(old_state, crtc, unused, i) {
+	for_each_new_crtc_in_state(old_state, crtc, unused, i) {
 		struct drm_crtc_commit *commit = old_state->crtcs[i].commit;
 		int ret;
 
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/7] drm/atomic: Use new iterator macros in drm_atomic_helper_wait_for_flip_done, again.

[Daniel Vetter]

On Wed, Jul 19, 2017 at 04:39:14PM +0200, Maarten Lankhorst wrote:
> for_each_obj_in_state is about to be removed, so use the correct new
> iterator macro.
> 
> I renamed the variable to 'unused', but forgot to convert
> drm_atomic_helper_wait_for_flip_done to the new iterator macro,
> so make it work this time.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: David Airlie <airlied@linux.ie>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 70146f809db5..a46b51291006 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1270,7 +1270,7 @@ void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
>  	struct drm_crtc *crtc;
>  	int i;
>  
> -	for_each_crtc_in_state(old_state, crtc, unused, i) {
> +	for_each_new_crtc_in_state(old_state, crtc, unused, i) {
>  		struct drm_crtc_commit *commit = old_state->crtcs[i].commit;
>  		int ret;
>  
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/7] drm/atomic: Clean up drm_atomic_helper_async_check

[Maarten Lankhorst]

Instead of assigning plane to __plane, iterate over plane
which is the same thing. Simplify the check for n_planes != 1,
at that point we know plane != NULL as well.

Rename obj_state to new_obj_state, and get rid of the bogus stuff.
crtc->state->state is NEVER non-null, if it is, there is a bug.
We should definitely try to prevent async updates if there are flips
queued, but this code will simply not be executed and needs to be
rethought.

Also remove the loop too, because we're trying to loop over the crtc until
we find plane_state->crtc == crtc, which we already know is non-zero.
It's dead code anyway. :)

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Gustavo Padovan <gustavo.padovan@collabora.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 56 ++++++++++---------------------------
 1 file changed, 15 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index a46b51291006..c538528a794a 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1372,68 +1372,42 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
 				   struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
-	struct drm_crtc_commit *commit;
-	struct drm_plane *__plane, *plane = NULL;
-	struct drm_plane_state *__plane_state, *plane_state = NULL;
+	struct drm_crtc_state *new_crtc_state;
+	struct drm_plane *plane;
+	struct drm_plane_state *new_plane_state;
 	const struct drm_plane_helper_funcs *funcs;
-	int i, j, n_planes = 0;
+	int i, n_planes = 0;
 
-	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
-		if (drm_atomic_crtc_needs_modeset(crtc_state))
+	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
+		if (drm_atomic_crtc_needs_modeset(new_crtc_state))
 			return -EINVAL;
 	}
 
-	for_each_new_plane_in_state(state, __plane, __plane_state, i) {
+	for_each_new_plane_in_state(state, plane, new_plane_state, i)
 		n_planes++;
-		plane = __plane;
-		plane_state = __plane_state;
-	}
 
 	/* FIXME: we support only single plane updates for now */
-	if (!plane || n_planes != 1)
+	if (n_planes != 1)
 		return -EINVAL;
 
-	if (!plane_state->crtc)
+	if (!new_plane_state->crtc)
 		return -EINVAL;
 
 	funcs = plane->helper_private;
 	if (!funcs->atomic_async_update)
 		return -EINVAL;
 
-	if (plane_state->fence)
+	if (new_plane_state->fence)
 		return -EINVAL;
 
 	/*
-	 * Don't do an async update if there is an outstanding commit modifying
-	 * the plane.  This prevents our async update's changes from getting
-	 * overridden by a previous synchronous update's state.
+	 * FIXME: We should prevent an async update if there is an outstanding
+	 * commit modifying the plane.  This prevents our async update's
+	 * changes from getting overwritten by a previous synchronous update's
+	 * state.
 	 */
-	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
-		if (plane->crtc != crtc)
-			continue;
-
-		spin_lock(&crtc->commit_lock);
-		commit = list_first_entry_or_null(&crtc->commit_list,
-						  struct drm_crtc_commit,
-						  commit_entry);
-		if (!commit) {
-			spin_unlock(&crtc->commit_lock);
-			continue;
-		}
-		spin_unlock(&crtc->commit_lock);
-
-		if (!crtc->state->state)
-			continue;
-
-		for_each_plane_in_state(crtc->state->state, __plane,
-					__plane_state, j) {
-			if (__plane == plane)
-				return -EINVAL;
-		}
-	}
 
-	return funcs->atomic_async_check(plane, plane_state);
+	return funcs->atomic_async_check(plane, new_plane_state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_async_check);
 
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/7] drm/atomic: Clean up drm_atomic_helper_async_check

[Daniel Vetter]

On Wed, Jul 19, 2017 at 04:39:15PM +0200, Maarten Lankhorst wrote:
> Instead of assigning plane to __plane, iterate over plane
> which is the same thing. Simplify the check for n_planes != 1,
> at that point we know plane != NULL as well.
> 
> Rename obj_state to new_obj_state, and get rid of the bogus stuff.
> crtc->state->state is NEVER non-null, if it is, there is a bug.
> We should definitely try to prevent async updates if there are flips
> queued, but this code will simply not be executed and needs to be
> rethought.
> 
> Also remove the loop too, because we're trying to loop over the crtc until
> we find plane_state->crtc == crtc, which we already know is non-zero.
> It's dead code anyway. :)
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Gustavo Padovan <gustavo.padovan@collabora.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 56 ++++++++++---------------------------
>  1 file changed, 15 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index a46b51291006..c538528a794a 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1372,68 +1372,42 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>  				   struct drm_atomic_state *state)
>  {
>  	struct drm_crtc *crtc;
> -	struct drm_crtc_state *crtc_state;
> -	struct drm_crtc_commit *commit;
> -	struct drm_plane *__plane, *plane = NULL;
> -	struct drm_plane_state *__plane_state, *plane_state = NULL;
> +	struct drm_crtc_state *new_crtc_state;
> +	struct drm_plane *plane;
> +	struct drm_plane_state *new_plane_state;
>  	const struct drm_plane_helper_funcs *funcs;
> -	int i, j, n_planes = 0;
> +	int i, n_planes = 0;
>  
> -	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> -		if (drm_atomic_crtc_needs_modeset(crtc_state))
> +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> +		if (drm_atomic_crtc_needs_modeset(new_crtc_state))
>  			return -EINVAL;
>  	}
>  
> -	for_each_new_plane_in_state(state, __plane, __plane_state, i) {
> +	for_each_new_plane_in_state(state, plane, new_plane_state, i)
>  		n_planes++;
> -		plane = __plane;
> -		plane_state = __plane_state;
> -	}
>  
>  	/* FIXME: we support only single plane updates for now */
> -	if (!plane || n_planes != 1)
> +	if (n_planes != 1)
>  		return -EINVAL;
>  
> -	if (!plane_state->crtc)
> +	if (!new_plane_state->crtc)
>  		return -EINVAL;
>  
>  	funcs = plane->helper_private;
>  	if (!funcs->atomic_async_update)
>  		return -EINVAL;
>  
> -	if (plane_state->fence)
> +	if (new_plane_state->fence)
>  		return -EINVAL;
>  
>  	/*
> -	 * Don't do an async update if there is an outstanding commit modifying
> -	 * the plane.  This prevents our async update's changes from getting
> -	 * overridden by a previous synchronous update's state.
> +	 * FIXME: We should prevent an async update if there is an outstanding
> +	 * commit modifying the plane.  This prevents our async update's
> +	 * changes from getting overwritten by a previous synchronous update's
> +	 * state.
>  	 */
> -	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> -		if (plane->crtc != crtc)
> -			continue;
> -
> -		spin_lock(&crtc->commit_lock);
> -		commit = list_first_entry_or_null(&crtc->commit_list,
> -						  struct drm_crtc_commit,
> -						  commit_entry);
> -		if (!commit) {
> -			spin_unlock(&crtc->commit_lock);
> -			continue;
> -		}
> -		spin_unlock(&crtc->commit_lock);
> -
> -		if (!crtc->state->state)
> -			continue;
> -
> -		for_each_plane_in_state(crtc->state->state, __plane,
> -					__plane_state, j) {

I'm pretty sure this oopses, because crtc->state->state is NULL after
commit. I think Gustavo needs to review this first (and write a nasty igt
testcase to catch it) before we remove this. I think the correct check is
to simply bail out if our current crtc has a pending commit (i.e.
!list_empty(&crtc->commit_list) should be all we need to check.

But I think we need a testcase for this.

Otherwise the patch looks ok I think.
-Daniel


> -			if (__plane == plane)
> -				return -EINVAL;
> -		}
> -	}
>  
> -	return funcs->atomic_async_check(plane, plane_state);
> +	return funcs->atomic_async_check(plane, new_plane_state);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_async_check);
>  
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/7] drm/atomic: Clean up drm_atomic_helper_async_check

[Maarten Lankhorst]

Op 25-07-17 om 10:23 schreef Daniel Vetter:
> On Wed, Jul 19, 2017 at 04:39:15PM +0200, Maarten Lankhorst wrote:
>> Instead of assigning plane to __plane, iterate over plane
>> which is the same thing. Simplify the check for n_planes != 1,
>> at that point we know plane != NULL as well.
>>
>> Rename obj_state to new_obj_state, and get rid of the bogus stuff.
>> crtc->state->state is NEVER non-null, if it is, there is a bug.
>> We should definitely try to prevent async updates if there are flips
>> queued, but this code will simply not be executed and needs to be
>> rethought.
>>
>> Also remove the loop too, because we're trying to loop over the crtc until
>> we find plane_state->crtc == crtc, which we already know is non-zero.
>> It's dead code anyway. :)
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Gustavo Padovan <gustavo.padovan@collabora.com>
>> ---
>>  drivers/gpu/drm/drm_atomic_helper.c | 56 ++++++++++---------------------------
>>  1 file changed, 15 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index a46b51291006..c538528a794a 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1372,68 +1372,42 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>>  				   struct drm_atomic_state *state)
>>  {
>>  	struct drm_crtc *crtc;
>> -	struct drm_crtc_state *crtc_state;
>> -	struct drm_crtc_commit *commit;
>> -	struct drm_plane *__plane, *plane = NULL;
>> -	struct drm_plane_state *__plane_state, *plane_state = NULL;
>> +	struct drm_crtc_state *new_crtc_state;
>> +	struct drm_plane *plane;
>> +	struct drm_plane_state *new_plane_state;
>>  	const struct drm_plane_helper_funcs *funcs;
>> -	int i, j, n_planes = 0;
>> +	int i, n_planes = 0;
>>  
>> -	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>> -		if (drm_atomic_crtc_needs_modeset(crtc_state))
>> +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
>> +		if (drm_atomic_crtc_needs_modeset(new_crtc_state))
>>  			return -EINVAL;
>>  	}
>>  
>> -	for_each_new_plane_in_state(state, __plane, __plane_state, i) {
>> +	for_each_new_plane_in_state(state, plane, new_plane_state, i)
>>  		n_planes++;
>> -		plane = __plane;
>> -		plane_state = __plane_state;
>> -	}
>>  
>>  	/* FIXME: we support only single plane updates for now */
>> -	if (!plane || n_planes != 1)
>> +	if (n_planes != 1)
>>  		return -EINVAL;
>>  
>> -	if (!plane_state->crtc)
>> +	if (!new_plane_state->crtc)
>>  		return -EINVAL;
>>  
>>  	funcs = plane->helper_private;
>>  	if (!funcs->atomic_async_update)
>>  		return -EINVAL;
>>  
>> -	if (plane_state->fence)
>> +	if (new_plane_state->fence)
>>  		return -EINVAL;
>>  
>>  	/*
>> -	 * Don't do an async update if there is an outstanding commit modifying
>> -	 * the plane.  This prevents our async update's changes from getting
>> -	 * overridden by a previous synchronous update's state.
>> +	 * FIXME: We should prevent an async update if there is an outstanding
>> +	 * commit modifying the plane.  This prevents our async update's
>> +	 * changes from getting overwritten by a previous synchronous update's
>> +	 * state.
>>  	 */
>> -	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>> -		if (plane->crtc != crtc)
>> -			continue;
>> -
>> -		spin_lock(&crtc->commit_lock);
>> -		commit = list_first_entry_or_null(&crtc->commit_list,
>> -						  struct drm_crtc_commit,
>> -						  commit_entry);
>> -		if (!commit) {
>> -			spin_unlock(&crtc->commit_lock);
>> -			continue;
>> -		}
>> -		spin_unlock(&crtc->commit_lock);
>> -
>> -		if (!crtc->state->state)
>> -			continue;
>> -
>> -		for_each_plane_in_state(crtc->state->state, __plane,
>> -					__plane_state, j) {
> I'm pretty sure this oopses, because crtc->state->state is NULL after
> commit. I think Gustavo needs to review this first (and write a nasty igt
> testcase to catch it) before we remove this. I think the correct check is
> to simply bail out if our current crtc has a pending commit (i.e.
> !list_empty(&crtc->commit_list) should be all we need to check.
It didn't oops. Right above it was a null check. It was never executed. :)

obj->state->state is always NULL. Excluding a brief moment during swap_state where this may oops,  this code willl never do a thing.
> But I think we need a testcase for this.
>
> Otherwise the patch looks ok I think.
> -Daniel
>
>
>> -			if (__plane == plane)
>> -				return -EINVAL;
>> -		}
>> -	}
>>  
>> -	return funcs->atomic_async_check(plane, plane_state);
>> +	return funcs->atomic_async_check(plane, new_plane_state);
>>  }
>>  EXPORT_SYMBOL(drm_atomic_helper_async_check);
>>  
>> -- 
>> 2.11.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/7] drm/atomic: Clean up drm_atomic_helper_async_check

[Daniel Vetter]

On Tue, Jul 25, 2017 at 11:11 AM, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> Op 25-07-17 om 10:23 schreef Daniel Vetter:
>> On Wed, Jul 19, 2017 at 04:39:15PM +0200, Maarten Lankhorst wrote:
>>>      /*
>>> -     * Don't do an async update if there is an outstanding commit modifying
>>> -     * the plane.  This prevents our async update's changes from getting
>>> -     * overridden by a previous synchronous update's state.
>>> +     * FIXME: We should prevent an async update if there is an outstanding
>>> +     * commit modifying the plane.  This prevents our async update's
>>> +     * changes from getting overwritten by a previous synchronous update's
>>> +     * state.
>>>       */
>>> -    for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>>> -            if (plane->crtc != crtc)
>>> -                    continue;
>>> -
>>> -            spin_lock(&crtc->commit_lock);
>>> -            commit = list_first_entry_or_null(&crtc->commit_list,
>>> -                                              struct drm_crtc_commit,
>>> -                                              commit_entry);
>>> -            if (!commit) {
>>> -                    spin_unlock(&crtc->commit_lock);
>>> -                    continue;
>>> -            }
>>> -            spin_unlock(&crtc->commit_lock);
>>> -
>>> -            if (!crtc->state->state)
>>> -                    continue;
>>> -
>>> -            for_each_plane_in_state(crtc->state->state, __plane,
>>> -                                    __plane_state, j) {
>> I'm pretty sure this oopses, because crtc->state->state is NULL after
>> commit. I think Gustavo needs to review this first (and write a nasty igt
>> testcase to catch it) before we remove this. I think the correct check is
>> to simply bail out if our current crtc has a pending commit (i.e.
>> !list_empty(&crtc->commit_list) should be all we need to check.
> It didn't oops. Right above it was a null check. It was never executed. :)
>
> obj->state->state is always NULL. Excluding a brief moment during swap_state where this may oops,  this code willl never do a thing.

Oh right. It's still completely buggy, and I'd like to fix that first,
testcase included. Can you pls poke Gustavo a bit (or maybe he's on
vacation)?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/7] drm/atomic: Clean up drm_atomic_helper_async_check

[Maarten Lankhorst]

Op 25-07-17 om 11:27 schreef Daniel Vetter:
> On Tue, Jul 25, 2017 at 11:11 AM, Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
>> Op 25-07-17 om 10:23 schreef Daniel Vetter:
>>> On Wed, Jul 19, 2017 at 04:39:15PM +0200, Maarten Lankhorst wrote:
>>>>      /*
>>>> -     * Don't do an async update if there is an outstanding commit modifying
>>>> -     * the plane.  This prevents our async update's changes from getting
>>>> -     * overridden by a previous synchronous update's state.
>>>> +     * FIXME: We should prevent an async update if there is an outstanding
>>>> +     * commit modifying the plane.  This prevents our async update's
>>>> +     * changes from getting overwritten by a previous synchronous update's
>>>> +     * state.
>>>>       */
>>>> -    for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>>>> -            if (plane->crtc != crtc)
>>>> -                    continue;
>>>> -
>>>> -            spin_lock(&crtc->commit_lock);
>>>> -            commit = list_first_entry_or_null(&crtc->commit_list,
>>>> -                                              struct drm_crtc_commit,
>>>> -                                              commit_entry);
>>>> -            if (!commit) {
>>>> -                    spin_unlock(&crtc->commit_lock);
>>>> -                    continue;
>>>> -            }
>>>> -            spin_unlock(&crtc->commit_lock);
>>>> -
>>>> -            if (!crtc->state->state)
>>>> -                    continue;
>>>> -
>>>> -            for_each_plane_in_state(crtc->state->state, __plane,
>>>> -                                    __plane_state, j) {
>>> I'm pretty sure this oopses, because crtc->state->state is NULL after
>>> commit. I think Gustavo needs to review this first (and write a nasty igt
>>> testcase to catch it) before we remove this. I think the correct check is
>>> to simply bail out if our current crtc has a pending commit (i.e.
>>> !list_empty(&crtc->commit_list) should be all we need to check.
>> It didn't oops. Right above it was a null check. It was never executed. :)
>>
>> obj->state->state is always NULL. Excluding a brief moment during swap_state where this may oops,  this code willl never do a thing.
> Oh right. It's still completely buggy, and I'd like to fix that first,
> testcase included. Can you pls poke Gustavo a bit (or maybe he's on
> vacation)?
> -Daniel

The only thing we have atm excercising it is kms_cursor_legacy, but that doesn't check if flips can be overwritten.
Perhaps we should make IGT tests a requirement for features in the future?

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/7] drm/rcar-du: Use new iterator macros, v2.

[Maarten Lankhorst]

for_each_obj_in_state is about to be removed, so use the correct new
iterator macros.

Also look at new_plane_state instead of plane->state when looking up
the hw planes in use. They should be the same except when reallocating,
(in which case this code is skipped) and we should really stop looking
at obj->state whenever possible.

Changes since v1:
- Actually compile correctly.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-renesas-soc@vger.kernel.org
---
 drivers/gpu/drm/rcar-du/rcar_du_plane.c | 57 ++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
index dcde6288da6c..50fd793c38d1 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
@@ -51,12 +51,9 @@
  */
 
 static bool rcar_du_plane_needs_realloc(struct rcar_du_plane *plane,
+					const struct rcar_du_plane_state *cur_state,
 					struct rcar_du_plane_state *new_state)
 {
-	struct rcar_du_plane_state *cur_state;
-
-	cur_state = to_rcar_plane_state(plane->plane.state);
-
 	/* Lowering the number of planes doesn't strictly require reallocation
 	 * as the extra hardware plane will be freed when committing, but doing
 	 * so could lead to more fragmentation.
@@ -141,16 +138,17 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
 	unsigned int groups = 0;
 	unsigned int i;
 	struct drm_plane *drm_plane;
-	struct drm_plane_state *drm_plane_state;
+	struct drm_plane_state *old_drm_plane_state, *new_drm_plane_state;
 
 	/* Check if hardware planes need to be reallocated. */
-	for_each_plane_in_state(state, drm_plane, drm_plane_state, i) {
-		struct rcar_du_plane_state *plane_state;
+	for_each_oldnew_plane_in_state(state, drm_plane, old_drm_plane_state, new_drm_plane_state, i) {
+		struct rcar_du_plane_state *old_plane_state, *new_plane_state;
 		struct rcar_du_plane *plane;
 		unsigned int index;
 
 		plane = to_rcar_plane(drm_plane);
-		plane_state = to_rcar_plane_state(drm_plane_state);
+		old_plane_state = to_rcar_plane_state(old_drm_plane_state);
+		new_plane_state = to_rcar_plane_state(new_drm_plane_state);
 
 		dev_dbg(rcdu->dev, "%s: checking plane (%u,%tu)\n", __func__,
 			plane->group->index, plane - plane->group->planes);
@@ -159,19 +157,19 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
 		 * the full reallocation procedure. Just mark the hardware
 		 * plane(s) as freed.
 		 */
-		if (!plane_state->format) {
+		if (!new_plane_state->format) {
 			dev_dbg(rcdu->dev, "%s: plane is being disabled\n",
 				__func__);
 			index = plane - plane->group->planes;
 			group_freed_planes[plane->group->index] |= 1 << index;
-			plane_state->hwindex = -1;
+			new_plane_state->hwindex = -1;
 			continue;
 		}
 
 		/* If the plane needs to be reallocated mark it as such, and
 		 * mark the hardware plane(s) as free.
 		 */
-		if (rcar_du_plane_needs_realloc(plane, plane_state)) {
+		if (rcar_du_plane_needs_realloc(plane, old_plane_state, new_plane_state)) {
 			dev_dbg(rcdu->dev, "%s: plane needs reallocation\n",
 				__func__);
 			groups |= 1 << plane->group->index;
@@ -179,7 +177,7 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
 
 			index = plane - plane->group->planes;
 			group_freed_planes[plane->group->index] |= 1 << index;
-			plane_state->hwindex = -1;
+			new_plane_state->hwindex = -1;
 		}
 	}
 
@@ -204,7 +202,7 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
 
 		for (i = 0; i < group->num_planes; ++i) {
 			struct rcar_du_plane *plane = &group->planes[i];
-			struct rcar_du_plane_state *plane_state;
+			struct rcar_du_plane_state *new_plane_state;
 			struct drm_plane_state *s;
 
 			s = drm_atomic_get_plane_state(state, &plane->plane);
@@ -226,16 +224,16 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
 				continue;
 			}
 
-			plane_state = to_rcar_plane_state(plane->plane.state);
-			used_planes |= rcar_du_plane_hwmask(plane_state);
+			new_plane_state = to_rcar_plane_state(s);
+			used_planes |= rcar_du_plane_hwmask(new_plane_state);
 
 			dev_dbg(rcdu->dev,
 				"%s: plane (%u,%tu) uses %u hwplanes (index %d)\n",
 				__func__, plane->group->index,
 				plane - plane->group->planes,
-				plane_state->format ?
-				plane_state->format->planes : 0,
-				plane_state->hwindex);
+				new_plane_state->format ?
+				new_plane_state->format->planes : 0,
+				new_plane_state->hwindex);
 		}
 
 		group_free_planes[index] = 0xff & ~used_planes;
@@ -246,15 +244,16 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
 	}
 
 	/* Reallocate hardware planes for each plane that needs it. */
-	for_each_plane_in_state(state, drm_plane, drm_plane_state, i) {
-		struct rcar_du_plane_state *plane_state;
+	for_each_oldnew_plane_in_state(state, drm_plane, old_drm_plane_state, new_drm_plane_state, i) {
+		struct rcar_du_plane_state *old_plane_state, *new_plane_state;
 		struct rcar_du_plane *plane;
 		unsigned int crtc_planes;
 		unsigned int free;
 		int idx;
 
 		plane = to_rcar_plane(drm_plane);
-		plane_state = to_rcar_plane_state(drm_plane_state);
+		old_plane_state = to_rcar_plane_state(old_drm_plane_state);
+		new_plane_state = to_rcar_plane_state(new_drm_plane_state);
 
 		dev_dbg(rcdu->dev, "%s: allocating plane (%u,%tu)\n", __func__,
 			plane->group->index, plane - plane->group->planes);
@@ -262,8 +261,8 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
 		/* Skip planes that are being disabled or don't need to be
 		 * reallocated.
 		 */
-		if (!plane_state->format ||
-		    !rcar_du_plane_needs_realloc(plane, plane_state))
+		if (!new_plane_state->format ||
+		    !rcar_du_plane_needs_realloc(plane, old_plane_state, new_plane_state))
 			continue;
 
 		/* Try to allocate the plane from the free planes currently
@@ -271,15 +270,15 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
 		 * group and thus minimize flicker. If it fails fall back to
 		 * allocating from all free planes.
 		 */
-		crtc_planes = to_rcar_crtc(plane_state->state.crtc)->index % 2
+		crtc_planes = to_rcar_crtc(new_plane_state->state.crtc)->index % 2
 			    ? plane->group->dptsr_planes
 			    : ~plane->group->dptsr_planes;
 		free = group_free_planes[plane->group->index];
 
-		idx = rcar_du_plane_hwalloc(plane, plane_state,
+		idx = rcar_du_plane_hwalloc(plane, new_plane_state,
 					    free & crtc_planes);
 		if (idx < 0)
-			idx = rcar_du_plane_hwalloc(plane, plane_state,
+			idx = rcar_du_plane_hwalloc(plane, new_plane_state,
 						    free);
 		if (idx < 0) {
 			dev_dbg(rcdu->dev, "%s: no available hardware plane\n",
@@ -288,12 +287,12 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
 		}
 
 		dev_dbg(rcdu->dev, "%s: allocated %u hwplanes (index %u)\n",
-			__func__, plane_state->format->planes, idx);
+			__func__, new_plane_state->format->planes, idx);
 
-		plane_state->hwindex = idx;
+		new_plane_state->hwindex = idx;
 
 		group_free_planes[plane->group->index] &=
-			~rcar_du_plane_hwmask(plane_state);
+			~rcar_du_plane_hwmask(new_plane_state);
 
 		dev_dbg(rcdu->dev, "%s: group %u free planes mask 0x%02x\n",
 			__func__, plane->group->index,
-- 
2.11.0

Re: [PATCH v2 3/7] drm/rcar-du: Use new iterator macros, v2.

[Daniel Vetter]

On Wed, Jul 19, 2017 at 04:39:16PM +0200, Maarten Lankhorst wrote:
> for_each_obj_in_state is about to be removed, so use the correct new
> iterator macros.
> 
> Also look at new_plane_state instead of plane->state when looking up
> the hw planes in use. They should be the same except when reallocating,
> (in which case this code is skipped) and we should really stop looking
> at obj->state whenever possible.
> 
> Changes since v1:
> - Actually compile correctly.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: linux-renesas-soc@vger.kernel.org

Didn't compile-test, but looks correct now.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_plane.c | 57 ++++++++++++++++-----------------
>  1 file changed, 28 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> index dcde6288da6c..50fd793c38d1 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> @@ -51,12 +51,9 @@
>   */
>  
>  static bool rcar_du_plane_needs_realloc(struct rcar_du_plane *plane,
> +					const struct rcar_du_plane_state *cur_state,
>  					struct rcar_du_plane_state *new_state)
>  {
> -	struct rcar_du_plane_state *cur_state;
> -
> -	cur_state = to_rcar_plane_state(plane->plane.state);
> -
>  	/* Lowering the number of planes doesn't strictly require reallocation
>  	 * as the extra hardware plane will be freed when committing, but doing
>  	 * so could lead to more fragmentation.
> @@ -141,16 +138,17 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
>  	unsigned int groups = 0;
>  	unsigned int i;
>  	struct drm_plane *drm_plane;
> -	struct drm_plane_state *drm_plane_state;
> +	struct drm_plane_state *old_drm_plane_state, *new_drm_plane_state;
>  
>  	/* Check if hardware planes need to be reallocated. */
> -	for_each_plane_in_state(state, drm_plane, drm_plane_state, i) {
> -		struct rcar_du_plane_state *plane_state;
> +	for_each_oldnew_plane_in_state(state, drm_plane, old_drm_plane_state, new_drm_plane_state, i) {
> +		struct rcar_du_plane_state *old_plane_state, *new_plane_state;
>  		struct rcar_du_plane *plane;
>  		unsigned int index;
>  
>  		plane = to_rcar_plane(drm_plane);
> -		plane_state = to_rcar_plane_state(drm_plane_state);
> +		old_plane_state = to_rcar_plane_state(old_drm_plane_state);
> +		new_plane_state = to_rcar_plane_state(new_drm_plane_state);
>  
>  		dev_dbg(rcdu->dev, "%s: checking plane (%u,%tu)\n", __func__,
>  			plane->group->index, plane - plane->group->planes);
> @@ -159,19 +157,19 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
>  		 * the full reallocation procedure. Just mark the hardware
>  		 * plane(s) as freed.
>  		 */
> -		if (!plane_state->format) {
> +		if (!new_plane_state->format) {
>  			dev_dbg(rcdu->dev, "%s: plane is being disabled\n",
>  				__func__);
>  			index = plane - plane->group->planes;
>  			group_freed_planes[plane->group->index] |= 1 << index;
> -			plane_state->hwindex = -1;
> +			new_plane_state->hwindex = -1;
>  			continue;
>  		}
>  
>  		/* If the plane needs to be reallocated mark it as such, and
>  		 * mark the hardware plane(s) as free.
>  		 */
> -		if (rcar_du_plane_needs_realloc(plane, plane_state)) {
> +		if (rcar_du_plane_needs_realloc(plane, old_plane_state, new_plane_state)) {
>  			dev_dbg(rcdu->dev, "%s: plane needs reallocation\n",
>  				__func__);
>  			groups |= 1 << plane->group->index;
> @@ -179,7 +177,7 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
>  
>  			index = plane - plane->group->planes;
>  			group_freed_planes[plane->group->index] |= 1 << index;
> -			plane_state->hwindex = -1;
> +			new_plane_state->hwindex = -1;
>  		}
>  	}
>  
> @@ -204,7 +202,7 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
>  
>  		for (i = 0; i < group->num_planes; ++i) {
>  			struct rcar_du_plane *plane = &group->planes[i];
> -			struct rcar_du_plane_state *plane_state;
> +			struct rcar_du_plane_state *new_plane_state;
>  			struct drm_plane_state *s;
>  
>  			s = drm_atomic_get_plane_state(state, &plane->plane);
> @@ -226,16 +224,16 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
>  				continue;
>  			}
>  
> -			plane_state = to_rcar_plane_state(plane->plane.state);
> -			used_planes |= rcar_du_plane_hwmask(plane_state);
> +			new_plane_state = to_rcar_plane_state(s);
> +			used_planes |= rcar_du_plane_hwmask(new_plane_state);
>  
>  			dev_dbg(rcdu->dev,
>  				"%s: plane (%u,%tu) uses %u hwplanes (index %d)\n",
>  				__func__, plane->group->index,
>  				plane - plane->group->planes,
> -				plane_state->format ?
> -				plane_state->format->planes : 0,
> -				plane_state->hwindex);
> +				new_plane_state->format ?
> +				new_plane_state->format->planes : 0,
> +				new_plane_state->hwindex);
>  		}
>  
>  		group_free_planes[index] = 0xff & ~used_planes;
> @@ -246,15 +244,16 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
>  	}
>  
>  	/* Reallocate hardware planes for each plane that needs it. */
> -	for_each_plane_in_state(state, drm_plane, drm_plane_state, i) {
> -		struct rcar_du_plane_state *plane_state;
> +	for_each_oldnew_plane_in_state(state, drm_plane, old_drm_plane_state, new_drm_plane_state, i) {
> +		struct rcar_du_plane_state *old_plane_state, *new_plane_state;
>  		struct rcar_du_plane *plane;
>  		unsigned int crtc_planes;
>  		unsigned int free;
>  		int idx;
>  
>  		plane = to_rcar_plane(drm_plane);
> -		plane_state = to_rcar_plane_state(drm_plane_state);
> +		old_plane_state = to_rcar_plane_state(old_drm_plane_state);
> +		new_plane_state = to_rcar_plane_state(new_drm_plane_state);
>  
>  		dev_dbg(rcdu->dev, "%s: allocating plane (%u,%tu)\n", __func__,
>  			plane->group->index, plane - plane->group->planes);
> @@ -262,8 +261,8 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
>  		/* Skip planes that are being disabled or don't need to be
>  		 * reallocated.
>  		 */
> -		if (!plane_state->format ||
> -		    !rcar_du_plane_needs_realloc(plane, plane_state))
> +		if (!new_plane_state->format ||
> +		    !rcar_du_plane_needs_realloc(plane, old_plane_state, new_plane_state))
>  			continue;
>  
>  		/* Try to allocate the plane from the free planes currently
> @@ -271,15 +270,15 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
>  		 * group and thus minimize flicker. If it fails fall back to
>  		 * allocating from all free planes.
>  		 */
> -		crtc_planes = to_rcar_crtc(plane_state->state.crtc)->index % 2
> +		crtc_planes = to_rcar_crtc(new_plane_state->state.crtc)->index % 2
>  			    ? plane->group->dptsr_planes
>  			    : ~plane->group->dptsr_planes;
>  		free = group_free_planes[plane->group->index];
>  
> -		idx = rcar_du_plane_hwalloc(plane, plane_state,
> +		idx = rcar_du_plane_hwalloc(plane, new_plane_state,
>  					    free & crtc_planes);
>  		if (idx < 0)
> -			idx = rcar_du_plane_hwalloc(plane, plane_state,
> +			idx = rcar_du_plane_hwalloc(plane, new_plane_state,
>  						    free);
>  		if (idx < 0) {
>  			dev_dbg(rcdu->dev, "%s: no available hardware plane\n",
> @@ -288,12 +287,12 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
>  		}
>  
>  		dev_dbg(rcdu->dev, "%s: allocated %u hwplanes (index %u)\n",
> -			__func__, plane_state->format->planes, idx);
> +			__func__, new_plane_state->format->planes, idx);
>  
> -		plane_state->hwindex = idx;
> +		new_plane_state->hwindex = idx;
>  
>  		group_free_planes[plane->group->index] &=
> -			~rcar_du_plane_hwmask(plane_state);
> +			~rcar_du_plane_hwmask(new_plane_state);
>  
>  		dev_dbg(rcdu->dev, "%s: group %u free planes mask 0x%02x\n",
>  			__func__, plane->group->index,
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/7] drm/rcar-du: Use new iterator macros, v2.

[Laurent Pinchart]

Hi Maarten,

Thank you for the patch.

On Wednesday 19 Jul 2017 16:39:16 Maarten Lankhorst wrote:
> for_each_obj_in_state is about to be removed, so use the correct new
> iterator macros.
> 
> Also look at new_plane_state instead of plane->state when looking up
> the hw planes in use. They should be the same except when reallocating,
> (in which case this code is skipped) and we should really stop looking
> at obj->state whenever possible.
> 
> Changes since v1:
> - Actually compile correctly.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: linux-renesas-soc@vger.kernel.org
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_plane.c | 57 ++++++++++++++---------------
>  1 file changed, 28 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> b/drivers/gpu/drm/rcar-du/rcar_du_plane.c index dcde6288da6c..50fd793c38d1
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> @@ -51,12 +51,9 @@
>   */
> 
>  static bool rcar_du_plane_needs_realloc(struct rcar_du_plane *plane,
> +					const struct rcar_du_plane_state *cur_state,

I would call this old_state to be consistent with the naming scheme used in
the subsystem.

>  					struct rcar_du_plane_state *new_state)

The function doesn't need the plane argument anymore, it can be removed.

>  {
> -	struct rcar_du_plane_state *cur_state;
> -
> -	cur_state = to_rcar_plane_state(plane->plane.state);
> -
>  	/* Lowering the number of planes doesn't strictly require reallocation
>  	 * as the extra hardware plane will be freed when committing, but doing
>  	 * so could lead to more fragmentation.
> @@ -141,16 +138,17 @@ int rcar_du_atomic_check_planes(struct drm_device
> *dev, unsigned int groups = 0;
>  	unsigned int i;
>  	struct drm_plane *drm_plane;
> -	struct drm_plane_state *drm_plane_state;
> +	struct drm_plane_state *old_drm_plane_state, *new_drm_plane_state;

One variable declaration per line please (here and below).

>  	/* Check if hardware planes need to be reallocated. */
> -	for_each_plane_in_state(state, drm_plane, drm_plane_state, i) {
> -		struct rcar_du_plane_state *plane_state;
> +	for_each_oldnew_plane_in_state(state, drm_plane, old_drm_plane_state,
> new_drm_plane_state, i) {

Could you please break lines to keep them below the 80 columns limit (here and
below) ?

> +		struct rcar_du_plane_state *old_plane_state, *new_plane_state;
>  		struct rcar_du_plane *plane;
>  		unsigned int index;
> 
>  		plane = to_rcar_plane(drm_plane);
> -		plane_state = to_rcar_plane_state(drm_plane_state);
> +		old_plane_state = to_rcar_plane_state(old_drm_plane_state);
> +		new_plane_state = to_rcar_plane_state(new_drm_plane_state);
> 
>  		dev_dbg(rcdu->dev, "%s: checking plane (%u,%tu)\n", __func__,
>  			plane->group->index, plane - plane->group->planes);

[snip]

Feel free to use this version of the patch.

>From 21ea2b1e4dcdfb1fd0ff44776434219793e0ef75 Mon Sep 17 00:00:00 2001
From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date: Wed, 12 Jul 2017 12:43:40 +0200
Subject: [PATCH v3] drm: rcar-du: Use new iterator macros

for_each_obj_in_state is about to be removed, so use the correct new
iterator macros.

Also look at new_plane_state instead of plane->state when looking up
the hw planes in use. They should be the same except when reallocating,
(in which case this code is skipped) and we should really stop looking
at obj->state whenever possible.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
Changes since v2:

- Removed plane argument to rcar_du_plane_needs_realloc()
- Avoid lines longer than 80 columns when possible
- Declare one variable per line

Changes since v1:

- Actually compile correctly.
---
 drivers/gpu/drm/rcar-du/rcar_du_plane.c | 72 +++++++++++++++++----------------
 1 file changed, 38 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
index dcde6288da6c..4bad0b79d3f2 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
@@ -50,23 +50,20 @@
  * automatically when the core swaps the old and new states.
  */
 
-static bool rcar_du_plane_needs_realloc(struct rcar_du_plane *plane,
-					struct rcar_du_plane_state *new_state)
+static bool rcar_du_plane_needs_realloc(
+				const struct rcar_du_plane_state *old_state,
+				const struct rcar_du_plane_state *new_state)
 {
-	struct rcar_du_plane_state *cur_state;
-
-	cur_state = to_rcar_plane_state(plane->plane.state);
-
 	/* Lowering the number of planes doesn't strictly require reallocation
 	 * as the extra hardware plane will be freed when committing, but doing
 	 * so could lead to more fragmentation.
 	 */
-	if (!cur_state->format ||
-	    cur_state->format->planes != new_state->format->planes)
+	if (!old_state->format ||
+	    old_state->format->planes != new_state->format->planes)
 		return true;
 
 	/* Reallocate hardware planes if the source has changed. */
-	if (cur_state->source != new_state->source)
+	if (old_state->source != new_state->source)
 		return true;
 
 	return false;
@@ -141,16 +138,20 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
 	unsigned int groups = 0;
 	unsigned int i;
 	struct drm_plane *drm_plane;
-	struct drm_plane_state *drm_plane_state;
+	struct drm_plane_state *old_drm_plane_state;
+	struct drm_plane_state *new_drm_plane_state;
 
 	/* Check if hardware planes need to be reallocated. */
-	for_each_plane_in_state(state, drm_plane, drm_plane_state, i) {
-		struct rcar_du_plane_state *plane_state;
+	for_each_oldnew_plane_in_state(state, drm_plane, old_drm_plane_state,
+				       new_drm_plane_state, i) {
+		struct rcar_du_plane_state *old_plane_state;
+		struct rcar_du_plane_state *new_plane_state;
 		struct rcar_du_plane *plane;
 		unsigned int index;
 
 		plane = to_rcar_plane(drm_plane);
-		plane_state = to_rcar_plane_state(drm_plane_state);
+		old_plane_state = to_rcar_plane_state(old_drm_plane_state);
+		new_plane_state = to_rcar_plane_state(new_drm_plane_state);
 
 		dev_dbg(rcdu->dev, "%s: checking plane (%u,%tu)\n", __func__,
 			plane->group->index, plane - plane->group->planes);
@@ -159,19 +160,19 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
 		 * the full reallocation procedure. Just mark the hardware
 		 * plane(s) as freed.
 		 */
-		if (!plane_state->format) {
+		if (!new_plane_state->format) {
 			dev_dbg(rcdu->dev, "%s: plane is being disabled\n",
 				__func__);
 			index = plane - plane->group->planes;
 			group_freed_planes[plane->group->index] |= 1 << index;
-			plane_state->hwindex = -1;
+			new_plane_state->hwindex = -1;
 			continue;
 		}
 
 		/* If the plane needs to be reallocated mark it as such, and
 		 * mark the hardware plane(s) as free.
 		 */
-		if (rcar_du_plane_needs_realloc(plane, plane_state)) {
+		if (rcar_du_plane_needs_realloc(old_plane_state, new_plane_state)) {
 			dev_dbg(rcdu->dev, "%s: plane needs reallocation\n",
 				__func__);
 			groups |= 1 << plane->group->index;
@@ -179,7 +180,7 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
 
 			index = plane - plane->group->planes;
 			group_freed_planes[plane->group->index] |= 1 << index;
-			plane_state->hwindex = -1;
+			new_plane_state->hwindex = -1;
 		}
 	}
 
@@ -204,7 +205,7 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
 
 		for (i = 0; i < group->num_planes; ++i) {
 			struct rcar_du_plane *plane = &group->planes[i];
-			struct rcar_du_plane_state *plane_state;
+			struct rcar_du_plane_state *new_plane_state;
 			struct drm_plane_state *s;
 
 			s = drm_atomic_get_plane_state(state, &plane->plane);
@@ -226,16 +227,16 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
 				continue;
 			}
 
-			plane_state = to_rcar_plane_state(plane->plane.state);
-			used_planes |= rcar_du_plane_hwmask(plane_state);
+			new_plane_state = to_rcar_plane_state(s);
+			used_planes |= rcar_du_plane_hwmask(new_plane_state);
 
 			dev_dbg(rcdu->dev,
 				"%s: plane (%u,%tu) uses %u hwplanes (index %d)\n",
 				__func__, plane->group->index,
 				plane - plane->group->planes,
-				plane_state->format ?
-				plane_state->format->planes : 0,
-				plane_state->hwindex);
+				new_plane_state->format ?
+				new_plane_state->format->planes : 0,
+				new_plane_state->hwindex);
 		}
 
 		group_free_planes[index] = 0xff & ~used_planes;
@@ -246,15 +247,18 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
 	}
 
 	/* Reallocate hardware planes for each plane that needs it. */
-	for_each_plane_in_state(state, drm_plane, drm_plane_state, i) {
-		struct rcar_du_plane_state *plane_state;
+	for_each_oldnew_plane_in_state(state, drm_plane, old_drm_plane_state,
+				       new_drm_plane_state, i) {
+		struct rcar_du_plane_state *old_plane_state;
+		struct rcar_du_plane_state *new_plane_state;
 		struct rcar_du_plane *plane;
 		unsigned int crtc_planes;
 		unsigned int free;
 		int idx;
 
 		plane = to_rcar_plane(drm_plane);
-		plane_state = to_rcar_plane_state(drm_plane_state);
+		old_plane_state = to_rcar_plane_state(old_drm_plane_state);
+		new_plane_state = to_rcar_plane_state(new_drm_plane_state);
 
 		dev_dbg(rcdu->dev, "%s: allocating plane (%u,%tu)\n", __func__,
 			plane->group->index, plane - plane->group->planes);
@@ -262,8 +266,8 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
 		/* Skip planes that are being disabled or don't need to be
 		 * reallocated.
 		 */
-		if (!plane_state->format ||
-		    !rcar_du_plane_needs_realloc(plane, plane_state))
+		if (!new_plane_state->format ||
+		    !rcar_du_plane_needs_realloc(old_plane_state, new_plane_state))
 			continue;
 
 		/* Try to allocate the plane from the free planes currently
@@ -271,15 +275,15 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
 		 * group and thus minimize flicker. If it fails fall back to
 		 * allocating from all free planes.
 		 */
-		crtc_planes = to_rcar_crtc(plane_state->state.crtc)->index % 2
+		crtc_planes = to_rcar_crtc(new_plane_state->state.crtc)->index % 2
 			    ? plane->group->dptsr_planes
 			    : ~plane->group->dptsr_planes;
 		free = group_free_planes[plane->group->index];
 
-		idx = rcar_du_plane_hwalloc(plane, plane_state,
+		idx = rcar_du_plane_hwalloc(plane, new_plane_state,
 					    free & crtc_planes);
 		if (idx < 0)
-			idx = rcar_du_plane_hwalloc(plane, plane_state,
+			idx = rcar_du_plane_hwalloc(plane, new_plane_state,
 						    free);
 		if (idx < 0) {
 			dev_dbg(rcdu->dev, "%s: no available hardware plane\n",
@@ -288,12 +292,12 @@ int rcar_du_atomic_check_planes(struct drm_device *dev,
 		}
 
 		dev_dbg(rcdu->dev, "%s: allocated %u hwplanes (index %u)\n",
-			__func__, plane_state->format->planes, idx);
+			__func__, new_plane_state->format->planes, idx);
 
-		plane_state->hwindex = idx;
+		new_plane_state->hwindex = idx;
 
 		group_free_planes[plane->group->index] &=
-			~rcar_du_plane_hwmask(plane_state);
+			~rcar_du_plane_hwmask(new_plane_state);
 
 		dev_dbg(rcdu->dev, "%s: group %u free planes mask 0x%02x\n",
 			__func__, plane->group->index,
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/7] drm/rcar-du: Use new iterator macros, v2.

[Laurent Pinchart]

Hi Maarten,

On Wednesday 26 Jul 2017 14:53:33 Laurent Pinchart wrote:
> On Wednesday 19 Jul 2017 16:39:16 Maarten Lankhorst wrote:
> > for_each_obj_in_state is about to be removed, so use the correct new
> > iterator macros.
> > 
> > Also look at new_plane_state instead of plane->state when looking up
> > the hw planes in use. They should be the same except when reallocating,
> > (in which case this code is skipped) and we should really stop looking
> > at obj->state whenever possible.
> > 
> > Changes since v1:
> > - Actually compile correctly.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: linux-renesas-soc@vger.kernel.org

I plan to send a pull request to Dave in the middle of next week with a bunch 
of rcar-du-drm patches that will generate a few conflicts with this one. 
They're all simple to solve as they're located in comments, but that will be 
annoying nonetheless. I can include a rebased version of this patch in my pull 
request if it can help, depending on when you plan to get this series merged 
in drm-misc-next.

-- 
Regards,

Laurent Pinchart

[PATCH v2 4/7] drm/omapdrm: Fix omap_atomic_wait_for_completion

[Maarten Lankhorst]

Use the new iterator macro and look for crtc_state->active instead of
enable, only crtc_state->enable implies that vblanks will happen.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 022029ea6972..66d3c6bfd6a8 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -57,13 +57,13 @@ static void omap_fb_output_poll_changed(struct drm_device *dev)
 static void omap_atomic_wait_for_completion(struct drm_device *dev,
 					    struct drm_atomic_state *old_state)
 {
-	struct drm_crtc_state *old_crtc_state;
+	struct drm_crtc_state *new_crtc_state;
 	struct drm_crtc *crtc;
 	unsigned int i;
 	int ret;
 
-	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
-		if (!crtc->state->enable)
+	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
+		if (!new_crtc_state->active)
 			continue;
 
 		ret = omap_crtc_wait_pending(crtc);
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/7] drm/omapdrm: Fix omap_atomic_wait_for_completion

[Daniel Vetter]

On Wed, Jul 19, 2017 at 04:39:17PM +0200, Maarten Lankhorst wrote:
> Use the new iterator macro and look for crtc_state->active instead of
> enable, only crtc_state->enable implies that vblanks will happen.

s/enable/active/, since enable only means logically enabled (aka resources
reserved). With that my r-b holds.
-Daniel

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 022029ea6972..66d3c6bfd6a8 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -57,13 +57,13 @@ static void omap_fb_output_poll_changed(struct drm_device *dev)
>  static void omap_atomic_wait_for_completion(struct drm_device *dev,
>  					    struct drm_atomic_state *old_state)
>  {
> -	struct drm_crtc_state *old_crtc_state;
> +	struct drm_crtc_state *new_crtc_state;
>  	struct drm_crtc *crtc;
>  	unsigned int i;
>  	int ret;
>  
> -	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> -		if (!crtc->state->enable)
> +	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> +		if (!new_crtc_state->active)
>  			continue;
>  
>  		ret = omap_crtc_wait_pending(crtc);
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/7] drm/omapdrm: Fix omap_atomic_wait_for_completion

[Maarten Lankhorst]

Op 25-07-17 om 10:27 schreef Daniel Vetter:
> On Wed, Jul 19, 2017 at 04:39:17PM +0200, Maarten Lankhorst wrote:
>> Use the new iterator macro and look for crtc_state->active instead of
>> enable, only crtc_state->enable implies that vblanks will happen.
> s/enable/active/, since enable only means logically enabled (aka resources
> reserved). With that my r-b holds.
> -Daniel
Ok thanks. I've pushed patch 1, 4, 5 and 6.

I'll wait a bit longer for the conflict in patch 3, and for a better solution on async in patch 2.

It seems the nouveau patches caused a bit of a conflict in nv50_disp_atomic_commit_tail
because of better event handling compared to drm-misc, I've fixed up drm-tip.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 5/7] drm/msm: Convert to use new iterator macros, v2.

[Maarten Lankhorst]

for_each_obj_in_state is about to be removed, so convert
to the new iterator macros.

Just like in omap, use crtc_state->active instead of
crtc_state->enable when waiting for completion.

Changes since v1:
- Fix compilation.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Vincent Abriou <vincent.abriou@st.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Russell King <rmk+kernel@armlinux.org.uk>
Cc: Rob Herring <robh@kernel.org>
Cc: Markus Elfring <elfring@users.sourceforge.net>
Cc: Sushmita Susheelendra <ssusheel@codeaurora.org>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Tested-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c |  4 ++--
 drivers/gpu/drm/msm/msm_atomic.c        | 18 +++++++++---------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
index bcd1f5cac72c..f7f087419ed8 100644
--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
@@ -114,7 +114,7 @@ static void mdp4_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *st
 	mdp4_enable(mdp4_kms);
 
 	/* see 119ecb7fd */
-	for_each_crtc_in_state(state, crtc, crtc_state, i)
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i)
 		drm_crtc_vblank_get(crtc);
 }
 
@@ -126,7 +126,7 @@ static void mdp4_complete_commit(struct msm_kms *kms, struct drm_atomic_state *s
 	struct drm_crtc_state *crtc_state;
 
 	/* see 119ecb7fd */
-	for_each_crtc_in_state(state, crtc, crtc_state, i)
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i)
 		drm_crtc_vblank_put(crtc);
 
 	mdp4_disable(mdp4_kms);
diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index badfa8717317..025d454163b0 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -84,13 +84,13 @@ static void msm_atomic_wait_for_commit_done(struct drm_device *dev,
 		struct drm_atomic_state *old_state)
 {
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *new_crtc_state;
 	struct msm_drm_private *priv = old_state->dev->dev_private;
 	struct msm_kms *kms = priv->kms;
 	int i;
 
-	for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
-		if (!crtc->state->enable)
+	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
+		if (!new_crtc_state->active)
 			continue;
 
 		kms->funcs->wait_for_crtc_commit_done(kms, crtc);
@@ -195,7 +195,7 @@ int msm_atomic_commit(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	struct drm_plane *plane;
-	struct drm_plane_state *plane_state;
+	struct drm_plane_state *old_plane_state, *new_plane_state;
 	int i, ret;
 
 	ret = drm_atomic_helper_prepare_planes(dev, state);
@@ -211,19 +211,19 @@ int msm_atomic_commit(struct drm_device *dev,
 	/*
 	 * Figure out what crtcs we have:
 	 */
-	for_each_crtc_in_state(state, crtc, crtc_state, i)
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i)
 		c->crtc_mask |= drm_crtc_mask(crtc);
 
 	/*
 	 * Figure out what fence to wait for:
 	 */
-	for_each_plane_in_state(state, plane, plane_state, i) {
-		if ((plane->state->fb != plane_state->fb) && plane_state->fb) {
-			struct drm_gem_object *obj = msm_framebuffer_bo(plane_state->fb, 0);
+	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
+		if ((new_plane_state->fb != old_plane_state->fb) && new_plane_state->fb) {
+			struct drm_gem_object *obj = msm_framebuffer_bo(new_plane_state->fb, 0);
 			struct msm_gem_object *msm_obj = to_msm_bo(obj);
 			struct dma_fence *fence = reservation_object_get_excl_rcu(msm_obj->resv);
 
-			drm_atomic_set_fence_for_plane(plane_state, fence);
+			drm_atomic_set_fence_for_plane(new_plane_state, fence);
 		}
 	}
 
-- 
2.11.0

[PATCH v2 6/7] drm/nouveau: Convert nouveau to use new iterator macros, v2.

[Maarten Lankhorst]

Use the new atomic iterator macros, the old ones are about to be
removed. With the new macros, it's more easy to get old and new state so
get them from the macros instead of from obj->state.

Changes since v1:
- Don't mix up old and new state. (danvet)
- Rebase on top of interruptible swap_state changes.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: nouveau@lists.freedesktop.org
---
 drivers/gpu/drm/nouveau/nv50_display.c | 72 +++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c
index 747c99c1e474..7abfb561b00c 100644
--- a/drivers/gpu/drm/nouveau/nv50_display.c
+++ b/drivers/gpu/drm/nouveau/nv50_display.c
@@ -2103,7 +2103,7 @@ nv50_head_atomic_check(struct drm_crtc *crtc, struct drm_crtc_state *state)
 
 	NV_ATOMIC(drm, "%s atomic_check %d\n", crtc->name, asyh->state.active);
 	if (asyh->state.active) {
-		for_each_connector_in_state(asyh->state.state, conn, conns, i) {
+		for_each_new_connector_in_state(asyh->state.state, conn, conns, i) {
 			if (conns->crtc == crtc) {
 				asyc = nouveau_conn_atom(conns);
 				break;
@@ -3905,9 +3905,9 @@ static void
 nv50_disp_atomic_commit_tail(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *new_crtc_state;
 	struct drm_crtc *crtc;
-	struct drm_plane_state *plane_state;
+	struct drm_plane_state *new_plane_state;
 	struct drm_plane *plane;
 	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nv50_disp *disp = nv50_disp(dev);
@@ -3926,8 +3926,8 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state)
 		mutex_lock(&disp->mutex);
 
 	/* Disable head(s). */
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		struct nv50_head_atom *asyh = nv50_head_atom(crtc->state);
+	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
+		struct nv50_head_atom *asyh = nv50_head_atom(new_crtc_state);
 		struct nv50_head *head = nv50_head(crtc);
 
 		NV_ATOMIC(drm, "%s: clr %04x (set %04x)\n", crtc->name,
@@ -3940,8 +3940,8 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state)
 	}
 
 	/* Disable plane(s). */
-	for_each_plane_in_state(state, plane, plane_state, i) {
-		struct nv50_wndw_atom *asyw = nv50_wndw_atom(plane->state);
+	for_each_new_plane_in_state(state, plane, new_plane_state, i) {
+		struct nv50_wndw_atom *asyw = nv50_wndw_atom(new_plane_state);
 		struct nv50_wndw *wndw = nv50_wndw(plane);
 
 		NV_ATOMIC(drm, "%s: clr %02x (set %02x)\n", plane->name,
@@ -4006,8 +4006,8 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state)
 	}
 
 	/* Update head(s). */
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		struct nv50_head_atom *asyh = nv50_head_atom(crtc->state);
+	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
+		struct nv50_head_atom *asyh = nv50_head_atom(new_crtc_state);
 		struct nv50_head *head = nv50_head(crtc);
 
 		NV_ATOMIC(drm, "%s: set %04x (clr %04x)\n", crtc->name,
@@ -4019,14 +4019,14 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state)
 		}
 	}
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (crtc->state->event)
+	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
+		if (new_crtc_state->event)
 			drm_crtc_vblank_get(crtc);
 	}
 
 	/* Update plane(s). */
-	for_each_plane_in_state(state, plane, plane_state, i) {
-		struct nv50_wndw_atom *asyw = nv50_wndw_atom(plane->state);
+	for_each_new_plane_in_state(state, plane, new_plane_state, i) {
+		struct nv50_wndw_atom *asyw = nv50_wndw_atom(new_plane_state);
 		struct nv50_wndw *wndw = nv50_wndw(plane);
 
 		NV_ATOMIC(drm, "%s: set %02x (clr %02x)\n", plane->name,
@@ -4056,23 +4056,23 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state)
 		mutex_unlock(&disp->mutex);
 
 	/* Wait for HW to signal completion. */
-	for_each_plane_in_state(state, plane, plane_state, i) {
-		struct nv50_wndw_atom *asyw = nv50_wndw_atom(plane->state);
+	for_each_new_plane_in_state(state, plane, new_plane_state, i) {
+		struct nv50_wndw_atom *asyw = nv50_wndw_atom(new_plane_state);
 		struct nv50_wndw *wndw = nv50_wndw(plane);
 		int ret = nv50_wndw_wait_armed(wndw, asyw);
 		if (ret)
 			NV_ERROR(drm, "%s: timeout\n", plane->name);
 	}
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (crtc->state->event) {
+	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
+		if (new_crtc_state->event) {
 			unsigned long flags;
 			/* Get correct count/ts if racing with vblank irq */
 			drm_crtc_accurate_vblank_count(crtc);
 			spin_lock_irqsave(&crtc->dev->event_lock, flags);
-			drm_crtc_send_vblank_event(crtc, crtc->state->event);
+			drm_crtc_send_vblank_event(crtc, new_crtc_state->event);
 			spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
-			crtc->state->event = NULL;
+			new_crtc_state->event = NULL;
 			drm_crtc_vblank_put(crtc);
 		}
 	}
@@ -4097,7 +4097,7 @@ nv50_disp_atomic_commit(struct drm_device *dev,
 {
 	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nv50_disp *disp = nv50_disp(dev);
-	struct drm_plane_state *plane_state;
+	struct drm_plane_state *old_plane_state;
 	struct drm_plane *plane;
 	struct drm_crtc *crtc;
 	bool active = false;
@@ -4127,9 +4127,10 @@ nv50_disp_atomic_commit(struct drm_device *dev,
 	if (ret)
 		goto err_cleanup;
 
-	for_each_plane_in_state(state, plane, plane_state, i) {
-		struct nv50_wndw_atom *asyw = nv50_wndw_atom(plane_state);
+	for_each_old_plane_in_state(state, plane, old_plane_state, i) {
+		struct nv50_wndw_atom *asyw = nv50_wndw_atom(old_plane_state);
 		struct nv50_wndw *wndw = nv50_wndw(plane);
+
 		if (asyw->set.image) {
 			asyw->ntfy.handle = wndw->dmac->sync.handle;
 			asyw->ntfy.offset = wndw->ntfy;
@@ -4192,18 +4193,19 @@ nv50_disp_outp_atomic_add(struct nv50_atom *atom, struct drm_encoder *encoder)
 
 static int
 nv50_disp_outp_atomic_check_clr(struct nv50_atom *atom,
-				struct drm_connector *connector)
+				struct drm_connector_state *old_connector_state)
 {
-	struct drm_encoder *encoder = connector->state->best_encoder;
-	struct drm_crtc_state *crtc_state;
+	struct drm_encoder *encoder = old_connector_state->best_encoder;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct drm_crtc *crtc;
 	struct nv50_outp_atom *outp;
 
-	if (!(crtc = connector->state->crtc))
+	if (!(crtc = old_connector_state->crtc))
 		return 0;
 
-	crtc_state = drm_atomic_get_existing_crtc_state(&atom->state, crtc);
-	if (crtc->state->active && drm_atomic_crtc_needs_modeset(crtc_state)) {
+	old_crtc_state = drm_atomic_get_old_crtc_state(&atom->state, crtc);
+	new_crtc_state = drm_atomic_get_new_crtc_state(&atom->state, crtc);
+	if (old_crtc_state->active && drm_atomic_crtc_needs_modeset(new_crtc_state)) {
 		outp = nv50_disp_outp_atomic_add(atom, encoder);
 		if (IS_ERR(outp))
 			return PTR_ERR(outp);
@@ -4224,15 +4226,15 @@ nv50_disp_outp_atomic_check_set(struct nv50_atom *atom,
 				struct drm_connector_state *connector_state)
 {
 	struct drm_encoder *encoder = connector_state->best_encoder;
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *new_crtc_state;
 	struct drm_crtc *crtc;
 	struct nv50_outp_atom *outp;
 
 	if (!(crtc = connector_state->crtc))
 		return 0;
 
-	crtc_state = drm_atomic_get_existing_crtc_state(&atom->state, crtc);
-	if (crtc_state->active && drm_atomic_crtc_needs_modeset(crtc_state)) {
+	new_crtc_state = drm_atomic_get_new_crtc_state(&atom->state, crtc);
+	if (new_crtc_state->active && drm_atomic_crtc_needs_modeset(new_crtc_state)) {
 		outp = nv50_disp_outp_atomic_add(atom, encoder);
 		if (IS_ERR(outp))
 			return PTR_ERR(outp);
@@ -4248,7 +4250,7 @@ static int
 nv50_disp_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 {
 	struct nv50_atom *atom = nv50_atom(state);
-	struct drm_connector_state *connector_state;
+	struct drm_connector_state *old_connector_state, *new_connector_state;
 	struct drm_connector *connector;
 	int ret, i;
 
@@ -4256,12 +4258,12 @@ nv50_disp_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 	if (ret)
 		return ret;
 
-	for_each_connector_in_state(state, connector, connector_state, i) {
-		ret = nv50_disp_outp_atomic_check_clr(atom, connector);
+	for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) {
+		ret = nv50_disp_outp_atomic_check_clr(atom, old_connector_state);
 		if (ret)
 			return ret;
 
-		ret = nv50_disp_outp_atomic_check_set(atom, connector_state);
+		ret = nv50_disp_outp_atomic_check_set(atom, new_connector_state);
 		if (ret)
 			return ret;
 	}
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 6/7] drm/nouveau: Convert nouveau to use new iterator macros, v2.

[Daniel Vetter]

On Wed, Jul 19, 2017 at 04:39:19PM +0200, Maarten Lankhorst wrote:
> Use the new atomic iterator macros, the old ones are about to be
> removed. With the new macros, it's more easy to get old and new state so
> get them from the macros instead of from obj->state.
> 
> Changes since v1:
> - Don't mix up old and new state. (danvet)
> - Rebase on top of interruptible swap_state changes.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Cc: nouveau@lists.freedesktop.org

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/nouveau/nv50_display.c | 72 +++++++++++++++++-----------------
>  1 file changed, 37 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c
> index 747c99c1e474..7abfb561b00c 100644
> --- a/drivers/gpu/drm/nouveau/nv50_display.c
> +++ b/drivers/gpu/drm/nouveau/nv50_display.c
> @@ -2103,7 +2103,7 @@ nv50_head_atomic_check(struct drm_crtc *crtc, struct drm_crtc_state *state)
>  
>  	NV_ATOMIC(drm, "%s atomic_check %d\n", crtc->name, asyh->state.active);
>  	if (asyh->state.active) {
> -		for_each_connector_in_state(asyh->state.state, conn, conns, i) {
> +		for_each_new_connector_in_state(asyh->state.state, conn, conns, i) {
>  			if (conns->crtc == crtc) {
>  				asyc = nouveau_conn_atom(conns);
>  				break;
> @@ -3905,9 +3905,9 @@ static void
>  nv50_disp_atomic_commit_tail(struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = state->dev;
> -	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc_state *new_crtc_state;
>  	struct drm_crtc *crtc;
> -	struct drm_plane_state *plane_state;
> +	struct drm_plane_state *new_plane_state;
>  	struct drm_plane *plane;
>  	struct nouveau_drm *drm = nouveau_drm(dev);
>  	struct nv50_disp *disp = nv50_disp(dev);
> @@ -3926,8 +3926,8 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state)
>  		mutex_lock(&disp->mutex);
>  
>  	/* Disable head(s). */
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		struct nv50_head_atom *asyh = nv50_head_atom(crtc->state);
> +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> +		struct nv50_head_atom *asyh = nv50_head_atom(new_crtc_state);
>  		struct nv50_head *head = nv50_head(crtc);
>  
>  		NV_ATOMIC(drm, "%s: clr %04x (set %04x)\n", crtc->name,
> @@ -3940,8 +3940,8 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state)
>  	}
>  
>  	/* Disable plane(s). */
> -	for_each_plane_in_state(state, plane, plane_state, i) {
> -		struct nv50_wndw_atom *asyw = nv50_wndw_atom(plane->state);
> +	for_each_new_plane_in_state(state, plane, new_plane_state, i) {
> +		struct nv50_wndw_atom *asyw = nv50_wndw_atom(new_plane_state);
>  		struct nv50_wndw *wndw = nv50_wndw(plane);
>  
>  		NV_ATOMIC(drm, "%s: clr %02x (set %02x)\n", plane->name,
> @@ -4006,8 +4006,8 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state)
>  	}
>  
>  	/* Update head(s). */
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		struct nv50_head_atom *asyh = nv50_head_atom(crtc->state);
> +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> +		struct nv50_head_atom *asyh = nv50_head_atom(new_crtc_state);
>  		struct nv50_head *head = nv50_head(crtc);
>  
>  		NV_ATOMIC(drm, "%s: set %04x (clr %04x)\n", crtc->name,
> @@ -4019,14 +4019,14 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state)
>  		}
>  	}
>  
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		if (crtc->state->event)
> +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> +		if (new_crtc_state->event)
>  			drm_crtc_vblank_get(crtc);
>  	}
>  
>  	/* Update plane(s). */
> -	for_each_plane_in_state(state, plane, plane_state, i) {
> -		struct nv50_wndw_atom *asyw = nv50_wndw_atom(plane->state);
> +	for_each_new_plane_in_state(state, plane, new_plane_state, i) {
> +		struct nv50_wndw_atom *asyw = nv50_wndw_atom(new_plane_state);
>  		struct nv50_wndw *wndw = nv50_wndw(plane);
>  
>  		NV_ATOMIC(drm, "%s: set %02x (clr %02x)\n", plane->name,
> @@ -4056,23 +4056,23 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state)
>  		mutex_unlock(&disp->mutex);
>  
>  	/* Wait for HW to signal completion. */
> -	for_each_plane_in_state(state, plane, plane_state, i) {
> -		struct nv50_wndw_atom *asyw = nv50_wndw_atom(plane->state);
> +	for_each_new_plane_in_state(state, plane, new_plane_state, i) {
> +		struct nv50_wndw_atom *asyw = nv50_wndw_atom(new_plane_state);
>  		struct nv50_wndw *wndw = nv50_wndw(plane);
>  		int ret = nv50_wndw_wait_armed(wndw, asyw);
>  		if (ret)
>  			NV_ERROR(drm, "%s: timeout\n", plane->name);
>  	}
>  
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		if (crtc->state->event) {
> +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> +		if (new_crtc_state->event) {
>  			unsigned long flags;
>  			/* Get correct count/ts if racing with vblank irq */
>  			drm_crtc_accurate_vblank_count(crtc);
>  			spin_lock_irqsave(&crtc->dev->event_lock, flags);
> -			drm_crtc_send_vblank_event(crtc, crtc->state->event);
> +			drm_crtc_send_vblank_event(crtc, new_crtc_state->event);
>  			spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> -			crtc->state->event = NULL;
> +			new_crtc_state->event = NULL;
>  			drm_crtc_vblank_put(crtc);
>  		}
>  	}
> @@ -4097,7 +4097,7 @@ nv50_disp_atomic_commit(struct drm_device *dev,
>  {
>  	struct nouveau_drm *drm = nouveau_drm(dev);
>  	struct nv50_disp *disp = nv50_disp(dev);
> -	struct drm_plane_state *plane_state;
> +	struct drm_plane_state *old_plane_state;
>  	struct drm_plane *plane;
>  	struct drm_crtc *crtc;
>  	bool active = false;
> @@ -4127,9 +4127,10 @@ nv50_disp_atomic_commit(struct drm_device *dev,
>  	if (ret)
>  		goto err_cleanup;
>  
> -	for_each_plane_in_state(state, plane, plane_state, i) {
> -		struct nv50_wndw_atom *asyw = nv50_wndw_atom(plane_state);
> +	for_each_old_plane_in_state(state, plane, old_plane_state, i) {
> +		struct nv50_wndw_atom *asyw = nv50_wndw_atom(old_plane_state);
>  		struct nv50_wndw *wndw = nv50_wndw(plane);
> +
>  		if (asyw->set.image) {
>  			asyw->ntfy.handle = wndw->dmac->sync.handle;
>  			asyw->ntfy.offset = wndw->ntfy;
> @@ -4192,18 +4193,19 @@ nv50_disp_outp_atomic_add(struct nv50_atom *atom, struct drm_encoder *encoder)
>  
>  static int
>  nv50_disp_outp_atomic_check_clr(struct nv50_atom *atom,
> -				struct drm_connector *connector)
> +				struct drm_connector_state *old_connector_state)
>  {
> -	struct drm_encoder *encoder = connector->state->best_encoder;
> -	struct drm_crtc_state *crtc_state;
> +	struct drm_encoder *encoder = old_connector_state->best_encoder;
> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>  	struct drm_crtc *crtc;
>  	struct nv50_outp_atom *outp;
>  
> -	if (!(crtc = connector->state->crtc))
> +	if (!(crtc = old_connector_state->crtc))
>  		return 0;
>  
> -	crtc_state = drm_atomic_get_existing_crtc_state(&atom->state, crtc);
> -	if (crtc->state->active && drm_atomic_crtc_needs_modeset(crtc_state)) {
> +	old_crtc_state = drm_atomic_get_old_crtc_state(&atom->state, crtc);
> +	new_crtc_state = drm_atomic_get_new_crtc_state(&atom->state, crtc);
> +	if (old_crtc_state->active && drm_atomic_crtc_needs_modeset(new_crtc_state)) {
>  		outp = nv50_disp_outp_atomic_add(atom, encoder);
>  		if (IS_ERR(outp))
>  			return PTR_ERR(outp);
> @@ -4224,15 +4226,15 @@ nv50_disp_outp_atomic_check_set(struct nv50_atom *atom,
>  				struct drm_connector_state *connector_state)
>  {
>  	struct drm_encoder *encoder = connector_state->best_encoder;
> -	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc_state *new_crtc_state;
>  	struct drm_crtc *crtc;
>  	struct nv50_outp_atom *outp;
>  
>  	if (!(crtc = connector_state->crtc))
>  		return 0;
>  
> -	crtc_state = drm_atomic_get_existing_crtc_state(&atom->state, crtc);
> -	if (crtc_state->active && drm_atomic_crtc_needs_modeset(crtc_state)) {
> +	new_crtc_state = drm_atomic_get_new_crtc_state(&atom->state, crtc);
> +	if (new_crtc_state->active && drm_atomic_crtc_needs_modeset(new_crtc_state)) {
>  		outp = nv50_disp_outp_atomic_add(atom, encoder);
>  		if (IS_ERR(outp))
>  			return PTR_ERR(outp);
> @@ -4248,7 +4250,7 @@ static int
>  nv50_disp_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>  {
>  	struct nv50_atom *atom = nv50_atom(state);
> -	struct drm_connector_state *connector_state;
> +	struct drm_connector_state *old_connector_state, *new_connector_state;
>  	struct drm_connector *connector;
>  	int ret, i;
>  
> @@ -4256,12 +4258,12 @@ nv50_disp_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>  	if (ret)
>  		return ret;
>  
> -	for_each_connector_in_state(state, connector, connector_state, i) {
> -		ret = nv50_disp_outp_atomic_check_clr(atom, connector);
> +	for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) {
> +		ret = nv50_disp_outp_atomic_check_clr(atom, old_connector_state);
>  		if (ret)
>  			return ret;
>  
> -		ret = nv50_disp_outp_atomic_check_set(atom, connector_state);
> +		ret = nv50_disp_outp_atomic_check_set(atom, new_connector_state);
>  		if (ret)
>  			return ret;
>  	}
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 7/7] drm/atomic: Remove deprecated accessor macros

[Maarten Lankhorst]

Now that the last users have been converted, we can finally get rid of
for_each_obj_in_state, we have better macros to replace them with.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: David Airlie <airlied@linux.ie>
---
 include/drm/drm_atomic.h    | 75 ---------------------------------------------
 include/drm/drm_connector.h |  3 +-
 include/drm/drm_crtc.h      |  8 ++---
 include/drm/drm_plane.h     |  8 ++---
 4 files changed, 9 insertions(+), 85 deletions(-)

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 7cd0f303f5a3..679e746f0459 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -563,31 +563,6 @@ int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state);
 void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
 
 /**
- * for_each_connector_in_state - iterate over all connectors in an atomic update
- * @__state: &struct drm_atomic_state pointer
- * @connector: &struct drm_connector iteration cursor
- * @connector_state: &struct drm_connector_state iteration cursor
- * @__i: int iteration cursor, for macro-internal use
- *
- * This iterates over all connectors in an atomic update. Note that before the
- * software state is committed (by calling drm_atomic_helper_swap_state(), this
- * points to the new state, while afterwards it points to the old state. Due to
- * this tricky confusion this macro is deprecated.
- *
- * FIXME:
- *
- * Replace all usage of this with one of the explicit iterators below and then
- * remove this macro.
- */
-#define for_each_connector_in_state(__state, connector, connector_state, __i) \
-	for ((__i) = 0;							\
-	     (__i) < (__state)->num_connector &&				\
-	     ((connector) = (__state)->connectors[__i].ptr,			\
-	     (connector_state) = (__state)->connectors[__i].state, 1); 	\
-	     (__i)++)							\
-		for_each_if (connector)
-
-/**
  * for_each_oldnew_connector_in_state - iterate over all connectors in an atomic update
  * @__state: &struct drm_atomic_state pointer
  * @connector: &struct drm_connector iteration cursor
@@ -651,31 +626,6 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
 		for_each_if (connector)
 
 /**
- * for_each_crtc_in_state - iterate over all connectors in an atomic update
- * @__state: &struct drm_atomic_state pointer
- * @crtc: &struct drm_crtc iteration cursor
- * @crtc_state: &struct drm_crtc_state iteration cursor
- * @__i: int iteration cursor, for macro-internal use
- *
- * This iterates over all CRTCs in an atomic update. Note that before the
- * software state is committed (by calling drm_atomic_helper_swap_state(), this
- * points to the new state, while afterwards it points to the old state. Due to
- * this tricky confusion this macro is deprecated.
- *
- * FIXME:
- *
- * Replace all usage of this with one of the explicit iterators below and then
- * remove this macro.
- */
-#define for_each_crtc_in_state(__state, crtc, crtc_state, __i)	\
-	for ((__i) = 0;						\
-	     (__i) < (__state)->dev->mode_config.num_crtc &&	\
-	     ((crtc) = (__state)->crtcs[__i].ptr,			\
-	     (crtc_state) = (__state)->crtcs[__i].state, 1);	\
-	     (__i)++)						\
-		for_each_if (crtc_state)
-
-/**
  * for_each_oldnew_crtc_in_state - iterate over all CRTCs in an atomic update
  * @__state: &struct drm_atomic_state pointer
  * @crtc: &struct drm_crtc iteration cursor
@@ -735,31 +685,6 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
 		for_each_if (crtc)
 
 /**
- * for_each_plane_in_state - iterate over all planes in an atomic update
- * @__state: &struct drm_atomic_state pointer
- * @plane: &struct drm_plane iteration cursor
- * @plane_state: &struct drm_plane_state iteration cursor
- * @__i: int iteration cursor, for macro-internal use
- *
- * This iterates over all planes in an atomic update. Note that before the
- * software state is committed (by calling drm_atomic_helper_swap_state(), this
- * points to the new state, while afterwards it points to the old state. Due to
- * this tricky confusion this macro is deprecated.
- *
- * FIXME:
- *
- * Replace all usage of this with one of the explicit iterators below and then
- * remove this macro.
- */
-#define for_each_plane_in_state(__state, plane, plane_state, __i)		\
-	for ((__i) = 0;							\
-	     (__i) < (__state)->dev->mode_config.num_total_plane &&	\
-	     ((plane) = (__state)->planes[__i].ptr,				\
-	     (plane_state) = (__state)->planes[__i].state, 1);		\
-	     (__i)++)							\
-		for_each_if (plane_state)
-
-/**
  * for_each_oldnew_plane_in_state - iterate over all planes in an atomic update
  * @__state: &struct drm_atomic_state pointer
  * @plane: &struct drm_plane iteration cursor
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 4bc088269d05..e3d89feb1def 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -890,8 +890,7 @@ struct drm_connector {
 	 * This is protected by @drm_mode_config.connection_mutex. Note that
 	 * nonblocking atomic commits access the current connector state without
 	 * taking locks. Either by going through the &struct drm_atomic_state
-	 * pointers, see for_each_connector_in_state(),
-	 * for_each_oldnew_connector_in_state(),
+	 * pointers, see for_each_oldnew_connector_in_state(),
 	 * for_each_old_connector_in_state() and
 	 * for_each_new_connector_in_state(). Or through careful ordering of
 	 * atomic commit operations as implemented in the atomic helpers, see
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 3a911a64c257..c4c949ea20da 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -807,10 +807,10 @@ struct drm_crtc {
 	 * This is protected by @mutex. Note that nonblocking atomic commits
 	 * access the current CRTC state without taking locks. Either by going
 	 * through the &struct drm_atomic_state pointers, see
-	 * for_each_crtc_in_state(), for_each_oldnew_crtc_in_state(),
-	 * for_each_old_crtc_in_state() and for_each_new_crtc_in_state(). Or
-	 * through careful ordering of atomic commit operations as implemented
-	 * in the atomic helpers, see &struct drm_crtc_commit.
+	 * for_each_oldnew_crtc_in_state(), for_each_old_crtc_in_state() and
+	 * for_each_new_crtc_in_state(). Or through careful ordering of atomic
+	 * commit operations as implemented in the atomic helpers, see
+	 * &struct drm_crtc_commit.
 	 */
 	struct drm_crtc_state *state;
 
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 9ab3e7044812..a1b3aa5d1223 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -514,10 +514,10 @@ struct drm_plane {
 	 * This is protected by @mutex. Note that nonblocking atomic commits
 	 * access the current plane state without taking locks. Either by going
 	 * through the &struct drm_atomic_state pointers, see
-	 * for_each_plane_in_state(), for_each_oldnew_plane_in_state(),
-	 * for_each_old_plane_in_state() and for_each_new_plane_in_state(). Or
-	 * through careful ordering of atomic commit operations as implemented
-	 * in the atomic helpers, see &struct drm_crtc_commit.
+	 * for_each_oldnew_plane_in_state(), for_each_old_plane_in_state() and
+	 * for_each_new_plane_in_state(). Or through careful ordering of atomic
+	 * commit operations as implemented in the atomic helpers, see
+	 * &struct drm_crtc_commit.
 	 */
 	struct drm_plane_state *state;
 
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 7/7] drm/atomic: Remove deprecated accessor macros

[Daniel Vetter]

On Wed, Jul 19, 2017 at 04:39:20PM +0200, Maarten Lankhorst wrote:
> Now that the last users have been converted, we can finally get rid of
> for_each_obj_in_state, we have better macros to replace them with.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: David Airlie <airlied@linux.ie>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  include/drm/drm_atomic.h    | 75 ---------------------------------------------
>  include/drm/drm_connector.h |  3 +-
>  include/drm/drm_crtc.h      |  8 ++---
>  include/drm/drm_plane.h     |  8 ++---
>  4 files changed, 9 insertions(+), 85 deletions(-)
> 
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 7cd0f303f5a3..679e746f0459 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -563,31 +563,6 @@ int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state);
>  void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
>  
>  /**
> - * for_each_connector_in_state - iterate over all connectors in an atomic update
> - * @__state: &struct drm_atomic_state pointer
> - * @connector: &struct drm_connector iteration cursor
> - * @connector_state: &struct drm_connector_state iteration cursor
> - * @__i: int iteration cursor, for macro-internal use
> - *
> - * This iterates over all connectors in an atomic update. Note that before the
> - * software state is committed (by calling drm_atomic_helper_swap_state(), this
> - * points to the new state, while afterwards it points to the old state. Due to
> - * this tricky confusion this macro is deprecated.
> - *
> - * FIXME:
> - *
> - * Replace all usage of this with one of the explicit iterators below and then
> - * remove this macro.
> - */
> -#define for_each_connector_in_state(__state, connector, connector_state, __i) \
> -	for ((__i) = 0;							\
> -	     (__i) < (__state)->num_connector &&				\
> -	     ((connector) = (__state)->connectors[__i].ptr,			\
> -	     (connector_state) = (__state)->connectors[__i].state, 1); 	\
> -	     (__i)++)							\
> -		for_each_if (connector)
> -
> -/**
>   * for_each_oldnew_connector_in_state - iterate over all connectors in an atomic update
>   * @__state: &struct drm_atomic_state pointer
>   * @connector: &struct drm_connector iteration cursor
> @@ -651,31 +626,6 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
>  		for_each_if (connector)
>  
>  /**
> - * for_each_crtc_in_state - iterate over all connectors in an atomic update
> - * @__state: &struct drm_atomic_state pointer
> - * @crtc: &struct drm_crtc iteration cursor
> - * @crtc_state: &struct drm_crtc_state iteration cursor
> - * @__i: int iteration cursor, for macro-internal use
> - *
> - * This iterates over all CRTCs in an atomic update. Note that before the
> - * software state is committed (by calling drm_atomic_helper_swap_state(), this
> - * points to the new state, while afterwards it points to the old state. Due to
> - * this tricky confusion this macro is deprecated.
> - *
> - * FIXME:
> - *
> - * Replace all usage of this with one of the explicit iterators below and then
> - * remove this macro.
> - */
> -#define for_each_crtc_in_state(__state, crtc, crtc_state, __i)	\
> -	for ((__i) = 0;						\
> -	     (__i) < (__state)->dev->mode_config.num_crtc &&	\
> -	     ((crtc) = (__state)->crtcs[__i].ptr,			\
> -	     (crtc_state) = (__state)->crtcs[__i].state, 1);	\
> -	     (__i)++)						\
> -		for_each_if (crtc_state)
> -
> -/**
>   * for_each_oldnew_crtc_in_state - iterate over all CRTCs in an atomic update
>   * @__state: &struct drm_atomic_state pointer
>   * @crtc: &struct drm_crtc iteration cursor
> @@ -735,31 +685,6 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
>  		for_each_if (crtc)
>  
>  /**
> - * for_each_plane_in_state - iterate over all planes in an atomic update
> - * @__state: &struct drm_atomic_state pointer
> - * @plane: &struct drm_plane iteration cursor
> - * @plane_state: &struct drm_plane_state iteration cursor
> - * @__i: int iteration cursor, for macro-internal use
> - *
> - * This iterates over all planes in an atomic update. Note that before the
> - * software state is committed (by calling drm_atomic_helper_swap_state(), this
> - * points to the new state, while afterwards it points to the old state. Due to
> - * this tricky confusion this macro is deprecated.
> - *
> - * FIXME:
> - *
> - * Replace all usage of this with one of the explicit iterators below and then
> - * remove this macro.
> - */
> -#define for_each_plane_in_state(__state, plane, plane_state, __i)		\
> -	for ((__i) = 0;							\
> -	     (__i) < (__state)->dev->mode_config.num_total_plane &&	\
> -	     ((plane) = (__state)->planes[__i].ptr,				\
> -	     (plane_state) = (__state)->planes[__i].state, 1);		\
> -	     (__i)++)							\
> -		for_each_if (plane_state)
> -
> -/**
>   * for_each_oldnew_plane_in_state - iterate over all planes in an atomic update
>   * @__state: &struct drm_atomic_state pointer
>   * @plane: &struct drm_plane iteration cursor
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 4bc088269d05..e3d89feb1def 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -890,8 +890,7 @@ struct drm_connector {
>  	 * This is protected by @drm_mode_config.connection_mutex. Note that
>  	 * nonblocking atomic commits access the current connector state without
>  	 * taking locks. Either by going through the &struct drm_atomic_state
> -	 * pointers, see for_each_connector_in_state(),
> -	 * for_each_oldnew_connector_in_state(),
> +	 * pointers, see for_each_oldnew_connector_in_state(),
>  	 * for_each_old_connector_in_state() and
>  	 * for_each_new_connector_in_state(). Or through careful ordering of
>  	 * atomic commit operations as implemented in the atomic helpers, see
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 3a911a64c257..c4c949ea20da 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -807,10 +807,10 @@ struct drm_crtc {
>  	 * This is protected by @mutex. Note that nonblocking atomic commits
>  	 * access the current CRTC state without taking locks. Either by going
>  	 * through the &struct drm_atomic_state pointers, see
> -	 * for_each_crtc_in_state(), for_each_oldnew_crtc_in_state(),
> -	 * for_each_old_crtc_in_state() and for_each_new_crtc_in_state(). Or
> -	 * through careful ordering of atomic commit operations as implemented
> -	 * in the atomic helpers, see &struct drm_crtc_commit.
> +	 * for_each_oldnew_crtc_in_state(), for_each_old_crtc_in_state() and
> +	 * for_each_new_crtc_in_state(). Or through careful ordering of atomic
> +	 * commit operations as implemented in the atomic helpers, see
> +	 * &struct drm_crtc_commit.
>  	 */
>  	struct drm_crtc_state *state;
>  
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 9ab3e7044812..a1b3aa5d1223 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -514,10 +514,10 @@ struct drm_plane {
>  	 * This is protected by @mutex. Note that nonblocking atomic commits
>  	 * access the current plane state without taking locks. Either by going
>  	 * through the &struct drm_atomic_state pointers, see
> -	 * for_each_plane_in_state(), for_each_oldnew_plane_in_state(),
> -	 * for_each_old_plane_in_state() and for_each_new_plane_in_state(). Or
> -	 * through careful ordering of atomic commit operations as implemented
> -	 * in the atomic helpers, see &struct drm_crtc_commit.
> +	 * for_each_oldnew_plane_in_state(), for_each_old_plane_in_state() and
> +	 * for_each_new_plane_in_state(). Or through careful ordering of atomic
> +	 * commit operations as implemented in the atomic helpers, see
> +	 * &struct drm_crtc_commit.
>  	 */
>  	struct drm_plane_state *state;
>  
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/atomic: Remove deprecated atomic iterator macros, v2.

[Patchwork]

== Series Details ==

Series: drm/atomic: Remove deprecated atomic iterator macros, v2.
URL   : https://patchwork.freedesktop.org/series/27582/
State : success

== Summary ==

Series 27582v1 drm/atomic: Remove deprecated atomic iterator macros, v2.
https://patchwork.freedesktop.org/api/1.0/series/27582/revisions/1/mbox/

Test gem_sync:
        Subgroup basic-store-all:
                fail       -> PASS       (fi-ivb-3520m) fdo#100007
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                skip       -> PASS       (fi-skl-x1585l) fdo#101781

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#101781 https://bugs.freedesktop.org/show_bug.cgi?id=101781

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:442s
fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:431s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:352s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:540s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:517s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:487s
fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:491s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:600s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:423s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:417s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:420s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:503s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:493s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:466s
fi-kbl-7560u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:574s
fi-kbl-r         total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:587s
fi-pnv-d510      total:279  pass:222  dwarn:2   dfail:0   fail:0   skip:55  time:567s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:459s
fi-skl-6700hq    total:279  pass:262  dwarn:0   dfail:0   fail:0   skip:17  time:594s
fi-skl-6700k     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:474s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:476s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:435s
fi-skl-x1585l    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:494s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:535s
fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:405s

f1c32d67a1e143cba1b0ad042448f39d75d9ccaa drm-tip: 2017y-07m-19d-13h-38m-55s UTC integration manifest
db13c8a drm/atomic: Remove deprecated accessor macros
ffff026 drm/nouveau: Convert nouveau to use new iterator macros, v2.
82bf6c3 drm/msm: Convert to use new iterator macros, v2.
d377fe4 drm/omapdrm: Fix omap_atomic_wait_for_completion
2ddcf93 drm/rcar-du: Use new iterator macros, v2.
f83fb92 drm/atomic: Clean up drm_atomic_helper_async_check
2c07ba5 drm/atomic: Use new iterator macros in drm_atomic_helper_wait_for_flip_done, again.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5236/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx