Re: [Intel-gfx] [PATCH 1/8] drm/i915/skl+: Prepare for removing data rate from skl watermark state

[Maarten Lankhorst]

Op 20-10-16 om 15:11 schreef Paulo Zanoni:
> Em Qua, 2016-10-12 às 15:28 +0200, Maarten Lankhorst escreveu:
>> Caching is not required, drm_atomic_crtc_state_for_each_plane_state
>> can be used to inspect all plane_states that are assigned to the
>> current crtc_state, so we can just recalculate every time.
> But can't the current for_each_plane_in_state() do the same thing? Why
> is the new macro better? What's the real point here?
for_each_plane_in_state looks at all planes in the current atomic state. It 
doesn't
enumerate planes on a crtc that are not part of it.

This macro takes a crtc_state, and enumerates all plane_states assigned to it,
whether they are part of the atomic state or not. This can be done because 
acquiring
a plane state also requires acquiring crtc_state.

Updating the plane state with this macro is not allowed, because it requires 
that the
plane has to be part of the atomic state. This is why a const drm_plane_state 
is returned.
> As someone who just downloaded the series and started looking at patch
> 1 without looking at the others, this commit message makes zero sense.
> I'd really like if you could explain how the paragraph above is
> connected with the patch below. What does the macro really have to do
> with caching? Perhaps you could elaborate more on the plans of the next
> patches and explain how the changes below enable the grand plan. Please
> do this in the commit message instead of just an email reply.
>
>> Signed-off-by: Maarten Lankhorst 
>> ---
>>  drivers/gpu/drm/i915/intel_pm.c | 27 ---
>>  1 file changed, 12 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>> b/drivers/gpu/drm/i915/intel_pm.c
>> index 6af1587e9d84..b96a899c899d 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -31,6 +31,7 @@
>>  #include "intel_drv.h"
>>  #include "../../../platform/x86/intel_ips.h"
>>  #include 
>> +#include 
>>  
>>  /**
>>   * DOC: RC6
>> @@ -3242,18 +3243,17 @@ skl_get_total_relative_data_rate(struct
>> intel_crtc_state *intel_cstate)
>>  struct drm_crtc *crtc = cstate->crtc;
>>  struct drm_device *dev = crtc->dev;
>>  struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> -const struct drm_plane *plane;
>> +struct drm_plane *plane;
>>  const struct intel_plane *intel_plane;
>> -struct drm_plane_state *pstate;
>> +const struct drm_plane_state *pstate;
>>  unsigned int rate, total_data_rate = 0;
>>  int id;
>> -int i;
>>  
>>  if (WARN_ON(!state))
>>  return 0;
>>  
>>  /* Calculate and cache data rate for each plane */
>> -for_each_plane_in_state(state, plane, pstate, i) {
>> +drm_atomic_crtc_state_for_each_plane_state(plane, pstate,
>> cstate) {
>
> Now we can cut the "if (intel_plane->pipe != intel_crtc->pipe)" check
> this code has.
>
>>  id = skl_wm_plane_id(to_intel_plane(plane));
>>  intel_plane = to_intel_plane(plane);
>>  
>> @@ -3356,7 +3356,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
>> *cstate,
>>  struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>  struct intel_plane *intel_plane;
>>  struct drm_plane *plane;
>> -struct drm_plane_state *pstate;
>> +const struct drm_plane_state *pstate;
>>  enum pipe pipe = intel_crtc->pipe;
>>  struct skl_ddb_entry *alloc = >wm.skl.ddb;
>>  uint16_t alloc_size, start, cursor_blocks;
>> @@ -3392,14 +3392,14 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
>> *cstate,
>>  alloc_size -= cursor_blocks;
>>  
>>  /* 1. Allocate the mininum required blocks for each active
>> plane */
>> -for_each_plane_in_state(state, plane, pstate, i) {
>> +drm_atomic_crtc_state_for_each_plane_state(plane, pstate,
>> >base) {
>>  intel_plane = to_intel_plane(plane);
>>  id = skl_wm_plane_id(intel_plane);
>>  
>>  if (intel_plane->pipe != pipe)
>>  continue;
> Same thing here: cut the check above?
>
>>  
>> -if (!to_intel_plane_state(pstate)->base.visible) {
>> +if (!pstate->visible) {
> I lol'd :)
> I'd probably have done this in a separate patch, since it doesn't seem
> to match the commit message.
>
>>  minimum[id] = 0;
>>  y_minimum[id] = 0;
>>  continue;
>> @@ -3948,7 +3948,7 @@ skl_ddb_add_affected_planes(struct
>> intel_crtc_state *cstate)
>>  
>>  WARN_ON(!drm_atomic_get_existing_crtc_state(state, crtc));
>>  
>> -drm_for_each_plane_mask(plane, dev, crtc->state->plane_mask) 
>> {
>> +drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) 
>
> This should be in a separate patch with a separate commit message
> explaining what exactly changes and why the current code is bad. And as
> Matt pointed, this code is completely based
> on drm_atomic_add_affected_planes(), so if there's something to fix
> 

Re: [Intel-gfx] [PATCH 1/8] drm/i915/skl+: Prepare for removing data rate from skl watermark state

[Paulo Zanoni]

Em Qua, 2016-10-12 às 15:28 +0200, Maarten Lankhorst escreveu:
> Caching is not required, drm_atomic_crtc_state_for_each_plane_state
> can be used to inspect all plane_states that are assigned to the
> current crtc_state, so we can just recalculate every time.

But can't the current for_each_plane_in_state() do the same thing? Why
is the new macro better? What's the real point here?

As someone who just downloaded the series and started looking at patch
1 without looking at the others, this commit message makes zero sense.
I'd really like if you could explain how the paragraph above is
connected with the patch below. What does the macro really have to do
with caching? Perhaps you could elaborate more on the plans of the next
patches and explain how the changes below enable the grand plan. Please
do this in the commit message instead of just an email reply.

> 
> Signed-off-by: Maarten Lankhorst 
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 27 ---
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 6af1587e9d84..b96a899c899d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -31,6 +31,7 @@
>  #include "intel_drv.h"
>  #include "../../../platform/x86/intel_ips.h"
>  #include 
> +#include 
>  
>  /**
>   * DOC: RC6
> @@ -3242,18 +3243,17 @@ skl_get_total_relative_data_rate(struct
> intel_crtc_state *intel_cstate)
>   struct drm_crtc *crtc = cstate->crtc;
>   struct drm_device *dev = crtc->dev;
>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - const struct drm_plane *plane;
> + struct drm_plane *plane;
>   const struct intel_plane *intel_plane;
> - struct drm_plane_state *pstate;
> + const struct drm_plane_state *pstate;
>   unsigned int rate, total_data_rate = 0;
>   int id;
> - int i;
>  
>   if (WARN_ON(!state))
>   return 0;
>  
>   /* Calculate and cache data rate for each plane */
> - for_each_plane_in_state(state, plane, pstate, i) {
> + drm_atomic_crtc_state_for_each_plane_state(plane, pstate,
> cstate) {


Now we can cut the "if (intel_plane->pipe != intel_crtc->pipe)" check
this code has.

>   id = skl_wm_plane_id(to_intel_plane(plane));
>   intel_plane = to_intel_plane(plane);
>  
> @@ -3356,7 +3356,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   struct intel_plane *intel_plane;
>   struct drm_plane *plane;
> - struct drm_plane_state *pstate;
> + const struct drm_plane_state *pstate;
>   enum pipe pipe = intel_crtc->pipe;
>   struct skl_ddb_entry *alloc = >wm.skl.ddb;
>   uint16_t alloc_size, start, cursor_blocks;
> @@ -3392,14 +3392,14 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>   alloc_size -= cursor_blocks;
>  
>   /* 1. Allocate the mininum required blocks for each active
> plane */
> - for_each_plane_in_state(state, plane, pstate, i) {
> + drm_atomic_crtc_state_for_each_plane_state(plane, pstate,
> >base) {
>   intel_plane = to_intel_plane(plane);
>   id = skl_wm_plane_id(intel_plane);
>  
>   if (intel_plane->pipe != pipe)
>   continue;

Same thing here: cut the check above?

>  
> - if (!to_intel_plane_state(pstate)->base.visible) {
> + if (!pstate->visible) {

I lol'd :)
I'd probably have done this in a separate patch, since it doesn't seem
to match the commit message.

>   minimum[id] = 0;
>   y_minimum[id] = 0;
>   continue;
> @@ -3948,7 +3948,7 @@ skl_ddb_add_affected_planes(struct
> intel_crtc_state *cstate)
>  
>   WARN_ON(!drm_atomic_get_existing_crtc_state(state, crtc));
>  
> - drm_for_each_plane_mask(plane, dev, crtc->state->plane_mask) 
> {
> + drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) 


This should be in a separate patch with a separate commit message
explaining what exactly changes and why the current code is bad. And as
Matt pointed, this code is completely based
on drm_atomic_add_affected_planes(), so if there's something to fix
here, there's probably something to fix there.


> {
>   id = skl_wm_plane_id(to_intel_plane(plane));
>  
>   if (skl_ddb_entry_equal(_ddb->plane[pipe][id],
> @@ -4063,14 +4063,12 @@ skl_print_wm_changes(const struct
> drm_atomic_state *state)
>   to_intel_atomic_state(state);
>   const struct drm_crtc *crtc;
>   const struct drm_crtc_state *cstate;
> - const struct drm_plane *plane;
>   const struct intel_plane *intel_plane;
> - const struct drm_plane_state *pstate;
>   const struct skl_ddb_allocation *old_ddb = _priv-
> >wm.skl_hw.ddb;
>   const struct skl_ddb_allocation *new_ddb = 

Re: [Intel-gfx] [PATCH 1/8] drm/i915/skl+: Prepare for removing data rate from skl watermark state

[Maarten Lankhorst]

Op 20-10-16 om 00:13 schreef Matt Roper:
> On Wed, Oct 12, 2016 at 03:28:14PM +0200, Maarten Lankhorst wrote:
>> Caching is not required, drm_atomic_crtc_state_for_each_plane_state
>> can be used to inspect all plane_states that are assigned to the
>> current crtc_state, so we can just recalculate every time.
>>
>> Signed-off-by: Maarten Lankhorst 
>> ---
>>  drivers/gpu/drm/i915/intel_pm.c | 27 ---
>>  1 file changed, 12 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c 
>> b/drivers/gpu/drm/i915/intel_pm.c
>> index 6af1587e9d84..b96a899c899d 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -31,6 +31,7 @@
>>  #include "intel_drv.h"
>>  #include "../../../platform/x86/intel_ips.h"
>>  #include 
>> +#include 
>>  
>>  /**
>>   * DOC: RC6
>> @@ -3242,18 +3243,17 @@ skl_get_total_relative_data_rate(struct 
>> intel_crtc_state *intel_cstate)
>>  struct drm_crtc *crtc = cstate->crtc;
>>  struct drm_device *dev = crtc->dev;
>>  struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> -const struct drm_plane *plane;
>> +struct drm_plane *plane;
>>  const struct intel_plane *intel_plane;
>> -struct drm_plane_state *pstate;
>> +const struct drm_plane_state *pstate;
>>  unsigned int rate, total_data_rate = 0;
>>  int id;
>> -int i;
>>  
>>  if (WARN_ON(!state))
>>  return 0;
>>  
>>  /* Calculate and cache data rate for each plane */
>> -for_each_plane_in_state(state, plane, pstate, i) {
>> +drm_atomic_crtc_state_for_each_plane_state(plane, pstate, cstate) {
>>  id = skl_wm_plane_id(to_intel_plane(plane));
>>  intel_plane = to_intel_plane(plane);
>>  
>> @@ -3356,7 +3356,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>>  struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>  struct intel_plane *intel_plane;
>>  struct drm_plane *plane;
>> -struct drm_plane_state *pstate;
>> +const struct drm_plane_state *pstate;
>>  enum pipe pipe = intel_crtc->pipe;
>>  struct skl_ddb_entry *alloc = >wm.skl.ddb;
>>  uint16_t alloc_size, start, cursor_blocks;
>> @@ -3392,14 +3392,14 @@ skl_allocate_pipe_ddb(struct intel_crtc_state 
>> *cstate,
>>  alloc_size -= cursor_blocks;
>>  
>>  /* 1. Allocate the mininum required blocks for each active plane */
>> -for_each_plane_in_state(state, plane, pstate, i) {
>> +drm_atomic_crtc_state_for_each_plane_state(plane, pstate, 
>> >base) {
>>  intel_plane = to_intel_plane(plane);
>>  id = skl_wm_plane_id(intel_plane);
>>  
>>  if (intel_plane->pipe != pipe)
>>  continue;
>>  
>> -if (!to_intel_plane_state(pstate)->base.visible) {
>> +if (!pstate->visible) {
>>  minimum[id] = 0;
>>  y_minimum[id] = 0;
>>  continue;
>> @@ -3948,7 +3948,7 @@ skl_ddb_add_affected_planes(struct intel_crtc_state 
>> *cstate)
>>  
>>  WARN_ON(!drm_atomic_get_existing_crtc_state(state, crtc));
>>  
>> -drm_for_each_plane_mask(plane, dev, crtc->state->plane_mask) {
>> +drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) {
> Is this change necessary?  Any plane that differs between the two masks
> should already be part of our state, so I don't think this changes the
> behavior at all.  The original 'crtc->state->plane_mask' form is closer
> to the drm_atomic_add_affected_planes() that this function is modeled
> after so my slight preference would be to leave it alone for
> consistency.
>
> Aside from that, this patch is
Not completely, but I was removing it since I'm trying to get rid of all 
pointers to obj->state as much as I can.
All accesses should be through some wrapper functions, still working out the 
specifics.

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

Re: [Intel-gfx] [PATCH 1/8] drm/i915/skl+: Prepare for removing data rate from skl watermark state

[Matt Roper]

On Wed, Oct 12, 2016 at 03:28:14PM +0200, Maarten Lankhorst wrote:
> Caching is not required, drm_atomic_crtc_state_for_each_plane_state
> can be used to inspect all plane_states that are assigned to the
> current crtc_state, so we can just recalculate every time.
> 
> Signed-off-by: Maarten Lankhorst 
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 27 ---
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 6af1587e9d84..b96a899c899d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -31,6 +31,7 @@
>  #include "intel_drv.h"
>  #include "../../../platform/x86/intel_ips.h"
>  #include 
> +#include 
>  
>  /**
>   * DOC: RC6
> @@ -3242,18 +3243,17 @@ skl_get_total_relative_data_rate(struct 
> intel_crtc_state *intel_cstate)
>   struct drm_crtc *crtc = cstate->crtc;
>   struct drm_device *dev = crtc->dev;
>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - const struct drm_plane *plane;
> + struct drm_plane *plane;
>   const struct intel_plane *intel_plane;
> - struct drm_plane_state *pstate;
> + const struct drm_plane_state *pstate;
>   unsigned int rate, total_data_rate = 0;
>   int id;
> - int i;
>  
>   if (WARN_ON(!state))
>   return 0;
>  
>   /* Calculate and cache data rate for each plane */
> - for_each_plane_in_state(state, plane, pstate, i) {
> + drm_atomic_crtc_state_for_each_plane_state(plane, pstate, cstate) {
>   id = skl_wm_plane_id(to_intel_plane(plane));
>   intel_plane = to_intel_plane(plane);
>  
> @@ -3356,7 +3356,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   struct intel_plane *intel_plane;
>   struct drm_plane *plane;
> - struct drm_plane_state *pstate;
> + const struct drm_plane_state *pstate;
>   enum pipe pipe = intel_crtc->pipe;
>   struct skl_ddb_entry *alloc = >wm.skl.ddb;
>   uint16_t alloc_size, start, cursor_blocks;
> @@ -3392,14 +3392,14 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>   alloc_size -= cursor_blocks;
>  
>   /* 1. Allocate the mininum required blocks for each active plane */
> - for_each_plane_in_state(state, plane, pstate, i) {
> + drm_atomic_crtc_state_for_each_plane_state(plane, pstate, 
> >base) {
>   intel_plane = to_intel_plane(plane);
>   id = skl_wm_plane_id(intel_plane);
>  
>   if (intel_plane->pipe != pipe)
>   continue;
>  
> - if (!to_intel_plane_state(pstate)->base.visible) {
> + if (!pstate->visible) {
>   minimum[id] = 0;
>   y_minimum[id] = 0;
>   continue;
> @@ -3948,7 +3948,7 @@ skl_ddb_add_affected_planes(struct intel_crtc_state 
> *cstate)
>  
>   WARN_ON(!drm_atomic_get_existing_crtc_state(state, crtc));
>  
> - drm_for_each_plane_mask(plane, dev, crtc->state->plane_mask) {
> + drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) {

Is this change necessary?  Any plane that differs between the two masks
should already be part of our state, so I don't think this changes the
behavior at all.  The original 'crtc->state->plane_mask' form is closer
to the drm_atomic_add_affected_planes() that this function is modeled
after so my slight preference would be to leave it alone for
consistency.

Aside from that, this patch is

Reviewed-by: Matt Roper 

I remember when I first wrote an early version of this code it was just
doing a drm_atomic_get_plane_state() on each plane unconditionally,
which was non-optimal.  When I reworked it, I wasn't aware of
drm_atomic_crtc_state_for_each_plane_state (or maybe it didn't exist
yet), so I used caching as an alternative.  But the new approach is much
better so I'm glad we can get rid of the caching.


Matt

>   id = skl_wm_plane_id(to_intel_plane(plane));
>  
>   if (skl_ddb_entry_equal(_ddb->plane[pipe][id],
> @@ -4063,14 +4063,12 @@ skl_print_wm_changes(const struct drm_atomic_state 
> *state)
>   to_intel_atomic_state(state);
>   const struct drm_crtc *crtc;
>   const struct drm_crtc_state *cstate;
> - const struct drm_plane *plane;
>   const struct intel_plane *intel_plane;
> - const struct drm_plane_state *pstate;
>   const struct skl_ddb_allocation *old_ddb = _priv->wm.skl_hw.ddb;
>   const struct skl_ddb_allocation *new_ddb = _state->wm_results.ddb;
>   enum pipe pipe;
>   int id;
> - int i, j;
> + int i;
>  
>   for_each_crtc_in_state(state, crtc, cstate, i) {
>   if (!crtc->state)
> @@ -4078,10 +4076,9 @@ skl_print_wm_changes(const struct drm_atomic_state 
> *state)
>  
>   pipe = 

[Intel-gfx] [PATCH 1/8] drm/i915/skl+: Prepare for removing data rate from skl watermark state

[Maarten Lankhorst]

Caching is not required, drm_atomic_crtc_state_for_each_plane_state
can be used to inspect all plane_states that are assigned to the
current crtc_state, so we can just recalculate every time.

Signed-off-by: Maarten Lankhorst 
---
 drivers/gpu/drm/i915/intel_pm.c | 27 ---
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 6af1587e9d84..b96a899c899d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -31,6 +31,7 @@
 #include "intel_drv.h"
 #include "../../../platform/x86/intel_ips.h"
 #include 
+#include 
 
 /**
  * DOC: RC6
@@ -3242,18 +3243,17 @@ skl_get_total_relative_data_rate(struct 
intel_crtc_state *intel_cstate)
struct drm_crtc *crtc = cstate->crtc;
struct drm_device *dev = crtc->dev;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-   const struct drm_plane *plane;
+   struct drm_plane *plane;
const struct intel_plane *intel_plane;
-   struct drm_plane_state *pstate;
+   const struct drm_plane_state *pstate;
unsigned int rate, total_data_rate = 0;
int id;
-   int i;
 
if (WARN_ON(!state))
return 0;
 
/* Calculate and cache data rate for each plane */
-   for_each_plane_in_state(state, plane, pstate, i) {
+   drm_atomic_crtc_state_for_each_plane_state(plane, pstate, cstate) {
id = skl_wm_plane_id(to_intel_plane(plane));
intel_plane = to_intel_plane(plane);
 
@@ -3356,7 +3356,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
struct intel_plane *intel_plane;
struct drm_plane *plane;
-   struct drm_plane_state *pstate;
+   const struct drm_plane_state *pstate;
enum pipe pipe = intel_crtc->pipe;
struct skl_ddb_entry *alloc = >wm.skl.ddb;
uint16_t alloc_size, start, cursor_blocks;
@@ -3392,14 +3392,14 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
alloc_size -= cursor_blocks;
 
/* 1. Allocate the mininum required blocks for each active plane */
-   for_each_plane_in_state(state, plane, pstate, i) {
+   drm_atomic_crtc_state_for_each_plane_state(plane, pstate, 
>base) {
intel_plane = to_intel_plane(plane);
id = skl_wm_plane_id(intel_plane);
 
if (intel_plane->pipe != pipe)
continue;
 
-   if (!to_intel_plane_state(pstate)->base.visible) {
+   if (!pstate->visible) {
minimum[id] = 0;
y_minimum[id] = 0;
continue;
@@ -3948,7 +3948,7 @@ skl_ddb_add_affected_planes(struct intel_crtc_state 
*cstate)
 
WARN_ON(!drm_atomic_get_existing_crtc_state(state, crtc));
 
-   drm_for_each_plane_mask(plane, dev, crtc->state->plane_mask) {
+   drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) {
id = skl_wm_plane_id(to_intel_plane(plane));
 
if (skl_ddb_entry_equal(_ddb->plane[pipe][id],
@@ -4063,14 +4063,12 @@ skl_print_wm_changes(const struct drm_atomic_state 
*state)
to_intel_atomic_state(state);
const struct drm_crtc *crtc;
const struct drm_crtc_state *cstate;
-   const struct drm_plane *plane;
const struct intel_plane *intel_plane;
-   const struct drm_plane_state *pstate;
const struct skl_ddb_allocation *old_ddb = _priv->wm.skl_hw.ddb;
const struct skl_ddb_allocation *new_ddb = _state->wm_results.ddb;
enum pipe pipe;
int id;
-   int i, j;
+   int i;
 
for_each_crtc_in_state(state, crtc, cstate, i) {
if (!crtc->state)
@@ -4078,10 +4076,9 @@ skl_print_wm_changes(const struct drm_atomic_state 
*state)
 
pipe = to_intel_crtc(crtc)->pipe;
 
-   for_each_plane_in_state(state, plane, pstate, j) {
+   for_each_intel_plane_on_crtc(dev, to_intel_crtc(crtc), 
intel_plane) {
const struct skl_ddb_entry *old, *new;
 
-   intel_plane = to_intel_plane(plane);
id = skl_wm_plane_id(intel_plane);
old = _ddb->plane[pipe][id];
new = _ddb->plane[pipe][id];
@@ -4094,13 +4091,13 @@ skl_print_wm_changes(const struct drm_atomic_state 
*state)
 
if (id != PLANE_CURSOR) {
DRM_DEBUG_ATOMIC("[PLANE:%d:plane %d%c] ddb (%d 
- %d) -> (%d - %d)\n",
-plane->base.id, id + 1,
+intel_plane->base.base.id, id 
+ 1,
 pipe_name(pipe),
 old->start, old->end,