Re: [Intel-gfx] [PATCH 4/6] drm/i915/gen9: Make skl_wm_level per-plane

[Maarten Lankhorst]

Op 05-10-16 om 22:33 schreef Paulo Zanoni:
> Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu:
>> Having skl_wm_level contain all of the watermarks for each plane is
>> annoying since it prevents us from having any sort of object to
>> represent a single watermark level, something we take advantage of in
>> the next commit to cut down on all of the copy paste code in here.
> I'd like to start my review pointing that I really like this patch. I
> agree the current form is annoying.
>
> See below for some details.
>
>
>> Signed-off-by: Lyude 
>> Cc: Maarten Lankhorst 
>> Cc: Ville Syrjälä 
>> Cc: Matt Roper 
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h  |   6 +-
>>  drivers/gpu/drm/i915/intel_drv.h |   6 +-
>>  drivers/gpu/drm/i915/intel_pm.c  | 208 +--
>> 
>>  3 files changed, 100 insertions(+), 120 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index d26e5999..0f97d43 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1648,9 +1648,9 @@ struct skl_wm_values {
>>  };
>>  
>>  struct skl_wm_level {
>> -bool plane_en[I915_MAX_PLANES];
>> -uint16_t plane_res_b[I915_MAX_PLANES];
>> -uint8_t plane_res_l[I915_MAX_PLANES];
>> +bool plane_en;
>> +uint16_t plane_res_b;
>> +uint8_t plane_res_l;
>>  };
>>  
>>  /*
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 35ba282..d684f4f 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -468,9 +468,13 @@ struct intel_pipe_wm {
>>  bool sprites_scaled;
>>  };
>>  
>> -struct skl_pipe_wm {
>> +struct skl_plane_wm {
>>  struct skl_wm_level wm[8];
>>  struct skl_wm_level trans_wm;
>> +};
>> +
>> +struct skl_pipe_wm {
>> +struct skl_plane_wm planes[I915_MAX_PLANES];
>>  uint32_t linetime;
>>  };
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>> b/drivers/gpu/drm/i915/intel_pm.c
>> index af96888..250f12d 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3668,67 +3668,52 @@ static int
>>  skl_compute_wm_level(const struct drm_i915_private *dev_priv,
>>   struct skl_ddb_allocation *ddb,
>>   struct intel_crtc_state *cstate,
>> + struct intel_plane *intel_plane,
>>   int level,
>>   struct skl_wm_level *result)
>>  {
>>  struct drm_atomic_state *state = cstate->base.state;
>>  struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
>>> base.crtc);
>> -struct drm_plane *plane;
>> -struct intel_plane *intel_plane;
>> -struct intel_plane_state *intel_pstate;
>> +struct drm_plane *plane = _plane->base;
>> +struct intel_plane_state *intel_pstate = NULL;
>>  uint16_t ddb_blocks;
>>  enum pipe pipe = intel_crtc->pipe;
>>  int ret;
>> +int i = skl_wm_plane_id(intel_plane);
>> +
>> +if (state)
>> +intel_pstate =
>> +intel_atomic_get_existing_plane_state(state,
>> +  intel_
>> plane);
>>  
>>  /*
>> - * We'll only calculate watermarks for planes that are
>> actually
>> - * enabled, so make sure all other planes are set as
>> disabled.
>> + * Note: If we start supporting multiple pending atomic
>> commits against
>> + * the same planes/CRTC's in the future, plane->state will
>> no longer be
>> + * the correct pre-state to use for the calculations here
>> and we'll
>> + * need to change where we get the 'unchanged' plane data
>> from.
>> + *
>> + * For now this is fine because we only allow one queued
>> commit against
>> + * a CRTC.  Even if the plane isn't modified by this
>> transaction and we
>> + * don't have a plane lock, we still have the CRTC's lock,
>> so we know
>> + * that no other transactions are racing with us to update
>> it.
>>   */
>> -memset(result, 0, sizeof(*result));
>> -
>> -for_each_intel_plane_mask(_priv->drm,
>> -  intel_plane,
>> -  cstate->base.plane_mask) {
>> -int i = skl_wm_plane_id(intel_plane);
>> -
>> -plane = _plane->base;
>> -intel_pstate = NULL;
>> -if (state)
>> -intel_pstate =
>> -intel_atomic_get_existing_plane_stat
>> e(state,
>> -
>>   intel_plane);
>> +if (!intel_pstate)
>> +intel_pstate = to_intel_plane_state(plane->state);
>>  
>> -/*
>> - * Note: If we start supporting multiple pending
>> atomic commits
>> - * against the same planes/CRTC's in the future,
>> plane->state
>> - * 

Re: [Intel-gfx] [PATCH 4/6] drm/i915/gen9: Make skl_wm_level per-plane

[Paulo Zanoni]

Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu:
> Having skl_wm_level contain all of the watermarks for each plane is
> annoying since it prevents us from having any sort of object to
> represent a single watermark level, something we take advantage of in
> the next commit to cut down on all of the copy paste code in here.

I'd like to start my review pointing that I really like this patch. I
agree the current form is annoying.

See below for some details.


> 
> Signed-off-by: Lyude 
> Cc: Maarten Lankhorst 
> Cc: Ville Syrjälä 
> Cc: Matt Roper 
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |   6 +-
>  drivers/gpu/drm/i915/intel_drv.h |   6 +-
>  drivers/gpu/drm/i915/intel_pm.c  | 208 +--
> 
>  3 files changed, 100 insertions(+), 120 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index d26e5999..0f97d43 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1648,9 +1648,9 @@ struct skl_wm_values {
>  };
>  
>  struct skl_wm_level {
> - bool plane_en[I915_MAX_PLANES];
> - uint16_t plane_res_b[I915_MAX_PLANES];
> - uint8_t plane_res_l[I915_MAX_PLANES];
> + bool plane_en;
> + uint16_t plane_res_b;
> + uint8_t plane_res_l;
>  };
>  
>  /*
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 35ba282..d684f4f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -468,9 +468,13 @@ struct intel_pipe_wm {
>   bool sprites_scaled;
>  };
>  
> -struct skl_pipe_wm {
> +struct skl_plane_wm {
>   struct skl_wm_level wm[8];
>   struct skl_wm_level trans_wm;
> +};
> +
> +struct skl_pipe_wm {
> + struct skl_plane_wm planes[I915_MAX_PLANES];
>   uint32_t linetime;
>  };
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index af96888..250f12d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3668,67 +3668,52 @@ static int
>  skl_compute_wm_level(const struct drm_i915_private *dev_priv,
>    struct skl_ddb_allocation *ddb,
>    struct intel_crtc_state *cstate,
> +  struct intel_plane *intel_plane,
>    int level,
>    struct skl_wm_level *result)
>  {
>   struct drm_atomic_state *state = cstate->base.state;
>   struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
> >base.crtc);
> - struct drm_plane *plane;
> - struct intel_plane *intel_plane;
> - struct intel_plane_state *intel_pstate;
> + struct drm_plane *plane = _plane->base;
> + struct intel_plane_state *intel_pstate = NULL;
>   uint16_t ddb_blocks;
>   enum pipe pipe = intel_crtc->pipe;
>   int ret;
> + int i = skl_wm_plane_id(intel_plane);
> +
> + if (state)
> + intel_pstate =
> + intel_atomic_get_existing_plane_state(state,
> +   intel_
> plane);
>  
>   /*
> -  * We'll only calculate watermarks for planes that are
> actually
> -  * enabled, so make sure all other planes are set as
> disabled.
> +  * Note: If we start supporting multiple pending atomic
> commits against
> +  * the same planes/CRTC's in the future, plane->state will
> no longer be
> +  * the correct pre-state to use for the calculations here
> and we'll
> +  * need to change where we get the 'unchanged' plane data
> from.
> +  *
> +  * For now this is fine because we only allow one queued
> commit against
> +  * a CRTC.  Even if the plane isn't modified by this
> transaction and we
> +  * don't have a plane lock, we still have the CRTC's lock,
> so we know
> +  * that no other transactions are racing with us to update
> it.
>    */
> - memset(result, 0, sizeof(*result));
> -
> - for_each_intel_plane_mask(_priv->drm,
> -   intel_plane,
> -   cstate->base.plane_mask) {
> - int i = skl_wm_plane_id(intel_plane);
> -
> - plane = _plane->base;
> - intel_pstate = NULL;
> - if (state)
> - intel_pstate =
> - intel_atomic_get_existing_plane_stat
> e(state,
> - 
>   intel_plane);
> + if (!intel_pstate)
> + intel_pstate = to_intel_plane_state(plane->state);
>  
> - /*
> -  * Note: If we start supporting multiple pending
> atomic commits
> -  * against the same planes/CRTC's in the future,
> plane->state
> -  * will no longer be the correct pre-state to use
> for the
> -  * calculations here and we'll need to change 

[Intel-gfx] [PATCH 4/6] drm/i915/gen9: Make skl_wm_level per-plane

[Lyude]

Having skl_wm_level contain all of the watermarks for each plane is
annoying since it prevents us from having any sort of object to
represent a single watermark level, something we take advantage of in
the next commit to cut down on all of the copy paste code in here.

Signed-off-by: Lyude 
Cc: Maarten Lankhorst 
Cc: Ville Syrjälä 
Cc: Matt Roper 
---
 drivers/gpu/drm/i915/i915_drv.h  |   6 +-
 drivers/gpu/drm/i915/intel_drv.h |   6 +-
 drivers/gpu/drm/i915/intel_pm.c  | 208 +--
 3 files changed, 100 insertions(+), 120 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d26e5999..0f97d43 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1648,9 +1648,9 @@ struct skl_wm_values {
 };
 
 struct skl_wm_level {
-   bool plane_en[I915_MAX_PLANES];
-   uint16_t plane_res_b[I915_MAX_PLANES];
-   uint8_t plane_res_l[I915_MAX_PLANES];
+   bool plane_en;
+   uint16_t plane_res_b;
+   uint8_t plane_res_l;
 };
 
 /*
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 35ba282..d684f4f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -468,9 +468,13 @@ struct intel_pipe_wm {
bool sprites_scaled;
 };
 
-struct skl_pipe_wm {
+struct skl_plane_wm {
struct skl_wm_level wm[8];
struct skl_wm_level trans_wm;
+};
+
+struct skl_pipe_wm {
+   struct skl_plane_wm planes[I915_MAX_PLANES];
uint32_t linetime;
 };
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index af96888..250f12d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3668,67 +3668,52 @@ static int
 skl_compute_wm_level(const struct drm_i915_private *dev_priv,
 struct skl_ddb_allocation *ddb,
 struct intel_crtc_state *cstate,
+struct intel_plane *intel_plane,
 int level,
 struct skl_wm_level *result)
 {
struct drm_atomic_state *state = cstate->base.state;
struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
-   struct drm_plane *plane;
-   struct intel_plane *intel_plane;
-   struct intel_plane_state *intel_pstate;
+   struct drm_plane *plane = _plane->base;
+   struct intel_plane_state *intel_pstate = NULL;
uint16_t ddb_blocks;
enum pipe pipe = intel_crtc->pipe;
int ret;
+   int i = skl_wm_plane_id(intel_plane);
+
+   if (state)
+   intel_pstate =
+   intel_atomic_get_existing_plane_state(state,
+ intel_plane);
 
/*
-* We'll only calculate watermarks for planes that are actually
-* enabled, so make sure all other planes are set as disabled.
+* Note: If we start supporting multiple pending atomic commits against
+* the same planes/CRTC's in the future, plane->state will no longer be
+* the correct pre-state to use for the calculations here and we'll
+* need to change where we get the 'unchanged' plane data from.
+*
+* For now this is fine because we only allow one queued commit against
+* a CRTC.  Even if the plane isn't modified by this transaction and we
+* don't have a plane lock, we still have the CRTC's lock, so we know
+* that no other transactions are racing with us to update it.
 */
-   memset(result, 0, sizeof(*result));
-
-   for_each_intel_plane_mask(_priv->drm,
- intel_plane,
- cstate->base.plane_mask) {
-   int i = skl_wm_plane_id(intel_plane);
-
-   plane = _plane->base;
-   intel_pstate = NULL;
-   if (state)
-   intel_pstate =
-   intel_atomic_get_existing_plane_state(state,
- 
intel_plane);
+   if (!intel_pstate)
+   intel_pstate = to_intel_plane_state(plane->state);
 
-   /*
-* Note: If we start supporting multiple pending atomic commits
-* against the same planes/CRTC's in the future, plane->state
-* will no longer be the correct pre-state to use for the
-* calculations here and we'll need to change where we get the
-* 'unchanged' plane data from.
-*
-* For now this is fine because we only allow one queued commit
-* against a CRTC.  Even if the plane isn't modified by this
-* transaction and we don't have a plane lock, we still have
-* the