Re: [Intel-gfx] [PATCH v2] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

2023-01-27 Thread Hogander, Jouni 
On Fri, 2023-01-27 at 14:00 +, Souza, Jose wrote:
> On Fri, 2023-01-27 at 10:27 +0200, Jouni Högander wrote:
> > SEL_FETCH_CTL registers are armed immediately when plane is
> > disabled.
> > SEL_FETCH_* instances of plane configuration are used when doing
> > selective update and normal plane register instances for full
> > updates.
> > Currently all SEL_FETCH_* registers are written as a part of noarm
> > plane configuration. If noarm and arm plane configuration are not
> > happening within same vblank we may end up having plane as a part
> > of
> > selective update before it's PLANE_SURF register is written.
> > 
> > Fix this by splitting plane selective fetch configuration into arm
> > and
> > noarm versions and call them accordingly. Write SEL_FETCH_CTL in
> > arm
> > version.
> 
> Does this helps to revert the set of SFF and CFF at the same time?

No, this one is a separate issue.

> 
> > 
> > v2:
> >  - drop color_plane parameter from arm part
> >  - dev_priv -> i915 in arm part
> > 
> > Cc: Ville Syrjälä 
> > Cc: José Roberto de Souza 
> > Cc: Mika Kahola 
> > Cc: Vinod Govindapillai 
> > Cc: Stanislav Lisovskiy 
> > Cc: Luca Coelho 
> > Signed-off-by: Jouni Högander 
> > ---
> >  drivers/gpu/drm/i915/display/intel_cursor.c   |  3 +-
> >  drivers/gpu/drm/i915/display/intel_psr.c  | 28 +--
> > 
> >  drivers/gpu/drm/i915/display/intel_psr.h  |  6 +++-
> >  .../drm/i915/display/skl_universal_plane.c    |  4 ++-
> >  4 files changed, 30 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c
> > b/drivers/gpu/drm/i915/display/intel_cursor.c
> > index d190fa0d393b..ae9f0b6c92db 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> > @@ -532,7 +532,8 @@ static void i9xx_cursor_update_arm(struct
> > intel_plane *plane,
> > skl_write_cursor_wm(plane, crtc_state);
> >  
> > if (plane_state)
> > -   intel_psr2_program_plane_sel_fetch(plane,
> > crtc_state, plane_state, 0);
> > +   intel_psr2_program_plane_sel_fetch_arm(plane,
> > crtc_state,
> > + 
> > plane_state);
> > else
> > intel_psr2_disable_plane_sel_fetch(plane,
> > crtc_state);
> 
> Missing rename intel_psr2_disable_plane_sel_fetch() to
> intel_psr2_disable_plane_sel_fetch_arm().

Yes, this makes sense. I will update the patch. Thank you for the
review.

> 
> With this LGTM.
> Reviewed-by: José Roberto de Souza 
> 
> 
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 7a72e15e6836..a3f4451eb66d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -1559,7 +1559,25 @@ void
> > intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
> > intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe,
> > plane->id), 0);
> >  }
> >  
> > -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> > +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane
> > *plane,
> > +   const struct
> > intel_crtc_state *crtc_state,
> > +   const struct
> > intel_plane_state *plane_state)
> > +{
> > +   struct drm_i915_private *i915 = to_i915(plane->base.dev);
> > +   enum pipe pipe = plane->pipe;
> > +
> > +   if (!crtc_state->enable_psr2_sel_fetch)
> > +   return;
> > +
> > +   if (plane->id == PLANE_CURSOR)
> > +   intel_de_write_fw(i915, PLANE_SEL_FETCH_CTL(pipe,
> > plane->id),
> > + plane_state->ctl);
> > +   else
> > +   intel_de_write_fw(i915, PLANE_SEL_FETCH_CTL(pipe,
> > plane->id),
> > + PLANE_SEL_FETCH_CTL_ENABLE);
> > +}
> > +
> > +void intel_psr2_program_plane_sel_fetch_noarm(struct intel_plane
> > *plane,
> > const struct
> > intel_crtc_state *crtc_state,
> > const struct
> > intel_plane_state *plane_state,
> > int color_plane)
> > @@ -1573,11 +1591,8 @@ void
> > intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> > if (!crtc_state->enable_psr2_sel_fetch)
> > return;
> >  
> > -   if (plane->id == PLANE_CURSOR) {
> > -   intel_de_write_fw(dev_priv,
> > PLANE_SEL_FETCH_CTL(pipe, plane->id),
> > - plane_state->ctl);
> > +   if (plane->id == PLANE_CURSOR)
> > return;
> > -   }
> >  
> > clip = _state->psr2_sel_fetch_area;
> >  
> > @@ -1605,9 +1620,6 @@ void
> > intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> > val = (drm_rect_height(clip) - 1) << 16;
> > val |= (drm_rect_width(_state->uapi.src) >> 16)

Re: [Intel-gfx] [PATCH v2] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

2023-01-27 Thread Souza, Jose 
On Fri, 2023-01-27 at 10:27 +0200, Jouni Högander wrote:
> SEL_FETCH_CTL registers are armed immediately when plane is disabled.
> SEL_FETCH_* instances of plane configuration are used when doing
> selective update and normal plane register instances for full updates.
> Currently all SEL_FETCH_* registers are written as a part of noarm
> plane configuration. If noarm and arm plane configuration are not
> happening within same vblank we may end up having plane as a part of
> selective update before it's PLANE_SURF register is written.
> 
> Fix this by splitting plane selective fetch configuration into arm and
> noarm versions and call them accordingly. Write SEL_FETCH_CTL in arm
> version.

Does this helps to revert the set of SFF and CFF at the same time?

> 
> v2:
>  - drop color_plane parameter from arm part
>  - dev_priv -> i915 in arm part
> 
> Cc: Ville Syrjälä 
> Cc: José Roberto de Souza 
> Cc: Mika Kahola 
> Cc: Vinod Govindapillai 
> Cc: Stanislav Lisovskiy 
> Cc: Luca Coelho 
> Signed-off-by: Jouni Högander 
> ---
>  drivers/gpu/drm/i915/display/intel_cursor.c   |  3 +-
>  drivers/gpu/drm/i915/display/intel_psr.c  | 28 +--
>  drivers/gpu/drm/i915/display/intel_psr.h  |  6 +++-
>  .../drm/i915/display/skl_universal_plane.c|  4 ++-
>  4 files changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c 
> b/drivers/gpu/drm/i915/display/intel_cursor.c
> index d190fa0d393b..ae9f0b6c92db 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -532,7 +532,8 @@ static void i9xx_cursor_update_arm(struct intel_plane 
> *plane,
>   skl_write_cursor_wm(plane, crtc_state);
>  
>   if (plane_state)
> - intel_psr2_program_plane_sel_fetch(plane, crtc_state, 
> plane_state, 0);
> + intel_psr2_program_plane_sel_fetch_arm(plane, crtc_state,
> +plane_state);
>   else
>   intel_psr2_disable_plane_sel_fetch(plane, crtc_state);

Missing rename intel_psr2_disable_plane_sel_fetch() to 
intel_psr2_disable_plane_sel_fetch_arm().

With this LGTM.
Reviewed-by: José Roberto de Souza 


>  
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c 
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index 7a72e15e6836..a3f4451eb66d 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1559,7 +1559,25 @@ void intel_psr2_disable_plane_sel_fetch(struct 
> intel_plane *plane,
>   intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
>  }
>  
> -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
> + const struct intel_crtc_state 
> *crtc_state,
> + const struct intel_plane_state 
> *plane_state)
> +{
> + struct drm_i915_private *i915 = to_i915(plane->base.dev);
> + enum pipe pipe = plane->pipe;
> +
> + if (!crtc_state->enable_psr2_sel_fetch)
> + return;
> +
> + if (plane->id == PLANE_CURSOR)
> + intel_de_write_fw(i915, PLANE_SEL_FETCH_CTL(pipe, plane->id),
> +   plane_state->ctl);
> + else
> + intel_de_write_fw(i915, PLANE_SEL_FETCH_CTL(pipe, plane->id),
> +   PLANE_SEL_FETCH_CTL_ENABLE);
> +}
> +
> +void intel_psr2_program_plane_sel_fetch_noarm(struct intel_plane *plane,
>   const struct intel_crtc_state 
> *crtc_state,
>   const struct intel_plane_state 
> *plane_state,
>   int color_plane)
> @@ -1573,11 +1591,8 @@ void intel_psr2_program_plane_sel_fetch(struct 
> intel_plane *plane,
>   if (!crtc_state->enable_psr2_sel_fetch)
>   return;
>  
> - if (plane->id == PLANE_CURSOR) {
> - intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, 
> plane->id),
> -   plane_state->ctl);
> + if (plane->id == PLANE_CURSOR)
>   return;
> - }
>  
>   clip = _state->psr2_sel_fetch_area;
>  
> @@ -1605,9 +1620,6 @@ void intel_psr2_program_plane_sel_fetch(struct 
> intel_plane *plane,
>   val = (drm_rect_height(clip) - 1) << 16;
>   val |= (drm_rect_width(_state->uapi.src) >> 16) - 1;
>   intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_SIZE(pipe, plane->id), val);
> -
> - intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id),
> -   PLANE_SEL_FETCH_CTL_ENABLE);
>  }
>  
>  void intel_psr2_program_trans_man_trk_ctl(const struct intel_crtc_state 
> *crtc_state)
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.h 
> b/drivers/gpu/drm/i915/display/intel_psr.h
> index 2ac3a465..c87ae2e6ee6c 100644
> ---

Re: [Intel-gfx] [PATCH v2] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

2023-01-27 Thread Coelho, Luciano 
On Fri, 2023-01-27 at 10:27 +0200, Jouni Högander wrote:
> SEL_FETCH_CTL registers are armed immediately when plane is disabled.
> SEL_FETCH_* instances of plane configuration are used when doing
> selective update and normal plane register instances for full updates.
> Currently all SEL_FETCH_* registers are written as a part of noarm
> plane configuration. If noarm and arm plane configuration are not
> happening within same vblank we may end up having plane as a part of
> selective update before it's PLANE_SURF register is written.
> 
> Fix this by splitting plane selective fetch configuration into arm and
> noarm versions and call them accordingly. Write SEL_FETCH_CTL in arm
> version.
> 
> v2:
>  - drop color_plane parameter from arm part
>  - dev_priv -> i915 in arm part
> 
> Cc: Ville Syrjälä 
> Cc: José Roberto de Souza 
> Cc: Mika Kahola 
> Cc: Vinod Govindapillai 
> Cc: Stanislav Lisovskiy 
> Cc: Luca Coelho 
> Signed-off-by: Jouni Högander 
> ---

Looks good to me:

Reviewed-by: Luca Coelho 

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v2] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

2023-01-27 Thread Govindapillai, Vinod 
Hi Jouni,

On Fri, 2023-01-27 at 10:27 +0200, Jouni Högander wrote:
> SEL_FETCH_CTL registers are armed immediately when plane is disabled.
> SEL_FETCH_* instances of plane configuration are used when doing
> selective update and normal plane register instances for full updates.
> Currently all SEL_FETCH_* registers are written as a part of noarm
> plane configuration. If noarm and arm plane configuration are not
> happening within same vblank we may end up having plane as a part of
> selective update before it's PLANE_SURF register is written.
> 
> Fix this by splitting plane selective fetch configuration into arm and
> noarm versions and call them accordingly. Write SEL_FETCH_CTL in arm
> version.
> 
> v2:
>  - drop color_plane parameter from arm part
>  - dev_priv -> i915 in arm part
> 
> Cc: Ville Syrjälä 
> Cc: José Roberto de Souza 
> Cc: Mika Kahola 
> Cc: Vinod Govindapillai 
> Cc: Stanislav Lisovskiy 
> Cc: Luca Coelho 
> Signed-off-by: Jouni Högander 
> ---
>  drivers/gpu/drm/i915/display/intel_cursor.c   |  3 +-
>  drivers/gpu/drm/i915/display/intel_psr.c  | 28 +--
>  drivers/gpu/drm/i915/display/intel_psr.h  |  6 +++-
>  .../drm/i915/display/skl_universal_plane.c    |  4 ++-
>  4 files changed, 30 insertions(+), 11 deletions(-)

Reviewed-by: Vinod Govindapillai 

BR
Vinod
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c
> b/drivers/gpu/drm/i915/display/intel_cursor.c
> index d190fa0d393b..ae9f0b6c92db 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -532,7 +532,8 @@ static void i9xx_cursor_update_arm(struct intel_plane 
> *plane,
> skl_write_cursor_wm(plane, crtc_state);
>  
> if (plane_state)
> -   intel_psr2_program_plane_sel_fetch(plane, crtc_state, 
> plane_state, 0);
> +   intel_psr2_program_plane_sel_fetch_arm(plane, crtc_state,
> +  plane_state);
> else
> intel_psr2_disable_plane_sel_fetch(plane, crtc_state);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c 
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index 7a72e15e6836..a3f4451eb66d 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1559,7 +1559,25 @@ void intel_psr2_disable_plane_sel_fetch(struct 
> intel_plane *plane,
> intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
>  }
>  
> -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
> +   const struct intel_crtc_state 
> *crtc_state,
> +   const struct intel_plane_state 
> *plane_state)
> +{
> +   struct drm_i915_private *i915 = to_i915(plane->base.dev);
> +   enum pipe pipe = plane->pipe;
> +
> +   if (!crtc_state->enable_psr2_sel_fetch)
> +   return;
> +
> +   if (plane->id == PLANE_CURSOR)
> +   intel_de_write_fw(i915, PLANE_SEL_FETCH_CTL(pipe, plane->id),
> + plane_state->ctl);
> +   else
> +   intel_de_write_fw(i915, PLANE_SEL_FETCH_CTL(pipe, plane->id),
> + PLANE_SEL_FETCH_CTL_ENABLE);
> +}
> +
> +void intel_psr2_program_plane_sel_fetch_noarm(struct intel_plane *plane,
> const struct intel_crtc_state 
> *crtc_state,
> const struct intel_plane_state 
> *plane_state,
> int color_plane)
> @@ -1573,11 +1591,8 @@ void intel_psr2_program_plane_sel_fetch(struct 
> intel_plane *plane,
> if (!crtc_state->enable_psr2_sel_fetch)
> return;
>  
> -   if (plane->id == PLANE_CURSOR) {
> -   intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, 
> plane->id),
> - plane_state->ctl);
> +   if (plane->id == PLANE_CURSOR)
> return;
> -   }
>  
> clip = _state->psr2_sel_fetch_area;
>  
> @@ -1605,9 +1620,6 @@ void intel_psr2_program_plane_sel_fetch(struct 
> intel_plane *plane,
> val = (drm_rect_height(clip) - 1) << 16;
> val |= (drm_rect_width(_state->uapi.src) >> 16) - 1;
> intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_SIZE(pipe, plane->id), 
> val);
> -
> -   intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id),
> - PLANE_SEL_FETCH_CTL_ENABLE);
>  }
>  
>  void intel_psr2_program_trans_man_trk_ctl(const struct intel_crtc_state 
> *crtc_state)
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.h 
> b/drivers/gpu/drm/i915/display/intel_psr.h
> index 2ac3a465..c87ae2e6ee6c 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.h
> +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> @@ -46,10 +46,14 @@ bool

[Intel-gfx] [PATCH v2] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

2023-01-27 Thread Jouni Högander 
SEL_FETCH_CTL registers are armed immediately when plane is disabled.
SEL_FETCH_* instances of plane configuration are used when doing
selective update and normal plane register instances for full updates.
Currently all SEL_FETCH_* registers are written as a part of noarm
plane configuration. If noarm and arm plane configuration are not
happening within same vblank we may end up having plane as a part of
selective update before it's PLANE_SURF register is written.

Fix this by splitting plane selective fetch configuration into arm and
noarm versions and call them accordingly. Write SEL_FETCH_CTL in arm
version.

v2:
 - drop color_plane parameter from arm part
 - dev_priv -> i915 in arm part

Cc: Ville Syrjälä 
Cc: José Roberto de Souza 
Cc: Mika Kahola 
Cc: Vinod Govindapillai 
Cc: Stanislav Lisovskiy 
Cc: Luca Coelho 
Signed-off-by: Jouni Högander 
---
 drivers/gpu/drm/i915/display/intel_cursor.c   |  3 +-
 drivers/gpu/drm/i915/display/intel_psr.c  | 28 +--
 drivers/gpu/drm/i915/display/intel_psr.h  |  6 +++-
 .../drm/i915/display/skl_universal_plane.c|  4 ++-
 4 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c 
b/drivers/gpu/drm/i915/display/intel_cursor.c
index d190fa0d393b..ae9f0b6c92db 100644
--- a/drivers/gpu/drm/i915/display/intel_cursor.c
+++ b/drivers/gpu/drm/i915/display/intel_cursor.c
@@ -532,7 +532,8 @@ static void i9xx_cursor_update_arm(struct intel_plane 
*plane,
skl_write_cursor_wm(plane, crtc_state);
 
if (plane_state)
-   intel_psr2_program_plane_sel_fetch(plane, crtc_state, 
plane_state, 0);
+   intel_psr2_program_plane_sel_fetch_arm(plane, crtc_state,
+  plane_state);
else
intel_psr2_disable_plane_sel_fetch(plane, crtc_state);
 
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c 
b/drivers/gpu/drm/i915/display/intel_psr.c
index 7a72e15e6836..a3f4451eb66d 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -1559,7 +1559,25 @@ void intel_psr2_disable_plane_sel_fetch(struct 
intel_plane *plane,
intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
 }
 
-void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
+void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
+   const struct intel_crtc_state 
*crtc_state,
+   const struct intel_plane_state 
*plane_state)
+{
+   struct drm_i915_private *i915 = to_i915(plane->base.dev);
+   enum pipe pipe = plane->pipe;
+
+   if (!crtc_state->enable_psr2_sel_fetch)
+   return;
+
+   if (plane->id == PLANE_CURSOR)
+   intel_de_write_fw(i915, PLANE_SEL_FETCH_CTL(pipe, plane->id),
+ plane_state->ctl);
+   else
+   intel_de_write_fw(i915, PLANE_SEL_FETCH_CTL(pipe, plane->id),
+ PLANE_SEL_FETCH_CTL_ENABLE);
+}
+
+void intel_psr2_program_plane_sel_fetch_noarm(struct intel_plane *plane,
const struct intel_crtc_state 
*crtc_state,
const struct intel_plane_state 
*plane_state,
int color_plane)
@@ -1573,11 +1591,8 @@ void intel_psr2_program_plane_sel_fetch(struct 
intel_plane *plane,
if (!crtc_state->enable_psr2_sel_fetch)
return;
 
-   if (plane->id == PLANE_CURSOR) {
-   intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, 
plane->id),
- plane_state->ctl);
+   if (plane->id == PLANE_CURSOR)
return;
-   }
 
clip = _state->psr2_sel_fetch_area;
 
@@ -1605,9 +1620,6 @@ void intel_psr2_program_plane_sel_fetch(struct 
intel_plane *plane,
val = (drm_rect_height(clip) - 1) << 16;
val |= (drm_rect_width(_state->uapi.src) >> 16) - 1;
intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_SIZE(pipe, plane->id), val);
-
-   intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id),
- PLANE_SEL_FETCH_CTL_ENABLE);
 }
 
 void intel_psr2_program_trans_man_trk_ctl(const struct intel_crtc_state 
*crtc_state)
diff --git a/drivers/gpu/drm/i915/display/intel_psr.h 
b/drivers/gpu/drm/i915/display/intel_psr.h
index 2ac3a465..c87ae2e6ee6c 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.h
+++ b/drivers/gpu/drm/i915/display/intel_psr.h
@@ -46,10 +46,14 @@ bool intel_psr_enabled(struct intel_dp *intel_dp);
 int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
struct intel_crtc *crtc);
 void intel_psr2_program_trans_man_trk_ctl(const struct intel_crtc_state 
*crtc_state);
-void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
+void