Re: [PATCH 2/6] drm/amd/display: Use new DRM API where possible

[Harry Wentland]

On 2017-10-13 01:26 PM, Andrey Grodzovsky wrote:
> 
> 
> On 10/13/2017 12:18 PM, Harry Wentland wrote:
>> On 2017-10-12 05:15 PM, sunpeng...@amd.com wrote:
>>> From: "Leo (Sunpeng) Li" 
>>>
>>> To conform to DRM's new API, we should not be accessing a DRM object's
>>> internal state directly. Rather, the DRM for_each_old/new_* iterators,
>>> and drm_atomic_get_old/new_* interface should be used.
>>>
>>> This is an ongoing process. For now, update the DRM-facing atomic
>>> functions, where the atomic state object is given.
>>>
>>> Signed-off-by: Leo (Sunpeng) Li 
>>> ---
>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 131 
>>> +++---
>>>   1 file changed, 66 insertions(+), 65 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> index cc024ab..d4426b3 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -3873,28 +3873,31 @@ static void amdgpu_dm_commit_planes(struct 
>>> drm_atomic_state *state,
>>>   {
>>>   uint32_t i;
>>>   struct drm_plane *plane;
>>> -    struct drm_plane_state *old_plane_state;
>>> +    struct drm_plane_state *old_plane_state, *new_plane_state;
>>>   struct dc_stream_state *dc_stream_attach;
>>>   struct dc_plane_state *plane_states_constructed[MAX_SURFACES];
>>>   struct amdgpu_crtc *acrtc_attach = to_amdgpu_crtc(pcrtc);
>>> -    struct dm_crtc_state *acrtc_state = to_dm_crtc_state(pcrtc->state);
>>> +    struct drm_crtc_state *new_pcrtc_state =
>>> +    drm_atomic_get_new_crtc_state(state, pcrtc);
>>> +    struct dm_crtc_state *acrtc_state = to_dm_crtc_state(new_pcrtc_state);
>>>   int planes_count = 0;
>>>   unsigned long flags;
>>>     /* update planes when needed */
>>> -    for_each_old_plane_in_state(state, plane, old_plane_state, i) {
>>> -    struct drm_plane_state *plane_state = plane->state;
>>> -    struct drm_crtc *crtc = plane_state->crtc;
>>> -    struct drm_framebuffer *fb = plane_state->fb;
>>> +    for_each_oldnew_plane_in_state(state, plane, old_plane_state, 
>>> new_plane_state, i) {
>>> +    struct drm_crtc *crtc = new_plane_state->crtc;
>>> +    struct drm_crtc_state *new_crtc_state =
>>> +    drm_atomic_get_new_crtc_state(state, crtc);
>>> +    struct drm_framebuffer *fb = new_plane_state->fb;
>>>   bool pflip_needed;
>>> -    struct dm_plane_state *dm_plane_state = 
>>> to_dm_plane_state(plane_state);
>>> +    struct dm_plane_state *dm_plane_state = 
>>> to_dm_plane_state(new_plane_state);
>>>     if (plane->type == DRM_PLANE_TYPE_CURSOR) {
>>>   handle_cursor_update(plane, old_plane_state);
>>>   continue;
>>>   }
>>>   -    if (!fb || !crtc || pcrtc != crtc || !crtc->state->active)
>>> +    if (!fb || !crtc || pcrtc != crtc || !new_crtc_state->active)
>>>   continue;
>>>     pflip_needed = !state->allow_modeset;
>>> @@ -3918,13 +3921,13 @@ static void amdgpu_dm_commit_planes(struct 
>>> drm_atomic_state *state,
>>>   dc_stream_attach = acrtc_state->stream;
>>>   planes_count++;
>>>   -    } else if (crtc->state->planes_changed) {
>>> +    } else if (new_crtc_state->planes_changed) {
>>>   /* Assume even ONE crtc with immediate flip means
>>>    * entire can't wait for VBLANK
>>>    * TODO Check if it's correct
>>>    */
>>>   *wait_for_vblank =
>>> -    pcrtc->state->pageflip_flags & 
>>> DRM_MODE_PAGE_FLIP_ASYNC ?
>>> +    new_pcrtc_state->pageflip_flags & 
>>> DRM_MODE_PAGE_FLIP_ASYNC ?
>>>   false : true;
>>>     /* TODO: Needs rework for multiplane flip */
>>> @@ -3942,7 +3945,7 @@ static void amdgpu_dm_commit_planes(struct 
>>> drm_atomic_state *state,
>>>   if (planes_count) {
>>>   unsigned long flags;
>>>   -    if (pcrtc->state->event) {
>>> +    if (new_pcrtc_state->event) {
>>>     drm_crtc_vblank_get(pcrtc);
>>>   @@ -3968,7 +3971,7 @@ int amdgpu_dm_atomic_commit(
>>>   bool nonblock)
>>>   {
>>>   struct drm_crtc *crtc;
>>> -    struct drm_crtc_state *new_state;
>>> +    struct drm_crtc_state *old_crtc_state, *new_state;
>>>   struct amdgpu_device *adev = dev->dev_private;
>>>   int i;
>>>   @@ -3979,8 +3982,8 @@ int amdgpu_dm_atomic_commit(
>>>    * it will update crtc->dm_crtc_state->stream pointer which is used in
>>>    * the ISRs.
>>>    */
>>> -    for_each_new_crtc_in_state(state, crtc, new_state, i) {
>>> -    struct dm_crtc_state *old_acrtc_state = 
>>> to_dm_crtc_state(crtc->state);
>>> +    for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_state, 
>>> i) {
>>> +    struct dm_crtc_state *old_acrtc_state = 
>>> 

Re: [PATCH 2/6] drm/amd/display: Use new DRM API where possible

[Andrey Grodzovsky]

On 10/13/2017 12:18 PM, Harry Wentland wrote:

On 2017-10-12 05:15 PM, sunpeng...@amd.com wrote:

From: "Leo (Sunpeng) Li" 

To conform to DRM's new API, we should not be accessing a DRM object's
internal state directly. Rather, the DRM for_each_old/new_* iterators,
and drm_atomic_get_old/new_* interface should be used.

This is an ongoing process. For now, update the DRM-facing atomic
functions, where the atomic state object is given.

Signed-off-by: Leo (Sunpeng) Li 
---
  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 131 +++---
  1 file changed, 66 insertions(+), 65 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index cc024ab..d4426b3 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3873,28 +3873,31 @@ static void amdgpu_dm_commit_planes(struct 
drm_atomic_state *state,
  {
uint32_t i;
struct drm_plane *plane;
-   struct drm_plane_state *old_plane_state;
+   struct drm_plane_state *old_plane_state, *new_plane_state;
struct dc_stream_state *dc_stream_attach;
struct dc_plane_state *plane_states_constructed[MAX_SURFACES];
struct amdgpu_crtc *acrtc_attach = to_amdgpu_crtc(pcrtc);
-   struct dm_crtc_state *acrtc_state = to_dm_crtc_state(pcrtc->state);
+   struct drm_crtc_state *new_pcrtc_state =
+   drm_atomic_get_new_crtc_state(state, pcrtc);
+   struct dm_crtc_state *acrtc_state = to_dm_crtc_state(new_pcrtc_state);
int planes_count = 0;
unsigned long flags;
  
  	/* update planes when needed */

-   for_each_old_plane_in_state(state, plane, old_plane_state, i) {
-   struct drm_plane_state *plane_state = plane->state;
-   struct drm_crtc *crtc = plane_state->crtc;
-   struct drm_framebuffer *fb = plane_state->fb;
+   for_each_oldnew_plane_in_state(state, plane, old_plane_state, 
new_plane_state, i) {
+   struct drm_crtc *crtc = new_plane_state->crtc;
+   struct drm_crtc_state *new_crtc_state =
+   drm_atomic_get_new_crtc_state(state, crtc);
+   struct drm_framebuffer *fb = new_plane_state->fb;
bool pflip_needed;
-   struct dm_plane_state *dm_plane_state = 
to_dm_plane_state(plane_state);
+   struct dm_plane_state *dm_plane_state = 
to_dm_plane_state(new_plane_state);
  
  		if (plane->type == DRM_PLANE_TYPE_CURSOR) {

handle_cursor_update(plane, old_plane_state);
continue;
}
  
-		if (!fb || !crtc || pcrtc != crtc || !crtc->state->active)

+   if (!fb || !crtc || pcrtc != crtc || !new_crtc_state->active)
continue;
  
  		pflip_needed = !state->allow_modeset;

@@ -3918,13 +3921,13 @@ static void amdgpu_dm_commit_planes(struct 
drm_atomic_state *state,
dc_stream_attach = acrtc_state->stream;
planes_count++;
  
-		} else if (crtc->state->planes_changed) {

+   } else if (new_crtc_state->planes_changed) {
/* Assume even ONE crtc with immediate flip means
 * entire can't wait for VBLANK
 * TODO Check if it's correct
 */
*wait_for_vblank =
-   pcrtc->state->pageflip_flags & 
DRM_MODE_PAGE_FLIP_ASYNC ?
+   new_pcrtc_state->pageflip_flags & 
DRM_MODE_PAGE_FLIP_ASYNC ?
false : true;
  
  			/* TODO: Needs rework for multiplane flip */

@@ -3942,7 +3945,7 @@ static void amdgpu_dm_commit_planes(struct 
drm_atomic_state *state,
if (planes_count) {
unsigned long flags;
  
-		if (pcrtc->state->event) {

+   if (new_pcrtc_state->event) {
  
  			drm_crtc_vblank_get(pcrtc);
  
@@ -3968,7 +3971,7 @@ int amdgpu_dm_atomic_commit(

bool nonblock)
  {
struct drm_crtc *crtc;
-   struct drm_crtc_state *new_state;
+   struct drm_crtc_state *old_crtc_state, *new_state;
struct amdgpu_device *adev = dev->dev_private;
int i;
  
@@ -3979,8 +3982,8 @@ int amdgpu_dm_atomic_commit(

 * it will update crtc->dm_crtc_state->stream pointer which is used in
 * the ISRs.
 */
-   for_each_new_crtc_in_state(state, crtc, new_state, i) {
-   struct dm_crtc_state *old_acrtc_state = 
to_dm_crtc_state(crtc->state);
+   for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_state, 
i) {
+   struct dm_crtc_state *old_acrtc_state = 
to_dm_crtc_state(old_crtc_state);
struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
  
  		if 

Re: [PATCH 2/6] drm/amd/display: Use new DRM API where possible

[Harry Wentland]

On 2017-10-12 05:15 PM, sunpeng...@amd.com wrote:
> From: "Leo (Sunpeng) Li" 
> 
> To conform to DRM's new API, we should not be accessing a DRM object's
> internal state directly. Rather, the DRM for_each_old/new_* iterators,
> and drm_atomic_get_old/new_* interface should be used.
> 
> This is an ongoing process. For now, update the DRM-facing atomic
> functions, where the atomic state object is given.
> 
> Signed-off-by: Leo (Sunpeng) Li 
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 131 
> +++---
>  1 file changed, 66 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index cc024ab..d4426b3 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3873,28 +3873,31 @@ static void amdgpu_dm_commit_planes(struct 
> drm_atomic_state *state,
>  {
>   uint32_t i;
>   struct drm_plane *plane;
> - struct drm_plane_state *old_plane_state;
> + struct drm_plane_state *old_plane_state, *new_plane_state;
>   struct dc_stream_state *dc_stream_attach;
>   struct dc_plane_state *plane_states_constructed[MAX_SURFACES];
>   struct amdgpu_crtc *acrtc_attach = to_amdgpu_crtc(pcrtc);
> - struct dm_crtc_state *acrtc_state = to_dm_crtc_state(pcrtc->state);
> + struct drm_crtc_state *new_pcrtc_state =
> + drm_atomic_get_new_crtc_state(state, pcrtc);
> + struct dm_crtc_state *acrtc_state = to_dm_crtc_state(new_pcrtc_state);
>   int planes_count = 0;
>   unsigned long flags;
>  
>   /* update planes when needed */
> - for_each_old_plane_in_state(state, plane, old_plane_state, i) {
> - struct drm_plane_state *plane_state = plane->state;
> - struct drm_crtc *crtc = plane_state->crtc;
> - struct drm_framebuffer *fb = plane_state->fb;
> + for_each_oldnew_plane_in_state(state, plane, old_plane_state, 
> new_plane_state, i) {
> + struct drm_crtc *crtc = new_plane_state->crtc;
> + struct drm_crtc_state *new_crtc_state =
> + drm_atomic_get_new_crtc_state(state, crtc);
> + struct drm_framebuffer *fb = new_plane_state->fb;
>   bool pflip_needed;
> - struct dm_plane_state *dm_plane_state = 
> to_dm_plane_state(plane_state);
> + struct dm_plane_state *dm_plane_state = 
> to_dm_plane_state(new_plane_state);
>  
>   if (plane->type == DRM_PLANE_TYPE_CURSOR) {
>   handle_cursor_update(plane, old_plane_state);
>   continue;
>   }
>  
> - if (!fb || !crtc || pcrtc != crtc || !crtc->state->active)
> + if (!fb || !crtc || pcrtc != crtc || !new_crtc_state->active)
>   continue;
>  
>   pflip_needed = !state->allow_modeset;
> @@ -3918,13 +3921,13 @@ static void amdgpu_dm_commit_planes(struct 
> drm_atomic_state *state,
>   dc_stream_attach = acrtc_state->stream;
>   planes_count++;
>  
> - } else if (crtc->state->planes_changed) {
> + } else if (new_crtc_state->planes_changed) {
>   /* Assume even ONE crtc with immediate flip means
>* entire can't wait for VBLANK
>* TODO Check if it's correct
>*/
>   *wait_for_vblank =
> - pcrtc->state->pageflip_flags & 
> DRM_MODE_PAGE_FLIP_ASYNC ?
> + new_pcrtc_state->pageflip_flags & 
> DRM_MODE_PAGE_FLIP_ASYNC ?
>   false : true;
>  
>   /* TODO: Needs rework for multiplane flip */
> @@ -3942,7 +3945,7 @@ static void amdgpu_dm_commit_planes(struct 
> drm_atomic_state *state,
>   if (planes_count) {
>   unsigned long flags;
>  
> - if (pcrtc->state->event) {
> + if (new_pcrtc_state->event) {
>  
>   drm_crtc_vblank_get(pcrtc);
>  
> @@ -3968,7 +3971,7 @@ int amdgpu_dm_atomic_commit(
>   bool nonblock)
>  {
>   struct drm_crtc *crtc;
> - struct drm_crtc_state *new_state;
> + struct drm_crtc_state *old_crtc_state, *new_state;
>   struct amdgpu_device *adev = dev->dev_private;
>   int i;
>  
> @@ -3979,8 +3982,8 @@ int amdgpu_dm_atomic_commit(
>* it will update crtc->dm_crtc_state->stream pointer which is used in
>* the ISRs.
>*/
> - for_each_new_crtc_in_state(state, crtc, new_state, i) {
> - struct dm_crtc_state *old_acrtc_state = 
> to_dm_crtc_state(crtc->state);
> + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_state, 
> i) {
> + struct dm_crtc_state *old_acrtc_state = 
> 

[PATCH 2/6] drm/amd/display: Use new DRM API where possible

[sunpeng.li]

From: "Leo (Sunpeng) Li" 

To conform to DRM's new API, we should not be accessing a DRM object's
internal state directly. Rather, the DRM for_each_old/new_* iterators,
and drm_atomic_get_old/new_* interface should be used.

This is an ongoing process. For now, update the DRM-facing atomic
functions, where the atomic state object is given.

Signed-off-by: Leo (Sunpeng) Li 
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 131 +++---
 1 file changed, 66 insertions(+), 65 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index cc024ab..d4426b3 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3873,28 +3873,31 @@ static void amdgpu_dm_commit_planes(struct 
drm_atomic_state *state,
 {
uint32_t i;
struct drm_plane *plane;
-   struct drm_plane_state *old_plane_state;
+   struct drm_plane_state *old_plane_state, *new_plane_state;
struct dc_stream_state *dc_stream_attach;
struct dc_plane_state *plane_states_constructed[MAX_SURFACES];
struct amdgpu_crtc *acrtc_attach = to_amdgpu_crtc(pcrtc);
-   struct dm_crtc_state *acrtc_state = to_dm_crtc_state(pcrtc->state);
+   struct drm_crtc_state *new_pcrtc_state =
+   drm_atomic_get_new_crtc_state(state, pcrtc);
+   struct dm_crtc_state *acrtc_state = to_dm_crtc_state(new_pcrtc_state);
int planes_count = 0;
unsigned long flags;
 
/* update planes when needed */
-   for_each_old_plane_in_state(state, plane, old_plane_state, i) {
-   struct drm_plane_state *plane_state = plane->state;
-   struct drm_crtc *crtc = plane_state->crtc;
-   struct drm_framebuffer *fb = plane_state->fb;
+   for_each_oldnew_plane_in_state(state, plane, old_plane_state, 
new_plane_state, i) {
+   struct drm_crtc *crtc = new_plane_state->crtc;
+   struct drm_crtc_state *new_crtc_state =
+   drm_atomic_get_new_crtc_state(state, crtc);
+   struct drm_framebuffer *fb = new_plane_state->fb;
bool pflip_needed;
-   struct dm_plane_state *dm_plane_state = 
to_dm_plane_state(plane_state);
+   struct dm_plane_state *dm_plane_state = 
to_dm_plane_state(new_plane_state);
 
if (plane->type == DRM_PLANE_TYPE_CURSOR) {
handle_cursor_update(plane, old_plane_state);
continue;
}
 
-   if (!fb || !crtc || pcrtc != crtc || !crtc->state->active)
+   if (!fb || !crtc || pcrtc != crtc || !new_crtc_state->active)
continue;
 
pflip_needed = !state->allow_modeset;
@@ -3918,13 +3921,13 @@ static void amdgpu_dm_commit_planes(struct 
drm_atomic_state *state,
dc_stream_attach = acrtc_state->stream;
planes_count++;
 
-   } else if (crtc->state->planes_changed) {
+   } else if (new_crtc_state->planes_changed) {
/* Assume even ONE crtc with immediate flip means
 * entire can't wait for VBLANK
 * TODO Check if it's correct
 */
*wait_for_vblank =
-   pcrtc->state->pageflip_flags & 
DRM_MODE_PAGE_FLIP_ASYNC ?
+   new_pcrtc_state->pageflip_flags & 
DRM_MODE_PAGE_FLIP_ASYNC ?
false : true;
 
/* TODO: Needs rework for multiplane flip */
@@ -3942,7 +3945,7 @@ static void amdgpu_dm_commit_planes(struct 
drm_atomic_state *state,
if (planes_count) {
unsigned long flags;
 
-   if (pcrtc->state->event) {
+   if (new_pcrtc_state->event) {
 
drm_crtc_vblank_get(pcrtc);
 
@@ -3968,7 +3971,7 @@ int amdgpu_dm_atomic_commit(
bool nonblock)
 {
struct drm_crtc *crtc;
-   struct drm_crtc_state *new_state;
+   struct drm_crtc_state *old_crtc_state, *new_state;
struct amdgpu_device *adev = dev->dev_private;
int i;
 
@@ -3979,8 +3982,8 @@ int amdgpu_dm_atomic_commit(
 * it will update crtc->dm_crtc_state->stream pointer which is used in
 * the ISRs.
 */
-   for_each_new_crtc_in_state(state, crtc, new_state, i) {
-   struct dm_crtc_state *old_acrtc_state = 
to_dm_crtc_state(crtc->state);
+   for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_state, 
i) {
+   struct dm_crtc_state *old_acrtc_state = 
to_dm_crtc_state(old_crtc_state);
struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
 
if (drm_atomic_crtc_needs_modeset(new_state)